# W2-T5 Review: Company Knowledge Configuration and Public Review Summary

## Verdict

- Spec compliance: ✅ Spec compliant
- Task quality: **Approved**
- Blocking findings: **None**

## Scope Reviewed

- Task brief: `changes/n04-onboard/.superpowers/sdd/task-briefs/T5.md`
- Execution contract: `changes/n04-onboard/execution-contract.md`
- Spec: `changes/n04-onboard/specs/company-knowledge/spec.md`
- Diff: `7370e51..a5730a7`
- Implementer report: unverified implementer claims were checked against the diff and focused tests.

## Strengths

- The two endpoint groups use the required paths and contracts: COMPANY configuration is exposed at `CompanyKnowledgeController.java:32-49`, and the STUDENT company overview is exposed at `CompanyKnowledgeController.java:52-60`.
- COMPANY ownership and required role checks are enforced in `CompanyKnowledgeService.java:357-379`; STUDENT overview access is independently checked in the same service.
- Brand-new configuration returns `configured=false`, version `0`, and null content at `CompanyKnowledgeService.java:62-70`. Configured reads return the expected content and version at `CompanyKnowledgeService.java:323-331`.
- Upsert validation enforces the required per-field limits, at least one non-empty field, trimming, and HTML-tag rejection at `CompanyKnowledgeService.java:132-164`. Existing records require a current version and stale versions return 9706 at `CompanyKnowledgeService.java:92-102`.
- The company overview uses whitelist DTOs only: `CompanyKnowledgeOverviewResponse.java:16-52` excludes `ownerUserId` and other internal company fields, while `ReviewSummaryView.java:17-35` contains no review hashes, moderation reasons, IP, or User-Agent data.
- Public-review filtering is correctly applied at the query layer: `JobReviewRepository.java:35-67` and `JobReviewDimensionScoreRepository.java:46-63` constrain the company scope, `status='PUBLISHED'`, and moderation status `AUTO_APPROVED`/`APPROVED`.
- Threshold behavior is implemented at `CompanyKnowledgeService.java:166-191`: `EMPTY`, `INSUFFICIENT`, and `SUFFICIENT` are distinguished at 0, 1-2, and 3+ reviews, with aggregate queries skipped below 3.
- Tag parsing, deterministic count-descending/lexicographic sorting, and the 10-tag cap are implemented at `CompanyKnowledgeService.java:237-276`. The eight-dimension output is complete and deterministic at `CompanyKnowledgeService.java:287-320`, including zero-sample `INSUFFICIENT` dimensions.
- Error 9706 is defined and mapped to HTTP 409 at `ErrorCode.java:181` and `GlobalExceptionHandler.java:168-172`.
- The focused behavior tests assert observable service and HTTP contracts rather than only implementation shape. The public-filter repository integration test independently excludes non-approved and other-company reviews at `OnboardingRepositoryIntegrationTest.java:197-264`.

## Issues

### Critical (Must Fix)

None.

### Important (Should Fix)

None.

### Minor (Nice to Have)

- `CompanyKnowledgeServiceTest.java:216-237` covers one public review and `:240-287` covers three, but there is no exact two-review case. A threshold mutation from `< 3` to `< 2` would still pass these tests, leaving the 2/3 boundary insufficiently pinned. Add a two-review test asserting `INSUFFICIENT` and no aggregate output.
- The `INSUFFICIENT` branch test at `CompanyKnowledgeServiceTest.java:234-236` verifies the state, count, and that the distribution query is not called, but it does not assert that `averageRating`, `ratingDistribution`, `topTags`, and `dimensionSummary` are absent from the response. Current production behavior is correct at `CompanyKnowledgeService.java:177-181`, but the test would not catch a future mean-leak regression.
- Tag edge coverage is incomplete at `CompanyKnowledgeServiceTest.java:251-254`: the fixture has only three distinct tags and normal JSON representation. Removing the `.limit(MAX_TOP_TAGS)` guard or breaking the recursive H2 outer-quoted JSON handling would not fail the current suite. Add an 11+ tag case and a quoted JSON fixture if the recursive compatibility behavior is intended to remain supported.
- The public-filter integration test covers `PUBLISHED + PENDING` and another company, but not `HIDDEN + APPROVED`. The query itself explicitly filters status and is correct, so this is a coverage gap rather than a current behavior defect.
- `ErrorCode.java:181` and `GlobalExceptionHandler.java:168-172` are outside the T5 brief's explicit file list. The additive changes are necessary for the required 9706/409 contract, but file ownership should be reconciled during T8/documentation review.

## Deferred Concurrency Risk

Two concurrent first-upserts can both observe no existing row; `CompanyKnowledgeService.java:79-89` then attempts two inserts and only `ObjectOptimisticLockingFailureException` is translated at `:123-129`. A unique-key `DataIntegrityViolationException` can therefore escape as a different error. This is not a T5 blocking finding because B3/T6 owns the concurrency gate, but the B3 integration suite should exercise simultaneous first-create and define the expected conflict response.

## Evidence

Command run:

```powershell
$env:JAVA_HOME='D:\java'; $env:Path='D:\java\bin;'+$env:Path
& 'D:\maven\apache-maven-3.9.7\bin\mvn.cmd' '-Dtest=CompanyKnowledgeServiceTest,CompanyKnowledgeControllerTest,OnboardingRepositoryIntegrationTest' test
```

Result:

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

`git diff --check 7370e51..a5730a7` passed. The test output contains existing Spring/Mockito/H2 configuration warnings (Mockito dynamic agent, explicit H2 dialect, open-in-view, generated security password); they did not affect the result and are not introduced by this task.

## Receipt Command

```bash
ssf execution review changes/n04-onboard --wave W2-T5 --base 7370e51 --head a5730a7 --report "D:/1/zhi/zhilai/changes/n04-onboard/.superpowers/sdd/reviews/W2-T5.md" --verdict pass
```