Vuong Hoang
1240212ff2
Codex's CWD when invoked by the Volva daemon. Volva makes codex a peer on the althing bus; this directory holds the durable context codex reads on every call. Seeded with: - AGENTS.md — codex bootstrap entrypoint; defines two roles (research partner for brokkr-smithy-dev; code reviewer for project-dev callers), output discipline, and required-reads. - CONTEXT.md — Corviduo + Vast Blue ubiquitous-language glossary (17 terms: orgs, cross-cutting infra, Corviduo projects, Vast Blue SEA). Mirrors the user-level CONTEXT.md baseline. - INVENTORY.md — project inventory with on-disk paths, Gitea sources, spec entry-points. Drawn from brokkr-smithy/docs/improvement-targets. - PRINCIPLES.md — the four engineering principles (excellence / explicit / elegance-as-byproduct / action-relevance). - REVIEW-GUIDE.md — code-review discipline. Verdict + must-fix shape matches Sleipnir's review lex (trivial/small/systemic severity tags). Project-specific patterns for Sleipnir, Worldtree, Skaldsong. - RESEARCH-PARTNER-GUIDE.md — research-partner discipline. Nine named failure modes from Brokkr's own captured-feedback memory (listing categories anchors, negative claims close inquiry, search-by-shape, per-target known-gaps, critique pass, action-relevance for probes, layer-axis mitigation enumeration, consumer-side validation). - transient/ — per-call ephemera convention; caller drops transient/<slug>.md before the althing message references it. Gitignored except for the README. Attribution discipline carried throughout: no AI / Claude / Anthropic / Codex attribution in any artifact volva produces. Operator is Vuong Hoang.
106 lines
8.8 KiB
Markdown
106 lines
8.8 KiB
Markdown
# 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/<project>.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 <name>` 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/<N>.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 <slug>` 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.
|