adversarial-review

An independent review pass with one job: find what the implementation got wrong. Automated verification proves the code does what the spec said. This proves the spec and the code were not both wrong in the same direction.

Mindset

• Zero trust. Passing tests are evidence the tests pass, nothing more.
• Assume the tests were written to pass. A test suite authored alongside the implementation tends to encode the same misunderstanding.
• Attack, do not admire. Do not summarise what the change does well.
• Report only what you can point at. Every finding names a file and line and a concrete input or sequence that breaks it. No "consider maybe".

Context boundary

Work from the change artifacts and the diff. Do not read the implementation author's reasoning, planning notes or self-reported test summaries before forming your own view - they anchor you to the same assumptions. Read them last, if at all.

.harness/run-events.jsonl is one of those: it holds what the author's runs already concluded. scripts/harness/run status is not - it carries an identity and no judgement, which is why the step below can use it without breaking this boundary. Neither is scripts/harness/verify --floor: it reads the diff, not the author's runs, and it answers the same way whoever asks - dimension 10 starts from it.

Run this in a fresh session, and prefer a different model from the one that wrote the code.

1. Map the change

Before attacking anything, list every file the change touches. That list is the checklist for the whole pass. Identify an entry by (path, status), not by path alone - a path appears twice when a deletion is followed by a recreation.

Every entry ends in one of two states: reviewed, or skipped with a concrete reason - "generated file, no hand-written logic", "lockfile, the diff is machine output". "Looked minor" is not a reason, and neither is running out of attention.

Reviewing one file does not cover its counterpart. An implementation file does not cover its header, its interface, its test or its configuration, and the smaller member of a pair is exactly where a forgotten update hides.

The counts go in the output. A review that silently covered two thirds of the diff reads exactly like one that covered all of it, and the verdict it ends on is worth nothing - which is what this checklist exists to make visible.

2. Plan the attack

Go through the mapped files once and write down, for each risk point:

• the severity you expect - blocking, should fix, nit;
• what is at stake, concretely: where it is, what breaks, who notices;
• what you have to read to confirm it or kill it, named - this function's callers, that migration, the test that claims to cover it.

Then go and confirm them. The plan holds hypotheses, not findings: an entry you could not confirm is dropped, and saying that costs nothing. Planning first is also what keeps the pass from spending its whole attention on the first file and arriving at the last one with none left.

If the change carries no identifiable risk, write that instead of padding the list.

3. What to attack

1. Incorrect assumptions. Every place the code assumes a value exists, is non-empty, is unique, is ordered, is in a given state, or arrives once.
2. State preconditions. Operations allowed on entities whose state should forbid them - editing something still being generated, mutating an archived record, acting on a soft-deleted row.
3. Authorisation. Can one user read, mutate or enumerate another user's data? Check every new read path, not just the writes.
4. Input handling. Empty, whitespace-only, maximum-length, wrong-type, wrong-encoding, unicode edge cases, injection payloads in any field that reaches a query, a shell, a template or a log.
5. Concurrency and idempotency. Two callers at once; a retried request; a partial failure halfway through a multi-step write.
6. Error paths. What the caller actually sees when it fails. An unhandled exception surfacing as a blank screen is a defect, not an edge case.
7. Spec drift. Requirements in the change artifacts with no implementation, and implementation with no requirement behind it.
8. Consistency. Response shapes, naming and error formats that diverge from the documented conventions in AGENTS.md and the standards it indexes.
9. Architectural conformance. Check the code against docs/harness/architecture-rules.md and the reference it points at in .agents/skills/architecture-guidelines/. What to look for: a dependency pointing outward instead of toward the domain; business rules in a component, a controller or an adapter; a loose primitive where a typed value belongs; an external payload deserialised straight into a domain type without a DTO and an explicit mapper; an error from a transport or an SDK leaking upward instead of being translated; a screen state expressed as combinable booleans; the same datum held as truth in two places. Report only what the project actually adopted - the adoption map says which rules apply here, and a rule listed as not applicable is not a finding.

10. A weakened bar. Faced with a red check, an agent takes the cheapest road to green, and every one of these roads ends with the check passing. Read the removed lines as carefully as the added ones - that is where all of them show. Start from scripts/harness/verify --floor, which lists what a pattern can find - markers, suppressions, deleted test files, assertions removed, the gate's config touched - without running a test. It is the starting list, not the boundary: a loosened expected value or a lowered threshold has no pattern, and those are yours to find.

    • A test got easier. A skip or focus marker added, a test file deleted, an assertion removed or loosened in a test that stayed, an expected value rewritten to match what the code now returns.
    • A checker got silenced. A new suppression comment - @ts-ignore, eslint-disable, # noqa, # type: ignore, // ignore:, #[allow], @SuppressWarnings - or one that drops code from coverage instead of testing it.
    • A threshold moved. A coverage or performance budget lowered, a lint rule turned off or down a severity, a step removed from harness_gates in scripts/harness/config.sh, a path taken out of HARNESS_CODE_PATHS.
    • Work is unfinished. A stub that throws "not implemented", an empty catch turning a failure into silence, a TODO standing where the implementation should be.

    None of these is wrong by itself: a test can legitimately change when the specification did. What makes it a finding is the missing justification. Each one is blocking unless the change's own report names it and quotes the requirement that backs it - the same bar run-task sets for a check it calls wrong. Tightening the bar needs no justification; loosening it always does.

11. Tests that prove nothing new. A new test that re-proves what an existing one already asserts, the same rule tested again at a slower layer, a table of near-identical cases, or anything "Which tests to write" in .agents/skills/architecture-guidelines/SKILL.md says not to test. Name the existing test it duplicates, or the rule it breaks. It is should fix: it does not make the change wrong, it makes every later task slower.
12. Tests that cannot fail for their reason. A test that passes whether or not the behaviour it names works proves nothing, and it reads exactly like one that does. Look for: an expected value computed by the helper under test, or copied from production; a mock or a fixture that does what production should - the acknowledgement, the ordering, the stored row; a negative case rejected by a different guard than the one it names, or never reaching that branch; an assertion on a declaration - an import, a flag, a call shape - where the behaviour it promises goes unchecked; a count asserted without the value. When a test is the only proof of a fix or a refusal in the change, break the code the way the bug would and run it: green is blocking. Otherwise it is should fix.
13. Decisions nobody made. Wherever the request and the change artifacts left a gap, the code filled it - a data shape, a storage location, a dependency, an error contract, a default, a timeout, a scope read one way rather than the other - and the diff does not flag any of them. Reconstruct them from the diff and the artifacts, not from the author's notes, which name the decisions the author noticed and not the others. Give each one a verdict - sound, unsound, or the human's (taste, product direction, an external cost) - and a confidence that the human would have made the same call. "It works" is not a verdict on a decision: a buffer resized, a timeout bumped or a special case whose size happens to cover the failing input leaves the cause where it was. Ask what property makes it hold in general; if the answer is "this case passes", it is unsound. An unsound decision is a finding at the severity of what it breaks, naming the decision to redo it from rather than a patch on top. Sound and the human's are not findings; they go in the report's decisions list. An empty list on nontrivial work is not a pass - ask what the request left unspecified, because something filled it.

Cross-file findings are part of the job: an inconsistency between two files in the change, an update applied to one caller and not the other, a contract broken on one side. Read whatever context you need, but file every finding against a file that the change actually touches.

For a dedicated security pass following OWASP Top 10 and NIST, use the code-auditing skill instead - this skill covers correctness broadly.

4. Fact-check your own findings

A finding is not evidence because you produced it. Go through the list once more with the opposite job: not "is this worth reporting" but "does the diff prove this wrong".

The two mistakes available here are not equally expensive. Reporting a wrong finding costs somebody a few minutes. Dropping a correct one destroys it in silence - it reaches nobody, and nobody learns it was dropped. So when the evidence falls short of proof, it stays. "Suspicious", "I cannot verify this", "low value" and "the code looks fine to me now" all mean it stays.

Drop a finding on exactly two grounds:

• A - it describes code that is not there. The symbol, statement or construct it names appears nowhere in the file it was filed against. The same construct in a sibling file does not rescue it.
• B - a diff line contradicts it literally. It calls an identifier unused and the diff uses it; it says a check is missing and the diff contains it; it says a value is hardcoded and the diff reads it from a variable. The contradiction has to be readable straight off the diff - needing a chain of reasoning to reach it means there is none.

Never drop a finding whose subject is memory safety, concurrency, a declaration that disagrees with its definition, a behavioural or compatibility change, or a parameter the function accepts and never uses - whatever you concluded about it. Those are the categories where a wrongly dropped finding is most expensive and your own confidence is worth least, including your confidence that the language or the runtime does not behave the way the finding claims.

None of these is a ground: you disagree with the fix it proposes; you consider the flagged code acceptable as written; you cannot confirm it; it quotes the wrong line while describing something the diff does contain. Judge the claim, not the citation.

This step shares an author with the findings it judges, which makes it weaker than it looks - it still removes what the diff flatly refutes. When the pass runs in a fresh session with a different model, as the context boundary asks, so does this step, and then it is worth considerably more.

5. Rounds

A review that ends not safe to commit sends the change back to be fixed, and then it is reviewed again. Each pass is a round, and the rounds are not alike.

Round 1 reviews the whole change. Record in the report what it reviewed: git rev-parse HEAD, and when the tree had uncommitted changes, the hash git stash create prints - it records them as a commit without touching the tree or the stash list.

Every later round reviews only what changed since the last one: git diff against the commit the previous report recorded, plus any file that did not exist then. The rest was reviewed already. Read the previous report first - it is a review, not the author's reasoning, so the context boundary does not apply to it - and close every one of its findings as resolved, still open or dismissed with a reason. Do not raise again what it already raised: a finding the fix did not resolve is still open, not new, and one dismissed with a reason stays dismissed unless the diff contradicts the reason.

Name the class, not only the instance. When a finding is one case of a pattern - a missing bound, an unchecked error, the same assumption in three callers - say the pattern and list every instance in the change, so one round fixes all of them instead of one round each.

Three rounds is the cap. If the third still ends with something blocking, stop reviewing: the verdict is not safe to commit, and the report says the approach is what needs reconsidering - a fourth round on the same approach is where a change goes to spend fifty passes fixing what the last fix broke. That goes to the human.

Output

Open with the coverage line: the round, files in the change, files reviewed, files skipped, and the reason for each skip. From round 2, the coverage is the files the round's diff touched.

Then group findings by severity: Blocking, Should fix, Nit - and Pre-existing, apart from the rest.

A finding is pre-existing when the defect was there before this change and the change neither wrote it, nor modified it, nor made it reachable in a new way. It is reported - somebody should know - but it never blocks, it is never counted as blocking or unresolved, and it is not the change's to fix. If the change makes it worse, newly reachable or newly exposed, it is the change's, at its real severity.

Each finding:

• file:line
• what breaks;
• the exact input, state or sequence that triggers it;
• why the existing tests miss it.

Then the decisions from dimension 13, grouped by verdict - the human's, unsound, sound - and least confident first within each group. Each one says what gap forced it, what it does today and the alternative it ruled out, and what later work it constrains: the reader did not live the session and must be able to judge it without the diff. Trivial ones - internal naming, cosmetics - compress to a count.

Close with a single explicit verdict: safe to commit or not safe to commit. If you found nothing blocking, say so plainly - do not invent findings to look thorough.

Where the report lives

Write it where this change carries its reports, next to the verification report of the step before, dated: <date>-adversarial-review.md. The project's docs/harness/mandatory-steps.md names that folder. A project that keeps no such folder puts it in the pull request body instead - what is not allowed is leaving it only in the session. Say the same thing in the session too; the file is not a summary of it, it is it.

This is not bookkeeping. The session that applies these fixes is often another session, on another runtime, and a report that only exists in your terminal is a report it cannot read. Measured on 2026-09-17: a review recorded 13 should-fix and 9 nit findings in the run's events, every finding pointed at a file and a line, and none of them existed anywhere a later session could open. The counts survived; the review did not.

It also outlives the task. The verification report says what was checked; this one says what was checked against the code, and the human reading the pull request weeks later has no other way to learn what was considered and dismissed.

Then record the same thing where a machine can read it, in addition to the report above and never instead of it:

scripts/harness/run event review review=adversarial round=<n> \
  files_in_change=<n> files_reviewed=<n> files_skipped=<n> \
  blocking=<n> should_fix=<n> nit=<n> pre_existing=<n> \
  verdict=<safe_to_commit|not_safe_to_commit>

Counts and a verdict, not a score: the findings themselves stay in the report file, where the file, the line and the triggering input live. A review collapsed into a number is a review nobody can act on. Without a run context the command writes nothing and exits 0, so it is safe to run anywhere. Run it again after the fixes, adding unresolved=<n>, because the review the human reads is the one that ran last.

Once the change has a pull request, its verdict also goes on the commit: scripts/harness/land --review <pr> blocking=<n> unresolved=<n>, the same counts, from a checkout at the pull request's head. It posts the commit status norma/review - success only with nothing blocking and nothing unresolved - and a project that sets HARNESS_LAND_REVIEW=required lands no head without a passing one. The event and the report say what the review found; the status says which commit it found it in, which is the only thing a merge can check.

The coverage contract of stage 1 and the two grounds of stage 4 are adapted from alibaba/open-code-review (Apache-2.0), which runs them as separate passes around its review agent. Dimensions 12 and 13 adapt audit-tests and audit-decisions from dzhng/skills (MIT).