Repository navigation
fix(compare): normalize JSON output before validation - #704
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 21, 2026, 8:28 PM ET / September 22, 2026, 00:28 UTC. ClawSweeper reviewWhat this changesNormalize compare’s JSON alias before validation and honor command-local output selection for early errors, with regression tests and documentation. Merge readiness✅ Ready for maintainer review This remains a useful, well-supported fix: pinned main and v0.18.0 still exhibit the underlying defect. No actionable correctness or security findings remain. Priority: P2 Review scores
Verification
How this fits togetherThe compare command sends one prompt to multiple ACP agents and summarizes their results. CLI output selection controls both those summaries and diagnostics produced before agents start. flowchart TD
A[CLI arguments and configuration] --> B[Resolve output selection]
B --> C[Validate invocation]
C -->|Invalid| D[Formatted diagnostic]
C -->|Valid| E[Run ACP agents]
E --> F[Comparison summary]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Use the shared format resolver and existing argument scanner so compare summaries and early diagnostics consistently honor the established JSON alias. Do we have a high-confidence way to reproduce the issue? Yes: current-main source rejects Is this the best way to solve the issue? Yes: normalizing the existing alias before shared validation is a focused repair, and reusing the existing scanner preserves option-value and delimiter boundaries. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 29de86019e82. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
|
Landed as 8d29425 from reviewed head b7bb6cd. The merged tree matches the reviewed/native-tested tree; Compare now resolves its JSON alias before validation and selects matching diagnostics for early configuration/input failures. The existing value-aware scan keeps literal option values and prompt delimiters intact. Comparison arrays, shared JSON-RPC errors, disabled-comparison classification, and format precedence retain their established roles. Validation passed: 64 focused tests, the full code/docs gate (2,218 tests plus 180 declared coverage tests), independent native assessment (9/18 before, 18/18 after), isolated review, all 12 CI jobs, and CodeQL. The first remote gate caught a missing brace in a test fixture; that was corrected before the successful final gate/review, with production package bytes unchanged. Public review found no actionable defects. Both AWS leases are released with broker cleanup complete. Native fixture archives were hash-verified before removing the exact scratch directories. All raw protocol, API-output and process receipts remain retained, including the three resolved intermediate CLI lookup races and the stated unknown peer kernel exit statuses. |
Compare applied
--jsonafter strict-output validation, so a valid--json-strict compare … --jsoninvocation failed with “requires --format json” before starting an agent. Early configuration and prompt-input errors also ignored command-local JSON selection.Resolve the alias into the shared format field before validation and remove compare's late override. Reuse its value-aware argument scan for early error formatting. The alias still forces JSON; without it, explicit root format takes precedence over local format, then configuration. Consumed option values and prompt text after
--remain literal. Disabled compare keeps its existingRUNTIME/exit-1 identity. Executed comparisons remain arrays, including when an agent fails; shared invocation errors remain JSON-RPC objects.Validation
pnpm run checkandpnpm run check:docspass on AWS Crabbox: 2,218 tests passed, four platform skips, and 180 declared coverage tests passed. Runrun_e6f6d2350cafb8a17feffaf25cab2247, leasecbx_83e04c39eda3.fastanderrorACP peers exchange initialize/new/prompt messages; the error peer returns a JSON-RPC prompt error rather than exiting to simulate failure.The mixed-result command, with only local executable/path prefixes abbreviated, was:
Its baseline exits 2 without admitting a peer:
{"jsonrpc":"2.0","id":null,"error":{"code":-32602,"message":"--json-strict requires --format json","data":{"acpxCode":"USAGE","origin":"cli","sessionId":"unknown"}}}The candidate exits 1 with empty stderr and the actual complete comparison array:
[{"agent":"fast","status":"ok","stop_reason":"end_turn","wall_ms":1287,"input_tokens":null,"output_tokens":null,"total_tokens":null,"final_message":"COMPARE_ALIAS:fast","error":null,"permission_requests":0,"permission_denied":0},{"agent":"error","status":"error","stop_reason":null,"wall_ms":1400,"input_tokens":null,"output_tokens":null,"total_tokens":null,"final_message":"","error":"SYNTHETIC_COMPARE_ALIAS_FAILURE","permission_requests":0,"permission_denied":0}]Actual request and completed error response from the admitted error peer:
{"jsonrpc":"2.0","id":2,"method":"session/prompt","params":{"sessionId":"synthetic-compare-error-31e04367-180c-4074-a211-eddb450d1d10","prompt":[{"type":"text","text":"SYNTHETIC_COMPARE_ALIAS"}]}} {"jsonrpc":"2.0","id":2,"error":{"code":-32603,"message":"SYNTHETIC_COMPARE_ALIAS_FAILURE"}}The 18-case set checks both the repair and its boundaries:
RUNTIME/-32603, exit 1, empty stderr, no peersUSAGE/-32602 rejection, exit 2For example, disabled compare still emits “compare subcommand is disabled by configuration (disableExec: true)” and takes precedence over a missing prompt file. A literal
--jsonconsumed by--fileor root--system-prompt, or placed after the prompt delimiter, does not select JSON.All 36 CLI captures complete with unchanged runtime fingerprints. Before: seven admitted peers, 21 typed RPC pairs and 27 completed writes. Candidate: ten peers, 30 pairs and 38 writes. CLI exits are directly observed; all 53 admitted CLI/peer identities retire without observer rescue. Peer kernel exit codes/signals remain unknown. Expected peer counts and output are assessed separately from capture integrity, so the baseline's early no-peer failures remain valid evidence.
Capture instrumentation pauses CLI/peer startup for native identity admission. It retains the existing stdin-iterator invocation observer; every case starts with closed stdin, and no payload is injected. The operator separately verified source/package provenance and the allowlisted child environment. All 529 source hashes and 38 package hashes match after commit hooks. Three intermediate CLI lookups raced their exits (one baseline and two candidate cases); initial admission, the owned OS wait and final retirement remained complete. Brief
ps/exited-Node displays are retained as non-endpoint observations, without descendant claims. This bounded proof covers local output selection and shared error handling, not arbitrary Commander parser exits, permission accounting, general stdin behavior, or provider behavior.