fix(runtime): land permission switches before the next turn's first tool call (#3349) - #3615
fix(runtime): land permission switches before the next turn's first tool call (#3349)#3615chinawch007 wants to merge 2 commits into
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the careful admission-gate work and the unusually thorough race coverage. I found one security-timing gap at this exact head:
[P1] Apply permission reductions before waiting for the current turn to finish
runSessionQueuedQuiescentMutation closes admission for future claims but waits until hasActiveRuns(sessionId) is false before it runs the transition (packages/runtime/src/runtime-kernel.ts:578-587,664-703). The narrower boundary, shell-run termination, and backend disposal therefore do not begin until the current turn has fully ended (packages/runtime/src/session-manager.ts:1727-1789).
For a mid-turn Bypass→Auto/Explore request, the durable boundary remains bypass throughout that wait. ToolRuntime now correctly rereads the boundary per dispatch (packages/runtime/src/tool-runtime.ts:1326-1352), but every later tool call in that same turn still reads the old unrestricted boundary, and background shell authority is not terminated yet. This window is not usefully bounded in wall-clock time: the same dispatch path explicitly supports long-running installs, builds, training, and subagent loops, and a turn can issue multiple more tools.
The tests currently encode this gap: session-manager.test.ts:5164-5185 asserts the switch remains unsettled and the old boundary remains until turn 1 ends; the seeded sweep at :5498-5527 only verifies turns begun after the switch promise resolves. The per-dispatch test manually flips a fake boundary between calls, but the production transition cannot commit that flip while a run is active.
Please split widening from tightening. Delayed widening is a UX tradeoff, but tightening should establish a dispatch fence and revoke promptly—either stop the live turn or atomically install the narrower boundary so its next dispatch uses it, with the chosen contract also fencing already-running shell/subagent resources. An integrated Bypass→Ask/Explore regression should have the current turn attempt another write-capable/Bash dispatch after the user request but before terminal completion, and assert that it sees the narrower authority or that the turn was stopped. The successor-turn assertions should remain as a separate invariant.
There was a problem hiding this comment.
Additional review pass at exact head 8ba7c4c907ec01e56035b0ee2e2742f1ec5d7417 (MERGEABLE).
The existing [P1] is mechanically correct — confirmed by my own trace, not by reading it.
The tightening path: setPermissionMode → runSessionQueuedQuiescentMutation([sessionId], …) (session-manager.ts), which closes the admission gate immediately but runs the mutation — the durable boundary write included — only after quiescence (runtime-kernel.ts waitForSessionQuiescence loops on hasActiveRuns). So while the current turn is live, store.readExecutionBoundary still returns the old, wider boundary. ToolRuntime does re-read that store per dispatch (the new executionBoundaryDisplayMode derivation), but what it reads has not changed yet — the narrowing takes effect at the first dispatch after turn end, not after the user's request. The queueing machinery is direction-agnostic, so tightening inherits the same deferral as widening: session-manager.test.ts (Auto → Bypass requested mid-turn … the switch commits in the gap as soon as turn 1 settles) encodes this gap as the expected behavior, with expect((await store.readExecutionBoundary(session.id)).kind).toBe('managed') while the turn is still running.
The expensive parts the quiescence protects (backend disposal, descendant shell fencing) justify waiting — but the boundary write itself is a single store mutation, and the per-dispatch reread this PR added is precisely the mechanism that would let an early write take effect at the next tool call. Splitting "commit the narrower boundary now, dispose the old backend at quiescence" would close the window without destabilizing the running turn. Agree with the existing P1's framing: delayed widening is a UX tradeoff, delayed tightening is an authority window.
Checks note: the test check at this head is failure, but the failure is the ASF license-header gate — runtime-kernel-queued-quiescent-mutation.test.ts and tool-runtime-permission-mode.test.ts are missing headers, the job exits before running suites. So no test evidence exists on this head; the header gate failing early means the red X overstates what's known. npm run write:asf-headers should fix it.
No additional findings beyond the existing P1.
简体中文
独立复读了机制,既有 P1 成立:权限收窄请求进入排队静默 mutation,边界写入推迟到当前 turn 结束后的静默期;期间 ToolRuntime 虽然每次派发都重读边界,但读到的还是旧的宽边界。测试把这个窗口编码成了预期行为。另外这个 head 的 test 红是 ASF license header 门禁(两个新测试文件缺头文件注释),测试套件根本没跑——不是测试失败。
|
Thank you for the precise review — the finding is confirmed and fixed in 2127aaa. We took the second contract you offered: atomically install the narrower boundary, the live turn is not stopped. Split of widening and tightening. Widening keeps the inter-turn-gap semantics unchanged: a delayed grant only affects turns that start later, which we agree is a UX tradeoff. Tightening no longer goes through the queued quiescent mutation at all.
Two properties worth naming explicitly:
Regression, per your spec. The mid-turn narrowing tests that previously encoded the gap now assert the new contract: narrowing with a live descendant commits promptly and fences the lineage shells instead of rejecting at commit time. |
f98dc67 to
59ac0d5
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
Reviewed at exact head 59ac0d51571ad1d7bd0bc2b2195a02a0988e978d against base bfba2536132b0c4024a32dfd3804d2bfa40ce9ea. I found one P1 plus two blocking gates.
[P1] A mixed tightening update can publish a read-only configuration while the live backend still composes broader authority
session.configuration.update is a full configuration operation, not a permission-only operation. Desktop accepts Partial<SessionConfiguration> and expands it into the full record, but transitionSessionConfiguration chooses the commit strategy only from the requested permissionMode. On the tightening path, commitTighteningTransition therefore commits the entire new configuration while the current backend remains alive and is only invalidated later.
A concrete valid update is a live Session changing from agent + bypass to plan + ask in one request. The durable record and execution boundary become plan + ask, but the active ToolRuntime still carries the backend-frozen agent collaboration mode. Its next dispatch combines that old mode with the newly-read ask boundary, rather than Plan's required explore, so a write-capable tool can still be admitted after the stored configuration already says the Session is read-only Plan.
The smallest safe contract is: while a run is live, if a tightening request also changes any backend-composed non-permission field, reject the atomic update as session_busy. Do not partially commit it. If a split transition is desired instead, that needs its own atomic contract. Please add one Host-operation regression for bypass/agent -> ask/plan that attempts another write-capable dispatch in the same Turn and proves it cannot receive writable authority.
Blocking CI: this head does not compile, so none of the claimed suites ran
All three hosted checks are red. The CI test job stops in TypeScript compilation because these two new fixtures still specify the removed SessionHeader.lastUsedAt field:
runtime-kernel-queued-quiescent-mutation.test.ts:299tool-runtime-permission-mode.test.ts:174
Both fail with TS2353. package and windows_recovery are also red on this exact head. This is mechanical to fix, but local test counts from before the rebase are not evidence for the current commit.
Blocking simplification: remove the global boundary-revision backend watcher
The new activation-time watcher treats any boundary revision change as evidence that a backend generation is stale. That revision is not a backend-composition fingerprint: a normal approved sandbox expansion increments it even though expansions are intentionally consumed live per dispatch and change neither model nor backend-composed Session configuration. The watcher therefore adds a durable read on every activation and needlessly rebuilds backend/transport/composer state after valid expansions, while masking missing ownership behind an over-broad proxy fact.
Configuration transitions already own backend disposal/invalidation. Remove boundaryRevision, readBoundaryRevision, resolveReusableGeneration, and the two forced-revision tests; fix any writer that bypasses the transition authority instead of watching unrelated state. Also remove the fixed-seed 100-iteration sweep: its behaviors are already covered by direct deterministic gate tests. Keep the high-value claim/run waiting, successor admission, deadlock, immediate-tightening/current-dispatch, shell-lineage, and mixed-configuration regressions.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
59ac0d5 to
4792af5
Compare
|
All three findings are addressed — each as its own commit on top of a fresh rebase onto current main, every commit carrying the required trailer. [P1] Mixed tightening publishing a read-only configuration the live backend cannot enforceConfirmed — this was a real authorization gap in the immediate-tightening path, and the sharpest way to state the root cause is exactly yours: the fast path's safety argument ("the fresh boundary reaches the next dispatch") only holds for permission-only requests, while Fixed in 81ebbf2 with the smallest safe contract you specified: a tightening that also changes any backend-composed field (backend, connection, model, thinking level, collaboration mode, orchestration mode) rejects The regression drives your scenario end to end: Blocking CI: compilationFixed in d63f8ca — Blocking simplification: boundary-revision watcherRemoved in 4792af5 — One commit-per-finding for review convenience: |
|
CI filed, could you take a look? |
4597465 to
ba731ea
Compare
|
Thanks for your attention, fixed the problem. |
Astro-Han
left a comment
There was a problem hiding this comment.
The earlier review-level P1 about delaying revocation remains relevant to the real shell teardown ordering and is not repeated here. Four independent exact-head authority/recovery gaps remain in the revised transition path. Review analysis was assisted by Codex and an independent @Reviewer agent. Astro-Han verified the exact-head state transitions, production lineage/backend ownership, and severity before publication and owns this review.
ba731ea to
99b40d3
Compare
|
All four findings are fixed — one commit each, on top of a fresh rebase onto current main, every commit carrying the trailer. [P1·①] Structural classification for the tightening/widening split — 1a1120fConfirmed. Classification now uses display-mode authority levels ( [P1·②]
|
99b40d3 to
ffb913a
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at ffb913a97c. All four previous findings check out, sweep and revision watcher gone, CI green.
[P1] The lineage rollback hands write authority back
commitTighteningTransition — when constraining a descendant fails, the catch restores every descendant it had already narrowed.
That is rollback reflex, but commit() already made the parent's narrower boundary durable (your own comment says so). Atomicity was gone one step earlier, so restoring buys no consistency — it only widens authority that was correctly revoked:
- without:
parent=ask, child1=ask, child2=bypass - with:
parent=ask, child1=bypass, child2=bypass
Narrowing is idempotent and monotone; keeping what was achieved is never worse.
Reachable normally: a child with a live run gets an approved expansion mid-tighten, so the re-read boundary is no longer contained and constrainDescendantBoundary throws.
The retry cannot converge either — commit() wrote header and boundary together, so setPermissionMode short-circuits on previous.permissionMode === mode && executionBoundaryMatchesPermissionMode(...) and returns success. The user sees "already Auto" while a child keeps writing under Bypass.
Fix: delete restoreDescendantBoundary and its call; set the quarantine on constrain failure and have the short-circuit consult hasExecutionQuarantine.
Two notes
constrainDescendantBoundarynever callsupdateCachedHeaderunlikesetPermissionMode; the kernel's cached child header stays stale until the invalidation rebuilds.- Title still says "before the next turn's first tool call" — that is only the widening half now.
Direction: should widening commit immediately too?
Tightening already proves the mechanism — commit on the tail, defer disposal, per-dispatch reads pick it up. Widening is the safer direction, and mixed updates already have an answer in the file (changesBackendComposition && hasActiveRuns → session_busy).
The asymmetry looks inherited, not designed: both directions used to wait, tightening changed because waiting was an authority window, widening stayed. "A delayed grant is a UX tradeoff" explains why the delay is tolerable, not why it is better — and pressing "stop asking me" and still being asked for twenty minutes reads as a broken button.
If widening commits immediately, these lose their only consumer:
runSessionQueuedQuiescentMutationand the kernel's third mutation semantics- the admission gate;
admissionBarrierreturns to the tail claimSeq/claimSequence/ the frontiersessionQuiescenceWaiters,waitForSessionQuiescence,hasUnsettledExecutionClaims,isSessionExecuting,wakeSessionQuiescenceWaiters+ 3 call sites- the widening/tightening split and
PERMISSION_AUTHORITY_LEVELS— after commit, direction is measurable with the existingexecutionBoundaryContainsinstead of a second ordering over mode names runtime-kernel-queued-quiescent-mutation.test.tsentirely
Every new concurrency primitive this PR adds to the kernel exists only for widening — tightening uses the pre-existing runSessionAdmissionMutation. That machinery is also what needs the deadlock argument and the 389-line interleaving file.
Price, worth stating in the notes: "change model + widen" goes from waiting a turn to failing fast with a retry.
Not blocking and not mine to decide, but cheaper here than after the machinery ships.
Review assistance: Claude (Claude Code) traced the transition paths, kernel claim/gate state, and retry short-circuits at this head; I verified the state transitions, the constrain-failure reachability, and the consumer analysis, and own this review.
ffb913a to
f81ed60
Compare
Direction: yes — widening should commit immediately tooAgreed. Having carried this question through the P1 rounds, I want to lay out the full case, state the price honestly, and propose how to land it. 1. The asymmetry is inherited, not chosenThe series' own history shows this directly: the first fix queued both directions behind live execution. Tightening was then moved to immediate commit because waiting was an authority window ("next dispatch, not the next turn"). Widening simply stayed on the older queued design. No one ever argued the wait was better — the in-code comment ("a delayed grant is a UX tradeoff, not a hazard") explains why the delay is tolerable, not why it is preferable. 2. The mechanism already covers both directionsThis PR consolidated both halves of the permission decision onto the live read model: tools derive 3. The issue's contract is a floor, not a ceiling#3349 states the expected behavior as "a permission change is observed by the next turn that starts after it, before that turn's first tool call." That is the minimal guarantee whose absence constituted the bug. Immediate observability — the running turn's subsequent dispatches seeing the change too — is a strict superset. Nothing in the issue or the thread asks the running turn to be shielded from a grant the user just requested; the root-cause section in fact treats the live/frozen read split as the defect to remove. 4. The UX cost of waiting is concrete, and it is this product's own premiseUnder the queued design a grant lands when the current turn settles — the admission gate holds successor admissions back, so the wait equals the remainder of the current turn. Long agentic turns are exactly the scenario this issue was filed about. A user who confirms "Bypass — stop asking me" and then keeps answering approval prompts for the rest of a twenty-minute turn reads that as a broken button, not a safety property. 5. What the machinery costsEverything the queued path added to the kernel serves only widening — tightening runs on the pre-existing
Keeping it means keeping the deadlock-freedom argument current and carrying that test surface indefinitely, for the sole benefit of delaying a grant the user has already confirmed. 6. The price, stated honestly"Change model + widen" mixed updates go from waiting a turn to failing fast with 7. Consistency with what this PR already establishedTightening already produces turns that run under mixed authority (dispatch N under Bypass, dispatch N+1 under Ask), and that semantics passed review — for the dangerous direction. There is no safety argument for giving the safe direction stricter timing. One uniform semantic — "a switch lands on the next dispatch" — also dissolves the small corner we currently document, where a widening-to-non-bypass request during lineage repair still routes through the immediate path. 8. Relationship to #3347#3347 stages model/thinking/permission together for next-turn application and is currently on hold. Two notes:
And the cost instinct that paused #3347's 716-line staging machinery — "what would change my mind is a concrete case where waiting actually cost you something" — is exactly the standard the widening wait fails: the button in §4 is that case. |
|
maintainer最新的一条code review意见中有一条提议,即可以使放宽权限(ask->bypass)在turn内完成,这样代码复杂性会减少很多,也不会带来额外的负面影响。 |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-read at 9e125a7f. Last round's P1 is fixed, CI is green. Nice work on the lineage rounds.
Direction: yes, widen immediately too. I looked for a reason the grant has to wait and found none. A wider boundary can only under-grant inside the live turn, since every frozen consumer (tool-runtime.ts:1432, the plan prompt) fails closed against it. Descendants need nothing, because their admission check is executionBoundaryContains(parent, child) and a wider parent only makes that easier. Desktop never sees the mixed-update price: the picker sends a permission-only patch, so changesBackendComposition is false. And #3347 is a different seam, so nothing is lost for it.
Correcting my consumer list: runSessionQuiescentMutation, runSessionAdmissionMutation, admissionBarrier and the busy error all have other callers and stay. What goes is the gate half (sessionAdmissionGates, admissionBarrierFor, claimSeq and the frontier, the quiescence waiters, runSessionQueuedQuiescentMutation, widensExecutionAuthority), roughly 220 production lines plus the queued-path tests. narrowsExecutionAuthority and the level table stay too: executionBoundaryContains needs two boundaries and this site has one mode, and shell fencing and descendant constraint must still run only when narrowing. The !shellRuns → operation_unavailable guard in commitTighteningTransition has to move inside the fencing branch once widening shares that path.
Tests the merged path should keep proving: a mid-turn ask→bypass is seen by the next Bash dispatch, an already-running shell stays sandboxed and is not killed, a grant does not touch descendants, a mixed widening with a live run fails session_busy, and the idle successor-turn case from #3349 stays.
Please take the P1 and the lineage P2s below in the same round as the deletion, so the PR lands as one state.
Rebase: #3749 moved the settings actions to features/session-settings/use-session-setting-intent.ts. Drop the currentMode === mode short-circuit there, gate the bypass confirm on currentMode !== 'bypass', and drop 9e125a7f entirely; the ratchet it worked around is gone.
Evidence: static read against main 6c632b13, test:dist green for core, runtime and runtime-host, lineage findings confirmed with throwaway probes. The P1 is derived, not reproduced.
AI-assisted review: drafted with Maka; I verified the consumer list, the P1 read site, the lineage reachability and the rebase target myself.
简体中文
方向确认:放宽也立即提交。删除面比我上次列的小,narrowsExecutionAuthority 要留。下面的 P1 和两条 lineage P2 请和删机制同一轮修。Rebase 落到 use-session-setting-intent.ts,9e125a7f 可以丢掉。
| // controllable and derives no mode — the last known header mode is the | ||
| // best available answer there. | ||
| const boundaryMode = executionBoundaryDisplayMode(executionBoundary); | ||
| const permissionMode = |
There was a problem hiding this comment.
[P1 · ①] This derivation is right, but L1432 and L1461 still read the frozen header.permissionMode. Tightening now keeps the backend alive, so after a mid-turn Ask → Explore those sites still see 'ask' while the boundary is read-only, and client-capability tools (which run outside the sandbox) get admitted. Use the mode derived here at both sites; the gate can then be stated on the boundary alone.
| const fencedSessionIds = [sessionId, ...initialDescendants]; | ||
| const narrows = narrowsExecutionAuthority(initialBoundary, nextPermissionMode); | ||
| const widens = widensExecutionAuthority(initialBoundary, nextPermissionMode); | ||
| const unconstrained = await this.linkedLineageEscapes(sessionId, initialBoundary); |
There was a problem hiding this comment.
[P2 · ①] One approved expansion in a child makes linkedLineageEscapes(parent) true forever, since nothing ties the child's record to the parent. From then on every parent update, including model-only, takes the tightening branch: shells killed, child projected down to explore, the approved grant silently revoked, and with a live turn the model switch rejects session_busy right after #3749 made it instant. Probe confirmed. unconstrained should classify permission requests only.
| const descendantBoundaries = new Map<string, ExecutionBoundary>(); | ||
| for (const descendantSessionId of descendantSessionIds) { | ||
| for (const descendantSessionId of descendants) { | ||
| descendantBoundaries.set( |
There was a problem hiding this comment.
[P2 · ②] Descendant boundaries are read here, before commit(), and nothing serialises the child. A child setPermissionMode(bypass) in that window is judged already contained, skipped, and L1861 resumes its shells on the stale value. Re-read inside constrainDescendantBoundary.
| // disposal quarantine that is not a grant, still tightens — those repairs | ||
| // live only there. | ||
| const nextContainsLineage = nextPermissionMode === 'bypass' || !unconstrained; | ||
| if (narrows || (unconstrained && !nextContainsLineage) || (quarantined && !widens)) { |
There was a problem hiding this comment.
[P2 · ②, dissolves with the direction] nextContainsLineage is just === 'bypass', so explore → ask during lineage repair still lands on the live turn (probe: boundary flipped to ask mid-stream). After the merge this should collapse to narrows versus everything else.
| const previous = await this.deps.store.readHeader(sessionId); | ||
| const boundary = await this.deps.store.readExecutionBoundary(sessionId); | ||
| const leavingDeepResearch = isDeepResearchSession(previous.labels) && mode !== 'explore'; | ||
| if ( |
There was a problem hiding this comment.
[P3] Nothing in production calls SessionManager.setPermissionMode; Desktop and CLI both go through catalog updateConfiguration. This predicate is a copy of the coordinator's. Delete the method or have the coordinator call it, but keep one.
| // already matches would otherwise hide the leftover child. | ||
| const quarantined = this.#manager.hasExecutionQuarantine?.(input.sessionId) ?? false; | ||
| const unconstrainedLineage = | ||
| (await this.#manager.hasUnconstrainedLinkedLineage?.(input.sessionId)) ?? false; |
There was a problem hiding this comment.
[P2 · ①] hasUnconstrainedLinkedLineage is a full store.list() and runs on every update before the if, then again in commitExecutionResourceTransition, up to four scans per picker click. Move it behind sessionConfigurationMatches && boundaryMatchesConfiguration, and use the store's subagentParentSessionId filter.
9e125a7 to
9c8905b
Compare
Direction round: how each finding was addressedLanded as Direction — grants commit immediately; the machinery is deletedBoth directions now share one commit path on the admission-mutation tail. Fencing (shell termination) and descendant constraining run only when the change actually narrows someone:
A bypass target contains every local descendant, so a grant constrains no one, kills no shell, and touches nothing. One deliberate reading of "only when narrowing": the gate is will someone be constrained, not Deleted with the queue (~220 production lines, net −624 with tests): P1 · client-capability gate on the live boundaryBoth sites (the admission check and the P2 · lineage classification by permission intent
P2 · fresh read at constraint time
P2 · the
|
Astro-Han
left a comment
There was a problem hiding this comment.
@chinawch007 The property is met and the lineage rounds were careful work. But I want to call the shape this round instead of running a fifth round of line comments, because part of the size is my fault.
Where the lines go. Of roughly 700 effective production lines, about 105 serve #3349: the live boundary read at dispatch, and lifting the busy refusal. About 310 serve the narrowing direction (lineage re-enumeration, descendant projection, spawn and settlement fencing), plus about 73 for the capability revalidation those forced, plus the CAS, the mixed-update check and the desktop confirm. The issue asked for none of it.
Why it grew. main refuses a permission change while the session is not quiescent. That refusal is not a gap, it is load-bearing: quiescence is exactly what lets a narrowing terminate lineage shells and settle pending boundary requests with no extra machinery. This PR moves narrowing off that guarantee, so a mid-turn revocation becomes possible, and then has to rebuild by hand everything quiescence was giving for free. That is the 310 lines, and it is why every round found another case the reconciliation had not anticipated.
My part. The issue body ended with "See the assignee's plan in the comments for a full proposed fix (queued quiescent commit + boundary-derived permissionMode + revision guard)". Those are my words, prescribing an implementation in an issue that should have stated only a property. You built what it asked for. I have rewritten #3349 to state the property and the constraint, and the queue and revision guard are gone from it.
The two directions are not symmetric. A widening grant cannot over-authorize anyone: every consumer holding the older, tighter value fails closed against a wider boundary (tool-runtime.ts capability gate, the plan prompt), and a descendant's admission check executionBoundaryContains(parent, child) only gets easier. Quiescence buys a grant nothing and buys a narrowing everything. The defect is not the refusal, it is that the refusal is applied one direction too wide.
The shape. Fork on narrowsExecutionAuthority: a widening writes the boundary and returns, a narrowing stays on main's path unchanged. Add the live boundary read at dispatch. The queue, the admission gate, the revision guard, the CAS, constrainDescendantBoundary, the lineage escape detection, the spawn and settlement fences, the dispatch-time capability revalidation, and the storage and desktop changes then all leave together.
Four things worth knowing before you start, two of which correct advice I gave earlier:
- Fork inside
commitExecutionBoundaryTransition, notcommitExecutionResourceTransition. The latter also servesrelocateSessionWorkspace, wherenextPermissionModeoften equals the current mode, sonarrowsExecutionAuthorityreturns false and a model, orchestration or cwd change would slip past a fence that is not protecting the permission boundary at all. Three existing tests catch this (session-manager.test.ts:3834,:4011,:4058). Forking one level down leavescommitExecutionResourceTransitionand the narrowing path at zero diff. - Use
runtimeKernel.invalidateBackend, notdisposeBackend. Invalidation already means "dispose now if idle, otherwise hand it to the next activation".AiSdkBackend.dispose()callsstop('user_stop')whenactiveTurns.size > 0, so disposing on a grant that lands mid-turn kills the turn the user is watching. - Keep the plan overlay on the derived mode. Plan mode writes only the header's
permissionMode;setCollaborationModenever touches the boundary. Deriving purely from the boundary turns plan+managed fromexploreintoaskand opens the client-capability gate. Real regression, so the composer's rule has to be shared rather than dropped. resolveCollaborationPermissionModedoes have to move to@maka/core, sincepackages/runtimecannot reachruntime-host. That part of your change stands.
I wrote this shape against current main rather than assert its size: 4 files in 3 packages, +65/−16 production and +94/−3 tests, covering the plain-session case, a mid-turn grant, and a narrowing that still rejects session_busy while busy. @maka/runtime test:dist 3177 tests, 0 failures. I am not going to push it over yours; the number is only there to show the cost is the shape, not your care.
Two things to handle separately:
constrainDescendantBoundaryfixes something real. Onmaina parent narrowing leaves each descendant's durable boundary at bypass, so after a restart the child still dispatches unsandboxed. That predates this PR and deserves its own issue. Allowing mid-turn narrowing raises its reachability, which is one more reason not to allow it.- The
waiting_for_userrefusal stays. Flipping to Bypass and approving the pending request are different acts.
CI: the red test job is the CLI production dependency audit, not your code. main cleared it in #4578, so a rebase turns it green. You are 29 commits behind.
Evidence boundary: static read of 9baef203 against main 9225f80b; the minimal shape implemented and run on a scratch branch off main; the line accounting measured with git diff --numstat at each round's head, not estimated.
AI-assisted review: drafted with Maka; I verified the line accounting, the fork point, the dispose behaviour and the plan-mode regression myself.
简体中文
属性达成了,lineage 那几轮做得很细。但这轮我想谈形状,不再逐行提意见,因为体量这件事我自己也有责任。
行数去了哪。 大约 700 行有效生产代码里,只有约 105 行在修 #3349:派发时读实时 boundary,以及放开忙时的拒绝。约 310 行是在处理收紧方向(重新列举 lineage、把后代 boundary 压回来、spawn 和 settlement 的围栏),再加约 73 行是它们逼出来的 capability 重校验,另外还有 CAS、混合更新检查和 desktop 确认框。这些 issue 都没有要求。
为什么会涨。 main 在会话没静下来时拒绝改权限。这不是漏掉的功能,而是撑住整个设计的前提:正因为没有活着的 turn,收紧才能直接杀掉整条 lineage 的 shell、结清等待确认的请求,不需要任何额外机制。这个 PR 让收紧不再依赖这个前提,turn 跑到一半也能收权,于是原本白拿的保证全部要自己手写一遍。那就是那 310 行,也是为什么每一轮评审都能发现一种之前没考虑到的情况。
我的责任。 issue 正文最后一句是 "See the assignee's plan in the comments for a full proposed fix (queued quiescent commit + boundary-derived permissionMode + revision guard)",是我写的。issue 本该只说清要什么属性,我却把实现方案也写了进去,队列和 revision guard 都在里面。你是照着 issue 做的。我已经重写了 #3349,只留属性和约束,那两样都删掉了。
放宽和收紧不对称。 放宽不可能让谁越权:所有还拿着旧的、更严的值的地方,遇到更宽的 boundary 都是往严的方向判(tool-runtime.ts 的 capability 准入、plan prompt),子会话的准入条件 executionBoundaryContains(parent, child) 也只会更容易通过。所以「等会话静下来」这个前提,对放宽毫无用处,对收紧却是全部。问题不在于那条拒绝,而在于它多管了一个方向。
建议的形状。 在 narrowsExecutionAuthority 上分成两条路:放宽就直接写 boundary 然后返回;收紧完全走 main 原来的路,一行不改。再加上派发时读实时 boundary。这样队列、admission gate、revision guard、CAS、constrainDescendantBoundary、lineage 逃逸检测、spawn 和 settlement 围栏、派发时的 capability 重校验,以及 storage 和 desktop 的改动,就可以一起删掉。
动手前有四点值得先知道,其中两点是在纠正我之前给的建议:
- 分叉点要放在
commitExecutionBoundaryTransition里,不是commitExecutionResourceTransition。 后者还服务relocateSessionWorkspace,那里的nextPermissionMode经常和当前模式相同,narrowsExecutionAuthority会返回 false,于是换模型、换 orchestration、换 cwd 都会绕过一道本来就不是在保护权限边界的检查。有三个现成的测试会挂(session-manager.test.ts:3834、:4011、:4058)。往下一层分叉的话,commitExecutionResourceTransition和整条收紧路径可以完全不动。 - 用
runtimeKernel.invalidateBackend,别用disposeBackend。 invalidate 本身就是「空闲就现在销毁,忙就留给下次激活时处理」。而AiSdkBackend.dispose()在activeTurns.size > 0时会调stop('user_stop'),所以放宽如果正好落在 turn 中间,dispose 会把用户正在看的那个 turn 直接掐掉。 - 推导出来的模式要保留 plan 的覆盖。 plan 模式只改 header 里的
permissionMode,setCollaborationMode从来不动 boundary。如果完全从 boundary 推导,plan + managed 就会从explore变成ask,把 client-capability 的准入放开。这是真实的回归,所以 composer 那条规则要共用,不能丢。 resolveCollaborationPermissionMode确实得搬到@maka/core,因为packages/runtime引用不到runtime-host。你这部分改得对。
体量我没有停在嘴上说,而是照这个形状在当前 main 上写了一遍:3 个包 4 个文件,生产代码 +65/−16,测试 +94/−3,覆盖普通会话的场景、turn 中途放宽,以及忙的时候收紧仍然报 session_busy。@maka/runtime test:dist 3177 个测试全过。我不会把我这版盖到你的上面,写出这个数字只是想说明,一直在付代价的是形状,不是你的用心。
另外两件事分开做:
constrainDescendantBoundary修的是真问题。main上父会话收紧之后,每个子会话存下来的 boundary 还停在 bypass,重启后子会话照样不进沙箱。这个问题在本 PR 之前就存在,值得单独开一个 issue。允许 turn 中途收紧反而让它更容易被撞到,这也是不该允许的一个理由。waiting_for_user时的拒绝保持不变。切到 Bypass 和批准那条正在等确认的请求,是两件不同的事。
CI:红的 test job 是 CLI 生产依赖审计,和你的代码无关。main 已经在 #4578 修好,rebase 之后就绿了。你现在落后 29 个提交。
|
Pushed the shape I described as a reference branch:
+65/−16 production, +94/−3 tests, 简体中文把上面说的形状推成了一个参考分支 生产 +65/−16,测试 +94/−3, |
…pache#3349) The header carries the permission mode the backend was composed with, and a backend generation outlives many turns. A permission change does not recompose it, so `ctx.permissionMode` stayed at whatever the mode was when the backend was built while the boundary the same dispatch reads for sandboxing had already moved. The picker said Bypass, Bash stayed sandboxed, and approvals kept prompting. The boundary is the authority, so the mode is read off the boundary this dispatch is about to run against. The header answers only for an externally isolated boundary, which projects to no local mode at all. Plan mode writes only the header, never the boundary, so the collaboration overlay still has to apply on top; deriving purely from the boundary would turn plan+managed from explore into ask and open the client-capability gate. That rule now lives in @maka/core because both the composer and tool dispatch have to reach the same answer, and packages/runtime cannot reach runtime-host. Generated-by: OpenAI Codex
… quiescence (apache#3349) A permission change was refused whenever the Session was not quiescent. That requirement is load-bearing for a narrowing: quiescence is what lets it terminate lineage shells and settle pending boundary requests with no extra machinery. It buys a widening nothing. Every consumer holding the older, tighter value fails closed against a wider boundary, and a descendant's admission check only gets easier, so a grant cannot over-authorize anyone. The refusal was applied one direction too wide, and under a Goal the continuation holds a claim near-continuously, so the user's own grant could not land at all. A widening now writes the boundary and returns; a narrowing keeps the existing path unchanged. The fork sits in commitExecutionBoundaryTransition rather than commitExecutionResourceTransition, which also serves relocateSessionWorkspace where the next mode frequently equals the current one: forking there would let a model, orchestration or cwd change slip past a fence that is not protecting the permission boundary. Backend refresh moves to invalidateBackend, which disposes now when the Session is idle and otherwise defers to the next activation. Disposing directly would call stop('user_stop') on a live Turn and kill the Turn the user is watching. setExecutionBoundaryKind gets the same treatment, so both entry points answer alike. Generated-by: OpenAI Codex
9baef20 to
1c7894c
Compare
|
Thanks — I understand the shape you were describing now. The key distinction is that permission widening should be allowed to take effect within the current turn, while permission narrowing must retain the existing quiescence requirement and transition semantics. I have rebased the branch onto the latest
I also removed the broader queueing, lineage, fencing, revalidation, persistence, and desktop-layer changes from the earlier implementation, since they are not required for this behavior. The resulting branch contains only these two commits on top of the latest Validation completed:
Thanks for spelling out the intended fork and providing the reference implementation — it made the required boundary between widening and narrowing clear. |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-read at 1c7894ca. The rewrite landed the shape from the last round: the fork sits inside commitExecutionBoundaryTransition so commitExecutionResourceTransition and the whole narrowing path are at zero diff, it uses invalidateBackend, the plan overlay survives, and resolveCollaborationPermissionMode moved to @maka/core. All four points check out at this head. The kernel queue, the admission gate, the revision guard, the CAS, the lineage machinery and the sweep are gone, and with them 8 of the 10 open inline threads. tool-runtime.ts L1432 and L1461 (thread on 9e125a7f) are fixed: both read the derived mode now.
Two things still to do, one of them the P3 I filed last round that has become the blocker.
P1: the widening fast path has no production caller, so the session_busy half of #3349 is unfixed for users. Desktop's sessions:setPermissionMode IPC calls updateConfiguration (apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts:181-184), which is session.configuration.update -> session-catalog-coordinator.ts:608 -> SessionManager.transitionSessionConfiguration, and that calls commitExecutionResourceTransition directly at session-manager.ts:1090, one level above your fork. The CLI does the same through runtime-host-session-driver.ts:645-654. commitExecutionBoundaryTransition has exactly two callers: SessionManager.setPermissionMode, which production still never calls (my earlier P3), and setExecutionBoundaryKind, whose only production caller is the one-shot run-command-core.ts:350. So a picker switch during a live Turn still hits the hasActiveRuns check at session-manager.ts:1691 and gets session_busy, and the new session-manager.test.ts:4493 assertion proves the new path only through a method nobody calls.
Either route a permission-only patch from transitionSessionConfiguration into the same fork, or have the catalog coordinator use commitExecutionBoundaryTransition for that case. Whichever way, the regression has to be at the Host operation layer (session.configuration.update), not on SessionManager.setPermissionMode, and that method should then either be on the production path or be deleted. One authority, not two.
P2 is inline on tool-runtime.ts.
P3: execution-model-composition.ts:539 re-exports resolveCollaborationPermissionMode only so runtime-host/src/__tests__/execution-model-composition.test.ts:99 keeps its import. Point the test at @maka/core/collaboration and drop the line, otherwise the move leaves two import paths for one rule.
P3: the PR body still describes runSessionQueuedQuiescentMutation, the admission gate, the revision guard and the 100-iteration sweep, none of which exist at this head. It needs a full rewrite before squash, and the accounting is worth redoing honestly: ToolRuntime already read the boundary live on main, and builtin-tools.ts:775-779 takes the Bash sandbox profile from boundary.profile, so "the picker said Bypass while Bash stayed sandboxed" was not the live defect. What the dispatch change actually moves is ctx.permissionMode, consumed at the client capability gate, client-capability-coordinator.ts:1061 and mcp-tools.ts:131.
Rebase: 43 commits behind.
Evidence boundary: static read of 1c7894ca against main cd4aa3d8; caller chains and the expansion path traced in source; not reproduced at runtime.
简体中文
形状按上轮建议落地了,分叉点、invalidateBackend、plan 覆盖、resolveCollaborationPermissionMode 搬家四条都对,收窄路径零 diff,10 条旧 inline 里 8 条随代码删除而消解,tool-runtime 那条 P1 已修。
剩两件。P1:放宽快路生产上够不到。Desktop 和 CLI 的 picker 都走 session.configuration.update -> transitionSessionConfiguration -> commitExecutionResourceTransition,绕过了你分叉的那一层;commitExecutionBoundaryTransition 只有 setPermissionMode(生产零调用)和 setExecutionBoundaryKind(只有 CLI 启动时一次)两个调用者。所以 turn 跑着切 Bypass 仍然 session_busy,新测试只证明了一个没人调用的方法。回归请打在 Host operation 层。
P2 在 tool-runtime.ts 行内。另有两条 P3:兼容再导出,以及正文仍在描述已删除的机制,squash 前要重写。
| ? CLIENT_CAPABILITY_PREPARATION_MESSAGE | ||
| : clientCapabilityBoundary.kind !== 'bypass' && this.input.header.permissionMode !== 'ask' | ||
| : clientCapabilityBoundary.kind !== 'bypass' && | ||
| this.livePermissionMode(clientCapabilityBoundary) !== 'ask' |
There was a problem hiding this comment.
P2 (reach: normal path): deriving the mode from the boundary lets one approved sandbox expansion promote an Explore Session to ask and open this gate.
executionBoundaryDisplayMode (core/src/sandbox-boundary.ts:217-227) decides read-only structurally via isReadOnlyPermissionProfile (permission-profile.ts:148-154), and applySandboxBoundaryExpansion (sandbox-boundary.ts:384-408) adds a write entry or sets network: enabled, which sqlite-session-metadata-store.ts:775-779 writes as the durable managed boundary. So after a user approves one specific write in an Explore (non-plan) Session, livePermissionMode returns 'ask', this admission check flips from always refusing to admitting, and client-capability-coordinator.ts:1061 agrees. Capabilities whose managedClientCapabilityGrantTarget is undefined (:1070-1075) then run with no second approval at all. The user granted a path, and got the client capability channel.
The same Session is also incoherent in the picker: setPermissionMode's short circuit at session-manager.ts:1571-1577 uses the name based executionBoundaryMatchesPermissionMode, and an expanded profile is still named read-only, so re-selecting Explore returns success and changes nothing while dispatch keeps reading ask.
This is the name based versus structural mismatch from my ba731ea4 thread, landing on the other side now. I graded it P2 rather than P1 because the common capability still needs a per-session grant and nothing durable changes, but the mirror of it was a P1 last round, so argue me up if you disagree.
Smallest fix: do not let the boundary widen the mode on its own. Take the boundary's mode when it is bypass, and otherwise keep the Session's current permission mode, read live rather than reconstructed. The boundary does not carry which mode the user selected, so it cannot be the authority for that fact.
Summary
Fixes #3349
A permission switch (Auto→Bypass) was not observed by the next turn. Under Goal continuation the switch was rejected with
session_busy— the quiescent mutation bailed eagerly whenever any execution claim existed, and claims are near-continuous while a Goal admits successor turns back to back. When a switch did commit mid-turn, tools still acted on apermissionModefrozen at backend build time, so the picker said Bypass while Bash stayed sandboxed and approvals kept prompting. The issue asks for one property in any session, not just Goals: a permission change is observed by the next turn that starts after it, before that turn's first tool call.What changed
Kernel — execution serialization:
runSessionQueuedQuiescentMutation: a config change closes a per-session admission gate and waits for quiescence — every claim that predates the request has settled and no run is active — before its operation joins the mutation tail. The gate (not the tail) is what new claims observe, so admission mutations a running turn depends on — graph operator provisioning — still pass; waiting never holds a resource the waited-on execution needs, which keeps the queue deadlock-free. Claims only cover admission (a turn's claim settles at run bind), so runs are watched throughhasActiveRuns; a run registers before its claim settles, leaving no instant where an in-flight admission is invisible.setPermissionMode,setExecutionBoundaryKindandtransitionSessionConfigurationroute through it. The eagerhasActiveRunsrejections are gone — quiescence is now the kernel's single authority. The wait is scoped to the primary session; descendant activity is rejected at commit time (session_busy) instead of waited on, a truthful failure rather than a potential hang.waiting_for_userstill rejects, now also when it appears mid-queue. Behavior change: a switch during an active turn waits (bounded by one turn) instead of rejecting.Read model — the boundary as the single authority (#1611):
ctx.permissionModeis derived live from the durable boundary at every tool dispatch (executionBoundaryDisplayModeplus the shared plan-mode downgrade), so a committed switch reaches the very next tool call without waiting for a backend rebuild; an external boundary falls back to the last known header mode.resolveCollaborationPermissionModeandexecutionBoundaryMatchesPermissionModemove to core, so the composer (build time) and the tool runtime (dispatch time) share one rule. The matcher now derives structurally: a read-only profile widened by an approved expansion no longer reads as explore, and a custom read-only profile is preserved instead of being reset.ensureActiverebuilds on drift — immediately when idle, or via the invalidation flush once live runs exit. Any write path that skips backend disposal self-heals within one activation.How this meets the issue's stated goals
executionBoundary.kind === 'bypass'andctx.permissionMode === 'bypass'together.session_busy.permissionMode(live derivation),ensureActivereusing a stale generation (revision guard), and the catalog short-circuit that could bless a divergence (consistency check).Verification
Regression tests were written first and verified failing against the pre-change behavior: a gated turn with a mid-turn Auto→Bypass switch (queues, commits in the gap, next turn rebuilt from the committed mode), and the plain no-Goal session from the issue — the successor turn's first tool call sees
executionBoundary.kind === 'bypass'andpermissionMode === 'bypass'together, the reporter's primary case.A seeded interleaving sweep (100 iterations, fixed seed) alternates widening and narrowing switches across idle, mid-turn, and racing-the-release interleavings, asserting every turn started after a switch resolved observes the committed configuration.
External review findings each carry their own regression test: the admission-gate deadlock interleaving, the widened read-only and external-boundary matcher cases, and the interaction-pause rejection.
Not run: the full runtime-host suite and the remaining workspace suites, and manual Desktop verification of the picker.
npm run format:check— clean (8 formatting nits auto-fixed and folded in)npm run lint— 2304 files, no issuesnpm run typechecknpm --workspace @maka/runtime run build— cleannpm --workspace @maka/runtime run test:dist— 3110 tests, 3097 pass, 13 skipRoot cause
Three cooperating defects: the eager bail in
runSessionQuiescentMutation, the build-time freeze ofheader.permissionModeinto the tool context, and a catalog short-circuit that compared only header fields. The fix converges on the boundary as the single read model and moves quiescence authority into the kernel.AI use
Select exactly one:
Tool(s) and scope: ZCode (Z.ai GLM) authored the implementation, tests, and review fixes; the contributor directed the design, reviewed each finding, and made the rebase decisions.
Checklist
Does this PR entail a change in behavior?