feat(skills): full agentskills.io spec compliance#71
Merged
Conversation
- escape catalog XML and drop file:// prefix on <location> - skip skills missing a required description; add Skill.Validate - add license/compatibility/metadata/allowed-tools/disable-model-invocation frontmatter fields plus a malformed-YAML (unquoted colon) fallback - scan ~/.agents/skills and dedupe by name with project>user precedence - treat --skills-dir as a direct directory; add --skill-disable + DisableSkill/EnableSkill SDK methods - enumerate bundled resources via <skill_resources> on activation - add activate_skill MCP tool with enum-constrained name and session dedup - protect activated skill content from compaction pruning - gate project-local skills on a persisted trust allowlist via SkillTrustPrompt and an interactive CLI prompt - document new fields, flags, and SDK surface across README and docs site Fixes #65
|
Connected to Huly®: KIT-72 |
|
Warning Review limit reached
More reviews will be available in 29 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements full agentskills.io spec compliance: expands the Changesagentskills.io spec compliance
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as cmd/root.go
participant Kit as pkg/kit/kit.go
participant TrustStore as internal/trust
participant SkillsLoader as internal/skills
participant ActivateTool as internal/skilltool
User->>CLI: kit run (with project dir containing SKILL.md)
CLI->>Kit: New(opts with SkillTrustPrompt, SkillsDisable)
Kit->>SkillsLoader: LoadUserSkills() + LoadProjectSkills(cwd)
SkillsLoader-->>Kit: user skills + project skills
Kit->>TrustStore: IsTrusted(projectDir)
alt not yet trusted
TrustStore-->>Kit: false
Kit->>CLI: SkillTrustPrompt(projectDir, skillCount)
CLI->>User: interactive prompt
User-->>CLI: TrustProject
CLI-->>Kit: TrustProject decision
Kit->>TrustStore: Trust(projectDir) — persists JSON
end
Kit->>Kit: applySkillDisableList(SkillsDisable)
Kit->>ActivateTool: skilltool.New(names, liveProvider)
Kit-->>User: agent ready with activate_skill tool
User->>ActivateTool: Run(name="coding-skill")
ActivateTool->>SkillsLoader: LoadSkill(path) — strip frontmatter
SkillsLoader-->>ActivateTool: skill content + resources
ActivateTool-->>User: <skill_content name="coding-skill">...</skill_content>
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 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 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 (4)
internal/skilltool/skilltool_test.go (1)
74-84: ⚡ Quick winAdd a regression test that disabled skills cannot be activated
Current tests validate known/unknown and dedup flows, but they don’t assert
DisableModelInvocationenforcement. Please add a case where provider returns a matching skill withDisableModelInvocation: trueand activation returns an error response.🤖 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/skilltool/skilltool_test.go` around lines 74 - 84, Add a new test function following the pattern of TestActivateSkill_UnknownSkill that validates the DisableModelInvocation enforcement. Create a provider function that returns a skills.Skill with a matching name but with DisableModelInvocation set to true, instantiate the tool with New(), call tool.Run() with a request for that skill name, and assert that the response contains an appropriate error message indicating the skill is disabled. This ensures that skills marked as disabled cannot be activated even when they exist in the provider.pkg/kit/skills_spec_test.go (1)
91-91: 💤 Low valueUnused
promptedcounter variable.The
promptedvariable is declared and incremented but never asserted. Consider adding verification that the prompt was called exactly once, or remove the counter if not needed.♻️ Optional: Verify prompt was called
// Trust decision → project skills loaded and directory persisted. prompted := 0 trusted, err := loadSkills(&Options{ SessionDir: projectDir, SkillTrustPrompt: func(_ string, _ int) TrustDecision { prompted++ return TrustProject }, }) if err != nil { t.Fatal(err) } if len(trusted) != 1 { t.Fatalf("expected 1 skill when trusted, got %d", len(trusted)) } + if prompted != 1 { + t.Fatalf("expected prompt to be called once, got %d", prompted) + }🤖 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 `@pkg/kit/skills_spec_test.go` at line 91, The `prompted` counter variable is declared and incremented but never verified in an assertion, making it unused. Either add an assertion at the end of the test to verify that `prompted` equals the expected count (likely 1 to confirm the prompt was called exactly once), or remove the variable declaration and any increment operations if it is not needed for the test validation.internal/trust/trust.go (1)
95-99: 💤 Low valueConsider using
RLockfor read-only operation.
IsTrustedis a read-only method but usesLock()instead ofRLock(). While functionally correct, usingsync.RWMutexwithRLock()would allow concurrent reads.However, since the struct uses
sync.Mutex(notsync.RWMutex), this is consistent with the current design. If contention becomes a concern, the mutex type could be upgraded.♻️ Optional: Upgrade to RWMutex for concurrent reads
type Store struct { - mu sync.Mutex + mu sync.RWMutex path string trusted map[string]bool }Then in
IsTrusted:func (s *Store) IsTrusted(dir string) bool { - s.mu.Lock() - defer s.mu.Unlock() + s.mu.RLock() + defer s.mu.RUnlock() return s.trusted[normalize(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/trust/trust.go` around lines 95 - 99, To optimize for concurrent reads in the Store type, upgrade the mutex from sync.Mutex to sync.RWMutex in the Store struct definition. Then in the IsTrusted method, replace the s.mu.Lock() call with s.mu.RLock() and change the corresponding defer statement from defer s.mu.Unlock() to defer s.mu.RUnlock(). This allows multiple goroutines to read the trusted map simultaneously while maintaining exclusive write access for mutation operations elsewhere in the code.www/pages/cli/commands.md (1)
116-124: ⚡ Quick winAdd language identifier to fenced code block for the trust prompt example.
Line 118 is missing a language identifier on the fenced code block. Use
```textor```shellto satisfy the markdown linter (MD040).Proposed fix
-This project provides 2 skills under .agents/skills or .kit/skills: +This project provides 2 skills under .agents/skills or .kit/skills: +```text🤖 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 `@www/pages/cli/commands.md` around lines 116 - 124, The fenced code block displaying the trust prompt example (starting with "This project provides 2 skills under .agents/skills or .kit/skills:") is missing a language identifier after the opening triple backticks, which causes the MD040 markdown linter to fail. Add a language identifier such as `text` or `shell` after the opening ``` to specify the code block language and satisfy the linter requirement.Source: Linters/SAST tools
🤖 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/compaction/compaction.go`:
- Around line 401-413: The isProtectedMessage function treats any message
containing skill content markers as protected without verifying the source or
role of the message, allowing users to inject these markers to prevent their
messages from being compacted. Modify the isProtectedMessage function to add a
role or source check that ensures only messages from trusted sources (such as
system or assistant messages, not user messages) are treated as protected
content. This prevents users from exploiting the protection mechanism by
injecting marker substrings to keep arbitrary history from being compacted.
In `@internal/skills/skills.go`:
- Around line 414-423: The LoadUserSkills function silently discards errors from
LoadSkillsFromDir calls by using underscore placeholders instead of checking the
error returns. This can mask permission denied, broken symlink, or transient I/O
errors that result in partial skill catalogs. Modify the LoadUserSkills function
to capture the error returns from both LoadSkillsFromDir calls (when loading
from the user home directory and global skills directory), check if errors occur
that are not ENOENT, and wrap any non-ENOENT errors with fmt.Errorf providing
context about which skill directory failed to load, then return the error up the
call chain or log it appropriately. This applies to all instances where
LoadSkillsFromDir is called in this code section.
In `@internal/skilltool/skilltool.go`:
- Around line 98-104: The check-and-set operation for preventing duplicate skill
activation is not atomic because the lock is released before the skill is
actually marked as activated. Move the t.mu.Unlock() call that currently occurs
after checking t.activated[name] to instead occur after the skill has been
written to the t.activated map (around line 134-136 where the activation
happens). This ensures the entire check to see if a skill is already activated
and the subsequent marking of it as activated happens within a single lock
section, preventing two concurrent calls from both passing the duplicate check
and both activating the same skill.
- Around line 108-113: The skill lookup loop iterating through t.provider() only
validates that s.Name matches the requested name, but does not check if the
skill is marked with DisableModelInvocation. Add an additional condition to the
if statement that checks the DisableModelInvocation flag on the skill object s
to ensure disabled skills cannot be activated. The if statement should verify
both that the name matches AND that the skill is not disabled before setting the
path and breaking.
---
Nitpick comments:
In `@internal/skilltool/skilltool_test.go`:
- Around line 74-84: Add a new test function following the pattern of
TestActivateSkill_UnknownSkill that validates the DisableModelInvocation
enforcement. Create a provider function that returns a skills.Skill with a
matching name but with DisableModelInvocation set to true, instantiate the tool
with New(), call tool.Run() with a request for that skill name, and assert that
the response contains an appropriate error message indicating the skill is
disabled. This ensures that skills marked as disabled cannot be activated even
when they exist in the provider.
In `@internal/trust/trust.go`:
- Around line 95-99: To optimize for concurrent reads in the Store type, upgrade
the mutex from sync.Mutex to sync.RWMutex in the Store struct definition. Then
in the IsTrusted method, replace the s.mu.Lock() call with s.mu.RLock() and
change the corresponding defer statement from defer s.mu.Unlock() to defer
s.mu.RUnlock(). This allows multiple goroutines to read the trusted map
simultaneously while maintaining exclusive write access for mutation operations
elsewhere in the code.
In `@pkg/kit/skills_spec_test.go`:
- Line 91: The `prompted` counter variable is declared and incremented but never
verified in an assertion, making it unused. Either add an assertion at the end
of the test to verify that `prompted` equals the expected count (likely 1 to
confirm the prompt was called exactly once), or remove the variable declaration
and any increment operations if it is not needed for the test validation.
In `@www/pages/cli/commands.md`:
- Around line 116-124: The fenced code block displaying the trust prompt example
(starting with "This project provides 2 skills under .agents/skills or
.kit/skills:") is missing a language identifier after the opening triple
backticks, which causes the MD040 markdown linter to fail. Add a language
identifier such as `text` or `shell` after the opening ``` to specify the code
block language and satisfy the linter requirement.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00cdb372-2299-4f14-a034-336adcc0b6d4
📒 Files selected for processing (23)
README.mdcmd/root.gocmd/skill_trust.gointernal/compaction/compaction.gointernal/compaction/compaction_test.gointernal/config/config.gointernal/extensions/api.gointernal/skills/prompt_builder_test.gointernal/skills/skills.gointernal/skills/skills_test.gointernal/skilltool/skilltool.gointernal/skilltool/skilltool_test.gointernal/trust/trust.gointernal/trust/trust_test.gopkg/kit/kit.gopkg/kit/skills.gopkg/kit/skills_spec_test.gowww/pages/cli/commands.mdwww/pages/cli/flags.mdwww/pages/configuration.mdwww/pages/extensions/capabilities.mdwww/pages/sdk/options.mdwww/pages/sdk/overview.md
- log (instead of discard) genuine errors from skill directory loads so permission/read failures no longer yield a silently partial catalog - make activate_skill dedup atomic by holding the lock across check and mark, preventing concurrent double-activation - reject activation of disable-model-invocation skills in the tool's runtime lookup, mirroring their catalog/enum exclusion - add regression test for disabled-skill activation
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.
Description
Brings Kit's skills subsystem into full compliance with the agentskills.io specification in a single coordinated sweep. Kit was already substantially aligned (3-tier progressive disclosure,
<available_skills>catalog,/skill:activation, runtime mutation SDK), so this PR closes the remaining 16 gaps clustered across read-side robustness, safety/policy, and cross-client polish.The highest-impact fixes address two real defects: catalog descriptions containing
<,>, or&previously produced malformed XML that some models refuse to parse, and a freshly cloned repo with.agents/skills/would silently inject its instructions into the system prompt oncd— a genuine prompt-injection vector. Both are now handled (XML escaping; a persisted project-trust gate). Skills authored for other clients (withlicense/compatibilityfields or the common unquoted-colondescription: Use when: …mistake) now load correctly, and skills missing a requireddescriptionare skipped with a logged warning instead of appearing undiscoverable in the catalog.The change is intentionally one PR because the pieces are tightly coupled — each new frontmatter field cascades through the parser struct, the SDK type alias, the extension bridge, catalog rendering, and the docs, and the compaction-protection tag must match the activation-time wrapper which must match the catalog-time stripping.
Fixes #65
Type of Change
What changed
Read-side robustness
name/description/compatibility/locationin the catalog; drop thefile://prefix on<location>(bare paths theread/fstools expect).description(logged warning); addSkill.Validate() []Diagnostic.license,compatibility,metadata,allowed-tools,disable-model-invocation, threaded throughskills.Skill,extensions.Skill,convertSkill, and the.kit.ymltemplate.description: Use when: …) and retry once.Discovery & precedence
~/.agents/skills/scope (now 4 canonical scopes).namewith explicit project > user precedence and collision warnings (was dedupe-by-path).--skills-dirnow scans the directory directly instead of appending.agents/.kitbeneath it (matches documented intent).Safety / policy
internal/trustpackage persists an allowlist at~/.config/kit/trusted-projects.json;Options.SkillTrustPromptcallback + interactive CLI prompt gate first-time project-skill loading. Defaults to load-without-prompting when no callback is set (back-compatible).<skill>/<skill_content>wrappers are preserved verbatim instead of being summarized away.Activation & SDK polish
internal/skilltoolactivate_skillMCP tool with enum-constrainedname, bundled-resource enumeration, and per-session dedup; registered only when ≥1 skill is loaded.<skill_resources>enumeration ofscripts//references//assets/on/skill:activation.--skill-disableflag,skill-disableconfig key,disable-model-invocationhonoring, plusKit.DisableSkill/EnableSkillSDK methods.Skill.BaseDir(),Skill.Resources(),Skill.Validate().Checklist
go fmt,go vet,golangci-lint runall clean)go test -race ./...)Additional Information
New files
internal/trust/{trust.go,trust_test.go}— persisted project-trust allowlistinternal/skilltool/{skilltool.go,skilltool_test.go}—activate_skillMCP toolcmd/skill_trust.go— interactive CLI trust promptpkg/kit/skills_spec_test.go— SDK-level coverage (direct--skills-dir, disable list, trust gate)Backward compatibility
Optionsfields (SkillsDisable,SkillTrustPrompt) and frontmatter fields are all optional; existing skills and SDK callers are unaffected.SkillTrustPromptdefaults tonil→ project skills load without prompting, preserving prior behavior. The CLI only prompts in interactive TTY sessions (skipped for--quiet, piped stdin, and one-shot positional prompts).--skills-dirsemantics change is a fix to match the documented/expected behavior; the flag was previously scanning<dir>/.agents/skillsand<dir>/.kit/skillsrather than<dir>itself.Verification
go build ./...,go vet, andgolangci-lint runclean (the pre-existingexamples/extensionsbuild error is unrelated and present onmaster).npm run buildvia tome).Summary by CodeRabbit
Release Notes
--skill-disableflag while keeping them accessible via/skill: