From 6e237f6d70be3f9ad5aa7e2a4f97272b5f350b66 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 22 Sep 2026 17:00:46 -0700 Subject: [PATCH 1/3] docs(sessions): plan for the managed-skill pre-spawn gate blast radius Co-Authored-By: Claude Opus 5.5 (1M context) --- .../2026-09-23-skill-gate-blast-radius.md | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md diff --git a/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md b/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md new file mode 100644 index 000000000..40f481410 --- /dev/null +++ b/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md @@ -0,0 +1,122 @@ +# 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 emits + `managed-skills-unavailable` `{ sessionId, skills }`. + +### 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. + +### `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. Both fail on + origin/main. +- `src/main/sessionManager.recover.test.ts`: `recover.failed` carries + `cause: 'missing-workspace'` / `'cli-not-found'` / `'provider-launch'`, + asserted through the lifecycle journal. These fail on origin/main because + there is no cause. +- `AgentCodeConventionsService` system test: `prepareForAgentSpawn` reports a + TLDR failure and still prepares Goal. On main a TLDR failure skipped Goal. +- Renderer: `managedSkillsUnavailableMessage` names the skill(s) and Settings. + +## Verification + +Once at the end: `npx tsc -b` and the touched vitest files, on Node 24. From 7aa30962d56bfb868d944640afb3a8be3c2d325a Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 22 Sep 2026 17:05:25 -0700 Subject: [PATCH 2/3] fix(sessions): a managed-skill reconcile failure no longer stops agents from starting The pre-spawn managed-skills reconcile rethrew for any session with TLDR or Goal, which is nearly every pane, so one broken skill stopped every agent of every provider (two outages from unrelated causes on Sept 18 and 19). Agents now launch without the broken skill. The MCP tools still carry their own server instructions, so what is lost is guidance, not the capability. - AgentCodeManagedSkillsService.prepareForAgentSpawn runs the audit and each requested product skill independently and reports failures instead of throwing, so a broken TLDR skill no longer also skips Goal. - SessionManager journals each failure (with the skill) and emits managed-skills-unavailable. The forwarder broadcasts it, and GlobalToast names the skill and points to Settings > Agents > Custom Skills. - recover.failed carries a closed-enum cause (missing-workspace, cli-not-found, provider-launch, unknown). Raw error text stays out of the stream. Fixes #1133 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../2026-09-23-skill-gate-blast-radius.md | 20 +- .../AgentCodeConventionsService.ts | 51 ++++- ...gentCodeCustomSkillsService.system.test.ts | 28 +++ src/main/index.ts | 9 +- src/main/sessionManager.lifecycle.test.ts | 81 +++++++- src/main/sessionManager.ts | 174 ++++++++++++++++-- src/main/sessionManager.wake.test.ts | 108 +++++++++-- src/main/sessions/forwarder.test.ts | 18 ++ src/main/sessions/forwarder.ts | 15 ++ src/preload/api/agentCodeCustomSkills.ts | 13 ++ src/renderer/src/ui/GlobalToast.tsx | 17 ++ src/shared/lifecycle/events.ts | 41 +++++ src/shared/types/tldr.test.ts | 21 +++ src/shared/types/tldr.ts | 38 ++++ 14 files changed, 591 insertions(+), 43 deletions(-) create mode 100644 src/shared/types/tldr.test.ts diff --git a/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md b/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md index 40f481410..437c5af57 100644 --- a/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md +++ b/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md @@ -107,15 +107,19 @@ failure is still in the journal as `conventions.pre_spawn_reconcile.error`. 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. Both fail on + still spawns and warns for the requested reporting skills. All of these fail on origin/main. -- `src/main/sessionManager.recover.test.ts`: `recover.failed` carries - `cause: 'missing-workspace'` / `'cli-not-found'` / `'provider-launch'`, - asserted through the lifecycle journal. These fail on origin/main because - there is no cause. -- `AgentCodeConventionsService` system test: `prepareForAgentSpawn` reports a - TLDR failure and still prepares Goal. On main a TLDR failure skipped Goal. -- Renderer: `managedSkillsUnavailableMessage` names the skill(s) and Settings. +- `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 diff --git a/src/main/agentCodeConventions/AgentCodeConventionsService.ts b/src/main/agentCodeConventions/AgentCodeConventionsService.ts index 8c4099ae4..407c4fb4b 100644 --- a/src/main/agentCodeConventions/AgentCodeConventionsService.ts +++ b/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 { + 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 { return this.serialize(async () => { await this.ensureInitializedLocked() diff --git a/src/main/agentCodeConventions/AgentCodeCustomSkillsService.system.test.ts b/src/main/agentCodeConventions/AgentCodeCustomSkillsService.system.test.ts index 97e69e2c8..d4e4fd30c 100644 --- a/src/main/agentCodeConventions/AgentCodeCustomSkillsService.system.test.ts +++ b/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 diff --git a/src/main/index.ts b/src/main/index.ts index e2841c133..4d69ab6f7 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -1030,11 +1030,10 @@ async function startApp(): Promise { 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, diff --git a/src/main/sessionManager.lifecycle.test.ts b/src/main/sessionManager.lifecycle.test.ts index 07a4e2218..a205ae74a 100644 --- a/src/main/sessionManager.lifecycle.test.ts +++ b/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 () => { diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index 37d4055b0..e9d539364 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -1,7 +1,9 @@ import { mainOperations } from '@main/performance/operations.js' import { ResponseTracker } from '@shared/performance/responseTracker.js' import { randomUUID } from 'crypto' -import { hasReportingDomain } from '@shared/types/tldr.js' +import { REPORTING_DOMAINS, type ReportingDomain } from '@shared/types/tldr.js' +import type { SessionStartFailureCause } from '@shared/lifecycle/events.js' +import type { ManagedSkillPreparationFailure } from '@main/agentCodeConventions/AgentCodeConventionsService.js' import { EventEmitter } from 'events' import path from 'node:path' import { performance } from 'perf_hooks' @@ -209,6 +211,10 @@ type ManagerEvents = { 'semantic-event': [{ sessionId: string; event: unknown }] /** Internal cleanup signal emitted exactly before a session leaves the manager. */ removed: [{ sessionId: string }] + /** A launching agent asked for product skills (TLDR/Goal) that the pre-spawn + * reconcile could not prepare, and it is being started without them (#1133). + * Carries domain names only, never the error: see the pre-spawn gate. */ + 'managed-skills-unavailable': [{ sessionId: string; skills: ReportingDomain[] }] exit: [{ sessionId: string; exitCode: number; signal?: number }] } @@ -351,6 +357,53 @@ class RecoveryCancelledError extends Error { } } +/** + * The provider CLI could not be resolved to an absolute path (#495 A9). + * + * WHY a class when the message was already specific: `recover.failed` needs + * a typed `cause` (#1133), and classifying by message substring would quietly + * turn into `unknown` the day someone rewords the Setup hint. The message itself + * is unchanged, because the renderer's spawn-error text reads it. + */ +export class ProviderCliNotFoundError extends Error { + constructor(kind: string) { + // Names a real place (#995): Setup opens from File › Setup… or the + // "Open Setup" command, and shows the install command for this CLI. + super(`${kind} CLI not found. Open Setup (File › Setup…) to install it or enter its path.`) + this.name = 'ProviderCliNotFoundError' + } +} + +/** + * Start-failure causes attached to errors at the boundary that knows them. + * + * WHY a WeakMap tag instead of wrapping the error in a typed class: provider + * start errors propagate through the Codex replacement compensation and back + * to callers and tests that match on the ORIGINAL error (identity and message). + * Wrapping would change what every one of them sees just so a journal field + * could be filled. The tag is invisible to everyone except + * `classifyStartFailure`, and it is garbage-collected with the error. + * Non-object throws cannot be tagged and fall through to `unknown`. + */ +const startFailureCauses = new WeakMap() + +function tagStartFailure(error: unknown, cause: SessionStartFailureCause): void { + // First tag wins: an error re-thrown through an outer boundary must keep the + // most specific layer that saw it first. + if (typeof error === 'object' && error !== null && !startFailureCauses.has(error)) { + startFailureCauses.set(error, cause) + } +} + +function classifyStartFailure(error: unknown): SessionStartFailureCause { + if (error instanceof MissingWorkspaceDirectoryError) return 'missing-workspace' + if (error instanceof ProviderCliNotFoundError) return 'cli-not-found' + if (typeof error === 'object' && error !== null) { + return startFailureCauses.get(error) ?? 'unknown' + } + return 'unknown' +} + function createRegistryLifecycle(generation: symbol): RegistryLifecycle { return { runId: randomUUID(), @@ -623,7 +676,13 @@ export class SessionManager extends EventEmitter { // Always-on incident journal. Optional so tests / non-journaled callers // still construct cleanly; null-guarded at every use. private readonly journal: AppRunJournal | null = null, - private readonly beforeAgentSessionStart: ((options: SessionSpawnOptions) => Promise) | null = null, + // The managed-skills pre-spawn reconcile (#1133). It resolves with the steps + // it could not complete. `void` is accepted for hooks with nothing to report + // (tests that only gate timing). A rejection is tolerated too; see the gate + // in spawnWithId for how each outcome is handled. + private readonly beforeAgentSessionStart: + | ((options: SessionSpawnOptions) => Promise) + | null = null, // Optional opt-in recording projection. Kept as a structural callback so // the process/session registry does not depend on the dev-debug recorder. private readonly recordCodexObservation: ( @@ -1935,6 +1994,13 @@ export class SessionManager extends EventEmitter { this.lifecycle.session('recover.failed', options.sessionId, { kind: claim.kind, code: 'start-failed', + // #1133: `start-failed` alone could not tell a deleted worktree from a + // missing CLI from a provider crash, so two workspace-wide outages had + // to be reconstructed by matching `seq` numbers. The cause is a closed + // enum (SESSION_START_FAILURE_CAUSES) for the same privacy reason the + // message below is flattened: the raw exception never enters this + // stream. + cause: classifyStartFailure(error), lifecycle: 'spawning', durationMs: performance.now() - claim.startedAt, }) @@ -2514,6 +2580,87 @@ export class SessionManager extends EventEmitter { } } + /** + * Run the managed-skills reconcile before an agent launch. It never fails the + * launch (#1133). + * + * WHY this can no longer abort a spawn, although it used to for TLDR/Goal: + * the old rule ("TLDR and Goal each promise a managed skill, so do not launch + * while silently omitting it") treated the skill as part of the capability. + * It is not. The TLDR and Goal MCP servers deliver their own server + * instructions with the tools, so an agent without the skill file loses some + * GUIDANCE about when to call them, not the ability to call them. Almost + * every pane has TLDR or Goal enabled, though, so that rule turned ANY + * managed-skill failure into a workspace-wide outage: every pane of every + * provider refused to start, while each restored transcript made the + * workspace look healthy. It happened twice in two days from two unrelated + * causes: a failed TLDR deployment on Sept 18 (on a build before grok, so + * the cause was never recovered), and grok's unsupported-provider health on + * Sept 19 (#1014). A degraded agent the user is told about beats N dead + * agents they are not. Launching without the skill is the default the user + * accepted in #1133. If a strict mode is ever wanted, it belongs behind an + * explicit setting, not back as the silent default. + * + * WHY not silently either: the old strictness existed so an omitted skill + * would never go unnoticed. That goal stays. Instead of refusing the launch, + * the manager emits `managed-skills-unavailable`, which the renderer shows as + * a toast naming the skill and pointing at Settings, where its health rows + * explain the cause. + * + * WHY the raw error only reaches the journal: reconcile errors can quote + * file paths in the user's provider skill folders. The journal is main-only. + * The warning carries domain names, and the renderer composes its own text. + */ + private async runPreSpawnSkillReconcile( + sessionId: string, + options: SessionSpawnOptions, + ): Promise { + if (!this.beforeAgentSessionStart) return + const requested = REPORTING_DOMAINS.filter(domain => options.builtInMcpDomains?.includes(domain)) + let failures: readonly ManagedSkillPreparationFailure[] + try { + failures = (await this.beforeAgentSessionStart(options)) ?? [] + } catch (error) { + // The hook is contracted to report failures, not throw them, so a throw + // is a bug or an unexpected I/O fault. Nothing says which skills were + // prepared, so assume none of the requested ones were. Under-warning + // here would bring back exactly the silent omission the old strict rule + // guarded against. + failures = requested.length > 0 + ? requested.map(skill => ({ skill, error })) + : [{ skill: 'conventions', error }] + } + for (const failure of failures) { + // Same event name as before #1133, so triage that already greps for it + // keeps working. `skill` says which step failed, which the old single + // catch could not. + this.journal?.recordError('conventions.pre_spawn_reconcile.error', failure.error, { + sessionId, + skill: failure.skill, + }) + } + // Only skills this agent ASKED for are worth interrupting the user about. A + // failed machine-wide audit (`conventions`) was always nonfatal and + // journal-only, and its degraded health already shows in Settings. A + // failure for a domain the launch did not request cannot happen with the + // production hook, and would not affect this agent anyway. + const unavailable = REPORTING_DOMAINS.filter(domain => + requested.includes(domain) && failures.some(failure => failure.skill === domain), + ) + if (unavailable.length === 0) return + // The launch-side fact that pairs with the `.error` rows above: "this + // session started WITHOUT these skills". Before #1133 the next row was + // `recover.failed`. Joined into a string because journal data stays flat. + this.journal?.record({ + area: 'conventions.pre_spawn_reconcile', + name: 'conventions.pre_spawn_reconcile.degraded', + severity: 'warn', + ids: { sessionId }, + data: { skills: unavailable.join(',') }, + }) + this.emit('managed-skills-unavailable', { sessionId, skills: unavailable }) + } + /** * Start a backend under an already-owned local routing id. * @@ -2642,9 +2789,7 @@ export class SessionManager extends EventEmitter { } } if (!binary) { - // Names a real place (#995): Setup opens from File › Setup… or the - // "Open Setup" command, and shows the install command for this CLI. - throw new Error(`${kind} CLI not found. Open Setup (File › Setup…) to install it or enter its path.`) + throw new ProviderCliNotFoundError(kind) } const initialSize = { cols: options.cols ?? 120, @@ -2666,18 +2811,7 @@ export class SessionManager extends EventEmitter { } this.throwIfSpawnCancelled(recoveryClaim, codexReplacementHandoff) if (this.beforeAgentSessionStart) { - try { - // Conventions reconciliation is a best-effort compatibility boundary, - // with degraded/conflict health in Settings. TLDR opts into a stricter - // launch contract below because its requested reporting skill is part - // of the capability, not an optional personal convention. - await this.beforeAgentSessionStart(options) - } catch (error) { - this.journal?.recordError('conventions.pre_spawn_reconcile.error', error) - // TLDR and Goal each promise a managed skill. Do not launch an - // enabled session while silently omitting that requested contract. - if (hasReportingDomain(options.builtInMcpDomains)) throw error - } + await this.runPreSpawnSkillReconcile(sessionId, options) this.throwIfSpawnCancelled(recoveryClaim, codexReplacementHandoff) } const provider = getMainProvider(kind) @@ -3039,6 +3173,11 @@ export class SessionManager extends EventEmitter { provider: kind, }) } catch (err) { + // Tagged here, not at recover(): this catch is the only place that + // knows the provider runtime existed and its start() is what threw. + // A cancellation that surfaces here is excluded because recover + // reports it as `recover.cancelled`, never `recover.failed`. + if (!(err instanceof RecoveryCancelledError)) tagStartFailure(err, 'provider-launch') await this.settleEntryStart(sessionId, agentEntry) this.lifecycle.session('provider.start.end', sessionId, { kind, @@ -3258,6 +3397,7 @@ export class SessionManager extends EventEmitter { // a half-dead TerminalSession that callers might still try to // write()/resize()/kill(). Roll back everything we added in // THIS spawn so the caller can retry from a clean slate. + if (!(err instanceof RecoveryCancelledError)) tagStartFailure(err, 'provider-launch') await this.settleEntryStart(sessionId, terminalEntry) performanceService.error('session.spawn.terminalStart.error', err, { sessionId }) throw err diff --git a/src/main/sessionManager.wake.test.ts b/src/main/sessionManager.wake.test.ts index c95222527..4f244455c 100644 --- a/src/main/sessionManager.wake.test.ts +++ b/src/main/sessionManager.wake.test.ts @@ -200,26 +200,112 @@ describe('SessionManager restart wake recovery', () => { expect(createSession).toHaveBeenCalledTimes(1) }) - it('fails a TLDR-enabled launch and revokes its token if the reporting skill cannot deploy', async () => { + // #1133. Before this, a managed-skill failure with TLDR or Goal requested + // REJECTED the spawn. Almost every pane requests one of them, so a single + // broken skill stopped every agent of every provider (Sept 18 and Sept 19, + // 2026). These pin the replacement contract: the agent starts with its MCP + // capability intact, the failure is journaled, and the renderer is told which + // skill is missing. On origin/main the spawn rejects and none of this holds. + function journalSpy() { + return { record: vi.fn(), recordError: vi.fn() } + } + + it('launches a TLDR agent without the skill when the TLDR skill cannot deploy, and says so', async () => { const { SessionManager } = await import('./sessionManager') const host = { registerSession: vi.fn((_scope: { sessionId: string }) => []), revokeSession: vi.fn() } - const reconcile = vi.fn(async () => { throw new Error('TLDR skill destination is user-owned') }) - const manager = new SessionManager(null, host as unknown as BuiltInMcpHttpHost, null, reconcile) + const journal = journalSpy() + const skillError = new Error('TLDR skill destination is user-owned') + const reconcile = vi.fn(async () => [{ skill: 'tldr' as const, error: skillError }]) + const manager = new SessionManager(null, host as unknown as BuiltInMcpHttpHost, journal as never, reconcile) + const warnings: unknown[] = [] + manager.on('managed-skills-unavailable', event => warnings.push(event)) const options = { kind: 'codex' as const, cwd: '/tmp/project', builtInMcpDomains: ['tldr' as const], tldrIdentity: 'summary-agent' } - await expect(manager.spawn(options)).rejects.toThrow('TLDR skill destination is user-owned') + + const result = await manager.spawn(options) + expect(reconcile).toHaveBeenCalledWith(options) - expect(createSession).not.toHaveBeenCalled() - expect(host.revokeSession).toHaveBeenCalledWith(host.registerSession.mock.calls[0]![0].sessionId) + expect(createSession).toHaveBeenCalledTimes(1) + expect(manager.getBackendSnapshot(result.sessionId)).toMatchObject({ lifecycle: 'live' }) + // The MCP capability survives: its session-scoped token must NOT be revoked + // the way the old abort path revoked it. Losing the skill costs guidance, + // and the tool still brings its own server instructions. + expect(host.revokeSession).not.toHaveBeenCalled() + expect(warnings).toEqual([{ sessionId: result.sessionId, skills: ['tldr'] }]) + // The raw error stays in main's journal, keyed by which step failed. + expect(journal.recordError).toHaveBeenCalledWith( + 'conventions.pre_spawn_reconcile.error', + skillError, + { sessionId: result.sessionId, skill: 'tldr' }, + ) + expect(journal.record).toHaveBeenCalledWith(expect.objectContaining({ + name: 'conventions.pre_spawn_reconcile.degraded', + ids: { sessionId: result.sessionId }, + data: { skills: 'tldr' }, + })) }) - it('fails a Goal-only launch the same way when the goal skill cannot deploy', async () => { + it('launches a Goal-only agent the same way when the Goal skill cannot deploy', async () => { const { SessionManager } = await import('./sessionManager') const host = { registerSession: vi.fn((_scope: { sessionId: string }) => []), revokeSession: vi.fn() } - const reconcile = vi.fn(async () => { throw new Error('Goal skill destination is user-owned') }) + const reconcile = vi.fn(async () => [{ skill: 'goal' as const, error: new Error('Goal skill destination is user-owned') }]) const manager = new SessionManager(null, host as unknown as BuiltInMcpHttpHost, null, reconcile) - await expect(manager.spawn({ kind: 'claude', cwd: '/tmp/project', builtInMcpDomains: ['goal'] })).rejects.toThrow('Goal skill destination is user-owned') - expect(createSession).not.toHaveBeenCalled() - expect(host.revokeSession).toHaveBeenCalledWith(host.registerSession.mock.calls[0]![0].sessionId) + const warnings: unknown[] = [] + manager.on('managed-skills-unavailable', event => warnings.push(event)) + + const result = await manager.spawn({ kind: 'claude', cwd: '/tmp/project', builtInMcpDomains: ['goal'] }) + + expect(createSession).toHaveBeenCalledTimes(1) + expect(host.revokeSession).not.toHaveBeenCalled() + expect(warnings).toEqual([{ sessionId: result.sessionId, skills: ['goal'] }]) + }) + + it('still launches and warns for every requested skill when the reconcile hook itself throws', async () => { + // The production hook reports failures instead of throwing them, so a throw + // means nobody knows which skills were prepared. Assuming none were keeps + // the user informed, where silence would bring back the omission the old + // strict rule existed to prevent. + const { SessionManager } = await import('./sessionManager') + const host = { registerSession: vi.fn((_scope: { sessionId: string }) => []), revokeSession: vi.fn() } + const journal = journalSpy() + const reconcile = vi.fn(async () => { throw new Error('managed skills state unreadable') }) + const manager = new SessionManager(null, host as unknown as BuiltInMcpHttpHost, journal as never, reconcile) + const warnings: unknown[] = [] + manager.on('managed-skills-unavailable', event => warnings.push(event)) + + const result = await manager.recover({ + sessionId: 'restored-reporting-pane', + kind: 'claude', + cwd: '/tmp/project', + builtInMcpDomains: ['goal', 'tldr'], + }) + + expect(result).toMatchObject({ ok: true, disposition: 'spawned' }) + expect(createSession).toHaveBeenCalledTimes(1) + // Fixed order, whatever order the domains were requested in. + expect(warnings).toEqual([{ sessionId: 'restored-reporting-pane', skills: ['tldr', 'goal'] }]) + expect(journal.recordError).toHaveBeenCalledTimes(2) + }) + + it('does not warn when only the machine-wide audit failed', async () => { + // A failed personal-conventions audit was always nonfatal and journal-only. + // Its health is in Settings, and it does not remove anything this agent + // asked for, so it must not start toasting on every launch. + const { SessionManager } = await import('./sessionManager') + const journal = journalSpy() + const reconcile = vi.fn(async () => [{ skill: 'conventions' as const, error: new Error('audit failed') }]) + const manager = new SessionManager(null, null, journal as never, reconcile) + const warnings: unknown[] = [] + manager.on('managed-skills-unavailable', event => warnings.push(event)) + + await manager.spawn({ kind: 'codex', cwd: '/tmp/project' }) + + expect(createSession).toHaveBeenCalledTimes(1) + expect(warnings).toEqual([]) + expect(journal.recordError).toHaveBeenCalledWith( + 'conventions.pre_spawn_reconcile.error', + expect.any(Error), + expect.objectContaining({ skill: 'conventions' }), + ) }) it('joins a second wake while the first backend recovery is still starting', async () => { diff --git a/src/main/sessions/forwarder.test.ts b/src/main/sessions/forwarder.test.ts index b05153e65..eeeba4928 100644 --- a/src/main/sessions/forwarder.test.ts +++ b/src/main/sessions/forwarder.test.ts @@ -55,3 +55,21 @@ it('delivers understood records before a fatal error through both real coalescer manager.removeAllListeners() } }) + +it('broadcasts a managed-skill warning to every window, carrying domain names only (#1133)', async () => { + // Broadcast, not session routing: the skill fault is machine-wide, and the + // session router would quarantine (and record a false routing gap for) a + // warning about an id no window has claimed yet. The payload is the renderer's + // entire input. The sessionId stays in main, and the reconcile error never + // reaches this event in the first place. + const { broadcastToWindows } = await import('@main/window/windowRegistry.js') + vi.mocked(broadcastToWindows).mockClear() + const manager = new EventEmitter() as SessionManager & EventEmitter + wireSessionForwarder(manager, new EventEmitter() as LspManager) + try { + manager.emit('managed-skills-unavailable', { sessionId: 'pane', skills: ['tldr', 'goal'] }) + expect(broadcastToWindows).toHaveBeenCalledWith('managed-skills:unavailable', { skills: ['tldr', 'goal'] }) + } finally { + manager.removeAllListeners() + } +}) diff --git a/src/main/sessions/forwarder.ts b/src/main/sessions/forwarder.ts index 0c42ee06c..1d09578e7 100644 --- a/src/main/sessions/forwarder.ts +++ b/src/main/sessions/forwarder.ts @@ -2,6 +2,10 @@ import type { SessionManager } from '@main/sessionManager.js' import { aliasScreenSnapshotForWire } from '@shared/types/session.js' import type { AgentScreenSnapshot } from '@shared/types/session.js' import type { LspManager } from '@main/lspManager.js' +import { + MANAGED_SKILLS_UNAVAILABLE_CHANNEL, + type ManagedSkillsUnavailableEvent, +} from '@shared/types/tldr.js' import { broadcastToWindows, @@ -177,6 +181,17 @@ export function wireSessionForwarder( }) manager.on('exit', payload => { sendToSessionWindow(payload.sessionId, 'session:exit', payload) + }) + // #1133. BROADCAST, not sendToSessionWindow, on purpose. Managed-skill health + // is machine-wide (one broken TLDR skill affects every window's next launch), + // so the warning is not about one pane. Session routing would also be wrong + // mechanically: it quarantines events for ids no window has claimed yet and + // records a routing gap for them, so a main-initiated spawn would raise a + // false "missed session events" notice instead of this warning. Only domain + // names cross, never the reconcile error (see runPreSpawnSkillReconcile). + manager.on('managed-skills-unavailable', ({ skills }) => { + const event: ManagedSkillsUnavailableEvent = { skills } + broadcastToWindows(MANAGED_SKILLS_UNAVAILABLE_CHANNEL, event) }) // Diagnostics are keyed by file, not by session: two windows can have the // same file open in their editors and both need them. diff --git a/src/preload/api/agentCodeCustomSkills.ts b/src/preload/api/agentCodeCustomSkills.ts index b4a3ed3fb..6b957b9eb 100644 --- a/src/preload/api/agentCodeCustomSkills.ts +++ b/src/preload/api/agentCodeCustomSkills.ts @@ -1,5 +1,12 @@ import { ipcRenderer } from 'electron' +import { subscribe } from '@preload/api/ipc.js' +import type { Unsub } from '@preload/api/types.js' +import { + MANAGED_SKILLS_UNAVAILABLE_CHANNEL, + type ManagedSkillsUnavailableEvent, +} from '@shared/types/tldr.js' + import type { AgentCodeCustomSkillDraft, AgentCodeCustomSkillPreviewResult, @@ -45,4 +52,10 @@ export const agentCodeCustomSkillsApi = { ipcRenderer.invoke('agent-code-custom-skills:reveal-recovery'), resetAgentCodeCustomSkillsRecovery: (): Promise => ipcRenderer.invoke('agent-code-custom-skills:reset-recovery'), + // #1133: an agent was launched without a product skill (TLDR/Goal) it asked + // for, because the pre-spawn reconcile could not prepare it. It lives here + // because TLDR and Goal ARE custom skills ("Managed by … MCP" rows), and this + // module is where the renderer already talks to them. + onManagedSkillsUnavailable: (cb: (event: ManagedSkillsUnavailableEvent) => void): Unsub => + subscribe(MANAGED_SKILLS_UNAVAILABLE_CHANNEL, cb), } diff --git a/src/renderer/src/ui/GlobalToast.tsx b/src/renderer/src/ui/GlobalToast.tsx index 83095557e..cde01c3b3 100644 --- a/src/renderer/src/ui/GlobalToast.tsx +++ b/src/renderer/src/ui/GlobalToast.tsx @@ -1,6 +1,7 @@ import { createContext, useCallback, useContext, useEffect, useRef, useState } from 'react' import { useAppStore } from '@renderer/app-state/hooks' +import { managedSkillsUnavailableMessage } from '@shared/types/tldr' // GlobalToast — app-wide toast system rendered in the top-right corner. // @@ -61,6 +62,22 @@ export function GlobalToastProvider({ children }: { children: React.ReactNode }) showToast(`${name}: ${event.message}`, 6000) }), [showToast]) + // #1133: a TLDR/Goal skill could not be prepared and agents were launched + // without it. WHY the global toast and not a pane toast or condition: + // - The fault is machine-wide skill health, not something one pane did. + // - Restoring a workspace fires this once per pane. The single slot collapses + // fifteen identical warnings into one, where pane toasts would put the + // same banner in every pane. + // - Provider conditions are interactive TUI states with actions. This has + // nothing to answer. + // 10 s and click-to-dismiss follow the warning-grade precedent above: long + // enough to read the Settings path, and dismissable so it never reads as + // stuck. The lasting record is the skill's health in Settings, which this + // text points to. + useEffect(() => window.api.onManagedSkillsUnavailable?.(event => { + showToast(managedSkillsUnavailableMessage(event.skills), 10_000) + }), [showToast]) + const dismiss = useCallback(() => { if (timerRef.current) { clearTimeout(timerRef.current) diff --git a/src/shared/lifecycle/events.ts b/src/shared/lifecycle/events.ts index 6a521cd3b..81a0897db 100644 --- a/src/shared/lifecycle/events.ts +++ b/src/shared/lifecycle/events.ts @@ -536,6 +536,47 @@ export const WAKE_CALLERS = [ export type WakeCaller = (typeof WAKE_CALLERS)[number] +/** + * Why a recovery's start failed: the `cause` on `recover.failed` (#1133). + * + * WHY a closed enum and not the exception message: `code: 'start-failed'` + * alone left two workspace-wide outages (Sept 18 and Sept 19, 2026) readable + * only by lining up `seq` numbers against a neighbouring + * `conventions.pre_spawn_reconcile.error`. The raw message cannot go here for + * the same reason it stays out of the recover IPC result: provider launch + * errors can embed environment values, proxy URLs and scoped MCP tokens, and + * this stream is always on and kept on disk. A classification carries the + * answer to "which layer refused?" and nothing else. + * + * WHY there is no `conventions-gate` value, though #1133 proposed one: after + * that fix the managed-skill reconcile can no longer fail a start. It + * degrades the session and warns instead. A value no code path can emit would + * read as a live failure mode to whoever triages the next journal. The + * reconcile's own failure is still journaled as + * `conventions.pre_spawn_reconcile.error`. + * + * Add a value only when a new failure can be classified from TYPED evidence (an + * error class or a tag set at a known boundary). Classifying by message + * substring would silently fall back to `unknown` the day the wording changes. + */ +export const SESSION_START_FAILURE_CAUSES = [ + // The pane's cwd no longer exists. Not retryable; see + // MissingWorkspaceDirectoryError. + 'missing-workspace', + // The provider CLI could not be resolved to an absolute path, even after the + // late re-resolve. The user has to act in Setup. + 'cli-not-found', + // The provider runtime was constructed and its start() threw: PTY spawn, + // headless bootstrap, proxy launch, or the provider exiting during startup. + 'provider-launch', + // Anything not classified above, for example a missing built-in MCP host or a + // registry race. Kept as an explicit value so "unclassified" reads + // differently from "this build predates causes" (the key is absent). + 'unknown', +] as const + +export type SessionStartFailureCause = (typeof SESSION_START_FAILURE_CAUSES)[number] + /** * The allowlisted top-level payload keys. * diff --git a/src/shared/types/tldr.test.ts b/src/shared/types/tldr.test.ts new file mode 100644 index 000000000..cff260b07 --- /dev/null +++ b/src/shared/types/tldr.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from 'vitest' + +import { managedSkillsUnavailableMessage } from './tldr' + +// #1133. This text is the only visible sign that an agent started without a +// requested TLDR/Goal skill. It must name the skill (otherwise the user cannot +// tell which Settings row to open) and name the place to fix it (both outages +// this replaced ended with someone hunting for the cause). +describe('managedSkillsUnavailableMessage', () => { + it('names one skill and the Settings location', () => { + expect(managedSkillsUnavailableMessage(['tldr'])).toBe( + 'TLDR skill could not be prepared, so agents started without it. Review Settings › Agents › Custom Skills.', + ) + }) + + it('names both skills in a fixed order whatever order they were reported in', () => { + expect(managedSkillsUnavailableMessage(['goal', 'tldr'])).toBe( + 'TLDR and Goal skills could not be prepared, so agents started without them. Review Settings › Agents › Custom Skills.', + ) + }) +}) diff --git a/src/shared/types/tldr.ts b/src/shared/types/tldr.ts index ec48c58fd..ea31e4cb5 100644 --- a/src/shared/types/tldr.ts +++ b/src/shared/types/tldr.ts @@ -85,6 +85,44 @@ export function hasReportingDomain(domains: readonly string[] | undefined): bool return Boolean(domains?.includes('tldr') || domains?.includes('goal')) } +/** The built-in MCP domains that come with a product-managed skill. Listed in a + * fixed order so every warning names the skills in the same order, whatever + * order the pre-spawn reconcile reported them in. */ +export const REPORTING_DOMAINS = ['tldr', 'goal'] as const +export type ReportingDomain = (typeof REPORTING_DOMAINS)[number] + +/** Main → renderer broadcast: one or more product skills could not be prepared + * for an agent that asked for them, and that agent was started without them + * (#1133). App-wide by design: skill health is machine-wide, not per-pane. */ +export const MANAGED_SKILLS_UNAVAILABLE_CHANNEL = 'managed-skills:unavailable' +export type ManagedSkillsUnavailableEvent = { skills: ReportingDomain[] } + +/** + * The user-facing text for a skipped product skill (#1133). + * + * WHY it names Settings › Agents › Custom Skills: TLDR and Goal show up there + * as "Managed by TLDR/Goal MCP" rows with their health and target rows, which + * is the only place the user can see WHY the skill is broken (a conflicting + * file, recovery required, a failed deploy). The toast is only a pointer. It + * has to say where to look, because both outages this replaced ended with + * someone hunting for the cause. + * + * WHY it says the agents are running: the old behavior was "nothing starts", + * and a warning that reads like the old failure would send the user off to + * restart panes that are fine. The part that is really lost is the skill's + * guidance. The MCP tool still carries its own server instructions. + * + * Kept pure and shared so the text is testable without a renderer, and so any + * future surface (remote client, notices) says exactly the same thing. + */ +export function managedSkillsUnavailableMessage(skills: readonly ReportingDomain[]): string { + const labels = REPORTING_DOMAINS + .filter(domain => skills.includes(domain)) + .map(domain => (domain === 'tldr' ? 'TLDR' : 'Goal')) + const subject = labels.length > 1 ? `${labels.join(' and ')} skills` : `${labels[0] ?? 'Managed'} skill` + return `${subject} could not be prepared, so agents started without ${labels.length > 1 ? 'them' : 'it'}. Review Settings › Agents › Custom Skills.` +} + /** * Why a hold-to-peek gesture ended, as far as main can tell (#1066). * From 7bef296ea7c9dfd414729130d9576d0edf7fb737 Mon Sep 17 00:00:00 2001 From: Julius Olsson Date: Tue, 22 Sep 2026 17:21:32 -0700 Subject: [PATCH 3/3] fix(sessions): pre-spawn skill warning is joinable, cancel-safe and throttled Review round on #1139: - The conventions.pre_spawn_reconcile.error rows now carry ids.sessionId, like the .degraded row, so triage can join them. recordError takes an optional ids argument for this. - .degraded and the managed-skills-unavailable warning are emitted only after the post-reconcile cancellation check, so a restore cancelled during the reconcile no longer journals a false "launching without skills" row. - Corrected the WHY comment: before #1133 an audit throw shared the single catch and also aborted TLDR/Goal launches, so it was not "always nonfatal". - GlobalToast holds back a repeat of the same skill-set warning for 60 s, so a skill that stays broken does not re-toast on every later launch. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../2026-09-23-skill-gate-blast-radius.md | 23 +++++-- src/main/incident/AppRunJournal.ts | 9 ++- src/main/sessionManager.ts | 69 ++++++++++++++----- src/main/sessionManager.wake.test.ts | 52 +++++++++++++- src/renderer/src/ui/GlobalToast.tsx | 18 +++++ 5 files changed, 146 insertions(+), 25 deletions(-) diff --git a/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md b/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md index 437c5af57..8ff5a3624 100644 --- a/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md +++ b/docs/superpowers/plans/2026-09-23-skill-gate-blast-radius.md @@ -58,8 +58,17 @@ the blast radius for any future cause. 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 emits - `managed-skills-unavailable` `{ sessionId, skills }`. +- 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) @@ -83,7 +92,10 @@ 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. +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 @@ -107,8 +119,9 @@ failure is still in the journal as `conventions.pre_spawn_reconcile.error`. 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. All of these fail on - origin/main. + 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 diff --git a/src/main/incident/AppRunJournal.ts b/src/main/incident/AppRunJournal.ts index 3bf4a9c26..f5056b2ac 100644 --- a/src/main/incident/AppRunJournal.ts +++ b/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): 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, ids?: AppRunJournalIds): void { this.record({ area: areaFromName(name), name, severity: 'error', + ...(ids ? { ids } : {}), data: { ...data, error: normalizeError(error), diff --git a/src/main/sessionManager.ts b/src/main/sessionManager.ts index e9d539364..97e5ac265 100644 --- a/src/main/sessionManager.ts +++ b/src/main/sessionManager.ts @@ -2614,8 +2614,8 @@ export class SessionManager extends EventEmitter { private async runPreSpawnSkillReconcile( sessionId: string, options: SessionSpawnOptions, - ): Promise { - if (!this.beforeAgentSessionStart) return + ): Promise { + if (!this.beforeAgentSessionStart) return [] const requested = REPORTING_DOMAINS.filter(domain => options.builtInMcpDomains?.includes(domain)) let failures: readonly ManagedSkillPreparationFailure[] try { @@ -2634,23 +2634,57 @@ export class SessionManager extends EventEmitter { // Same event name as before #1133, so triage that already greps for it // keeps working. `skill` says which step failed, which the old single // catch could not. - this.journal?.recordError('conventions.pre_spawn_reconcile.error', failure.error, { - sessionId, - skill: failure.skill, - }) + // `ids.sessionId`, not `data.sessionId`: the `.degraded` row and every + // `recover.*` row for this pane are keyed by ids, and triage follows + // one pane by filtering on ids. The error row explains WHY that pane is + // degraded, so it has to be found by the same filter. Written + // unconditionally, even if the spawn is cancelled right after: the + // reconcile really did fail, and that is machine-wide skill health, not + // a claim about this session. + this.journal?.recordError( + 'conventions.pre_spawn_reconcile.error', + failure.error, + { skill: failure.skill }, + { sessionId }, + ) } - // Only skills this agent ASKED for are worth interrupting the user about. A - // failed machine-wide audit (`conventions`) was always nonfatal and - // journal-only, and its degraded health already shows in Settings. A - // failure for a domain the launch did not request cannot happen with the - // production hook, and would not affect this agent anyway. - const unavailable = REPORTING_DOMAINS.filter(domain => + // Only skills this agent ASKED for are worth interrupting the user about. + // A machine-wide audit failure (`conventions`) does not toast. Be precise + // about history here, because it matters when reading old journals: + // before #1133 an audit throw shared the hook's single catch, so it too + // ABORTED every TLDR/Goal launch (only launches without reporting domains + // survived it). A pre-#1133 outage such as the Sept 18 cluster can + // therefore be an audit failure as easily as a skill deploy failure. The + // `.error` row's message tells them apart. Now it is journal-only: the + // audit never removes a skill the agent asked for (the product skills are + // prepared in their own steps), and its degraded health already shows in + // Settings. A failure for a domain the launch did not request cannot + // happen with the production hook, and would not affect this agent anyway. + return REPORTING_DOMAINS.filter(domain => requested.includes(domain) && failures.some(failure => failure.skill === domain), ) + } + + /** + * Publish that a launch is going ahead without some requested product skills. + * + * WHY this is separate from the reconcile and runs only after the + * post-reconcile cancellation check: the reconcile can take a while (it + * serializes behind other skill writes), and a restore can be cancelled + * meanwhile because the pane closed or a Codex replacement superseded it. + * Journaling `.degraded` before that check wrote a false "this session is + * launching without its skills" row for a session that never launched. That + * is exactly the kind of untrustworthy journal fact #1133 set out to remove. + * The toast would also have claimed "agents started" off a launch that did + * not happen. + */ + private reportSkillsUnavailable(sessionId: string, unavailable: readonly ReportingDomain[]): void { if (unavailable.length === 0) return - // The launch-side fact that pairs with the `.error` rows above: "this - // session started WITHOUT these skills". Before #1133 the next row was - // `recover.failed`. Joined into a string because journal data stays flat. + // Pairs with the `.error` rows (same ids.sessionId): "this session is + // launching WITHOUT these skills". Before #1133 the next row was + // `recover.failed`. It says "launching", not "started", because the + // provider can still fail afterwards; that shows up as `recover.failed` + // with its own cause. Joined into a string because journal data stays flat. this.journal?.record({ area: 'conventions.pre_spawn_reconcile', name: 'conventions.pre_spawn_reconcile.degraded', @@ -2658,7 +2692,7 @@ export class SessionManager extends EventEmitter { ids: { sessionId }, data: { skills: unavailable.join(',') }, }) - this.emit('managed-skills-unavailable', { sessionId, skills: unavailable }) + this.emit('managed-skills-unavailable', { sessionId, skills: [...unavailable] }) } /** @@ -2811,8 +2845,9 @@ export class SessionManager extends EventEmitter { } this.throwIfSpawnCancelled(recoveryClaim, codexReplacementHandoff) if (this.beforeAgentSessionStart) { - await this.runPreSpawnSkillReconcile(sessionId, options) + const unavailableSkills = await this.runPreSpawnSkillReconcile(sessionId, options) this.throwIfSpawnCancelled(recoveryClaim, codexReplacementHandoff) + this.reportSkillsUnavailable(sessionId, unavailableSkills) } const provider = getMainProvider(kind) const createStartedAt = performance.now() diff --git a/src/main/sessionManager.wake.test.ts b/src/main/sessionManager.wake.test.ts index 4f244455c..5465053aa 100644 --- a/src/main/sessionManager.wake.test.ts +++ b/src/main/sessionManager.wake.test.ts @@ -232,10 +232,13 @@ describe('SessionManager restart wake recovery', () => { expect(host.revokeSession).not.toHaveBeenCalled() expect(warnings).toEqual([{ sessionId: result.sessionId, skills: ['tldr'] }]) // The raw error stays in main's journal, keyed by which step failed. + // Keyed by ids.sessionId like the `.degraded` row, so triage filtering on + // one pane finds the reason next to the consequence. expect(journal.recordError).toHaveBeenCalledWith( 'conventions.pre_spawn_reconcile.error', skillError, - { sessionId: result.sessionId, skill: 'tldr' }, + { skill: 'tldr' }, + { sessionId: result.sessionId }, ) expect(journal.record).toHaveBeenCalledWith(expect.objectContaining({ name: 'conventions.pre_spawn_reconcile.degraded', @@ -286,6 +289,50 @@ describe('SessionManager restart wake recovery', () => { expect(journal.recordError).toHaveBeenCalledTimes(2) }) + it('neither warns nor journals a degraded launch for a restore cancelled during the reconcile', async () => { + // The reconcile serializes behind other skill writes, so a pane can close + // while it runs. That session never launches. A `.degraded` row or a toast + // saying "agents started without it" would be a false fact about it. The + // reconcile's own `.error` row still lands, because the skill really is + // broken machine-wide. + const { SessionManager } = await import('./sessionManager') + const host = { registerSession: vi.fn((_scope: { sessionId: string }) => []), revokeSession: vi.fn() } + const journal = journalSpy() + let releaseReconcile!: () => void + const reconcileGate = new Promise(resolve => { releaseReconcile = resolve }) + const reconcile = vi.fn(async () => { + await reconcileGate + return [{ skill: 'tldr' as const, error: new Error('TLDR skill destination is user-owned') }] + }) + const manager = new SessionManager(null, host as unknown as BuiltInMcpHttpHost, journal as never, reconcile) + const warnings: unknown[] = [] + manager.on('managed-skills-unavailable', event => warnings.push(event)) + + const recovering = manager.recover({ + sessionId: 'closing-pane', + kind: 'claude', + cwd: '/tmp/project', + builtInMcpDomains: ['tldr'], + }) + await vi.waitFor(() => expect(reconcile).toHaveBeenCalledTimes(1)) + const killing = manager.kill('closing-pane') + releaseReconcile() + await killing + + await expect(recovering).resolves.toMatchObject({ ok: false, code: 'cancelled' }) + expect(createSession).not.toHaveBeenCalled() + expect(warnings).toEqual([]) + expect(journal.record).not.toHaveBeenCalledWith(expect.objectContaining({ + name: 'conventions.pre_spawn_reconcile.degraded', + })) + expect(journal.recordError).toHaveBeenCalledWith( + 'conventions.pre_spawn_reconcile.error', + expect.any(Error), + { skill: 'tldr' }, + { sessionId: 'closing-pane' }, + ) + }) + it('does not warn when only the machine-wide audit failed', async () => { // A failed personal-conventions audit was always nonfatal and journal-only. // Its health is in Settings, and it does not remove anything this agent @@ -304,7 +351,8 @@ describe('SessionManager restart wake recovery', () => { expect(journal.recordError).toHaveBeenCalledWith( 'conventions.pre_spawn_reconcile.error', expect.any(Error), - expect.objectContaining({ skill: 'conventions' }), + { skill: 'conventions' }, + { sessionId: expect.any(String) }, ) }) diff --git a/src/renderer/src/ui/GlobalToast.tsx b/src/renderer/src/ui/GlobalToast.tsx index cde01c3b3..1819e6c11 100644 --- a/src/renderer/src/ui/GlobalToast.tsx +++ b/src/renderer/src/ui/GlobalToast.tsx @@ -28,6 +28,8 @@ import { managedSkillsUnavailableMessage } from '@shared/types/tldr' // is unaffected by it moving up, whereas lowering the dialog would break the // modal stacking the entire surface registry depends on. +const MANAGED_SKILLS_WARNING_REPEAT_MS = 60_000 + type GlobalToastContextValue = { showToast: (message: string, durationMs?: number) => void } @@ -74,7 +76,23 @@ export function GlobalToastProvider({ children }: { children: React.ReactNode }) // enough to read the Settings path, and dismissable so it never reads as // stuck. The lasting record is the skill's health in Settings, which this // text points to. + // + // WHY a 60 s repeat window per skill set: main warns on EVERY launch while + // the skill stays broken (each new pane, orchestration child and wake). The + // single slot only collapses one restore burst. Without the window, a + // broken skill re-raised the same 10 s toast all day, pushed out extension + // notifications that share the slot, and brought back a toast the user had + // just dismissed mid-burst. The key is the skill SET, so a newly failing + // skill (TLDR, then TLDR+Goal) still shows at once. Main's journal keeps + // every occurrence, so nothing diagnostic is lost here. Per window, because + // every window gets the broadcast and each has its own user-visible slot. + const lastSkillsWarningRef = useRef<{ key: string; at: number } | null>(null) useEffect(() => window.api.onManagedSkillsUnavailable?.(event => { + const key = [...event.skills].sort().join(',') + const now = Date.now() + const last = lastSkillsWarningRef.current + if (last && last.key === key && now - last.at < MANAGED_SKILLS_WARNING_REPEAT_MS) return + lastSkillsWarningRef.current = { key, at: now } showToast(managedSkillsUnavailableMessage(event.skills), 10_000) }), [showToast])