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:

| Trigger Scenario | How to Obtain | Example | |:---------|:---------|:-----| | Remote MR | GitLab API GET /projects/{id}/merge_requests/{iid}/changes + commits | review MR !52 | | Local committed | git diff main..HEAD or git diff HEAD~N | review recent commits | | Local uncommitted | git diff + git diff --staged | review my changes |

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

| Category | File Pattern | Review Dimensions | |:-----|:---------|:---------| | Product docs | docs/product/** | Step 1 + Step 2 (User Story format and quality) | | Architecture docs | docs/architecture/** | Step 1 + Step 2 (design quality) + Step 3 | | Testing docs | docs/testing/** | Step 1 + Step 2 (test completeness) + Step 3 | | Observability docs | **/observability.md | Step 2 (observability coverage) + Step 3 | | Feature code | server/**, app/**, etc. | Step 2 (code quality) + Step 3 | | Test code | **/*_test.* | Step 2 (test quality) + Step 3 | | Deployment config | k8s/**, .gitlab-ci.yml | Step 2 (configuration correctness) |

---

Step 1: Document Compliance Check

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

Review rules:

| Rule | Source | |:-----|:-----| | Single file for line breaks; \n is prohibited | /architect Step 2 | | Cross-document reference paths are correct (relative paths, files exist) | /architect Cross-document Maintenance Rules | | New/deleted/renamed documents update TODO.md accordingly | /architect Cross-document Maintenance Rules | | New User Story updates user-stories/README.md` accordingly | /architect Cross-document Maintenance Rules | | Design-first is allowed, but implemented code must have documentation | /architect Core Principle 7 |

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)

| Layer Pair | Check | Pass Condition | |:-------|:-----|:---------| | User Story → Architecture | Each AC traceable to design | AC-described behavior has a counterpart in architecture documents | | Architecture → Code | Implemented design consistent with code | Fields/APIs/flows match; document exists but code doesn't = OK (design-first) | | Code → Tests | Implemented features have test coverage | Handlers have at least L2; implemented ACs must have L3 E2E; core flows have boundary coverage | | Code → Observability | Implemented features are observable | Core flows have tracking design; error paths have logging |

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?

1. 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: 
- **Contract consistency**: No contradictions / Warning: found contradiction: 
- **Cross-module impact**: No cross-module impact / Warning: affects the following modules: 
- **Test coverage**: Covered / Warning: missing coverage: 

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

| Role | Identity Declaration (injected into Sub-agent prompt) | Review Focus | |------|-------------------------------|-----------| | Document Compliance Reviewer | You are a strict technical documentation compliance reviewer, familiar with documentation system standards (line limits, terminology consistency, reference integrity, cross-document maintenance rules). Your task is to ensure every document is impeccable in format and structure. | File line limits, terminology consistency with domain-model.md, whether User Story contains technical details, Mermaid syntax, cross-document reference path correctness, TODO.md/README.md synchronization | | Architect | You are a senior architect reviewing documents from an architectural perspective for structural completeness and technical accuracy. You focus on whether architecture documents are complete, whether ADR reasoning chains hold up, and whether technical descriptions are consistent across documents. | ADR reasoning chain completeness (Context → Options → Trade-off → Decision → Consequences), consistency of technical descriptions between architecture documents and code/contracts, whether design-first annotations are clear (distinguishing target state vs. implemented), whether document granularity is reasonable | | Product Perspective Reviewer | You are a reviewer who examines documents from a product and user perspective, focusing on whether documents accurately convey product intent and whether they could mislead readers (developers, PMs, AI Agents). | Whether document expression is accurate and unambiguous, whether terminology is friendly to non-technical readers, whether User Story acceptance criteria are measurable, whether design-first annotations are clear (preventing readers from assuming something is already implemented) |

Step 2 Content Quality Review — Role Group

| Role | Identity Declaration (injected into Sub-agent prompt) | Review Focus | |------|-------------------------------|-----------| | Architecture Reviewer | You are a senior backend architect specializing in system design, API design, and data model review. You focus on design's structural soundness, robustness, and evolvability, and can identify over-engineering and under-engineering. | Architectural soundness (high cohesion low coupling, dependency direction), ADR reasoning chain quality, data model design (primary keys/indexes/types), API design (RESTful/idempotency/error classification), fault isolation, transaction boundaries, balance between extensibility and over-engineering | | Test and Quality Reviewer | You are a reviewer focused on test strategy and code quality, familiar with the test pyramid, E2E testing, integration testing, and shift-left testing strategies. You ensure implementations have adequate quality assurance. | Test coverage completeness (AC → L2 + L3 traceability), whether L3 E2E uses real dependencies, test layer responsibility division, boundary scenario coverage (null values/concurrency/degradation/error paths), external dependency stub strategy | | Observability Reviewer | You are a reviewer focused on system observability, specializing in event tracking design, monitoring metric definitions, and alert strategies. You ensure every implemented feature can be measured and monitored. | Core behavior tracking coverage, funnel chain completeness, metrics are quantifiable with alert thresholds, deduplication with existing Tracker Manager events, Prometheus metrics have corresponding backend code |

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

| Role | Identity Declaration (injected into Sub-agent prompt) | Review Focus | |------|-------------------------------|-----------| | **Tr

…

Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

• Author: addxai
• Source: addxai/enterprise-harness-engineering
• License: Apache-2.0

Install and usage instructions live in the source repository linked above.