review: the live producer (live A/B phase 2)#234
Conversation
…action and case staging Phases 2a/2b of the live A/B eval plan (#232), stacked on the Phase 1 corpus format (#233). agent-extract.ts turns review.md's '## agent:' sections into data (name, description, pinned model, prompt body). It takes the markdown as a string with no fs access, because the baseline arm of an A/B reads the merge-base review.md via git show. Parsing is strict; a malformed or model-less section throws listing every problem, since a silently dropped agent would skew an eval arm without failing it. An integration test extracts the real review.md (21 agents) and pins the staging-root reference so a future rename fails a test instead of silently staging nothing. live-stage.ts materializes the production staging layout for one live-enabled corpus case: pr-context.json (review.md Step 1's shape, synthetic identity fields), full.diff / pr.diff / full-stripped.diff (all the case diff; corpus diffs carry no generated files and no pattern-triage pass runs), files.json + review-files.json with the hasPatch cross-check derived from the diff parse, provenance.json, routing.json from the deterministic router, an out/ directory, and the post-change checkout copied from the case tree. rewriteAgentPrompt swaps the production staging root for the staged context dir. Everything sits behind an injected-fs seam and is memfs-tested. Model dispatch (phase 2c) is deliberately absent; it needs the Agent SDK dependency decision and arrives separately.
🦋 Changeset detectedLatest commit: 82bc5ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… runner (phase 2c)
live-producer.ts runs the live roster over one staged case behind an
injected LiveAgentRunner seam (the judge.ts pattern), so its logic is
stub-tested with zero model calls. Roster: the default finders
(correctness-reviewer, skill-auditor) plus the router's lensesToSpawn;
no pattern-triage or thread-reconciler in eval. It maps all three
sub-agent output contracts into the shapes the deterministic runner
consumes: label-shape findings (correctness lens, or conventions for
the skill-auditor so labelForFinding reproduces the best-practice
variants; confidence defaulted to 0.7 per the production claims rule),
structured-schema lens findings validated as-is, and the validator's
{claims: [...]} verdicts into CaseVerification[]. It stages
claims.json for the validator, resolves {{#runtime-import}} directives
against the case tree (a case opts into a skills index by carrying the
file), retries once on malformed output with the rejection fed back,
keeps partial results when an agent fails twice, prefixes colliding
finding ids, and reports per-agent cost/turns/wall-clock.
live-runner.ts is the one module that touches a real model runtime:
an Agent SDK query() per dispatch with Read/Grep/Glob only, cwd pinned
to the staged checkout, the agent's pinned model, and hard turn and
wall-clock caps; plus the CLI smoke entry
(tsx live-runner.ts --case <id>, requires ANTHROPIC_API_KEY). The
investigation-cap CLI the prompts reference is not staged; its own
denied-budget fallback applies. Adds @anthropic-ai/claude-agent-sdk
as a dev dependency.
jwbron
changed the title
…rpus' into jwbron/live-eval-producer-staging
…s with the case id Live agents choose their own finding ids, so every case's first correctness finding was live-correctness-reviewer-1; ids were unique within a case but collided across cases, and judge.ts's score join requires arm-global uniqueness. Caught by the first real A/B run (both arms completed, then judge aggregation threw). Ids are now <caseId>:<id> from the moment of parsing, so claims.json, the validator round-trip, the matcher, and the judge all see the same namespaced id.
…orpus' into tmp-refresh
…rpus' into jwbron/live-eval-producer-staging
…gin/jwbron/live-eval-producer-staging' into jwbron/live-eval-producer-staging
…rpus' into jwbron/live-eval-producer-staging
khan-actions-bot
requested review from
a team,
jaredly and
jeresig
and removed request for
a team
khan-actions-bot
requested review from
a team and
jaredly
and removed request for
a team
Review Guidancegithub-actions (4 files)
Common patterns2 test files: Identical 2 implementation files: Identical real- Excluded from review (2 files)Not individually reviewed — generated or formatting-only:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (entry.isDirectory()) { | ||
| copyDir(from, to, fs); | ||
| } else if (entry.isFile()) { | ||
| fs.writeFileSync(to, fs.readFileSync(from, "utf8")); |
There was a problem hiding this comment.
suggestion (non-blocking): copyDir round-trips every file through readFileSync(from, "utf8") + writeFileSync, and StageFs exposes no byte-preserving read/write — so a case tree containing a real binary (image, fixture archive) is silently corrupted in the staged checkout, and the live arm then investigates files that differ from what the case recorded. Consider adding a copyFileSync (or Buffer-based) member to StageFs and using it here; memfs supports both, so the test seam need not force the utf8 restriction.
Low-confidence notes (2)
workflows/review/eval/live-producer.ts:307— cross-agent id-collision resolution is order-dependent under concurrent dispatch, so a genuine collision is not resolved reproducibly across runs.workflows/review/eval/live-runner.ts:56— unverified (SDK not in checkout): doesallowedToolsrestrict the toolset underpermissionMode: "bypassPermissions", or only auto-approve it? If it does not restrict, Bash/Write/WebFetch stay reachable despite the read-only module doc.
| (output) => parseVerifications(output, knownIds), | ||
| ); | ||
| perAgent.push(report); | ||
| validation = parsed ?? []; |
There was a problem hiding this comment.
suggestion (non-blocking): The documented validator-failure fallback here (a twice-failed claim-validator yields validation = [] while produced findings survive) has no test — every produceLive test drives the validator happy path or skips it. Current behavior is correct; the gap is that a regression making a failed validator throw or drop findings would go uncaught. Consider a case that scripts the validator to return non-JSON twice and asserts findings.length > 0, validation is [], and the validator agent is marked failed.
| }); | ||
|
|
||
| headings.forEach((heading, i) => { | ||
| const sectionEnd = headings[i + 1]?.index ?? lines.length; |
There was a problem hiding this comment.
thought (non-blocking): A section ends at the next ## agent: heading or EOF, so a non-agent ## section appended after the last agent would be folded into that agent's prompt with no parse error — the silent skew the strict parser exists to prevent. It holds today only because agent sections are currently terminal in review.md. Ending a section at the next ## heading of any kind (or asserting in the integration test that none follows the agents) would close the gap.
| const DEFAULT_CONCURRENCY = 4; | ||
|
|
||
| /** The real filesystem, in the staging seam's shape (mirrors live-stage). */ | ||
| const NODE_FS: StageFs = { |
There was a problem hiding this comment.
nitpick (non-blocking): The sibling live-stage.ts:54 and corpus/loader.ts:724 both name this real-fs StageFs adapter DEFAULT_FS; here it is NODE_FS, though the line-133 comment says it "mirrors live-stage." Renaming to DEFAULT_FS keeps one searchable name for the concept across the eval directory.
| const verification = raw["verification"]; | ||
| if (typeof id !== "string" || !knownIds.has(id)) { | ||
| throw new Error( | ||
| `claims[${index}].id does not match a produced finding`, |
There was a problem hiding this comment.
suggestion (non-blocking): parseVerifications's rejection guards (unknown id here, invalid verification state just below) are untested — every scripted validator supplies valid ids and states. Consider a case emitting an id not among the produced findings (or verification: "maybe") and asserting the validator is marked failed and validation is [].
| anchor: { | ||
| type: "line", | ||
| path: raw["path"], | ||
| line: raw["line"], |
There was a problem hiding this comment.
suggestion (non-blocking): fromLabelShape hardcodes anchor.type: "line" with line: raw["line"], which validateFinding rejects for any non-positive-integer line. Since the label-shape contract in review.md uses "line": 0 as its placeholder and parseAgentFindings maps eagerly, a single line: 0 / omitted-line / PR-level entry throws and sinks the whole agent's batch (retry, then agent marked failed, losing its other valid findings). Consider treating a missing/zero line as a file- or PR-level anchor rather than throwing.
| producing_hunt: `live:${agentName}`, | ||
| model_authored_prose: | ||
| discussion === "" ? subject : `${subject} ${discussion}`.trim(), | ||
| ...(typeof raw["suggestion"] === "string" && raw["suggestion"] !== "" |
There was a problem hiding this comment.
nitpick (non-blocking): The suggestion -> suggested_patch round-trip (here, then carried back out as suggestion in buildClaims) is never asserted — no LABEL_FINDING fixture sets suggestion. A regression dropping it would silently strip suggested patches from live output with no failing test.
2 participants
Phase 2 of the live A/B eval plan (#232), complete, stacked on the Phase 1 corpus format (#233): everything needed to run the real model sub-agents over a live-enabled corpus case.
2a:
eval/agent-extract.tsParses
review.md's## agent:sections into{name, description, model, prompt}. String in, no filesystem access, because the baseline arm of an A/B extracts the merge-basereview.mdviagit show. Parsing is strict: missing/unterminated frontmatter, a name mismatch, a missing model, or an empty body throws with every problem listed, since a silently dropped agent would skew an eval arm without failing it. An integration test extracts the realreview.md(21 agents) and pins the default roster's reference to the production staging root so a rename fails a test instead of silently staging nothing.2b:
eval/live-stage.tsMaterializes the production
/tmp/gh-aw/review/layout for one case, perreview.mdStep 1's staging contract:pr-context.json(synthetic identity fields),full.diff/pr.diff/full-stripped.diff,files.json+review-files.jsonwith thehasPatchcross-check,provenance.json,routing.jsonfrom the deterministic router,out/, and the post-change checkout from the case's tree.rewriteAgentPromptswaps the production staging root across a prompt.2c:
eval/live-producer.ts+eval/live-runner.tsproduceLivedispatches the live roster (default finders plus the router'slensesToSpawn; no pattern-triage or thread-reconciler in eval) behind an injectedLiveAgentRunnerseam, the same pattern asjudge.ts's injected model, so all its logic is stub-tested with zero model calls. It maps the three sub-agent output contracts into exactly the shapes the deterministic runner consumes (RecordedFinding[]forproduceFindings,CaseVerification[]for the validation replay): label-shape findings get the code-assigned lens (correctness, orconventionsfor the skill-auditor solabelForFindingreproduces the best-practice label variants) and the production 0.7 confidence default; lens findings validate against the schema as-is; the claim-validator's{claims: [...]}output becomes verifications. It stagesclaims.json, resolves{{#runtime-import}}directives from the case tree (a case opts into a skills index by carrying the file), retries once on malformed output with the rejection fed back, keeps partial results when an agent fails twice, prefixes colliding finding ids, and accounts per-agent cost/turns/wall-clock.live-runner.tsis the only module touching a real model runtime: one Agent SDKquery()per dispatch, Read/Grep/Glob only, cwd pinned to the staged checkout, the agent's pinned model, hard turn and wall-clock caps, plus the CLI smoke entry:pnpm dlx tsx workflows/review/eval/live-runner.ts --case <id>(requiresANTHROPIC_API_KEY). Adds@anthropic-ai/claude-agent-sdkas a dev dependency.Documented deviations from production (module doc): no pattern-triage pass, runtime imports resolve from the case tree or a fixed note, and the investigation-cap CLI is not staged (its own denied-budget fallback applies).
Test plan:
pnpm run test --run: 680 tests green (21 new across the three test files: extraction fixtures + real-review.md integration, staging via memfs, producer via a scripted stub runner covering happy path, retry, double-failure, empty-findings skip, id collisions, missing-agent error, and runtime-import resolution).pnpm run typecheckand eslint clean.ANTHROPIC_API_KEY, which this environment does not have; it is the first thing to run when testing this PR (any of the 13 live case ids, e.g.--case incident-money-rounding). Expected cost: roughly $0.50 to $2 per case with the default roster.Next steps (human)
Key-bearing validationDONE, superseded by the phase 4 acceptance runs, which exercised this producer end to end with a real key over 7 live cases x 2 arms x 2 PRs (runs 29052332224 and 29052334404); measured ~$0.55-0.72 per case per arm, inside the plan's expected envelope. The CLI smoke remains available for local iteration.live-runner.ts):permissionMode: "bypassPermissions"with tools restricted to Read/Grep/Glob and cwd pinned to the staged checkout. Confirm you are comfortable with that posture for CI.live-producer.ts's module doc (no pattern-triage, runtime-import resolution, investigation-cap absence); each is a measurement caveat for the A/B.@anthropic-ai/claude-agent-sdk. Confirm it clears whatever dependency review applies to this repo.