diff --git a/src/cli/skill-update.ts b/src/cli/skill-update.ts index faaae95..ce0151f 100644 --- a/src/cli/skill-update.ts +++ b/src/cli/skill-update.ts @@ -32,7 +32,10 @@ import { matchesSkillUpdateFilter, resolveCheckoutSubpath, } from '../core/skill-update.js'; -import { discoverSkillEntriesFromPluginRoot } from '../core/skills.js'; +import { + type DiscoveredSkillEntry, + discoverSkillEntriesFromPluginRoot, +} from '../core/skills.js'; import { syncUserWorkspace, syncWorkspace } from '../core/sync.js'; import { getUserWorkspaceConfigPath } from '../core/user-workspace.js'; import type { @@ -47,6 +50,7 @@ import { } from '../models/workspace-config.js'; import { parseMarketplaceManifest } from '../utils/marketplace-manifest-parser.js'; import { + formatPluginSource, getPluginCachePath, isGitHubUrl, parseGitHubUrl, @@ -142,6 +146,12 @@ export interface SkillUpdateInventory { installations: SkillUpdateInstallation[]; skippedLocalSources: string[]; failures: SkillUpdateInventoryFailure[]; + /** Direct config entries consuming a cache, including those with no enabled skills. */ + directRemoteConsumers: Array<{ + scope: SkillUpdateScope; + source: string; + nodeId: string; + }>; } export interface PrepareSkillUpdateOptions { @@ -236,7 +246,7 @@ function posixPath(path: string): string { } function enabledSkills( - entries: Awaited>, + entries: DiscoveredSkillEntry[], pluginName: string, config: WorkspaceConfig, pluginSkills: PluginSkillsConfig | undefined, @@ -288,16 +298,21 @@ function pluginSkillsConfig( return typeof plugin === 'string' ? undefined : plugin.skills; } -async function isStandaloneSkillRoot( +function isStandaloneSkillRoot( root: string, - entries: Awaited>, -): Promise { - if (!existsSync(join(root, 'SKILL.md')) || entries.length !== 1) return false; + entries: DiscoveredSkillEntry[], +): boolean { + if ( + !existsSync(join(root, 'SKILL.md')) || + entries.length !== 1 || + resolve(entries[0]?.skillPath ?? '') !== resolve(root) + ) { + return false; + } return ![ '.claude-plugin', '.github', '.mcp.json', - 'agents', 'commands', 'hooks', 'mcp.json', @@ -381,7 +396,7 @@ async function inventoryDirect( pluginName, currentSha: await revision(cachePath), skills, - standaloneSkillSource: await isStandaloneSkillRoot(root, discovered), + standaloneSkillSource: isStandaloneSkillRoot(root, discovered), }); } @@ -498,6 +513,8 @@ export async function buildSkillUpdateInventory( const installations: SkillUpdateInstallation[] = []; const skippedLocalSources: string[] = []; const failures: SkillUpdateInventoryFailure[] = []; + const directRemoteConsumers: SkillUpdateInventory['directRemoteConsumers'] = + []; const deferred: SkillUpdateInstallation[] = []; const deferredErrors: Array< SkillUpdateInventoryFailure & { errorCause: unknown } @@ -549,6 +566,16 @@ export async function buildSkillUpdateInventory( for (const [configIndex, plugin] of config.plugins.entries()) { const rawSource = getPluginSource(plugin); const effectiveSource = getEffectivePluginSource(plugin); + const direct = isGitHubUrl(effectiveSource) + ? parseGitHubUrl(effectiveSource) + : null; + if (direct) { + directRemoteConsumers.push({ + scope, + source: rawSource, + nodeId: getPluginCachePath(direct.owner, direct.repo, direct.branch), + }); + } let installation: SkillUpdateInstallation | null | 'local'; try { // Inline Git refs also use `@` (owner/repo@ref). Direct GitHub @@ -630,7 +657,12 @@ export async function buildSkillUpdateInventory( error: `Could not safely inventory shared source: ${sharedFailure.errorCause instanceof Error ? sharedFailure.errorCause.message : String(sharedFailure.errorCause)}`, }); } - return { installations, skippedLocalSources, failures }; + return { + installations, + skippedLocalSources, + failures, + directRemoteConsumers, + }; } async function inspectInstallation( @@ -939,10 +971,18 @@ export function hasProjectSkillConfig(workspacePath: string): boolean { export function unitDisplayName( unit: SkillUpdatePreflight['units'][number], ): string { - const sources = [ - ...new Set(unit.installations.map((entry) => entry.rawSource)), + const labels = [ + ...new Set( + unit.installations.flatMap((installation) => + installation.standaloneSkillSource + ? installation.skills + .filter((skill) => skill.enabled) + .map((skill) => skill.name) + : [formatPluginSource(installation.rawSource)], + ), + ), ]; - return sources.length > 0 ? sources.join(', ') : basename(unit.id); + return labels.length > 0 ? labels.join(', ') : basename(unit.id); } export interface SkillUpdateSummary { diff --git a/src/cli/tui/actions/plugins.ts b/src/cli/tui/actions/plugins.ts index 375e160..b6cf1f0 100644 --- a/src/cli/tui/actions/plugins.ts +++ b/src/cli/tui/actions/plugins.ts @@ -23,7 +23,7 @@ import { type MarketplaceEntry, type MarketplacePluginsResult, } from '../../../core/marketplace.js'; -import { updatePlugin } from '../../../core/plugin.js'; +import { resetFetchCache, updatePlugin } from '../../../core/plugin.js'; import { formatVerboseSyncLines } from '../../format-sync.js'; import { parseMarketplaceManifest } from '../../../utils/marketplace-manifest-parser.js'; import { getWorkspaceStatus } from '../../../core/status.js'; @@ -32,6 +32,19 @@ import { getHomeDir } from '../../../constants.js'; import type { TuiContext } from '../context.js'; import type { TuiCache } from '../cache.js'; import { removeInstalledSkill } from '../../skill-removal.js'; +import { + buildSkillUpdateInventory, + executePreparedSkillUpdate, + inspectSkillUpdateUnit, + resolveNonInteractiveSkillUpdateDecisions, + unitDisplayName, +} from '../../skill-update.js'; +import { + buildPhysicalRefreshUnits, + buildSkillUpdatePreflight, + type SkillUpdatePreflight, + type SkillUpdateScope, +} from '../../../core/skill-update.js'; const { select, text, confirm, multiselect, autocomplete } = p; @@ -229,16 +242,81 @@ export async function runUpdateAllPlugins( const results: Array<{ plugin: string; action: string; error?: string }> = []; let needsProjectSync = false; let needsUserSync = false; + const scopes = [ + ...new Set(pluginsToUpdate.map(({ scope }) => scope)), + ] as SkillUpdateScope[]; + const workspacePath = context.workspacePath ?? process.cwd(); + const inventory = await buildSkillUpdateInventory(workspacePath, scopes); + const standaloneIds = new Set( + inventory.installations + .filter( + (installation) => + installation.standaloneSkillSource && + scopes.includes(installation.scope), + ) + .map((installation) => installation.id), + ); + const standaloneUnits = buildPhysicalRefreshUnits( + inventory.installations, + ).filter((unit) => + unit.installations.some((installation) => + standaloneIds.has(installation.id), + ), + ); + const handledPlugins = new Set(); + let standalonePlan: SkillUpdatePreflight | undefined; + + if (standaloneUnits.length > 0) { + const installations = standaloneUnits.flatMap( + (unit) => unit.installations, + ); + const nodeIds = new Set( + standaloneUnits.flatMap((unit) => unit.nodes.map((node) => node.id)), + ); + const failures = inventory.failures.filter((failure) => + failure.nodeIds.some((nodeId) => nodeIds.has(nodeId)), + ); + standalonePlan = await buildSkillUpdatePreflight( + { + installations, + selectedScopes: scopes, + failures, + }, + { inspectUnit: inspectSkillUpdateUnit }, + ); + + for (const installation of installations) { + if (scopes.includes(installation.scope)) { + handledPlugins.add(`${installation.scope}:${installation.rawSource}`); + } + } + for (const failure of failures) { + handledPlugins.add(`${failure.scope}:${failure.source}`); + } + for (const consumer of inventory.directRemoteConsumers) { + if ( + scopes.includes(consumer.scope) && + nodeIds.has(consumer.nodeId) + ) { + handledPlugins.add(`${consumer.scope}:${consumer.source}`); + } + } + } + resetFetchCache(); + // Refresh generic sources before standalone execution performs its offline + // scope sync, otherwise that sync's fetch-cache entries can mask updates. for (const { spec, scope } of pluginsToUpdate) { - const result = await updatePlugin(spec, scope === 'project' ? projectDeps : userDeps); + if (handledPlugins.has(`${scope}:${spec}`)) continue; + const result = await updatePlugin( + spec, + scope === 'project' ? projectDeps : userDeps, + ); const entry: { plugin: string; action: string; error?: string } = { plugin: spec, action: result.action, }; - if (result.error) { - entry.error = result.error; - } + if (result.error) entry.error = result.error; results.push(entry); if (result.action === 'updated') { if (scope === 'project') needsProjectSync = true; @@ -246,14 +324,58 @@ export async function runUpdateAllPlugins( } } - // Sync if any plugins were updated - if (needsProjectSync || needsUserSync) { + const standaloneSyncedScopes = new Set(); + if (standalonePlan) { + const prepared = { inventory, plan: standalonePlan }; + const execution = await executePreparedSkillUpdate( + prepared, + resolveNonInteractiveSkillUpdateDecisions(standalonePlan), + workspacePath, + ); + const planById = new Map( + standalonePlan.units.map((unit) => [unit.id, unit]), + ); + + for (const scope of execution.syncedScopes) { + standaloneSyncedScopes.add(scope); + } + for (const result of execution.units) { + const unit = planById.get(result.id); + const action = + result.status === 'updated' || result.status === 'removed' + ? 'updated' + : result.status === 'failed' + ? 'failed' + : 'skipped'; + results.push({ + plugin: unit ? unitDisplayName(unit) : result.id, + action, + ...(result.error && { error: result.error }), + }); + } + if (execution.units.some((result) => + result.status === 'updated' || result.status === 'removed' + )) { + cache?.invalidate(); + } + } + + // Generic sources have already refreshed above. Materialize from those cache + // revisions without letting a retained or failed standalone unit advance. + if ( + (needsProjectSync && !standaloneSyncedScopes.has('project')) || + (needsUserSync && !standaloneSyncedScopes.has('user')) + ) { s.message('Updating...'); - if (needsProjectSync && context.workspacePath) { - await syncWorkspace(context.workspacePath); + if ( + needsProjectSync && + !standaloneSyncedScopes.has('project') && + context.workspacePath + ) { + await syncWorkspace(context.workspacePath, { offline: true }); } - if (needsUserSync) { - await syncUserWorkspace(); + if (needsUserSync && !standaloneSyncedScopes.has('user')) { + await syncUserWorkspace({ offline: true }); } cache?.invalidate(); } diff --git a/tests/unit/cli/tui-plugin-update.test.ts b/tests/unit/cli/tui-plugin-update.test.ts new file mode 100644 index 0000000..a66a970 --- /dev/null +++ b/tests/unit/cli/tui-plugin-update.test.ts @@ -0,0 +1,407 @@ +import { afterEach, describe, expect, mock, test } from 'bun:test'; +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { dump } from 'js-yaml'; +import { getPluginCachePath } from '../../../src/utils/plugin-path.js'; + +const noteMock = mock((_message: string, _title?: string) => {}); +const spinner = { + start: mock((_message?: string) => {}), + message: mock((_message?: string) => {}), + stop: mock((_message?: string) => {}), +}; + +mock.module('@clack/prompts', () => ({ + autocomplete: mock(async () => ''), + confirm: mock(async () => false), + isCancel: () => false, + multiselect: mock(async () => []), + note: noteMock, + select: mock(async () => ''), + spinner: () => spinner, + text: mock(async () => ''), +})); + +// The prompt module must be mocked before loading the TUI action. +const { runUpdateAllPlugins } = await import( + '../../../src/cli/tui/actions/plugins.js' +); + +const SOURCE = + 'https://github.com/mattpocock/skills/tree/main/skills/engineering/setup-matt-pocock-skills'; +const EMPTY_SOURCE = + 'https://github.com/mattpocock/skills/tree/main/skills/empty'; +const GENERIC_SOURCE = 'https://github.com/example/plugins'; +const SKILL_ROOT = 'skills/engineering/setup-matt-pocock-skills'; +const SKILL_PATH = `${SKILL_ROOT}/SKILL.md`; +const SKILL_AGENT_PATH = `${SKILL_ROOT}/agents/openai.yaml`; +const EMPTY_SKILL_PATH = 'skills/empty/SKILL.md'; +const GENERIC_SKILL_PATH = 'skills/generic/SKILL.md'; +const originalEnvironment = { + ALLAGENTS_TEST_HOME: process.env.ALLAGENTS_TEST_HOME, + GIT_CONFIG_GLOBAL: process.env.GIT_CONFIG_GLOBAL, + HOME: process.env.HOME, + XDG_CACHE_HOME: process.env.XDG_CACHE_HOME, + XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, + XDG_DATA_HOME: process.env.XDG_DATA_HOME, +}; + +function runGit(path: string, args: string[]): string { + const result = Bun.spawnSync(['git', '-C', path, ...args], { + env: process.env, + stdout: 'pipe', + stderr: 'pipe', + }); + if (result.exitCode !== 0) { + throw new Error( + `git ${args.join(' ')} failed in ${path}: ${result.stderr.toString().trim()}`, + ); + } + return result.stdout.toString().trim(); +} + +async function createRemote( + root: string, + name: string, + files: Record, +): Promise<{ remote: string; upstream: string }> { + const remote = join(root, `${name}.git`); + const upstream = join(root, `${name}-upstream`); + await mkdir(remote, { recursive: true }); + runGit(remote, ['init', '--bare', '--initial-branch=main']); + runGit(root, ['clone', remote, upstream]); + runGit(upstream, ['config', '--local', 'user.name', 'TUI Update Test']); + runGit(upstream, [ + 'config', + '--local', + 'user.email', + 'tui-update@example.test', + ]); + for (const [path, content] of Object.entries(files)) { + const target = join(upstream, path); + await mkdir(dirname(target), { recursive: true }); + await writeFile(target, content); + } + runGit(upstream, ['add', '.']); + runGit(upstream, ['commit', '-m', 'fixture v1']); + runGit(upstream, ['push', 'origin', 'main']); + return { remote, upstream }; +} + +async function advanceRemote( + upstream: string, + path: string, + content: string, + version: string, +): Promise { + await writeFile(join(upstream, path), content); + runGit(upstream, ['add', '--all']); + runGit(upstream, ['commit', '-m', `fixture ${version}`]); + runGit(upstream, ['push', 'origin', 'main']); + return runGit(upstream, ['rev-parse', 'HEAD']); +} + +async function removeRemotePath( + upstream: string, + path: string, + version: string, +): Promise { + await rm(join(upstream, path), { recursive: true, force: true }); + runGit(upstream, ['add', '--all']); + runGit(upstream, ['commit', '-m', `fixture ${version}`]); + runGit(upstream, ['push', 'origin', 'main']); +} + +async function createUpdateFixture( + options: { includeGeneric?: boolean; includeEmptyConsumer?: boolean } = {}, +) { + const { includeGeneric = true, includeEmptyConsumer = false } = options; + const root = await mkdtemp(join(tmpdir(), 'allagents-tui-skill-update-')); + const home = join(root, 'home'); + const workspace = join(root, 'workspace'); + const gitConfig = join(root, 'gitconfig'); + await mkdir(home, { recursive: true }); + await mkdir(workspace, { recursive: true }); + + const skillRepository = await createRemote(root, 'skills', { + [SKILL_PATH]: + '---\nname: setup-matt-pocock-skills\ndescription: test skill\n---\n# standalone v1\n', + [SKILL_AGENT_PATH]: + 'interface:\n display_name: Setup Matt Pocock Skills\n short_description: Set up repository skills\n', + [EMPTY_SKILL_PATH]: + '---\nname: empty\ndescription: disabled skill\n---\n# empty v1\n', + }); + const genericRepository = await createRemote(root, 'plugins', { + [GENERIC_SKILL_PATH]: + '---\nname: generic\ndescription: generic skill\n---\n# generic v1\n', + }); + + await writeFile( + gitConfig, + `[url "file://${skillRepository.remote}"]\n\tinsteadOf = https://github.com/mattpocock/skills.git\n[url "file://${genericRepository.remote}"]\n\tinsteadOf = https://github.com/example/plugins.git\n`, + ); + process.env.ALLAGENTS_TEST_HOME = home; + process.env.HOME = home; + process.env.XDG_CACHE_HOME = join(home, '.cache'); + process.env.XDG_CONFIG_HOME = join(home, '.config'); + process.env.XDG_DATA_HOME = join(home, '.local/share'); + process.env.GIT_CONFIG_GLOBAL = gitConfig; + + const cache = getPluginCachePath('mattpocock', 'skills', 'main'); + const genericCache = getPluginCachePath('example', 'plugins'); + await mkdir(dirname(cache), { recursive: true }); + runGit(root, [ + 'clone', + '--branch', + 'main', + 'https://github.com/mattpocock/skills.git', + cache, + ]); + runGit(root, [ + 'clone', + 'https://github.com/example/plugins.git', + genericCache, + ]); + + const plugins: Array = [ + SOURCE, + ]; + if (includeGeneric) plugins.push(GENERIC_SOURCE); + if (includeEmptyConsumer) { + plugins.push({ source: EMPTY_SOURCE, skills: [] }); + } + await mkdir(join(workspace, '.allagents'), { recursive: true }); + await writeFile( + join(workspace, '.allagents/workspace.yaml'), + dump({ + version: 2, + repositories: [], + clients: ['claude'], + plugins, + }), + ); + + return { + root, + workspace, + cache, + genericCache, + skillRepository, + genericRepository, + context: { + hasWorkspace: true, + workspacePath: workspace, + projectPluginCount: plugins.length, + userPluginCount: 0, + needsSync: false, + hasUserConfig: false, + marketplaceCount: 0, + }, + }; +} + +function restoreEnvironment(): void { + for (const [name, value] of Object.entries(originalEnvironment)) { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; + } +} + +afterEach(() => { + restoreEnvironment(); + noteMock.mockClear(); + spinner.start.mockClear(); + spinner.message.mockClear(); + spinner.stop.mockClear(); +}); + +describe('interactive plugin updates', () => { + test( + 'updates a root standalone skill with support agents using its semantic name', + async () => { + const fixture = await createUpdateFixture(); + try { + const skillShaV2 = await advanceRemote( + fixture.skillRepository.upstream, + SKILL_PATH, + '---\nname: setup-matt-pocock-skills\ndescription: test skill\n---\n# standalone v2\n', + 'v2', + ); + const genericShaV2 = await advanceRemote( + fixture.genericRepository.upstream, + GENERIC_SKILL_PATH, + '---\nname: generic\ndescription: generic skill\n---\n# generic v2\n', + 'v2', + ); + + await runUpdateAllPlugins(fixture.context); + + expect(noteMock).toHaveBeenCalledTimes(1); + expect(noteMock).toHaveBeenCalledWith( + `✓ ${GENERIC_SOURCE} (updated)\n✓ setup-matt-pocock-skills (updated)\n\nUpdated: 2 Skipped: 0 Failed: 0`, + 'Update Results', + ); + expect(noteMock.mock.calls[0]?.[0]).not.toContain(SOURCE); + expect(runGit(fixture.cache, ['rev-parse', 'HEAD'])).toBe( + skillShaV2, + ); + expect(runGit(fixture.genericCache, ['rev-parse', 'HEAD'])).toBe( + genericShaV2, + ); + expect( + await readFile(join(fixture.cache, SKILL_PATH), 'utf-8'), + ).toContain('# standalone v2'); + expect( + await readFile( + join(fixture.genericCache, GENERIC_SKILL_PATH), + 'utf-8', + ), + ).toContain('# generic v2'); + expect( + await readFile( + join( + fixture.workspace, + '.claude/skills/setup-matt-pocock-skills/SKILL.md', + ), + 'utf-8', + ), + ).toContain('# standalone v2'); + expect( + await readFile( + join(fixture.workspace, '.claude/skills/generic/SKILL.md'), + 'utf-8', + ), + ).toContain('# generic v2'); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + } + }, + 15_000, + ); + + test( + 'materializes unrelated generic updates without advancing retained or failed standalone checkout', + async () => { + const fixture = await createUpdateFixture(); + try { + const initialSkillSha = runGit(fixture.cache, ['rev-parse', 'HEAD']); + await removeRemotePath( + fixture.skillRepository.upstream, + SKILL_PATH, + 'v2', + ); + const genericShaV2 = await advanceRemote( + fixture.genericRepository.upstream, + GENERIC_SKILL_PATH, + '---\nname: generic\ndescription: generic skill\n---\n# generic v2\n', + 'v2', + ); + + await runUpdateAllPlugins(fixture.context); + + expect(noteMock).toHaveBeenCalledWith( + `✓ ${GENERIC_SOURCE} (updated)\n- setup-matt-pocock-skills (skipped)\n\nUpdated: 1 Skipped: 1 Failed: 0`, + 'Update Results', + ); + expect(runGit(fixture.cache, ['rev-parse', 'HEAD'])).toBe( + initialSkillSha, + ); + expect(runGit(fixture.genericCache, ['rev-parse', 'HEAD'])).toBe( + genericShaV2, + ); + expect( + await readFile(join(fixture.cache, SKILL_PATH), 'utf-8'), + ).toContain('# standalone v1'); + expect( + await readFile( + join( + fixture.workspace, + '.claude/skills/setup-matt-pocock-skills/SKILL.md', + ), + 'utf-8', + ), + ).toContain('# standalone v1'); + expect( + await readFile( + join(fixture.workspace, '.claude/skills/generic/SKILL.md'), + 'utf-8', + ), + ).toContain('# generic v2'); + + noteMock.mockClear(); + await removeRemotePath( + fixture.skillRepository.upstream, + SKILL_ROOT, + 'v3', + ); + const genericShaV3 = await advanceRemote( + fixture.genericRepository.upstream, + GENERIC_SKILL_PATH, + '---\nname: generic\ndescription: generic skill\n---\n# generic v3\n', + 'v3', + ); + + await runUpdateAllPlugins(fixture.context); + + expect(noteMock).toHaveBeenCalledWith( + `✓ ${GENERIC_SOURCE} (updated)\n✗ setup-matt-pocock-skills (failed) - Declared plugin root no longer exists for ${SOURCE}\n\nUpdated: 1 Skipped: 0 Failed: 1`, + 'Update Results', + ); + expect(runGit(fixture.cache, ['rev-parse', 'HEAD'])).toBe( + initialSkillSha, + ); + expect(runGit(fixture.genericCache, ['rev-parse', 'HEAD'])).toBe( + genericShaV3, + ); + expect( + await readFile(join(fixture.cache, SKILL_PATH), 'utf-8'), + ).toContain('# standalone v1'); + expect( + await readFile( + join(fixture.workspace, '.claude/skills/generic/SKILL.md'), + 'utf-8', + ), + ).toContain('# generic v3'); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + } + }, + 25_000, + ); + + test( + 'does not refresh an empty direct install sharing a standalone cache', + async () => { + const fixture = await createUpdateFixture({ + includeGeneric: false, + includeEmptyConsumer: true, + }); + try { + const initialSkillSha = runGit(fixture.cache, ['rev-parse', 'HEAD']); + await removeRemotePath( + fixture.skillRepository.upstream, + SKILL_PATH, + 'v2', + ); + + await runUpdateAllPlugins(fixture.context); + + expect(noteMock).toHaveBeenCalledWith( + '- setup-matt-pocock-skills (skipped)\n\nUpdated: 0 Skipped: 1 Failed: 0', + 'Update Results', + ); + expect(noteMock.mock.calls[0]?.[0]).not.toContain(EMPTY_SOURCE); + expect(runGit(fixture.cache, ['rev-parse', 'HEAD'])).toBe( + initialSkillSha, + ); + expect( + await readFile(join(fixture.cache, SKILL_PATH), 'utf-8'), + ).toContain('# standalone v1'); + } finally { + await rm(fixture.root, { recursive: true, force: true }); + } + }, + 15_000, + ); +});