From f6568ccf4503c5a9072cb257d85f4d37004e8a21 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Thu, 25 Jun 2026 13:39:01 -0700 Subject: [PATCH] Harden managed skill update stabilization --- src/main/runtime/rpc/methods/skills.test.ts | 31 +++++++++++++ src/main/skills/managed-skill-events.ts | 6 ++- .../skills/managed-skill-test-fixtures.ts | 14 ++++-- .../managed-skill-update-runner.test.ts | 45 ++++++++++++++++++- .../skills/managed-skill-update-runner.ts | 8 ++-- .../managed-skill-updates-safety.test.ts | 33 ++++++++------ src/main/skills/managed-skill-updates.test.ts | 9 +++- src/main/skills/managed-skill-updates.ts | 4 +- 8 files changed, 122 insertions(+), 28 deletions(-) diff --git a/src/main/runtime/rpc/methods/skills.test.ts b/src/main/runtime/rpc/methods/skills.test.ts index ecc9191a46f..89401c9cf95 100644 --- a/src/main/runtime/rpc/methods/skills.test.ts +++ b/src/main/runtime/rpc/methods/skills.test.ts @@ -3,6 +3,7 @@ import { RpcDispatcher } from '../dispatcher' import type { RpcRequest, RpcResponse } from '../core' import { SKILL_METHODS } from './skills' import { + onManagedSkillEvent, publishManagedSkillFallback, publishManagedSkillUpdated } from '../../../skills/managed-skill-events' @@ -129,4 +130,34 @@ describe('skills runtime RPC methods', () => { result: { type: 'end' } }) }) + + it('continues publishing managed skill events when one listener throws', () => { + const received: unknown[] = [] + const unsubscribeThrowing = onManagedSkillEvent(() => { + throw new Error('listener failed') + }) + const unsubscribeReceiving = onManagedSkillEvent((event) => { + received.push(event) + }) + const fallbackEvent = { + status: 'fallback', + skillName: 'orca-linear', + context: 'linear-worktree', + runtime: 'host', + scope: 'missing', + reason: 'missing-install', + uiKey: 'host::orca-linear:linear-worktree', + message: 'Install orca-linear.', + request: { skillName: 'orca-linear', context: 'linear-worktree' } + } satisfies ManagedAgentSkillFallback + + try { + publishManagedSkillFallback(fallbackEvent) + } finally { + unsubscribeThrowing() + unsubscribeReceiving() + } + + expect(received).toEqual([{ type: 'fallback', event: fallbackEvent }]) + }) }) diff --git a/src/main/skills/managed-skill-events.ts b/src/main/skills/managed-skill-events.ts index 3a544a86170..2fdacb02ed6 100644 --- a/src/main/skills/managed-skill-events.ts +++ b/src/main/skills/managed-skill-events.ts @@ -34,6 +34,10 @@ export function publishManagedSkillUpdated(result: ManagedAgentSkillEnsureResult function publishManagedSkillEvent(event: ManagedSkillEvent): void { for (const listener of managedSkillEventListeners) { - listener(event) + try { + listener(event) + } catch { + // Why: one broken stream subscriber should not interrupt other skill update listeners. + } } } diff --git a/src/main/skills/managed-skill-test-fixtures.ts b/src/main/skills/managed-skill-test-fixtures.ts index 995ccb77917..9fda538364a 100644 --- a/src/main/skills/managed-skill-test-fixtures.ts +++ b/src/main/skills/managed-skill-test-fixtures.ts @@ -1,5 +1,5 @@ import { homedir } from 'node:os' -import { join } from 'node:path' +import { join, sep } from 'node:path' import { ORCHESTRATION_SKILL_NAME } from '../../shared/agent-feature-install-commands' import type { DiscoveredSkill, @@ -9,11 +9,16 @@ import type { } from '../../shared/skills' export const TEST_MANAGED_HOME_ROOT = join(homedir(), '.agents', 'skills') +export const TEST_ALICE_HOME = join(sep, 'home', 'alice') +export const TEST_CENTRAL_SKILLS_ROOT = join(sep, 'mnt', 'central', 'skills') +export const TEST_WORKSPACE_ROOT = join(sep, 'workspace', 'current') +export const TEST_OTHER_WORKSPACE_ROOT = join(sep, 'workspace', 'other') +export const TEST_WORKSPACE_AGENT_SKILLS_ROOT = join(TEST_WORKSPACE_ROOT, '.agents', 'skills') export const orchestrationRequest: ManagedAgentSkillEnsureRequest = { skillName: ORCHESTRATION_SKILL_NAME, context: 'agent-orchestration', - discoveryTarget: { runtime: 'host', projectRootPath: '/workspace/current' } + discoveryTarget: { runtime: 'host', projectRootPath: TEST_WORKSPACE_ROOT } } export function discoveredSkill( @@ -21,6 +26,7 @@ export function discoveredSkill( ): DiscoveredSkill { const rootPath = patch.rootPath ?? TEST_MANAGED_HOME_ROOT const directoryPath = patch.directoryPath ?? join(rootPath, patch.name) + const realDirectoryPath = patch.realDirectoryPath ?? directoryPath return { id: patch.id ?? `${patch.sourceKind}-${patch.name}`, name: patch.name, @@ -30,10 +36,10 @@ export function discoveredSkill( sourceLabel: patch.sourceLabel ?? patch.sourceKind, rootPath, directoryPath, - realDirectoryPath: patch.realDirectoryPath ?? directoryPath, + realDirectoryPath, directoryIsSymlink: patch.directoryIsSymlink ?? false, skillFilePath: patch.skillFilePath ?? join(directoryPath, 'SKILL.md'), - realSkillFilePath: patch.realSkillFilePath ?? join(directoryPath, 'SKILL.md'), + realSkillFilePath: patch.realSkillFilePath ?? join(realDirectoryPath, 'SKILL.md'), skillFileIsSymlink: patch.skillFileIsSymlink ?? false, installed: patch.installed ?? true, fileCount: patch.fileCount ?? 1, diff --git a/src/main/skills/managed-skill-update-runner.test.ts b/src/main/skills/managed-skill-update-runner.test.ts index 896b75cd06c..8996d762e26 100644 --- a/src/main/skills/managed-skill-update-runner.test.ts +++ b/src/main/skills/managed-skill-update-runner.test.ts @@ -1,8 +1,14 @@ import { EventEmitter } from 'node:events' import { spawn } from 'node:child_process' +import { join, sep } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import { ORCHESTRATION_SKILL_NAME } from '../../shared/agent-feature-install-commands' +import { homeDiscovery, lockfile, orchestrationRequest } from './managed-skill-test-fixtures' import { createManagedSkillUpdateRunner } from './managed-skill-update-runner' +import { + abortManagedSkillUpdateProcesses, + ManagedSkillUpdateCoordinator +} from './managed-skill-updates' vi.mock('node:child_process', () => ({ spawn: vi.fn() @@ -17,6 +23,8 @@ class FakeChildProcess extends EventEmitter { } } +const TEST_ALICE_HOME = join(sep, 'home', 'alice') + describe('createManagedSkillUpdateRunner', () => { afterEach(() => { vi.useRealTimers() @@ -49,10 +57,22 @@ describe('createManagedSkillUpdateRunner', () => { expect(child.kill).toHaveBeenCalledTimes(1) }) + it('does not spawn when the shutdown signal already aborted', async () => { + const controller = new AbortController() + controller.abort() + const runner = createManagedSkillUpdateRunner({ signal: controller.signal, timeoutMs: 1_000 }) + + await expect(runner(ORCHESTRATION_SKILL_NAME)).resolves.toEqual({ + status: 'failure', + error: 'aborted' + }) + expect(spawn).not.toHaveBeenCalled() + }) + it('spawns the single-skill global update command without a shell from a neutral cwd', async () => { const child = new FakeChildProcess() vi.mocked(spawn).mockReturnValue(child as never) - const runner = createManagedSkillUpdateRunner({ cwd: '/home/alice', timeoutMs: 1_000 }) + const runner = createManagedSkillUpdateRunner({ cwd: TEST_ALICE_HOME, timeoutMs: 1_000 }) const resultPromise = runner(ORCHESTRATION_SKILL_NAME) child.emit('close', 0) @@ -62,7 +82,7 @@ describe('createManagedSkillUpdateRunner', () => { 'npx', ['--yes', 'skills', 'update', 'orchestration', '--global', '--yes'], { - cwd: '/home/alice', + cwd: TEST_ALICE_HOME, shell: false, stdio: 'ignore', windowsHide: true @@ -93,4 +113,25 @@ describe('createManagedSkillUpdateRunner', () => { expect(killer.unref).toHaveBeenCalledTimes(1) platform.mockRestore() }) + + it('allows future default coordinators to update after aborting current update processes', async () => { + const child = new FakeChildProcess() + vi.mocked(spawn).mockReturnValue(child as never) + abortManagedSkillUpdateProcesses() + const coordinator = new ManagedSkillUpdateCoordinator({ + backgroundUpdatesEnabled: () => true, + discoverHostSkills: async () => homeDiscovery(), + readTextFile: async () => lockfile(ORCHESTRATION_SKILL_NAME, 'same-hash') + }) + + const resultPromise = coordinator.ensureManagedReady(orchestrationRequest) + await vi.waitFor(() => expect(spawn).toHaveBeenCalled()) + child.emit('close', 0) + + await expect(resultPromise).resolves.toMatchObject({ + status: 'ready', + skillName: ORCHESTRATION_SKILL_NAME, + context: 'agent-orchestration' + }) + }) }) diff --git a/src/main/skills/managed-skill-update-runner.ts b/src/main/skills/managed-skill-update-runner.ts index 0394a1919d1..ce47f4d271a 100644 --- a/src/main/skills/managed-skill-update-runner.ts +++ b/src/main/skills/managed-skill-update-runner.ts @@ -41,6 +41,10 @@ async function runManagedSkillUpdate( cwd: string, signal: AbortSignal | undefined ): Promise { + if (signal?.aborted) { + return { status: 'failure', error: 'aborted' } + } + const command = buildManagedSkillUpdateCommand(skillName) return new Promise((resolve) => { let settled = false @@ -73,10 +77,6 @@ async function runManagedSkillUpdate( killChildTree() finish({ status: 'timeout' }) }, timeoutMs) - if (signal?.aborted) { - onAbort() - return - } signal?.addEventListener('abort', onAbort, { once: true }) child.once('error', (error) => { diff --git a/src/main/skills/managed-skill-updates-safety.test.ts b/src/main/skills/managed-skill-updates-safety.test.ts index 416f622dc1f..4f3ec5c70d0 100644 --- a/src/main/skills/managed-skill-updates-safety.test.ts +++ b/src/main/skills/managed-skill-updates-safety.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from 'vitest' +import { join } from 'node:path' import { ORCA_LINEAR_SKILL_NAME, ORCHESTRATION_SKILL_NAME @@ -6,6 +7,10 @@ import { import type { ManagedAgentSkillEnsureRequest, ManagedAgentSkillName } from '../../shared/skills' import { TEST_MANAGED_HOME_ROOT, + TEST_ALICE_HOME, + TEST_CENTRAL_SKILLS_ROOT, + TEST_OTHER_WORKSPACE_ROOT, + TEST_WORKSPACE_AGENT_SKILLS_ROOT, discoveredSkill, discovery, homeDiscovery, @@ -24,8 +29,8 @@ describe('ManagedSkillUpdateCoordinator safety cases', () => { discoveredSkill({ name: ORCHESTRATION_SKILL_NAME, sourceKind: 'repo', - rootPath: '/workspace/current/.agents/skills', - directoryPath: '/workspace/current/.agents/skills/orchestration' + rootPath: TEST_WORKSPACE_AGENT_SKILLS_ROOT, + directoryPath: join(TEST_WORKSPACE_AGENT_SKILLS_ROOT, 'orchestration') }) ], reason: 'project-install' @@ -36,8 +41,8 @@ describe('ManagedSkillUpdateCoordinator safety cases', () => { discoveredSkill({ name: ORCHESTRATION_SKILL_NAME, sourceKind: 'repo', - rootPath: '/workspace/current/.agents/skills', - directoryPath: '/workspace/current/.agents/skills/orchestration' + rootPath: TEST_WORKSPACE_AGENT_SKILLS_ROOT, + directoryPath: join(TEST_WORKSPACE_AGENT_SKILLS_ROOT, 'orchestration') }) ], reason: 'ambiguous-install' @@ -65,10 +70,10 @@ describe('ManagedSkillUpdateCoordinator safety cases', () => { discoveredSkill({ name: ORCHESTRATION_SKILL_NAME, sourceKind: 'home', - directoryPath: '/home/alice/.agents/skills/orchestration', - realDirectoryPath: '/mnt/central/skills/orchestration', - skillFilePath: '/home/alice/.agents/skills/orchestration/SKILL.md', - realSkillFilePath: '/mnt/central/skills/orchestration/SKILL.md' + directoryPath: join(TEST_ALICE_HOME, '.agents', 'skills', 'orchestration'), + realDirectoryPath: join(TEST_CENTRAL_SKILLS_ROOT, 'orchestration'), + skillFilePath: join(TEST_ALICE_HOME, '.agents', 'skills', 'orchestration', 'SKILL.md'), + realSkillFilePath: join(TEST_CENTRAL_SKILLS_ROOT, 'orchestration', 'SKILL.md') }) ], reason: 'symlinked-global-install' @@ -91,15 +96,15 @@ describe('ManagedSkillUpdateCoordinator safety cases', () => { it('does not treat Codex or Claude home skills as managed global installs', async () => { const updateRunner = vi.fn(async () => ({ status: 'success' as const })) const coordinator = new ManagedSkillUpdateCoordinator({ - homeDir: () => '/home/alice', + homeDir: () => TEST_ALICE_HOME, discoverHostSkills: async () => discovery([ discoveredSkill({ name: ORCHESTRATION_SKILL_NAME, sourceKind: 'home', providers: ['codex'], - rootPath: '/home/alice/.codex/skills', - directoryPath: '/home/alice/.codex/skills/orchestration' + rootPath: join(TEST_ALICE_HOME, '.codex', 'skills'), + directoryPath: join(TEST_ALICE_HOME, '.codex', 'skills', 'orchestration') }) ]), readTextFile: async () => lockfile(ORCHESTRATION_SKILL_NAME, 'old-hash'), @@ -124,8 +129,8 @@ describe('ManagedSkillUpdateCoordinator safety cases', () => { discoveredSkill({ name: ORCHESTRATION_SKILL_NAME, sourceKind: 'repo', - rootPath: '/workspace/current/.agents/skills', - directoryPath: '/workspace/current/.agents/skills/orchestration' + rootPath: TEST_WORKSPACE_AGENT_SKILLS_ROOT, + directoryPath: join(TEST_WORKSPACE_AGENT_SKILLS_ROOT, 'orchestration') }) ]) ) @@ -140,7 +145,7 @@ describe('ManagedSkillUpdateCoordinator safety cases', () => { coordinator.ensureManagedReady(orchestrationRequest), coordinator.ensureManagedReady({ ...orchestrationRequest, - discoveryTarget: { runtime: 'host', projectRootPath: '/workspace/other' } + discoveryTarget: { runtime: 'host', projectRootPath: TEST_OTHER_WORKSPACE_ROOT } }) ]) diff --git a/src/main/skills/managed-skill-updates.test.ts b/src/main/skills/managed-skill-updates.test.ts index a93d178d37d..16cb17187a3 100644 --- a/src/main/skills/managed-skill-updates.test.ts +++ b/src/main/skills/managed-skill-updates.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it, vi } from 'vitest' import { ORCHESTRATION_SKILL_NAME } from '../../shared/agent-feature-install-commands' -import { homeDiscovery, lockfile, orchestrationRequest } from './managed-skill-test-fixtures' +import { + TEST_OTHER_WORKSPACE_ROOT, + homeDiscovery, + lockfile, + orchestrationRequest +} from './managed-skill-test-fixtures' import { ManagedSkillUpdateCoordinator } from './managed-skill-updates' describe('ManagedSkillUpdateCoordinator', () => { @@ -113,7 +118,7 @@ describe('ManagedSkillUpdateCoordinator', () => { const first = await coordinator.ensureManagedReady(orchestrationRequest) const second = await coordinator.ensureManagedReady({ ...orchestrationRequest, - discoveryTarget: { runtime: 'host', projectRootPath: '/workspace/other' } + discoveryTarget: { runtime: 'host', projectRootPath: TEST_OTHER_WORKSPACE_ROOT } }) expect(first.status).toBe('updated') diff --git a/src/main/skills/managed-skill-updates.ts b/src/main/skills/managed-skill-updates.ts index 6239b73385b..d38366528cc 100644 --- a/src/main/skills/managed-skill-updates.ts +++ b/src/main/skills/managed-skill-updates.ts @@ -43,10 +43,12 @@ type ManagedSkillCoordinatorDeps = { } const DEFAULT_COOLDOWN_MS = 60_000 -const defaultUpdateAbortController = new AbortController() +let defaultUpdateAbortController = new AbortController() export function abortManagedSkillUpdateProcesses(): void { defaultUpdateAbortController.abort() + // Why: AbortSignal is one-shot; future coordinators need a fresh shutdown signal. + defaultUpdateAbortController = new AbortController() } export class ManagedSkillUpdateCoordinator {