diff --git a/src/cli.ts b/src/cli.ts index b28ee6a0..f3a659a9 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -250,7 +250,7 @@ function describeSkillResult(result: AgentSkillApplyResult, target: string): { o }, symlink: { ok: false, - text: `${target} (or its parent) is a symlink, refusing to write through it.`, + text: `${target}, its parent or its reference folder is a symlink, refusing to write through it.`, }, }; return messages[result]; @@ -320,7 +320,11 @@ skillCmd if (result === 'symlink') { // A shared-skills layout (`.agents/skills -> ../.claude/skills`) is deliberate: // skip it rather than fail the whole sweep. - console.log(palette.warn(`- Skipped ${target}: it (or its parent) is a symlink, so it was not touched`)); + console.log( + palette.warn( + `- Skipped ${target}: it, its parent or its reference folder is a symlink, so it was not touched` + ) + ); continue; } removedAny = true; diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 38eeeb01..9708b5f1 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -1358,6 +1358,31 @@ async function isSymlink(path: string): Promise { } } +/** + * Whether a folder INSIDE the copy that holds skill files (today only `reference/`) is a + * symlink. The checks on the copy and its parent stop at the copy itself, and a write or + * unlink of `reference/.md` follows a linked `reference/` wherever it points: + * measured, `reference -> ` made install overwrite a `history.md` there and + * uninstall delete it. A cloned repository can commit that link, and a Docker case's + * container can plant it in the bind-mounted workspace that the host injects into, so + * both are refused like a linked copy. Checked once before the first write or unlink; a + * link swapped in after the check (a race against the container) is not covered. + */ +async function agentSkillSubdirIsSymlink(skillDir: string, files: AgentSkillFile[]): Promise { + const subdirs = new Set(['reference']); + for (const file of files) { + // Every folder on the way to the file, outermost first: `a/b/x.md` checks `a`, then `a/b`. + const parts = dirname(file.relPath) + .split(/[\\/]/) + .filter((part) => part !== '' && part !== '.'); + for (let i = 1; i <= parts.length; i++) subdirs.add(join(...parts.slice(0, i))); + } + for (const subdir of subdirs) { + if (await isSymlink(join(skillDir, subdir))) return true; + } + return false; +} + /** * The ignore file every Codeman-managed skill copy carries, and what it holds: `*` * matches everything in the copy's own folder (the ignore file included), so a user's @@ -1422,17 +1447,19 @@ export type AgentSkillApplyResult = | 'removed' // our copy deleted | 'absent' // nothing there to remove | 'foreign' // a copy exists but is not ours; left untouched - | 'symlink'; // the skill dir (or its parent) is a symlink; left untouched + | 'symlink'; // the skill dir, its parent or a folder inside it is a symlink; left untouched /** * Install or refresh the Codeman agent skill into `skillDir` (a `.../codeman` * directory, e.g. `/.claude/skills/codeman`, `~/.claude/skills/codeman` or * `/.agents/skills/codeman`). * - * Refuses two shapes rather than writing through them: + * Refuses these shapes rather than writing through them: * - a SYMLINK at the skill dir or its `skills/` parent: this repo's own dogfooding * layout (`.claude/skills/codeman -> ../../skills/codeman`) would otherwise have * the injector overwrite the repo source through the link; + * - a SYMLINK at a folder inside the copy (`reference/`), which would carry the skill's + * files to wherever it points (`agentSkillSubdirIsSymlink`); * - a FOREIGN copy (SKILL.md present without our marker): that is the user's own * skill, and per the statusLine rule we never clobber what we did not write. * @@ -1464,6 +1491,7 @@ export async function installAgentSkillInto(skillDir: string): Promise [{ relPath: 'SKILL.md', content: '' }]); + // An unlink through a linked `reference/` deletes files wherever it points. + if (await agentSkillSubdirIsSymlink(skillDir, files)) return 'symlink'; for (const file of files) { await unlink(join(skillDir, file.relPath)).catch(() => {}); } diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index a9158eaa..8e82704f 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -1003,7 +1003,7 @@ async function injectAgentSkill(casePath: string, dir: AgentSkillDir): Promise { expect(await installAgentSkillInto(skillDir())).toBe('symlink'); expect(await readdir(join(casePath, 'real-skills'))).toEqual([]); }); + + // A cloned repository can commit `reference` as a link, and a Docker case's container + // can plant one in the bind-mounted workspace the host injects into (on by default). + it('refuses a symlinked reference/ folder inside the copy, so nothing is written where it points', async () => { + const outside = join(casePath, 'outside'); + await mkdir(outside, { recursive: true }); + await writeFile(join(outside, 'history.md'), 'my notes\n'); + await mkdir(skillDir(), { recursive: true }); + await symlink(outside, join(skillDir(), 'reference')); + + expect(await applyAgentSkill(casePath, true)).toBe('symlink'); + expect(await readdir(outside)).toEqual(['history.md']); + expect(await readFile(join(outside, 'history.md'), 'utf-8')).toBe('my notes\n'); + // Refused before the first write: no SKILL.md, no ignore file. + expect(await readdir(skillDir())).toEqual(['reference']); + }); + + it('refuses a symlinked reference/ folder planted beside an existing Codeman copy', async () => { + await applyAgentSkill(casePath, true); + const outside = join(casePath, 'outside'); + await mkdir(outside, { recursive: true }); + await writeFile(join(outside, 'recipes.md'), 'my recipes\n'); + await rm(join(skillDir(), 'reference'), { recursive: true }); + await symlink(outside, join(skillDir(), 'reference')); + const skillMd = await readFile(join(skillDir(), 'SKILL.md'), 'utf-8'); + await writeFile(join(skillDir(), 'SKILL.md'), `stale\n${MARKER_PREFIX}: old -->\n`); + + expect(await applyAgentSkill(casePath, true)).toBe('symlink'); + expect(await readdir(outside)).toEqual(['recipes.md']); + expect(await readFile(join(outside, 'recipes.md'), 'utf-8')).toBe('my recipes\n'); + // The stale copy is left as found, not refreshed through the link. + expect(await readFile(join(skillDir(), 'SKILL.md'), 'utf-8')).not.toBe(skillMd); + }); }); describe('removeAgentSkillFrom / applyAgentSkill(disabled)', () => { + it('refuses to delete through a symlinked reference/ folder inside the copy', async () => { + await applyAgentSkill(casePath, true); + const outside = join(casePath, 'outside'); + await mkdir(outside, { recursive: true }); + // Files named like the packaged reference files: an unlink through the link would take them. + for (const name of await readdir(join(skillDir(), 'reference'))) { + await writeFile(join(outside, name), 'not yours\n'); + } + const names = (await readdir(outside)).sort(); + expect(names.length).toBeGreaterThan(0); + await rm(join(skillDir(), 'reference'), { recursive: true }); + await symlink(outside, join(skillDir(), 'reference')); + + expect(await applyAgentSkill(casePath, false)).toBe('symlink'); + expect((await readdir(outside)).sort()).toEqual(names); + for (const name of names) expect(await readFile(join(outside, name), 'utf-8')).toBe('not yours\n'); + expect(existsSync(join(skillDir(), 'SKILL.md'))).toBe(true); + }); + it('removes our copy and prunes the emptied directories', async () => { await applyAgentSkill(casePath, true); expect(await applyAgentSkill(casePath, false)).toBe('removed');