review: replace the Claude Agent SDK harness with Pi, and sandbox tool subprocesses with srt

.changeset/review-pi-harness-seam.md

@@ -0,0 +1,84 @@
+---
+"review": minor
+---
+
+review: make the Pi runner the only sub-agent harness and remove the Claude
+Agent SDK runner
+
+Scripted dispatch now runs every sub-agent through the Pi-backed runner
+(`lib/dispatch-runner-pi.ts`, built on `@earendil-works/pi-ai` +
+`@earendil-works/pi-agent-core`). The Claude Agent SDK runner
+(`lib/dispatch-runner.ts`) is deleted, along with the
+`REVIEW_DISPATCH_RUNNER` selection seam; a leftover `REVIEW_DISPATCH_RUNNER`
+setting now fails the run loudly instead of silently selecting a harness that
+no longer exists. `@anthropic-ai/claude-agent-sdk` leaves the dependency tree
+entirely, and `zod` is no longer a direct dependency (it stays in the lockfiles
+transitively, via pi-ai / sandbox-runtime / mcp-sdk).
+
+The removal is grounded in the re-anchoring harness A/B (run 30666183461):
+two full-corpus repeats with identical model pins and byte-identical
+review.md showed arm-to-arm quality parity (recall 41/46 vs 40/46 and 40/46
+vs 41/46, verdict agreement 32/35 vs 33/35 both repeats, comparable
+investigation depth by tool-call count) with the Pi arm at roughly half the
+cost (1.78x and 1.94x) and 60% of the wall clock (1.65x both repeats).
+
+No model pin changes. The pins still resolve through Pi's Anthropic catalog
+(`resolveModelId`), which throws on an unknown pin rather than silently
+substituting. Pi reports usage with a per-component `cost` breakdown, so
+`AgentResult.usd` no longer inherits the api-proxy default-pricing path's
+cache-write under-count.
+
+The corpus recall figures, noise bands, and drift budget measured on the SDK
+loop era do not transfer numerically; the re-anchoring run above is the
+reference point for Pi-harness numbers going forward.
+
+Two posture details the SDK runner used to own and the Pi runner now owns
+explicitly. Sub-agent turns retry transient provider failures twice: pi-ai
+does not read `ANTHROPIC_MAX_RETRIES` (it calls the Anthropic SDK with
+`maxRetries: 0` and defaults its own retry helper to 0), so the runner passes
+the budget itself; without it one 429/529 on any turn sheds a whole review
+lens. And hitting the turn cap now reports `stopReason=max_turns` instead of
+looking like a clean free-text finish, so the single contract-parse retry
+tells an out-of-turns agent to conclude rather than correcting a JSON shape
+that was never the problem.
+
+Every reviewer tool subprocess now also runs inside an OS sandbox
+(`@anthropic-ai/sandbox-runtime`, the engine behind Claude Code's own
+sandbox: bubblewrap on Linux, Seatbelt on macOS): the checkout is mounted
+read-only (the one writable staging path is the investigation-cap journal,
+plus a scratch dir), and tool-level network is denied outright. In
+production this stacks inside the awf firewall; in the eval, which runs on a
+bare runner VM, it is the only boundary the tools have, and the A/B workflow
+now installs bubblewrap+socat for it. Sandbox initialization is fail-closed:
+if it cannot start (bubblewrap missing, user namespaces blocked in a nested
+container), dispatch refuses to run rather than silently degrading;
+`REVIEW_SANDBOX=off` is the explicit, logged escape hatch that restores the
+pre-sandbox posture.
+
+One consequence of that network denial, caught by the new sandbox smoke job
+before it could reach a consumer: the sub-agent prompts used to run the
+investigation cap as `npx -y tsx workflows/review/lib/investigation-cap.ts`,
+and inside the sandbox npx cannot resolve tsx (the registry is denied; `npx
+--offline` fails with ENOTCACHED even after the cache is warmed, and tsx
+itself cannot start there at all because it opens a unix socket, which
+bubblewrap refuses with EPERM). Every cap request would have failed, and the
+prompt's own fallback for a denied budget is "stop investigating and report
+what you have" — bounded investigation would have gone quietly inert. The
+prompts now invoke it as `node
+--disable-warning=MODULE_TYPELESS_PACKAGE_JSON
+workflows/review/lib/investigation-cap.ts`, which works on the node 24 the
+agent job already pins (native type stripping). Because type stripping
+reparses the file as ESM, the CLI's entry guard is now argv-based rather than
+`require.main === module` (which never fires under ESM and would have turned
+every cap request into a silent no-op success), and `node:fs` is a static
+import. Two constraints on that file follow: its imports stay type-only or
+node: builtins, and it is invoked with `node`, never `tsx`.
+
+New in the A/B workflow: a `sandbox-smoke` job that exercises the PRODUCTION
+tool surface under srt, which the measured arms deliberately do not (they stay
+on Read/Grep/Glob so the corpus calibration holds). Its boundary probes run
+through the same `createToolExec()` the production runner calls and assert
+each of read-the-checkout, write-the-checkout, the cap journal via the real
+CLI, the scratch dir, and outbound TCP lands on the policy's side; they cost
+nothing and are the hard gate. A second phase dispatches one live case on the
+full tool surface to prove the loop reaches Bash at all.

.changeset/review-pi-tool-surface.md

@@ -0,0 +1,25 @@
+---
+"review": minor
+---
+
+review: trim the sub-agent tool surface to Read/Grep/Bash, and window Read
+
+Two tools leave `createReviewTools`, raised in review on #305: LS wrapped
+`ls -la` verbatim and added nothing over sandboxed Bash, and Glob's
+`find -path` emulation was wrong rather than merely limited (`*` matched
+across `/`, so a reviewer asking for `src/*.ts` silently received nested
+files too). Directory listing and file finding go through Bash, where the
+model owns the semantics of its own command. Every tool runs through the
+same sandboxed executor, so the named tools that remain earn their place on
+model ergonomics, not containment: Read for windowed, line-numbered file
+views, Grep because structured params avoid the shell-quoting failure class.
+
+Read gains `offset`/`limit` windowing. Previously a large file was silently
+truncated at the output cap and its tail was unreachable, a recall defect;
+the window keeps `cat -n` line numbers so findings still anchor on real
+lines, and a partial view says which lines of how many it shows.
+
+The eval's measured arms now run this production surface unrestricted
+(previously they were pinned to Read/Grep/Glob, a surface production never
+ran), so the A/B measures what production ships, by construction. The
+re-anchoring run for the new surface is on the PR.

.github/workflows/review-eval-ab.yml

@@ -22,6 +22,14 @@
 # cap. A new push cancels a superseded run (concurrency below). A
 # `skip-live-eval` label skips the job entirely.
 #
+# Two jobs: `live-ab` measures quality arm to arm on the eval's three-tool
+# surface, and `sandbox-smoke` proves the PRODUCTION tool surface (Bash plus the
+# investigation-cap CLI) actually works inside the srt sandbox. They are
+# separate because the corpus is calibrated on the three-tool surface: giving
+# the measured arms Bash would move every number, and leaving the production
+# surface unexercised is how a fail-closed sandbox gets discovered on a
+# consumer's PR.
+#
 # NOTE: this workflow's name must stay distinct from any gh-aw workflow a
 # consumer runs (same-named workflows share a gh-aw concurrency group per PR
 # and cancel each other; see the round-two doc's operational floor).
@@ -62,6 +70,23 @@ on:
         type: boolean
         required: false
         default: false
+      probes_only:
+        description: "Sandbox smoke: run the free boundary probes only, skipping the live production-surface case"
+        type: boolean
+        required: false
+        default: false
+      smoke_case:
+        description: "Sandbox smoke: live case id for the production-surface phase (default incident-cache-missing-key)"
+        required: false
+      harness_probe:
+        description: "Run the harness probe for the lost finding instead of the eval (dispatch only)"
+        type: boolean
+        required: false
+        default: false
+      harness_probe_config:
+        description: "Harness probe config: all | as-is | fortified | no-submit"
+        required: false
+        default: "all"
 
 permissions:
   contents: read
@@ -72,15 +97,141 @@ concurrency:
   cancel-in-progress: true
 
 jobs:
+  # The sandbox smoke, separate from the measured arms on purpose: the A/B runs
+  # the eval's three-tool surface (Read/Grep/Glob) because the corpus was
+  # calibrated on it, so nothing in this workflow exercised the surface
+  # PRODUCTION runs — Bash, and the investigation-cap CLI writing its journal
+  # from inside a sandbox built to deny writes. This job covers exactly that,
+  # and reports no quality metrics.
+  #
+  # Cost shape: Phase A (the boundary probes) is free and always runs, and it
+  # is the hard gate. Phase B dispatches one live case on the production tool
+  # surface, so drafts get probes only; a ready PR and workflow_dispatch run
+  # both. See workflows/review/eval/sandbox-smoke.ts.
+  sandbox-smoke:
+    name: Production tool surface under srt
+    runs-on: ubuntu-latest
+    # A hung sandbox must fail the check, not squat a runner for six hours.
+    # srt starts a proxy it never shuts down, so a process that finishes its
+    # work can still outlive it (run 30867526519 printed a PASSED verdict and
+    # then held the runner for 25 minutes). The script now exits explicitly;
+    # this is the backstop for the next thing that holds the loop open.
+    timeout-minutes: 20
+    # `inputs.harness_probe != true` matters: a dispatch asking only for the
+    # probe used to start this job too, because "any workflow_dispatch" is how
+    # the eval is run by hand. Two probe dispatches therefore ran two full A/Bs
+    # nobody asked for (~$18). A dispatch now means what it says.
+    if: >-
+      (github.event_name == 'workflow_dispatch' && inputs.harness_probe != true) ||
+      (github.event_name != 'workflow_dispatch' &&
+       github.head_ref != 'changeset-release/main' &&
+       !contains(github.event.pull_request.labels.*.name, 'skip-live-eval') &&
+       (github.event.action != 'labeled' ||
+        github.event.label.name == 'full-eval'))
+    steps:
+      - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
+      # node 24 is load-bearing, not incidental: the cap CLI runs inside the
+      # sandbox as `node <file>.ts` on native type stripping, because npx
+      # cannot resolve tsx with the network denied and tsx cannot start there
+      # at all (it opens a unix socket, which bubblewrap refuses). The agent
+      # job pins node 24 for the same reason; this job proves the pairing.
+      - uses: ./actions/shared-node-cache
+        with:
+          node-version: "24.x"
+      - name: Install sandbox dependencies
+        # srt's Linux runtime dependencies, none of them in the runner image.
+        # ripgrep is not optional and not obvious: srt calls it to expand
+        # deny-path globs into concrete paths for bwrap, so a missing rg fails
+        # initialization outright -- which is how this job first failed (run
+        # 30867013094, "Sandbox dependencies not available: ripgrep (rg) not
+        # found").
+        run: |
+          sudo apt-get update -qq && sudo apt-get install -y -qq bubblewrap socat ripgrep
+          # bwrap needs a usable user namespace, and Ubuntu 24.04 runners
+          # restrict unprivileged userns under AppArmor, at which point bwrap
+          # cannot bring up loopback in its netns ("bwrap: loopback: Failed
+          # RTM_NEWADDR: Operation not permitted", run 30867350588) and every
+          # sandboxed command dies before it executes. The runner is ephemeral
+          # and ours, so relax it here, with before/after probes so the log
+          # says whether that was actually the cause. Production runs in a
+          # different environment and has to answer this separately.
+          . /etc/os-release && echo "runner: ${PRETTY_NAME}"
+          sysctl kernel.apparmor_restrict_unprivileged_userns || true
+          bwrap --unshare-net --dev-bind / / true \
+            && echo "bwrap netns: OK before" || echo "bwrap netns: FAILS before"
+          sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true
+          bwrap --unshare-net --dev-bind / / true \
+            && echo "bwrap netns: OK after" || echo "bwrap netns: FAILS after"
+      - name: Run the sandbox smoke
+        env:
+          ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
+          # Drafts pay nothing: probes only. A ready PR or a dispatch runs the
+          # live phase too, which is the only check of the tool DEFINITIONS.
+          PROBES_ONLY: ${{ inputs.probes_only == true || (github.event_name == 'pull_request' && github.event.pull_request.draft == true) }}
+          SMOKE_CASE: ${{ inputs.smoke_case || '' }}
+        run: |
+          EXTRA=""
+          if [ "$PROBES_ONLY" = "true" ]; then
+            EXTRA="--probes-only"
+          fi
+          if [ -z "$ANTHROPIC_API_KEY" ]; then
+            # A fork PR gets no secrets; the boundary probes still run and
+            # still gate. Said out loud so a green check is not read as
+            # "the production tool surface was exercised".
+            echo "ANTHROPIC_API_KEY not available; running the boundary probes only." >&2
+            EXTRA="--probes-only"
+          fi
+          if [ -n "$SMOKE_CASE" ]; then
+            EXTRA="$EXTRA --case $SMOKE_CASE"
+          fi
+          pnpm dlx tsx workflows/review/eval/sandbox-smoke.ts $EXTRA
+
+  # One question, one dispatch: why does the Pi arm lose
+  # incident-sql-missing-index:dm-default-backfill? Never runs on a PR -- it is a
+  # 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:
+    name: Harness probe (lost finding)
+    runs-on: ubuntu-latest
+    timeout-minutes: 45
+    if: github.event_name == 'workflow_dispatch' && inputs.harness_probe == true
+    steps:
+      - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
+      - uses: ./actions/shared-node-cache
+        with:
+          node-version: "24.x"
+      - name: Install sandbox dependencies
+        # The probe dispatches real sub-agents through the production runner, so
+        # it initializes srt and needs the same three dependencies and the same
+        # userns relaxation the other jobs do (see their notes).
+        run: |
+          sudo apt-get update -qq && sudo apt-get install -y -qq bubblewrap socat ripgrep
+          sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true
+      - name: Run the harness probe
+        env:
+          ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}
+          PROBE_CONFIG: ${{ inputs.harness_probe_config || 'all' }}
+        run: |
+          if [ -z "$ANTHROPIC_API_KEY" ]; then
+            echo "ANTHROPIC_API_KEY is required for the harness probe." >&2
+            exit 1
+          fi
+          pnpm dlx tsx workflows/review/eval/harness-probe.ts --config "$PROBE_CONFIG"
+
   live-ab:
     name: Live A/B over the eval corpus
     runs-on: ubuntu-latest
+    # Generous: a full-eval run over the live corpus is ~75 minutes of real
+    # model work. This only catches a hang (see the sandbox-smoke note).
+    timeout-minutes: 180
     # Drafts wait until ready; the changeset release PR matches the path
     # filter (it bumps workflows/review/package.json) but changes no
     # behavior; skip-live-eval opts a PR out.
     if: >-
-      github.event_name == 'workflow_dispatch' ||
-      (github.event.pull_request.draft == false &&
+      (github.event_name == 'workflow_dispatch' && inputs.harness_probe != true) ||
+      (github.event_name != 'workflow_dispatch' &&
+       github.event.pull_request.draft == false &&
        github.head_ref != 'changeset-release/main' &&
        !contains(github.event.pull_request.labels.*.name, 'skip-live-eval') &&
        (github.event.action != 'labeled' ||
@@ -92,6 +243,31 @@ jobs:
           # `git show`, so the full history is needed.
           fetch-depth: 0
       - uses: ./actions/shared-node-cache
+      - name: Install sandbox dependencies
+        # The eval runs the review sub-agents through the same srt tool
+        # sandbox production dispatch uses (dispatch-runner-pi.ts,
+        # fail-closed); bubblewrap, socat, and ripgrep are its Linux runtime
+        # dependencies and none is in the runner image. rg is the easy one to
+        # miss: srt shells out to it to expand deny-path globs for bwrap, and
+        # without it initialization throws before any case runs -- which is
+        # why this job has produced no green srt measurement yet.
+        run: |
+          sudo apt-get update -qq && sudo apt-get install -y -qq bubblewrap socat ripgrep
+          # bwrap needs a usable user namespace, and Ubuntu 24.04 runners
+          # restrict unprivileged userns under AppArmor, at which point bwrap
+          # cannot bring up loopback in its netns ("bwrap: loopback: Failed
+          # RTM_NEWADDR: Operation not permitted", run 30867350588) and every
+          # sandboxed command dies before it executes. The runner is ephemeral
+          # and ours, so relax it here, with before/after probes so the log
+          # says whether that was actually the cause. Production runs in a
+          # different environment and has to answer this separately.
+          . /etc/os-release && echo "runner: ${PRETTY_NAME}"
+          sysctl kernel.apparmor_restrict_unprivileged_userns || true
+          bwrap --unshare-net --dev-bind / / true \
+            && echo "bwrap netns: OK before" || echo "bwrap netns: FAILS before"
+          sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0 || true
+          bwrap --unshare-net --dev-bind / / true \
+            && echo "bwrap netns: OK after" || echo "bwrap netns: FAILS after"
       - name: Run the live A/B
         env:
           ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }}

package.json

@@ -10,8 +10,10 @@
         "build": "tsc -p actions/tsconfig.json"
     },
     "devDependencies": {
-        "@anthropic-ai/claude-agent-sdk": "^0.3.205",
+        "@anthropic-ai/sandbox-runtime": "0.0.67",
         "@changesets/cli": "^2.29.8",
+        "@earendil-works/pi-agent-core": "0.83.0",
+        "@earendil-works/pi-ai": "0.83.0",
         "@khanacademy/eslint-config": "^0.1.0",
         "@swc-node/register": "^1.11.1",
         "@types/node": "^25.3.3",
@@ -27,8 +29,7 @@
         "octokit": "^5.0.5",
         "prettier": "^2.6.2",
         "typescript": "^5.9.3",
-        "vitest": "^4.0.10",
-        "zod": "^4.4.3"
+        "vitest": "^4.0.10"
     },
     "packageManager": "pnpm@10.0.0+sha512.b8fef5494bd3fe4cbd4edabd0745df2ee5be3e4b0b8b08fa643aa3e4c6702ccc0f00d68fa8a8c9858a735a0032485a44990ed2810526c875e416f001b17df12b"
 }