build: /review-feature user-preferences

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

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

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

@@ -0,0 +1,79 @@
+# Code Review: User Preferences API
+
+## Summary
+**Overall Assessment: PASS**
+
+The implementation is well-structured, follows the hexagonal architecture pattern correctly, and satisfies all acceptance criteria from the spec. All 18 unit tests pass. The code follows project conventions for error handling, request binding, response envelopes, and URL parameter syntax. No blockers found.
+
+## Findings
+
+### Blockers
+None.
+
+### Warnings
+- [ ] `internal/api/handlers/preferences.go:63` — `UpdatedAt` uses hardcoded `Z` format (`"2006-01-02T15:04:05Z"`) instead of `time.RFC3339`. If a non-UTC time is ever passed (e.g., from a database driver returning local time), the `Z` suffix would be incorrect. Currently safe because `time.Now().UTC()` is used consistently and PostgreSQL `TIMESTAMPTZ` stores UTC, but `time.RFC3339` would be more robust.
+- [ ] `internal/service/preferences.go` / `internal/api/handlers/preferences_test.go` — Mock repository is duplicated between service tests and handler tests. Consider extracting to a shared `internal/testutil/` package to avoid maintaining two copies. Low priority since the mock is small and stable.
+
+### Suggestions
+- [ ] `internal/domain/preferences.go` — Domain validation could return all validation errors at once instead of short-circuiting on the first error. This would give clients a better experience when multiple fields are invalid. Low priority for MVP.
+- [ ] `internal/adapter/postgres/preferences.go` — No integration tests for the PostgreSQL adapter. This is acceptable for now since the SQL is straightforward (PK lookup + upsert), but integration tests would increase confidence for future schema changes.
+
+## Spec Alignment
+
+The implementation matches the spec completely:
+
+| Acceptance Criterion | Status | Evidence |
+|---------------------|--------|----------|
+| GET returns stored preferences | PASS | Handler test `returns_stored_preferences` |
+| GET for unknown user returns defaults (not 404) | PASS | Service test `returns_defaults_for_unknown_user`, handler test `returns_defaults_for_unknown_user` |
+| PUT creates preferences (upsert) | PASS | Service test `creates_new_preferences_from_defaults`, handler test `creates_preferences_with_full_update` |
+| PUT merges partial data (deep merge) | PASS | Service test `merges_partial_data_with_existing_preferences`, handler test `creates_preferences_with_partial_update` |
+| PUT validates theme | PASS | Service test `rejects_invalid_theme`, handler test `returns_400_for_invalid_theme` |
+| PUT validates language non-empty | PASS | Service test `rejects_empty_language` |
+| PUT validates notifications.digest | PASS | Service test `rejects_invalid_digest` |
+| notifications.email/push are booleans | PASS | Enforced by Go type system (JSON binding) |
+| Invalid values return 400 | PASS | `mapPreferencesDomainError` maps domain errors to `httperror.BadRequest` |
+| Invalid user_id returns 400 | PASS | UUID parse check in both handlers, tested in handler tests |
+| Standard {data, meta} envelope | PASS | Uses `httpresponse.OK()` |
+| OpenAPI spec documents endpoints | PASS | `spec.go` defines GET/PUT with schemas, parameters, and responses |
+| Preferences persisted in PostgreSQL | PASS | `adapter/postgres/preferences.go` with JSONB column |
+| Database migration creates table | PASS | `migrations/001_create_user_preferences.sql` |
+| Hexagonal architecture | PASS | domain -> service -> port -> adapter separation is clean |
+| Service and handler unit tests | PASS | 10 service tests + 8 handler tests, all passing |
+| Example scaffolding removed | PASS | No example files found |
+
+## Test Coverage Assessment
+
+**Service layer (10 tests):**
+- GetPreferences: defaults for unknown user, stored for existing user
+- UpdatePreferences: create from defaults, partial merge, theme-only, language-only, all fields, invalid theme, invalid digest, empty language
+
+**Handler layer (8 tests):**
+- Get: stored preferences, defaults for unknown, invalid UUID
+- Upsert: full update, partial update, invalid UUID, invalid theme, empty body
+
+**Coverage gaps (non-critical):**
+- No test for invalid digest at handler level (covered at service level)
+- No test for invalid language at handler level (covered at service level)
+- No integration tests for PostgreSQL adapter
+- No test for concurrent access patterns (mock uses sync.RWMutex properly)
+
+All acceptance criteria have at least one corresponding test. The test suite is thorough for an MVP.
+
+## Code Quality Notes
+
+**Strengths:**
+- Clean hexagonal architecture with proper dependency injection
+- Pointer-based partial update pattern correctly distinguishes "not provided" from zero values
+- Deep merge logic is simple and correct
+- Domain validation is self-contained with clear error types
+- Error mapping from domain to HTTP is explicit and maintainable
+- Uses `app.Wrap()`, `app.BindAndValidate()`, `httpresponse.OK()` per project conventions
+- Uses `{user_id}` brace syntax for chi URL parameters (not colon syntax)
+- Compile-time interface verification (`var _ port.PreferencesRepository = ...`)
+- SQL uses parameterized queries ($1, $2) — no injection risk
+- Atomic upsert with `ON CONFLICT DO UPDATE`
+
+**Minor observations:**
+- `toPreferencesResponse` hardcodes time format rather than using `time.RFC3339` (warning above)
+- Mock repository duplicated across test files (warning above)

.sdlc/state.yaml

@@ -6,9 +6,9 @@ active_work:
         - slug: user-preferences
           phase: implementation
 blocked: []
-last_updated: 2026-02-09T02:42:14.016683372Z
-last_action: TRANSITION
-last_actor: cli
+last_updated: 2026-02-09T02:45:08.97591473Z
+last_action: PASS_ARTIFACT
+last_actor: user
 history:
     - timestamp: 2026-02-09T02:15:19.934883983Z
       action: CREATE_FEATURE
@@ -90,3 +90,8 @@ history:
       feature: user-preferences
       actor: cli
       result: success
+    - timestamp: 2026-02-09T02:45:08.975914069Z
+      action: PASS_ARTIFACT
+      feature: user-preferences
+      actor: user
+      result: success