PR Review and Decide — Runbook

A fast, repeatable loop for reviewing a PR and reaching a verified verdict (merge-ready, or needs-changes with specific findings). Scope stops at the verdict — fixing, merging, and QA labelling are separate actions.

Base branch for all PRs: docs/new-schema-design.

0. See the changes (Emacs)Copy Anchor Link

One command, full-frame diff:

C-c v r   →  enter PR number (or branch)

shan/review-pr fetches the PR branch and shows its range diff (origin/<base>...origin/<branch>) filling the frame. Navigation:

• n / p — next / previous hunk
• TAB — fold / unfold a file or hunk
• RET on a line — jump to that line in the real source file
• q — bury the diff, restore your previous layout

Terminal equivalents: gh pr view <N> (context), gh pr diff <N> (diff), gh pr diff <N> --name-only (file list).

1. Load context (30 seconds)Copy Anchor Link

gh pr view <N> --json title,body,author,baseRefName,headRefName,additions,deletions,changedFiles,labels

Read what the PR claims to do — you will verify it, not trust it. Note the author and the size. A large additions count on few files is a red flag (see the structure.sql gotcha below).

2. Run the repo-specific gotcha checksCopy Anchor Link

This is where real bugs hide. Run these before reading logic.

structure.sql corruption (recurring)Copy Anchor Link

If the diff touches backend/db/structure.sql:

• No migration in the PR → the structure.sql diff MUST be zero. Any diff is a dump from a polluted local DB, not intent.
• PR has a migration → the ONLY structure.sql change must be exactly that migration's objects (columns/tables/indexes it adds).

Authoritative check (don't trust raw +/- counts — reordering inflates them):

BASE=origin/docs/new-schema-design
git show $BASE:backend/db/structure.sql   | grep -oE "CREATE TABLE [a-z_]+\.[a-z_0-9]+" | sort -u > /tmp/base_t.txt
git show <pr-head>:backend/db/structure.sql | grep -oE "CREATE TABLE [a-z_]+\.[a-z_0-9]+" | sort -u > /tmp/pr_t.txt
comm -23 /tmp/base_t.txt /tmp/pr_t.txt   # tables DROPPED on merge — must be empty
comm -13 /tmp/base_t.txt /tmp/pr_t.txt   # foreign tables ADDED — must be empty

Fix (if it's your call to fix): no-migration PR → restore structure.sql from base; migration PR → restore from base then splice in only that migration's objects. See memory/project-no-migration-prs-corrupt-structure-sql.

"Claimed fixed" is not "landed"Copy Anchor Link

If the PR or author says "addressed the review", verify the fix is actually in the current head commit / the merged base — commits get orphaned (a fix can be committed to a branch that the squash-merge never included). Check the resulting code, not the claim.

Ratchets are the gate, local runs aren't always authoritativeCopy Anchor Link

• Frontend: lint / typecheck / test baselines in .ci-baselines/*. Verify with frontend/bin/ci (or the shell app: tsc -b for a single change — ignore the pre-existing cms/ baseline errors).
• Backend: the domains ratchet is cloud-calibrated; a local bin/ci isn't the final word. macOS needs OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES PARALLEL_WORKERS=1; RLS tests need rake db:rls_test_role:provision.

RLS divergenceCopy Anchor Link

"Works locally, fails on staging with not-found" on tenant data = RLS bypassed by the dev owner locally but enforced on staging. Establish the tenant GUC.

3. Verify the load-bearing claims against codeCopy Anchor Link

For each diagnosis the PR rests on, open the file and confirm — don't trust file:line refs (they drift). Where behavior is the claim, run the thing:

• bin/rails runner for undefined-method / class-load / return-value checks
• a single targeted test (bin/rails test path:line)
• tsc -b for a frontend type change

The bar: you can state the failure scenario (inputs → wrong output), not just "looks off".

4. VerdictCopy Anchor Link

• Merge-ready — state what you verified (tests run, claims confirmed, gotchas clear), so the verdict is auditable.
• Needs-changes — findings ranked most-severe first, each with: file:line, the concrete failure scenario, and the fix. Separate blockers from nits.

Leave the issue OPEN with the QA label — QA owns the close decision. Put concrete checkbox manual-QA steps on the PR or issue.

Emacs setup referenceCopy Anchor Link

Configured in ~/.emacs.d/lisp/init-vc.el:

• shan/magit-display-buffer — diffs/revisions/logs open full-frame (fixes the cramped side-popup from same-window-except-diff).
• shan/review-pr (C-c v r) — fetch + range-diff a PR by number or branch.
• magit-diff-refine-hunk 'all — word-level highlighting within hunks.
• shan/pr-base-branch — change if the base branch moves off docs/new-schema-design.

Forge is installed (C-x g → f to forge-pull); use it for browsing PR/issue metadata and comments. Use C-c v r for the code diff itself.