Limit Pi skills to explicit prompt references#81
Merged
glittercowboy merged 1 commit intomainfrom Apr 28, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughClaude skill discovery and selection are now prompt-aware: the Pi executor only discovers and forwards Claude skill paths when the prompt contains explicit Changes
Sequence DiagramsequenceDiagram
participant User as "Prompt Input"
participant RefCheck as "PromptHasClaudeSkillReference()"
participant Discover as "Skill Discovery (FS)"
participant Select as "SelectClaudeSkillsForPrompt()"
participant Executor as "runPiExecutor()"
participant Pi as "Pi Executor"
User->>RefCheck: send prompt text
RefCheck-->>Executor: boolean (hasReference)
alt hasReference
Executor->>Discover: enumerate available skills
Discover->>Select: provide available skills + prompt
Select->>Select: extract tokens, match names, dedupe
Select-->>Executor: return filtered skills (paths)
Executor->>Pi: invoke with --skill flags for each path
else noReference
Executor->>Executor: keep skillPaths = nil
Executor->>Pi: invoke without --skill flags
end
Pi-->>Executor: return execution result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~23 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 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: 1
🧹 Nitpick comments (1)
internal/skills/discovery_test.go (1)
104-130: Add a regression case for path-like slash tokens.Current coverage checks plain text and explicit tokens, but not prompts containing filesystem-like tokens (e.g.,
/tmp/file) that must not count as skill references.✅ Suggested test addition
+func TestPromptHasClaudeSkillReferenceIgnoresPathLikeTokens(t *testing.T) { + if PromptHasClaudeSkillReference("open /tmp/notes.md") { + t.Fatal("expected false for path-like token without explicit skill directive") + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/skills/discovery_test.go` around lines 104 - 130, Add a regression test to ensure SelectClaudeSkillsForPrompt does not treat filesystem-like slash tokens as skill references: create a test (e.g., TestSelectClaudeSkillsForPromptIgnoresPathLikeTokens) with an available skills slice including "project-skill" and others, call SelectClaudeSkillsForPrompt with a prompt containing path-like strings such as "/tmp/file" or "/var/log/app.log" alongside a real explicit skill token (e.g., "/skill:project-skill"), and assert that only the explicit token(s) are selected (len(got) and got[*].Path checks) so path-like tokens are ignored; reference the SelectClaudeSkillsForPrompt function and update discovery_test.go with this new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/skills/discovery.go`:
- Line 23: The current regex in skillRefPattern is too permissive and matches
any slash-prefixed token (e.g., /tmp); change skillRefPattern so it only matches
when the slash is followed by an explicit directive "skill:" or "namespace:"
(i.e., require "/skill:" or "/namespace:" after the slash) so that the discovery
logic that uses skillRefPattern (the skill discovery routine around the current
usage at lines ~72-87) only triggers for explicit skill/namespace directives;
update the pattern accordingly and run existing tests to ensure no other callers
rely on the old permissive matching.
---
Nitpick comments:
In `@internal/skills/discovery_test.go`:
- Around line 104-130: Add a regression test to ensure
SelectClaudeSkillsForPrompt does not treat filesystem-like slash tokens as skill
references: create a test (e.g.,
TestSelectClaudeSkillsForPromptIgnoresPathLikeTokens) with an available skills
slice including "project-skill" and others, call SelectClaudeSkillsForPrompt
with a prompt containing path-like strings such as "/tmp/file" or
"/var/log/app.log" alongside a real explicit skill token (e.g.,
"/skill:project-skill"), and assert that only the explicit token(s) are selected
(len(got) and got[*].Path checks) so path-like tokens are ignored; reference the
SelectClaudeSkillsForPrompt function and update discovery_test.go with this new
test.
🪄 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 Plus
Run ID: 68fdaedb-cefa-4e7a-a048-9b3127f86888
📒 Files selected for processing (4)
internal/session/actor.gointernal/session/actor_test.gointernal/skills/discovery.gointernal/skills/discovery_test.go
glittercowboy
force-pushed
the
codex/skill-prompt-latency
branch
from
5a3fb85 to
f21f264
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/session/actor.go`:
- Around line 610-615: The code is checking for Claude skill references against
the composed "prompt" (which includes injected context) instead of the raw user
prompt; update calls to use the user-facing prompt variable (e.g., task.Prompt
or userPrompt) when calling skills.PromptHasClaudeSkillReference and
skills.SelectClaudeSkillsForPrompt so token-gating only considers the user's
original text; leave skills.DiscoverClaudeSkills as-is. Ensure you reference the
user prompt variable consistently in both checks to prevent context-injected
"/skill:" matches from enabling discovery.
🪄 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 Plus
Run ID: 9f40e9d5-99ee-4015-824e-b43ce596e8a4
📒 Files selected for processing (4)
internal/session/actor.gointernal/session/actor_test.gointernal/skills/discovery.gointernal/skills/discovery_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/skills/discovery.go
- internal/skills/discovery_test.go
glittercowboy
force-pushed
the
codex/skill-prompt-latency
branch
from
f21f264 to
d18c014
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/skills/discovery.go (1)
23-23:⚠️ Potential issue | 🟠 MajorRestrict token matching to
/skill:and/namespace:only.Line 23 still matches arbitrary
/<prefix>:...tokens (for example,/foo:bar), which can trigger skill discovery on plain prompts that were meant to stay catalog-free.Suggested fix
-var skillRefPattern = regexp.MustCompile(`(?:^|\s)/(?:skill:([A-Za-z0-9:_-]+)|([A-Za-z0-9_-]+:[A-Za-z0-9:_-]+))`) +var skillRefPattern = regexp.MustCompile(`(?:^|\s)/(?:skill:([A-Za-z0-9:_-]+)|(namespace:[A-Za-z0-9:_-]+))`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/skills/discovery.go` at line 23, The regex stored in skillRefPattern currently matches any "/prefix:token" and should be tightened to only trigger for "/skill:" and "/namespace:"; update the regexp in internal/skills/discovery.go (skillRefPattern) so it only recognizes leading "/skill:..." or "/namespace:..." tokens (preserving the allowed character set [A-Za-z0-9:_-] for the token) and keeps the same word-boundary or leading-space behavior (i.e., still anchored with (?:^|\s)/).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/skills/discovery.go`:
- Line 23: The regex stored in skillRefPattern currently matches any
"/prefix:token" and should be tightened to only trigger for "/skill:" and
"/namespace:"; update the regexp in internal/skills/discovery.go
(skillRefPattern) so it only recognizes leading "/skill:..." or "/namespace:..."
tokens (preserving the allowed character set [A-Za-z0-9:_-] for the token) and
keeps the same word-boundary or leading-space behavior (i.e., still anchored
with (?:^|\s)/).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f60498f0-006f-4a5a-996c-534e83502fef
📒 Files selected for processing (4)
internal/session/actor.gointernal/session/actor_test.gointernal/skills/discovery.gointernal/skills/discovery_test.go
glittercowboy
force-pushed
the
codex/skill-prompt-latency
branch
from
d18c014 to
565f780
Compare
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:project-skill.Verification
go test ./... && go build ./...Release
daemon/v0.3.17.Summary by CodeRabbit