Repository navigation
feat: support grouped skill directories - #270
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (27)
Disabled knowledge base sources:
🔥 Files not summarized due to errors (1)
WalkthroughChangesThe PR centralizes Skill workspace lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as skill create command
participant Metadata as normalizeSkillGroup
participant Scaffolder as createWorkspaceSkill
participant Filesystem
CLI->>Metadata: normalize --group path
Metadata-->>CLI: canonical group path
CLI->>Scaffolder: create grouped skill
Scaffolder->>Filesystem: stage and validate SKILL.md
Filesystem-->>Scaffolder: committed skill paths
Scaffolder-->>CLI: output skill and group
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| internal/skillscan/scan.go | Adds canonical deterministic recursive discovery with explicit entry and definition limits, addressing the previously unbounded traversal. |
| internal/workspace/scanner.go | Discovers nested workspace definitions by frontmatter identity while preserving registry-compatible ordering for duplicate handling. |
| internal/skills/registry_workspace_cache.go | Loads workspace roots in precedence order so lower-priority definitions remain available to shadow diagnostics. |
| internal/cli/skill_create.go | Implements staged grouped-skill creation with containment checks, symlink rejection, atomic rename, and cleanup handling. |
| internal/api/core/conversions_workspace_provider.go | Publishes resolved frontmatter identity through workspace API payloads. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Roots[Skill roots] --> Scanner[Shared skill scanner]
Scanner --> Identity[Frontmatter identity]
Identity --> Workspace[Workspace resolver]
Workspace --> Registry[Skills registry and precedence]
Registry --> Surfaces[CLI / HTTP / UDS / native tools]
Create[skill create --group] --> Stage[Validate staging directory]
Stage --> Roots
Reviews (2): Last reviewed commit: "fix: review round" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/skillscan/scan.go (1)
184-201: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winResolve the root once instead of per candidate.
pathWithinRootcallsfilepath.EvalSymlinks(root)for every discoveredSKILL.md, so a full scan performs up toMaxCandidatesredundant root resolutions on a path that runs on every workspace resolve/watcher refresh. Resolve the root once before the walk and pass it into the containment check.♻️ Proposed refactor
-func pathWithinRoot(root string, candidate string) error { - resolvedRoot, err := filepath.EvalSymlinks(root) - if err != nil { - return fmt.Errorf("resolve root %q: %w", root, err) - } - resolvedCandidate, err := filepath.EvalSymlinks(candidate) +func pathWithinRoot(resolvedRoot string, candidate string) error { + resolvedCandidate, err := filepath.EvalSymlinks(candidate) if err != nil { return fmt.Errorf("resolve definition %q: %w", candidate, err) }Then in
ScanDirectory, compute it once beforefilepath.WalkDir:resolvedRoot, err := filepath.EvalSymlinks(absRoot) if err != nil { return DirectoryResult{}, fmt.Errorf("skillscan: resolve root %q: %w", absRoot, err) }and call
pathWithinRoot(resolvedRoot, candidate)inside the walk.🤖 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/skillscan/scan.go` around lines 184 - 201, Update ScanDirectory to resolve absRoot once with filepath.EvalSymlinks before filepath.WalkDir, returning the existing contextual error on failure. Change pathWithinRoot to accept the already-resolved root and remove its per-candidate root resolution, while preserving candidate resolution and containment validation for each discovered SKILL.md.
🤖 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/cli/skill_create.go`:
- Around line 61-77: Update the staging-directory flow after validation and
before os.Rename in the skill creation function to explicitly chmod stagingDir
to 0755. Handle any chmod error with the same contextual error-wrapping style,
then preserve the existing commit behavior so the renamed skill directory has
the intended permissions.
In `@internal/cli/skill_workspace_metadata.go`:
- Around line 50-59: Update the skill-group segment loop to retain each
normalized value returned by normalizeSkillName, collect those normalized
segments, and join the collection for the returned path. Preserve empty-segment
validation and error propagation while ensuring inputs with surrounding
whitespace produce trimmed directory names.
---
Nitpick comments:
In `@internal/skillscan/scan.go`:
- Around line 184-201: Update ScanDirectory to resolve absRoot once with
filepath.EvalSymlinks before filepath.WalkDir, returning the existing contextual
error on failure. Change pathWithinRoot to accept the already-resolved root and
remove its per-candidate root resolution, while preserving candidate resolution
and containment validation for each discovered SKILL.md.
🪄 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: 947dcd0a-6978-4956-aa99-e9e83f2faf77
⛔ Files ignored due to path filters (12)
.release-notes/grouped-skill-directories-1785293468.mdis excluded by!**/*.mddocs/qa/scenarios/ET-nested-skill-groups.mdis excluded by!**/*.mddocs/qa/scenarios/REL-release-candidate-plan.mdis excluded by!**/*.mdpackages/site/content/runtime/cli-reference/skill/create.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/configuration/file-locations.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/configuration/skill-md.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/skills/index.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/skills/skill-md.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/workspaces/config-overlays.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/workspaces/multi-root.mdxis excluded by!**/*.mdxpackages/site/content/runtime/core/workspaces/resolver.mdxis excluded by!**/*.mdxskills/compozy/references/tools-and-skills.mdis excluded by!**/*.md
📒 Files selected for processing (26)
internal/api/core/conversions_workspace_provider.gointernal/api/core/session_workspace_internal_test.gointernal/api/httpapi/handlers_test.gointernal/api/udsapi/handlers_test.gointernal/cli/skill.gointernal/cli/skill_commands_mutation.gointernal/cli/skill_create.gointernal/cli/skill_output.gointernal/cli/skill_test.gointernal/cli/skill_workspace_metadata.gointernal/daemon/agent_skill_resources_integration_test.gointernal/daemon/daemon_integration_test.gointernal/daemon/native_tools_test.gointernal/skills/loader.gointernal/skills/loader_test.gointernal/skills/loader_values.gointernal/skills/registry_snapshot.gointernal/skills/registry_test.gointernal/skillscan/scan.gointernal/skillscan/scan_test.gointernal/workspace/resolver_integration_test.gointernal/workspace/resolver_test.gointernal/workspace/scanner.gointernal/workspace/skill_identity.gointernal/workspace/workspace.gointernal/workspace/workspace_test.go
💤 Files with no reviewable changes (2)
- internal/skills/loader_values.go
- internal/skills/loader_test.go
Comment on lines
+50
to
+59
| segments := strings.Split(trimmed, "/") | ||
| for _, segment := range segments { | ||
| if segment == "" { | ||
| return "", errors.New("skill group must not contain empty segments") | ||
| } | ||
| if _, err := normalizeSkillName(segment); err != nil { | ||
| return "", fmt.Errorf("skill group segment %q: %w", segment, err) | ||
| } | ||
| } | ||
| return strings.Join(segments, "/"), nil |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the normalized segment instead of discarding it.
normalizeSkillName trims each segment but the result is dropped (_), so --group "marketing / campaigns" passes validation and then creates directories literally named "marketing " and " campaigns". Rebuild the joined path from normalized segments.
🛠️ Proposed fix
segments := strings.Split(trimmed, "/")
- for _, segment := range segments {
+ for i, segment := range segments {
if segment == "" {
return "", errors.New("skill group must not contain empty segments")
}
- if _, err := normalizeSkillName(segment); err != nil {
+ normalized, err := normalizeSkillName(segment)
+ if err != nil {
return "", fmt.Errorf("skill group segment %q: %w", segment, err)
}
+ segments[i] = normalized
}
return strings.Join(segments, "/"), nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| segments := strings.Split(trimmed, "/") | |
| for _, segment := range segments { | |
| if segment == "" { | |
| return "", errors.New("skill group must not contain empty segments") | |
| } | |
| if _, err := normalizeSkillName(segment); err != nil { | |
| return "", fmt.Errorf("skill group segment %q: %w", segment, err) | |
| } | |
| } | |
| return strings.Join(segments, "/"), nil | |
| segments := strings.Split(trimmed, "/") | |
| for i, segment := range segments { | |
| if segment == "" { | |
| return "", errors.New("skill group must not contain empty segments") | |
| } | |
| normalized, err := normalizeSkillName(segment) | |
| if err != nil { | |
| return "", fmt.Errorf("skill group segment %q: %w", segment, err) | |
| } | |
| segments[i] = normalized | |
| } | |
| return strings.Join(segments, "/"), nil |
🤖 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/cli/skill_workspace_metadata.go` around lines 50 - 59, Update the
skill-group segment loop to retain each normalized value returned by
normalizeSkillName, collect those normalized segments, and join the collection
for the returned path. Preserve empty-segment validation and error propagation
while ensuring inputs with surrounding whitespace produce trimmed directory
names.
Add unlimited-depth skill discovery across bundled and filesystem sources, preserve frontmatter identity through workspace surfaces, and support transactional workspace scaffolding with --group. Document grouped layouts, add release notes and QA coverage, and keep the official skill within its startup prompt budget.
pedronauck
force-pushed
the
nested-skills
branch
from
July 29, 2026 04:04
d23029b to
0d01b1a
Compare
Merged
This was referenced Aug 1, 2026
Merged
Merged
This was referenced Aug 5, 2026
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 13, 2026
Merged
Merged
Merged
Merged
3 tasks done
This was referenced Aug 14, 2026
Merged
Merged
Merged
Merged
Merged
This was referenced Aug 24, 2026
Merged
Merged
This was referenced Sep 10, 2026
Merged
Merged
Merged
Merged
This was referenced Sep 27, 2026
Merged
Merged
This was referenced Oct 6, 2026
Merged
Merged
Merged
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SKILL.mddefinitions at arbitrary depth below every supported skill root, allowing transparent organizational groups such asmarketing/content/campaign-brief/.nameas the skill identity across precedence, workspace payloads, HTTP/UDS, native tools, watcher refreshes, and resource publication.compozy skill create <name> --group <relative/path>with transactional scaffolding and path/symlink safety checks.What changed
Unified discovery
internal/skillscanas the canonical scanner for filesystem and bundledfs.FSsources.Identity and runtime behavior
SKILL.md,mcp.json, hooks, references, scripts, and provenance sidecars.CLI authoring
--groupflag tocompozy skill createand its generated CLI reference.Documentation and release artifacts
HEAD; the startup prompt budget regression remains green..release-notes/grouped-skill-directories-1785293468.md.Validation
make verify— full monorepo gate passed: codegen, installer, source-size, Bun lint/typecheck/test, web build, Go fmt/lint/test/build, and boundaries.compozy__workspace_describeexposes the resolved nested identity.list,view, andwhere.QA tracker
Final status: untested for manual scenario QA. Automated integration coverage is green, but no real-user QA session was run in this implementation slice.
docs/qa/scenarios/ET-nested-skill-groups.mdfor empty groups, nested discovery, live add/edit/remove, shadow precedence, management surfaces, and cross-workspace isolation.pass→untestedreset indocs/qa/scenarios/REL-release-candidate-plan.md; this commit includes it as requested, without claiming a new release-candidate QA run.Compozy Impact Audit
compozy__workspace_describenow reports resolved nested skill identities;compozy__skill_listandcompozy__skill_viewconsume the unified catalog. No tool IDs, toolsets, descriptors, schemas, digests, risk flags, availability diagnostics, or capability gates changed.config.tomlkey, default, validation rule, or lifecycle changed.skills/compozy/references/tools-and-skills.mdwith grouped discovery andskill create --group, compacted to remain within the startup prompt budget.web/route, component, hook, cache key, or UI control changed. Existing consumers receive the same workspace skill payload shape with corrected names; runtime docs and generated CLI docs document the new behavior.Compatibility
This is additive public behavior with an intentional greenfield deletion of the fixed-depth implementation. Existing flat skill directories, precedence, disabled-skill handling, shadows, list/view/where operations, and sidecars continue to work unchanged. No migration or compatibility bridge is required.
Summary by CodeRabbit
New Features
skill create --group <path>to scaffold workspace skills into nested group directories.Bug Fixes
name(when present), improving correct identity and resolution across workspace/bundled discovery.Security & Reliability
--groupvalues, including whitespace normalization and protections against path traversal and symlink escapes.