Pull Request Guideline

Spirit (the 3 non-negotiables)

• Scope before narrative: the reviewer must know the risk level and the blast radius before opening the diff, not after reading it.
• Attach evidence to every claim: each change points at its test, each risk points at its reproduction, each "not affected" points at something checkable.
• Tests must match the stated risks: a PR that warns about a race condition and ships only sequential tests has not been verified.

A description exists to help the reviewer approve quickly and correctly — to know what to read closely, what to skim, and how to confirm the result without rebuilding your environment. It is not a log of what you did.

Risk level

Put a single line at the very top of the description:

Risk: High

Judge the tier by what the change can break, not by how many lines it touches:

• High — inventory, money, authorization, database migrations, multi-table transactions, background jobs, anything with a concurrent writer.
• Medium — a new endpoint or screen on an isolated module, a refactor with no behavior change, a dependency upgrade.
• Low — copy, styling, comments, documentation, test-only changes.

The tier decides how much of the template is required:

• Low: sections 1, 2, 7, and 8 only. Delete the rest.
• Medium: all sections; section 5 may hold a single entry.
• High: all sections, and section 5 must cover every failure mode you can name.

Do not label a PR Low to skip the form. An under-stated risk level costs the reviewer more than a long description.

Pull request size

No template rescues a 2000-line diff. Before writing the description, check whether the change splits into independently reviewable parts: schema first, then behavior; refactor first, then feature. If a PR mixes a rename across 40 files with one logic change, the logic change will not be reviewed.

Section guide

1. Purpose

Two or three sentences on the problem being solved. No technical detail here — that belongs in section 2 onward.

2. Changes and tests

List each change paired with the test that covers it, one pair per line. Pairing is what turns "tests added" from a checkbox into something verifiable, and it exposes any change that has no test at all.

- Update Lot status after receiving
  -> test: tests/Feature/LotReceivingTest.php::test_lot_status_updates

- Aggregate order status (Pending / In progress / Completed)
  -> test: tests/Feature/LotReceivingTest.php::test_order_status_aggregation

If a change has no test, write no test and the reason. An honest gap is reviewable; a missing line is not.

3. Scope

Two lists:

• Affected: the specific modules, endpoints, jobs, or screens.
• Not affected: what the reviewer can safely skip — with the evidence that supports it.

"Not affected" is the most dangerous field in the template, because it tells the reviewer to stop looking. A bare assertion is not enough. Anchor it: name the callers of the changed method, state that the diff stays inside given directories, or point at the contract that did not move.

4. Where to start reading

Name the entry point. This matters most on PRs that span a transaction or several tables, where file order in the diff tells the reviewer nothing.

Start reading at: LotReceivingService::complete()

5. Risks and verification

One block per risk. Each block must be complete — this is where the third principle is enforced by structure rather than by reminder.

Risk: two requests updating the same Lot concurrently
Why: partial receiving can double-count stock
Reproduce: POST /lots/{id}/receive twice in parallel with the same payload
Expected: one stock movement; the second request fails or waits
Evidence: SELECT ... FOR UPDATE in LotReceivingService::lockLot(),
          tests/Feature/LotReceivingTest.php::test_concurrent_receive

Cover the cases that break, not just the happy path: partial input, full input, repeated submission, and failure midway through a transaction so no phantom state survives a rollback.

State the invariants in Expected as absolutes the system must hold — no duplicate stock movement, Lot status and order status always consistent.

On concurrency evidence. A true concurrent test needs multiple connections and is often impractical. When you cannot write one, an acceptable substitute is the lock statement itself plus a manual concurrent script and its recorded output. What is not acceptable is a sequential test named as if it were concurrent.

Also cover, when relevant:

• Authorization: which policy or gate changed, and who gains or loses access.
• Query cost: new N+1 risk or query count on the hot path. See Model and Custom Query Builder.

6. Migration, deploy and rollback

Answer definitively. Do not leave the options in place.

- Migration: Yes — add completed_by, completed_at to lots
- Deploy window: old code runs against the new schema for ~2 minutes; both columns are nullable, so it is safe
- In-flight jobs: queued ReceiveLot payloads keep the old shape; the handler accepts both
- Breaking API change: No
- Rollback: migration down is safe; no data has been generated yet

The deploy window and in-flight jobs are separate questions from the migration itself. A migration can be reversible and still break the minutes during which old code and new schema coexist. See Migration.

7. Evidence

Filled in properly, this is the fastest path to approval:

• Before and after screenshot or video.
• One real API request and response.
• The related ticket or specification.

Write N/A with a reason when an item genuinely does not apply — a backend-only PR has no screenshot. A field marked N/A - backend only reads as considered; an empty field reads as skipped, and empty fields drain trust from the fields you did fill in.

8. Checklist

Keep it short and honest. Unchecked boxes are information; boxes checked by reflex are not.

Blank template

The same content ships as .github/pull_request_template.md, so it appears automatically when a pull request is opened.

Risk: [High / Medium / Low]

<!-- Low risk: keep sections 1, 2, 7, 8 and delete the rest. -->

## 1. Purpose

## 2. Changes and tests

- [change] -> test: [test path]

## 3. Scope

Affected:

- [module / endpoint / job]

Not affected (with evidence):

- [area] — [why the reviewer can skip it]

## 4. Where to start reading

Start reading at:

## 5. Risks and verification

```text
Risk:
Why:
Reproduce:
Expected:
Evidence:
```

## 6. Migration, deploy and rollback

- Migration: Yes / No —
- Deploy window (old code, new schema):
- In-flight jobs / queued payloads:
- Breaking API change: Yes / No —
- Rollback plan:

## 7. Evidence

- Screenshot / video:
- API request / response:
- Ticket / issue:

## 8. Checklist

- [ ] Self-reviewed the diff
- [ ] Tested the happy path
- [ ] Tested validation and edge cases
- [ ] Added or updated tests
- [ ] Verified the migration up and down
- [ ] No leftover logs or debug code
- [ ] The PR contains only changes related to this request

Review questions

Before opening a pull request, ask:

1. Can the reviewer tell from the first line how much attention this needs?
2. Does every change in section 2 name a test, or an explicit reason it has none?
3. Would the reviewer regret trusting the "not affected" list?
4. Does every risk in section 5 have a reproduction and an expected invariant?
5. Is every risk you named covered by a test or by recorded evidence?
6. Can a reviewer confirm the result without rebuilding your environment?
7. Should this be two pull requests?