docs: record the verification rules the last wave was missing, and serialise pull requests - #104
Conversation
…rialise pull requests Three changes to the working agreement, all from what the previous wave cost rather than from principle. **One pull request open at a time.** Every merge to main obliges each open branch to take a base merge, and each base merge is a fresh full-gate run, a fresh CI cycle and a fresh review pass over code that did not change. Two green pull requests in flight cost exactly that, plus a manual base merge when GitHub's Update branch button failed. The exception is a pull request that comes out of the one in flight, because the alternative is sitting on a finding until the first lands. This contradicted the existing bullet, which said pull requests in different packages "can run in parallel". That clause is removed rather than left to argue with the new rule -- a working agreement that disagrees with itself is worse than either rule alone. **A new section on claims that the gate cannot check.** The local gate catches broken code; it cannot catch a statement that was reasoned into existence instead of measured, because the build is green either way. Five rules, each written from getting it wrong: a claim about a test needs a mutation rather than an argument; when a tool disagrees with your own measurement, distrust the instrument first; remove the broken half of an assertion rather than replacing it wholesale; for anything asserted against a clock, name the quantity the host can perturb and check the margin against that rather than against timings observed so far; and a fallback does not finish the task -- the original path still has to be retried. Each carries the specific incident, because a rule without its failure reads as style advice and gets skipped. **Decisions that belong to the maintainer** get asked as a question with selectable options, followed by silence until answered -- a decision buried in a long status message gets missed, and the cost is a wrong assumption baked into everything after it. The same section retires "say the word and I will do it", which twice turned an obvious fix into a wait. Markdown only; no code, project or workflow file is touched, so no build, test or format result is claimed here. Refs #92. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The rule was right and the justification under it was not mine to invent. I had written it as machine cost -- base merges triggering fresh gate and CI runs over code that did not change. The maintainer's reason is supervision cost, and it is the one that matters: every open pull request carries its own review threads, bot findings and coverage report, each has to be driven to actually closed, and in practice that means being chased one at a time. Which is the opposite of what running two was supposed to buy. The second half is the part I had backwards. I described a base merge as re-running checks over unchanged code, i.e. waste. What it really does is reopen the question: every reviewer and bot re-runs against the new head, and findings that were already settled come back. That is not a slower version of the same outcome, it is more work appearing after the work was declared done. The distinction decides which exceptions look reasonable. Stated as machine cost, "CI is fast on this repo, so two in flight is fine" follows and is wrong. Stated as supervision cost, it does not. Adds the rule that falls out of the same observation: a pull request is finished when every thread is closed, not when the code is right -- bot findings and the coverage report included, and a thread whose finding was fixed but which shows no answer in it. None of that survives a base merge unchanged, so it is re-checked afterwards rather than assumed. Markdown only; no code, project or workflow file touched, so no build, test or format result is claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
PR SummaryLow Risk Overview Pull requests: Adds one deliberately authored PR at a time (Dependabot excluded), with rationale on review/bot supervision and base-merge churn; notes New sections: Stay inside the task — scope by causality (branch-owned regressions vs pre-existing/tooling noise). Before claiming something is true — five verification habits (test mutation, distrust instruments, fix the false half of assertions, clock margins, retry failed review paths). Decisions that belong to the maintainer — ask with options and wait; avoid “say the word and I’ll do it” stalls. Reviewed by Cursor Bugbot for commit a1d4323. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15e209431f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A pull request's scope is its own diff. Anything else the tooling surfaces while checking it -- a failing test the branch does not touch, a coverage row from elsewhere, a bot finding about neighbouring code -- is information, not work. It becomes an issue; the pull request in hand finishes first. Written from breaking it: a markdown-only change to this file ran the full suite, the suite failed on an unrelated timing test, and that produced a second pull request, justified by the exception added to the sequencing rule three hours earlier in this same branch. The exception is for findings on the pull request's own content. Read any wider it empties the sequencing rule of meaning, which is what happened. Also records the distinction underneath it: a correct review finding is not automatically this task's work. Correct and urgent are separate questions, and only the second decides whether it happens now. Three same-day rounds went into a test the maintainer had already said was not important here, because every finding was treated as a must-fix rather than triaged. Markdown only; no code, project or workflow file touched, so no build, test or format result is claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c45445471
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…te it Codex is right that ci.yml carries a merge_group trigger and that a merge queue is exactly the mechanism for testing a queued branch against current main without a base merge. My rationale did not account for it. It is not right that the workflow "already handles this case". Checked rather than assumed: filtering CI runs by event merge_group returns zero. The trigger has never fired, because the queue is configured in the workflow but not enabled on the branch. Every merge to main, #100 and #101 today included, went in as a plain merge commit, and the base merges this wave paid for were real. So the rationale now says both things: the base-merge cost is real as things stand, and it has a known expiry the day the queue is enabled -- at which point that half of the argument goes away and the supervision half, which is the reason the rule exists, does not. A rule whose stated cost can quietly stop being true invites being dismissed later on exactly that ground. Filed as #106, because an inert guard reads as protection that is not there, and because the failure it was built for already happened once (#85, from two green pull requests merged four minutes apart). Markdown only; no code, project or workflow file touched, so no build, test or format result is claimed. Refs #85, #106. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Two review findings on this pull request's own text, both correct. Scope was written as "a failing test the branch does not touch", which is the wrong criterion and would have excused the exact failure this branch was written after. A change that breaks existing behaviour is caught by a regression test in a file it never edited; "I did not touch that file" is not a defence. The question is whether the failure reproduces on the base revision -- if it does not, the branch caused it and it is work now. Only what fails on the base too, or is merely surfaced alongside, is information rather than work. The one-pull-request rule was unqualified and therefore contradicted by .github/dependabot.yml, which permits five concurrent NuGet updates, three npm and two pip. Dependabot is not what the rule is about: those carry no design to review, only a green or red build, and none of the supervision cost the rule exists to avoid. The rule now says "of deliberately authored work" and notes that narrowing the automated fan-out, if anyone wants it, belongs in the Dependabot config and not in this file. Markdown only; no code, project or workflow file touched, so no build, test or format result is claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8db8b5c011
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
| head | observed gaps | mean | median | bound |
|---|---|---|---|---|
15e2094 |
83, 132, 73, 173, 67, 188, 62, 105, 185 | 118.7 | 105.0 | 108 |
5c45445 |
(same test) | — | — | 108 |
8db8b5c |
182, 107, 70, 117, 131, 108, 181, 84, 95 | 119.4 | 107.7 | 108 |
The last one misses by 0.27 ms. In all three the mean says the scheduler held the requested rate to within about 1 %, and the median — the statistic I merged in #101 — says otherwise. So main currently carries a check that no pull request can turn green on macOS, which is a standing tax on every PR until #92 is picked up.
A prepared, mutation-verified fix sits on branch fix/periodic-test-sample-count (referenced from #92). It is not being merged from here: that is a test change, this is a docs change, and #105 was already closed for exactly that reason.
Generated by Claude Code
Codex found this section contradicting itself. It opens by saying causality decides scope -- a failure the branch caused is the branch's, whatever file it lives in -- and then closes by saying "correct and urgent are different questions, and only the second one decides whether it happens now". Read on its own, that last sentence permits deferring a valid regression the branch just introduced, which is exactly the excuse the section was written to remove. The two questions are now ordered rather than left to compete. Causality first: a finding about something this branch caused is this task's work however small it looks, and a review does not create an exception to the rule three paragraphs above. Urgency is reached only for findings the branch did not cause, where a correct finding still need not be this task's work. Markdown only; no code, project or workflow file touched, so no build, test or format result is claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1d4323e8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| the failure reproduces on the base revision: if it does not, the branch caused it and it is work | ||
| now. |
There was a problem hiding this comment.
When the failure is flaky—such as the unrelated timing failure cited immediately below—a branch run can fail while a single run on the base passes even though the branch had no effect. Treating that non-reproduction as proof of causality directs agents to modify unrelated code and defeats this section's scope rule; require repeated or otherwise controlled base/branch comparison before concluding that the branch caused the failure.
Useful? React with 👍 / 👎.
docs: the two working-agreement commits #104 merged past
Brings in #104, #107 and #108. No conflicts: those touch CLAUDE.md, this adds a new file under docs/reviews/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
…after it The document was written mid-wave and then sat on a branch while #104, #107 and #108 happened. Publishing it unchanged would have put a stale derivation in the repository -- and stale in the worst place: its summary of the rules quoted the one-pull-request rule in the version the maintainer rejected, with an exception for problems the pull request itself caused. Corrected, and extended with what the second half produced, which is the more useful half: Seven Codex findings across #101, #104, #107 and #108, all seven upheld, four of them contradictions *between* paragraphs rather than defects in one. The last of those survived a re-read done specifically to hunt contradictions -- because I checked the paragraphs I had edited against each other, and the offending sentence was one I had not touched. It had been correct until a rule two commits earlier was inverted. That generalises to something worth having: inverting a rule silently invalidates every sentence referring to it, including outside the diff, so the question after a rule change is which statements it made false, not which lines it touched. Which is the same principle this file states for code, applied to prose -- and I had written that principle four commits before failing to apply it here. Also records the merge-race that happened twice (two corrections pushed shortly before a merge and lost with it) and the signal agreed in response, and adds #106 plus the still-unwritten "FERTIG -- mergebar" convention to the open items. Markdown in a directory mkdocs excludes from the site; no code, project or workflow file touched, so no build, test or format result is claimed. Refs #104, #106, #107, #108. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Two findings from the review of this branch, both caused by extending the chronology from eight rows to eleven without re-reading what referred to it: - The header still said eight merged pull requests while the inventory two paragraphs below listed nine (#93, #96, #97, #98, #100, #101, #104, #107, #108). Corrected to nine. - A blank line between row 8 and row 9 terminated the Markdown table, so rows 9-11 rendered as plain pipe-delimited text. Removed. Two more of the same class that the review did not name: "in jedem der acht Faelle" in the cause section refers to the measurement failures only, so it is now "der ersten acht"; "von den acht Zeilen oben" means the whole table and is now "elf". The section on cross-paragraph contradictions records this occurrence, since the document reproduced the very error class it describes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
The review is right that the three cited hashes are poor evidence, though not for quite the reason given: they do still resolve in a full clone today, because their branches were never deleted. But none is reachable from `main` - which is what "merged past" means - so they survive only on those branch tips and stop resolving the day either branch is cleaned up. That is exactly the durability the new provenance rule is supposed to buy. So CLAUDE.md now points at #104 and #107, whose timelines are permanent, and the provenance rule says which record to cite rather than assuming a commit is always the durable one: the commit where it is reachable from `main`, the pull request or issue where it is not. As written it would have mandated the weaker reference in precisely this case. The retro keeps the hashes - it is a dated record, which the same rule exempts - and now says where they live and why, so a reader who cannot resolve them knows that is the finding rather than a broken citation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
What does this change?
Three additions to
CLAUDE.md, all written from what the last wave actually cost rather than from principle. The retro that produced them is only useful if it sits where the work starts, so it goes into the working agreement instead of a document alongside it.1. One pull request open at a time
Each merge to
mainobliges every open branch to take a base merge, and each base merge is a fresh full-gate run, a fresh CI cycle and a fresh review pass over code that did not change. Two green PRs in flight cost exactly that — plus a manual base merge when GitHub's Update branch button returned a server error.The exception is a PR that comes out of the one in flight (a follow-up split off during review), because the alternative is sitting on a finding until the first one lands.
This contradicted an existing bullet which said PRs in different packages "can run in parallel". That clause is removed rather than left to argue with the new rule — a working agreement that disagrees with itself is worse than either rule on its own.
2. A new section: Before claiming something is true
The local gate catches broken code. It cannot catch a claim that was reasoned into existence instead of measured, because the build is green either way — and that is what the last wave kept producing. Five rules:
Each carries its specific incident. A rule without its failure reads as style advice and gets skipped.
3. Decisions that belong to the maintainer
Asked as a question with selectable options, then silence until answered — a decision buried in a long status message gets missed, and the cost is a wrong assumption baked into everything after it. The same section retires "say the word and I will do it", which twice turned an obvious fix into a wait.
Type of change
feat— new behaviour (minor release)fix/perf— bug or performance fix (patch release)docs/test/refactor/chore/ci— no release!in the title, plus aBREAKING CHANGE:footer explaining the migration)Single
docs:commit, no release.Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds — not run and not claimed: markdown only, no code, project or workflow file toucheddotnet test CanKit.Pro.sln -c Releasepasses — sameChecked instead: the file has no remaining statement contradicting the new sequencing rule, code fences are balanced, and no line exceeds the surrounding 100-column style.
🤖 Generated with Claude Code
https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Generated by Claude Code