---
name: pr-review
description: Review a community pull request against Print Farm Manager's specific failure modes: adjacent-code drift, part-count integrity, driver contract compliance, docs completeness. Use for any GitHub PR from an outside contributor. Takes a PR number as argument.
---

# Review a Community PR

The repo is public and gets real PRs from strangers and regulars. Review them against this repo's rules, not generic lint taste. The standing lesson: the worst bug a community PR ever nearly shipped was NOT in the diff. It was adjacent code the diff made stale (a hardcoded restore column list that silently dropped `serial_number`, `loaded_material`, `loaded_color` after export gained them). Review the neighborhood, not just the diff.

## Phase 1: Context

1. `gh pr view <number>` and `gh pr diff <number>`. Read the description, linked issues, and any prior review threads (`gh pr view <number> --comments`). Do not repeat findings another reviewer already raised; build on them.
2. `gh pr checkout <number>` so you can run and grep the real tree.
3. Identify what kind of PR it is: driver/connector, server logic, client UI, docs, CI, dependency bump. That selects the deep-dive lists below.
4. Check CI state (`gh pr checks <number>`). The test job gates everything; a red suite is an automatic changes-requested.

## Phase 2: Baseline checks (every PR)

- Conventions: synchronous better-sqlite3 (no `await` on DB calls), epoch-ms timestamps, INTEGER booleans, route factory pattern, COALESCE partial updates, static routes before `/:id`, error shape `{ error: '...' }` with correct 400/404/409.
- Windows safety: `path.join`, no bash-only assumptions, `split(/[\\/]/)` when parsing stored filepaths.
- Schema: any migration must be additive (`try/catch ALTER TABLE` in db.js). Anything destructive or framework-shaped is changes-requested.
- Tests: `npm test` locally on the checked-out branch. Then read the new tests and ask: do they seed and assert the NEW tables/fields/behavior, or do they only re-assert what already passed? A test that cannot fail is a finding.
- Docs: component doc updated, dated CHANGELOG entry present in house format. Missing docs is a real finding, not a nitpick (CONTRIBUTING.md requires them).
- New prose dash check on added lines only:
  `gh pr diff <number> | grep -Pn '^\+.*[\x{2013}\x{2014}]'`
- Security (repo is public, app is LAN-only by design): no secrets or real access codes in code, tests, or fixtures; file endpoints validate paths (no traversal); nothing that adds cloud calls or telemetry.
- Dependencies: any new dependency needs strong justification; native modules must build on Windows under Node 22/23.

## Phase 3: Adjacent-code audit

For every file the diff touches, open the WHOLE file and its counterpart in the sync-pairs table (CLAUDE.md "Sync pairs"). Ask: does this change make any unchanged code stale?

Specific known traps:
- server/routes/backup.js: export and restore must handle the same tables and columns. Column lists must derive from the live schema, never hardcoded.
- scheduler.js eligibility SQL vs `routes/parts.js` dispatch-status: the diagnostic must mirror dispatch reality.
- Driver registration: a new brand must hit all six touchpoints (driver file, drivers/index.js, models.js VALID_CONNECTORS, printers.js NO_API_KEY_TYPES if keyless, Settings.jsx brand spots, db.js migration if new fields).
- Duplicated derived-status logic ("awaiting sign-off") across Dashboard/Fleet/Printers: a change to one copy needs all three.

## Phase 4: Deep dives by PR type

**Anything touching jobs, parts, scheduler, set-ready, or backup restore:**
- Trace every path that writes `parts.completed_qty`. For each: what unique real-world event backs it? Can it fire twice across a server restart, a Bambu MQTT reconnect, or an OFFLINE-to-FINISHED flap? Recovery paths must gate on process lifetime (`finished_at > scheduler.startedAt`), never wall-clock windows. Any doubt here is a P1.
- Check hold semantics: nothing may clear `is_held` except operator endpoints and the documented recovered-to-PRINTING path.

**Driver PRs, additionally:**
- Full contract check against docs/driver-authoring.md: four functions, getStatus never throws (OFFLINE on error), canonical statuses only, cancel maps to STOPPED not ERROR, `UPLOAD_CONFLICT` code, no DB access in the driver, module-level Map for persistent state.
- Payloads verified against the official protocol docs (fetch them; do not trust the PR's claims about field formats).
- Hardware evidence: the PR must state what real hardware it was tested on. Mock-only drivers can merge but must be labeled community-maintained/unvalidated in docs, per driver-authoring.md.

**Client PRs, additionally:**
- useToast/useConfirm rules (elements rendered, no window.confirm/alert), background-poll errors swallowed vs mutation errors toasted, refetch-after-mutate, palette hexes copied not invented, 600 px breakpoint respected, `npm run build` passes.

**Dependency-bump PRs (including Dependabot):**
- Read the changelog of the bumped package for breaking changes in the used API surface. Confirm lockfile consistency and that `npm ci && npm test` passes. Never recommend enabling auto-merge.

## Phase 5: Verdict and delivery

1. Rank findings by severity, house style: **[P1]** correctness/data-loss/security (blocks merge), **[P2]** should fix before or shortly after merge, then minor notes. For each finding: file, line, what breaks, and a concrete failure scenario ("restore a backup taken after this PR and loaded_color comes back NULL").
2. Verify each P1 by actually exercising the code where feasible (run the test, hit the route with curl against a dev server, or write a quick throwaway test) rather than reasoning from the diff alone.
3. Deliver as Joel directs: default is a summary to him with a recommended verdict (approve / approve-with-nits / request-changes). Only post to GitHub (`gh pr review`) when he asks.
4. Merge mechanics to remember:
   - docs/CHANGELOG.md prepend conflicts: `.gitattributes` has `merge=union` for it, but GitHub's server-side merge IGNORES that. Resolve locally and push back to the PR branch (regulars leave "allow maintainer edits" on).
   - Squash-merge titles should follow the `type(scope): summary` convention.
5. If the PR is good work, say so specifically. Community goodwill is an asset; regulars like seanlw follow the conventions and deserve fast, substantive reviews.