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

W2 T4 Review: Company Custom Checklist CRUD and Idempotency

Verdict

• Spec compliance: PASS
• Task quality: Approved
• Overall: PASS
• Base: 037ddc7
• Head: 7370e51
• Blocking findings: none
• Deferred blocker: concurrent create / 50-item limit races are not resolved by this task and must be treated as mandatory T6 concurrency-gate obligations.

Verification

• Read task-reviewer-prompt.md, writing-good-tests.md, task-briefs/T4.md, the execution contract, specs/onboarding-checklist/spec.md, the design, and the complete 037ddc7..7370e51 diff. Focused reads were used only where the diff output was truncated mid-function.
• Scope check: the commit changes 9 files: the 7 files named by the T4 brief plus additive changes to ErrorCode.java and GlobalExceptionHandler.java.
• Re-ran the T4 evidence command from mydreamjob-server with JAVA_HOME=D:\java and Maven 3.9.7.
• Result: Tests run: 22, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.
• git diff --check 037ddc7..7370e51 reported no whitespace errors.
• RED-before-GREEN ordering cannot be reconstructed from the final commit; the independently reproduced current HEAD is GREEN.

Spec Compliance

• COMPANY ownership is enforced centrally: requireCompanyOwner checks the COMPANY role and resolves the company by ownerUserId, missing roles return FORBIDDEN, and missing company profiles return COMPANY_NOT_FOUND (OnboardingChecklistService.java:229-239). The role/profile tests are at OnboardingCompanyChecklistServiceTest.java:269-297.
• Company listing is isolated to the current company and includes inactive rows: findAll() results are filtered by scopeType=COMPANY, scopeId, and companyId, then sorted (OnboardingChecklistService.java:121-131, :241-257). The test verifies own active/inactive items and excludes another company and a STANDARD item (OnboardingCompanyChecklistServiceTest.java:47-68).
• Create validation covers title, description, category, sortOrder, required default, and idempotency-key length in both the request DTO and service (CompanyChecklistItemCreateRequest.java:9-33, OnboardingChecklistService.java:266-289). Invalid requests return BAD_REQUEST before repository access (OnboardingCompanyChecklistServiceTest.java:71-88).
• Create normalization and hashing are deterministic: title/description/idempotency key are trimmed, blank optional strings become null, required defaults to false, sortOrder defaults to 1000, and the canonical request is SHA-256 hashed (OnboardingChecklistService.java:140-148, :315-349). The persistence test verifies the normalized entity and hash shape (OnboardingCompanyChecklistServiceTest.java:91-122).
• Sequential idempotency replay and conflict behavior are correct: an existing (company, idempotencyKey) with the same hash returns the original row without a second save; a different hash raises 9704 (OnboardingChecklistService.java:150-163; tests at OnboardingCompanyChecklistServiceTest.java:125-166).
• The 50-active-item boundary is enforced in the sequential path: active rows are counted, and 50 existing active rows produce 9703 before save (OnboardingChecklistService.java:165-170; test at OnboardingCompanyChecklistServiceTest.java:169-185).
• Update applies every mutable field and checks the supplied version before mutation (OnboardingChecklistService.java:189-209). Stale versions return 9702 and do not save (OnboardingCompanyChecklistServiceTest.java:188-228).
• Delete is a soft delete and repeated deletion is idempotent: the active flag is set false with the current version, then a second delete returns the already-inactive row without another save (OnboardingChecklistService.java:211-227; test at OnboardingCompanyChecklistServiceTest.java:231-251).
• Cross-company mutation is blocked by the same ownership predicate used by update and delete; an item belonging to another company returns 9701 and remains unchanged (OnboardingChecklistService.java:252-264; delete isolation test at OnboardingCompanyChecklistServiceTest.java:254-266).
• The response DTO is whitelist-only and exposes item identity, source, key, display fields, sort/active/version; it does not expose company IDs, owner IDs, idempotency keys, request hashes, timestamps, or internal audit fields (CompanyChecklistItemView.java:14-23).
• Error mappings are correct: 9702, 9703, and 9704 are defined (ErrorCode.java:175-179) and map to HTTP 409 (GlobalExceptionHandler.java:167-172); 9701 remains mapped to 404. Controller tests cover 409 for 9702/9704 and 404 for 9701 (OnboardingCompanyChecklistControllerTest.java:120-132, :167-189, :211-233).
• Controller wiring matches the fixed endpoints and role-neutral transport contract: all four company CRUD methods resolve the authenticated user and delegate to the service (OnboardingChecklistController.java:65-108). Authentication is covered at OnboardingCompanyChecklistControllerTest.java:235-250.

Test Falsifiability

• The service tests assert observable results and side effects: persisted scope/company/defaults, replay returning the original ID with one save, conflict preventing a save, stale version preventing mutation, and repeat deletion not saving twice. Each has a plausible production mutation that would fail it.
• Controller tests exercise real MockMvc security, validation, serialization, and exception mapping with a mocked service. They would catch wrong paths, missing role passthrough, weak request validation, and incorrect 409/404 mappings.
• The 50-item test catches blocking at exactly 50, but does not independently prove that 49 active items are allowed or that inactive items do not count toward the limit. The implementation is correct by inspection, but the lower boundary/filtering semantics are not fully falsifiable from the current test.
• Cross-company isolation is directly tested for delete only. Update and list use the same ownership predicate, and the controller test mocks the cross-company update result, so the service-level update isolation path is not independently exercised.
• The HTTP mapping for 9703 is statically present but has no focused MockMvc assertion; only the service error code is tested. This is non-blocking because T6 owns the full HTTP/concurrency integration gate.

Concurrent Create / Limit Race Disposition

The reported race is real, but it is not a blocking finding for T4.

• The current create path reads findAll() and checks idempotency/active count before inserting (OnboardingChecklistService.java:150-186). There is no row lock or atomic counter.
• V38 does have a unique key on (company_id, idempotency_key) (V38__onboarding_checklist_and_company_knowledge.sql/equivalent migration line 27; entity declaration OnboardingChecklistItem.java:29-35). Simultaneous identical creates can therefore make one request fail with DataIntegrityViolationException; saveCompanyItem only translates optimistic-lock failures (OnboardingChecklistService.java:366-373), so the loser can surface as a 500 instead of a safe replay.
• The 50-item limit is also non-atomic: two concurrent creates at 49 active items can both observe 49 and insert, producing 51 active items.
• T4's brief and Done Definition require sequential validation, replay/conflict, and the 50-item boundary, but do not assign concurrent execution to T4. The execution contract maps concurrency to B3, and the T6 brief explicitly requires concurrent/idempotent conflict behavior, error mapping, no partial writes, and the concurrency integration test (task-briefs/T6.md:12, :18, :21, :30-33, :51).

Mandatory T6 test obligations:

1. Fire two concurrent creates with the same company, key, and normalized body: assert exactly one row, both callers receive the same successful item (or the loser receives the same successful replay), and neither receives an internal/500 response.
2. Fire two concurrent creates with the same key and differing bodies: assert exactly one row, one successful create, and one 409 9704, not a unique-constraint 500.
3. Start with 49 active items and concurrently create two distinct items: assert the final active count is 50, one request succeeds, one returns 409 9703, and no partial/extra row remains.
4. Start at 50 active items and verify new creates return 409 9703; verify replaying an existing idempotency key still returns the original item without counting as a new item.
5. Repeat the key test across two companies to prove (company_id, idempotency_key) isolation and no cross-company replay.

Issues

Critical (Must Fix)

None.

Important (Should Fix)

None for T4. The concurrent create/limit race is deferred by the approved execution plan to T6, but it must not be closed without the obligations above.

Minor (Nice to Have)

• Strengthen OnboardingCompanyChecklistServiceTest.java:169-185 with a 49-active success case and an inactive-item-does-not-count case; the current test proves the upper rejection only.
• Add a service-level cross-company update test alongside the existing delete isolation test, or explicitly retain this as a T6 integration obligation.
• Add a focused MockMvc assertion for 9703 -> HTTP 409, since only the service error code is currently exercised for the limit path.
• ErrorCode.java and GlobalExceptionHandler.java are outside the seven files named by task-briefs/T4.md:14-23. The additions are limited to the T4 error codes and are necessary for the required 409 contract; reconcile ownership in the task ledger so T6 does not duplicate or renumber them.

Residual Verification Warning

• Real MySQL 8.4 execution and true concurrent JPA behavior were not exercised by this T4 unit/MockMvc suite. H2-backed MockMvc tests passed; the remaining database/concurrency proof belongs to the B3/T6 verification gate.

Assessment

Task quality: Approved

Reasoning: T4 implements the approved sequential company CRUD, ownership isolation, validation, deterministic idempotency replay/conflict, optimistic update, soft-delete idempotency, DTO minimization, and error-code mappings. The only blocking-risk item is the explicitly deferred concurrent create/limit race, which must be proven and fixed at T6.

Controller receipt command when ssf is available:

ssf execution review changes/n04-onboard --wave W2 --base 037ddc7 --head 7370e51 --report changes/n04-onboard/.superpowers/sdd/reviews/W2-T4.md --verdict pass