-
Notifications
You must be signed in to change notification settings - Fork 600
[feat] Gate Pi native builtins through the permission relay #5066
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
219be14
docs(agent-workflows): pi-builtin-gating design workspace (relay-gate…
mmabrouk a2f494a
docs(pi-builtin-gating): address review round 1
mmabrouk c516951
docs(pi-builtin-gating): review round 2: the folder is the delivery v…
mmabrouk 848a17d
docs(pi-builtin-gating): fold the Codex round-2 re-review (7 fixes)
mmabrouk 2eb843f
docs(pi-builtin-gating): Phase 0 stage-A spike PASSED; add the match …
mmabrouk 8512074
feat(runner): relay permission record + builtin identity table (pi-ga…
mmabrouk 6e8fdb0
feat(runner): extension builtin gating + grant enforcement + env plum…
mmabrouk d845a1f
docs: pi-builtin-gating implemented + live-QA'd; sync the documentati…
mmabrouk 3a56656
test(service): replay pin for the gated-bash paused envelope
mmabrouk a5c6e46
feat(runner): land the shared-file hunks (pi-gating phases 1-4 comple…
mmabrouk 3502fc5
feat(runner): land the shared-file gating hunks (phases 1-4 completion)
mmabrouk 8d1b836
test(runner): run-plan gating cases + relay permission test alignment…
mmabrouk 189ae48
docs: CodeRabbit round on the implementation PR
mmabrouk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
53 changes: 53 additions & 0 deletions
53
docs/design/agent-workflows/projects/pi-builtin-gating/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # Pi builtin gating: route native tools through the permission relay | ||
|
|
||
| Pi ships seven native builtin tools (`read`, `bash`, `edit`, `write`, `grep`, `find`, | ||
| `ls`). Before this work they ran outside our permission system. The author could not say | ||
| "ask before `bash`", and the "which builtins are enabled" list the UI shows did nothing. | ||
| This workspace explains why, picks a design, and lays out the build. | ||
|
|
||
| This PR now ships BOTH the workspace and the implementation: the runner-side permission | ||
| record and builtin identity table, the extension's policy hook and grant enforcement, the | ||
| env plumbing, and the test suite. status.md tracks the live state (implemented, | ||
| live-QA'd, S1 5/5 bar met); build-notes.md records how the build actually went. The two | ||
| gaps below are written in the present tense of the design phase; they are both closed. | ||
|
|
||
| ## The two gaps | ||
|
|
||
| 1. **`bash: ask` is inexpressible.** Our permission plan supports `allow | ask | deny` per | ||
| tool and four global modes. Custom tools and Claude's own tools honor it. Pi's seven | ||
| builtins never reach a gate, so any authored rule on them is silently ignored. An agent | ||
| that should pause before running a shell command just runs it. | ||
|
|
||
| 2. **The builtin grant list is dead.** The run request carries `tools?: string[]` | ||
| ("Built-in tools to enable", `services/runner/src/protocol.ts:423`). The SDK still emits | ||
| it and the playground still writes the author's selection into it. Nothing in the runner | ||
| reads it. It went dead on 2026-06-24 in commit `0e71bd0f7a`, which deleted the old | ||
| in-process engine that was its only consumer. So every builtin is always on, whatever the | ||
| author selected. | ||
|
|
||
| ## The design in three sentences | ||
|
|
||
| Our Pi extension gains a `tool_call` hook. Pi fires it before every builtin runs, and the | ||
| hook can block. For a granted builtin, the hook asks the runner for a decision through the | ||
| existing relay-directory file protocol, the runner decides with the real tool name and real | ||
| arguments in the one shared decision module (`decide()` in | ||
| `services/runner/src/permission-plan.ts`), and the hook blocks or allows on the answer. | ||
|
|
||
| ## Reading order | ||
|
|
||
| 1. [context.md](context.md): why this matters and how it ties to the approval-boundary work. | ||
| 2. [research.md](research.md): the verified facts about the code this builds on, with | ||
| file and line references. | ||
| 3. [design.md](design.md): the relay record shape, the decision mapping, the env config, | ||
| grant-list enforcement, and the pause/resume flow and its failure modes. | ||
| 4. [plan.md](plan.md): the phased build, tests, and the live spike that de-risks the design. | ||
| 5. [status.md](status.md): current state, the decision log (Option B over A and C), and | ||
| open risks. | ||
|
|
||
| ## The decisions to weigh in on | ||
|
|
||
| - The shape of the new permission record on the relay protocol (design.md). A Codex round | ||
| already settled the record into a discriminated union and the gate onto the existing | ||
| `"harness"` executor; the remaining call is whether that shape reads right to you. | ||
| - How to mitigate the re-issue risk if Pi treats a blocked call as terminal (design.md, | ||
| plan.md Phase 0). This is the top open risk and the reason Phase 0 is a spike, not code. |
27 changes: 27 additions & 0 deletions
27
docs/design/agent-workflows/projects/pi-builtin-gating/build-notes.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
|
|
||
| ## 2026-07-04 implementation run | ||
|
|
||
| - **Phase 0 stage-A spike PASSED** (see status.md): cross-turn re-issue 3/3 with identical | ||
| args; the match projection was born from the one drift case (a spontaneous `timeout` | ||
| param on bash). | ||
| - **Phases 1-4 implemented** (Codex xhigh implemented, orchestrator reviewed each diff; | ||
| Sonnet wrote the Phase 4 cross-cutting tests). Runner suite grew 444 -> 525, tsc clean, | ||
| extension bundle rebuilt. Key review confirmations: `approvedCallKey` routes through the | ||
| shared `storedDecisionKeyShape`, so the gate side and the transcript-extraction side can | ||
| never disagree on the resume key; the relay permission branch writes verdicts verbatim on | ||
| every path; the run-plan predicate honors the kill switch and the `undefined` vs `[]` | ||
| grant distinction. | ||
| - **Live QA immediately caught the design's own predicted failure class.** First S1 run | ||
| showed zero gating. Not stale runner code: the dev sidecar mounts only `src/` from the | ||
| worktree, so the runner computed gating correctly while the Pi extension installed into | ||
| each sandbox came from the image's BAKED `dist/` bundle (dated July 1, no hook). New | ||
| runner + old extension is the silent direction of the version skew: the old bundle never | ||
| writes a permission record, so the `protocol: 1` pin has nothing to reject. Fixed on the | ||
| box by copying the fresh bundle into the container (survives restart, not recreation). | ||
| Durable fixes noted: mount or build `dist/` in the sidecar recipe, and a possible | ||
| runner-side hardening (fail loud when a gating-active Pi turn ends with zero permission | ||
| records for a policy that must gate) filed as a follow-up candidate. | ||
| - **Concurrency note:** implemented alongside mcp-mvp-claude's live client-tools work in | ||
| the same worktree. Six files' hunks (relay.ts, responder.ts, agenta.ts, sandbox_agent.ts, | ||
| two tests) are frozen worktree-only until their lane merges; the lane is deliberately | ||
| unpushed while its tip would not compile standalone. Board rows carry the handshake. |
70 changes: 70 additions & 0 deletions
70
docs/design/agent-workflows/projects/pi-builtin-gating/context.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| # Context: why gate Pi's builtins | ||
|
|
||
| ## What a builtin is | ||
|
|
||
| Pi runs an agent loop with seven tools built into the harness: `read`, `bash`, `edit`, | ||
| `write`, `grep`, `find`, and `ls`. The model calls them the same way it calls a custom tool. | ||
| The difference is where they execute. A custom tool runs through our code. A builtin runs | ||
| inside Pi itself, and Pi never asks us for permission first. | ||
|
|
||
| ## Gap 1: the author cannot gate a builtin | ||
|
|
||
| Our permission model is small and clear. An author sets one global default and optional | ||
| per-tool rules. Each tool resolves to `allow`, `ask`, or `deny`. The four global modes are | ||
| `allow`, `ask`, `deny`, and `allow_reads` (reads run, everything else asks). This model is | ||
| described in the approval-boundary workspace at | ||
| [projects/approval-boundary/how-approvals-work.md](../approval-boundary/how-approvals-work.md). | ||
|
|
||
| The model works for every tool that reaches a gate. Claude's tools reach a gate because | ||
| Claude asks over its own protocol. Our custom tools reach a gate because we execute them | ||
| and check first. Pi's builtins reach no gate at all. So `bash: ask` does nothing on Pi. The | ||
| shell command runs. This is the sharpest gap, because `bash` is the one builtin most authors | ||
| would want to pause on. | ||
|
|
||
| ## Gap 2: the grant list stopped working | ||
|
|
||
| The run request has a field for the enabled builtins: `tools?: string[]` at | ||
| `services/runner/src/protocol.ts:423`, commented "Built-in tools to enable". The SDK fills | ||
| it from the author's selection (`PiAgentTemplate.wire_tools` in | ||
| `sdks/python/agenta/sdk/agents/dtos.py:829` emits `"tools": list(self.builtin_names)`), and | ||
| the playground's Pi settings control writes that selection. | ||
|
|
||
| The runner ignores the field. It went dead in commit `0e71bd0f7a` (2026-06-24, "remove | ||
| legacy in-process backend"). That commit deleted the old in-process engine, and that engine | ||
| held the only code that read the list. So the selection an author makes in the UI has no | ||
| effect. Every builtin is always available. The commit message is honest that it removed the | ||
| in-process path as dead code; the grant list was collateral, and no one has re-wired it since. | ||
|
|
||
| ## Why the two gaps share one fix | ||
|
|
||
| Both gaps are the same missing thing: a point where the runner sees a builtin call before it | ||
| runs and gets to decide. Add that one interception point and both gaps close. A non-granted | ||
| builtin gets refused. A granted builtin gets the author's `allow | ask | deny`. | ||
|
|
||
| ## Tie to the approval-boundary redesign | ||
|
|
||
| The approval-boundary workspace ([projects/approval-boundary/](../approval-boundary/)) | ||
| established a doctrine we follow here: **one decision module.** The rule about whether a tool | ||
| runs, pauses, or is denied lives in exactly one place, `decide()` in | ||
| `services/runner/src/permission-plan.ts`, and every gate calls it. That workspace's plan | ||
| ([projects/approval-boundary/plan.md](../approval-boundary/plan.md)) lists the gates: the SDK | ||
| settings renderer for Claude, the ACP responder for Claude's live gates, and the relay for | ||
| tools the runner executes itself. | ||
|
|
||
| Builtin gating adds a fourth caller of the same `decide()`. It does not add a second policy | ||
| engine. The whole point of the design below is that the runner decides builtins with the same | ||
| function, the same `GateDescriptor`, and the same stored-decision resume machinery that | ||
| custom-tool asks already use. The extension only reports the call and enforces the answer. | ||
| This keeps the doctrine intact: the sandbox never decides policy; it asks the runner. | ||
|
|
||
| ## Non-goals | ||
|
|
||
| - No new policy language. Authors express builtin rules with the existing `allow | ask | | ||
| deny` and the four global modes. | ||
| - No per-builtin permission field on the config yet. The SDK currently drops a per-builtin | ||
| permission loudly (`BuiltinToolConfig._drop_unenforceable_permission` in | ||
| `sdks/python/agenta/sdk/agents/tools/models.py:87`) because nothing enforced it. This | ||
| design makes enforcement possible, so re-enabling that field becomes a small follow-up, but | ||
| it is out of scope here. Authors still gate builtins through global-mode rules and pattern | ||
| rules (for example `bash(git:*)`). | ||
| - No change to Claude. Claude already gates its own tools. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: Agenta-AI/agenta
Length of output: 3359
Drop the
code-tool relay path from this docThese passages still describe the relay as enforcing
codetool execution, butbuildRunPlanrejects code tools up front andrunCodeToolis only a backstop. Remove thecodereferences here and in the later duplicate passage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 2189
🏁 Script executed:
Length of output: 3804
@mmabroukThanks 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:codeas something the relay directly executes today.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
buildRunPlanrejects code tools up front andrunCodeToolis unsupported (CODE_TOOL_UNSUPPORTED_MESSAGE). Could you drop thecodemention from these two remaining spots as well, to be consistent with the fix already applied at Line 335?