Skip to content

fix(review): claude reviewer path actually works (#51 C2/C3/H3) - #91

Merged
huiyu merged 1 commit into
3.0from
fix/51-claude-reviewer-path
Aug 24, 2026
Merged

fix(review): claude reviewer path actually works (#51 C2/C3/H3)#91
huiyu merged 1 commit into
3.0from
fix/51-claude-reviewer-path

Conversation

@huiyu

@huiyu huiyu commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Campaign-2 fix #2 — CRITICAL C2+C3 (re-opens #51 B4). From the independent re-verification: #84 fixed only the codex direction; the entire claude reviewer path (Codex→Claude) was dead.

Bugs

  • C2 claude's --json-schema takes inline JSON, not a file path (verified against claude 2.1.241 — a path exits 1 with "not valid JSON"). The code reused codex's schemaPath for both.
  • C3 claude -p --output-format json returns an array of stream events, not {findings}. The runner treated the event list as the findings list → 9 phantom empty findings (even once C2 is fixed).

Fixes

  • C2 --json-schema JSON.stringify(FINDINGS_SCHEMA) on the claude branch; codex's --output-schema keeps the file path.
  • C3 extractClaudeStructuredOutput() unwraps the terminal result event (or {result} envelope) to the schema-conforming payload before normalizing; a non-conforming reviewer degrades to invalid, never phantoms.
  • H3 (meta-lesson) the tests were falsely green — fabricated {findings} claude never emits + name-only smoke test. Now: the invocation test asserts --json-schema is inline JSON not the path (deterministic, always runs); the runner test drives the real event-array shape and asserts one real finding not one-per-event; a live binary-gated test actually runs claude with our inline schema and asserts the flag-parse error is absent.

Verification

  • test:cross-review: 23/23 — including the live claude smoke test (ran claude 17.7s, inline schema accepted).
  • Adapters regenerated — the codex mu-review mirror carried the identical bug, and it is the copy that runs under the Codex host.

🤖 Generated with Claude Code

Independent re-verification found #84 fixed only the codex direction; the entire
claude reviewer path (the Codex->Claude direction) was dead.

- C2: claude's --json-schema takes INLINE JSON, not a file path (verified against
  claude 2.1.241 — a path exits 1 with "not valid JSON"). Pass
  JSON.stringify(FINDINGS_SCHEMA) on the claude branch; codex's --output-schema
  keeps the file path.
- C3: claude -p --output-format json returns an ARRAY of stream events, not
  {findings}. The runner treated the event list as the findings list -> 9 phantom
  empty findings. Added extractClaudeStructuredOutput() to unwrap the terminal
  result event (or {result} envelope) to the schema-conforming payload before
  normalizing; a non-conforming reviewer degrades to invalid, never phantoms.
- H3 (meta-lesson): the tests were falsely green — the claude runner test fed a
  fabricated {findings} claude never emits, and the smoke test checked flag NAMES
  only. Now: the invocation test asserts --json-schema is inline JSON (not the
  path) deterministically; the runner test drives the REAL event-array shape and
  asserts one real finding not one-per-event; a new binary-gated live test runs
  claude with our inline schema and asserts the flag-parse error is absent (would
  have caught C2). extractClaudeStructuredOutput unit-tested across every shape.

Adapters regenerated (the codex mu-review mirror carried the identical bug —
and it is the copy that runs under the Codex host).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KwyFyRC6pepaK6DuMK7YeT
@huiyu
huiyu merged commit e09bf55 into 3.0 Aug 24, 2026
1 check passed
@huiyu
huiyu deleted the fix/51-claude-reviewer-path branch August 24, 2026 13:14
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