# W2 Wave Review: Onboarding Checklist, Company CRUD, and Company Knowledge

## Verdict

- **Wave:** W2
- **Base:** `3e7c6e7`
- **Head:** `a5730a7`
- **Verdict:** **PASS**
- **Blocking findings:** None
- **Deferred T6 obligations:** Mandatory, as listed below

## Scope Reviewed

Read and cross-checked:

- `changes/n04-onboard/execution-contract.md`
- `changes/n04-onboard/specs/onboarding-checklist/spec.md`
- `changes/n04-onboard/specs/company-knowledge/spec.md`
- `changes/n04-onboard/design.md`
- `.superpowers/sdd/task-briefs/T3.md`
- `.superpowers/sdd/task-briefs/T4.md`
- `.superpowers/sdd/task-briefs/T5.md`
- `.superpowers/sdd/reviews/W2-T3.md`
- `.superpowers/sdd/reviews/W2-T4.md`
- `.superpowers/sdd/reviews/W2-T5.md`

Inspected the combined diff `3e7c6e7..a5730a7`, including all production additions for student checklist access, company checklist CRUD, company knowledge configuration/overview, shared error changes, and the associated service/controller tests.

## Evidence

Command:

```powershell
cd D:\1\zhi\zhilai\mydreamjob-server
$env:JAVA_HOME='D:\java'
$env:Path='D:\java\bin;'+$env:Path
& 'D:\maven\apache-maven-3.9.7\bin\mvn.cmd' '-Dtest=Onboarding*Test,CompanyKnowledge*Test' test
```

Result:

```text
Tests run: 66, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS
```

Per-class results:

- `CompanyKnowledgeControllerTest`: 8 passed
- `CompanyKnowledgeServiceTest`: 14 passed
- `OnboardingChecklistControllerTest`: 5 passed
- `OnboardingChecklistServiceTest`: 10 passed
- `OnboardingCompanyChecklistControllerTest`: 10 passed
- `OnboardingCompanyChecklistServiceTest`: 12 passed
- `OnboardingMigrationTest`: 1 passed
- `OnboardingRepositoryIntegrationTest`: 5 passed
- `OnboardingSeedDataIntegrationTest`: 1 passed

`git diff --check 3e7c6e7..a5730a7` passed with no whitespace errors.

## Cross-Task Integration Findings

### Student checklist and company CRUD

- Company-created items use the same `onboarding_checklist_items` identity space and predicates consumed by the effective checklist query.
- `OnboardingChecklistItemRepository.findEffectiveChecklistItems` returns active STANDARD items plus active COMPANY items scoped to the requested company, with stable `sortOrder ASC, id ASC` ordering.
- Company soft deletion sets `active=false`, which removes the item from the student effective checklist while preserving the row and any student progress history.
- Student progress is keyed by the shared `itemId`, so a company item created through T4 is addressable through T3 without a translation layer.
- Cross-company ownership checks use the same `scopeType`, `scopeId`, and `companyId` predicate for company listing/update/delete paths.

### Company knowledge and checklist interactions

- Checklist and knowledge controllers share only the `/onboarding` namespace and API response conventions; their paths, services, tables, and ownership checks do not collide.
- Company knowledge reads are scoped by `company_id`; student overviews fall back to `BASIC_ONLY` without creating knowledge rows.
- Review aggregation is isolated to published, approved reviews through company jobs and does not mutate checklist or review data.

### Error-code consistency

The new codes are defined once and consistently mapped:

- `9701` `ONBOARDING_CHECKLIST_ITEM_NOT_FOUND` -> HTTP 404
- `9702` `ONBOARDING_CHECKLIST_ITEM_CONCURRENT_UPDATE` -> HTTP 409
- `9703` `ONBOARDING_ITEM_LIMIT_EXCEEDED` -> HTTP 409
- `9704` `ONBOARDING_IDEMPOTENCY_CONFLICT` -> HTTP 409
- `9705` `ONBOARDING_PROGRESS_CONCURRENT_UPDATE` -> HTTP 409
- `9706` `ONBOARDING_KNOWLEDGE_CONCURRENT_UPDATE` -> HTTP 409

The shared exception-handler additions preserve the existing mappings for prior error codes.

### Sensitive-field check

The W2 DTOs are whitelist-shaped. Internal fields such as `ownerUserId`, `studentUserId`, `requestHash`, raw idempotency keys, review hashes, moderation reasons, tokens, IP addresses, and User-Agent values are not exposed in the onboarding response DTOs. Internal repository/entity field usage is restricted to persistence and ownership logic.

### Existing API semantics

The combined diff changes only shared error-code handling outside the new onboarding package. No existing company, job, offer, review, or dimension-score controller path, request contract, response contract, or authorization behavior was changed. The exception-handler edits are additive mappings for the new 97xx codes.

## Non-Blocking Findings

1. `GlobalExceptionHandler.java` was modified even though the W2 task briefs list only the onboarding files and `ErrorCode.java` explicitly. The changes are additive, necessary to realize the approved 404/409 contract, and do not alter existing error mappings. Reconcile ownership and scope wording during T6/T8 so the file is not treated as an unaccounted change.
2. The W2 suite uses service tests and MockMvc with mocked services. It does not yet prove a database-backed end-to-end flow from COMPANY create, through STUDENT checklist read, to STUDENT completion update. T6 integration tests should close this cross-layer gap.
3. Existing review coverage still lacks the exact two-review boundary, a full `HIDDEN + APPROVED` exclusion case, a 10-tag cap/quoted-JSON case, and direct MockMvc assertions for some conflict mappings. These are coverage gaps, not current W2 implementation defects.
4. Real MySQL 8.4 execution remains unverified; the W2 suite uses H2 `MODE=MySQL`. B3/T6 verification must run the approved MySQL path before the final gate.
5. `SecurityConfig` currently relies on `anyRequest().authenticated()` for `/onboarding/**`; the explicit path matcher and complete anonymous/STUDENT/COMPANY/ADMIN matrix remain T6 work.

## Mandatory T6 Obligations Carried Forward

The following must be implemented and proven by T6 before W3 can be considered complete:

### Concurrent company checklist create and limit

1. Fire two concurrent creates with the same company, same normalized `idempotencyKey`, and same body. Assert exactly one row, both callers receive the same successful item or a safe replay, and neither receives an internal 500.
2. Fire two concurrent creates with the same `idempotencyKey` but different bodies. Assert exactly one row, one successful create, and one 409 `9704`, not a unique-constraint 500.
3. Start with 49 active company items and concurrently create two distinct items. Assert the final active count is exactly 50, one request succeeds, one returns 409 `9703`, and no partial or 51st row remains.
4. Start with 50 active company items and assert new creates return 409 `9703`. Prove inactive items are excluded from the active limit and that replaying an existing idempotency key remains successful without counting as a new item.
5. Repeat the idempotency-key race across two companies and assert `(company_id, idempotency_key)` isolation with no cross-company replay.

### First progress version and first-create race

6. When no `student_onboarding_progress` row exists, a request carrying a non-zero `version` must return 409 `9705`, including the first completion path.
7. Exercise simultaneous first progress creation for the same student/item. Assert one row, one safe result, and a deterministic conflict/replay mapping rather than an uncaught unique-constraint 500.

### First company-knowledge upsert race

8. Exercise simultaneous first knowledge upserts for the same company. Assert one row, one successful create, and a deterministic `9706` or other contract-approved conflict response; do not allow an uncaught unique-constraint 500.
9. Confirm the same behavior through the HTTP integration tests with the final error/status mapping.

## Readiness for W3/T6

The W2 implementation and review evidence are sufficient to proceed. T6 has the required service surfaces, shared error constants, ownership predicates, and test scaffolding. If a database-level lock becomes necessary to make the 50-item boundary or first-create race deterministic, update the T6 scope/contract explicitly before editing repository or migration files; do not silently widen the file scope.

## Final Assessment

**Verdict: PASS**

W2 is spec-compliant at the service and controller contract level, passes the required evidence suite, preserves existing API semantics, maintains response-field minimization, and provides the necessary interfaces for T6. The unresolved concurrency and first-create-version behavior is correctly deferred to W3/T6 and must not be closed without the obligations above.