diff --git a/.claude/skills/api-v2-routes/SKILL.md b/.claude/skills/api-v2-routes/SKILL.md new file mode 100644 index 000000000..234f75a3b --- /dev/null +++ b/.claude/skills/api-v2-routes/SKILL.md @@ -0,0 +1,168 @@ +--- +name: api-v2-routes +description: Use when adding or changing a resource on the Huma-backed /api/v2 API (new endpoints, porting a v1 resource, editing pkg/routes/api/v2/). Covers per-operation Huma handlers, the shared envelopes, error/auth bridging, REST verb conventions, and what's automatic. +user-invocable: true +--- + +# Adding /api/v2 routes for a CRUDable resource + +`/api/v2` is served by [Huma v2](https://github.com/danielgtaylor/huma) mounted on an Echo group via the vendored `pkg/modules/humaecho5` adapter. Unlike v1's generic `WebHandler`, each operation is a typed Huma handler registered explicitly. The handlers are thin: they pull auth off the context, call the same `pkg/web/handler.Do*` functions v1 uses, and translate domain errors into RFC 9457 responses. + +**Reference implementation:** `pkg/routes/api/v2/labels.go` is the canonical example — copy its shape. Shared envelopes live in `pkg/routes/api/v2/types.go`; the auth/error bridge in `pkg/routes/api/v2/errors.go`; config in `pkg/routes/api/v2/huma.go`. + +## Prerequisite: the model must be CRUDable + +v2 handlers call `handler.DoReadAll/DoReadOne/DoCreate/DoUpdate/DoDelete`, which invoke the model's `Can*` methods. If the model isn't already a working v1 resource, do the model work first — invoke the **`crudable`** skill. Permissions are enforced at the model level; **never** re-check them in a v2 handler. + +**Every exposed model field needs a `doc:` tag.** v2's schema is reflected from struct tags at runtime; Huma cannot read the Go doc comments swaggo uses for v1. A field without `doc:"..."` ships with no description in the spec. Add the tag alongside the existing comment (keep both — swaggo still reads the comment for v1, and they should stay in sync): + +```go +// The title of the label. You'll see this one on tasks associated with it. +Title string `json:"title" minLength:"1" maxLength:"250" doc:"The title of the label. You'll see this one on tasks associated with it."` +``` + +These model edits are safe for v1 — swaggo, XORM, and govalidator all ignore the `doc` tag. (Huma *does* read validation tags like `minLength`/`maxLength`/`enum`/`format`, so those carry over without a `doc` tag.) As with operations, a `doc` tag earns its place when it says something the field name and type don't: a format hint ("hex, 6 chars"), a read-only note ("set by the server; ignored on write"), units, or allowed values. "The label description." on a `Description` field is filler. See `pkg/models/label.go` for the reference. + +## Steps + +### 1. Create `pkg/routes/api/v2/.go` + +Define the list-response body, a `RegisterRoutes(api huma.API)` function, and one handler per operation. Mirror `labels.go` exactly: + +```go +// Element type matches what models..ReadAll returns; extra fields +// tagged json:"-" keep the wire shape identical to the plain model. +type fooListBody struct { + Body Paginated[*models.Foo] +} + +func RegisterFooRoutes(api huma.API) { + tags := []string{"foos"} + + Register(api, huma.Operation{ + OperationID: "foos-list", + Summary: "List foos", + Description: "Returns the foos the authenticated user has access to, paginated.", + Method: http.MethodGet, Path: "/foos", Tags: tags, + }, foosList) + Register(api, huma.Operation{OperationID: "foos-read", Summary: "Get a foo", Description: "...", Method: http.MethodGet, Path: "/foos/{id}", Tags: tags}, foosRead) + Register(api, huma.Operation{OperationID: "foos-create", Summary: "Create a foo", Description: "...", Method: http.MethodPost, Path: "/foos", Tags: tags}, foosCreate) + Register(api, huma.Operation{OperationID: "foos-update", Summary: "Update a foo", Description: "...", Method: http.MethodPut, Path: "/foos/{id}", Tags: tags}, foosUpdate) + Register(api, huma.Operation{OperationID: "foos-delete", Summary: "Delete a foo", Description: "...", Method: http.MethodDelete, Path: "/foos/{id}", Tags: tags}, foosDelete) +} +``` + +Use the package's `Register` wrapper, **not** `huma.Register` directly — it sets `DefaultStatus` from the verb (POST → 201, DELETE → 204). Don't spell out `DefaultStatus` unless you need a non-default code. Don't set `Security:` per operation — it's applied globally in `NewAPI`. + +**Every operation needs a `Summary` and `Description`.** v2's OpenAPI spec is generated from these `Operation` fields at runtime — unlike v1's swaggo, Huma cannot read Go doc comments, so anything you don't put in the `Operation` (or in a `doc:` tag, see below) is simply absent from the spec and the docs UI. An operation without them ships undocumented. + +**Make the description document the non-obvious — don't restate the verb+noun.** "Deletes a label" adds nothing over `DELETE /labels/{id}`. Spend the description on what a consumer *can't* infer from the method/path/schema: permission scope ("only the owner may delete it"; "returns only labels you can see, not a global list"), full-replace vs partial (PUT replaces, PATCH merges), read-only/conditional behavior (ETag → `If-None-Match` → 304), side effects (create sets ownership), non-obvious status codes. If the honest description is just the verb+noun, a short summary alone is fine — don't pad. See `labels.go` for the calibration. + +### 2. Write the handlers + +Every handler: pull auth with `authFromCtx(ctx)`, call the matching `handler.Do*`, wrap returned errors in `translateDomainError`. Use the shared envelopes from `types.go` (`singleBody`, `singleReadBody`, `emptyBody`, `ListParams`, `Paginated`/`NewPaginated`). + +- **List** takes `*ListParams` (gives you `page`/`per_page`/`q` for free) and returns `*fooListBody`. **You must type-assert the `DoReadAll` result to the concrete slice** — `result` is `any`, and a blind cast or a generic wrapper silently serialises `[]` (the "generic-any silent-empty trap"). Return a hard error on mismatch: + ```go + items, ok := result.([]*models.Foo) + if !ok { + return nil, fmt.Errorf("foos.ReadAll returned unexpected type %T", result) + } + return &fooListBody{Body: NewPaginated(items, total, in.Page, in.PerPage)}, nil + ``` +- **Read** embeds `conditional.Params` in its input, builds an ETag from `id + Updated.UnixNano()`, calls `in.PreconditionFailed(etag, label.Updated)` when `in.HasConditionalParams()`, and returns `*singleReadBody[Model]` with the **quoted** ETag (`"`+etag+`"`). +- **Create / Update** take a `Body Model` input and return `*singleBody[Model]`. Update sets `in.Body.ID = in.ID` (URL wins over body). +- **Delete** returns `*emptyBody`. + +### 3. Wire it into the group + +In `pkg/routes/routes.go`, `registerAPIRoutesV2`, add the registration **before** `EnableAutoPatch(api)`: + +```go +apiv2.RegisterFooRoutes(api) +// ... other resources ... +apiv2.EnableAutoPatch(api) // MUST stay last — walks registered GET+PUT pairs +``` + +That's the only edit outside the v2 package for a standard CRUD resource. + +## REST verb conventions (v2 inverts v1) + +| Operation | v1 | v2 | +|---|---|---| +| create | PUT | **POST** | +| update | POST | **PUT** (and PATCH) | +| read / read-all / delete | GET / GET / DELETE | same | + +## Non-CRUDable / custom routes + +Not everything is plain CRUD — bulk operations, custom actions (`POST /tasks/{id}/duplicate`), sub-resource toggles, RPC-ish endpoints. These still go through Huma and reuse most of the machinery, but two responsibilities move **into your handler** because there's no `handler.Do*` doing them for you: + +1. **Permission enforcement is now yours.** This is the one place the "never check permissions in the handler" rule inverts. With no generic `Do*` to call the model's `Can*`, the handler must do it explicitly — load the relevant entity and call its permission method, then refuse on denial. Mirror the v1 custom-handler shape (`pkg/routes/api/v1/task_attachment.go`): + ```go + func tasksDuplicate(ctx context.Context, in *struct{ ID int64 `path:"id"` }) (*singleBody[models.Task], error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + s := db.NewSession() + defer s.Close() + + t := &models.Task{ID: in.ID} + can, err := t.CanUpdate(s, a) // or whichever Can* gates this action + if err != nil { + _ = s.Rollback() + return nil, translateDomainError(err) + } + if !can { + return nil, huma.Error403Forbidden("forbidden") + } + // ... do the work against s ... + if err := s.Commit(); err != nil { + return nil, translateDomainError(err) + } + return &singleBody[models.Task]{Body: t}, nil + } + ``` +2. **Session / transaction management is now yours.** The `Do*` helpers open and commit their own `xorm.Session`; custom handlers open one with `db.NewSession()`, `defer s.Close()`, and `Commit`/`Rollback` explicitly for anything that writes. + +Otherwise the same rules apply: register with the `Register` wrapper, pull auth via `authFromCtx`, route every error through `translateDomainError`, and reuse the `types.go` envelopes — or define a small body struct when none fits (don't bend a custom response into `singleBody` if it's awkward). + +**Verb choice:** pick by semantics, not the CRUD table. Non-idempotent actions are `POST`. AutoPatch only synthesises PATCH for GET+PUT *pairs*, so standalone custom routes are never touched. + +**Token permissions still automatic, but mind the derived name:** `collectRoutesForAPITokens` keys a route off its prefix-stripped path, so `POST /api/v2/tasks/{id}/duplicate` lands under the `tasks` group as a `duplicate` permission. Single-segment custom paths fall into the `other` group. Name the path so the derived `(group, permission)` reads sensibly — that string is what users grant tokens against. + +## What's automatic — do NOT hand-roll + +- **PATCH** — `EnableAutoPatch` synthesises a JSON-Merge-Patch PATCH for every GET+PUT pair. Don't register PATCH yourself. +- **API token permissions** — `collectRoutesForAPITokens` walks the Echo router after registration, so your new routes land in the v2 token table automatically under the same `(group, permission)` keys as their v1 names. PATCH is intentionally not stored; `CanDoAPIRoute` accepts it as an alias for the stored PUT (see `pkg/models/api_routes.go`). +- **Security schemes** — `JWTKeyAuth` + `APITokenAuth` are declared globally in `NewAPI`. For a public endpoint, set `Security: []map[string][]string{}` on that operation and add its path to `unauthenticatedAPIPaths` in `routes.go`. +- **Error shape** — `translateDomainError` maps any `web.HTTPErrorProcessor` (e.g. `ErrFooDoesNotExist`) onto Huma's status error, producing RFC 9457 `application/problem+json`. Errors without HTTP semantics become 500. +- **OpenAPI spec / Scalar docs / `$schema` URLs** — handled in `huma.go`. Leave `Servers` alone (the relative entry must stay at index 0). + +## Anti-patterns (these get flagged) + +- Re-checking permissions in the handler instead of trusting `handler.Do*` → the model's `Can*`. +- Blind `result.([]*models.Foo)` without the `ok` check, or returning the `any` straight into the envelope — silent empty lists. +- `huma.Register` instead of the package `Register` wrapper (loses the verb-based status). +- Per-operation `Security:` lines (now global) or registering a manual PATCH (AutoPatch does it). +- Returning a raw model error instead of routing it through `translateDomainError` → leaks a 500 instead of the right code. +- Unquoted ETag in the response header. +- Operations without `Summary`/`Description`, or model fields without `doc:` tags — they ship undocumented because Huma can't read Go comments. + +## Tests (mandatory) + +Mirror the v1 webtest shape so v2 parity is readable side-by-side. Use the `webHandlerTestV2` harness in `pkg/webtests/integrations.go` — it takes the same `urlParams` map as v1's `webHandlerTest`. See `pkg/webtests/huma_label_test.go`: + +- One `Test` covering list/read/create/update/delete, positive + negative (forbidden, nonexistent), mirroring the v1 model test. +- v2-only behaviour (ETag/304, PATCH merge-patch) goes in separate top-level `Test_*` funcs using the `humaRequest`/`humaTokenFor` helpers in `pkg/webtests/huma_helpers_test.go`. +- The RFC 9457 error-body shape is asserted **once** globally in `TestHuma_ErrorShapeIsRFC9457` — don't re-assert the full problem+json shape per resource, just the status code. + +Run with `mage test:filter Test` while iterating. Save output to a file per the project test-output rule. + +## Related + +- `crudable` skill — the model-layer prerequisite +- `pkg/routes/api/v2/labels.go` — reference resource +- `pkg/routes/api/v2/{types,errors,huma}.go` — shared envelopes, bridge, config +- `pkg/web/handler/core.go` — the `Do*` functions handlers call diff --git a/AGENTS.md b/AGENTS.md index 6cb251b1c..67593378b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -11,12 +11,24 @@ The project consists of: - `desktop/` – Electron wrapper application - `docs/` – Documentation website +## API Version Policy — new work goes to /api/v2 + +**`/api/v1` is effectively deprecated and frozen.** It still runs and is fully supported for existing clients, but it should not grow. + +- **Every new route goes on `/api/v2`** (the Huma-backed API in `pkg/routes/api/v2/`). This includes new CRUDable entities, new custom/non-CRUD endpoints, and new actions on existing resources. +- **Before adding any v2 route, invoke the `api-v2-routes` skill** — it covers both CRUD and non-CRUD shapes. +- **Touch `/api/v1` only to:** fix a bug, or port an existing resource to v2. Do not add net-new functionality there. +- Models in `pkg/models/` are shared by both APIs — a new entity still gets its model + `Can*` methods (invoke `crudable`); only the HTTP surface differs (v2, not v1). + +If a task says "add an endpoint for X" without naming a version, it means v2. + ## Skills Before writing code in these areas, invoke the matching skill with the `Skill` tool. They are short checklists derived from recurring review feedback — loading them up front avoids rework. - Adding or modifying a model in `pkg/models/` (new CRUD, new or changed `Can*` methods, anything touching permissions): invoke `crudable`. - Creating or editing any file under `pkg/migration/`: invoke `migration`. +- Adding **any** new API route (new entity, custom action, or porting from v1) — all new routes go on the Huma-backed `/api/v2`, editing `pkg/routes/api/v2/`: invoke `api-v2-routes`. See the API Version Policy above. ## Plans and Worktrees @@ -172,11 +184,10 @@ Modern Vue 3 composition API application with TypeScript: ### Adding New Features **Backend Changes:** -1. Create/modify models in `pkg/models/` with proper CRUD and Permissions interfaces as required -2. Add database migration if needed: `mage dev:make-migration ` +1. Create/modify models in `pkg/models/` with proper CRUD and Permissions interfaces as required (invoke the `crudable` skill) +2. Add database migration if needed: `mage dev:make-migration ` (invoke the `migration` skill) 3. Create/update services in `pkg/services/` for complex business logic -4. Add API routes in `pkg/routes/api/v1/` following existing patterns -5. Update Swagger annotations +4. Add API routes on **`/api/v2`** in `pkg/routes/api/v2/` — invoke the `api-v2-routes` skill. Do **not** add new routes to `/api/v1`; it is frozen (see API Version Policy above) **Frontend Changes:** 1. Create TypeScript interfaces in `src/modelTypes/` matching backend models @@ -192,10 +203,11 @@ Modern Vue 3 composition API application with TypeScript: 4. Update TypeScript interfaces in frontend `src/modelTypes/` ### API Development -- All API endpoints follow RESTful conventions under `/api/v1/` -- Use generic web handlers in `pkg/web/handler/` for standard CRUD operations -- Implement proper permissions checking using the Permissions interface -- Add Swagger annotations for automatic documentation generation +- **New endpoints go on `/api/v2`** (Huma-backed, `pkg/routes/api/v2/`). `/api/v1` is frozen — see the API Version Policy near the top. Invoke the `api-v2-routes` skill before writing v2 routes. +- v2 verb conventions differ from v1: POST creates, PUT/PATCH update (v1 used PUT to create, POST to update). +- Both versions reuse the generic `pkg/web/handler/` `Do*` functions for standard CRUD, which enforce permissions via the model's `Can*` methods. +- Implement permission checks at the model level via the Permissions interface — never in the route handler (the exception: non-CRUD v2 actions must call `Can*` explicitly; the skill covers this). +- v2 generates its OpenAPI spec from Go types automatically — no Swagger annotations. v1's swaggo annotations stay as-is but no new ones are needed. ### Testing - Backend: Feature tests alongside source files, web tests in `pkg/webtests/`