feat(runner): show the real change on the card, commit those bytes - #5760
Conversation
|
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:
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 |
| if (!toolCallId) return true; | ||
| return input.state.store.recordsFor(toolCallId).length === 0; | ||
| }, | ||
| onDenied: (toolCallId) => { |
There was a problem hiding this comment.
This is the discard seam for a denied gate. Both denial paths call it before the harness receives the answer.
The store is scoped to the session, not to the turn. The turn clears it only when no gate parked. So when a human denies one commit and carries a second one, the denied call keeps live records for the full 15 minute expiry.
Those records are enough to execute. On Claude and on Codex the relay guard passes every ask verdict, because the harness raises its own dialog and the runner records no grant. An execute record in the relay directory that carries the denied call id and the same arguments then verifies, consumes, and commits the exact content the human rejected. The relay directory is writable from inside the sandbox.
execution-authorization.md section 3.5 lists a denied gate among the discard events. Section 10 requires the test.
If this call moves after respondPermission, the window returns.
| // still holds them, so this is false and the parked answer stands; on a COLD resume the store | ||
| // is new and empty, and the human's old `{approved: true}` describes bytes no longer in | ||
| // memory (execution-authorization.md 7.2). | ||
| shouldRegateStaleApproval: ({ gate, toolCallId }) => { |
There was a problem hiding this comment.
This predicate answers one question. Is this allow a replayed answer with nothing behind it?
A cold resume destroys the environment and the frozen bytes with it. The conversation still carries the human's {approved: true}, so decide returns allow. Executing on that runs content nobody saw. The old behavior spent the approval, failed at the relay with authorization_missing, and showed the user a failed commit instead of a second card.
The first test is the narrow part. An allow under an ask permission can only come from a stored answer, because decide consults the decision store on no other path. A policy allow is the explicit exception in contract section 4 and must still resolve inline at the relay.
If you widen this test to every allow, you break that exception. If you drop the record count test, a live resume stops consuming its parked approval and the human is asked twice for one commit.
| if (required.length === 0) return { ok: true, args: input.args }; | ||
|
|
||
| try { | ||
| if (this.options.store.recordsFor(input.toolCallId).length === 0) { |
There was a problem hiding this comment.
This check runs at the relay for every harness. It does not depend on a dialog having been raised.
That independence is the point. The relay guard answers whether the permission policy allows the tool. On a non Pi harness it passes ask for compatibility, because the harness owns that dialog and the runner records no grant. A forged record can ride that pass.
This hook demands a record the runner minted itself at the gate. The record binds the exact tool, the exact arguments the model wrote, and the exact bytes a human saw. A missing record fails the call closed.
The block below is the single exception. It resolves inline only on an explicit policy allow. That is a positive statement by the policy owner. A missing record is the absence of information, and the two must not be treated alike.
| // must leave no record and no frozen bytes behind. | ||
| const diffs = await this.buildDiffs(input.args, resolved, operations); | ||
|
|
||
| const contentDigest = strictDigest(resolved.args); |
There was a problem hiding this comment.
The digest covers the model's original arguments, which still hold the markers. It does not cover the resolved arguments.
This is what makes a substituted replay fail. An attacker who sends the resolved content in place of the marker produces a different digest, so verification refuses the call.
The manifest is built once, here, from the same bytes the authorization freezes. It is never rebuilt at execution from a second read of the workspace. A second read is a second chance for the content to differ from what the human approved.
| pause.markAnsweredDeny(decision.toolCallId); | ||
| // Before the harness is answered, and for the same reason as the ACP deny path above: | ||
| // the records this call parked on must not outlive the human's "no". | ||
| approvedContent.onDenied(decision.toolCallId); |
There was a problem hiding this comment.
This is the live resume denial. The gate parked in an earlier turn, and this turn answers it on the same session.
The discard runs before respondPermission below. The order matters, because the relay is live for the rest of this turn and an execute record can arrive at any point in it.
The two marks above this line are bookkeeping only. markToolCallDenied shapes the event the frontend renders. markAnsweredDeny protects the failed frame from a sibling pause sweep. Neither blocks execution, so neither is a substitute for the discard.
| }); | ||
|
|
||
| return { | ||
| onResolveApprovedContent: async ({ toolName, toolCallId, args }) => { |
There was a problem hiding this comment.
This gate hook resolves the markers, freezes the bytes, and mints the records before the card is shown.
It runs only after a non deny verdict, and only for a gate that is about to pause. A denied call performs zero workspace reads. Reading a file for a call that will never run leaks its existence into runner memory, spends the byte budget of the turn, and on Daytona starts a process inside the sandbox for a call the policy already refused.
A resolution failure denies the gate rather than showing a card. A card the runner could not build in full would ask the human to approve less than it appears to show.
6899d7d to
07e8498
Compare
f5dc465 to
0899359
Compare
07e8498 to
ade362e
Compare
0899359 to
730dee1
Compare
|
@coderabbitai review |
|
ade362e to
650dd2d
Compare
477c1ac to
a9af42e
Compare
650dd2d to
809ae8f
Compare
a9af42e to
c4d1a0f
Compare
|
@coderabbitai review |
|
809ae8f to
3d926c4
Compare
6920fe4 to
bf1c586
Compare
f8227a4 to
92da348
Compare
4ef3731 to
b6f7f66
Compare
92da348 to
cdb28d0
Compare
b6f7f66 to
609644c
Compare
cdb28d0 to
a1b8bc3
Compare
b52fda2 to
4e469c9
Compare
a1b8bc3 to
79bd16f
Compare
4e469c9 to
6b0e995
Compare
Gate-time resolution and freezing after a non-deny verdict only (a
denied call performs zero workspace reads), one authorization record per
marker, and a RelayExecutionAuthorizer that verifies and synchronously
consumes on EVERY harness independent of any dialog, which is what
closes the forged-ask hole; the guard's non-Pi ask pass stays as
compatibility. Inline resolution only on an explicit allow verdict
computed against an empty decision store. The diff is computed
runner-side over the exact bytes the digest binds. Also fixes the S3a
whole-value marker bug that broke the founding use case, and implements
catalogGeneration per the adapter matrix.
GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved
using the "ours" side. The commit tree contains additional directories:
.conflict-side-0 — our tree
.conflict-side-1 — their tree
.conflict-base-0 — the merge base tree
.auto-resolution — the auto-resolved tree
.conflict-files — metadata about conflicted files
To manually resolve, check out this commit, remove the directories
listed above, resolve the conflicts, and amend the commit.
…me re-gates marker calls (final review F2, F8)
GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved
using the "ours" side. The commit tree contains additional directories:
.conflict-side-0 — our tree
.conflict-side-1 — their tree
.conflict-base-0 — the merge base tree
.auto-resolution — the auto-resolved tree
.conflict-files — metadata about conflicted files
To manually resolve, check out this commit, remove the directories
listed above, resolve the conflicts, and amend the commit.
…-call-error-detail)
…ay the user declined (E2E W7, U5)
…rded, before dispatch
…arguments; refusals travel as errors with their reason. CONTRACT CHANGE: a guard deny is now an MCP error result (the loop still continues); six tests updated deliberately. Plus the x-ag-ephemeral lift marker.
…of two byte-identical gated calls leaves the approved twin's set matchable under the denied id (bounded, recorded in open-issues)
…ope, so the gate now unwraps {tool,server,arguments} before minting; the Daytona manifest walk uses find's backslash-zero escape because the transport rejects NUL argv bytes (no workspace-file commit could ever be approved on Daytona); the deny path names the argv that died. Three regression tests; three tests that pinned the envelope corrected (that wrong expectation survived two fixed reports).
GitButler-Conflict: This is a GitButler-managed conflicted commit. Files are auto-resolved
using the "ours" side. The commit tree contains additional directories:
.conflict-side-0 — our tree
.conflict-side-1 — their tree
.conflict-base-0 — the merge base tree
.auto-resolution — the auto-resolved tree
.conflict-files — metadata about conflicted files
To manually resolve, check out this commit, remove the directories
listed above, resolve the conflicts, and amend the commit.
…efresh-workspace leaves the live set (both halves pinned; capability-only would shadow-DISAGREE forever, live-set-only lets a future flip go live untested); three vacuous live-routes tests retargeted to the model route
…through the catch (code, named path, next step, and the import-root listing with directory markers) into the operator log; the deny is untouched. The model-facing half needs a pre-gate seam and is a recorded decision, not shipped here.
…ecided (the confirm boolean cannot distinguish them); the settled decision, the no-reshape rule, and go-ask-the-user survive without the false attribution; all three sites share the text and the bundle carries it
…routing change they pin (exactly-one-live-route exists only from this lane; three lanes failed in isolation)
…the s3a reader shipped a statChain walk bug its own tests catch; the fix incl. the NUL manifest separator lives here)
…n suite they extend (hunk attribution owns these regions here)
Railway Preview Environment
|
1 participant
Context
Part of the agent-config-editing stack. Targets
agent-config-editing-s7e. Read the stack bottom up.s3a can read a workspace file. s3b-core can bind an approval to a call. Neither is connected to anything. This lane wires them into the permission gate and the relay, which is where the property users care about starts holding: what a human reads on the approval card is what gets committed.
Two things had to be true at once. The card must show the substance of the change, not a path and a byte count. And the call that executes must be provably the call that was approved, on every harness, including the ones where the runner never sees a dialog.
Changes
The approval path
At the gate, before the card, the runner resolves every
@ag.filemarker, freezes those exact bytes, mints one authorization record per marker, and returns a manifest. The order matters. A denied call performs zero workspace reads, because reading a file for a call that will never run leaks its existence into runner memory, spends the turn's byte budget, and on Daytona runs a process inside the sandbox for a call the policy already refused.The manifest carries each file's path, size, digest, and executable bit. When a
setreplaces a whole field from one file, it also carries a unified diff against the text it replaces. That old text is fetched at the operation's ownbase_revision_id, never the session's revision, and the reason is a real hole rather than a technicality: the session may run revision N while the model correctly supplies head N+1, and diffing against N shows the human an N-to-new change, passes the base check, and replaces N+1 with text nobody compared against it. Nothing fails and the wrong thing commits. A fetch failure fails closed rather than showing a complete-content approval, since approving a replacement without seeing what it replaces is the one thing that mode exists to prevent.The diff is written here rather than pulled in as a dependency. The runner is a standalone package whose image ships to every sandbox host, and a bounded line diff is a hundred lines of well-understood code. It trims common prefix and suffix first and degrades a still-too-large middle to a whole-block replacement rather than spending the turn's budget. Counts and digests stay exact either way.
At execution, the relay verifies the complete record set, consumes it, substitutes the frozen bytes, and runs. This check runs for every harness and does not depend on a dialog having been raised, which is what makes a forged request file useless on the non-Pi
askpath. The catalog generation is captured at mint and compared at consume, because a tool namedcommit_revisionunder generation N may have a different schema, permission, or execution binding under N+1, and an approval minted under N does not describe that call.A refused call is now an error, not a success (contract change)
This is the one behavior change a reviewer should look at closely, because it changes what every caller sees.
A refusal used to travel as an ordinary return string, so the relay wrote
{ok: true, text}and every transport reported a refused commit as a SUCCESS.On Codex the old shape surfaced as a blank successful tool result: the model saw no error and no text, invented an explanation, and told the user to approve again. A refusal that reads as success is worse than one that reads as a crash, because only the second makes the model stop.
RelayRefusalnow exists so the reason cannot be mistaken for output at any layer between the relay and the harness. The model still gets the same text and the loop still continues; onlyisErrorchanges.Six existing tests asserted
res.ok === truefor a refusal. They now assert the refusal, which is the point of the change rather than a side effect of it.Approvals that survive the harness, not just the happy path
Four defects, all found by live QA rather than by the suite, all in the path between a human saying yes and the bytes landing.
Codex wraps tool arguments in an MCP envelope. A recorded MCP call's input is
{tool, server, arguments}, not the tool's own arguments, so the marker scan found noworkflow_revisionat the top level and minted nothing. Every approved commit then died withauthorization_missing. The gate now unwraps that exact shape, and only that shape, before scanning. Three existing tests had pinned the envelope as the expected gate payload, which is how this survived two earlier fix reports; they are corrected.The Daytona transport rejects NUL bytes inside command arguments. The manifest walk asked
findfor a NUL-separated-printfformat, so on Daytona the walk failed and no workspace-file commit could EVER be approved there, on any harness. Local never crosses that transport, which is why the suite stayed green. The walk now usesfind's own backslash-zero escape.An approved import could not find its own authorization. The shim mints under a fresh
randomUUID()while the harness gate carries its own id, so an exact-id lookup came back empty and a genuinely approved commit was refused.findSetByCallnow falls back to matching a complete, unconsumed, unexpired set by tool name, argument digest, and required markers. The digest still binds the model's original arguments, so the security property is unchanged: the fallback finds the right record, it does not weaken what a record proves.A denied gate kept its authorization alive. Neither denial path discarded the records, and the store is session-scoped: it survives the turn whenever a sibling gate is still parked. So after a human denied one commit and carried another, the denied call's records stayed valid for the full fifteen-minute expiry. On Claude and Codex the relay guard passes
ask, so a forged execute record carrying that call's id and arguments would consume the live record and commit exactly what the human rejected. Both denial paths now discard every record for the call and release its bytes before the harness is answered.A fifth, from the same round: a cold resume spent the human's approval on nothing. The environment is gone and so are the frozen bytes, but the conversation still carries their
{approved: true}, and the gate consumed it and replied allow. Execution then failed withauthorization_missing, so the user saw a failed commit instead of a second card. The gate now recognizes a replayed answer for a marker-carrying call with no records behind it, reads the file again, mints fresh records, and shows a new card. The check is narrow on purpose: an allow whose effective permission isaskcan only be a replayed answer, so a policy allow still resolves inline as contract section 4 requires.Editing instructions or skills rebuilds the sandbox again
The lifecycle work had made the
workspaceFilesfacet a live route: an instructions edit rewroteAGENTS.mdon the running sandbox and kept the session. Gate cellmatrix_l5proved that route is a silent lie. Every harness reads its instruction file once, at session start, so the refresh wrote the new file andapplyReconcilePlancommitted the incoming configuration as applied, the pool then reported the new fingerprint, every later turn matched and continued warm, and the model went on answering from the instructions it started with. The user's edit had no effect until something else evicted the session. A cold session with the identical configuration obeyed it immediately, which is what isolated the runner rather than the model.workspaceFilesnow routes torebuild-sandbox, andrefresh-workspacehas leftLIVE_ACTION_KINDS. An instructions edit costs a sandbox again, which is what it cost before the optimisation, and the edit takes effect on the very next turn.Both halves are deliberate. Changing only the live set would leave the router planning a workspace refresh (outcome
reuse) against a coordinator that rebuilds, so every instructions edit in production would log a permanent DISAGREE that no router work could drive to zero. Changing only the capability table would leave a live-set entry nothing routes to, so a future flip back would go live again without any guard firing. The applier keeps the refresh arm, unreachable, because the intended next shape is refresh and then reopen the session, and that needs the reopen to build its session init from the incoming request first.Smaller changes
A
descriptionargument is lifted out of the payload and never saved. The schema marks it withx-ag-ephemeral: true, so the runner strips it by reading the advertised marker rather than by hard-coding a tool name, and the SDK can move or tolerate the field without the two sides drifting.When a marker cannot resolve, the structured reason now survives instead of being reduced to one sentence: the failure code, the path the model named, the operation it sat in, a next step, and
available, the entries that really are under the import root. That last field usually names the correction. The model-facing half is deliberately not shipped, and it is a recorded decision rather than an oversight: answering a permission gate issession.respondPermission(id, reply)wherePermissionReplyis"once" | "always" | "reject", a bare enum with no room for text, so nothing on that path can carry the reason to the model. Today it reaches the operator log. Delivering it needs a channel that does not exist yet, and the design work for that is a follow-up.Refusal text no longer names a decider the caller cannot know. The Pi extension gates through a confirm that resolves to a boolean, so a policy deny, a live human decline, a replayed stored decline and a fail-closed reject all arrive identical; the old wording claimed "policy" for all four, and a model reading it after a human declined one change concluded the tool was unavailable and stopped asking.
Tests
workspace-import.test.ts,tool-direct.test.ts, and the lifecycle suites). This is the first lane whose tip holds every symbol they touch, so anywhere lower they would not compile.What to QA
.agenta-imports/and commit it from that file. The approval card shows a diff of the old text against the new, not just a path.sandbox=daytona. The approval goes through and the revision lands. Before this branch, no workspace-file commit could be approved on Daytona at all.