review-standards

# Review Standards Severity classification and evidence bar. These rules exist so findings are classified correctly from the start, rather than filtered afterwards. ## Severity | Severity | Qualifies | |---|---| | **critical** | Logic errors causing runtime failures or incorrect generated SQL; clear violations of documented patterns (`.claude/rules/`, `.claude/skills/`); injection risks in generated SQL; type issues that will cause runtime errors | | **advisory** | Code genuinely hard to follow or maintain; a missed established codebase pattern. Keep only the strongest-evidence advisories | ## Evidence bar - **Critical = broken now, not "could break".** Cite the exact code path: input → broken call → observable failure. Can't replace "could fail" with "does fail" backed by specific lines → downgrade or drop. - **A crash claim resting on a data state needs a producer of that state.** "Throws when the list is empty / the value is NULL" is only critical with a cited call path, fixture, or test that produces that state. No producer found → the invariant probably holds outside the diff; ask as an advisory question, never post as critical. - **Read the PR description and linked issue before finalizing.** A scenario the description already rules out or declares intentional is refuted context, not a finding. - **A claim about what a platform accepts or rejects is runnable.** Add the case to the matching integration suite and run it, rather than asserting from memory. - **Repo grep can't prove a third-party claim.** A claim about a Doctrine DBAL or ORM method's existence or behaviour needs the installed package's source or the vendor doc. This repo supports DBAL 3 and 4 and ORM 2 and 3 at once, so check which majors the claim holds for. Unverifiable → don't post. - **A behaviour change applied consistently across the diff is presumed intentional.** When the diff itself systematically implements the "bug" (every platform node, every call site), post one confirm-intent finding per mechanism, not per-site criticals. - **Pre-existing defects get labelled and get a disposition.** A defect predating the change (check the old side of the hunk, or `git blame`) is at most an advisory opening with "Pre-existing:" that names which way it goes, fix now or a follow-up issue, never a critical against this change. - **Read ±20 lines around each hunk before claiming.** Diffs hide guards, error handlers, and fallbacks; many "missing null check" claims evaporate two lines outside the hunk. - **Advisory needs one of**: a failing scenario, a concrete code suggestion, or a reference to an existing codebase pattern. None of the three → drop. - **A proposed change ships as code.** Write the replacement into the comment; can't write it out → drop it. - **A suggested replacement passes the repo's own lints** (`composer phpcs`, `composer phpstan`, `composer psalm`). A suggestion tripping an enforced rule is incomplete: fix the suggestion, or name the suppression it needs. - **A fix with more than one viable shape names them all**, each with what it costs; don't pick for the author. - **The finding states what breaks and what to do.** Keep out the library internals you read to confirm it and any line about what you read or ran. - **An item left open ends with the question you want answered**, not with a statement of what you want from the author. - **Every referral names its target**: a code location by symbol or `file:line`, never "the earlier comment" or "the header". - **A convention advisory also needs a consequence.** Name what the deviation breaks or risks; "consistency" alone doesn't qualify. A finding that hedges itself ("purely a nit", "isn't broken today") is below the bar: drop it, don't reword the hedge away. - **Pattern-violation claims need verification.** Confirm the pattern exists (in `.claude/rules/`, a skill, or sibling code) before flagging a violation. - **Zero findings is a valid outcome.** Don't manufacture comments to look thorough.