12 KiB
status, phase, depth, files_reviewed, files_reviewed_list, findings, reviewed
| status | phase | depth | files_reviewed | files_reviewed_list | findings | reviewed | |||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| issues | 03-first-vertical-slice-genres-end-to-end | standard | 39 |
|
|
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:
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:
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:
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 trapped. 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