fix(sessions): agents start without a broken managed skill instead of not starting at all

docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md

@@ -0,0 +1,139 @@
+# Managed-skill pre-spawn gate: shrink the blast radius (#1133)
+
+## Problem
+
+`SessionManager.spawnWithId` runs `beforeAgentSessionStart(options)` before
+every agent spawn. That hook (wired in `src/main/index.ts`) runs the managed
+skills `audit()` and then `ensureTldrSkill()` / `ensureGoalSkill()` for the
+requested reporting domains. On a throw, the manager journals
+`conventions.pre_spawn_reconcile.error` and then:
+
+```ts
+if (hasReportingDomain(options.builtInMcpDomains)) throw error
+```
+
+Almost every pane has TLDR or Goal enabled, so one managed-skill failure turns
+into "no agent in any provider can start". Recovery then journals
+`recover.failed code=start-failed` with no cause, so the journal cannot say why
+the start failed.
+
+## Evidence
+
+From the issue's incident journals:
+
+| Run | Build | Reconcile errors | Message |
+|---|---|---|---|
+| `2026-09-18T00-49-30…77c310` | `0b744b88` (pre-grok) | 17 | `TLDR skill deployment failed` (cause unknown) |
+| `2026-09-19T04-09-36…9e113a` | post-#844 | 15 | `TLDR skill is unavailable or conflicts…` (#1014) |
+| `2026-09-19T04-13-05…fddcd2` | post-#844 | 15 | same (#1014) |
+
+On Sept 19, every reconcile error is followed at `seq+1`, in the same
+millisecond, by `recover.failed` (15/15 and 14/14). #1017 and #1037 fixed
+specific ways a skill could fail (unsupported providers, discovery errors). The
+Sept 18 cluster predates grok and #1017 does not explain it. This change fixes
+the blast radius for any future cause.
+
+## Decided behavior (user-accepted default, can be changed)
+
+1. A pre-spawn reconcile failure no longer aborts a spawn, whatever domains are
+   requested. The agent launches without the broken managed skill. The TLDR
+   and Goal MCP servers still deliver their own server instructions, so the
+   session loses the skill's guidance, not the capability.
+2. The failure is visible: a warning names the skill and points to
+   Settings › Agents › Custom Skills, where TLDR/Goal health already shows.
+3. `recover.failed` carries a typed `cause` from a closed enum. Raw exception
+   text still stays out of IPC.
+
+## Approach
+
+### Hook contract (main)
+
+- The pre-spawn hook resolves to a list of per-step failures
+  (`{ skill: 'conventions' | 'tldr' | 'goal'; error }`) instead of throwing on
+  the first failure. New `AgentCodeManagedSkillsService.prepareForAgentSpawn`
+  runs the audit and each requested product skill **independently**. Before,
+  a failed audit or TLDR skipped Goal as well.
+- `SessionManager` stays the policy owner. It journals each failure under the
+  existing `conventions.pre_spawn_reconcile.error` name, with a `skill` field,
+  so existing triage greps keep working. It never rethrows. If the hook itself
+  throws, which is unexpected, every requested reporting skill is treated as
+  unavailable, so the warning still fires.
+- For requested reporting skills that failed, the manager journals
+  `conventions.pre_spawn_reconcile.degraded` and emits
+  `managed-skills-unavailable` `{ sessionId, skills }`. It does this only
+  AFTER the post-reconcile cancellation check, so a restore cancelled during
+  the reconcile writes no false "launching without skills" row and no toast.
+- Both the `.error` and `.degraded` rows carry `ids.sessionId`. `recordError`
+  gained an optional `ids` parameter for this, so the two rows can be joined.
+- Audit history, which matters for reading old journals: before this change an
+  audit throw shared the hook's single catch and also aborted every TLDR/Goal
+  launch. So the Sept 18 cluster could have been an audit failure. Now an
+  audit failure only goes to the journal.
+
+### Warning channel (renderer)
+
+Existing channels, by fit:
+
+- Provider conditions: interactive, provider-specific TUI states. Wrong shape.
+- Pane toast (`showPaneToast`): renderer-driven only. It would also show on
+  every restoring pane.
+- `session:*` routed channels: session-window routing quarantines events for
+  unowned ids and records routing gaps. A skill warning for a main-initiated
+  spawn would show up as a false routing gap.
+- **GlobalToast**: app-wide, click-to-dismiss, and already fed by a main→renderer
+  push (`extensions:notification`). Managed-skill health is app-wide, not
+  per-pane, so this fits best.
+
+Decision: the forwarder broadcasts `managed-skills:unavailable { skills }` to
+all windows. Preload exposes `onManagedSkillsUnavailable`, and
+`GlobalToastProvider` subscribes and shows a long (10 s), dismissible toast.
+The message comes from a pure `managedSkillsUnavailableMessage(skills)`.
+When a whole workspace restores, the single-slot toast collapses N identical
+warnings into one.
+
+Persistent state stays in Settings (custom-skill health rows). The toast only
+points there. A 60 s repeat window, per window and keyed by the skill set,
+stops the same toast from coming back on every later launch while the skill
+stays broken. A new skill set still shows immediately, and main's journal
+still records every occurrence.
+
+### `recover.failed` cause
+
+`SESSION_START_FAILURE_CAUSES` in `src/shared/lifecycle/events.ts`:
+
+- `missing-workspace`: `MissingWorkspaceDirectoryError`.
+- `cli-not-found`: new `ProviderCliNotFoundError`. Its message is unchanged,
+  because the renderer's spawn-error text reads it.
+- `provider-launch`: the provider's `start()` threw. The manager tags the
+  error through a WeakMap and does not wrap it, because callers and tests
+  depend on the original error's identity and message.
+- `unknown`: anything else.
+
+`conventions-gate` is deliberately **not** a value. After this change the gate
+cannot fail a start, so the value would never be emitted. The reconcile
+failure is still in the journal as `conventions.pre_spawn_reconcile.error`.
+
+## Tests
+
+- `src/main/sessionManager.wake.test.ts`: the two tests that asserted the old
+  abort (TLDR / Goal) now assert the new contract. The spawn resolves, the
+  provider session is created, the MCP token is **not** revoked, and
+  `managed-skills-unavailable` names the skill. A hook that throws outright
+  still spawns and warns for the requested reporting skills. A restore
+  cancelled during the reconcile neither warns nor journals `.degraded`. All
+  of these fail on origin/main.
+- `src/main/sessionManager.lifecycle.test.ts` (the diagnostic-stream suite):
+  `recover.failed` carries `cause: 'missing-workspace'` / `'cli-not-found'` /
+  `'provider-launch'` / `'unknown'`, and a provider start error's text does not
+  appear in the event. These fail on origin/main because there is no cause.
+- `AgentCodeCustomSkillsService.system.test.ts`: `prepareForAgentSpawn`
+  reports a TLDR failure (a user-owned file in the way) and still prepares
+  Goal. On main a TLDR failure skipped Goal.
+- `src/main/sessions/forwarder.test.ts`: the warning is broadcast with domain
+  names only.
+- `src/shared/types/tldr.test.ts`: `managedSkillsUnavailableMessage` names the
+  skill(s) in a fixed order and points to Settings.
+
+## Verification
+
+Once at the end: `npx tsc -b` and the touched vitest files, on Node 24.

src/main/agentCodeConventions/AgentCodeConventionsService.ts

@@ -1,8 +1,18 @@
 import {
   GOAL_INSTRUCTIONS, GOAL_SKILL_DESCRIPTION, GOAL_SKILL_NAME,
-  TLDR_INSTRUCTIONS, TLDR_SKILL_DESCRIPTION, TLDR_SKILL_NAME,
+  REPORTING_DOMAINS, TLDR_INSTRUCTIONS, TLDR_SKILL_DESCRIPTION, TLDR_SKILL_NAME,
+  type ReportingDomain,
 } from '@shared/types/tldr.js'
 
+/** One step of the pre-spawn reconcile that did not succeed (#1133).
+ * `conventions` is the machine-wide audit of personal/custom/installed skills.
+ * The reporting domains are the product skills a launching agent asked for.
+ * `error` is the raw exception. It is journaled in main and never crosses IPC. */
+export type ManagedSkillPreparationFailure = {
+  skill: 'conventions' | ReportingDomain
+  error: unknown
+}
+
 // Product-owned skills (TLDR #888, Goal #936). They ride the same write-ahead
 // ownership journal as personal skills but belong to an MCP capability, so
 // users cannot create, edit, disable, or delete them, and their names are
@@ -816,6 +826,45 @@ export class AgentCodeManagedSkillsService {
     return this.ensureProductSkill(PRODUCT_SKILLS[1]!)
   }
 
+  /**
+   * The pre-spawn reconcile SessionManager runs before every agent launch
+   * (#1133). Resolves with what could not be prepared. It never rejects on a
+   * step failure.
+   *
+   * WHY each step runs independently rather than stopping at the first throw:
+   * the previous launcher ran audit → TLDR → Goal in one chain, so a failed
+   * audit or a failed TLDR skill also skipped Goal. That was harmless while any
+   * failure aborted the whole spawn anyway. Now the agent launches regardless,
+   * so a step that was never attempted would be a skill silently dropped for no
+   * reason of its own. Every step runs through `serialize`, so running them
+   * one after another here keeps the write-ahead journal ordering the chain had.
+   *
+   * WHY failures are returned instead of logged here: SessionManager owns the
+   * launch policy and the journal/lifecycle stream for the session, and it
+   * decides which failures the user has to hear about. This method only
+   * reports facts. Raw errors stay in main. The caller journals them and never
+   * forwards their text over IPC.
+   */
+  async prepareForAgentSpawn(
+    domains: readonly string[] | undefined,
+  ): Promise<ManagedSkillPreparationFailure[]> {
+    const failures: ManagedSkillPreparationFailure[] = []
+    try {
+      await this.audit()
+    } catch (error) {
+      failures.push({ skill: 'conventions', error })
+    }
+    for (const domain of REPORTING_DOMAINS) {
+      if (!domains?.includes(domain)) continue
+      try {
+        await (domain === 'tldr' ? this.ensureTldrSkill() : this.ensureGoalSkill())
+      } catch (error) {
+        failures.push({ skill: domain, error })
+      }
+    }
+    return failures
+  }
+
   private ensureProductSkill(product: ProductSkill): Promise<void> {
     return this.serialize(async () => {
       await this.ensureInitializedLocked()

src/main/agentCodeConventions/AgentCodeCustomSkillsService.system.test.ts

@@ -560,6 +560,34 @@ describe('product-owned Goal skill', () => {
   })
 })
 
+describe('pre-spawn preparation (#1133)', () => {
+  // The launcher used to chain audit → TLDR → Goal and let the first throw end
+  // the chain, which was harmless only while any throw aborted the spawn.
+  // Agents now launch regardless, so a broken TLDR skill must not also cost
+  // the agent its Goal skill. On origin/main `prepareForAgentSpawn` does not
+  // exist; the old chain skipped Goal.
+  it('reports a TLDR failure and still prepares the Goal skill', async () => {
+    const { service, targets } = await harness()
+    const file = customPath(targets[0]!, 'agent-code-tldr')
+    await writeFileWithParents(file, 'User owned instructions')
+
+    const failures = await service.prepareForAgentSpawn(['tldr', 'goal'])
+
+    expect(failures).toEqual([
+      { skill: 'tldr', error: expect.objectContaining({ message: expect.stringContaining('TLDR skill deployment failed') }) },
+    ])
+    expect(await readFile(file, 'utf8')).toBe('User owned instructions')
+    const snapshot = await service.getCustomSkillsSnapshot()
+    expect(snapshot.skills.find(skill => skill.managedBy === 'goal')?.health).toBe('active')
+  })
+
+  it('touches only the product skills the launching agent asked for', async () => {
+    const { service } = await harness()
+    await expect(service.prepareForAgentSpawn(undefined)).resolves.toEqual([])
+    expect((await service.getCustomSkillsSnapshot()).skills).toEqual([])
+  })
+})
+
 describe('product skills with an unsupported registered provider', () => {
   // Regression for #1014: grok (registered, personalAgentSkills.supported:false)
   // must not stop the TLDR product skill from deploying — pre-spawn reconcile

src/main/incident/AppRunJournal.ts

@@ -18,6 +18,7 @@ import type {
   AppRunIncidentInput,
   AppRunJournalEvent,
   AppRunJournalEventInput,
+  AppRunJournalIds,
   AppRunJournalManifest,
 } from '@main/incident/journalTypes.js'
 import { sanitizePerformanceData } from '@shared/performance/serialization.js'
@@ -215,11 +216,17 @@ export class AppRunJournal {
     this.pending.push(event)
   }
 
-  recordError(name: string, error: unknown, data?: Record<string, unknown>): void {
+  // `ids` is optional so the ~all existing callers stay untouched. It exists
+  // because an error row that belongs to one session must be joinable on
+  // `ids.sessionId` like that session's other rows (#1133 review). A sessionId
+  // tucked into `data` is invisible to a triage filter on ids, so the error
+  // that explains a `.degraded` row could not be found next to it.
+  recordError(name: string, error: unknown, data?: Record<string, unknown>, ids?: AppRunJournalIds): void {
     this.record({
       area: areaFromName(name),
       name,
       severity: 'error',
+      ...(ids ? { ids } : {}),
       data: {
         ...data,
         error: normalizeError(error),

src/main/index.ts

@@ -1030,11 +1030,10 @@ async function startApp(): Promise<void> {
     tmuxAvailable ? tmuxRegistry : null,
     builtInMcpHost,
     appRunJournal,
-    async options => {
-      await agentCodeConventionsService.audit()
-      if (options.builtInMcpDomains?.includes('tldr')) await agentCodeConventionsService.ensureTldrSkill()
-      if (options.builtInMcpDomains?.includes('goal')) await agentCodeConventionsService.ensureGoalSkill()
-    },
+    // Reports failures instead of throwing them (#1133). SessionManager
+    // decides what a failure means for the launch; see
+    // runPreSpawnSkillReconcile for why that is never "abort".
+    options => agentCodeConventionsService.prepareForAgentSpawn(options.builtInMcpDomains),
     (sessionId, sessionRunId, observation) => {
       sessionRecorders?.recordCodexTranscriptObservation(
         sessionId,

src/main/sessionManager.lifecycle.test.ts

@@ -32,8 +32,23 @@ vi.mock('@providers/registry.main.js', () => ({
   getMainProvider: () => ({ createSession, deliverPrompt }),
 }))
 
+// Mutable so the cli-not-found case can make lookup fail. The resolver and
+// setup-state writes are mocked too, because a failed cached lookup triggers a
+// late PATH re-resolve that would otherwise probe the developer's real
+// machine, where the CLI may well be installed.
+const toolchain = vi.hoisted(() => ({ path: '/usr/bin/true' }))
+
 vi.mock('@main/setup/toolchain.js', () => ({
-  getToolPath: () => '/usr/bin/true',
+  getToolPath: () => toolchain.path,
+  refreshToolchainFromState: vi.fn(async () => {}),
+}))
+
+vi.mock('@main/setup/binaryResolver.js', () => ({
+  resolveToolPath: vi.fn(async () => null),
+}))
+
+vi.mock('@main/setup/setupState.js', () => ({
+  updateToolPaths: vi.fn(async () => {}),
 }))
 
 vi.mock('@main/performance/PerformanceService.js', () => ({
@@ -76,6 +91,70 @@ describe('SessionManager lifecycle journal', () => {
     createSession.mockReset()
     createSession.mockImplementation(() => new FakeAgentSession())
     deliverPrompt.mockReset()
+    toolchain.path = '/usr/bin/true'
+  })
+
+  describe('recover.failed cause (#1133)', () => {
+    // `code: 'start-failed'` alone made two workspace-wide outages readable
+    // only by lining up `seq` numbers against a neighbouring reconcile error.
+    // Each case pins one closed-enum cause from TYPED evidence, and checks that
+    // the exception text never reaches the stream. On origin/main there is no
+    // `cause` key at all.
+    it('classifies a deleted workspace folder as missing-workspace', async () => {
+      const { assertWorkspaceDirectoryExists, MissingWorkspaceDirectoryError } = await import('@main/workspaceDirectory.js')
+      vi.mocked(assertWorkspaceDirectoryExists).mockRejectedValueOnce(
+        new MissingWorkspaceDirectoryError('/tmp/deleted-worktree'),
+      )
+      const { SessionManager } = await import('./sessionManager')
+      const spy = journalSpy()
+      const manager = new SessionManager(null, null, spy.journal as never)
+
+      await manager.recover({ sessionId: 'gone', kind: 'claude', cwd: '/tmp/deleted-worktree' })
+
+      expect(spy.find('recover.failed')?.data).toMatchObject({ code: 'start-failed', cause: 'missing-workspace' })
+    })
+
+    it('classifies an unresolvable provider CLI as cli-not-found', async () => {
+      toolchain.path = ''
+      const { SessionManager } = await import('./sessionManager')
+      const spy = journalSpy()
+      const manager = new SessionManager(null, null, spy.journal as never)
+
+      const result = await manager.recover({ sessionId: 'no-cli', kind: 'codex', cwd: '/tmp/project' })
+
+      expect(result).toMatchObject({ ok: false, code: 'start-failed' })
+      expect(createSession).not.toHaveBeenCalled()
+      expect(spy.find('recover.failed')?.data).toMatchObject({ cause: 'cli-not-found' })
+    })
+
+    it('classifies a provider whose start() throws as provider-launch, without its message', async () => {
+      createSession.mockImplementation(() => {
+        const session = new FakeAgentSession()
+        session.start.mockRejectedValueOnce(new Error('spawn failed: TOKEN=secret-value'))
+        return session
+      })
+      const { SessionManager } = await import('./sessionManager')
+      const spy = journalSpy()
+      const manager = new SessionManager(null, null, spy.journal as never)
+
+      await manager.recover({ sessionId: 'crashy', kind: 'claude', cwd: '/tmp/project' })
+
+      const failed = spy.find('recover.failed')
+      expect(failed?.data).toMatchObject({ code: 'start-failed', cause: 'provider-launch' })
+      expect(JSON.stringify(failed)).not.toContain('secret-value')
+    })
+
+    it('falls back to unknown for a failure no boundary classified', async () => {
+      const { SessionManager } = await import('./sessionManager')
+      const spy = journalSpy()
+      // No built-in MCP host, but MCP domains requested: the spawn throws before
+      // the provider exists, and no typed boundary owns that error.
+      const manager = new SessionManager(null, null, spy.journal as never)
+
+      await manager.recover({ sessionId: 'no-host', kind: 'claude', cwd: '/tmp/project', builtInMcpDomains: ['tldr'] })
+
+      expect(spy.find('recover.failed')?.data).toMatchObject({ cause: 'unknown' })
+    })
   })
 
   it('records the full cold-start ladder in order', async () => {