Lint is green. The bot left forty comments about naming. Somebody typed LGTM. Then billing double-charged, or the migration locked the table, or the "internal" route was sitting on the public router.
If you have been reviewing AI-authored diffs for more than a month, you already know this shape. The model is fluent. The linter is happy. The PR description is a file list. The thing that will page you is not in the comments.
I stopped opening those PRs. Not because I am anti-agent — I use Cursor and Claude Code every day. I stopped because a review that cannot name a rollback is a vibe check with extra tokens.
This is the rubric I run before a PR exists. Eight blockers. If it is not one of these, stay quiet. Review bots that cry wolf get muted in a week, by me first.
A blocker is something that can page someone, corrupt data, leak access, or ship a change nobody can undo without a forward-fix.
Naming, file length, "consider extracting a helper," and architecture taste are not blockers. Those conversations can happen after the change is safe, or never.
Every finding has to cite path:line
. If you cannot cite it, it is not a finding. "This feels risky" is not a finding.
Scoring is three buckets:
Pass means zero blockers. WarnBlocker when the change is user-visible or writes data, and there is no way to turn it off or reverse it without a forward-fix. Missing feature flag on a risky path. Irreversible migration. Deleted endpoint with no sunset. Old code deleted in the same PR that turns the new path on for everyone.
The tax-calculator swap is the one I keep seeing. Billing now always calls newTaxEngine.calculate(order)
. legacyTax
is gone. No flag. CI is green because the new function has unit tests.
You cannot "just revert" after that ships. The function is gone, you do not know which orders are wrong, and you are doing a forward-fix under money pressure. Gate it, or keep the legacy path behind something you can flip at 2am without a deploy.
Blocker when request/response shape, status codes, error codes, event payloads, or CLI flags change in a way current clients will mis-handle. Renames. Type narrowing. Removed fields. Newly-required fields. Same field name, new meaning.
This is the silent one. GET /v1/invoices/:id
used to return amount
as integer cents. The model "improves readability" and returns (invoice.cents / 100).toFixed(2)
. Field name stays amount
. The review comment is "nice, more readable for clients."
Existing mobile builds will treat "12.50" as 12 cents, or throw, and you cannot force-update them this afternoon. Add amount_decimal
, or ship /v2
, and keep amount
as integer cents until the old clBlocker when the schema or the meaning of stored data changes and existing rows will be wrong, null, or unreadable. New NOT NULL
with no default and no UPDATE
. Enum value reused. Timezone or units changed silently. Application starts writing the new field on new rows only and pretends history does not exist.
The smallest version is:
- status TEXT
+ status TEXT NOT NULL
No default. No backfill. The migration fails on apply, or the check is skipped and reads throw later. Backfill first. Set NOT NULL
in a follow-up. Or pick a default that matches current product meaning and say so.
Pass when expand / migrate / contract is in the PR, or a sequenced follow-up is linked and this PR is only the safe expand. A TODO with an owner is a warning, not a pass.
Blocker when a new or changed path reads or writes anything sensitive and the diff does not show the same authz check the rest of the resource uses. IDOR by taking an id from the client. "Internal" endpoint newly reachable from the public router. Trusting a client-supplied role.
Models love a new DELETE /accounts/:id
that looks like GET
and PATCH
but does not call the guard. The test hits /accounts/1
with no session and asserts the status is not 500. That test will pass if the handler no-ops, deletes the wrong row, or deletes anyone's row.
Reuse the existing guard. Deny cross-account ids. A comment that says "add auth later" is Blocker when the risky behavior has no test, or the new tests assert mocks and implementation details instead of the behavior: status, persisted data, denied access, rollback. Snapshots of HTML with no behavioral assert. "Should not throw" as the only test for a migration. expect(status).not.toBe(500)
as the only test for a delete.
The rule I make the model say out loud: name the test that would fail if this blocker were present. If you cannot name it, it is a blocker.
For that delete endpoint the tests that would actually fail are: 401/403 with no session, 403 on someone else's id, and the target row is gone. Anything short of that is coverage theater.
Pass when at least one test would fail if the production hole in this PR were present. Warning: happy-path only, and the change is not security or data. Still mergeable. Still worth a sentence.
Blocker when a partial rollout is required — the change is risky or large — and the new code path is not gated, or the flag is checked in one layer but not the others.
The classic miss is the HTTP handler checking flags.newIndexer
while the SQS worker that writes the same index always calls indexV2
. You flip the flag off. The API goes back. The queue keeps mutating v2. You have not rolled back. You have forked reality.
Wrap every entry point, or split the queue. An ungated change that is small and revertible can pass. A flag name that does not match the rest of the codebase is a warning. Still wrap it.Blocker when timeouts, retries, or error handling can duplicate side effects — double charge, double email, double write — or swallow the error and report success. Missing idempotency key on a payment, webhook, or handler. Catch-all that returns 200.
This is the one that pages billing. The code looks careful: a try/catch
that logs and returns 200 so "the webhook does not retry forever." Now the provider thinks you processed a payment you did not, or you process it twice because the first attempt timed out after the charge succeeded.
Pass when side effects are idempotent or uniquely constrained, and failures surface. Info-level logs for a user-facing failure are a warning. They are not a rollback plan.
Blocker when secrets, tokens, or raw PII are added to logs, client bundles, URLs, git, or error messages. Verbose debug enabled on a production path. A request logger that dumps the whole body.I do not wait for GitHub to tell me. The agent is forbidden from opening a PR, creating a review commit, or telling me a change is ready until it has run this rubric on the full diff against the base branch, plus a secret scan, plus the test-gap question. If there is any blocker, it fixes and re-runs. Only then does it write a PR body a human would read: why, rollback, risk, proof. Not a file list.
CI gets the same prompt. Start with comments. A red X on every AI PR is how a gate gets deleted in a week. Do not fail the build on coverage percent. Do not fork the rubric per person. If someone bypasses a blocker, they write the why in the PR body. That is the process working, not the process failing.
Run it on your last five merged PRs before you argue with the list. If it catches nothing you would have wanted caught, the rubric is wrong for your shop — write down the miss. If it is silent and you are not, that is a bug in the eight, not a training problem.
The eight are deliberately short. I have watched teams paste fifty-three rules into a .cursorrules
file and then ignore the file because it nags about import order with the same urgency as an IDOR. Urgency has to be scarce or it is not urgency.
I packaged the gate so I am not pasting the prompt into a new repo every Monday. Local skills for Cursor and Claude Code, an AGENTS.md
snippet that refuses to open the PR until pass, the same review in a GitHub Action, and five annotated diffs so a team copies the shape instead of the adjectives. A zip, not a hosted bot with your API key. I already pay for the model.
Shipcheck is $39 Solo / $69 Team. Thirty-day refund if it does not catch anything you would have wanted caught on those last five PRs: https://nguyenverse517.gumroad.com/l/wlguz
Pass when redaction matches existing production practice. .env.example
placeholders that are clearly fake are silent. Spend the comment on the access token in the query string, not on POSTGRES_PASSWORD=changeme
.
This is a different question from blocker 1. One asks whether you can undo. The other asks whether the undo covers every process that will keep writing after you flip the switch.
not a warning. Report it as a blocker. Comments are not access control.
Pass when the new path reuses the existing guard, or is explicitly public and that fact is called out in the PR body, not in a TODO.
ients are gone.
Pass when the change is additive and optional, or versioned, or every known client ships in the same PR. Warning: undocumented but backward compatible. Still write the sentence. Do not block on it.
Pass when a flag exists, or the change is trivially revertible (pure code, no data, no contract), or the PR names the rollback and who can execute it. Warning: the flag exists but defaults on in production. That is a flag with a lying name.
ings may remain. You can merge with warnings. You cannot merge with a blocker unless you own it in writing.
If the run is a pass, do not invent work. "Looks good, here are nine suggestions" is a failed run. Silence is the feature.