radin-review · diff
git:20261015.55c3bae to git:20261015.5ee7c80
87 added, 142 removed. Audit A to A.
---
name: radin-review
description: |
Review a commit, PR, directory or range for the simplest native solution,
maintainability and documented repo rubrics, triage findings with the user.
Use for /radin-review, "review and log to backlog".
---
# Review
- Review a caller-specified scope in one pass. A human caller then picks, per
- kept finding group, to add it to the backlog or tackle it now. An agent
- caller gets the findings back and acts on them itself.
-
- One question drives the review: is this the simplest, most native solution
- that still meets the thermo-nuclear constraints and this repo's documented
- rubrics? The originating backlog entry or its plan is context for that
- question: behaviour it asked for but the scope lacks, and behaviour the scope
- adds unasked, are ordinary findings.
-
- **Agent caller** — an execution leaf (scope `staged`, step 5a, before it
- commits) or `radin-batch`'s Phase 6 reviewer. An agent reaches neither the
- user nor a spawned agent's result, so run every step inline, skip each
- `AskUserQuestion`, write no backlog entry and return the findings. Take the
- spec the invoking prompt names (Step 2, rung 1). Prove each `critical` finding
- on the real artifact: run the changed code path and read its output (rung
- `ran`) instead of citing it. Step 4 ends for an agent caller after the
- call-site test; Step 6 says no triage happened.
+ Review a scope, then triage the findings with the user: each kept group goes
+ to the backlog or gets tackled now. An agent caller reads
+ `RADIN_LIB/radin-review-core.md` and follows it instead of this skill.
## Step 1: Resolve the scope
```bash
RADIN_CLI scope [<arg>]
```
- It prints `type`/`scope`/`command`/`passes` lines, and Step 3 invokes exactly
- the skills the `passes` line names. Route on its exit code:
+ It prints `type`/`scope`/`command`/`passes` lines, and Step 3 hands the
+ reviewer exactly the skills the `passes` line names. Route on its exit code:
- **1**: not a commit, PR, directory, range, or `since <date>`. Report it and
stop.
- **2**: ambiguous, candidates on stderr (e.g. both a PR number and a
- directory). Ask which one; a sub-agent caller reports both readings and
- stops.
+ directory). Ask which one.
- State the resolved scope in one line — `Scope: commit a1b2c3d`,
- `Scope: directory src/auth/` — then capture both inputs the axes need, here
+ State the resolved scope in one line, `Scope: commit a1b2c3d` or
+ `Scope: directory src/auth/`, then capture both inputs the review needs, here
and once:
- **The diff command.** The `command` line is the scope's one diff command.
Record it verbatim and paste that same string into every later Bash call and
- both briefs.
+ the brief.
- **The commit list.** `RADIN_CLI scope --commits [<the same scope arg>]`
prints one `<hash> <subject>` line per commit. A `dir` scope prints nothing:
it has no commit list, and the brief says so.
- Run the `command` before the review starts. No output — for a `dir` scope, no
- file under the path — means report "nothing to review in `<scope>`" and stop.
+ Run the `command` before the review starts. No output (for a `dir` scope, no
+ file under the path) means report "nothing to review in `<scope>`" and stop.
## Step 2: Resolve the spec
- The review reads the originating backlog entry or its plan as context. Work
- these rungs in order and stop at the first that yields a spec:
+ Work these rungs in order and stop at the first that yields a spec:
- 1. **The invoking prompt named one** — a backlog id, a title, or a path. For an
- id or title:
- `RADIN_CLI backlog field "<it>" TASK_FILE` and
- `RADIN_CLI backlog field "<it>" PLAN_PATHS` (its exit 1 means no plan; the
- task file alone is then the spec). For a path: that file is the spec.
- 2. **The backlog holds it.** One call joins entries to the scope:
+ 1. **The invoking prompt named one**: a backlog id, a title, or a path. For an
+ id or title, `RADIN_CLI backlog field "<it>" TASK_FILE` and
+ `RADIN_CLI backlog field "<it>" PLAN_PATHS` (exit 1 means no plan; the task
+ file alone is then the spec). For a path, that file is the spec.
+ 2. **The backlog holds it.**
```bash
RADIN_CLI scope --specs [<the same scope arg as Step 1>]
```
It prints one `id`, `created`, `branch`, `commit`, `source` line per
candidate, oldest first. `source` says how the entry joined: `commit` (the
task's commit is in scope), `reflog` (a squash or rebase rewrote that
commit, and the review branch's reflog still lists it) or `pending` (an
- open entry recorded on the review branch). Route on the result:
+ open entry recorded on the review branch).
- **Exit 4**: a recorded commit no longer exists and HEAD sits on no branch.
Report the stderr line and stop the review.
- **No output**: fall to rung 3.
- - **Lines**: take every line without asking. Never ask the user to confirm,
- order or drop entries. A sub-agent caller takes the `commit` and `reflog`
- lines only: a `pending` entry is unfinished work, not what the scope
- implemented.
+ - **Lines**: take every line without asking.
A `pending` id resolves as in rung 1. For the others,
`RADIN_CLI state completed-show "<id>"` names the plan the task ran
- against: its `plan` line carries the recorded paths, comma-separated. Also
- run `RADIN_CLI state trace "<id>"` for each completed id: when its `facts`
- line names a file, that file is part of the entry.
+ against in its `plan` line, comma-separated. `RADIN_CLI state trace "<id>"`
+ adds the file its `facts` line names to the entry.
- Then sort the entries into two roles. Read each plan and decide:
+ Sort the entries into two roles by reading each plan:
- **Follow-up**: the plan answers a review finding (it quotes a
`**Finding:**`, a `**Severity:**` or a `review` origin) or fixes work an
earlier entry shipped. These are **decisions**.
- **Feature**: every other entry. These are the **spec**.
- The sort is yours; never put it to the user. When every entry is a
- follow-up, there is no spec, only decisions: the review runs with them and
- reports `no spec available` in Step 6.
+ When every entry is a follow-up, there is no spec, only decisions, and
+ Step 6 reports `no spec available`.
3. **Ask.** One `AskUserQuestion` offering the top entries from
- `RADIN_CLI backlog list` as options, with the tool's free-text field
- carrying a path the user types instead. "There isn't one" falls to rung 4.
- 4. **Skip.** No spec: the review runs without one. Say so once here, and report
- `no spec available` in Step 6. A sub-agent caller arrives here straight from
- rung 2.
+ `RADIN_CLI backlog list` as options, with the free-text field carrying a
+ path. "There isn't one" falls to rung 4.
+ 4. **Skip.** No spec: say so once here, and report `no spec available` in
+ Step 6.
## Step 3: Run the review
- Dispatch one sub-agent. The brief is self-contained: paste in full the resolved
- scope line, Step 1's `command` string and commit list verbatim, and the
- citation rule below. The spec, when Step 2 found one, goes in as absolute
- paths with "read it in full before reviewing"; the decisions go in the same
- way, labelled: "these entries fixed earlier findings on purpose; the scope's
- behaviour they describe is settled, so never raise it again as a finding unless
- a changed line contradicts the decision."
+ When the invoking prompt hands over a core review's findings for this scope,
+ skip the dispatch and relay those.
- > Every finding cites one `path:line` the scope introduced: for a diff scope
- > the lines it adds or changes, for a `dir` scope the files under that path.
- > Read surrounding code for context, never to find findings. A problem that
- > predates the scope qualifies only when a changed line is what makes it wrong,
- > and that changed line is the citation. Skip anything a formatter or linter
- > already enforces.
- >
- > Give each finding a severity (`critical`, `warning` or `nit`) and its
- > evidence: the rung it reached — `cited` (you pointed at the line), `traced`
- > (you followed a caller or data path and showed the bad case happens) or
- > `ran` (you executed it and saw it fail) — then the reasoning or output.
- > Trace callers and data paths through `codebase-memory-mcp`'s MCP tools
- > before Grep: a graph hit is a pointer, so read the file before you cite it,
- > and never conclude a caller is absent from an empty result.
+ Dispatch one sub-agent with a self-contained brief:
- **Brief.** Invoke `/thermo-nuclear` against the scope, then every skill Step
- 1's `passes` line names. Report — per file/hunk where relevant —
- (a) every place the scope is more code, more abstraction or less native than
- the task needs, and every structural finding the passes raise: name it and
- quote the hunk; (b) every place the scope breaks a rubric this repo documents:
- cite the rubric, file and rule; (c) with a spec: requirements it asked for that
- are missing, only partly implemented or implemented wrongly, and behaviour the
- scope adds that it never asked for — quote the spec line, and cite the
- `path:line` it lands on. A documented repo rubric overrides a pass's generic
- judgement, and a pass finding stays a labelled judgement call
- ("possible <name>") — the label reaches the backlog entry an agent later acts
- on without the review in front of it. Report findings only, each as its claim, citation, severity and evidence.
+ - `Read RADIN_LIB/radin-review-core.md and follow it at level session` (level
+ `diff` for a `staged` scope).
+ - The resolved scope line, Step 1's `command` string and commit list,
+ verbatim.
+ - The spec and the decisions as absolute paths with "read it in full before
+ reviewing". Label the decisions: "these entries fixed earlier findings on
+ purpose; the scope's behaviour they describe is settled, so raise it again
+ only when a changed line contradicts the decision."
+ - The skills Step 1's `passes` line names, as extra passes to invoke after
+ the core's two.
- Relay the report before any triage, in the order the sub-agent gave it.
+ Relay its report before any triage, in the order the sub-agent gave it.
## Step 4: Filter, classify, triage
Nothing is logged or acted on until the user agrees to it. The citations go
through one filter, which drops the out-of-scope findings:
```bash
printf '%s\n' "<path:line per finding, one per line>" |
RADIN_CLI scope --in-scope [<the same scope arg as Step 1>]
```
Keep the findings on the `in` lines, drop the `out` ones, and carry the
- `dropped` count into Step 6. Cite one line per finding; for a range, its first
- line. A finding about missing behaviour cites the in-scope line the
- requirement should have landed on — the hunk with the partial behaviour, or the
- one nearest where it belongs. A finding with genuinely no in-scope line stays
- in Step 3's relay, and Step 6 names it as unlogged.
-
- Then test each `cited` finding that depends on an input or a state ("what if
- this is null"): it survives only when you find a call site or data path that
- produces that input. Find the callers through `codebase-memory-mcp`'s MCP
- tools, read them to settle it, and dismiss the finding when none does — an
- empty graph result is not proof, so confirm with Grep before dismissing. A `traced` or `ran` finding already carries that proof.
-
- An agent caller stops here: its survivors go to Step 6 as Step 3's claim,
- citation, severity and evidence.
+ `dropped` count into Step 6. A finding about missing behaviour cites the
+ in-scope line the requirement should have landed on. A finding with no
+ in-scope line stays in Step 3's relay, and Step 6 names it as unlogged.
- Then classify each survivor: **fix** for
- incorrect behavior, including a missing, partial or wrongly implemented
- requirement, and **refactor** for everything else, scope creep included.
+ Classify each survivor: **fix** for incorrect behavior, including a missing,
+ partial or wrongly implemented requirement, and **refactor** for everything
+ else, scope creep included.
- Then attribute each survivor's origin:
+ Attribute each survivor's origin:
```bash
printf '%s\n' "<path:line per survivor>" |
RADIN_CLI scope --attribute [<the same scope arg as Step 1>]
```
Each line carries the citation, its task id and its plan paths (comma-joined);
`none` in either field, a `dir` scope or an untraced commit means class
`none`. Otherwise settle the class in this order:
1. `RADIN_CLI state plan-covers "<plan csv>" "<cited path>"` prints `out`:
class `executor`, the code touched a file outside the plan's Changes.
2. Otherwise read the plan's `## Decisions`: class `plan` when a Decisions line
prescribed the flaw or left the choice open for the executor to guess,
`executor` when the plan was right and the code deviated. Quote that
- Decisions line, or write `no Decision covers it`. For class `plan`, also name the plan-template section whose entry
- would have caught the flaw (`outcome`, `decisions`, `changes`,
- `reach-and-reuse`, `rubrics-triggered`, `modes-and-states`, `order`,
- `testing`, `out-of-scope`), or leave it empty when no section would have.
+ Decisions line, or write `no Decision covers it`. For class `plan`, also
+ name the plan-template section whose entry would have caught the flaw
+ (`outcome`, `decisions`, `alternatives`, `changes`, `reach-and-reuse`,
+ `rubrics-triggered`, `modes-and-states`, `order`, `testing`, `risks`,
+ `out-of-scope`), or leave it empty when no section would have.
- Then group the survivors. Merge
- survivors into a group when one fix touches them all:
+ Merge survivors into a group when one fix touches them all:
- They cite the same file, or overlapping hunks, or share one root cause or
remedy.
- - They share one task id, origin class and plan paths. Attribution then
- stays true for the whole group.
+ - They share one task id, origin class and plan paths, so attribution stays
+ true for the whole group.
A group takes `fix` when any member is `fix`, and the highest member severity.
- Leave a survivor alone when no other survivor shares its fix.
- Sort each group into a bucket, each with a one-line
- reason. A group takes its highest member's bucket.
-
- - **Act on**: survives every filter below, is `critical` or `warning`, and
- carries a `traced` or `ran` rung, a quoted spec line or a cited rubric.
- - **Consider**: survives every filter, is `critical` or `warning`, and carries
- only a `cited` rung or a "possible <name>" label.
- - **Noted**: survives every filter and is a `nit`. When every survivor is
- Noted, Step 6 says the code is fine.
- - **Dismissed**: fails a filter. The call-site test above dismisses
- hypotheticals. A finding asking for an interface, config or generalisation
- with one caller or one value is a premature abstraction. A finding that cites
- no rubric, quotes no spec line and shows no incorrect behavior is "I would
- have done it differently".
+ Sort each group into a bucket, each with a one-line reason. A group takes its
+ highest member's bucket.
- More than five Act on groups: re-test the weakest against the last two filters
- and demote it to Consider before printing.
+ - **Act on**: `critical` or `warning`, with a `traced` or `ran` rung, a quoted
+ spec line or a cited rubric.
+ - **Consider**: `critical` or `warning`, with only a `cited` rung or a
+ "possible <name>" label.
+ - **Noted**: a `nit`. When every survivor is Noted, Step 6 says the code is
+ fine.
+ - **Dismissed**: a finding asking for an interface, config or generalisation
+ with one caller or one value (premature abstraction), or one that cites no
+ rubric, quotes no spec line and shows no incorrect behavior ("I would have
+ done it differently").
Print the groups as a numbered list in the order Act on, Consider, Noted,
- Dismissed — number, bucket and reason, category, severity, location, the
+ Dismissed: number, bucket and reason, category, severity, location, the
finding in a clause. A group lists each member on a lettered sub-line with its
- location and clause. It carries no further detail.
+ location and clause.
Then gate on one `AskUserQuestion` with two single-select questions:
- Which groups: **Recommended only** (the Act on groups) or **All** (Act on
and Consider).
- What to do with them: **Add to backlog** or **Tackle now**.
A free-text answer names the numbers you printed, Noted and Dismissed ones
included; read it as that subset of groups and nothing more, and restate the
subset in one line before continuing.
## Step 5: Act on the agreed findings
Build one body per agreed group. It carries what a sub-agent with no review
context needs to act on the group. A group repeats `**Finding:**`,
`**Severity:**` and `**Evidence:**` once per member, each opened with the
member's `path:line`; the `**Preferred remedy:**` covers all members:
```
**Scope:** <what was reviewed, from Step 1>
**Finding:**
<the problem as the review stated it: direct, specific. A finding against the
spec opens with the spec line the brief quoted.>
**Severity:** <critical|warning|nit>
**Origin:** task <id|none> · plan <path|none> · class <plan|executor|none> — <the quoted ## Decisions line, or the reason>
**Evidence:** <rung: cited|traced|ran> — <the reasoning or output the brief gave>
**Preferred remedy:**
<the concrete restructuring suggested>
```
For class `plan` and `executor`, end the `**Origin:**` line with
`Read <plan path> ## Decisions before changing code.`, so the fix's sub-agent
reads the original plan's decisions.
Act on each agreed group in the order presented, and on nothing the user
discarded:
- **Add to backlog**:
```bash
RADIN_CLI backlog add <fix|refactor> "<short title>" <<'EOF'
<the body>
EOF
RADIN_CLI backlog set-meta <id> location "<the cited path:line>"
```
For a group, `location` is the first member's citation. Flag the entry
`RADIN_CLI backlog set-meta <id> planning needed` unless its remedy touches
one or two files with one obvious approach.
- **Tackle now**: invoke `/radin-do` with the body as its prompt, one group at
a time, each to its Phase 5 summary before the next. `/radin-do` records the
entry itself, so run no `add`.
After the last group, journal the agreed findings in one call (skip it when
nothing was agreed) and keep its output for Step 6. Write one row per member
finding, not per entry, so the rollup counts findings. A class `plan`
finding carries its Step 4 section as a fourth column, empty when it has none;
other classes carry no fourth column:
```bash
printf '<task id or empty>\t<class>\t<plan path or none>[\t<section>]\n...' |
RADIN_CLI state review-findings
```
## Step 6: Report back
- **Reply:** the resolved scope reviewed, and the spec path or `no spec available`.
- An agent caller then lists every finding as claim, citation, severity and
- evidence, with the dropped-as-out-of-scope count, says no triage happened, and
- stops. Otherwise: one line per agreed group, stating how many findings it
- covers and the backlog id or the `/radin-do` outcome; the origin rollup, `review-findings`'
- output verbatim (for example `12 findings: 9 plan (3 tasks), 2 executor, 1 none`,
- then one line per task); three counts, no detail, for findings the user
- discarded, dropped as out of scope, and dropped for having no in-scope line; one
- line each, finding and reason, for every Dismissed group so the user can
- overrule; the Noted count; the backlog index path when any group went to the backlog. Zero findings, or only
- Noted ones: say the review passed and the code is fine. Every finding discarded: report that. End with one line: total findings, and the worst issue.
+ **Reply:**
+
+ - The resolved scope, and the spec path or `no spec available`.
+ - One line per agreed group: how many findings it covers, and the backlog id
+ or the `/radin-do` outcome.
+ - The origin rollup, `review-findings`' output verbatim (for example
+ `12 findings: 9 plan (3 tasks), 2 executor, 1 none`, then one line per
+ task).
+ - Three counts, no detail: findings the user discarded, dropped as out of
+ scope, and dropped for having no in-scope line.
+ - One line each, finding and reason, for every Dismissed group, so the user
+ can overrule.
+ - The Noted count, and the backlog index path when any group went to the
+ backlog.
+ - Zero findings, or only Noted ones: the review passed and the code is fine.
+ - A last line: total findings, and the worst issue.