fix(cli): exit 1 when a denied sandbox boundary is never retried - #4506
fix(cli): exit 1 when a denied sandbox boundary is never retried#4506ggbdpq wants to merge 3 commits into
Conversation
820d8d9 to
31492a9
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
[P1] Correlate the retry itself, not just the tool name
packages/cli/src/runtime-host-run-command.ts:644 treats any later successful call with the same toolName as proof that the blocked call recovered. A tool name does not identify an invocation or its target. For example, a denied Read of /outside/secret.txt followed in a later step by a successful Read of /workspace/README.md still clears the unresolved boundary failure. run-command-core.ts then removes the boundary diagnostic from its failure set, prints the final apology as ordinary output, and exits 0, so scripted callers can still accept an operation that never completed.
I reproduced this through the production runFixtureCommand seam on exact head 31492a9dba447e2e2566ae539a81e7561f8f60ae: two Read starts carried different args.path values, the first result carried sandbox_boundary_required, and the second succeeded in a later step. The expected exit code was 1; the command returned 0. The new tests only vary toolName, while the existing positive recovery fixture gives both calls the default Read name and empty args, so neither case distinguishes a retry from a different invocation of the same tool.
Please correlate a stable retry identity, or compare a stable semantic target derived consistently for live and durable observations. If the available wire data cannot prove that the success belongs to the blocked operation, keep the failure unresolved. Add live, durable, and command-exit regressions with two same-named calls whose targets differ.
The exact-head CLI build succeeded, the complete CLI suite passed 723 tests with 3 platform skips, the changed files pass Biome and git diff --check, the hosted test job is successful on this head, and the current-main merge is clean. I am withholding approval for the false-success path above.
Automated review notice: This comment was posted by an automated review agent operated by WAWQAQ. It is not an independent human review and does not replace one.
|
Fixed in 6ee47cd — the recovery correlation now compares the retried call's semantic target, not just its name:
Local: target suite 40/40 pass on Windows; the full CLI suite shows only the pre-existing platform failures (LaunchAgent/EPERM fsync/symlink) also present without this change. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The problem is #4388 and it is real; the classifier change is on the right owner and the four new tests do fail on the old code (I short-circuited the name and args checks in dist and all four went red). CI, lint, format and the 40-case file are green on 6ee47cd3, merge-tree against main is clean.
The args half does not work on the path maka run actually uses. The Guest projector (session-projector.ts:619) sets args: undefined on every live tool_start, by design: session-continuity.ts:69 says live frames carry a bounded, redacted argsPreview and never the full args, and shell runs carry only shellRunRef. The TUI already reads event.args ?? event.argsPreview for that reason. So in production stableArgsJson(event.args) always returns a fresh unprovable:N, argsJson never equals failedArgsJson, and recovered is unreachable on live observation: a sandbox failure that really was widened and retried still classifies as unresolved, run-command-core.ts returns 1 and drops finalOutput even on a completed Turn. The tests pass because the fixture hands the classifier args: {}, a shape the producer never emits. The body's "both live tool_start events and durable tool_call messages carry args" is not true for live.
That leaves a decision I would rather make now than after another round. #4389 (jsiu93, approved, green) fixes the same issue on the same hunk with the tool-name check alone, which live events do carry, and its tests also pin empty stdout and the exact stderr diagnostic. Your first commit 31492a9d is semantically that change; the second adds args on top. I intend to merge #4389 first. If you want to keep this PR, rebase it on #4389 and reduce it to what live observation can prove: either compare argsPreview / shellRunRef and fail closed when the preview is truncated, saying so in the body, or keep args only for the durable path and add one live regression built from the projector's real shape (args: undefined plus argsPreview). If neither is worth it, closing in favour of #4389 is fine.
Two simplifications that survive either way, both tested by me against the 40 cases with no change in result: @maka/core/tool-args-identity already exports stableJsonStringify, which is what tool-ledger-scanner.ts and tool-recovery-bundle.ts use to say "same call", so the hand-written normaliser can go; and unprovableArgsSeq, a process-wide mutable counter whose only job is to be unequal, is redundant with the failedArgsJson !== undefined guard three lines below, so undefined is the sentinel.
One more for whichever PR lands: the sandbox-boundary scenario in run-command-fixture.ts is the only one that emits sandbox_boundary_request, which is the sequence #4388 actually reports, and it still hard-codes sandboxBoundary: 'none'; a recovered variant asserting exit 1 would pin the original report. And run-command-core.ts:314 lets recovered clear unclassifiedBoundaryFailure, which line 406 set from a hard fact (this run denied a widening); a heuristic should not erase that, but I could not build a false success after this PR, so noting it, not blocking on it.
Evidence boundary: static read against main 61224f51; cli build, the run-command test file, lint and format run locally; the live-path claim is from the projector and continuity protocol source, not a live Host run; durable StoredMessage.args not traced through the transcript RPC.
AI-assisted review: drafted with Maka; I verified the projector's args: undefined, the overlap with #4389 and the two dist substitutions myself.
简体中文
问题真实、owner 正确、新测试在旧代码上确实会红。但 args 这半在 maka run 实际走的 live 路径上不生效：Guest projector 把每个 live tool_start 的 args 写死为 undefined（协议注释明确 live 只带有损的 argsPreview），于是生产中 recovered 不可达，真正扩权重试成功的情况也会 exit 1 并吞掉 stdout；测试全绿是因为 fixture 给了生产不会产出的 args: {}。正文「live 事件携带 args」不成立。#4389 已 approve 且用 tool name 就解决了同一 hunk、同一 issue，我打算先合它；本 PR 要留就 rebase 到 #4389 上并只保留 live 能证明的部分（比较 argsPreview/shellRunRef 并 fail-closed，或 args 只用于 durable 路径并补真实形状的 live 回归），否则关掉也可以。两处简化：复用 @maka/core/tool-args-identity 的 stableJsonStringify，删掉 unprovableArgsSeq 计数器，均已实测等价。
| * keeping the failure unresolved (fail-closed). | ||
| */ | ||
| let unprovableArgsSeq = 0; | ||
|
|
There was a problem hiding this comment.
On the live path this branch is the only one ever taken: session-projector.ts:619 sets args: undefined on every tool_start (session-continuity.ts:69 says live frames carry only argsPreview). So recovered is unreachable in production and a genuinely widened retry still exits 1. Also @maka/core/tool-args-identity already has stableJsonStringify, and undefined works as the sentinel because of the guard on line 770.
|
On the Host path the denial never reaches the exit decision: |
6ee47cd to
8d8d482
Compare
|
Pushed 8d8d482 addressing both follow-ups:
Red/green would have been welcome - the added regressions cover the three shapes: live and durable same-name/different-target (exit 1), same-target retry still recovers (exit 0), and denied-widening-then-success (exit 1 with the diagnostic). CI is running on 8d8d482. |
8d8d482 to
f1130cb
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Rebase onto the landed name-only fix is done, and the widening pin closes the Host-path gap yihanzhu reported: I reproduced the shape and the run now exits 1. Three of the four new cases go red when I neutralise the pin and the args comparison in dist, so they are real regressions. test is green on this head, the merge against main is clean, and the two changed files pass Biome check and format. I built and ran packages/cli in a worktree: tsc -p tsconfig.json clean, dist/__tests__/runtime-host-run-command.test.js 39/39.
The live-path point from my last round is still open, and it now points somewhere different than it did before.
[P2] recovered cannot fire on the live wire, and after this PR it probably cannot fire at all
session-projector.ts:616 still sets args: undefined on every projected tool_start, so stableArgsJson(event.args) returns a fresh unprovable:N for every live call and call.argsJson === unresolved[0].failedArgsJson is never true. I forced the fixture's toolStart helper to emit the projector's real shape (args: undefined) and three tests went red, including two that predate this PR: still reports recovered when the retry targets the same semantic args (live), returns exit code 0 when a root Graph boundary failure recovers, and reports a recovered sandbox boundary from live and durable Turns. They pass only because the helper hands the classifier args: {}.
What changed since my last round is the consequence. Your second commit argues that a non-interactive run can never grant a widening, and both surfaces that consume this classifier bear that out: runtime-host-run-command.ts:934 answers every sandbox_boundary interaction with decision: 'deny', and activation-command.ts:521 does the same. A blocked call cannot succeed against the same target unless the boundary moved, and the boundary cannot move here. So recovered is not just unreachable on live observation, it looks unreachable on the durable path too, and the args correlation exists to make a distinction that no run can reach.
If that reading holds, the smaller change is to drop recovered rather than to correlate into it: sandboxBoundary becomes 'none' | 'unresolved', and stableArgsJson, the argsJson field on both observation constructors, the pin, run-command-core.ts:314, and the !== 'recovered' clause at activation-command.ts:592 all go with it. That version of this PR is a net deletion that produces the same exit codes on every path I could reach. If you think there is a run that legitimately recovers, I would rather see that path named and covered by a live regression built from the projector's real shape than keep the mechanism on the strength of a fixture the producer never emits.
[P2] The sandbox_boundary_request branch in run-command-core has no production caller
run-command-core.ts:404 sets unclassifiedBoundaryFailure and denies the request, but #observeTurn (runtime-host-run-command.ts:456) drops that event before the core loop sees it, and the actual denial is issued by NonInteractiveInteractionController. createRuntimeHostRunContext is the only production MakaRunContext, so the only thing that reaches that branch is the synthetic sandbox-boundary scenario in run-command-fixture.ts. Your pin is the live-path replacement for exactly that fact, which makes this the thing the change renders redundant. Removing it means reworking that fixture scenario, so I am fine with it as a follow-up, but I would like it named rather than left sitting there as a second-looking authority.
[P2] Exit 1 also swallows the final message, and the pin makes that the common case
run-command-core.ts:449 returns 1 before writeStdout, so a pinned run prints nothing on stdout. The Turn that triggers the pin is precisely the one the runtime prompts to explain itself: after a denial the model gets SANDBOX_BOUNDARY_FINALIZATION_PROMPT and is told to give a concise final status of what stayed blocked. Today that text is produced and then discarded. The exit code is right; losing the explanation with it is a worse deal than it was when the pin did not exist. Not a blocker for this PR, and the suppression predates it, but it is now the normal outcome rather than a corner.
Evidence boundary: worktree build of packages/cli at f1130cb4, the target test file, Biome check and format on the two changed files; the red-on-base and args: undefined results come from substitutions in dist, not from a rebuild of the base tree. The claim that no non-interactive run grants a widening rests on reading the two deny sites and createRuntimeHostRunContext being the only production context, not on a live Host run. I did not run the full CLI suite.
AI-assisted review: drafted with Maka. I verified the projector's args: undefined, the three dist substitutions, both deny sites, and the dead run-command-core branch myself.
简体中文
已经关掉的：rebase 到 main 上的 tool name 版本做好了；widening pin 确实补上了 yihanzhu 说的 Host 路径缺口，我把 pin 和 args 比较在 dist 里短路掉之后，四个新用例里有三个变红，是真回归。CI 绿、merge 干净、Biome 过、cli 单独 tsc 干净、目标测试 39/39。
还剩的主要是一件事，而且它的结论跟上一轮不一样了。live 路径上 projector 仍然把 tool_start 的 args 写成 undefined，所以 args 比较永远不成立；我把 fixture 的 toolStart 改成生产真实形状之后，有三个 recovered 相关用例变红，其中两个还是这次改动之前就有的。关键在于你第二个 commit 自己给出的理由：非交互运行不可能批准扩权，而 maka run 和 maka activate 两条路都是无条件 deny。既然边界不会移动，被拦住的同一个目标就不可能后来成功，那 recovered 这个状态在两条路上都到不了，args 相关也就是在为一个没人能走到的区分服务。真要收敛，更小的改法是把 recovered 整个删掉，sandboxBoundary 只留 'none' | 'unresolved'，顺带 stableArgsJson、两处 argsJson 装配、这个 pin、run-command-core.ts:314 和 activation-command.ts:592 一起走，退出码在我能走到的路径上完全一样，而这个版本是净删除。如果你认为确实存在合法 recovered 的运行，希望把那条路径写清楚，并补一个用 projector 真实形状构造的 live 回归。
另外两点：run-command-core.ts:404 那个分支在生产里根本到不了（事件被 #observeTurn 丢掉，真正的 deny 在 NonInteractiveInteractionController），它正好就是这次 pin 在 live 路径上的替代品，属于这次改动让它变冗余的东西，放到后续处理可以，但希望明确提一句。还有 exit 1 会连 finalOutput 一起吞掉，而被 pin 的这类 Turn 恰恰是运行时专门提示模型去解释「还有什么被挡住」的那一类，现在这段话被产出来又丢掉了；退出码没问题，但这个副作用从边角情况变成了常态。
证据边界：在 worktree 里构建 packages/cli、跑目标测试、对两个改动文件跑 Biome；红/绿和 args: undefined 的结论都来自 dist 替换而不是重新构建基线；「非交互不可能批准扩权」是读两处 deny 加上只有一个生产 context 得出的，不是实跑 Host。全仓测试没跑。
本次评审借助 Maka 起草，projector 的 args: undefined、三处 dist 替换、两处 deny、以及那个死分支我都自己核过。
| * missing) yield a unique unprovable marker that never matches anything, | ||
| * keeping the failure unresolved (fail-closed). | ||
| */ | ||
| let unprovableArgsSeq = 0; |
There was a problem hiding this comment.
This counter is now redundant with the guard you added on line 688: return undefined for unprovable args and failedArgsJson !== undefined already keeps the failure unresolved, while a provable success can never equal undefined. That also drops a module-level mutable that makes stableArgsJson non-idempotent. And @maka/core/tool-args-identity already exports stableJsonStringify, which is what the rest of the repo uses to say "same call" and which throws on the values you are catching here, so the whole helper reduces to a try/catch around it.
| assert.equal(exitCode, 1); | ||
| }); | ||
|
|
||
| test('still reports recovered when the retry targets the same semantic args (live)', async () => { |
There was a problem hiding this comment.
This asserts a live recovery the wire cannot produce: session-projector.ts:616 sets args: undefined on every projected tool_start, so both calls serialize to distinct unprovable:N and the real run exits 1. It passes because toolStart defaults to args: {}. If recovered stays, this case needs the projector's shape (args: undefined plus argsPreview).
github-actions
Bot
added
effort/M
Review of the args-correlation approach showed that `recovered` is unreachable on every path `maka run` can take: - The live projector sets `args: undefined` on every tool_start (session-continuity carries only argsPreview), so serialized-args equality can never hold for a live observation. - Non-interactive runs deny every widening request (both deny sites answer decision deny), so the boundary cannot move mid-Turn and a blocked call cannot later succeed against the same target. `sandboxBoundary` therefore collapses to none|unresolved: a sandbox failure stays unresolved for the rest of the Turn on both the live and durable paths, stableArgsJson and the args plumbing go away, and run-command-core / activation-command lose their recovered clauses. Exit codes are unchanged on every reachable path. Named follow-ups: the sandbox_boundary_request branch in run-command-core (~:404) has no production caller now that nothing clears the failure set, and an unresolved exit still swallows the Turn's final message - the text the runtime asks the model to produce after a denial - which matters more now that unresolved is the pin outcome. Generated-by: GLM-5.3-Flash (ZCode)
f1130cb to
28f9b88
Compare
|
Took your reading - the branch is now a net deletion, pushed as 28f9b88 (single commit after the rebase; the args-correlation and pin commits were dropped since the deletion subsumes them):
Target suite 39/39 on Windows; Biome clean on the four changed files. CI is running on 28f9b88. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Deleting recovered is the right convergence and this is the shape I hoped for. The heuristic claimed to prove that a blocked call had recovered, while comparing only tool name and step difference, which cannot support that conclusion. Removing it is a net production deletion with no replacement authority.
But the body argues for a different version of the change than the one here, and the conclusion it states about exit codes is the opposite of what the diff does.
[P1] "Exit codes are unchanged on every reachable path" is not true, and it would land in the squash message
The Summary says that, while the Checklist in the same body ticks Behavior change: Yes. The diff's own test flip is the counterexample: returns exit code 0 when a root Graph boundary failure recovers becomes keeps a root Graph boundary failure unresolved even when a later same-named call succeeds, with assertions moving from exitCode === 0 / stdout === 'Final graph answer\n' to exitCode === 1 / stdout === ''. That fixture is Read at step-1 failing and Read at step-2 succeeding.
And that shape is common in real runs, not a corner: tool-runtime.ts:1190 emits stepId when defined, the tool_start projection passes it through, and stepId is currentStepMessageId, which differs per assistant step. So "same tool, later step, succeeded" happens all the time, and main clears recovered on it today.
The two reasons the body gives, that the live projector nulls args and that a blocked call cannot later succeed against the same target, are arguments about a version with args/target correlation that was dropped in the rebase. Main's condition never compares args or requires the same target, so neither reason applies to what is actually being deleted.
The correct argument is simpler and still supports the deletion: the condition compares information that cannot establish recovery, and non-interactive runs deny widening unconditionally so the boundary cannot move, meaning real recovery does not exist. What follows is a deliberate tightening, and the body should say so.
It also misses a second surface. activation-command.ts:588-592 drops && invocation?.sandboxBoundary !== 'recovered', so an activation that previously returned completed with exit 0 now returns blocked with permission_required and exit 3. That JSON outcome is a contract for cloud callers, and nothing in the body mentions it.
Per the repo's squash rules the message has to stand on its own about compatibility impact, so this needs rewriting before merge, not after.
[P2] Deleting the only reader left a write-only mechanism behind
#unresolvedSandboxFailures still stores { failedStepId, failedToolName }, and :644 only reads .size > 0. Neither field is read anywhere in the file now, and #callByToolUseId exists solely to populate them. The tool_call variant of TurnOutcomeObservation and the stepId projections in observationFromSessionEvent (:689) and observationFromStoredMessage (:726) go with them.
A mechanism that records which step and which tool failed, while participating in no decision, will read to the next person as though it still does. Collapsing to a Set<string> and dropping #callByToolUseId and the tool_call variant takes the production deletion from about 28 lines to about 60 and changes no assertion. Since this change is what made them redundant, it belongs here.
[P2] The exit code now commonly arrives with empty stdout
run-command-core.ts returns 1 before writeStdout(withTrailingNewline(outcome.finalOutput)). The runtime specifically prompts the model for a closing explanation of what was blocked, and that text is generated and then dropped. A caller doing maka run ... > out.txt gets an empty file and exit 1, with only the stderr line to go on.
The suppression predates this PR, but this PR turns it from an edge case into the normal result, and you list it as an agreed follow-up. The exit code is the contract; silently discarding the explanation is not part of it. Writing finalOutput to stdout (or stderr) while still exiting 1 would settle it. If it stays a follow-up, the body should state that pinned runs produce empty stdout.
[P2] The activation change has neither a test nor a mention
Following from the P1 point: grep finds no recovered case in activation-command.test.ts before or after, so nothing pins the transition from completed to blocked. One case with a stream boundary failure plus a completed invocation, asserting blocked / permission_required / exit 3, would cover it.
[P3] Two smaller notes
The new fixtures thread successArgs / failureArgs through four helpers, but the classifier reads no args at all, and after this change it reads neither stepId nor toolName either. So the different-target case, the same-target case and the rewritten root Graph case all traverse identical code and assert the same thing three times. It reads as though the tests prove target discrimination, which the code does not have, and the body's "a same-target retry also exits 1 (the boundary cannot move)" implies the same. Keeping one live and one durable case plus the deniedWideningEvents one (which also checks the stderr diagnostic) would cover it.
The body says 2 of the new cases fail on unpatched main. Working through main's clearing condition I get all four, including deniedWideningEvents, since request_sandbox_boundary is excluded by the toolName check and the later Read at step-3 still triggers clearing. I could not build to confirm, so it may be that "unpatched main" means an earlier base; worth naming the SHA.
Next step
Keep the direction. Before merge: rewrite the body and squash message so they describe the tightening and include maka activate; delete the write-only state this change orphaned; add the activation regression. The stdout point can stay a follow-up if the visible consequence is stated.
Manual acceptance: one real maka run following #4388's repro but having the model's second call read a file inside the workspace, confirming exit 1, empty stdout, and the boundary diagnostic still on stderr.
Evidence boundary: static read of 28f9b88 against merge base 9d4002b38, no checkout, no build, no dependency install, no tests run. That recovered is reachable on the live path comes from the source chain tool-runtime.ts:1190 to the tool_start projection to ai-sdk-backend.ts:2321, not from running a Host. I did not trace the durable path's StoredMessage.stepId producer, and I did not re-run the reproduction reported in this thread.
AI-assisted review: drafted with Maka.
…te the outcome Per review on apache#4506: with the recovery heuristic gone, the classifier's per-failure step/tool bookkeeping participated in no decision while reading as though it did. The unresolved set is now just the blocked tool-use ids, and the tool_call observation variant with its step/name projections is deleted. Also pins the activation transition this deletion creates: an activation whose stream carried a sandbox boundary failure while the invocation completed now blocks with permission_required and exits 3 instead of completing with exit 0, and trims the same-target fixture that traversed identical code to the different-target case.
|
All four review items are addressed, pushed in d617b59 on top of the net deletion:
Suites: |
format:check on CI wants the sameStep ternary on a single line; the previous round verified build and tests but skipped the format gate.
4 participants
Summary
recoveredstate unreachable on every pathmaka runcan take: (a) the live projector setsargs: undefinedon everytool_start(live frames carry only a redactedargsPreview), so args equality can never hold for a live observation, and (b) non-interactive runs deny every widening request, so the boundary cannot move mid-Turn and a blocked call cannot later succeed against the same target.sandboxBoundarycollapses tonone | unresolved: a sandbox failure stays unresolved for the rest of the Turn on both the live and durable paths.stableArgsJson, the args plumbing, the widening pin, and therecoveredclauses inrun-command-core/activation-commandare all removed. The classifier's bookkeeping collapses with them: which step and which tool failed participated in no decision, so the map is now a set of tool-use ids and thetool_callobservation variant is gone.Compatibility impact
This is the point of the PR, stated explicitly:
maka runexit codes change on reachable paths. A Turn whose boundary request was denied now exits1with the boundary diagnostic on stderr even when a later same-named (or any later) tool call succeeded — main clearsrecoveredon exactly that shape today, and the flipped fixture (keeps a root Graph boundary failure unresolved even when a later same-named call succeeds) pins the transition fromexit 0+ stdout toexit 1+ empty stdout. The runtime asks the model for a closing explanation after a denial; on an unresolved exit that text is suppressed, so callers doingmaka run ... > out.txtget an empty file. That suppression predates this PR but is now the common path; writing the explanation out while keeping exit1is the named follow-up if review wants it settled here.maka activatechanges: an activation whose stream carried a sandbox boundary failure while the invocation completed now returnsblocked/permission_required/ exit3instead ofcompleted/ exit0(activation-command.tsdrops the!== 'recovered'escape). A new regression pins this transition.1; a denied widening followed by an unrelated successful read exits1with the diagnostic on stderr; the boundary stays unresolved across live and durable Turns.Named follow-ups (agreed with review, not in this PR): the
sandbox_boundary_requestbranch inrun-command-corenow has no production caller and should be removed with its fixture scenario reworked; and surfacing the suppressed final message on unresolved exits.Verification
node --test dist/__tests__/runtime-host-run-command.test.js(packages/cli)maka activatetransition pinnednode --test dist/__tests__/activation-command.test.js(packages/cli)npm run format:checknpm run check:asf-headersnode --test "dist/**/*.test.js"main@6e6af952a(pre-#4389). The base has since moved to9d4002b38, which already includes #4389's name-based fix, so per-case red counts against the current base were not re-measuredAI use
Analysis, patch, and tests were produced with GLM-5.3-Flash (ZCode) under the contributor's direction; the contributor reviewed and is the human contributor of record.
Checklist
maka rununresolved exits (exit1, empty stdout) are now the pinned common path, andmaka activatereturnsblocked/ exit3where it previously completed with exit0.