lxq/changes/n04-onboard/.superpowers/sdd/reviews/W2-T3.md

W2 T3 Review: Student Checklist and Completion APIs

Verdict

• Spec compliance: PASS
• Task quality: Approved
• Overall: PASS
• Base: 3e7c6e7
• Head: 037ddc7
• Blocking findings: none
• Cannot verify from this task alone: actual MySQL persistence behavior and a concurrent first-create race; these remain B3 integration-gate checks.

Verification

• Read task-reviewer-prompt.md, writing-good-tests.md, the T3 brief, execution contract, checklist specification, design response/error contracts, the complete 3e7c6e7..037ddc7 diff, and focused source/test files where the diff output was truncated.
• Scope check: the diff contains exactly ten files: the eight planned T3 files plus additive changes to ErrorCode.java and GlobalExceptionHandler.java.
• Re-ran the T3 evidence command from mydreamjob-server with JAVA_HOME=D:\java and Maven 3.9.7.
• Result: Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.
• git diff --check 3e7c6e7..037ddc7 reported no whitespace errors.
• The final commit cannot reconstruct RED-before-GREEN ordering, but the independently reproduced current HEAD is GREEN.

Spec Compliance

• Effective checklist fallback is correct: OnboardingChecklistService.java:45-48 delegates to OnboardingChecklistItemRepository.java:13-25, whose query returns active standard items plus active items for the requested company only. OnboardingChecklistServiceTest.java:49-74 verifies the standard-only fallback and customized=false.
• Merging and stable ordering are correct: OnboardingChecklistService.java:114-126 sorts by sortOrder ASC, id ASC; :154-169 builds the merged response and sets customized from the presence of a COMPANY item. OnboardingChecklistServiceTest.java:77-103 independently verifies order 2,3,1, source, progress, and customized=true.
• Company existence is enforced before checklist/item/progress work in both GET and PUT paths (OnboardingChecklistService.java:42-43, :62-63). OnboardingChecklistServiceTest.java:121-130 verifies COMPANY_NOT_FOUND and no later repository interaction.
• STUDENT-only access is enforced at service level with FORBIDDEN for non-students (OnboardingChecklistService.java:104-111); OnboardingChecklistServiceTest.java:133-142 verifies the behavior before company lookup.
• Completion PUT uses absolute state semantics and avoids writes for an identical state (OnboardingChecklistService.java:77-89). OnboardingChecklistServiceTest.java:175-191 verifies a stale version is ignored for an identical state, returns current version/completedAt, and performs no save.
• First-create behavior is correct for the service contract: a new progress entity defaults to version 0 (StudentOnboardingProgress.java:52-55), is saved once, and the response returns version 0 when no persisted version exists (OnboardingChecklistService.java:90-99, :185-186). OnboardingChecklistServiceTest.java:145-172 verifies the inserted entity, completedAt, count, total, and rate.
• Stale-version conflict behavior is correct: a different desired state with a non-current version raises ONBOARDING_PROGRESS_CONCURRENT_UPDATE before mutation (OnboardingChecklistService.java:82-85), defined as 9705 in ErrorCode.java:177, and mapped to HTTP 409 in GlobalExceptionHandler.java:167-169. OnboardingChecklistServiceTest.java:193-210 verifies the error code and no save.
• completedAt lifecycle is correct: first completion writes a timestamp, cancellation clears it, and identical-state requests preserve it (OnboardingChecklistService.java:86-95, :183-184). Covered by OnboardingChecklistServiceTest.java:145-172, :175-191, and :213-238.
• Completion-rate math is correct and deterministic: the service uses the effective item IDs, counts only completed rows, returns 0.0 for an empty list, and rounds one decimal place (OnboardingChecklistService.java:215-233). Tests cover 0/1, 1/1, empty, orphan-row exclusion, and 1/3 = 33.3 (OnboardingChecklistServiceTest.java:49-118, :77-103).
• Item scoping is correct: PUT resolves the item from the effective checklist query, so an item from another company, an inactive item, or a missing item produces 9701 without touching progress (OnboardingChecklistService.java:65-71); OnboardingChecklistServiceTest.java:240-253 verifies no progress interaction.
• DTOs are whitelist-only and contain no owner, token, IP, moderation, or internal audit fields (ChecklistItemView.java:14-27, EffectiveChecklistResponse.java:14-22, ProgressUpdateRequest.java:8-14, ProgressUpdateResponse.java:14-22). Controller tests assert the externally visible response contract (OnboardingChecklistControllerTest.java:44-103).
• The two task errors are mapped correctly: 9701 maps to 404 and 9705 maps to 409 (ErrorCode.java:175-177, GlobalExceptionHandler.java:65-80, :167-169). The service tests independently assert the business error codes.

Test Falsifiability

• The service tests exercise observable service behavior rather than implementation-only helpers: fallback, merge order, progress aggregation, idempotency, conflict, timestamp lifecycle, and scoping each have a plausible production mutation that would fail them.
• The controller tests exercise real MockMvc security/exception handling with a mocked service; they verify authentication, role error propagation, company-not-found status, request identity, and serialized DTO fields.
• OnboardingChecklistServiceTest.java:77-103 specifically catches removal or incorrect rounding of the one-decimal completion rate through 1/3 = 33.3.
• The main test gap is status mapping for 9705: the service test proves the error code and GlobalExceptionHandler.java:168-169 proves the 409 branch statically, but no controller/integration test would fail if that handler branch were removed. This is non-blocking because the mapping is present and B3 owns the HTTP integration matrix.

Issues

Critical (Must Fix)

None.

Important (Should Fix)

None.

Minor (Nice to Have)

• OnboardingChecklistControllerTest.java:105-135 covers 401, 403, and 404 but has no PUT case returning 409 for ONBOARDING_PROGRESS_CONCURRENT_UPDATE. Add a focused MockMvc assertion for status 409 and code 9705, or retain it as the B3 end-to-end obligation.
• GlobalExceptionHandler.java:65-80 and ErrorCode.java:175-177 are outside the explicit eight-file T3 scope (task-briefs/T3.md:14-23). Both changes are additive and necessary for the required 404/409 contract, and ErrorCode.java is explicitly allowed by the execution contract; the task brief and T6 ownership should be reconciled so T6 does not duplicate or unintentionally renumber them.
• The focused test run emits existing Spring/Mockito/H2 configuration warnings from the full @SpringBootTest controller context (OnboardingChecklistControllerTest.java:34-42). They do not affect the result, but a cleaner test context would reduce output noise.

Residual Verification Warning

• The tests use mocks for the first insert, so the actual JPA insert version and a simultaneous first-create race are not established here. The current code can rely on the unique constraint for sequential idempotency, but concurrent first inserts should be exercised at the B3 gate.
• Real MySQL 8.4 execution was not performed; H2 MODE=MySQL passed and the remaining MySQL proof belongs to the verification gate.

Assessment

Task quality: Approved

Reasoning: T3 implements the approved student checklist and completion behavior with correct fallback, ordering, scoping, idempotency, timestamp lifecycle, rate calculation, DTO minimization, and error-code behavior. The only findings are non-blocking test/plan reconciliation items.

Controller receipt command when ssf is available:

ssf execution review changes/n04-onboard --wave W2 --base 3e7c6e7 --head 037ddc7 --report changes/n04-onboard/.superpowers/sdd/reviews/W2-T3.md --verdict pass