feat(skill):assets-script-support#591
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds shared filesystem copying and hashing, stages plugin and skill installations, preserves complete skill trees with assets, hardens path handling, and changes skill consumers to use formatted skill output. ChangesFilesystem copy and installation
Skill assets and formatted output
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SkillSource
participant SkillInstaller
participant fscopy
participant InstalledSkill
SkillSource->>SkillInstaller: provide fetched skill tree
SkillInstaller->>fscopy: CopyTree(sourceDir, stagingDir)
SkillInstaller->>fscopy: HashTree(stagingDir)
SkillInstaller->>InstalledSkill: swap staging directory into place
SkillInstaller-->>InstalledSkill: return installed directory path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/skills/install_test.go (1)
320-323: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the copied script keeps its executable bit.
This test would still pass if
CopyTreecopiedinstall.shas non-executable, breaking script usability. Check the mode too.Proposed test addition
- if _, err := os.Stat(filepath.Join(destDir, "remote", "install.sh")); err != nil { + info, err := os.Stat(filepath.Join(destDir, "remote", "install.sh")) + if err != nil { t.Fatalf("install must copy fetched scripts into the skills dir: %v", err) } + if info.Mode().Perm()&0o111 == 0 { + t.Fatalf("install must preserve executable permissions for fetched scripts: mode=%v", info.Mode().Perm()) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/skills/install_test.go` around lines 320 - 323, The install test around CopyTree only checks that remote/install.sh exists, but it does not verify the copied file remains executable. Update the assertion in install_test.go to also inspect the file mode for the copied install.sh and confirm the executable bit is preserved, using the existing destDir/remote/install.sh path check in the test.internal/skills/skills_test.go (2)
432-455: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTruncation test should assert the output stays within
maxSkillOutputSize.The test verifies the truncation note suffix and UTF-8 validity but doesn't check that
len(out) <= maxSkillOutputSize. A regression that accidentally exceeds the cap would go undetected.✅ Proposed addition
if !utf8.ValidString(out) { t.Fatalf("FormatOutput produced invalid UTF-8 after truncation (split rune)") } + if len(out) > maxSkillOutputSize { + t.Fatalf("output exceeds maxSkillOutputSize: got %d bytes, cap %d", len(out), maxSkillOutputSize) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/skills/skills_test.go` around lines 432 - 455, The truncation test in TestFormatOutputTruncatesOnRuneBoundary only checks the suffix and UTF-8 validity, so add an assertion that FormatOutput’s result length stays within maxSkillOutputSize. Update the test to verify len(out) <= maxSkillOutputSize alongside the existing checks, using the existing FormatOutput and maxSkillOutputSize symbols to keep the cap enforcement covered.
320-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the
maxAssetSizeskip behavior.Files larger than 1 MB are silently skipped by
loadAssets, but no test exercises this boundary. A regression that accidentally includes oversized files (e.g., large binaries) would go undetected.✅ Proposed test
func TestLoadSkipsAssetsLargerThanMaxSize(t *testing.T) { dir := t.TempDir() writeSkillWithAssets(t, dir, "s", "---\nname: s\n---\nbody", map[string]string{ "big.bin": strings.Repeat("x", maxAssetSize+1), "ok.txt": "small\n", }, ) loaded, err := Load(dir) if err != nil { t.Fatalf("Load: %v", err) } if len(loaded) != 1 { t.Fatalf("expected 1 skill, got %d", len(loaded)) } for _, a := range loaded[0].Assets { if a.Name == "big.bin" { t.Errorf("oversized asset should be skipped, got %+v", a) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/skills/skills_test.go` around lines 320 - 367, Add a test that exercises the maxAssetSize skip behavior in loadAssets/Load by creating one oversized asset and one normal asset with writeSkillWithAssets, then asserting the oversized file is omitted from Skill.Assets while the smaller file is still present. Use the existing Load and skill asset collection checks from TestLoadDiscoversAssetsRecursively as the reference point, and make sure the new test names the oversized file clearly so the regression is easy to spot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/fscopy/fscopy.go`:
- Around line 128-130: The tree-hash stream in fscopy.go is not self-delimiting
because the current header in the tree-walk logic only tags path and executable
state, allowing different file trees to serialize to the same bytes. Update the
hashing stream to include explicit file size and type metadata before each
file’s content in the code path that builds the header for each entry, and keep
the boundary encoding unambiguous in the tree-hash routine. Add a regression
test covering the collision case described in the review comment so the
tree-hash implementation rejects that ambiguity going forward.
- Around line 36-50: CopyTree currently checks entries with os.Lstat but
CopyFile later reopens srcPath with os.Open, which can race with a symlink swap
and bypass the earlier check. Update the CopyFile path (and its call from
CopyTree) to verify the opened file still matches the originally inspected
inode, using a no-follow open strategy or a post-open stat comparison such as
os.SameFile, and fail closed if the source changed after the symlink check.
In `@internal/skills/install.go`:
- Around line 144-152: The install flow in install.go removes the existing skill
before the new tree is copied, which can leave no installed skill if copy fails.
Update the install logic around the target replacement path so the full copy is
written to a temporary directory first using the existing copy helper, then
atomically swap it into place only after success, with rollback/cleanup on
failure. Keep the current install result and locking behavior intact, and use
the existing symbols RemoveAll, fscopy.CopyTree, and the install target
replacement code to locate and refactor this sequence.
In `@internal/skills/skills.go`:
- Around line 72-76: In FormatOutput, the fallback from asset.Name to asset.Path
can expose absolute, symlink-resolved paths in model output for manually
constructed Skill values. Update the asset formatting logic in the loop over
skill.Assets to avoid using asset.Path as a fallback and instead use a safe,
non-sensitive placeholder or relative identifier when asset.Name is empty,
keeping the output free of home-directory paths.
---
Nitpick comments:
In `@internal/skills/install_test.go`:
- Around line 320-323: The install test around CopyTree only checks that
remote/install.sh exists, but it does not verify the copied file remains
executable. Update the assertion in install_test.go to also inspect the file
mode for the copied install.sh and confirm the executable bit is preserved,
using the existing destDir/remote/install.sh path check in the test.
In `@internal/skills/skills_test.go`:
- Around line 432-455: The truncation test in
TestFormatOutputTruncatesOnRuneBoundary only checks the suffix and UTF-8
validity, so add an assertion that FormatOutput’s result length stays within
maxSkillOutputSize. Update the test to verify len(out) <= maxSkillOutputSize
alongside the existing checks, using the existing FormatOutput and
maxSkillOutputSize symbols to keep the cap enforcement covered.
- Around line 320-367: Add a test that exercises the maxAssetSize skip behavior
in loadAssets/Load by creating one oversized asset and one normal asset with
writeSkillWithAssets, then asserting the oversized file is omitted from
Skill.Assets while the smaller file is still present. Use the existing Load and
skill asset collection checks from TestLoadDiscoversAssetsRecursively as the
reference point, and make sure the new test names the oversized file clearly so
the regression is easy to spot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 598a5835-83ab-456a-b8d2-269c054568a1
📒 Files selected for processing (9)
internal/fscopy/fscopy.gointernal/plugins/activate.gointernal/plugins/install.gointernal/skills/install.gointernal/skills/install_test.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/tools/skill.gointernal/tui/skill_commands.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
I checked out the branch — go build ./..., go vet, and the skills/plugins tests all pass. The shared fscopy package is a nice move: defining the symlink/.git/non-regular-file filtering once for both the skill and plugin installers is the right shape, and the feature itself (shipping scripts/assets alongside SKILL.md, copying data only, never executing) is sound. Three things I'd want fixed before this merges, all in the new copy path:
-
TOCTOU in CopyFile (internal/fscopy/fscopy.go:50).
CopyTreefilters withos.Lstatand rejects symlinks, butCopyFilethen reopens withos.Open, which follows symlinks. Between theLstatand theOpen, a regular file can be swapped for a symlink and the read follows it out of the install dir — exactly the escape the symlink check exists to prevent. Since skills install from a fetched, untrusted source, open withO_NOFOLLOW(or open-then-stat andos.SameFileagainst theLstatresult, failing closed on mismatch). The window is small but the threat model is real and the fix is cheap. -
RemoveAll before copy in Install (internal/skills/install.go:152).
os.RemoveAll(target)runs beforeCopyTree, so a copy failure (disk full, permission) leaves the user with no installed skill while the lockfile still points at the removed dir — a broken, stale install. Copy into a temp dir underdirand swap on success, with rollback of the temp on failure, so a failed upgrade never deletes the working skill. -
Home-path leak in FormatOutput (internal/skills/skills.go:73-75). The
asset.Pathfallback is the absolute, symlink-resolved install path. The comment above explicitly promises the absolute path is never sent to the model, and this fallback breaks that invariant for a manually-constructedSkillwith an emptyName. Usefilepath.Base(asset.Path)(or a placeholder) so the fallback can't emit the home directory.
The hash self-delimiting concern from the earlier review is already resolved in the current code (the header now tags type + size), so that one's good.
One coverage gap: internal/fscopy ships with no tests. For a package whose whole job is security-sensitive filtering (reject symlinks, skip .git, skip FIFOs/devices, self-delimiting hash), I'd add at least CopyTree-rejects-symlink, CopyTree-skips-.git, and a HashTree collision test before relying on it from two installers.
Process note: the linked issue #584 is still open without the issue-approved label, and the PR checklist leaves that box unchecked. Per the contribution policy that's a kevin/owner call to confirm — flagging it, not granting it from review. Requesting changes for the three issues above; the issue-approval step is the other open gate.
fix(tui): update picker_test for switchProviderModel 4-value return (Gitlawb#589) tea.Cmd) and updated every call site except picker_test.go:771, which still assigned three values. That broke the internal/tui test build and turned Smoke red on main. Add the missing receiver here. fix: make HashTree self-delimiting and fix skills format-output test - fscopy: tag dir/file type and byte size in HashTree so the stream is self-delimiting and two different trees cannot hash identically. - skills: match the %q-quoted skills root in the format-output test so it no longer fails on Windows path escaping. fix: make HashTree self-delimiting and fix skills format-output test │ ✓ exec_command │ - fscopy: tag dir/file type and byte size in HashTree so the stream is │ self-delimiting and two different trees cannot hash identically. │ - skills: match the %q-quoted skills root in the format-output test so it │ no longer fails on Windows path escaping.
67bcf5a to
3278775
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/fscopy/fscopy.go`:
- Around line 94-96: The hashing logic in CopyFile should use the same file
state that the copy operation installs, rather than relying on pre-open Lstat
metadata and only the executable bit. Update the code around CopyFile and the
hash/header generation path to derive the hash inputs from the opened file
descriptor and the final post-Chmod mode, so permission-only changes are
detected consistently and the header matches the bytes actually read. Also avoid
path-swapping issues by hashing from the already-open file handle instead of
reusing metadata captured before the file is opened.
In `@internal/fscopy/open_nounix.go`:
- Around line 13-32: The openRegularRead and openRegularWrite helpers still have
a TOCTOU gap because they do Lstat and then call os.Open/os.OpenFile, so a
swapped-in symlink can still be followed. Fix this by making the non-unix path
fail closed: either use a true no-follow open mechanism if available, or return
an unsupported/error path instead of relying on Lstat before open. Keep the
changes localized to openRegularRead and openRegularWrite so CopyFile and
HashTree continue to use the hardened behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33def50b-f0dd-4129-8418-ea14dadb61e3
📒 Files selected for processing (3)
internal/fscopy/fscopy.gointernal/fscopy/open_nounix.gointernal/fscopy/open_unix.go
✅ Files skipped from review due to trivial changes (1)
- internal/fscopy/open_unix.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
I checked out feat/skill-assets-support, merged origin/main, and ran the full pipeline locally:
make lint✅go test -race -count=1 ./...✅- no merge conflicts with
main
I also verified that the linked issue #584 still does not carry the issue-approved label. Per CONTRIBUTING.md: "A PR must link a parent issue that already carries the issue-approved label. PRs without that label are closed without review." That process gate needs to be resolved before merge.
Beyond the process gate, I agree with @Vasanthdev2004's requested changes and can confirm they are still present in the current branch:
-
Non-atomic install replacement —
internal/skills/install.go:148andinternal/plugins/install.go:134bothos.RemoveAll(target)beforefscopy.CopyTree. A copy failure after the removal leaves the user with no installed skill/plugin while the lockfile still points at the removed directory. This needs to copy into a temp directory underdirand swap on success, rolling back on failure. -
Absolute-path leak in
FormatOutput—internal/skills/skills.go:72-75still falls back toasset.Pathwhenasset.Nameis empty.asset.Pathis the absolute, symlink-resolved install path. For a manually constructedSkillthis breaks the invariant that absolute home paths are never sent to the model. Usefilepath.Base(asset.Path)or a placeholder instead. -
TOCTOU on Windows —
internal/fscopy/open_nounix.go:13-33usesos.Lstatbeforeos.Open/os.OpenFilebecauseO_NOFOLLOWis unavailable. That leaves a TOCTOU window on the non-unix path where a regular file can be swapped for a symlink between the check and the open. Since skills install from fetched, untrusted sources, this should fail closed: open the descriptor first, then verify it is still the same file withos.SameFileor equivalent, and reject symlinks.
Additional finding:
internal/fscopyships with no tests — the package exists solely to centralize security-sensitive copy/hash behavior (symlink rejection,.gitskipping, non-regular-file skipping, deterministic hashing). I see? github.com/Gitlawb/zero/internal/fscopy [no test files]in the test output. Add at minimum a symlink-rejection test, a.git/hidden-file skip test, a non-regular-file skip test, and a HashTree collision/regression test before relying on it from two installers.
Non-blocking suggestions (already raised by the bot; worth addressing while the PR is in flight):
- Assert
len(out) <= maxSkillOutputSizeinTestFormatOutputTruncatesOnRuneBoundary. - Add a test that files larger than
maxAssetSizeare silently skipped byloadAssets. - Assert the copied script keeps its executable bit in
internal/skills/install_test.go.
Verdict: Request changes. The feature shape is sound, but the install atomicity and path-leak issues are safety-relevant and need to be fixed before merge. The issue-approved label also needs to be applied to #584 per project policy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/plugins/install.go (1)
175-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the swap logic into
fscopyto avoid duplication.The swap-into-place pattern (stat target → rename to backup → rename staging to target → rollback on failure) is nearly identical between
copyAndSwapIntoPlacehere andswapDirIntoPlaceininternal/skills/install.go. Both implement the same atomic-rename-with-rollback logic with only cosmetic differences (error messages, staging cleanup location).Consider extracting a shared
fscopy.SwapDir(staging, target string) errorfunction to eliminate the duplication and ensure both install paths benefit from future fixes to the swap logic.This is deferred-friendly — both implementations are correct independently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/plugins/install.go` around lines 175 - 219, The swap-into-place flow in copyAndSwapIntoPlace duplicates the same atomic rename with rollback logic already used by swapDirIntoPlace, so extract that shared behavior into fscopy as a reusable SwapDir helper. Update copyAndSwapIntoPlace to call the shared helper after CopyTree succeeds, keeping only the staging creation/copy and any plugin-specific cleanup or error wrapping there. Ensure the new helper owns the target stat, backup rename, swap, and rollback steps so both install paths stay consistent and future fixes apply in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/skills/install.go`:
- Around line 144-165: Create the staging directory in the skills root itself,
not in filepath.Dir(dir), because swapDirIntoPlace uses os.Rename and will hit
EXDEV when dir is a mount point. Update the staging setup in install.go so the
temp dir is created under dir (matching the plugin install pattern in
copyAndSwapIntoPlace), and keep the existing copy-then-swap flow intact. Verify
the target, backup, and staging paths all remain on the same filesystem for both
fresh installs and replacements.
---
Nitpick comments:
In `@internal/plugins/install.go`:
- Around line 175-219: The swap-into-place flow in copyAndSwapIntoPlace
duplicates the same atomic rename with rollback logic already used by
swapDirIntoPlace, so extract that shared behavior into fscopy as a reusable
SwapDir helper. Update copyAndSwapIntoPlace to call the shared helper after
CopyTree succeeds, keeping only the staging creation/copy and any
plugin-specific cleanup or error wrapping there. Ensure the new helper owns the
target stat, backup rename, swap, and rollback steps so both install paths stay
consistent and future fixes apply in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ef3a44e-4139-4360-b5c7-acc67e5de2f4
📒 Files selected for processing (9)
internal/fscopy/fscopy.gointernal/fscopy/fscopy_test.gointernal/fscopy/open_nounix.gointernal/fscopy/open_windows.gointernal/plugins/install.gointernal/plugins/install_test.gointernal/skills/install.gointernal/skills/install_test.gointernal/skills/skills.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/fscopy/open_nounix.go
- internal/fscopy/fscopy.go
- internal/skills/skills.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Complete the resolved CodeRabbit no-follow request on non-Unix targets
internal/fscopy/open_nounix.go:46
The fallback checks the destination withLstat, then callsos.OpenFilewithO_TRUNC, and only checks the path again afterward. A source that swaps in a symlink in that interval can make the installer truncate the symlink target before the second check reports an error (and can swap it back to evade that check). The analogous read path has the same final-component race. This is the still-valid issue from CodeRabbit's resolved review thread; please use a true no-follow primitive on these targets or fail closed rather than opening a path that may now be a symlink. -
[P2] Preserve truncated skill content and its closing framing
internal/skills/skills.go:94
For an oversized one-line skill body, the backward newline search finds the newline immediately after the opening<skill>tag, so the returned output is effectively just that tag plus(output truncated): none of the instructions survive, and neither</skill>nor</skill_assets>is emitted despite the documented guarantee. This now breaks the core tool, plugin skill tool, and direct TUI invocation for valid large single-line Markdown/code skills. Reserve space for the note and closing tags while retaining a rune-safe content prefix; only line-align where it does not erase the body, and cover this case in the truncation test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/skills/skills.go`:
- Around line 107-131: Truncation in the skill output builder can cut through
the assets section and duplicate the body’s closing tag. Update the truncation
logic around the output construction to identify the end of the body before
<skill_assets>, clamp cut to that boundary, and omit assets when truncation
occurs so the result contains exactly one closing tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20698fa4-a741-429d-8641-d8558b2ffa9f
📒 Files selected for processing (4)
internal/fscopy/open_nounix.gointernal/fscopy/open_nounix_test.gointernal/skills/skills.gointernal/skills/skills_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/skills/skills_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make the Windows copy path usable
internal/fscopy/open_windows.go:24-38
openRegularWritepassesCREATE_ALWAYStogether withFILE_FLAG_OPEN_REPARSE_POINT. Windows documents that those flags cannot be combined, so every regular-file copy fails before either installer can finish on Windows. This makes installing any nonempty skill or plugin fail on that platform. Use a supported no-follow create/open sequence and verify the opened object before writing. -
[P1] Reject a symlinked
SKILL.mdbefore reporting success
internal/skills/install.go:114-160
Manifest validation follows aSKILL.mdsymlink, but the newfscopy.HashTreeandCopyTreedeliberately skip symlinks. A source whose manifest links to valid content therefore passes validation, is swapped in, and gets a lock entry/result, while the installed directory has noSKILL.mdand cannot be loaded. Require the source manifest to be a regular non-symlink file (or validate the staged copy) before committing the install. -
[P1] Do not follow a source directory that changes into a symlink during traversal
internal/fscopy/fscopy.go:36-52
CopyTreeclassifies a directory withLstatand then reopens that path recursively withReadDir;HashTreerepeats the same pattern at118-153.O_NOFOLLOWonly protects the final file component, so a concurrently writable local source can replace the checked directory with a symlink and make the recursion copy/hash readable files outside the source tree. Traverse through no-follow directory descriptors (or snapshot/revalidate the whole tree) rather than reopening descendants by pathname. -
[P2] Record the hash of the tree that is actually installed
internal/skills/install.go:125-168
The full source tree is hashed before it is copied to staging. If a local source changes afterHashTreereturns (including a same-size rewrite), the staged tree can differ from bothskills.lockand the successful result'sHash; the new verifiable-install/update contract is then false. Hash the completed staging tree before the swap, or otherwise copy from an immutable snapshot. -
[P2] Keep
FormatOutputwithin its advertised hard limit on every overflow path
internal/skills/skills.go:66-104
The assets-overflow branch returnsbody + "\n(output truncated)"without reserving bytes for the note, so a body near 100 KiB exceedsmaxSkillOutputSizeprecisely when assets are omitted. An oversized frontmatternamecan also make the opening tag alone exceed the cap because the body-truncation path floors the cut atcontentStart. These strings feed both the skill tool and TUI prompt. Reserve room for the framing/note or omit the note when the complete body already consumes the cap. -
[P2] Bound asset discovery before building the model-facing asset list
internal/skills/skills.go:245-305
The new recursive loader appends and sorts every eligible sub-1 MiB file, andFormatOutputrenders the entire list before checking its 100 KiB output cap. A fetched skill with a very large file tree can therefore consume unbounded memory/CPU or stall every skill load despite the output-size safeguard. Apply a count/metadata budget during traversal and stop rendering once that budget is reached.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Obtain approval for the linked issue before accepting this external contribution
CONTRIBUTING.md:16-24
The author is aCONTRIBUTOR, and the linked issue #584 currently has only theenhancementlabel—not the requiredissue-approvedlabel. The contribution policy says community PRs opened before that approval are closed without review, and the PR checklist leaves this requirement unchecked. Obtain core-team approval on #584 before this implementation can proceed. -
[P1] Keep installer staging directories out of the live discovery roots
internal/skills/install.go:131,internal/plugins/install.go:183
Both installers create the staging directory directly under the root that their loaders scan, but neitherskills.loadnorplugins.Loadignores.zero-*-install-*directories. While a copy is in progress, another Zero process can discover the partial staged manifest; for skills its dot-prefixed directory sorts before the real install and can win duplicate-name resolution, while plugins can load and activate an unvalidated, incompletely copied tree. Stage outside the scanned root or explicitly exclude installer staging/backup directories from discovery. -
[P1] Validate and hash the staged plugin tree, not the mutable source
internal/plugins/install.go:99-155
Plugin installation parses and hashespluginDir, then separately copies that source into staging and records the earlier hash. A mutable local source can change its manifest or executable files between those operations, producing installed bytes that do not matchplugins.lockor the returned result; a symlinkedplugin.jsonis an immediate example, because source parsing follows it whileCopyTreeomits it, so install reports success for a target with no manifest. Mirror the skill flow: copy first, validatestaging/plugin.json, hash staging, then swap it into place. -
[P1] Make replacement recoverable across process interruption
internal/skills/install.go:230-242,internal/plugins/install.go:203-217
The claimed atomic replacement is two renames: moving the existing target to.old, then moving staging to the target. A kill or power loss after the first rename leaves the canonical install absent, the old tree stranded under a randomized backup name, and the lockfile pointing at the missing target; no startup or install recovery restores it. Use an exchange/recoverable transaction and recover owned backups before discovery or subsequent installs. -
[P1] Do not block on a FIFO swapped in after directory enumeration
internal/fscopy/verify_unix.go:113-118
readDirAtclassifies an entry as regular, butopenRegularReadAtthen opens it withoutO_NONBLOCK. A concurrently writable local source can replace that entry with a FIFO in thefstatat→openatwindow;O_NOFOLLOWdoes not reject FIFOs, so the open blocks indefinitely before the laterStatcan reject the type. This can hang skill/plugin installation and hashing. Open nonblocking and reject non-regular descriptors before any blocking read. -
[P2] Do not re-resolve verified directories by pathname on Windows
internal/fscopy/verify_nounix.go:18-45
The Windows build first obtains a no-follow directory handle, then ignores that handle and callsos.ReadDir(parent.Name())plus pathname-based recursion. A local source that is writable concurrently can replace the verified root or child with a junction/reparse point between those steps, causing copy/hash to traverse files outside the selected source. Keep traversal bound to verified handles or revalidate every directory identity before using its pathname. -
[P2] Remove the depth limit that hides installed assets
internal/skills/skills.go:379-380
CopyTreeinstalls arbitrary-depth trees, but asset discovery stops below a directory depth of 65. A valid skill withd/.../script.shat that depth installs successfully yet is never advertised byFormatOutput, contradicting the recursive/full-tree asset contract. The new depth-90 tests mask this by skipping when no assets are found. Since directory symlinks are already not traversed, remove or redesign this cap without silently dropping copied assets. -
[P2] Preserve nested files named
SKILL.mdas assets
internal/skills/skills.go:409-413
The loader skips every recursively encounteredSKILL.md, although only the root manifest has been loaded as skill content. A copied asset such astemplates/SKILL.mdor an embedded sub-skill manifest is therefore omitted fromSkill.Assetsand model-facing output. Restrict the exclusion to the root manifest rather than matching the basename at every depth. -
[P2] Replace the quadratic directory ordering implementation
internal/fscopy/verify_unix.go:123-132
The new shared traversal reads every entry and then insertion-sorts it with no directory-size bound. A plugin or skill tree with a wide directory turns install and hash setup into O(n²) work before copying; the previous hash path used the standard O(n log n) sort. Usesort.Strings(or a documented entry limit) so a large but valid source cannot make installation CPU-bound for an unbounded period.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Recover replacement backups from the directory where swaps actually write them
internal/skills/install.go:247
swapDirIntoPlaceandswapStagedPluginIntoPlacestash the previous install next to the target, so the crash backup is created inside the live root as<skillsDir>/.zero-skill-replace-<name>or<pluginsDir>/.zero-plugin-replace-<id>. HoweverRecoverPendingscansfilepath.Dir(dir)instead ofdir, soLoadnever sees the real backup after a kill between stashing the old tree and moving the new tree into place. In that state the canonical install remains missing while the lockfile still points at it; if both target and backup exist, the hidden backup can also be scanned as ordinary skill/plugin content. The new recovery tests simulate backups in the parent directory, so they pass while missing the production path. Please make the swap backup location and the bulk recovery scan agree, and avoid sweeping or restoring parent-level staging/backup names that may belong to a sibling root. -
[P2] Do not re-resolve verified Windows directories by pathname
internal/fscopy/verify_nounix.go:19
On Windows,noFollowOpenDiropens a handle and rejects final-component reparse points, but the shared!unixtraversal immediately ignores that handle and callsos.ReadDir(parent.Name()), then recurses viasrcPathand hashes viahashTreeIntoPath. A writable local source can replace the verified root or child directory with a junction/reparse point between the handle check and those pathname operations, causing skill/plugin install or hashing to traverse a different tree than the one that was verified. That breaks the safe-copy/hash contract for Windows install flows. Please keep traversal bound to verified handles on Windows or otherwise revalidate each directory identity before using its path.
I don't have windows development environment, so I added a todo to the code for the problem "[P2] Do not re-resolve verified Windows directories by pathname". |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
I ran this review from a linux operating system btw.
Findings
[P1] Obtain approval for the linked issue before accepting this external contribution
CONTRIBUTING.md:16
The author is a CONTRIBUTOR, and the linked issue #584 currently has only the enhancement label—not the required issue-approved label. The PR checklist leaves this prerequisite unchecked, while the contribution policy says an external PR opened before approval must be closed without review. Obtain core-team approval on #584 before this implementation proceeds.
[P1] Restore the sandbox's default secret boundary
internal/sandbox/profile.go:103, internal/sandbox/runner.go:377
This removes both the inherited-environment scrubber and the default deny-read carve-outs while restricted profiles still grant / as a read root. An ordinary sandboxed workspace command can now read ~/.aws, gcloud/Azure credentials, and GOOGLE_APPLICATION_CREDENTIALS, and can obtain provider, cloud, and VCS tokens from its environment; it can copy or exfiltrate them whenever network is allowed. Restore the scrubbing and credential-store deny rules (or provide an equivalent default protection) rather than requiring every user to enumerate their own secrets.
[P1] Do not put macOS keyring secrets in the process arguments
internal/keyring/keyring.go:65
Keyring.Set now invokes security ... -w , so a same-user process can read an API key or password from the short-lived command line. The replaced security -i flow intentionally carried that value on stdin, and the new package comment acknowledges the exposure. Keep the secret out of argv for the normal macOS keyring backend.
[P1] Scope and synchronize install-recovery state to its owning root
internal/skills/install.go:338, internal/plugins/install.go:336
RecoverPending scans the shared parent and treats every matching backup or staging directory as belonging to the requested root. With sibling roots such as /data/skills-a and /data/skills-b, loading skills-a can restore or delete skills-b's interrupted replacement; it can also delete a live staging tree while another install is copying because there is no ownership/liveness check. The recovery namespace needs root identity plus transaction synchronization so a load or sibling root cannot corrupt or abort an active install.
[P2] Finish the Windows directory-confinement implementation before claiming the copy path is safe
internal/fscopy/verify_nounix.go:52
The Windows build opens and validates a directory, then discards that handle and enumerates parent.Name() by pathname; hashing also recurses by pathname. A concurrently writable source can replace the root or child with a junction/reparse point between those operations, making copy/hash traverse a different tree. The file documents this as an unresolved TODO, so the earlier Windows hardening request is not fixed at the current head.
[P2] Preserve the Windows retry for atomic state replacement
internal/sessions/store.go:844, internal/cron/store.go:147, internal/swarm/mailbox.go:327
Replacing RenameWithRetry with one-shot os.Rename removes the explicit retry for ordinary Windows sharing/lock violations from antivirus, indexing, or a concurrent reader. Session checkpoints/metadata, cron updates, and mailbox sends now fail immediately under that transient condition instead of using the former bounded retry; the Windows retry helper and coverage were deleted with no replacement.
[P2] Keep the recovery path for silent Windows sandbox failures
internal/tools/sandbox_denial.go:46
The removed branch classified a nonzero, output-free restricted-token failure as a likely sandbox denial. Those executable/DLL startup failures are often silent, and without that classification the agent loop does not offer the approval-gated unsandboxed retry; users receive an opaque blank failure instead. Retain the fallback and its regression coverage.
[P2] Respect an explicitly supplied Windows command environment
internal/sandbox/windows_runner.go:370
windowsSandboxChildEnv clones CommandSpec.Env but then overwrites its PATH, TERM, COMSPEC, SystemRoot, and WINDIR from the host process. A caller selecting a project-local toolchain through CommandSpec.Env consequently resolves executables against the host instead. Derive these defaults from the supplied environment when it is present, as the non-Windows path does.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Re-review (post 29 commits, head f8cd21b)
Thanks for the substantial follow-up since my Jul 8 review — the branch is materially stronger and CI is fully green.
Prior code blockers — resolved ✅
- Atomic install — staging outside scan root, validate/hash staging copy, swap with recovery
FormatOutputpath leak — skill-relative paths;filepath.Basefallback onlyfscopyTOCTOU — fd-held Unix traversal; Windows reparse-point opens; fail-closed on unsupported targetsfscopytests — comprehensive security/regression suite in place
Remaining blocker — process gate ❌
Issue #584 still has only the enhancement label — issue-approved is not present. Per CONTRIBUTING.md, community PRs without that label should not merge.
Action needed: Core team must add issue-approved to #584 before merge. Once that's done, I'm comfortable approving the code as-is.
Verdict: Code is ready; waiting on the governance step.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore the MCP server's configured resource scope
internal/cli/serve.go:46
The refactor still resolves-C/--cwdfor the tool registry, but no longer passes it asmcp.ServeOptions.WorkspaceRoot;mcp.Servetherefore falls back to the process working directory forresources/listandresources/read. This makes resources expose a different (and potentially unintended) directory from the tools. The same change also removes the previously documented--add-dirparser and scoped registry, so existingzero serve --mcp --add-dir <path>invocations now fail as an unknown flag. Restore theWorkspaceRoot/Scopewiring and the compatible flag handling. -
[P1] Do not recover or delete transaction artifacts belonging to another root or a live install
internal/skills/install.go:338
RecoverPendingscans the parent shared by all configured roots and treats every matching backup/staging prefix as its own. With sibling roots such as/data/skills-aand/data/skills-b, an interrupted replacement offooinskills-bcan be restored into (or deleted while loading)skills-a; a normalLoadcan also remove another install's still-active staging directory while it is copying or hashing. The plugin implementation has the same behavior. Namespace artifacts by their owning root and do not sweep them without a transaction/liveness guarantee. -
[P2] Keep the installed tree and lockfile transactionally consistent
internal/skills/install.go:196
The new tree is swapped into the live target beforewriteLockpersists its hash and source. A write error or interruption at that point leaves the new bytes active with the old lock entry (or no entry on a first install), andRecoverPendingonly handles directory renames so it cannot repair the stale provenance. Plugins follow the same sequence. Commit both pieces atomically/recoverably, or retain enough rollback state to restore the old tree if writing the lock fails. -
[P1] Do not claim a no-follow tree traversal on Windows while it still re-resolves paths
internal/fscopy/verify_nounix.go:52
Windows builds this!uniximplementation. AfternoFollowOpenDirverifies a directory handle,copyTreeAtimmediately usesos.ReadDir(parent.Name())and recurses by pathname; hashing does the same. A writable source can replace a verified root or child with a junction/reparse directory between those operations, causing the installer or recorded hash to traverse an unverified tree. Implement handle-bound traversal/revalidation on Windows, or fail closed there rather than shipping the safe-copy contract with this documented gap.
…-support # Conflicts: # internal/skills/skills_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep recovery scoped to its own root and live transaction
internal/skills/install.go:338
RecoverPendingscans the parent shared by all configured roots, then treats every matching backup or staging prefix as its own. A load of/data/skills-acan therefore restore a crashed/data/skills-b/foobackup intoskills-a(or delete it), and it can deleteskills-b's live staging directory while an install is copying or hashing. The plugin implementation has the same behavior. Namespace transaction artifacts by their canonical root and synchronize recovery with active installs; a read-only load must not be able to delete or relocate another root's data. -
[P1] Serialize lockfile updates with installs
internal/skills/install.go:186
Two installs into the same root both read the entire lock map before either writes it. Installing different skills concurrently therefore lets the lastwriteLockoverwrite the other entry; same-name interleavings can record one source/hash for the other tree. This loses provenance and bypasses the different-source clash guard on the next install. Apply a root-level transaction/lock (also in the plugin installer) around the read-modify-write and tree replacement. -
[P2] Make the tree and its provenance lock one recoverable transaction
internal/skills/install.go:196
The staged tree becomes live, and its rollback backup is removed, beforewriteLockpersists the new hash/source. A lock write error, kill, or power loss then leaves new bytes with the old lock entry (or no entry on a first install); recovery only sees directory artifacts and commits that mismatch. Keep rollback state until the lock is durably committed, or journal both updates so recovery can converge them. Plugins have the same ordering. -
[P1] Do not advertise no-follow copying on Windows while traversal re-resolves paths
internal/fscopy/verify_nounix.go:52
Windows builds this fallback. AfternoFollowOpenDirverifies a directory handle,copyTreeAtcallsos.ReadDir(parent.Name())and recursively reopens child paths. A writer can replace the verified root or child with a junction/reparse directory between those operations, making copy/hash traverse an unverified tree outside the intended source. Use handle-relative enumeration/recursion on Windows or fail closed for this path. -
[P2] Do not expose local skill directories in model-facing output
internal/skills/skills.go:79
FormatOutputnow includesskill.Dir, whichloadfills with an absolute host path. The permission-allowed skill tools and slash invocation send that value to the model, exposing home, configuration, or project layout simply by loading a skill. Omit the directory or use a non-sensitive skill-relative identifier. -
[P2] Keep asset paths out of the list API
internal/skills/skills.go:406
ListFromRootsclears skill content but retains the newly added assets, whosePathvalues are absolute. The CLI's JSON list path serializes this result, so listing skills can now disclose up to 500 local paths per skill and substantially expand its output, unlikeListwhich deliberately clears assets. ClearAssetsinListFromRoots(or expose a separate explicit detail mode).
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving — everything I asked for on Jul 8 is in on this head. CopyFile now opens no-follow and fstats the descriptor it actually reads (with the unix walk fd-held end to end), install stages the copy and only swaps it into place after the copy succeeds — a failed upgrade can no longer wipe the working skill — and the asset fallback is basename-only. fscopy also grew the symlink/.git/hash-sensitivity tests I wanted, and CI is green on all three platforms.
On the points still open from jatmn's review: the lockfile read-modify-write race and the swap-before-lock ordering exist on main today — this PR didn't introduce them, and I'd rather track root-level locking as a follow-up than grow this branch further. The recovery scan crossing roots needs a non-default layout where two skills roots share a parent, and the worst case of the staging sweep is a failed install with the old skill left intact, so that's follow-up material for me too. The Windows handle-held traversal TODO is the right call — that shouldn't be written blind, and I can pick it up since I'm on Windows. And skill.Dir in the output is the feature: the model can't use scripts it can't locate.
The one gate that isn't code: #584 still doesn't carry issue-approved, so merging waits on kevin either way.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep recovery from deleting another transaction's live staging tree
internal/skills/install.go:338
RecoverPendingscans the shared parent directory and unconditionally removes every.zero-skill-install-*entry, whileInstallcreates exactly such an entry there before copying and hashing. Because ordinary skill discovery invokes this recovery path, a concurrentzero skills/agent load or another install can delete an active staging tree and make the first install fail. The same root-crossing behavior can also restore or delete an interrupted sibling root's replacement artifact; the plugin implementation has the same defect. Namespace and authenticate recovery artifacts to their owning root, and synchronize recovery with active installs rather than sweeping prefix matches. -
[P1] Do not ship the Windows installer with a reparse-point traversal race
internal/fscopy/verify_nounix.go:52
Windows builds this!uniximplementation. It verifies a directory handle and then discards that guarantee by enumeratingparent.Name()and recursively resolving child paths. A writable source can swap the verified root or child for a junction between those operations, causing the skill/plugin installer (and its hash) to traverse files outside the selected source tree. Final-component checks inCopyFiledo not protect intermediate junction traversal; use handle-bound enumeration/recursion or fail closed on Windows. -
[P1] Serialize the lockfile read-modify-write with replacement installs
internal/skills/install.go:186
Concurrent installs into the same root each read the full lock map before either writes it. Installing two different skills/plugins therefore allows the laterwriteLockto discard the other new entry; same-name interleavings can also defeat the different-source clash check. A root-level transaction lock must cover the lock read, conflict check, replacement, and persistence in both installers. -
[P2] Commit the replacement tree and provenance record as one recoverable transaction
internal/skills/install.go:196
The new skill tree becomes live and the old backup is removed beforewriteLockrecords its source and hash. A write error or process interruption then leaves new executable/instruction content installed with an old (or missing, potentially malformed) lock entry, whileInstallreports failure; laterinfoand clash checks no longer describe the bytes being loaded. Preserve rollback/journal state until the lock update is durably committed, and apply the same ordering to plugins.
…-support # Conflicts: # internal/plugins/install.go # internal/skills/install.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Install the auxiliary skill tree rather than only the manifest
internal/skills/skills.go:632
This adds recursive asset discovery, but the normalskills.Installpath still hashes and stages onlySKILL.md; none of the newfscopy.CopyTree/HashTreeAPIs has a production caller. Installing a source containingscripts/run.shor templates therefore succeeds while silently discarding every auxiliary file, leaving the newly advertised asset/script support unusable for the standard install workflow. Copy and hash the validated skill directory as a tree, and add an end-to-end installation test with a nested asset. -
[P1] Restore atomic cron job-ID reservation
internal/cron/store.go:102
allocIDnow performs an unlockedStatcheck, thenwriteJobcreates the directory afterward. ConcurrentAddcalls in the same second can both choose the same timestamp ID and race on the samemetadata.json, so one scheduled job is overwritten even though both callers report success; a concurrent remove can also delete the gap and havewriteJobresurrect it. This removes the prior mkdir-and-lock reservation protocol along with its concurrency regression tests. Reserve the ID atomically and hold the per-ID lock through the initial write.
4 participants
Summary
feat: add skill assets support #584
Linked issue
Closes #584
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit
Summary by CodeRabbit
New Features
SKILL.md), and install results point to the installed skill directory root.Bug Fixes
.git, refuses symlinks, confines traversal, and preserves permissions (including executable bits).