diff --git a/README.md b/README.md index 0d95f55..6183352 100644 --- a/README.md +++ b/README.md @@ -272,20 +272,25 @@ receive the skills as files. Add `--json` for a machine-readable report, and | Codex | its own plugin CLI (`--project` copies files instead) | managed by the plugin CLI | yes | start a new session | | Cursor | copied files | `~/.cursor/skills`, or `.agents/skills` with `--project` | Cursor 3.14.27, cursor-agent 2026.08.04-aaa8809 | a new session (a new chat) | | Windsurf (ships as Devin) | copied files | `~/.codeium/windsurf/skills`, or `.windsurf/skills` with `--project` | Devin 3.6.27, devin CLI 3000.3.27 | a new session (the next `devin` run) | -| Gemini CLI · GitHub Copilot | copied files | each vendor's documented directory | not yet | — | +| Antigravity | copied files | `~/.gemini/config/skills`, or `.agents/skills` with `--project` | Antigravity 2.0.1 | nothing — it re-reads its skills every turn, including in a conversation already open | +| GitHub Copilot | copied files | `~/.copilot/skills`, or `.agents/skills` with `--project` | not yet | — | The project directories overlap in a way the rows above do not show: Windsurf -also reads `.agents/skills`, which is where `--project` puts things for Cursor -and Codex. A project-scope install aimed at one of them therefore makes those -skills visible in the others, and removing them for one changes what the others -see. Use user scope when you want a host's skills to itself. +also reads `.agents/skills`, which is where `--project` puts things for Cursor, +Codex and Antigravity. A project-scope install aimed at one of them therefore +makes those skills visible in the others, and removing them for one changes what +the others see. Use user scope when you want a host's skills to itself. "Not yet" means the skills directory for that assistant comes from its documentation and has not been confirmed by a verification run of our own: the install works, but we cannot promise the assistant reads from where we put the -files. The remaining two are marked `(paths not yet verified)` -in the report and carry `"verified": false` in `--json`. Cursor and Windsurf -were verified with reply-cli 0.5.1. +files. GitHub Copilot is marked `(paths not yet verified)` in the report and +carries `"verified": false` in `--json`. Cursor and Windsurf were verified with +reply-cli 0.5.1, Antigravity with the release that adds it. + +Antigravity is the one host that needs nothing to pick up new skills — it +re-reads them every turn, so an install lands in a conversation you already have +open. Installing skills is not the same as connecting Reply: `reply-adapter` needs a Reply.io login (`reply auth login`) to actually do anything. diff --git a/src/__tests__/skills/adapter-flat.test.ts b/src/__tests__/skills/adapter-flat.test.ts index 6f35177..3158d1b 100644 --- a/src/__tests__/skills/adapter-flat.test.ts +++ b/src/__tests__/skills/adapter-flat.test.ts @@ -2,7 +2,7 @@ import {describe, it, expect, beforeEach, afterEach, vi} from 'vitest'; import fs from 'fs'; import os from 'os'; import path from 'path'; -import {clone_repo, copy_dir, run_flat, skills_target} from '../../skills/adapter-flat'; +import {canonical, clone_repo, copy_dir, run_flat, skills_target} from '../../skills/adapter-flat'; import {host_by_id} from '../../skills/hosts'; import {PACKS_FALLBACK, resolve_packs} from '../../skills/packs'; import {journal_entry, record_pack} from '../../skills/journal'; @@ -28,8 +28,8 @@ const cursor = (): Detected_host=> // A second flat host that shares its project-scope directory with cursor // (both resolve `.agents/skills`), used to exercise the multi-host sharing // path (see 'run_flat shared project directories across hosts' below). -const gemini = (): Detected_host=> - ({def: host_by_id('gemini-cli'), config_dir: path.join(home, '.gemini')}); +const copilot = (): Detected_host=> + ({def: host_by_id('github-copilot'), config_dir: path.join(home, '.copilot')}); // A native host reachable by run_flat only under --project (it has no // user_skills_dir), used to exercise the "no directory for this scope" path. @@ -468,9 +468,9 @@ describe('run_flat project-scope entries belong to one repository', ()=>{ // sibling's journal entry. describe('run_flat shared project directories across hosts', ()=>{ it('does not delete a sibling host\'s install of the same pack from a shared project directory', async()=>{ - fs.mkdirSync(path.join(home, '.gemini'), {recursive: true}); + fs.mkdirSync(path.join(home, '.copilot'), {recursive: true}); await run_flat({...flat_opts('install', core_only), scope: 'project'}); - await run_flat({...flat_opts('install', core_only), scope: 'project', host: gemini()}); + await run_flat({...flat_opts('install', core_only), scope: 'project', host: copilot()}); const shared = path.join(root, 'project', '.agents', 'skills', 'ai-sdr-core-skill', 'SKILL.md'); expect(fs.existsSync(shared)).toBe(true); @@ -478,12 +478,12 @@ describe('run_flat shared project directories across hosts', ()=>{ const outcome = await run_flat({...flat_opts('remove', core_only), scope: 'project'}); expect(outcome.packs).toEqual([{name: 'ai-sdr-core', action: 'removed', version: '0.1.0'}]); expect(journal_entry('cursor', 'project', 'ai-sdr-core', env())).toBeUndefined(); - // gemini-cli's install still claims the shared file, so it survives. + // github-copilot's install still claims the shared file, so it survives. expect(fs.existsSync(shared)).toBe(true); - expect(journal_entry('gemini-cli', 'project', 'ai-sdr-core', env())?.version).toBe('0.1.0'); + expect(journal_entry('github-copilot', 'project', 'ai-sdr-core', env())?.version).toBe('0.1.0'); const list_outcome = await run_flat({ - ...flat_opts('list', core_only), scope: 'project', host: gemini(), + ...flat_opts('list', core_only), scope: 'project', host: copilot(), clone: async()=>{ throw new Error('list must not clone'); }, }); expect(list_outcome.packs).toEqual([{name: 'ai-sdr-core', action: 'current', version: '0.1.0'}]); @@ -501,6 +501,51 @@ describe('run_flat with no skills directory for the scope', ()=>{ }); }); +// The journal records what we wrote; it is not evidence the files are still +// there. A directory deleted by hand, moved by a host upgrade, or eaten by a +// sync tool leaves the entry untouched, and every command used to believe it: +// `install` reported `current` and copied nothing, exit 0, with `list` agreeing, +// so the one command a user reaches for to fix a broken install was the one that +// could never fix it. +describe('run_flat when the journal outlives the files', ()=>{ + it('reports the pack as needing repair, and repairs it, rather than calling it current', async()=>{ + await run_flat(flat_opts('install', core_only)); + const target = path.join(home, '.cursor', 'skills'); + fs.rmSync(target, {recursive: true, force: true}); + + const listed = await run_flat(flat_opts('list', core_only)); + expect(listed.packs?.[0].action).toBe('failed'); + expect(listed.packs?.[0].detail).toContain('run `reply skills install` to repair'); + + // And the command that hint names actually fixes it — `installed`, not + // `current`: at an unchanged version the files were still missing. + const outcome = await run_flat(flat_opts('install', core_only)); + expect(outcome.packs?.map(p=>p.action)).toEqual(['installed']); + expect(fs.existsSync(path.join(target, 'ai-sdr-core-skill', 'SKILL.md'))).toBe(true); + }); + + // The deadlock. A host's skills directory moving between releases leaves + // entries pointing outside the new target root; `remove` refuses to delete + // outside it, which is right, but must not keep an entry for files that are + // not there — `install` reads that entry as `current`, and the only way out + // was editing the journal by hand. + it('forgets an entry whose recorded files are outside the target root and already gone', async()=>{ + const moved_away = path.join(root, 'old-location', 'ai-sdr-core-skill', 'SKILL.md'); + record_pack('cursor', 'user', 'ai-sdr-core', { + version: '0У.1.0', ref: 'main', commit: 'deadbee', scope: 'user', + files: [moved_away], complete: true, installed_at: '2026-07-30T00:00:00.000Z', + }, env()); + + const removed = await run_flat(flat_opts('remove', core_only)); + + expect(removed.packs?.map(p=>p.action)).toEqual(['removed']); + expect(journal_entry('cursor', 'user', 'ai-sdr-core', env())).toBeUndefined(); + // And the pack installs again with no hand-editing. + const outcome = await run_flat(flat_opts('install', core_only)); + expect(outcome.packs?.map(p=>p.action)).toEqual(['installed']); + }); +}); + // Minor, requested alongside the fixes above: clone_repo must not leave its // temp directory behind on either failure path, and must check the exit code // of `git rev-parse HEAD` rather than journaling an empty commit. @@ -738,4 +783,121 @@ describe('run_flat differently-cased journaled paths', ()=>{ expect(fs.readFileSync(path.join(target, 'ai-sdr-core-skill', 'SKILL.md'), 'utf8')) .toBe('old content'); }); + + // The two above are each skipped on half the platforms, so on any single + // runner neither covers the canonicalisation itself: replacing + // `fs.realpathSync.native` with `path.resolve` keeps the whole suite green. + // Injecting the canonicaliser — the seam detect.ts already uses for `exists` + // — puts the filesystem's opinion under the test's control, so what gets + // asserted is the logic: ownership recognised through a case difference, + // everywhere, and a foreign file still refused. + // Stands in for a case-insensitive filesystem by resolving each segment + // against what is on disk, case-blind. Lower-casing the whole path instead + // names a file that exists nowhere — `SKILL.MD` becomes `skill.md`, not the + // `SKILL.md` that is there — which passed on macOS only because the raw + // existence check answered first, and failed on Linux. + const ignoring_case = (target_path: string): string=>{ + const [root, ...segments] = target_path.split(path.sep); + let resolved = root || path.sep; + for (const segment of segments.filter(Boolean)) + { + const listed = fs.existsSync(resolved) && fs.statSync(resolved).isDirectory() + ? fs.readdirSync(resolved).find(entry=>entry.toLowerCase() === segment.toLowerCase()) + : undefined; + resolved = path.join(resolved, listed ?? segment); + } + return resolved; + }; + + it('recognises our own file through a case difference on any platform', async()=>{ + const target = seed_differently_cased_entry(); + // The stub answers with the spelling on disk, so the assertion below is + // about the adapter rather than about the runner's filesystem. + expect(ignoring_case(path.join(target, 'AI-SDR-CORE-SKILL', 'SKILL.MD'))) + .toBe(path.join(target, 'ai-sdr-core-skill', 'SKILL.md')); + + const outcome = await run_flat({...flat_opts('update', core_only), canonicalise: ignoring_case}); + + expect(outcome.packs).toEqual([{name: 'ai-sdr-core', action: 'current', version: '0.1.0'}]); + expect(fs.readFileSync(path.join(target, 'ai-sdr-core-skill', 'SKILL.md'), 'utf8')) + .not.toBe('old content'); + }); + + // Pins the branch only a case-sensitive runner reached, on every platform: + // the recorded path does not exist as written anywhere, and the canonicaliser + // is the only thing that maps it to the file that does. Without it, `install` + // and `list` disagree with owns_dir about whether the pack is present, and a + // re-copy reports `installed` on Linux and `current` on macOS for the same + // inputs — which is exactly how CI caught this. + it('counts a recorded file as present when only its canonical form exists', async()=>{ + const target = path.join(home, '.cursor', 'skills'); + const real = path.join(target, 'ai-sdr-core-skill', 'SKILL.md'); + fs.mkdirSync(path.dirname(real), {recursive: true}); + fs.writeFileSync(real, 'old content'); + const recorded = path.join(target, 'recorded-under-another-name', 'SKILL.md'); + record_pack('cursor', 'user', 'ai-sdr-core', { + version: '0.1.0', ref: 'main', commit: 'deadbee', scope: 'user', + files: [recorded], complete: true, installed_at: '2026-07-30T00:00:00.000Z', + }, env()); + + const outcome = await run_flat({ + ...flat_opts('install', core_only), + canonicalise: (target_path)=>target_path === recorded ? real : target_path, + }); + + // Complete, at the target version, and its file is there — so nothing to + // do. Judging presence on the raw path alone would call this a repair. + expect(outcome.packs).toEqual([{name: 'ai-sdr-core', action: 'current', version: '0.1.0'}]); + expect(fs.readFileSync(real, 'utf8')).toBe('old content'); + }); + + it('still refuses a file no journal entry claims, whatever the canonical form', async()=>{ + const target = path.join(home, '.cursor', 'skills'); + fs.mkdirSync(path.join(target, 'ai-sdr-core-skill'), {recursive: true}); + fs.writeFileSync(path.join(target, 'ai-sdr-core-skill', 'SKILL.md'), 'someone else wrote this'); + + const outcome = await run_flat({...flat_opts('install', core_only), canonicalise: ignoring_case}); + + expect(outcome.packs).toEqual([{ + name: 'ai-sdr-core', action: 'failed', detail: 'conflicts with an existing skill: ai-sdr-core-skill', + }]); + expect(fs.readFileSync(path.join(target, 'ai-sdr-core-skill', 'SKILL.md'), 'utf8')) + .toBe('someone else wrote this'); + }); +}); + +// The injected-canonicaliser tests above cover how owns_dir uses canonicalisation, +// but not the default's own behaviour: swapping `fs.realpathSync.native` for +// `path.resolve` leaves them all green. Two cases separate the pair, because +// neither runs everywhere — a symlink where the filesystem is case-sensitive, a +// differently-cased path where it is not. +describe('canonical', ()=>{ + // Skipped on Windows, where fs.symlinkSync needs Developer Mode or an + // elevated shell and otherwise throws EPERM — the case test below pins the + // same default there, since Windows folds case. + it.skipIf(process.platform === 'win32')('answers with the path the filesystem really uses, following a symlink', ()=>{ + const real = path.join(root, 'real-skills'); + fs.mkdirSync(real, {recursive: true}); + const link = path.join(root, 'linked-skills'); + fs.symlinkSync(real, link); + + expect(canonical(link)).toBe(fs.realpathSync.native(real)); + expect(canonical(link)).not.toBe(link); + }); + + // Keeps the default pinned where the symlink case cannot run — Windows + // refuses symlinks to an unprivileged account, and folds case. + it.skipIf(!CASE_INSENSITIVE_FS)('answers with the spelling on disk, not the one asked for', ()=>{ + const on_disk = path.join(root, 'Real-Skills'); + fs.mkdirSync(on_disk, {recursive: true}); + const asked_for = path.join(root, 'real-skills'); + + expect(canonical(asked_for)).toBe(fs.realpathSync.native(on_disk)); + expect(canonical(asked_for)).not.toBe(path.resolve(asked_for)); + }); + + it('falls back to a resolved path when the target does not exist', ()=>{ + const missing = path.join(root, 'nowhere', 'SKILL.md'); + expect(canonical(missing)).toBe(path.resolve(missing)); + }); }); diff --git a/src/__tests__/skills/hosts.test.ts b/src/__tests__/skills/hosts.test.ts index 5d5c00b..9c1fb76 100644 --- a/src/__tests__/skills/hosts.test.ts +++ b/src/__tests__/skills/hosts.test.ts @@ -1,3 +1,4 @@ +import path from 'path'; import {describe, it, expect} from 'vitest'; import {HOSTS, host_by_id, host_ids} from '../../skills/hosts'; import {UsageError} from '../../utils/errors'; @@ -9,12 +10,17 @@ describe('host registry', ()=>{ it('covers the hosts v1 promises', ()=>{ expect(host_ids()).toEqual(expect.arrayContaining([ - 'claude-code', 'codex', 'cursor', 'gemini-cli', 'github-copilot', 'windsurf', + 'claude-code', 'codex', 'cursor', 'github-copilot', 'windsurf', ])); }); it('marks only the hosts we actually verified', ()=>{ - expect(HOSTS.filter(h=>h.verified).map(h=>h.id)).toEqual(['claude-code', 'codex', 'cursor', 'windsurf']); + expect(HOSTS.filter(h=>h.verified).map(h=>h.id)) + .toEqual(['claude-code', 'codex', 'antigravity', 'cursor', 'windsurf']); + }); + + it('marks only the hosts that pick up skills without a new session', ()=>{ + expect(HOSTS.filter(h=>!h.needs_new_session).map(h=>h.id)).toEqual(['antigravity']); }); it('gives every native host a CLI and every flat host a skills directory', ()=>{ @@ -34,6 +40,18 @@ describe('host registry', ()=>{ expect(host.project_skills_dir, host.id).toBeDefined(); } }); + + it('never lets one host\'s config directory contain another\'s', ()=>{ + const dirs = HOSTS.flatMap(h=>h.config_dirs.map(dir=>({id: h.id, dir}))); + for (const a of dirs) + { + for (const b of dirs.filter(other=>other.id !== a.id)) + { + expect(a.dir === b.dir || a.dir.startsWith(`${b.dir}${path.sep}`), `${a.id} inside ${b.id}`) + .toBe(false); + } + } + }); it('builds Claude Code argument vectors', ()=>{ const cli = host_by_id('claude-code').cli!; diff --git a/src/__tests__/skills/journal.test.ts b/src/__tests__/skills/journal.test.ts index f5cba5b..ece42a7 100644 --- a/src/__tests__/skills/journal.test.ts +++ b/src/__tests__/skills/journal.test.ts @@ -46,9 +46,9 @@ describe('skills journal', ()=>{ it('keeps hosts isolated', ()=>{ record_pack('cursor', 'user', 'ai-sdr-core', entry(), env()); - record_pack('gemini-cli', 'user', 'ai-sdr-core', entry('0.9.0'), env()); + record_pack('github-copilot', 'user', 'ai-sdr-core', entry('0.9.0'), env()); expect(journal_entry('cursor', 'user', 'ai-sdr-core', env())?.version).toBe('0.1.0'); - expect(journal_entry('gemini-cli', 'user', 'ai-sdr-core', env())?.version).toBe('0.9.0'); + expect(journal_entry('github-copilot', 'user', 'ai-sdr-core', env())?.version).toBe('0.9.0'); }); it('keeps scopes isolated on the same host', ()=>{ diff --git a/src/__tests__/skills/orchestrate.test.ts b/src/__tests__/skills/orchestrate.test.ts index ea086e1..0fa4afe 100644 --- a/src/__tests__/skills/orchestrate.test.ts +++ b/src/__tests__/skills/orchestrate.test.ts @@ -3,6 +3,7 @@ import fs from 'fs'; import os from 'os'; import path from 'path'; import {HOSTS} from '../../skills/hosts'; +import {record_pack} from '../../skills/journal'; import {run_skills} from '../../skills/orchestrate'; import {human_lines} from '../../skills/report'; import type {Detect_deps} from '../../skills/detect'; @@ -277,18 +278,23 @@ describe('run_skills', ()=>{ // finally empties, the guard below fails loudly and someone decides what // this should assert, instead of the assertion quietly passing on nothing. const unverified = HOSTS.find(h=>!h.verified); - // Registry order rather than a fixed list: the report prints hosts in that - // order, so deriving it keeps this correct even if an unverified host is - // added ahead of the two the fixture always detects. - const detected_ids = (id: string): string[]=> - HOSTS.filter(h=>['claude-code', 'cursor', id].includes(h.id)).map(h=>h.id); + // Derived from which directories exist rather than from a list of ids, in + // registry order, because creating one host's config directory can reveal + // another whose own directory is an ancestor of it — a fixed list silently + // missed the second host and asserted a report the detector would never + // produce. Nested config directories are legitimate (Antigravity's is + // `.gemini/antigravity`), so containment, not equality, decides. + const detected_defs = (made: string[]): typeof HOSTS=> + HOSTS.filter(h=>h.config_dirs.some(dir=> + made.some(m=>m === dir || m.startsWith(`${dir}${path.sep}`)))); it('carries each host\'s verified flag into the report', async()=>{ expect(unverified, 'every host is verified — decide what this should assert now').toBeDefined(); + const made = ['.claude', '.cursor', unverified!.config_dirs[0]]; fs.mkdirSync(path.join(home, unverified!.config_dirs[0]), {recursive: true}); const report = await run_skills(opts()); expect(report.hosts.map(h=>[h.host, h.verified])).toEqual( - detected_ids(unverified!.id).map(id=>[id, id !== unverified!.id]), + detected_defs(made).map(h=>[h.id, h.verified]), ); expect(human_lines(report).join('\n')).toContain('paths not yet verified'); }); @@ -299,6 +305,35 @@ describe('run_skills', ()=>{ expect(report.hosts[0].verified).toBe(false); }); + // Retiring a host leaves its journal branch behind, and nothing else in this + // file can reach it: every other path iterates the registry, and `--agent + // ` is a usage error. Without this, `remove` quietly stops being + // able to take those packs out and no command ever says so — which is what + // happened when gemini-cli was retired with packs installed under it. + it('reports packs recorded for a host the registry no longer has', async()=>{ + const orphan_file = path.join(home, '.gemini', 'skills', 'ai-sdr-core-skill', 'SKILL.md'); + record_pack('gemini-cli', 'user', 'ai-sdr-core', { + version: '0.3.0', ref: 'main', commit: 'deadbee', scope: 'user', + files: [orphan_file], complete: true, installed_at: '2026-07-30T00:00:00.000Z', + }, {REPLY_CONFIG_DIR: path.join(root, 'config')}); + + const report = await run_skills(opts({operation: 'list'})); + + expect(report.orphans).toEqual([{ + host: 'gemini-cli', scope: 'user', packs: ['ai-sdr-core'], files: 1, sample: orphan_file, + }]); + const out = human_lines(report).join('\n'); + expect(out).toContain('no longer supports'); + expect(out).toContain('delete them by hand'); + }); + + it('says nothing about orphans when every journal branch is a known host', async()=>{ + await run_skills(opts({agents: ['cursor']})); + const report = await run_skills(opts({operation: 'list'})); + expect(report.orphans).toBeUndefined(); + expect(human_lines(report).join('\n')).not.toContain('no longer supports'); + }); + it('reports no commit when the clone failed', async()=>{ const failing_clone = async()=>{ throw new Error('git not found'); diff --git a/src/__tests__/skills/report.test.ts b/src/__tests__/skills/report.test.ts index de28cfb..6f8cc9f 100644 --- a/src/__tests__/skills/report.test.ts +++ b/src/__tests__/skills/report.test.ts @@ -138,16 +138,16 @@ describe('human_lines', ()=>{ it('marks a host whose paths are not yet verified, and only that host', ()=>{ const out = text(report([ host({verified: true}), - host({host: 'gemini-cli', label: 'Gemini CLI', verified: false}), + host({host: 'github-copilot', label: 'GitHub Copilot', verified: false}), ])); - expect(out).toMatch(/Gemini CLI .*paths not yet verified/); + expect(out).toMatch(/GitHub Copilot .*paths not yet verified/); expect(out.split('\n').filter(l=>l.includes('paths not yet verified'))).toHaveLength(1); }); it('says nothing about verification for a host that reported no packs', ()=>{ const out = text(report([host({ - host: 'gemini-cli', label: 'Gemini CLI', verified: false, status: 'skipped', packs: undefined, - reason: 'not-detected', detail: 'Gemini CLI is not installed on this machine', + host: 'github-copilot', label: 'GitHub Copilot', verified: false, status: 'skipped', packs: undefined, + reason: 'not-detected', detail: 'GitHub Copilot is not installed on this machine', })])); expect(out).not.toContain('paths not yet verified'); }); @@ -178,6 +178,44 @@ describe('human_lines', ()=>{ expect(out).toMatch(/new session/i); }); + // The line asks the user to do something, so it must only reach hosts that + // need it. Antigravity re-reads its skills every turn, and telling its users + // to restart is an instruction the host does not require. + it('does not ask for a new session when the only changed host does not need one', ()=>{ + const out = text(report([host({needs_new_session: false})])); + expect(out).not.toMatch(/new session/i); + }); + + it('names the hosts when a run touched both kinds', ()=>{ + const out = text(report([ + host({label: 'Cursor', needs_new_session: true}), + host({host: 'antigravity', label: 'Antigravity', needs_new_session: false}), + ])); + expect(out).toContain('Start a new session in Cursor so the skills load.'); + expect(out).not.toContain('Antigravity so the skills'); + }); + + it('keeps the unqualified line when every changed host needs a session', ()=>{ + const out = text(report([ + host({label: 'Cursor', needs_new_session: true}), + host({host: 'codex', label: 'Codex', needs_new_session: true}), + ])); + expect(out).toContain('Start a new session in each assistant so the skills load.'); + }); + + it('reports packs left behind for a host the registry no longer has', ()=>{ + const out = text(report([host()], { + action: 'list', + orphans: [{ + host: 'gemini-cli', scope: 'user', packs: ['ai-sdr-core', 'reply-adapter'], + files: 94, sample: '/home/u/.gemini/skills/sending-guardrails/SKILL.md', + }], + })); + expect(out).toContain("ai-sdr-core, reply-adapter recorded for 'gemini-cli'"); + expect(out).toContain('94 file(s) left on disk'); + expect(out).toContain('delete them by hand'); + }); + it('marks an outdated pack as an available update on list, not as updated', ()=>{ const out = text(report([host({ packs: [{name: 'ai-sdr-core', action: 'upgraded', version: '0.2.0', from: '0.1.0'}], diff --git a/src/skills/adapter-flat.ts b/src/skills/adapter-flat.ts index 18152b3..23de7f0 100644 --- a/src/skills/adapter-flat.ts +++ b/src/skills/adapter-flat.ts @@ -68,10 +68,12 @@ const copy_dir = (from: string, to: string, written: string[]): void=>{ // it via `..`. Every path handed to delete_files must pass this: the journal // is JSON in the user's config directory, and a hand-edited or stale entry // must not be able to name a file outside the host's own skills directory. -// `path.relative` — not string equality — so this agrees with the OS on -// whether two differently-cased paths are the same file, which matters on -// Windows: is_within, owns_dir and the protected-files check must all reach -// the same answer for the same pair of paths. +// `path.relative` — not string equality — so `..` and redundant separators are +// resolved before the decision. Note it compares case-insensitively only on +// Windows: on macOS, whose filesystem ignores case, two spellings of one path +// come out as different files here, which is why ownership checks canonicalise +// first (see `canonical`). Deliberately strict for containment: refusing to +// delete a path we cannot prove is inside the root is the safe direction. const is_within = (root: string, target: string): boolean=>{ const rel = path.relative(root, target); return rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel); @@ -80,6 +82,16 @@ const is_within = (root: string, target: string): boolean=>{ const paths_equal = (a: string, b: string): boolean=> path.relative(a, b) === ''; +// Whether the files an entry claims are still there. A complete entry at the +// right version says nothing about the filesystem, so `install` and `list` ask +// this before reporting a pack installed. Canonical form too, so this agrees +// with owns_dir about a path recorded in a different spelling. +const entry_files_present = ( + entry: Journal_entry, + canonicalise: (target: string)=>string = canonical, +): boolean=> + entry.files.every(file=>fs.existsSync(file) || fs.existsSync(canonicalise(file))); + // What delete_files could not do. `outside` are paths the containment check // refused — a tampered or stale journal entry naming somewhere else; `failed` // are files the OS would not delete. Both are returned rather than swallowed: @@ -103,7 +115,10 @@ const delete_files = (files: string[], target_root: string, protected_files: Set const resolved = path.resolve(file); if (!is_within(resolved_root, resolved)) { - outside.push(resolved); + if (fs.existsSync(resolved)) + { + outside.push(resolved); + } continue; } // A sibling host still claims this one; not deleting it is the point, @@ -198,16 +213,31 @@ const claimed_by_others = ( return claimed; }; +// The path as the filesystem spells it, so two spellings of one file compare +// equal — `path.relative` is case-sensitive on POSIX while macOS is not. Only +// answerable for a path that exists; anything else comes back resolved. +const canonical = (target: string): string=>{ + try { + return fs.realpathSync.native(target); + } catch { + return path.resolve(target); + } +}; + // True when every file under `dir` is accounted for by files we already know -// about — our own previous install of this pack, or a sibling host's install -// of the same pack at a shared directory. Anything else sitting at `dir` is -// foreign (typically user-authored) and must not be clobbered. Reuses -// is_within rather than a raw prefix check, so this agrees with delete_files' -// containment check on a differently-cased path (routine on Windows). -const owns_dir = (dir: string, known_files: Iterable): boolean=>{ +// about — our own previous install, or a sibling host's at a shared directory. +// Anything else there is foreign and must not be clobbered. Raw comparison keeps +// this agreeing with delete_files; canonical recognises our own skill through a +// case difference, without which an install refuses a file it wrote itself. +const owns_dir = ( + dir: string, + known_files: Iterable, + canonicalise: (target: string)=>string = canonical, +): boolean=>{ + const real_dir = canonicalise(dir); for (const file of known_files) { - if (is_within(dir, file)) + if (is_within(dir, file) || is_within(real_dir, canonicalise(file))) { return true; } @@ -294,6 +324,9 @@ type Flat_opts = { env?: Env; dry_run?: boolean; clone?: Clone_fn; + // Injected like detect.ts's `exists`, so case handling is assertable without + // depending on the runner's filesystem. Defaults to asking the OS. + canonicalise?: (target: string)=>string; }; const run_flat = async(opts: Flat_opts): Promise=>{ @@ -304,6 +337,7 @@ const run_flat = async(opts: Flat_opts): Promise=>{ const cwd = opts.cwd ?? process.cwd(); const tmp_root = opts.tmp_root ?? os.tmpdir(); const dry_run = opts.dry_run === true; + const canonicalise = opts.canonicalise ?? canonical; const clone = opts.clone ?? clone_repo; const id = host.def.id; const base: Host_outcome = { @@ -331,6 +365,11 @@ const run_flat = async(opts: Flat_opts): Promise=>{ const entry = journal_entry(id, scope, pack_name, opts.env); return entry && belongs_here(entry) ? entry : undefined; }; + + const previous_for = (pack_name: string): Journal_entry | undefined=>{ + const entry = entry_for(pack_name); + return entry && entry_files_present(entry, canonicalise) ? entry : undefined; + }; const record_for = (pack_name: string, data: Journal_entry): void=> record_pack(id, scope, pack_name, project_root ? {...data, project_root} : data, opts.env); const forget_for = (pack_name: string): Journal_entry | undefined=> @@ -350,8 +389,10 @@ const run_flat = async(opts: Flat_opts): Promise=>{ } // An incomplete entry never reads as current, regardless of // version — it is a copy that did not finish, and needs a repair - // install, not a clean bill of health. - if (!entry.complete) + // install, not a clean bill of health. Files that have since gone + // missing are the same answer for the user: the pack is not usable + // and `install` is what fixes it. + if (!entry.complete || !entry_files_present(entry, canonicalise)) { outcomes.push({ name: pack.name, @@ -440,7 +481,8 @@ const run_flat = async(opts: Flat_opts): Promise=>{ : packs; const pending = targets.filter(p=>{ const entry = entry_for(p.name); - return operation === 'update' || !entry || !entry.complete || entry.version !== p.version; + return operation === 'update' || !entry || !entry.complete || entry.version !== p.version + || !entry_files_present(entry, canonicalise); }); for (const pack of targets) { @@ -457,7 +499,7 @@ const run_flat = async(opts: Flat_opts): Promise=>{ { for (const pack of pending) { - outcomes.push(copied_outcome(pack.name, pack.version, entry_for(pack.name))); + outcomes.push(copied_outcome(pack.name, pack.version, previous_for(pack.name))); } return {...base, packs: outcomes}; } @@ -501,6 +543,7 @@ const run_flat = async(opts: Flat_opts): Promise=>{ } const from = path.join(cloned.dir, 'plugins', pack.name, 'skills'); const previous = entry_for(pack.name); + const previously_installed = previous_for(pack.name); const elsewhere = others_claim(pack.name); const known_files = previous ? [...previous.files.map(f=>path.resolve(f)), ...elsewhere] @@ -508,7 +551,7 @@ const run_flat = async(opts: Flat_opts): Promise=>{ const skill_dirs = fs.readdirSync(from, {withFileTypes: true}).filter(e=>e.isDirectory()); const collision = skill_dirs.find(skill=>{ const dst_dir = path.join(target_root, skill.name); - return fs.existsSync(dst_dir) && !owns_dir(dst_dir, known_files); + return fs.existsSync(dst_dir) && !owns_dir(dst_dir, known_files, canonicalise); }); if (collision) { @@ -553,7 +596,7 @@ const run_flat = async(opts: Flat_opts): Promise=>{ complete: true, installed_at: new Date().toISOString(), }); - outcomes.push(copied_outcome(pack.name, pack.version, previous, cloned.commit)); + outcomes.push(copied_outcome(pack.name, pack.version, previously_installed, cloned.commit)); } } catch (error) { // outcomes.length is not "something landed" — every entry pushed so @@ -578,5 +621,5 @@ const run_flat = async(opts: Flat_opts): Promise=>{ return {...cloned_base, packs: outcomes, status: status_of(outcomes), hint: blocked_hint(blocked_names)}; }; -export {clone_repo, copy_dir, skills_target, run_flat}; +export {canonical, clone_repo, copy_dir, skills_target, run_flat}; export type {Clone_fn, Clone_result, Flat_opts}; diff --git a/src/skills/hosts.ts b/src/skills/hosts.ts index 769fa38..de23859 100644 --- a/src/skills/hosts.ts +++ b/src/skills/hosts.ts @@ -38,6 +38,7 @@ const HOSTS: Host_def[] = [ binary_paths: [], cli: claude_cli, project_skills_dir: path.join('.claude', 'skills'), + needs_new_session: true, verified: true, }, { @@ -53,29 +54,43 @@ const HOSTS: Host_def[] = [ // Codex's plugin mechanism is user-scoped, so --project falls back to // copying into the repository's .agents/skills. project_skills_dir: path.join('.agents', 'skills'), + needs_new_session: true, verified: true, }, { - id: 'cursor', - label: 'Cursor', + id: 'antigravity', + label: 'Antigravity', kind: 'flat-skills-dir', - config_dirs: ['.cursor'], + // .gemini/config is this host's global "customization root" and .agents + // its workspace one; skills under either need no manifest and no plugin. + // Detection keys on .gemini/antigravity, NOT on .gemini — that belonged + // to the retired Gemini CLI, so matching it detected that host wherever + // this one ran. Written on first launch, so it is there before any + // install. Do not point this at the directory the skills go into. + config_dirs: [path.join('.gemini', 'antigravity')], binaries: [], binary_paths: [], - user_skills_dir: path.join('.cursor', 'skills'), + user_skills_dir: path.join('.gemini', 'config', 'skills'), + // Its workspace root, found by walking up to the repository root. The + // host's own GetAllSkills answers for user scope only, so a project + // install is confirmed by what a session in that repository loads. project_skills_dir: path.join('.agents', 'skills'), + // Alone among the hosts: re-reads its skills every turn, so an install + // lands in a conversation already open. + needs_new_session: false, verified: true, }, { - id: 'gemini-cli', - label: 'Gemini CLI', + id: 'cursor', + label: 'Cursor', kind: 'flat-skills-dir', - config_dirs: ['.gemini'], + config_dirs: ['.cursor'], binaries: [], binary_paths: [], - user_skills_dir: path.join('.gemini', 'skills'), + user_skills_dir: path.join('.cursor', 'skills'), project_skills_dir: path.join('.agents', 'skills'), - verified: false, + needs_new_session: true, + verified: true, }, { id: 'github-copilot', @@ -86,6 +101,7 @@ const HOSTS: Host_def[] = [ binary_paths: [], user_skills_dir: path.join('.copilot', 'skills'), project_skills_dir: path.join('.agents', 'skills'), + needs_new_session: true, verified: false, }, { @@ -100,6 +116,7 @@ const HOSTS: Host_def[] = [ binary_paths: [], user_skills_dir: path.join('.codeium', 'windsurf', 'skills'), project_skills_dir: path.join('.windsurf', 'skills'), + needs_new_session: true, verified: true, }, ]; diff --git a/src/skills/orchestrate.ts b/src/skills/orchestrate.ts index 9278a8a..261b555 100644 --- a/src/skills/orchestrate.ts +++ b/src/skills/orchestrate.ts @@ -2,11 +2,15 @@ import os from 'os'; import {run_flat, type Clone_fn} from './adapter-flat'; import {run_native} from './adapter-native'; import {default_detect_deps, select_hosts, type Detect_deps} from './detect'; +import {host_ids} from './hosts'; +import {read_journal} from './journal'; import {DEFAULT_REF, REPO, load_packs, resolve_packs} from './packs'; import {summarize} from './report'; import {UsageError} from '../utils/errors'; import type {Env} from '../config'; -import type {Host_def, Host_outcome, Operation, Pack, Report, Runner, Scope} from './types'; +import type { + Host_def, Host_outcome, Operation, Orphaned_packs, Pack, Report, Runner, Scope, +} from './types'; // The one flow all four commands share: detect hosts, resolve packs in // dependency order, run the right adapter per host, collect outcomes. Every @@ -66,8 +70,41 @@ const not_detected = (def: Host_def): Host_outcome=>({ host: def.id, label: def.label, kind: def.kind, status: 'skipped', reason: 'not-detected', detail: `${def.label} was requested with --agent but is not installed on this machine`, verified: def.verified, + needs_new_session: def.needs_new_session, }); +// Journal branches belonging to no host in the registry. Everything else here +// iterates hosts, so a retired assistant's packs go unreachable the moment its +// entry leaves hosts.ts — reading the journal directly is what still finds them. +const orphaned_packs = (env?: Env): Orphaned_packs[]=>{ + const known = new Set(host_ids()); + const orphans: Orphaned_packs[] = []; + for (const [host, scopes] of Object.entries(read_journal(env).hosts)) + { + if (known.has(host)) + { + continue; + } + for (const [scope, packs] of Object.entries(scopes)) + { + const entries = Object.entries(packs); + if (!entries.length) + { + continue; + } + const files = entries.flatMap(([, entry])=>entry.files ?? []); + orphans.push({ + host, + scope: scope as Scope, + packs: entries.map(([name])=>name), + files: files.length, + sample: files[0], + }); + } + } + return orphans; +}; + const run_skills = async(opts: Skills_opts): Promise=>{ const deps = opts.deps ?? {}; const detect = deps.detect ?? default_detect_deps(); @@ -121,10 +158,13 @@ const run_skills = async(opts: Skills_opts): Promise=>{ hint: `re-run \`reply skills ${opts.operation}\` once the underlying error for ${host.def.label} is resolved`, }; } - // Stamped here rather than in each adapter: whether an assistant's - // paths have been confirmed is registry data, not something an - // adapter computes, and doing it once means no path can forget it. - hosts.push({...outcome, verified: host.def.verified}); + // Stamped here rather than per adapter: both are registry data, and + // doing it once means no path can forget either. + hosts.push({ + ...outcome, + verified: host.def.verified, + needs_new_session: host.def.needs_new_session, + }); } for (const def of missing) { @@ -148,6 +188,10 @@ const run_skills = async(opts: Skills_opts): Promise=>{ // requested pack itself is always the last element. const canonical = opts.requested.map(r=>resolve_packs([r], registry).slice(-1)[0].name); + // `list` alone: it answers "what is installed where". The other operations + // act on hosts, and can do nothing about one they no longer know. + const orphans = opts.operation === 'list' ? orphaned_packs(deps.env) : []; + return { action: opts.operation, source: commit ? {repo: REPO, ref, commit} : {repo: REPO, ref}, @@ -155,6 +199,7 @@ const run_skills = async(opts: Skills_opts): Promise=>{ resolved: packs.map(p=>p.name), hosts, summary: summarize(hosts, opts.operation), + ...(orphans.length ? {orphans} : {}), }; }; diff --git a/src/skills/report.ts b/src/skills/report.ts index 1ecaab3..7f0991e 100644 --- a/src/skills/report.ts +++ b/src/skills/report.ts @@ -77,12 +77,12 @@ const host_line = (host: Host_outcome, report_action: Operation): string=>{ return `${mark} ${label}· ${parts.join('; ')}${note}`; }; -// Whether this run put anything new in front of the assistant, and so whether -// the user has to start a new session. The action labels alone cannot answer -// it: an unchanged version reports `current` in every adapter, which is right, -// but a flat host re-copying a newer commit at that same version did rewrite -// the files. `refreshed` carries exactly that, so both facts are consulted. -const changed = (report: Report): boolean=>report.hosts.some(h=> +// Hosts this run put something new in front of. The action labels alone cannot +// answer it: an unchanged version reports `current` in every adapter, which is +// right, but a flat host re-copying a newer commit at that same version did +// rewrite the files. `refreshed` carries exactly that, so both facts are +// consulted. +const changed_hosts = (report: Report): Host_outcome[]=>report.hosts.filter(h=> (h.packs ?? []).some(p=> p.action === 'installed' || p.action === 'upgraded' || p.refreshed === true)); @@ -119,9 +119,23 @@ const human_lines = (report: Report): string[]=>{ lines.push(pc.dim(` ${pack.name}: ${pack.detail}`)); } } - if (changed(report) && report.action !== 'list') + // Only the hosts that need it — restarting one that re-reads its skills + // every turn is work the host does not ask for. Named when a run touched + // both kinds. + const restart = changed_hosts(report).filter(h=>h.needs_new_session !== false); + if (restart.length && report.action !== 'list') { - lines.push(pc.dim('Start a new session in each assistant so the skills load.')); + lines.push(pc.dim(restart.length === changed_hosts(report).length + ? 'Start a new session in each assistant so the skills load.' + : `Start a new session in ${or_list(restart.map(h=>h.label))} so the skills load.`)); + } + for (const orphan of report.orphans ?? []) + { + lines.push(pc.yellow(`⚠ ${orphan.packs.join(', ')} recorded for '${orphan.host}'` + + ', an assistant this version no longer supports')); + lines.push(pc.dim(` ${orphan.files} file(s) left on disk` + + `${orphan.sample ? `, starting with ${orphan.sample}` : ''}` + + ' — delete them by hand; no --agent value reaches them now')); } return lines; }; diff --git a/src/skills/types.ts b/src/skills/types.ts index 24c768b..95b58d2 100644 --- a/src/skills/types.ts +++ b/src/skills/types.ts @@ -47,6 +47,9 @@ type Host_def = { // Skills directory relative to the project root (flat hosts, and native // hosts whose plugin mechanism cannot express a project install). project_skills_dir?: string; + // Whether the user must start a new session before the assistant sees newly + // installed skills. Required, so a new host decides it rather than omits it. + needs_new_session: boolean; verified: boolean; }; @@ -93,10 +96,25 @@ type Host_outcome = { // reports, so no adapter can forget it and a --json consumer can tell a // confirmed success from an unconfirmed one. verified?: boolean; + // Mirrors Host_def.needs_new_session, stamped by the orchestrator like + // `verified`: the reporter needs to know which hosts that line applies to. + needs_new_session?: boolean; }; type Operation = 'install' | 'list' | 'update' | 'remove'; +// Packs the journal records for a host no longer in the registry: a retirement +// leaves our files on disk with nothing able to reach them, since iteration +// covers registry hosts and a retired `--agent` id is a usage error. +type Orphaned_packs = { + host: string; + scope: Scope; + packs: string[]; + files: number; + // One recorded path, so the message points at a real directory. + sample?: string; +}; + type Report = { action: Operation; source: {repo: string; ref: string; commit?: string}; @@ -104,6 +122,7 @@ type Report = { resolved: string[]; hosts: Host_outcome[]; summary: {installed: number; skipped: number; failed: number}; + orphans?: Orphaned_packs[]; }; type Run_result = {code: number; stdout: string; stderr: string}; @@ -113,7 +132,7 @@ type Run_result = {code: number; stdout: string; stderr: string}; type Runner = (bin: string, args: string[])=>Promise; export type { - Pack, Pack_registry, Scope, Host_kind, Host_cli, Host_def, + Pack, Pack_registry, Scope, Host_kind, Host_cli, Host_def, Orphaned_packs, Pack_action, Pack_outcome, Host_status, Host_outcome, Operation, Report, Run_result, Runner, };