# Review & PR

Give reviewers separate findings, observable behaviour and a clear account of merge risk.

## Tidy code can still deliver the wrong thing

The export button follows every convention, but downloads only the current page. The ticket asked for every filtered result. A different change might export correctly while bypassing the agreed permission boundary. A single review score hides which problem remains.

Before reading on, predict what a clean type check proves about export coverage. It proves only the properties it checks. Your goal here is a change description another person can use to judge behaviour and risk.

Figure: Review requirements and standards as two independent dimensions. ([s08-two-axes](/art/s08-two-axes.en.svg))

## Keep two independent findings

Fix the start and end of the comparison. Locate the ticket, specification and repository standards. Check requested behaviour on one axis and repository commitments on the other. If no specification exists, record that gap and establish the intended behaviour.

Separate a documented violation from a possible design smell. A violation needs a rule reference; a suggestion needs a reason. Keep the two conclusions even when both concern the same code.

<!-- compare -->
| Axis | Basis | Location | Finding | Evidence |
|---|---|---|---|---|
| Requirements | Ticket: export every result | Export entry | Only the current page is exported | 42 matches produce 20 rows |
| Standards | Repository permission rule | Download handler | Uses the shared permission entry | An anonymous request is rejected |

Review structure adapted from [Matt Pocock's skills repository][skills], MIT: `skills/engineering/code-review/SKILL.md`. The scenario and wording are ours.

## Give the reviewer something to verify

Use three parts for the PR. Describe the changed behaviour, compare results for the same input, then explain merge risk. Include the environment, affected callers, rollback conditions and anything still unverified.

Figure: A PR explains the change, before-and-after evidence and merge risks. ([s08-pr-blocks](/art/s08-pr-blocks.en.svg))

> showing hard evidence that the changes work and assessing merge risk
>
> — Matt Pocock, [source](https://x.com/mattpocockuk/status/2107062474914550098), 2026-10-05

An agent's confidence cannot fill the evidence column. Link a run, screenshot or log and say which claim it supports. A check that has not run remains an open item.

The three-part structure adapts `skills/engineering/pr/SKILL.md` from [skills][skills], MIT. That file credits Dex Horthy / HumanLayer's [show-me][showme]. We retain that credit without taking content from the upstream work.

## Exercise: make an export fix reviewable

### Add evidence and a rollback condition

- Description A: Fixed export. Checks passed. Ready to merge.
- Description B: CSV now uses the entire filtered result; identical filters produced 20 rows before and 42 after.
- Task: Choose the more reviewable version. Add the run location, a failure case and a rollback condition.

Suggested approach: B gives a comparison to investigate. Identify the fixture and run, then show that anonymous downloads fail. An application rollback may suffice if the CSV interface stays compatible and persistent data is untouched; otherwise investigate recovery separately.

> **Our inference**
> Our suggestion: describe reversibility as a conditional claim. Reverting code does not necessarily undo a file already sent, an email delivered or an external write.

## Done means someone else can assess it

A reader can locate the comparison boundary, evidence and basis for each axis, then state both conclusions independently. They can also identify affected users, rollback conditions and unverified behaviour. Missing information means more documentation or verification is needed.

Carry this state into a fresh session with the [handoff lesson](/en/context-and-handoff).

## Quick reference

- **Requirements** Check acceptance behaviour; make a missing specification visible.
- **Standards** Separate rule violations from suggestions that require judgement.
- **PR structure** Change overview, before-and-after evidence, merge risk.

## Sources and attribution

- Matt Pocock, [review evidence and merge risk](https://x.com/mattpocockuk/status/2107062474914550098), 2026-10-05.
- [skills repository][skills]: the two structures identified above; Copyright (c) 2026 Matt Pocock.
- [show-me upstream credit][showme], Dex Horthy / HumanLayer; attribution only.
- [Site copyright details](/en/about-and-copyright). The complete MIT licence for the adapted structures follows at the end of this page.

[skills]: https://github.com/mattpocock/skills
[showme]: https://github.com/humanlayer/skills/blob/main/plugins/show-me/skills/show-me/SKILL.md

## skills MIT notice

The structure adapted here comes from [mattpocock/skills](https://github.com/mattpocock/skills), MIT licence, Copyright (c) 2026 Matt Pocock. The complete licence follows, identical to [the repository LICENSE](https://github.com/mattpocock/skills/blob/main/LICENSE).

### Full licence: mattpocock/skills

```text
MIT License

Copyright (c) 2026 Matt Pocock

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
```

Unofficial, non-commercial study notes, with no affiliation with or endorsement by Matt Pocock. Quoted works belong to their authors; MIT reuse carries separate copyright and licence notices.