mo-self-review

Compatibility: designed for Codex CLI and compatible agents with a MatrixOne git working tree. Use `unhappy-path-audit` for deep Q1-Q3 ownership, wait, and growth analysis. ## Review contract The quality bar is complete coverage of every changed contract, not equal depth for every file. Read every changed hunk once, build one change/risk map, and route deep analysis only to applicable closures. This is not spot checking: every lens must map to a closure or have a concrete not-applicable reason. Keep two execution modes distinct: | Mode | Responsibility | |---|---| | **Self/delivery review** | May fix in-scope code; must close blockers and ensure required evidence exists before push/PR update. | | **External PR review** | Review read-only by default. Verify and reuse qualifying author/CI evidence; run only missing or contradiction-resolving checks. Report the decision, but submit GitHub `APPROVE`/`REQUEST_CHANGES` only when the user authorized that external mutation. | An explicit developer decision closes a subjective tradeoff when its assumptions and rationale are recorded. Do not repeatedly relitigate it without materially new evidence. It does not erase a demonstrated correctness/security/data/hang/ leak/compatibility failure or a mandatory user/repository gate. The PR author or reviewer cannot self-waive such a gate; only its policy owner can explicitly change an allowed exception, with owner/scope/rationale recorded. User/system permission boundaries cannot be waived. A blocker requires either a concrete failure path (`input/state -> consequence`) or an objectively missing mandatory artifact/validation. Preferences, plausible concerns, maintainability suggestions, and micro-nits are non-blocking or omitted. Assign severity only after the finding is verified. ## Resolve target and scope | Args | Target | |---|---| | *(empty)* | current branch vs verified PR base when one exists; otherwise a freshly verified remote default branch | | git ref / `vs <ref>` / `base=<ref>` | current branch vs the explicit ref | | all digits | that GitHub PR vs its declared base | | `<ref> <scope...>` | explicit base with trailing focus | | anything else | focus text with verified default base | | `docs` / `help` | show the workflow only | Resolve explicit user base, then PR base, then fresh remote default. Never silently use stale local `main`. Record base ref/object, head object, merge-base, committed range, staged/unstaged changes, and untracked files. Report freshness limits when the remote cannot be verified. A focus narrows presentation and deep tracing, not awareness of in-scope changes that can interact with it. ## Resource map | Trigger | Read | |---|---| | Every non-trivial review: change map, R0-R3 routing, evidence reuse, efficient validation | [../mo-dev/references/validation-evidence.md](../mo-dev/references/validation-evidence.md) | | Credible shared-state/lifecycle/synchronization/timing failure mode | [../mo-dev/references/race-validation.md](../mo-dev/references/race-validation.md) | | Large/complex feature or major refactor; design document (RFC optional) | [../mo-dev/references/feature-design-review.md](../mo-dev/references/feature-design-review.md) | | Shared state, cancel/close, callbacks, retry/restart, pooling/reuse, async cleanup | [references/concurrency-lifecycle.md](references/concurrency-lifecycle.md) | | Production behavior or changed/added/removed/merged/optimized UT/BVT | [../mo-dev/references/testing-contract.md](../mo-dev/references/testing-contract.md) | | Index algorithm dispatch/plugin/registry/ISCP paths | [../mo-dev/references/index-plugin.md#9-reviewing-an-index-plugin-change](../mo-dev/references/index-plugin.md#9-reviewing-an-index-plugin-change) | ## Single-pass execution 1. **Resolve and inventory.** Establish the exact scope above. Inspect status, diff stat/name-status, generated/delivery artifacts, then read every changed hunk once. 2. **Build the change map.** Group files into behavioral closures; record invariant/purpose, owner/consumers, reverse arcs, R0-R3 triggers, applicable lenses, and required evidence. Reuse this map instead of rereading the whole diff once per lens. 3. **Run the design gate first.** Classify the complete feature/refactor or PR series. Ordinary fixes/maintenance are exempt. When triggered, review the exact approved design revision before implementation. Missing/unapproved or failed design makes the decision `REQUEST_CHANGES` and short-circuits full implementation polishing. 4. **Trace applicable closures.** For each map row, follow edited code into the first owner, all changed consumers/reverse arcs, and terminal success/failure nodes. Apply the lenses below. R0/R1 does not pay R3 modeling cost; any small R3 closure does. 5. **Audit evidence.** Check selection, mode, revision, terminal status, and semantic freshness. Reuse valid exact-head author/CI evidence. In self mode, produce missing proof; in external mode, run only gaps or checks needed to resolve a concrete uncertainty. 6. **Converge once.** Verify every candidate against source and full closure, discard speculation, respect recorded decisions, then return one ranked finding/fix-or-decision list and final decision. Do not request another broad review pass. If an optional code-review workflow is available, it may help discover candidates from the same resolved range. Its output does not replace the change map, closure verification, evidence audit, or final severity calibration. ## Review lenses Classify every lens for every mapped closure. Group rows that share the same applicability and reason instead of emitting a prose Cartesian product. `N/A` needs a fact such as “no executable behavior or generated consumer changed,” not “diff is small.” | Lens | Question | |---|---| | Correctness/boundaries | Does the invariant hold for ordinary and reachable zero/empty/nil/boundary/invalid inputs? | | Contract/consumers | Are callers, readers, receivers, interfaces, generated artifacts, and public error/result semantics aligned? | | State/concurrency | Are transitions, linearization, shared ownership, stale generations, races, and exactly-once side effects defined? | | Control/liveness | Can cancel/close/reject/timeout terminate independently of the work it controls? | | Resource lifecycle | Does each resource have one effective cleanup owner on success, partial failure, cancel, panic, reset, and reuse? | | Boundedness/scale | Are queues, retries, caches, retained state, logs/metrics, memory/disk/FDs, and work admission bounded? | | Compatibility/security | Are API/wire/disk/catalog/config, mixed-version/migration/rollback, auth, tenant, and trust boundaries preserved? | | Performance/operations | Did hot-path work, allocations, I/O, synchronization, startup, capacity, rollout, observability, or blast radius change materially? | | Platform/delivery | Are build tags, OS/arch, CGo/native loading, generated files, packaging, and final committed artifacts correct? | | Test architecture | Do tests prove distinct contracts with minimum deterministic data/setup, correct UT/BVT layer, isolation, cleanup, and real selection? | ## First-principles finding proof Do not report “might.” For each candidate: 1. State the invariant and concrete reachable input/state. 2. Follow all guards, ownership transfers, defer/cancel/timer/retry/release paths. 3. Identify the wrong result, crash, hang, data/security/compatibility impact, or exactly missing required proof. 4. Re-read the cited source after forming the hypothesis. 5. Keep it only if no existing path closes the failure; calibrate severity last. The design-first gate is an artifact/decision proof rather than a runtime counterexample: cite the exact trigger and missing/unresolved design requirement. ## Functional and unhappy-path closure Trace only applicable arcs, but trace them to terminal nodes: | Change | Minimum closure | |---|---| | persistence/format | create/write -> read -> backup/restore -> upgrade/restart | | operator/pipeline | prepare -> call/send -> receive -> reset/cleanup -> error/cancel | | resource/state machine | create/admit -> transfer/transitions -> fail/retry -> close/reuse generation | | config/API/protocol | parse/default -> producer -> every consumer -> compatibility/fallback | | shared test fixture | create -> scenario admission -> cleanup after `FailNow` -> reset -> next scenario -> destroy | | BVT | clean/readiness -> public action -> positive/negative oracle -> restore/teardown -> same-instance rerun | For touched resources, waits, or accumulating state, use `unhappy-path-audit`: - **Q1:** every creation reaches exactly one effective destruction owner; - **Q2:** every wait-for chain reaches guaranteed release/cancel/bound; - **Q3:** every accumulation has a capacity/admission/recycle/terminal bound. Apply its full-graph, can-fail/block, bound/release, line-reread, and calibrate-last filters. Do not load or run the deep audit for a closure that has no resource, wait, asynchronous generation, or growth dimension. ## Test and evidence gate Use the testing contract for purpose, orthogonality, fixture cost, BVT, and cleanup. Use the validation/evidence reference for R0-R3 depth and semantic evidence reuse; load the race reference only when its trigger applies. Non-negotiable outcomes: - changed behavior maps to the cheapest focused oracle and to BVT when a public SQL/protocol contract requires it; - new/heavy fixtures demonstrate existing-case search, a distinct isolation or topology need, minimum data, deterministic control, and measured cost when unavoidable; - merged/deleted cases map every prior positive/negative/boundary/metadata/ privilege/session/error oracle to a retained named scenario; - sleeps, retries, probabilistic scheduling, huge data, repeated processes, and generated-result acceptance never substitute for injection, barriers, scoped configuration, cleanup, or normal comparison; - race/package/topology/repetition evidence is required only by its mapped risk trigger, but cannot be skipped when that trigger applies. Evidence remains valid across unrelated edits. Invalidate and rerun only when a relevant semantic input, oracle, fixture, build mode/tag, dependency, topology, or base-side contract changed; ambiguity means stale. Pending, skipped, zero-test, partial-output, or surviving-process runs are not green. ## Convergence and decision - Record accepted tradeoffs and every won't-fix/known gap with owner/rationale. Reopen only with materially new evidence that invalidates its assumptions. - Fix harmless local nits silently in self mode or omit them in external mode. Do not spend reviewer cycles on style that automation can decide. - An upstream design/range/artifact blocker may stop downstream review. For code findings, complete the other applicable mapped closures so one review returns the converged blocker set rather than serial surprises. - Final external decision is `REQUEST_CHANGES` when any blocker remains, otherwise `APPROVE`/no-blocker recommendation. Perform the GitHub mutation only when authorized. ## Exit gate ```text □ exact range plus committed/staged/unstaged/untracked scope recorded □ every changed hunk read and represented in one R0-R3 change map □ every lens mapped to a closure or a concrete N/A reason □ design gate classified; triggered design approved and implementation aligned □ applicable owners/consumers/reverse arcs and terminal unhappy paths closed □ Q1-Q3 and concurrency/generation models completed only where triggered □ UT/BVT/fixture/oracle decisions complete where behavior/tests changed □ required evidence validly reused or passed; stale/missing/pending proof explicit □ each finding has a concrete path or objective gate failure and source re-read □ decisions logged, severity calibrated last, zero unresolved blockers for PASS □ final delivery diff/generated artifacts checked in self mode ``` For index-plugin changes, additionally run the linked section-9 candidate searches and prove hook interfaces, registration/build tags, ISCP/CDC where applicable, CPU-runnable tests, and public-path behavior. Candidate greps are not standalone findings.