diff --git a/.claude/shared/engineering-rules.md b/.claude/shared/engineering-rules.md index 2fca32f08..a58d265cf 100644 --- a/.claude/shared/engineering-rules.md +++ b/.claude/shared/engineering-rules.md @@ -81,7 +81,7 @@ The two rules that hold everything else together: **Config safety:** no silent fallbacks for critical config — unset or unrecognised values are errors, not defaults (a quiet default has shipped the wrong environment to production). Env values compared exactly — set with `printf`, never `echo` (stray newline). Build-time-inlined vars (`NEXT_PUBLIC_*`) need a fresh build; dashboard edits alone do nothing. Never print secret values — check presence, not content; keys never in argv, shell history, or committed files. -**Mutation-test the fix and state the count** ("disabling receipt verification turns 5 red"). A suite equally green with and without the fix proves nothing. +**Mutation-test the fix and state the count** ("disabling receipt verification turns 5 red"). A suite equally green with and without the fix proves nothing. **Commit before you mutate.** Mutation testing means breaking the code and putting it back, and the obvious way to put it back — `git checkout -- ` — reverts to HEAD, which discards the fix along with the mutation whenever the fix is not yet committed. Checkpoint first, then mutate against that commit. The cost of getting it wrong is the entire change, and it does not announce itself: if the tests were committed the suite goes red and reads like a mutation that worked, and if they were not it goes green and reads like a mutation that did nothing. Both are the same accident. **Pair every absence-assertion with a control.** "Nothing bad happened" must run beside "something happened at all", or it passes against code that never ran — one review caught four vacuous passes this way. **Check that "never happened" assertions can fail** — a `calls === 0` assertion on a path where the counter can't increment proves nothing. Make it fail by hand first. @@ -148,7 +148,7 @@ The review round is where contributor quality is actually measured. In order: **Merge what's ready promptly** — an approved PR sitting unmerged makes every stacked PR staler. Keep stacks rebased after the base merges. Call out schema migrations in merge notes. -**Approvals don't survive a re-roll.** A dependency PR once carried a sign-off for one major version after the bot re-rolled it to the next. (The ruleset dismisses stale approvals on push now — but a re-approval is a re-review, not a click.) +**One approval carries, and it survives later pushes.** The ruleset no longer dismisses a review when the branch moves: an author who fixes review findings does not have to go back for a second click, which was the single biggest source of stalled PRs in practice. **The cost is real and named rather than waved away:** an approval now attests to the commit it was given on, not to the head that merges. A dependency PR once carried a sign-off for one major version after the bot re-rolled it to the next, and that failure gets easier, not harder, under this rule. What still gates every head is the required `ci` check and **branch must be up to date with `main`** — machines, not judgement. So the discipline moves from the platform to the reviewer: **re-read the diff before merging anything that moved after you approved it**, and treat a bot re-roll as a new PR rather than the one you looked at. **After merging, check what closed.** An issue once closed the second a *related* PR merged — the PR's body said in bold it does not close that issue, and its footer said `Refs` — because a Development-sidebar link outranks anything in the body. It stayed wrongly closed for days. Rules: **sidebar links close issues; prose does not stop them** — unlink what you don't intend to close. **A ticket is done when its acceptance criteria are met, not when a related PR merges** — re-read them before closing. **If closing orphans remaining work, file the successor first** and link it from the closing comment. Run `/post-merge ` after every merge until it's a habit. @@ -162,7 +162,7 @@ The review round is where contributor quality is actually measured. In order: ## 7. What is enforced by the platform vs. by discipline -Enforced (ruleset + repo settings from `pm-kit/protection`): PR required · 1 approval · stale approvals dismissed on push · required `ci` check · **branch must be up to date with `main`** · no force-push · no deletion · squash-only · PR title = commit. Everything else in this document rests on you. Treat a green check as information, not permission, until you know what it ran. +Enforced (ruleset + repo settings from `pm-kit/protection`): PR required · 1 approval (which survives later pushes — see §6) · required `ci` check · **branch must be up to date with `main`** · no force-push · no deletion · squash-only · PR title = commit. Everything else in this document rests on you. Treat a green check as information, not permission, until you know what it ran. --- @@ -186,6 +186,7 @@ Places where reasonable conventions pulled in different directions; the decision - **"Squash to logical commits whose messages carry rationale and trailers" vs squash-merge-to-one-commit.** The repo squashes with PR title as subject and PR body as message, so the *PR body* is where rationale, verification, limits, and trailers live — the same content, in the place the platform actually preserves. Intermediate commits on the branch can be whatever helps you; they don't reach `main`. - **PR template shape.** What & why / Scope-not / Judgement calls / Verification / Merge order. The one section teams usually lack is **Judgement calls** — it's now its own section. - **"One issue per problem" vs "cluster findings".** Not actually opposed: cluster by *fix boundary* (same-diff test) and split by *schedule/priority* (different-schedule test). Both tests are in §1. +- **Dismissing stale approvals on push vs one approval that carries.** Chose the latter. Dismissal is the more correct rule and we ran it for months; what it bought in theory it spent in queue time, because the common case is not a bot re-roll, it is an author pushing exactly the fixes the reviewer asked for and then waiting on a second click for them. Kept the compensating controls that do not need a human: required `ci` on every head, and branch-up-to-date. The uncovered case — content changing materially under an approval — is now a reviewer habit rather than a gate, and §6 says so in those words rather than pretending the hole is closed. - **Required review-thread resolution (ruleset) vs approve-with-nits.** Both rejected. Mechanical thread resolution is click-blocking, not review quality — the ruleset doesn't require it. But approve-with-nits is gone too: no approval while any finding is open, however small. Every review point ends as a fix commit, a *filed* follow-up issue, or an explicit won't-do agreed in the thread (§4, §5). - **"Require branch up to date" vs the parallelism tax of many small PRs on shared files.** Kept strict up-to-date — stale-head merges have broken production; the tax is a click and a CI run. If the queue gets deep (>~5 concurrent PRs on shared files), enable GitHub **merge queue**, which performs the up-to-date test automatically and removes the clicks. Auto-merge on. - **Squash-only vs stacked PRs.** Squash makes stacks slightly more work (second-lander rebases `--onto main`). Kept squash — the clean linear log and title-as-commit are worth more than stack convenience; keep stacks ≤ 2 deep and prefer independent branches off `main`.