diff --git a/.sdlc/features/user-preferences/manifest.yaml b/.sdlc/features/user-preferences/manifest.yaml index b8e7e73..513fe28 100644 --- a/.sdlc/features/user-preferences/manifest.yaml +++ b/.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 diff --git a/.sdlc/features/user-preferences/review.md b/.sdlc/features/user-preferences/review.md new file mode 100644 index 0000000..6ae5536 --- /dev/null +++ b/.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` diff --git a/.sdlc/state.yaml b/.sdlc/state.yaml index 0e76a80..f74be19 100644 --- a/.sdlc/state.yaml +++ b/.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