Code Review

Code Review

Reviews code changes across three dimensions per /architect standards: document compliance, content quality, and end-to-end consistency.

Each review step uses a multi-role parallel, multi-round iterative review mechanism to ensure depth and thoroughness.

Core principles:

• Documents exist, code doesn't → Allowed (design-first)
• Code exists, documents don't → Fail (undocumented implementation)

Execution Flow

Step 0: Collect change scope → Get the diff, classify by file type
Step 1: Document compliance check → Check against /architect format rules item by item (multi-role parallel, minimum 3 rounds)
Step 2: Content quality review → Understand the content, evaluate against /architect quality standards (multi-role parallel, minimum 3 rounds)
Step 3: End-to-end consistency → Trace User Story ↔ design ↔ code ↔ tests ↔ observability (multi-role parallel, minimum 3 rounds)
Step 4: Summary and scoring → Aggregate all round results, red-line determination + improvement suggestions + pass/fail

Hard requirement: Steps 1-3 must have at least 3 rounds of review. Marking Review complete in Round 1 or Round 2 and moving to the next step is not allowed. Round 3 is the final confirmation round, ensuring all findings are thorough and free of false positives.

Step 0: Collect Change Scope

Auto-detect the change source and uniformly obtain the diff:

After obtaining the diff, classify by file type to determine review dimensions:

Step 1: Document Compliance Check

Only check documents involved in the changes, item by item per /architect rules.

Review rules:

Multi-round review: Reviewed in parallel by the Step 1 role group, minimum 3 rounds (see "Multi-Role Review Process" section).

Step 2: Content Quality Review

After understanding the change content, evaluate quality against /architect standards. Review does not define standards; it only evaluates.

Architecture Documents — Per /architect Step 2 Architecture Quality Principles

• Are design goals clear with constraints?
• Does the ADR reasoning chain hold up (not just correct format, but sound conclusions)?
• Is the data model design reasonable (primary key strategy, indexes, field type choices with rationale)?
• Is the API design appropriate (RESTful, error classification, idempotency description)?
• Structural: High cohesion low coupling, reasonable dependency direction, vertical slices, Core separation
• Robustness: Fault isolation strategy, idempotency, transaction boundaries, input validation
• Evolvability: Extension points reserved, interface minimization

Testing Documents — Per /architect Step 5 + testing-strategy Skill Standards

• Does each implemented User Story AC have L3 E2E coverage (API level is insufficient; ACs describe user behavior and must be verified end-to-end)?
• L3 E2E must use real dependencies (real DB, real services), not stubs — stubs are a shift-left strategy (L1/L2); E2E verifies the real path
• Does each implemented API handler have at least an L2 integration test?
• Do external dependencies have built-in Stubs (shift-left testing: L1/L2 should not depend on external services being online; managed via mock-engine)?
• Do test scenarios cover all ACs of the User Story (verified through traceability matrix: AC → L2 + L3)?
• Are boundary scenarios considered (null values, concurrency, degradation, error paths)?
• Is the responsibility division across layers (L1-L4) reasonable (don't use higher-level tests to cover lower-level logic)?

Observability Documents — Per /architect Step 2/4 Observability Standards

• Do core user behaviors have event tracking (exposure/click/conversion funnel)?
• Is the funnel chain complete (entry → conversion → end/abandon)?
• Are metric definitions quantifiable with alert thresholds?
• Is there cross-reference with existing Tracker Manager events (avoid duplication)?
• Do Prometheus metrics have corresponding backend code (at minimum, annotated with implementation plan)?

Feature Code — Per Architecture Documents + Quality Principles

• Does it conform to the design described in architecture documents (fields, types, API paths consistent)?
• Is error handling appropriate (distinguishing retryable vs non-retryable)?
• Are transaction boundaries correct (are cross-table operations within the same transaction)?
• Are degradation strategies implemented (if documentation specifies degradation, does code have corresponding logic)?
• Is input validation in place (at system boundaries)?

Multi-round review: Reviewed in parallel by the Step 2 role group, minimum 3 rounds (see "Multi-Role Review Process" section).

Step 3: End-to-End Consistency Review

Trace the chain (only review parts involved in the changes):

User Stories (docs/product/user-stories/)
  ↓ Does each AC have corresponding technical design?
Architecture Design (docs/architecture/)
  ↓ Is the implemented design consistent with the code?
Feature Code (server/, app/, etc.)
  ↓ Does the implemented feature have tests?
Test Plan + Test Code (docs/testing/ + *_test.*)
  ↓ Does the implemented feature have observability design?
Observability (observability.md + tracking/metrics code)

Multi-round review: Reviewed in parallel by the Step 3 role group, minimum 3 rounds (see "Multi-Role Review Process" section).

Step 4: Summary and Scoring

Aggregate review results from all rounds across Steps 1-3, and output the final conclusion.

Red Lines (fail if found)

• Feature code is implemented but has no corresponding architecture document (undocumented implementation) — treated as a quality issue
• Implemented feature has no E2E test coverage — treated as a defect, not "to be added later"
• Implemented feature has no observability (no tracking, no dashboard, no A/B) — treated as a defect; without measurement, delivery is incomplete
• User Story contains technical implementation details (class names, API paths, database fields)
• Plaintext secrets in code/configuration
• Misleading naming (can mislead AI Agents into generating code based on incorrect semantics)

Pre-Release Acceptance Checklist (check when changes involve new feature launches)

• Analytics metrics and event tracking have been verified for accurate reporting
• Monitoring dashboards have been created and display correctly
• A/B experiments have been configured (if applicable)

Suggested Improvements (non-blocking)

• Document exceeds 400 lines without splitting
• domain-model.md not updated with new terminology
• ADR format incomplete (missing Options or Trade-off)
• Test coverage could be strengthened for boundary scenarios
• Architecture diagrams do not reflect new components described in documentation

Multi-Role Review Process

Steps 1-3 each use the following multi-round review mechanism:

Review Round Rules

Code Review only reviews and outputs opinions; it does not modify code or documents. The purpose of multiple rounds is cross-validation, eliminating false positives, and catching omissions.

1. Round 1 — Independent review: 2-3 independent Sub-agents from the step's role group review in parallel per role definition, each recording discovered issues
2. Round 2 — Cross-validation: The same group of Agents review each other's Round 1 findings, verifying whether false positives exist, whether issues were missed, whether findings from different roles contradict each other, and adding new findings
3. Round 3+ — Final confirmation: At least a third round of review. Each Agent confirms all findings are thorough, free of false positives, and free of omissions, reaching consensus on issue severity
4. Consensus: All Agents mark Review complete in the review record before proceeding to the next step

Hard requirement: Minimum 3 rounds of review. Marking Review complete in Round 1 or Round 2 and moving to the next step is not allowed. Round 3 is the final confirmation round, ensuring all findings are thorough and free of false positives.

Impact Scope Assessment (required in every review round)

This is one of the most important review responsibilities. In every round of review, every Agent must perform an impact scope assessment.

In every review round, all Agents must additionally perform the following checks:

1. Document consistency: Does this change contradict existing PRDs, User Stories, or design documents under docs/? If inconsistencies are found, they must be noted in the review opinion and correction must be requested
2. Contract consistency: Does this change contradict API contracts or data schemas? Have interface changes been synchronized with contract documents?
3. Cross-module impact: Does this change affect code, tests, or documents in other modules? For example:
   • Modified API contract → Have callers been updated accordingly?
   • Modified data structure → Have persistence layer and serialization layer been updated accordingly?
   • Modified state logic → Have all modules depending on that state been updated accordingly?
4. Test coverage: Do the features affected by this change have corresponding test cases? Are new features covered in the test plan?

Impact scope assessment result recording format:

### Impact Scope Assessment — Round N
- **Document consistency**: No contradictions / Warning: found contradiction: <specific description>
- **Contract consistency**: No contradictions / Warning: found contradiction: <specific description>
- **Cross-module impact**: No cross-module impact / Warning: affects the following modules: <list>
- **Test coverage**: Covered / Warning: missing coverage: <specific description>

Review Role Definitions

Each review step uses different combinations of Sub-agent roles. When launching a Sub-agent, the corresponding role's identity declaration + review focus areas must be injected at the beginning of the prompt, so it reviews from that professional perspective.

Step 1 Document Compliance Check — Role Group

Step 2 Content Quality Review — Role Group

Step 3 End-to-End Consistency Review — Role Group

Role Injection Method

When launching a review Sub-agent, the prompt structure is:

{Role identity declaration}

You are reviewing the following changes: {change scope description}
Review focus: {this role's review focus areas}
Current round: Round {N}

Please complete the following tasks:

**Task 1: Item-by-item review**
Please review item by item and output your opinions in this format:
- [ ] <specific issue description, with file path and line number references>

If no new issues are found (only allowed in Round 3+), output: Review complete — all findings confirmed

**Task 2: Cross-validation** (required for Round 2+)
Review other roles' findings from the previous round and determine:
- Are there false positives? Mark `[False positive]` with reasoning
- Are there omissions? Add new findings
- Do findings from different roles contradict? Mark and provide judgment

**Task 3: Impact scope assessment**
Please assess the impact scope of this change, check the following areas and output results:
- **Document consistency**: Does it contradict existing documents under docs/?
- **Contract consistency**: Does it contradict API contracts or data schemas?
- **Cross-module impact**: Does it affect code, tests, or documents in other modules?
- **Test coverage**: Do affected features have test case coverage?

Review Record Format

Multi-round review results for each Step are recorded in the output in this format:

## Step N: <Step Name> Review

### Round 1 — Independent Review

#### Role A
- [ ] <Issue 1, with file path and line number>
- [ ] <Issue 2>

#### Role B
- [ ] <Issue 1>

#### Impact Scope Assessment — Round 1
- **Document consistency**: No contradictions
- **Contract consistency**: Warning: found contradiction: <specific description>
- **Cross-module impact**: Warning: affects the following modules: <list>
- **Test coverage**: Covered

### Round 2 — Cross-Validation

#### Role A
- **Cross-validation**: Role B's Issue 1 confirmed valid / [False positive] <reasoning>
- [ ] <Newly discovered issue (if any)>

#### Role B
- **Cross-validation**: Role A's Issue 1 confirmed valid; Issue 2 [False positive] — <reasoning>
- [ ] <Newly discovered issue (if any)>

#### Impact Scope Assessment — Round 2
- **Document consistency**: No contradictions
- **Contract consistency**: Warning: confirmed contradiction exists (Round 1 finding stands)
- **Cross-module impact**: Supplementary confirmation that impact scope is complete
- **Test coverage**: Covered

### Round 3 — Final Confirmation

#### Role A
Review complete — all findings confirmed, no omissions

#### Role B
Review complete — all findings confirmed, no omissions

#### Impact Scope Assessment — Round 3
- **Document consistency**: No contradictions
- **Contract consistency**: Warning: maintaining Round 1 finding (awaiting code author's fix)
- **Cross-module impact**: No omissions
- **Test coverage**: Covered

### Final Conclusion
- Role A: Review complete (Round 3 confirmed)
- Role B: Review complete (Round 3 confirmed)
- **Confirmed issues: N, false positives excluded: N**

Output Structure

After multi-round review is completed within Sub-agents, final results must be output in the following 6-section structure.

Output Destination

For MR scenarios, submit via GitLab API:

curl -s --request POST --header "PRIVATE-TOKEN: $GITLAB_TOKEN" \
  --header "Content-Type: application/json" \
  --data '{"body": "<full 6-section review results>"}' \
  "https://gitlab.example.com/api/v4/projects/{project_id}/merge_requests/{iid}/notes"

MR scenario conversation summary:

- **Conclusion**: Pass / Conditional pass / Fail
- **Score**: X/10
- **Red-line issues**: List (if any)
- **Key findings**: List top 3-5 issues
- **Detailed review written to MR notes**: <MR link>

6-Section Output Structure

1. Scope

- **Change source**: MR !XX / local N commits / local uncommitted
- **Changed files**: N files
- **Modules involved**: List product modules/services

2. Document Compliance

Final conclusion from multi-round review, listed item by item per rule:

Attach multi-round review records (per Review Record Format, including each round's role opinions, cross-validation, and impact scope assessments).

3. Content Quality

Evaluate item by item per file types involved in the changes. Provide specific suggestions, referencing file paths and line numbers.

• Architecture documents: Design goals, ADR quality, data model, API design, structural/robust/evolvability
• Testing documents: AC coverage, L3 E2E real dependencies, test layer division, boundary scenarios
• Observability: Tracking coverage, funnel completeness, quantifiable metrics, alert thresholds
• Feature code: Consistency with architecture documents, error handling, transaction boundaries, degradation strategy, input validation

Attach multi-round review records.

4. End-to-End Consistency

Traceability matrix (final conclusion from multi-round review):

Attach multi-round review records.

5. Summary

- **Conclusion**: Pass / Conditional pass / Fail
- **Score**: X/10
- **Should pass**: Yes/No, with reasoning
- **Red-line issues**: List (if any)
- **Suggested improvements**: Prioritized (P1/P2)
- **Review round statistics**: Step 1: N rounds / Step 2: N rounds / Step 3: N rounds

6. Multi-Round Review Process Summary

Summary of review rounds, role participation, key cross-validation conclusions, and false positive exclusion statistics for each Step.

Examples

Bad — Only checks format, not content

overview.md <= 400 lines
domain-model updated
→ "Pass"

Problem: Format is compliant but content was not read. overview.md describes 3 new tables, none implemented in code and none annotated as "to be implemented," leading readers to believe everything is already in place.

Bad — Single-round pass, skipping multi-round verification

Step 1 Round 1:
  Document Compliance Reviewer: No issues
  Product Perspective Reviewer: No issues
→ Proceed to Step 2

Problem: Violates the minimum 3-round rule. Even if Round 1 has no issues, Round 2 and Round 3 confirmation rounds must still be executed.

Good — Three-dimensional review that reads and understands content before judging (final output)

1. Document compliance: overview.md 436 lines → Fail, exceeds 400-line limit
2. Content quality: species_content uses 300KB JSON blob storage, ADR did not document trade-off → Warning, add ADR
3. End-to-end consistency:
   - species_content in doc but not in code → Allowed (design-first)
   - handleSpeciesDetail() still SELECTs description but doc says it was deleted → Fail, implemented portion inconsistent with documentation
→ "Conditional pass, 7/10"

Good — Multi-round cross-validation process (review process record)

Step 2 Round 1 (independent review):
  Architecture Reviewer:
  - [ ] overview.md:136 — species_content uses 300KB JSON blob, ADR missing trade-off
  - [ ] api.md:52 — DELETE /species/{id} missing idempotency description
  Test and Quality Reviewer:
  - [ ] test-plan.md — species_content CRUD missing L3 E2E test cases
  Observability Reviewer:
  - [ ] observability.md — species_content write success rate has no monitoring metric

Round 2 (cross-validation):
  Architecture Reviewer:
  - Cross-validation: Test reviewer's L3 E2E gap confirmed valid; observability reviewer's metric gap confirmed valid
  - [ ] overview.md:142 — Supplementary finding: ADR also missing "Consequences" section
  Test and Quality Reviewer:
  - Cross-validation: Architecture reviewer's ADR trade-off gap confirmed valid; idempotency issue confirmed valid
  Observability Reviewer:
  - Cross-validation: All Round 1 findings confirmed valid, no false positives

Round 3 (final confirmation):
  Architecture Reviewer: Review complete — 5 issues confirmed, no omissions
  Test and Quality Reviewer: Review complete — all findings confirmed
  Observability Reviewer: Review complete — all findings confirmed
  Confirmed issues: 5, false positives excluded: 0

→ "Conditional pass, 7/10"

Bad — Treating design-first as an error

species_content table: Document has DDL, code not implemented → "Design and implementation inconsistent"

Problem: Design-first is allowed. Documents describe the target state; code catching up incrementally is normal.

Good — Distinguishing design-first from undocumented code

species_content table: In doc but not in code → Allowed (design-first)
handleDeleteEvent(): Code has soft-delete logic, not mentioned in documentation → Undocumented implementation

Relationship with Other Review Skills