review-correctness

Files (7)

• SKILL.md
• learning-exemplars.template.md · references/
• work-quality-base.template.md · references/
• work-quality-human.template.md · references/
• work-quality-polecats.template.md · references/
• work-quality-system.template.md · references/
• rubric-ledger.md

SKILL.md (first lines)

# The correctness check

The correctness check is the standing review of whether a change is correct
and safe as merged. It is the concern, not a tool: no reviewer is presumed.

## What it reads

The whole diff at `<reviewed_oid>`, in `<worktree>`:
`git diff origin/<review_base>...HEAD`. A large diff gets read in chunks, not
sampled. The review formula printed the rest. The anchor `<anchor_bead>` says
what the change was asked to do. The PR as published is what the What to check
list's last dimensions read. What `gctk rig` recorded at `<reviewed_oid>` is
the test evidence, below. The rig's guidance for this check is
`docs/review-correctness.md` at the reviewed commit, printed when the commit
carries one.

## What it judges

### Settle the test evidence

Every run of the rig's tests through `gctk rig`, whether by a self-review, the
refinery or an earlier review, is recorded against the commit it ran at, and
every worktree of the rig's clone reads the records. The review formula printed
a line for each check `gctk rig` records at `<reviewed_oid>`, and the gctk
binary it read them with. That binary is the gate, and a suite you run through
the gate calls it by that path. The `test` line is the test evidence:

- A pass of the whole suite (scope `full`) or of the subset the commit's diff
  selects (scope `affected`) is evidence. Run no suite.
- A fail the line calls pre-existing fails on the base too, and the trackers it
  names carry the failure. It goes in the verdict as context, with its
  trackers, and is not run again.
- Any other fail is the commit's. Run the record's command again through the
  gate, `gctk rig test --scope <its scope> --command '<its command>' --force`,
  to see what failed, and check whether it fails on the base too.
- Nothing recorded, a pass that ran nothing (scope `none`, a rig that declares
  no test command), or a record gctk could not read is no evidence. Run the
  suites the diff touches in `<worktree>` through the gate, so that run is
  recorded as well, `gctk rig test --scope affected --command '<the suites>'`,
  and record the real pass and fail counts. With no gctk binary there is no
  gate, so run them plain. A suite is one the diff touches when the diff
  changes it or a file it reads or runs. When the diff touches none, as when it
  changes only a document that no suite reads, there is nothing to run. Run
  nothing, and the Verification line says so, with how you found that no suite
  touches the diff.

Either way you may run one targeted suite for a gap you suspect, and the run of
each new case against the pre-change code that the Testing dimension asks for
is yours. Run those plain, each as `bash -c '<suite>'`, because a gate call
with another command replaces the reviewed commit's record. Every suite runs
under the claim-lease keepalive the review formula names. A failing suite is
not automatically a finding: check whether it fails on the base too. A
pre-existing failure belongs in the verdict as context, not as this branch's
defect.

### What to check

Work the whole list on every round. A P0 or a P1 is not a stopping point: a
blocking finding discovered a round later costs another rework and another
full review, so one verdict enumerates every blocking finding this diff
carries, not the first one that appears.

- **Intent** — does it do what the anchor bead asked? Deviations are fine
  when they are better; flag them either way.

The hunting angles come next. Each reads the whole diff from its own
direction, and a defect one direction passes over can be in plain view from
another. So work every angle, even after another has raised a finding at the
same place. A defect two angles find is one finding, raised under the angle
that names its cause.

- **Line by line** — read every changed line inside the whole function, block
  or section that encloses it, not the hunk alone. Bugs in the unchanged lines
  of a touched function are in scope, because a change's defect often sits in
  a line it left alone: one that now receives a new input, runs under a new
  condition or in a new order, or reads a value the change redefined. At each
  line, ask what input or state makes it wrong: a condition that tests the
  wrong thing or misses a case, a boundary handled on one side only, a value
  used before it is checked, two writers that race, a limit or a cache that
  never resets, an error swallowed or reported as success, outside input taken
  unchecked, the failure mode nobody wrote a test for. A defect in an
  unchanged line is this diff's when the change brought it about, and one the
  line already had is real but not this diff's.
- **Removed behavior** — for every line the diff deletes or replaces, name
  the invariant it held: the check it made, the case it refused or answered,
  the error it reported, the guarantee its callers relied on. Then find where
  the new code re-establishes that invariant, on every path the old line
  covered. A replacement that drops a refusal, narrows a check, loses an error
  path, answers a batch differently from its items one at a time, or ports a
  function without one of its cases is a finding, unless the change says why
  the behavior goes and nothing still relies on it.
- **Cross-file trace** — follow everything the change touches into the files
  around it, to its callers and its callees. Does it break a caller, a stored
  format, a metadata key, or a script that greps for the old shape? Find the
  other call sites of anything it changed; the recurring defect class is a
  predicate fixed in one copy and left stale in two. Read each callee the
  change now relies on, for what it does with the input the change now passes
  it. A fact the change works out from a shape, such as a path, a name or a
  format, when another component records it, is a finding, and the fix reads
  that record. Search the same way for what it adds: a rule the diff writes
  where existing code already defines it, or one edit repeated across copies
  of a code path, is a finding until one definition serves every site, and a
  follow-up to fold the copies later does not answer it. Where the surface has
  no include mechanism, that definition is a marker-fenced block a test holds
  every copy to.
- **Language pitfalls** — hunt the defects each language and framework in the
  diff is known for, file by file: how it splits, quotes and expands, how it
  treats a missing or null value, what it converts silently, what it does by
  default on an error. A failed read taken as an empty or clean answer is one
  in every language. Where the rig's guidance for this check names the
  pitfalls this repository has met, and which of them its gates catch, hunt
  those first, and hardest where no gate reaches.
- **Wrappers and delegates** — where the change adds or edits a wrapper, a
  proxy, an adapter, or any code that hands its work to another, check the
  hand-off in both directions. Each argument, flag, input and environment
  value a caller passes must reach the delegate as the caller meant it, and
  each result, exit status, error and signal the delegate returns must reach
  the caller. A default the wrapper adds, a timeout or a retry it imposes, or
  an output it filters is checked against every caller that goes through it,

From zookanalytics/gc-toolkit, MIT.

More from zookanalytics/gc-toolkit