feat(harness): agent-sdk adapter with per-call tool gate (#21)

[queso]

Part of #21 (slice 3). Adds the agent-sdk harness adapter, which drives the Claude Code CLI through @anthropic-ai/claude-agent-sdk query() and asks the kernel's tool gate before every tool call. It is stacked on #91 (the gate and its contract). Its base is feat/21-tool-gate so the diff shows only this slice, and GitHub retargets it to main when #91 merges.

What is in it

• The adapter (src/worker/harness-adapter-agent-sdk.ts), registered as agent-sdk with canGatePerCall. It relies on what the Supervised harness adapters: per-call gating via vendor SDK callbacks (Claude Agent SDK, Codex app-server, OpenCode SDK) #21 spikes measured:
  • spawnClaudeCodeProcess starts the CLI detached, in its own per-invocation cgroup, registered with trackProcessGroup, and ended with the same killContained the harness runner uses. This reaches a setsid grandchild (Harness containment does not reap Claude Bash-tool commands: the CLI setsids them out of the process group #77).
  • It builds the whole allowlisted env itself, because the SDK does not merge process.env. It defaults to the claude on PATH, resolved to a file, not the SDK's bundled binary.
  • hooks.PreToolUse calls the gate through callGateFailClosed. An allow returns no permissionDecision, so allowedTools still decides and nothing is widened.
  • Errors use the codes the executor already handles: harness-timeout, harness-idle-timeout, harness-rate-limited (parks as before) and harness-nonzero-exit. Authentication failure is classified on assistant.error or terminal_reason, never on subtype, which reads success for it.
  • SDK messages go through the feat(harness): harness event stream, onEvent sink and Claude stream-json mapper #70 mapper (mapClaudeStreamMessage, split out of mapClaudeStreamLine), so rate-limit and usage events reach onEvent unchanged.
  • Supports _ENV, _COMMAND, _MODEL and _ISOLATE_CONFIG. _AGENT and _PLUGIN_DIRS fail registry construction. No session resume.
• Executor and critic wiring. The gate is built per invoke only for an adapter with canGatePerCall. Owned paths reach it only when the flow enforces them and the card declares some, the condition runOwnedPathsIntegrity uses. The critic gate may write only verdict.json. A hold moves the card to hold without spending an execution attempt, on the maker and critic paths. No fold site was added or moved, and the hold's recovered usage goes through the existing folds. The mutation manifest guards now name the new test file too.
• Journal. gate-decision is a harness_events kind with decision, gate_code, agent_id and a 200-character reason (no tool input bodies). journal inspect and tail print it, including (no span) invocations. The journal DB has no version number, so the four columns come from its existing idempotent ALTER ladder.
• Hold billing. A hold answers the call with a deny plus continue: false and a stopReason, so the CLI stops and emits its result message (about 15 ms in a live run, which returned 14453 tokens and $0.03). The thrown error carries that usage, and the existing folds bill it. After the first hold every later call is denied without asking the gate. If no result arrives within 5 s (HOLD_STOP_WAIT_MS) the process is killed as on a timeout, and the error carries no usage. interrupt() was measured and rejected: it yields an error result and the iterator throws. A deny alone was rejected too: the model retried the tool.
• Load-time error. A station on a gating adapter with an empty tools list or unrestricted_tools: true fails at load with HARNESS_GATED_ADAPTER_NEEDS_TOOLS. Either would deny every tool, including the Write the station needs for its output.
• Docs. The middle claim in docs/harness-containment.md, the agent-sdk notes in docs/harness-adapter-registration.md, and the CLAUDE.md wired inventory.

Review fix on top of the slice

The executor passed ownedPaths: [] to the gate for a card with no owned paths in an enforcing flow. runOwnedPathsIntegrity treats that as an opt-out, so the gate would have denied every write the integrity check leaves unenforced. The new executor tests run against the real gate and fail without the fix. The adapter's wait for the CLI exit event also left a 5 s timer running after each call.

Dockerfile

bun add pulls the SDK's optional platform binaries (232 MB glibc, 226 MB musl), which the adapter does not use because it runs the claude on PATH. The image now installs with --omit=optional. Checked against the real lockfile: 110 MB instead of 568 MB, and the SDK still imports.

Known limits

• Hold billing depends on the CLI emitting a result after continue: false. It did in every live run, but the 5 s kill fallback (no usage) was only unit-tested. Parallel tool calls in one turn (the latch denies the later ones), a hold from inside a subagent, and a held critic were not observed live.
• Not verified: named agents, session resume, OpenCode, black-box journeys for the SDK path, nested or parallel subagents, hook timeout behavior (a throwing gate is tested), running without pathToClaudeCodeExecutable, and a host without cgroup v2 (unit tests inject process-group containment).

Tests

• 4029 pass, 10 skip, 0 fail. bun run test:mutation protects all 5 fold sites. bun test blackbox/ passes 70. tsc is clean.
• 50 adapter unit tests on a fake SDK stream. The containment conformance suite passes through the real SDK spawn, including the setsid grandchild.
• An opt-in live test (CONDUIT_E2E_AGENT_SDK=1) ran twice against haiku: usage and cost populated, and a Bash write was blocked by the gate.

🤖 Generated with Claude Code

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 75a8d6aa-878a-4824-8a03-70313bbc693f

📥 Commits

Reviewing files that changed from the base of the PR and between 984bb14 and b858b11.

⛔ Files ignored due to path filters (1)

• bun.lock is excluded by !**/*.lock

📒 Files selected for processing (24)

• CLAUDE.md
• Dockerfile
• docs/harness-adapter-registration.md
• docs/harness-containment.md
• docs/installation.md
• package.json
• scripts/mutation-check.ts
• src/cli/journal-harness-events.test.ts
• src/controller/executor-harness-gate-hold.test.ts
• src/controller/executor.ts
• src/controller/gate-rework.ts
• src/flow/load-harness.test.ts
• src/flow/load.ts
• src/integration/harness-e2e-agent-sdk.test.ts
• src/persistence/db.ts
• src/quality/gate.ts
• src/worker/harness-adapter-agent-sdk.test.ts
• src/worker/harness-adapter-agent-sdk.ts
• src/worker/harness-adapter-claude.ts
• src/worker/harness-adapter.ts
• src/worker/harness-events-claude.ts
• src/worker/harness-events-journal.test.ts
• src/worker/harness-events-journal.ts
• src/worker/harness-registry-binding.test.ts

 _______________________________________
< r/CodeReview: Where I'm the top post. >
 ---------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Nitpick review — comment

The change adds a new agent-SDK harness adapter with per-invocation cgroup containment, gate-hold plumbing in the executor, and gate-decision journaling, and it is largely well-tested. Three real defects survive: the adapter tracks only the most recently spawned child, so any second spawn escapes killAll and the finally-block cleanup; the gate-hold path in executeHarnessStation passes err where the catch binding is invokeErr, mis-attributing (or breaking) the hold; and the run-scoped config dir leaks if the SDK import or containment probe throws before the try/finally begins. The CLAUDE.md journal-kind inventory is also now stale (missing gate-decision), and the rework-pass critic-hold branch lacks a test. Verdict: comment — fix the tracking slot, the err/invokeErr mixup, and the config-dir cleanup window before merge.

5 inline comment(s).

[github-actions]

Nitpick review — comment

The change introduces an agent-sdk harness adapter with per-call tool gating, gate-decision journaling threaded through persistence and the executor, and solid test coverage. Two issues warrant fixing before merge: the adapter tracks only the most recently spawned child process, so any additional spawn leaks its process group and cgroup past the containment boundary, and the maker-path gate-hold in the executor passes err to escalateToHold where the catch binding is invokeErr, mis-attributing (or failing to compile) the hold's error. Remaining items are non-blocking: a stale 'writes only' kind list in CLAUDE.md that now contradicts the gate-decision sentence in the same paragraph, raw stderr tails embedded in thrown error messages, a misleading resolution failure for relative executable paths, a scrap-shaped journal outcome on held cards, and potentially flaky wall-clock ceilings in the new tests.

7 inline comment(s).

[github-actions]

Nitpick review — comment

The change adds a gated agent-sdk harness adapter with cgroup containment, gate-decision journaling, and load-time validation, well covered by tests. Two real risks survive: the adapter tracks only the most recent spawned child, so any second CLI process would escape cleanup, and the maker gate-hold path passes the wrong error binding (err instead of invokeErr) to escalateToHold. The change's breadth — a new process-execution path, SQL writes, and deploy config — warrants human review before merge.

5 inline comment(s).

[github-actions]

Nitpick review — comment

This PR integrates a new Agent SDK harness adapter with per-call tool gating, gate-hold escalation in the executor, gate-decision journaling, and Docker packaging. The core design is sound and well-tested, but the maker-path gate-hold escalation passes the wrong error variable (err instead of invokeErr), so the hold's journal attribution carries the wrong error object or fails outright. Given the new execution surface and infra changes, human review of the change is recommended before merge.

3 inline comment(s).

1 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change adds an Agent SDK harness adapter with per-call tool gating, a new 'hold' gate-decision path through the executor, and the supporting persistence and Docker changes. Two real risks survive: the gate-hold call in executeHarnessStation passes err where the surrounding scope binds the thrown error as invokeErr, which either fails to compile or attributes the hold to the wrong error; and the Dockerfile drops the SDK's pinned binaries in favor of whatever claude resolves to on PATH, an unpinned executable running with the station's tool permissions. The journal's reason sanitization invariant also rests entirely on the producer side with no enforcement or test at the write boundary. Given the execution-path and infra exposure, human review is recommended; verdict is comment — no blocking defect, but the err/invokeErr mismatch and the unpinned binary should be fixed before merge.

4 inline comment(s).

1 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change adds a substantial agent-SDK harness adapter with per-call tool gating, event journaling, and Docker packaging, and the core gating and journaling paths look coherent. Two real issues survive review: the gate-hold escalation in the executor passes err where the surrounding code consistently uses the caught invokeErr, risking the wrong error being journaled on the hold path, and the new harness_events schema comment promises reason sanitization that the persistence layer's own insert path does not perform. Given the breadth of exec exposure, SQL changes, and infra edits in this diff, human review is recommended before merge.

3 inline comment(s).

3 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change introduces an agent-SDK harness adapter with per-call tool gating, gate-hold escalation, and harness-event journaling, and it is well covered by dedicated tests. Two issues warrant attention: the maker-path gate-hold escalation passes err where the caught binding in scope is invokeErr (a likely ReferenceError or mis-attributed hold cause), and the new journal migration ladder for pre-existing harness_events tables has no test exercising the ALTER path against an old-schema database. The agent-sdk adapter also documents that named agents are unimplemented but does not fail closed if one is configured. None of these are merge-blocking, but the err/invokeErr mismatch should be fixed before merge.

1 inline comment(s).

2 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change wires a new agent-sdk harness adapter (with per-call tool gating, run-scoped config isolation, and gate-decision journaling) into the executor, persistence, and docs, and is well covered by a large new test suite. Two things warrant attention before merge: the run-scoped config directory is created just outside the try whose finally cleans it up, so a throw from env building would leak a directory containing a credentials symlink, and the new hold path passes err to escalateToHold while interpolating invokeErr — worth confirming they are the same object, since that error is what the human resolving the hold sees. Verdict: comment; no blocking issues.

2 inline comment(s).

2 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change wires the new agent-sdk harness adapter, per-call gating, and harness event journaling into the executor and CLI, and the overall structure matches the existing harness patterns. One real defect survives: the maker gate-hold path passes err to escalateToHold inside a catch that binds invokeErr, so the hold record can carry the wrong or a missing error. Two lower-severity items — an empty critic tool allowlist that can wedge gates into permanent holds, and missing coverage for the new isolate-config wiring — are worth addressing but non-blocking.

2 inline comment(s).

1 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change adds the agent-SDK harness adapter with per-call tool gating, the gate-hold path through the executor, and supporting journal/loader/docs work, and it is largely coherent with the existing gate and budget machinery. One real defect survives: the hold escalation in executeHarnessStation passes err where every other reference in the block uses invokeErr, so the hold's recorded cause may be the wrong error object. Two smaller items: child stderr tails flow unfiltered into persistent error/journal metadata, and the waived-case validation message steers users into a sibling rejection.

1 inline comment(s).

2 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change adds a new Agent SDK harness adapter with per-call tool gating, a new 'hold' GateDecision path, journal persistence for gate decisions, and a slimmer production Docker image. The core design is sound and well tested, but three issues warrant attention before merge: the maker gate-hold path passes the wrong error binding to escalateToHold, the new adapter silently bypasses the documented named-agent hold-on-miss contract, and the hold reason can degrade to an empty string on an empty error message. Given the change's blast radius across execution, gating, and journaling, human review is recommended.

3 inline comment(s).

4 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change adds an Agent SDK harness adapter wired through the controller, worker, gate, and persistence layers, with substantial accompanying tests. Two things warrant attention before merge: the gate-hold path in the executor passes err to escalateToHold while the caught error in that scope is invokeErr, which either throws or attributes the hold to the wrong error; and a held card is journaled with a scrap-shaped outcome, which can misclassify it for consumers that read span outcomes. Smaller items: the gate-hold reason reaches persisted surfaces without the sanitization the journal writer applies, and a docs paragraph was inserted between a sentence and the code block it introduces.

1 inline comment(s).

3 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change adds an Agent SDK harness adapter with per-call gating and a new gate-hold escalation path in the executor, with substantial test coverage. Two real issues survive: the gate-hold escalation passes err where the catch binding is invokeErr, so the hold record can carry the wrong error (or fail to compile), and the journaled span outcome conflates a held card with a scrapped one. A minor diagnostics issue in the adapter's exit-code tracking is also worth a look; nothing here is merge-blocking.

2 inline comment(s).

2 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change introduces a new agent-sdk harness adapter with strong accompanying test coverage, plus gate/executor wiring and a Dockerfile tweak. One real defect survives: the maker-path hold escalation in executor.ts passes err where the caught error is invokeErr, so the hold can journal the wrong cause. A few lower-severity items (an abort-listener edge case in the adapter's spawn path, a potentially flaky wall-clock assertion, and untested env-option routing for the new adapter) are worth addressing; given the change's size and its reach into exec and persistence paths, human review is recommended before merge.

2 inline comment(s).

2 previously-acknowledged finding(s) not re-posted (resolved review threads).

[github-actions]

Nitpick review — comment

The change introduces a new agent-sdk harness adapter with per-call tool gating, a gate-hold decision path, and config wiring, with substantial accompanying tests. Three issues need attention before merge: the maker gate-hold path in executor.ts passes err where the catch-bound invokeErr is clearly intended, the Dockerfile now relies on an unpinned claude binary resolved from PATH at runtime with no install step in the image, and the new hold GateDecision branch makes the documented five-fold-site inventory (and possibly the mutation-check manifest) stale, with a risk that the hold path's critic usage is never folded into budgets. Given the wide blast radius of this change to the worker/executor core, human review is recommended.

2 inline comment(s).

4 previously-acknowledged finding(s) not re-posted (resolved review threads).