build: /review-feature user-preferences

.sdlc/features/user-preferences/manifest.yaml

@@ -35,7 +35,7 @@ artifacts:
         status: pending
         path: qa-results.md
     review:
-        status: pending
+        status: needs_fix
         path: review.md
     spec:
         status: approved

.sdlc/features/user-preferences/review.md

@@ -0,0 +1,90 @@
+# Code Review: User Preferences API
+
+## Summary
+**Overall Assessment: NEEDS_FIX**
+
+The implementation is solid overall — clean hexagonal architecture, good test coverage, correct merge semantics, and proper error handling. However, there are two blockers related to the `UpdatedAt` time format (loses sub-second precision and timezone info) and the handler's manual JSON decoding approach bypassing the framework's `DecodeJSONStrict` for unknown-field rejection. There are also several warnings and suggestions.
+
+## Findings
+
+### Blockers
+
+- [ ] `services/preferences-api/internal/api/handlers/preferences.go:81` **`UpdatedAt` formatted with hardcoded layout instead of `time.RFC3339Nano`** -- The format string `"2006-01-02T15:04:05Z"` always appends a literal `Z` regardless of the actual timezone, and truncates sub-second precision. Use `time.RFC3339Nano` (or at minimum `time.RFC3339`) which is the standard Go approach and matches the ISO 8601 format shown in the spec. While `UpdatedAt` is currently always set to UTC, this is a correctness issue that will bite when the code is copy-pasted or when the timestamp source changes.
+
+- [ ] `services/preferences-api/internal/api/handlers/preferences.go:116-154` **Manual JSON decoding instead of using framework's `httpresponse.DecodeJSON` / `DecodeJSONStrict`** -- The handler manually decodes to `map[string]json.RawMessage`, then re-unmarshals the preferences payload. The framework provides `httpresponse.DecodeJSONStrict()` which handles unknown-field rejection, empty body, and malformed JSON in a consistent way. The current approach works but (a) duplicates framework logic, (b) doesn't use `app.BindAndValidate()` as specified in the design doc (AC/design says "Always use `app.Bind()` or `app.BindAndValidate()`"), and (c) the manual key-checking for the nested `preferences` object is fragile — adding a new preference key requires updating the `allowedKeys` map in the handler rather than leveraging struct-based validation. The CLAUDE.md critical rules state: "Always use `app.Bind()` or `app.BindAndValidate()`. Never use raw `json.NewDecoder`." While the handler uses `httpresponse.DecodeJSON` (not raw `json.NewDecoder`), it's still manually orchestrating decode steps the framework is designed to handle.
+
+### Warnings
+
+- [ ] `services/preferences-api/internal/adapter/memory/preferences.go:36-37` **Redundant copy of `Notifications` field** -- Lines `cp := *p` already copies all value-type fields including `Notifications` (which is a struct, not a pointer). The subsequent `cp.Notifications = p.Notifications` is a no-op. Same issue on lines 46-47 in `Upsert`. Not a bug, but misleading — suggests the author intended deep-copy logic but `NotificationSettings` contains no reference types.
+
+- [ ] `services/preferences-api/internal/api/handlers/preferences.go:183-192` **`authorizeAccess` silently allows all access when auth is disabled** -- When `auth.GetUser(ctx)` returns `nil` (no auth context), the function returns `nil` (allow). This is the intended behavior per the design (auth is opt-in), but the logic means that with auth disabled, *any* user can access *any other user's* preferences without restriction. This matches the design doc ("Auth is gated by `AUTH_ENABLED` config") but should be explicitly noted as a known security trade-off in local dev vs. production.
+
+- [ ] `services/preferences-api/internal/service/preferences.go:17` **Logger type is `*logging.Logger` not `*slog.Logger`** -- The task spec (T5) says `NewPreferencesService(repo port.PreferencesRepository, logger *slog.Logger)` but the implementation uses `*logging.Logger`. This follows the actual codebase pattern (the `logging` package wraps slog), so the implementation is correct and the task spec was slightly wrong. No action needed, but noting the deviation.
+
+- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go` **Handler tests don't validate response envelope `meta` field** -- The spec requires responses in `{data, meta}` format. Tests check `data` but never assert the presence or structure of `meta` (with `request_id`, `timestamp`). This should be verified in at least one test.
+
+- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go:106-141` **`TestPreferences_Update_CreateNew` doesn't set auth context** -- The test creates a PUT request without calling `withAuthUser()`. This works because `authorizeAccess` returns nil when no user is in context, but the test isn't testing the authenticated create path — it's testing the "auth disabled" path. For a feature that specifies auth is required, at least one PUT success test should include auth context.
+
+### Suggestions
+
+- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go` **Add test for PUT with `{"preferences": null}` (ER-9 from QA plan)** -- The QA plan lists this as error case ER-9 but no handler test covers it explicitly. The current code handles it (line 126 checks `string(prefsRaw) == "null"`), but a test would ensure this remains covered.
+
+- [ ] `services/preferences-api/internal/api/handlers/preferences_test.go` **Add test for PUT with extra top-level fields (ER-20 from QA plan)** -- The QA plan lists `{"preferences": {"theme": "dark"}, "extra": true}` as an error case. The handler code does check for unknown top-level keys (lines 131-135), but there's no test exercising this path.
+
+- [ ] `services/preferences-api/internal/domain/preferences_test.go` **Add test for `MergeFrom` with nil notifications but other fields set** -- QA plan UT-D9 specifies this case. The existing "nil update does nothing" test covers `nil` update entirely, but doesn't cover the case where `Theme` and `Language` are set while `Notifications` is `nil`.
+
+- [ ] `services/preferences-api/internal/service/preferences_test.go` **Service tests use in-memory adapter instead of a mock** -- The task spec (T5) says "Tests use mock repository implementing `port.PreferencesRepository`" but the implementation uses the real in-memory adapter. This is a pragmatic choice (the adapter is simple), but coupling service tests to the adapter means adapter bugs could mask service bugs. For a feature this small it's acceptable.
+
+## Spec Alignment
+
+The implementation largely matches the spec. Key alignment points:
+
+| Spec Requirement | Status |
+|-----------------|--------|
+| GET returns 200 with preferences | Implemented and tested |
+| GET returns 404 when none exist | Implemented and tested |
+| PUT creates on first call (upsert) | Implemented and tested |
+| PUT merges with shallow merge | Implemented and tested |
+| PUT returns full merged result | Implemented and tested |
+| PUT validates `preferences` field present | Implemented and tested |
+| PUT rejects unknown top-level preference keys | Implemented and tested |
+| Both endpoints require auth | Implemented (conditional on `AUTH_ENABLED`) |
+| Own-user access + admin override | Implemented and tested |
+| `user_id` validated as UUID | Implemented and tested |
+| In-memory persistence | Implemented and tested |
+| OpenAPI spec documents both endpoints | Implemented |
+| Domain validation for allowed values | Implemented and tested |
+| Example scaffold removed | Confirmed (no example references remain) |
+
+**Gaps:**
+- The handler uses manual JSON decoding rather than `app.BindAndValidate()` as specified in both the design doc and CLAUDE.md critical rules.
+- The time format for `updated_at` doesn't use standard Go time constants.
+
+## Test Coverage Assessment
+
+**23 tests total, all passing.** Coverage by acceptance criterion:
+
+| AC | Criterion | Tests | Coverage |
+|----|-----------|-------|----------|
+| AC-1 | GET 200 | `TestPreferences_GetAfterUpdate` | Covered |
+| AC-2 | GET 404 | `TestPreferences_Get_NotFound` | Covered |
+| AC-3 | PUT upsert | `TestPreferences_Update_CreateNew` | Covered |
+| AC-4 | PUT merge | `TestPreferences_Update_MergeExisting`, domain merge tests | Well covered |
+| AC-5 | PUT returns full set | `TestPreferences_Update_CreateNew` (checks response) | Covered |
+| AC-6 | PUT validates preferences present | `TestPreferences_Update_MissingPreferencesField`, `TestPreferences_Update_EmptyBody` | Covered |
+| AC-7 | PUT rejects unknown keys | `TestPreferences_Update_UnknownPreferenceKey` | Covered |
+| AC-8 | Auth required | Not directly tested (auth disabled in tests) | **Partial** |
+| AC-9 | Own-user + admin | `TestPreferences_Get_Forbidden`, `TestPreferences_Get_AdminAccess`, `TestPreferences_Update_Forbidden` | Covered |
+| AC-10 | UUID validation | `TestPreferences_Get_InvalidUUID`, `TestPreferences_Update_InvalidUUID` | Covered |
+| AC-11 | In-memory persistence | Adapter tests + `TestPreferences_GetAfterUpdate` | Covered |
+| AC-12 | OpenAPI spec | No automated test | **Not tested** |
+| AC-13 | Domain validation rules | Domain tests (7 validation subtests) + `TestPreferences_Update_InvalidTheme` | Covered |
+| AC-14 | Handler test coverage | 13 handler tests | Covered |
+| AC-15 | Service test coverage | 9 service subtests | Covered |
+| AC-16 | Example scaffold removed | Verified via grep | Confirmed |
+
+**Missing test coverage:**
+- AC-8: No test verifies that requests without auth tokens are rejected when auth is enabled
+- AC-12: No test verifies OpenAPI spec renders correctly
+- QA plan ER-9: No test for `{"preferences": null}`
+- QA plan ER-20: No test for extra top-level fields outside `preferences`

.sdlc/state.yaml

@@ -6,9 +6,9 @@ active_work:
         - slug: user-preferences
           phase: implementation
 blocked: []
-last_updated: 2026-02-08T02:01:30.334181257Z
-last_action: COMPLETE_TASK
-last_actor: cli
+last_updated: 2026-02-08T02:08:48.467658449Z
+last_action: NEEDS_FIX_ARTIFACT
+last_actor: user
 history:
     - timestamp: 2026-02-08T01:41:06.381900491Z
       action: CREATE_FEATURE
@@ -95,3 +95,8 @@ history:
       feature: user-preferences
       actor: cli
       result: success
+    - timestamp: 2026-02-08T02:08:48.467657587Z
+      action: NEEDS_FIX_ARTIFACT
+      feature: user-preferences
+      actor: user
+      result: success