From bd2e127292034e404a9f0b66777d5fde27f3bce2 Mon Sep 17 00:00:00 2001 From: Utkarsh Singh <6995377+vib795@users.noreply.github.com> Date: Mon, 17 Aug 2026 19:41:02 -0500 Subject: [PATCH] fix: reclaim skill links from a moved install, and stop setup failing in silence (0.6.4) Reported from an AVD desktop. Skills had been installed from a checkout in Downloads. The checkout was later deleted. Every skill link then pointed at a path that no longer existed, and no command in this tool could repair it: doctor -> "leftovers from an uninstall; remove them or run `agent-memory setup`" uninstall -> "[kept] ...\.agents\skills\handoff (not ours)" x12 setup -> no output at all, exit to prompt, nothing changed A closed loop. Each command named another as the fix. The ownership test was the cause. unlinkSkills decided a link was ours by asking whether it resolved to the skills directory we are packaged in right now: resolve(readlinkSync(link)) === join(packaged, name) which is false for every link made by an install that has since moved, and false for every link whose target was deleted. A disowned link is unreclaimable: the uninstall keeps it, and setup cannot replace what it will not clear. The links most in need of removal were the exact set the check refused to touch. Ownership is now: a link at a path we manage is ours if it points at our packaged skills, or if it dangles, or if it points into another agent-memory tree -- the last recognised by its parent holding all three of our skills, which a hand-written skill directory would not. Reclaiming a link that points nowhere costs nothing and is the whole difference between a recoverable machine and a deadlocked one. A live link to a directory outside any agent-memory tree is still kept, and a test now pins both halves so widening ownership cannot quietly swallow a skill the user wrote themselves. The silent setup is the second half, and I could not reproduce it. A dangling link on POSIX is cleared correctly and setup recovers, so the failure is specific to a Windows junction or to that desktop. Rather than claim a root cause I have not seen, this makes setup survivable and legible: - clear() now verifies the removal actually happened. `rmSync` with `force` swallows ENOENT, and a junction whose target is gone can answer that stat with ENOENT while the reparse point stays on disk -- the removal reports success, changes nothing, and the symlink that follows fails EEXIST. This is my best candidate for the reported silence, and it is a real bug either way. - Each skill installs inside its own try. One wedged path used to abort the run and discard the report with it, so eleven successes and one failure produced no output and no hint that anything had worked. - compact runs last and touches every registered skill path, which is the set most likely to hold a stale entry. Its failure no longer throws away the linking report; it is reported alongside it, with the two commands that retry just that step. - setup names what failed, with the path and the error, and points at uninstall. Four tests: a vanished install's link is reclaimed, a hand-made skill is not, setup replaces a stale link, and a throwing compact does not sink the run. Figures re-measured for ARCHITECTURE.md: 3,574 lines, 97 tests, cli.js 980, setup.js 282. Still zero runtime and zero dev dependencies. Co-Authored-By: Claude Opus 5 (1M context) --- .claude-plugin/marketplace.json | 2 +- .claude-plugin/plugin.json | 2 +- ARCHITECTURE.md | 10 ++-- package-lock.json | 4 +- package.json | 2 +- src/cli.js | 20 ++++++++ src/setup.js | 84 ++++++++++++++++++++++++++----- test/integration.test.js | 87 +++++++++++++++++++++++++++++++++ 8 files changed, 189 insertions(+), 22 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index ffb1ab0..24cd35a 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -10,7 +10,7 @@ "name": "agent-memory", "source": "./", "description": "Durable cross-repository memory for coding agents — built to survive an IT security review. Zero runtime dependencies, zero dev dependencies, no install script: nothing runs when you install it, and granting it your agents is a separate explicit command. Independently scanned, with a passing verdict. Adds /handoff, /remember and /recall over one local markdown store. Requires the CLI: npm install -g @vib795/agent-memory (Node >= 22.5).", - "version": "0.6.3", + "version": "0.6.4", "author": { "name": "Utkarsh Singh", "url": "https://github.com/vib795" diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 9a19773..aa19799 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "agent-memory", "displayName": "agent-memory", - "version": "0.6.3", + "version": "0.6.4", "description": "Durable cross-repository memory for coding agents — built to survive an IT security review. Zero runtime dependencies, zero dev dependencies, no install script: nothing runs when you install it, and granting it your agents is a separate explicit command. Independently scanned, with a passing verdict. Adds /handoff, /remember and /recall over one local markdown store. Requires the CLI: npm install -g @vib795/agent-memory (Node >= 22.5).", "author": { "name": "Utkarsh Singh", diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1f01e7f..caf2145 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -3,8 +3,8 @@ How agent-memory is built, and why it is built that way. Every figure here was read from the source rather than written from memory, at -v0.6.2: 15 modules, 3,494 lines of JavaScript, zero runtime dependencies and zero -dev dependencies, 93 tests. +v0.6.4: 15 modules, 3,574 lines of JavaScript, zero runtime dependencies and zero +dev dependencies, 97 tests. --- @@ -456,12 +456,12 @@ The properties that must stay true. Each is covered by the test suite. | Module | Lines | Responsibility | |---|--:|---| -| `cli.js` | 960 | 13 commands; one process, one answer | +| `cli.js` | 980 | 13 commands; one process, one answer | | `index-db.js` | 317 | SQLite cache: DDL, reindex, FTS5 search | | `pii.js` | 274 | export-time disclosure control | | `store.js` | 256 | notes as source of truth; frontmatter; atomic upsert | | `compact.js` | 283 | dedupe, supersede, decay, regenerate | -| `setup.js` | 222 | install into detected agent surfaces | +| `setup.js` | 282 | install into detected agent surfaces | | `staleness.js` | 213 | commit-distance staleness and capture gap | | `digest.js` | 209 | two-tier routing: description + tree | | `config.js` | 191 | engagement resolution; two-file config split | @@ -471,7 +471,7 @@ The properties that must stay true. Each is covered by the test suite. | `redact.js` | 97 | capture-time fail-closed secret redaction | | `promptfile.js` | 78 | VS Code prompt files derived from SKILL.md | | `atomic.js` | 57 | atomic write; imports nothing from this package | -| **total** | **3,494** | zero dependencies, 93 tests | +| **total** | **3,574** | zero dependencies, 97 tests | `atomic.js` deliberately imports nothing from the package: `store.js` already imports `config.js`, so putting the atomic write in either would create a cycle. diff --git a/package-lock.json b/package-lock.json index b766493..7f7d1f2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@vib795/agent-memory", - "version": "0.6.3", + "version": "0.6.4", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@vib795/agent-memory", - "version": "0.6.3", + "version": "0.6.4", "license": "MIT", "bin": { "agent-memory": "src/cli.js" diff --git a/package.json b/package.json index c8a8413..038a86a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@vib795/agent-memory", - "version": "0.6.3", + "version": "0.6.4", "description": "Durable cross-repo knowledge graph for GitHub Copilot and Claude Code. Markdown source of truth, disposable SQLite index, zero runtime dependencies.", "keywords": [ "github-copilot", diff --git a/src/cli.js b/src/cli.js index 2770889..4e54faa 100755 --- a/src/cli.js +++ b/src/cli.js @@ -164,6 +164,26 @@ function cmdSetup() { ); } + // Naming what did not install is the whole point. A run that silently installs + // eleven of twelve reads as a success and sends the user back to an editor that is + // still missing a skill. + if (r.failed.length) { + lines.push('', ' Failed:'); + for (const f of r.failed) lines.push(` [failed] ${f.path} — ${f.error}`); + lines.push( + '', + ' A path that will not clear is usually a leftover link from an install that has', + ' since moved. `agent-memory uninstall` reclaims those, then run setup again.', + ); + } + if (r.compactError) { + lines.push( + '', + ` Skills are installed, but refreshing the /recall description failed: ${r.compactError}`, + ' Run `agent-memory index` then `agent-memory compact` to retry just that step.', + ); + } + return { ok: true, ...r, diff --git a/src/setup.js b/src/setup.js index bdf097a..4af9644 100644 --- a/src/setup.js +++ b/src/setup.js @@ -55,11 +55,45 @@ function clear(path) { // end stay put — which is exactly what passing `recursive` here would risk. rmdirSync(path); } + // `force` tells rmSync to swallow ENOENT, and a junction whose target is gone can + // answer the stat rmSync makes with ENOENT while the reparse point itself stays on + // disk. The removal then reports success and changes nothing, after which + // symlinkSync fails EEXIST and the copy fallback lands on a path that still + // exists. Checking is what turns that silent no-op into an error naming the path. + if (isLink(path)) rmdirSync(path); } else if (existsSync(path)) { rmSync(path, { recursive: true, force: true }); } } +/** + * Decide whether a skill link at a path we manage was put there by this tool. + * + * Identity cannot be "points at where we live right now". An install that has since + * moved, or a checkout that was deleted, leaves a link that test would disown — and a + * disowned link is unreclaimable: `uninstall` keeps it as "not ours" and `setup` + * cannot replace what it will not clear, so the user is left holding a broken link + * that no command in this tool will fix. + */ +function ownsLink(link, name) { + let dest; + try { + dest = resolve(readlinkSync(link)); + } catch { + // A link sitting at one of our paths whose target cannot even be read is ours to + // clear. Nothing downstream can use it either. + return true; + } + if (dest === join(packagedSkillsDir(), name)) return true; + // Dangling. Nothing is lost by reclaiming a link that points nowhere, and leaving it + // is precisely what deadlocks both commands. + if (!existsSync(dest)) return true; + // Live, but pointing into some other agent-memory tree — an older global install, or + // a checkout the user set up from. The signature is that its parent holds all three + // of our skills, which a hand-written skill directory would not. + return SKILLS.every((s) => existsSync(join(dirname(dest), s, 'SKILL.md'))); +} + /** * Link one skill into one agent directory. * @@ -130,7 +164,6 @@ export function danglingSkillLinks() { * that indexed them; removing them is a separate, deliberate act. */ export function unlinkSkills() { - const packaged = packagedSkillsDir(); const removed = []; const kept = []; @@ -141,11 +174,9 @@ export function unlinkSkills() { if (!existsSync(link) && !isLink(link)) continue; let owned = false; try { - owned = isLink(link) - ? resolve(readlinkSync(link)) === join(packaged, name) - : existsSync(join(link, 'SKILL.md')); + owned = isLink(link) ? ownsLink(link, name) : existsSync(join(link, 'SKILL.md')); } catch { - // A link we cannot read is a link we cannot claim. Leave it. + // A directory we cannot stat is one we cannot claim. Leave it. owned = false; } if (owned) { @@ -182,15 +213,30 @@ export function setup({ compactFn } = {}) { const targets = installableTargets(); const installed = []; const copies = []; + const failed = []; for (const target of targets) { for (const name of SKILLS) { - const r = - target.kind === 'skill-dir' - ? linkSkill(name, target.dir) - : writePromptFile(name, target.dir); - installed.push({ ...r, target: target.id, label: target.label }); - if (r.mode === 'copy') copies.push(r); + // Isolated per skill. One unwritable target used to abort the whole run and + // discard the report with it, so a user whose `.copilot` link was wedged got no + // output at all and no hint that the other eleven had been fine. A failure here + // is data to print, not a reason to stop. + try { + const r = + target.kind === 'skill-dir' + ? linkSkill(name, target.dir) + : writePromptFile(name, target.dir); + installed.push({ ...r, target: target.id, label: target.label }); + if (r.mode === 'copy') copies.push(r); + } catch (err) { + failed.push({ + name, + target: target.id, + label: target.label, + path: join(target.dir, target.kind === 'skill-dir' ? name : `${name}.prompt.md`), + error: err.message, + }); + } } } @@ -209,11 +255,25 @@ export function setup({ compactFn } = {}) { // compact is passed in so this module does not pull the database into memory just // to make some symlinks. - const result = compactFn ? compactFn() : null; + // + // Its failure must not sink the run. Linking is already done and written to disk by + // this point, so throwing here would throw away an accurate report of work that + // actually happened and leave the user with no idea any of it succeeded. compact + // touches every registered skill path, which is exactly the set most likely to hold + // a stale entry, so it is the step most likely to throw. + let result = null; + let compactError = null; + try { + result = compactFn ? compactFn() : null; + } catch (err) { + compactError = err.message; + } return { targets, installed, copies, + failed, + compactError, skillPaths, home: agentHome(), digest: result?.digest ?? null, diff --git a/test/integration.test.js b/test/integration.test.js index 882ebc6..1a0d50c 100644 --- a/test/integration.test.js +++ b/test/integration.test.js @@ -1078,6 +1078,93 @@ test('uninstall removes our links and prompt files, spares foreign ones, keeps n } }); +test('uninstall reclaims a link whose install location moved or vanished', () => { + // The field report this fixes: skills installed from a checkout, the checkout later + // deleted, and every link then disowned because it no longer resolved to the current + // package. `uninstall` kept them as "not ours" and `setup` would not replace what it + // would not clear, so no command in the tool could repair the machine. + seed().close(); + const home = fakeHome({ claude: true }); + try { + const dir = skillTargets()[0]; + mkdirSync(dir, { recursive: true }); + const stale = join(dir, 'handoff'); + symlinkSync(join(home, 'deleted-checkout', 'skills', 'handoff'), stale); + + const r = unlinkSkills(); + assert.ok(r.removed.includes(stale), 'a link pointing nowhere is ours to clear'); + assert.ok(!r.kept.includes(stale), "and must not be reported as someone else's"); + assert.equal(existsSync(stale), false); + } finally { + delete process.env.AGENT_MEMORY_SKILLS_HOME; + rmSync(home, { recursive: true, force: true }); + } +}); + +test('a hand-made skill directory is still not ours to remove', () => { + // The other half of the same rule. Widening ownership to reclaim stale links must + // not widen it to a skill the user wrote, which is why a lone SKILL.md with no + // siblings stays put. + seed().close(); + const home = fakeHome({ claude: true }); + try { + const dir = skillTargets()[0]; + mkdirSync(dir, { recursive: true }); + const mine = join(home, 'my-own-skills', 'handoff'); + mkdirSync(mine, { recursive: true }); + writeFileSync(join(mine, 'SKILL.md'), '---\nname: handoff\n---\nmine\n', 'utf8'); + const link = join(dir, 'handoff'); + symlinkSync(mine, link); + + const r = unlinkSkills(); + assert.ok(r.kept.includes(link), 'a live link outside any agent-memory tree is left alone'); + assert.equal(existsSync(join(mine, 'SKILL.md')), true, 'and its target survives'); + } finally { + delete process.env.AGENT_MEMORY_SKILLS_HOME; + rmSync(home, { recursive: true, force: true }); + } +}); + +test('setup replaces a link left behind by a vanished install', () => { + seed().close(); + const home = fakeHome({ claude: true }); + try { + const dir = skillTargets()[0]; + mkdirSync(dir, { recursive: true }); + symlinkSync(join(home, 'gone', 'skills', 'handoff'), join(dir, 'handoff')); + + const r = setup({}); + assert.deepEqual(r.failed, [], 'a stale link is cleared, not a failure'); + assert.ok(existsSync(join(dir, 'handoff', 'SKILL.md')), 'link now resolves to the package'); + } finally { + delete process.env.AGENT_MEMORY_SKILLS_HOME; + rmSync(home, { recursive: true, force: true }); + } +}); + +test('setup reports a failing compact instead of discarding the whole run', () => { + // compact runs last and touches every registered skill path, so it is the step most + // likely to throw on a machine with a stale registration. Linking is already on disk + // by then; throwing away the report of it leaves the user with no output at all. + seed().close(); + const home = fakeHome({ claude: true }); + try { + const r = setup({ + compactFn: () => { + throw new Error('ENOENT: no such file or directory'); + }, + }); + assert.match(r.compactError, /ENOENT/); + assert.ok(r.installed.length > 0, 'the skills that did install are still reported'); + for (const name of SKILLS) { + assert.ok(existsSync(join(skillTargets()[0], name)), `${name} installed despite compact`); + } + } finally { + delete process.env.AGENT_MEMORY_SKILLS_HOME; + rmSync(home, { recursive: true, force: true }); + } +}); + test('a link left pointing at a removed package is detected', () => { seed().close(); const home = mkdtempSync(join(tmpdir(), 'agent-memory-home-'));