Repository navigation
Conversation
…picking a statistic The median gap I merged in #101 fails on a real macOS runner, on a run where the scheduler was right. Observed gaps 83, 132, 73, 173, 67, 188, 62, 105, 185: mean 118.7 ms against a 120 ms period, so the rate was correct to within 1 %, but the median was 105 ms and the bound is 108. Nine alternating gaps have one more short than long, and the median picks the short side. My argument that the median is immune held for a minority of collapsed gaps and not for oscillation, which is what a loaded host actually produces. Third statistic on this assertion, and the first chosen by its error rather than by intuition. Writing out what is measured: t(i) = slot(i) * period + late(i), slots distinct and increasing so the gaps telescope and mean gap = (slots spanned / gaps) * period + (late(last) - late(first)) / gaps The first term is at least the period. The second is the entire problem, and it is bounded by the number of gaps and nothing else. That settles both earlier attempts at once: the plain mean over nine gaps divides a cold start by nine and loses 27 ms of a 120 ms period, which is how a 239 ms late first tick produced 106.8 ms; and the median has no such term to shrink, so its error tracks the shape of the jitter rather than its size and cannot be sized at all. So the sample count becomes the knob that makes the assertion sound rather than a free parameter: 21 samples, the first gap discarded as the only one measured from a cold schedule, and the mean of the remaining twenty. The residual endpoint term then stays under a tenth of a period unless the lateness swings by 240 ms between the second emission and the last -- an order of magnitude beyond anything observed. Oscillation cancels in a mean by construction, so the run above passes. The collection budget moves off ShortTimeout with it: 21 samples need 2.5 s before any tick is dropped, and a loaded runner coalescing to 2x or 3x needs several times that. Four times the nominal run keeps bounding the rate from above -- the one direction this assertion deliberately does not cover -- without failing for slowness. Verified: all three scheduler mutations still caught (period halved, skipped ticks released back to back, rate 15 % fast); 20/20 clean under an 8x CPU overload; suite 614/614; build 0 warnings / 0 errors with CI=true; format clean. Refs #92. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
PR SummaryLow Risk Overview The periodic rate check is reworked to match an explicit error-term model: collect 21 samples (up from 10), use a collection timeout of 4× nominal run time instead of No production code changes—test-only hardening for FR-J1939-007 periodic send timing. Reviewed by Cursor Bugbot for commit 92fb9c8. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Closing unmerged. This should have been an issue, not a pull request. It was opened off the back of CI on #104 — a markdown-only PR — by invoking an exception I had written into #104 myself a few hours earlier. That reading was self-serving: the exception is for findings on the PR's own content, not for anything the full test suite happens to surface while checking a docs change. The work is not lost. The finding and the derivation are recorded on #92, which is the right home and already open, and the branch Generated by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92fb9c8f42
ℹ️ 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".
| // of a period for anything short of a 240 ms swing between the second emission's lateness | ||
| // and the last one's -- an order of magnitude beyond what a loaded runner has produced | ||
| // here. Oscillation cancels in a mean by construction, so the case above passes. | ||
| var measured = gaps.GetRange(1, gaps.Count - 1); |
There was a problem hiding this comment.
With the usual exactly-21-emission snapshot, gaps contains 20 entries and this GetRange discards one, so the asserted mean covers only 19 gaps—not the 20 used to size the error bound. Consequently, an otherwise valid run whose observer lateness falls by 229–240 ms between the second and last emissions can average below the 108 ms threshold even though the stated 20-gap calculation says it should pass. Collect 22 emissions (or revise the bound for 19 measured gaps) so the implemented sample size matches the derivation.
Useful? React with 👍 / 👎.
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
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
…ho owns the call Codex, P1 and correct: inverting the exception left the paragraph below it saying the opposite. It still read "The exception is for findings on the pull request's own content" -- exactly the reading the inversion removes -- so the agreement could still be quoted to justify splitting out a regression the branch caused. Contradiction between paragraphs again, third of this kind here. Rewriting it turned up the more useful point. Under the corrected rule the #105 failure was *not* the misread exception: the macOS test failure genuinely was out of scope, correctly identified as information. What made it work was deciding, mid-task, that it deserved a pull request rather than the issue it should have been. So the anecdote now says that, which is what actually happened, rather than the tidier story I had written. That exposed a hole the inversion had opened. "Warrants its own pull request" had no owner, and an author who may decide it for their own detour has the escape back. The default is now the issue, and the exception is the maintainer wanting a pull request instead -- which is exactly how #105 was in fact handled, by being closed and recorded on #92. 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
The review is right that chronology row 9 contradicted the rule the document derives. It recorded "a second pull request for a problem the docs branch did not cause" as the failure, while the rule summary in the same document says that is exactly what the exception permits when the maintainer wants it. CLAUDE.md states the real failure at its worked example: the out-of-scope identification was correct, and what turned information into work was deciding mid-task that it deserved a pull request rather than the issue that is the default - a call the exception does not hand to the author. The row now says that, so the evidence supports the rule instead of contradicting it. The cause is the same as the previous finding: I summarised from memory of the rule as it stood before c25dc89 inverted it, rather than from the rule as it now reads two pages further down. Recorded in the list of instances. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
What does this change?
The median gap I merged in #101 fails on a real macOS runner, on a run where the scheduler was right. Caught by CI on #104 — a markdown-only PR, so the failure came in from
main.mainis red because of my change.The mean says the rate was correct to within 1 %. The median says 105 and fails. Nine alternating gaps have one more short than long, and the median picks the short side. My claim that the median is immune held for a minority of collapsed gaps and not for oscillation — which is what a loaded host actually produces.
Sized by the error term this time, not chosen by intuition
Third statistic on this assertion, and the first one derived rather than guessed. Writing out what is being measured — each emission on its own grid slot, late by however long the host stalled:
the gaps telescope, so
The first term is at least the period, because slots are distinct. The second is the entire problem — and it is bounded by the number of gaps, and nothing else.
That disposes of both earlier attempts at once:
So the sample count stops being a free parameter and becomes the thing that makes the assertion sound: 21 samples, first gap discarded as the only one measured from a cold schedule, mean of the remaining twenty. The residual endpoint term then stays under a tenth of a period unless lateness swings by 240 ms between the second emission and the last — an order of magnitude beyond anything observed here. Oscillation cancels in a mean by construction, so the failing run above passes.
The collection budget moves off
ShortTimeoutwith it: 21 samples need 2.5 s before a single tick is dropped, and a loaded runner coalescing to 2× or 3× needs several times that. Four times the nominal run still bounds the rate from above — the one direction this assertion deliberately does not cover.Verification
Note on the one-PR-at-a-time rule
#104 is open, and that PR is what introduces the rule. This invokes its stated exception — a pull request arising out of the one in flight — on the narrowest reading: #104's own CI surfaced it. The wider reason is that
mainis red from a change of mine, and queueing that behind a docs PR is the wrong trade. #104 touches onlyCLAUDE.md, this onlyJ1939NodeTests.cs, so neither will force a base merge on the other.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)Test-only, so no release-worthy change ships; the commit is typed
fix(test):to say what it is rather than to trigger a bump.Checklist
dotnet build CanKit.Pro.sln -c Releasesucceeds — 0 warnings / 0 errors with-p:CI=truedotnet test CanKit.Pro.sln -c Releasepasses — 614/614 onnet10.0Also run:
dotnet format --verify-no-changesclean.🤖 Generated with Claude Code
https://claude.ai/code/session_011Zd6AyAtcZApgfRC2Rkitj
Generated by Claude Code