Infer targets from host-specific skill paths#144
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic target inference for skills based on their relative file paths, allowing skills located in specific directories (e.g., .cursor/skills) to be automatically associated with their respective targets. The changes include new logic in the discovery process to fallback to path-based inference when targets are not explicitly defined in frontmatter, along with caching for known target names to improve performance. Feedback focuses on making the path inference more flexible by removing restrictive directory checks and optimizing the project target lookup by caching the configuration and removing redundant uniqueness checks.
| if len(parts) < 3 || parts[1] != "skills" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This check is quite restrictive as it assumes host-specific skills always follow the host/skills/... pattern. If a target is configured with a custom project path that doesn't have skills as the second component (e.g., my-target/myskill), inference will fail even if it matches the configuration. Consider making this check more flexible or relying on the configuration-based matching first to support custom target paths.
| targets := make([]string, 0, 2) | ||
| seen := make(map[string]bool) | ||
|
|
||
| for targetName, pathCfg := range config.ProjectTargets() { | ||
| if targetName == "" || pathCfg.Path == "" { | ||
| continue | ||
| } | ||
|
|
||
| targetPath := filepath.ToSlash(pathCfg.Path) | ||
| if matchesHostSkillPath(relPath, targetName, targetPath) && !seen[targetName] { | ||
| seen[targetName] = true | ||
| targets = append(targets, targetName) | ||
| } | ||
| } | ||
|
|
||
| return targets |
There was a problem hiding this comment.
The seen map is redundant here because config.ProjectTargets() returns a map[string]TargetConfig, ensuring that each targetName in the loop is already unique. Additionally, calling config.ProjectTargets() inside this function (which is called for every skill discovered) is inefficient as it reconstructs the map and iterates over all specs every time. Consider caching the project targets map using a sync.Once pattern, similar to knownTargetNameSet.
func inferTargetsFromProjectPaths(relPath string) []string {
targets := make([]string, 0, 2)
// Note: config.ProjectTargets() should ideally be cached to avoid repeated map allocations
for targetName, pathCfg := range config.ProjectTargets() {
if targetName == "" || pathCfg.Path == "" {
continue
}
targetPath := filepath.ToSlash(pathCfg.Path)
if matchesHostSkillPath(relPath, targetName, targetPath) {
targets = append(targets, targetName)
}
}
return targets
}
|
Superseded by the latest update: #144 (comment) |
There was a problem hiding this comment.
Pull request overview
This PR updates skill discovery so that when a nested SKILL.md omits targets: frontmatter, targets can be inferred from host-specific skill directory layouts (e.g. .cursor/skills/..., .factory/skills/..., openclaw/skills/...) to prevent cross-host syncing.
Changes:
- Infer
DiscoveredSkill.Targetsfrom known host-specific skill source paths during full discovery (while keeping Lite discovery behavior unchanged). - Apply inferred targets both for normal discovered skills and for ignored skills when
includeIgnoredis enabled. - Add unit + integration tests covering inferred targets and ensuring Lite mode does not infer.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/integration/skill_targets_test.go | Adds an integration test ensuring host-packaged skills only sync to the intended configured target. |
| internal/sync/discover_walk.go | Implements path-based target inference during discovery and adds helper functions/caching. |
| internal/sync/discover_test.go | Adds unit tests for inferred targets (and confirms Lite mode keeps Targets == nil). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parts := strings.Split(relPath, "/") | ||
| if len(parts) < 3 || parts[1] != "skills" { | ||
| return nil | ||
| } | ||
|
|
||
| // Prefer explicit project paths from known targets (e.g. ".factory/skills"). | ||
| targets := inferTargetsFromProjectPaths(relPath) | ||
| if len(targets) > 0 { | ||
| return targets | ||
| } | ||
|
|
| func inferTargetsFromProjectPaths(relPath string) []string { | ||
| targets := make([]string, 0, 2) | ||
| seen := make(map[string]bool) | ||
|
|
||
| for targetName, pathCfg := range config.ProjectTargets() { | ||
| if targetName == "" || pathCfg.Path == "" { | ||
| continue | ||
| } | ||
|
|
||
| targetPath := filepath.ToSlash(pathCfg.Path) | ||
| if matchesHostSkillPath(relPath, targetName, targetPath) && !seen[targetName] { | ||
| seen[targetName] = true | ||
| targets = append(targets, targetName) | ||
| } | ||
| } |
| func inferTargetsFromProjectPaths(relPath string) []string { | ||
| targets := make([]string, 0, 2) | ||
| seen := make(map[string]bool) | ||
|
|
||
| for targetName, pathCfg := range config.ProjectTargets() { | ||
| if targetName == "" || pathCfg.Path == "" { | ||
| continue | ||
| } | ||
|
|
||
| targetPath := filepath.ToSlash(pathCfg.Path) | ||
| if matchesHostSkillPath(relPath, targetName, targetPath) && !seen[targetName] { | ||
| seen[targetName] = true | ||
| targets = append(targets, targetName) | ||
| } | ||
| } | ||
|
|
||
| return targets | ||
| } |
|
@runkids I pushed an update after the bot review comments. What changed:
Re-ran locally: go test ./internal/sync
go test ./internal/...
go build -o bin/skillshare ./cmd/skillshare
SKILLSHARE_TEST_BINARY=/Users/larry_1/Opensource/skillshare/bin/skillshare go test ./tests/integration -run TestSkillTargetsAll passed locally. Current Please review this PR when possible. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Thanks! Will ship the next version. |
Summary
Fixes target filtering for multi-host skill repos that package host-specific skill trees such as:
.cursor/skills/....factory/skills/...openclaw/skills/....agents/skills/...When a nested
SKILL.mdomitstargets:frontmatter, discovery currently leavesTargetsasnil. During sync,nilmeans "sync to every target", so a host-specific skill can be copied into unrelated tools. This makes the same logical skill appear multiple times in tools that index nestedSKILL.mdfiles, especially with gstack-style repos.Changes
Targets == niland sync everywhere.DiscoverSourceSkillsLitebehavior unchanged, because lite discovery is used by commands that do not need target filtering.*/skills/*fallback, so paths such as.skills/<skill>are covered;config.GroupedProjectTargets()so inference returns the canonical grouped target, e.g..agents/skills->universal.Linked Issue
Fixes #145
Tests
Current
gh pr checksoutput only reports the Vercel deployment authorization failure for the fork PR. The local checks above pass.