Repository navigation
feat(skills): follow symlinked skill directories - #1003
vernonstinebaker wants to merge 2 commits into
Conversation
`nullclaw skills list` skipped any skills/ entry whose kind was a symlink, so a shared canonical skill repo linked into each workspace was invisible. Directory walks now follow a symlink when the target is a directory and skip broken or non-directory targets. Archive installs still reject symlink entries inside downloaded archives. Fixes nullclaw#995
DonPrus
left a comment
DonPrus
left a comment
There was a problem hiding this comment.
The top-level and nested directory discovery changes are useful, and the archive audit still rejects archive symlinks. I read the full diff, #995 discussion, CLI/agent loading and removal paths, and checked current main (clean merge). One related path needs to be made safe before approval:
[P1] Removing a skill discovered through a symlinked category deletes the canonical copy. With workspace/skills/category -> ../canonical and canonical/shared/SKILL.md, this PR discovers shared. removeSkill("shared") then calls deleteTreeAbsolute on workspace/skills/category/shared. That helper opens the parent directory, follows the category link and recursively deletes canonical/shared. This destroys the shared repository's skill, potentially affecting every workspace using it. A direct skill symlink has different unlink behavior and does not cover this case. Please track/reject removal through a symlink ancestor (or define a safe unlink-only operation) and cover both direct and category links, preserving the canonical target.
I reproduced the deletion in a temporary fixture: 194/195 targeted tests passed, with only the added canonical-preservation regression failing; no reported leaks. The probe was retained outside the PR branch. Real skills/configuration were protected by a filesystem sandbox.
Two documentation corrections would also be useful: loadSkill prefers SKILL.toml, then skill.json, then SKILL.md, whereas the new guide omits TOML and calls Markdown preferred; and the top-level skillforge config example is not parsed by Config or scheduled by the runtime (the library module exists, but those settings do not enable automatic discovery). Please label that library capability accurately rather than presenting the JSON as an active configuration recipe.
Leaving this without approval because the removal behavior can cause data loss.
`removeSkill` built `<workspace>/skills/<name>` and called `deleteTreeAbsolute` on it. When the skill was only reachable through a symlinked category -- `skills/category -> ../canonical`, with `canonical/shared/SKILL.md` -- the direct path missed, so removeSkill fell through to the `listSkills` result and deleted `skills/category/shared`. That path follows the category link, so the recursive delete destroyed `canonical/shared`: the copy every other workspace sharing that repo depends on. A leaf symlink behaved differently, which is why the direct case was not the reported symptom. Removal is now split by what the path actually is: - A symlinked *ancestor* between `skills/` and the target is refused with `error.SkillRemovalThroughSymlink`. There is no safe way to delete a file under a symlinked category, and silently skipping the request would leave the caller thinking it succeeded. - A *leaf* symlink is unlinked with `deleteFileAbsolute` rather than deleted recursively, so the skill leaves this workspace and the canonical target survives. - A plain directory is deleted as before. The ancestor walk checks one component at a time via directory iteration, reusing the same entry-kind check this branch already added for discovery, so a nested category is caught too. Tests: refusal through a symlinked category with the canonical copy asserted intact; unlink of a directly symlinked skill with the canonical copy asserted intact; and a control that a plain local skill still deletes. The first two skip on Windows, which needs privileges to create symlinks. Docs, both languages, per AGENTS.md 7.6: - loadSkill prefers SKILL.toml, then skill.json, then SKILL.md. The guide omitted TOML and called the Markdown form preferred. - The top-level `skillforge` block is not parsed by Config and nothing in the runtime schedules it. It is now labelled a library capability rather than an active configuration recipe.
|
All three findings addressed in The P1 — deletion through a symlinked categoryYour reproduction was correct and I confirmed the mechanism in the current head. Removal is now decided by what the path actually is:
On the refusal: I chose an explicit error rather than silently skipping. There is no safe way to delete a file under a symlinked category, and returning success would leave the caller believing the skill was removed when it still exists. If you'd rather it be a no-op, that's a one-line change. The ancestor walk checks one component at a time via directory iteration, reusing the entry-kind check this branch already added for discovery, so nested categories are caught too. Tests
The first two skip on Windows, which needs privileges to create symlinks. Documentation, both languages
Validation
NoteThis unblocks #1008, which deliberately deferred its skills page to this PR. |
This branch has not been deployed
2 participants
Summary
nullclaw skills listand the category scan now follow a symlink when it points at a skill directory, and skip broken or non-directory targets.Fixes #995
Test plan
zig build test --summary allpassed on push (pre-push hook)nullclaw skills listshows a skill directory that is a symlink to a realskill.json/SKILL.mdskills/is omitted and does not fail the listingMade with Cursor