Skip to content

[#5153] feat(runner): park Pi approval gates over the ACP permission plane - #5185

Merged
jp-agenta merged 3 commits into
big-agentsfrom
feat/pi-approval-parking
Jul 10, 2026
Merged

[#5153] feat(runner): park Pi approval gates over the ACP permission plane#5185
jp-agenta merged 3 commits into
big-agentsfrom
feat/pi-approval-parking

Conversation

@mmabrouk

@mmabrouk mmabrouk commented Jul 9, 2026

Copy link
Copy Markdown
Member

Context

A Pi approval destroys the session the moment the gate fires. When a human clicks Approve, a fresh Pi session cold-replays a flattened text transcript and the model re-issues the tool call from that text as a NEW tool_use. The re-issued call's arguments can drift from the ones the human actually approved, which is where both production Pi approval failures came from (argument drift, task restart). Claude's ACP gate already avoids this: keep-alive slice 2 (#5158) parks the live session with the permission request held open and answers it on the click, so the original call runs with its original arguments. Pi had no such path.

This makes both Pi gates (the builtin gate and the custom-tool gate) parkable. Within the keep-alive approval TTL, a human's answer resumes the exact original tool call on the live session (the byte-exact tier). Outside it, everything degrades to today's cold decision-map path, unchanged.

The mechanism is the proven Option C of the parkable-gates design: the gate rides ctx.ui.confirm(title, message) from the in-sandbox Pi extension, where message is a JSON envelope carrying the real gate identity. The pi-acp bridge turns that dialog into a real ACP session/request_permission, which lands at exactly the responder seam that already holds, parks, and answers Claude gates. The relay keeps carrying tool execution and results. Only the approval wait moves.

Changes

The dialog gate is the unconditional Pi behavior: no feature flag. The relay's old permission plane (the kind: "permission" record protocol and its polling) is deleted in the same change; the dialog is where the human decision happens, and the relay carries tool execution and results. Because the relay dir is sandbox-writable, the relay does NOT trust that a record passed the dialog: an execution guard re-checks every record runner-side (see the review round below). Envelope detection on the runner is scoped to Pi runs structurally (the permission responder receives the resolved-specs map only on Pi runs, and its presence turns detection on), so Claude runs never enter it, including on a title collision.

The envelope (one new sandbox-internal contract). New pi-gate-envelope.ts, shared by the runner and the esbuild-bundled extension so build and parse stay one source of truth:

{ "v": 1, "kind": "agenta.gate", "gate": "pi-builtin" | "pi-custom-tool",
  "toolName": "...", "toolCallId": "...", "input": { } }

The bridge natively forwards only the dialog strings with a synthetic pi-ui-<uuid> tool-call id, so the real identity rides message. Parsing is strict and version-checked. A request whose title matches but whose envelope does not parse fails closed (reject), never falls through: under a default-allow plan a fallthrough would confirm an unapproved execution.

Slice 1 (runner classification, acp-interactions.ts). On an envelope hit, the synthetic tool-call id is normalized to the envelope's real id at the top of handleRequest, before the gate descriptor, the pause bookkeeping, the emitted approval card, and the park record read it. The gate descriptor is built from the envelope identity, and permission metadata (specPermission, readOnlyHint) is recovered runner-side by spec lookup (piBuiltinIdentity for builtins), so an author-allow tool stays instant-allow, an author-deny tool stays instant-deny, and a read-only builtin auto-allows. The card's rawInput is set to the real args so it renders like a relay-gate card, with context-bound paths redacted (see the review round below).

Slice 2 (extension switch, agenta.ts + dead code removal). Both gates await ctx.ui.confirm(PI_GATE_DIALOG_TITLE, envelope) instead of polling the relay. No dialog opts are passed, so Pi arms no reaper and the wait parks indefinitely; any cancellation resolves it to a fail-closed block. Arguments are validated before the dialog, so a malformed call errors to the model instead of prompting a human. The custom-tool gate is scoped to non-client specs; client tools keep their browser-fulfilled relay path. The old permission plumbing is deleted: the extension's relayPermissionCheck poll and its dispatch.ts helpers, the relay watcher's permission handling (handlePermissionRelayRequest, the kind: "permission" record protocol, RelayPermissions and the permissions.decide re-check on execution requests), and the planned double-gate bridge (with the relay re-check gone, nothing consumes it). A builtin-only run starts no relay at all (useToolRelay is now custom-tools-only).

Slice 3 (park + resume, sandbox_agent.ts + server.ts). ParkedApproval.gateType is widened to include "pi-acp-permission", the park records the plane, and the server.ts resume guard accepts it. The reply is already correct (once/reject; the sandbox-agent daemon maps it to the dialog option by kind), so no reply-type widening. The pool, TTL, eviction, and multi-gate safe-degrade paths key on parkedApproval, not the harness, so they need no change.

Review round (codex P1/P2): the relay execution guard and context-binding redaction.

P1 — the guard. The relay dir is sandbox-writable, so the model could forge an <id>.req.json execute record with bash and bypass the dialog gate entirely; with the old relay-side re-check deleted, a forged record for an ask or deny custom tool would have executed. startToolRelay now takes a RelayExecutionGuard (Pi runs only) that re-checks every record runner-side: author-allow executes, author-deny never does, and an ask tool executes only by consuming a per-turn grant (ApprovedExecutionGrants, keyed on approvedCallKey(name, args) with a count). Grants are recorded at the two places an approval-equivalent allow is issued: handlePiGate when a custom-tool gate resolves to allow (author/policy/stored-decision — this also covers the cold path, where the re-issued call's dialog consumes the durable decision), and the keep-alive resume branch before respondPermission("once") (the resume turn has a fresh relay and ledger). A forged or replayed record fails closed with a deny result text; the guard's decide() runs over an empty stored-decision store so it can never race the dialog for the conversation's stored decisions. Claude runs are unchanged (their harness gates fire before the relay, exactly as before this PR).

P2 — redaction. For a callRef tool with contextBindings, the runner overwrites bound argument paths from runContext at execution, so the approval card and the stored-decision key must not carry the model's values for them (the human would approve args that never run — the same drift class this PR exists to kill, and the pre-#5185 relay path already redacted them). buildPiGateDescriptor now strips bound paths (restored redactContextBoundArgs), the card's rawInput and the park record carry the redacted shape, and the guard's grant/consume sides key on the same redaction, so approval, display, decision key, and grant all agree.

Before (cold): Pi calls a gated tool, the runner records a durable interaction, ends the turn paused, destroys the session; the human clicks Approve; a fresh session cold-replays and the model re-issues the call, which the decision map matches by name plus canonical arguments (and re-fires the gate on drift).

After (warm, within the TTL): the resume answers the held dialog, the hook returns allow, and the original call runs with its original arguments inside the original prompt(). Call N+1 to the LLM carries the real result for the original id. Byte-exact.

Scope / risk

  • On by default, no flag. Keep-alive (AGENTA_RUNNER_SESSION_KEEPALIVE) still gates the pool and the parking: with it off, a Pi ask still gets the instant dialog decision and real card identity, and degrades to the cold durable-decision path. The live warm/cold matrix is the separate slice-4 QA stage.
  • No /run wire SHAPE change. The envelope is sandbox-internal; the interaction_request event and the golden fixtures / both contract tests are untouched. One value-semantics change: a Pi approval's interaction_request.id is now the ACP permission id (the tool-call id still rides payload.toolCallId), matching what Claude approvals already emit; the old relay path used the tool-call id for both. The FE resolves cards by echoing the interaction id, so this aligns Pi with the path Claude already exercises.
  • Not in scope: the client-tool MCP pause (Gate 3, different transport), harness session resume (session/load, tier 2, owned by the harness-session-resume project), and any pi-acp upstream change (the envelope makes the current bridge sufficient; the structured-metadata field is a recorded follow-up).
  • Daytona inherits the "keep-alive off" row: the pool does not park Daytona sandboxes yet (keep-alive slice 3 deferred), so a Daytona ask still pauses, destroys, and cold-resumes. The dialog transport itself works there, so Daytona parking needs no Pi-specific work once pool parking lands.
  • The old relay permission path is deleted outright (extension poll, record protocol, and tests); there is no flag and no in-place rollback lever — rolling back means reverting the release. (An earlier draft of this PR kept the old path behind a flag for one release; that lever was dropped when the dialog gate became the unconditional Pi behavior.)
  • Pre-existing, unchanged: on a CLAUDE run the relay never re-checked records (its harness gates fire before the relay), so the forged-record exposure the new guard closes for Pi predates this PR there. Recorded as a follow-up; fixing it here would change Claude behavior this PR deliberately leaves alone.

Tests / notes

  • Slice 0 (the hard gate) passed live on the dev box, driving a real Pi session through the sandbox-agent daemon (not pi-acp directly) on the codex subscription. The envelope arrived byte-exact at a runner-side onPermissionRequest handler (hostile probe string included), the daemon exposed availableReplies: ["once","reject"], and respondPermission(id, "once") resolved the held dialog to allow so the original park_probe ran with its original token, both immediately and after a 180-second hold with no reaper. This also confirmed the daemon maps the reply by kind (no reply-mapping needed) and that the ACP request's toolCallId is the synthetic pi-ui-<uuid> while the envelope carries the real id (so the normalization is required).
  • 740 runner unit tests pass (the count first went DOWN with the deleted relay-permission machinery — its surviving grant-list regression pin moved to tests/unit/builtin-grant-list.test.ts — then up with the review-round coverage; the 2 sandbox-agent-qa-transcript-replay fixture failures are pre-existing and unrelated). tsc --noEmit clean. New coverage: envelope build/parse incl. malformed fail-closed and the hostile probe string; id normalization everywhere the id is read; gate classification with runner-side metadata lookup; the Claude title-collision passthrough (a Claude run never enters envelope detection); the unknown-builtin and unresolved-custom-tool fail-closed rejects; the client-spec exclusion; the extension-level dialog gate; validate-before-dialog (a malformed call errors to the model, never prompts a human); the Pi dialog gate park + resume at the dispatch seam; the relay execution guard (tests/unit/tool-relay-guard.test.ts: forged record fails closed, grant consume-once, author allow/deny, Claude parity, redacted-grant matching); context-binding redaction at the descriptor, card, and park record.

How to QA

Prerequisites: the local EE dev stack with keep-alive on. load-env hosting/docker-compose/ee/.env.ee.dev then run.sh --ee --dev. Set AGENTA_RUNNER_SESSION_KEEPALIVE=1 on the runner sidecar and use a Pi agent whose tool or builtin has an ask permission. The dialog gate itself needs no configuration (on by default for Pi).

Steps:

  1. In the playground, run a Pi agent and prompt it to call a gated tool (or a gated builtin like bash).
  2. When the approval card appears, wait 20-30 seconds, then click Approve.
  3. Observe the tool result.

Expected result: the approval card shows the real tool name and arguments (not agenta-approval / envelope JSON). After Approve, the ORIGINAL tool call runs with its original arguments in the same turn (no cold rebuild, no re-issued call). A Deny reports the call as blocked and the turn continues on the same session.

Automated tests:

cd services/runner && pnpm test && pnpm run typecheck

(Key files: tests/unit/pi-gate-envelope.test.ts, tests/unit/sandbox-agent-acp-interactions.test.ts, tests/unit/tool-relay-guard.test.ts, tests/unit/responder.test.ts, tests/unit/extension-tools.test.ts, tests/unit/session-keepalive-approval.test.ts, tests/unit/sandbox-agent-pi-assets.test.ts.)

Edge cases: (1) a Claude run must behave exactly as before (it never enters envelope detection, even for a tool titled agenta-approval). (2) A custom tool that is author-allow must NOT newly pause; a read-only builtin must auto-allow; a tool name with no resolved spec must be blocked, never allowed. (3) After a >5-minute wait (past the approval TTL), the click should degrade to the cold decision-map path, not fail the turn. (4) With keep-alive OFF, an ask still pauses and cold-resumes via the durable decision.

Plan and evidence: docs/design/agent-workflows/projects/pi-approval-parking/ (ships in #5153).

https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W

@vercel

vercel Bot commented Jul 9, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agenta-documentation Ready Ready Preview, Comment Jul 10, 2026 9:35am

Request Review

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a shared Pi approval-gate envelope contract and routes Pi permission approvals through an extension UI dialog plane, alongside the existing Claude ACP and relay gates. Generalizes ParkedApproval.gateType, extends keep-alive park/resume to accept Pi dialog gates, and introduces a relay double-gate decision bridge, with accompanying tests.

Changes

Pi dialog gate approval parking

Layer / File(s) Summary
Pi gate envelope contract
services/runner/src/engines/sandbox_agent/pi-gate-envelope.ts, services/runner/tests/unit/pi-gate-envelope.test.ts
New module defines envelope constants, types, buildPiGateEnvelope, and a fail-closed parsePiGateEnvelope, with unit tests for round-trip and malformed-input handling.
Pi extension dialog gating
services/runner/src/extensions/agenta.ts, services/runner/tests/unit/extension-tools.test.ts
Adds piDialogAllows and threads dialogGate into builtin tool_call handling and custom tool execution to gate via ctx.ui.confirm, denying by default on missing UI or errors.
Pi extension env flag wiring
services/runner/src/engines/sandbox_agent/pi-assets.ts, services/runner/tests/unit/sandbox-agent-pi-assets.test.ts
Adds dialogGateActive option to buildPiExtensionEnv, setting AGENTA_AGENT_PI_DIALOG_GATE when active.
Responder-side Pi gate detection and pause
services/runner/src/engines/sandbox_agent/acp-interactions.ts, services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts
Adds ParkedApprovalGateType, PiToolSpecMeta, buildPiGateDescriptor, and Pi envelope detection/fail-closed rejection in attachPermissionResponder, with gate-type-aware pausing for Claude and Pi gates.
ParkedApproval generalization and sandbox agent wiring
services/runner/src/engines/sandbox_agent.ts
Generalizes ParkedApproval.gateType, adds piDialogGateEnabled(), and wires dialogGateActive/piToolSpecsByName and gate-type recording into the Pi run path.
Relay double-gate bridge
services/runner/src/responder.ts, services/runner/tests/unit/responder.test.ts
Adds ConversationDecisions.appendDecision and ApprovalResponderOptions.bridgeRelayDoubleGate to re-queue consumed allow decisions for relay-consumed Pi double gates.
Keep-alive park/resume for Pi gates
services/runner/src/server.ts, services/runner/tests/unit/session-keepalive-approval.test.ts
Expands awaiting_approval resume validation to accept pi-dialog-permission alongside claude-acp-permission, with tests for park/resume and warm-resume relay decisions.

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Pi as Pi runtime
  participant Extension as agenta.ts
  participant User as ctx.ui.confirm dialog
  participant Runner as sandbox_agent / acp-interactions
  participant Responder as ApprovalResponder

  Pi->>Extension: tool_call(ctx, toolCall)
  Extension->>Extension: buildPiGateEnvelope(gate, toolName, input)
  Extension->>User: ctx.ui.confirm(title, envelope message)
  alt UI unavailable or error
    Extension-->>Pi: deny (fail-closed)
  else user responds
    User-->>Extension: allow/deny
    Extension-->>Pi: block or proceed via relay
  end

  Runner->>Runner: parsePiGateEnvelope(request)
  Runner->>Runner: buildPiGateDescriptor(envelope, toolSpecs)
  Runner->>Responder: onPermission(gate)
  Responder-->>Runner: allow/deny/pendingApproval
  Runner-->>Runner: pause with gateType (claude-acp-permission or pi-dialog-permission)
Loading

Possibly related PRs

  • Agenta-AI/agenta#5041: Builds directly on the same ApprovalResponder/ConversationDecisions keepalive/resume logic extended here with Pi dialog-gate handling.
  • Agenta-AI/agenta#5156: Shares the same keep-alive/approval parking-resume plumbing in server.ts and sandbox_agent.ts that this PR extends for Pi gates.
  • Agenta-AI/agenta#5158: Introduces the slice-2 approval-park/resume machinery (ParkedApproval, onUserApprovalGate, awaiting_approval validation) that this PR generalizes for Pi dialog gates.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 60.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main change: parking Pi approval gates through the ACP permission plane.
Description check ✅ Passed The description is detailed and clearly matches the Pi approval-parking changes in the pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/pi-approval-parking

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

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

): Promise<void> => {
const toolCall = req?.toolCall;
if (toolCall && typeof toolCall === "object") {
toolCall.toolCallId = envelope.toolCallId;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id normalization (B2): the pi-acp bridge delivers a synthetic pi-ui-<uuid> toolCallId; the envelope carries the real one. Swapping it in place here, before the gate descriptor, pause bookkeeping, the emitted card, and the park record read req.toolCall, is what lets the warm resume match the parked id (approvalDecisionForToolCall) and keeps pause suppression from mis-keying and clobbering the FE approval card. Stamping only the emitted payload would not be enough.

if (piGate.matched) {
if (!piGate.envelope) {
log?.(
`[HITL] pi-gate malformed envelope id=${id}; reject (fail closed)`,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fail closed (M4): a request whose title matches but whose envelope does not parse is rejected here, never allowed to fall through to the spec-less path below. Under a default-allow permission plan a fallthrough would resolve to allow and confirm an unapproved execution; strict-parse-then-reject makes the worst case a blocked tool call. rejectRequest answers reject WITHOUT creating/resolving a durable interaction (none was recorded for a request refused before classification).

Comment thread services/runner/src/responder.ts Outdated
// so appending in those cases would leave a stale decision for a later gate. Builtins (executor
// "harness") have no relay second gate, so they are excluded too.
if (
request.gate.executor === "relay" &&

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double-gate accounting (M2): a pi-custom-tool dialog gate and the relay's execution check both decide() the same key. When the dialog answered from a STORED decision it consumed one queued entry, so the relay would find none and pause again. We re-append exactly that decision. Guarded to fire ONLY for executor 'relay' + effective 'ask' + a resolved allow/deny: a policy allow/deny consumes nothing (decide returns before stored.take), a pause consumes nothing, and builtins have no relay second gate. The warm-resume path never reaches here (the dialog is answered via respondPermission), so it seeds the relay via extractApprovalDecisions instead and does not double-append.

Comment thread services/runner/src/server.ts Outdated
if (
!parked ||
(parked.gateType !== "claude-acp-permission" &&
parked.gateType !== "pi-dialog-permission")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resume guard (B3): widened from the single Claude literal to accept the Pi dialog gate too. Both planes resume via respondPermission on the live session, and the sandbox-agent daemon maps the once/reject reply to the dialog option by kind, so no reply-type widening is needed. Without accepting pi-dialog-permission here a Pi park would fall to the 'unrecognized-gate-type' mismatch below and always degrade cold — fail-closed but never warm (the (b)-half of the plan's B3, which also widens the ParkedApproval union in sandbox_agent.ts).

@mmabrouk
mmabrouk force-pushed the feat/pi-approval-parking branch from 0b6a8f3 to 19f8e5d Compare July 9, 2026 19:41
Comment thread services/runner/src/responder.ts Outdated
// consumed nothing); relay executor only (builtins have no relay second gate); and only
// when the dialog-gate bridge is on (see ApprovalResponderOptions).
if (
this.options.bridgeRelayDoubleGate &&

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review-round tightening of the double-gate accounting: the re-append now (a) requires the explicit bridgeRelayDoubleGate option, set only on Pi runs with the dialog gate active (on Claude the relay never enforces, so an appended decision has no consumer and would leak an allow to a LATER identical call — a flag-off behavior change), and (b) fires for ALLOW only (on a deny the extension short-circuits without relaying, so an appended deny would auto-deny a later identical call that should re-prompt).

@mmabrouk

mmabrouk commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

An independent correctness review (Opus subagent) ran against the initial commit. It confirmed the fail-closed malformed-envelope path, the id-normalization ordering, the client-tool exclusion, the gateType plumbing, and the runner-side metadata recovery, and found one real blocker in the double-gate accounting: the re-append fired for deny (which has no relay consumer, since the extension short-circuits on a deny) and was not scoped to Pi (a Claude relay-shaped ask gate answered from a stored decision would have appended an orphan allow, changing flag-off behavior). Both are fixed in the amended commit: the bridge is behind an explicit ApprovalResponderOptions flag set only for Pi dialog-gate runs, and it re-appends allow only. New tests pin the bridge-off (Claude-shaped) case and the deny no-append case. Stale comments the review flagged (the 'never mutated' note in acp-interactions.ts and the 'exactly one gate per call' note in sandbox_agent.ts) were corrected in the same amend.

@mmabrouk

mmabrouk commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Second review round (Codex xhigh, independent): no blockers, two must-fixes, both applied in the amended commit 3fe24e1.

MF-1 (flag-off byte-identity): envelope detection ran on every ACP permission request and pre-filtered on the dialog title alone, so with the flag off a Claude gate whose title collided with the literal dialog title (editing a file with that name, a bash command equal to it) would have been auto-rejected where big-agents pauses or resolves it. Detection is now behind an explicit dialogGateEnabled input to attachPermissionResponder, set only for a Pi run with AGENTA_RUNNER_PI_DIALOG_GATE on. A title-collision passthrough test pins the off path.

MF-2 (unknown builtin fail-closed): an envelope naming an unrecognized builtin fell through as a harness gate with the raw sandbox-supplied name, diverging from the relay (which denies unknown builtins) and letting an untrusted envelope put a fabricated name on the approval card. buildPiGateDescriptor now returns no descriptor for an unknown builtin and the caller rejects, with a test.

Nits included: an unanswerable (empty-id) malformed request now pauses instead of silently dropping (matching the base path, so the in-sandbox confirm dies with teardown rather than hanging); stale docs updated (the parkable-gate JSDoc, the approvalToPark comment, and the non-claude-gate-no-park log renamed to non-parkable-gate-no-park). The warm-resume relay-seeding test gap is closed with a real dispatch-level test: it drives runTurn's resume branch and asserts the relay permissions captured at the startToolRelay seam allow the folded approval.

Also recorded in the PR body follow-ups: the pre-existing seeded-decision lingering (one click can auto-resolve a later byte-identical call in the same resumed turn, both harnesses, big-agents behavior); candidate fix is consuming the parked gate's own decision on the resume branch.

Tests: 769 pass (+4), same 2 known sandbox-agent-qa-transcript-replay fixture failures, tsc --noEmit clean.

@mmabrouk

mmabrouk commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Live QA: Pi approval parking (slice 4)

Ran the warm/cold matrix live on the Hetzner dev box, programmatically (direct /run NDJSON to the runner, no UI). All Pi cells pass. Flag-off Pi path is byte-identical to before.

Environment

  • Isolated QA runner started from this lane's working tree (services/runner/src bind-mounted, extension rebuilt on boot from the lane source so dist/extensions/agenta.js carries the dialog gate). Ran as a separate container on port 8791, NOT the live subscription sidecar (that one was left untouched, still serving the playground).
  • Flags: AGENTA_RUNNER_SESSION_KEEPALIVE=1, AGENTA_RUNNER_PI_DIALOG_GATE=1. Harness pi_core, model openai-codex/gpt-5.4-mini, credentialMode=runtime_provided (codex subscription login). Sandbox local. Pool keyed by runContext.project.id + sessionId.
  • Ask gate exercised: the Pi builtin Bash under a permissions.default=ask plan (echo <token>), which routes through the pi-builtin dialog gate. code-kind custom tools are removed (F-010), so the builtin gate is the clean ask surface for a bare /run.

Matrix results

a) Warm approve (KA+DG on, within TTL): PASS. Initial turn parked warm, resume answered the held dialog, original call ran in the original turn.

[HITL] pi-gate {"gate":"pi-builtin","toolName":"Bash","executor":"harness","readOnlyHint":false}
[HITL] gate toolName="Bash" permission=ask outcome=pendingApproval
prompt stopReason=paused
[keepalive] park-approval key=... tool=Bash
[keepalive] park key=... ttl=300000ms state=awaiting_approval
-- resume --
[keepalive] resume-approve key=... tool=Bash
[keepalive] resume answered gate reply=once tool=Bash
prompt stopReason=end_turn

The resume executed the SAME toolCallId (call_nRgZ...|fc_...) with the original args {"command":"echo PARKTOKEN_5FD391"}, tool_result PARKTOKEN_5FD391\n, final output PARKTOKEN_5FD391, stopReason=end_turn. No cold replay (no re-issued call, the original prompt continued). Session re-parked idle after. The dialog toolCallId is the real Pi id, not a synthetic pi-ui-*, confirming the slice-1 id normalization. Reply is once (daemon maps to allow), confirming the B1 finding.

b) Warm deny: PASS. Resume answered reject, tool did not execute, turn continued live, no orphaned pending state.

[keepalive] resume-reject key=... tool=Bash
[keepalive] resume answered gate reply=reject tool=Bash

tool_result: "Denied by the permission policy." isError=true (no token in output). Turn completed end_turn; session re-parked idle (state=idle (re-park)).

c) Approve after TTL (cold): PASS. Approval TTL lowered to 8s for this run. Park expired, session destroyed, resume fell to the cold path, the durable decision ran the tool after replay, nothing crashed.

[keepalive] park key=... ttl=8000ms state=awaiting_approval
[keepalive] approval-ttl-expire key=... (TTL 8000ms)
[keepalive] evict key=... reason=approval-ttl-expire
-- resume --
[keepalive] miss key=...; cold
[HITL] cold replay: transcript 287->287 chars, ... resumeFrame=true
[HITL] resume state: decisions=["Bash#{\"command\":\"echo PARKTOKEN_B9BE34\"}"]
[HITL] gate toolName="Bash" permission=ask outcome=allow

The model re-issued the call (new toolCallId), the stored decision keyed by canonical name+args answered the re-fired gate outcome=allow, tool_result PARKTOKEN_B9BE34\n, output PARKTOKEN_B9BE34, ok=true end_turn.

d) Flag-off regression (DG off, KA on): PASS (Pi), Claude cross-check partial.

Pi ask run with AGENTA_RUNNER_PI_DIALOG_GATE=0: no [HITL] pi-gate line anywhere (the dialog plane is dormant), the gate takes the legacy path, session is NOT parked, resume goes cold:

prompt stopReason=paused
[keepalive] non-parkable-gate-no-park key=...
-- resume --
[keepalive] miss key=...; cold
[HITL] cold replay: ... resumeFrame=true

Cold resume completed and ran the tool (PARKTOKEN_254932). This is the "KA on, DG off" matrix row exactly, byte-identical to pre-lane Pi behavior.

Claude subscription (haiku, self-managed): the run works end to end and returns the token, but I could not force a Claude permission gate from a bare /run. Forcing the Claude ACP gate needs the app-rendered .claude/settings.json permission slice (only the Python service renders it) plus a writable Claude config dir (the QA mount is read-only, so Claude's session-env write hit ENOENT). The Claude ACP park/resume code is not touched by this lane (DG gates only the Pi extension), so there is no regression risk here; the warm Claude approve-once path is the one validated under #5158. Worth a quick playground re-confirm on the live sidecar before default-on if you want full belt-and-suspenders.

Anomalies (non-blocking)

  • sessions/persist DROPPED ... HTTP 401 and heartbeat 401 on every run. Expected: the QA /run carries no valid API credential (cred=MISSING), so record-persist and heartbeat to the API are rejected. Parking and resume are in-runner-memory and unaffected; the matrix is unaffected.

Verdict

Ready for review. The Pi approval-parking invariant holds live: warm approve resumes the original call in the original turn (no cold replay), warm deny blocks cleanly, TTL expiry falls to the durable cold path, and the flag off is inert. Suggested follow-up: pin one recorded warm-approve and one cold-approve as agent-replay regression tests (slice 4 note), and a one-line playground re-confirm of the Claude gate before flipping DG default-on.

@mmabrouk
mmabrouk marked this pull request as ready for review July 9, 2026 20:31
@mmabrouk

mmabrouk commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. feature labels Jul 9, 2026
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 02dd9ccb-c1cf-4db6-971a-036e5d0b4a67

📥 Commits

Reviewing files that changed from the base of the PR and between ead0700 and 3fe24e1.

📒 Files selected for processing (13)
  • services/runner/src/engines/sandbox_agent.ts
  • services/runner/src/engines/sandbox_agent/acp-interactions.ts
  • services/runner/src/engines/sandbox_agent/pi-assets.ts
  • services/runner/src/engines/sandbox_agent/pi-gate-envelope.ts
  • services/runner/src/extensions/agenta.ts
  • services/runner/src/responder.ts
  • services/runner/src/server.ts
  • services/runner/tests/unit/extension-tools.test.ts
  • services/runner/tests/unit/pi-gate-envelope.test.ts
  • services/runner/tests/unit/responder.test.ts
  • services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts
  • services/runner/tests/unit/sandbox-agent-pi-assets.test.ts
  • services/runner/tests/unit/session-keepalive-approval.test.ts

Comment thread services/runner/src/engines/sandbox_agent/acp-interactions.ts Outdated
Comment thread services/runner/tests/unit/sandbox-agent-acp-interactions.test.ts Outdated
mmabrouk added a commit that referenced this pull request Jul 9, 2026
…on plane removed

The dialog gate shipped as the unconditional Pi behavior (Mahmoud, 2026-07-10): no
AGENTA_RUNNER_PI_DIALOG_GATE / AGENTA_AGENT_PI_DIALOG_GATE pair, gateType
pi-acp-permission (symmetric with claude-acp-permission), the relay permission
plumbing and the planned double-gate FIFO bridge deleted, builtin-only runs start
no relay. plan.md carries the final mechanism and the reduced warm/cold matrix
(keep-alive is the only remaining switch); research.md marks its §1/§7 relay
permission mechanics as the replaced code; open-questions #4 (flag coupling)
resolved by there being no flag; status.md records slice-0 evidence, the review
rounds, and the scope change. Implementation: PR #5185.

Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W
@mmabrouk

mmabrouk commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Scope change (Mahmoud): the dialog gate is now the ONLY Pi permission path, with no feature flag, and the old relay permission plumbing is deleted (commit 68c3176).

What changed in this round:

  • Flag removed entirely (AGENTA_RUNNER_PI_DIALOG_GATE / AGENTA_AGENT_PI_DIALOG_GATE and the runner helper). Envelope detection stays scoped to Pi runs structurally: the responder receives the resolved-specs map only on Pi runs, and its presence turns detection on, so the Claude title-collision protection from the previous review round is preserved without any env check.
  • Dead code deleted, execution kept: the extension's relayPermissionCheck poll and its dispatch.ts helpers; relay.ts's permission plane (handlePermissionRelayRequest, the kind:permission record protocol, RelayPermissions, the watcher's permissions.decide re-check); the double-gate FIFO bridge in ConversationDecisions (with the relay re-check gone, the dialog is the single enforcement point and nothing consumes a re-appended decision). relayToolCall and the execute/result flow are untouched; client tools keep their relay pass-through. A builtin-only run starts no relay at all (useToolRelay is custom-tools-only). Net: relay.ts 546 -> 321 lines, dispatch.ts 274 -> 167, four dead test files deleted (the grant-list regression pin survives in tests/unit/builtin-grant-list.test.ts).
  • gateType renamed to "pi-acp-permission" (symmetric with "claude-acp-permission": both ride the same ACP permission plane).
  • Both CodeRabbit findings fixed: a custom-tool envelope with no resolved spec now fails closed like an unknown builtin (replied on the thread), and the test helper's rawInput.title tracks the overridden title.
  • Arguments are validated before the dialog, so a malformed call errors to the model instead of prompting a human.

Docs updated in the same window on the #5153 lane (plan.md final mechanism + reduced warm/cold matrix, research.md landed-note, open-questions #4 resolved, status.md provenance).

Tests: 729 pass, tsc clean; the count went down because the dead permission-relay tests were deleted with their machinery. The 2 known sandbox-agent-qa-transcript-replay fixture failures remain pre-existing.

mmabrouk added 3 commits July 10, 2026 10:32
Both Pi approval gates (builtin + custom-tool) stop expressing an approval
as a file-relay poll and raise it as ctx.ui.confirm carrying a JSON envelope
with the real gate identity. The pi-acp bridge turns that into a real ACP
session/request_permission, which the runner classifies, decides, and (under
keep-alive) parks and resumes on the live session, exactly like Claude's gate.
Within the approval TTL a human's answer resumes the original tool call with
its original arguments; outside it, everything degrades to today's cold path.

Behind AGENTA_RUNNER_PI_DIALOG_GATE (default off); flag-off is byte-identical.

- pi-gate-envelope.ts: the shared, strict, version-checked envelope contract.
- acp-interactions.ts: envelope detection + real-id normalization + runner-side
  metadata recovery + fail-closed malformed reject + gateType plumbing.
- responder.ts: FIFO appendDecision + consume-1-append-1 for the custom-tool
  double gate.
- sandbox_agent.ts / server.ts: ParkedApproval gateType union + resume guard.
- agenta.ts / pi-assets.ts: the extension switch and the flag env.

Slice 0 (live daemon confidence run) passed both criteria on the dev box.

Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W
… the relay permission plane

The dialog gate is now the unconditional Pi behavior. AGENTA_RUNNER_PI_DIALOG_GATE
and AGENTA_AGENT_PI_APPROVAL... flags are gone; envelope detection stays scoped to
Pi runs structurally (the responder receives the resolved-specs map only on Pi runs,
and its presence turns detection on), so a Claude gate whose title collides with the
dialog title still takes the base path.

Dead code removed with it:
- extension relayPermissionCheck poll and its dispatch.ts plumbing
- relay.ts permission plane: handlePermissionRelayRequest, the kind:permission
  record protocol, RelayPermissions and the watcher's decide enforcement
- the double-gate FIFO bridge in ConversationDecisions (with the relay re-check
  gone, the dialog is the single enforcement point)
- builtin-only runs start no relay (useToolRelay = custom tools only)

Also: gateType renamed to pi-acp-permission (symmetric with claude-acp-permission);
CodeRabbit fixes (a custom-tool envelope with no resolved spec fails closed like an
unknown builtin; test title-sync nit); args validated before the dialog so a
malformed call errors to the model instead of prompting a human.

Claude-Session: https://claude.ai/code/session_01CSTSEXSe4DDhoXCFjZpZ5W
…xt-bound args from Pi approval gates

Review findings on the parking change (codex P1/P2):

P1: the relay dir is sandbox-writable, so an execute record forged with bash
bypassed the in-sandbox dialog gate entirely once the relay's permission plane
was deleted. The relay now re-checks every record runner-side (Pi runs only):
author-allow executes, author-deny never does, and an ask tool executes only by
consuming a per-turn grant recorded when the dialog gate (or a parked-approval
resume) resolved that exact call to allow. Grants are keyed on
approvedCallKey(name, args) with a count, so a forged or replayed record for an
ask tool fails closed.

P2: for a callRef tool with contextBindings, the approval card and the
stored-decision key carried the model's values for bound paths the runner
overwrites from runContext at execution — the human approved args that never
run. buildPiGateDescriptor now strips bound paths (restored redaction helper),
and the grant/consume sides key on the same redacted shape.

Also: validate-before-dialog extension test, pi-coding-agent execute-arity
contract comment, guard wiring pinned in orchestration tests.

Claude-Session: https://claude.ai/code/session_012EjviaDzVYboubuwgESDwS
@mmabrouk
mmabrouk force-pushed the feat/pi-approval-parking branch from 68c3176 to 3606e5d Compare July 10, 2026 09:34
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jul 10, 2026
@mmabrouk

Copy link
Copy Markdown
Member Author

Review-fix commit 3606e5d5cb addresses the codex review round:

  • P1 (approval bypass): the relay dir is sandbox-writable, so a forged execute record bypassed the dialog gate once the relay's permission plane was deleted. The relay now re-checks every record runner-side on Pi runs: an ask tool executes only by consuming a per-turn grant recorded when the dialog (or a parked-approval resume) allowed that exact call; forged/replayed records fail closed. Claude runs unchanged.
  • P2 (misleading approvals): context-bound argument paths (runner-filled from runContext at execution) are stripped from the approval card, the stored-decision key, the park record, and the grant key, so the human approves exactly the args that run.
  • Also: the stale rollback-lever paragraph in the description is fixed (the old relay path is deleted outright, no flag), the interaction_request.id value-semantics note is added, and new tests cover the guard (tests/unit/tool-relay-guard.test.ts), the redaction, and validate-before-dialog.

740 unit tests pass, tsc --noEmit clean (the 2 sandbox-agent-qa-transcript-replay fixture failures are pre-existing).

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jp-agenta
jp-agenta merged commit 16e9a0e into big-agents Jul 10, 2026
14 of 20 checks passed
junaway pushed a commit that referenced this pull request Jul 10, 2026
Brings in #5185 (Pi approval parking over the ACP permission plane).

Conflicts in services/runner/src/{engines/sandbox_agent.ts,server.ts} were
comment-only: #5185 rewords the parkable-gate doc blocks to cover the Pi ACP
plane alongside Claude ACP, and adds piToolSpecsByName / onPiGateAllowed above
one of them. Took the incoming wording; verified statically that no executable
line from either side was dropped. The S5 sandbox lifecycle (reconnect,
park-to-warm) and the Pi gate envelope coexist untouched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants