Repository navigation
docs: the two working-agreement commits #104 merged past - #107
Merged
Merged
Conversation
…iled Sharpening the causality rule with the half it was missing. "It is this task's work" still left the escape open: write a follow-up issue for your own regression and finish the pull request anyway. That is the move that turns a five-minute fix into debt, and it is the move I made -- #105 was opened for a regression #101 had introduced, which is how the defect ended up parked on #92 instead of closed where it came from. It is also strictly worse than fixing it, and not only by the usual argument. The option expires: once the branch is merged the defect can no longer be closed where it was introduced, and a diff someone still had in their head becomes archaeology on main with nobody's name on it. A follow-up issue for your own regression is a promise to pay later at a higher price. The review paragraph gets the same closing: "it is only a P2" is the same deferral as "I will file it", reached by a different route, so priority does not reopen the question that causality already answered. 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
Read as a whole rather than one finding at a time, because fixing them singly is what has kept this pull request generating new heads and new reviews. The exception to the sequencing rule contradicted the scope rule. It permitted a follow-up for "a fix the review made necessary elsewhere" -- but elsewhere means not caused by this branch, and the scope rule sends exactly that to an issue. It is now one narrow case with three conditions: a change this pull request's own review makes necessary, too large to fold in without making the diff unreviewable, and split with the maintainer's agreement. The third is the load bearing one. An author who decides alone that their own finding deserves its own pull request has re-derived the parallel working the rule exists to stop, which is what happened with #105. The exception also sat after the merge-queue paragraph, so "The exception is..." read as an exception to the merge queue. It now follows the rule it belongs to, and the merge-queue passage is marked as the aside it is. The causality rule had nothing to say about the case a reader actually hits: the causing pull request is already merged, so it cannot be closed there. That is now stated -- an issue is all that remains, and it is the next thing worked rather than queued behind whatever else is open. Debt already owed does not also get to wait. Dropped a sentence that restated the urgency point a second time in the same paragraph; two phrasings of one rule read as two rules. 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 The one-PR-at-a-time exception is tightened to three conditions (including maintainer agreement to split); author-only “follow-up PR” splits and review findings elsewhere are explicitly out of scope. The merge-queue / Under Stay inside the task, branch-caused regressions must be fixed in the same PR, not deferred to follow-up issues, with expanded rationale (debt, post-merge archaeology) and a rule that once merged, owed debt is next work, not queued behind other open PRs. Review guidance is aligned: causality first, and “P2” / review deferral is treated like filing a follow-up; a duplicated urgency sentence is removed. Reviewed by Cursor Bugbot for commit 72a572d. 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: 1e2f3e3136
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codex, correctly: the two cannot both be followed. The sequencing rule permits a branch-caused change too large to fold in to become its own pull request when the maintainer agrees; the scope rule requires every branch-caused failure to be closed in this pull request and forbids a follow-up. In exactly the case the exception describes, the instructions contradict -- which is the ambiguity this branch exists to remove, reintroduced one section further down. Resolved by naming the exception where the stricter rule is stated, rather than dropping either. Dropping the exception would be wrong: a maintainer deciding a fix is too large to fold in is a legitimate call, and the same authority makes every other call here. Leaving the rules to argue would be worse than before. What the pair actually says, once written as one rule: the author never defers their own regression alone. Who may authorise an exception is the only question left open, and the answer is not the author. That is also the distinction #105 failed -- an approved split and a self-approved one look identical in the commit history and are not the same act. 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
2 of 9 tasks
dborgards
pushed a commit
that referenced
this pull request
Sep 13, 2026
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
dborgards
pushed a commit
that referenced
this pull request
Sep 13, 2026
…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
2 of 9 tasks
dborgards
pushed a commit
that referenced
this pull request
Sep 13, 2026
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
This was referenced Sep 13, 2026
dborgards
pushed a commit
that referenced
this pull request
Sep 13, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
#104 merged at
a1d4323. Two commits had been pushed to that branch after it and did not land:9b5248f9f172d6Both are cherry-picked here unchanged. Nothing new was written for this PR.
Why it is worth a follow-up rather than leaving it
maincurrently carries a working agreement that contradicts itself in the place that matters most. The sequencing rule's exception still reads:"Elsewhere" means not caused by this branch — and the scope rule three sections down sends exactly that to an issue. As merged, whichever half suits the moment can be quoted, which is the failure mode the section was written to prevent.
It also lacks the rule you agreed with explicitly:
Without it, "it is this task's work" still permits writing a follow-up issue for your own regression and finishing anyway — the move that made #105.
The four changes
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)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: no statement left contradicting the sequencing rule, code fences balanced, no line over the surrounding 100-column style, and the diff against
mainis exactly the two commits and nothing else.Note on
macos-latest: it will be red here as it is on every PR —J1939NodeTests.StartPeriodicSend_SingleFrame_FiresAtConfiguredPeriod, from #101, tracked on #92. It reproduces on the base revision, so by this PR's own criterion it is not this PR's to fix.🤖 Generated with Claude Code
https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Generated by Claude Code