Skip to content

fix(cli): honor --json contract on all error paths - #26

Merged
andrei-hasna merged 1 commit into
mainfrom
fix/audit-knowledge-json-error-contract
Jul 24, 2026
Merged

fix(cli): honor --json contract on all error paths#26
andrei-hasna merged 1 commit into
mainfrom
fix/audit-knowledge-json-error-contract

Conversation

@andrei-hasna

Copy link
Copy Markdown
Contributor

Bug

--json contract violated on all error paths. With --json, error paths emitted empty stdout and a plaintext [ERROR] CLI error {...} line plus Error: .. on stderr (exit 1). Consumers parsing <cmd> --json could neither detect nor read failures.

Repro (before): knowledge db stats --json, knowledge project-panel --json, knowledge providers check --json, knowledge add --json (no args) — all print nothing to stdout.

Root cause

The top-level import.meta.main catch handler in src/cli.ts unconditionally wrote the failure to stderr (log('error', 'CLI error', ...) + console.error("Error: ..")) and never consulted the --json flag, so stdout stayed empty on every thrown error regardless of --json.

Fix

Introduce emitCliError(error, argv):

  • When --json is present, emit a machine-parseable { ok: false, error, message } object on stdout (mirroring the existing { ok: true, ... } success contract via the shared output() helper).
  • Otherwise, keep the existing plaintext stderr diagnostic.
  • Exit code stays 1 in both modes.

The exported helper is reused by the test bin wrapper so it mirrors the real entrypoint (no duplicated, drift-prone catch logic). No version bump, no publish; prepublishOnly rebuilds the bin/ artifacts at publish time.

Test

Adds regression tests in tests/cli.test.ts:

  • --json error paths emit a machine-parseable object on stdout — covers add, providers check, unknown command, and get (missing --id); asserts non-empty stdout that parses to { ok: false, error: <string> }.
  • non-json error paths keep plaintext stderr and empty stdout.

Both fail before the fix and pass after.

Verification

bunx tsc -p tsconfig.build.json --noEmit clean. bun test tests/cli.test.ts: branch 44 pass / 6 fail vs origin/main baseline 42 pass / 6 fail — the 2 new tests pass and the same 6 pre-existing, environment-dependent failures (real-HOME auth/legacy-migration state, slow-box timeouts) occur on both, so no new failures introduced.

🤖 Generated with Claude Code

@andrei-hasna

Copy link
Copy Markdown
Contributor Author

Adversarial review verdict: REQUEST_CHANGES (do not merge)

(Posted as a comment — GH blocks a formal request-changes review from the PR author account.)

The core fix is correct in isolation — verified before/after that add --json, get --json, project-panel --json, providers check <x> --json, and unknown-command --json all now emit a parseable {"ok":false,"error":...} on stdout with exit 1, while non-json paths keep plaintext on stderr and empty stdout. The two new tests are genuine (spawn the real binary and JSON.parse stdout), not tautological. No secrets in the diff. prepublishOnly rebuilds bin/knowledge.js, so the fix will ship on publish.

Two blocking issues:

1. Regression: reintroduces the internal-stack leak that #23 already fixed

main moved on since this branch was cut. Commit 36ae510 (#23, "don't leak internal Error stack on usage/validation errors") changed the top-level catch to log('debug', 'CLI error', { message, stack }) so the internal stack (bundled bin path + minified frames) is NOT printed on stderr at the default log level.

This PR's emitCliError uses log('error', 'CLI error', { message, stack }) unconditionally. At default LOG_LEVEL=info, error prints, so the [ERROR] CLI error {...,"stack":...} line — full stack included — is emitted on stderr again. Reproduced:

  • this branch: knowledge add -> stderr has "stack":"Error: ...\n at run (.../cli.ts:1490)"
  • main: same command -> no stack.

Fix: log the stack at debug (as #23 does), not error.

2. Branch conflicts with current main

origin/main advanced by #23 and #24, both touching src/cli.ts and tests/cli.test.ts. PR is now CONFLICTING/DIRTY and cannot be squash-merged. Rebase onto latest main, resolve the error-handler conflict preserving #23's debug-level stack suppression, and re-run the added tests.

(Suite note: full-suite bun test failures on this station are pre-existing/environmental — inventory retrieves legacy items fails identically on the base commit in isolation; sync tests pass in isolation and only time out under full-suite concurrency; base main fails a different non-overlapping set. Not caused by this change.)

Fatal CLI errors emitted nothing machine-parseable on stdout even when the
caller passed --json, so consumers parsing `<cmd> --json` could not detect or
read failures (they only got a plaintext line on stderr).

Route fatal errors through a new emitCliError() helper. The human-readable
diagnostic is always written to stderr (`Error: <msg>`), preserving existing
stderr-reading behavior; when --json is present it additionally emits a
machine-parseable { ok: false, error, message } object on stdout (matching the
{ ok: true, ... } success contract). The internal stack stays behind debug
logging. The shared helper is also used by the test bin wrapper so it mirrors
the real entrypoint.

Adds regression tests covering error paths under --json (machine-parseable
stdout) and the non-json path (empty stdout, plaintext stderr).
@andrei-hasna
andrei-hasna force-pushed the fix/audit-knowledge-json-error-contract branch from 782ec9f to a5ca0d1 Compare July 24, 2026 15:11
@andrei-hasna
andrei-hasna merged commit 6441578 into main Jul 24, 2026
3 of 7 checks passed
@andrei-hasna
andrei-hasna deleted the fix/audit-knowledge-json-error-contract branch July 24, 2026 15:14
andrei-hasna added a commit that referenced this pull request Jul 24, 2026
Bring in #22/#26/#27/#29/#3 (hosted-url scrub, --json error contract,
reject unknown top-level commands, FTS query parser, public-package
docs hardening). Renumber this change to 0.2.90 (main consumed 0.2.88
and 0.2.89); rebuild bin/ and dist/ from merged sources.
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.

1 participant