Skip to content

refactor(app): make auto-accept permissions an app-level setting - #44608

Merged
Hona merged 4 commits into
anomalyco:v2from
Hona:global-auto-accept
Aug 24, 2026
Merged

refactor(app): make auto-accept permissions an app-level setting#44608
Hona merged 4 commits into
anomalyco:v2from
Hona:global-auto-accept

Conversation

@Hona

@Hona Hona commented Aug 24, 2026

Copy link
Copy Markdown
Member

Summary

Auto-accept permissions was scoped per session (and per directory, per server) in the desktop/web app. This PR makes it a single app-level setting: one client-local boolean that applies to every session, tab, project, and server connection at once.

  • The setting is now settings.permissions.autoApprove — a client-local persisted settings slot that already existed but was consumed nowhere. The settings switch, the mod+shift+a command, and the auto-responder all read and write this one value.
  • A new 52-line module (session/requests/auto-approve.ts) is wired into each server connection. It approves new requests from permission.asked events and sweeps already-pending requests per known session directory while the setting is on (covers toggle-on, app startup, and late-synced sessions).

Removed

  • server-permission.tsx: the persisted per-server autoAccept map, storage migrations, enable versioning, TTL-pruned responded cache, and dead API surface (enableConfiguredDirectory, permissionsEnabled, isPermissionAllowAll had no callers)
  • auto-respond.ts + test: session/directory accept keys, parent-lineage inheritance, key relocation
  • usePermission context, ctx.permission on ServerCtx, the storybook mock and its vite alias
  • Dead-parameter cascade: useSessionTabAvatarState(directory), SettingsGeneral.sessionID, DialogSettings.sessionID, createPermissionScopeController, and the new-session directory-inheritance block in composer-adapter.ts

Net: +74 / −679 lines.

Behavior notes

  • The settings switch is now always enabled and no longer depends on the focused session or server.
  • Toggling the setting sweeps pending requests on every connected server, so the e2e assertion that pinned requests to a single server was removed; the rest of the spec is unchanged and passes, including "auto-accept responds for an unfocused server session".
  • Previously persisted per-session auto-accept state is left orphaned and not migrated; the setting starts off, which is the safe default for a scope expansion this large.
  • No i18n changes: the existing English copy is already scope-neutral.

Verification

  • bun typecheck and bun run typecheck:e2e pass in packages/app
  • 42 unit tests pass (bun run test)
  • Both remote-session-settings e2e tests pass

@Hona
Hona requested a review from Brendonovich as a code owner August 24, 2026 05:07
Copilot AI lite review requested due to automatic review settings August 24, 2026 05:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Hona

Hona commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

Addressed in ab7dc5d. Each finding was verified against the code before fixing:

1. Reconnects do not resweep — fixed. Verified the stream never replays: createClientConnection.connect() calls api.event.subscribe() with no cursor or Last-Event-ID, and every reconnect starts fresh at server.connected (packages/client/src/solid/connection.ts:73-89). The sweep effect now tracks sdk.connection.status() and reruns on every connected transition.

2. Unloaded servers are not swept — fixed. Verified data.session.list() is just Object.values(store.session.info) (packages/client/src/solid/data.ts:209), populated only by explicit syncs or events. The sweep now inventories session.active(), which is server-wide — the handler has no Location dependency (packages/server/src/handlers/session.ts:141-147) and delegates to the process-global execution coordinator (packages/core/src/session.ts:870). Unknown IDs are resolved with data.session.sync(id) (fetch-by-ID, data.ts:1322). This inventory is complete because a pending request can only belong to an active execution: Permission.assert wraps Deferred.await with Effect.ensuring(pending.delete(...)) (packages/core/src/permission.ts:251-255), so interruption clears the entry. The one exception is the external session.permission.create API, which creates a detached pending entry via Permission.ask — it has no in-repo caller and its permission.asked event still covers it while connected; a request created that way on an idle session during a disconnect remains the only unswept case.

3. Sweeps discard workspaceID — fixed. Locations now deduplicate on directory + workspaceID and the query passes { directory, workspace: location.workspaceID }. Wire format verified: the server parses location[workspace] (packages/server/src/location.ts:31), the generated client serializes nested query objects as key[child] and skips undefined (packages/client/src/promise/generated/client.ts:1881-1893), and PermissionRequestListInput names the field workspace while SessionInfo.location carries workspaceID.

4. Failed reply invisible — bounded retry added. approve retries up to 2 more times with linear backoff, re-checking disposed/enabled before each retry. After exhaustion the deduplication entry is removed, so any later sweep (reconnect, re-enable) retries the request.

5. Unbounded responded set — capped. Insertion-order eviction above 1000 entries, matching the bound the old implementation used.

Testing gap — covered. remote-session-settings.spec.ts now:

  • test 1: server A has no tab and is never visited, holds a pre-seeded pending request, and the mock session.active reports its sessions running; the toggle on server B's settings must produce exactly one reply: "once" POST to server A (toEqual, not toContainEqual).
  • new test 3: after toggling on and completing an initial sweep, a pending request is seeded while transport.disconnect() severs the stream (never delivered as an event); the assertion requires the reconnect sweep alone to reply to it.

All 3 e2e tests pass, plus bun typecheck, typecheck:e2e, and the 42 unit tests.

@Hona

Hona commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

All three findings verified as valid and addressed in fb7f250.

1. Retry after disable — fixed. Confirmed the gap: the catch checked enabled() before scheduling the retry, but disabling during the 1–2s backoff still let the fired timer approve. approve() now rechecks enabled() in its initial guard, making it the single authoritative check for every entry path (event, sweep, retry timer); the redundant pre-scheduling check was removed.

2. Idle detached requests — mitigated. Confirmed the path: session.permission.create calls Permission.ask, which registers a pending entry with no awaiting fiber (packages/core/src/permission.ts:224-229), unlike assert whose entry dies with its fiber. The sweep inventory is now the union of active-session locations and locally known session locations (data.session.list()), so a detached request on any session this client has loaded is recovered. The remaining uncovered case — a detached request on a session this client has never loaded — would need a server-wide pending-request inventory endpoint (a protocol + server + generated-client change). Since session.permission.create has no in-repo caller and its live permission.asked event is still handled while connected, I left that endpoint out as disproportionate; happy to add it if external usage materializes. The residual case is documented in the module comment.

3. Sweep failures — bounded retry added. A sweep now reports completeness: session.active failure, any session-resolution failure, or any per-location listing failure marks it incomplete, and the effect retries up to 2 more times with linear backoff. A generation counter makes newer sweeps (reconnect, re-enable) supersede scheduled retries, and the timer rechecks disposed/enabled before rerunning.

Coverage. The reconnect e2e test now also injects a 500 on the first /api/permission/request after the reconnect, so the assertion can only pass through the bounded sweep retry. All 3 e2e tests, 42 unit tests, and both typechecks pass.

@Hona

Hona commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

Thanks for the correction on the exhausted-retry finding. The three remaining findings are all valid and addressed in 7163804.

1. High: stale locations for moved active sessions — fixed. Confirmed the mechanism: data.session.sync(id) is a no-op for already-synced sessions because sync.run returns immediately once the key is complete (packages/client/src/solid/data.ts:156-161), so a cached location was never refreshed. The sweep now calls data.session.invalidate(id) before sync(id) for every active session, so each sweep lists permissions from fresh locations. A failed resync falls back to the cached location for that pass and marks the sweep incomplete, so the bounded retry re-attempts. The reconnect e2e test now asserts that /api/session/:id is re-fetched after the reconnect.

2. Medium: idle detached requests / late discovery — fixed for everything the client can see. Added a reactive effect that approves any pending request present in the local store (store.session.permission), which is exactly how a previously unknown idle session's requests surface: opening its view runs data.session.permission.sync(id). This closes the "request is visible to the client but hidden by the setting and never approved" hole, and it needs no network. New e2e test: a request served only from /api/session/:id/permission (never from a location sweep or an event) is approved once auto-approve is on. The residual case — a detached request on a session this client has never loaded — still requires a server-wide pending-request inventory endpoint; my position from the previous round is unchanged (no in-repo caller of session.permission.create, disproportionate protocol change), but I'm happy to add the endpoint if you want it in this PR.

3. Medium: reply mock returned 200 instead of 204 — fixed. Confirmed against the generated client: session.permission.reply declares successStatus: 204 and request() rejects any other status (packages/client/src/promise/generated/client.ts:1439, :297). The mock now fulfills the reply route with a bare 204, so every reply assertion in the spec now proves a successful approval; previously each mocked reply failed client-side and the assertions only observed the attempt.

All 4 e2e tests, 42 unit tests, and both typechecks pass.

@Hona
Hona merged commit 46d1f1f into anomalyco:v2 Aug 24, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants