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..8ff5a3624 --- /dev/null +++ b/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. 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/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/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..97e5ac265 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,121 @@ 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. + // `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 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 + // 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', + 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 +2823,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,19 +2845,9 @@ 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 - } + const unavailableSkills = await this.runPreSpawnSkillReconcile(sessionId, options) this.throwIfSpawnCancelled(recoveryClaim, codexReplacementHandoff) + this.reportSkillsUnavailable(sessionId, unavailableSkills) } const provider = getMainProvider(kind) const createStartedAt = performance.now() @@ -3039,6 +3208,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 +3432,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..5465053aa 100644 --- a/src/main/sessionManager.wake.test.ts +++ b/src/main/sessionManager.wake.test.ts @@ -200,26 +200,160 @@ 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. + // 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, + { skill: 'tldr' }, + { sessionId: result.sessionId }, + ) + 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') + 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('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(host.revokeSession).toHaveBeenCalledWith(host.registerSession.mock.calls[0]![0].sessionId) + 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 + // 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), + { skill: 'conventions' }, + { sessionId: expect.any(String) }, + ) }) 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..1819e6c11 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. // @@ -27,6 +28,8 @@ import { useAppStore } from '@renderer/app-state/hooks' // 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 } @@ -61,6 +64,38 @@ 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. + // + // 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]) + 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). *