fix(skills): install by name resolves catalog source - #1038
Conversation
The example's tmp/ holds gitignored browser-agent runtime artifacts, like agents/ already excluded above it. markdownlint . was scanning them and blocking every commit via the pre-commit hook.
`infer skills install <name>` hardcoded the download location as <repo>/tree/main/skills/<name>, so a catalog skill whose body lives in another repo could not be installed by name. adl (moved to inference-gateway/adl/.agents/skills/adl) failed with "no files found under inference-gateway/skills/skills/adl @ main". Resolve a bare name against the catalog entry's `source` before falling back to the shorthand convention. URLs and <org>/<skill> forms are unchanged; an unreachable or silent catalog falls back as before.
|
@infer Can you review this PR? use /ponytail:ponytail /go /go-concurrency /go-spec-reviewer |
|
Model: Todos
Pull Request✅ Infer Result: SuccessLet me also check the I've read the full diff, all changed files, and the surrounding code. Here is my review. Review: PR #1038 —
|
| Path | Fixed? |
|---|---|
infer skills install adl (CLI) |
✅ Yes — installSkill calls ResolveInstallURL |
/skill:adl in chat (activation) |
❌ No — DownloadSkill uses SkillTreeURL |
The fix: The pattern already exists in ResolveInstallURL — reuse it via c.Lookup:
func (c *CatalogClient) DownloadSkill(ctx context.Context, name string) (string, error) {
destBase, err := dynamicSkillsDir()
if err != nil {
return "", err
}
sourceURL := SkillTreeURL(c.repository, name)
if entry, ok := c.Lookup(ctx, name); ok && entry.Source != "" {
sourceURL = entry.Source
}
installer := NewInstaller(c.repository)
absPath, err := installer.InstallFromGitHub(ctx, sourceURL, destBase, false)
if err != nil {
return "", fmt.Errorf("failed to download skill %q from catalog: %w", name, err)
}
return filepath.Join(absPath, skillEntryFile), nil
}One extra lookup, zero new infrastructure. Add a test case for adl in the existing download test pattern (or a new table test) that confirms the source URL is picked up.
Go idioms (good)
-
(string, bool)return pattern inResolveInstallURL: consistent with the existingLookupsignature and standard Go comma-ok idiom. This is correct and matches the Go skill guidance. -
Early guard on
/and:at the top ofResolveInstallURL: correctly scopes the method to bare skill names only, leavingExpandShorthandin charge of everything else. This is the idiomatic "fail fast, small function" approach. -
strings.ContainsAny(input, "/:")for the guard: concise and correct. One line, exactly right for ponytail rung 6. -
catalogEntrystruct — addingSource stringwith ajson:"source"tag is the simplest possible schema change. No new types, noomitemptydebate required (empty stringok=falsein the consumer). Good. -
sourcedIndextest constant — uses string concatenation to build JSON inline, avoiding a separate testdata file. This follows the existingtwoSkillIndexpattern. Ponytail-compatible. -
The
ResolveInstallURLmethod on*CatalogClient— no pointer aliasing issue becausec.Lookuponly reads the index (thec.muprotects the slice). Correct.
Concurrency (good, nothing to change)
ResolveInstallURLcallsc.Lookup, which callsc.Index, which holdsc.mu.Lock(). All accesses toc.indexare serialized. Correct.installSkillcreates a freshCatalogClientper invocation — no shared state between runs.Discover(which callsDownloadSkill) runs from the TUI's event loop, one-goroutine-at-a-time for message processing. No concurrent mutations.- All methods propagate
ctxcorrectly; no leaked goroutines.
Ponytail review
Climbing the ladder:
- Does this need to exist? Yes — skills with external sources are uninstallable by name. The bug is real.
- Already in this codebase? Yes —
c.Lookupwas already there,ParseGitHubTreeURLalready processes the kind of URL the catalog source provides. No new URL-parsing. - Stdlib does it?
strings.ContainsAny,strings.Split,url.Parse(already inInstallFromGitHub) — all stdlib. - Native platform? N/A.
- Already-installed dep? N/A.
- One line? Not quite one line, but five lines for the method body is close. Rung 7.
- Minimum code that works. Yes.
Ponytail issues:
-
Incomplete fix (see Critical section above):
DownloadSkillis the other caller that needs the same pattern. The PR description states "For in-repo skills the catalog source equals what shorthand already produced, so their behavior is unchanged" — which is true forinstallSkill, butDownloadSkillwas missed entirely. The smallest change in the wrong place isn't lazy, it's a second bug. -
.markdownlintignoreuses em dashes (—) in comment lines (pre-existing on line 10, new on line 12). The repo rules forbid em/en dashes. While this is not introduced by the main fix commit, the first commit (chore(lint)) should have used plain hyphens to match repo convention. Minor — fix with a follow-upchore(lint): replace em dashes with hyphens in markdownlintignore comments. -
go-spec-reviewerwas invoked but this PR is already implemented code, not a design spec. The skill's description says "Review a Go design spec before implementation begins." Not applicable here — this is a code review, not a spec review. The other three skills (ponytail, go, go-concurrency) are fully relevant and applied above.
Proposals
Required before merge:
- Fix
DownloadSkillincatalog.go:331-344to resolve from the catalog source when available (diff above in the Critical section).
Nice-to-have (not blocking):
-
Replace em dashes with hyphens in
.markdownlintignorecomments (both the pre-existing one and the new one) to match the repo style guide. -
When fixing
DownloadSkill, add a test case incatalog_test.gothat exercises the source-based download path — either a newTestDownloadSkill_SourceURLor extend the table-test pattern used byTestResolveInstallURL. Usehttptest.Serverfor the catalog and a separatehttptest.Serverfor the GitHub API tree response to verify the resolved URL is used.
Summary of changed files
| File | Lines | Assessment |
|---|---|---|
.markdownlintignore |
+1/-0 | Fine (minor: uses em dashes) |
cmd/skills.go |
+8/-2 | Good — help text updated, ResolveInstallURL wired in correctly |
internal/services/skills/catalog.go |
+17/-0 | Good — struct field + new method; DownloadSkill needs same fix |
internal/services/skills/catalog_test.go |
+33/-0 | Good — clean table test; add DownloadSkill coverage alongside |
The review has been completed and delivered above. All four tasks are marked done:
- ✅ Read the PR diff
- ✅ Read all changed files for thorough review
- ✅ Reviewed against Go idioms, concurrency patterns, and ponytail principles
- ✅ Written final review with findings and proposals
The key finding: DownloadSkill in catalog.go needs the same source-resolution fix applied to installSkill — the progressive-discovery/activation path (/skill:adl in chat) remains broken for externally-sourced skills. The diff for the fix is in the review above (a one-liner using c.Lookup).
Model: ollama_cloud/deepseek-v4-flash:preview · Exit Code: 0 · Duration: 4m 1s
Tokens: 419,308 in · 7,057 out · 426,365 total (11 requests)
Tool calls: 17 total · 100% success rate
Traces
Session: 1786116900-5774c847
session (standard, success) 3m53s
├── chat ollama_cloud/deepseek-v4-flash:preview 4.5s
│ ╰── POST /v1/chat/completions [inference-gateway] 4.4s
│ ╰── HTTP POST [inference-gateway] 4.4s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 4.4s
│ ╰── HTTP POST [inference-gateway] 4.4s
├── execute_tool TodoWrite call_cj39qkvm 85µs
├── chat ollama_cloud/deepseek-v4-flash:preview 2.8s
│ ╰── POST /v1/chat/completions [inference-gateway] 2.8s
│ ╰── HTTP POST [inference-gateway] 2.8s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 2.8s
│ ╰── HTTP POST [inference-gateway] 2.8s
├── execute_tool Bash call_xignzbr6 4ms
├── execute_tool Bash call_i2qv9poh 5ms
├── chat ollama_cloud/deepseek-v4-flash:preview 4.9s
│ ╰── POST /v1/chat/completions [inference-gateway] 4.9s
│ ╰── HTTP POST [inference-gateway] 4.9s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 4.9s
│ ╰── HTTP POST [inference-gateway] 4.9s
├── execute_tool TodoWrite call_oa271tqa 71µs
├── chat ollama_cloud/deepseek-v4-flash:preview 5.3s
│ ╰── POST /v1/chat/completions [inference-gateway] 5.3s
│ ╰── HTTP POST [inference-gateway] 5.3s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 5.3s
│ ╰── HTTP POST [inference-gateway] 5.3s
├── execute_tool Read call_pu2m1acy 190µs
├── execute_tool Read call_ec4tuu94 303µs
├── execute_tool Read call_v8upwq3i 218µs
├── execute_tool Read call_jd697rfi 172µs
├── execute_tool Read call_q0mjcwdn 262µs
├── chat ollama_cloud/deepseek-v4-flash:preview 34.5s
│ ╰── POST /v1/chat/completions [inference-gateway] 34.5s
│ ╰── HTTP POST [inference-gateway] 34.5s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 34.5s
│ ╰── HTTP POST [inference-gateway] 34.5s
├── execute_tool Grep call_4nnay829 364µs
├── execute_tool Grep call_540cc78i 104ms
├── chat ollama_cloud/deepseek-v4-flash:preview 4.6s
│ ╰── POST /v1/chat/completions [inference-gateway] 4.6s
│ ╰── HTTP POST [inference-gateway] 4.6s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 4.6s
│ ╰── HTTP POST [inference-gateway] 4.6s
├── execute_tool Read call_qvuk7wmr 246µs
├── chat ollama_cloud/deepseek-v4-flash:preview 14.8s
│ ╰── POST /v1/chat/completions [inference-gateway] 14.8s
│ ╰── HTTP POST [inference-gateway] 14.8s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 14.8s
│ ╰── HTTP POST [inference-gateway] 14.8s
├── execute_tool Grep call_0pb647d8 87ms
├── execute_tool Read call_cpmokf8d 348µs
├── chat ollama_cloud/deepseek-v4-flash:preview 21.9s
│ ╰── POST /v1/chat/completions [inference-gateway] 21.9s
│ ╰── HTTP POST [inference-gateway] 21.9s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 21.9s
│ ╰── HTTP POST [inference-gateway] 21.9s
├── execute_tool TodoWrite call_bssgnjhl 65µs
├── chat ollama_cloud/deepseek-v4-flash:preview 28.3s
│ ╰── POST /v1/chat/completions [inference-gateway] 28.3s
│ ╰── HTTP POST [inference-gateway] 28.3s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 28.3s
│ ╰── HTTP POST [inference-gateway] 28.3s
├── execute_tool TodoWrite call_7mohvq5m 60µs
├── chat ollama_cloud/deepseek-v4-flash:preview 1m29s
│ ╰── POST /v1/chat/completions [inference-gateway] 1m29s
│ ╰── HTTP POST [inference-gateway] 1m29s
│ ╰── POST /proxy/:provider/*path [inference-gateway] 1m29s
│ ╰── HTTP POST [inference-gateway] 1m29s [error: *errors.errorString]
├── execute_tool TodoWrite call_thms7bua 81µs
╰── chat ollama_cloud/deepseek-v4-flash:preview 21.2s
╰── POST /v1/chat/completions [inference-gateway] 21.2s
╰── HTTP POST [inference-gateway] 21.2s
╰── POST /proxy/:provider/*path [inference-gateway] 21.2s
╰── HTTP POST [inference-gateway] 21.2s
Stats
Tool Calls
╭───────────┬───────┬───────┬──────╮
│ Tool │ Calls │ Fail% │ Avg │
├───────────┼───────┼───────┼──────┤
│ Read │ 7 │ 0% │ 0ms │
│ TodoWrite │ 5 │ 0% │ 0ms │
│ Grep │ 3 │ 0% │ 64ms │
│ Bash │ 2 │ 0% │ 5ms │
╰───────────┴───────┴───────┴──────╯
Token Usage
╭────────────────────────────────────────┬────────┬────────┬────────────┬────────┬──────╮
│ Model │ Prompt │ Cached │ Completion │ Total │ Cost │
├────────────────────────────────────────┼────────┼────────┼────────────┼────────┼──────┤
│ ollama_cloud/deepseek-v4-flash:preview │ 419308 │ 0 │ 7057 │ 426365 │ - │
╰────────────────────────────────────────┴────────┴────────┴────────────┴────────┴──────╯
Sessions
╭───────────┬──────────┬──────────╮
│ Execution │ Mode │ Sessions │
├───────────┼──────────┼──────────┤
│ headless │ standard │ 1 │
╰───────────┴──────────┴──────────╯
Triggered by edenreich · Infer Action · OSS
|
@infer Can you address the review comment above? use /go /ponytail:ponytail |
|
Model: Todos
Pull Request✅ Infer Result: SuccessClean working tree, all changes committed and pushed to PR #1038. SummaryAddressed the review comment's critical finding and two nice-to-have items: Critical fix applied
The change is minimal (7 lines): sourceURL := SkillTreeURL(c.repository, name)
if entry, ok := c.Lookup(ctx, name); ok && entry.Source != "" {
sourceURL = entry.Source
}Plus
Verification
Model: Tokens: 1,270,725 in · 8,134 out · 1,278,859 total (21 requests) Tool calls: 27 total · 100% success rate TracesStatsTriggered by edenreich · Infer Action · OSS |
…k to SkillTreeURL The progressive-discovery / activation path (/skill:adl in chat) was still using the hardcoded SkillTreeURL convention instead of consulting the catalog entry's source. Reuses the same c.Lookup + entry.Source pattern already proven in installSkill. Also replaces em dash with plain hyphen in .markdownlintignore comment (repo convention).
|
🎉 This PR is included in version 0.164.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Summary
infer skills install <name>resolved the download location purely by convention —SkillTreeURLbuilds<configured-repo>/tree/main/skills/<name>— and never consulted the catalog'ssource. Any catalog skill whose body lives outsideinference-gateway/skills(or outside theskills/<name>/layout) was therefore uninstallable by name.This surfaced when the adl skill moved to
inference-gateway/adl/.agents/skills/adl: the catalog and registry updated correctly, butinfer skills install adlfailed with:Fix
Resolve a bare skill name against the catalog entry's
source(already a/tree/<ref>/<path>URL thatInstallFromGitHubaccepts) before falling back to the shorthand convention:catalogEntrynow parsessource.CatalogClient.ResolveInstallURLmaps a bare name to its catalogsource. Inputs that already carry their own location (a full URL or an<org>/<skill>shorthand) and names the catalog does not list returnok=false, leaving today's shorthand expansion in charge.installSkillconsults it beforeInstallFromGitHub.For in-repo skills the catalog
sourceequals what shorthand already produced, so their behavior is unchanged. When the catalog is unreachable, install falls back exactly as before.Verification
go test ./internal/services/skills/... ./cmd/...— green; new table testTestResolveInstallURLcovers external-source, in-repo, unknown-name, full-URL, and<org>/<skill>inputs.golangci-lint run— 0 issues.infer skills install adl→ installs.infer/skills/adl/SKILL.mdfrominference-gateway/adl.infer skills install skill-creator→ still installs frominference-gateway/skills(no regression).Notes
chore(lint)) is unrelated hygiene: the pre-commit hook'smarkdownlint .was blocking all commits on gitignoredexamples/telegram-channel/tmp/runtime artifacts; added them to.markdownlintignorebeside the existingagents/exclusion.inference-gateway/skills(the catalog is correct) or the registry.fix:scope, so noinference-gateway/docsticket is required.