Skip to content

fix(permission): stop mode changes escalating boundaries and skipping backends - #1546

Merged
Astro-Han merged 1 commit into
mainfrom
worktree-permission-mode-propagation
Jul 28, 2026
Merged

fix(permission): stop mode changes escalating boundaries and skipping backends#1546
Astro-Han merged 1 commit into
mainfrom
worktree-permission-mode-propagation

Conversation

@jackwener

Copy link
Copy Markdown
Member

Two ways a session's permission mode ended up different from the mode actually being enforced.

1. Flipping the global default escalated sessions that never chose it

The composer's picker is global — it writes chatDefaults.permissionMode, and applySettingsRuntimeEffects pushes that mode onto every existing session, exempting only Deep Research by label.

But four flows deliberately create sessions at explore as their read-only boundary:

flow created at exempt before?
Deep Research explore
Bot conversations (prompted by a remote sender) explore
Expert-team review crews explore
Cron automations explore

So changing the picker in an unrelated chat silently moved three of them to execute/bypass. Bot sessions re-pinned on their next inbound message; expert-team and cron sessions never did.

Now exempt by mode rather than by label: explore is structurally unreachable as a global default (ChatDefaultPermissionMode excludes it) and the picker never offers it, so a session sitting in explore was put there by a deliberate seed. That covers every current flow and any future one without anyone having to remember to add it — which is exactly what went wrong when only Deep Research was listed.

2. A lowered mode was persisted but not enforced

AiSdkBackend snapshots the header at construction and ToolRuntime reads header.permissionMode at every decision, so a mode change only takes effect if the backend is rebuilt. setPermissionMode disposes the backend for exactly this reason and says so in a comment.

changesBackendConfig — which is what updateSession consults — did not list permissionMode.

So every path that lowered a mode via updateSession was advisory. The one that matters: the bot-incoming guard re-pins a bound conversation to explore before handling a remote message. If that session already had a live execute/bypass backend (e.g. the user had opened it in the desktop app and sent a message), the re-pin wrote explore to storage and left the permissive backend serving the remote sender.

Adding permissionMode closes it for every caller, not just that one.

Tests

New contract pins that no explore-pinned session is escalated, that ordinary sessions still follow the new default, and that busy (running / waiting_for_user) sessions are still left to reconcile later. changesBackendConfig gains a case for the mode.

Gates

1208 core + 2667 runtime + 2900 desktop tests green · biome clean.

Second of the permission-review fixes; #1541 was the first.

… backends

Two ways a session's permission mode ended up different from the mode
actually being enforced.

**1. Flipping the global default escalated sessions that never chose it.**

The composer's picker is global — it writes `chatDefaults.permissionMode`,
and `applySettingsRuntimeEffects` then pushes that mode onto every
existing session, exempting only Deep Research by label.

But four flows deliberately create sessions at `explore` as their
read-only boundary: Deep Research, bot conversations (prompted by a
remote sender), expert-team review crews ("read-only review crews", says
the comment), and cron automations. Three of them were not exempt. So
changing the picker in an unrelated chat silently moved them to
`execute`/`bypass`. Bot sessions re-pinned on their next inbound message;
expert-team and cron sessions never did.

Now exempt by MODE rather than by label: `explore` is structurally
unreachable as a global default (`ChatDefaultPermissionMode` excludes it)
and the picker never offers it, so a session sitting in `explore` was put
there by a deliberate seed. That covers every current flow and any future
one without needing to remember to add it — which is precisely what went
wrong when only Deep Research was listed.

**2. A lowered mode was persisted but not enforced.**

`AiSdkBackend` snapshots the header at construction and `ToolRuntime`
reads `header.permissionMode` at every decision, so a mode change only
takes effect if the backend is rebuilt. `setPermissionMode` disposes the
backend for exactly this reason and says so. `changesBackendConfig` —
which is what `updateSession` consults — did not list `permissionMode`.

So every path that lowered a mode via `updateSession` was advisory. The
one that matters: the bot-incoming guard re-pins a bound conversation to
`explore` before handling a remote message. If the session already had a
live `execute`/`bypass` backend (e.g. the user opened it in the desktop
and sent one message), the re-pin wrote `explore` to storage and left the
permissive backend serving the remote sender.

Adding `permissionMode` closes it for every caller, not just that one.

Tests: a new contract pins that no explore-pinned session is escalated
while ordinary sessions still follow the default and busy sessions are
still left alone; `changesBackendConfig` gains a case for the mode.

Gates: 1208 core + 2667 runtime + 2900 desktop tests green, biome clean.
@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed at 73801551d8fd1a74df18d6450544b69d0eedc797.

P2: Updating permissionMode through updateSession can orphan a pending permission request.

Adding permissionMode to the backend-config change check makes the Bot guard in bot-incoming-main.ts dispose the cached backend when it pins a session back to explore. Unlike setPermissionMode, this path does not reject waiting_for_user.

The reproduced state is:

beforeStatus=waiting_for_user
beforeMode=ask
afterStatus=waiting_for_user
afterMode=explore
disposeCalls=1
permissionResponses=0

The UI still says the session is waiting, but the backend holding the continuation is gone, so the permission response has no target. Please route this transition through the permission-mode authority, or apply the same waiting-state guard before disposal. Add a real SessionManager test for a waiting request and for an idle cached backend. The current test only checks that the patch counts as a backend-config change.

@Astro-Han Astro-Han 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.

Approved at 73801551. The waiting-for-user disposal case remains a P2 follow-up, not a P0/P1 blocker. CI is green.

@Astro-Han
Astro-Han merged commit 262a9ea into main Jul 28, 2026
3 checks passed
@Astro-Han
Astro-Han deleted the worktree-permission-mode-propagation branch July 28, 2026 13:57
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