# 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:

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