Skip to content

fix(skills): preserve real Claude skills directories - #136

Merged
kunchenguid merged 3 commits into
kunchenguid:mainfrom
Azd325:fix/skills-dir-guidance
Sep 18, 2026
Merged

kunchenguid merged 3 commits into
kunchenguid:mainfrom
Azd325:fix/skills-dir-guidance

Conversation

@Azd325

@Azd325 Azd325 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Intent

Fix skills-directory guidance so it is scope-aware instead of destructive: when a real .claude/skills directory already exists, backpass previously suggested replacing it with a symlink; now it leaves the real directory untouched and instead offers scope-aware alternatives (one-run --skills-dir, project config, or user config) so overflow skills land somewhere configured rather than clobbering existing content. This matches the already-promised non-clobber default (contract-class: restore, not a new always-on behavior). Follow-up commits addressed reviewer (Greptile) P2 findings: quote the --skills-dir path in the emitted warning so a CLAUDE_CONFIG_DIR containing spaces doesn't break the copy-pasted command, and added test coverage for space-containing paths.

What Changed

  • Preserve real .claude/skills directories and replace symlink-replacement guidance with scope-aware alternatives for configuring --skills-dir or skillsDir.
  • Quote one-run skills-directory paths in warnings and add coverage for paths containing spaces.

Risk Assessment

✅ Low: The change is narrowly scoped to non-destructive skills-directory guidance, shell quoting, documentation, and behavioral coverage; no source-verifiable material issues were found.

Testing

Drove the isolated CLI and actual skills write/resolution code, including preservation and space-containing guidance; no transient worktree artifacts remain. Full synthesis was untested because no live agent credential/evidence corpus was available.

  • Live validation: ✅ go - 3 of 4 scenarios driven live against the product
Scenario Result Live Evidence
Run backpass against a repository with a real .claude/skills directory ✅ pass live Live isolated backpass status --no-ui command and filesystem verification
Write a new skill while an existing skills directory is present ✅ pass live Executable product API check using resolveOverflowTarget and writeSkill
Use a skills path containing spaces and inspect emitted alternatives ✅ pass live Executable product API check with .claude config/skills
Run backpass propose end-to-end and observe the warning during synthesis ⏸️ untested no No configured synthesis-agent credential/evidence corpus was available in this isolated test environment; provide a logged-in supported agent and analyzed transcripts to drive this scenario.
Evidence: Live skills guidance verification
Isolated `backpass status` preserved the real `.claude/skills` directory and existing skill file. Executable skills-path check reported `canonicalWritten: true`, `realDirStillReal: true`, and `existingPreserved: true`.

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • Live validation: ✅ go - 3 of 4 scenarios driven live against the product
Scenario Result Live Evidence
Run backpass against a repository with a real .claude/skills directory ✅ pass live Live isolated backpass status --no-ui command and filesystem verification
Write a new skill while an existing skills directory is present ✅ pass live Executable product API check using resolveOverflowTarget and writeSkill
Use a skills path containing spaces and inspect emitted alternatives ✅ pass live Executable product API check with .claude config/skills
Run backpass propose end-to-end and observe the warning during synthesis ⏸️ untested no No configured synthesis-agent credential/evidence corpus was available in this isolated test environment; provide a logged-in supported agent and analyzed transcripts to drive this scenario.
  • node --test test/skills.test.js
  • Isolated backpass status --no-ui with a real .claude/skills directory
  • Executable resolveOverflowTarget/writeSkill check with a space-containing skills path
  • git status --short cleanup check
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@greptile-apps

greptile-apps Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, although the existing non-blocking Windows command-guidance issue remains unresolved.

Reviews (4) · Last reviewed commit: "no-mistakes(ci): Fixed shell quoting for..."

Comment thread src/skills.js Outdated
Comment thread src/skills.js
`merge it in and replace it with the symlink (ln -s ${target} ${claudeSkillsDir}).`
`left untouched. Backpass writes new skills to ${CANONICAL_SKILLS_DIR}, which Claude will not load ` +
`through ${claudeSkillsDir}. To write directly to the existing directory, use ` +
`--skills-dir ${shellQuote(claudeSkillsDir)} for one run, set "skillsDir": "${claudeSkillsDir}" in .backpassrc.json ` +

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Windows guidance uses POSIX quoting

The warning is also emitted on Windows, but it always wraps the --skills-dir value in POSIX single quotes. In cmd.exe, those quotes are treated as part of the path rather than grouping the argument, so copying the suggested command fails even for the default .claude/skills path. Please render the argument for the active platform or provide platform-specific guidance.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate.

Diff reviewed on ae6940b14dc5ad4cbf85541017dbb8b1f7774162. Fork PR from @Azd325. CI (ubuntu + macos) and Guard are green on this HEAD. Mergeable true; merge state CLEAN. Greptile Review SUCCESS with non-blocking P2 notes (cmd.exe POSIX quoting) — not a gate (repo does not use Greptile). No workflow-file changes; security clean (warning/docs/tests only).

Contract-class: restore. Unconfigured write path is unchanged: overflow still targets .agents/skills, and a real .claude/skills directory was already left untouched. This only replaces the destructive ln -s / "replace it with the symlink" guidance with scope-aware one-run / project / user skillsDir options plus manual conflict-checked merge — matching the already-promised non-clobber default. A bugfix claim does not reclassify this; the default path behavior is not a new always-on path.

VISION per-rule

  • Evidence is the only currency: aligns (no claim/quote path change).
  • The human owns the weights: aligns (explicit skillsDir / scope choice instead of destructive symlink).
  • Nothing the model says is taken on faith: aligns (no model-trust change).
  • The budget is the constraint: aligns (no budget change).
  • It reads what you already have and owns nothing: aligns (no new store/upload).
  • Failure is loud and named: aligns (warning names the gap and the commands that fix it; no quieter degraded path).
  • Scope: aligns (no user-level write from project; no rewrite offer).

Blocker (author, not captain): Require no-mistakes is not green for this HEAD binding. The latest body-compliance run (edited event 145) failed with Pipeline attestation head_sha does not match the current PR head (race while HEAD was still a981c35c). The PR body now has an attestation for ae6940b, but no successful no-mistakes run has validated that pairing. Please run git push no-mistakes (or otherwise re-trigger an opened/edited/reopened body-compliance check) so Require no-mistakes goes green on this HEAD. I will not merge until that check is green.

@Azd325

Azd325 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Require no-mistakes was rerun at the current PR head () and passed successfully: https://github.com/kunchenguid/backpass/actions/runs/35120660255

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate.

Re-triage after newer activity. Prior blocker cleared on old HEAD ae6940b (Require no-mistakes success 35120660255), but the branch was behind main (#137 / release). I updated this branch onto latest main (maintainerCanModify); new HEAD is e9111da97c88983d83512322baf76d8752e52823.

CI + Guard are green on the new HEAD (CI 35163585826, Guard 35163585825). Mergeable true; merge state CLEAN. Greptile SUCCESS — not a gate. No workflow-file changes; security clean (warning/docs/tests only). Diff intent unchanged vs prior review.

Contract-class: restore. Unconfigured write path unchanged: overflow still targets .agents/skills; a real .claude/skills directory stays untouched. Guidance only replaces destructive symlink language with scope-aware skillsDir options — already-promised non-clobber default.

VISION per-rule

  • Evidence is the only currency: aligns (no claim/quote path change).
  • The human owns the weights: aligns (explicit skillsDir / scope choice instead of destructive symlink).
  • Nothing the model says is taken on faith: aligns (no model-trust change).
  • The budget is the constraint: aligns (no budget change).
  • It reads what you already have and owns nothing: aligns (no new store/upload).
  • Failure is loud and named: aligns (warning names the gap and the fix commands; no quieter degraded path).
  • Scope: aligns (no user-level write from project; no rewrite offer).

Blocker (author, not captain): Require no-mistakes has no successful run bound to HEAD e9111da after the main update (attestation still targets the pre-update SHA). Please run git push no-mistakes so Require no-mistakes goes green on this HEAD. I will not merge until that check is green. No Firstmate captain-decision hold.

@Azd325
Azd325 force-pushed the fix/skills-dir-guidance branch from e9111da to 4d9cefb Compare September 17, 2026 20:44
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate.

Re-triage after newer activity (force-push / no-mistakes commits). New HEAD is 4d9cefbc51eaae0da1206f70f47380445f916aab.

CI + Guard are green on this HEAD (CI 35272674117, Guard 35272674200). Mergeable true; merge state CLEAN; not behind main. Greptile SUCCESS — not a gate. No workflow-file changes; security clean (warning/docs/tests only). Diff intent unchanged: non-destructive scope-aware guidance + shell quoting for paths with spaces.

Contract-class: restore. Unconfigured write path unchanged: overflow still targets .agents/skills; a real .claude/skills directory stays untouched. Guidance only replaces destructive symlink language with scope-aware skillsDir options — already-promised non-clobber default.

VISION per-rule

  • Evidence is the only currency: aligns (no claim/quote path change).
  • The human owns the weights: aligns (explicit skillsDir / scope choice instead of destructive symlink).
  • Nothing the model says is taken on faith: aligns (no model-trust change).
  • The budget is the constraint: aligns (no budget change).
  • It reads what you already have and owns nothing: aligns (no new store/upload).
  • Failure is loud and named: aligns (warning names the gap and the fix commands; no quieter degraded path).
  • Scope: aligns (no user-level write from project; no rewrite offer).

Blocker (author, not captain): Require no-mistakes has no successful run bound to HEAD 4d9cefb (queried Actions by exact SHA; no body-compliance / Require no-mistakes run; PR body attestation still targets ae6940b). Synchronize does not fire that check. Please run git push no-mistakes (or otherwise re-trigger an opened/edited/reopened body-compliance check) so Require no-mistakes goes green on this HEAD. I will not merge until that check is green. No Firstmate captain-decision hold.

@Azd325 Azd325 changed the title fix(skills): make .claude/skills guidance non-destructive fix(skills): clarify existing skills directory guidance Sep 18, 2026
@Azd325 Azd325 changed the title fix(skills): clarify existing skills directory guidance fix(skills): preserve real Claude skills directories Sep 18, 2026
@Azd325

Azd325 commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Require no-mistakes was rerun at the current PR head (4d9cefb) and passed successfully: https://github.com/kunchenguid/backpass/actions/runs/35383253468

@kunchenguid
kunchenguid merged commit 158dcd6 into kunchenguid:main Sep 18, 2026
8 checks passed
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: this is merged. Thank you @Azd325 — really appreciate you taking the time on this.

Re-triage after author note that Require no-mistakes passed on HEAD 4d9cefbc (35383253468). Verified live: CI + Guard green (CI 35272674117, Guard 35272674200); Require no-mistakes SUCCESS bound to this HEAD; mergeable CLEAN; ahead of main by 3 / behind 0; body attestation matches 4d9cefbc. No workflow-file changes; security clean (warning/docs/tests only). Greptile SUCCESS — not a gate.

Contract-class: restore. Unconfigured write path unchanged: overflow still targets .agents/skills; a real .claude/skills directory stays untouched. Guidance only replaces destructive symlink language with scope-aware skillsDir options — already-promised non-clobber default.

VISION per-rule

  • Evidence is the only currency: aligns (no claim/quote path change).
  • The human owns the weights: aligns (explicit skillsDir / scope choice instead of destructive symlink).
  • Nothing the model says is taken on faith: aligns (no model-trust change).
  • The budget is the constraint: aligns (no budget change).
  • It reads what you already have and owns nothing: aligns (no new store/upload).
  • Failure is loud and named: aligns (warning names the gap and the fix commands; no quieter degraded path).
  • Scope: aligns (no user-level write from project; no rewrite offer).

Squash-merged as restore + green CI including no-mistakes + safe review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants