You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "chinawch007 (via GitHub)" <gi...@apache.org> on 2026/09/14 12:30:20 UTC
[PR] refactor(runtime): remove obsolete permission mode compatibility path [maka]
chinawch007 opened a new pull request, #5300:
URL: https://github.com/apache/maka/pull/5300
## Summary
Fixes #4795
`SessionManager.setPermissionMode` has no production callers, but its wrapper and legacy-store fallback still expose a competing path for changing persisted permission modes. Remove that internal API, `setPermissionModeWithLegacyStore`, and their two unused helpers. Desktop and CLI retain their existing permission controls through `session.configuration.update` → `transitionSessionConfiguration`.
- Move the legacy concurrency regression, Deep Research label cleanup, overlapping-Turn check, and pending Interaction rejection onto the versioned configuration path. Preserve conflict fencing, unrelated labels, and unchanged configuration/boundary revisions after rejection.
- Keep Runtime Store configuration capabilities optional, but document that both snapshot reads and atomic versioned updates are required to mutate configuration. Replace legacy compatibility coverage with cases proving that missing either or both capabilities returns `operation_unavailable` without fallback writes.
The shared boundary-transition policy remains intact. `mode_change` message writes were already removed in #4879; this cleanup preserves the current boundary log and does not restore obsolete transcript writes.
## Verification
Passed locally after rebuilding:
- `npm run lint`, `npm run format:check`, `npm run build`, and `npm run typecheck`.
- `knip --workspace apps/desktop` and `knip --workspace packages/ui` using the installed repository binary.
- 427 tests passed, 0 failed across the compiled `session-manager`, `session-catalog-coordinator`, `execution-model-composition`, `sqlite-session-metadata-store`, CLI `runtime-host-session-driver`, and Desktop `runtime-host-client-uds` test files. This includes the existing real Host regressions for permission widening during ordinary Turns and active Goal continuations.
Not run: the full workspace test suite or manual Desktop interaction. The migrated behavioral tests preserve existing semantics and are expected to pass before and after this API cleanup; they are not new failing-before bug regressions.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Codex implemented the cleanup, migrated and extended tests, ran verification, and drafted this description. The commit includes `Generated-by: Codex`; retain it in the final squash commit.
## Checklist
- [ ] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): remove obsolete permission mode compatibility path [maka]
Posted by "chinawch007 (via GitHub)" <gi...@apache.org>.
chinawch007 commented on PR #5300:
URL: https://github.com/apache/maka/pull/5300#issuecomment-5679849038
## Overall review
https://github.com/apache/maka/pull/5300#pullrequestreview-5205178216
Thanks for checking this. I agree that retaining `SessionManager.setExecutionBoundaryKind` leaves the same architectural problem in place. The follow-up (`b844456f0`) removes that method as well as the unversioned write requirement from Runtime's `SessionStore`. The shared transition policy and Storage transaction implementation remain intact; Desktop and CLI still use the configuration operation.
I migrated the descendant shell-revocation and missing-admission-authority tests to `transitionSessionConfiguration`, kept the configuration-path read/write/network revocation cases, and removed the obsolete direct-route cases and their unused fixture. The direct-only same-mode Explore reset is no longer an API to preserve; the production configuration path's no-op semantics are unchanged.
The revised description now explains why the Store capabilities remain optional: production `SessionAuthorityStore` already requires both, while lightweight Runtime test stores do not all need configuration mutation. I also left the “fail without it” box unchecked, as discussed.
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 `Generated-by: Codex` trailer.
## Concurrent update errors
https://github.com/apache/maka/pull/5300#discussion_r4011645393
Agreed that accepting either error obscured the test contract. I split this into two deterministic cases:
- Commit a widening, then reuse the original snapshot and strictly assert `SessionConfigurationRevisionConflictError`, including expected/actual revisions and unchanged committed state.
- Use a shared configuration snapshot and gate both initial boundary reads until both requests have observed the same Explore boundary. Then strictly assert that the losing request gets the boundary `operation_conflict`, without stopping the active Turn or revoking its shells. A fresh retry remains blocked as narrowing until the Turn settles.
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 capabilities
https://github.com/apache/maka/pull/5300#discussion_r4011645395
Removed the redundant “both missing” case and both `assert.fail` write probes. I retained the two individual missing-capability cases because they specify that each method is required: checking only one method in a future change should not silently weaken the contract. Each case now only asserts `operation_unavailable` and unchanged header/revision/boundary state.
## Store contract comments
https://github.com/apache/maka/pull/5300#discussion_r4011645398
Combined 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 setter
https://github.com/apache/maka/pull/5300#discussion_r4011645403
Removed `SessionManager.setExecutionBoundaryKind` and its requirement on Runtime's Store contract. The remaining useful tests now exercise `transitionSessionConfiguration`; duplicate direct-route cases and the unused `AtomicBoundaryMemorySessionStore` are gone. Storage's transaction primitive and the shared permission-transition policy remain in place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): remove obsolete permission mode compatibility path [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #5300:
URL: https://github.com/apache/maka/pull/5300
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): remove obsolete permission mode compatibility path [maka]
Posted by "chinawch007 (via GitHub)" <gi...@apache.org>.
chinawch007 commented on PR #5300:
URL: https://github.com/apache/maka/pull/5300#issuecomment-5731111729
Thanks for checking the production callers and the migrated coverage. I've addressed the two test issues in `239da0784` and clarified the Storage scope below:
1. **Preserve `executorId`.** `configurationForHeader` now copies it from the header. A new regression test uses a plugin-executor session with an active Turn reported by the Runtime kernel and verifies that permission widening succeeds, retains the executor identity, and neither stops nor disposes the backend. Removing the field copy makes the test fail with `session_busy`, confirming that it detects the unintended resource-transition path.
2. **Restore the single-authority guard.** `VersionedConfigurationMemorySessionStore.updateHeader` now rejects patches containing `permissionMode`, and `updateHeaderVersioned` goes through the same guard. Only `updateSessionConfiguration` projects the permission mode, boundary, and revision. I also removed the memory store's unused boundary setter and its duplicate header write. Mutation checks confirmed that injecting either ordinary or versioned header permission writes makes the test fail at the guard; disabling the guard lets the ordinary bypass pass again. This checks which entry point performs the write; the existing SQLite transaction/rollback tests continue to cover persistence atomicity.
3. **Keep Storage's public boundary mutation API outside this PR.** One distinction worth preserving: the private `setExecutionBoundaryKindSync` is still called by production `updateSessionConfiguration` inside its transaction. It remains part of the canonical configuration commit implementation. Any follow-up should distinguish that implementation from the publicly exposed boundary setter.
Validation: all 560 tests across the same nine affected test files passed after rebuilding, along with lint, format, full build, typecheck, and both prescribed knip checks. The full workspace test suite and manual Desktop interaction were not run.
AI use: Codex assisted with the implementation, verification, and this reply draft; the follow-up commit includes `Generated-by: Codex`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): remove obsolete permission mode compatibility path [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5300:
URL: https://github.com/apache/maka/pull/5300#discussion_r4011645395
##########
packages/runtime/src/__tests__/session-manager.test.ts:
##########
@@ -5545,29 +5557,63 @@ describe('SessionManager permission mode updates', () => {
}),
);
- const summary = await manager.setPermissionMode(session.id, 'ask');
+ const current = await store.readHeaderRecordSnapshot(session.id);
+ const next = await manager.transitionSessionConfiguration(session.id, {
+ expectedRevision: current.revision,
+ clearConnectionBlock: false,
+ permissionModeOnly: true,
+ configuration: configurationForHeader(current.header, { permissionMode: 'ask' }),
+ });
- assert.strictEqual(summary.permissionMode, 'ask');
- assert.deepStrictEqual(summary.labels, ['kept']);
- assert.deepStrictEqual((await store.readHeader(session.id)).labels, ['kept']);
+ assert.strictEqual(next.header.permissionMode, 'ask');
+ assert.strictEqual(next.revision, current.revision + 1);
+ assert.deepStrictEqual(next.header.labels, ['kept']);
+ const persisted = await store.readHeaderRecordSnapshot(session.id);
+ assert.deepStrictEqual(persisted.header, next.header);
+ assert.strictEqual(persisted.revision, next.revision);
});
- test('temporarily preserves setPermissionMode for legacy SessionStore implementations', async () => {
- const store = new MemorySessionStore();
- const manager = new SessionManager({
- store,
- backends: new BackendRegistry(),
- newId: nextId(),
- now: nextNow(6_100),
- });
- const session = await manager.createSession(makeInput({ permissionMode: 'ask' }));
-
- const summary = await manager.setPermissionMode(session.id, 'bypass');
+ for (const missing of [
+ ['readHeaderRecordSnapshot'],
+ ['updateSessionConfiguration'],
+ ['readHeaderRecordSnapshot', 'updateSessionConfiguration'],
+ ] as const) {
+ test(`configuration changes require Store capabilities: missing ${missing.join(', ')}`, async () => {
+ const store = new VersionedConfigurationMemorySessionStore();
+ const manager = new SessionManager({
+ store,
+ backends: new BackendRegistry(),
+ newId: nextId(),
+ now: nextNow(6_100),
+ });
+ const session = await manager.createSession(makeInput({ permissionMode: 'ask' }));
+ const readSnapshot = store.readHeaderRecordSnapshot.bind(store);
+ const current = await readSnapshot(session.id);
+ const boundary = await store.readExecutionBoundary(session.id);
+ for (const capability of missing) {
+ Object.defineProperty(store, capability, { value: undefined });
+ }
+ store.updateHeader = async () => assert.fail('Must not fall back to header writes');
Review 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.
##########
packages/runtime/src/__tests__/session-manager.test.ts:
##########
@@ -4766,8 +4763,17 @@ describe('SessionManager permission mode updates', () => {
);
const conflict = results[1];
assert.ok(conflict?.status === 'rejected');
- assert.ok(conflict.reason instanceof SessionConfigurationTransitionError);
- assert.strictEqual(conflict.reason.code, 'operation_conflict');
+ // Either the configuration revision or the boundary revision can fence
+ // the stale request, depending on when its snapshot was observed.
+ if (
+ route === 'configuration' &&
Review 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.)
##########
packages/runtime/src/session-manager.ts:
##########
@@ -648,7 +648,17 @@ export interface SessionStore {
patch: SessionHeaderPatch,
expectedRevision: number,
): Promise<VersionedSessionHeader>;
+ /**
+ * Versioned configuration authority requires both this method and
+ * updateSessionConfiguration. Stores may omit these capabilities when they
+ * do not support configuration changes; Runtime rejects those operations
+ * with operation_unavailable rather than falling back to unversioned writes.
+ */
readHeaderRecordSnapshot?(sessionId: string): Promise<VersionedSessionHeader>;
+ /**
+ * Atomically check the expected revision and commit configuration, execution
+ * boundary and the new revision. Requires readHeaderRecordSnapshot.
Review 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.
##########
packages/runtime/src/session-manager.ts:
##########
@@ -1640,64 +1650,6 @@ export class SessionManager {
return this.runtimeKernel.listActiveInteractions?.(sessionId) ?? [];
}
- async setPermissionMode(sessionId: string, mode: PermissionMode): Promise<SessionSummary> {
- const readHeaderRecordSnapshot = this.deps.store.readHeaderRecordSnapshot?.bind(
- this.deps.store,
- );
- if (!readHeaderRecordSnapshot || !this.deps.store.updateSessionConfiguration) {
- // Temporary compatibility bridge for SessionStore embeddings that predate
- // versioned configuration authority. A follow-up PR will shortly remove
- // setPermissionMode and this redundant fallback after callers migrate.
- return this.setPermissionModeWithLegacyStore(sessionId, mode);
- }
- const current = await readHeaderRecordSnapshot(sessionId);
- const next = await this.transitionSessionConfiguration(sessionId, {
- expectedRevision: current.revision,
- clearConnectionBlock: false,
- permissionModeOnly: true,
- configuration: sessionConfigurationWithPermissionMode(current.header, mode),
- });
- return headerToSummary(next.header);
- }
-
- private async setPermissionModeWithLegacyStore(
- sessionId: string,
- mode: PermissionMode,
- ): Promise<SessionSummary> {
- const previous = await this.deps.store.readHeader(sessionId);
- const boundary = await this.deps.store.readExecutionBoundary(sessionId);
- const leavingDeepResearch = isDeepResearchSession(previous.labels) && mode !== 'explore';
- if (
- previous.permissionMode === mode &&
- executionBoundaryMatchesPermissionMode(boundary, mode) &&
- !leavingDeepResearch
- ) {
- return headerToSummary(previous);
- }
-
- const labels = leavingDeepResearch
- ? previous.labels.filter((label) => label !== DEEP_RESEARCH_SESSION_LABEL)
- : previous.labels;
- const kind = mode === 'bypass' ? 'bypass' : 'managed';
- await this.commitExecutionBoundaryTransition(sessionId, boundary, mode, async () => {
- const current = await this.deps.store.readHeader(sessionId);
- if (current.status === 'waiting_for_user') {
- throw new SessionConfigurationTransitionError(
- 'session_busy',
- 'Session has a pending Interaction',
- );
- }
- return () =>
- this.deps.store.setExecutionBoundaryKind(sessionId, kind, {
- permissionMode: mode,
- labels,
- });
- });
- const next = await this.deps.store.readHeader(sessionId);
- this.runtimeKernel.updateCachedHeader(sessionId, next);
- return headerToSummary(next);
- }
-
async setExecutionBoundaryKind(
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org