review-lessons v1.0.0A

Immutable. This exact content never changes and is served at /api/v1/blob/b0e8568fb4f7c013.

---
name: review-lessons
description: "Library of lessons from past build-review loops — not a stage. Use when a review or fix round hits a specific case: a reviewer BLOCKING that contradicts the brief, a test that passes but checks nothing, a feature dead in production, SQLite/WAL migrations, MCP tool surfaces, REST API authz, app ↔ external-source sync, Android/APK verification, fill-template extraction, CLI/git mechanics, reviewer outages. Load the one file the case needs; never load the whole library."
metadata:
  version: 1.0.0
  author: moonlight-lupin
  license: MIT
  platforms: [linux, macos, windows]
  hermes:
    tags: [lessons, review-findings, pitfalls, sqlite, mcp, rest-api, security-property-tests, data-integrity, test-quality, dev-pipeline]
    related_skills: [review-commit, build, test-evals]
---

# Review Lessons

What past loops taught, filed by case. The dev-pipeline's PROCESS lives in
the stage skills — round budgets, severity floor and stop conditions in
`review-commit/references/confirm-round-protocol.md`, model names in
`orchestrator/references/lanes.md`. Where a lesson here disagrees with those
files, they win: lessons are dated evidence, the protocol is the current rule.

**How to use:** find the case below, read that one file, apply it to the
finding in front of you. Don't preload — most of these are project-specific.

**Old names:** these files predate v0.2. "build-review-loop", "codex-confirm-loop"
and "subagent-driven-development" in them mean today's `review-commit` and
`build` stages; model names in them are what ran at the time, not current
pins. Numbered references ("pitfall #N", "rule #N") point at the retired v0.1
stage SKILL.md numbering, which no longer exists — treat them as historical
context, not as pointers to read.

## By case

### Adjudicating findings and fix rounds
| Case | File |
|---|---|
| Reviewer BLOCKING contradicts the dispatch spec or a contract test; deferred findings as design questions | `references/review-findings-adjudication.md` |
| Fix at the wrong layer, red-test triage, two-sided fixes, live verification | `references/pitfalls-fix-round-verification.md` |
| Builder self-report, regressions from fixes, mid-build user steers, brief races | `references/loop-pitfalls.md` |
| Confirm-loop probes: exact repro shapes, vacuous tests, mutation-testing a fix, race probes, kernel state | `references/confirm-loop-lessons.md` |
| Two reviewers in parallel, synthesizing their findings | `references/parallel-dual-review.md`, `references/cross-model-parallel-review.md` |
| A reviewer lane is down; YAML frontmatter generated by f-strings | `references/generated-frontmatter-and-reviewer-outages.md` |
| Plugin ship gates and fix-round contracts | `references/plugin-gates-and-fix-rounds.md` |

### Test quality
| Case | File |
|---|---|
| Property tests for isolation / scope / role surfaces; inert tests; exempt paths | `references/security-property-tests.md` |
| In-process scope property tests | `references/scope-test-in-process-execution.md` |
| Feature has tests but no production caller | `references/production-reachability-review.md` |
| Tests to update when the architecture shifts mid-loop | `references/architecture-refactor-test-updates.md` |
| Upgrade-path tests | `references/upgrade-path-test-pattern.md` |
| Before/after evidence pairs for delivery | `references/evidence-pairs.md` |

### Data and storage
| Case | File |
|---|---|
| Invariants, foreign keys, idempotency, money as TEXT/Decimal, migration fixtures | `references/data-integrity-principles.md` |
| SQLite table-rebuild migrations | `references/sqlite-migration-pitfalls.md` |
| SQLite WAL migration review findings | `references/sqlite-wal-review-findings.md` |
| Replacing N modules with one (caller inventory) | `references/full-module-replacement-scoping.md` |
| Reconciliation logic | `references/reconciliation-review-findings.md` |

### Surfaces and domains
| Case | File |
|---|---|
| REST API: routers, authz, SQL patterns | `references/rest-api-review-checklist.md` |
| MCP tool surfaces and design review | `references/mcp-category-tool-implementation.md`, `references/mcp-design-review-patterns.md` |
| App ↔ external source-of-truth sync (Sheets, SaaS mirrors) | `references/sync-pipeline-review-patterns.md` |
| Android APK / WebView verification | `references/mobile-app-verification.md` |
| Fill-template document extraction | `references/extraction-from-filled-docs.md` |
| Styling against a shipped minified bundle | `references/minified-bundle-selector-research.md` |

### Mechanics and process history
| Case | File |
|---|---|
| CLI flags, git mechanics, contract-test mechanics, push hygiene | `references/tool-mechanics-pitfalls.md` |
| Dispatch, polling, parallel builds, brief races | `references/pitfalls-dispatch-semantics.md` |
| CI and repo hygiene | `references/ci-and-repo-hygiene.md` |
| Batching a phase by dependency and risk | `references/multi-batch-sequencing.md` |
| Release-hardening-shaped cycle | `references/beta-gate-release-hardening.md` |
| Integrating an operator's field briefing | `references/field-briefing-integration.md` |
| Port notes: delegate-skills, planning-with-files | `references/delegate-skills-adaptation.md`, `references/planning-with-files-adaptation.md` |

## Adding a lesson

Add a lesson only when a loop taught something a future loop would get wrong
without it. One file per case; put the date and project type in the title;
add a row above. If the lesson changes how the loop runs (a cap, a stop rule,
who reviews), change `confirm-round-protocol.md` instead — a rule filed as a
lesson becomes a second, contradicting copy.