Skills & plugins: skill tool, skills loader, plugin manifests#123
Conversation
Module of the runtime-core split (off main, after #119 tools merged). Clean extract — internal/skills is new; internal/plugins had no drift on main. No new deps. - internal/skills: loads */SKILL.md frontmatter from the skills dir (resolves its own XDG dir); no YAML dependency. - skill tool (internal/tools/skill.go), read-only, registered in CoreReadOnlyTools (so it's in the agent core + the MCP read-only default). Path-safe: it Load()s all skills and matches by exact name — it never builds a path from the model-supplied arg, so there is no traversal surface. knownToolNames gains "skill" for the specialist cross-package invariant; CoreReadOnlyTools count test bumped 4->5. - internal/plugins: plugin manifest metadata enrichment (Author/License/Keywords/Interface). FORWARD code — the cli plugin-loading wiring lands later. MCP hardening (nested-schema + hung-server fix) is a SEPARATE follow-up PR. build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds optional plugin manifest metadata and formatting; a SKILL.md-based skills loader with confinement, parsing, deduplication, List/Get, and tests; a read-only skill tool that returns skill Markdown; and integrates "skill" into specialist validation and the core tools registry. ChangesSkills System Integration
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SkillTool
participant SkillsLoader
Caller->>SkillTool: Run(name or skill)
SkillTool->>SkillsLoader: Load(dir)
SkillsLoader-->>SkillTool: skills or error
alt exact skill match
SkillTool-->>Caller: StatusOK + Content
else no match / no skills / load error
SkillTool-->>Caller: StatusError + message (available names or reason)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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/plugins/plugins.go`:
- Around line 116-121: The JSON omitempty on non-pointer struct fields is
ineffective for Author and Interface, so change the Plugin struct fields Author
and Interface to pointer types (e.g., *PluginAuthor and *PluginInterface) so
zero values serialize as null/omitted; update all places that construct or copy
Plugin values (constructors, unmarshalling, tests, or any code referencing
Plugin.Author or Plugin.Interface) to use pointers or nil appropriately and
adjust any dereferences (e.g., checks like if p.Author != nil) to preserve
behavior.
In `@internal/skills/skills_test.go`:
- Around line 215-230: Update the TestListReturnsNamesAndDescriptions test to
also assert that List strips the skill body by checking that listed[0].Content
(and listed[1].Content) is an empty string; locate the test function
TestListReturnsNamesAndDescriptions and the call to List(dir) and add assertions
after the existing Name/Description checks to ensure Content == "" for each
returned item to prevent leaking full skill bodies from List.
In `@internal/skills/skills.go`:
- Around line 47-50: DefaultDir currently falls back to a relative
".local/share/zero/skills" when $XDG_DATA_HOME and $HOME are empty; change the
fallback to an absolute location to avoid binding to CWD. In the DefaultDir
function in internal/skills/skills.go, when base is empty (after checking
XDG_DATA_HOME and os.UserHomeDir()), set base to an absolute fallback such as
filepath.Join(os.TempDir(), "zero", "skills") or call filepath.Abs on the
computed base so it becomes absolute; use os.TempDir() and/or filepath.Abs to
guarantee an absolute path before returning filepath.Join(base, "zero",
"skills").
In `@internal/tools/skill.go`:
- Around line 33-43: The parameters Schema for the skill loader currently only
defines "name" and sets AdditionalProperties: false, but runtime code accepts an
alias "skill" (causing schema validation to reject calls before Run); either add
"skill" to the Schema Properties (and adjust Required/validation) or remove
alias handling so only "name" is accepted—update the Schema block (Parameters:
Schema, Properties map entries for "name"/"skill", Required slice and
AdditionalProperties) to match the runtime alias behavior used by the skill
loading logic (e.g., the Run handler that reads "skill"/"name").
🪄 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 Plus
Run ID: 78178b75-39af-4d33-8476-7897b60bd718
📒 Files selected for processing (9)
internal/plugins/plugins.gointernal/plugins/plugins_test.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/specialist/manifest.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/skill.gointernal/tools/skill_test.go
…s, dir fallback, List test
- plugins: Author/Interface are now *PluginAuthor/*PluginInterface so omitempty actually omits them — a non-pointer struct is never empty to encoding/json, so the old form emitted author:{}/interface:{} and changed the serialized shape of plugins that don't set them. parseAuthor/parseInterface return nil when all fields are empty; formatAuthor takes the pointer. (Major)
- skill: schema now declares the 'skill' alias alongside 'name' (and drops strict Required) so the alias survives schema validators that reject unknown keys (AdditionalProperties:false); Run still enforces exactly-one-of via aliasedStringArg. (Major)
- skills.DefaultDir: return "" instead of a relative ".local/share/..." path when neither XDG_DATA_HOME nor a resolvable HOME exists, so skills can't bind to the process CWD (load("") already no-ops). (Minor)
- skills_test: TestListReturnsNamesAndDescriptions now asserts List strips Content so a listing can't silently leak full skill bodies. (Minor)
build/vet/-race/full-suite + GOOS=windows build green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review — addressed all 4: (1) omitempty — Author/Interface are now pointers so empty metadata is actually omitted (parse funcs return nil when empty); (2) skill schema — declares the |
|
Risk surface note (per policy on PRs touching skills/plugins/MCP): The skill tool is registered into Plugin-loading CLI wiring is deferred, so the plugin manifest code introduces no new runtime attack surface in this PR. Triggering a full review now. 🧠 Learnings used✅ Action performedFull review finished. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: request changes.
The earlier metadata/alias/default-dir/listing issues look fixed on this head. One safety blocker remains before exposing skill as a permission-allow core/MCP tool: the loader should reject or confine symlinked SKILL.md files so a skills directory cannot be used to read arbitrary files outside the skills root.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tools/skill.go (1)
68-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMinor UX: Empty dir in error message could be clearer.
When
skills.DefaultDirreturns""(e.g., HOME and XDG_DATA_HOME unavailable), the error message on Line 69 would render as"no skills are available (looked in )."with an empty string interpolated. Consider handling this edge case for clarity.Suggested improvement
if len(loaded) == 0 { + if tool.dir == "" { + return errorResult("Error: no skills are available (skills directory not configured).") + } return errorResult(fmt.Sprintf("Error: no skills are available (looked in %s).", tool.dir)) }🤖 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/tools/skill.go` around lines 68 - 70, The error message when no skills are loaded interpolates tool.dir and can produce "looked in " if tool.dir is empty; update the return in the len(loaded) == 0 branch to detect an empty tool.dir (or use skills.DefaultDir if applicable) and substitute a clear placeholder like "no skills directory configured" (e.g., compute dirDisplay := tool.dir; if dirDisplay=="" { dirDisplay = "no skills directory configured" } and use dirDisplay in the fmt message) so the message reads clearly when skills.DefaultDir returns "".
🤖 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.
Outside diff comments:
In `@internal/tools/skill.go`:
- Around line 68-70: The error message when no skills are loaded interpolates
tool.dir and can produce "looked in " if tool.dir is empty; update the return in
the len(loaded) == 0 branch to detect an empty tool.dir (or use
skills.DefaultDir if applicable) and substitute a clear placeholder like "no
skills directory configured" (e.g., compute dirDisplay := tool.dir; if
dirDisplay=="" { dirDisplay = "no skills directory configured" } and use
dirDisplay in the fmt message) so the message reads clearly when
skills.DefaultDir returns "".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ea6b28a-7ecf-4013-80db-1e74285c938f
📒 Files selected for processing (5)
internal/plugins/plugins.gointernal/plugins/plugins_test.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/tools/skill.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/plugins/plugins_test.go
- internal/plugins/plugins.go
- internal/skills/skills.go
- internal/skills/skills_test.go
…skills root skill is a permission-allow read-only core/MCP tool, so the loader must not follow a symlinked SKILL.md (or skill dir) out of the skills root and become an arbitrary-file reader. load() now resolves the skills root via EvalSymlinks and, for each entry, resolves SKILL.md through symlinks and verifies the real path stays under the root (confineSkillPath); an escaping or unreadable path is skipped rather than read. Mirrors the grep/read_file confinement. Legit skills under a symlinked root (e.g. macOS /tmp) still load because the root is resolved too. Regression: TestLoadSkipsSymlinkedSkillFileEscapingRoot (a SKILL.md symlinked to a secret outside the root is skipped, not read). build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@Vasanthdev2004 good catch — fixed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/skills/skills.go (1)
116-127:⚠️ Potential issue | 🟠 MajorPrevent symlink-based local file disclosure via
skilltool (SKILL.mdread)
internal/skills/skills.gobuildsmanifestPath := filepath.Join(dir, entry.Name(), skillFileName)and then callsos.ReadFile(manifestPath)(line ~122) without rejecting symlinked skill directories or symlinkedSKILL.md. Sinceinternal/tools/skill.goreturns the loadedskill.Contentdirectly as theskilltool output, a local user-controlled skills root (ZERO_SKILLS_DIR/ XDG) containing a malicious symlinkedSKILL.mdcan let the model retrieve arbitrary files outside the skills root (the tool can also enumerate available skills via the “unknown skill” error).scrubResultSecretsonly redacts known secret patterns; it does not prevent arbitrary content leakage.Add symlink containment checks before reading (skip symlink directories and ensure
SKILL.mdis not a symlink, or resolve and verify it stays under the resolved skills root). Also add a regression test for a symlinkedSKILL.mdbeing skipped.Reject symlinks before reading `SKILL.md`
for _, entry := range entries { if !entry.IsDir() { continue } + // Reject symlinked skill directories to prevent reading arbitrary files. + if entry.Type()&os.ModeSymlink != 0 { + continue + } manifestPath := filepath.Join(dir, entry.Name(), skillFileName) + // Verify SKILL.md itself is not a symlink. + info, err := os.Lstat(manifestPath) + if err != nil { + continue + } + if info.Mode()&os.ModeSymlink != 0 { + continue + } data, err := os.ReadFile(manifestPath) if err != nil { // Missing or unreadable SKILL.md (including the case where it is a // directory) is skipped, not fatal — one bad skill must not hide the rest. continue }🤖 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.go` around lines 116 - 127, The code builds manifestPath := filepath.Join(dir, entry.Name(), skillFileName) and reads it without symlink checks; change the logic in the loop that iterates entries in internal/skills/skills.go (the block using entries, entry, dir, manifestPath, skillFileName) to first skip any entry that is a symlink (use os.Lstat and check ModeSymlink) and then, before calling os.ReadFile, check whether manifestPath itself is a symlink (skip if so) or resolve the absolute path (via filepath.EvalSymlinks or similar) and verify the resolved path has the skills root (resolved dir) as its prefix to ensure containment; update the SKILL loading code path that returns skill.Content accordingly and add a regression test that creates a symlinked SKILL.md (and a symlinked skill directory) and asserts those are skipped.
🧹 Nitpick comments (1)
internal/plugins/plugins.go (1)
481-495: 💤 Low valueConsider including
Homepagein the formatted output.
formatPluginMetadatarenders author, license, and keywords but omits theHomepagefield that is parsed at line 369. Including it would provide users with complete metadata visibility inzero pluginslistings.📝 Suggested addition
func formatPluginMetadata(plugin LoadedPlugin) []string { meta := []string{} if author := formatAuthor(plugin.Author); author != "" { meta = append(meta, "author: "+author) } if plugin.License != "" { meta = append(meta, "license: "+plugin.License) } + if plugin.Homepage != "" { + meta = append(meta, "homepage: "+plugin.Homepage) + } if len(plugin.Keywords) > 0 { meta = append(meta, "keywords: "+strings.Join(plugin.Keywords, ", ")) } return meta }🤖 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/plugins.go` around lines 481 - 495, formatPluginMetadata omits the plugin Homepage field; update the function (formatPluginMetadata) to check LoadedPlugin.Homepage and when non-empty append "homepage: "+plugin.Homepage to the meta slice (similar to author/license/keywords) so the homepage appears in zero plugins listings.
🤖 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/tools/registry.go`:
- Around line 174-176: The Skill tool registration (NewSkillTool("") in
CoreReadOnlyTools) is fine but skills.Load must block symlink attacks before
this tool is exposed; update skills.Load to detect and reject symlinked SKILL.md
files (use os.Lstat to check ModeSymlink or resolve with filepath.EvalSymlinks
and verify the resolved path is inside the allowed skills.DefaultDir), return an
error when a symlink or an out-of-dir target is found, and ensure NewSkillTool
remains read-only until this validation is in place.
---
Duplicate comments:
In `@internal/skills/skills.go`:
- Around line 116-127: The code builds manifestPath := filepath.Join(dir,
entry.Name(), skillFileName) and reads it without symlink checks; change the
logic in the loop that iterates entries in internal/skills/skills.go (the block
using entries, entry, dir, manifestPath, skillFileName) to first skip any entry
that is a symlink (use os.Lstat and check ModeSymlink) and then, before calling
os.ReadFile, check whether manifestPath itself is a symlink (skip if so) or
resolve the absolute path (via filepath.EvalSymlinks or similar) and verify the
resolved path has the skills root (resolved dir) as its prefix to ensure
containment; update the SKILL loading code path that returns skill.Content
accordingly and add a regression test that creates a symlinked SKILL.md (and a
symlinked skill directory) and asserts those are skipped.
---
Nitpick comments:
In `@internal/plugins/plugins.go`:
- Around line 481-495: formatPluginMetadata omits the plugin Homepage field;
update the function (formatPluginMetadata) to check LoadedPlugin.Homepage and
when non-empty append "homepage: "+plugin.Homepage to the meta slice (similar to
author/license/keywords) so the homepage appears in zero plugins listings.
🪄 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 Plus
Run ID: a1142f5c-459f-4a1f-9397-916a7604ce4d
📒 Files selected for processing (9)
internal/plugins/plugins.gointernal/plugins/plugins_test.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/specialist/manifest.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/skill.gointernal/tools/skill_test.go
|
@coderabbitai review — the symlink disclosure (your only actionable item, also raised by @Vasanthdev2004) is fixed on the current head |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: approved.
Rereviewed the latest head (efffc7c). The previous skill-loader symlink issue is fixed: SKILL.md is resolved and confined to the resolved skills root before reading, with a regression test for an escaping symlink.
Validation run locally on this head:
go test ./internal/skills ./internal/tools ./internal/plugins ./internal/specialistgo test ./...
Both passed. GitHub CI smoke checks were green when rechecked; CodeRabbit was still pending at that exact moment.
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Inspect the symlink confinement implementation in skills.go
fd -e go skills.go --exec cat {}
# Also check for confineSkillPath and EvalSymlinks usage
rg -n "confineSkillPath|EvalSymlinks|HasPrefix|strings.HasPrefix|filepath.Rel" --type go -C3Length of output: 97553
Triggering a full review now. 🧠 Learnings used✅ Action performedFull review finished. |
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 101-113: confineSkillPath currently only ensures manifestPath is
under rootReal but can return special files; before callers (e.g., the
os.ReadFile(realPath) call) attempt to read, reject non-regular files by
stat-ing the resolved path: call os.Stat on the resolved real path returned by
confineSkillPath (or perform the stat inside confineSkillPath) and check
FileInfo.Mode().IsRegular(); if not regular (FIFO, device, directory, symlink
target that's not a regular file), return false so reads never proceed on
special files.
🪄 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 Plus
Run ID: 0365c251-275c-41e4-91b6-09342a0f35f1
📒 Files selected for processing (2)
internal/skills/skills.gointernal/skills/skills_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/skills/skills_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/skills/skills.go (1)
101-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-regular in-root
SKILL.mdtargets before reading.
confineSkillPathonly enforces path confinement. On Line 163,os.ReadFile(realPath)can still block on FIFO/device files inside the skills root, so a user-controlled skills directory can hang the read-only skill tool. This is a blocker before remote MCP exposure.🔧 Proposed fix
func confineSkillPath(rootReal string, manifestPath string) (string, bool) { real, err := filepath.EvalSymlinks(manifestPath) if err != nil { return "", false } + info, err := os.Stat(real) + if err != nil || !info.Mode().IsRegular() { + return "", false + } rel, err := filepath.Rel(rootReal, real) if err != nil { return "", false } if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { return "", false } return real, true }Based on learnings: if the PR touches skills/plugins/MCP, call out the risk surface and whether it introduces a blocker.
Also applies to: 163-166
🤖 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.go` around lines 101 - 113, confineSkillPath currently only enforces path confinement but allows non-regular targets (e.g. FIFOs/devices) which later let os.ReadFile hang; update confineSkillPath (or immediately before the os.ReadFile(realPath) call) to perform an os.Lstat (or os.Stat) on the resolved path `real` and check fileInfo.Mode().IsRegular(); if the file is not regular, return false (or surface an error) so the code never attempts to read non-regular in-root `SKILL.md` targets.
🧹 Nitpick comments (2)
internal/plugins/plugins.go (1)
421-438: 💤 Low valueMinor: Trim called twice per string.
Line 430-431 trims each string twice (once in the condition, once in append).
♻️ Optional micro-optimization
for _, item := range items { - if text, ok := item.(string); ok && strings.TrimSpace(text) != "" { - values = append(values, strings.TrimSpace(text)) + if text, ok := item.(string); ok { + trimmed := strings.TrimSpace(text) + if trimmed != "" { + values = append(values, trimmed) + } } }🤖 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/plugins.go` around lines 421 - 438, In coerceMetaStringSlice, avoid calling strings.TrimSpace twice for each item by trimming once into a local variable (e.g., trimmed := strings.TrimSpace(text)) and then using that trimmed value for the empty check and for append; update the loop inside coerceMetaStringSlice to use the single-trimmed variable.internal/tools/registry.go (1)
174-176: Skills/MCP risk surface acknowledged; not a blocker for this PR.The skill tool reads from a user-controlled directory (ZERO_SKILLS_DIR or XDG skills path). The implementation includes symlink confinement (EvalSymlinks + confineSkillPath per PR objectives) and never builds filesystem paths from model input (exact name match only). The tool is correctly marked read-only. Before exposing this tool to remote clients via MCP serve, confirm that the read-only label is enforced at the MCP boundary. Per PR objectives, MCP wiring is deferred work, so this is noted for future integration but does not block this PR.
Based on learnings, this PR touches skills/plugins/MCP and I've verified the risk surface does not introduce a blocker.
🤖 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/tools/registry.go` around lines 174 - 176, The NewSkillTool is being registered to read user-controlled skill dirs; ensure that before exposing it via MCP serve you enforce the read-only label at the MCP boundary by validating MCP request handling/wiring to respect the tool's read-only intent, confirm the implementation uses EvalSymlinks and confineSkillPath for symlink confinement and that no filesystem paths are constructed from model input (only exact name matches are accepted), and add a checklist or TODO in the MCP wiring code to verify and enforce read-only access control when integrating skills/plugins/MCP in the next PR.
🤖 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.
Duplicate comments:
In `@internal/skills/skills.go`:
- Around line 101-113: confineSkillPath currently only enforces path confinement
but allows non-regular targets (e.g. FIFOs/devices) which later let os.ReadFile
hang; update confineSkillPath (or immediately before the os.ReadFile(realPath)
call) to perform an os.Lstat (or os.Stat) on the resolved path `real` and check
fileInfo.Mode().IsRegular(); if the file is not regular, return false (or
surface an error) so the code never attempts to read non-regular in-root
`SKILL.md` targets.
---
Nitpick comments:
In `@internal/plugins/plugins.go`:
- Around line 421-438: In coerceMetaStringSlice, avoid calling strings.TrimSpace
twice for each item by trimming once into a local variable (e.g., trimmed :=
strings.TrimSpace(text)) and then using that trimmed value for the empty check
and for append; update the loop inside coerceMetaStringSlice to use the
single-trimmed variable.
In `@internal/tools/registry.go`:
- Around line 174-176: The NewSkillTool is being registered to read
user-controlled skill dirs; ensure that before exposing it via MCP serve you
enforce the read-only label at the MCP boundary by validating MCP request
handling/wiring to respect the tool's read-only intent, confirm the
implementation uses EvalSymlinks and confineSkillPath for symlink confinement
and that no filesystem paths are constructed from model input (only exact name
matches are accepted), and add a checklist or TODO in the MCP wiring code to
verify and enforce read-only access control when integrating skills/plugins/MCP
in the next PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e35a1fa1-bc30-427e-814e-7dbb965e7c79
📒 Files selected for processing (9)
internal/plugins/plugins.gointernal/plugins/plugins_test.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/specialist/manifest.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/skill.gointernal/tools/skill_test.go
…gle-trim metadata - skills: confineSkillPath now Lstats the resolved path and skips non-regular targets (directory/FIFO/device/socket). A FIFO/device named SKILL.md (or an in-root symlink to one) would otherwise make os.ReadFile block indefinitely — skill is a permission-allow tool over a user-controlled dir. Regression: TestLoadSkipsNonRegularSkillFile (unix-tagged; mkfifo SKILL.md, asserts Load skips it within a timeout rather than hanging). (Major) - plugins: coerceMetaStringSlice trims each item once into a local var instead of calling strings.TrimSpace twice. (Nitpick) build/vet/-race/full-suite + GOOS=windows build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Verified each against current code:
build/vet/-race/full-suite + windows green. @coderabbitai review |
|
The MCP TODO deferral rationale is reasonable for this PR given:
I’ll trigger another review on the current head. ✏️ Learnings added
🧠 Learnings used✅ Action performedReview finished.
|
2 participants
Module — skills & plugins
Next reviewable slice of the runtime-core split (off
main, now that #119 tools merged). Clean extract —internal/skillsis new,internal/pluginshad zero drift on main. No new deps.What's in it
internal/skills— loads*/SKILL.mdfrontmatter from the skills dir (resolves its own XDG~/.local/share/zero/skills); no YAML dependency.skilltool (internal/tools/skill.go), read-only, registered inCoreReadOnlyTools(so it's in the agent core and the MCP read-only default viaserve.go). Path-safe by construction: itLoad()s all skills and matches by exactname— it never builds a filesystem path from the model-supplied arg, so there's no traversal surface. The dir is read viaos.ReadDir, not user input.internal/plugins— plugin manifest metadata enrichment (Author/License/Keywords/Interface). Forward code — the cli plugin-loading wiring lands in a later PR.Cross-package
specialist.knownToolNamesgains"skill"(theTestKnownToolNamesMatchCoreRegistryinvariant —skillis now a core tool); theCoreReadOnlyToolscount test goes 4→5.Testing
go build ./...,go vet ./...,go test ./...,go test -race ./internal/skills/ ./internal/tools/, andGOOS=windows go build ./...all green.Part of decomposing #101 (draft); subagents excluded.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests