feat(skill):assets-script-support#591
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds shared filesystem copy and hashing helpers, migrates plugin and skill installation to staged directory swaps, expands skills with directory and asset metadata, and changes skill output call sites to use formatted skill rendering. Changesfscopy package and install migration
Skill asset discovery and formatted output
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Install
participant fscopy
participant StagingDir
participant TargetDir
Install->>fscopy: HashTree(sourceDir)
Install->>StagingDir: CopyTree(source, staging)
Install->>TargetDir: swap staging into place
alt copy or swap fails
Install->>TargetDir: rollback or preserve existing install
else swap succeeds
Install->>TargetDir: remove backup
end
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.
CengSin
force-pushed
the
feat/skill-assets-support
branch
from
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
3 participants
Summary
feat: add skill assets support #584
Linked issue
Feat #584
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit