Skip to content

Allow direct local client connections to select a binary version - #552

Open
sdairs wants to merge 2 commits into
stack/466-validate-client-selectorsfrom
stack/469-select-client-version
Open

Allow direct local client connections to select a binary version#552
sdairs wants to merge 2 commits into
stack/466-validate-client-selectorsfrom
stack/469-select-client-version

Conversation

@sdairs

@sdairs sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add direct-mode local client --version selection for installed ClickHouse binaries without modifying the global default
  • intentionally let partial numeric selectors choose the newest installed match, while exact selectors require that installed version
  • define actionable zero, one, multiple, stale-default, missing-explicit, and named-mode behavior
  • document connection selection versus local binary selection and reject named-mode version overrides during clap parsing
  • cover parser behavior and the complete isolated fake-binary subprocess matrix

Tests

  • cargo test -p clickhousectl
  • cargo clippy -p clickhousectl --all-targets -- -D warnings
  • cargo fmt --all --check

Closes #469

Comment thread crates/clickhousectl/src/local/mod.rs
@sdairs

sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Carry-over review from PR #500 (old PR for #469). Functionally this PR is stronger than #500 (parse-time typed validation with genuinely good postgres/latest rejection messages, deliberate+documented zero/one/multiple fallback policy, broader subprocess matrix, default-file non-mutation asserted). Two required items:

  1. Trim the --version help noise — sdairs' only review comment on Allow direct local client connections to select a binary version #500 applies here, more strongly. On Allow direct local client connections to select a binary version #500, sdairs wrote: "good but adds too much noise to agent context help. it is unlikely that thorough explanation of a --version flag is needed beyond the actual flag description", and then addressed it: "removed the redundant --version explanation from agent-context help, restored the existing concise connection/default wording, and retained the flag and its description." This PR re-adds exactly that explanation in crates/clickhousectl/src/local/cli.rs:~246-254 (after_help): the connection-vs-binary sentence and a full resolution-semantics paragraph ("Direct mode without --version uses a valid default… sole installed version… stale default must be repaired…"), and it locks the noise in with a test (clickhouse_client_help_distinguishes_connection_and_binary_selection, cli.rs:~971-984) asserting six substrings — so trimming later breaks the suite. Issue Allow direct local client connections to select a binary version #469's criterion 5 does require help to distinguish connection selection from binary selection, so some text is warranted — but Allow direct local client connections to select a binary version #500's accepted end state satisfied that with the flag description alone ("Installed local client version for direct host/port mode (e.g. 25, 25.12, or 25.12.9.61). Does not change the default.") plus the README. Cut the resolution-semantics lines from after_help, keep the detail in README and the flag description, and adjust the pinned test accordingly.

  2. Fix the IO-error masking (open low-severity Cursor finding here). try_resolve_local (crates/clickhousectl/src/version_manager/resolve.rs:~35) swallows list_installed_versions() IO errors via .ok()?, so an explicit --version against an unreadable ~/.clickhouse/versions reports ClientVersionNotInstalled, while the no-default branch propagates the IO error. Inconsistent and misleading. Propagate the error in the explicit-version path too (Allow direct local client connections to select a binary version #500's explicit path used list_installed_versions()? and did not have this).

Cosmetic: partial-version "newest installed match" is a mild expansion beyond the issue's "explicit exact version" framing — it is documented and user-directed, so acceptable, but confirm intentional.

@sdairs
sdairs force-pushed the stack/469-select-client-version branch 4 times, most recently from 569ad10 to a301f21 Compare August 26, 2026 19:07
@sdairs
sdairs force-pushed the stack/469-select-client-version branch 2 times, most recently from 4396b01 to 8506510 Compare August 26, 2026 19:12
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from 8506510 to 49ffd6c Compare August 26, 2026 19:20
@sdairs
sdairs force-pushed the stack/469-select-client-version branch 2 times, most recently from 0b56079 to c335826 Compare August 26, 2026 19:28
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from c335826 to 63d1194 Compare August 26, 2026 19:31
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from 63d1194 to 537c6f3 Compare August 26, 2026 19:34
@sdairs
sdairs force-pushed the stack/469-select-client-version branch 2 times, most recently from 1695102 to f2f1c5a Compare August 26, 2026 20:10
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from f2f1c5a to fbfbf93 Compare August 26, 2026 20:30
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from fbfbf93 to 3f87608 Compare August 26, 2026 20:35
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from 3f87608 to 100f161 Compare August 26, 2026 20:38
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from 100f161 to 407781c Compare August 26, 2026 20:41
@sdairs
sdairs force-pushed the stack/469-select-client-version branch 2 times, most recently from 209656c to ed7ca6b Compare August 26, 2026 20:49
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from ed7ca6b to 88a5e8f Compare August 26, 2026 20:55

@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 1 potential issue.

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 88a5e8f. Configure here.

Comment thread crates/clickhousectl/src/local/cli.rs
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from 88a5e8f to a5a089b Compare August 27, 2026 06:34
@sdairs
sdairs force-pushed the stack/469-select-client-version branch 2 times, most recently from abffd3e to 6a08fb4 Compare August 27, 2026 06:49
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from e0d31c1 to 6b9dd61 Compare August 27, 2026 09:19
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from 6b9dd61 to 7838635 Compare August 27, 2026 10:13
@sdairs
sdairs force-pushed the stack/469-select-client-version branch from 7838635 to 0367412 Compare August 27, 2026 10:51
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.

Allow direct local client connections to select a binary version

1 participant