From e5138c86dfb05d4a5583a1dfc092de1a950b19d3 Mon Sep 17 00:00:00 2001 From: Jeremy McSpadden Date: Tue, 24 Mar 2026 14:33:51 -0500 Subject: [PATCH] docs(contributing): define execution-based review validation standard (#2364) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(contributing): define execution-based review validation standard Expand the Review process section to make explicit that reviewers are expected to build and run tests locally — not just read the diff. Also codifies what contributors must provide (regression tests for bug fixes, failure-path tests for features) to unblock review. Previously the section offered only logistics (PR size, response etiquette). This adds the missing standard for what "reviewed" actually means. * docs(contributing): add worktree checkout as explicit reviewer step 0 The prior commit defined build + test execution as the review standard but omitted the prerequisite: checking out the branch locally before reviewing. Without it the list implied reviewers could run commands without having the branch. Also adds the closing line that correctness claims require completing all five steps. --- CONTRIBUTING.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 46690bec6..20606ddd3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -158,6 +158,32 @@ PRs go through automated review first, then human review. To help us review effi - Respond to review comments. If you disagree, explain why — discussion is welcome. - If your PR has been open for a while without review, ping in Discord. We're a small team and things slip. +### What reviewers verify + +Reading a diff is not the same as verifying a change. Our review standard is execution-based, not static-analysis-based. + +**What reviewers do:** + +1. **Check out the branch** — check out the PR branch locally (or in a worktree). Don't review from the diff view alone. +2. **Build the branch** — run `npm run build`. A diff that doesn't compile is not reviewable. +3. **Run the test suite** — run `npm test`. CI status is a signal, not a substitute for local verification. +4. **Trace root cause for bug fixes** — confirm the diff addresses the root cause described in the issue, not just the symptom. +5. **Check for a regression test** — bug fixes must include a test that would have caught the original bug. If it's absent, the fix is incomplete. + +Only after completing these steps should a reviewer make claims about correctness. + +**What "looks right" means:** + +"Looks right" is the starting point for review, not the conclusion. "The tests pass" only means the tests pass — not that the claimed bug is fixed or the feature works as described. A well-written commit message on a broken change is still a broken change. + +### What contributors must provide to unblock review + +- **Bug fixes** — include a regression test. A fix without a test is an assertion, not a proof. +- **Features** — include tests covering the primary success path and at least one failure path. +- **Behavior changes** — update or replace any existing tests that cover the changed behavior. Don't leave passing-but-wrong tests in place. + +If your PR claims to fix issue #N, reviewers will verify the fix addresses the root cause described in #N — not just that CI is green. + ## Local development ```bash