[PR #6258] [MERGED] fix(cli): exclude declared sensitive values from parser diagnostics #7233

New issue

Closed

opened 2026-11-01 21:46:21 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:21 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6258
Author: @kaoru0822-kitauji
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/6252-sensitive-parser-diagnostics

📝 Commits (2)

ba5ec3c fix(cli): exclude declared sensitive values from parser diagnostics
1219e54 fix(cli): redact whole rejected output-flag diagnostics

📊 Changes

5 files changed (+539 additions, -10 deletions)

View changed files

📝 src/cli.ts (+3 -1)
➕ src/cli/explicitSensitiveArguments.ts (+183 -0)
📝 src/cli/webNetworkCaptureCommands.ts (+2 -6)
📝 src/cliOutput.ts (+80 -3)
➕ tests/boundary/cli/explicitSensitiveArgumentsCli.test.ts (+271 -0)

📄 Description

What broke: CLI parser and output-flag rejections for inspect-web-network-capture repeated literals declared with --sensitive-value (for example Unknown flag: --no-x=<literal> and Invalid format: "<literal>"), because Incur rejects them before the service applies the declarations.
Fix: The CLI serve boundary reads declarations from the tokens Incur leaves for command parsing, whether or not parsing succeeds. An outermost middleware on commands that accept the option redacts parse, validation and thrown failure text. Incur builds rejected output-flag messages outside any middleware, so REA reports that first rejection itself, with the same whole-message redaction.
Risk: Low. Without a declaration, the path is a no-op. Success results are untouched. Error type, code, exit status and output structure are kept, and only diagnostic text changes. The mirrored output-flag message is pinned to Incur's output by a test that compares them byte for byte.

Summary

Fixes #6252.

--sensitive-value declarations already covered service, decoder and file-read failures. They were skipped for diagnostics produced before command dispatch:

Incur's command parser (unknown and negated flags, missing values, validation)
Incur's builtin output-flag pass (--format, --token-limit, --token-offset)

The capture docs and the sensitive_values contract both say diagnostic text excludes declared literals. With this change, those early diagnostics follow the same rule, and the rejection, exit status and a useful message are kept.

Problem and expected behavior

Smallest trigger (on base c537c52c9):

rea inspect-web-network-capture missing.har har --sensitive-value SYNTHETIC_CAPTURE_LITERAL_83A4 --no-unknown-option=SYNTHETIC_CAPTURE_LITERAL_83A4 --json
{ "code": "UNKNOWN", "message": "Unknown flag: --no-unknown-option=SYNTHETIC_CAPTURE_LITERAL_83A4" }

Violated invariant: diagnostic text must not repeat a value the caller explicitly declared sensitive. Incur stops at the first malformed token, so a declaration placed after that token was never parsed at all.

Change and scope

src/cli/explicitSensitiveArguments.ts (new) owns the option definition (moved from webNetworkCaptureCommands.ts) and how it applies to diagnostics before the service runs:
- Declarations are read from the tokens Incur leaves for command parsing, in both the --sensitive-value/--sensitiveValue and =value forms. The read does not depend on Incur's parse succeeding.
- Every registered command whose options include sensitiveValue gets an outermost command middleware. Incur runs parsing and validation inside that chain. The middleware re-throws ParseError, ValidationError, IncurError or Error with declared literals redacted from the message, hint, fieldErrors[].message and fieldErrors[].received. Type, code, retryable, exitCode, field paths and schema expectations stay as they are. Declarations are scoped to the invocation with AsyncLocalStorage.
- Incur rejects an output-flag value in its builtin pass, before it resolves the command and outside any middleware, using fixed text plus the value. When declarations are present, the serve boundary reports that first rejection itself. The whole message is redacted, just as the middleware does: for example, --sensitive-value token --token-limit token gives Invalid value for --[REDACTED]-limit: "[REDACTED]". The UNKNOWN code and Incur's toon or TTY rendering are kept.
- The contract is the same on every path: diagnostic text is redacted whole, including fixed template words that match a declaration. Error codes, document keys and framework framing (the TTY Error: prefix, usage text) stay intact.
src/cliOutput.ts is now the single owner of the Incur builtin-flag rules. cliOutputValueRejection mirrors Incur's acceptance rule and message, and parseCliOutputArguments uses it too. splitIncurBuiltinArguments mirrors Incur's builtin pass, so --sensitive-value --json X declares X just as Incur parses it.
src/cli.ts sends serve through this step after the existing argument normalization.

Not included:

A command name that does not resolve (rea <literal> --sensitive-value <literal>) still repeats in COMMAND_NOT_FOUND, because no command accepted the declaration.
Success output is not re-redacted. The service still owns that redaction and keeps its contract keys and enums intact.
A failed --evidence_output write echoes the caller-selected output path in details.path, even when that path contains a declared literal. This is a post-parse delivery path, not parser diagnostics. It is left out of this PR and can be followed up separately if that path should follow the declarations.

Contribution scope: ordinary fix within the existing CLI boundary. Maintainer scope decision not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: CLI serve boundary (createCli), before Incur's builtin pass and command parse.
Contribution scope fit and linked maintainer decision: existing redaction contract, not applicable.
CLI and MCP/tool-catalog contract: CLI diagnostic text only. The option name, description and schema are unchanged. MCP is unchanged, since it already validates through the shared application boundary.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none. Evidence output is untouched.
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
Generated metadata, package, or installation impact: none. metadata:check passes.

Expected effects:

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: tests/boundary/cli/explicitSensitiveArgumentsCli.test.ts runs the compiled public CLI and covers:
- a negated unknown flag
- an unknown flag name
- a declaration placed after the rejected token
- the camel-case = declaration
- a builtin flag between the declaration and its value
- spaced and = forms of --format, plus --token-limit and a partially declared --token-offset
- replacement markers that are themselves declared, where the rejected value stays rejected
- whole-message redaction where a declaration matches a flag name (token), overlaps another (tok/token), or matches fixed text (Invalid, Expected); declared codes and keys (UNKNOWN, code, message) stay intact
- parity: with an unused declaration, the REA-rendered output-flag rejection is byte-identical to Incur's own output for --format, --token-limit= and --token-offset
- fixed parser text (flag) redacted while JSON keys and codes stay intact
- validation text redacted while code and fieldErrors are kept
- controls: an undeclared token is still shown verbatim, and a declared value that the parser accepts (--format json) is still honored
Base reproduction (first commit): with the src/ changes removed, the original 12-case file fails 11 cases. Only the control passes.
Executed real-CLI probes (node scripts/rea.mjs, isolated HOME/XDG/TMPDIR): before the fix, 9 malformed invocations leaked the literal: the issue's three, plus --format=, --token-offset, an unknown flag name, a declaration after the rejected token, and the toon and yaml negated forms. After the fix, none leak. TTY (human) output through script is also redacted: 0 occurrences for the negated flag, --format, --token-limit, --record, --protobuf-framing and the unknown flag name.

User-visible CLI output (after):

{ "code": "UNKNOWN", "message": "Unknown flag: --no-unknown-option=[REDACTED]" }
code: UNKNOWN
message: "Invalid format: \"[REDACTED]\". Expected one of: toon, json, yaml, md, jsonl"

Remaining proof gaps: Windows and Linux were not run (only macOS). The published package was not tested. Thrown non-parse errors inside a command run are covered by the same middleware, but no test triggers one. The output-flag message mirror is written against the pinned Incur 0.6.1. The parity tests fail if an Incur upgrade changes that text.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Issue reproduction script plus extended probes on the built CLI: all parser and output-flag cases leak before the fix and none leak after. The only remaining leak is the out-of-scope unresolved command.
npm run test:focused -- tests/boundary/cli/explicitSensitiveArgumentsCli.test.ts tests/boundary/cli/cliOutput.test.ts tests/boundary/cli/dispatcher.test.ts tests/boundary/cli/closedStdout.test.ts tests/boundary/cli/cliOptionAliases.test.ts tests/boundary/cli/cliIntegerArguments.test.ts tests/boundary/cli/cliToolCoverage.test.ts tests/boundary/cli/cliStartupImports.test.ts tests/boundary/browser/historicalWebCapture.test.ts tests/boundary/browser/sazWebCapture.test.ts src/application/WebNetworkCaptureService.test.ts: 11 files, 173 passed, 1 skipped (after the review fix). The new file alone: 20 passed.
The same new test file with the src/ changes removed: 11 failed, 1 passed (expected).
npm run check:fast: passed.
TTY parity check with script: the declared and undeclared --format bogus outputs are identical (Error: Invalid format: ...).
npm run format:check: passed.
npm run metadata:check: passed.

Compatibility, safety, and release

Breaking changes or migration steps: none. Only diagnostic text changes, and only when declarations are present.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (macOS CLI only).
Package or release metadata impact: none.
Security, privacy, process, or containment review: narrows the exposure of explicitly declared literals. Nothing is treated as secret based on patterns.

Author readiness

Human owner: @kaoru0822-kitauji

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/6258 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/6252-sensitive-parser-diagnostics` --- ### 📝 Commits (2) - [`ba5ec3c`](https://github.com/morluto/rea/commit/ba5ec3cf36945fbe92813c3edeaf34b447ff5cd2) fix(cli): exclude declared sensitive values from parser diagnostics - [`1219e54`](https://github.com/morluto/rea/commit/1219e54b104051b9f10cef303225cf22e2ce6976) fix(cli): redact whole rejected output-flag diagnostics ### 📊 Changes **5 files changed** (+539 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `src/cli.ts` (+3 -1) ➕ `src/cli/explicitSensitiveArguments.ts` (+183 -0) 📝 `src/cli/webNetworkCaptureCommands.ts` (+2 -6) 📝 `src/cliOutput.ts` (+80 -3) ➕ `tests/boundary/cli/explicitSensitiveArgumentsCli.test.ts` (+271 -0) </details> ### 📄 Description **What broke:** CLI parser and output-flag rejections for `inspect-web-network-capture` repeated literals declared with `--sensitive-value` (for example `Unknown flag: --no-x=<literal>` and `Invalid format: "<literal>"`), because Incur rejects them before the service applies the declarations. **Fix:** The CLI serve boundary reads declarations from the tokens Incur leaves for command parsing, whether or not parsing succeeds. An outermost middleware on commands that accept the option redacts parse, validation and thrown failure text. Incur builds rejected output-flag messages outside any middleware, so REA reports that first rejection itself, with the same whole-message redaction. **Risk:** Low. Without a declaration, the path is a no-op. Success results are untouched. Error type, code, exit status and output structure are kept, and only diagnostic text changes. The mirrored output-flag message is pinned to Incur's output by a test that compares them byte for byte. ## Summary Fixes #6252. `--sensitive-value` declarations already covered service, decoder and file-read failures. They were skipped for diagnostics produced before command dispatch: - Incur's command parser (unknown and negated flags, missing values, validation) - Incur's builtin output-flag pass (`--format`, `--token-limit`, `--token-offset`) The capture docs and the `sensitive_values` contract both say diagnostic text excludes declared literals. With this change, those early diagnostics follow the same rule, and the rejection, exit status and a useful message are kept. ## Problem and expected behavior Smallest trigger (on base `c537c52c9`): ``` rea inspect-web-network-capture missing.har har --sensitive-value SYNTHETIC_CAPTURE_LITERAL_83A4 --no-unknown-option=SYNTHETIC_CAPTURE_LITERAL_83A4 --json { "code": "UNKNOWN", "message": "Unknown flag: --no-unknown-option=SYNTHETIC_CAPTURE_LITERAL_83A4" } ``` Violated invariant: diagnostic text must not repeat a value the caller explicitly declared sensitive. Incur stops at the first malformed token, so a declaration placed after that token was never parsed at all. ## Change and scope - `src/cli/explicitSensitiveArguments.ts` (new) owns the option definition (moved from `webNetworkCaptureCommands.ts`) and how it applies to diagnostics before the service runs: - Declarations are read from the tokens Incur leaves for command parsing, in both the `--sensitive-value`/`--sensitiveValue` and `=value` forms. The read does not depend on Incur's parse succeeding. - Every registered command whose options include `sensitiveValue` gets an outermost command middleware. Incur runs parsing and validation inside that chain. The middleware re-throws `ParseError`, `ValidationError`, `IncurError` or `Error` with declared literals redacted from the message, hint, `fieldErrors[].message` and `fieldErrors[].received`. Type, `code`, `retryable`, `exitCode`, field paths and schema expectations stay as they are. Declarations are scoped to the invocation with `AsyncLocalStorage`. - Incur rejects an output-flag value in its builtin pass, before it resolves the command and outside any middleware, using fixed text plus the value. When declarations are present, the serve boundary reports that first rejection itself. The whole message is redacted, just as the middleware does: for example, `--sensitive-value token --token-limit token` gives `Invalid value for --[REDACTED]-limit: "[REDACTED]"`. The `UNKNOWN` code and Incur's toon or TTY rendering are kept. - The contract is the same on every path: diagnostic text is redacted whole, including fixed template words that match a declaration. Error codes, document keys and framework framing (the TTY `Error:` prefix, usage text) stay intact. - `src/cliOutput.ts` is now the single owner of the Incur builtin-flag rules. `cliOutputValueRejection` mirrors Incur's acceptance rule and message, and `parseCliOutputArguments` uses it too. `splitIncurBuiltinArguments` mirrors Incur's builtin pass, so `--sensitive-value --json X` declares `X` just as Incur parses it. - `src/cli.ts` sends `serve` through this step after the existing argument normalization. Not included: - A command name that does not resolve (`rea <literal> --sensitive-value <literal>`) still repeats in `COMMAND_NOT_FOUND`, because no command accepted the declaration. - Success output is not re-redacted. The service still owns that redaction and keeps its contract keys and enums intact. - A failed `--evidence_output` write echoes the caller-selected output path in `details.path`, even when that path contains a declared literal. This is a post-parse delivery path, not parser diagnostics. It is left out of this PR and can be followed up separately if that path should follow the declarations. Contribution scope: ordinary fix within the existing CLI boundary. Maintainer scope decision not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: CLI serve boundary (`createCli`), before Incur's builtin pass and command parse. - Contribution scope fit and linked maintainer decision: existing redaction contract, not applicable. - CLI and MCP/tool-catalog contract: CLI diagnostic text only. The option name, description and schema are unchanged. MCP is unchanged, since it already validates through the shared application boundary. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. Evidence output is untouched. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata, package, or installation impact: none. `metadata:check` passes. Expected effects: - [ ] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [x] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/cli/explicitSensitiveArgumentsCli.test.ts` runs the compiled public CLI and covers: - a negated unknown flag - an unknown flag name - a declaration placed after the rejected token - the camel-case `=` declaration - a builtin flag between the declaration and its value - spaced and `=` forms of `--format`, plus `--token-limit` and a partially declared `--token-offset` - replacement markers that are themselves declared, where the rejected value stays rejected - whole-message redaction where a declaration matches a flag name (`token`), overlaps another (`tok`/`token`), or matches fixed text (`Invalid`, `Expected`); declared codes and keys (`UNKNOWN`, `code`, `message`) stay intact - parity: with an unused declaration, the REA-rendered output-flag rejection is byte-identical to Incur's own output for `--format`, `--token-limit=` and `--token-offset` - fixed parser text (`flag`) redacted while JSON keys and codes stay intact - validation text redacted while `code` and `fieldErrors` are kept - controls: an undeclared token is still shown verbatim, and a declared value that the parser accepts (`--format json`) is still honored - Base reproduction (first commit): with the `src/` changes removed, the original 12-case file fails 11 cases. Only the control passes. - Executed real-CLI probes (`node scripts/rea.mjs`, isolated HOME/XDG/TMPDIR): before the fix, 9 malformed invocations leaked the literal: the issue's three, plus `--format=`, `--token-offset`, an unknown flag name, a declaration after the rejected token, and the toon and yaml negated forms. After the fix, none leak. TTY (human) output through `script` is also redacted: 0 occurrences for the negated flag, `--format`, `--token-limit`, `--record`, `--protobuf-framing` and the unknown flag name. - User-visible CLI output (after): ``` { "code": "UNKNOWN", "message": "Unknown flag: --no-unknown-option=[REDACTED]" } code: UNKNOWN message: "Invalid format: \"[REDACTED]\". Expected one of: toon, json, yaml, md, jsonl" ``` - Remaining proof gaps: Windows and Linux were not run (only macOS). The published package was not tested. Thrown non-parse errors inside a command run are covered by the same middleware, but no test triggers one. The output-flag message mirror is written against the pinned Incur 0.6.1. The parity tests fail if an Incur upgrade changes that text. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - Issue reproduction script plus extended probes on the built CLI: all parser and output-flag cases leak before the fix and none leak after. The only remaining leak is the out-of-scope unresolved command. - `npm run test:focused -- tests/boundary/cli/explicitSensitiveArgumentsCli.test.ts tests/boundary/cli/cliOutput.test.ts tests/boundary/cli/dispatcher.test.ts tests/boundary/cli/closedStdout.test.ts tests/boundary/cli/cliOptionAliases.test.ts tests/boundary/cli/cliIntegerArguments.test.ts tests/boundary/cli/cliToolCoverage.test.ts tests/boundary/cli/cliStartupImports.test.ts tests/boundary/browser/historicalWebCapture.test.ts tests/boundary/browser/sazWebCapture.test.ts src/application/WebNetworkCaptureService.test.ts`: 11 files, 173 passed, 1 skipped (after the review fix). The new file alone: 20 passed. - The same new test file with the `src/` changes removed: 11 failed, 1 passed (expected). - `npm run check:fast`: passed. - TTY parity check with `script`: the declared and undeclared `--format bogus` outputs are identical (`Error: Invalid format: ...`). - `npm run format:check`: passed. - `npm run metadata:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only diagnostic text changes, and only when declarations are present. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (macOS CLI only). - Package or release metadata impact: none. - Security, privacy, process, or containment review: narrows the exposure of explicitly declared literals. Nothing is treated as secret based on patterns. ## Author readiness Human owner: @kaoru0822-kitauji - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:46:21 +00:00