From 32982b089f62d63c3bde05fdbc91fa67ae0f0d3b Mon Sep 17 00:00:00 2001 From: rdev-worker Date: Sun, 8 Feb 2026 10:51:39 +0000 Subject: [PATCH] build: /review-feature user-preferences --- .sdlc/features/user-preferences/manifest.yaml | 2 +- .sdlc/features/user-preferences/review.md | 72 +++++++++++++++++++ .sdlc/state.yaml | 11 ++- 3 files changed, 81 insertions(+), 4 deletions(-) create mode 100644 .sdlc/features/user-preferences/review.md diff --git a/.sdlc/features/user-preferences/manifest.yaml b/.sdlc/features/user-preferences/manifest.yaml index 38cfb4b..6941486 100644 --- a/.sdlc/features/user-preferences/manifest.yaml +++ b/.sdlc/features/user-preferences/manifest.yaml @@ -36,7 +36,7 @@ artifacts: status: pending path: qa-results.md review: - status: pending + status: needs_fix path: review.md spec: status: approved diff --git a/.sdlc/features/user-preferences/review.md b/.sdlc/features/user-preferences/review.md new file mode 100644 index 0000000..e941013 --- /dev/null +++ b/.sdlc/features/user-preferences/review.md @@ -0,0 +1,72 @@ +# Code Review: User Preferences API + +## Summary +**Overall assessment: NEEDS_FIX** + +The implementation is well-structured and follows the hexagonal architecture pattern correctly. Domain validation, service layer, handlers, routes, OpenAPI spec, and database migration are all present and properly wired. Tests pass and cover the key scenarios. However, there is one blocker (compiled binary committed to git) and several warnings that should be addressed before merging. + +## Findings + +### Blockers + +- [ ] `services/preferences-api/server` — **Compiled binary (12MB) committed to git** — A Go compiled binary has been tracked and committed to the repository. This bloats the repo, is platform-specific, and should never be checked in. Remove it from git tracking with `git rm services/preferences-api/server` and add it to `.gitignore`. + +### Warnings + +- [ ] `services/preferences-api/internal/domain/preferences.go:34-43` — **Map iteration order is non-deterministic** — `ValidatePreferences` iterates over the map and returns on the first error. Due to Go's random map iteration, the same invalid input can produce different error messages across runs. This is not a bug per se, but makes error messages inconsistent for users sending multiple invalid keys. Consider sorting keys before validation or collecting all errors. + +- [ ] `services/preferences-api/internal/api/handlers/preferences.go:47-65` — **Missing domain error mapping on Get path** — The `Get` handler returns `h.svc.Get(...)` errors raw (line 60-61), while the `Update` handler correctly maps domain errors via `mapPreferencesDomainError`. While `Get` currently cannot produce domain validation errors, any future change to the service's Get method that introduces domain errors would leak as 500s. Consider applying `mapPreferencesDomainError` consistently. + +- [ ] `services/preferences-api/internal/domain/preferences.go` — **No domain-level unit tests** — The QA plan specifies 8 domain validation unit tests (DV-1 through DV-8), but there are no test files in the `domain` package. Domain validation is tested indirectly via the service layer tests, but direct domain tests provide better isolation and clearer failure diagnostics. AC-13 specifically requires "Domain layer validates preference keys and values independently of HTTP layer." + +- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go:80-82` — **`auth.SetUser` used directly in tests bypasses middleware** — The handler tests use `withAuthUser` to inject the user into context, but there is no test verifying that the auth middleware itself is applied to the route group. Auth enforcement is tested by convention (route registration review) rather than by execution. A test that hits the route without auth context would provide stronger assurance, though this may require middleware integration testing. + +- [ ] `services/preferences-api/internal/api/routes.go:32` — **Auth is conditionally applied based on `cfg.AuthEnabled`** — If `AUTH_ENABLED` is not set to `"true"`, all preference endpoints are completely unauthenticated. While this is useful for development, it means `auth.MustGetUser` in the handler will panic in production if the middleware isn't applied. Consider either: (a) documenting this clearly, (b) using `auth.GetUser` with a nil check instead of `auth.MustGetUser`, or (c) making auth always required in production builds. + +### Suggestions + +- [ ] `services/preferences-api/internal/adapter/postgres/preferences.go` — **No adapter-level tests** — The PostgreSQL adapter has no test coverage. While testing against a real database is harder, the adapter could be tested with integration tests or at minimum documented as requiring manual verification. + +- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go:21-22` — **Test user UUIDs don't match** — `otherUserID` is `550e8400-e29b-41d4-a716-446655440001` which only differs from `testUserID` (`...440000`) by the last digit. This works but could be more distinctive for readability (e.g., a completely different UUID). + +- [ ] `services/preferences-api/internal/api/spec.go:24-30` — **UpdatePreferencesRequest schema doesn't mark individual preference keys as optional** — The OpenAPI schema has `preferences` as required, but individual preference keys within the object aren't marked as optional. Since partial updates are supported, the schema should indicate that individual preference fields are optional to accurately document the API behavior. + +## Spec Alignment + +The implementation matches the spec well across all major requirements: + +| Spec Requirement | Status | Notes | +|-----------------|--------|-------| +| GET returns preferences as key-value pairs | OK | | +| GET returns empty preferences (not 404) for new users | OK | Service layer creates empty struct | +| PUT creates or updates (upsert semantics) | OK | PostgreSQL `ON CONFLICT` | +| PUT supports partial updates (merge) | OK | JSONB `||` operator merges | +| Supported preference keys validated | OK | Domain layer enforces closed set | +| Invalid values rejected with 400 | OK | Domain validation with descriptive errors | +| Unknown keys rejected with 400 | OK | | +| Both endpoints require authentication (401) | OK | Via `auth.Middleware` (when enabled) | +| Users can only access own preferences (403) | OK | `checkOwnership` in both handlers | +| PostgreSQL persistence | OK | | +| Standard `{data, meta}` envelope | OK | Via `httpresponse.OK` | +| OpenAPI spec documented | OK | Full spec with schemas, params, security | +| Domain layer validates independently | Partial | Validation exists but no direct unit tests | +| Hexagonal architecture followed | OK | Clean separation across all layers | +| Service layer unit tests | OK | 10 test cases | +| Handler integration tests | OK | 10 test cases | + +## Test Coverage Assessment + +### Covered by tests: +- Service layer: Get (empty user, existing user, repo error), Update (valid prefs, unknown key, invalid theme, invalid language, invalid notifications_enabled, repo error, partial merge) +- Handler layer: Get (200 existing, 200 empty, 400 invalid UUID, 403 mismatch), Update (200 success, 400 unknown key, 400 invalid value, 400 missing prefs, 400 invalid UUID, 403 mismatch) + +### Missing test coverage: +- **Domain validation unit tests** — DV-1 through DV-8 from QA plan are not implemented +- **Edge case EC-3** — Empty preferences object `{}` on PUT (what happens?) +- **Error cases ER-20, ER-21, ER-22** — Null values, nested objects, arrays as preference values +- **Auth middleware integration** — No test verifying unauthenticated requests get 401 +- **PostgreSQL adapter** — No tests for the adapter layer + +### All tests pass: +- `go test ./...` — 20 tests, all PASS +- `go vet ./...` — clean diff --git a/.sdlc/state.yaml b/.sdlc/state.yaml index 2693bbf..37a379d 100644 --- a/.sdlc/state.yaml +++ b/.sdlc/state.yaml @@ -7,9 +7,9 @@ active_work: branch: feature/user-preferences phase: implementation blocked: [] -last_updated: 2026-02-08T10:47:08.881050803Z -last_action: COMPLETE_TASK -last_actor: cli +last_updated: 2026-02-08T10:51:22.657760934Z +last_action: NEEDS_FIX_ARTIFACT +last_actor: user history: - timestamp: 2026-02-08T09:52:56.804287195Z action: CREATE_FEATURE @@ -106,3 +106,8 @@ history: feature: user-preferences actor: cli result: success + - timestamp: 2026-02-08T10:51:22.657566949Z + action: NEEDS_FIX_ARTIFACT + feature: user-preferences + actor: user + result: success