Manage Servers shows the service's version, not the viewer's, as the installed version (#5655)

[erikdarlingdata]

Fixes #5655

Why

The Darling viewer's Manage Servers grid had an "Installed Version" column that filled every row with the viewer's own version (InstalledVersion = appVersion). It said nothing about the service, so an old viewer against a newer service looked up to date. The store had no record of the service's version at all.

What changes

• Store, new rung V174. config.service_version, a small table keyed by the service host (machine name), like config.password_key_service. Columns: service_host, product_version, process_started_at (timestamptz), updated_at (timestamptz, default now()). It is not a column of config.config_service, because that row's trg_service_self_bump trigger bumps config_version. Every service then reloads on each start. The rung is one guarded CREATE TABLE IF NOT EXISTS, so a re-run changes nothing. Store schema version is now 174.
• These are deliberately the first timestamptz columns among the stored tables. Every other stored timestamp is a naive timestamp holding UTC, and PgMigrations.cs has no timestamptz. (The only other timestamp with time zone in the schema is the stats_reset column in the result of a function that reads pg_stat_statements_info. It is never stored.) The two new columns hold an absolute instant that the viewer shows in a zone the user picks. The columns are written and read as NpgsqlDbType.TimestampTz with Kind=Utc. The instant round-trips under any session TimeZone, and PostgreSQL keeps microseconds. So the store's zone cannot skew the shown time. A naive column relies on every writer using UTC. Nothing compares these columns with now() in a window, and the upsert's now() is a write. StoreSqlClockDisciplineTests finds naive columns by name and skips timestamptz, so it does not see these two and stays correct for the rest of the schema. Its stated rationale said there was not one timestamptz column in the schema. That stopped being true here. I rewrote that paragraph to name this exception and the reason.
• Grants. No explicit GRANT in the rung, on purpose. The config schema's blanket GRANT SELECT ON ALL TABLES already gives admin and viewer SELECT. So do the owner's default privileges. Both are in DarlingManagedRoles and in tools/provision-roles.sql (same blanket lines, so self-managed stores match). ServiceVersionGrantLiveTests proves both roles can read the table when it existed before provisioning and when it is created after. The web dashboard does not read this table (see below), so it needs no grant of its own.
• Probe sentinel. ViewerDataService.StoreSchemaProbeSql gains to_regclass('config.service_version') as the newest sentinel (ordinal 149), with a hasServiceVersion arm in MapProbedSchemaVersion that returns 174, and the GetBoolean(149) argument.
• Service. At startup, before the config provider's first read, DarlingServiceVersionRecorder upserts this host's row. It writes DarlingCliCommands.ProductVersion() (the same method --version, the diagnostics bundle and MCP get_collection_health use) and the process start time, cut to whole microseconds. Cancellation is not swallowed.
• A failed write is tried again. A write that fails at start logs one warning ("The service tries it again on every sweep") and the service goes on. DarlingServiceVersionWriter holds the write. DarlingWorker calls it again at the top of every sweep until it succeeds, right after the password key's pin step (which is also tried on every sweep while it waits). Later failures log at debug only, like the pin step's repeats, and a write that landed is not repeated. Until it lands the viewer shows unknown (older than 3.11) for this host, which is the same text it shows for a service older than 3.11.
• Viewer. The per-row Installed Version column is removed. Above the grid, Manage Servers shows the service version once, next to the viewer's own. One host reads Service version: 3.11.0 (HOST, started 2026-10-01 08:00 UTC), several hosts read one HOST: 3.11.0, started 2026-10-01 08:00 UTC line each. The start time is UTC, or local time when the viewer is set to local. It shows how old a host's row is: the service writes the row once at startup, so a retired host keeps its old one. No row or no table reads Service version: unknown (older than 3.11). Display only, no warning. The reader (ServiceVersionTable.ReadAsync) catches only SQLSTATE 42P01.
• When the version read fails. If GetServiceVersionsAsync itself fails, the line shows unavailable and the list still loads. The read runs after GetMonitoredServersAsync inside the window's outer try. If that earlier read throws, the window shows its error box. The version line is not assigned at all, so it stays as it was (blank on the first load). I made the code comment say so. The code is unchanged: a blank line next to an error box is the right answer there, and the _dataService is null path blanks it too.
• Web dashboard. Checked, nothing to change. The only "installed version" in the web pages (server-tabs.js, installed_version) is the PostgreSQL extension version per database, from the extension census. It is not the app version.
• Lite. Unchanged: there the app is the collector, so its own version on each row is correct.
• Also fixed in this PR. Removed the dead InstalledVersion property (and the unused INotifyPropertyChanged it was the only user of) from ViewerServerEntry. Updated the neighbouring newest-rung pins, which count rungs or probe ordinals: AwsPerServerRoleRungTests, LegacyPinCandidateRungTests, PlanRegressionDailyRungTests, QueryStoreIntervalPartitionRungTests (rung counts 3 to 4, and the V174 table dropped when a store is rewound), CaggGroupIndexDropLiveTests (drops the V174 table when it simulates an older store), and QueryStoreComposeStampTests / QueryStoreComposeStampLiveTests (the V173 "newest rung" pins).
• Host order test. The reader fact named "in host order" now asserts the order. the two rows go in as ZED then ALPHA (both upper case so the order is the same under any collation) and the read must return ALPHA, ZED, so a lost ORDER BY service_host fails it.
• CHANGELOG. The Manage Servers shows the service's version, not the viewer's, as the installed version (#5655) #5668 entry now also says that a write that fails at start is tried again on every sweep.

Test plan

New tests: ServiceVersionRungTests, ServiceVersionLiveTests, ServiceVersionGrantLiveTests, ServiceVersionTextTests, ServiceVersionWriterTests. Live tests ran on a scratch PostgreSQL 18.6 with TimescaleDB on port 55896 (UTC), stopped and its directory deleted afterwards.

Covered:

• Reader returns the rows, in host order (asserted without re-sorting), with microsecond times kept. It returns none for an empty table and for a missing table. It does not swallow any other error (a login with no access gets 42501).
• Startup write upserts. A second start of the same host updates its one row, and a second host adds a row. config_version does not move. The default overload records this machine's host and ProductVersion(). A failed write returns false and logs a warning. A cancelled write still cancels.
• Retry (ServiceVersionWriterTests): a failed first write is tried again on the next sweep and on each one after, a write that landed is not repeated, a cancelled attempt escapes, the first failure warns and later ones log at debug. ServiceVersionLiveTests.TheWorker_TriesAFailedVersionWriteAgain_OnEverySweep_UntilItLands pins the worker source: the retry sits after the pin step's per-sweep call, inside the sweep loop.
• Rung: dense ladder, one guarded CREATE, no data moved, columns and types on a fresh store. A re-run keeps a row. An upgraded store (table dropped, version rows removed) gets the table back.
• Viewer text for one host, two hosts and unknown. The XAML and code-behind no longer carry the column.
• Probe maps a full store to 174 and a store without the table to 173.

RED (each change made on the finished branch, the named test failed, then reverted):

• column put back in the XAML: ServiceVersionTextTests.TheGrid_HasNoInstalledVersionColumn_AndTheWindowShowsBothVersionsOnce failed
• startup call removed from the worker: ServiceVersionLiveTests.TheWorker_RecordsTheVersion_BeforeTheConfigViewIsBuilt failed
• reader catching every PostgresException: ServiceVersionLiveTests.TheReader_AnswersOnlyTheUndefinedTableError_AnyOtherFailureReachesTheCaller failed
• upsert changed to ON CONFLICT DO NOTHING: ServiceVersionLiveTests.TheStartupWrite_Upserts_ASecondStartUpdatesTheRow_AndEachHostKeepsOne failed
• this round: ORDER BY service_host replaced with ORDER BY 1 = 1: TheReader_ReturnsEveryHostsRow_InHostOrder_WithMicrosecondTimes failed. The worker's retry condition made false && ...: TheWorker_TriesAFailedVersionWriteAgain_OnEverySweep_UntilItLands failed. The writer's "already written" early return removed: ASuccessfulFirstWrite_IsNotRepeated and AFailedFirstWrite_IsTriedAgainOnTheNextSweep_AndNotRepeatedOnceItLands failed.
• the rung, probe and reader tests cannot compile on origin/dev, since the table and classes do not exist there.

Totals on this head (Darling.Tests, build 0 warnings, 0 errors, scratch PostgreSQL):

• *ServiceVersion*, StoreSqlClockDiscipline, RepoFileAdoption, *Rung*, *SchemaVersion*, ViewerSchemaVersionGate: 474 passed, 0 failed, 0 skipped.
• Every *Census*, *Ratchet*, *Hygiene*, *DocComment*, StartupCommandTimeout and StorageCommandTimeout class: 644 passed.
• Stage=Guard: 1481 passed.
• Earlier on the previous head (before this round): the same families plus a wider batch (5733 passed, 13 skipped, 0 failed), and the V173 pin moves took QueryStoreComposeStamp* from 3 failed of 26 to 26 passed.
• Not run: the full suite (CI decides) and Installer.Tests.

Things to check

• The rung number: V174 is the next free number on origin/dev at push time.
• A host that is retired keeps its row, since the service writes only at start (and retries only a failed write), so no age cutoff is safe. It shows as one more line. This PR does not address that.
• admin also holds INSERT/UPDATE/DELETE on config tables through the existing blanket grants, so the table is not owner-only like the password key tables. It is display text only.
• The text unknown (older than 3.11) assumes this ships in 3.11.
• The viewer reads CurrentDisplayMode once per refresh, so switching the display mode while Manage Servers is open leaves the start time in the old zone until the next refresh. Every other time on the grid is bound the same way. Unchanged.

[claude]

Non-blocking. The write is one-shot and its bool is discarded, so a write that fails is never retried for the life of the process — and the line it feeds then says unknown (older than 3.11) about a service that is ≥3.11, until the next restart.

Two things make this worth a line rather than nothing:

• RecordAsync is pinned to return false on failure (ServiceVersionLiveTests.AFailedWrite_LogsAWarning_ReturnsFalse_AndDoesNotThrow), and the only production call site ignores it. A tested return value nobody reads usually means a retry was meant to hang off it.
• The sibling startup store-write right below this takes the other branch on purpose: _pinStepPending at The first start after the schema 165 upgrade takes the legacy password pins before it reads the server list, so saved passwords keep working #5456 is retried on every sweep, with the comment "a pin step that could not run is tried again on every sweep". .github/REVIEW-TRAPS.md names the shape under alerting — "'Did I send' is not 'was it delivered': a once-a-day gate in process memory … never retries a failed delivery (The daily digest and sweep rollup asked 'did I send' when the channel asks 'was it delivered': the once-a-day gate now stamps delivered-today in the store, so a restart re-announces nothing and a failed delivery still retries (#3580) #3633)".

The window is genuinely narrow — PgMigrations.MigrateAsync succeeded a few hundred lines up on the same pooled data source, so the store was reachable and (being the migrating login) owns the table and holds INSERT. A failover landing between the two is the realistic case. Re-attempting from the hourly store-maintenance tick would close it for the cost of one call; the PR body's "a failed write logs one warning and the service goes on" is accurate but silent about the permanence.

[erikdarlingdata]

Claude posting for Erik Darling. Fixed in 4a55317. DarlingServiceVersionWriter holds the write and DarlingWorker tries it again on every sweep (after the pin step's per-sweep call) until it succeeds. The first failure warns, later ones log at debug. ServiceVersionWriterTests pins that a failed first write is retried and a landed one is not repeated.

[claude]

Non-blocking, test-only. This fact is named …_InHostOrder_…, but both sides of the assertion are re-sorted with the same comparer, so it asserts set equality and nothing about order. Drop the ORDER BY service_host from ServiceVersionTable.SelectSql and this still passes.

.github/REVIEW-TRAPS.md under Tests and censuses: "A pin that encodes the lie: ACappedResult_WithholdsTheUnsatisfiedSummary_… handed the builder two facets at limit = 2 and asserted truncated — the >= limit inference as its own proof (#3699). Ask what a pin would fail ON."

The contract isn't unpinned overall — TheStartupWrite_Upserts_ASecondStartUpdatesTheRow_AndEachHostKeepsOne asserts new[] { ("HOST-A", …), ("HOST-B", …) } positionally, which would fail on a lost ORDER BY — so this is about the name over-claiming rather than lost coverage. Assert.Equal(new[] { "alpha", "ZED" }, rows.Select(r => r.ServiceHost).ToArray()) with neither .Order(...) says what the name says. (Worth noting while you're here: ServiceVersionText.ServiceLines re-sorts with OrdinalIgnoreCase anyway, so no display surface depends on the SQL's order — which is fine, just means the ORDER BY is belt-and-braces rather than load-bearing.)

[erikdarlingdata]

Claude posting for Erik Darling. Fixed in 4a55317. The read does have ORDER BY service_host (ServiceVersionTable.cs:69), so the fact now asserts the order without re-sorting. The rows go in as ZED then ALPHA (both upper case, so the order holds under any collation). With the ORDER BY replaced by a constant, the fact fails.

[claude]

Non-blocking, but it invalidates a guard's stated premise. These are the first timestamptz columns in the whole PostgreSQL store. Every other timestamp on the ladder is naive timestamp with (now() AT TIME ZONE 'UTC') — grep -c timestamptz PgMigrations.cs is 0, and e.g. PgMigrations.cs:1348-1349 is the config shape you'd otherwise copy.

I traced the behaviour and the choice is sound here: timestamptz + NpgsqlDbType.TimestampTz + Kind=Utc round-trips an absolute instant, GetFieldValue<DateTime> returns Kind=Utc, and ServiceVersionText.FormatStarted's SpecifyKind(…, Utc) is then a no-op, so the rendered time is right under any store session TimeZone — strictly more robust than the naive convention. Nothing goes red either: StoreSqlClockDisciplineTests.NaiveTimestampColumns' scrape is …\s+timestamp\b(?!\s*(?:with|tz)), and \b fails inside timestamptz, so these two names never enter the discriminator's vocabulary; and the upsert's now() is a write, which that scan disclaims by design (its own benign fixture list literally contains INSERT … ON CONFLICT (a) DO UPDATE SET updated_at = now()).

What is now stale is that guard's rationale, which states the invariant as measured fact at StoreSqlClockDisciplineTests.cs:22-23: "Every timestamp column in both stores is timestamp without time zone holding naive UTC — measured, there is not one timestamptz column in the schema". After this rung that sentence is false, and it is the sentence a future reader uses to decide whether a bare now() comparison is safe. One clause there noting the config.service_version exception (and why the scan is unaffected) keeps the guard's reasoning honest.

The PR body names the column types but doesn't flag this as a first-in-the-store deviation, which is the only reason I'm raising it.

[erikdarlingdata]

Claude posting for Erik Darling. Fixed in 4a55317. The rationale in StoreSqlClockDisciplineTests now names the two config.service_version columns as the deliberate exception and says why the scan is unaffected. The PR body now says these are the first timestamptz columns among the stored tables and why.

[claude]

Non-blocking, scoping note. The body says "A failed read shows unavailable", and the comment here says the same. That holds only for a failure of GetServiceVersionsAsync itself. The service-version read sits inside the outer try and after GetMonitoredServersAsync() on line 67, so when that read throws — the read-only-seat / transient case the outer catch at line 148 exists for — ServiceVersionBlock is never assigned at all and the line renders blank, not unavailable.

Blank is arguably the right answer there (the window has just put up a MessageBox and the grid is empty, so claiming anything about the service's version would be noise), and the _dataService is null path deliberately blanks it too on line 58. So this is a divergence between the prose and the code rather than a defect — worth one clause in the comment so the next reader doesn't go looking for an unavailable that can't appear on that path.

One more on the same block, also non-blocking: ViewerTimeHelper.CurrentDisplayMode is read once per refresh, so toggling the global display mode while Manage Servers is open leaves this line in the old zone until the next Add/Edit/Delete. Every other time on the grid converts through the per-row Clock at bind time and has the same property, so this is consistent with the window rather than new.

[erikdarlingdata]

Claude posting for Erik Darling. Fixed in 4a55317 by changing the comment and the PR body, not the code. They now say that unavailable appears only when GetServiceVersionsAsync itself fails, and that when GetMonitoredServersAsync throws the line is left as it was, with the error box as the message. A blank line beside an error box is the right answer there.

[claude]

Blind-read notes before the body

Two things I checked independently that the body asserts but does not show the arithmetic for, recorded because they are the parts of a rung PR that fail silently and permanently.

The probe quadruple reconciles across all four demoted rung tests. CONTRIBUTING's step 4 is the one with no recovery path ("Miss it and a fully-migrated store probes one rung short, so the connect-time gate refuses a store that is in fact current — permanently"). All four parts are present — ViewerDataService.cs:1417 (sentinel), :1426 (GetBoolean(149)), :1435 (bool hasServiceVersion = false as the last parameter), :1447 (if (hasServiceVersion) return 174; above the V173 arm) — and the three neighbouring rung tests' own ordinals land on the same new ordinal from three different bases, which is the check that the renumbering was not done by eye:

test	ProbeOrdinal	new top sentinel	=
AwsPerServerRoleRungTests	144	+ 5	149
PlanRegressionDailyRungTests	143	+ 6	149
LegacyPinCandidateRungTests	142	+ 7	149
ServiceVersionRungTests	149	+ 0	149

and ServiceVersionRungTests pins both ends of the new parameter (parameters[149].Name == "hasServiceVersion", parameters[148].Name == "hasQueryStoreComposeStamp"), so a sentinel inserted in the middle later fails here rather than in the field.

Parity: the trap does not apply, and I went looking for the counterpart rather than taking the body's word. .github/REVIEW-TRAPS.md closes with "a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit". Lite ships the identical column — Lite/Windows/ManageServersWindow.xaml:58 bound to InstalledVersion, filled from GetAppVersion() at Lite/Windows/ManageServersWindow.xaml.cs:54, with Lite/Models/ServerConnection.cs:286-288 saying "Same value for every row since one Lite process monitors all servers". But that is the same sentence as the defect only in shape: in Lite the app is the collector, so its own version per row is a true statement about what monitors that server, and the Darling bug was that the viewer is not the collector. There is no Darling state the shared engine now asks Lite for, so CONTRIBUTING's two-store clause (DuckDbInitializer.CurrentSchemaVersion + Schema.GetAllTableStatements() + the DuckDbSchemaTests count) has nothing to bind. The body's one-line "Lite. Unchanged: there the app is the collector, so its own version on each row is correct" is the right call, for the right reason.

The only residual is cosmetic and I am not asking for it: Lite's grid is now the last place in the product that repeats one app's own version down a column. If that column ever gets the same treatment it is a Lite-only change with no store behind it.

Four non-blocking inline notes posted separately (startup write never retried; an InHostOrder pin that sorts both sides; the store's first timestamptz columns vs a guard's stated premise; unavailable vs blank on the outer-catch path). Verdict follows.

[claude]

Verdict: LGTM

Claims

• VERIFIED — V174 adds config.service_version with the four listed columns; version now 174 — ServiceVersionTable.cs:45-51, PgMigrations.cs:1059+:1068, StorageVersion.cs:19.
• VERIFIED — one guarded CREATE, no data moved, never names config_service — TheScript_IsOneGuardedCreate_OfOneSchemaQualifiedTable_WithNoGrantAndNoDataMoved.
• VERIFIED — no GRANT needed — DarlingManagedRoles.cs:1204 (blanket config SELECT) + :1223-1225 (owner ADP), mirrored at tools/provision-roles.sql:193/:790; provisioning runs after the migration (DarlingWorker.cs:2629 then :2793).
• VERIFIED — ServiceVersionGrantLiveTests covers both claimed cases and proves SELECT rather than assuming it: its post-provisioning arm expects 0 rows, which a role without SELECT could not return.
• VERIFIED — sentinel at ordinal 149, arm returning 174, GetBoolean(149) — ViewerDataService.cs:1417/:1426/:1435/:1447; reconciled against all three demoted rung tests (144+5 = 143+6 = 142+7 = 149) in the top-level note.
• VERIFIED — the write precedes the config provider's first read — DarlingWorker.cs:3062 above :3064, pinned by TheWorker_RecordsTheVersion_BeforeTheConfigViewIsBuilt.
• VERIFIED — writes ProductVersion() (DarlingCliCommands.cs:342) and the start cut to microseconds (ToStoredInstant); a failed write warns and never throws, cancellation not swallowed (DarlingServiceVersionRecorder.cs:1017).
• VERIFIED — ReadAsync answers only 42P01 that way — ServiceVersionTable.cs:1193, pinned both directions.
• VERIFIED — column gone, both versions shown once, the four text shapes and the display-mode rule as described — ManageServersWindow.xaml, ServiceVersionText.cs:1337/:1340/:1355-1372; eight ServiceVersionTextTests facts.
• VERIFIED — the web dashboard needed nothing (server-tabs.js:5278 is the per-database extension version, from DarlingMcpPgServerStateTools.cs:280); Lite untouched and correct (Lite/Windows/ManageServersWindow.xaml:58); dead InstalledVersion + INotifyPropertyChanged removed with no reader left (ViewerServerEntry.cs:37); both rewind paths updated; and the three self-flagged residuals hold.
• UNVERIFIED — every test-run total (19/21, 1015, 1481, 5733, "0 warnings") — unexecutable on a read-only tree; the viewer half needs a net10.0-windows build. I verified the tests exist and what each asserts.
• UNVERIFIED — the live-run and RED-check claims — all three live classes Assert.SkipWhen on an unset DARLING_TEST_PG, and a reverted mutation leaves no trace. I confirmed by reading that each named test would be the one to fail.
• UNVERIFIED — "V174 is the next free number on origin/dev at push time" — this checkout is a merge into the base. Locally consistent: the ladder is ordered, dense through 174 and duplicate-free (MigrationLadderPins).

Riskiest lines

1. ViewerDataService.cs:1447 — the hasServiceVersion arm and its three dependent sites; CONTRIBUTING's step with no recovery path. Pinned by ServiceVersionRungTests.TheProbeNamesTheTable_AndMapsAFullyMigratedStoreToThisTopRung (both directions); the probe SQL's own column count only on the live path.
2. DarlingWorker.cs:3062 — the one-shot write. Pinned for placement by TheWorker_RecordsTheVersion_BeforeTheConfigViewIsBuilt; UNPINNED for recovery (inline note).
3. ServiceVersionTable.cs:45-51 — CreateSql, shipped-rung text from merge on and the store's first timestamptz DDL. Pinned by TheRungIsRegisteredOnce_… and the live column-shape string.

Traps near this diff

• "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — Lite ships the same column (Lite/Windows/ManageServersWindow.xaml:58), but there the app is the collector, so its own version per row is true; reasoning in the top-level note.
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — PgMigrations.cs:1059 aliases ServiceVersionTable.CreateSql.
• "A pin that encodes the lie … the >= limit inference as its own proof (#3699). Ask what a pin would fail ON." — applies — ServiceVersionLiveTests.cs:57 sorts both sides of an …_InHostOrder_… fact. Test-only; inline note.
• "'Did I send' is not 'was it delivered' … never retries a failed delivery (#3633)." — applies in shape — DarlingWorker.cs:3062's failure is never retried. Narrow window, display-only; inline note.
• "An empty analysis window is 'unavailable', not an all-clear (#3553)." — honoured — ServiceVersionText.cs:1337/:1340 keep "no row" and "read failed" distinct.

Divergence (blind read vs PR body)

Nothing refuted; the body volunteers its own residuals. Three narrower/wider readings, all inline, none blocking:

1. "A failed read shows unavailable" holds only for that read — a throw from GetMonitoredServersAsync() on the line above leaves the block blank (ManageServersWindow.xaml.cs:67).
2. timestamptz is named as a type but not as the store's first; StoreSqlClockDisciplineTests.cs:22-23 asserts the opposite as measured fact. Nothing goes red and the choice is sound — the guard's rationale is what is now stale.
3. A public type loses INotifyPropertyChanged; mentioned, but the callers check was mine — nothing binds or subscribes.

Reviewer checklist

• Run the live half against DARLING_TEST_PG — it is the only path checking the probe SQL's column count against MapProbedSchemaVersion's arity, which fails there and nowhere else.
• Build the viewer on Windows and open Manage Servers against a V174 store — confirm the multi-host Environment.NewLine text is not clipped beside ViewerVersionBlock.
• Confirm the two lines agree on one build: GetAppVersion() cuts to Major.Minor.Build, ProductVersion() keeps the full informational version.
• Restart twice; check one row per host with a moving process_started_at, then break the store just after migration and see if the line reads unknown (older than 3.11) for the run.
• Confirm V174 is still free on dev at merge — a gap is the silent unrepairable direction.

[claude]

Verdict: LGTM

Claims

• VERIFIED — "both now pick the V173 script by version" — QueryStoreComposeStampLiveTests.cs:404 and :494 both now use PgMigrations.Scripts.Single(s => s.Version == QueryStoreComposeStamp.RungVersion).Sql in place of Scripts[^1].Sql.
• VERIFIED — the two renamed tests match the body's description exactly — TheRung_IsTheTopOfTheLadder_AndTheStorageVersion → TheRung_IsV173_AboveV172_AndBelowTheStorageVersion and TheProbe_MapsAFullyMigratedStoreToThisTopRung → TheProbe_HasThisRungsSentinelAndMapsItsArmToV173 (QueryStoreComposeStampTests.cs:359, :427).
• VERIFIED — the new assertions match the live MapProbedSchemaVersion logic rather than just re-describing it — ViewerDataService.cs:1393-1404 returns 174 when hasServiceVersion, else 173 when hasQueryStoreComposeStamp, else falls through; the test's behind (both false → 172) and atThisRung (only the +1 ordinal false → 173) cases reproduce exactly that branching (QueryStoreComposeStampTests.cs:447-455).
• VERIFIED — the "below" lookup is no longer position-based — PgMigrations.Scripts.Where(s => s.Version < rung.Version).Max(s => s.Version) replaces Scripts[^2].Version, which is necessary now that V173 is no longer second-from-top (QueryStoreComposeStampTests.cs:367-368).
• UNVERIFIED — "the read-only-roles test failed with 42P01 because it re-ran the newest rung, V174, after dropping V173 tables" — consistent with the code (V174's script creates config.service_version, not the dropped compose-stamp tables, so the subsequent SELECT against them would hit undefined-table), but the actual error code/run isn't checkable from a read-only tree.
• UNVERIFIED — "Before: 3 failed of 26; after: 26 passed" — a live test-run total, not executable here.

Riskiest lines

1. Darling/Darling.Tests/QueryStoreComposeStampLiveTests.cs:494 — re-creates the dropped compose-stamp tables from the correct (V173) script rather than the unrelated top rung — pinned by TheReadOnlyRoles_CanSelectTheThreeTables_WithNoGrantInTheRung (self-pinning, test-only change).
2. Darling/Darling.Tests/QueryStoreComposeStampTests.cs:441-455 — arity/ordinal and branch-mapping assertions against MapProbedSchemaVersion — pinned by TheProbe_HasThisRungsSentinelAndMapsItsArmToV173.
3. Darling/Darling.Tests/QueryStoreComposeStampTests.cs:367-368 — version-based "below" lookup — pinned by TheRung_IsV173_AboveV172_AndBelowTheStorageVersion.

Traps near this diff

• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply to the new code — the old version of this test asserted RungVersion == top.Version and SchemaVersion == top.Version (two values pinned equal to a third rather than looked up directly); the new code looks the rung up directly by version (QueryStoreComposeStampTests.cs:363), removing that shape rather than repeating it.
• "A roster-based census does not see the file it does not roster: EveryPagedTool_ObservesTruncation_… walked ten tools and missed three in another file (#3699)." — does not apply — this commit touches exactly the two files (QueryStoreComposeStampTests.cs, QueryStoreComposeStampLiveTests.cs) that encode the V173-specific pins; it isn't a roster walk.

Divergence (blind read vs PR body)

None — the body's account of this follow-up (which tests were renamed, which two Scripts[^1].Sql call sites were changed and why) matches the diff on every checkable claim.

Reviewer checklist

• Run Darling.Tests against DARLING_TEST_PG to confirm the stated "3 failed of 26 → 26 passed" result, since the live half (including the 42P01 claim) can't be executed from a read-only tree.