Gabriel Brown
86f6e391b7
Agents now start pre-mr-review on the work account themselves, through a Klaude delegate, and continue from its verdict. A council of models on my own accounts replaces the single reviewer before it, and adding a seat is one table row. Proof comes from T3's browser, with mp4 recordings, and capture-proof stays as the fallback. Ticket pages carry their own data, so index.html renders alone in T3's file viewer, and worktree threads share the main checkout's docs. A PreToolUse hook sends subagents in T3 threads to delegate_task. Blocker, proof, review and MR summary rules come from ideas in t3dotgg/tstack.
138 lines
7.1 KiB
Markdown
138 lines
7.1 KiB
Markdown
# The review council
|
|
|
|
Several fresh agents from different model families review the same change, each knowing nothing
|
|
of how it was built. You judge what they return. This runs before `pre-mr-review`, because
|
|
that audit may not run anything and so cannot catch a claim that is plausible on the page and
|
|
false in reality. A reviewer that can run things can. It has caught a mitigation proven by
|
|
construction to be wrong, on a branch whose own audit had blessed it.
|
|
|
|
Order matters. `pre-mr-review`'s verdict gets pasted verbatim into `mr.md`, and anything that
|
|
lands after it invalidates that verdict. Council first, fix, commit, then the audit over the
|
|
final tree.
|
|
|
|
## Seats
|
|
|
|
| Seat | `providerInstanceId` | `model` |
|
|
|---|---|---|
|
|
| Claude | `claudeAgent` | omit it, so the seat runs that provider's default |
|
|
| MiMo | `opencode` | `opencode-go/mimo-v2.6-pro` |
|
|
|
|
Klaude never takes a seat. It is the work account, and its only job is the `pre-mr-review` run
|
|
in Phase 2 step 7, which stays a separate required step after the council.
|
|
|
|
Adding a seat is adding a row. `orchestrator_capabilities` has the IDs, with `search` for a
|
|
model it does not list. Nothing else here changes with the seat count. T3 can leave out a model
|
|
the provider really has, as it does with `opencode-go/*`. List such a model under that provider's
|
|
`customModels` in `~/.t3/userdata/settings.json`, which T3 picks up without a restart.
|
|
|
|
## When it runs
|
|
|
|
Run it whenever the change adds or modifies a procedure, touches schema or migrations, touches
|
|
permissions or auth, involves concurrency, or has a risk table in its ticket. When unsure, run
|
|
it. A copy tweak, a styling fix, or a one line correction with no logic skips it.
|
|
|
|
## Convene
|
|
|
|
1. Confirm the suite is green, and note the base and head of the commit range. A seat should
|
|
never spend its review on code the suite already rejects.
|
|
2. In one turn, call `delegate_task` once per seat with the brief below: `role: "review"`,
|
|
`mode: "async"`, a title of `Council <seat> <KEY>`, and the seat's `target`. Leave `isolation`
|
|
unset, so each seat reads this checkout, and leave `runtimeMode` unset.
|
|
3. End your turn. T3 wakes you as each seat finishes. Leave the checkout alone until every seat
|
|
has returned, because they are reading it.
|
|
4. A seat refused with `model_unavailable`, or one that fails: record it with `ticket-page event`,
|
|
carry on with the rest, and name it at hand-off. Keep the seat empty rather than filling it with
|
|
another model. If no seat can run, ask Gib with AskUserQuestion. A timeout or silence is not
|
|
an approval.
|
|
|
|
Outside T3 Code (no `T3CODE_THREAD_ID`), run one fresh subagent from the harness with the same
|
|
brief. Never review the change in your own context: the value here is the reviewer not being
|
|
the author.
|
|
|
|
### The brief
|
|
|
|
A delegate gets none of this thread, so the brief is all it knows. Fill the placeholders.
|
|
|
|
```
|
|
Review only. Do not edit, commit, push, run anything destructive, or start subagents.
|
|
Repo: <absolute path> Range: <base>..<head> Ticket: <KEY>
|
|
|
|
Read in this order, or you will report conventions this project already settled:
|
|
1. .claude/docs/epics/<EPIC-KEY>/summary.md, if it exists
|
|
2. <scratch path of `ticket-page md <KEY>`>
|
|
3. <target-dir>/resources/
|
|
4. AGENTS.md
|
|
5. git diff <base>..<head>, then the code around it
|
|
|
|
Verify rather than suspect. You may run tests and read-only checks. Try to refute each finding
|
|
before you report it. A plausible worry is not a finding.
|
|
|
|
Re-derive every "proven by X" already written into the ticket's risk or test tables. A false
|
|
proof claim in Jira is worse than an unproven one, because it stops anyone else from checking.
|
|
|
|
Answer these against the ticket and plan, not the diff alone, quoting the AC ID or plan step:
|
|
- What did the ticket ask for that is missing or only partly done?
|
|
- What behavior is in the diff that nothing asked for?
|
|
- What looks implemented but is implemented wrongly?
|
|
|
|
Hold the change to its smallest correct form. Flag code, tests, or guards the ticket does not need.
|
|
blocker = the intended case is wrong. should = a real bug callers will hit. nit = everything else,
|
|
including rare paths, small performance costs, and style.
|
|
|
|
Reply with:
|
|
VERDICT: APPROVE or CHANGES
|
|
S1 [blocker|should|nit] [CONFIRMED|PLAUSIBLE] <file>:<line> <claim>. Failure: <concrete scenario>. Fix: <smallest fix>
|
|
```
|
|
|
|
## Judge the findings
|
|
|
|
Findings are not automatically true, and reviewers produce confident wrong answers. Check each
|
|
one against the code yourself, and regrade it: a rare path is a nit whatever label it came with.
|
|
Two seats reporting the same thing is one finding with more weight behind it. A finding only one
|
|
seat raised is still worth checking, since catching what the others miss is why the seats differ.
|
|
|
|
Put each on the page as you judge it, with its grade and its seats in the text:
|
|
`finding <KEY> add "[should] <text> (Claude, MiMo)" --severity CONFIRMED --where <file:line>`, then
|
|
`fixed --commit <sha>`, or `rejected --reason "<why>"`.
|
|
|
|
- **Fix every blocker and should you confirm,** each meaningful fix as its own commit. These do
|
|
not defer to a follow-up ticket unless Gib decides that.
|
|
- **Take the smallest fix.** Prefer deleting or narrowing code over a new lock, cache, helper, or
|
|
option. Compare `git diff --shortstat <base>` before and after: if review grew the diff, cut
|
|
first. Review loops grow a change one cheap-looking fix at a time, and it is your job to hold
|
|
the scope.
|
|
- **Decline nits in one line** with `rejected --reason`, and list them at hand-off as follow-ups.
|
|
Agreeing just to end the review is how a change bloats.
|
|
- **When a fix changes a mechanism rather than a line,** write a test that fails against the old
|
|
behavior and passes against the new one, and confirm it fails by reverting the fix. A test that
|
|
passes either way proves nothing.
|
|
- **If a finding invalidates something already in Jira or a deliverable,** correct it there too.
|
|
Fixing the code does not fix the false claim.
|
|
|
|
## Stop
|
|
|
|
One round is the default. `pre-mr-review` reviews the whole diff, your fixes included, and serves
|
|
as the next pass. A second round runs only if a fix changed a mechanism: fresh `delegate_task`
|
|
calls, the same brief, the ledger of what you fixed and rejected with your evidence, and an
|
|
instruction to check only the open ids and the changed code. Two rounds is the limit. A finding
|
|
still disputed then goes to Gib with AskUserQuestion, both sides in two or three lines each.
|
|
|
|
## Checking a dev review
|
|
|
|
`review-ticket` borrows the seats for a draft instead of a diff. Same `delegate_task` settings, and
|
|
no code gets changed. Give each seat this:
|
|
|
|
```
|
|
Review only. Do not edit anything, write to Jira, or start subagents.
|
|
Repo: <absolute path> Draft: <dir>/review.md Story: <scratch path of the fetched story>
|
|
|
|
Check the draft against the code. Report each Current State claim that is false, each "does not
|
|
exist" that does, each named pattern that is a poor fit or has a better neighbour, each seam the
|
|
Relationships section gets wrong, and any story AC the implementation guidance would not satisfy.
|
|
Say whether the estimate misses work the code paths table leaves out.
|
|
|
|
Reply with:
|
|
VERDICT: APPROVE or CHANGES
|
|
S1 [blocker|should|nit] <section> <claim>. Evidence: <file:line>. Fix: <suggestion>
|
|
```
|