Add basic SkillService implementation and API handlers #3804
Conversation
Implement the SkillService with CRUD operations and wire it into the API server, replacing the stub handlers for list, install, uninstall, and getSkillInfo endpoints. The service lives in pkg/skills/skillsvc/ (sub-package) to avoid an import cycle between pkg/skills and pkg/storage. It delegates to the existing SkillStore for persistence and validates all inputs via ValidateSkillName before any store operations. Server wiring creates a default SkillStore in createDefaultManagers() and closes it on shutdown via the cleanup() method. The validate, build, and push endpoints remain as 501 stubs for future work. Closes #3741 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
github-actions
bot
added
size/XL
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3804 +/- ##
==========================================
+ Coverage 66.81% 66.92% +0.10%
==========================================
Files 438 439 +1
Lines 43321 43434 +113
==========================================
+ Hits 28946 29067 +121
+ Misses 12131 12120 -11
- Partials 2244 2247 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I brought up some of the issues below in Slack but I'll summarise here for the wider audience.
This PR builds on top of an API contract that may need some tweaks. Currently, as it is, is a mix between REST and RPC like conventions which is inconsistent with the rest of ToolHives API.
I believe how other ToolHive resources do it:
- Groups:
POST / (create),DELETE /{name} (delete),GET / (list),GET /{name} (get) - Workloads:
POST / (create),DELETE /{name} (delete),GET / (list),GET /{name} (get)
How skills does it:
POST /api/v1beta/skills/install (create)- should bePOST /api/v1beta/skills/(although it was mentioned the issue on long running requests)POST /api/v1beta/skills/uninstall (delete)- should beDELETE /api/v1beta/skills/{name}orDELETE /api/v1beta/skills/{name}?scope=XGET /api/v1beta/skills/ (list)- fineGET /api/v1beta/skills/{name} (get)- fine
Install and uninstall seems like CREATE and DELETE with verb-named endpoints. validate, build and push aren't really REST (not CRUD on the skills resources), however given its probably more complexity and hassle to support RPC for those, maybe they can be done as sub-resources on the skills resources? Something like /skills/{id}/builds etc? Am aware we won't be able to make it perfect
There is also a route conflict risk between the GET /api/v1beta/skills{name} and GET /api/v1beta/skills/install if someone names a skill "install", "uninstall", "validate", "build" or "push". Doing some googling, Chi may resolve this by preferring statics routes over parametersed ones, but its a collision none the less in standard REST patterns.
Apologies for the late response on the skills stuff in general, I've not taken a look at any of it until now due to other priorities.
| skill, err := s.store.Get(ctx, opts.Name, skills.ScopeUser, "") | ||
| if err != nil { | ||
| if errors.Is(err, storage.ErrNotFound) { | ||
| return &skills.SkillInfo{Installed: false}, nil |
There was a problem hiding this comment.
Is this returning installed: false in the case of a not found? Shouldn't we respond with 404 instead?
This is inconsistent with:
- REST semantics (GET a resource that doesn't exist = 404)
- The uninstall endpoint in this same PR, which correctly returns 404 for missing skills
- ToolHive's own workloads/groups APIs, which return 404 for missing resources
| return nil, httperr.WithCode(err, http.StatusBadRequest) | ||
| } | ||
|
|
||
| skill, err := s.store.Get(ctx, opts.Name, skills.ScopeUser, "") |
There was a problem hiding this comment.
We hardcode to ScopeUser? but on other endpoints we respect the scope from the request (with a default fallback)? A skill installed with scope: "project" would show up in GET /skills?scope=project but not in GET /skills/{name}? Is that right?
There was a problem hiding this comment.
Additionally, we don't seem to do scope validation? So someone could send ?scope=foobar and its silently accepted because the defaultScope() function just checks if the scope string is empty?
| } | ||
|
|
||
| // Uninstall removes an installed skill. | ||
| func (s *service) Uninstall(ctx context.Context, opts skills.UninstallOptions) error { |
There was a problem hiding this comment.
I think Uninstall should use DELETE /skills/{name} or something similar. Otherwise we break HTTP method semantics (POST is not idempotent, but uninstall should be) and API caches can't distinguish between install and uninstall (both are POST to different sub-paths)
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusCreated) |
There was a problem hiding this comment.
nit: 201 is good, but typically you'd also respond with a Location header too to let clients know where to find the skill thats been created without having to parse the response body.
| // supported and will be added when InfoOptions gains a Scope field. | ||
| func (s *service) Info(ctx context.Context, opts skills.InfoOptions) (*skills.SkillInfo, error) { | ||
| if err := skills.ValidateSkillName(opts.Name); err != nil { | ||
| return nil, httperr.WithCode(err, http.StatusBadRequest) |
There was a problem hiding this comment.
nit: we're leaking http concerns into svc layer. In other parts of toolhive we define typed errors and the middleware/handler maps them to http codes. Also the handler that calls this function seems to already do its own validation for empty names, so this line maybe redundant?
| return httperr.WithCode(fmt.Errorf("not implemented"), http.StatusNotImplemented) | ||
| func (s *SkillsRoutes) uninstallSkill(w http.ResponseWriter, r *http.Request) error { | ||
| var req uninstallSkillRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { |
There was a problem hiding this comment.
nit: there's no check for empty body. so if there's an empty body we give a 400 invalid request body. this is ok, but slightly misleading, a better check for empty body with an error of 400 request body is required is more helpful
2 participants
Summary
This PR delivers the service layer and API wiring needed to make the skills
endpoints functional, building on the SQLite SkillStore merged in #3801.
Why a sub-package?
pkg/skillsandpkg/storagehave a circular import(storage uses skills types, skills needs the store interface). The service
implementation lives in
pkg/skills/skillsvc/to break this cycle cleanly.What each endpoint does now
GET /api/v1beta/skills?scope=filterPOST /api/v1beta/skills/installPOST /api/v1beta/skills/uninstallGET /api/v1beta/skills/{name}{installed: false}POST /api/v1beta/skills/validatePOST /api/v1beta/skills/buildPOST /api/v1beta/skills/pushServer lifecycle changes
createDefaultManagers()now opens aSkillStoreviasqlite.NewDefaultSkillStore()and passes it to
skillsvc.New(). The store reference is tracked as anio.Closeron the
Serverstruct and closed incleanup()— before unix socket removal, evenon crash paths. When callers inject their own service via
WithSkillManager(), theyown the store lifecycle (documented in the method comment).
Service design decisions
{installed: false}for missing skills, not 404comes when
InfoOptionsgains aScopefieldValidateSkillNamebefore touching the storeScopeUservia extracteddefaultScope()helperHandler patterns
Handlers follow the same pattern as
groups.go: decode JSON → validate requiredfields → call service → encode response. Error mapping uses
httperr.WithCode()for 4xx and lets
ErrorHandlersanitize 5xx to generic messages.Closes #3741
Large PR Justification
The PR is ~980 lines total but only ~200 lines are production code (across 3 modified
files and 1 new file). The remaining ~780 lines are test files:
pkg/skills/skillsvc/skillsvc_test.go(367 lines) — service unit testspkg/api/v1/skills_test.go(380 lines) — handler testsdocs/server/— auto-generated swagger docsThe production code cannot be split further — the service implementation, API handlers,
and server wiring are tightly coupled (handlers call the service, server creates the
service). Shipping them separately would leave broken 501 stubs between PRs.
Test plan
validation errors, error propagation, scope defaulting, and version passthrough
covering JSON decoding, empty/malformed input, status code mapping (400/404/409/501),
scope+version passthrough, and response format validation
task lint-fixclean,task buildcompiles, race detector passes🤖 Generated with Claude Code