Pull requests and review

A PR is one concern, explained in a four-section body, and its author merges it. This page answers how big a PR is, what its body says, who must approve, what the AI reviewer does, and when to ask a teammate. The required checks stay exactly ci-passed and pr-title.

One concern per PR, with no size gate

Impact: HIGH a squash commit is the unit of bisect, revert and release note

• If the description says “also”, split it. A PR that needs two types (feat and refactor) is two PRs (see git workflow).
• A rename or move is its own refactor PR, merged before the feature that needs it. Agents get this wrong most often: they rename what they touch. A dependency bump is its own build PR.
• A request to split is answered with a split, not a justification. No line count is measured anywhere. Work still awaiting verification is a draft.
• Accepted cost: a single-concern 2,000-line feature passes; whether it should be smaller is a judgement.

❌ Incorrect — three concerns in one squash commit:

feat(billing): invoice export
  - renames OrderService → Orders across 40 files
  - bumps effect
  - adds the export endpoint

✅ Correct — one concern each, the rename first:

refactor(orders): rename OrderService to Orders (#430)
build: bump effect (#431)
feat(billing): invoices can be exported as CSV (#432)

Source: notes/11-repo-operations/pull-requests-and-review.md · Decision 1

Write a four-section body; ! must carry upgrade steps

Impact: HIGH a breaking change’s release-note link must lead somewhere

• The body carries what the title and diff cannot: Why, What changes, Upgrade steps (only for !), and optional Review notes for where to start, what is risky and what was checked by hand.
• No “Test plan” or checklist. ci-passed already says the tests ran.
• A second step in the existing pr-title job fails a ! title unless ### Upgrade steps has content once template comments are stripped. Title and body are read through env, never interpolated into the script.
• The heading has two readers: the release notes copy ### Upgrade steps under the Breaking line. Renaming it changes the pr-title step and scripts/release-notes.ts in the same PR.

❌ Incorrect — a permanent breaking change with nothing for the operator:

feat(auth)!: API keys require an expiry

updated keys

✅ Correct — the step that checks presence, inside the required job:

- name: a breaking change carries upgrade steps
  env:
    TITLE: ${{ github.event.pull_request.title }}
    BODY: ${{ github.event.pull_request.body }}
  run: |
    re='^[a-z]+(\([a-z0-9/-]+\))?!: '
    [[ "$TITLE" =~ $re ]] || exit 0
    n=$(awk '/^### Upgrade steps/{f=1;next} /^#+ /{f=0} f' <<<"$BODY" | sed 's/<!--.*-->//g' | grep -c '[^[:space:]]' || true)
    [ "$n" -gt 0 ] || { echo "::error::A '!' PR needs a filled '### Upgrade steps' section"; exit 1; }

Source: notes/11-repo-operations/pull-requests-and-review.md · Decision 2, amended

Require no approval; give every PR a human owner

Impact: MEDIUM one person’s absence never stops merges

• The author merges their own PR when the etiquette below allows it. The ruleset states required_approving_review_count: 0 and require_code_owner_review: false explicitly, so nobody “fixes” it.
• Every agent PR has a human owner: the person who started the agent. The owner reads the whole diff before merging, not a bot’s summary.
• No CODEOWNERS. Who to ask is decided by triggers, not by a path-to-person map.
• Accepted cost: for an ordinary agent PR, the only human reader is the person who prompted it.

❌ Incorrect — a required approval with no bypass on a small team:

required_approving_review_count   1
CODEOWNERS  apps/server/src/billing/  @one-person   ← on holiday, billing freezes

✅ Correct — stated explicitly, so it stays that way:

ruleset on main → pull_request
  required_approving_review_count   0
  require_code_owner_review         false
no CODEOWNERS file

Source: notes/11-repo-operations/pull-requests-and-review.md · Decision 3

Run an advisory AI review on every ready PR

Impact: MEDIUM the only systematic second reader of an agent’s diff

• Automatic on every non-draft, same-repo PR. Comments only: it never approves and never requests changes, and it is never in ci-passed.
• A sticky summary names the SHA it reviewed and says “no findings” when clean. A missing or stale summary is how a broken reviewer gets noticed.
• .github/review-rules.md lists what the linter cannot see, and follows the decided notes. A judgement that becomes a lint rule leaves the file.
• The author verifies each finding against the source: fix real ones, answer false ones on the thread with a one-line reason. Do not merge with an unanswered finding.

❌ Incorrect — a bot that approves, or a nondeterministic check that gates merges:

steps:
  - run: gh pr review --approve        # reads as a review that did not happen
# ai-review listed under ci-passed     # an outage blocks every merge

✅ Correct — advisory, and loud when it is silent:

# .github/workflows/ai-review.yml (shape)
on: { pull_request: { types: [opened, reopened, synchronize, ready_for_review] } }
permissions: { contents: read, pull-requests: write }
jobs:
  ai-review:
    if: >-
      !github.event.pull_request.draft &&
      github.event.pull_request.head.repo.full_name == github.repository
    continue-on-error: true
    # inline comments + one sticky "AI review of <sha>: N findings"

Source: notes/11-repo-operations/pull-requests-and-review.md · Decision 4

Label the PRs that carry judgement calls

Impact: MEDIUM the risky file is not hidden as file 17 of 22

• Three path labels: migration (packages/db/migrations/**), contract (packages/contracts/**) and auth (identity/ and authz/). They mark where migrations, API evolution and authorization leave a judgement to review.
• Informational: never required, never failing, nothing keys a merge off a label. When a path moves, its glob moves in the same PR.
• No linked-issue requirement, no size label, no template-compliance bot.

❌ Incorrect — a rule only agents read:

AGENTS.md

- If you touch a migration, mention it in the PR.

✅ Correct — a deterministic label at the top of the PR:

.github/labeler.yml

migration:
  - changed-files: [{ any-glob-to-any-file: 'packages/db/migrations/**' }]
contract:
  - changed-files: [{ any-glob-to-any-file: 'packages/contracts/**' }]
auth:
  - changed-files: [{ any-glob-to-any-file: ['apps/server/src/identity/**', 'apps/server/src/authz/**'] }]

Source: notes/11-repo-operations/pull-requests-and-review.md · Decision 5

Self-merge, ask on risk, answer the same working day

Impact: MEDIUM “review” in other notes means an actual teammate

• Merge your own PR when the checks are green and every AI finding is fixed or answered.
• Ask a teammate first when the title has ! or the PR carries migration, contract or auth, and wait for their review.
• A requested review gets an answer the same working day: approve, request changes, or say when. “Monday morning” is an answer; silence is not.
• No auto-merge: it would merge before the advisory review is answered. While main is red, only the fix merges. Merge timing is free, because a merge deploys only to staging.

❌ Incorrect — a labelled PR merged on feel:

feat(billing): add invoice status   [migration]
  merged by author 2 minutes after green, no one asked

✅ Correct — the convention, written where people and agents read it:

## Review                    (AGENTS.md and CONTRIBUTING.md)
- Merge your own PR when the checks are green and every AI finding is fixed or answered.
- Ask a teammate to review first when the title has `!`, or the PR carries `migration`,
  `contract` or `auth`. Wait for their review before merging.
- A requested review gets an answer the same working day: approve, request changes, or say when.
- Whoever merges fixes the title first, never the squash dialog.

Source: notes/11-repo-operations/pull-requests-and-review.md · Decision 6

Deferred

• One required approval, stale approvals dismissed on push (never a bypass) — trigger: a production incident traces back to a PR that only its author read.