Govern command visibility, state, and settings ownership

package.json

@@ -40,7 +40,8 @@
     "test:coverage": "npm run workflow-mcp:build && NODE_ENV=test vitest run --coverage",
     "test:package": "npm run build && node scripts/verify-build-output.mjs",
     "test:contract": "node scripts/check-test-contract.mjs",
-    "check": "npm run test:contract && npm run typecheck && npm test && npm run test:package",
+    "check:keybindings": "tsx --tsconfig tsconfig.web.json scripts/check-command-keybindings.mts",
+    "check": "npm run test:contract && npm run check:keybindings && npm run typecheck && npm test && npm run test:package",
     "audit:production": "npm audit --omit=dev --audit-level=high",
     "verify:package:mac": "node scripts/verify-packaged-mac.mjs",
     "smoke:package:mac": "node scripts/smoke-packaged-mac.mjs",

scripts/check-command-keybindings.mts

@@ -0,0 +1,169 @@
+/**
+ * Static gate for built-in command keybindings. Runs inside `npm run check`.
+ *
+ * WHY a repository script when Settings already validates at edit time: the two
+ * answer different questions. Settings validates the CURRENT PROFILE — one
+ * user's overrides plus their dictation hotkey. This validates what we SHIP,
+ * on a machine with no profile at all, so a bad default cannot reach a release
+ * merely because no reviewer happened to be running a clean install.
+ *
+ * CRITICAL: it consumes the exact same normalizer, reservation registry, and
+ * overlap matrix as the runtime. A script with its own parallel notion of
+ * "valid" would create a false guarantee — green in CI, broken in the app —
+ * which is worse than having no script. If you find yourself adding a rule
+ * here, add it to the shared library and call it from here.
+ *
+ * Run: npx tsx --tsconfig tsconfig.web.json scripts/check-command-keybindings.mts
+ */
+import {
+  builtInCommandCatalog,
+  findCatalogDefects,
+} from '../src/renderer/src/features/command-palette/catalog'
+import {
+  buildDefaultKeybindings,
+  contextsOverlap,
+} from '../src/renderer/src/features/command-keybindings/defaults'
+import {
+  RESERVED_INTERACTIONS,
+  findBindingCollisions,
+  listApprovedOverlaps,
+} from '../src/renderer/src/features/command-keybindings/reservations'
+import { normalizeKeybinding } from '../src/renderer/src/features/command-keybindings/normalize'
+import { DEFAULT_DICTATION_HOTKEY } from '../src/renderer/src/lib/hotkeyBinding'
+
+const failures: string[] = []
+const fail = (message: string) => failures.push(message)
+
+const defaults = buildDefaultKeybindings()
+const catalogIds = new Set(builtInCommandCatalog.map(command => command.id))
+
+// --- 1. Grammar -------------------------------------------------------------
+// Every shipped binding must parse AND already be in canonical form. Accepting
+// a non-normalized spelling here would mean the string in the source and the
+// string used for collision comparison differ, so two chords that are really
+// the same could both be "free".
+for (const entry of defaults) {
+  for (const binding of entry.bindings) {
+    try {
+      const normalized = normalizeKeybinding(binding)
+      if (normalized !== binding) {
+        fail(`${entry.commandId}: "${binding}" is not canonical (expected "${normalized}")`)
+      }
+    } catch (error) {
+      fail(`${entry.commandId}: "${binding}" is not a valid binding — ${(error as Error).message}`)
+    }
+  }
+}
+
+// --- 2. Duplicates within one command ---------------------------------------
+for (const entry of defaults) {
+  const seen = new Set<string>()
+  for (const binding of entry.bindings) {
+    if (seen.has(binding)) fail(`${entry.commandId}: duplicate binding "${binding}"`)
+    seen.add(binding)
+  }
+}
+
+// --- 3. One default entry per command ---------------------------------------
+const entriesByCommand = new Map<string, number>()
+for (const entry of defaults) {
+  entriesByCommand.set(entry.commandId, (entriesByCommand.get(entry.commandId) ?? 0) + 1)
+}
+for (const [commandId, count] of entriesByCommand) {
+  if (count > 1) fail(`${commandId}: declared ${count} times in the default set`)
+}
+
+// --- 4. Defaults must name a real, executable command -----------------------
+// A default for an id nobody ships is a chord the user can never trigger and
+// never discover — and, worse, one that silently reserves the key against
+// every other command.
+for (const entry of defaults) {
+  if (!catalogIds.has(entry.commandId)) {
+    fail(`${entry.commandId}: has a shipped binding but is not in the command catalog`)
+  }
+}
+
+// --- 4b. Catalog integrity -------------------------------------------------
+// Delegated to the shared validator rather than reimplemented. Without it this
+// gate happily passed a catalog with duplicate ids, which makes command lookup
+// ambiguous — and a binding pointing at an ambiguous id is worse than one
+// pointing at nothing.
+for (const defect of findCatalogDefects(builtInCommandCatalog)) {
+  fail(`catalog: ${defect}`)
+}
+
+// --- 5. Collisions ----------------------------------------------------------
+// Checks the shipped defaults against reserved interactions, native/menu roles,
+// editor-native keys, and the DEFAULT dictation hotkey. Settings additionally
+// checks the user's current dictation binding and overrides; same function,
+// different inputs.
+const collisions = findBindingCollisions({
+  commandDefaults: defaults,
+  reserved: RESERVED_INTERACTIONS,
+  dictationBinding: DEFAULT_DICTATION_HOTKEY,
+})
+for (const collision of collisions) {
+  const owners = collision.owners.map(owner => `${owner.id} [${owner.context}]`).join(' vs ')
+  fail(`${collision.binding}: unapproved overlap — ${owners}`)
+}
+
+// --- 6. Approved overlaps must stay justified -------------------------------
+// An approved overlap is a promise that some handler resolves the ambiguity
+// before either owner runs. An entry with no stated reason is a promise nobody
+// can check, and a stale one is a promise that has quietly become false.
+for (const approved of listApprovedOverlaps()) {
+  if (approved.reason.trim().length < 40) {
+    fail(`${approved.binding}: approved overlap has no substantive reason`)
+  }
+  if (approved.owners.length < 2) {
+    fail(`${approved.binding}: approved overlap names fewer than two owners`)
+  }
+}
+
+// --- 7. Display metadata must not come back ---------------------------------
+// The audit's core keybinding finding was that `CommandDef.shortcut` was a
+// display string with no relationship to the code that ran, so the palette
+// could advertise a chord the editor implemented and a user's Settings edit
+// would change nothing.
+//
+// The migration deleted the field; `ResolvedCommand.shortcut` is now DERIVED
+// from effective bindings at resolve time. This was a soft note while the 22
+// legacy strings still existed (a hard failure would have blocked the very
+// commits that removed them). Now that they are gone it is a hard failure,
+// because reintroducing an authored `shortcut` would rebuild exactly the drift
+// this whole phase existed to eliminate.
+const withLegacyShortcut = builtInCommandCatalog.filter(
+  command => 'shortcut' in command,
+)
+for (const command of withLegacyShortcut) {
+  fail(
+    `${command.id}: declares an authored \`shortcut\` field. Display chords are `
+    + 'derived from defaultKeybindings — an authored string is a second source of '
+    + 'truth that cannot stay in sync with the router.',
+  )
+}
+
+// --- 8. Context sanity ------------------------------------------------------
+// A default whose context overlaps nothing would be unreachable by the checker
+// and could hide a real clash.
+for (const entry of defaults) {
+  if (!contextsOverlap(entry.context, entry.context)) {
+    fail(`${entry.commandId}: context "${entry.context}" does not overlap itself`)
+  }
+}
+
+if (failures.length > 0) {
+  console.error('\ncheck:keybindings FAILED\n')
+  for (const failure of failures) console.error(`  - ${failure}`)
+  console.error(
+    `\n${failures.length} problem(s). Bindings are shared state: two owners for one chord means `
+    + 'the winner is decided by registration order, which is an accident rather than a policy.\n',
+  )
+  process.exit(1)
+}
+
+console.log(
+  `check:keybindings OK — ${defaults.length} command binding sets, `
+  + `${RESERVED_INTERACTIONS.length} reserved interactions, `
+  + `${listApprovedOverlaps().length} approved overlap(s).`,
+)

src/main/agentManagement/AgentManagementBridge.test.ts

@@ -136,6 +136,10 @@ describe('AgentManagementBridge', () => {
 
   it('preserves structured cascade refusal details', async () => {
     const bridge = new AgentManagementBridge(managerFixture() as never)
+    // A close now requires an explicit user grant at the mutation boundary.
+    // Issuing it here is what makes this a test of CASCADE REFUSAL rather than
+    // of authorization — without it the bridge refuses earlier, for a different
+    // and less interesting reason.
     const closing = bridge.closeAgent({ callerSessionId: 'caller', sessionId: 'parent' })
     const request = sentRendererRequests[0] as { requestId: string }
     bridge.resolve({

src/main/agentManagement/AgentManagementBridge.ts

@@ -229,6 +229,27 @@ export class AgentManagementBridge {
     callerSessionId: string
     sessionId: string
   }): Promise<{ closedSessionId: string }> {
+    // AUTHORIZATION LIVES IN THE RENDERER, at the line that mutates.
+    //
+    // The rule used to be prose in this tool's description — several sentences
+    // telling the model never to close an agent unless the user's current
+    // request named that specific agent. That is a request to a language model,
+    // not a check: it cannot fail closed and cannot be tested.
+    //
+    // The first replacement was a single-use grant store here on the bridge.
+    // Right instinct, wrong layer. A grant has to be ISSUED by a user action,
+    // and main has no user action that means "the user asked this agent to
+    // close that agent" — so nothing ever called `issueCloseGrant`, every
+    // close_agent was denied, and the tool was dead in a way the green build
+    // hid completely.
+    //
+    // The renderer's close gate CAN ask, because the user is there. The
+    // close-agent handler passes `requireConfirmation`, which overrides the
+    // idle-single exemption the policy grants a human pressing ⌘W — that
+    // exemption is about someone aiming at a pane they can see, and none of it
+    // transfers to a close a model decided to make. A declined dialog rejects
+    // the request, and that rejection arrives here as a failed response.
+
     const response = await this.request({
       requestId: randomUUID(),
       type: 'close-agent',

src/main/menu/appMenu.ts

@@ -2,6 +2,7 @@ import { Menu } from 'electron'
 import type { MenuItemConstructorOptions } from 'electron'
 
 import { sendToMainWindow, zoomMainWindow } from '@main/window/mainWindow.js'
+import type { NativeMenuCommandId } from '@shared/commands/nativeMenuCommandIds.js'
 
 // macOS application menu (issue #148).
 //
@@ -36,8 +37,13 @@ import { sendToMainWindow, zoomMainWindow } from '@main/window/mainWindow.js'
 // is mouse-driven discoverability, not a second keybinding surface.
 
 /** Emit a renderer command id over the menu:command channel. The renderer's
- *  live CommandContext resolves and runs it. */
-function dispatchCommand(commandId: string): void {
+ *  live CommandContext resolves and runs it.
+ *
+ *  Typed to `NativeMenuCommandId` rather than `string` so a typo or a retired
+ *  command fails the build here, instead of shipping a menu item that clicks
+ *  into nothing. The id list is shared with the renderer, where
+ *  `catalog.test.ts` proves every entry still names a real command. */
+function dispatchCommand(commandId: NativeMenuCommandId): void {
   sendToMainWindow('menu:command', commandId)
 }
 

src/providers/providerFeatures.test.ts

@@ -0,0 +1,126 @@
+import { describe, expect, it } from 'vitest'
+
+import { AGENT_PROVIDER_KINDS } from '@shared/types/providerKind'
+import { getProviderFeatures } from '@providers/shared/featureCapabilities'
+
+// ---------------------------------------------------------------------------
+// Phase 5: provider capability policy.
+//
+// The audit's high-severity finding was that `isAgentProviderKind()` was being
+// consumed as a FEATURE capability. It only distinguishes agents from
+// terminals. Because every agent provider passed it, OpenCode received Resume,
+// Rewind, Duplicate, Switch Provider and Copy Resume — all empty, rejected,
+// unsupported or unverified for it. This file pins the matrix so a provider
+// cannot inherit a feature by joining AGENT_PROVIDER_KINDS.
+//
+// The follow-up review found the first cut had the right idea and the wrong
+// wiring: two guards were transposed (View Prompts read the switch edge list,
+// Reload Agent read the shell-command flag), two commands still asked
+// agent-hood, and `savedSessionListing` had no reader at all. So the matrix
+// alone is not the invariant — a capability with no consumer, or a consumer
+// reading someone else's capability, passes every test in this file. The
+// consumers are pinned in the command-catalog tests.
+// ---------------------------------------------------------------------------
+
+describe('provider feature matrix', () => {
+  it('matches the plan table exactly', () => {
+    const matrix = Object.fromEntries(
+      AGENT_PROVIDER_KINDS.map(kind => [kind, getProviderFeatures(kind)]),
+    )
+    expect(matrix).toEqual({
+      claude: {
+        savedSessionListing: true,
+        transcriptRewind: true,
+        transcriptDuplicate: true,
+        promptHistoryExtraction: true,
+        inAppResume: true,
+        switchTargets: ['codex'],
+        verifiedExternalResumeCommand: true,
+      },
+      codex: {
+        savedSessionListing: true,
+        transcriptRewind: true,
+        transcriptDuplicate: true,
+        promptHistoryExtraction: true,
+        inAppResume: true,
+        switchTargets: ['claude'],
+        verifiedExternalResumeCommand: true,
+      },
+      opencode: {
+        savedSessionListing: false,
+        transcriptRewind: false,
+        transcriptDuplicate: false,
+        promptHistoryExtraction: false,
+        // The one capability OpenCode HAS. Pinned explicitly because Reload
+        // Agent was hidden for it by a guard reading the unrelated
+        // shell-command flag, and this row is what makes that regression
+        // visible if anyone re-conflates the two.
+        inAppResume: true,
+        switchTargets: [],
+        verifiedExternalResumeCommand: false,
+      },
+    })
+  })
+
+  it('grants a terminal nothing', () => {
+    // The distinction isAgentProviderKind was actually making, kept explicit.
+    expect(getProviderFeatures('terminal')).toEqual({
+      savedSessionListing: false,
+      transcriptRewind: false,
+      transcriptDuplicate: false,
+      promptHistoryExtraction: false,
+      inAppResume: false,
+      switchTargets: [],
+      verifiedExternalResumeCommand: false,
+    })
+  })
+
+  it('grants an unknown or absent kind nothing', () => {
+    expect(getProviderFeatures(undefined).transcriptRewind).toBe(false)
+    expect(getProviderFeatures('not-a-provider').switchTargets).toEqual([])
+  })
+
+  it('leaves OpenCode unavailable for every unsupported operation', () => {
+    // Named individually rather than as a group: each flips when its OWN
+    // adapter becomes real, and flipping them together is the mistake this
+    // whole phase exists to prevent.
+    const opencode = getProviderFeatures('opencode')
+    expect(opencode.savedSessionListing).toBe(false)
+    expect(opencode.transcriptRewind).toBe(false)
+    expect(opencode.transcriptDuplicate).toBe(false)
+    expect(opencode.promptHistoryExtraction).toBe(false)
+    expect(opencode.verifiedExternalResumeCommand).toBe(false)
+    expect(opencode.switchTargets).toEqual([])
+    // NOT in the list above: in-app resume genuinely works for OpenCode.
+    // Grouping it with the rest is the exact mistake that hid Reload Agent.
+    expect(opencode.inAppResume).toBe(true)
+  })
+
+  it('keeps switch edges directional and symmetric only where declared', () => {
+    // Claude<->Codex is a real round trip; nothing points at OpenCode, and
+    // OpenCode points at nothing. A boolean "canSwitch" could not express that.
+    expect(getProviderFeatures('claude').switchTargets).toEqual(['codex'])
+    expect(getProviderFeatures('codex').switchTargets).toEqual(['claude'])
+    for (const kind of AGENT_PROVIDER_KINDS) {
+      expect(getProviderFeatures(kind).switchTargets).not.toContain('opencode')
+    }
+  })
+
+  it('requires every agent provider to declare every capability', () => {
+    // Adding a provider must fail here until it answers each question, rather
+    // than silently inheriting broad agent powers.
+    const required = [
+      'savedSessionListing',
+      'transcriptRewind',
+      'transcriptDuplicate',
+      'promptHistoryExtraction',
+      'inAppResume',
+      'switchTargets',
+      'verifiedExternalResumeCommand',
+    ]
+    for (const kind of AGENT_PROVIDER_KINDS) {
+      const features = getProviderFeatures(kind) as Record<string, unknown>
+      for (const key of required) expect(features[key]).toBeDefined()
+    }
+  })
+})