Adversarial review of a code change before it merges — the real bugs, most consequential first, each checked against the code before it is reported, with a fix that works. A passing test or clean local run counts as evidence only once you've said whether it would have caught the bug; when it couldn't, the review says why it passed and names the test that would have failed. Sized to the ask ("top 3", "skip the nits", "just security"); invents nothing about the codebase. Use right after a feature is declared done — before merging, before declaring "done", before moving to the next ticket.
Pack dev-teamTier heavyJob Build and review softwarev0.2.0
In Claude Code, with the dev-team plugin installed, invoke it as:
/dev-team:devils-advocate
Try it
A realistic prompt from the skill's acceptance evals:
Here's the payment-retry handler I just finished — it retries failed charges up to 3 times with exponential backoff, and the tests pass. Review it before I open the PR.
What a good result looks like
Every cited test or run is judged: could it have caught each bug, and if not, why did it pass and what test would fail.
Every finding survives a trace through the code and the stated rules. No false positives.
The answer to "can this merge?" comes first; findings are ordered by consequence.
Fixes work, and are code when the change is small.
The scope asked for is respected.
Nothing about the codebase is invented.
Example output
review of a rate limiter that "passes its tests" · the skill's own worked example, source
Input
"Implemented the per-user rate limiter, tests pass, ready for review."
# test_ratelimit.py
def test_burst_then_deny():
for _ in range(3):
assert allow("u1", rate=1, burst=3)
assert not allow("u1", rate=1, burst=3)
Output
Not ready to merge. Under concurrent requests for one user it admits more than the limit, and the test can't see that.
Race on the shared dict — ratelimit.py:6-10. Two requests for the same user both read tokens=1.0, both pass, both write back 0.0, so parallel bursts get through at up to twice the intended rate. Fix: hold a per-user lock around the read-modify-write:
_locks = collections.defaultdict(threading.Lock)
def allow(user_id, rate, burst):
with _locks[user_id]:
... # existing body unchanged
Memory grows without bound — ratelimit.py:2. Nothing ever leaves _buckets, so every distinct user_id stays for the life of the process. Fix: a cachetools.TTLCache in place of the dict, with a TTL longer than burst / rate.
What the test proves.test_burst_then_deny runs on one thread, so it passes with the race in place. It also never waits, so refill is untested. Tests that would fail today:
Ten threads calling allow("u1", rate=1, burst=3) at once; assert at most three return True.
After draining the bucket, allow is false at 0.5 s and true at 1.0 s (with a patched clock).
Why this passes the quality bar
The merge answer comes first. Each finding names the line, a concrete failure and a fix as code. The cited test is judged for what it could catch, and the missing tests are scenarios. The free first burst for a new user was left out: it is a product choice, not a bug in this change.
How it's checked
Measured against plain Claude Code (2026-10-23): it met 69% of its checks, against 63% without it, and a blind judge preferred the agent without it. How skills are checked
51 assertions across 7 acceptance evals, and 9 trigger queries. Every skill's checks
What the output must do
Every finding has a reproducible scenario a senior engineer can replay mentally — named input, current behavior, expected behavior
Every finding cites file:line, not just a section or function name
Severity is calibrated — blockers do not outnumber majors; inflated findings are demoted
Findings are grouped by the four lenses (edge cases, assumptions, staff-engineer pushback, test-coverage gaps), skipping lenses that do not apply rather than padding
Test-coverage gaps name the concrete missing test case, not 'add more tests'
Assertion theater is called out — tests that execute the code without asserting the behavior
Output ends with a 'what's solid' section so the author knows what not to touch
The review scopes to the change under review, never sweeping the whole codebase
It flags that sign_up compares the confirmed count with > instead of >=, so a shift with capacity N confirms N+1 volunteers
It flags that cancel picks the waitlisted row with ORDER BY created_at DESC, so the most recent person on the waitlist is promoted, not the first
It flags that the same volunteer can sign up twice for one shift because nothing checks for an existing sign-up
It flags that the 24-hour cutoff subtracts local-time datetime.now() from a start time stored in UTC, so the window is off by the server's UTC offset
It points out that test_waitlist_when_full puts three volunteers into a capacity-2 shift before asserting, so it passes because of the capacity bug
It points out that test_no_waitlist_raises passes even when ShiftFull is never raised
It does not report SQL injection, since every query is parameterised
It says plainly that the change is not ready to merge as it stands
It answers the question near the top: not as it stands, or only on a copy of the file
It flags that emails are compared exactly, so addresses differing only in letter case or surrounding spaces are kept as separate subscribers
It flags that split(",") breaks on a quoted field containing a comma, which shifts the column treated as the email
It flags that the script overwrites the input file in place with no backup, and recommends writing to a new file or keeping a copy
It notes that when rows share an email the last one wins, and that whether that is the right row to keep is something to confirm
It gives a concrete fix, such as reading with the csv module and normalising the email with strip and lower
It does not state as fact what the other columns hold or what the real file contains beyond what was given
It flags that monthsOverdue subtracts month numbers without the year, so an invoice due in November or December and still open in January gets a zero or negative count and no fee
It flags that the $75 cap is applied to one month's amount before multiplying by the months, so a large balance overdue for two or more months is charged more than $75 (for example $10,000 over two months gives $150)
It flags that applyLateFees adds the whole fee to the balance on every nightly run with nothing recording what was already charged, so the same invoice is charged again each night
It points out that the cap test uses a single month overdue, where a per-month cap and a total cap give the same $75, so it cannot catch the cap bug
It points out that the applyLateFees test only checks the balance went up after one run, and that no test runs it twice or checks the amount
It names a missing test for an invoice that is overdue across a year boundary
It does not report the grace-period check as a bug: charging nothing at 5 days late or fewer matches the stated rule
It says plainly that the change is not ready to merge as it stands
It flags that adding a NOT NULL column with no default fails on a table that already has rows, so the first statement will error on prod
It says the clean run on the laptop does not show the migration works on prod, for example because the local orders table may have been empty
It flags locking or long-running work on a large table, from the single full-table UPDATE or the non-concurrent index build, and gives a concrete mitigation such as batching the backfill or CREATE INDEX CONCURRENTLY
It flags that a customer with no region, or a region longer than 2 characters, would break the backfill, and says what customers.region holds is not known from what was given
It gives a corrected order of steps: add the column as nullable, backfill it, then add the NOT NULL constraint
It does not state the row count of orders, how long the migration will take, or what customers.region contains as fact
It leads with three main concerns as asked, and anything beyond the three is kept to a brief mention
It finds that quantity is only checked against the upper limit, so a negative quantity gives a negative cost that passes the balance check and adds points to the member's balance, and puts this among its top issues
It finds that the points are deducted and committed before fulfillment.ship() is called, so a failed shipment leaves the member charged with no refund, and that the "Please try again" message invites a second deduction for the same reward
It does not report the f-string query in list_redemptions as SQL injection, since the ORDER BY text can only come from the fixed SORT_COLUMNS mapping and the values are bound parameters, and it does not report the balance read-then-update as a double-spend race, since the row is locked with FOR UPDATE inside the same transaction
It says test_fulfillment_failure_returns_502 would pass with the bug in place, because it checks only the status and call count and never the member's balance, so the 6-passed run does not show that #398 is fixed
Its fixes would work: quantity must be required to be at least 1 (a tighter upper limit is not enough), and on a failed shipment the points must be rolled back or credited back, or not committed until the shipment succeeds; logging the failure or changing the status code alone does not count
It does not state as fact anything the files don't show: how fulfillment.ship() or the transaction helper behave beyond what the code and comments say, whether the fulfillment service rejects a negative quantity, how many members or redemptions there are, or what else calls redeem()
It leads with three issues, most serious first, and makes no style or naming comments; anything beyond the three is at most a brief mention
It finds that the goroutines append to the shared errs slice with no lock, a data race that can lose errors or corrupt the slice when more than one push fails
It does not flag the closure capturing the loop variable s as a bug, since on Go 1.22 and later each loop iteration gets its own s
It says the green test run and the staging run don't show the code is safe: with every push succeeding the append never runs, and a plain go test without -race wouldn't report the race anyway
Its fix would work: a mutex around the append, or a per-subscriber slot in a slice sized to len(subs); if it suggests errgroup it says that this changes the behaviour (only the first error comes back, and with a group context the rest of the sends are cancelled)
It does not state as fact what push() does inside (retries, timeouts, how it uses ctx), how many subscribers there are, or that this has already failed in production
It keeps to what's actually broken, as asked: a short reply with no style or naming suggestions, not padded out with a long list of minor or hypothetical concerns
When it should and shouldn't activate
Should activate:
Review this code I just wrote before I merge it
Anything I might have missed in this implementation?
Adversarially review this diff for edge cases and broken assumptions
I just finished the feature and the tests pass — is it ready to ship?
Find the bugs a staff engineer would push back on in review
Should leave to another skill:
Critique this design document
Convene multiple senior personas to deliberate this architecture decision
Implement this ticket from its acceptance criteria
Run the full gate and reviewers and tell me if this is safe to merge