Skip to content

A falling gauge read as a counter reset because the store did not know it was a gauge: perfmon_stats stores each counter's cntr_type (Darling V132 / Lite v62), the collector writes a gauge as its level with no delta, and every reader classifies by the stored type with the /sec name proxy as the pre-rung fallback (partial #3653 A7) - #3708

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/3653-cntr-type-rung
Sep 19, 2026

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

A falling gauge read as a counter reset because the store did not know it was a gauge

Partial for #3653 — A7 Viewer rendering, the fourth clause: "cntr_type not stored, so gauges are delta'd as activity (a rung)". Darling rung V132 (perfmon-counter-type), Lite schema v62. Numbered max(dev) + 1 at open and re-checked against origin/dev immediately before push (131 / 61 were still the top). Does not close the issue.

The lie the product told

PerfmonStatsCollector selected object_name, counter_name, instance_name, cntr_value from sys.dm_os_performance_counters and never cntr_type, so the store held nothing that said which of its perfmon rows were COUNTS and which were LEVELS. Every row went through the same CalculateDeltaWithInterval call: Total Server Memory (KB) was differenced exactly like Batch Requests/sec, and a target that released memory — a FALLING level — presented to the shared delta calculator as a counter reset. The calculator did the honest thing for a counter and wrote its (0, 0) "no delta knowable" marker; for a gauge that is the wrong claim at exactly the moment the drop mattered, and the perfmon chart drew a level's meaningless per-sweep difference as activity (#3540 A7: "a falling gauge = fake counter reset"). #3702 could only classify by a NAME-SUFFIX PROXY (/sec = rate, everything else "Δ per interval") because the row could not tell it; the proxy could label a level honestly as a delta but could never plot it as the level it is.

The lane's load-bearing question resolved early: cntr_value was ALREADY stored (PayloadColumns has carried it since extraction). The rung is therefore ONE column, cntr_type integer NULL, appended last on both SKUs.

The row contract

Row cntr_type cntr_value delta_cntr_value sample_interval_seconds Readers
Rate (272696576 PERF_COUNTER_BULK_COUNT, 272696320 PERF_COUNTER_COUNTER) stored the cumulative count the calculator's delta (0 = marker) measured (0 = marker) per second = delta / interval; the marker plots NaN
Gauge (65792 PERF_COUNTER_LARGE_RAWCOUNT, 65536 PERF_COUNTER_RAWCOUNT) stored the reading NULL NULL the level, plotted as cntr_value; the calculator is never called
Other (1073874176 PERF_AVERAGE_BULK, 1073939712 PERF_LARGE_RAW_BASE, 537003264 / 537003008 fractions, any unrecognised id) stored the raw numerator / base the calculator's delta measured per interval — the delta as stored, labelled "(Δ/interval)"; never divided into a rate, never a level
Pre-V132 / pre-v62 NULL as always as always as always classified as before the rung: the #3702 name proxy (/sec → per second, else per interval)

NULL, not (0, 0), on a gauge — 0 is the calculator's marker, a claim about a delta, and a gauge has no delta to know. NULL/NULL beside a stored gauge type says "this row's number is cntr_value" and nothing else. The calculator is not called for a gauge row, so no gauge key enters its cache from a pass; the seeders still restore every perfmon row's cntr_value on a restart (they read no type), which is harmless — an unused baseline under a key no delta call will present. The pass window rolls on the rate rows of the same sweep and perfmon passes no series age, so nothing that reads the window can miss the skipped keys.

The pre-rung fallback is chosen so history renders as the operator last saw it, and the type decides the moment a row carries one. Because a counter's type does not change, the viewers take ANY point's non-null type as the series' type: a gauge's whole 30-day window plots as a level the morning after the upgrade (its pre-rung rows stored the value all along) — the correction, not a discontinuity. A pre-rung gauge row in a store that never collects again keeps plotting its stored delta per interval, as #3702 shipped it.

A point whose instance rows disagree on type reads NULL. The trend reads sum a counter's instances per collection, and the type column is CASE WHEN MIN(cntr_type) = MAX(cntr_type) THEN MAX(cntr_type) END — byte-identical across all four trend reads (pinned). The wait-statistics object's Lock waits / Page latch waits family carries a rate, a gauge and an average under one counter name; their SUM was never one quantity (#3702 recorded it), no single type can describe it, and the equality is what stops it being classified by whichever instance's id sorts first. That family stays on the proxy and plots per interval, exactly as before; its honest end state is one series per instance — a picker change, its own lane.

The base-fraction finding (stated, not fixed)

The default list carries ten PERF_AVERAGE_BULK rows per collection: the Average wait time (ms) instance of each SQLServer:Wait Statistics counter (Lock waits, Page latch waits, Non-Page latch waits, Page IO latch waits, Memory grant queue waits, Thread-safe memory objects waits, Log buffer waits, Log write waits, Network IO waits, Wait for the worker). Their cntr_value is a cumulative numerator; the average the instance name promises is Δnumerator / Δbase over a PERF_LARGE_RAW_BASE sibling this store does not join. They have been differenced like bulk counts forever and still are: the rung stores the type, classifies them Other, and the viewers plot their per-interval delta under a label that says so — TRUE (it is the wait milliseconds that accrued) while not the average the name promises. Ten rows, not one, so per the brief it is stated here and in the rung doc, the vocabulary's remarks and the collector-definition pin, and not fixed tonight. No PERF_LARGE_RAW_BASE row is in the default list by name; whether the DMV exposes a base INSTANCE under the same counter names (which the counter-name-only filter would collect) needs a SQL Server to answer — CI's collector job is where that is first seen.

What changed, and why this shape

Schema. PgMigrations V132: ALTER TABLE collect.perfmon_stats ADD COLUMN IF NOT EXISTS cntr_type integer; + CREATE OR REPLACE VIEW collect.v_perfmon_stats (Postgres freezes a passthrough's SELECT * list at CREATE — V14/V80/V81/V127/V128). Nullable, no DEFAULT, no backfill: a pre-rung row never recorded its type, the store holds no way to recover one from a name, and a DEFAULT is the compressed-chunk trap (TimescaleDB has to rewrite compressed segments to honour one). StorageVersion 132. DuckDbInitializer v62: ALTER TABLE perfmon_stats ADD COLUMN IF NOT EXISTS cntr_type INTEGER (REQUIRED on Lite for the v60 reason — the appender writes one value per declared column and fails EndRow() on a database without it). A fresh store on either SKU gets the column from the generated CREATE TABLE (the collector definition carries it, appended last); the ALTER no-ops there.

Collector (PerfmonStatsCollector): selects pc.cntr_type; Row gains int CntrType; PayloadColumns appends cntr_type Integer (positional writers on both stores; an ALTER can only land at the end). WritePayload branches on IsGauge(row.CntrType): the gauge arm writes (value, NULL, NULL, type) with no calculator call; the other arm is the pre-rung write plus the type. The doc line #3702 reported as falsified ("the Viewer carries it unplotted") is corrected in passing.

One vocabulary, two projects. PerformanceMonitor.Common/PerfmonCounterTypes (new): the DMV ids as named constants, RateTypes / GaugeTypes, Kind(int) → Rate | Gauge | Other, Kind(int?) (null in, null out), Word(int?) → rate | gauge | other | null for the MCP. Every reader on both SKUs classifies through it. The COLLECTOR cannot see it — PerformanceMonitor.Collectors and PerformanceMonitor.Common reference neither each other nor a third that could hold the ids, and Collectors is kept free of the MCP SDK and credential store Common carries — so the collector spells its write-side gauge set itself (PerfmonStatsCollector.GaugeCounterTypes = { 65792, 65536 }) and Lite.Tests/PerfmonCounterTypeTests.TheCollectorsGaugeSet_IsTheVocabularysGaugeSet holds the two spellings equal and asserts the two assemblies really are unrelated. Two ints on the write side pinned to the read side is the twin-constant idiom the repo already uses across SKU boundaries; the alternatives were a ProjectReference from Collectors to Common (dragging the MCP SDK into the collectors library for two ints) or a four-arm kind→basis switch duplicated in both viewers.

Shaping (DeltaSeriesShaping, #3702's helper): DeltaBasis.Level; DeltaSample(Time, long? Delta, long? IntervalSeconds, long? Value = null) — Delta nullable because a gauge row stores none, Value the raw cntr_value a Level series plots; BasisFor(counterName, cntrType) — Rate → PerSecond, Gauge → Level, Other → PerInterval, NULL → the one-argument proxy, which is now documented as the fallback and still never says Level; Shape for Level plots Value and reads neither delta nor interval (a pre-rung gauge row's (0, 0) marker was a claim about a delta nobody should have taken, not about the level, so it does not break the line); a null delta on a delta basis is NaN, not 0. LegendLabel keeps a gauge's name (its name is its unit). YAxisLabel composes from the bases present: per second / Δ per interval / value (gauge) alone, or the parts present joined — per second (rates) · Δ per interval (others) · value (gauges) — so a mixed axis names every kind of number on it and only those (the two-basis constant MixedAxisLabel is replaced by MixedAxisParts + MixedAxisSeparator).

Readers, both SKUs. LocalDataService.Perfmon.cs (Lite), ViewerDataService.Perfmon.cs (Darling viewer), DarlingTrendReader.PerfmonTrendSql, DarlingDataReader.LatestPerfmonStatsSql: the two latest-snapshot reads select the row's own cntr_type; the four trend reads select the agreed-type expression. Row types: DeltaValue long → long? on PerfmonRow and PerfmonTrendPoint on both SKUs and on the Darling MCP PerfmonTrendPoint (whose SampleIntervalSeconds also becomes long? — a pre-column row now publishes null where it published a manufactured 0, the #3702 Lite change reaching its twin); int? CntrType added everywhere. No reader coerces a NULL delta to 0 (pinned per method body).

Viewers, both SKUs. ViewerServerTab.Perfmon.cs / ServerTab.Pickers.cs: var seriesType = trend.Select(t => t.CntrType).LastOrDefault(t => t.HasValue); var basis = DeltaSeriesShaping.BasisFor(counterName, seriesType); and the sample carries t.Value. Nothing else in the chart bodies moved.

MCP perfmon tools, both SKUs (get_perfmon_stats, get_perfmon_trend): each counter row publishes cntr_type and counter_kind; the trend envelope publishes the series' cntr_type and counter_kind; a gauge's delta_value (and, on the trend, sample_interval_seconds) is null. The descriptions say what a caller must know to read the payload — "'gauge' means value IS the reading", "the per-second figure is delta_value divided by sample_interval_seconds, never delta_value alone", "null counter_kind is a row written before the type was stored — classify it by name" — in the same words on both SKUs (pinned). counter_kind's three words are one lower-case spelling from one function; null is the pre-rung row, said in the description rather than replaced by a fourth word.

Compose (MeasureCatalog.cs): comment only. The stale "cntr_type is not stored" is gone; the catalog now states why NO row guard is compiled on the column — for rows written since the rung the write path makes one redundant (a gauge's delta_cntr_value IS NULL, so perfmon_value_delta over a gauge aggregates only NULLs and comes back empty rather than inventing activity), and for rows written before it the type is NULL on gauge and rate alike, so no row-level predicate can separate them; a guard would be dead text under both populations. The brief's hypothesis that gauges are not exposed as Cumulative is confirmed the other way round: perfmon has a Gauge measure (perfmon_value) and a Delta measure (perfmon_value_delta), each spanning every counter through the counter_name dimension, and no Cumulative measure — the #3695 MeasuredDeltaFilter on the Delta measure is unaffected (a gauge row's NULL interval passes IS DISTINCT FROM 0 and its NULL delta adds nothing).

Viewer probe (ViewerDataService.cs, a boundary extension every rung makes): the V132 COLUMN sentinel (perfmon_stats / cntr_type, ordinal 107), hasPerfmonCounterType top arm returning 132, V131's arm re-worded as one rung behind. Without it RequiredStoreSchemaVersion == StorageVersion.SchemaVersion would map every current store one rung low and the connect-time gate would refuse it.

Doc lines corrected: Darling/README.md's Perfmon tab row ("a per-counter delta trend" → shaped by the stored type), the collector's #2234 comment (the Viewer no longer carries the interval unplotted).

Blast radius

What this PR does NOT do

Live verification

PostgreSQL 18 + TimescaleDB 2.28.1 (timescale/timescaledb:2.28.1-pg18, throwaway container on Docker context desktop-linux, torn down):

  • Fresh ladder V1→V132 via PgMigrations.MigrateAsync inside the gated tests: 131 rungs applied (V45 gap intact), stamped 132; perfmon_stats.cntr_type integer / nullable / no default, LAST; v_perfmon_stats projects it last.
  • Simulated pre-V132 store with a COMPRESSED chunk, through a throwaway net10.0 harness over the built PerformanceMonitor.Darling.Storage.dll (outside the worktree, deleted): full ladder → ConvertToHypertablesAsync (70) → ApplyCompressionPolicyAsync (70) → DROP VIEW / DROP COLUMN cntr_type / re-freeze the passthrough / unstamp 132 → three pre-rung rows 40 days back (a gauge whose level FALLS, written as the calculator would have: (0, 0) — the fake-reset row itself — and a rate) → compress_chunk → chunk_compression_stats reads Compressed → MigrateAsync applied exactly one rung, stamped 132; the column is integer / YES / no default; the view's column list ends sample_interval_seconds, cntr_type; the chunk is still Compressed; the three pre-rung rows read cntr_type NULL through the compressed chunk and the refreshed view; a typed gauge row inserted into the compressed chunk's range and a typed rate row today both land and read back; the agreed-type expression over the gauge gives NULL, NULL, 65792 across the three points; compress_chunk recompresses the old chunk.
  • Release fixtures MigrationUpgradeLadderLiveTests (v3.3.0 / 3.5.0 / 3.6.0 / 3.7.0 loaded as their release built them, climbed to 132): 5/5.
  • The real writer: PerfmonCounterTypeLivePostgresTests drives PerfmonStatsCollector.WritePayload through PgCollectorRowWriter over a real NpgsqlBinaryImporter — DarlingCollectorRunner's COPY loop in shape — with a real DarlingDeltaCalculator, two passes five minutes apart, the gauge falling 8,388,608 → 8,000,000. Stored: the gauge's two rows (value, NULL, NULL, 65792); the rate's first row the (0, 0) marker and its second (5900, 900, 300, 272696576). Read back through ViewerDataService.GetPerfmonTrendsByCountersAsync (gauge → Level, plots [8388608, 8000000]; rate → PerSecond, plots [NaN, 3.0]), DarlingTrendReader.GetPerfmonTrendAsync, DarlingMcpTrendTools.GetPerfmonTrend (counter_kind: gauge, null deltas and intervals; rate with 900 / 300; the planted mixed family → null) and DarlingMcpDataTools.GetPerfmonStats (the planted mixed family's two instances each with its OWN kind, the gauge's delta_value: null).

DuckDB in-process (Lite.Tests/PerfmonCounterTypeReadTests, real LocalDataService and real MCP tools over the shared fixture): the same four counters — gauge, rate, pre-rung, mixed — through the single trend, the batched trend, the latest snapshot and both tools; plus v61→v62 on a real DuckDB file: a database initialized at the current schema, every view and index over perfmon_stats dropped (DuckDB counts both as dependents), cntr_type dropped, the v61 passthrough re-frozen, a pre-rung (0, 0) gauge row planted, the version stamped 61, re-initialized → version 62, cntr_type INTEGER / YES last, the index and v_perfmon_stats rebuilt, the old row NULL through the view, a typed row lands.

Test plan

New — Darling.Tests/PerfmonCounterTypeRungTests (7): the rung at the top of a dense ladder; one nullable integer ADD COLUMN in the generator's type + one passthrough refresh and nothing else (no DEFAULT / NOT NULL / UPDATE / INSERT / DELETE / DROP / INDEX / TABLE), the view in AllPassthroughViews and not in PayloadResolvingViews, generated CREATE TABLE and COPY list carry the column LAST, the rung doc's phrases, HypertableCount 71 and 7 payload columns unmoved; the probe's three sites + top arm (all-true → StorageVersion.SchemaVersion, one behind → 131, arm order, no table name in the arm's prose); every Darling perfmon read selects the type with the one agreed-type expression (byte-identical to Lite's two) and the row types keep NULL as null (per method body); both chart bodies classify by seriesType with the proxy as fallback; the Compose catalog states why there is no guard. Darling.Tests/PerfmonCounterTypeLivePostgresTests (gated, above). Lite.Tests/PerfmonCounterTypeTests (17): every documented id's kind + unknown → Other, the ids are the DMV's, Word's spellings, the cross-assembly gauge-set pin + the two assemblies unrelated, the v62 block's text, the DuckDB generator's trailing nullable column, both SKUs' MCP descriptions in the same words. Lite.Tests/PerfmonCounterTypeReadTests (2, real DuckDB, above).

Changed — Lite.Tests/PerfmonStatsCollectorDefinitionTests: the query selects cntr_type and the payload declares it seventh/last; a rate row (…, 987654, 9876540, 137, 272696576) with one calculator call; a gauge row (…, 8388608, null, null, 65792 | 65536) with NO calculator call (Theory over both gauge ids); a PERF_AVERAGE_BULK row differenced like a rate and typed. Lite.Tests/DeltaSeriesShapingTests (48, was 31): a stored type decides over the name (ten cases including the proxy's mis-classes corrected and an unknown id → Other), a NULL type is the proxy byte for byte, Level plots the value and ignores delta/interval including a pre-rung (0, 0) marker, a null delta is NaN on both delta bases, labels for three bases and every mixed combination. Lite.Tests/MeasurementContractCensusTests rule 8: TheGaugeRule_WaitsOnTheCounterTypeRung (which pinned the ABSENCE of the column so the rung would red it) becomes TheGaugeRule_TheCollectorWritesNoDeltaForAGaugeType_AndEveryReaderClassifiesByTheStoredType — the column last, the one delta call structurally inside the IsGauge guard, every product perfmon delta read (≥ 5) selecting the type; the header's rule 8 row rewritten from "NOT CENSUS-ABLE" to "PINNED". Lite.Tests/DuckDbSchemaEquivalenceTests.IntentionalAppendedColumns += perfmon_stats.cntr_type (the golden is not edited, per its own rule). Darling.Tests/NotificationRoutesRungTests hands its top-rung claims to V132's file (the standing handoff idiom). Darling.Tests/ViewerPerfmonShapingParityTests pins the two-argument BasisFor and the four-field DeltaSample.

Executed here — Darling.Tests whole, WindowsDesktop stripped from the runtimeconfig, DARLING_TEST_PG on the rig: every class touching this change green, including the live ones (PerfmonCounterTypeRungTests 7/7, PerfmonCounterTypeLivePostgresTests 1/1, MigrationUpgradeLadderLiveTests 5/5, MigrationLadderPins, NotificationRoutesRungTests 10/10 + live, PgLogEventMetricsRungTests, ViewerPerfmon* 12/12, DarlingMcpTrendTools* 45/45, DarlingMcpDataTools* 63/63, McpPayloadContractCensusTests 49/49, McpPageContractTests, McpLatestSnapshotStampTests, McpZeroIsAMeasurementTests, DarlingComposeTests 280/280, MeasurementContractCensusTests, both DeltaFamilyInterval*RungTests, PgSchemaGeneratorTests, ViewerCollectorCoverageTests, DocCommentHygieneTests 77/77, RepoFileAdoptionTests, CommentFilterAdoptionTests, PgCopyWriterTests, AsOfWindowAnchorTests); the suite's remaining reds are the platform artefacts (WPF PresentationCore, Microsoft.Data.SqlClient's System.Configuration.ConfigurationManager) in classes this PR does not touch. Lite through a selective net10.0-windows harness (AssemblyName Lite.Tests, WindowsDesktop stripped): PerfmonStatsCollectorDefinitionTests 8/8, DeltaSeriesShapingTests 48/48, PerfmonCounterTypeTests 17/17, PerfmonCounterTypeReadTests 2/2, MeasurementContractCensusTests 10/10, DuckDbSchemaEquivalenceTests 4/4, DeltaFamilyIntervalColumnTests 6/6, DuckDbSchemaTests 7/7, McpLatestSnapshotStampTests 12/12; McpStatusEnvelopeTests and DeltaFamilyUnknowableRowReadTests green except one wait-stats test each whose read path loads PresentationFramework on macOS (#3702 recorded the same two).

First executed in CI, stated plainly: the SQL Server side of the collector — sys.dm_os_performance_counters.cntr_type arriving as int at ordinal 4 through a real SqlDataReader, and the actual type ids the engine reports for the default list (the vocabulary's ids are the documented Windows PERF_* words; the collector-definition pins feed them through a fake reader). WPF drawing the level series is first seen at install.

Builds, -c Release -p:EnableWindowsTargeting=true: PerformanceMonitor.Common, PerformanceMonitor.Collectors, PerformanceMonitor.Darling.Storage, PerformanceMonitor.Darling.Service, PerformanceMonitor.Darling.Viewer, PerformanceMonitorLite, Darling.Tests — 0 warnings; Lite.Tests — dev's one pre-existing xUnit2000 in LogTailOverlapThresholdPinTests.cs:95, untouched.

Which component(s) does this affect?

  • Lite
  • Darling
  • Lite Tests
  • Darling Tests
  • SQL collection scripts
  • Documentation
  • Full Dashboard (deprecated)
  • CLI Installer (deprecated)

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • I have tested my changes against at least one SQL Server version — the collector's SQL text gains one column from a documented DMV column; the store side is verified against PG18 + TimescaleDB 2.28.1 (fresh, compressed-chunk climb, four release fixtures) and DuckDB in-process (fresh and v61→v62) as above; the SQL Server read is CI's
  • I have not introduced any hardcoded credentials or server names

…w it was a gauge: perfmon_stats stores each counter's cntr_type (Darling V132 / Lite v62), the collector writes a gauge as its level with no delta, and every reader classifies by the stored type with the /sec name proxy as the pre-rung fallback (partial #3653 A7)

The DMV's cntr_type was never selected, so the collector differenced every
counter it read - Total Server Memory (KB) exactly like Batch Requests/sec -
and a target that released memory presented to the shared delta calculator
as a counter reset: the (0, 0) marker written at the moment the drop
mattered. cntr_value was already stored (the lane's load-bearing question),
so the rung is one nullable integer column, appended last, on both SKUs.

Rows: a rate (PERF_COUNTER_BULK_COUNT) keeps its write plus the type; a
gauge (PERF_COUNTER_LARGE_RAWCOUNT) writes (value, NULL delta, NULL
interval, type) and never reaches the calculator; the average/fraction/base
family keeps the delta write and is classified Other (a stated finding: the
ten Wait Statistics "Average wait time (ms)" instances are numerators whose
average needs the base this store does not join); a pre-rung row has NULL
type and renders as it did before.

One vocabulary (PerformanceMonitor.Common.PerfmonCounterTypes) for every
reader; the collector's gauge set is spelled on its side of the assembly
boundary and pinned equal. DeltaSeriesShaping gains Level; both viewers'
perfmon charts take the series' stored type; both SKUs' MCP perfmon tools
publish cntr_type + counter_kind and null a gauge's delta. Compose gets no
row guard (redundant for new rows, powerless on old; stated in the catalog).
The viewer probe gains the V132 sentinel and top arm; V131's test hands the
top-rung claims to PerfmonCounterTypeRungTests.
@erikdarlingdata
erikdarlingdata merged commit 5b999e0 into dev Sep 19, 2026
7 of 8 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/3653-cntr-type-rung branch September 19, 2026 09:24
erikdarlingdata added a commit that referenced this pull request Sep 19, 2026
…le, Opus full tier (#3710) (#3718)

* REVIEW-TRAPS.md: the repository's recurring defect shapes, one line each with the PR that bit (#3710)

Seven layers (SQL, collectors and deltas, analysis, alerting and notifications,
storage and MCP payloads, tests and censuses, workflows and CI, repository
mechanics), fifty-odd lines, every one citing the pull request or issue that
verified it: the >= limit truncation inference (#3594/#3679/#3699), ORDER BY
3 + 4 folding to a constant (#3613), MAX(max_dop) across plans (#3648), integer
division in cntr_value_per_second (#3653 Q9), per-collection alert firing
against a shorter cooldown (#3628/#3636), restart zeros in deltas and baselines
(#3595/#3698/#3705/#3708), ScoreAll skipping amplifiers on a zero base and the
non-virtual GetActiveEdges (#3688), the PgTargetFactKeys censuses (#3688),
pg_total_relation_size vs the heap and the two TimescaleDB views that hide
materializations and history (#3610/#3585), the -infinity crash backoff (#3629),
the Slack block budget and the surrogate-pair cut (#3612/#3618/#3625/#3644),
LCK_M_SCH_M folding into LCK so no SCH_M fact exists (#3709), one wait profile
per run (#3709), and the CRLF round-trip. Each citation was read before it was
written down; a wrong citation is worse than none.

The review workflow reads this file before the PR body and must say, per trap
the diff is near, whether it applies here and why.

* Claude review: blind read before the body, callers rule, structured verdict with a ledger, and an Opus/Sonnet tier by changed path (#3710)

The prompt runs in two phases. Phase 1 forbids gh pr view / git log / git show
until the reviewer has read the diff, every changed region in context, the
callers of every changed public symbol (Grep, both SKUs and both test projects)
and .github/REVIEW-TRAPS.md, and written its own account. Phase 2 reads the
body and grades every claim VERIFIED / UNVERIFIED / REFUTED, then reports the
DIVERGENCE between the two accounts. The verdict is a fixed block -- Claims,
Riskiest lines with the pinning test or UNPINNED, Traps near this diff,
Divergence, Reviewer checklist -- ending in one machine-readable ledger line,
posted through gh pr review --body-file - with a quoted heredoc (no Write tool;
the allowlist is unchanged and still read-only).

The #3650 verdict check now fetches the newest verdict in the window by id and
fails the job when the body lacks the "## Verdict:" header or the ledger, or
when the ledger's verdict disagrees with the review's API state (a
"CHANGES REQUESTED" body submitted with --comment is the shape the guard would
wave through). The guard's own arm is untouched and still keys on the state.

A dorny/paths-filter@v4 step (the repo's pin style) routes changes under the
shared libraries, Darling Storage/Analysis/Service, Lite Services/Mcp/Database,
install/ and every .sql file to the full tier -- claude-opus-5[1m], 60 turns --
and everything else to claude-sonnet-5 at 30 turns; one decision step emits
tier/model/max_turns, one review step consumes them (two steps would double the
"review posted" accounting the guard does), and the prompt carries the tier and
model into the ledger. A filter that dies degrades to FULL, not light.

Step name "Claude review" and the ALWAYS POST sentinel are unchanged: both are
read by claude-review-guard.yml.

* Review tier: Alerting and Notifications read at the full tier (#3710)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant