M4 — Stage 2: Review (compose model + fallback) #14

david · 2026-07-24T20:41:46Z

david commented

2026-07-24 20:41:46 +00:00

Summary

Implements M4 — Stage 2: Review (#5): reviews implementation.diff against the project's own standards and real build/test/lint results, producing a severity-grouped review-findings.md via a fresh, read-only pi agent session. Deterministic orchestration around the LLM black box, with degrade-to-default when REVIEW_SKILL_PATH is unavailable.

Closes #5.

Changes

src/artifacts: writeReviewFindings / readReviewFindings / reviewFindingsPath — verbatim round-trip for review-findings.md (mirrors implementationDiffArtifact).
src/review (new module):
- buildReviewWrapper() — thin pipeline contract: inputs (diff, standards docs, Jira context), run build/test/lint gates, review-only, and the exact severity-grouped findings structure the final message must be.
- gatherStandardsDocs() — collects existing standards docs (AGENTS.md, CLAUDE.md, README.md, DOMAIN.md, CODING_STANDARDS.md) in documented order, skipping missing ones.
- buildReviewUserPrompt() — renders Jira context + fenced diff + titled standards sections, with placeholders for empty/absent inputs.
- REVIEW_TOOLS (read, bash, grep, find, ls — read-only + bash; no write/edit) + DEFAULT_REVIEW_SKILL_BODY.
- runReviewStage(...) — read diff + gather docs → compose prompt (skill body + degrade-to-default) → run pi session → capture final message verbatim into review-findings.md. All external effects are injectable seams (runStageFn, readSkillFile, readDiff, gatherDocs).

Design note

The review stage runs with a read-only + bash allowlist, so the agent cannot author the artifact itself. pi-loop captures the agent's final message (RunStageResult.text) verbatim — the same raw-capture precedent as M3's diff capture. Findings well-formedness is driven by wrapper + default-body instructions and validated as a black-box property in the gated E2E (M8).

Testing

All tests pass (202 total; 34 new)
Full test suite run with no regressions (baseline 168 → 202)
tsc --noEmit, eslint ., prettier --check all clean

Checklist

Self-reviewed the diff
Code follows project conventions (vertical-slice modules, barrel imports, injectable seams)
Descriptive naming (no single-character variables)
Documentation: artifact schema/structure already documented in DESIGN/IMPLEMENTATION_PLAN; no schema change

## Summary Implements **M4 — Stage 2: Review** (#5): reviews `implementation.diff` against the project's own standards and real build/test/lint results, producing a severity-grouped `review-findings.md` via a fresh, **read-only** pi agent session. Deterministic orchestration around the LLM black box, with degrade-to-default when `REVIEW_SKILL_PATH` is unavailable. Closes #5. ## Changes - **`src/artifacts`**: `writeReviewFindings` / `readReviewFindings` / `reviewFindingsPath` — verbatim round-trip for `review-findings.md` (mirrors `implementationDiffArtifact`). - **`src/review`** (new module): - `buildReviewWrapper()` — thin pipeline contract: inputs (diff, standards docs, Jira context), run build/test/lint gates, review-only, and the exact severity-grouped findings structure the final message must be. - `gatherStandardsDocs()` — collects existing standards docs (`AGENTS.md`, `CLAUDE.md`, `README.md`, `DOMAIN.md`, `CODING_STANDARDS.md`) in documented order, skipping missing ones. - `buildReviewUserPrompt()` — renders Jira context + fenced diff + titled standards sections, with placeholders for empty/absent inputs. - `REVIEW_TOOLS` (`read, bash, grep, find, ls` — read-only + bash; no `write`/`edit`) + `DEFAULT_REVIEW_SKILL_BODY`. - `runReviewStage(...)` — read diff + gather docs → compose prompt (skill body + degrade-to-default) → run pi session → capture final message verbatim into `review-findings.md`. All external effects are injectable seams (`runStageFn`, `readSkillFile`, `readDiff`, `gatherDocs`). ## Design note The review stage runs with a read-only + bash allowlist, so the agent cannot author the artifact itself. pi-loop captures the agent's final message (`RunStageResult.text`) verbatim — the same raw-capture precedent as M3's diff capture. Findings well-formedness is driven by wrapper + default-body instructions and validated as a black-box property in the gated E2E (M8). ## Testing - [x] All tests pass (202 total; 34 new) - [x] Full test suite run with no regressions (baseline 168 → 202) - [x] `tsc --noEmit`, `eslint .`, `prettier --check` all clean ## Checklist - [x] Self-reviewed the diff - [x] Code follows project conventions (vertical-slice modules, barrel imports, injectable seams) - [x] Descriptive naming (no single-character variables) - [x] Documentation: artifact schema/structure already documented in DESIGN/IMPLEMENTATION_PLAN; no schema change

david added 1 commit

2026-07-24 20:41:47 +00:00

M4 — Stage 2: Review (compose model + fallback) 70722b9967

Implements M4 — Stage 2: Review (#5): reviews implementation.diff against the
project's own standards and real build/test/lint results, producing a
severity-grouped review-findings.md via a fresh, read-only pi agent session.

Closes #5.

- src/artifacts: writeReviewFindings / readReviewFindings / reviewFindingsPath
  (verbatim round-trip; mirrors implementationDiffArtifact).
- src/review (new module):
  - buildReviewWrapper() — thin pipeline contract: inputs (diff, standards docs,
    Jira context), run build/test/lint gates, review-only, and the exact
    severity-grouped findings structure the final message must be.
  - gatherStandardsDocs() — collects existing standards docs (AGENTS.md,
    CLAUDE.md, README.md, DOMAIN.md, CODING_STANDARDS.md) in order, skips missing.
  - buildReviewUserPrompt() — renders Jira context + fenced diff + titled
    standards sections, with placeholders for empty/absent inputs.
  - REVIEW_TOOLS (read, bash, grep, find, ls — read-only + bash) +
    DEFAULT_REVIEW_SKILL_BODY.
  - runReviewStage(...) — read diff + gather docs → compose prompt (skill body +
    degrade-to-default) → run pi session → capture final message verbatim into
    review-findings.md. All external effects are injectable seams.

david added 1 commit

2026-07-24 21:00:55 +00:00

M4 review fixes — validate findings, dedupe standards list, test coverage 7b8c701656

- Add validateFindings helper + REQUIRED_FINDINGS_HEADINGS (pure, unit-tested);
  wire into runReviewStage and surface non-fatal wellFormed flag on the result.
- Interpolate STANDARDS_DOC_NAMES into buildReviewWrapper to prevent drift.
- Rename misleading empty-content test; add throwing-readDoc skip-branch coverage.
- Add empty-skill-body degrade test at the stage seam.
- Document the wellFormed structural check in DESIGN.md.

Gates: lint clean, build clean, 213 tests pass (was 202).

david merged commit 78a6c20805 into main

2026-07-24 21:15:35 +00:00

david deleted branch issue-5/stage-2-review

2026-07-24 21:15:36 +00:00

david referenced this pull request from a commit

2026-07-24 21:15:37 +00:00

M4 — Stage 2: Review (compose model + fallback) (#14)

Sign in to join this conversation.

No reviewers