refactor(runtime): remove obsolete permission mode compatibility path - #5300
chinawch007 wants to merge 2 commits into
Conversation
Remove SessionManager.setPermissionMode, its legacy-store fallback, and helpers used only by that path. Production Desktop and CLI permission changes continue through the versioned configuration authority. Migrate concurrency, Deep Research cleanup, and pending Interaction tests to transitionSessionConfiguration. Document the paired optional Store capabilities and verify missing capabilities reject without fallback writes. Fixes apache#4795 Generated-by: Codex
5d5e674 to
1d845e4
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Reviewed at head 1d845e4. I re-verified the premise on current main: SessionManager.setPermissionMode has no production caller (Desktop sessions:setPermissionMode and the CLI driver both go through session.configuration.update → transitionSessionConfiguration), and the removed legacy fallback was not a faithful mirror of the authority (no revision fence, no archived check, non-atomic header + boundary writes). So the removal is a real entropy reduction, and transitionSessionConfiguration covers every check the deleted helpers had. Both production paths are unaffected. Lint, format and the compiled session-manager suite (191/191) pass on this head; the migrated tests also pass on main, which matches what the description says.
Two things I'd like to see before this lands, both about the one authority acceptance criterion in #4795:
-
setExecutionBoundaryKindis the same shape as the path this PR deletes.packages/runtime/src/session-manager.ts:1653-1688derives apermissionModeitself and persists it throughstore.setExecutionBoundaryKind(...), which insqlite-session-metadata-store.ts:4889-4899patchespermissionModeandlabelsinto the header with no version fence, and without the deep-research cleanup, archived check or revision check thattransitionSessionConfigurationapplies. It also has zero production callers (the only non-test caller,run-command-core.ts:343, resolves toRuntimeHostRunRuntime, which forwards to the driver's configuration update). The test file now promotes it to a'direct'route, which gives it a longer life. By the issue's own standard I think it should go in the same change, or the PR should say why it can't (I couldn't find a blocker: same file, same policy, same caller count). -
The description says capabilities stay optional, but not why. The reason is checkable and worth writing down:
SessionAuthorityStorealready requiresreadHeaderRecordSnapshotandupdateSessionConfiguration, the only production store implements both, and optionality only serves a handful of test fixtures. Making them required would also touchrelocateSessionWorkspace, which sharesrequireSessionConfigurationStore, so keeping them optional here is the right scope. Saying that answers the review item in #4795 instead of stepping around it.
Line-level notes are inline. One small correction for the description: the migrated tests pass on main unchanged, so the "fail without it" box should stay unticked, which is fine for a pure removal.
AI assistance: I used Claude Code to survey callers and run the ablation; every finding above was checked by me against the code and the test run.
| // Either the configuration revision or the boundary revision can fence | ||
| // the stale request, depending on when its snapshot was observed. | ||
| if ( | ||
| route === 'configuration' && |
There was a problem hiding this comment.
P2. With each request reading its own snapshot inside update(), the two requests are no longer guaranteed to observe the same revision, so the assertion accepts either SessionConfigurationRevisionConflictError or operation_conflict. Those map to different client-visible outcomes at the Host (configurationSuccess(revisionConflict) vs configurationFailure), so the test no longer pins which one a stale request gets. The old 'legacy' route was deterministic. Suggest reading both snapshots before Promise.allSettled so both requests share one expectedRevision, then asserting the revision-conflict branch only. (The comment above about both requests observing Explore also no longer matches the code.)
| for (const capability of missing) { | ||
| Object.defineProperty(store, capability, { value: undefined }); | ||
| } | ||
| store.updateHeader = async () => assert.fail('Must not fall back to header writes'); |
There was a problem hiding this comment.
P3. requireSessionConfigurationStore is a single if (!a || !b), so the three missing-capability cases exercise the same throw; the third is the union of the first two. One case is enough. The two assert.fail probes are also unreachable now: transitionSessionConfiguration throws at its first line before any store write could happen. The valuable part of this block is that fallback writes are forbidden, and one case says that.
| readHeaderRecordSnapshot?(sessionId: string): Promise<VersionedSessionHeader>; | ||
| /** | ||
| * Atomically check the expected revision and commit configuration, execution | ||
| * boundary and the new revision. Requires readHeaderRecordSnapshot. |
There was a problem hiding this comment.
P3. This second paragraph restates the first (capabilities may be absent, no unversioned fallback). The first paragraph is the one that answers #4795; this one can go.
| return headerToSummary(next); | ||
| } | ||
|
|
||
| async setExecutionBoundaryKind( |
There was a problem hiding this comment.
See the top-level comment: this method persists permissionMode outside transitionSessionConfiguration with none of its checks and has no production caller. Same shape as the path this PR removes.
Remove the unused SessionManager boundary setter and its unversioned Store requirement. Migrate descendant revocation and admission checks to configuration authority and remove obsolete direct-route fixtures. Split stale configuration revisions from gated concurrent boundary conflicts, asserting each error precisely. Simplify capability tests and explain Runtime optionality alongside the atomic Store contract. Refs apache#4795 Generated-by: Codex
Overall reviewThanks for checking this. I agree that retaining I migrated the descendant shell-revocation and missing-admission-authority tests to The revised description now explains why the Store capabilities remain optional: production After rebuilding, all 559 affected tests passed. Full lint, format, build, typecheck, and Desktop/UI knip checks also passed. The full workspace test suite and manual Desktop interaction were not run. Codex assisted with the implementation, verification, and drafting these replies; the follow-up commit retains its Concurrent update errorsAgreed that accepting either error obscured the test contract. I split this into two deterministic cases:
One detail I found when implementing the suggestion: pre-reading the configuration snapshot alone does not guarantee a configuration-revision error. Inside the serialized commit, the boundary revision is checked before the second configuration-revision check. The explicit read gate fixes that interleaving, while the separate stale-snapshot case pins the revision-conflict result. No production error ordering or Host mapping changed. Missing Store capabilitiesRemoved the redundant “both missing” case and both Store contract commentsCombined the comments into one contract block. It explains the paired capabilities, why Runtime keeps them optional despite the production authority requiring them, and the absence of a fallback. I retained the atomic expected-revision/configuration/boundary requirement because that is a separate implementation obligation, rather than a restatement of optionality. Remaining boundary setterRemoved |
Summary
Fixes #4795
Remove the unused
SessionManager.setPermissionModewrapper, its legacy-store fallback, and the equally unusedSessionManager.setExecutionBoundaryKindentry point. Both could bypass the versioned configuration authority. Desktop and CLI retain their permission controls throughsession.configuration.update→transitionSessionConfiguration.SessionStore, obsolete helpers, and unused test stubs. Preserve the shared transition policy and Storage transaction implementation.SessionAuthorityStorealready requires both; optionality supports lightweight test stores that do not mutate configuration. Document the paired capability and atomic revision/configuration/boundary contract. Test each missing capability independently, without redundant combined cases or write probes.This removes internal APIs without changing production permission-switching behavior. Same-configuration updates retain their existing no-op semantics. Historical
mode_changewrites were removed in #4879; current boundary logging remains unchanged.Verification
Passed locally at
b844456f0after rebuilding:npm run lint,npm run format:check,npm run build, andnpm run typecheck.knip --workspace apps/desktopandknip --workspace packages/ui, using the installed repository binary.session-manager,runtime-kernel-interaction,session-manager-terminal-ledger, andruntime-event-read-model; Hostsession-catalog-coordinatorandexecution-model-composition; Storagesqlite-session-metadata-store; CLIruntime-host-session-driver; Desktopruntime-host-client-uds. These include the real Host permission regressions for ordinary Turns and active Goal continuations.Not run: the full workspace test suite or manual Desktop interaction. Behavioral tests protect existing semantics; this API removal does not introduce a failing-before behavior fix, so the corresponding checklist item remains unchecked.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the cleanup and review follow-up, migrated and extended tests, ran verification, and drafted this description. Affected commits include
Generated-by: Codex; retain the trailer in the final squash commit.Checklist
Does this PR entail a change in behavior?