feat(skill-registry): support loading pre-curated skill registry via load command and --load flag#976
Open
decode2 wants to merge 3 commits into
Open
Conversation
…load command and --load flag Adds support for loading an existing pre-curated .atl/skill-registry.md file into the current project without forcing a full scan/regeneration, resolving issue Gentleman-Programming#516. Key changes: - Added LoadRegistry function to internal/skillregistry package - Added gentle-ai skill-registry load <path> subcommand and --load <path> flag - Updated help output and added full unit test coverage in registry_test.go Closes Gentleman-Programming#516
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Changesskill-registry load command
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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/app/app.go`:
- Around line 261-275: The skill-registry command parsing in app.go should fail
fast for load when the source path is missing instead of falling through to
LoadRegistry with an empty path. Update the argument handling around
cmd/loadPath so that gentle-ai skill-registry load without a non-flag path
returns the usage error immediately, while still preserving the existing path
extraction behavior in the load branch and the later LoadRegistry call.
In `@internal/skillregistry/registry_test.go`:
- Around line 459-479: The LoadRegistry test currently only checks a fully
invalid file and copied registry content, so it misses the real contract. Extend
registry_test.go around LoadRegistry to add a partial-invalid case for a file
missing the required “## Skills”/“| Skill |” markers, and assert that a
successful load also writes the cache fingerprint at CacheRelPath in addition to
RegistryRelPath. Use the existing LoadRegistry, readFile, and
RegistryRelPath/CacheRelPath symbols to keep the test aligned with the
implementation.
In `@internal/skillregistry/registry.go`:
- Around line 181-184: The validation in registry.go’s load path is too
permissive because it accepts a file when any one of the markers is present.
Update the check around the content scan so the skill registry is only accepted
when all required markers are present, and return the existing invalid-format
error otherwise. Keep the fix localized to the registry loading logic that
populates `.atl/skill-registry.md`, using the current header/table marker
strings in the condition.
- Around line 210-215: The cache update in registry generation is swallowing
write failures, so the loaded registry can be treated as successfully preserved
even when the cache file is not written. In Regenerate, update the JSON marshal
and filemerge.WriteFileAtomic handling around the cacheFile Fingerprint write so
any error is returned to the caller instead of ignored. Keep the success path
only when both the marshal and atomic write succeed, and use the existing
cachePath/loadPath context to surface the failure clearly.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80d66b57-f8d2-4721-80f1-c52262002ce3
📒 Files selected for processing (4)
internal/app/app.gointernal/app/help.gointernal/skillregistry/registry.gointernal/skillregistry/registry_test.go
Comment on lines
+181
to
+184
| content := string(data) | ||
| if !strings.Contains(content, "## Skills") && !strings.Contains(content, "| Skill |") && !strings.Contains(content, "# Skill Registry") { | ||
| return Result{}, fmt.Errorf("invalid skill registry format in %q: missing required headers or table", loadPath) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require all registry markers before accepting the load.
Line 182 only rejects files when all markers are missing. A file with # Skill Registry but no ## Skills section or | Skill | table header still passes, gets copied into .atl/skill-registry.md, and is cached as a successful load.
Suggested fix
- if !strings.Contains(content, "## Skills") && !strings.Contains(content, "| Skill |") && !strings.Contains(content, "# Skill Registry") {
+ if !strings.Contains(content, "# Skill Registry") ||
+ !strings.Contains(content, "## Skills") ||
+ !strings.Contains(content, "| Skill |") {
return Result{}, fmt.Errorf("invalid skill registry format in %q: missing required headers or table", loadPath)
}📝 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
| content := string(data) | |
| if !strings.Contains(content, "## Skills") && !strings.Contains(content, "| Skill |") && !strings.Contains(content, "# Skill Registry") { | |
| return Result{}, fmt.Errorf("invalid skill registry format in %q: missing required headers or table", loadPath) | |
| } | |
| content := string(data) | |
| if !strings.Contains(content, "# Skill Registry") || | |
| !strings.Contains(content, "## Skills") || | |
| !strings.Contains(content, "| Skill |") { | |
| return Result{}, fmt.Errorf("invalid skill registry format in %q: missing required headers or table", loadPath) | |
| } |
🤖 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/skillregistry/registry.go` around lines 181 - 184, The validation in
registry.go’s load path is too permissive because it accepts a file when any one
of the markers is present. Update the check around the content scan so the skill
registry is only accepted when all required markers are present, and return the
existing invalid-format error otherwise. Keep the fix localized to the registry
loading logic that populates `.atl/skill-registry.md`, using the current
header/table marker strings in the condition.
…ion and registry parsing
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/skillregistry/registry_test.go`:
- Around line 459-470: In the negative tests around LoadRegistry, make sure you
are asserting the actual registry-format validation failure instead of any
generic error. Update the invalidPath and partialPath setup in registry_test.go
to fail the test if os.WriteFile cannot create the fixtures, and tighten the
LoadRegistry assertions so they verify the expected invalid-format error path
from LoadRegistry rather than passing on unrelated read or setup failures.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 023a06bd-7601-486f-90ce-be9121b7b716
📒 Files selected for processing (3)
internal/app/app.gointernal/skillregistry/registry.gointernal/skillregistry/registry_test.go
…d check test fixture writes
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
Closes #516
Adds support for loading a pre-curated
.atl/skill-registry.mdfile directly into a project without forcing a full scan and regeneration.Problem
Currently,
gentle-ai skill-registry refreshalways regenerates.atl/skill-registry.mdby scanning all project and user skill directories. Any manual curation (compact rules, custom path conventions, curated subsets across projects) gets wiped. When developers maintain a standardized, curated registry across multiple projects with similar tech stacks, there was no way to import that pre-curated file directly.Solution
LoadRegistryfunction: AddedLoadRegistry(loadPath, cwd string, force bool)tointernal/skillregistry. Validates the registry file structure (## Skills/ table headers), copies it atomically to.atl/skill-registry.md, and updates the cache.runSkillRegistryininternal/app/app.goto support:gentle-ai skill-registry load <path>gentle-ai skill-registry refresh --load <path>internal/app/help.godocumentation and added comprehensive unit test coverage (TestLoadRegistryValidatesAndLoadsRegistry).Test Plan
go test ./internal/skillregistry/...— unit tests pass.go test ./internal/app/...— CLI package tests pass.go build ./...— workspace builds cleanly.Summary by CodeRabbit
New Features
skill-registry load <path>to import a pre-curated skill registry into the current project.--load <path>option to load from an external registry, with clear status output (loaded vs refreshed) and cache-based skipping when unchanged.Bug Fixes
Tests