Skip to content

Give local JSON errors the same detail and hints as human output - #633

Open
sdairs wants to merge 2 commits into
fix/600-local-remove-global-guardfrom
fix/608-local-json-error-parity
Open

Give local JSON errors the same detail and hints as human output#633
sdairs wants to merge 2 commits into
fix/600-local-remove-global-guardfrom
fix/608-local-json-error-parity

Conversation

@sdairs

@sdairs sdairs commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Why

local --json errors were consistently thinner than the human Error: ... line they replace, which is backwards: JSON is the surface coding agents read. Issue #608 reported three cases; this fixes them and the rest of the mapping they were symptoms of.

What

  • Wrong hint. local server remove <running> suggested clickhousectl local server list; it now suggests clickhousectl local server stop <name> — the step that actually recovers — and carries the human message's stop it first with ... clause.
  • Lost detail. Errors whose text clickhousectl composes itself are now rendered verbatim, so local server start --config does-not-exist reports config 'does-not-exist' not found in <dir> (available: analytics.xml) in JSON instead of {"code":"local_error","message":"Local command failed"}.
  • Conflated code. local remove <not-installed-version> now reports a new version_not_installed code with a clickhousectl local list hint, instead of version_unavailable / clickhousectl local list --remote, which described an unresolvable download.

The mapping became an exhaustive match over Error. Every variant picks one of two documented rules: parity (render the error's own Display) or redaction (a curated summary). A new Error variant now has to be classified rather than silently collapsing into the fallback. Only text interpolating output we do not control stays summarized — subprocess stderr and log tails (startup_exit, startup_timeout), Docker daemon strings (docker_error), download bodies (download_failed), OS/serde sources (io_error, local_error).

New codes replace the fallback where it was hiding real classifications: version_not_installed, version_selection_required, version_already_installed, unsupported_client_version, unsupported_platform, config_not_found, invalid_config_name, invalid_server_name, unsupported_argument, docker_unavailable, docker_error, network_error. Docker unavailability is built by local::docker from a classified failure kind and platform guidance and never renders the daemon's own text, so it now reaches JSON in full.

Two small adjacent fixes the audit turned up:

  • The --config passthrough guard used Error::Exec, whose payload is subprocess text and therefore redacted, so a pure usage mistake produced an opaque local_error. It now uses a dedicated Error::UnsupportedArgument.
  • The bare server remove selection error gained "no server was removed" in human mode, which JSON already stated.

error.command is now Option<String> so a hint can name the concrete server or version.

Tests

cargo fmt --all, cargo clippy --workspace --all-targets -- -D warnings, cargo test -p clickhousectl (all green), plus cargo check --workspace --all-features.

  • Inline exact-payload tests for the three reported cases.
  • self_composed_errors_serialize_their_human_message_verbatim asserts message == error.to_string() across every parity-classified error, so the two surfaces cannot drift again.
  • errors_carrying_foreign_output_stay_summarized asserts the complement, including that no injected secret or path is serialized.
  • Subprocess coverage in local_structured_errors_test.rs for missing config, escaping config name, passthrough argument, the not-installed version, and the Docker diagnostic (still asserting the socket path and metadata contents never leak).
  • Existing exact-JSON expectations updated in the postgres readiness/validation, server selection and server state-machine suites.

README documents the two message rules and the extended error-code table.

Stacking

Part of a stacked chain: based on fix/600-local-remove-global-guard, not main. Keeps JSON parity with the local remove guards added there (version_is_default, server_running for VersionInUse).

Fixes #608

🤖 Generated with Claude Code

@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 9786ed4. Configure here.

Comment thread crates/clickhousectl/src/local/output.rs
Comment thread crates/clickhousectl/src/local/output.rs
sdairs and others added 2 commits August 28, 2026 20:41
`local --json` errors were thinner than the human `Error: ...` line they
replace, which is backwards: JSON is the surface agents read.

- `server remove <running>` now points at `local server stop <name>`, the
  step that actually recovers, instead of `local server list`.
- Messages that clickhousectl composes itself are rendered verbatim, so a
  missing `--config` name lists the configs dir and the available files in
  JSON too, rather than collapsing to `local_error`/"Local command failed".
- `local remove <not-installed-version>` reports the new
  `version_not_installed` code with `local list`, no longer conflating a
  local miss with an unresolvable download (`version_unavailable` and
  `local list --remote`).

The mapping is now an exhaustive match over `Error`: every variant picks
either parity (its own `Display` text) or redaction (a curated summary),
so a new variant must be classified rather than silently collapsing. Only
text that interpolates foreign output stays summarized -- subprocess
stderr, Docker daemon strings, download bodies, OS/serde sources -- and
new codes replace the fallback for config, server-name, version-state,
platform, Docker and network failures. Docker unavailability, whose
diagnostic is built by `local::docker` from a classified failure kind and
never renders the daemon's text, now reaches JSON in full.

Also splits the `--config` passthrough guard out of `Error::Exec` into
`Error::UnsupportedArgument` so its self-composed guidance is rendered,
and adds "no server was removed" to the bare `server remove` selection
error in human mode, where JSON already said it.

Tests: exact-payload unit tests for the three reported cases, a parity
test asserting `message == error.to_string()` for every self-composed
error, a redaction test asserting foreign output stays out, and
subprocess coverage for the missing-config, escaping-config and
passthrough-argument errors. README documents the two message rules and
the extended code table.

Fixes #608

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rors

The JSON error contract states that error.message carries the human text
whenever clickhousectl composes it itself, with redaction reserved for
foreign output. Two variants violated that: the container name-conflict
guidance was collapsed into the redacted docker_error summary, and every
Error::Postgres message — all but one of which are self-composed
validation or state guidance ending in a recovery hint — fell into the
local_error fallback.

Following the structural-contract rule (a failure a caller must tell
apart gets its own variant, never message sniffing), split them:

- Error::ContainerNameConflict renders parity as container_name_conflict,
  leaving Error::DockerError as the redacted daemon-text bucket.
- Error::PostgresUsage renders parity as postgres_error; Error::Postgres
  keeps only the foreign-text psql exec failure and stays redacted.

Human output is byte-identical for every migrated site.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sdairs
sdairs force-pushed the fix/608-local-json-error-parity branch from c06b9ab to 2157d67 Compare August 28, 2026 19:42
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.

Local error messages degrade in JSON mode (hint text, bare local_error, version_unavailable)

1 participant