Fixes #3069

[erikdarlingdata]

HeaviestHourlyRefreshObservedCeilingSeconds keeps the value 864. What changes is what its doc comment claims about where that value came from, plus a pin that holds every figure derived from it to agreeing with the series the comment now publishes.

What the doc said, and what it says now

Before, the constant was introduced as "the longest run recorded ... under the narrowed HourlyRefreshStartOffset window", and its measured-range paragraph read: under the 1-day window the highest figure recorded is the 864 s here, with the drift chart's most recent readings for the same job at 194 s, 225 s and 335 s, "climbing with volume rather than settling".

Both halves of that are now superseded. The companions did not keep climbing — the same job went on to 594 s and settled back into the 300s — and 864 s is not demonstrably a maximum of the narrowed regime at all.

Now the comment states, in this order:

• Where the number came from. One run, 13:30:00 to 13:44:24 on the boundary day, on the one store that carries this workload, read from that store's per-run job history with an explicit succeeded column that said t.
• The transition-run ambiguity. The narrowed HourlyRefreshStartOffset reached that store's refresh policies at 13:44:23 — one second before that run ended. So either 864 s is the transition run, in flight while start_offset was narrowed underneath it and therefore neither cleanly pre- nor post-treatment; or the boundary timestamp was itself derived from that run's completion, in which case the boundary and the constant are the same observation and cannot corroborate each other. Separating those two needs clock times nobody has read.
• Why "conservative" is the wrong word for it, safe though it is. Conservative reads as measured-then-rounded-up, and nothing measured this as a ceiling. It is a reading whose regime membership is undetermined which happens to exceed every cleanly post-boundary run observed — a partly-narrowed window simply costs more than a fully-narrowed one. The value bounds the grid safely without having been established as a bound: the safety is incidental and the provenance is still owed.
• The distribution, with the summary figures the published rows actually yield.
• The hole, and the rule that keeps the whole unstatused series out of this constant, below.
• Why the value is unchanged in both directions, below.

The series, and its status provenance

Ten consecutive runs of this job's refresh policy, each read with an explicit succeeded column and every one of them t, in start order across the ten hours after the boundary:

864, 194, 222, 225, 335, 594, 465, 359, 293, 355 seconds.

Setting the first aside for the reason above, the remaining nine run from 194 s to 594 s, mean 338 s, middle reading 335 s — every one below RefreshSlotWarningSeconds. The largest clears the 900 s slot by 306 s, where the constant clears it by 36 s.

Three things are recorded about the series rather than smoothed over:

• One run was never observed — the one starting near 00:35:05. By the next look the catalog's last-run figure had already advanced past it.
• No reading from the hourly self-metrics snapshot may set this constant — stated as a rule about the source rather than as a list of the readings it excludes, for the reason in the scope section below. StoreSelfMetrics.BackgroundJobInsertSql (object_kind = 'background_job') records last_run_duration with no status column at all, while HeaviestRefreshRuntimeSql and Query Store aggregate tax scales serially with fleet size — measure, alert, and evaluate before large onboardings #2136's JobCadenceReadSql both filter last_run_status = 'Success', so an unfiltered series can carry an aborted run's duration. A test checks that reason against the shipped SQL rather than restating it.
• HeaviestHourlyRefreshObservedCeilingSeconds is one sample of a twelve-hour-old regime, and #3035's grid plus a build-time assertion are sized against it #3069's comment records the median of the same window as ~347 s, and the rows published there do not produce that. 347 is the midpoint of 335 and 359, so it is an even-count median over eight of the ten rather than the nine clean ones; the nine clean rows have a middle of 335 s. The doc holds the figure its own listed series yields, and says why the other one differs.

Why the value is unchanged, in both directions

Upward is asserted as a failure by design. TimescaleSupportTests carries HeaviestHourlyRefreshObservedCeilingSeconds < RefreshPhaseStepMinutes * 60, built by #3055 so the number could not be renumbered through — past the slot width the refresh runs into its neighbour and the grid needs redesigning, not renumbering. Nothing here touches that assertion.

Downward is the subtler trap, and it is why the honest reading of the observation does not license editing the number. Correcting toward the observed 594 s would leave 306 s of margin in place of 36 s — and the reason CompressionPhaseMinutes excludes the heaviest slot whole rather than guarding it rests on that 36 s being real. At 864 s the run occupies 14.4 of the slot's 15 minutes, so no minute of it a CompressionPhaseGuardMinutes band could recover; at 594 s it occupies 9.9, and minutes 10 through 14 of that slot become recoverable. A downward edit therefore reopens #3035's exclude-vs-guard design decision rather than merely re-provenancing a number, and eleven hours of one quiet night does not settle that.

So the resolution is the one #3069 states for itself — the post-fix distribution came back with its maximum inside the slot, so the constant gets its provenance recorded and the grid stands as designed. RefreshSlotWarningSeconds, the build-time assertion and #3035's grid are all untouched, and no migration rung is added.

The pin, and the evidence that it can fail

RefreshCeilingProvenancePinTests parses the real TimescaleSupport.cs (resolved from [CallerFilePath], so there is no second copy to go stale) and requires every figure the comment states to follow from the constants or from the series the comment itself publishes. Nothing in it hardcodes 864, 900, 36, 306 or 338.

What it derives:

• the series contains HeaviestHourlyRefreshObservedCeilingSeconds exactly once — the link that makes the transition-run paragraph about this number
• the stated run count equals the listed rows; the stated low, high, mean and middle equal what the listed rows compute
• the stated slot clearance equals RefreshPhaseSlotSeconds minus the listed maximum, and the stated margin equals it minus the constant
• the quoted run's own clock arithmetic: end - start equals the constant, and end - boundary equals exactly one second — the transition-run finding, as a subtraction
• occupies 14.4 of the 15 minutes equals the constant in minutes and RefreshPhaseStepMinutes, with a separate assertion that the constant is still an exact tenth of a minute so a rounded claim cannot wear an exact one's clothes
• Assert.All over the whole clean inventory — every cleanly post-boundary reading is below the constant and below the watch line — rather than one true clause standing in for the set, with the population asserted non-empty first
• the inadmissibility reason against the shipped strings: HeaviestRefreshRuntimeSql and JobCadenceReadSql filter to successful runs, BackgroundJobInsertSql does not and carries no status column at all. The predicate is red-proofed in both directions on synthetic copies, because a predicate that could only return true would satisfy the two positive lines and prove nothing about the negative one — which is the load-bearing half.

Red-proof, run on the committed tree. EveryNumericPin_ReportsAnInjectedDrift bumps each captured number one at a time in a mutated copy, asserts the pattern still matches (so a mutation that merely broke a regex cannot pass as a caught drift), then requires verification to fail with something other than a parse miss. It ran 45 injections and all 45 were caught; the count is asserted equal to the captured-number total, so a sweep that enumerated nothing goes red rather than green. One at a time is the point: a bump to a middle-ranked series reading changes neither the range nor the middle, and is caught only because the mean is stated too. Verify also requires it consumed every pin in Pins(), so a pattern cannot sit in the list looking like it guards something.

Twelve hand-injected mutations were run separately against the real files, each with a build-exit check distinct from the run so a stale dll could not print a clean Failed: 0:

mutation	result
constant 864 -> 870 (still inside the slot, still above the watch line, still an exact tenth)	2 tests red
one non-extremal series reading 222 -> 223 (range and middle unchanged)	2 tests red
stated mean 338 -> 339	red
stated clearance 306 -> 305	2 tests red
boundary clock 13:44:23 -> 13:44:22	2 tests red
midpoint pair 359 -> 358	2 tests red
occupancy 14.4 -> 14.5; slot 15 -> 16	red, each
a pinned sentence reworded so the pattern misses	3 tests red
the series loses a row but keeps its stated count	2 tests red
BackgroundJobInsertSql gains a status filter	red
HeaviestRefreshRuntimeSql loses its status filter	red
a pin declared in Pins() that Verify never consumes	red

One mutation that did not go red is worth recording: rewording a paragraph heading nothing asserts on. That is a mutation aimed at text the assertions do not read, so it is not a test of the pin — not a gap in it.

Verification, and its scope

Darling.Tests cannot run on macOS, so the actual test sources were compiled into throwaway net10.0 xunit v3 harnesses staged in the gitignored project bin/, each with a build-exit check separate from the run:

• the new pin: 5 tests, Failed 0, Skipped 0, Not Run 0
• TimescaleSupportTests: 46 tests, Failed 0, Not Run 0, 15 skipped and every one of them a _AgainstDevPostgres live-store test. The three facts that read this constant were also run individually by name — CompressionPhaseGrid_ClearsEveryRefreshSlotsGuardBand_AndTheHeaviestRefreshsSlotWhole, TheRefreshSlotReading_CarriesItsOwnVerdict_AndReportsOverrunAsNegativeHeadroom, TheRefreshSlotLogLine_IsLeveledByBand_AndSaysNothingWithoutAReading — each Total: 1, Failed: 0.
• DocCommentHygieneTests: 40 tests, Failed 0, and proved to be reading the edited file by injecting a stacked <summary> into the new doc run and watching two of its facts go red.

PerformanceMonitor.Darling.Storage and Darling.Tests both build with -p:EnableWindowsTargeting=true and 0 errors; the 49 analyzer warnings on Darling.Tests are all pre-existing xUnit nags in other files, none in the new one. CI's Windows build job remains the arbiter for the full suite.

What is still outstanding, and is not claimed here: the status-filtered per-bucket split of the full per-run series (collect.store_metrics, object_kind = 'background_job') on the boundary timestamp, giving count, median and maximum each side of it. That is the read that would give the constant real provenance in either direction, it needs a session against the one store that carries this workload, and it was not taken. The doc comment names it as the outstanding read rather than leaving a future reader to rediscover that it is missing.

One whole-tree guard this had to join, and a stale count beside it

The first CI round came back with exactly one failure across both suites — Lite.Tests 3402/0, Darling.Tests 7794 with a single red, so the new pin's own tests passed on Windows and on the Linux Postgres job. The red was CommentFilterAdoptionTests.EveryLinePrefixCommentFilter_StatesTheBoundItRestsOn: DocProseFor walks the contiguous /// run above a declaration, which is a line-prefix comment filter, and that guard requires every such site to be enumerated with the bound it rests on.

It is registered as the COLLECTS a doc run kind — the same kind DocCommentHygieneTests and both AnalysisPassTokenThreadingTests twins are on. The prefix defines the run rather than approximating comment-stripping, and asking for CSharpSourceWalker would leave nothing to read, since the figures being pinned live only in comments. The stated bound is that the walk stops at the first non-/// line, so a block comment between the doc run and its declaration truncates it — and that direction is loud rather than quiet, because the collected prose is asserted to open at <summary> and close at </summary> before any figure is compared.

The kind count in that class's own doc comment was already stale. It read "Four kinds live here ... Two collect a doc-comment run" against three such entries, so it was wrong by one before this PR and would have been wrong by two after. Corrected to "Four" in the same commit rather than filed, since it is one word adjacent to the line being added and this change is what makes it worse.

Red-proofed locally rather than assumed: removing the new census entry reproduces the exact CI failure, same expected/actual pair (Lite.Tests/AnalysisPassTokenThreadingTests.cs against Darling.Tests/RefreshCeilingProvenancePinTests.cs at index 4), which is also what confirms the local harness reads the same tree CI does. All four harnesses re-run green afterwards with build exit codes checked separately: pin 5/0, CommentFilterAdoptionTests 2/0, DocCommentHygieneTests 40/0, TimescaleSupportTests 46/0.

Review round two: the pin's prose coupling, cut where it was cuttable and defended where it was not

The review granted the arithmetic and objected to maintenance surface — ~80 lines of prose tied to 13 near-verbatim patterns, where a copy-edit touching no number reds CI. Two thirds of that was fair.

Fixed at the extractor. DocProseFor strips <b> emphasis and normalises em/en dashes to a hyphen before any pattern runs, so all seven <b> couplings and the \S dash stand-in are gone. Patterns are trimmed to the span bracketing their numbers: 122 verbatim anchor words down to 96, with the drift sweep still at 45 injections — so the reduction cost nothing in coverage. The population-merge mutation now addresses its number by ordinal through the same source-rewrite the sweep uses, instead of string-replacing an emphasis-tagged reading, which would have found nothing if someone unbolded it.

The failure message now names all three correct fixes — update the pattern, fix the prose, or delete the figure and drop the pin — and says why editing a pattern is sanctioned rather than a loophole: EveryNumericPin_ReportsAnInjectedDrift re-derives every case from the pattern you write, so one widened into a no-op goes red. #3073 already ships that same protection (EveryPin_ReportsAnInjectedDrift), and its own handling of the third outcome is split three ways: the prose-pin message lists two outcomes and omits pattern-editing; a code comment above that assertion (ReadmeDerivedCountPinTests.cs:198-200) discourages it, scoped to a formulaic README inventory sentence; and its --test-connection denominator sweep directs it outright (:147, "Fix the pattern rather than the count."). So this message states, for the prose kind, the outcome the prose-pin message left unstated — on the mechanism #3073 already carried that makes it safe. Not a divergence from policy, and not an oversight being corrected either: a deliberate split, stated here for the kind that invites editing.

The multiset alternative was measured, not argued away. The doc run holds 65 numerals, 39 distinct, and the derived figures repeat: 36 four times, 594/194/335/9 three times each, 306/864/10 twice. Set-semantics containment false-passes on three of four injected single-figure corruptions, catching only 338 — the one figure stated exactly once. A count-bearing multiset would catch them only by encoding per-figure occurrence counts, which is the restatement the bar bans and which reds on any rewording that mentions a figure once more or less. The underlying reason is that a numeral bag cannot police which claim a number belongs to, and 36 versus 306 — same constant, same slot, different sentence — is exactly the error a future editor makes.

The census summary, pinned rather than filed

CommentFilterAdoptionTests had to gain an entry for the new pin's /// walk (the COLLECTS a doc run kind, alongside DocCommentHygieneTests and both AnalysisPassTokenThreadingTests twins). While registering it, s_bounded's doc comment turned out to describe the census by count and to be wrong: it read "Two collect a doc-comment run" while three entries already did — stale by one before this PR, and it would have been stale by two after. 4 COLLECTS + 1 stated bound + 1 SQL + 1 demonstrator = 7 keys, which is the map's size.

The durable finding is the asymmetry: that guard catches a new member by name and nothing caught its own summary drifting, in a file whose entire job is adoption tracking. So the summary is now pinned rather than just corrected. All five counted claims are derived — the kind total plus one claim per kind — from the map and its entry text; none was underivable, and the kind labels are required to account for every entry so the breakdown cannot quietly stop summing to the map.

Note this loose-on-prose/strict-on-numbers shape is available here precisely because a count survives arbitrary rewording as long as the numeral is still there. It is not available for a figure whose meaning depends on which sentence it sits in, which is why the thirteen patterns stay.

Red-proofed four ways, each confirmed red then reverted: prose count wrong against the map; a numeral spelled outside the word vocabulary (the word-to-digit mapping is the load-bearing part, since the prose spells counts as words); a fifth COLLECTS entry added with the prose untouched; an entry whose bound names no recognised kind. The pin also caught a mistake of mine on its first run — it was anchored on the class summary rather than on s_bounded's own doc comment, and the wrong doc run yields prose with no counted claim in it, so it failed loudly instead of comparing nothing.

One more thing the prose was carrying alone

The build-time assertion CompressionPhaseGuardMinutes * 60 < HeaviestHourlyRefreshObservedCeilingSeconds still passes at 594 (420 < 594). So it does not protect the exclude-whole decision at all: the reasoning that no minute of the heaviest slot is guard-recoverable was carried by the doc comment with nothing asserting it. That is not fixed here — fixing it means re-deriving #3035 — but it is now stated where the next reader will find it, and it is on #3069.

CI

Round two on c25b262be was fully green: all 8 checks, including the Windows build at 8m15s, Darling PostgreSQL tests at 4m13s and Darling whole-tree guards. Round one's only failure was the census entry, now fixed. Round three is running on 1811b3a66.

The snapshot enumeration, and the scope rule it taught

A third snapshot reading (286 s, at the 04:20Z self-metrics snapshot, total_runs 331→332) arrived while this PR was in review, and it made one sentence wrong: "Two further readings of 342 s and 348 s come from the hourly self-metrics snapshot". Unlike the ten-run series — which is scoped to a window that has ended and so cannot be falsified by a later run — that sentence carried no scope and read as a complete enumeration of a set that grows every hour this job runs. A frozen enumeration wearing a complete one's clothes, which is #3072's defect and the thing this PR's own pin exists to prevent.

Fixed by deleting the count, not by writing "Three" — #3073's delete class. The paragraph's real subject is the inadmissibility rule: the self-metrics snapshot records last_run_duration with no status column while HeaviestRefreshRuntimeSql and JobCadenceReadSql both filter last_run_status = 'Success', so no reading from it may set this constant. That is a statement about the source and does not go stale. The readings stay as illustration, scoped.

The pin had the same defect with a test wrapped around it. Its pattern was readings of ([0-9]+) s and ([0-9]+) s come from — the arity was the count, encoded in a regex. It now captures the whole list however long. Proven both directions: appending a fourth reading and cutting back to one both stay green, while folding a status-verified series value into the list reds on disjointness.

And the scope has to CLOSE the population, not merely date it. "The readings so far" carries a scope and rots anyway, because a doc comment has no timestamp of its own for a relative phrase to be read against. The prose says "the complete set of snapshot readings up to 04:20Z" — a window that has ended — and the pin demands the closing preposition with an absolute stamp. Red-proofed: replacing it with a relative scope reds, and dropping it entirely reds.

The scope marker is checked for presence, not value. The timestamp is evidence; pinning it to a derived quantity would be inventing a bound this constant does not have.

The provenance correction is now corroborated by contact, not only by absence

An overlap did occur: the refresh ran 03:52:17→03:57:03 and a compression policy fired at 03:52:57, forty seconds into it. Contact made the refresh faster, not slower — 286 s, the fastest reading of its tracked series. So the one mechanism that could have pushed the real post-boundary maximum back toward 864 s and retroactively justified that number did not occur under direct contact. 594 s stands as the post-boundary maximum.

Carrying the bound honestly: the compression run was 0.9 s, which is consistent with compressed quickly without contention or with found little eligible at 04:00 on a Sunday, and last_run_duration alone cannot separate them. That ambiguity is on the compression side and does not weaken the refresh-side measurement, which is direct. Durations and run counts are observed; start times are derived from the finish-to-start rule.

Re-verified after origin/dev moved, and one credit correction

origin/dev advanced to 0f888e90c (#3075) while this was in review, and it touched the same file — CommentFilterAdoptionTests.cs — with git merge-tree reporting zero conflicts, which is the case worth checking rather than trusting. #3075's edit is confined to that class's summary (the build.yml filter paragraph); this branch edits s_bounded's own doc comment and the map below it, so they do not overlap. origin/dev is merged in here so CI runs the combined tree rather than two halves.

Every count re-derived after the merge, not assumed: 4 COLLECTS + 1 stated bound + 1 SQL + 1 demonstrator = 7 map keys; the drift sweep still at 45 injections; all four local harnesses green on the merged tree. Worth noting the check that matters is the pin itself — a grep for COLLECTS now returns 5, because it also matches s_kinds' own label table, which is exactly why the count is derived from the map in code rather than counted by eye.

Credit correction: the CommentFilterAdoptionTests "Two → Four" fix repaired a pre-existing stale count, not an adjustment for this PR's entry. Three entries already collected a doc run when the prose said "Two". Adding the fourth made it wrong by two instead of one, but the drift predates this branch.

One thing left for #3075's lane rather than taken here. That PR's new summary text says CrossAppGuardCiGateTests "does see the two Lite.Tests keys below" — a fresh counted claim of exactly the kind this PR pins, one doc run away from the one now pinned, and derivable (count map keys prefixed Lite.Tests/). It is correct today. It is not pinned here because it is another lane's just-landed prose and the census pin deliberately covers s_bounded's doc run only; flagging rather than silently extending.

The two-kinds numeral policy, written down where the second implementation of it lives

This reasoning has now been derived from scratch twice by different readers, and both times it lived only in a review thread. It is repo policy and it is sharper than anything either #3072 or #3073 states individually:

A numeral that restates an adjacent list gets deletion offered in its failure message — dropping the sentence is as correct as correcting the figure. A numeral that is program output gets "fix the pattern rather than the count", because a transcript has to keep showing what the tool really prints.

ReadmeDerivedCountPinTests implements both halves and states neither, so the rule has to be inferred from the disagreement between two of its own failure texts. Three sentences now sit in this class's summary, citing both issues, naming which kind the thirteen patterns here are — derived arithmetic restated in prose, hence the deletion escape throughout — and marked explicitly as narration of an existing rule rather than a counted claim, so nothing pins it. A pin on a sentence about pinning is where this stops being useful.

A stale justification in this PR's own new file, and why it was re-pointed rather than deleted

Review found RefreshCeilingProvenancePinTests.cs:97-99 justifying its ASCII-only patterns by a \S dash stand-in — a mechanism deleted three commits earlier, when DocProseFor gained dash normalisation. Verified before fixing: zero of the thirteen patterns use \S, and zero carry a non-ASCII character; the one literal hyphen, in ([0-9]+)-second slot, is a plain ASCII hyphen in the prose.

That is exactly the defect this file exists to catch — a comment claiming a mechanism the code no longer implements — occurring one level up, in the guard's own commentary, about its own patterns. Conceded rather than argued.

Re-pointed, not deleted, because only the reason was wrong. The patterns are still ASCII-only on purpose; what makes that possible is DocProseFor normalising — and – to a plain hyphen before any pattern runs, so none of the thirteen has to match a dash variant. Deleting the sentence would have lost a true and load-bearing fact.

Corrected and enforced, since the replacement sentence makes a checkable claim and leaving it as prose would repeat the mistake more slowly. NoPattern_NeedsToMatchADashVariant_WhichIsWhatTheNormalisationBuys asserts no pattern carries a non-ASCII character, and drives the real DocProseFor over an arranged doc run so the normalisation claim has something behind it. Red-proofed both ways: an em dash injected into a pattern reds, and neutering the em-dash normalisation reds — the latter caught by this test alone.

The asymmetry this is the second instance of. CommentFilterAdoptionTests caught a new member of its census while nothing caught its own summary drifting; these pins watch TimescaleSupport.cs's doc run while nothing watches this file's own prose. The boundary is probably right — pinning a test file's commentary is where this stops paying — but two instances make it a pattern rather than an incident, and it is named in the lane report rather than built, because the honest scope of this fix is one sentence and one assertion.

Proportionality, and the two patterns that did not earn their place

Review's second concern was the ratio: ~1,054 lines of regex-driven prose verification against a doc comment on a constant whose value does not change. Legitimate, and the second concern on this axis — the first produced 122 → 96 anchor words. Applying the reviewer's own framing to a re-audit produced a real cut: 13 → 11 patterns, 94 → 69 anchor words, 45 → 37 injections.

The ratio conflates two things. The coupling to prose is 11 patterns, mean 6.3 anchor words each. The rest — the injection sweep, the ordinal source-rewrite, Verify's consumption check, the anti-truncation guards, and 177 lines of doc comment — is what makes the pin trustworthy, and none of it needs editing when the prose changes; a copy-editor touches at most one pattern per reworded sentence. 337 of the 844 lines are comments and blanks.

Dropped as incidental: two patterns pinning seven captures of arithmetic that explained why #3069's ~347 s median is not reproducible from its own published rows. That figure has no referent in this codebase, nothing downstream depends on it, and the reading it contrasted with is already pinned against the series. The derivation is out of the prose and both pins are gone with it; ~347 s remains a bare quotation — the same category as the snapshot readings, correctly unpinned. A third pattern captured the clean count for the third time; that capture is gone too.

The 11 that remain each derive a figure from the constants or from the published series, and each was wrong or unprovenanced in the version this PR replaces: the series itself (must contain the constant exactly once), end − start == 864, end − boundary == 1, stated count vs rows listed, the five clean-series statistics, Slot − max / Slot − Ceiling twice over, Ceiling/Slot/margin, Ceiling / 60 and RefreshPhaseStepMinutes, the series minimum, and the snapshot readings' disjointness and closed scope.

Bound on the claim, because overselling the pin would be the same mistake as the \S sentence. The constant's value is not what this guards and is not unguarded either — TimescaleSupportTests carries the build-time envelope assertion and peak.ClearOfSlotSeconds independently pins 306. This pin guards the provenance narrative, which is what nothing else could guard and precisely what #3069 was filed about.

Coverage re-proved after the cut, each red then reverted: middle reading 335 → 336; clearance 306 → 305; constant 864 → 870; a non-extremal series reading 222 → 223, caught only via the stated mean; margin 36 → 63.

[claude]

Reviewed the diff (doc-comment rewrite on HeaviestHourlyRefreshObservedCeilingSeconds in TimescaleSupport.cs, plus the new RefreshCeilingProvenancePinTests.cs). Summary:

Correctness — Manually re-derived every figure the new doc comment states (10-run series, 9-run clean series after setting the transition run aside, min/max/mean/median, slot clearances at 900 s, the 594↔306↔36 downward-edit consequence, the 13:30:00→13:44:24 = 864 s clock arithmetic, and the #3069 median discrepancy) against the actual TimescaleSupportTests/pin-test logic — everything is internally consistent, no arithmetic or off-by-one bugs found. TimescaleSupport.HeaviestHourlyRefreshObservedCeilingSeconds itself is unchanged (still 864), so the build-time slot assertion (< RefreshPhaseStepMinutes * 60) and RefreshSlotWarningSeconds/CompressionPhaseMinutes derivations are unaffected.

Lite/Darling parity — No parity concern. TimescaleSupport.cs is TimescaleDB/PostgreSQL-specific (compression/refresh scheduling grid), which only exists on the Darling side; Lite's DuckDB storage layer has no equivalent construct, so there's nothing for Lite to keep in sync with here.

Security / SQL style — No SQL, no user input, no new external I/O beyond File.ReadAllText on a [CallerFilePath]-resolved path within the repo (test-only, not attacker-controlled). Not applicable: T-SQL style conventions (nothing SQL changed).

Performance — Test-only change; no runtime/collector code paths touched.

Left one inline note on the new test file about the maintenance cost of tightly regex-pinning free-form doc-comment prose (13 patterns that must match near-verbatim wording) — not a bug, but worth considering for how costly future wording-only edits to that comment will be.

[claude]

Review

Went through the full diff (TimescaleSupport.cs doc-comment rewrite, the new RefreshCeilingProvenancePinTests.cs, and the CommentFilterAdoptionTests.cs census update).

Scope check: this touches only HeaviestHourlyRefreshObservedCeilingSeconds's doc comment and adds drift-guard tests. The constant's value (864) is unchanged, and no production logic changed — this is comments + tests only.

Correctness: hand-verified all the arithmetic the new doc paragraph and the pin tests assert:

• 13:30:00→13:44:24 = 864 s, matches the constant.
• Boundary 13:44:23 is exactly 1 s before the run ends — the transition-run claim checks out.
• Clean series (dropping the 864 s reading) = [194,222,225,335,594,465,359,293,355]: min 194, max 594, mean 338, median 335 — all match the stated figures.
• Slot clearance: 900−594=306, 900−864=36 — matches.
• 347 is indeed the midpoint of 335/359, and the "8 of 10 vs 9 clean" arithmetic is internally consistent.
• 864/60 = 14.4 exactly (864×10 mod 60 == 0), matching the CompressionPhaseMinutes occupancy sentence.

RefreshCeilingProvenancePinTests.DocProseFor correctly handles this file's CRLF line endings (trims \r before checking the /// prefix), so the doc-run walk and <summary>/</summary> anti-truncation checks should behave as intended.

The CommentFilterAdoptionTests.cs census update is consistent — the new file's line.StartsWith("///", ...) call is a genuine comment-prefix site needing an entry, and the "Two"→"Four" count fix matches the actual number of COLLECTS a doc run entries in the dictionary (4).

Lite/Darling parity: no concern. TimescaleSupport.cs is Darling/PostgreSQL-TimescaleDB-specific (continuous aggregate refresh policies); Lite has no DuckDB equivalent of this concept, so there's no counterpart file that needed a matching change.

Security/performance: no input handling, SQL construction, or hot-path code touched; the added tests do in-memory regex/string work only (reading TimescaleSupport.cs once via [CallerFilePath]), so no meaningful runtime cost.

No bugs found. Style follows this repo's established (unusually verbose) doc-comment convention already used in DocCommentHygieneTests.cs/AnalysisPassTokenThreadingTests.cs, and CONTRIBUTING.md's "comments explain WHY, at length" guidance.

[claude]

Reviewed. This is a doc-comment + test-only change to TimescaleSupport.cs (Darling/Postgres side) — no production code paths change, and the constant stays 864.

Verified by hand, independent of the new pin test:

• Clock arithmetic: 13:30:00 → 13:44:24 = 864 s; boundary 13:44:23 is exactly 1 s before the run ends.
• Clean series (864 set aside): 194, 222, 225, 335, 594, 465, 359, 293, 355 → min 194, max 594, mean 338 (sum 3042/9), median 335. All match the prose.
• Slot math: 900 - 594 = 306, 900 - 864 = 36, 864/60 = 14.4 of 15 minutes. All match.
• 347 = (335+359)/2, and both are members of the clean series — the "8 of 10 vs 9 clean" explanation for why HeaviestHourlyRefreshObservedCeilingSeconds is one sample of a twelve-hour-old regime, and #3035's grid plus a build-time assertion are sized against it #3069's median doesn't reproduce is internally consistent.
• The "Two collect a doc-comment run" → "Four" fix in CommentFilterAdoptionTests.cs now matches the actual count (4 COLLECTS-prefixed entries in s_bounded after this PR adds the RefreshCeilingProvenancePinTests.cs entry).
• Summed the capture groups across all drift-swept Pins() by hand: 6+3+1+10+5+3+1+6+3+1+3+3 = 45, matching the PR description's "45 injections, all caught" claim.

Lite/Darling parity: no drift here. TimescaleDB compression/refresh scheduling is Darling-only (Postgres/Timescale) per CONTRIBUTING.md's architecture section — Lite uses DuckDB and has no equivalent concept, so no counterpart change is expected or missing.

T-SQL style guide: not applicable — no SQL in this diff, only C# doc comments and xUnit tests.

Security/perf: none — no I/O, no external input, no new runtime code paths; the new test file only parses/asserts against the checked-in source at test time via [CallerFilePath].

No correctness bugs found. Nice touch pinning HeaviestRefreshRuntimeSql/JobCadenceReadSql (status-filtered) against StoreSelfMetrics.BackgroundJobInsertSql (unfiltered) with a red-proofed two-directional predicate — that's the one place a copy-paste of the wrong SQL string would have silently broken the exclusion rationale, and it's covered.

[claude]

Review

Scope: doc-comment-only changes to TimescaleSupport.cs (no runtime/query logic touched, HeaviestHourlyRefreshObservedCeilingSeconds stays 864), plus two new/expanded test files (RefreshCeilingProvenancePinTests.cs, CommentFilterAdoptionTests.cs) that regex-parse the doc prose to keep its narrative numbers self-consistent.

Correctness — Verified the arithmetic the new doc comment publishes against the pin tests by hand:

• Series 864, 194, 222, 225, 335, 594, 465, 359, 293, 355; excluding the transition run (864) leaves 9 readings, min 194 / max 594 / mean 338 / median 335 — matches the stated "remaining 9 run from 194 s to 594 s, mean 338 s, middle reading 335 s".
• Slot clearance: 900 - 594 = 306, 900 - 864 = 36 — matches.
• Median-discrepancy explanation: (335+359)/2 = 347 — matches the "347 is the midpoint of 335 and 359" claim, and 8/10 vs 9/10 clean-count math checks out.
• Clock arithmetic: 13:30:00→13:44:24 = 864s; boundary 13:44:23 is exactly 1s before the run ends — matches.
• 864 / 60 = 14.4 exactly (exact tenth of a minute) — matches the "occupies 14.4 of the 15 minutes" claim in CompressionPhaseMinutes's doc comment.
• Confirmed against the shipped SQL: HeaviestRefreshRuntimeSql and JobCadenceReadSql both filter last_run_status = 'Success'; StoreSelfMetrics.BackgroundJobInsertSql has no status column at all — matches the inadmissibility claim in the doc comment.
• CommentFilterAdoptionTests's census fix ("Two" → "Four" collect a doc-comment run) is also correct: counting the s_bounded map by kind gives COLLECTS=4, STATED BOUND=1, NOT C#=1, DEMONSTRATES=1, summing to the map's 7 entries.

No bugs found in the pin/verification logic itself (ordinal-based number addressing, width handling on digit-count-changing bumps, anti-truncation <summary>/</summary> checks, drift-sweep non-vacuity counting) — it's self-checking in several places (Assert.Equal(expected, swept), unused-pin detection) which would catch most classes of "the guard silently stopped guarding" mistakes.

Lite/Darling parity — Not applicable here. TimescaleSupport.cs is Darling/PostgreSQL-TimescaleDB-specific (continuous aggregate refresh policies, compression phase grid); Lite has no TimescaleDB and no equivalent construct, confirmed via grep. No counterpart file needs updating.

Security / Performance — None of concern. Test-only file I/O reads the repo's own TimescaleSupport.cs/CommentFilterAdoptionTests.cs via [CallerFilePath] (no untrusted input), and none of this executes at runtime — it's Darling.Tests only.

One observation, not a blocker: this is a heavyweight mechanism (a second ~800-line test file doing regex extraction over doc-comment prose) to keep a single constant's provenance narrative from drifting. It's well guarded against silently going blind, but it does mean every future wording tweak to that doc comment risks needing a matching pattern update in RefreshCeilingProvenancePinTests.Pins(). The PR description notes this tradeoff was already raised and partly addressed in review round two (pattern trimming, <b>/dash normalization at the extractor) — worth keeping an eye on whether this pattern is meant to generalize to other doc comments or stays a one-off for this constant.

[claude]

Reviewed. This PR is doc-comments-and-tests-only — the constant HeaviestHourlyRefreshObservedCeilingSeconds stays 864, no other production logic changes, and it's Darling-only (TimescaleSupport.cs is PostgreSQL/TimescaleDB-specific), so no Lite counterpart is expected and there's no parity drift here.

Verified the arithmetic the new doc comment and RefreshCeilingProvenancePinTests both assert:

• Clean series (864 set aside): 194,222,225,335,594,465,359,293,355 → min 194, max 594, mean 338, median 335 — matches.
• Slot clearance: 900−594=306, 900−864=36 — matches.
• Transition-run clock math: 13:44:24 − 13:30:00 = 864; boundary 13:44:23 is exactly 1s before — matches.
• Occupancy: 864/60 = 14.4 of 15 minutes — matches.
• Drift-sweep total: independently recounted captured numbers across the 13 drift-swept patterns → 45, matching the PR's claimed injection count.
• CommentFilterAdoptionTests' "Two→Four" fix: counted s_bounded by kind (4 COLLECTS, 1 STATED BOUND, 1 NOT C#, 1 DEMONSTRATES = 7) — "Four kinds live here… Four collect" is correct.

One nit posted inline: the new Pins() doc comment justifies "ASCII-only patterns" by saying \S stands in for a dash, but none of the 13 shipped patterns use \S or match across an em/en dash — mildly ironic given this PR's whole point is not letting a comment claim a mechanism the code doesn't have.

No security, correctness, or performance concerns beyond that — this is test/documentation infrastructure with no runtime behavior change.

[claude]

Review summary

Scope check first: this PR touches no T-SQL at all — the only production-code diff is to XML doc comments on TimescaleSupport.cs (the 864 constant's value is unchanged, only its provenance narrative is rewritten), plus two Darling.Tests files that mechanically verify that narrative. TimescaleSupport is Darling-only runtime setup for TimescaleDB (per CONTRIBUTING.md's architecture section — "TimescaleDB conversion is deliberately not on the ladder... applied only when the extension is present"), so there's no Lite counterpart and no Lite/Darling parity gap here.

Correctness: I traced all 13 regex pins in RefreshCeilingProvenancePinTests.cs by hand against the actual doc comment text in TimescaleSupport.cs — the clock arithmetic (13:30:00→13:44:24 = 864s, boundary 13:44:23 = ended - boundary == 1), the series stats (mean 338, median 335, min/max 194/594 over the 9 "clean" readings), the slot-clearance figures (36s / 306s against a 900s slot), and the 14.4/15 minute-occupancy figure all check out and agree with the constants (RefreshPhaseSlotSeconds = 900, RefreshPhaseStepMinutes = 15). I didn't find a logic bug in the extraction/mutation/verification machinery itself (DocProseFor, Numbers, RewriteNumberInDocRun, Verify).

Security / perf: No SQL, no external input, no new file/network/process use beyond reading the test's own source file via [CallerFilePath] (a fixed relative path, not attacker-influenced). No performance-sensitive code touched — this is test-only + doc-comment.

The one thing worth raising (left as an inline comment on Pins()): this is a very large amount of machinery — 844 new lines plus a 210-line addition to CommentFilterAdoptionTests.cs — built to keep a comment's prose arithmetically self-consistent, using regex patterns hand-tied to exact wording. That's a real, ongoing maintenance coupling: future wording edits to this comment (even ones that don't touch a figure) have a good chance of red-lining CI until the corresponding pattern is updated too. The failure messages do the right thing by explicitly sanctioning "update the pattern" as a legitimate fix rather than treating every red as "the prose is wrong," which takes the edge off, but it's worth maintainers being deliberate about accepting this trade-off given the change being protected has no runtime behavior difference.

Nothing here blocks the PR from my read — flagging the complexity/maintenance-surface point for the team's awareness rather than as a defect.

[claude]

Reviewed this PR against CONTRIBUTING.md conventions, Lite/Darling parity, security, correctness, and performance. Summary: no issues found.

Scope. All three files are Darling-only (TimescaleSupport.cs doc comment + two new/updated test files). TimescaleSupport is Timescale/PostgreSQL-specific with no Lite counterpart, so there's no parity drift to flag here — Lite doesn't need a mirrored change.

Correctness — verified by hand, not just read:

• Traced every one of the 11 regex pins in RefreshCeilingProvenancePinTests.Pins() against the actual (post-edit) doc comment text in TimescaleSupport.cs — each pattern matches the prose exactly as written (including after DocProseFor's <b>-stripping and dash normalization).
• Re-derived the arithmetic independently: clock spans (13:30:00→13:44:24 = 864s; boundary 13:44:23 is exactly 1s before end), the clean series [194,222,225,293,335,355,359,465,594] (sum 3042, mean 338, median 335), slot clearance (900−594=306, 900−864=36), and occupancy (864s = 14.4 of 15 min). All match what the constants (RefreshPhaseSlotSeconds=900, RefreshPhaseStepMinutes=15, RefreshSlotWarningSeconds=750) and the published series actually produce.
• Confirmed the SQL-provenance claim in TheInadmissibilityClaim_MatchesTheShippedSql against the real strings: HeaviestRefreshRuntimeSql and JobCadenceReadSql both filter last_run_status = 'Success'; StoreSelfMetrics.BackgroundJobInsertSql has no status filter — matches the doc comment's claim exactly.
• Confirmed the CommentFilterAdoptionTests census counts: 4 COLLECTS + 1 STATED BOUND + 1 NOT C# + 1 DEMONSTRATES = 7 map entries, matching the corrected "Four ... collect a doc-comment run" text.
• All cross-referenced members (HeaviestRefreshRuntimeSql, JobCadenceReadSql, StoreSelfMetrics.BackgroundJobInsertSql, CompressionPhaseMinutes, etc.) exist and are public, so the new test file compiles against real symbols rather than something renamed out from under it.

Security/perf. No new I/O beyond File.ReadAllText on the test's own source file via [CallerFilePath] (not attacker-controlled), no new SQL text, nothing on a runtime hot path — this is test/doc-comment only.

No inline comments filed; nothing rose to the level of a defect.