Skip to content

fix(cli): honor no-passphrase before prompts - #87

Open
h1-hunt wants to merge 1 commit into
mainfrom
fix/issue-84-passphrase-precedence
Open

fix(cli): honor no-passphrase before prompts#87
h1-hunt wants to merge 1 commit into
mainfrom
fix/issue-84-passphrase-precedence

Conversation

@h1-hunt

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

Copy link
Copy Markdown
Collaborator

Summary

  • make --no-passphrase consistently win over both bare and valued --passphrase
  • avoid an interactive prompt when contradictory flags explicitly force passphrase-less behavior
  • cover wallet creation and signing with focused regressions

Validation

  • npm run test -w @h402/cli -- tests/passphrase.test.ts — 14/14 passed
  • npm test — CLI 214/214 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 #84

@h1-hunt

h1-hunt commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

GPT Reviewer

Verdict: REJECT

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

The precedence fix appears correct in the helpers, but the required real CLI-path regression coverage is missing.

Findings

  1. MINOR — Contradictory flags are tested only at helper level (packages/cli/tests/passphrase.test.ts:57)
    • Evidence: The added regressions construct ParsedArgs-like flag objects and call signWithWalletPassphrase/createPassphrase directly. They never exercise parseArgs → assertKnownFlags → command dispatch, and the valued contradiction (--no-passphrase --passphrase value) is not tested at all. This does not satisfy the mandatory invariant requiring both contradictory forms to be validated through the real CLI path for creation and signing, including prompt avoidance and passphrase-less behavior.
    • Suggested fix: Add dispatch-level tests for wallet creation and signing covering both --no-passphrase --passphrase value and --no-passphrase --passphrase. Parse the actual argv, run known-flag validation and dispatch with mocked wallet/signing dependencies or an isolated temporary HOME, then assert no prompt/error, undefined passphrase use, expected JSON output, and no unintended side effects.

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (3.124s)
  • typecheck: PASS (2.86s)
  • lint: PASS (2.264s)
  • test: PASS (17.796s)
  • build: PASS (2.231s)
  • 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: REJECT

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

The fix itself is correct: both signWithWalletPassphrase and createPassphrase now resolve --no-passphrase before any promptBarePassphrase() path, so the contradictory forms (--no-passphrase with bare or valued --passphrase) yield passphrase-less behavior without a TTY prompt or non-TTY error, while bare --passphrase alone still prompts and valued --passphrase still passes through; the protected-wallet error ladder and JSON/stdout contracts are unchanged, and typecheck/lint/tests/build/pack validation all pass. However, the new contradictory-flag behavior is validated only via synthetic helper-unit flag objects (args({ "no-passphrase": true, passphrase: true })), not through the real parseArgs → assertKnownFlags → command path. The bundle's mandatory invariants explicitly require validating through that real path, and the test file's own cli#29 regression block exists precisely because passphrase flag behavior must survive real parsing, not just direct unit calls. Whether parseArgs actually yields { "no-passphrase": true, passphrase: true } for the real argv — in either flag order (e.g. --passphrase --no-passphrase could conceivably consume the next token as a value) — is unverified by any test, leaving the headline fix unguarded at the layer where the bug class lives. One minor actionable finding; everything else is clean.

Findings

  1. MINOR — Contradictory --no-passphrase/--passphrase regression is tested only at helper-unit level, not through the real parse→known-flag→dispatch path (packages/cli/tests/passphrase.test.ts:64)
    • Evidence: Both new tests construct flags synthetically: signWithWalletPassphrase(args({ "no-passphrase": true, passphrase: true }), ...) and createPassphrase(args({ "no-passphrase": true, passphrase: true })). No test drives parseArgs(["call", "web/search", "--no-passphrase", "--passphrase"]) (or the wallet-create equivalent, or the reverse flag order) through assertKnownFlags into the fixed helpers. The mandatory invariants state "Validate through the real parse→known-flag validation→dispatch path, not only a helper unit," and the same file's existing block is commented "Reviewer regression (cli#29): bare --passphrase must survive the real dispatch path — parseArgs + assertKnownFlags + command — not just direct unit calls." Whether parseArgs actually produces { "no-passphrase": true, passphrase: true } for the combined argv (particularly the --passphrase --no-passphrase ordering, where an optional-value flag could consume the next token) is unverified; if it did not, the fix would be inert through the actual CLI while these unit tests still pass.
    • Suggested fix: Extend the existing "bare --passphrase through the CLI dispatch path" describe block with the contradictory combinations, mirroring its established pattern: parse ["call", "web/search", "--no-passphrase", "--passphrase"] (and the reverse order, plus the valued form --passphrase secret --no-passphrase), assert parsed.flags["no-passphrase"] === true and parsed.flags.passphrase === true, assert assertKnownFlags(["call"], parsed.flags) does not throw, then assert signWithWalletPassphrase(parsed, "vault", sign) resolves with sign called with undefined. Add the analogous wallet-create case by asserting createPassphrase(parsed) resolves undefined for parseArgs(["wallet", "create", "--name", "x", "--no-passphrase", "--passphrase"]) after assertKnownFlags, keeping the test side-effect-free (no full walletCommand invocation needed).

Shared exact-snapshot validation

  • diff-check: PASS (0.0s)
  • npm-ci: PASS (3.124s)
  • typecheck: PASS (2.86s)
  • lint: PASS (2.264s)
  • test: PASS (17.796s)
  • build: PASS (2.231s)
  • No paid h402/provider calls were made.

@h1-hunt h1-hunt added rejected-claude Claude Reviewer found issues on this PR head needs-rework labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.

Bare --passphrase and valued --passphrase resolve --no-passphrase conflicts in opposite directions

1 participant