fix(skills): safely repair nested self-links

[steipete]

Summary

Fix skill mirror synchronization creating nested ancestor loops when a Claude skill is a real directory and its Codex registration points back to that directory. Passing that real directory to ln -sfn creates a link inside it rather than replacing the destination; the prior top-level audit missed the resulting loop because it still resolved successfully.

• Preserve real destination files and directories, accepting same-directory local ownership and reporting other conflicts.
• Add an explicit, allowlisted --repair-nested-self-links mode with dry-run, whole-batch topology validation, per-unlink revalidation, and no ordinary sync/pruning/instruction-pointer work.
• Detect same-name nested self-links in the read-only fleet audit and document the distinction from broad repair.
• Add isolated HOME fixtures and run them in CI with the test-only ripgrep dependency installed explicitly.

Verification

• Full fixture suite passed on macOS Bash 3.2, including exact-scope preservation, invalid-batch refusal, redirected roots, partial failures, idempotence, and ordinary-sync nonrecurrence.
• The unchanged recurrence assertion fails against the original helper and passes against this patch.
• Shell syntax checks, skill validation, and diff whitespace checks passed.
• A scoped live repair removed only the six confirmed nested symlinks. Before/after snapshots preserved real directory identities, file contents, valid backlinks, unrelated registrations, and instruction pointers. Recursive discovery succeeded from both registries with unchanged skill-path sets; a second repair was a no-op.

The repair is intentionally limited to the documented nested-self-link topology, not a general symlink graph repair. Filesystem rechecks detect drift but are not an atomic guarantee against concurrent writers.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed August 30, 2026, 5:44 PM ET / 21:44 UTC.

ClawSweeper review

What this changes

The PR preserves locally owned skill directories during normal synchronization, adds an allowlisted repair for nested self-links, detects the loop in the fleet audit, and adds fixture tests and CI coverage.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

Keep this owner-authored PR open for normal merge review. The current main baseline still uses the unsafe directory-targeting link operation, while this branch adds a narrowly validated repair and regression coverage.

Priority: P2
Reviewed head: 424d22cf0f7162cf8907c62f3038a27b8753c729

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused repair with strong source-level safety checks and substantial fixture coverage; the deliberate compatibility change is documented and needs normal merge judgment.
Proof confidence	🌊 off-meta tidepool	Not applicable: This owner-authored PR is exempt from the external-contributor proof gate; its supplied description nevertheless records a scoped live repair and before/after preservation checks for the changed filesystem path.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: This owner-authored PR is exempt from the external-contributor proof gate; its supplied description nevertheless records a scoped live repair and before/after preservation checks for the changed filesystem path.
Evidence reviewed	7 items	Current-main gap: The fetched main baseline still calls the link helper's direct ln -sfn operation without a real-destination guard, so it does not contain the central fix. Normal-sync safeguard: The branch preserves real destination objects, accepts a directory only when it is already the intended directory, and otherwise records a conflict instead of invoking ln -sfn on it. Scoped repair boundary: The repair mode requires an explicit allowlist, validates roots and the exact literal backlink topology, rechecks each candidate, and unlinks only the nested leaf.
Findings	None	None.
Security	None	None.

How this fits together

The skill mirror synchronizer projects local skill directories into Claude and Codex discovery roots. Its audit reports drift, while the new scoped repair removes only a verified nested symlink loop without rebuilding the broader mirror.

flowchart LR
  A[Local skill directories] --> B[Skill mirror synchronizer]
  B --> C[Claude skill registry]
  B --> D[Codex skill registry]
  C --> E[Fleet link audit]
  E --> F[Scoped nested-link repair]
  F --> C

Loading

Before merge

• Resolve merge risk (P1) - Normal synchronization will now exit nonzero when it encounters a real destination conflict, including a real instruction pointer; this intentionally preserves user data but may require operator action on existing machines.
• Complete next step (P2) - No discrete repair finding was identified; this owner-authored PR should proceed through normal merge review.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Change composition	production +129, tests +425, docs +24, CI +8	The substantial fixture suite exercises the safety-sensitive filesystem change more extensively than the production implementation grows.

Merge-risk options

Maintainer options:

1. Accept the explicit conflict failure (recommended)
   Land the documented behavior that preserves real destinations and requires operators to resolve incompatible existing objects rather than silently replacing them.
2. Preserve warning-only instruction pointers
   Narrow the changed exit behavior if normal sync must continue succeeding when only a real global instruction file is present.

Technical review

Best possible solution:

Retain the fail-safe conflict behavior and the narrow allowlisted repair, with the documented nonzero conflict outcome as the upgrade contract.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: the fetched main helper directly invokes ln -sfn on destination paths, and the supplied fixture describes the resulting nested-directory case. The review did not execute the fixture because the checkout must remain read-only.

Is this the best way to solve the issue?

Yes. Separating an explicitly allowlisted, topology-validated leaf cleanup from ordinary synchronization is a narrower and safer repair than broad symlink-graph rewriting.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against efa3b2f0f2b4.

Labels

Label changes:

• add P2: The fix affects local skill discovery and repair behavior with a bounded user and operator blast radius.
• add merge-risk: 🚨 compatibility: Existing setups with real destination objects will now receive a deliberate nonzero conflict result instead of continuing after a warning or replacement attempt.
• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This owner-authored PR is exempt from the external-contributor proof gate; its supplied description nevertheless records a scoped live repair and before/after preservation checks for the changed filesystem path.

Label justifications:

• P2: The fix affects local skill discovery and repair behavior with a bounded user and operator blast radius.
• merge-risk: 🚨 compatibility: Existing setups with real destination objects will now receive a deliberate nonzero conflict result instead of continuing after a warning or replacement attempt.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This owner-authored PR is exempt from the external-contributor proof gate; its supplied description nevertheless records a scoped live repair and before/after preservation checks for the changed filesystem path.

Evidence

What I checked:

• Current-main gap: The fetched main baseline still calls the link helper's direct ln -sfn operation without a real-destination guard, so it does not contain the central fix. (scripts/sync-skills:23, efa3b2f0f2b4)
• Normal-sync safeguard: The branch preserves real destination objects, accepts a directory only when it is already the intended directory, and otherwise records a conflict instead of invoking ln -sfn on it. (scripts/sync-skills:139, 424d22cf0f71)
• Scoped repair boundary: The repair mode requires an explicit allowlist, validates roots and the exact literal backlink topology, rechecks each candidate, and unlinks only the nested leaf. (scripts/sync-skills:49, 424d22cf0f71)
• Regression coverage: The disposable-HOME fixture suite covers recurrence against the old helper, whole-batch refusal, scoped preservation, drift checks, partial failure handling, and ordinary synchronization conflicts. (scripts/test-sync-skills:119, 424d22cf0f71)
• Feature provenance: The original sync helper and its direct ln -sfn line date to the initial skill-mirror commit; the current line is unchanged from that commit. (scripts/sync-skills:146, 590e75daa12d)
• Introduced-change identity: The verified test merge has the fetched main as first parent and this exact PR head as second parent; its delta contains the synchronizer, audit, tests, docs, changelog, and CI changes. (scripts/sync-skills:49, 0e62108b79fc)

Likely related people:

• Peter Steinberger: Raw commit 590e75d adds scripts/sync-skills:23 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 590e75daa12d; files: scripts/sync-skills)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.