# W3/T6 Review: Permissions, Error Codes, Concurrency, and Privacy

## Verdict

- **Base:** `a5730a7`
- **Head:** `ca85ab5`
- **Diff:** 6 files, +936/-3
- **Spec compliance:** ❌ Issues found
- **Task quality:** Needs fixes
- **Blocking findings:** 1 Important

## Scope Reviewed

Read the T6 brief, execution contract, checklist and company-knowledge specs, design decisions, reviewer guidance, production diff, and all three added integration-test classes.

Verified the explicit `/onboarding/**` authenticated matcher, STUDENT/COMPANY service role checks, company ownership predicates, checklist HTTP response whitelists, privacy assertions, optimistic-version mappings, and the placement of pessimistic locks relative to transaction reads.

## Evidence

Default focused suite:

```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=OnboardingAuthorizationIntegrationTest,OnboardingConcurrencyIntegrationTest,OnboardingPrivacyTest' test
```

Result:

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

Production-isolation regression:

```powershell
& 'D:\maven\apache-maven-3.9.7\bin\mvn.cmd' -q '-Dtest=OnboardingConcurrencyIntegrationTest' '-Dspring.datasource.hikari.transaction-isolation=TRANSACTION_REPEATABLE_READ' test
```

Result:

```text
Tests run: 10, Failures: 5, Errors: 0, Skipped: 0
```

The focused 49→50 test failed because both concurrent creates succeeded:

```text
concurrentCreatesAtLimitBoundaryNeverWriteFiftyFirstItem
Expected size: 1 but was: 2
```

`git diff --check a5730a7..ca85ab5` passed. No implementation files were modified during review.

## Strengths

- `SecurityConfig.java:130` explicitly marks `/onboarding/**` as authenticated; anonymous requests return 401 through the configured entry point.
- Student and company role checks are enforced in service methods, not only by token claims. Company ownership is checked through `owner_user_id` before item mutation.
- Cross-company update/delete paths return `9701` without returning the target item or modifying it.
- `OnboardingChecklistService.java:554` maps both optimistic-lock and duplicate-insert failures for progress to `9705`; `CompanyKnowledgeService.java:138` similarly maps knowledge conflicts to `9706`.
- Soft-delete preserves history and student effective-checklist queries filter inactive items.
- Privacy tests use response JSON for several write/read/error paths and assert removal of `ownerUserId`, `studentUserId`, hashes, tokens, and moderation internals.

## Issues

### Critical

None.

### Important

1. **The 50-active-item guard is not transaction-safe under the production database's default isolation.** `OnboardingChecklistService.java:153` calls `requireCompanyOwner()`, whose normal read at `OnboardingChecklistService.java:256` establishes the transaction snapshot before `lockCompany()` runs at `OnboardingChecklistService.java:155`. The subsequent `findAll()` at `OnboardingChecklistService.java:168` and active count at `OnboardingChecklistService.java:182` are ordinary consistent reads, so a waiting writer can still observe the pre-commit snapshot. With 49 active rows, two concurrent creates can both observe 49, both pass the check, and both insert, producing 51 active items. MySQL defaults to `REPEATABLE READ` and `application.yml` does not override it. The H2 test at `OnboardingConcurrencyIntegrationTest.java:40-41` also does not set the production isolation level, which is why the default run masks the defect.

   Reproduction command:

   ```powershell
   mvn -q '-Dtest=OnboardingConcurrencyIntegrationTest' '-Dspring.datasource.hikari.transaction-isolation=TRANSACTION_REPEATABLE_READ' test
   ```

   The test fails at `OnboardingConcurrencyIntegrationTest.java:111` with two successful `CompanyChecklistItemView` results instead of one success and one `9703` conflict; the seeded 49 rows plus both inserts means the database has 51 active items.

   **Required fix:** make the post-lock idempotency and active-count decisions from locking/current reads, or explicitly run these write transactions at `READ_COMMITTED`. A company-row `PESSIMISTIC_WRITE` lock alone is insufficient when the transaction snapshot was established by a pre-lock ordinary read. Add a regression test that runs the 49→50 boundary under the same isolation model as MySQL or against MySQL 8.4.

### Minor

1. Several concurrency tests over-constrain outcomes beyond the T6 contract. `OnboardingConcurrencyIntegrationTest.java:75` requires both same-key callers to succeed, while the task brief permits `9704`; `OnboardingConcurrencyIntegrationTest.java:184` and `OnboardingConcurrencyIntegrationTest.java:202` similarly require all progress/knowledge callers to succeed even though the contract permits deterministic `9705`/`9706` conflict responses. Under `TRANSACTION_REPEATABLE_READ`, those tests fail for contract-permitted outcomes. Assert exactly one successful mutation and one allowed conflict/replay, and reject unexpected exception types.

2. `CompanyKnowledgeService.java:135-140` only translates optimistic-lock and integrity exceptions. Under the repeatable-read regression run, the same-version knowledge test surfaced an unmapped H2 transaction-rollback/deadlock exception and failed the `9706` assertion at `OnboardingConcurrencyIntegrationTest.java:238-241`. Verify and, if reproducible on MySQL, map lock acquisition/lock-timeout failures at the service boundary to the appropriate `9702`/`9705`/`9706` contract error.

## Cannot Verify From Diff

- Real MySQL 8.4 execution was not available in this review. The repeatable-read H2 reproduction is sufficient to reject the current locking design, but the final B3 gate must also run the real MySQL path and assert 50 active items after concurrent boundary creation.

## Assessment

The permission, ownership, privacy, and default-isolation tests are well structured, and the focused suite is green. The implementation is not ready to accept because its central concurrency guarantee can be violated under the production MySQL default isolation: concurrent creates can write the 51st active checklist item.

## Receipt

```bash
ssf execution review changes/n04-onboard --wave W3 --base a5730a7 --head ca85ab5 --report changes/n04-onboard/.superpowers/sdd/reviews/W3-T6.md --verdict fail
```