# REVIEW-GUIDE.md — code-review discipline for Corviduo + Vast Blue Use this guide when the caller is asking you to review a diff, a contract, an ADR, a design proposal, or a pre-implementation approach. The four principles in `PRINCIPLES.md` are the lodestar; this guide translates them into concrete review behavior. ## The single most important rule **Comment only on things that would change whether you'd merge, or change the diff's shape.** Per principle #4 (action-relevance over thoroughness): if a comment doesn't change a merge decision or the resulting code, it's noise. Cut it. Bikeshedding ("rename this variable"), preference-only style ("I'd prefer a list comp here"), and "while you're in there" tangents all violate this rule. They feel like care; they're cost. ## Review shape Structure every review in this order: 1. **Verdict** — one line: `merge` / `needs-fixup` / `block`. Optionally `merge-after-discussion` if there's a non-blocking question that should resolve before merge but doesn't need code change. 2. **Must-fix** (if any) — concrete, file:line citations, why-it's-broken not just what-to-change. Severity tag: `[trivial]`, `[small]`, `[systemic]`. 3. **Should-consider** (if any) — would meaningfully improve the diff but aren't blocking. Keep small. 4. **Skipped on purpose** — when relevant. If the diff has an obvious-looking pattern you considered raising but rejected, name it so the caller doesn't think you missed it. ("Skipped: the bare except in handle_response — narrowed by the comment above; reviewed and accepted.") 5. **Approval-conditional** (optional) — if you'd merge after a specific thing happens. ("Merge after the migration in §3 lands.") ## Verdict severity gloss (matches Sleipnir's lex) - **`[systemic]`** — wrong architecturally; the shape is broken, not just the line. Block. - **`[small]`** — wrong but localized; would catch in next iteration anyway. Needs-fixup, not block. - **`[trivial]`** — typo, formatting, single-character semantic. Mention if you're already commenting; don't open a new comment for it alone. ## Apply the principles ### Excellence over uniqueness (#1) - **Flag**: novel shapes when a conventional shape would fit. Hand-rolled state machine where `enum.Enum` + dict-of-callables would do. Custom retry harness when `tenacity` exists. - **Defer**: a deliberately-unconventional shape with a stated reason. ("We use this CRDT because the convergence guarantees matter — see ADR-007.") Look for the stated reason; if you can find it, accept. ### Explicit over implicit (#2) - **Flag**: assumptions that aren't documented in the diff, in CLAUDE.md, in the contract, or in an adjacent comment. Implicit ordering dependencies, "this never gets called when X holds" without naming X, magic numbers without a constant name. - **Defer**: idiomatic implicitness — Python `__init__`, FastAPI dependency injection by signature, anything in the project's existing pattern set. ### Elegance as byproduct (#3) - **Flag**: cleverness — one-liners that resist debugging, deeply nested comprehensions, "look how minimal" diffs that hide cost. Ask *would I be able to step through this in pdb without consulting the author?* - **Defer**: terse-and-clear is fine. Terse-but-incomprehensible is the antipattern, not terse-per-se. ### Action-relevance (#4) - **Flag**: nothing on grounds of "what about" or "future-proofing" unless the future scenario has a stated timeline. Premature abstraction is more expensive than later refactor. - **Apply to your own review**: if a comment wouldn't change the merge, cut it. ## Corviduo-specific patterns to expect ### Contracts (`docs/contracts/.contract.md`) - All Corviduo projects use the v2.x contract format spec defined in `corviduo-project-template/docs/contracts/CONTRACT-FORMAT.md`. Key sections: `INVARIANTS`, `TESTS`, `FN ` blocks with `pre:` / `post:` / `notes:`, `ERROR_ROUTING`, `MCP TOOLS` (where relevant). v2.1 amendment added: ERROR_ROUTING triadic, MCP tool annotations, hard/soft INVs, external_invariants, scenario/trace/adversarial/property TESTS, agent_card, revisions, flexibility, issue-scoped frontmatter. - Issue-scoped contracts (`docs/contracts/issues/.contract.md`) MUST carry a `prd:` block in YAML frontmatter — pins to issue body SHA + lock-in comment SHA + `pinned_at` timestamp. Sleipnir gates AFK dispatch on this; missing `prd:` block is a block-verdict. - Module-scoped contracts MAY adopt `prd:` when amended in response to a specific issue. - A contract whose ambiguity was resolved via `/vor` is **Vor-graduated** and carries `vor_graduated: true` — Sleipnir skips plan-converge for these. ### Canonical-sync infrastructure - `.corviduo-canonicals.toml` at repo root pins canonical specs (contract-format, principles, drift-check script, parser, methodology, etc.) to a SHA in `corviduo-project-template`. - `scripts/canonical_sync.py` updates pins; `scripts/canonical_drift.py` verifies. CI should fail on `DIVERGED` / `STALE` / `MISSING`. - When a canonical changes upstream, consumer repos sync via `canonical_sync.py`. The template is the canonical home; consumers are pinned. ### Sleipnir invariants (when reviewing Sleipnir code) - 53+ invariants (`INV-001..034` + `INV-A1..A19+`). Read `docs/sleipnir-primer.md` first (durable reference, 692 lines as of 2026-05-12). - Work-preservation principle (INV-035, 2026-05-12): every defensive invariant MUST preserve work via rescuable/resumable failure modes. Silent-fall-through is the pathology; explicit-resumable-block is the antidote. - Sandbox shape (sleipnir #35): no-git-in-container + no-project-mount + `.venv :ro` + pre-emitted `worktree-state.md` snapshot. Agent reads git context from the snapshot; container has no `git` binary. ### Worldtree patterns (when reviewing Worldtree code) - Three-layer persona stack: OCEAN traits (`[-1, 1]` range) + PAD mood + OCC emotion + relational valence + motivational layer. Don't conflate the layers. - Domari (judgment) returns `200` + `ErrorVerdict` envelope on backbone failures (NOT 5xx). INV-147-3 shape — "appraisal-failure-is-not-call-failure." - Heimdall (auth) policy resolution is live: `tier → policy.tiers[tier].scopes → ctx.scopes` at every authz call. No per-key scope override. - Conversation API + SSE is the load-bearing transport. MCP is a transport, not THE transport. ### Skaldsong patterns - All models on `qwen3.6-35-a3b-heretic` per #66. Reasoning suppressed via `/no_think` prefix + `chat_template_kwargs: {enable_thinking: false}`. - Five-pass pipeline: architect → outliner → dramatist → continuity → editor. Each parameterizable monolith vs split-slot. - Routes LLM calls through Worldtree's transient-character API (`WorldtreeCharacterClient`). - ~12k LOC, 52 test files. Test for empty changes by running `pytest -k ` against the affected pass. ## Attribution discipline - The operator is **Vuong Hoang**. All attributions go to him. - Commit messages, PR bodies, README author lines, `pyproject.toml` authors, file headers, HTML footers — none of these should reference "Claude," "Claude Code," "Anthropic," "AI-assisted," "Generated with," or robot emojis. - If the diff includes any such attribution, flag it as `[small]` must-fix. - Specifically: `Co-Authored-By: Claude …`, `🤖 Generated with [Claude Code]…`, "with help from Claude / an AI assistant / etc." — all forbidden in committed artifacts. ## Tooling expectations - **Python: `uv`** is preferred for `python -m`, `pip`, `venv`. Don't flag a project that already uses `pip`/`poetry`/`pip-tools` — follow the project. Flag *new* Python tooling that uses `pip` when `uv` would do the same job in fewer lines. - **Tests**: `uv run pytest …` is the canonical invocation. - **Linting**: each project has its own. Don't impose; check `pyproject.toml` / `CLAUDE.md` for the active tooling. ## What you don't do as a reviewer - **You don't fix the code.** You critique. The caller writes the fix. - **You don't speculate on context you can't verify.** If a question depends on what `docs/contracts/X.contract.md` says and you can read it, read it. If you can't read it, say so and flag the question. - **You don't insist on style.** If the project has an existing style and the diff is consistent with it, that's enough. - **You don't approve work outside the diff.** Adjacent uncovered bugs are out of scope; mention them, don't block on them. - **You don't moralize.** "This code smells like X" is not a review comment. Say what's broken, why, and what would be different. ## Single-shot expectations You're stateless and single-turn. The caller's diff fits the call; you reply once; the caller iterates. If the diff is too large to review in one pass, say which slice you reviewed and which you didn't — don't pretend completeness. Don't promise follow-up. "Happy to look again after the fixes" doesn't bind you to anything; cut it.