From dabcc1071fd2dc78f121c6c132897f8733257c4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Obudzi=C5=84ski?= Date: Wed, 16 Sep 2026 15:52:32 +0200 Subject: [PATCH 1/6] feat(config): add skillSearchPaths for read-only awareness of shared skill libraries Skills that live outside the repo (a machine-wide shared tree, or ~/.claude/skills) now count as existing for AGENTS.md reference resolution, the failed-trigger rule, and extraction dedup, so backpass stops proposing duplicates of skills it does not own. skillSearchPaths is an array of directory paths (~ expanded) accepted wherever skillsDir is; loadConfig folds it into the existing skillsDirs awareness list, consulted after skillsDir in list order. It is read-only: every write still targets skillsDir, and a skill resolving outside the repo is withheld from the synthesis staging copy. Refs kunchenguid/backpass#32 --- AGENTS.md | 6 ++++++ README.md | 11 +++++++++++ src/config.js | 30 +++++++++++++++++++++++++++++- test/config.test.js | 19 +++++++++++++++++++ test/skills.test.js | 22 ++++++++++++++++++++++ test/workspace.test.js | 31 ++++++++++++++++++++++++++++++- 6 files changed, 117 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 00a2b68..c04b243 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -236,6 +236,12 @@ list` only sees this clone. `attachSiblingClones` in `src/repo.js` also searches are stat'd (`isDirectoryEntry`), fail-soft, and a broken or cyclic link reads as absent. One library reached through k links is k loaded entries, billed k times - `loadedCopies` multiplies a description-line delta by that count in both `buildProposal` and the writer's projection. + `skillSearchPaths` (config) is read-only awareness for skills that live outside the repo + (a shared/fleet library); `loadConfig` expands `~` and folds it into the existing + `skillsDirs` awareness list (consulted after `skillsDir`), so every read site already sees + it. Writes never target a search path: `resolveOverflowTarget` only returns `skillsDir`, + and a skill resolving outside the repo is withheld from the synthesis staging copy + (`src/workspace.js`). - **Memory resolution is pointer-aware** (`resolveMemoryFiles` in `src/memory.js`): the first configured file is canonical, a `@AGENTS.md`-only CLAUDE.md is a pointer, and a second full file is warned about, never silently ignored or double-written. diff --git a/README.md b/README.md index 7579462..50feb56 100644 --- a/README.md +++ b/README.md @@ -682,6 +682,7 @@ CLI flags on top: "memoryFiles": ["AGENTS.md"], "budgetTokens": 5000, "skillsDir": ".agents/skills", + "skillSearchPaths": [], "maxEditsPerRun": null, "minGapEvidence": 2, "gapLedgerMaxAge": "90d", @@ -728,6 +729,16 @@ regular settings; its path and user-only settings include `memoryFiles`, `skills instead, such as `.claude/skills`, configure that path; a missing configured directory falls back to the default. Backpass normalizes path separators and trailing slashes. +`skillSearchPaths` lists additional directories to consult for _existing_ skills, +alongside `skillsDir` and consulted after it in list order. `~` is expanded. Use it when +your canonical skill library lives outside the repo - a machine-wide shared tree, or +`~/.claude/skills` - so backpass recognizes those skills as already existing: an +`AGENTS.md` reference into the shared tree is not treated as dangling, a failed trigger +tunes the shared skill's description instead of duplicating its content, and an +extraction whose content substantially matches a shared skill becomes a pointer edit +rather than new content. This is read-only awareness: backpass never writes into a +search path - every write still targets only `skillsDir`. + ```json { "user": { diff --git a/src/config.js b/src/config.js index ebf16f1..cef6551 100644 --- a/src/config.js +++ b/src/config.js @@ -47,6 +47,15 @@ export const DEFAULT_CONFIG = { memoryFiles: ["AGENTS.md", "CLAUDE.md"], budgetTokens: 5000, skillsDir: ".agents/skills", + /** + * Extra directories to consult for *existing* skills, alongside `skillsDir`, when + * deciding whether an AGENTS.md pointer already resolves, whether a failed trigger + * needs a description edit instead of duplicate content, and whether an extraction + * should point at a shared skill rather than create a new one. `~` is expanded. These + * are read-only awareness: writes always target only `skillsDir`, never a search path + * (a skill resolving outside the repo is withheld from the synthesis staging copy). + */ + skillSearchPaths: [], /** `null` means adaptive: see `effectiveMaxEdits` in proposal.js. An integer pins it. */ maxEditsPerRun: null, minGapEvidence: 2, @@ -142,6 +151,14 @@ export const USER_CONFIG_DEFAULTS = { }, }; +/** Expand a leading `~` to the home directory; other paths pass through unchanged. */ +export function expandHomePath(p, home = os.homedir()) { + if (typeof p !== "string") return p; + if (p === "~") return home; + if (p.startsWith("~/")) return path.join(home, p.slice(2)); + return p; +} + export function parseScopeKind(value) { if (value === undefined || value === null || value === "") return "project"; if (value === "project" || value === "user") return value; @@ -247,6 +264,11 @@ function validate(config, { kind = "project" } = {}) { throw new UserError("config.skillsDirs must be an array of paths"); } } + if (config.skillSearchPaths !== undefined) { + if (!Array.isArray(config.skillSearchPaths) || config.skillSearchPaths.some((d) => typeof d !== "string")) { + throw new UserError("config.skillSearchPaths must be an array of paths"); + } + } const includeProjects = config.discovery.includeProjects; const excludeProjects = config.discovery.excludeProjects; if ( @@ -378,7 +400,13 @@ export function loadConfig(repoRoot, overrides = {}, { kind = "project" } = {}) if (config.discovery.includeCursorIde && !config.discovery.harnesses.includes("cursor-ide")) { config.discovery.harnesses = [...config.discovery.harnesses, "cursor-ide"]; } - return validate(config, { kind: scopeKind }); + const validated = validate(config, { kind: scopeKind }); + // `skillSearchPaths` is the read-side awareness key. It rides the existing `skillsDirs` + // awareness list (consulted after `skillsDir` in list order) rather than a second plumbing; + // `~` is expanded here so the loaders that join `repoRoot` never mishandle a home path. + const searchPaths = (validated.skillSearchPaths || []).map((p) => expandHomePath(p)); + if (searchPaths.length) validated.skillsDirs = [...(validated.skillsDirs || []), ...searchPaths]; + return validated; } /** diff --git a/test/config.test.js b/test/config.test.js index ed8300a..ca09637 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -84,6 +84,25 @@ test("skillsDir rejects malformed configuration values", () => { } }); +test("skillSearchPaths defaults to none and rejects non-array-of-strings values", () => { + assert.deepEqual(loadConfig(tempRepo()).skillSearchPaths, []); + for (const skillSearchPaths of ["~/.claude/skills", 42, [1], [{}], {}]) { + assert.throws(() => loadConfig(tempRepo({ skillSearchPaths })), UserError); + } +}); + +test("skillSearchPaths expands ~ and feeds the read-only awareness list without touching skillsDir", () => { + const home = os.homedir(); + const config = loadConfig(tempRepo({ skillSearchPaths: ["~/.hermes/skills-shared", "~/.claude/skills"] })); + // The write target is untouched; the search paths join the awareness roots, ~ expanded. + assert.equal(config.skillsDir, ".agents/skills"); + assert.deepEqual(config.skillSearchPaths, ["~/.hermes/skills-shared", "~/.claude/skills"]); + assert.deepEqual(config.skillsDirs, [ + path.join(home, ".hermes", "skills-shared"), + path.join(home, ".claude", "skills"), + ]); +}); + test("--include-cursor-ide is the only way the deferred store is scanned", () => { const config = loadConfig(tempRepo(), { discovery: { includeCursorIde: true } }); assert.ok(config.discovery.harnesses.includes("cursor-ide")); diff --git a/test/skills.test.js b/test/skills.test.js index e5a9e40..1375cf2 100644 --- a/test/skills.test.js +++ b/test/skills.test.js @@ -177,6 +177,28 @@ test("user skill discovery uses only configured harness roots", () => { ); }); +test("a skill in an outside search path counts as existing, and never becomes the write target", () => { + const root = tmpRepo(); + // The canonical library is a shared tree living outside the repo (e.g. ~/.claude/skills). + const shared = fs.mkdtempSync(path.join(os.tmpdir(), "backpass-shared-skills-")); + const shSkill = path.join(shared, "xsearch-percall-row-ceiling", "SKILL.md"); + fs.mkdirSync(path.dirname(shSkill), { recursive: true }); + fs.writeFileSync(shSkill, "---\nname: xsearch-percall-row-ceiling\ndescription: cap rows per call\n---\n\nbody\n"); + + // Awareness: an AGENTS.md pointer to this skill resolves through the search path, so it + // is not dangling and backpass must not propose re-creating it in the repo. + const aware = loadProjectSkills(root, CANONICAL_SKILLS_DIR, [shared]); + assert.deepEqual( + aware.map((skill) => skill.name), + ["xsearch-percall-row-ceiling"], + ); + assert.ok(path.isAbsolute(aware[0].path), "an outside skill keeps its absolute path"); + + // Read-only: the overflow write target is always the repo's own skillsDir, never a + // search path, whatever the search paths contain. + assert.equal(resolveOverflowTarget(root, CANONICAL_SKILLS_DIR).dir, CANONICAL_SKILLS_DIR); +}); + test("project skill discovery includes separate canonical and Claude roots without double-counting symlinks", () => { const root = tmpRepo(); const canonical = path.join(root, CANONICAL_SKILLS_DIR, "generated", "SKILL.md"); diff --git a/test/workspace.test.js b/test/workspace.test.js index e1fef8b..240aa3b 100644 --- a/test/workspace.test.js +++ b/test/workspace.test.js @@ -5,7 +5,7 @@ import os from "node:os"; import path from "node:path"; import { readMemoryFile } from "../src/memory.js"; -import { loadProjectSkills, loadSkills, skillDescriptionTokens } from "../src/skills.js"; +import { loadProjectSkills, loadSkills, resolveProjectSkillDirs, skillDescriptionTokens } from "../src/skills.js"; import { estimateTokens } from "../src/tokens.js"; import { State } from "../src/state.js"; import { @@ -352,6 +352,35 @@ test("two links to one shared library are both billed, and exactly one of them i ]); }); +test("a skill in an outside search path is loaded for awareness but never staged for writing", () => { + // The canonical library is a shared tree outside the repo, named by skillSearchPaths. + const shared = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-search-path-"))); + fs.mkdirSync(path.join(shared, "db")); + fs.writeFileSync(path.join(shared, "db", "SKILL.md"), SKILL); + + const repo = makeRepo({ "AGENTS.md": AGENTS }); + const state = new State(repo.root).ensure(); + const memoryFile = readMemoryFile(repo.root, "AGENTS.md"); + + // The search path joins the awareness roots (this is what config merges into skillsDirs). + const skillDirs = resolveProjectSkillDirs(repo.root, ".agents/skills", [shared]); + assert.ok(skillDirs.includes(shared), "the outside search path is an awareness root"); + assert.deepEqual( + loadProjectSkills(repo.root, ".agents/skills", [shared]).map((s) => s.name), + ["db"], + "the shared skill is visible for reference/dedup awareness", + ); + + // Project scope (allowExternal false) must withhold the outside skill from staging, so + // synthesis can never emit an edit that writes into the shared library. + const workspace = prepareWorkspace({ state, repo, memoryFile, skillsDir: ".agents/skills", skillDirs }); + assert.deepEqual([...workspace.originals.keys()], ["AGENTS.md"]); + assert.deepEqual(walkStaged(path.join(workspace.root, workspacePathFor(shared))), []); + assert.deepEqual(workspace.unstageable, [ + { path: path.join(shared, "db"), reason: "resolves outside the repository" }, + ]); +}); + test("a skill file linked out of the repo through an in-repo library is never staged", () => { const outside = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-outside-store-"))); fs.writeFileSync(path.join(outside, "SKILL.md"), SKILL); From 878d5acbe2b083eed449286a8570a4a821527c4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Obudzi=C5=84ski?= Date: Wed, 16 Sep 2026 17:12:20 +0200 Subject: [PATCH 2/6] =?UTF-8?q?no-mistakes(ci):=20Fixed=20the=20three=20se?= =?UTF-8?q?lected=20Greptile=20findings=20on=20PR=20#138=20(skillSearchPat?= =?UTF-8?q?hs=20read-only=20awareness):=20-=20ci-4/ci-5=20(src/config.js:4?= =?UTF-8?q?08,=20"search=20paths=20become=20writable"=20/=20"external=20wr?= =?UTF-8?q?ites=20escape=20detection"):=20`skillSearchPaths`=20were=20merg?= =?UTF-8?q?ed=20into=20`skillsDirs`,=20which=20`prepareWorkspace`=20also?= =?UTF-8?q?=20uses=20to=20decide=20what's=20staged/writable.=20In=20user?= =?UTF-8?q?=20scope=20`allowExternal`=20disables=20all=20confinement,=20so?= =?UTF-8?q?=20a=20configured=20search-path=20root=20was=20staged=20and=20w?= =?UTF-8?q?ritable=20there,=20and=20even=20where=20staging=20correctly=20w?= =?UTF-8?q?ithheld=20it=20(project=20scope),=20it=20was=20excluded=20from?= =?UTF-8?q?=20the=20repo=20fingerprint=20like=20any=20other=20withheld=20s?= =?UTF-8?q?kill,=20so=20a=20direct=20write=20to=20it=20would=20silently=20?= =?UTF-8?q?escape=20detection.=20Fixed=20by=20threading=20the=20raw=20`ski?= =?UTF-8?q?llSearchPaths`=20roots=20(`searchPathRoots`)=20into=20`prepareW?= =?UTF-8?q?orkspace`,=20`skillStagingRefusal`=20(used=20by=20`--target`),?= =?UTF-8?q?=20and=20`measureWorkspace`,=20adding=20an=20unconditional=20`R?= =?UTF-8?q?EAD=5FONLY=5FSEARCH=5FPATH`=20refusal=20that=20applies=20regard?= =?UTF-8?q?less=20of=20`allowExternal`/confinement,=20and=20keeping=20sear?= =?UTF-8?q?ch-path=20skills=20in=20the=20synthesis=20fingerprint=20(unlike?= =?UTF-8?q?=20ordinary=20withheld=20skills)=20so=20a=20direct=20write=20to?= =?UTF-8?q?=20one=20now=20aborts=20the=20run=20loudly.=20Touched:=20src/wo?= =?UTF-8?q?rkspace.js,=20src/synthesize.js,=20src/scope.js,=20src/target.j?= =?UTF-8?q?s.=20-=20ci-6=20(README.md:737,=20"trigger=20tuning=20is=20impo?= =?UTF-8?q?ssible"):=20corrected=20the=20doc=20claim=20that=20a=20failed?= =?UTF-8?q?=20trigger=20tunes=20a=20shared=20skill's=20description=20?= =?UTF-8?q?=E2=80=94=20a=20search-path=20skill=20is=20read-only=20in=20eve?= =?UTF-8?q?ry=20scope,=20so=20it's=20now=20documented=20as=20"reported=20a?= =?UTF-8?q?s=20already=20covered,=20not=20tuned."=20ci-1/ci-2/ci-3=20were?= =?UTF-8?q?=20left=20untouched=20per=20instructions=20(GitHub=20holding=20?= =?UTF-8?q?fork=20workflow=20runs=20for=20maintainer=20approval;=20not=20a?= =?UTF-8?q?=20code=20defect).=20Added=20regression=20tests=20(test/workspa?= =?UTF-8?q?ce.test.js,=20test/synthesize.test.js)=20covering:=20a=20search?= =?UTF-8?q?=20path=20staying=20read-only=20in=20user=20scope=20unlike=20an?= =?UTF-8?q?=20ordinary=20external=20skills=20dir,=20a=20newly-created=20fi?= =?UTF-8?q?le=20under=20a=20search-path=20root=20being=20reported=20`stray?= =?UTF-8?q?`=20rather=20than=20a=20measurable=20`created`=20change,=20and?= =?UTF-8?q?=20an=20end-to-end=20synthesis=20run=20rejecting=20loudly=20whe?= =?UTF-8?q?n=20the=20harness=20writes=20directly=20to=20a=20search-path=20?= =?UTF-8?q?skill.=20Verified:=20lint,=20format=20check,=20typecheck,=20and?= =?UTF-8?q?=20the=20full=20test=20suite=20(676=20tests)=20all=20pass?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 3 +- src/scope.js | 3 ++ src/synthesize.js | 34 +++++++++++++++--- src/target.js | 5 ++- src/workspace.js | 76 +++++++++++++++++++++++++++++++++-------- test/synthesize.test.js | 28 +++++++++++++++ test/workspace.test.js | 73 ++++++++++++++++++++++++++++++++++++++- 7 files changed, 200 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 50feb56..00cfedb 100644 --- a/README.md +++ b/README.md @@ -734,7 +734,8 @@ alongside `skillsDir` and consulted after it in list order. `~` is expanded. Use your canonical skill library lives outside the repo - a machine-wide shared tree, or `~/.claude/skills` - so backpass recognizes those skills as already existing: an `AGENTS.md` reference into the shared tree is not treated as dangling, a failed trigger -tunes the shared skill's description instead of duplicating its content, and an +against a shared skill is reported as already covered instead of proposed as a duplicate +(backpass cannot tune its description - the file stays read-only, in every scope), and an extraction whose content substantially matches a shared skill becomes a pointer edit rather than new content. This is read-only awareness: backpass never writes into a search path - every write still targets only `skillsDir`. diff --git a/src/scope.js b/src/scope.js index 78bc29d..08e1e57 100644 --- a/src/scope.js +++ b/src/scope.js @@ -199,6 +199,7 @@ function resolveProjectScope(repo, config) { modelCwd: repo.root, memoryFiles: config.memoryFiles, skillDirs: config.skillsDirs || [], + skillSearchPaths: (config.skillSearchPaths || []).map((p) => expandUserPath(p)), overflowDir: config.skillsDir, associate: (descriptor, options = {}) => { const result = associateProject(descriptor, repo, { @@ -231,6 +232,7 @@ function resolveUserScope(cwd, config, { strict = false, home = os.homedir(), as const memoryFiles = (config.memoryFiles || []).map((file) => pathInRoot(file, root, home)); const overflowDir = pathInRoot(config.skillsDir || ".agents/skills", root, home); const skillDirs = (config.skillsDirs || []).map((dir) => pathInRoot(dir, root, home)); + const skillSearchPaths = (config.skillSearchPaths || []).map((p) => expandUserPath(p, home)); const repo = syntheticUserRepo(root); const stateDir = userStateDir(); const associationCache = new Map(); @@ -275,6 +277,7 @@ function resolveUserScope(cwd, config, { strict = false, home = os.homedir(), as modelCwd: stateDir, memoryFiles, skillDirs, + skillSearchPaths, overflowDir, associate, associateRemote: (descriptor, { facts, host }) => associateUserRemote(descriptor, { facts, host, strict }), diff --git a/src/synthesize.js b/src/synthesize.js index 74b7db0..78d86cd 100644 --- a/src/synthesize.js +++ b/src/synthesize.js @@ -2,7 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import { extractJson, isBlankOutput, openSession, usageRecord } from "./acpx.js"; -import { userClaudeSkillsDir } from "./config.js"; +import { expandHomePath, userClaudeSkillsDir } from "./config.js"; import { renderEvidenceForPrompt } from "./fold.js"; import { renderInstructionIndex, resolveMemoryPath } from "./memory.js"; import { renderPrompt, render, loadPrompt } from "./prompts.js"; @@ -17,7 +17,13 @@ import { import { isSuppressedByRejection } from "./state.js"; import { SURFACE_TARGET } from "./target.js"; import { emitProgress } from "./progress.js"; -import { measureWorkspace, prepareWorkspace, repoFingerprint, workspacePathFor } from "./workspace.js"; +import { + READ_ONLY_SEARCH_PATH, + measureWorkspace, + prepareWorkspace, + repoFingerprint, + workspacePathFor, +} from "./workspace.js"; import { UserError, color, info, warn } from "./logger.js"; /** @@ -149,7 +155,9 @@ function harnessCountsOf(transcripts) { * reaches that decision. An ordinary repository skill stays fingerprinted either way. A * fingerprinted path can still resolve outside the repository - that is the ordinary * user-scope layout - so a change there is reported for what it is rather than as a direct - * repository edit. + * repository edit. A configured `skillSearchPaths` root is the one withheld reason that + * stays fingerprinted anyway: its read-only promise is not "backpass will never write + * this," it is "nothing may ever write this," so a change there must still fail the run. */ function assertRepoUntouched(repo, before, workspaceRoot) { const after = repoFingerprint(repo, Object.keys(before)); @@ -224,6 +232,11 @@ function synthesisSetup({ memoryFile, summary, config, repo, harnessCounts, scop for (const w of overflow.warnings) warn(w); const skillDirs = resolveProjectSkillDirs(repo.root, overflow.dir, config.skillsDirs || [], { exact: userScope }); const skillFiles = loadProjectSkills(repo.root, overflow.dir, config.skillsDirs || [], { exact: userScope }); + // `skillSearchPaths` rides `skillDirs` for awareness (config.js), but staging must + // refuse it unconditionally - unlike the rest of `skillDirs`, it is never writable, in + // no scope, so `prepareWorkspace` needs the raw roots to enforce that independently of + // `allowExternal`. + const searchPathRoots = (config.skillSearchPaths || []).map((p) => expandHomePath(p)); // The budget is the whole always-loaded surface whatever the target: a skill target // moves it by that skill's description-line delta, nothing else changes. const descriptionTokens = skillDescriptionTokens(skillFiles); @@ -259,6 +272,7 @@ function synthesisSetup({ memoryFile, summary, config, repo, harnessCounts, scop overflow, skillDirs, skillFiles, + searchPathRoots, target, descriptionTokens, maxEdits, @@ -497,6 +511,7 @@ export async function synthesizeProposal({ overflow, skillDirs, skillFiles, + searchPathRoots, target, descriptionTokens, maxEdits, @@ -523,6 +538,7 @@ export async function synthesizeProposal({ skillDirs, stagedSkills, allowExternal: scope?.kind === "user", + searchPathRoots, }; let workspace = prepareWorkspace(workspaceOptions); const stagedSkillsDir = @@ -569,9 +585,19 @@ export async function synthesizeProposal({ const editPromptFile = path.join(promptDir, "synthesis-edit.md"); fs.writeFileSync(editPromptFile, renderPrompt("synthesis", editValues)); + // A search-path skill is unstageable like any other read-only skill, but unlike the + // rest of them the read-only promise it carries must be enforceable: a direct write to + // it has to be detected, not silently excused the way an ordinary withheld skill is + // (design note above `assertRepoUntouched`). It stays in the fingerprint so a change + // there still fails the run loudly. const fingerprint = repoFingerprint(repo, [ memoryFile.path, - ...skillFiles.filter((skill) => !readOnlyReason(skill.path)).map((skill) => skill.path), + ...skillFiles + .filter((skill) => { + const reason = readOnlyReason(skill.path); + return !reason || reason === READ_ONLY_SEARCH_PATH; + }) + .map((skill) => skill.path), ]); const sessionName = `backpass-synth-${process.pid}`; const timeoutSeconds = Math.max(config.timeoutSeconds, 900); diff --git a/src/target.js b/src/target.js index cfaef07..b3fe291 100644 --- a/src/target.js +++ b/src/target.js @@ -71,7 +71,10 @@ export function resolveTarget(spec, scope) { const skill = skillMatches[0]; // A targeted run writes exactly one file, and staging is what decides whether that // file can be in the copy at all. Ask it here so the refusal names its own cause. - const refusal = skillStagingRefusal(root, skill.path, { allowExternal: user }); + const refusal = skillStagingRefusal(root, skill.path, { + allowExternal: user, + searchPathRoots: scope.skillSearchPaths || [], + }); if (refusal) { throw new UserError( `--target ${spec} is at ${skill.path}, which ${refusal}`, diff --git a/src/workspace.js b/src/workspace.js index 3e859cf..1265b87 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -45,6 +45,7 @@ export function prepareWorkspace({ skillDirs = [skillsDir], stagedSkills = null, allowExternal = false, + searchPathRoots = [], }) { const root = workspaceRoot(state); fs.rmSync(root, { recursive: true, force: true }); @@ -64,6 +65,10 @@ export function prepareWorkspace({ // refusal there drops the whole round. Leaving it out of staging is what makes it // impossible for such a file to become an edit at all. const confineTo = confinementRoot(repo.root, allowExternal); + // A configured `skillSearchPaths` root is read-only in every scope, unlike the rest of + // `skillDirs` - `allowExternal` lets user scope write its own harness directories + // wherever they resolve, but must never reach a root the config promised to leave alone. + const searchPathIdentities = new Set(searchPathRoots.map(realPath).filter(Boolean)); const skillMappings = skillDirs.map((logical) => ({ logical, staged: workspacePathFor(logical), @@ -76,7 +81,7 @@ export function prepareWorkspace({ const confined = []; const toLogical = (relative) => path.isAbsolute(sourceDir) ? path.join(sourceDir, relative) : path.posix.join(sourceDir, relative); - for (const relative of walkFiles(skillsSource, "", confineTo, confined)) { + for (const relative of walkFiles(skillsSource, "", confineTo, confined, searchPathIdentities)) { const from = path.join(skillsSource, relative); const logical = toLogical(relative); const identity = realPath(from); @@ -86,7 +91,7 @@ export function prepareWorkspace({ // drops the round, so staging declares it read-only instead of offering the edit. // It is decided for every loaded skill, before a narrowed run drops the ones it does // not write, so "backpass will never write this file" means the same thing on both. - const refusal = stagingRefusal(from, confineTo); + const refusal = stagingRefusal(from, confineTo, searchPathIdentities); if (refusal) { unstageable.push({ path: logical, reason: refusal, identity }); continue; @@ -125,7 +130,7 @@ export function prepareWorkspace({ if (identity) stagedIdentities.set(identity, logical); stagedPaths.set(logical, staged); } - unstageable.push(...confined.map((relative) => ({ path: toLogical(relative), reason: READ_ONLY_OUTSIDE_REPO }))); + unstageable.push(...confined.map(({ relative, reason }) => ({ path: toLogical(relative), reason }))); } fs.mkdirSync(path.join(root, workspacePathFor(skillsDir)), { recursive: true }); @@ -139,6 +144,7 @@ export function prepareWorkspace({ stagedPaths, originals, confineTo, + searchPathIdentities, unstageable, stagedIdentities, }; @@ -183,7 +189,7 @@ function withinRoot(root, resolved) { return relative !== "" && !relative.startsWith("..") && !path.isAbsolute(relative); } -function walkFiles(dir, prefix = "", confineTo = null, confined = []) { +function walkFiles(dir, prefix = "", confineTo = null, confined = [], searchPathIdentities = null) { const out = []; let entries; try { @@ -192,11 +198,13 @@ function walkFiles(dir, prefix = "", confineTo = null, confined = []) { return out; } // One rule for taking a file, wherever the walk reaches it: a path that resolves - // outside the root is named for the caller instead of staged, so the containment - // invariant cannot hold on one branch and not its sibling. + // outside the root, or inside a configured `skillSearchPaths` root, is named for the + // caller instead of staged, so the containment invariant cannot hold on one branch and + // not its sibling. const take = (absolute, relativePath) => { - if (!confineTo || withinRoot(confineTo, realPath(absolute))) out.push(relativePath); - else confined.push(relativePath); + const reason = confinementReason(confineTo, searchPathIdentities, realPath(absolute)); + if (!reason) out.push(relativePath); + else confined.push({ relative: relativePath, reason }); }; for (const entry of entries.sort((a, b) => a.name.localeCompare(b.name))) { const relative = prefix ? path.posix.join(prefix, entry.name) : entry.name; @@ -210,8 +218,9 @@ function walkFiles(dir, prefix = "", confineTo = null, confined = []) { if (!identity) continue; // Pruned here, but named: the caller tells the model these are read-only rather // than letting a proposed edit to one be discarded without a reason. - if (!withinRoot(confineTo, identity)) { - confined.push(relative); + const reason = confinementReason(confineTo, searchPathIdentities, identity); + if (reason) { + confined.push({ relative, reason }); continue; } // A link may point at anything - in the layout that motivated following links at @@ -229,7 +238,7 @@ function walkFiles(dir, prefix = "", confineTo = null, confined = []) { if (prefix === "" && isFile(leaf)) take(leaf, path.posix.join(relative, SKILL_FILENAME)); continue; } - out.push(...walkFiles(child, relative, confineTo, confined)); + out.push(...walkFiles(child, relative, confineTo, confined, searchPathIdentities)); } else if (target === "file") { take(path.join(dir, entry.name), relative); } @@ -241,15 +250,40 @@ function walkFiles(dir, prefix = "", confineTo = null, confined = []) { const READ_ONLY_OUTSIDE_REPO = "resolves outside the repository"; const READ_ONLY_UNREADABLE = "could not be read when the staging copy was built"; const READ_ONLY_UNWRITABLE = "resolves to a location that cannot be written"; +/** A configured `skillSearchPaths` root: read-only in every scope, never subject to `allowExternal`. */ +export const READ_ONLY_SEARCH_PATH = "resolves inside a configured skillSearchPaths root"; /** The root a project-scope walk may not leave; user scope owns files anywhere. */ function confinementRoot(repoRoot, allowExternal) { return allowExternal ? null : realPath(repoRoot) || path.resolve(repoRoot); } +/** True when `identity` is one of `roots` or nested under one of them. */ +function isUnderAny(roots, identity) { + if (!identity) return false; + for (const root of roots) { + if (identity === root || identity.startsWith(`${root}${path.sep}`)) return true; + } + return false; +} + +/** + * Why confinement refuses `identity`, or null when it may be walked/staged. Checked + * before `withinRoot` so a configured `skillSearchPaths` root is named for what it is + * even when it happens to sit inside the confined root (or, in user scope, when nothing + * is confined at all) - `allowExternal` must never blur into this promise. + */ +function confinementReason(confineTo, searchPathIdentities, identity) { + if (searchPathIdentities && isUnderAny(searchPathIdentities, identity)) return READ_ONLY_SEARCH_PATH; + if (!withinRoot(confineTo, identity)) return READ_ONLY_OUTSIDE_REPO; + return null; +} + /** Why staging withholds a skill file from the copy, or null when it can stage it. */ -function stagingRefusal(absolute, confineTo) { - if (!withinRoot(confineTo, realPath(absolute))) return READ_ONLY_OUTSIDE_REPO; +function stagingRefusal(absolute, confineTo, searchPathIdentities = null) { + const identity = realPath(absolute); + const reason = confinementReason(confineTo, searchPathIdentities, identity); + if (reason) return reason; return readOnlyResolvedPath(absolute) ? READ_ONLY_UNWRITABLE : null; } @@ -258,9 +292,10 @@ function stagingRefusal(absolute, confineTo) { * the copy will not hold could never emit an edit for it, so it is refused by name here * rather than after a synthesis turn that was told the file is the one it may write. */ -export function skillStagingRefusal(repoRoot, skillPath, { allowExternal = false } = {}) { +export function skillStagingRefusal(repoRoot, skillPath, { allowExternal = false, searchPathRoots = [] } = {}) { const absolute = path.isAbsolute(skillPath) ? skillPath : path.join(repoRoot, skillPath); - return stagingRefusal(absolute, confinementRoot(repoRoot, allowExternal)); + const searchPathIdentities = new Set(searchPathRoots.map(realPath).filter(Boolean)); + return stagingRefusal(absolute, confinementRoot(repoRoot, allowExternal), searchPathIdentities); } /** Why measurement dropped a file the model wrote: the note the human reads must say which. */ @@ -269,6 +304,8 @@ export const STRAY_OUTSIDE_REPO = "it resolves outside the repository, which pro export const strayAliasReason = (owner) => `it is the same file already staged as ${owner}`; export const STRAY_UNWRITABLE = "it resolves to a location that cannot be written, so staging withheld it from the copy"; +export const STRAY_READ_ONLY_SEARCH_PATH = + "it resolves inside a configured skillSearchPaths root, which stays read-only in every scope"; /** A created file counts as a skill only in the layouts `loadSkills` reads. */ export function isSkillFilePath(relative, skillsDir) { @@ -426,6 +463,7 @@ export function measureWorkspace(workspace) { stagedPaths = new Map([...workspace.originals.keys()].map((file) => [file, workspacePathFor(file)])), originals, confineTo = null, + searchPathIdentities = new Set(), stagedIdentities = new Map(), unstageable = [], } = workspace; @@ -478,6 +516,14 @@ export function measureWorkspace(workspace) { stray.push({ file: logical, reason: STRAY_OUTSIDE_SURFACE }); continue; } + // A mapping rooted in a configured `skillSearchPaths` directory is read-only in every + // scope: `confineTo` is null in user scope, so this has to be checked independently of + // it, before a file the model wrote there could be mistaken for a created skill. + const mappingIdentity = mapping.source ? realPath(mapping.source) : null; + if (mappingIdentity && searchPathIdentities.has(mappingIdentity)) { + stray.push({ file: logical, reason: STRAY_READ_ONLY_SEARCH_PATH }); + continue; + } // Staging leaves out a skill that resolves outside the repository; measurement must // not carry one back in as a created file, which apply could never write either. // The gate is apply's own, so the two can never disagree about what is reachable. diff --git a/test/synthesize.test.js b/test/synthesize.test.js index 35485a4..d382e9d 100644 --- a/test/synthesize.test.js +++ b/test/synthesize.test.js @@ -739,6 +739,34 @@ test("a staged skill that resolves outside the repository is reported as such, n }); }); +test("a skill reached only through skillSearchPaths is never staged, and a direct write to it is still caught by the fingerprint", async () => { + const shared = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-search-path-"))); + fs.mkdirSync(path.join(shared, "db")); + fs.writeFileSync(path.join(shared, "db", "SKILL.md"), DB_SKILL); + + // Unlike an ordinary withheld skill, a `skillSearchPaths` root carries a promise that + // must be enforceable: a direct write to it has to fail the run loudly, never be + // excused the way a skill staging withheld for other reasons is. + const searched = setup({ edit: {} }, { overrides: { skillSearchPaths: [shared] } }); + const skillPath = path.join(shared, "db", "SKILL.md"); + fs.writeFileSync( + process.env.FAKE_ACPX_SCRIPT, + JSON.stringify({ + edit: { [skillPath]: { replace: [["Keep transactions short.", "Keep every transaction short."]] } }, + annotations: [{ reply: { edits: [] } }], + }), + ); + + await assert.rejects(searched.run(), (err) => { + assert.ok(err instanceof UserError); + assert.ok( + err.message.includes(`${skillPath} changed during synthesis; that path resolves outside the repository`), + err.message, + ); + return true; + }); +}); + test("a narrowed run does not fingerprint a skill linked into a store nothing may write", async () => { const store = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-narrowed-store-"))); fs.mkdirSync(path.join(store, "db")); diff --git a/test/workspace.test.js b/test/workspace.test.js index 240aa3b..a87171c 100644 --- a/test/workspace.test.js +++ b/test/workspace.test.js @@ -9,13 +9,16 @@ import { loadProjectSkills, loadSkills, resolveProjectSkillDirs, skillDescriptio import { estimateTokens } from "../src/tokens.js"; import { State } from "../src/state.js"; import { - isSkillFilePath, + READ_ONLY_SEARCH_PATH, STRAY_OUTSIDE_SURFACE, + STRAY_READ_ONLY_SEARCH_PATH, STRAY_UNWRITABLE, + isSkillFilePath, measureWorkspace, parseSkillFile, prepareWorkspace, repoFingerprint, + skillStagingRefusal, workspacePathFor, } from "../src/workspace.js"; import { makeRepo, stageAndMeasure, writeIn } from "./helpers/staging.js"; @@ -381,6 +384,74 @@ test("a skill in an outside search path is loaded for awareness but never staged ]); }); +test("a configured search path stays read-only even in user scope, unlike an ordinary external skills dir", () => { + const shared = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-search-path-"))); + fs.mkdirSync(path.join(shared, "db")); + fs.writeFileSync(path.join(shared, "db", "SKILL.md"), SKILL); + + const repo = makeRepo({ "AGENTS.md": AGENTS }); + const state = new State(repo.root).ensure(); + const memoryFile = readMemoryFile(repo.root, "AGENTS.md"); + const skillDirs = resolveProjectSkillDirs(repo.root, ".agents/skills", [shared]); + + // `allowExternal` is what lets user scope write its own harness directories wherever + // they resolve; it must never reach a root the config named in `skillSearchPaths`. + const workspace = prepareWorkspace({ + state, + repo, + memoryFile, + skillsDir: ".agents/skills", + skillDirs, + allowExternal: true, + searchPathRoots: [shared], + }); + assert.deepEqual([...workspace.originals.keys()], ["AGENTS.md"]); + assert.deepEqual(walkStaged(path.join(workspace.root, workspacePathFor(shared))), []); + assert.deepEqual(workspace.unstageable, [{ path: path.join(shared, "db"), reason: READ_ONLY_SEARCH_PATH }]); + + // `--target` asks this exact question before a run ever narrows to a name, and must + // get the same answer. + const refusal = skillStagingRefusal(repo.root, path.join(shared, "db", "SKILL.md"), { + allowExternal: true, + searchPathRoots: [shared], + }); + assert.equal(refusal, READ_ONLY_SEARCH_PATH); +}); + +test("a file created under a search-path root during synthesis is reported stray, never a created skill", () => { + const shared = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-search-path-new-"))); + + const repo = makeRepo({ "AGENTS.md": AGENTS }); + const state = new State(repo.root).ensure(); + const memoryFile = readMemoryFile(repo.root, "AGENTS.md"); + const skillDirs = resolveProjectSkillDirs(repo.root, ".agents/skills", [shared]); + + const workspace = prepareWorkspace({ + state, + repo, + memoryFile, + skillsDir: ".agents/skills", + skillDirs, + allowExternal: true, + searchPathRoots: [shared], + }); + // The library is empty, so there is no pre-existing file to refuse in advance - only + // the created-file path can catch a model that writes a brand new one under the root. + writeIn( + workspace.root, + `${workspacePathFor(shared)}/new/SKILL.md`, + "---\nname: new\ndescription: New.\n---\n\nBody\n", + ); + const measured = measureWorkspace(workspace); + assert.deepEqual( + measured.changes.filter((c) => c.kind === "created"), + [], + ); + assert.deepEqual(measured.stray, [ + { file: path.join(shared, "new", "SKILL.md"), reason: STRAY_READ_ONLY_SEARCH_PATH }, + ]); +}); + test("a skill file linked out of the repo through an in-repo library is never staged", () => { const outside = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-outside-store-"))); fs.writeFileSync(path.join(outside, "SKILL.md"), SKILL); From 583699ecaa4ea42bf3ab27e4d84ea362ea93145c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Obudzi=C5=84ski?= Date: Wed, 16 Sep 2026 17:39:15 +0200 Subject: [PATCH 3/6] harden(skills): make skillSearchPaths read-only guard fail closed and canonicalize consistently The read-only guard for skillSearchPaths roots could fail open in three ways: - the read-only identity set was built with fs.realpathSync over raw configured values, so a RELATIVE entry resolved against the process cwd while its real skill source resolved against the repo root - they never matched and the refusal never fired; - the --target refusal path (skillStagingRefusal) did not home-expand a ~-prefixed root, so realpath threw and the entry was dropped from the set; - any root that could not be canonicalized was silently discarded, deleting the promise instead of enforcing it. Fix with one shared canonicalizeSearchPathRoots helper: expand ~, resolve relative entries against the repo root (never cwd), then follow links; used by staging (prepareWorkspace) and the --target refusal (skillStagingRefusal), with the same set feeding the measurement fingerprint. Fail CLOSED - an existing root that cannot be canonicalized raises a clear UserError naming config.skillSearchPaths rather than being dropped (a not-yet-existing root keeps its resolved absolute identity, since nothing can load from or be written to it). Reject degenerate roots (empty string, filesystem root) at config validation. Regression tests: relative root resolved against repo root with cwd deliberately different; ~-prefixed root at the --target refusal site; unresolvable root failing closed; degenerate roots rejected at config validation. Full suite (680) green. Refs kunchenguid/backpass#32 --- src/config.js | 21 ++++++++++++ src/workspace.js | 44 +++++++++++++++++++++++-- test/config.test.js | 12 +++++++ test/workspace.test.js | 75 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 3 deletions(-) diff --git a/src/config.js b/src/config.js index cef6551..9f2deb8 100644 --- a/src/config.js +++ b/src/config.js @@ -151,6 +151,14 @@ export const USER_CONFIG_DEFAULTS = { }, }; +/** True for a filesystem root spelling: `/`, repeated separators, or a Windows volume root. */ +function isFilesystemRoot(p) { + const s = p.trim(); + if (/^\/+$/.test(s) || /^\\+$/.test(s)) return true; + if (/^[A-Za-z]:[/\\]*$/.test(s)) return true; + return false; +} + /** Expand a leading `~` to the home directory; other paths pass through unchanged. */ export function expandHomePath(p, home = os.homedir()) { if (typeof p !== "string") return p; @@ -268,6 +276,19 @@ function validate(config, { kind = "project" } = {}) { if (!Array.isArray(config.skillSearchPaths) || config.skillSearchPaths.some((d) => typeof d !== "string")) { throw new UserError("config.skillSearchPaths must be an array of paths"); } + for (const entry of config.skillSearchPaths) { + if (!entry.trim()) { + throw new UserError("config.skillSearchPaths entries must be non-empty path strings"); + } + // The filesystem root as a search path would mark every path read-only and leave + // nothing stageable - a degenerate value that must be rejected, not enforced. + if (isFilesystemRoot(entry)) { + throw new UserError( + `config.skillSearchPaths entry "${entry}" must not be the filesystem root`, + "name a specific shared skills directory", + ); + } + } } const includeProjects = config.discovery.includeProjects; const excludeProjects = config.discovery.excludeProjects; diff --git a/src/workspace.js b/src/workspace.js index 1265b87..03998e6 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -1,8 +1,10 @@ import fs from "node:fs"; +import os from "node:os"; import path from "node:path"; +import { expandHomePath } from "./config.js"; import { anchoredHunks, countOccurrences, span } from "./diff.js"; -import { warn } from "./logger.js"; +import { UserError, warn } from "./logger.js"; import { parseMemoryUnits, readOnlyResolvedPath, resolveMemoryPath } from "./memory.js"; import { isDirectoryEntry, parseFrontmatter, skillBody } from "./skills.js"; import { sha256 } from "./state.js"; @@ -68,7 +70,7 @@ export function prepareWorkspace({ // A configured `skillSearchPaths` root is read-only in every scope, unlike the rest of // `skillDirs` - `allowExternal` lets user scope write its own harness directories // wherever they resolve, but must never reach a root the config promised to leave alone. - const searchPathIdentities = new Set(searchPathRoots.map(realPath).filter(Boolean)); + const searchPathIdentities = canonicalizeSearchPathRoots(repo.root, searchPathRoots); const skillMappings = skillDirs.map((logical) => ({ logical, staged: workspacePathFor(logical), @@ -253,6 +255,42 @@ const READ_ONLY_UNWRITABLE = "resolves to a location that cannot be written"; /** A configured `skillSearchPaths` root: read-only in every scope, never subject to `allowExternal`. */ export const READ_ONLY_SEARCH_PATH = "resolves inside a configured skillSearchPaths root"; +/** + * Canonical identities of the configured `skillSearchPaths` roots, resolved EXACTLY the + * way a skill source is (`prepareWorkspace` builds a source with `path.join(repo.root, ...)` + * then compares `fs.realpathSync` identities): expand `~`, resolve a relative entry + * against the repository root - never the process working directory - then follow links. + * Building it any other way is how the read-only promise fails open: `fs.realpathSync` on a + * raw relative value resolves against `process.cwd()`, so it never matches the real source + * and the refusal never fires. + * + * Fail CLOSED: a configured root that exists but cannot be canonicalised raises a clear + * error naming `config.skillSearchPaths` rather than being dropped - silently discarding it + * would delete the promise instead of enforcing it. A not-yet-existing root (`ENOENT`) + * keeps its resolved absolute path as its identity: nothing can load from, or be written + * to, a directory that does not exist, so the promise stays whole either way. + */ +export function canonicalizeSearchPathRoots(repoRoot, roots = [], home = os.homedir()) { + const identities = new Set(); + for (const raw of roots) { + const expanded = expandHomePath(raw, home); + const absolute = path.isAbsolute(expanded) ? expanded : path.resolve(repoRoot, expanded); + try { + identities.add(fs.realpathSync(absolute)); + } catch (err) { + if (err && err.code === "ENOENT") { + identities.add(absolute); + continue; + } + throw new UserError( + `config.skillSearchPaths root "${raw}" cannot be resolved (${err.message})`, + "point it at a readable directory, or remove it from skillSearchPaths", + ); + } + } + return identities; +} + /** The root a project-scope walk may not leave; user scope owns files anywhere. */ function confinementRoot(repoRoot, allowExternal) { return allowExternal ? null : realPath(repoRoot) || path.resolve(repoRoot); @@ -294,7 +332,7 @@ function stagingRefusal(absolute, confineTo, searchPathIdentities = null) { */ export function skillStagingRefusal(repoRoot, skillPath, { allowExternal = false, searchPathRoots = [] } = {}) { const absolute = path.isAbsolute(skillPath) ? skillPath : path.join(repoRoot, skillPath); - const searchPathIdentities = new Set(searchPathRoots.map(realPath).filter(Boolean)); + const searchPathIdentities = canonicalizeSearchPathRoots(repoRoot, searchPathRoots); return stagingRefusal(absolute, confinementRoot(repoRoot, allowExternal), searchPathIdentities); } diff --git a/test/config.test.js b/test/config.test.js index ca09637..62f252c 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -91,6 +91,18 @@ test("skillSearchPaths defaults to none and rejects non-array-of-strings values" } }); +test("skillSearchPaths rejects degenerate roots: the empty string and the filesystem root", () => { + // An empty root, or the filesystem root, would mark everything read-only / nothing + // stageable - reject them loudly at config validation rather than enforcing them. + for (const bad of ["", " ", "/", "//", "\\", "C:\\", "C:/", "C:"]) { + assert.throws(() => loadConfig(tempRepo({ skillSearchPaths: ["~/.claude/skills", bad] })), UserError); + } + // A specific directory under the root is fine. + assert.deepEqual(loadConfig(tempRepo({ skillSearchPaths: ["/srv/shared-skills"] })).skillSearchPaths, [ + "/srv/shared-skills", + ]); +}); + test("skillSearchPaths expands ~ and feeds the read-only awareness list without touching skillsDir", () => { const home = os.homedir(); const config = loadConfig(tempRepo({ skillSearchPaths: ["~/.hermes/skills-shared", "~/.claude/skills"] })); diff --git a/test/workspace.test.js b/test/workspace.test.js index a87171c..5317680 100644 --- a/test/workspace.test.js +++ b/test/workspace.test.js @@ -13,6 +13,7 @@ import { STRAY_OUTSIDE_SURFACE, STRAY_READ_ONLY_SEARCH_PATH, STRAY_UNWRITABLE, + canonicalizeSearchPathRoots, isSkillFilePath, measureWorkspace, parseSkillFile, @@ -21,6 +22,7 @@ import { skillStagingRefusal, workspacePathFor, } from "../src/workspace.js"; +import { UserError } from "../src/logger.js"; import { makeRepo, stageAndMeasure, writeIn } from "./helpers/staging.js"; const AGENTS = "# M\n\n- one\n- two\n"; @@ -452,6 +454,79 @@ test("a file created under a search-path root during synthesis is reported stray ]); }); +test("a RELATIVE search-path root resolves against the repo root, not the process working directory", () => { + // The read-only promise fails open if a relative root is canonicalised against + // `process.cwd()` (the tests run with cwd = the backpass checkout, never the tmp repo), + // because the real skill source resolves against the repo root and the two never match. + const repo = makeRepo({ "AGENTS.md": AGENTS, "shared/db/SKILL.md": SKILL }); + assert.notEqual(fs.realpathSync(process.cwd()), fs.realpathSync(repo.root), "cwd must differ from the repo root"); + const state = new State(repo.root).ensure(); + const memoryFile = readMemoryFile(repo.root, "AGENTS.md"); + const skillDirs = resolveProjectSkillDirs(repo.root, ".agents/skills", ["shared"]); + + const workspace = prepareWorkspace({ + state, + repo, + memoryFile, + skillsDir: ".agents/skills", + skillDirs, + allowExternal: true, + searchPathRoots: ["shared"], + }); + assert.deepEqual(walkStaged(path.join(workspace.root, workspacePathFor("shared"))), []); + assert.deepEqual(workspace.unstageable, [{ path: "shared/db", reason: READ_ONLY_SEARCH_PATH }]); + + // The canonicalisation itself resolves a relative root against the repo root. + const identities = canonicalizeSearchPathRoots(repo.root, ["shared"]); + assert.ok(identities.has(fs.realpathSync(path.join(repo.root, "shared")))); +}); + +test("a ~-prefixed search-path root is home-expanded at the --target refusal site", () => { + const home = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-home-"))); + fs.mkdirSync(path.join(home, "shared", "db"), { recursive: true }); + fs.writeFileSync(path.join(home, "shared", "db", "SKILL.md"), SKILL); + const repo = makeRepo({ "AGENTS.md": AGENTS }); + const savedHome = process.env.HOME; + process.env.HOME = home; + try { + // The --target path asks `skillStagingRefusal` with the raw configured root; a + // "~"-prefixed root must be home-expanded there or realpath throws and it is dropped. + const refusal = skillStagingRefusal(repo.root, path.join(home, "shared", "db", "SKILL.md"), { + allowExternal: true, + searchPathRoots: ["~/shared"], + }); + assert.equal(refusal, READ_ONLY_SEARCH_PATH); + } finally { + if (savedHome === undefined) delete process.env.HOME; + else process.env.HOME = savedHome; + } +}); + +test("an unresolvable search-path root fails closed rather than being silently dropped", () => { + const repo = makeRepo({ "AGENTS.md": AGENTS, afile: "x" }); + const state = new State(repo.root).ensure(); + const memoryFile = readMemoryFile(repo.root, "AGENTS.md"); + // `afile` is a regular file, so `afile/sub` cannot be canonicalised (ENOTDIR). The + // read-only promise must abort loudly, naming the config key, not vanish. + const badRoot = path.join(repo.root, "afile", "sub"); + assert.throws( + () => canonicalizeSearchPathRoots(repo.root, [badRoot]), + (err) => err instanceof UserError && /skillSearchPaths/.test(err.message), + ); + assert.throws( + () => + prepareWorkspace({ + state, + repo, + memoryFile, + skillsDir: ".agents/skills", + skillDirs: [".agents/skills"], + searchPathRoots: [badRoot], + }), + UserError, + ); +}); + test("a skill file linked out of the repo through an in-repo library is never staged", () => { const outside = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "backpass-outside-store-"))); fs.writeFileSync(path.join(outside, "SKILL.md"), SKILL); From b16b4dbbd743fc58564f8c3d564e83b3c97069c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Obudzi=C5=84ski?= Date: Wed, 16 Sep 2026 18:01:23 +0200 Subject: [PATCH 4/6] no-mistakes(review): Fix skillSearchPaths ancestor-of-repo overlap and dedupe home-path helper --- src/config.js | 35 +++++++++++++++++++++++++++++++++-- src/scope.js | 9 ++------- src/workspace.js | 26 ++++++++++++++++++-------- test/config.test.js | 21 +++++++++++++++++++++ test/workspace.test.js | 30 ++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 17 deletions(-) diff --git a/src/config.js b/src/config.js index 9f2deb8..d81dd35 100644 --- a/src/config.js +++ b/src/config.js @@ -159,6 +159,11 @@ function isFilesystemRoot(p) { return false; } +/** True when `candidate` is `root`, or lies inside it. Both must already be resolved. */ +export function isAncestorOrEqual(root, candidate) { + return root === candidate || candidate.startsWith(`${root}${path.sep}`); +} + /** Expand a leading `~` to the home directory; other paths pass through unchanged. */ export function expandHomePath(p, home = os.homedir()) { if (typeof p !== "string") return p; @@ -247,7 +252,7 @@ export function sinceCutoff(since, now = Date.now()) { return window === null ? null : now - window; } -function validate(config, { kind = "project" } = {}) { +function validate(config, { kind = "project", repoRoot = null } = {}) { if (!Array.isArray(config.memoryFiles) || config.memoryFiles.length === 0) { throw new UserError("config.memoryFiles must be a non-empty array"); } @@ -288,6 +293,32 @@ function validate(config, { kind = "project" } = {}) { "name a specific shared skills directory", ); } + // A root that is the repo itself, or an ancestor of it (e.g. "~" with the repo + // checked out under $HOME), would mark every file the repo owns as "inside a + // search path" too, silently disabling writes to the repo's own configured skills + // directory. That is the same degenerate shape as the filesystem-root case above. + if (repoRoot) { + const expanded = expandHomePath(entry); + const absolute = path.isAbsolute(expanded) ? expanded : path.resolve(repoRoot, expanded); + let resolvedEntry; + try { + resolvedEntry = fs.realpathSync(absolute); + } catch { + resolvedEntry = absolute; + } + let resolvedRepoRoot; + try { + resolvedRepoRoot = fs.realpathSync(repoRoot); + } catch { + resolvedRepoRoot = path.resolve(repoRoot); + } + if (isAncestorOrEqual(resolvedEntry, resolvedRepoRoot)) { + throw new UserError( + `config.skillSearchPaths entry "${entry}" must not be the repository root, or an ancestor of it`, + "name a shared skills directory outside the repository", + ); + } + } } } const includeProjects = config.discovery.includeProjects; @@ -421,7 +452,7 @@ export function loadConfig(repoRoot, overrides = {}, { kind = "project" } = {}) if (config.discovery.includeCursorIde && !config.discovery.harnesses.includes("cursor-ide")) { config.discovery.harnesses = [...config.discovery.harnesses, "cursor-ide"]; } - const validated = validate(config, { kind: scopeKind }); + const validated = validate(config, { kind: scopeKind, repoRoot }); // `skillSearchPaths` is the read-side awareness key. It rides the existing `skillsDirs` // awareness list (consulted after `skillsDir` in list order) rather than a second plumbing; // `~` is expanded here so the loaders that join `repoRoot` never mishandle a home path. diff --git a/src/scope.js b/src/scope.js index 08e1e57..4ab9f1c 100644 --- a/src/scope.js +++ b/src/scope.js @@ -2,7 +2,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { parseScopeKind, userStateDir } from "./config.js"; +import { expandHomePath, parseScopeKind, userStateDir } from "./config.js"; import { associate as associateProject, associateRemote, globToRegExp } from "./discovery/association.js"; import { UserError, info } from "./logger.js"; import { gitProjectIdentity, gitToplevel, listWorktrees, normalizeRemote } from "./repo.js"; @@ -16,12 +16,7 @@ import { gitProjectIdentity, gitToplevel, listWorktrees, normalizeRemote } from * `~/.config/backpass/user/` (0700). A run is exactly one scope, chosen by `--scope`. */ -export function expandUserPath(p, home = os.homedir()) { - if (typeof p !== "string") return p; - if (p === "~") return home; - if (p.startsWith("~/")) return path.join(home, p.slice(2)); - return p; -} +export const expandUserPath = expandHomePath; /** * Path relative to `root` when it sits under it, otherwise the absolute path. diff --git a/src/workspace.js b/src/workspace.js index 03998e6..c07d972 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -2,7 +2,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { expandHomePath } from "./config.js"; +import { expandHomePath, isAncestorOrEqual } from "./config.js"; import { anchoredHunks, countOccurrences, span } from "./diff.js"; import { UserError, warn } from "./logger.js"; import { parseMemoryUnits, readOnlyResolvedPath, resolveMemoryPath } from "./memory.js"; @@ -269,24 +269,34 @@ export const READ_ONLY_SEARCH_PATH = "resolves inside a configured skillSearchPa * would delete the promise instead of enforcing it. A not-yet-existing root (`ENOENT`) * keeps its resolved absolute path as its identity: nothing can load from, or be written * to, a directory that does not exist, so the promise stays whole either way. + * + * A root that is the repository itself, or an ancestor of it, is dropped rather than + * registered: `config.js` validation rejects that shape at load time, but this function + * is also reachable directly (tests, future callers), and such a root would otherwise + * mark every file the repo owns as "inside a search path" - the repo's own containment + * must win for its own files. */ export function canonicalizeSearchPathRoots(repoRoot, roots = [], home = os.homedir()) { const identities = new Set(); + const repoIdentity = realPath(repoRoot) || path.resolve(repoRoot); for (const raw of roots) { const expanded = expandHomePath(raw, home); const absolute = path.isAbsolute(expanded) ? expanded : path.resolve(repoRoot, expanded); + let identity; try { - identities.add(fs.realpathSync(absolute)); + identity = fs.realpathSync(absolute); } catch (err) { if (err && err.code === "ENOENT") { - identities.add(absolute); - continue; + identity = absolute; + } else { + throw new UserError( + `config.skillSearchPaths root "${raw}" cannot be resolved (${err.message})`, + "point it at a readable directory, or remove it from skillSearchPaths", + ); } - throw new UserError( - `config.skillSearchPaths root "${raw}" cannot be resolved (${err.message})`, - "point it at a readable directory, or remove it from skillSearchPaths", - ); } + if (isAncestorOrEqual(identity, repoIdentity)) continue; + identities.add(identity); } return identities; } diff --git a/test/config.test.js b/test/config.test.js index 62f252c..2c79a01 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -103,6 +103,27 @@ test("skillSearchPaths rejects degenerate roots: the empty string and the filesy ]); }); +test("skillSearchPaths rejects a root that is the repo root, or an ancestor of it", () => { + // e.g. skillSearchPaths: ["~"] with the repo checked out under $HOME must never + // silently disable writes to the repo's own configured skills directory. + const parent = tempRepo(); + const nested = path.join(parent, "nested-repo"); + fs.mkdirSync(nested); + + fs.writeFileSync(path.join(nested, CONFIG_FILENAME), JSON.stringify({ skillSearchPaths: [parent] })); + assert.throws(() => loadConfig(nested), UserError, "an ancestor of the repo root is rejected"); + + fs.writeFileSync(path.join(nested, CONFIG_FILENAME), JSON.stringify({ skillSearchPaths: [nested] })); + assert.throws(() => loadConfig(nested), UserError, "the repo root itself is rejected"); + + // A directory nested INSIDE the repo is a different shape and stays accepted. + fs.writeFileSync( + path.join(nested, CONFIG_FILENAME), + JSON.stringify({ skillSearchPaths: [path.join(nested, "vendor")] }), + ); + assert.deepEqual(loadConfig(nested).skillSearchPaths, [path.join(nested, "vendor")]); +}); + test("skillSearchPaths expands ~ and feeds the read-only awareness list without touching skillsDir", () => { const home = os.homedir(); const config = loadConfig(tempRepo({ skillSearchPaths: ["~/.hermes/skills-shared", "~/.claude/skills"] })); diff --git a/test/workspace.test.js b/test/workspace.test.js index 5317680..abda75f 100644 --- a/test/workspace.test.js +++ b/test/workspace.test.js @@ -502,6 +502,36 @@ test("a ~-prefixed search-path root is home-expanded at the --target refusal sit } }); +test("a search-path root that is an ancestor of the repo never makes the repo's own skillsDir read-only", () => { + // config.js validation rejects this shape at load time, but `canonicalizeSearchPathRoots` + // and `prepareWorkspace` are reachable directly, so the repo's own containment must win + // here too - e.g. skillSearchPaths: ["~"] with the repo checked out under $HOME. + const repo = makeRepo({ "AGENTS.md": AGENTS, ".agents/skills/db/SKILL.md": SKILL }); + const ancestor = fs.realpathSync(os.tmpdir()); + assert.ok( + fs.realpathSync(repo.root).startsWith(`${ancestor}${path.sep}`), + "the repo must be nested under the ancestor root for this test to be meaningful", + ); + + assert.equal(canonicalizeSearchPathRoots(repo.root, [ancestor]).size, 0); + assert.equal(canonicalizeSearchPathRoots(repo.root, [repo.root]).size, 0); + + const state = new State(repo.root).ensure(); + const memoryFile = readMemoryFile(repo.root, "AGENTS.md"); + const skillDirs = resolveProjectSkillDirs(repo.root, ".agents/skills"); + + const workspace = prepareWorkspace({ + state, + repo, + memoryFile, + skillsDir: ".agents/skills", + skillDirs, + searchPathRoots: [ancestor], + }); + assert.deepEqual(workspace.unstageable, []); + assert.deepEqual(walkStaged(path.join(workspace.root, workspacePathFor(".agents/skills"))), ["db/SKILL.md"]); +}); + test("an unresolvable search-path root fails closed rather than being silently dropped", () => { const repo = makeRepo({ "AGENTS.md": AGENTS, afile: "x" }); const state = new State(repo.root).ensure(); From adc82431bdd30916dc759d3e83845fd3ebf6f272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Obudzi=C5=84ski?= Date: Wed, 16 Sep 2026 18:30:46 +0200 Subject: [PATCH 5/6] no-mistakes(document): docs(AGENTS): describe skillSearchPaths read-only enforcement across scopes --- AGENTS.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c04b243..7e309b4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -240,8 +240,15 @@ list` only sees this clone. `attachSiblingClones` in `src/repo.js` also searches (a shared/fleet library); `loadConfig` expands `~` and folds it into the existing `skillsDirs` awareness list (consulted after `skillsDir`), so every read site already sees it. Writes never target a search path: `resolveOverflowTarget` only returns `skillsDir`, - and a skill resolving outside the repo is withheld from the synthesis staging copy - (`src/workspace.js`). + and a skill resolving outside the repo is withheld from the synthesis staging copy in + every scope, including user scope where `allowExternal` would otherwise let staging reach + any external root (`confinementReason`/`canonicalizeSearchPathRoots` in `src/workspace.js`). + A search-path skill still rides the synthesis fingerprint unlike an ordinary withheld + skill, so a direct write to one fails the run instead of escaping detection + (`assertRepoUntouched` in `src/synthesize.js`). A configured root that equals or contains + the repo root or `skillsDir` is rejected at config validation and dropped by + `canonicalizeSearchPathRoots`, so the repo's own files can never be marked read-only by + their own search path. - **Memory resolution is pointer-aware** (`resolveMemoryFiles` in `src/memory.js`): the first configured file is canonical, a `@AGENTS.md`-only CLAUDE.md is a pointer, and a second full file is warned about, never silently ignored or double-written. From 48114794a86449aa1f5a8831a15edbdded19f3ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Obudzi=C5=84ski?= Date: Wed, 16 Sep 2026 18:54:53 +0200 Subject: [PATCH 6/6] no-mistakes(ci): Fixed ci-5: skillSearchPaths validation now rejects a root that equals or contains the repo's own skillsDir (e.g. ".agents" covering ".agents/skills"), not just the repo root. Root cause: config.js validate() only checked overlap against repoRoot; canonicalizeSearchPathRoots (the runtime drop-logic) lived in workspace.js and only took repoRoot too, so a skillsDir-containing search path passed config load and later made the repo's own skill files read-only during synthesis. Fix: moved canonicalizeSearchPathRoots into config.js (workspace.js now imports and re-exports it - single shared helper, no circular import since workspace.js already depended on config.js), extended its signature to take an optional skillsDir and drop overlapping roots against it too, and rewired validate() to call it instead of hand-rolling a second comparison. Updated prepareWorkspace/skillStagingRefusal/target.js call sites to pass skillsDir through so the runtime defense-in-depth also covers this case for direct callers. Added regression tests: config.test.js (load-time rejection for both the containing case ".agents"/".agents/skills" and the equal case) and workspace.test.js (canonicalizeSearchPathRoots + prepareWorkspace end-to-end, mirroring the existing repo-ancestor test). Verified: full `pnpm run check` (lint, format:check, typecheck, all 684 tests) passes --- src/config.js | 97 +++++++++++++++++++++++++++++++----------- src/target.js | 1 + src/workspace.js | 64 ++++++---------------------- test/config.test.js | 18 ++++++++ test/workspace.test.js | 25 +++++++++++ 5 files changed, 129 insertions(+), 76 deletions(-) diff --git a/src/config.js b/src/config.js index d81dd35..8537da1 100644 --- a/src/config.js +++ b/src/config.js @@ -172,6 +172,66 @@ export function expandHomePath(p, home = os.homedir()) { return p; } +/** + * Canonical identities of the configured `skillSearchPaths` roots, resolved EXACTLY the + * way a skill source is (`prepareWorkspace` builds a source with `path.join(repo.root, ...)` + * then compares `fs.realpathSync` identities): expand `~`, resolve a relative entry + * against the repository root - never the process working directory - then follow links. + * Building it any other way is how the read-only promise fails open: `fs.realpathSync` on a + * raw relative value resolves against `process.cwd()`, so it never matches the real source + * and the refusal never fires. + * + * Fail CLOSED: a configured root that exists but cannot be canonicalised raises a clear + * error naming `config.skillSearchPaths` rather than being dropped - silently discarding it + * would delete the promise instead of enforcing it. A not-yet-existing root (`ENOENT`) + * keeps its resolved absolute path as its identity: nothing can load from, or be written + * to, a directory that does not exist, so the promise stays whole either way. + * + * A root that is the repository itself, an ancestor of it (e.g. "~" with the repo checked + * out under $HOME), or that equals or contains the repo's own configured `skillsDir` (e.g. + * ".agents" covering ".agents/skills") is dropped rather than registered: `validate()` + * below rejects that shape at load time, but this function is also reachable directly + * (tests, future callers), and such a root would otherwise mark files backpass exists to + * write as "inside a search path" - the repo's own containment must win for its own files. + */ +export function canonicalizeSearchPathRoots(repoRoot, roots = [], skillsDir = null, home = os.homedir()) { + // Reference points (the repo root and its skillsDir) resolve leniently: any resolution + // failure falls back to the plain absolute path rather than aborting, since a not-yet- + // created skillsDir must still win its own containment check. + const resolveReference = (candidate) => { + const absolute = path.isAbsolute(candidate) ? candidate : path.resolve(repoRoot, candidate); + try { + return fs.realpathSync(absolute); + } catch { + return absolute; + } + }; + const identities = new Set(); + const repoIdentity = resolveReference(repoRoot); + const skillsIdentity = skillsDir ? resolveReference(skillsDir) : null; + for (const raw of roots) { + const expanded = expandHomePath(raw, home); + const absolute = path.isAbsolute(expanded) ? expanded : path.resolve(repoRoot, expanded); + let identity; + try { + identity = fs.realpathSync(absolute); + } catch (err) { + if (err && err.code === "ENOENT") { + identity = absolute; + } else { + throw new UserError( + `config.skillSearchPaths root "${raw}" cannot be resolved (${err.message})`, + "point it at a readable directory, or remove it from skillSearchPaths", + ); + } + } + if (isAncestorOrEqual(identity, repoIdentity)) continue; + if (skillsIdentity && isAncestorOrEqual(identity, skillsIdentity)) continue; + identities.add(identity); + } + return identities; +} + export function parseScopeKind(value) { if (value === undefined || value === null || value === "") return "project"; if (value === "project" || value === "user") return value; @@ -293,31 +353,18 @@ function validate(config, { kind = "project", repoRoot = null } = {}) { "name a specific shared skills directory", ); } - // A root that is the repo itself, or an ancestor of it (e.g. "~" with the repo - // checked out under $HOME), would mark every file the repo owns as "inside a - // search path" too, silently disabling writes to the repo's own configured skills - // directory. That is the same degenerate shape as the filesystem-root case above. - if (repoRoot) { - const expanded = expandHomePath(entry); - const absolute = path.isAbsolute(expanded) ? expanded : path.resolve(repoRoot, expanded); - let resolvedEntry; - try { - resolvedEntry = fs.realpathSync(absolute); - } catch { - resolvedEntry = absolute; - } - let resolvedRepoRoot; - try { - resolvedRepoRoot = fs.realpathSync(repoRoot); - } catch { - resolvedRepoRoot = path.resolve(repoRoot); - } - if (isAncestorOrEqual(resolvedEntry, resolvedRepoRoot)) { - throw new UserError( - `config.skillSearchPaths entry "${entry}" must not be the repository root, or an ancestor of it`, - "name a shared skills directory outside the repository", - ); - } + // A root that is the repo itself, an ancestor of it (e.g. "~" with the repo checked + // out under $HOME), or that equals or contains the repo's own configured skillsDir + // (e.g. ".agents" covering ".agents/skills") would mark files backpass exists to + // write as "inside a search path" too - the same degenerate shape as the + // filesystem-root case above. `canonicalizeSearchPathRoots` is the one place that + // decides this, both here (reject at load) and at runtime (drop for direct callers) - + // never duplicate the comparison. + if (repoRoot && canonicalizeSearchPathRoots(repoRoot, [entry], config.skillsDir).size === 0) { + throw new UserError( + `config.skillSearchPaths entry "${entry}" must not be the repository root or its skillsDir, or an ancestor of either`, + "name a shared skills directory outside the repository", + ); } } } diff --git a/src/target.js b/src/target.js index b3fe291..8b35d7e 100644 --- a/src/target.js +++ b/src/target.js @@ -74,6 +74,7 @@ export function resolveTarget(spec, scope) { const refusal = skillStagingRefusal(root, skill.path, { allowExternal: user, searchPathRoots: scope.skillSearchPaths || [], + skillsDir: scope.overflowDir, }); if (refusal) { throw new UserError( diff --git a/src/workspace.js b/src/workspace.js index c07d972..5537ee7 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -1,10 +1,9 @@ import fs from "node:fs"; -import os from "node:os"; import path from "node:path"; -import { expandHomePath, isAncestorOrEqual } from "./config.js"; +import { canonicalizeSearchPathRoots } from "./config.js"; import { anchoredHunks, countOccurrences, span } from "./diff.js"; -import { UserError, warn } from "./logger.js"; +import { warn } from "./logger.js"; import { parseMemoryUnits, readOnlyResolvedPath, resolveMemoryPath } from "./memory.js"; import { isDirectoryEntry, parseFrontmatter, skillBody } from "./skills.js"; import { sha256 } from "./state.js"; @@ -70,7 +69,7 @@ export function prepareWorkspace({ // A configured `skillSearchPaths` root is read-only in every scope, unlike the rest of // `skillDirs` - `allowExternal` lets user scope write its own harness directories // wherever they resolve, but must never reach a root the config promised to leave alone. - const searchPathIdentities = canonicalizeSearchPathRoots(repo.root, searchPathRoots); + const searchPathIdentities = canonicalizeSearchPathRoots(repo.root, searchPathRoots, skillsDir); const skillMappings = skillDirs.map((logical) => ({ logical, staged: workspacePathFor(logical), @@ -255,51 +254,10 @@ const READ_ONLY_UNWRITABLE = "resolves to a location that cannot be written"; /** A configured `skillSearchPaths` root: read-only in every scope, never subject to `allowExternal`. */ export const READ_ONLY_SEARCH_PATH = "resolves inside a configured skillSearchPaths root"; -/** - * Canonical identities of the configured `skillSearchPaths` roots, resolved EXACTLY the - * way a skill source is (`prepareWorkspace` builds a source with `path.join(repo.root, ...)` - * then compares `fs.realpathSync` identities): expand `~`, resolve a relative entry - * against the repository root - never the process working directory - then follow links. - * Building it any other way is how the read-only promise fails open: `fs.realpathSync` on a - * raw relative value resolves against `process.cwd()`, so it never matches the real source - * and the refusal never fires. - * - * Fail CLOSED: a configured root that exists but cannot be canonicalised raises a clear - * error naming `config.skillSearchPaths` rather than being dropped - silently discarding it - * would delete the promise instead of enforcing it. A not-yet-existing root (`ENOENT`) - * keeps its resolved absolute path as its identity: nothing can load from, or be written - * to, a directory that does not exist, so the promise stays whole either way. - * - * A root that is the repository itself, or an ancestor of it, is dropped rather than - * registered: `config.js` validation rejects that shape at load time, but this function - * is also reachable directly (tests, future callers), and such a root would otherwise - * mark every file the repo owns as "inside a search path" - the repo's own containment - * must win for its own files. - */ -export function canonicalizeSearchPathRoots(repoRoot, roots = [], home = os.homedir()) { - const identities = new Set(); - const repoIdentity = realPath(repoRoot) || path.resolve(repoRoot); - for (const raw of roots) { - const expanded = expandHomePath(raw, home); - const absolute = path.isAbsolute(expanded) ? expanded : path.resolve(repoRoot, expanded); - let identity; - try { - identity = fs.realpathSync(absolute); - } catch (err) { - if (err && err.code === "ENOENT") { - identity = absolute; - } else { - throw new UserError( - `config.skillSearchPaths root "${raw}" cannot be resolved (${err.message})`, - "point it at a readable directory, or remove it from skillSearchPaths", - ); - } - } - if (isAncestorOrEqual(identity, repoIdentity)) continue; - identities.add(identity); - } - return identities; -} +/** Re-exported so callers here need only import from one module; defined in `config.js` + * because `validate()` there must reuse it too (reject at load), not just this runtime + * (drop for direct callers). See its doc comment there for the full contract. */ +export { canonicalizeSearchPathRoots }; /** The root a project-scope walk may not leave; user scope owns files anywhere. */ function confinementRoot(repoRoot, allowExternal) { @@ -340,9 +298,13 @@ function stagingRefusal(absolute, confineTo, searchPathIdentities = null) { * the copy will not hold could never emit an edit for it, so it is refused by name here * rather than after a synthesis turn that was told the file is the one it may write. */ -export function skillStagingRefusal(repoRoot, skillPath, { allowExternal = false, searchPathRoots = [] } = {}) { +export function skillStagingRefusal( + repoRoot, + skillPath, + { allowExternal = false, searchPathRoots = [], skillsDir = null } = {}, +) { const absolute = path.isAbsolute(skillPath) ? skillPath : path.join(repoRoot, skillPath); - const searchPathIdentities = canonicalizeSearchPathRoots(repoRoot, searchPathRoots); + const searchPathIdentities = canonicalizeSearchPathRoots(repoRoot, searchPathRoots, skillsDir); return stagingRefusal(absolute, confinementRoot(repoRoot, allowExternal), searchPathIdentities); } diff --git a/test/config.test.js b/test/config.test.js index 2c79a01..30e4cef 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -124,6 +124,24 @@ test("skillSearchPaths rejects a root that is the repo root, or an ancestor of i assert.deepEqual(loadConfig(nested).skillSearchPaths, [path.join(nested, "vendor")]); }); +test("skillSearchPaths rejects a root that equals or contains the repo's own skillsDir", () => { + // skillsDir defaults to ".agents/skills". A search path of ".agents" contains it, and + // ".agents/skills" itself equals it - both would mark the repo's own write target + // read-only if they were not rejected the same way the repo-ancestor case already is. + const containing = tempRepo(); + fs.writeFileSync(path.join(containing, CONFIG_FILENAME), JSON.stringify({ skillSearchPaths: [".agents"] })); + assert.throws(() => loadConfig(containing), UserError, "a root containing skillsDir is rejected"); + + const equal = tempRepo(); + fs.writeFileSync(path.join(equal, CONFIG_FILENAME), JSON.stringify({ skillSearchPaths: [".agents/skills"] })); + assert.throws(() => loadConfig(equal), UserError, "a root equal to skillsDir is rejected"); + + // A sibling directory that does not overlap skillsDir stays accepted. + const sibling = tempRepo(); + fs.writeFileSync(path.join(sibling, CONFIG_FILENAME), JSON.stringify({ skillSearchPaths: ["shared"] })); + assert.deepEqual(loadConfig(sibling).skillSearchPaths, ["shared"]); +}); + test("skillSearchPaths expands ~ and feeds the read-only awareness list without touching skillsDir", () => { const home = os.homedir(); const config = loadConfig(tempRepo({ skillSearchPaths: ["~/.hermes/skills-shared", "~/.claude/skills"] })); diff --git a/test/workspace.test.js b/test/workspace.test.js index abda75f..9ab2f6e 100644 --- a/test/workspace.test.js +++ b/test/workspace.test.js @@ -532,6 +532,31 @@ test("a search-path root that is an ancestor of the repo never makes the repo's assert.deepEqual(walkStaged(path.join(workspace.root, workspacePathFor(".agents/skills"))), ["db/SKILL.md"]); }); +test("a search-path root that equals or contains the repo's own skillsDir never makes it read-only", () => { + // config.js validation rejects this shape at load time, but `canonicalizeSearchPathRoots` + // and `prepareWorkspace` are reachable directly, so the repo's own skillsDir containment + // must win here too - e.g. skillSearchPaths: [".agents"] with skillsDir ".agents/skills". + const repo = makeRepo({ "AGENTS.md": AGENTS, ".agents/skills/db/SKILL.md": SKILL }); + + assert.equal(canonicalizeSearchPathRoots(repo.root, [".agents"], ".agents/skills").size, 0, "containing case"); + assert.equal(canonicalizeSearchPathRoots(repo.root, [".agents/skills"], ".agents/skills").size, 0, "equal case"); + + const state = new State(repo.root).ensure(); + const memoryFile = readMemoryFile(repo.root, "AGENTS.md"); + const skillDirs = resolveProjectSkillDirs(repo.root, ".agents/skills"); + + const workspace = prepareWorkspace({ + state, + repo, + memoryFile, + skillsDir: ".agents/skills", + skillDirs, + searchPathRoots: [".agents"], + }); + assert.deepEqual(workspace.unstageable, []); + assert.deepEqual(walkStaged(path.join(workspace.root, workspacePathFor(".agents/skills"))), ["db/SKILL.md"]); +}); + test("an unresolvable search-path root fails closed rather than being silently dropped", () => { const repo = makeRepo({ "AGENTS.md": AGENTS, afile: "x" }); const state = new State(repo.root).ensure();