[erikdarlingdata]

Closes #2506.

#2504 anchored 57 Darling reads and 50 Lite reads and deliberately left four out — the four an incident investigation reaches for first. This finishes them.

What is anchored

All four, with the same parameter name, semantics and description text as the other 107, from the same McpHelpers.AsOfDescription constant. Verified byte-identical across SKUs including the em dashes.

Tool	What the anchor moves
get_analysis_facts	fact collection + scoring run over the anchored window
analyze_server	the same, plus the anomaly detector's hour-of-day × day-of-week baseline
compare_analysis	both windows — baseline_hours_back has always been measured from the comparison window's end
get_analysis_findings	a window on analysis time: what a scheduled pass was saying then

The window lives in the engine, so the anchor goes into the engine

The issue's framing is right: these tools hand a bare hours_back to AnalysisService / DarlingAnalysisService, which builds the window itself. I threaded it rather than leaving a subset unanchored, because the engine turned out to be already window-driven end to end — every bound in AnomalyDetector/PgAnomalyDetector and every baseline lookup already comes off context.TimeRangeStart/End. Anchoring the context is therefore enough to make the anomaly comparison correct for a past window, which is the part that actually matters: the issue notes that a run pinned to now compares Tuesday 03:00 against whatever baseline bucket today is in.

AnalyzeAsync, CollectAndScoreFactsAsync and GetRecentFindingsAsync take an optional asOfUtc on both SKUs. Null is every existing caller and is the pre-change window exactly:

• DarlingWorker's scheduled pass and Lite's CollectionBackgroundService
• Lite's Recommendations tab (RecommendationsTab.xaml.cs)
• the Darling WPF viewer (ViewerDataService.GetLatestFindingsAsync — untouched)
• LiteRecommendationsReader, which calls the store directly
• the alerting path, which consumes AnalyzeAsync's return value, not a store read

Two shared-path changes worth naming:

1. HasBaselineData's 30-day gate moved from DateTime.UtcNow.AddDays(-30) to context.TimeRangeEnd.AddDays(-30), on both SKUs. Identical for an unanchored pass (TimeRangeEnd is now), and the only form that agrees with the baseline provider — which reads the 30 days before context.TimeRangeStart — when the pass is anchored. It was the last clock-derived bound in that class.
2. GetRecentFindingsAsync gained an UPPER bound on analysis_time (LIMIT $3 → $4 on Darling; the pin moved with it). The read had a start and no end for its whole life. Without the bound an anchor could only move the window's start earlier and every anchored read would still have returned everything up to now — validated, advertised, and answering as of the present. The bound binds only when the caller anchored — see below.

The upper bound is anchored-only, and CI is why

The first push bounded it at "now" unanchored too, and two existing live tests went red — one per SKU (DarlingMcpToolsTests.EndToEnd_FindingsEnvelope_MuteFlow, McpAnalysisFindingsCommandTests.GetAnalysisFindings_CollapsesRepeatedChains), both planting a finding stamped 30 minutes ahead of now to exercise occurrence collapsing. They were right and I was wrong:

• It broke Anchor a windowed read at a past incident, not only at now (#2495) #2504's central promise that a caller sending only hours_back gets byte-for-byte the window it always got.
• And the mechanism is not synthetic. analysis_time is stamped by the writer's clock and would be filtered by the reader's. They are the same host today; the day they are not, a bounded default read drops the run written a moment earlier — a findings read that "sometimes misses the analysis that just finished", with nothing in it to point at a clock.

So unanchored the upper bound is an unreachable sentinel (NoUpperBound) and the read stays half-open; anchored it is the caller's own instant. Neither existing test was touched. A test on each store now asserts both halves — the default read returns an ahead-of-now row, the anchored read does not — so the sentinel cannot be tidied away by someone who reads it as dead code.

What analyze_server does with an anchor, and why

It accepts the anchor and refuses to persist the run. Option 2 in the issue, and the issue author's own inclination — with the rule made derived rather than a flag.

AnalysisContext.PersistFindings is AsOfUtc is null. An anchored pass writes no row and raises no AnalysisCompleted event; the findings come back in full. There is no legitimate caller for "anchored and persist", so there is no way to express it — including for the next caller, who will not have read the argument.

The reasoning, which is about analysis_time and not about the window:

• A finding row's identity, for every consumer we have, is analysis_time — the moment the pass ran, not the window it looked at.
• GetLatestFindingsSql keys on MAX(analysis_time), and that read is what the Darling viewer's Recommendations tab (MainWindow.xaml.cs:1582) and Lite's tab show as this server's current state. Persisting a backdated run would make last week's findings the current headline for every human looking at the viewer — an out-of-band change to what other people see, caused by one agent's exploratory read.
• get_analysis_findings filters on analysis_time and then collapses on (story_path_hash, incident_id) to produce occurrences / first_seen / last_seen / peak_severity. A backdated run's rows land in the same group as a live incident's and inflate exactly the stats an operator uses to decide whether it is getting worse.
• The event is gated with the insert for the same reason and not a weaker one: it is how findings reach AnalysisNotificationService. An alert about last Tuesday delivered today is the persistence problem with a shorter fuse.

On option 3 ("anchor and persist, with the window recorded on the row"): the issue says it needs a migration rung and a reader change. It does not — analysis_findings already carries time_range_start / time_range_end on both stores and get_analysis_findings already returns them as time_range. That is what makes option 3 the wrong answer rather than merely the expensive one: the window is on the row today and it does not help, because no consumer filters or orders on it. Making them do so is a separate, larger product change that would also alter what the scheduled path means, and it should be driven by someone actually wanting the persistence.

The refusal is explicit, not silent: every analyze_server result carries persisted and persistence_note (null on the ordinary run), both instructions files say it, and the tool description says it.

What I deliberately did not anchor

• get_pvs_stats, get_fleet_overview — out of scope per the issue, and for a more permanent reason: each mixes a latest-snapshot measurement with a windowed one, so anchoring half the answer dates the two halves differently. They keep their own bullet in the docs.
• get_store_metrics — windows in days over the store's own growth series. Already excluded; unchanged.
• audit_config — it calls CollectAndScoreFactsAsync with a hardcoded 1-hour window, but it takes no hours_back and is a current configuration audit; config history has its own anchored tool (get_server_config_changes). Anchoring it would mean inventing a window parameter the tool has never had. Left alone on purpose rather than by oversight.

Proving the anchor reaches the query

#2504's review found eight tools that advertised as_of, validated it, and then queried DateTime.UtcNow. This family can fail the same category one seam over, and none of the existing checks can see it: a tool that resolves the anchor and then calls AnalyzeAsync without it still contains windowEnd (from its own out var) and still never names DateTime.UtcNow — so it passes every line of the existing pin while answering as of now.

EveryToolThatTakesTheAnchor_ActuallyUsesIt gained an analysisService. arm, mirroring the dataService. one, with the anchorable method set read off both SKUs' shipped sources and asserted non-empty. Proved red before green: stripping asOfUtc: anchor flags all six tools across both SKUs, each with reaches=true — i.e. exactly the cases nothing else would have caught.

One subtlety that is load-bearing and easy to get backwards: the tools pass null, not the resolved windowEnd, when as_of was absent. ValidateWindow hands back "now" for an absent anchor, and passing that through would have made every ordinary run look anchored to the engine — which would have quietly stopped analyze_server persisting anything, ever.

Tests

• Darling.Tests/AnalysisAsOfAnchorTests.cs — ungated rules, plus a [Collection("live-postgres")] end-to-end on the pipeline fixture's own recipe (a >24h wait history in one hour×dow bucket, then a heavy window the following week). Anchored: facts score, analyze_server finds the ANOMALY_WAIT_PROFILE story, reports persisted:false, and leaves analysis_findings at zero rows. The same window unanchored produces findings and rows — so the zero is the anchor's doing and not a fixture with nothing to say. The findings read then returns a planted 30-hour-old pass when anchored and only the now-stamped rows when not, finding_count == 1, which is what proves the new upper bound is doing work. Cleanup goes through LiveStoreCleanup.RunAsync.
• Lite.Tests/AnalysisAsOfAnchorTests.cs — same claims on DuckDB, seeded under the derived server id (RemoteCollectorService.GetDeterministicHashCode(GetServerNameForStorage(...))), including the persistence A/B on an identical window and the refusal path (a bad anchor must not fall back to now and then run a real, persisting analysis).
• Every negative assertion has a positive control beside it, so none of them can pass by the read simply returning nothing.

What I could not verify

• The test suites did not run locally. Darling.Tests and Lite.Tests are net10.0-windows; they build on macOS with EnableWindowsTargeting and cannot execute. CI is the arbiter. The whole solution builds clean with no new warnings.
• The live Postgres test cannot run locally — DARLING_TEST_PG is not set here, so its window arithmetic and the anomaly firing on the anchored window were argued from the existing DarlingAnalysisPipelineTests fixture rather than observed. CI has now executed it (it caught a baseline_hours_back over the 168-hour cap on the first run).
• The EveryToolThatTakesTheAnchor_ActuallyUsesIt change was verified with a Python replica of the C# scan, run against the shipped sources both with and without the fix. The replica agrees with the C# by construction (same regexes, same paren matcher), but it is a replica.
• No anchored analysis has been run against a real store. The claim that a past window produces useful findings rests on the engine already being window-driven, which I read rather than measured.

[claude]

baseline_hours_back: 169 exceeds McpHelpers.MaxHoursBack (168, "7 days"). CompareAnalysis validates it with the plain McpHelpers.ValidateHoursBack(baseline_hours_back) (unmodified by this PR), which returns an error for any value > 168:

if (hoursBack > MaxHoursBack)
    return $"hours_back value '{hoursBack}' exceeds maximum of {MaxHoursBack} hours (7 days). Use a smaller value.";

So this call (and the control call on line 244) will return the validation-error string instead of the comparison payload, and the two Assert.Contains(...instant..., compared, ...) checks right below will fail against that error text — not the intended "both windows hang off the anchor" assertion. This is a real (albeit gated behind DARLING_TEST_PG) test breakage, not a hypothetical: the fixture needs baseline_hours_back == 169 (7 days + the 1‑hour offset baked into incidentEnd) to land the baseline on historyStart, but 169 can never pass validation as written.

Worth fixing the fixture's arithmetic (e.g. drop the extra hour so the anchor sits exactly 168h after historyStart, or otherwise keep baseline_hours_back <= 168) rather than relying on a value the tool will always refuse.

[erikdarlingdata]

Correct, and CI hit it on the same run — the live test failed with hours_back value '169' exceeds maximum of 168 hours. Fixed in 7882a9f.

Not by the route suggested, though: dropping the extra hour would put the anchor at incidentStart, and with hours_back = 1 the window becomes [incidentStart - 1h, incidentStart], which excludes the three incident collections at +5/+10/+15 minutes. The extra hour is load-bearing — the window has to START at incidentStart so the anomaly baseline is looked up in the same hour-of-day bucket the history fills.

So it uses 168 and the baseline lands an hour past the history rows. That does not weaken the assertion, because the assertion is on the two rendered instants rather than on the comparison having data: baseline_start and comparison_end are present in the data-bearing shape, the caveat shape, and the unavailable envelope alike. The claim under test is that both bounds moved with the anchor, and the unanchored control below it is what makes that claim non-vacuous. Comment updated to say so.

[claude]

Reviewed the diff. The engine-side anchor plumbing (Lite AnalysisService/AnomalyDetector/FindingStore and Darling DarlingAnalysisService/PgAnomalyDetector/PgFindingStore) is well-mirrored between the two SKUs — same signatures, same AsOfUtc/PersistFindings derivation, same upper-bound fix on GetRecentFindingsSql, same HasBaselineDataSql gate now keyed off context.TimeRangeEnd instead of the clock. Instructions text (DarlingMcpInstructions.cs / McpInstructions.cs) stays byte-parallel apart from the expected Darling-only fleet reads. AnalysisContext.PersistFindings being a derived read-only property rather than a settable flag is a good call given the stated "no legitimate caller for anchored+persist" invariant, and it's covered from both directions in the new tests.

One concrete bug found, left as an inline comment: in Darling/Darling.Tests/AnalysisAsOfAnchorTests.cs, the live-Postgres test calls CompareAnalysis(..., baseline_hours_back: 169, ...), but McpHelpers.MaxHoursBack is 168 and ValidateHoursBack rejects anything over that — so the call returns a validation-error string instead of the comparison payload, and the assertions checking for the anchored instants in the response will fail against that error text. The fixture's 7-day-plus-1-hour offset is what pushes it to 169; it needs adjusting to stay within the 168-hour cap (gated behind DARLING_TEST_PG, so this won't surface in ordinary CI, but it will fail the moment that env var is set).

No SQL-injection, secrets, or missing-index concerns — everything here is parameterized ($N / DuckDBParameter), and no T-SQL files are touched by this PR (the changed SQL constants are Darling's Postgres and Lite's DuckDB dialects, not subject to the T-SQL style section).

[claude]

Review: Anchor the analysis family at a past window (#2506)

Reviewed the full diff (CHANGELOG, AnalysisContext, both AnalysisService/DarlingAnalysisService, both AnomalyDetector/PgAnomalyDetector, both FindingStore/PgFindingStore, both MCP tool/instruction files, DarlingWebEndpoints, and both new AnalysisAsOfAnchorTests suites) against correctness, Lite/Darling parity, and security.

Correctness

• AnalysisContext.PersistFindings => AsOfUtc is null is threaded consistently into both AnalyzeAsync implementations, gating both the InsertFindingsAsync write and the AnalysisCompleted event together — no path where one fires without the other.
• The new upper bound on GetRecentFindingsSql/Lite's inline equivalent is correctly conditional (NoUpperBound sentinel when unanchored, windowEnd when anchored), preserving the half-open-window behavior for the scheduled/alerting paths while fixing the "anchor only moves the start" defect for anchored reads. Parameter ordering ($1..$4 vs $1..$3) matches between SQL text and bound values on both SKUs.
• Verified the callers that must stay unanchored (DarlingWorker.RunAnalysisPassAsync, Lite's CollectionBackgroundService, RecommendationsTab.xaml.cs) all call AnalyzeAsync without asOfUtc, and the appended-parameter placement (with the CA1068 justification) doesn't shift any of their positional arguments.
• HasBaselineDataAsync's 30-day gate now correctly derives from context.TimeRangeEnd (== "now" when unanchored) instead of the wall clock, matching the baseline provider's own window math.
• compare_analysis correctly hangs both comparisonEnd/comparisonStart and baselineEnd/baselineStart off windowEnd, and doesn't need the anchor-vs-windowEnd null distinction the other three tools need, since ComparePeriodsAsync never persists and never touches the anomaly detector's baseline gate.
• GetAnalysisFacts/GetAnalysisFindings/AnalyzeAsync all correctly pass anchor (null when as_of absent) rather than the resolved windowEnd, which is what keeps PersistFindings true for ordinary unanchored runs — confirmed this distinction is preserved on both SKUs.

Lite/Darling parity — no drift found. AnalysisContext, the PersistFindings derivation, the HasBaselineDataAsync signature change, the NoUpperBound sentinel and its rationale, the MCP tool descriptions/instructions text, and the new test suites are all mirrored line-for-line in intent between the two SKUs. analyze_server is correctly absent from Darling's /api/read/* web catalog (it's a write path), while get_analysis_facts/compare_analysis/get_analysis_findings correctly gained PAsOf() in both CatalogDescriptors and BuildReadDispatch. Lite has no equivalent web catalog to update (WPF-only), which is pre-existing asymmetry, not new drift. The EveryToolThatTakesTheAnchor_ActuallyUsesIt scan's new analysisService. arm is scanned once across both SKU directories, matching the existing dataService. arm's cross-SKU design.

Security — all new/changed SQL uses positional parameter binding ($1, $2, ...) on both Npgsql and DuckDB.NET; no string concatenation of caller-supplied values into query text. as_of refusal (unparseable / future) reuses the existing ValidateWindow convention rather than introducing new parsing. The one place raw string interpolation appears (DeleteRowsAsync in the new Darling test, building DELETE ... WHERE server_id = {ServerId}) is test-only cleanup code where ServerId is a compile-time-deterministic hash of a hardcoded constant, not attacker-reachable input — not flagging as a vulnerability.

No correctness bugs, parity gaps, or security issues found. Nothing to request changes on.