feat(ralph): parallel dual-reviewer, structured output, and infra discovery

[lavaman131]

Summary

Significantly hardens the Ralph workflow across all three SDK adapters (Claude, Copilot, OpenCode) by replacing the sequential confirmation-pass review loop with a parallel dual-reviewer strategy, adding SDK-native structured output validation, introducing parallel infrastructure-discovery sub-agents, and upgrading git context from a working-tree snapshot to a full branch-relative changeset.

Key Changes

Parallel Dual-Reviewer (all adapters)

• Replaced the sequential "two consecutive clean passes" approach (CONSECUTIVE_CLEAN_THRESHOLD = 2) with two simultaneous reviewer stages (reviewer-${iteration}-a and reviewer-${iteration}-b)
• The loop terminates only when both reviewers independently return zero actionable findings
• mergeReviewResults() unions findings from both reviewers and takes the more conservative overall_correctness verdict

SDK-Native Structured Output

• Claude: Uses claudeSdkQuery with outputFormat: { type: "json_schema", schema: REVIEW_RESULT_JSON_SCHEMA } — no manual JSON parsing
• Copilot: Uses defineTool("submit_review", { parameters: ReviewResultSchema }) (Zod-validated tool handler) — each parallel reviewer gets its own isolated tool instance
• OpenCode: Uses format: { type: "json_schema", schema: REVIEW_RESULT_JSON_SCHEMA } on the session prompt call
• Added ReviewFindingSchema and ReviewResultSchema Zod schemas in helpers/prompts.ts; REVIEW_RESULT_JSON_SCHEMA derived via z.toJSONSchema()

Parallel Infrastructure Discovery

• New buildInfraDiscoveryPrompts() generates prompts for three sub-agents that run in parallel before each review: codebase-locator, codebase-analyzer, and codebase-pattern-finder
• Discovery output is injected into the review prompt so the reviewer knows the exact build/test/lint commands to run for verification
• Review prompt now includes an explicit Verification Step requiring the reviewer to execute all discovered commands before writing findings

Branch-Relative Changeset (helpers/git.ts)

• Replaced safeGitStatusS() (just git status -s) with captureBranchChangeset(), which captures:
  • git diff <merge-base>...HEAD --stat and --name-status (committed changes from branch point)
  • git diff --stat (uncommitted staged/unstaged changes)
  • git status -s (working tree snapshot)
• Added discoverBaseBranch() that auto-detects the parent branch (main/master/develop) by finding the closest merge-base
• Git command failures are now collected in BranchChangeset.errors and surfaced to the reviewer rather than silently returning empty strings

Reviewer Agent Norms Guideline

• Added guideline Lavaman131/feature/24 7 agent #9 to all three reviewer agent configs (.claude, .github, .opencode): reviewers must respect project-level norms from AGENTS.md/CLAUDE.md and flag deviations only when they conflict with correctness or security, not personal preference

[claude]

Code Review — Ralph workflow robustness

Nice direction overall — replacing git status -s with a branch-relative changeset, adding SDK structured output, and parallelising the reviewer all meaningfully harden the loop. A few items worth addressing before merge.

Bugs

1. Claude SDK result field is result, not output (high) — src/sdk/workflows/builtin/ralph/claude/index.ts:68

raw = String((msg as Record<string, unknown>).output ?? "");

SDKResultMessage on subtype: \"success\" exposes result: string (see docs/claude-code/agent-sdk/sdk-references/typescript.md:814 and docs/.../core-concepts/sessions.md:114). There is no output field, so raw will always be \"\". Consequences:

• When structured output succeeds, the debugger prompt path still gets an empty reviewRaw.
• When the SDK hits error_max_structured_output_retries (a real failure mode), structured is null and raw is empty, so mergeReviewResults' text-parse fallback has nothing to work with, and hasActionableFindings(null, \"\") returns false — the loop silently terminates on a failed review. Use msg.result on success and capture msg.errors on error subtypes.

2. Reviewer sub-agent is bypassed on Claude (high) — claude/index.ts:52-82

Every other stage calls s.session.query(asAgentCall(\"reviewer\", …)), routing through the Claude Code reviewer sub-agent with its configured system prompt, allowed-tools, and permissions. queryWithStructuredOutput imports query directly from @anthropic-ai/claude-agent-sdk and runs a fresh top-level session — the reviewer sub-agent defined in .claude/agents/reviewer.md never executes. In addition, s.save(s.sessionId) persists the empty stage session, not the session where the review actually happened, so the graph view won't reflect reality. If bypassing the sub-agent was intentional, it's worth a comment explaining why; otherwise the structured output needs to be wired through the stage's session (or the sub-agent reviewer prompt needs to be inlined into the top-level query).

3. Unvalidated cast of structured_output (medium) — claude/index.ts:73

structured = (msg as Record<string, unknown>).structured_output as ReviewResult;

structured_output is typed unknown in the SDK, and error_max_structured_output_retries exists precisely because the model can fail to produce valid JSON. Run the payload through ReviewResultSchema.safeParse(...) before accepting it — otherwise a malformed payload silently becomes a malformed ReviewResult and propagates into filterActionable / the debugger prompt.

Design concerns

4. Cost/latency: three discovery sub-agents + two reviewers per iteration

Infra discovery (locator + analyzer + pattern-finder) is invoked every loop iteration, but the build/test infrastructure of a repo doesn't change between iterations. Up to 10 iterations × 5 sub-agents = 50 extra calls for data that's identical after iteration 1. Hoist discovery out of the loop (or memoize the discovery context after iteration 1). Same argument for captureBranchChangeset — cheap enough to keep in-loop, but the hot path is the agent calls.

5. mergeReviewResults concatenates findings with no deduplication — helpers/prompts.ts:97-98

Two reviewers seeing the same prompt will largely flag the same issues. [...findingsA, ...findingsB] doubles the debugger's input and, via hasActionableFindings, will keep the loop running for issues that are already in-flight. Deduplicate on (title, code_location.absolute_file_path, line_range) or similar before returning.

6. Verification step assumes the reviewer has Bash

buildReviewPrompt now tells the reviewer to "Execute them via Bash from the repository root." Review sub-agents are often configured read-only (no Bash), in which case this directive is silently ignored and the graded P0/P1 findings for "build failures" never materialise. Two better options: (a) add Bash to the reviewer's allowed-tools explicitly, or (b) run the verification commands deterministically from the workflow (similar to how captureBranchChangeset injects git data) and feed stdout/stderr + exit codes into the prompt.

7. Base-branch discovery only checks local heads — helpers/git.ts:95

refs/heads/main|master|develop misses the common case where a CI clone only has refs/remotes/origin/main. On GitHub Actions runners (actions/checkout@v4 default) only the PR branch is checked out, so discoverBaseBranch falls through to its hard-coded \"main\" default, then merge-base HEAD main fails and the errors[] array fills with noise. Worth also probing refs/remotes/origin/<candidate> and git symbolic-ref refs/remotes/origin/HEAD. Also, repos using non-conventional default branch names (trunk, prod, etc.) aren't covered.

Smaller items

• Sequential git calls: discoverBaseBranch runs rev-parse → merge-base → rev-list per candidate in series; captureBranchChangeset runs four diffs sequentially. Promise.all would shave a noticeable chunk off cold-start latency (Bun.spawn is cheap but not free).
• Copilot parallel tools share the submit_review name — OK because they live in separate sessions, but worth a comment so a future reader doesn't "simplify" it into one shared tool.
• Doc drift: prompts.ts header used to claim "Zero-dependency: no imports from the Atomic runtime." — the comment was removed but the module now imports zod, which is a new dep direction; confirm that's the intended trade-off.
• discoverBaseBranch tiebreaker on distance 0: if HEAD is the same commit as a candidate, rev-list --count returns 0 and wins; not actionable in the intended flow (feature branch off main) but could produce confusing output when running the workflow on main itself.

Test coverage

CLAUDE.md calls out TDD and bun test, but this PR adds captureBranchChangeset, discoverBaseBranch, mergeReviewResults, buildInfraDiscoveryPrompts, filterActionable (now exported), and queryWithStructuredOutput without any accompanying tests. At minimum, mergeReviewResults (pure, high-risk merge logic) and discoverBaseBranch (has several fallback paths) warrant unit tests — both are easy to exercise with mocked git output / hand-built StructuredReviewResults.

Style / conventions

Consistent with CLAUDE.md otherwise — Bun APIs (Bun.spawn), no any, ESM imports all look right. The as Record<string, unknown> casts in the Claude SDK shim should go away once the schema is validated with Zod (item 3).

Happy to help with a follow-up patch for items 1-3 if useful — those are the only ones I'd consider merge-blocking.