feat(config): add skillSearchPaths for read-only awareness of shared skill libraries - #138
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // 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]; |
There was a problem hiding this comment.
Appending skillSearchPaths to skillsDirs removes the distinction between read-only search paths and writable skill roots. In user scope, synthesis allows external roots, so a writable shared-library skill can be staged, measured, and later updated by the apply writer. A backpass --scope user run can therefore modify a search path even though the accepted intent requires these paths to remain read-only and all writes to target only skillsDir. Preserve the read-only provenance of search paths through staging and application.
Context Used: If there is a VISION.md file at the root of the repo, the PR must not conflict / diverge / drift from it. If the PR description has an "Intent" section, respect that as the accepted user intent. - Do make comments if anything in the implementation ... (source)
Knowledge Base Used: Workspace state and configuration
| // 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]; |
There was a problem hiding this comment.
In project scope, shared skills are withheld from staging, but their absolute source paths are still shown to the synthesis agent for grounding and excluded from the repository fingerprint. If the synthesis harness edits one of those paths directly, the shared library changes without assertRepoUntouched detecting it. This violates the accepted read-only intent and the repository directive that model behavior must be enforced in code rather than trusted to prompt compliance. Include these exposed files in change detection or provide their contents without exposing writable source paths.
Context Used: If there is a VISION.md file at the root of the repo, the PR must not conflict / diverge / drift from it. If the PR description has an "Intent" section, respect that as the accepted user intent. - Do make comments if anything in the implementation ... (source)
Knowledge Base Used:
…chenguid#138 (skillSearchPaths read-only awareness): - ci-4/ci-5 (src/config.js:408, "search paths become writable" / "external writes escape detection"): `skillSearchPaths` were merged into `skillsDirs`, which `prepareWorkspace` also uses to decide what's staged/writable. In user scope `allowExternal` disables all confinement, so a configured search-path root was staged and writable there, and even where staging correctly withheld it (project scope), it was excluded from the repo fingerprint like any other withheld skill, so a direct write to it would silently escape detection. Fixed by threading the raw `skillSearchPaths` roots (`searchPathRoots`) into `prepareWorkspace`, `skillStagingRefusal` (used by `--target`), and `measureWorkspace`, adding an unconditional `READ_ONLY_SEARCH_PATH` refusal that applies regardless of `allowExternal`/confinement, and keeping search-path skills in the synthesis fingerprint (unlike ordinary withheld skills) so a direct write to one now aborts the run loudly. Touched: src/workspace.js, src/synthesize.js, src/scope.js, src/target.js. - ci-6 (README.md:737, "trigger tuning is impossible"): corrected the doc claim that a failed trigger tunes a shared skill's description — a search-path skill is read-only in every scope, so it's now documented as "reported as already covered, not tuned." ci-1/ci-2/ci-3 were left untouched per instructions (GitHub holding fork workflow runs for maintainer approval; not a code defect). Added regression tests (test/workspace.test.js, test/synthesize.test.js) covering: a search path staying read-only in user scope unlike an ordinary external skills dir, a newly-created file under a search-path root being reported `stray` rather than a measurable `created` change, and an end-to-end synthesis run rejecting loudly when the harness writes directly to a search-path skill. Verified: lint, format check, typecheck, and the full test suite (676 tests) all pass
…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#32
…chenguid#138 (skillSearchPaths read-only awareness): - ci-4/ci-5 (src/config.js:408, "search paths become writable" / "external writes escape detection"): `skillSearchPaths` were merged into `skillsDirs`, which `prepareWorkspace` also uses to decide what's staged/writable. In user scope `allowExternal` disables all confinement, so a configured search-path root was staged and writable there, and even where staging correctly withheld it (project scope), it was excluded from the repo fingerprint like any other withheld skill, so a direct write to it would silently escape detection. Fixed by threading the raw `skillSearchPaths` roots (`searchPathRoots`) into `prepareWorkspace`, `skillStagingRefusal` (used by `--target`), and `measureWorkspace`, adding an unconditional `READ_ONLY_SEARCH_PATH` refusal that applies regardless of `allowExternal`/confinement, and keeping search-path skills in the synthesis fingerprint (unlike ordinary withheld skills) so a direct write to one now aborts the run loudly. Touched: src/workspace.js, src/synthesize.js, src/scope.js, src/target.js. - ci-6 (README.md:737, "trigger tuning is impossible"): corrected the doc claim that a failed trigger tunes a shared skill's description — a search-path skill is read-only in every scope, so it's now documented as "reported as already covered, not tuned." ci-1/ci-2/ci-3 were left untouched per instructions (GitHub holding fork workflow runs for maintainer approval; not a code defect). Added regression tests (test/workspace.test.js, test/synthesize.test.js) covering: a search path staying read-only in user scope unlike an ordinary external skills dir, a newly-created file under a search-path root being reported `stray` rather than a measurable `created` change, and an end-to-end synthesis run rejecting loudly when the harness writes directly to a search-path skill. Verified: lint, format check, typecheck, and the full test suite (676 tests) all pass
… 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#32
…d dedupe home-path helper
|
Speaking as Kun's firstmate. Diff reviewed on Contract-class: opt-in. Default is VISION per-rule
Blocker (author, not captain): Require no-mistakes is not green for this HEAD binding. Opened body-compliance succeeded on earlier head |
…nly enforcement across scopes
27600bd to
adc8243
Compare
…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
|
Speaking as Kun's firstmate. Re-triaged on new HEAD Fork workflows approved after security-clean review of tip vs main: CI 35124922668 and Guard 35124922671. Both are green (ubuntu + macos build-and-test; Generated files must not be hand-edited). Greptile Review pass — not a gate. No workflow-file changes; security clean. Mergeable; behind main by 1 ( Contract-class: opt-in. Tip still defaults VISION per-rule
Blocker (author, not captain): Body attestation already matches this HEAD ( |
|
Re-triggered as requested — the body edit fired an It is waiting on maintainer approval ( |
|
Speaking as Kun's firstmate: this is merged. Thank you @BartekObudzinski — really appreciate you taking the time on this. Approved fork Require no-mistakes 35572243013 on tip |
2 participants
Intent
Work upstream backpass issue #32 (#32): make backpass aware of skill libraries that live outside the repository, read-only, so it stops proposing duplicate skills.
The reported failure: a repo whose canonical skill library is a shared tree living outside the repo - a machine-wide shared skills directory, or ~/.claude/skills - ran a backward pass. Several of its AGENTS.md lines already pointed at a skill that does exist, in that shared tree, and every harness on that machine loads skills from there, so the pointers were resolving fine. backpass looked only inside the repo-local configured skills directory, concluded the pointer dangled, and proposed creating that skill again inside the repo. Applying that edit would fork one skill into two copies that drift - the exact rot the tool exists to prevent. The same blind spot skews the budget math for the other extraction edits: extraction is the right call, but the right target is sometimes "point at the existing shared skill" (zero new content) rather than "create a repo-local one".
The ask is a configuration key listing additional directories to consider, alongside the existing skillsDir, for example: {"skillsDir": ".agents/skills", "skillSearchPaths": ["
/.hermes/skills-shared", "/.claude/skills"]}.Semantics are read-only awareness. Skills found under the extra paths count as existing for: resolving skill references made from AGENTS.md - such a reference is not dangling; the existing "failed trigger leads to a description edit, not duplicate content" rule; dedup - an extraction whose content substantially matches an existing shared skill becomes a pointer edit instead of new content.
Writes still go only to the repo's own configured skills directory. backpass must never write into a shared library it does not own.
What Changed
skillSearchPathsconfig key (src/config.js) that expands~, validates entries, and folds them into the existingskillsDirsawareness list so skill references, failed-trigger checks, and dedup treat matching external skills as existing rather than dangling.src/workspace.js,src/synthesize.js,src/scope.js, andsrc/target.js: staging withholds them from writes in every scope (including user-scopeallowExternal), a search-path root that overlaps or contains the repo/skillsDiris rejected at config validation, and a search-path skill still rides the synthesis fingerprint so a direct write to one aborts the run instead of escaping detection.AGENTS.mdandREADME.mdto document the new key and its read-only semantics, and added/extended tests intest/config.test.js,test/skills.test.js,test/synthesize.test.js, andtest/workspace.test.jscovering validation, staging refusal, and the fingerprint/write-refusal path.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Risk Assessment
✅ Low: The fix round is a small, well-contained correction that resolves both round-1 findings correctly (ancestor/equal search-path roots are now rejected at config validation and defensively dropped in canonicalizeSearchPathRoots; expandUserPath is a clean re-export with identical behavior), verified with real-behavior tests, and does not touch the write/apply boundary or expand scope.
Testing
Built real sandbox git repo under fake $HOME and drove actual backpass CLI (node bin/backpass.js) live: reproduced reported regression (skillSearchPaths: ["~"] with repo under $HOME) and confirmed config validation now fails with clear error; confirmed repo's own skill stays writable --target with search paths configured; confirmed external shared skill counted read-only in status, both scopes; confirmed --target against skillSearchPaths skill in user scope refused by name (READ_ONLY_SEARCH_PATH). One scenario, a harness writing directly into a skillSearchPaths skill during full synthesis run caught by repo fingerprint, was never driven against the live product: only the mocked-acpx unit test (test/synthesize.test.js) ran, no real/fake AI harness end-to-end pass. That scenario is marked untested rather than pass.
Evidence: Live CLI: ancestor-of-repo skillSearchPaths ("~" with repo under $HOME) rejected at config load
Evidence: Live CLI: repo's own skill stays a valid writable --target with skillSearchPaths configured
Evidence: Live CLI: external shared skill counted read-only in status (project + user scope)
Evidence: Live CLI: --target on a skillSearchPaths skill in USER scope (allowExternal=true) is refused, never staged
Evidence: Targeted test run: fingerprint catches a direct write into a skillSearchPaths skill during synthesis
Evidence: Targeted test run: ancestor-overlap regression + dedupe-helper coverage
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
src/config.js:285- config.skillSearchPaths validation (isFilesystemRoot, src/config.js:154-160,275-292) only rejects the literal OS filesystem root, not a root that is an ancestor of (or identical to) the repository root or the configured skillsDir. In src/workspace.js confinementReason() (line314-318), the skillSearchPaths check runs BEFORE the repo-containment check, so if a search path is an ancestor of the repo (e.g. skillSearchPaths: [""] on a repo checked out under $HOME, a very common layout), every skill under the repo's own skillsDir also resolves 'inside' that search-path root and gets marked READ_ONLY_SEARCH_PATH. No error is raised; skill writing is silently disabled repo-wide even though the promise this change adds is specifically to leave the repo's own skillsDir writable. This directly contradicts the invariant the added isFilesystemRoot comment states ('a degenerate value that must be rejected, not enforced'), which only covers the bare '/'/drive-root spelling, not an ancestor-of-repo spelling. Fix: extend validation (or canonicalizeSearchPathRoots) to also reject a configured root that equals or contains the repo root / configured skillsDir.src/config.js:163- expandHomePath (src/config.js:163-168), added by this change, is byte-for-byte identical logic to the pre-existing expandUserPath in src/scope.js:19-24. scope.js already imports from config.js, so scope.js's expandUserPath could delegate to (or re-export) config.js's expandHomePath instead of duplicating the same 4-line implementation.🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Test** - 1 issue found → no changes applied ✅
backpass statusin a fresh temp repo with.backpassrc.jsonskillSearchPaths=["/"] exits 1:config.skillSearchPaths entry "/" must not be the filesystem rootbackpass statuswith skillSearchPaths=[".."] and ["."] both exit 1 withmust not be the repository root, or an ancestor of it. Confirmed as a genuine regression fix by running the identical repo/c…backpass statuswith skillSearchPaths pointing at an external /tmp shared dir containing db-helper/SKILL.md, plus a repo-local .agents/skills/local-only/SKILL.md, reports `overflow: 2 skill(s) in .a…backpass propose --target db-helperexits 1 immediately: `--target db-helper is at .../db-helper/SKILL.md, which resolves inside a configured skillSearchPaths root ... backpass loads and bills that…--scope userrun against a synthetic $HOME with harness-loaded skills directories, so no live result was est…backpass statuswith skillSearchPaths=["/"] in a temp repobackpass statuswith skillSearchPaths=[".."] and ["."] in a temp repo, plus the same repo/config against pre-fix commit 583699e for contrastbackpass statuswith a valid external skillSearchPaths root containing a real SKILL.md, alongside a repo-local skillbackpass propose --target db-helper(shared search-path skill) and--target local-only(repo skill) in the temp repo🔧 No changes applied.
✅ Re-checked - no issues remain.
node bin/backpass.js status --json (ancestor-of-repo skillSearchPaths rejection, live)node bin/backpass.js analyze --target own-skill --json (repo skill stays writable, live)node bin/backpass.js status --json / --scope user (external shared skill counted read-only, live, both scopes)node bin/backpass.js analyze --scope user --target db-tips --json (search-path skill refused as write target in user scope, live)node --test test/synthesize.test.js (fingerprint catches direct write to a skillSearchPaths skill)node --test test/workspace.test.js test/config.test.js test/skills.test.js (ancestor-overlap + dedupe-helper regressions)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Re-trigger note (2026-09-21)
This edit exists only to fire an
editedevent so thatRequire no-mistakesbody-compliance runs against the current head48114794…; that workflow fires onopened/edited/reopenedand never onsynchronize, so no compliance run has ever existed for this head. Requested in the triage comment above. Nothing in the intent, change description, risk assessment, testing table, or pipeline attestation blocks was modified.