Repository navigation
test(#2700): a ratchet on hand-written test timeouts, so the count can only fall - #2835
Merged
Merged
Conversation
…n only fall #2708 built TestTimeouts for #2700 and closed nothing: until #2829 the type's only references were its own test. All ~1,470 hand-written literals were still literals. A shared mechanism with no adopters is inert, and inertness is invisible without a count — so count it. The literal is wrong twice over, and the second reason is the one #2700 does not state: 1. CI is ~1.7x slower than the machine the number was chosen on, so 30 s leaves ~18 s of headroom and under contention that is gone (#2700). 2. 🚨 30 s IS the framework's own write bound. UpdateRemote fails a silent write at LateResponseWatchBound (30 s) + VerdictBoundGrace (1 s) = 31 s, the grace being deliberate so the caller cannot race an admissible verdict. A test bounded at 30 s gives up ONE SECOND before the framework can explain itself — always — so the failure says "emitted nothing at all" and never OwnerUnreachable (#2819). A ratchet rather than zero: 1,470 sites across ~287 files is a long tail, and a guard that cannot pass today gets disabled. The count may only fall. Seeded at 1490 against a tree of 1470. The 20 is a TRANSITIONAL allowance, documented as such: the tree gained 4 literals during the hour this was written — several sessions merge in parallel and a branch cut before the rule existed cannot know about it — so a zero-slack baseline would red main within the hour and punish authors for a rule that post-dates their branch. TheBaselineStaysCloseToTheTree caps the slack at 100 so the allowance cannot become permanent. Mutation-proven, over a planted tree running the REAL scan: - seeded at 0 the ratchet fires and reports the true count (1466 at the time); - 30_000 is caught (a digit separator is the cheapest way past a naive scan); - prose and string literals are NOT caught (this repo writes those at length); - TimeSpan.FromSeconds(45) is NOT caught — a reasoned bound is not the copied convention; - 🚨 the CONVERTED shape (TestTimeouts.Convergence) is NOT caught, so the guard cannot punish the fix; - bin/ is not scanned. It has already earned itself: pointed at current main it found 4 literals added after the baseline was taken. Verified: MeshWeaver.Documentation.Test 126/126 green, Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FhP7Jg65q8cAATnFfeaTK
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new governance/ratchet test to prevent additional hand-written “30 second” test timeout literals from being introduced, making regressions visible via a count that can only decrease over time.
Changes:
- Introduces
TestTimeoutLiteralRatchetGuardto scan thetest/tree for specific 30-second timeout literal shapes and fail if the total exceeds a seeded baseline. - Adds a “baseline stays close” assertion to prevent the baseline from drifting too far above the real tree count.
- Includes a mutation-based self-test that plants a temporary tree and verifies the scanner catches only the intended shapes while ignoring comments/strings and
bin/.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
Contributor
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
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.
#2708 built
TestTimeoutsfor #2700 and closed nothing. Until #2829, the type's only references were its own test — all ~1,470 hand-written literals were still literals. A shared mechanism with no adopters is inert, and inertness is invisible without a count. So this counts them.The literal is wrong twice over, and #2700 states only the first reason
UpdateRemotefails a silent write atLateResponseWatchBound(30 s) +VerdictBoundGrace(1 s) = 31 s — the grace deliberate, so the caller's failure cannot race a still-admissible verdict. A test bounded at 30 s therefore gives up one second before the framework can explain itself, every time, so the failure reads "the observable emitted nothing at all" and neverOwnerUnreachable(Flake: FileSystemObservableQueryTests.ObserveQuery_ScopeExact_OnlyNotifiesExactPath emits nothing within 30 s on a loaded shard — ejected #2814 from the merge queue #2819).The second is why this warrants a guard rather than a style note: the number is not merely tight, it is the one value that destroys the most information.
Why a ratchet, not zero
1,470 sites across ~287 files. A guard that cannot pass today gets disabled, so the count may only ever fall.
On the seed: 1490 against a tree of 1470. The 20 is a transitional allowance and is documented as one. The tree gained 4 literals in the hour this guard was written — several sessions merge in parallel, and a PR branched before this rule existed cannot know about it, so a zero-slack baseline would red
mainwithin the hour and punish authors for a rule that post-dates their branch.TheBaselineStaysCloseToTheTreecaps the slack at 100 so the allowance cannot quietly become permanent, and the class remarks say plainly that the next correct edit to the number is downward.I want to be explicit that this is the trade-off I chose rather than an oversight: a tight seed is a stronger guard and a worse neighbour, on a tree this busy.
Mutation-proven, over a planted tree running the REAL scan
Not by exercising the regex against strings — that is how three guards here passed in one day while their subjects broke.
rose to 1466).30_000is caught — a digit separator is the cheapest way past a naive scan.// never write 30.Seconds() by hand) and string literals are not caught; this repo writes those at length.TimeSpan.FromSeconds(45)is not caught — a different, reasoned bound is not the copied convention this is about.TestTimeouts.Convergence) is not caught, so the guard cannot punish the fix.bin/is not scanned.It has already earned itself
Pointed at current
mainrather than its own base, it found the 4 literals that landed after the baseline was taken. That is the regression it exists to stop, occurring inside the hour.Verification
MeshWeaver.Documentation.Test126/126 green (whole project, not a filter — a new guard can interact with existing ones). Release-warnaserrorclean.Does not close #2700: the remaining work there is adoption at ~1,470 call sites. This stops the number growing while that happens.