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-'));