diff --git a/.claude/shared/engineering-rules.md b/.claude/shared/engineering-rules.md index 4dd01984..2fca32f0 100644 --- a/.claude/shared/engineering-rules.md +++ b/.claude/shared/engineering-rules.md @@ -110,7 +110,7 @@ The two rules that hold everything else together: **Verify claims against code, never against the description.** Excellent descriptions are still wrong in places. Half-wrong claims: identify the right half. **Verify the feedback itself before implementing it** — reviewers are claims too; incorrect issue premises have been implemented verbatim when thirty seconds of grep would have caught them. -**Review against current `main`, on the head SHA you think you're reviewing.** Branches go stale in a queue; `gh pr diff` can serve stale content. **When CI is red, first ask what moved** — a runner with a floating Node version once turned main red with no commit behind it, and every open PR looked guilty. +**Review against current `main`, on the head SHA you think you're reviewing — and assert it mechanically, never from memory.** Branches go stale in a queue; `gh pr diff` can serve stale content. A review batch once ran against `refs/pull/*` refs cached earlier in the same session while quoting the current head, so four of six reviews re-derived findings the author had already fixed that morning. Nothing about that looks wrong from the inside: a stale review is internally consistent and every finding in it is true of *something*. It was caught by the author, from a suite count in the review body that did not match their branch. So the check is a comparison the command runs (`git rev-parse` against `headRefOid`, re-fetched first), not a habit — and the review body states the head SHA and the counts it observed, which is the only tripwire a stale review emits. **When CI is red, first ask what moved** — a runner with a floating Node version once turned main red with no commit behind it, and every open PR looked guilty. **A PR description is testimony, not evidence** — the review's job is to find where the narrative outran the code. **Recompute the math:** pinned constants, statistical formulas, thresholds — rederive independently before trusting them. Probe the standard edge classes and the fail-open/fail-closed direction of every guard; audit error responses for leaks; check read bounds on write paths. **Separate CI signal:** say which failures are code and which are infra/permissions. **Name what the PR got right** — a request-changes that ends "one round of targeted fixes, not a rethink" gets a better resolution round than a bare defect list; every finding includes the shape of the fix ("one line: strip the flag when block is true"). **For payment paths, auth, or anything moving money: adversarially verify findings** — a second independent pass attempting to refute each — before posting; plausible-but-wrong findings burn author trust and review rounds. @@ -131,7 +131,7 @@ The review round is where contributor quality is actually measured. In order: 1. **Reproduce before touching anything.** Run the reviewer's scenario and confirm it. If you can't reproduce, say so plainly — and still fix on the evidence if the mechanism is sound. Never implement a fix for a finding you haven't understood: a plausible fix for a misdiagnosed problem is two bugs. 2. **Fix, then audit your own fix** the way the reviewer reviewed the original. If the audit finds a bug in the fix, disclose it unprompted. A fix that silently changes scope is a new unreviewed change. 3. **Report what the fix taught** — not "fixed"/"done": what you reproduced, what changed, what it revealed about adjacent code. If your earlier framing made the bug look smaller than it was, correct the record. -4. **Push back only with a measurement** — an on-chain read, a header trace, a control run — not intuition. +4. **Push back only with a measurement** — an on-chain read, a header trace, a control run — not intuition. And when the pushback is *"that is already fixed"*, treat it as a staleness signal before treating it as a disagreement: check `git merge-base --is-ancestor ` first. If the review read an ancestor, the correction belongs to the reviewer, with the measurement, before anything else is discussed — an author should not have to prove the same fix twice. 5. **Defer ownership calls.** If a finding opens a design question that belongs to someone else, present options with trade-offs and ask; time-box it ("say what shape you want and I'll push it today"). Don't grow a tightly-scoped PR on your own judgement. **Answer every review point explicitly: `FIXED` / `NOT-FIXED` / `DISAGREE-because`.** Responses that silently address some points force full re-reviews of everything. Every review ask ends as one of: a commit, a filed follow-up issue, or an explicit "won't do, because…" — never silence.