[feat] Gate Pi native builtins through the permission relay - #5066
Conversation
|
@coderabbitai review |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Pi builtin-gating design documentation and updates related agent-workflows pages to describe the new builtin permission relay flow, implementation phases, shipped status, and supporting reference material. ChangesPi Builtin Gating Documentation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: documentation, design Suggested reviewers: None specified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
✅ Action performedReview finished.
|
|
This is a design workspace awaiting review before any code. Three specific things I want feedback on:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/design/agent-workflows/projects/pi-builtin-gating/research.md (1)
107-115: 📐 Maintainability & Code Quality | 🔵 TrivialPin the Pi-doc citation to a stable source.
node_modules/@earendil-works/pi-coding-agent/docs/extensions.mdis a dependency-local path, so this research note will drift or disappear as the package changes. Please cite the canonical upstream docs or the exact version used here instead so the verification trail stays reproducible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f24a7948-bb16-4a2c-97cb-e432324a1ee7
📒 Files selected for processing (6)
docs/design/agent-workflows/projects/pi-builtin-gating/README.mddocs/design/agent-workflows/projects/pi-builtin-gating/context.mddocs/design/agent-workflows/projects/pi-builtin-gating/design.mddocs/design/agent-workflows/projects/pi-builtin-gating/plan.mddocs/design/agent-workflows/projects/pi-builtin-gating/research.mddocs/design/agent-workflows/projects/pi-builtin-gating/status.md
| ## The shape of the thing | ||
|
|
||
| Pi fires a `tool_call` event before it runs any tool, and the handler can block. We add that | ||
| handler to our extension. For each builtin call the handler does two checks in order: |
There was a problem hiding this comment.
If I understand correctly, we do already have an extension. For Pi, what are the current functionalities of our extension?
There was a problem hiding this comment.
Added a "What our extension does today" section right here. Today it does two things, neither about permissions: it registers the resolved custom/relay tools with pi.registerTool (each call round-trips to the runner through the relay directory), and it flushes tracing + usage on agent_end. This design adds the tool_call policy hook and the active-set edit to that same bundle.
| Pi fires a `tool_call` event before it runs any tool, and the handler can block. We add that | ||
| handler to our extension. For each builtin call the handler does two checks in order: | ||
|
|
||
| 1. **Grant.** Is this builtin one the author enabled? If not, the call never runs. |
There was a problem hiding this comment.
I don't feel that this is the right place to do this. We already have a configuration that specifies which tools to exist. This is actually a Pi-specific configuration. Let's keep it! If we want to add something, let's add policy, but then, if it's not enabled, why does Pi provide us with a way to do this? Let's not duplicate it!
There was a problem hiding this comment.
Agreed and folded: the hook now does policy only, and the grant check is gone from it. No new configuration exists anywhere in the design; the author's existing builtin selection (config -> wire tools field) stays the single source. The only thing this slice adds is enforcement of that existing config, which today has no reader at all (the last one died in 0e71bd0).
| handler to our extension. For each builtin call the handler does two checks in order: | ||
|
|
||
| 1. **Grant.** Is this builtin one the author enabled? If not, the call never runs. | ||
| 2. **Policy.** For a granted builtin, ask the runner: allow, deny, or ask? The runner |
There was a problem hiding this comment.
Sounds good. Let's keep the nomenclature similar to what we have.
There was a problem hiding this comment.
Done. The response record now transports decide()'s verdict verbatim (verdict: allow | deny | pendingApproval) instead of the invented effect: allow | block. The mapping to Pi's { block: true } mechanics lives in the extension hook at the Pi boundary.
|
|
||
| ## The grant list: shape Pi's active-tool set at turn start | ||
|
|
||
| The cleanest way to enforce "which builtins exist" is to not offer the non-granted ones at |
There was a problem hiding this comment.
So that's the current solution, which we specified the configuration for Pi already, like in the quarks. The cloud setting does not work, and then, if so, please explain why not. I don't think we need to do that, or at least we don't need to do this in the extension level.
There was a problem hiding this comment.
Verified why the Claude-settings-style route does not work for Pi, with evidence now in research.md ("Native tool-selection surfaces"): Pi's settings schema has NO tool field of any kind, so there is nothing to write into the per-run agent dir. Pi does take a grant list natively, but only at process launch (--tools flag, or the SDK's createAgentSession({tools}) that our deleted in-process engine used), and every layer of our path drops it: sandbox-agent's SessionCreateRequest has no tools field, ACP session/new carries only cwd + mcpServers, and pi-acp hard-codes pi's argv. So the extension is the only enforcement point we can reach today. The durable fix is upstream (a tools field on SessionCreateRequest forwarded by pi-acp into pi's argv) and is filed as a follow-up; when it lands, the extension edit disappears and the same config flows declaratively.
a7f33fa to
4d5b47e
Compare
| ## The shape of the thing | ||
|
|
||
| Pi fires a `tool_call` event before it runs any tool, and the handler can block. We add that | ||
| handler to our extension, and it does one job: the per-call policy check. For each builtin |
There was a problem hiding this comment.
REVIEW GUIDE This is the round-1 change: the tool_call hook now does per-call policy only. It no longer re-checks the grant list at call time. Check this matches your comment: a non-granted builtin should simply not be in Pi's active tool set, so no call for it ever fires and there is nothing to double-check here.
| (`--mode rpc --no-themes`, pi-acp index.js:134). Every layer between the runner and the flag | ||
| drops it. | ||
|
|
||
| That leaves one enforcement point we control today: our extension, which runs inside the |
There was a problem hiding this comment.
REVIEW GUIDE This is the crux for your review: no new config is added anywhere. The author's existing builtin selection (the tools wire field) stays the single source of truth. Pi's native surfaces are all unreachable on our path: the settings file has no tools field, and the --tools flag and SDK tools option get dropped by sandbox-agent's SessionCreateRequest, the ACP session/new schema, and pi-acp's hard-coded argv (evidence in research.md's new "Native tool-selection surfaces" section). setActiveTools inside our own extension is the only enforcement point we control today; the upstream fix is filed as a follow-up. Worth challenging whether that chain is really unreachable end to end, or whether there is a surface the research missed.
There was a problem hiding this comment.
Are you sure this is the only way? I would expect using folders, like we do in setup things. Would be also another way to do it other than ACP.
There was a problem hiding this comment.
But if that doesn't work, I think the solution of an extension makes sense, because this means that this same configuration, if used in Py, like the configuration that you have, using the other ways without ACP natively would work. Which is good enough, because that's one of the things we want to have a portqability for our configuration.
There was a problem hiding this comment.
Both folded into design.md as a new subsection ("The folder is the delivery vehicle either way", commit 55bf9e1). You are right that folders are the mechanism, and in fact they already are here: the extension ships inside the per-run agent dir, exactly like skills and prompts. The folder route and the extension route are the same route. The only thing Pi lacks is a declarative settings field for tools, so the folder carries a small piece of code instead of a config line; if Pi grows that field, the folder carries config and nothing else changes.
Your portability point is now a stated design goal: any Pi process that loads the agent dir enforces the same grant, including native pi use outside our ACP path. The two non-ACP alternatives fail exactly that test and are recorded as rejected: a pi binary shim in the sandbox image (reaches the native --tools flag today, but couples enforcement to our images and does nothing for native use), and waiting for the upstream passthrough (leaves the grant dead meanwhile, and only ever helps runs that go through sandbox-agent).
| | --- | --- | --- | | ||
| | `kind: "permission"` | protocol / routing | Marks this as a permission response so no other reader parses it as a tool result. | | ||
| | `ok` | protocol | False on a runner-side error; the handler then fails closed per the failure-mode table below. | | ||
| | `verdict: "allow" \| "deny" \| "pendingApproval"` | control | The decision module's own verdict, transported verbatim. The hook maps anything but `allow` to Pi's `{ block: true }`. Named after `decide()`'s vocabulary so the wire and the module read the same. | |
There was a problem hiding this comment.
REVIEW GUIDE This is the nomenclature fix from your comment. The verdict field now transports decide()'s own vocabulary verbatim (allow/deny/pendingApproval) instead of an invented effect field. The mapping from pendingApproval to Pi's { block: true } happens in the extension hook at the Pi boundary, not on the wire. Check this reads as one shared vocabulary across the runner and the wire, as you asked.
| paused tool call, emits the `interaction_request` event the playground renders as | ||
| Approve/Deny, records the durable interaction, and pauses the turn. It also writes the | ||
| response `{ kind: "permission", verdict: "pendingApproval", reason: "Waiting for approval of bash." }`. | ||
| 5. The hook reads the block and returns `{ block: true, reason }`. Pi does not run the shell |
There was a problem hiding this comment.
REVIEW GUIDE This is the pending-approval mechanic to sanity check. A pending verdict does two things at once: the runner pauses the turn through onPendingApproval, and the hook independently blocks the native Pi call so the shell command does not run while waiting. Resume then depends on Pi re-issuing the call after the block, which the doc calls out as the top open risk. Check this dual-effect design against the pause bugs you have seen before (the F-024 class of issue).
|
|
||
| ## Failure modes | ||
|
|
||
| **The re-issue risk (top risk).** Step 7 assumes Pi re-issues the `bash` call after the block |
There was a problem hiding this comment.
REVIEW GUIDE This is the single biggest risk in the whole design. Pi turns a hook block into an error tool result, so whether the model re-issues the call after approval is pure model behavior, unverified today. Plan.md's Phase 0 spikes this against the live runner pause and teardown before any code gets built. If Pi does not reliably re-issue, the doc says stop and come back with a different design (holding the turn open instead of blocking). Check you agree Phase 0 should be a hard gate, not a formality.
| - **SDK: exists.** `CreateAgentSessionOptions.tools?: string[]` (dist/core/sdk.d.ts:11-46). | ||
| The deleted in-process engine passed exactly this (`toolAllowlist` in the old runPi.ts, | ||
| removed in 0e71bd0f7a). | ||
| - **The chain that drops it:** the runner calls `sandbox.createSession({ agent, cwd, |
There was a problem hiding this comment.
REVIEW GUIDE This section answers your question about why the grant is not just written into Pi's config the way Claude gets .claude/settings.json. The evidence chain: Pi's settings file has no tools field at all, and its native --tools flag and SDK tools option exist but are dropped at every hop between the runner and the spawned pi process (sandbox-agent's SessionCreateRequest, the ACP session/new schema, and pi-acp's hard-coded argv all lack a tools passthrough). Check the file:line citations against the actual dependencies, since this is the load-bearing justification for the whole design.
| same arguments (so stored-decision matching would hit). | ||
| - Try two reason texts: a neutral one and one that explicitly says to retry after approval. | ||
| Record which produces a reliable re-issue. | ||
| - Then repeat the spike through the real runner pause and teardown, not just a throwaway local |
There was a problem hiding this comment.
REVIEW GUIDE This is the real Phase 0 spike, not a shortcut. It must observe the actual resumed transcript through the real runner pause and teardown, not just a throwaway local block, because Pi turns a block into an error tool result and re-issue is a pure model-behavior question. Check that this is scheduled and gated before Phase 1 starts, since Phase 1's code assumes re-issue works.
| seven, drop-and-log unknowns). Codex also confirmed the pending-and-block double effect is | ||
| sound because `toolRelay.stop()` drains inflight handlers before teardown. | ||
|
|
||
| ## Review round 1 (Mahmoud, 2026-07-04) |
There was a problem hiding this comment.
REVIEW GUIDE This is the decision log for your four review-round-1 comments. Each bullet maps to a design.md change: the hook now does policy only, no new config was added (Pi's native surfaces are unreachable, evidenced in research.md), the response nomenclature now matches decide()'s own verdict vocabulary, and the doc opens with what the extension already does today. Check each bullet matches your intent before approving.
55bf9e1 to
0f55183
Compare
|
Codex xhigh re-reviewed the post-round-2 workspace: verdict was "fix first" with 5 findings, all folded in 0f55183. The consistency misses (stale grant-check and block-ownership wording deeper in the docs) are cleaned up everywhere. Three technical points came from Codex reading Pi's shipped source and are now in the design:
Phase 0 also has a hard pass bar now: 5/5 approve runs, the builtin executes exactly once and only after approval, canonical args match, transcripts recorded. Anything less fails and the design comes back to you. Ready for your final review. |
- the tool_call hook does policy only; the grant never re-checks at call time - grant list: single existing config; documented why Pi's native surfaces (settings file absent; --tools flag and SDK param dropped by the ACP chain) are unreachable, extension setActiveTools as the only current enforcement point, upstream sandbox-agent/pi-acp passthrough filed as the follow-up - permission response transports decide()'s verdict verbatim (allow/deny/ pendingApproval) instead of the invented effect: allow|block - added what-the-extension-does-today context
…ehicle either way The extension ships in the per-run agent dir like skills do, so the folder route and the extension route are the same route; Pi only lacks a declarative settings field. Portability recorded as a goal: the same agent dir enforces the same grant under native pi use outside ACP. Shim and wait-for-upstream alternatives considered and rejected.
dosubot
Bot
added
size:XXL
|
@coderabbitai review |
✅ Action performedReview finished.
|
| rules: { pattern: string; permission: ToolPermission }[]; | ||
| } | ||
|
|
||
| export const PI_BUILTIN_TOOL_IDENTITY = { |
There was a problem hiding this comment.
REVIEW GUIDE
This table is the single owner of Pi builtin identity. It maps each builtin's lowercase Pi name to a canonical rule name (so Bash(...) prefix rules match Pi's lowercase bash tool), the read-only bit allow_reads consults, and it feeds the bash-args match projection born from the Phase 0 spike (a model added a spontaneous timeout param that would otherwise have broken prefix matching). Check that the projection in projectBashStoredDecisionArgs applies only to stored-decision matching (storedDecisionKeyShape) and never influences the actual permission decision in decide/effectivePermission.
| return PI_BUILTIN_CANONICAL_NAMES.get(toolName); | ||
| } | ||
|
|
||
| export function storedDecisionKeyShape( |
There was a problem hiding this comment.
REVIEW GUIDE
storedDecisionKeyShape is the one helper both sides of the resume boundary use to derive the decision key: the gate path (deciding whether a call needs approval) and the transcript-extraction path in responder.ts (matching a resumed approval back to its pending call). Routing both through this single function means they can never disagree on what identifies a call. Check that no other call site in the runner builds a decision key (toolName + args shape) directly instead of going through storedDecisionKeyShape.
| return typeof value === "object" && value !== null && !Array.isArray(value); | ||
| } | ||
|
|
||
| async function handlePermissionRelayRequest( |
There was a problem hiding this comment.
REVIEW GUIDE
handlePermissionRelayRequest writes decide()'s verdict verbatim on every branch, including pending approval; the Pi extension maps any non-allow verdict to a block. It fails closed on a protocol mismatch and on an unknown builtin name, and both cases log loudly before denying. Check that a response is written on every code path here: the extension hook polls with a 60s timeout, so a request that falls through without a written response would hang the tool call instead of failing fast.
| _RECORDINGS = Path(__file__).parent / "recordings" | ||
|
|
||
|
|
||
| async def test_batch_paused_run_replays_gated_bash_builtin(monkeypatch, fake_backend): |
There was a problem hiding this comment.
REVIEW GUIDE
This is the replay pin: it feeds a redacted real capture from a live-QA run (recordings/pi-gated-bash-pause.json) through the actual event-stream-to-fold path with no live LLM involved. It exists because the grant list regressed silently once before, in 0e71bd0f7a, with nothing catching it. Check that the fixture stays faithful to the live capture; the recording file documents its own provenance (date, harness config, exact prompt) so a future reader can tell whether it still represents a real run.
|
Implementation is complete per the reviewed design (docs already carry 8 REVIEW GUIDE comments from that pass; this PR now adds the code, and I've posted 7 new inline guides across QA status:
Three decisions are worth a second look during review:
Let me know which of these you want to dig into first, or if the inline guides raise anything that needs a change before merge. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
services/runner/tests/unit/tool-relay-permission-record.test.ts (1)
86-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared relay-test scaffolding.
relayRecordOncehere is near-identical torelayUntilPendingintool-relay-permission-parity.test.tsandrelayOnceintool-relay-permission.test.ts/tool-direct.test.ts(temp-dir setup, write request, poll for response file, stop relay, cleanup). Four copies of this pattern now exist across the new/touched test files; extracting a shared test helper module would reduce future drift when the relay's request/response contract changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 083c4ce7-e2ef-46ae-886d-9755413b4121
📒 Files selected for processing (30)
docs/design/agent-workflows/documentation/protocol.mddocs/design/agent-workflows/documentation/tools.mddocs/design/agent-workflows/interfaces/in-service/permission-responder.mddocs/design/agent-workflows/projects/approval-boundary/status.mddocs/design/agent-workflows/projects/pi-builtin-gating/README.mddocs/design/agent-workflows/projects/pi-builtin-gating/build-notes.mddocs/design/agent-workflows/projects/pi-builtin-gating/context.mddocs/design/agent-workflows/projects/pi-builtin-gating/design.mddocs/design/agent-workflows/projects/pi-builtin-gating/plan.mddocs/design/agent-workflows/projects/pi-builtin-gating/research.mddocs/design/agent-workflows/projects/pi-builtin-gating/status.mdservices/oss/tests/pytest/unit/agent/recordings/pi-gated-bash-pause.jsonservices/oss/tests/pytest/unit/agent/test_invoke_handler.pyservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/pi-assets.tsservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/src/extensions/agenta.tsservices/runner/src/permission-plan.tsservices/runner/src/responder.tsservices/runner/src/tools/dispatch.tsservices/runner/src/tools/relay.tsservices/runner/tests/unit/extension-tools.test.tsservices/runner/tests/unit/permission-record-fixture.test.tsservices/runner/tests/unit/sandbox-agent-pi-assets.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.tsservices/runner/tests/unit/tool-direct.test.tsservices/runner/tests/unit/tool-dispatch-permission.test.tsservices/runner/tests/unit/tool-relay-permission-parity.test.tsservices/runner/tests/unit/tool-relay-permission-record.test.tsservices/runner/tests/unit/tool-relay-permission.test.ts
✅ Files skipped from review due to trivial changes (2)
- docs/design/agent-workflows/projects/pi-builtin-gating/build-notes.md
- services/oss/tests/pytest/unit/agent/recordings/pi-gated-bash-pause.json
| runner executes directly (gateway, code) and, now, on Pi's own builtins, relayed through the | ||
| bundled extension's `tool_call` hook. It only needs to, because on Claude the harness settings | ||
| file plus the ACP responder already decide before a call reaches the relay. On Pi there is no | ||
| separate harness-side settings gate, so the relay is the enforcement point for everything Pi | ||
| runs, including its native builtins, and it gives Pi the same human-in-the-loop behavior | ||
| Claude gets. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm current status of code tools in run-plan.ts / code.ts
rg -n "CODE_TOOL_UNSUPPORTED_MESSAGE|hasCodeTool" services/runner/src -A3Repository: Agenta-AI/agenta
Length of output: 3359
Drop the code-tool relay path from this doc
These passages still describe the relay as enforcing code tool execution, but buildRunPlan rejects code tools up front and runCodeTool is only a backstop. Remove the code references here and in the later duplicate passage.
There was a problem hiding this comment.
Fixed in 189ae48: tools.md no longer implies the relay executes code tools today (they are declared but unsupported in the runner; CODE_TOOL_UNSUPPORTED_MESSAGE).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "code" docs/design/agent-workflows/documentation/tools.md -i | grep -i "relay"
echo "---"
rg -n "The tool relay" -A6 docs/design/agent-workflows/documentation/tools.md
echo "---status section---"
rg -n "Code tools" -A4 docs/design/agent-workflows/documentation/tools.mdLength of output: 2189
🏁 Script executed:
#!/bin/bash
rg -n "CODE_TOOL_UNSUPPORTED_MESSAGE|unsupported" docs/design/agent-workflows/documentation/tools.md -i
echo "---context 330-345---"
sed -n '330,345p' docs/design/agent-workflows/documentation/tools.md
echo "---context 525-545---"
sed -n '525,545p' docs/design/agent-workflows/documentation/tools.md
echo "---git log---"
git log --oneline -5 -- docs/design/agent-workflows/documentation/tools.mdLength of output: 3804
@mmabrouk Thanks for the fix — the builtin-gating paragraph (around Line 335) now correctly notes that code tools are "declared but not yet executable in the runner." However, the other two spots I flagged still carry the same implication and weren't updated:
- Line 389 (under "Two gates consult this same decision" → "The tool relay" bullet): "enforces permission on tools the runner executes directly (gateway, code)" — still lists
codeas something the relay directly executes today. - Line 537 (under "Status and known gaps"): "the relay decides everything Pi runs: gateway and code tools directly, and builtins relayed through the extension's
tool_callhook" — same issue.
Both of these still read as if code tools are actively executed/enforced by the relay path, which contradicts the fact that buildRunPlan rejects code tools up front and runCodeTool is unsupported (CODE_TOOL_UNSUPPORTED_MESSAGE). Could you drop the code mention from these two remaining spots as well, to be consistent with the fix already applied at Line 335?
mmabrouk
added
lgtm
- tools.md: code tools are declared but not yet executable in the runner; stop implying the relay executes them today - status.md: state -> merged; the reverse stale-bundle skew upgraded from hardening candidate to next slice (handshake record at before_agent_start, runner errors the turn when absent; prod builds bundle+runner atomically so the skew is a dev-mount artifact)
1 participant
Context
Pi's seven native builtins (
read,bash,edit,write,grep,find,ls) ran outside the permission system. An author could not say "ask beforebash": the builtins never crossed a gate, so authored rules on them were silently ignored. And the builtin selection in the playground did nothing, because the wire'stoolsfield lost its only reader on June 24 (0e71bd0f7aremoved the old in-process engine). Every builtin was always on, whatever the author chose.This PR carries the design workspace (docs/design/agent-workflows/projects/pi-builtin-gating/, reviewed in two rounds by Mahmoud and two Codex passes) plus the implementation.
Changes
The runner decides; the extension only reports and enforces:
tool_callhook. For each builtin call it writes a{kind: "permission", protocol: 1, toolName, toolCallId, args}record into the existing relay directory and waits. The runner's relay loop resolves it through the same shareddecide()every other gate uses, with the real name and real arguments, and writes the verdict back verbatim (allow | deny | pendingApproval). The extension maps anything butallowto Pi's{block: true}.askpauses the turn through the exact pause machinery relay tools already use; approve resumes and the model re-issues the call.before_agent_startwith the author's selection, leaving non-builtin tools untouched. A non-granted builtin never appears to the model. (Pi has no settings-file field for tools, and its native--toolsflag is dropped by every hop of the sandbox-agent/ACP chain, so the extension is the only reachable enforcement point; the upstream passthrough is a filed follow-up.)permission-plan.ts): canonical rule name (Bashfor Pi'sbash, so authoredBash(git:*)rules match), the read-only bitallow_readsconsults, and the match projection for stored approvals (bashmatches oncommandalone, because models sometimes add a benigntimeout;edit/writekeep full args so content changes never auto-match).tools: undefinedvs[]distinguished). All-allow runs with default grants skip gating entirely; gating-only runs start the relay with zero custom tools.protocolis denied loudly. The silent direction (new runner, stale extension bundle) is covered by a build guard and hit us live once during QA; a fail-loud hardening candidate is filed.Before:
bashran under adenypolicy, and deselecting builtins changed nothing. After:bash: askpauses for approval on Pi exactly like a relay tool, and a builtin the author did not grant does not exist for the model.Scope / risk
toolsselects only some builtins now actually loses the others (that is the point, but existing configs will feel it)./runwire and goldens are unchanged.Tests
0e71bd0f7aregression pin), typecheck clean.bash: ask(one prompt, never pre-approval, exactly once after, stable args), deny, read-free underallow_reads, grant enforcement (model does not see ungranted builtins), deny-all, the all-allow fast path, the headless paused envelope (stop_reason: "paused",pending_interaction.tool: "bash"), and a custom-tool regression with an unguessable-token proof.How to QA
Prerequisites: EE dev stack; restart the runner sidecar after checkout AND ensure it serves the freshly built extension bundle (
pnpm run build:extension; the dev sidecar mountssrc/only, so copy or mountdist/— a stale bundle silently disables gating).Steps:
echo qa-1234using the bash tool."Expected: step 2 shows exactly one Approve/Deny prompt that stays; step 3 approve resumes and runs the command exactly once (deny refuses cleanly, no error card); step 4 shows no bash in the model's tools and no execution; step 5 runs immediately with no prompt.
Automated tests:
cd services/runner && pnpm test.Edge cases: two gated builtins in one assistant message (one prompt, the second call is refused this turn with the another-approval-pending reason); a
Bash(git:*)rule with agit statuscommand (matches despite Pi's lowercase name); an approval followed by the model adding atimeoutparam on re-issue (still matches: the bash projection).Reading order for review
Workspace README, then design.md (the record protocol, the identity table, the folder-portability section), then status.md (decision log, live-QA results, follow-ups) and build-notes.md (how the build went, including the version-skew catch). Inline REVIEW GUIDE comments mark the load-bearing code.
https://claude.ai/code/session_01DGj7GKafjkZeQXMsryWhb2