diff --git a/.planning/phases/03-first-vertical-slice-genres-end-to-end/03-REVIEW.md b/.planning/phases/03-first-vertical-slice-genres-end-to-end/03-REVIEW.md new file mode 100644 index 0000000..06b4ed6 --- /dev/null +++ b/.planning/phases/03-first-vertical-slice-genres-end-to-end/03-REVIEW.md @@ -0,0 +1,190 @@ +--- +status: issues +phase: 03-first-vertical-slice-genres-end-to-end +depth: standard +files_reviewed: 39 +files_reviewed_list: + - bouncer/context.go + - bouncer/jwt.go + - bouncer/jwt_test.go + - cmd/summer/main.go + - examples/hello/hello_test.go + - examples/hello/plugins/greeter/plugin.go + - ../fonoteka.go/app/app.go + - ../fonoteka.go/go.mod + - ../fonoteka.go/main.go + - ../fonoteka.go/parity/genre_integration_test.go + - ../fonoteka.go/parity/genre_security_test.go + - ../fonoteka.go/parity/genre_smoke_test.go + - ../fonoteka.go/parity/genres_seed_test.go + - ../fonoteka.go/parity/manifest.yaml + - ../fonoteka.go/parity/parity_contract_test.go + - ../fonoteka.go/parity/parity_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/active_collection.go + - ../fonoteka.go/plugins/golem15/fonoteka/genre.go + - ../fonoteka.go/plugins/golem15/fonoteka/genre_handler.go + - ../fonoteka.go/plugins/golem15/fonoteka/plugin.go + - ../fonoteka.go/plugins/golem15/user/plugin.go + - ../fonoteka.go/README.md + - go.mod + - internal/build/build.go + - lagoon/commands.go + - lagoon/connection.go + - lagoon/connection_test.go + - lagoon/migrations.go + - lagoon/migrations_test.go + - lagoon/order.go + - lagoon/order_test.go + - lagoon/postgres_test.go + - pact/capabilities.go + - scripts/check-phase3.sh + - surf/middleware_test.go + - surf/params.go + - surf/params_test.go + - surf/router.go + - surf/serve.go +findings: + critical: 0 + warning: 7 + info: 4 + total: 11 +reviewed: 2026-09-17T18:45:00Z +--- + +# Phase 3: Code Review Report + +**Reviewed:** 2026-09-17T18:45:00Z +**Depth:** standard +**Files Reviewed:** 39 +**Status:** issues + +## Summary + +The slice is a real boot path: shared pgx-stdlib `*sql.DB` under GORM, per-plugin gormigrate, named `jwt.auth` / `inv.must-change-password` on ServeMux, and `GET /_fonoteka/api/v1/genres` with grouped owner-OR-editor then AND active `collection_id`. HS256 is pinned (`WithValidMethods`, `alg:none` rejected), empty JWT secret fails `Boot`, missing middleware names fail Assemble, and CORS preflight short-circuits before named auth. Tenant counts are parameterized; `lagoon.OrderBy` only emits an allow-listed identifier. The recorded genres fixture stays unmodified; corpus passing increments only after replay. + +No authentication bypass or SQL injection was found on the genre path. The defects that matter are in the new HTTP server and the two boot seams: `http.Server` has no timeouts, panic recovery can append a 500 body onto a committed response, `summer serve` drops `--addr`, and production `Activate` runs before the database is published while tests use `app.Handler` (Publish then Activate). PHP `JwtAuthenticate` maps unexpected lookup errors to 500; Go maps them to 401. Typed-param 404s are `net/http`'s HTML page. + +Caled files not in SUMMARY frontmatter (`cmd/summer/runtime.go`, `fonoteka` `user.go` / `password.go` / `migrations.go`) were read to trace those paths. + +## Warnings + +### WR-01: HTTP server has no header/body timeouts + +**File:** `surf/serve.go:49` +**Issue:** `ServeCommand` is the first production `http.Server` in the kernel. The struct sets only `Addr`, `Handler`, and `BaseContext`. `ReadHeaderTimeout` (and `ReadTimeout` / `WriteTimeout` / `IdleTimeout`) stay at zero. Go's default is "wait forever for the next header byte," which is the Slowloris hold-open. Graceful shutdown on SIGINT/SIGTERM is present; connection-level idle abuse is not. + +Phase 6 owns CORS/rate-limit depth. Timeouts are serve hardening for this command, not that work. + +**Fix:** + +```go +srv := &http.Server{ + Addr: addr, + Handler: h, + BaseContext: func(net.Listener) context.Context { return ctx }, + ReadHeaderTimeout: 10 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 30 * time.Second, + IdleTimeout: 60 * time.Second, +} +``` + +### WR-02: Panic recovery writes a second body after headers are committed + +**File:** `surf/router.go:290-300` +**Issue:** `recoverJSON` always `WriteHeader(500)` and writes `{"error":true,"message":"Internal server error"}`. If the handler (or later middleware) already called `WriteHeader` / `Write` and then panics, `WriteHeader` is superfluous and `Write` **appends** to the existing body. A 200 JSON payload can become `{"data":[...]}{"error":true,...}` with status still 200. The panic test panics before any write, so this is untested. + +**Fix:** Track whether the response started (a thin `http.ResponseWriter` wrapper with a `wrote` flag). On recover, if already written, log and return without a second body. If not, write the opaque 500 JSON. + +### WR-03: `summer serve` cannot pass `--addr` to the app binary + +**File:** `cmd/summer/main.go:38`, `cmd/summer/runtime.go:15-25` +**Issue:** App `surf.ServeCommand` declares `--addr` (default `:8080`). The tool binary registers `delegateCommand("serve", ...)`, which has no flags and forwards only `in.Args()`. Cobra on `summer serve` therefore rejects `--addr` as unknown, or (if forced as a positional) never maps it onto the app flag. `migrate:rollback --plugin` was special-cased; `serve --addr` was not. Operators following the two-binary model cannot change the listen address without invoking `./bin/fonoteka serve` directly. + +**Fix:** Declare the same `--addr` flag on the summer `serve` delegate and append `--addr`, value when set, matching `delegateRollbackCommand`. Or forward unknown flags via `cmd.Flags().Args()` / a dedicated passthrough. + +### WR-04: Production boots plugins before the database is published; tests do not + +**File:** `../fonoteka.go/main.go:31-37`, `surf/serve.go:35-43`, `../fonoteka.go/app/app.go:31-43` +**Issue:** Generated `run` calls `party.Activate` with no `*sql.DB` / `*gorm.DB` on the backpack. `ServeCommand` (and `lagoon.withDB`) open, `Publish`, then `Assemble`. The in-process seam `app.Handler` does `lagoon.Use` + `Publish` **then** `Activate`. Current `Boot` implementations only check the JWT secret, so both paths work. Any later plugin that `Lookup[*gorm.DB]()` in `Boot` (Phase 5 models, Phase 7 user) will fail under `fonoteka serve` / `fonoteka migrate` while `app.Handler` tests stay green. + +**Fix:** Publish the pool before `Activate` in generated `run` (open lazily in `Activate` is worse). Or generate `run` to match `app.Handler`: open, publish, activate, then register commands that reuse the published handles instead of `OpenFromApp` again. Add a test that boots the generated `run`/`ServeCommand` path, not only `app.Handler`. + +### WR-05: User-lookup failures return 401, not PHP's 500 + +**File:** `bouncer/jwt.go:52-56` +**Issue:** D-10 requires the same status and JSON as `JwtAuthenticate.php`. That middleware returns 401 for `UnauthorizedHttpException` and **500** `{"error":true,"message":"Authentication error"}` for unexpected errors (DB, config). Go uses `write401` for every `FindByID` error, including a missing GORM handle. A down database is indistinguishable from a bad token (401 vs 500). The string `"Authentication error"` matches; the status does not. + +**Fix:** + +```go +user, err := users.FindByID(r.Context(), uint(id)) +if err != nil { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _ = json.NewEncoder(w).Encode(map[string]any{"error": true, "message": "Authentication error"}) + return +} +``` + +Keep 401 for `user == nil` (`User not found`). Assert status 500 in the existing leak test when the provider returns an error. + +### WR-06: Typed-param failures are HTML `404 page not found` + +**File:** `surf/params.go:81-93`, `examples/hello/plugins/greeter/plugin.go:60-65` +**Issue:** HTTP-01 / T-03-07 require malformed and unknown ids to 404. `constrain` and the greeter handler call `http.NotFound`, which writes `text/plain` `404 page not found`. PHP API 404s on `/_fonoteka/api/v1/.../{id}` are JSON envelopes. Genres has no path param, so the recorded fixture is unaffected; every later integer route inherits this body. Tests only assert status. + +**Fix:** A small JSON 404 writer used by `constrain` and recommended to handlers: + +```go +func writeJSON404(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":true,"message":"Not found"}`)) +} +``` + +Keep status 404 for both malformed and unknown. + +### WR-07: Phase 3 gate leaks the synthetic smoke server on failure + +**File:** `scripts/check-phase3.sh:76-123` +**Issue:** `cleanup_smoke` is defined but never `trap`ped. `set -e` plus a failing `parity:record` / `parity:replay` in the subshell exits the script before the success-path `cleanup_smoke` call. The background `python3` `serve_forever` stays bound to the ephemeral port. Phase 2 REVIEW IN-04 reported the same pattern in `check-phase2.sh`; this script copied it. + +**Fix:** After defining `cleanup_smoke`, `trap cleanup_smoke EXIT` (and still call it on the happy path, or rely on the trap only). + +## Info + +### IN-01: Two plugin-id lists can drift + +**File:** `../fonoteka.go/app/app.go:18-19`, `../fonoteka.go/plugins.gen.go:11-14` +**Issue:** `app.PluginIDs` is hand-maintained. Generated `main.PluginIDs` comes from `summer.yaml`. Tests and `app.Handler` use the former; `fonoteka serve` uses the latter. Adding a plugin and running `summer build` updates production without updating the test seam. + +**Fix:** Share one generated list (blank-import friendly package, or `app.PluginIDs` written by `summer build`). + +### IN-02: Bearer prefix is exact `Bearer `, not PHP's 6-character strip + +**File:** `bouncer/jwt.go:86-96` +**Issue:** PHP `AuthHeaders` takes `substr($header, strlen('bearer'))` then `trim` — case-insensitive in practice and tolerant of `bearer ` / `BEARER `. Go `CutPrefix(h, "Bearer ")` rejects those. The Nuxt fixture sends `Bearer {{jwt:alice}}`, so replay is green. A client that sends `bearer` will 401 `Token not provided` against Go and succeed against PHP. + +**Fix:** Case-fold the scheme and accept any whitespace after it (`Authorization` RFC 6750). Keep the 401 body for missing/empty tokens. + +### IN-03: Fallback active collection filters `kind=collection`; PHP `resolve()` does not + +**File:** `../fonoteka.go/plugins/golem15/fonoteka/active_collection.go:119-126` +**Issue:** PHP `ActiveCollectionResolver::resolve` fallback is `Collection::accessibleBy($user)->orderBy('id')->first()` with no kind filter (`switchTo` is the path that adds `kind = collection`). Go `firstAccessibleRealCollection` requires `kind=collection`. Plan 02 chose that on purpose (wishlist must not become the tenant). Stored wishlist context is still accepted (`findAccessibleCollection` has no kind filter), matching PHP `find()`. A user whose lowest accessible row is a wishlist gets different fallbacks on PHP vs Go. Seed/parity always persist a real collection, so the fixture does not catch this. + +**Fix:** Keep the kind filter (it is the safer product rule) and add a Go test for "stored wishlist still counts wishlist albums; missing context skips wishlist IDs." Document the PHP `resolve()` divergence so Phase 12 does not "fix" it back. + +### IN-04: `migrate` / `migrate:status` fail closed on an empty JWT secret + +**File:** `../fonoteka.go/plugins/golem15/user/plugin.go:39-42`, `../fonoteka.go/main.go:31-36` +**Issue:** `Activate` runs before any command. Empty `golem15.user.jwt.secret` fails `Boot`, so `fonoteka migrate` cannot apply DDL without a JWT secret. That matches D-11 fail-boot, but it couples migrations to an HTTP credential. README already exports both env vars together. + +**Fix:** None required for v1 if the coupling is accepted. Optionally move the secret check into `Middlewares()` / `Assemble` so CLI migrate works with only `SUMMER_DATABASE__DSN`. + +--- + +_Reviewed: 2026-09-17T18:45:00Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_