Skip to content

Validate local Postgres start options - #547

Open
sdairs wants to merge 2 commits into
stack/461-install-workflow-pathsfrom
stack/460-validate-local-postgres
Open

Validate local Postgres start options#547
sdairs wants to merge 2 commits into
stack/461-install-workflow-pathsfrom
stack/460-validate-local-postgres

Conversation

@sdairs

@sdairs sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • validate local Postgres names, complete supported image tags, nonzero ports, and environment assignments before command execution
  • reject occupied explicit ports locally and make generated environment variable precedence deterministic while preserving -e POSTGRES_PASSWORD=... compatibility
  • defer omitted-port auto-selection until a fresh create is confirmed, so exhausted default ports cannot block resuming a stored instance
  • preserve the stored-settings note when resume ignores password overrides supplied through --env
  • add clap, bound-port, malformed input, duplicate/reserved variable, zero-request fake Docker socket, and resume regression coverage
  • update local Postgres edge-case expectations for clap-time port and version diagnostics

Validation layering

Clap parsers own individual value validation, and the post-parse hook owns cross-argument conflicts so invalid CLI input exits with usage code 2. The handler preflight intentionally revalidates at the side-effect boundary as defense in depth for non-CLI callers; explicit ports are checked there, while omitted ports remain unresolved until the fresh-create path.

Tests

  • cargo test -p clickhousectl --test local_postgres_start_validation_test
  • cargo test -p clickhousectl
  • cargo clippy -p clickhousectl --all-targets -- -D warnings
  • cargo fmt --all --check
  • bash -n scripts/test-postgres-integration.sh
  • direct CLI verification of the updated port-zero and unsupported-version diagnostics

The full Docker-backed scripts/test-postgres-integration.sh was not run locally because the configured OrbStack Docker socket was unavailable.

Closes #460

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ffaf557. Configure here.

Comment thread crates/clickhousectl/src/local/postgres.rs Outdated
Comment thread crates/clickhousectl/src/local/postgres.rs
@sdairs

sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Carry-over review from PR #486 (old PR for #460). The clap-time validation in this PR is better UX than #486's handler preflight (exit 2, env-key grammar, excellent reserved-key messages like "use --user instead of --env"), and the zero-request fake-socket criterion is met. But this PR has real regressions of work done during #486's review. Required:

  1. BLOCKER — omitted-port resolution regresses the resume fix sdairs landed on Validate local Postgres start options before Docker work #486. On Validate local Postgres start options before Docker work #486, sdairs adjudicated the port-preflight concern: "Partially valid. The exhausted omitted-port case could incorrectly block resume and is fixed in b538018: an omitted --port now remains unresolved through preflight and auto-selection runs only after the existing-instance branch determines this is a fresh create. I intentionally kept explicit-port validation in preflight per Validate local Postgres start options before Docker work #460." Here, validate_start_options calls resolve_port(port)? unconditionally (crates/clickhousectl/src/local/postgres.rs, StartPreflight.host_port: u16, not Option) before recover_current_project_servers() and the resume branch. An exhausted 5432–5532 auto-select range now aborts a resume that would reuse the stored binding — exactly the bug fixed in b538018. Fix: keep an omitted port as Option through preflight and auto-select only after the resume branch determines this is a fresh create (explicit ports may still validate in preflight, per the issue). Cursor Bugbot flagged this here; unanswered.

  2. Restore the resume regression test. Validate local Postgres start options before Docker work #486 had exhausted_auto_port_range_does_not_block_resume (exhausts 5432–5532, asserts resume still succeeds with the stored binding). This PR dropped it entirely — no resume-path coverage exists. Re-add it.

  3. Resume password-note regression (Cursor Bugbot support multiple versions of ch within a project #2 here, valid). Preflight strips -e POSTGRES_PASSWORD= out of extra_env before the resume-override check, so password-via---env alone no longer triggers the "resuming with stored settings" note. Validate local Postgres start options before Docker work #486 handled this with a pre-preflight has_extra_env flag. Restore it.

  4. Fix the red local postgres edge cases CI job. scripts/test-postgres-integration.sh expects --port 0 is not allowed (old runtime check) and the old is not supported version wording; this PR moved port-zero rejection into clap (0 is not in 1..=65535) and changed version-error wording without updating the script. Update the script expectations. Note: this also currently breaks the same CI job on every PR stacked above this one.

  5. Error text quality. Port-zero now shows clap's generic 0 is not in 1..=65535 instead of Validate local Postgres start options before Docker work #486's actionable "--port 0 is not allowed; pick a specific port or omit the flag". Consider a value_parser wrapper that emits the actionable message.

  6. Mild over-engineering note: the main.rs post-parse hook + LocalArgs::postgres_start_validation_error() plus full re-validation in validate_start_options duplicates what the clap parsers already enforce. Validate local Postgres start options before Docker work #486's single preflight function was leaner. Keep if the layering is intentional defense-in-depth, but confirm.

@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch 2 times, most recently from 227421d to d3774c9 Compare August 26, 2026 19:28
@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch 2 times, most recently from aec8111 to c249950 Compare August 26, 2026 19:34
@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch 2 times, most recently from 5108389 to 4ddfbde Compare August 26, 2026 20:10
@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch from 4ddfbde to 661ddcd Compare August 26, 2026 20:30
@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch from 661ddcd to 2a9150d Compare August 26, 2026 20:35
@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch from 2a9150d to 2c9fe63 Compare August 26, 2026 20:38
@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch from 2c9fe63 to 8b03560 Compare August 26, 2026 20:41
@sdairs
sdairs force-pushed the stack/460-validate-local-postgres branch from 8b03560 to e0790e0 Compare August 26, 2026 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate local Postgres start options before Docker work

1 participant