fix: make the Host's chat default the single authority for a Session's permission mode - #3396
Conversation
7303c3b to
47b2378
Compare
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
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 |
b8b91f8 to
43f97cb
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Review of exact head 43f97cba63710cbb3ad4ecf11c978a83b75dd6ef.
Five actionable findings remain: four merge blockers and one CLI documentation inconsistency. I am submitting a COMMENT review under the maintainer comment-only policy, not REQUEST_CHANGES.
Problem and mechanism: the PR correctly identifies two real problems. New Sessions should inherit the owning Runtime Host's chatDefaults.permissionMode unless a client makes an explicit per-Session choice, and the retired execute spelling should fold to ask only when reading durable records while remaining invalid on current wire/input boundaries. Removing the dead desktop resolver, narrowing the live enum, bumping the compatibility epoch, and centralizing the primary fold are all directionally sound.
The problem definition is incomplete in the current implementation, however. Desktop and TUI still turn cached policy snapshots into explicit create values, so the Host is not yet the sole authority. The persistence inventory also misses two durable nested locations: subagentRuntime.permissionCeiling in Session headers and the ScheduledTask copy embedded in pending fire claims.
First principles / Occam: the simplest correct rule is still the one stated by this PR: omit means Host default; explicit means a real per-Session override; every durable representation is canonicalized at its read boundary; live wire values stay strict. The current patch needs fewer client-side claims, not more fallback state.
Deletion and test quality: delete the Desktop first-send assertion that requires forwarding the offered default as permissionMode; it currently pins the opposite of the intended authority contract. Replace it with an assertion that ordinary first-send creation omits the field. The remaining decoder and protocol tests are useful, but add legacy coverage for nested permission ceilings and pending ScheduledTask claims. No broad architectural rewrite is required, though the TUI must separate display of the prospective Host default from an explicit driver override.
Merge verdict: not ready to merge until the four blocking findings are fixed and the stale help text is corrected.
Verification on this head:
- required GitHub CI
testis green and the PR is mergeable; npm run build:test,npm run typecheck --workspaces,npm run format:check, andgit diff --checkpassed;- 150 focused tests passed across Desktop first-send, Core permission decoders, Runtime Host protocol, CLI session driver, SessionStore, and ScheduledTask store;
- a direct upgrade reproducer returned
{"taskTableMode":"ask","pendingClaimMode":"execute"}; - a direct decoder probe returned
isSubagentSessionRuntime(...permissionCeiling: "execute") === false; - the activation help probe still prints
explore|execute|bypasswhile the parser accepts and advertisesask.
Residual risk after these fixes: retired-value folding remains hand-written across decoders, as tracked in #3413; renderer behavior still lacks a dedicated E2E surface. Those are manageable once every currently persisted representation is covered.
708f976 to
dc24eb1
Compare
jackwener
left a comment
There was a problem hiding this comment.
Automated review of exact head dc24eb14f5a378fefe11c4b06dc9ba90e9674bdf against current main@d62857a8357e9160926726a2a13096bc2dc2b91d.
P1 — a one-task permission choice is persisted as the Host default. In apps/desktop/src/renderer/app-shell.tsx:576-588, the new-task composer's setNewTaskPermissionMode now calls window.maka.settings.update({ chatDefaults: { permissionMode: mode } }); createAppShellChatActions then omits permissionMode from Session creation. Selecting Full access for one draft therefore rewrites the Runtime Host policy and makes later new tasks start in Full access too. That is both a user-visible regression and a permission-safety regression, and it directly contradicts this PR's stated contract that “clients omit permissionMode unless the user explicitly overrode it for that Session” and “a per-Session override never writes back to the default.”
The new app-shell-session-settings-actions.test.ts does not protect this: its harness supplies a draft-local callback and only proves the action delegates to it; it never exercises the production callback in app-shell.tsx, which is the part that mutates settings.
Keep the Host default as the fallback authority, but retain a draft-local explicit override: ordinary creation omits the field, while choosing a mode in the new-task composer stores that choice in the current draft and sends it once on create. Only the Settings surface should update chatDefaults. Add a production-seam test proving a composer choice does not call settings update and does reach the created Session.
The previous compatibility blockers are fixed: stored execute results now canonicalize, and epoch 37 is ahead of current main's 36. Required test is green.
Required conclusions:
- Optimal for the actual problem: not yet; Host-default ownership is correct, but the patch erased the distinct per-draft override boundary.
- Production code to delete: none identified.
- Tests to delete/replace: replace the delegate-only new-task permission test with coverage of the real
app-shell.tsxwiring, or add that stronger test alongside it. - Deeper refactor: no; restore the existing draft-local choice seam and use omission only when it is unset.
- Ready to merge: no.
- Residual risks/gaps: permission behavior is protected user-visible/security behavior and requires fresh independent human review after repair; new commits will also need exact-head CI and epoch validation.
dc24eb1 to
83d008c
Compare
|
Accepted and fixed on You are right, and checking why I made that change showed it was unnecessary in the first place. Settings already has a dedicated default-permission-mode picker writing The draft-local choice is restored, with the create path corrected in the same move:
On the test: I also reworded the commit that introduced the write-back so it no longer states a contract this branch does not end up honouring; it now covers only the duplicate-copy collapse it actually leaves behind. Verification on the new head: full 🤖 Addressed by Claude Code |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 83d008cf72921704e7ff263d86c951fecb014c9e against current main.
Verdict: not ready to merge.
The central problem definition is correct: ordinary Session creation should omit permissionMode, the Runtime Host policy should resolve the default, and only a real per-Session choice should be sent explicitly. The Host-side ownership, compatibility epoch bump, durable execute folds, and deletion of unused declarations are directionally sound.
Three permission-state defects remain:
- Desktop stores the explicit choice by Host/project key indefinitely. After a user selects Full access for task A, task B on the same target sends the same explicit bypass override without the user choosing it again.
- A failed TUI policy query is converted to
askfor display while Session creation still omits the field. A Host configured for bypass can therefore run the first prompt with Full access while the TUI says Auto. /newresets the driver mode to undefined, but the runner coalesces that to the previous Session mode. The new-session label can therefore remain stale, including the dangerous Auto-displayed / Full-access-created direction after a Host default change.
First principles / Occam: keep three states separate: an authoritative refreshable Host default, an explicit unknown/error display state, and a consumable one-draft override. Previous Session state must not be used as any of those.
Test quality: the new Desktop test invokes the action once and never renders or consumes useNewTaskChoice; its settings assertion cannot catch reuse on the next task. Add a two-consecutive-task test. Add TUI coverage for policy-query failure against a bypass Host and for a Host default change followed by /new.
Verification: exact-head CI is green. Local workspace typecheck passed after applying dependency patches; all 357 CLI tests passed; formatting and diff checks passed. The current tests do not exercise the three failing lifecycles above.
Required conclusions:
- Problem definition: correct, but the client display/override state model is incomplete.
- Optimality: no; stale state still overrides or mislabels Host authority.
- Production code to delete: no additional broad deletion identified.
- Low-quality tests: replace or strengthen the one-call “sent once” test; it does not prove consumption.
- Deeper refactor: a small state-boundary refactor is required, not a broad rewrite.
- Merge readiness: no.
- Residual risk: permission behavior remains under-tested across consecutive drafts, policy-query failure,
/new, and reconnect.
78a6585 to
5efe711
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
`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. The composer picker moves onto the selected Host's value here as well. Which authority it writes to is settled in "stop clients from sending the Host default back as an explicit choice" later in this branch: the choice stays local to the draft and is sent once on create, and only the Settings surface writes `chatDefaults`. Renderer behavior has no unit-test surface in this app (desktop tests cover `main`), so this part is typecheck- and lint-verified only. 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
…hoice Desktop and the TUI both read `chatDefaults.permissionMode` for display and then passed that snapshot as the create input. The Host's own `prepared.permissionMode ?? chatDefaults` could therefore never reach its right side: a cached value became the authority, and a Session could start with full access from a setting another client had already lowered. The TUI was worse in two further ways — the snapshot never refreshed across `/new`, and a failed policy query was converted to `ask` and then sent explicitly, overriding the Host with a value nobody configured. Ordinary creation now omits the field. Desktop drops `newChatPermissionMode` entirely, since the create input was its only consumer. The TUI keeps the value as `prospectivePermissionMode`, used for display and skill prediction but never handed to the driver, and the launcher shows it instead of a hardcoded `ask`, so a Host configured for Bypass no longer displays as Auto. That prospective value is derived through `createGenesisExecutionBoundary` and `executionBoundaryDisplayMode`, the same mapping live Sessions already use, so a Session before and after creation cannot label one set of permissions two ways. Refs #3385 Generated-by: Claude Code
Two more places declared a permission mode that no production code reads. `SubagentSessionRuntime.permissionCeiling` was documented as decode-only and had no reader, yet `isSubagentSessionRuntime` still validated it with strict `isPermissionMode` — so a child Session written before a mode was retired was rejected as malformed and became unopenable. The field leaves the type; the guard tolerates the key on stored records instead of validating it, which is what keeps those records readable without pretending the value means anything. `packages/core/src/workspace.ts` declared `defaults.permissionMode` on a `WorkspaceConfig` with no references anywhere in the repository and no entry in the package exports. The whole file goes. Refs #3385 Generated-by: Claude Code
Before a Session exists, the mode a client shows is `draft choice ?? Host chatDefaults`. Three places broke that expression in the same way — by refusing to admit they had no current reading: Desktop kept the draft choice forever. `useNewTaskChoice` is keyed by Host/project target, not by draft, and had no clear operation, so choosing Full access for one task attached it to every later task on that target. The choice is now consumed on a successful create and left in place on a failed one, which is what makes it a per-Session override rather than a standing setting. The CLI invented `ask` when the policy query failed. Understating the boundary is not the safe direction it reads as: creation omits the field either way, so a Host configured for Bypass ran the first prompt with full access while the TUI displayed Auto. A failed query now throws, like every other catalog read on that path — with no true value to show, the CLI should not start. `/new` fell back to the previous Session's mode. The driver holds the Host's chat default as display-only state and refreshes it when a Session ends, so a fresh Session reports the mode it will actually get; the runner's fallback is the launch reading rather than the label it happens to be showing. `#defaultPermissionMode` goes with them. No caller ever set it, so it was always `undefined`, and the leak it documented is prevented by omitting the field rather than by carrying a default. The dead mirror write on channel open goes too — `#activeBoundaryDisplayMode` already shadows it. Refs #3385 Generated-by: Claude Code
5efe711 to
d66bcbe
Compare
P1 is fixed on d66bcbe: composer choice is draft-local (newTaskPermissionChoice), Session create omits permissionMode unless that choice is set, and app-shell-first-send-cleanup.test.ts covers send-once without settings.update. Dismissing the stale CHANGES_REQUESTED.
jackwener
left a comment
There was a problem hiding this comment.
P1 on dc24eb14f is fixed at d66bcbe.
Composer choice is draft-local (newTaskPermissionChoice); setNewTaskPermissionMode no longer writes chatDefaults. Session create omits permissionMode unless that choice is set, then clears it. app-shell.tsx has no settings.update for this. app-shell-first-send-cleanup.test.ts asserts the choice reaches create once and settings.update is not called.
Stale CHANGES_REQUESTED dismissed. Approve this head.
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.Ordinary creation on every client now omits the field, while a real per-Session override is still sent once. In Desktop that override is the new-task composer's choice: it stays local to the draft, reaches the created Session, and never writes back to
chatDefaults— picking Full access for one task is not a statement about every later task, and only the Settings surface owns the default. Desktop and the TUI previously read the Host default for display and then sent that snapshot back as the create input, so the Host's ownprepared.permissionMode ?? chatDefaultscould never reach its right side — a cached value became the authority, and a Session could start with full access from a setting another client had already lowered. The TUI additionally never refreshed the snapshot across/newand converted a failed policy query into an explicitask. The prospective mode it still shows is derived throughcreateGenesisExecutionBoundaryandexecutionBoundaryDisplayMode, the same mapping live Sessions use, so a Session before and after creation cannot label one set of permissions two ways.With one authority in place, five concepts lose their last consumer and are removed:
resolveCreateSessionInput, which existed only to re-derive a mode the Host already knows;SubagentSessionRuntime.permissionCeiling, documented as decode-only, with no reader, yet still strictly validated — so a child Session written before a mode was retired was rejected as malformed and became unopenable;packages/core/src/workspace.ts, which declareddefaults.permissionModeon aWorkspaceConfigreferenced nowhere in the repository and absent from the package exports;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 37. 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_EPOCH37 → 38. 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 36 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 both scheduled-task read paths — the catalog table and the ScheduledTask copy embedded in pending fire claims, each of which previously did a bareJSON.parse(...) as ...with 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?