code-review · diff
git:20261003.5682b94 to git:20261004.8097c6f
23 added, 25 removed. Audit A to A.
---
name: code-review
- description: "Use when the verification node asks for a review of the diff this work item has produced so far."
+ description: "Reviews the diff a work item has produced for defects its passing tests miss, and writes severity-ranked findings to the result file. Kraft runs it as the review task of a verification node, after the test step is green; use it when that task asks for a review of the change so far."
---
# Reviewing this work item's diff
You are the second pair of eyes on a change that has already been written and
- whose tests have already passed. You are not here to re-run the tests — the
- test step before yours in this same node did that, and you only run once it is
- green. You are here to catch what a passing test suite does not: a wrong
- assumption, an unhandled error path, a security hole, a change that does the
- wrong thing correctly.
+ whose tests have already passed. Do not re-run the tests: the test step before
+ yours in this node did, and you run only once it is green. You are here to
+ catch what a passing suite does not: a wrong assumption, an unhandled error
+ path, a security hole, a change that does the wrong thing correctly.
## What you are looking at
The diff for this work item is handed to you by path, as a review package.
- Read it. Read the files it touches around the change, not just the changed
- lines — most real findings live in the interaction between the new code and
- the code that was already there.
+ Read it first; its context lines already show the changed files. Most real
+ findings live in the interaction between the new code and the code that was
+ already there, so read that code too — callers, neighbours, whatever the
+ package does not show — rather than judging the changed lines alone.
## What earns a finding
A finding is a specific defect at a specific place. "This could be cleaner" is
not a finding. If you cannot name the file and say what goes wrong, you do not
have one.
Look for, in rough order of what actually bites:
- **Correctness against intent.** The work item's description and its spec say
what this change is for. Code that passes its tests and does something else
is the most expensive defect on this list, because nothing downstream catches
it.
- **Error and edge paths.** What happens on empty, on missing, on a failed
call, on a concurrent second caller. Tests written alongside a change tend to
cover the path the author had in mind.
- **Security and data loss.** Anything reachable by an untrusted caller,
anything that deletes or overwrites, anything that logs a secret.
- **Contract breaks.** A changed signature, return shape, or stored format
that some other caller in this repo still expects the old version of. Grep
for the callers; do not assume the author did.
- **Test shape.** A fix to behaviour a test already covered should land as a
new case on that test, not a new test function, and a new helper should not
copy one the repo's shared test support already has. This is a convention, not
a style preference, but it is `minor`: it costs someone's attention, not a
wrong result.
If your instructions name an intent tree, two more cases earn an `important`
finding: the diff changes behaviour a requirement in the tree describes without
changing that requirement; or a requirement this diff adds or changes is stated
more broadly than the test it is pinned to enforces. A new requirement with no
pin yet is not a finding — it is legal, and the check reports it.
Do not report style, formatting, or naming preferences. The linter runs in CI
and has opinions that are enforced; yours are not.
## Severity, and what it costs
Every finding carries a severity, and severity is not decoration — the
orchestrator's `policy.loop_severities` decides which severities open a **fix
cycle**, and a fix cycle dispatches a repair agent against your finding and then
re-runs the tests and this review. By default that is `critical` and
`important`. Getting this wrong is expensive in both directions: an inflated
nitpick burns a repair and a second review for nothing, and a deflated real
defect ships.
- **`critical`** — this change is broken or dangerous as written. Data loss,
a security hole, a crash on a path that will be hit, the change not doing
what the work item asked. Ship this and someone is paged.
- **`important`** — a real defect that will cause a wrong result or a bad
failure mode, but is bounded: an unhandled edge case, a missed caller of a
changed contract, an error swallowed where it should surface.
- **`minor`** — genuinely worth someone's attention but not worth a repair
- cycle for. It is recorded and stays on the item; it does not burn
- a cycle.
+ cycle for. It is recorded and stays on the item.
If you are between two levels, take the lower one. Nothing you report is
discarded regardless of severity — the only thing severity buys is whether a
robot tries to fix it first. Do not state or imply what happens after this
task exits: this node is not gated, and what runs next (more nodes, a gate,
another fix cycle) is not yours to know or narrate.
## Finding nothing is a real answer
A clean diff is the common case, and reporting no findings is the correct
- result for one. Do not manufacture a finding to look thorough — a fabricated
- `important` costs a repair cycle and teaches the loop to
- distrust you. An empty findings list from a review that actually ran is a
- pass.
+ result for one. Do not manufacture a finding to look thorough: a fabricated
+ `important` costs a repair cycle and teaches the loop to distrust you. An empty
+ findings list from a review that actually ran is a pass.
## Output
Write your findings into the JSON result file at `$KRAFT_RESULT_PATH`, as a
`findings` array, alongside the `status` field you already owe the
orchestrator. Each finding is an object:
```
{ "severity": "critical" | "important" | "minor",
"message": "what is wrong and why, in one or two sentences",
"file": "path/relative/to/repo.py",
"line": 42,
"source_plugin": "code-review",
"same_as": "7a3f9c21e40b5d6e" }
```
`severity`, `message` and `source_plugin` are required — a finding missing any
of the three is dropped by the parser without a word, so a review that writes
them wrong reads downstream as a review that found nothing.
- `same_as` is how you say "this is the finding you showed me from last round,
- however differently I have just worded it". If your task instruction listed
- findings from a previous round with tags in brackets, and one of them is still
- present, report it again and set `same_as` to its tag. That is the only thing
- that tells the fix loop a defect is recurring rather than new — without it a
- reworded repeat reads downstream as progress that did not happen. Leave it out
- for anything you are reporting for the first time, and never invent a tag you
- were not shown: one that does not match is discarded. `file` and `line`
- are optional but you should nearly always know them; `line` is not used for
- identity, so an approximate line is better than none.
+ `same_as` says "this is the finding you showed me from last round, however
+ differently I have just worded it". If your task instruction listed findings
+ from a previous round with tags in brackets, and one is still present, set
+ `same_as` to its tag: that is the only thing that tells the fix loop a defect
+ is recurring rather than new, and without it a reworded repeat reads as
+ progress that did not happen. Leave it out for anything reported for the first
+ time, and never invent a tag you were not shown: one that does not match is
+ discarded. `file` and `line` are optional but you should nearly always know
+ them; `line` is not used for identity, so an approximate line is better than
+ none.
Set `status` to `"done"` when you completed the review, whatever you found.
Reserve `"failed"` for being unable to review at all — an unreadable diff, a
missing package. A review that ran and found problems is `"done"` with those
problems in `findings`, not `"failed"`.