feat(api): projects route shell (7 routes, REST-corrected) — #20#24
feat(api): projects route shell (7 routes, REST-corrected) — #20#24neversettle17-101 wants to merge 2 commits into
Conversation
ed9117b to
3906d56
Compare
illegalcall
left a comment
There was a problem hiding this comment.
Structure feedback, using the package-ownership rules documented in #41 as the reference point.
Direct take: the direction is good, but I would tighten a few boundaries before using this PR as the template for the next resources.
-
internal/projectis the right home for project-owned concepts. KeepingProject,Summary,Degraded, project behavior config shapes, and project command/result types together is better than putting them indomain,ports, orhttpd. This is the right feature-package direction. -
domainshould stay smaller than this PR implies in comments.domain.ProjectIDis appropriate because sessions/lifecycle/workspace need the same identity vocabulary. Full project read models and project config shapes should stay ininternal/project, as this PR does. -
The
project.Managerboundary is okay only if it is meant to be an application-service contract used beyond HTTP, for example HTTP + CLI. If it is only a controller seam, I would define the small interface inhttpd/controllersand letprojectexpose a concrete service later. Rule: reusable product workflow contracts live in the feature package; protocol-only seams live with the protocol consumer. -
project.GetResultneeds a clearer DTO/wire boundary. The comment saysStatusmirrors the wire discriminator, but the struct has no JSON tags and hasProjectplusDegraded, while OpenAPI defines{ status, project }whereprojectis oneOf Project/DegradedProject. Either makeGetResultan internal app result and map it inhttpd, or make it explicitly match the OpenAPI wire shape. Do not leave it halfway between both. -
The comments in
controllers/projects.gostill say managers come fromports/inbound.go. That is stale now that the controller depends onproject.Manager. -
Integration note: this PR targets
feat/issue-10; against currentmain, there are conflicts inbackend/go.mod,backend/go.sum, andbackend/internal/httpd/router.go. Since the package structure is the part we want to reuse, I would refresh it against currentmainbefore treating it as settled.
Overall: +1 on internal/project as the pilot feature package and +1 on keeping OpenAPI/HTTP concerns under httpd. The main thing I would change is the phrasing/pattern around “one Manager per resource” so it does not become another central-ports pattern, just distributed by package name.
illegalcall
left a comment
There was a problem hiding this comment.
Adding a few line-specific structure clarifications so the package boundary is easier to settle before later resources copy this pattern.
Greptile SummaryThis PR lands the
Confidence Score: 4/5Safe to merge for a route-shell PR; the encoding defect in The
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Router as chi Router
participant PC as ProjectsController
participant httpx
participant Mgr as project.Manager
participant Store as sqlite.Store
Client->>Router: HTTP request
Router->>PC: handler(w, r)
alt "Mgr == nil"
PC->>httpx: WriteError(500, SERVICE_UNAVAILABLE)
httpx-->>Client: 500 JSON envelope
else Mgr wired
PC->>httpx: "DecodeJSON(r, &input) [POST/PATCH only]"
alt decode error
httpx-->>Client: 400 INVALID_JSON envelope
end
PC->>Mgr: List/Get/Add/UpdateConfig/Remove(ctx, ...)
Mgr->>Store: DB operation
Store-->>Mgr: rows / ok / err
alt typed httpx.APIErr
Mgr-->>PC: httpx.APIErr
PC->>httpx: WriteAPIErr(w, r, e)
httpx-->>Client: 4xx/5xx JSON envelope
else success
Mgr-->>PC: result
PC->>httpx: WriteJSON(w, status, envelope)
httpx-->>Client: 2xx JSON body
end
end
Reviews (14): Last reviewed commit: "feat(api): implement project.Manager ove..." | Re-trigger Greptile |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
6edb65f to
0b6c493
Compare
|
Heads-up on the conflicts here: they're caused by #47 ("implement project routes with mock manager/store"), which squash-merged to Two parts of this PR, though, are genuinely net-new and not on It's green (backend build/vet/test incl. drift + parity tests, frontend gen + typecheck). One tradeoff called out there: regenerating the spec drops the hand-authored examples/ Suggest closing this PR as superseded by #47 (+ #59 for the codegen/frontend delta) — but it's your call, and I didn't want to touch it without flagging you first. 🙏 |
PR was building, leaving #24 conflicting on nearly every file. The route shell itself is now redundant, but two pieces of #24 are genuinely net-new and absent from main — salvage them here, rebuilt on top of #47's merged code: - Code-first OpenAPI: apispec/specgen reflects the controllers' request/response types and project DTOs (the same types the handlers use at runtime) into openapi.yaml via swaggest. `cmd/genspec` + `go:generate` regenerate the committed, embedded spec; a drift test (TestBuild_MatchesEmbedded) and a route parity test (TestRouteSpecParity) fail CI if the spec and the code disagree. This replaces main's hand-maintained openapi.yaml so the "single source of truth" claim is actually enforced, not aspirational. - Typed frontend client: frontend/src/api/schema.d.ts is generated from that spec via openapi-typescript (`npm run gen:api`), consumed by a small openapi-fetch client. The frontend now gets its types from the daemon contract instead of hand-maintaining them. specgen lives outside apispec (which controllers import for the 501 stub) to avoid an import cycle. Handlers now encode named response DTOs (controllers/dto.go) instead of map[string]any so the generator reflects the real wire shapes. A gen-verify CI job regenerates both artifacts and fails on a stale commit. Tradeoff: the generated spec drops the hand-authored examples / x-rest-audit notes from #47's openapi.yaml; those can be re-added as operation metadata in specgen if wanted. Behaviour-only patch (no handler logic changes). Supersedes the codegen + frontend parts of #24. Refs #20, #47.
The #47 PR was built up on top the current PR's previous commits.
I was planning to work on all these items and add an end to end description testing suite as well. Open to close this PR if we are okay with the above mentioned points. |
Squashed feat/20 for a clean rebase onto main (which now carries #47's parallel implementation). Keeps our architecture: httpx transport, swaggest-generated openapi.yaml + frontend TS types, CI drift guard, 5 REST-corrected routes, 500 SERVICE_UNAVAILABLE while the Manager is nil.
Implement the existing 5-method project.Manager backed directly by the sqlite project store (no extra store/port/adapter, no memory store). Correcting #47: reuses our interface, drops reload/repair, and uses one error type. - project/manager.go: NewManager(*sqlite.Store) calling project_store.go directly. Add validates path (git rev-parse) + derives/validates the id + path/id conflict checks (suggestedProjectId); List/Get/Remove are real; Get always status:"ok" and UpdateConfig returns a typed error until config persistence exists (registry-only scope). - httpx.APIErr: one typed error (status+kind+code+message+details) with BadRequest/NotFound/Conflict/Internal constructors. The manager returns these; controllers translate via writeErr. Replaces #47's separate project/errors.go. - Wire the real Manager into main.go (httpd.New now takes APIDeps). - Tests: manager_test.go drives all 5 methods + validation/conflict cases against a real temp-dir sqlite store; controller test covers the typed→HTTP translation. Verified live: real 201/200/400/404/409 over the daemon. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
746f385 to
10e5864
Compare
@AgentWrapper I will address these comments in a new PR. Closing the existing one. |
PR was building, leaving #24 conflicting on nearly every file. The route shell itself is now redundant, but two pieces of #24 are genuinely net-new and absent from main — salvage them here, rebuilt on top of #47's merged code: - Code-first OpenAPI: apispec/specgen reflects the controllers' request/response types and project DTOs (the same types the handlers use at runtime) into openapi.yaml via swaggest. `cmd/genspec` + `go:generate` regenerate the committed, embedded spec; a drift test (TestBuild_MatchesEmbedded) and a route parity test (TestRouteSpecParity) fail CI if the spec and the code disagree. This replaces main's hand-maintained openapi.yaml so the "single source of truth" claim is actually enforced, not aspirational. - Typed frontend client: frontend/src/api/schema.d.ts is generated from that spec via openapi-typescript (`npm run gen:api`), consumed by a small openapi-fetch client. The frontend now gets its types from the daemon contract instead of hand-maintaining them. specgen lives outside apispec (which controllers import for the 501 stub) to avoid an import cycle. Handlers now encode named response DTOs (controllers/dto.go) instead of map[string]any so the generator reflects the real wire shapes. A gen-verify CI job regenerates both artifacts and fails on a stale commit. Tradeoff: the generated spec drops the hand-authored examples / x-rest-audit notes from #47's openapi.yaml; those can be re-added as operation metadata in specgen if wanted. Behaviour-only patch (no handler logic changes). Supersedes the codegen + frontend parts of #24. Refs #20, #47.
Implements #20 (#18a) — Projects route shell + request/response transport on top of the Phase 1a skeleton (#14). Rebased on
main.Design reference: ao-routes-analysis — the legacy route audit + per-route verdicts this PR implements.
Scope
Maps the
/api/v1projects layer: routes, request decoding, response encoding, the OpenAPI contract, and the package boundaries. Noproject.Managerimplementation — that lands on top of this. With the (nil) Manager the route shell returns501; with a Manager injected the same handlers run end-to-end and a correct request returns2xx(verified below).This PR also lands the code-first OpenAPI + TypeScript codegen pipeline (Go is the source of truth;
openapi.yamland the frontend types are generated, with a CI drift guard) — see the OpenAPI section below.Route surface (5 canonical)
/api/v1/projects200 {projects: Summary[]}500/api/v1/projects201 {project}400 INVALID_JSON|PATH_REQUIRED|NOT_A_GIT_REPO,409 PATH_ALREADY_REGISTERED|ID_ALREADY_REGISTERED(details.suggestion),500/api/v1/projects/{id}200 {status:"ok"|"degraded", project}404 PROJECT_NOT_FOUND,500/api/v1/projects/{id}200 {project}400 INVALID_JSON|IDENTITY_FROZEN|INVALID_LOCAL_CONFIG,404,409 PROJECT_DEGRADED|PROJECT_MISSING_PATH,500/api/v1/projects/{id}200 {projectId, removedStorageDir}400 INVALID_PROJECT_ID,404,500Per the route-analysis verdicts,
POST /projects/reloadis dropped (a Next.js in-process cache hack) andPOST /projects/{id}/repairis deferred. LegacyPUT /projects/{id}andPOST /projects/{id}are unregistered →405.REST corrections vs legacy TS surface
PUT /projects/{id}aliased to PATCHerrorfield{status, project}ok/successflags{error: msg}{error, code, message, requestId, details?}Request ↔ response layer (where the shapes live)
Split by product command vs HTTP wire shape (
backend-code-structure.md):AddInput,UpdateConfigInputinternal/project/dto.go{projects},{project},{status, project}internal/httpd/controllers/dto.gogetProjectResponsemapsproject.GetResultonto the OpenAPIoneOfinternal/httpd/httpxhttpdandcontrollersuse it without an import cycleEach handler runs
nil-guard → decode → call Manager → encode. Onlydomain.ProjectIDstays indomain/; the controller depends solely onproject.Manager.OpenAPI — code-first, Go is the source of truth
openapi.yamlis generated from Go (do not hand-edit) and stays OpenAPI3.1.0:apispec.Build()(internal/httpd/apispec/build.go) reflects the contract types + an operation registry into the document viaswaggest/openapi-go;cmd/genspecwrites it ongo generate;apispecembeds/serves it atGET /api/v1/openapi.yamland slices it into the 501 bodies as before.description/enum/default) as struct tags on the existingproject.*/httpx.Errortypes; operation metadata (paths, status codes, summaries, theok|degradedoneOf) inBuild().requiredis derived from theomitemptyconvention by anInterceptProphook — structs stay clean. Clean schema/TS names (Project,Summary,APIError, …) viaInterceptDefName.openapi-typescriptgeneratesfrontend/src/api/schema.d.ts;openapi-fetchgives a typed client (src/api/client.ts). No hand-maintained DTOs on either side.embedded openapi.yaml == fresh Build()+ determinism + route↔spec parity; CIgen-verifyregenerates both artifacts andgit diff --exit-codes. (#19still owns runtime request/response validation against this doc.)Verified:
go generate+npm run gen:apiare deterministic and no-diff on a clean tree; spec parses as3.1.0; required arrays match the prior hand-written spec exactly.Testing
Route-shell coverage is complete — happy path, body validation, error mapping, and the 501/405/404 structural behaviour:
List_OK,Add_Created,Get_Discriminator(ok + degraded),UpdateConfig_OK,Remove_OKBodyValidation— POST & PATCH × {malformed, empty}400 INVALID_JSON+ full envelope; Manager is not called when decode failsManagerError— all 5 endpointsCanonical501(all 5 → 501 +specslice, rightoperationId)LegacyUnregistered(PUT/POST → 405, repair → 404),MissingRoute(404 JSON)OpenAPIYAMLServed+apispecunit tests/api/v1/openapi.yaml; embed loads, operation lookup, param inheritanceEvery non-2xx assertion runs through one
assertEnvelopehelper that pins R9:error,code,message, and a correlationrequestIdare all present.Verification
Correct request → 2xx (Manager wired with a fake; the impl PR drops in the real one):
Route shell (nil Manager) + envelopes:
Out of scope
project.Managerimplementation + persistence → handler-impl PR (builds on this).4xxerror taxonomy (currently any Manager error → documented500) → handler-impl PR.Closes part of #18 (umbrella). Refs #20.
🤖 Generated with Claude Code