fix: make the Host's chat default the single authority for a Session's permission mode - #3396
fix: make the Host's chat default the single authority for a Session's permission mode#3396Astro-Han wants to merge 6 commits into
Conversation
7303c3b to
47b2378
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head 47b23789fb746215a7a3e4c3f2bb2e9cd437be8e.
Two blockers remain:
-
The promised persisted-record fold is not actually applied to subagent tool results.
decodeCanonicalToolResultContentvalidates a legacypermissionMode: "execute"viadecodePersistedPermissionMode(...) !== undefinedand then returns the original object unchanged;decodeToolResultPreviewContentdoes the same. Afterexecuteis removed fromPermissionMode, these decoders therefore return a runtime value their output type forbids, and downstream code can still observe the retired spelling. Canonicalize the field toaskin the returned subagent value and add legacy-record tests for both decoders. -
Required CI is red on the new Desktop test. The mock committed session now carries
permissionMode: "ask", but the assertion still expects"bypass"(runtime-host-client-operations.test.ts, failure at the built line 139). That is internally inconsistent and currently fails the requiredtestjob.
The Host-default authority change itself is directionally sound: omitted create input reaches the Host policy, explicit create modes stay explicit, and Session/AgentRun/ScheduledTask durable folds cover their respective boundaries.
Merge readiness: not ready until both blockers are fixed, required CI is green on the replacement head, and an independent human approves it.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Follow-up against the same exact head 47b23789fb746215a7a3e4c3f2bb2e9cd437be8e after refreshing main@efe381f4fd: there is now a third blocker.
PR #3399 has merged and already consumes compatibility epoch 32 for removing request_authorization_code. This branch was based on epoch 31 and independently sets 32 for removing execute. After rebasing, retaining 32 would let a current-main epoch-32 peer (which still speaks execute) connect to a post-#3396 epoch-32 peer, defeating the handshake fence and allowing the exact mid-session frame rejection the bump is meant to prevent. Rebase and advance this change to epoch 33, preserving both epoch notes.
This is now also reflected by the PR's DIRTY merge state against current main.
3c12dbd to
5005723
Compare
5005723 to
942f717
Compare
|
All three blockers are addressed on 1. Subagent tool result fold. Correct, and fixed: I did not apply the same fold to 2. Desktop assertion. Fixed and squashed into the commit that introduced it, so no broken intermediate state remains. 3. Epoch. Rebased. That rebase surfaced a related problem: Separately, the missed fold in (1) is a symptom rather than a one-off: retired-value folding is hand-written per decoder with nothing enforcing it, and has now been independently re-implemented five times. Tracked in #3413, deliberately out of scope here. Verification on the new head: full 🤖 Addressed by Claude Code |
942f717 to
b8b91f8
Compare
`resolveCreateSessionInput` re-derived a new Session's permission mode, name and labels in the desktop main process, but nothing called it. The `sessions:create` handler uses the synchronous `resolveCreateSessionRequest` and forwards `mode` to the Runtime Host verbatim, which is what expands a product mode and what falls back to `chatDefaults.permissionMode`. The function survived only because its own tests kept it compiling, so it read like a second authority over the starting boundary while having no say in any Session actually created. Its tests move with it, except those that pin what still reaches the wire: an omitted mode staying omitted, the refusal of a directly-requested `explore`, and an unrecognized mode conferring nothing. The Deep Research expansion they also covered belongs to the Host, which owns it and tests it. `permission-mode-default.ts` stays: `runtime-host-boot` passes `resolveDefaultPermissionMode` to the skills IPC, which needs a concrete mode to predict what a new Session will start in and filter invocable skills accordingly. Refs #3385 Generated-by: Claude Code
`maka run` sent `yolo ? 'bypass' : 'ask'` and the session driver defaulted to `'ask'` twice more, so the CLI always put an explicit mode on the wire. An explicit mode overrides `chatDefaults.permissionMode`, which meant the configured default could never apply to a CLI Session and `--yolo` read as one half of a choice rather than a one-shot elevation. An omitted mode now stays omitted all the way to `session.create`, where the Runtime Host resolves it from its own Runtime Policy — the single authority for what a new Session starts in. `CreateSessionRequest` makes that sayable: `CreateSessionInput` requires a mode because the local runtime writes it straight onto the header, but a client talking to a Host is in a different position and needs a way to express "no explicit choice". The TUI reads the same policy value at startup instead of assuming Auto. Its indicator names the mode a new Session will actually get, so a Host configured for full access is no longer displayed as protected — the one direction the label must never be wrong in. `startNewSession` still falls back to the construction-time default rather than carrying a previous Session's elevation (#3020); that default is now `undefined` for `maka run`, which resolves to the configured mode instead of a hardcoded one. Refs #3385 Generated-by: Claude Code
Picking full access in the composer before starting a task lasted exactly one draft: `useNewTaskChoice` held it in an in-memory Map keyed by the draft, so the next new task was back on Auto. The setting that does persist — `chatDefaults.permissionMode` — was only reachable from Settings, which is where users were not looking. Before a Session exists, "the mode this task will start in" and "the configured default" are the same fact, so the draft-local copy could only ever be the stale one. The picker now reads the Host's value and writes it, which is what makes the choice survive to the next task. It writes to the Host that would run the task rather than to whichever Host is otherwise selected. Those differ once more than one Host is connected, and the mode shown in the composer belongs to the first one. The shell's second copy of the same setting goes with it. `app-shell` read the selected Host's `chatDefaults` in one place and a global copy hydrated from the *default* Host in another, so with several Hosts connected the placeholder session view could name a different Host's mode than the picker directly above it. Settings now asks the shell to re-read the value instead of handing it a third copy. Renderer behavior has no unit-test surface in this app (desktop tests cover `main`), so this is typecheck- and lint-verified only; the persistence is observable through the existing Settings picker. Refs #3385 Generated-by: Claude Code
`execute` had no behavior of its own. It compiled to the same workspace-write profile as `ask`, produced the same execution boundary, displayed as `ask`, and `executionBoundaryDisplayMode` — the single source for what is actually in force — could not return it at all. Five production sites existed only to fold it back into `ask`. Its two live writers were the Web Research and Implementation subagent definitions, where `'ask'` is behaviorally identical. Records written before this still carry it, so `decodePersistedPermissionMode` folds a stored `execute` to `ask` at the four persistence decode sites: session headers, agent run headers, subagent tool-result records, and chat default settings. Knowing which modes are retired now lives in one place instead of five ad-hoc comparisons. New input and wire values keep the strict check: the three protocol frame decoders and the desktop `sessions:setPermissionMode` IPC should reject a retired mode outright rather than quietly accept it. The compatibility epoch moves to 30 so a peer that still speaks `execute` is refused at the handshake instead of failing mid-Session. `maka activate --permission-mode` keeps accepting `execute` as an alias for `ask` — it is a public subcommand whose callers live outside this repo — and now offers `ask` by name. Its options type no longer excludes `ask`: that exclusion separated the two names while `execute` existed, but never the boundaries, which were always the same one. `LegacyPermissionMode` was a second spelling of the same member set and folds into `PermissionMode`. `isPermissionModeWithinCeiling` goes too: it had no production caller, and its only importer never called it. That also retires the implicit contract that `PERMISSION_MODES` array order encodes privilege strength. Tests that used `execute` as "a second mode that is not bypass" now say `ask` or `bypass` directly. One graph-provisioning test loses a distinction it was relying on — `ask` and `execute` were the only pair naming one boundary under two names — and now narrows from `bypass`, which is a real narrowing. Refs #3385 Generated-by: Claude Code
Scheduled tasks are read back with a bare `JSON.parse(...) as ScheduledTask`, so a record written before a permission mode was retired carried that value straight into `compilePermissionProfile`, which no longer has a branch for it. `normalizeCreateScheduledTaskInput` could not catch this: it validates new input and stored records never pass through it. Add `decodePersistedScheduledTask` next to the type it decodes and call it on the store's read path. It folds retired representations to their live equivalents and leaves everything else as stored — it is a compatibility fold, not a schema validator. Refs #3385 Generated-by: Claude Code
…ults `decodeCanonicalToolResultContent` accepted a stored `execute` and returned the record verbatim, so the decoder produced a value its own return type forbids and downstream readers could still observe the retired spelling. Fold it at the single exit every stored tool result passes through. `decodeToolResultPreviewContent` goes the other way: it decodes live open facts, never a stored record, and the compatibility epoch already refuses a peer old enough to send a retired mode. Accepting one there would only mask a handshake that should not have succeeded, so it returns to strict validation. Refs #3385 Generated-by: Claude Code
b8b91f8 to
43f97cb
Compare
2 participants
Summary
A new Session always started in
ask, whatever the user had chosen last time, because three places each claimed authority over the starting permission mode and none of them was the Host policy the user actually configures. This PR makeschatDefaults.permissionModein Runtime Policy the single authority: clients omitpermissionModeon create unless the user explicitly overrode it for that Session, and a per-Session override never writes back to the default.With one authority in place, three concepts lose their last consumer and are removed:
resolveCreateSessionInput, which existed only to re-derive a mode the Host already knows;executepermission mode, which had no semantics of its own — it compiled to the same profile asask, displayed asask, and produced the same execution boundary — and five sites that folded it away.Removing
executenarrows a wire enum, soRUNTIME_HOST_COMPATIBILITY_EPOCHgoes to 36. Records written before the removal decode through a single retired-mode fold (decodePersistedPermissionMode) rather than scattered per-site compatibility code;executestays accepted as a CLI input alias.Refs #3385
Not in scope:
packages/core/src/capability-audit.tsstill encodes scheduled-task status as a fake permission mode, tracked in #3386. Also out of scope, and surfaced by this work: retired-value folding is hand-written per decoder with nothing enforcing it — tracked in #3413.Verification
npm run typecheck --workspaces— cleannpm run format:checkandbiome linton touched files — cleandist:packages/runtimesession-manager (222/222),packages/corepermission + scheduled-task,packages/storagesession-store + agent-graph-schedule,packages/clirun-command + activation-command,apps/desktopmain-process suites — all passRollout
RUNTIME_HOST_COMPATIBILITY_EPOCH35 → 36. Mismatched peers are refused at connect, so a client and Host across this bump must be upgraded together — the routine handling for this epoch, unchanged from the 35 bumps before it.Persisted records that still carry
executeare folded toaskon read in every decoder that reaches them: Session headers, Agent Run headers, subagent tool results in the transcript, and the scheduled-task store, whose read path previously did a bareJSON.parse(...) as ScheduledTaskwith no decoding at all.Live wire decoders stay strict rather than folding.
decodeToolResultPreviewContentdecodes open facts from a connected peer, never a stored record, and the epoch fence already refuses a peer old enough to send a retired mode — accepting one there would mask a handshake that should not have succeeded.Review focus
The composer persistence change lives in
apps/desktop/src/renderer, which has no unit-test surface — the desktop suites cover the main process only. It is verified by hand, not by a test, and no E2E was added for it. Worth a second pair of eyes on whether that gap should block.AI use
Tool(s) and scope: Claude Code wrote the code, tests, and this description. The human contributor reviewed the final diff and commit messages and owns the submission. Affected commits carry
Generated-by: Claude Code.Checklist
Does this PR entail a change in behavior?