review: replace the Claude Agent SDK harness with Pi, and sandbox tool subprocesses with srt - #305
review: replace the Claude Agent SDK harness with Pi, and sandbox tool subprocesses with srt#305jwbron wants to merge 20 commits into
Conversation
🦋 Changeset detectedLatest commit: 1845b88 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 |
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comments that could not be inline-anchored
workflows/review/lib/dispatch-runner-pi.ts:4
suggestion (non-blocking): The "contract-identical" property rests on hand-maintained parity — the two runners live in separate test files that each mock their own library, with no shared conformance suite. And one field already diverges: on salvage the SDK runner zeroes cost (usd: 0/turns: 0, dispatch-runner.ts:169-171) while this runner keeps the accumulated usd/turns (lines 469-473). So a salvaged Pi run reports real cost and a salvaged SDK run reports zero — a per-arm bias in ex…
workflows/review/eval/README.md:233
thought (non-blocking): "the arm under test is the loop, not the toolbox" overstates the isolation. The arms share tool names, not implementations: the SDK arm gets the Agent SDK's ripgrep-backed Grep and offset/limit Read, while the Pi arm gets hand-rolled grep -rIn -E (POSIX ERE, --exclude-dir=.git only, no gitignore) and cat -n with a 30k cap (dispatch-runner-pi.ts:160,181). Regex dialect and ignore semantics change what each loop can find, independent of the loop itself. Worth s…
workflows/review/lib/dispatch-runner-pi.ts:146
note (non-blocking): This comment (and the changeset's "can never be handed edit or write") frames reviewers as unable to mutate the checkout, but the Bash tool below (lines 229-232) runs arbitrary bash -lc, so sed -i / rm / git checkout are all reachable — the no-mutation property is a naming convention, not a guarantee. This matches the SDK arm (parity, not a regression); the point is only that the containment boundary is the sandbox, not the tool list. Something like "no dedi…
workflows/review/lib/dispatch-runner-pi.ts:462
suggestion (non-blocking): A turn-cap overrun is accounted for differently across arms. The SDK arm throws on error_max_turns (dispatch-runner.ts:138), which dispatch.ts records as a shed lens; here the graceful shouldStopAfterTurn stop falls through to line 462 and returns lastText as an ordinary result, so dispatch.ts contract-parses prose and, when it isn't JSON, spends the malformed-output retry instead of shedding. Same underlying model behavior, different cost and accounting per…
workflows/review/lib/dispatch-runner-pi.ts:122
suggestion (non-blocking): The run() failure branch (kill / spawn-error → "command failed", lines 120-124) is untested — the grep-miss test only covers the numeric-exit-code path. If the typeof error.code !== "number" guard regresses, a killed or aborted tool call would surface as "(no output)" and the model would read a broken toolbox as "nothing matched." The catch-block's non-timeout rethrow (line 482) is similarly uncovered. Both are cheap to exercise (e.g. execute with an already-a…
workflows/review/lib/dispatch-runner-pi.ts:150
suggestion (non-blocking): Read runs cat -n over the whole file and caps at 30k chars, with no offset/limit. In production the agent can fall back to Bash, but the eval pins tools to Read/Grep/Glob (live-runner.ts:34), so a Pi-arm reviewer can't page past roughly the first 30k chars of a large file while the SDK arm's native Read supports offset/limit — an investigation-capability asymmetry inside the A/B. Adding optional offset/limit to the Pi Read tool would close it.
workflows/review/eval/live-runner.ts:155
nitpick (non-blocking): main() hardcodes runner: sdkRunner(), so the single-case smoke CLI (--case) ignores REVIEW_DISPATCH_RUNNER even though selectedRunner is defined just above and the README says the eval honors the env var. runner: selectedRunner() would let --case reproduce a Pi-arm run.
workflows/review/eval/live-runner.ts:112
suggestion (non-blocking): selectedRunner treats any value other than exactly "pi" as sdk, while the production entry point (dispatch.ts:897-902) throws on unknown values. A typo like REVIEW_DISPATCH_RUNNER=Pi in a local A/B would silently benchmark the SDK loop while the operator records a pi arm — the same silent-wrong-arm hazard resolveModelId guards against on the model axis. Consider throwing on anything other than sdk/pi/undefined.
workflows/review/lib/dispatch-runner-pi.ts:426
suggestion (non-blocking): shouldStopAfterTurn's captured !== undefined operand (stop after a submit, below the turn cap) is untested — the turn-cap test only drives the turns >= maxTurns operand. If that operand were dropped, an agent that already submitted would keep looping to the cap. A test that submits and then calls the passed shouldStopAfterTurn would cover it.
workflows/review/lib/dispatch-runner-pi.ts:302
suggestion (non-blocking): resolveModelId's prefix fallback accepts any catalog id that starts with the pin and takes the longest, with no suffix-shape check. Today's pins are unambiguous two-component aliases, but a future pin like claude-sonnet-4 against a catalog carrying both claude-sonnet-4-<date> and claude-sonnet-4-5-<date> would silently resolve to the 4-5 tier — the exact substitution the docstring says it must never do. Constraining the fallback to date-suffixed variants…
.github/workflows/review-eval-ab.yml:112
note (non-blocking): dispatch_runner and force_arms are independent inputs and force_arms defaults false — but a pi-vs-sdk harness A/B runs a byte-identical review.md, which the eval short-circuits to "no reviewable delta" unless --force-arms is set (eval/README.md:225-227). So the most natural first use, dispatching with just dispatch_runner=pi, clears the new guard and then runs nothing. Consider failing fast (or auto-enabling force-arms) when pi is selected without `force_a…
workflows/review/lib/dispatch-runner-pi.test.ts:43
question (non-blocking): The runner types Pi entirely through as casts and these mocks mirror the same assumptions (positional runAgentLoop args, a turn_end event carrying message.usage.cost.total, a shouldStopAfterTurn config key). If any of these differs in the real 0.83.0, the unit tests still pass while the cost / turn-cap paths fail silently in a live pi arm (e.g. usd: 0, cap never firing). Is an import-the-real-package smoke test planned to pin exports/arity to the shi…
First harness A/B: two runner bugs, not a verdict on PiSmoke A/B on this branch, both arms on identical pins (run 30592964392, $10.11 total, 9 cases):
Adversarial hard gate failed on the candidate arm: Do not read that table as a Pi result. Both failures trace to defects in the runner in this PR, now fixed in aebc9f2:
What the run did establish: the switch plumbs correctly end to end ( Next: re-run the smoke A/B on aebc9f2 before anyone reads a harness number off this branch. |
| */ | ||
| export const finalText = (texts: string[]): string => { | ||
| for (let i = texts.length - 1; i >= 0; i -= 1) { | ||
| if (texts[i].includes("{") && texts[i].includes("}")) { |
There was a problem hiding this comment.
suggestion (non-blocking): finalText returns the last turn that merely contains both { and }, not one that parses — so the sign-off-after-the-contract shape this commit set out to fix can still slip through. A closing turn like the `{}` default is fine — done is returned over the earlier turn carrying the real JSON, because it too has a brace pair, which reopens the "reviewer emitted its JSON then added a closing sentence" failure (run 30592964392) the docstring above says this guards against.
Consider validating the candidate before accepting it — e.g. slice from the first { to the last } and keep the turn only if JSON.parse succeeds, otherwise continue the backward scan. That makes the behavior match the docstring's promise ("the last turn that actually carries a JSON object") rather than "a turn that contains braces." The new regression test covers JSON-then-sentence, but not sentence-with-a-brace-then-nothing, so this gap would pass today's suite.
Re-run on aebc9f2: the failure is case-correlated, not plumbingRun 30596474354, same conditions (9-case smoke, identical pins,
The two fixes moved noise and cost in the right direction (noise now below the baseline's 57%), but the same two cases fail identically: Twice, the same two cases, and they are the security/adversarial pair. That is not random contract drift, so my free-text-final hypothesis was wrong as the explanation for these two (the fix was still correct on its own terms and is covered by a regression test). What the pattern points at instead: the candidate arm returned APPROVE where REQUEST_CHANGES was required on I am not patching on that hypothesis. Two guesses have now cost ~$20 of eval spend, and the eval report captures The next step should be instrumentation, not another arm: persist the raw final text (and the turn/tool-call counts) for any agent that fails contract parsing, so one run is diagnosable instead of one run per hypothesis. That is cheap, it is useful to the SDK arm too, and it is what the harness-parity read in the README asks for anyway. Leaving this draft here rather than iterating blind. |
There was a problem hiding this comment.
1 of 1 prior review thread is still unaddressed as of 1a55c28:
1 non-blocking thread still open
- suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:320:finalTextreturns the last turn that merely contains both{and}, not one that parses — so the sign-off-after-the-contract shape this commit set out to fix can still slip through. (Non-blocking; carried over from the prior review.)
Instrumented run: the failure is an empty final, on both harnessesRun 30650071285, passed (adversarial gate green on the candidate arm). The diagnosisThe captured raw output of every failed agent is length 0. Not prose, not a refusal message, not a contract cut off mid-emission: the sub-agent returned no final text at all, and So the malformed-output failure is not a Pi defect. It reproduces on the Claude Agent SDK harness, on the same security/auth-adjacent cases. An empty final on cyber-adjacent content is the signature #294 flags as the specialist-lens risk: "a refused lens is a silent coverage hole — it surfaces as a missing agent result, not an error." If that is what this is, it is observable today, in the eval corpus, on the default Either way the current error message actively misleads: an empty output is reported as if the model emitted something malformed. Corrections to my two earlier comments
First valid harness comparison
Tool calls per agent, the harness-parity signal:
Investigation depth matches closely, which is the evidence that Pi's loop is doing comparable work rather than scoring similarly by luck. That was the specific thing the counts were added to answer. What this does and does not establishIt does not establish that Pi is as good as the SDK harness. Three runs have now produced three different orderings on 9 cases, which is a direct demonstration of the run-to-run variance the eval README warns about. One smoke run is a tripwire. What it does establish: the seam works, the loop investigates at comparable depth, cost and noise are in-band, and the candidate arm can pass the adversarial gate. That is enough to justify the powered A/B, which is the measurement that can actually settle it. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
5 of 5 prior review threads are still unaddressed as of 151472e:
5 non-blocking threads still open
- nitpick (non-blocking)
workflows/review/eval/live-ab-report.ts:551: The new raw-output block wrapsf.outputin a bare triple-backtick fence. Captured reviewer output routinely contains t... - note (non-blocking)
workflows/review/eval/live-runner.ts:71: The new SDK-armtool_usecounting here has no direct test. The pi arm's count is covered in `dispatch-runner-pi.test.t... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:320:finalTextreturns the last turn that merely contains both{and}, not one that parses — so the sign-off-after-t... - nitpick (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:528: This commit addstoolCallsto both success returns (~499, ~508), but the salvage return in thiscatchblock omits it... - suggestion (non-blocking)
workflows/review/lib/dispatch.ts:143: This commit addstoolCalls?to the runner result, butPerAgentReportand theperAgent.push(...)in this file don't...
Stop reason:
|
| run | baseline recall | candidate recall |
|---|---|---|
| 30592964392 | 9/9 | 8/9 |
| 30596474354 | 9/9 | 8/9 |
| 30650071285 | 8/9 | 9/9 |
| 30654062900 | 9/9 | 7/9 |
Cost has stayed within $4.30-$5.16 on both arms throughout, and tool-call parity held where measured. The recall column is pure wobble: the 9-case smoke cannot separate these harnesses, which is exactly what the eval README says about single runs inside the noise bands. Only the powered A/B (full corpus x3, both arms) can answer the harness question.
There was a problem hiding this comment.
8 of 8 prior review threads are still unaddressed as of 49fd825:
8 non-blocking threads still open
- nitpick (non-blocking)
workflows/review/eval/live-ab-report.ts:551: The new raw-output block wrapsf.outputin a bare triple-backtick fence. - nitpick (non-blocking)
workflows/review/eval/live-producer.ts:131: New doc comment mischaracterizes whenstopReasonis set. - suggestion (non-blocking)
workflows/review/eval/live-producer.ts:634: The empty-output failure note'sstopReasonsuffix is never exercised by a test. - note (non-blocking)
workflows/review/eval/live-runner.ts:73: The new SDK-armtool_usecounting here has no direct test. - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:320:finalTextreturns the last turn that merely contains both{and}, not one that parses. - nitpick (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:566: This commit addstoolCallsto both success returns, but the salvage return in thiscatchblock omits it. - suggestion (non-blocking)
workflows/review/lib/dispatch.ts:143: This commit addstoolCalls?to the runner result, butPerAgentReportand theperAgent.push(...)in this file don't... - note (non-blocking)
workflows/review/lib/dispatch.ts:151:AgentResult.stopReasonis populated only by the Pi runner and the eval, never the default production SDK runner.
Note: re-review ran at scoped depth (re-review mode full).
Diagnosed: it is a refusal, and it hits the default rosterRun 30656579898, two cases, $1.57 total. The captured detail is unambiguous: It is a refusal. It is not context overflow. 5,207 total tokens, nowhere near a window. That hypothesis is dead, which is why the token counts were worth capturing. Why this matters more than the harness question#294 accepts refusal risk as a specialist-lens problem, detectable only via weekly drift, with
That is a concrete, cheap mitigation for a silent coverage hole in the default roster, and it is independent of Pi, of GPT-5.6, and of the cost question. Correction: my harness A/B was never a harness A/B
So every run I reported as "baseline sdk vs candidate pi" ran Pi on both arms. The recall column I presented four times as a harness comparison was the eval's own noise floor with an identical configuration on both sides. That reframes the earlier numbers rather than voiding them: four paired runs of an identical setup, with recall swinging 7/9-9/9 between arms, is a useful and slightly alarming measurement of how noisy a 9-case smoke is. But nothing so far compares the SDK harness to Pi's. Doing that properly needs a per-arm runner in |
03a7fd1 to
181e6b5
Compare
Path to removing Claude CodeThis PR deliberately does not remove anything: it adds a second runner, opt-in, with First, a distinction that keeps getting collapsed: "remove Claude Code" is two independent decisions, and they carry very different risk.
Step 1 — a real harness A/B (this PR, in flight)Before the per-arm runner fix in this branch,
Step 2 — powered A/BFull corpus x3, both arms, identical pins. The load-bearing measurement.
Step 3 — flip the default, keep the incumbent
Step 4 — delete the SDK runnerI would argue against this one even if steps 1-3 all pass. The SDK runner is the control arm: it is the reference the entire corpus was measured against, and it costs ~190 lines plus one dependency to keep. Delete it and the next comparison has no baseline — the GPT-5.6 Luna question this work originally came from, any Pi upgrade, any future model swap. That is removing the instrument to save almost nothing. If it goes anyway, it should be its own PR, after step 3 has soaked, and it should say plainly that the eval loses its harness control. Step 5 — the orchestrator (separate track, much higher bar)
The alternative is building a bespoke orchestrator harness on Recommendation: treat steps 1-3 as the live sequence, step 4 as probably-not, and step 5 as a separate proposal that has to justify itself on its own terms rather than riding the sub-agent result. |
ee85ff7 to
c22d9fd
Compare
b253aa8 to
fc0a447
Compare
…back when it refused (#315) 🖍 _This is an audit!_ 🖍 ## What Two things, split out of #305 so they are not blocked by the unproven Pi harness: **make a silent sub-agent failure visible**, and **fall back to another model when the pinned one refuses**. No model pin changes. No new dependencies. The default dispatch path is untouched. ## The bug this found `correctness-reviewer` is **refusing outright** on security-adjacent code, in production, today: ``` rawStopReason=refusal tokens=2in/5207total error=This request triggered restrictions on violative cyber content and was blocked under Anthropic's Usage Policy. ``` Reproduced on `incident-auth-bypass` and `adversarial-injection-approve` ([run 30656579898](https://github.com/Khan/actions/actions/runs/30656579898), $1.57). Not a context limit — 5,207 tokens. Not a harness defect — both the Claude Agent SDK and the Pi runner hit it identically, because it is the model. **The hazard was documented and the mitigation went on the wrong roles.** The README keeps the twelve specialist lenses on Opus *"because Fable's cyber safety classifiers can refuse benign security-focused analysis, and a refused security lens would be a silent coverage hole"*. Meanwhile the 2026-07-20 A/B moved `correctness-reviewer` — the **default roster's** load-bearing recall agent — onto Fable 5 for its recall gain. The shield went on the opt-in roles; the default finder went the other way and has been there since. A refused reviewer produces no error. It produces nothing, and the review proceeds without it. ## Visibility - An **empty final** is named as such (`empty output: the agent returned no final text`), distinct from unparseable prose, on both the eval and production contract parsers. The old message blamed the model's output shape for a request that was blocked before it produced any. - Both runners keep the detail they were discarding: **error message**, **provider raw stop reason**, and **token counts at failure**. The token counts are what ruled out context overflow. - A failed agent's **raw final text** is captured (truncated to 4k) and rendered in the A/B report under a collapsed heading. - Per-agent **tool-call counts** on both arms — the investigation-depth signal the README already tells readers to check. The cost of not having this: three eval runs, ~$30, chasing a contract bug that never existed. The instrumentation identified the real cause in one $1.57 scoped run. ## Fallback A refusal is **deterministic in the model** — the same prompt on the same pin refuses again — so the ordinary retry cannot help and the fallback must change the model. `lib/refusal-fallback.ts`: | Pinned model | Falls back to | Basis | | --- | --- | --- | | `claude-fable-5` | `claude-opus-4-8` | measured (run 30656579898) | | `claude-opus-5` | `claude-opus-4-8` | pre-emptive; #294 notes Opus 5 also ships elevated cyber safeguards | Three deliberate rules: - **One hop**, never back to a model that already refused. - **No fallback for an unlisted pin.** An unmapped model's refusal stands and is reported, so a new family's refusal profile stays visible instead of being papered over. - **Never silent.** The swap is recorded per agent (`fellBackTo`) and lands in the report and run artifact. Trading an invisible skip for an invisible model swap would defeat the point, and the drift corpus needs to see how often this fires. Anthropic's own refusal message points integrators at a fallback model. #294 states "gh-aw exposes no `fallbacks` parameter, so there's no server-side retry-on-another-model to lean on" — true of gh-aw, but scripted dispatch owns its runner, so the policy can live in our code regardless. ## Known gaps, deliberately not in this PR - **The fallback is wired into the eval producer, not the production dispatcher.** `lib/dispatch.ts` has its own retry path and still loses a refused reviewer silently. That is the more urgent half and wants its own review; this PR makes the failure *visible* on both paths, which is the prerequisite. - **Opus 4.8's own refusal behavior on this content is unverified.** A probe is running ([30658862532](https://github.com/Khan/actions/actions/runs/30658862532)). If it refuses too, the fallback target is a one-line change — and a much more serious finding, since Opus 4.8 is the incumbent for all twelve lenses and `claim-validator` on the assumption that it is safe here. - An empty final with a non-normal stop reason arguably belongs in the **dispatch-failure** path rather than the contract-parse path. Deferred: it changes production retry semantics. ## Relationship to #294 #294 moves `correctness-reviewer` from Fable 5 to Opus 5. That may resolve this refusal or reproduce it — #294's own body flags Opus 5's "elevated cybersecurity safeguards" and `stop_reason: \"refusal\"`. With this PR merged that becomes a $1.57 measurement on two named cases instead of a judgment call, and either way the fallback covers it. ## Checks 1591 tests pass (15 new); `tsc --noEmit` clean. `pnpm lint` still cannot run in this worktree (pre-existing duplicate-eslint-plugin clash with the parent checkout, same as #294). Author: jwbron Auditors: jaredly, somewhatabstract Required Reviewers: Approved By: Checks: ✅ 7 checks were successful, ⌛ 2 checks are pending Pull Request URL: #315
fc0a447 to
67de169
Compare
There was a problem hiding this comment.
Pull request review — REQUEST_CHANGES
Re-review of the Pi-backed sub-agent harness behind the runner seam. The base-branch merge shrank the diff since the last pass; this run re-reviewed the full diff but scoped inline comments to newly-changed code. 1 blocking item, plus a handful of non-blocking parity/coverage suggestions.
Blocking
- todo (blocking) —
dispatch-runner-pi.ts:422— theallowedToolstool-surface restriction ships with no test. This is the boundary on what the Pi sub-agent can touch; add a test that builds a runner with a restrictedallowedToolsand asserts the excluded tools don't reach the agent so it can't silently regress.
Non-blocking themes
- Tool-surface parity (Pi vs SDK). Glob (
find -pathcrosses/), Read (cat -n, 30k cap, no offset/limit), andrefusedkeyed offrawStopReasonvs the sibling runners'stopReasonall diverge subtly from the SDK harness. None is a proven defect, but together they mean a Pi arm can see a different world than the SDK arm it's being A/B'd against. - Coverage gaps on the new seam. The
selectedRunner/piRunnerselection, therefusedflag, and the diagnostics-into-errorMessageappend are all unexercised. - A/B harness.
selectedRunnersilently falls back to SDK on an unknown value (vsdispatch.tswhich throws), and the byte-identicalreview.mdshort-circuit now assumes both arms share a runner — which is no longer guaranteed.
Low-confidence question (not posted inline)
- question (non-blocking) —
dispatch-runner-pi.ts:408: provider re-registration overridesbaseUrlbut registers no credential. If pi-ai needs a client-side key for the steered provider this would fail at call time; if the proxy injects auth it's fine. Worth a one-line confirmation.
Prior review threads
3 of 10 prior review threads resolved; 7 still unaddressed as of 6c46cd3:
7 non-blocking threads still open
- suggestion (non-blocking)
workflows/review/eval/live-runner.ts:134: ThiserrorMessagecapture is unreachable; the throw guard above unwinds past the return carrying tokensAtFailure/stopR... - note (non-blocking)
workflows/review/eval/live-runner.ts:73: The new SDK-armtool_usecounting here has no direct test. - suggestion (non-blocking)
workflows/review/eval/live-runner.ts:98: The SDK arm'stokensAtFailure.inputreads onlyusage.input_tokens, omitting cache tokens, so at a real overflow it r... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:320:finalTextreturns the last turn that merely contains both{and}, not one that parses — so the sign-off-after-t... - nitpick (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:568: This commit addstoolCallsto both success returns, but the salvage return in this catch block omits it — a salvaged r... - suggestion (non-blocking)
workflows/review/lib/dispatch.ts:144: This commit addstoolCalls?to the runner result, butPerAgentReportand theperAgent.push(...)don't thread `resu... - note (non-blocking)
workflows/review/lib/dispatch.ts:152:AgentResult.stopReasonis populated only by the Pi runner and the eval, never the default production SDK runner, and n...
…ess behind the runner seam Rebuilt directly on main. The visibility and refusal-fallback work this branch used to carry landed separately via #315, so replaying it would have conflicted with itself; what remains here is the harness alone. Scripted dispatch gains a second AgentRunner implementation (lib/dispatch-runner-pi.ts) built on Pi's multi-provider libraries, selected by REVIEW_DISPATCH_RUNNER=pi. The Claude Agent SDK runner stays the default and the production path; no model pin moves, the orchestrator engine is untouched, and dispatch.ts imports neither runner directly (the CLI entry loads one lazily). With the env var unset, behavior is unchanged. A harness switch, not a model switch: both runners honor the same per-role model: pins, so pi-vs-sdk on an unchanged review.md isolates the loop. Pi's libraries are multi-provider, so once the harness is anchored, moving a role off Anthropic becomes a pin change rather than a second bespoke agent loop. live-ab.ts also gets PER-ARM runners. It previously built one and used it for both arms, which is right when the variable is review.md but made REVIEW_DISPATCH_RUNNER put both arms on the same harness — four earlier runs reported as sdk-vs-pi were in fact pi-vs-pi. The baseline is now always sdkRunner (the reference the corpus was measured on); the candidate takes the selected harness.
6c46cd3 to
d62a33b
Compare
…ary, and fix two selection-seam smells Addresses the blocking item plus three non-blocking ones from the re-review. Blocking: allowedTools shipped untested. It is the boundary on what a Pi sub-agent can touch, and it rests on a stronger guarantee than an allowlist: an unregistered tool cannot be called, because Pi has no permission layer to fall back on. Three tests now cover it: a restricted surface excludes Bash and LS, an omitted option grants the full five, and submit_result survives restriction (the contract channel is not an investigation tool, so restricting the toolbox must not cost the agent its only way to deliver a result). Also fixed: - The refused flag keyed only off rawStopReason while both sibling runners key off the normalized stopReason, and this file's own refusal fixture sets only stopReason, under which refused was false. Either field now trips it, so a refusal reaches dispatch.ts's fallback whichever field the provider fills. - selectedRunner silently mapped any non-pi value to the SDK runner, so a typo would run the SDK arm while the operator believed they were measuring Pi. It now throws like dispatch.ts does: the arm under test swapping invisibly is the worst failure this seam has. - The selection seam itself was untested; live-runner.test.ts covers the default, both known values, and the throw. Documented, not fixed, two tool-surface parity gaps the review found. Pi's Glob uses find -path, whose star crosses a slash, giving a WIDER file set than the SDK arm; Pi's Read has no offset/limit windowing, giving a NARROWER view of large files. Both change what a Pi arm can see relative to the SDK arm, so they are called out at the tool definitions for anyone reading an A/B result.
eb33b82 to
a265267
Compare
Review GuidanceCommon patterns3 files: Eval harness runner swap: replace -import {sdkRunner} from "./live-runner";
+import {piRunner} from "./live-runner";
...
- const runner = sdkRunner();
+ const runner = piRunner();2 files: Investigation-cap CLI invocation changed from -`cd gh-aw-review-lib && npx -y tsx workflows/review/lib/investigation-cap.ts request <id>`
+`cd gh-aw-review-lib && node --disable-warning=MODULE_TYPELESS_PACKAGE_JSON workflows/review/lib/investigation-cap.ts request <id>`2 files: Package dependency swap: remove - "`@anthropic-ai/claude-agent-sdk`": "0.3.205",
- "zod": "4.4.3"
+ "`@anthropic-ai/sandbox-runtime`": "0.0.67",
+ "`@earendil-works/pi-agent-core`": "0.83.0",
+ "`@earendil-works/pi-ai`": "0.83.0",Excluded from review (3 files)Not individually reviewed — generated, formatting-only, or
|
There was a problem hiding this comment.
6 of 6 prior review threads are still unaddressed as of a6d96bc:
6 non-blocking threads still open
- question (non-blocking)
workflows/review/eval/harness-probe.ts:35: The control arm is deleted in the same PR that documents an unexplained systematic quality loss, with the probe that wou... - thought (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:127: The re-anchoring A/B measured Pi WITHOUT the srt sandbox, yet this PR both designates that run the anchor and changes th... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:131: The one writable shared path (the cap journal) re-opens the cross-agent channel the sandbox closes; the runner already s... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:227: spawn()'s command-failure branch (command failed: ...) is untested.createReviewToolsgrep-miss returns `(no output)... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:477:finalTextreturns the last turn that merely contains both{and}, not one that parses — so the sign-off-after-t... - question (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:502: Does the dated-release fallback still count as a 'pin'? The tier-jump fix keeps date-floating:claude-opus-4-8resolve...
| # diagnosis, not a check, and it costs real model dollars (~$0.50 per config). | ||
| # Runs here rather than on a dev box because the Anthropic key lives in CI, | ||
| # which is also where every other eval dollar is already spent. | ||
| harness-probe: |
There was a problem hiding this comment.
note (non-blocking): harness-probe job + eval/harness-probe.ts are shipped here but framed by the description as future follow-up, not delivered work. The body's only reference to the dm-default-backfill loss is "One follow-up owed before the model question" and "Sequencing from here" item 3 ("after the dm-default-backfill transcript read"), i.e. future work; neither the body nor the changeset states this PR adds the probe tooling/job to do it. No linked Jira/Confluence ticket key is present in the title, so this was judged against the PR description and changeset alone. Everything in the three enumerated commits is present and matches intent; this is a scope-disclosure gap, not a functional one.
…step, and print before scoring Run 30872172052 dispatched all three configs, spent about $1.50 of model work, and then died with `TypeError: posted is not iterable` in matchCase. My bug: matchCase scores POSTED candidates, so it needs the deterministic runner's RunResult, and I handed it produceLive's output instead. runCase is the missing step, exactly as live-ab.ts wires it: it puts the produced findings through the same provenance gate, scope filter, and verdict logic production uses. The second change is the one that matters more. The probe now prints what the models produced BEFORE it scores anything, because the failure mode above threw away completed, paid-for output for a bug in the cheap step that came after it. Expensive results get printed the moment they exist.
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…e dispatch from running the eval Two bugs found by reading tonight's runs rather than by testing. **The A/B never exits.** Runs 30872172052, 30872187609 and 30877187141 each scored all nine cases on both arms, wrote their report artifact, and were then killed by the job timeout with the work already complete. Same cause as the sandbox smoke's hang: srt's initialize starts a proxy nothing shuts down, so the event loop never empties. This is why no A/B run has ever concluded green, and it means the July re-anchoring run showing "cancelled" was the same thing rather than an operator cancelling it. `live-ab.ts` and `harness-probe.ts` now exit explicitly once stdout has drained, draining first because Actions gives the process a pipe and exiting over a buffered write truncates the tail of the very report the job exists to produce. **A probe dispatch ran the whole eval.** `live-ab`'s condition starts with "any workflow_dispatch", because that is how the eval is run by hand, so a dispatch asking only for the harness probe started a full A/B beside it. Two probe dispatches therefore spent about $18 nobody asked for. Both `live-ab` and `sandbox-smoke` now exclude `inputs.harness_probe == true`, so a dispatch means what it says. The accidental runs did earn their money: three independent arm-pairs is what it took to settle the lost finding as noise (see the plan doc), which two repeats could not do.
There was a problem hiding this comment.
Approved — no blocking issues found.
Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable).
Note: skill-auditor not assessed this run (skill-auditor output unavailable).
Note: completeness not assessed this run (completeness output unavailable).
Note: holistic not assessed this run (holistic output unavailable).
Note: first-principles not assessed this run (first-principles output unavailable).
Note: conventions not assessed this run (conventions output unavailable).
Note: thread reconciliation not assessed this run (thread-reconciler output unavailable).
Note: re-review ran at scoped depth (re-review mode scoped).
|
@jwbron I don't see a jira ticket, so I just want to make sure I'm understanding this right; using Pi instead of claude sdk is going to be substantially faster/cheaper with no loss of quality? Is that the motivation for this work? |
Yes. I was initially adding pi to test out other (cheaper) models, particularly gpt 5.6 luna. However, when I did the initial side by side with claude code, I found that pi made reviews substantially cheaper and faster with no loss in quality. See here for details. I've since ran more A/B tests and have found that the quality and consistency is equivalent for both harnesses, so there's no reason to keep using claude code. |
mojadem
left a comment
mojadem
left a comment
There was a problem hiding this comment.
I'm excited about the results that drove this change! I have a few questions about the Pi-specific implementation.
- Why are we using
@earendil-works/pi-agent-corerather than the Pi SDK (@earendil-works/pi-coding-agent)? It feels like we are reimplementing a lot of what it already does, e.g. model, lifecycle, tool, retry, and event plumbing. - Is there a reason to provide Grep / Glob / LS tools when we are already providing a Bash tool? It feels like they don't add much considering we are already wrapping the execution in SRT for security.
Regardless, this seems like a step in the right direction. Will leave approval to maintainers of this repo.
|
Good questions; both deserved better answers than the code carried. 1. Why
The docstring's current stated reasons are weaker than this ("exactly these tools" is answerable with 2. Grep/Glob/LS vs Bash: you're right, and the defense we had (calibration) doesn't survive the fact that re-anchoring is now one ~$9, 23-minute A/B. Security-wise the named tools add nothing over sandboxed Bash. What survives on its own merits: Read (line-number fidelity, and it needs offset/limit windowing it doesn't have) and Grep (structured argv avoids the shell-quoting failure class). LS adds nothing over Bash. Glob is actually wrong, not just limited: |
…re hand-rolled The docstring justified the hand-rolled tool surface with "the reviewers need exactly these tools", which pi-coding-agent's createReadOnlyTools answers. The load-bearing reason, verified against the SDK's 0.83.0 dist, is that its tool layer cannot be uniformly sandboxed: grep spawns rg with no interceptable seam, read is in-process fs.readFile, and only bash has a spawnHook, so adopting the factories would leave Read/Grep running unwrapped in the credentialed runner process. Raised by mojadem on #305. Comment-only change.
…Grep/Bash, window Read, and align the eval to it LS added nothing over sandboxed Bash; Glob's find -path emulation was wrong, not just limited (* matched across /). Read gains offset/limit windowing so large files stop being silently truncated at the output cap with an unreachable tail. The eval's measured arms drop their Read/Grep/Glob restriction and run the production surface unrestricted, so the A/B measures what production ships by construction. Raised by mojadem on #305; re-anchor A/B run to follow on this PR.
|
The tool-surface change from the review discussion is now in this PR (3393ae2, arrived via #325 which auto-closed when this branch fast-forwarded over it): LS and Glob are gone (LS added nothing over sandboxed Bash; Glob's |
|
Re-anchor A/B on the new tool surface is green at 3393ae2 (run 30937356436, 29 minutes end to end):
No lost findings, no found-but-dropped, adversarial hard gate passed on the candidate arm, and the sandbox smoke (including the live phase on the Read/Grep/Bash surface) passed in the same run. The surface change's exit criterion is met at the branch head. |
There was a problem hiding this comment.
7 of 7 prior review threads are still unaddressed as of 3393ae2:
7 non-blocking threads still open
- note (non-blocking)
.github/workflows/review-eval-ab.yml:194: harness-probe job + eval/harness-probe.ts are shipped here but framed by the description as future follow-up, not delive... - question (non-blocking)
workflows/review/eval/harness-probe.ts:35: The control arm is deleted in the same PR that documents an unexplained systematic quality loss, with the probe that wou... - thought (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:140: The re-anchoring A/B measured Pi WITHOUT the srt sandbox, yet this PR both designates that run the anchor and changes th... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:144: The one writable shared path (the cap journal) re-opens the cross-agent channel the sandbox closes; the runner already s... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:277: spawn()'s command-failure branch (command failed: ...) is untested.createReviewToolsgrep-miss returns `(no output)... - suggestion (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:500:finalTextreturns the last turn that merely contains both{and}, not one that parses — so the sign-off-after-t... - question (non-blocking)
workflows/review/lib/dispatch-runner-pi.ts:525: Does the dated-release fallback still count as a 'pin'? The tier-jump fix keeps date-floating:claude-opus-4-8resolve...
| // 3. The cap journal is writable THROUGH THE REAL CLI, invoked exactly as | ||
| // the sub-agent prompts invoke it. Two things are under test at once | ||
| // and both are load-bearing: the one writable mount in the staging dir, | ||
| // and whether `npx -y tsx` can even start with the network denied (the |
There was a problem hiding this comment.
nitpick (non-blocking): The probe-3 comment still describes testing npx -y tsx under network denial, contradicting CAP_CLI_COMMAND's own "node, never tsx" contract. The sibling constant's doc (and investigation-cap.ts's entry-guard comment) explain that tsx cannot run inside the sandbox at all, which is precisely why the CLI moved to node-native type stripping — this leftover sentence describes the invocation this PR replaced. Introduced by this change.
A sketch, not a committable replacement:
Reword to: "and whether `node` type-stripping can run the CLI with the network denied".
| execute: async (_id, params, signal) => { | ||
| const path = String(params["path"] ?? ""); | ||
| const out = await exec(["cat", "-n", "--", path], cwd, signal); | ||
| return ok(windowLines(out, params["offset"], params["limit"])); |
There was a problem hiding this comment.
nitpick (non-blocking): windowLines is applied to cat's error output too, so a failed Read with a window masks the actual error message. The exec seam deliberately resolves spawn errors and non-zero exits as ordinary text ("command failed: ..." or the cat stderr), and windowLines then slices that text as if it were file content; only windowed reads are affected since an unwindowed call returns the text untouched. Introduced by this change (the windowing is new).
| return ok(windowLines(out, params["offset"], params["limit"])); | |
| Skip windowing when the output is not a numbered capture, e.g. `return ok(/^\s+1\t/.test(out) ? windowLines(out, params["offset"], params["limit"]) : out);` |
| // and IS a failure the model needs to see. | ||
| const failed = error !== null && typeof error.code !== "number"; | ||
| if (failed) { | ||
| resolve(`command failed: ${error.message}`); |
There was a problem hiding this comment.
suggestion (non-blocking): spawn's command-failure vs no-match classification is untested. The grep exit-1 (numeric code) path is covered by the real-grep 'grep miss' test, but the failure branch that distinguishes a spawn error / timeout-kill (non-numeric code) from an ordinary non-zero exit is not exercised. This is the one boundary that decides whether the model sees 'command failed: ...' or a misleading '(no output)'.
| resolve(`command failed: ${error.message}`); | |
| it("reports a failed spawn as a command failure, not as empty output", async () => { | |
| const out = await plainExec(["definitely-not-a-real-binary-xyz"], "/tmp"); | |
| expect(out).toContain("command failed"); | |
| expect(out).not.toBe("(no output)"); | |
| }); |
|
|
||
| // 4. The scratch dir is writable: the model's own workspace, which nothing | ||
| // downstream reads. | ||
| const scratchProbe = `${SCRATCH_DIR}/sandbox-smoke-probe`; |
There was a problem hiding this comment.
suggestion (non-blocking): The scratch-dir probe checks a fixed filename that persists across runs, so a stale file makes a denied write score as allowed. SCRATCH_DIR is a fixed /tmp path that createToolExec pre-creates and nothing cleans; probe 2 avoids this by probing inside a fresh mkdtemp dir, but probe 4 does not. CI runners are ephemeral so the gate itself is unaffected, but a local rerun can false-pass the one probe in a job whose stated contract is that every inconclusive check fails. Introduced by this change.
| const scratchProbe = `${SCRATCH_DIR}/sandbox-smoke-probe`; | |
| const scratchProbe = `${SCRATCH_DIR}/sandbox-smoke-probe-${process.pid}-${Date.now()}`; // or rmSync(scratchProbe, {force: true}) before the probe |
| signal, | ||
| cwd, | ||
| ); | ||
| return spawn(wrapped.argv, cwd, wrapped.env, signal); |
There was a problem hiding this comment.
note (non-blocking): Sandboxed tool subprocesses inherit the runner's full environment, so the mount/network sandbox does not scrub secrets from what Bash can read. spawn() defaults to process.env and srt's returned env extends it (the tests model exactly {...process.env, SRT_MARKER}), so deny-all networking closes the dial-out channel but not the read-env-and-echo-into-output channel; the env inheritance is pre-existing (plainExec and the deleted SDK harness behaved the same), but this change amplifies it by giving the eval's measured arms Bash for the first time (they were pinned to Read/Grep/Glob) in the one environment that holds the real key — production excludes ANTHROPIC_API_KEY from the container, so the exposure is eval-only today.
A sketch, not a committable replacement:
In makeSandboxedExec/plainExec, pass a minimal environment (PATH, HOME, GIT_*) instead of inheriting process.env, or strip known secret variables before spawning.
| "Failed RTM_", | ||
| "sandbox-exec:", | ||
| "seatbelt", | ||
| "Operation not permitted", |
There was a problem hiding this comment.
suggestion (non-blocking): "Operation not permitted" in WRAPPER_FAILURES conflates a policy denial (EPERM) with wrapper breakage. summarizeProbes fails any probe whose detail matches a wrapper signature even when satisfied is true, and EPERM text is exactly what a working Seatbelt (and some seccomp-based Linux) denial prints; the more specific "bwrap:"/"Failed RTM_"/"sandbox-exec:" signatures already cover run 30867350588's actual failure mode. Fail-noisy rather than fail-open, but it can permanently red the smoke on a platform where the boundary holds. Introduced by this change.
A sketch, not a committable replacement:
Drop the bare "Operation not permitted" substring (or apply it only to probes whose `expected` is "allowed", where EPERM genuinely means the wrapper broke rather than the policy working).
| * write truncates the last lines of the very table the job exists to print. The | ||
| * unref'd fallback covers a drain callback that never fires. | ||
| */ | ||
| const exitWhenFlushed = (code: number): void => { |
There was a problem hiding this comment.
thought (non-blocking): The identical exitWhenFlushed srt-drain workaround is duplicated across three eval entry points. Three byte-identical copies of a load-bearing, srt-version-coupled workaround. A single shared helper (e.g. in a small eval util) would keep them from drifting.
| ].join("\n"), | ||
| ); | ||
| summaryLine(""); | ||
| if (bashCalls === 0) { |
There was a problem hiding this comment.
thought (non-blocking): Phase B gates on the model choosing to call Bash, the same model-choice gate class the file's own doc rejects as flaky. The module doc says journal growth is 'REPORTED, not asserted: whether a reviewer opens a budget request is the model's call, and a gate that depends on it would be flaky' — but bashCalls === 0 fails the job on exactly such a call. The case selection ('an investigation is what reaches for Bash') mitigates, not removes, the nondeterminism, and Phase A already proves Bash-through-the-sandbox deterministically via the real cap CLI.
A sketch, not a committable replacement:
Either report Bash reach like the journal delta, or make it deterministic: assert the Bash tool *definition* reached the loop (it's in the registered tool list) and let Phase A's cap-CLI probe carry the 'Bash works in the sandbox' claim.
| * deterministic must-catch match. Whichever configuration recovers the spec | ||
| * names the cause; if none does, the honest conclusion is that the difference | ||
| * is loop-level rather than framing-level, and the plan's written acceptance | ||
| * applies. |
There was a problem hiding this comment.
question (non-blocking): The probe that investigates the Pi arm's reproducible lost finding ships in the same PR that deletes the only baseline it could be compared against. The PR's own description says the dm-default-backfill transcript read is owed 'before treating the pi numbers as the anchor for a model swap', yet the SDK harness is removed now, and the probe by construction varies only Pi-side hypotheses (system prompt, submit channel). I checked the probe: its no-submit config still runs the Pi loop, so a negative result names no cause — it only rules the two framing hypotheses out. Git history makes resurrection possible but expensive (deps already left both trees).
A sketch, not a committable replacement:
Consider running the probe (and the transcript read) before merging, or state explicitly in the plan that a null probe result is accepted as-is with no intent to resurrect the SDK arm — so the 'owed follow-up' has a defined, executable end state.
…ack (env scrub, Read errors, probe hygiene) Review feedback on #305 (the blocking allowedTools test gap was already closed at head): - Sandboxed tool subprocesses no longer inherit the runner's credentials: makeSandboxedExec scrubs ANTHROPIC_API_KEY, GITHUB_TOKEN, GH_TOKEN, GITHUB_MCP_SERVER_TOKEN, and MCP_GATEWAY_API_KEY from the env srt returns. The sandbox is a mount/network boundary, not an env boundary. Test added. - A failed Read is never windowed: cat's error text survives verbatim instead of being sliced into '(no lines in window...)'. Test added. - The spawn-failure vs plain-non-zero-exit classification is pinned by a test (ENOENT resolves 'command failed: ...'). - 'Operation not permitted' removed from WRAPPER_FAILURES: EPERM text is what a WORKING sandbox emits on a policy denial, so matching it failed probes whose denial was the policy working. - The scratch-dir probe filename is unique per run; a stale file from an earlier run can no longer score a denied write as allowed. - The probe-3 comment states the CAP CLI's real contract (node, never tsx). - The three byte-identical exitWhenFlushed copies fold into eval/exit-when-flushed.ts (runCli), one copy of a load-bearing, srt-coupled workaround.
|
Addressed the latest review wave in 1845b88 (the blocking allowedTools test gap was already closed at head by 3393ae2's tests):
The earlier waves' comments were checked against head; the ones not listed here were already addressed by the 08-03/08-04 commits. |
3 participants
What
lib/dispatch-runner-pi.ts, on@earendil-works/pi-ai+@earendil-works/pi-agent-core) becomes the only dispatch harness, for production and the eval. The Claude Agent SDK harness and theREVIEW_DISPATCH_RUNNERseam are deleted; a leftover runner setting fails the run loudly instead of silently selecting a harness.@anthropic-ai/claude-agent-sdkandzodleave both dependency trees.@anthropic-ai/sandbox-runtime, the engine behind Claude Code's own sandbox: bubblewrap on Linux, Seatbelt on macOS): checkout mounted read-only, tool-level network deny-all, fail-closed initialization,REVIEW_SANDBOX=offas the explicit, logged escape hatch.pin-YYYYMMDD, latest date first, throw otherwise); the old prefix fallback could silently jump tiers. No pin moves in this PR: all 21 pins untouched.Evidence
review.md): quality a wash, cost 1.78-1.94x and wall 1.65x in Pi's favor. Clean repeat: recall 41/46 vs 40/46, verdicts 32/35 vs 33/35, noise within the SDK arm's own between-repeat wobble, investigation depth held (tool calls 307 vs 275). Full read: the A/B context comment.Sandbox properties
ToolExec), so sandboxed and unsandboxed paths differ only in how an argv is spawned.echo > file) into a mount boundary: a prompt-injected reviewer cannot poison the checkout its siblings read or the staged files downstream phases trust. The investigation-cap journal and a scratch dir are the only writable paths.Risks and follow-ups
bwrap,socat,rg) are not in the pinned production agent image. Production runs a pinned release, so this gates the next installed-reviewer bump, not this merge; fail-closed init makes the failure loud either way. Decide install-in-pre-steps vs pinned binaries (bwrapPath/socatPath/ripgrep.command) on the bump PR.ANTHROPIC_BASE_URLsteering (Pi's bundled provider hardcodesapi.anthropic.com; the runner re-registers it on the steered URL): a wrong assumption is an auth error, never a silent proxy bypass.Checks
Full deterministic suite,
tsc --noEmit, and lint green; the live A/B, srt smoke, and production-tool-surface jobs are green at the head.