Skip to content

fix(cli): reject values on top-level flags - #88

Open
h1-hunt wants to merge 1 commit into
mainfrom
fix/issue-85-top-level-flag-arity
Open

fix(cli): reject values on top-level flags#88
h1-hunt wants to merge 1 commit into
mainfrom
fix/issue-85-top-level-flag-arity

Conversation

@h1-hunt

@h1-hunt h1-hunt commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • reject values captured by top-level --help and --version flags
  • return the standard non-zero JSON error instead of silently printing help
  • cover helper-level arity checks and real process behavior for value capture and masked invalid flags

Validation

  • npm run test -w @h402/cli -- tests/cli-help.test.ts — 25/25 passed
  • npm test — CLI 215/215 and core 14/14 passed
  • npm run typecheck
  • npm run lint
  • npm run build
  • npm run verify:pack
  • npm run smoke:pack
  • git diff --cached --check

Closes #85

@h1-hunt

h1-hunt commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

GPT Reviewer

Verdict: REJECT

Reviewed head 1565fbbac5d3c40b61e66682e342f51356b00247 against review snapshot base e426cef77d9fa2b1a5e846c15def15cbd4bbba25 using a bounded, tool-free review bundle.

The central arity fix remains bypassable for string "true" values, and the new process tests are not isolated from the user's HOME.

Findings

  1. MAJOR — Top-level flags still accept the value "true" (packages/cli/src/help.ts:245)
    • Evidence: The check rejects strings only when provided !== "true". Therefore parsed valued forms such as --version=true, --version true, --help=true, and --help true pass validation. The subsequent flagBoolean/help dispatch treats them as enabled and exits 0 with version/help output. A malformed --help=true can also be masked by a co-present bare --version. This directly violates the arity-none and parsed-string requirements.
    • Suggested fix: Reject every string value for top-level help and version; only boolean true should be accepted. Add real-process tests for inline and following true values, including a malformed valued flag co-present with the other bare flag.
  2. MINOR — Real-process tests inherit the developer's HOME (packages/cli/tests/cli-help.test.ts:149)
    • Evidence: Both new spawnSync calls provide only encoding, so child processes inherit the actual HOME and any h402 configuration. The mandatory process-test invariant requires a temporary HOME to keep validation tests hermetic and protect user state.
    • Suggested fix: Create a temporary directory for each process-test scope, pass it as HOME in the child environment, and remove it afterward.

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (1.846s)
  • typecheck: PASS (2.883s)
  • lint: PASS (2.278s)
  • test: PASS (17.641s)
  • build: PASS (2.241s)
  • No paid h402/provider calls were made.

@h1-hunt h1-hunt added the rejected-gpt GPT Reviewer found issues on this PR head label Jul 30, 2026
@h1-hunt

h1-hunt commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Claude Reviewer

Verdict: PASS

Reviewed head 1565fbbac5d3c40b61e66682e342f51356b00247 against review snapshot base e426cef77d9fa2b1a5e846c15def15cbd4bbba25 using a bounded, tool-free review bundle.

The PR correctly implements the issue #85 boundary: top-level --help/--version values captured by parseArgs (following or inline) are rejected with the standard non-zero JSON stderr envelope before any dispatch or side effect, while bare --help/--version, bare h402, and the positional help/version commands retain their stdout/exit-0 behavior. Moving assertTopLevelFlags ahead of the --version early return for the no-command case also fixes flag masking (--version --bogus, --version --help=wallet now fail), and the help-command branch is tightened consistently. Shapes leaving residual positionals (--version wallet create, --version=x search q) still fail loudly via unknown-command/assertKnownFlags with no help-exit-0 escape. The "true" string exemption matches the repo-wide flagBoolean/assertKnownFlags arity-none convention, and the new error message mirrors the existing per-command wording. Tests cover both the helper arity checks and real process behavior via tsx-spawned entrypoint runs asserting exit 1, empty stdout, and the parsed error envelope; shared validation confirms all suites, typecheck, lint, and build pass with the new tests executing. Pre-existing leniencies on the positional version/help commands (e.g. h402 version --bogus) are unchanged and outside the stated boundary. No actionable findings.

Findings

None.

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (1.846s)
  • typecheck: PASS (2.883s)
  • lint: PASS (2.278s)
  • test: PASS (17.641s)
  • build: PASS (2.241s)
  • No paid h402/provider calls were made.

@h1-hunt h1-hunt added passed-claude Claude Reviewer passed this PR head needs-rework labels Jul 30, 2026
@h1-hunt

h1-hunt commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Parent verification — GPT Reviewer findings

Verified against exact head 1565fbbac5d3c40b61e66682e342f51356b00247.

  1. The reported runtime behavior is reproducible: --version=true, --version true, --help=true, and --help true exit 0, and a bare --version can mask --help=true. However, the pre-existing command-flag contract explicitly preserves the string "true" because flagBoolean() accepts it (source), and owner issue h402 --version <word> silently prints help and exits 0 #85 asks the top-level path to add the same arity-none check. The issue's reproduced malformed word case (--version wallet) now exits 1 with the JSON error, and masked unknown/non-true valued flags are rejected. This makes explicit string-true acceptance a contract/owner-adjudication question, not a demonstrated regression from this PR.
  2. The new spawnSync tests do inherit the developer HOME. Their tested invalid inputs reach assertTopLevelFlags() before command dispatch (source) and fail before config, wallet, network, signing, or payment work. A temporary HOME would improve hermeticity, but no state-mutation failure premise was reproduced for these rejection-path tests.

The original reviewer artifact and labels remain unchanged for audit. This note qualifies the blocker interpretation; it does not rewrite the reviewer verdict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rework passed-claude Claude Reviewer passed this PR head rejected-gpt GPT Reviewer found issues on this PR head

Development

Successfully merging this pull request may close these issues.

h402 --version <word> silently prints help and exits 0

1 participant