P1
Will show up in production for users or callers if merged — crashes, wrong results, security/data harm, public contract breaks
Product
Review checklist
Every PR gets the same six categories. Pacecar does not invent a new rubric per author or per agent. Optional repo review.md items fold under these headings — they do not replace them.
No nits. Taste is the product: catch what will show up in production. Style, test pedagogy, and process chatter stay silent.
Severity
How loud we get
P2
Credible production risk; non-blocking watch item
Approval is applied from severity by rules: any P1 → request changes; only P2s → comment; nothing to raise → approve.
Categories
What we check
01
Security
Access control, attack surface, secrets, and data privacy — OWASP-aligned themes, not a full audit.
- Login and permission checks on new or changed endpoints, jobs, and admin paths
- Deny by default; least-privilege roles; no trust of client-only identity or tenant data
- Object-level authorization (no IDOR); tenant isolation on read and write
- No secrets in code, logs, errors, or client responses
- Safe handling of input: injection, path traversal, SSRF, unsafe deserialization, shell
- Webhook signature verification; maintained crypto; no weakened security defaults
02
Intent
The change matches what the PR claims — especially important for agent-written PRs.
- Diff matches the PR title and body; stated goals are met in code
- Respect out-of-scope and non-goal notes
- Missing parts of a “complete” claim are raised, not ignored
- Treat the description as a claim to verify — not as proof
03
Scope
The change stays inside the claimed area and fits the repo’s existing patterns.
- Changes stay in the paths and systems the PR describes
- Reuse existing helpers and abstractions; no parallel approaches without need
- No drive-by refactors or format-only noise mixed into behavior changes
- Dependency and config changes justified by the PR purpose
04
Logic
The code works — success path, errors, edges, and calculation accuracy.
- Main success path matches intended behavior and nearby rules
- Edge cases: empty input, limits, duplicates, overflow, end of data
- Error paths correct; concurrency, idempotency, pagination when relevant
- Money and dates: safe decimal types, time zones, inclusive/exclusive ranges
05
Reliability
Safe in production — timeouts, migrations, and tests that cover the real risk.
- Sound timeouts, retries, and cancellation; bounded resource use
- Migrations treated as production changes; flag locking DDL and unsafe NOT NULL / rewrites
- Prefer expand → migrate → contract for breaking schema changes
- Tests cover the main risk of the change; no deleting tests to hide breakage
06
Contracts
Shared APIs, schemas, and callers stay compatible — or ship a migration plan.
- Public and cross-package APIs stay compatible or versioned
- Schema and event changes are backward compatible or paired with a safe migration
- Callers in-repo updated when signatures or meaning change
- Shared config, env vars, and flags not removed without a path
Same bar on every open and update.
Install once. Reviews run on draft and ready PRs — without training models on your private code.