[Feature]: Add a first-class code-review stage after code generation

[jeromevdl]

Description

Add a per-unit code-review stage after code-generation and before build-and-test.

The review should independently verify:

• Requirements, user stories, acceptance criteria, business rules, and NFR completeness.
• Correctness, edge cases, error handling, concurrency, reliability, and performance.
• Clean code, DRY, KISS, YAGNI, SOLID, maintainability, and dead code.
• Compliance with existing project architecture, patterns, libraries, and standards.
• Test quality, meaningful coverage, regressions, and failure paths.
• Security, privacy, secrets, OWASP/CWE risks, dependencies, and supply-chain risks.
• API, schema, migration, and backward compatibility.
• Configuration, observability, accessibility, and documentation.
• Formatter, lint, type/static analysis, targeted tests, coverage, secret scanning, and configured dependency checks.

Use a dedicated aidlc-code-reviewer-agent. The developer agent should repair blocking findings, followed by re-review, for at most two repair rounds.

Use Case

Code Generation currently creates code and its summary within one stage and uses the architecture reviewer as an embedded reviewer. This does not provide a distinct, comprehensive source-code review boundary.

A first-class stage would ensure generated code:

• Implements everything agreed in preceding stages.
• Follows established repository conventions.
• Does not introduce unapproved dependencies or patterns.
• Includes adequate, meaningful tests.
• Does not introduce security issues or breaking changes.
• Has a durable, auditable review result before broader build and integration testing.

For gated Construction, each unit should require review approval. For autonomous Construction, units should be reviewed without interruption and presented at one aggregate gate.

Version

v2 (alpha)

Area

Construction phase rules

Additional Context

Proposed lifecycle:

code-generation -> code-review -> build-and-test

Express should remain the documented no-review path.

Suggested inputs:

• Implementation change manifest
• Approved code-generation plan and testing contract
• Requirements and user stories
• Acceptance criteria and business rules
• Functional and NFR designs
• Contracts and unit definitions
• Project practices, standards, and CodeKB context

Suggested outputs:

• code-review.md
• code-review-findings.json
• code-summary.md
• Final traceability.json

Severity behavior:

• Critical correctness, security, privacy, data-loss, and unapproved breaking-change findings are non-waivable.
• Major findings require an explicit human waiver with rationale.
• Minor findings remain visible but do not block approval.

Executable checks should use project-configured tools without installing new dependencies. Code Review should run targeted checks; the following Build and Test stage remains responsible for the complete build and broader integration/E2E suites.

When Code Review is present, it should replace Code Generation’s existing completion gate. The separate per-unit Code Generation Plan Approval remains mandatory.

[apackeer]

Thanks for the detailed proposal — after verifying it against the current tree, I support the direction, including the standalone stage. Below is an assessment of what already exists, the design decisions the stage forces, and a proposed single-PR delivery that lines up with work already in flight (#629/#646, #662).

The premise checks out

• code-generation embeds reviewer: aidlc-architecture-reviewer-agent (core/aidlc-common/stages/construction/code-generation.md), and that persona is explicitly a design-artifact reviewer — systems thinking, cross-references, quality-target feasibility (core/agents/aidlc-architecture-reviewer-agent.md). It is not a source-code reviewer.
• The §12a reviewer pass-list is the stage's produces[] record artifacts plus upstream paths (core/aidlc-common/protocols/stage-protocol-reviewer.md, Flow step 1). Generated application code lives at the workspace root, outside the record dir — so today's review covers the artifacts about the code more than the code itself.
• Review findings are not durable: §12a deletes any prior ## Review section on every re-dispatch; history survives only as audit rows. There is no reviewable, persistent review record.

So the gap is real: no comprehensive, source-bound, durable review boundary exists.

Why a standalone stage (not just a better reviewer on the existing seam)

The framework already decomposes exactly along the lines this issue proposes: agents answer who, stages answer when/how. Code review in real life is its own ceremony (a PR) with its own record and its own approval — modeling it as a property of code generation undersells it. Concretely, the stage buys four things the embedded reviewer: seam structurally cannot provide:

1. Durable artifacts. code-review.md + code-review-findings.json as declared produces[], covered by the artifact guard and sensors — versus §12a's deleted-on-redispatch review sections. The issue's "durable, auditable review result" requires this.
2. Scope selectability. A stage declares its own scopes: list — enterprise/feature can require code review while poc omits it, independently of other reviewers. The embedded seam only offers review_cap, which lowers every reviewer in a scope at once. Express stays the no-review path simply by omitting it from scopes:.
3. Its own sensors and gate. Formatter, secret-scan, and coverage checks belong in sensor manifests attached to the review stage (the framework's determinism→tool split — lint/type-check already work this way on code-generation), running at review time, with the review verdict as the primary content of its own approval gate.
4. Cost relocation, not addition. If the stage replaces code-generation's embedded architecture-review pass, per-unit review passes stay ~constant — relocated to a better boundary with the right persona. Design soundness remains covered upstream: functional-design, nfr-design, and infrastructure-design keep their architecture reviews.

Design decisions the stage forces (the real work)

1. Repair topology — the PR analogy answers it; the schema can't express it yet.
In a PR, the reviewer reviews and the author fixes. The stage's lead should therefore be aidlc-code-reviewer-agent (it authors the findings artifacts), with blocking-finding repairs dispatched to aidlc-developer-agent. But the generic §12a loop re-invokes the stage's lead to repair — so stage-definition.md needs an explicit repair-agent concept (e.g. a repair_agent: field or a review-stage protocol variant). Without it we get either self-review (reviewer fixes its own findings) or a developer-led stage that recreates today's topology.

Related spec nit: reviewer_max_iterations: 2 today means two review passes (≈ one repair round). The proposal's "at most two repair rounds" implies three passes — worth stating which is intended.

2. Autonomous placement — review where PRs happen: on the branch, pre-merge.
In autonomous Construction, units live in Bolt worktrees merged at finalize (core/aidlc-common/protocols/stage-protocol-swarm.md). A stage that runs after code-generation completes would review merged code — the PR analogy inverted, and per-unit isolation lost. The Bolt needs to become a two-stage per-unit pipeline: generate → review inside the unit's worktree, then finalize. That is genuine engine work, and it's also where the guarantee is strongest: the worktree route is the one place #646 gives true per-unit fingerprint attribution. Gated/sequential mode is easy by comparison — the stages just chain per unit, and the aggregate-gate behavior for autonomous runs falls out of the existing Bolt gate model.

3. Gate replacement resolves at graph-compile time.
"When Code Review is present, it replaces Code Generation's completion gate" should be a compile-time consequence of the active scope: scope includes code-review → suppress code-generation's completion gate (Plan Approval stays mandatory, as proposed) and the review stage's gate becomes THE per-unit decision point; scope excludes it → code-generation's gate stays. No runtime conditionals.

4. Severity model — rescope for v1, split the waiver machinery out.
Non-waivable criticals and human-waived majors are a third gate semantics; Construction gates are strictly 2-option (Approve / Request Changes), and the reviewer model is deliberately verdict-soft. For v1: map severities onto the existing machinery — critical/major findings ⇒ NOT-READY (drives the repair loop, surfaces at the gate); minor ⇒ findings listed in code-review-findings.json and quoted at the gate. A first-class waiver record (explicit rationale, audit row, non-waivable classes) is a framework-wide gate change that deserves its own issue.

5. Artifact ownership.
The stage should consume code-summary and traceability (they're code-generation's declared produces[], and build-and-test consumes them) and produce only its own artifacts. If review repairs change the code, the repair loop updates code-generation's artifacts in place — same as §12a rework today — rather than the review stage re-owning them.

Dependencies — both already in flight

• [Bug]: code-generation review receipts remain valid after workspace source changes #629 / PR fix: bind code-generation review receipts to the workspace source state (#629) #646 (source freshness): binds review receipts to a workspace source fingerprint; every completion route refuses on mismatch. The new stage inherits this automatically via workspace_requires: true. Prerequisite for the review guarantee meaning anything.
• [RFC]: per-unit attribution for code-generation review receipts #662 (per-unit change manifest): is this proposal's "implementation change manifest" input. It gives the reviewer a bounded per-unit path scope in gated multi-unit runs, and per-unit receipt attribution. Without it, the stage-level route can only make workspace-global claims.

Suggested delivery

One PR, landing the complete feature end to end, sequenced behind #646 and #662 (both in flight) so it can build on the fingerprint binding and the per-unit change manifest rather than re-deriving either:

• aidlc-code-reviewer-agent + knowledge/aidlc-code-reviewer-agent/ — review checklist focused on correctness, requirements/AC coverage, project conventions, tests, and security, sized to a reviewer turn budget; mechanical checks pushed into sensors.
• The code-review stage file (for_each: unit-of-work, workspace_requires: true, consumes the [RFC]: per-unit attribution for code-generation review receipts #662 manifest, produces code-review.md + code-review-findings.json), with the repair_agent schema extension and the compile-time gate replacement.
• Formatter and secret-scan sensor manifests attached to the stage.
• Bolt integration: the two-stage per-unit pipeline (generate → review in-worktree, pre-finalize) so autonomous Construction reviews before merge and presents one aggregate gate.
• Removal of code-generation's embedded reviewer: in the same change, so review passes are relocated rather than duplicated.

The one piece I'd still keep out is the waiver model (non-waivable criticals / human-waived majors) — v1 maps severities onto READY/NOT-READY + findings-at-gate as described above, and the waiver machinery gets its own issue since it changes gate semantics framework-wide.

Happy to help spec the repair_agent schema and Bolt pipeline details if this shape sounds right.

[jeromevdl]

Thanks for validating the premise and for connecting this proposal to #629/#646 and #662. Since then, #662 has an implementation in #813; this design should consume the final contract that lands there rather than duplicate it.

Decisions

1. What changes in Code Generation

Today, Code Generation runs an embedded aidlc-architecture-reviewer-agent pass through §12a after implementation. That review primarily sees Code Generation’s record artifacts and upstream contracts, not a bounded set of generated source files. Its ## Review section is also replaced on each pass.

The proposal changes that boundary:

• Remove reviewer: aidlc-architecture-reviewer-agent from Code Generation.
• Remove the corresponding embedded review from gated and swarm execution.
• Keep Code Generation’s planning, mandatory Plan Approval, implementation, tests, code-summary.md, traceability.json, and source manifest.
• When Code Review is in scope, Code Generation hands the unit directly to it without a completion gate.
• When Code Review is not in scope, Code Generation retains its completion gate but performs no embedded source review.
• Keep the architecture reviewer on upstream design stages.

2. Reviewer authors; developer repairs

code-review should be reviewer-led:

  lead_agent: aidlc-code-reviewer-agent
  repair_agent: aidlc-developer-agent
  for_each: unit-of-work
  workspace_requires: true

The code reviewer authors code-review.md and code-review-findings.json but never modifies application source. A blocking verdict dispatches aidlc-developer-agent, which repairs the source and updates code-generation-owned artifacts before the reviewer runs again.

I prefer the minimal reusable repair_agent: schema extension over a code-review-specific protocol. Existing reviewer-bearing stages would continue defaulting repairs to lead_agent; repair_agent overrides that role where author and reviewer are intentionally different.

3. Review budgets differ by Construction mode

To make the pass/repair distinction explicit:

• Gated: maximum 2 review passes, allowing 1 repair round.
• Autonomous: maximum 3 review passes, allowing 2 repair rounds.

The rationale is that gated execution reaches a human after each unit, while autonomous execution needs one additional self-healing opportunity before interrupting the run.

This slightly revises the issue’s original “two repair rounds” statement: it applies to autonomous execution, not gated execution. The normal stage cap remains expressed as review passes. The autonomous override should also be explicit in compiled stage/directive data rather than hidden in prose.

This remains mostly cost relocation: code-generation loses its current embedded review. Autonomous execution deliberately gains a possible third pass, but only after two NOT-READY results.

4. Scope membership is the control

I propose enabling code-review for:

• enterprise
• feature
• mvp
• classic
• infra
• bugfix
• refactor
• security-patch
• workshop

It would be omitted from poc and express. Express remains the documented no-review path.

Because this is a first-class stage, its inclusion should be controlled by scopes: rather than by review_cap. The existing scope cap can continue controlling embedded reviewers without silently disabling or weakening an explicitly selected code-review stage.

Source attribution and #813

#813 now supplies the implementation-change manifest proposed here, so this feature should build on its final contract rather than add another attribution mechanism.

The ownership model should be:

• Code Generation creates and owns source-manifest.json.
• Code Review receives the manifest and its claimed paths as engine-provided inputs.
• The reviewer inspects only those paths, plus declared upstream contracts and explicitly named integration points.
• The code-review receipt binds to the manifest and current claimed-path contents.
• Manifest edits, claimed-source edits, deletions, or unclaimed changes invalidate or block the review through the feat: per-unit attribution for code-generation review receipts (#662) #813 machinery.

#813 deliberately keeps the manifest outside produces[] for migration reasons. Code Review therefore should not reclassify or copy it as one of its artifacts. The remaining interface question is how a workspace-review stage names an upstream source manifest. My preference is a small generic stage/directive reference to the source-owning stage, rather than special-casing the path or duplicating the manifest.

For zero-Unit production scopes such as targeted bugfixes, Code Review should run once at stage level. To retain a bounded review surface, Code Generation should write the same manifest schema with unit omitted; the existing workspace fingerprint remains the outer integrity boundary.

Durable review artifacts

Code Review should produce only:

• code-review.md
• code-review-findings.json

code-summary.md, traceability.json, and source-manifest.json remain owned by Code Generation. Developer repairs update them in place.

The JSON artifact should preserve review history rather than only the final state. A possible v1 shape is:

  {
    "version": 1,
    "stage": "code-review",
    "unit": "u1-auth",
    "verdict": "READY",
    "passes": [
      {
        "pass": 1,
        "verdict": "NOT-READY",
        "findings": [
          {
            "id": "CR-001",
            "severity": "major",
            "category": "correctness",
            "path": "src/auth/login.ts",
            "location": "42-58",
            "finding": "Lockout state is not updated atomically.",
            "evidence": "Concurrent failed requests can overwrite the attempt count.",
            "recommendation": "Use the repository's atomic conditional update.",
            "status": "resolved"
          }
        ]
      },
      {
        "pass": 2,
        "verdict": "READY",
        "findings": []
      }
    ],
    "checks": [
      {
        "name": "unit-tests",
        "command": "npm test -- auth",
        "status": "passed"
      }
    ]
  }

Finding IDs remain stable between passes, and earlier findings remain visible with their resolution status. code-review.md provides the final human-readable verdict, check summary, resolved findings, and remaining findings.

Severity semantics

I agree that waiver machinery should stay out of v1:

• Critical or major open findings produce NOT-READY.
• Minor findings do not force NOT-READY.
• All remaining findings are quoted verbatim at the approval gate.
• After the pass budget is exhausted, the existing two-option gate remains Approve / Request Changes.

This preserves current verdict-soft gate semantics. Explicit waiver records, rationale, audit events, and genuinely non-waivable classes should be a separate framework-wide issue.

Mechanical checks

The stage should attach review-time sensor manifests for:

• Formatter check-only verification.
• Secret scanning.
• Coverage verification.
• Existing lint and type/static-analysis checks.
• Findings JSON/schema and verdict-consistency validation.

The reviewer also runs the unit’s targeted test commands and any project-configured dependency checks. Missing tooling is recorded as skipped with a reason; Code Review never installs new dependencies.

These checks operate on the manifest’s claimed paths and run during Code Review. Build and Test remains responsible for the complete build and broader integration/E2E suites.

Gated flow

For each unit:

1. Complete Code Generation Plan Approval.
2. Generate source, tests, summary, traceability, and source manifest.
3. Enter Code Review without presenting Code Generation’s completion gate.
4. Review the unit’s claimed source.
5. On NOT-READY, dispatch the developer and re-review once.
6. Present the Code Review gate after at most two passes.
7. Continue to the next unit only after that gate is approved.

A Request Changes decision resets that unit’s review attempt. Any repair must update the manifest and code-generation artifacts, and a fresh source-bound review is required.

Autonomous flow

The Bolt becomes a two-stage per-unit pipeline:

generate -> review -> repair/re-review if needed -> finalize

All review and repair work occurs inside the unit’s worktree. Finalization requires a terminal Code Review receipt bound to the exact immutable source that will be merged.

After all units converge:

• Do not rerun reviewers during settlement.
• Do not present a Code Generation completion gate.
• Present one aggregate gate owned by Code Review.

This preserves the PR analogy: review happens on the isolated branch before merge, not against already-merged source.

Compile-time gate replacement

Gate replacement should be derived when compiling the active scope:

• Scope includes code-review: suppress Code Generation’s completion gate and make Code Review the decision point.
• Scope excludes code-review: preserve Code Generation’s existing completion gate.
• Code Generation Plan Approval remains mandatory in both cases.

There should be no runtime scope conditional for this replacement, and compiled gate counts should reflect the resulting graph.

Delivery

I agree this should be one end-to-end PR, sequenced behind the final #813 contract. It should include:

• aidlc-code-reviewer-agent and bounded source-review knowledge.
• The new stage and durable artifacts.
• repair_agent and mode-specific pass-budget support.
• Formatter, secret-scan, and coverage sensors.
• Gated per-unit sequencing and autonomous Bolt integration.
• Compile-time gate replacement.
• Removal of only Code Generation’s embedded architecture reviewer.

The architecture reviewers on Functional Design, NFR Design, and Infrastructure Design remain unchanged, so design verification stays upstream while source review moves to the correct boundary.

[apackeer]

Cross-reference: the external half of the review story this issue proposes now has a design RFC (#970) and an implementation PR (#969) - PR-based integration for Construction.

They are deliberately complementary, not overlapping: this issue's code-review stage is internal review before Build and Test; #969 is the integration boundary after it. The composition point is concrete: the pr-integration stage assembles its PR evidence dossier by enumerating its consumes, so once a code-review stage exists and its artifacts (findings, waivers, traceability) are added to that list, internal review evidence lands in every PR body with no new mechanism - external reviewers would start from what the code review already established.