[erikdarlingdata]

Claude posting for Erik Darling.

Closes #5758.

Why

#5721 left two copies of the statement-identity rule for an unknown query hash: QueryHashIdentity in PerformanceMonitor.Common and a copy in SameStatementPileupDetector (PerformanceMonitor.Analysis), held equal only by a parity test. Analysis could not reference Common then because the project edge rewrites the packages.lock.json files under deprecated/. Erik has now allowed committing those lock files, so the copy can go.

What changes

• PerformanceMonitor.Analysis.csproj gets a ProjectReference to PerformanceMonitor.Common (Common references no project, so there is no cycle).
• SameStatementPileupDetector.StatementIdentity is now QueryHashIdentity.Identity(row.QueryHash, row.QueryText). The private IsUnknownHash is deleted. NormalizeText and ShortHash stay: Preview and the incident/story ids still use them (display only; NormalizeText's comment now says so).
• One correction to the brief: Common's StatementFingerprint already returns null for the withheld marker (it compares the trimmed text to SensitiveStatements.PlaceholderText, the same string as WithheldStatementMarker.Text). So the detector needs no separate marker check; StatementIdentity has no WithheldStatementMarker call any more. No input differs between the two definitions: the 15 hash x 21 text corpus (315 pairs) gives the same result both ways, with the expected value for each now written out.
• Doc comments on StatementIdentity and in QueryHashIdentity.cs say there is one definition. IsUnknownHash's comment went with the method.
• Lite.Tests/QueryHashIdentityParityTests.cs is renamed QueryHashIdentityTests.cs. The corpus is kept whole; the "copy agrees with Common" test now asserts an explicit expected identity per case (through QueryHashIdentity.Identity and the detector's row path), plus theories for IsUnknown/KnownHash per hash and StatementFingerprint per text.
• Lock files rewritten by dotnet restore PerformanceMonitor.sln --force-evaluate: Lite, Lite.Tests, Darling.Tests, Darling.Viewer, and deprecated/Dashboard and deprecated/Dashboard.Tests. They also refresh the stale [3.9.0, ) ranges to [3.10.0, ), and Lite's lock drops a stale System.Diagnostics.EventLog entry. Under deprecated/ only those two packages.lock.json files changed.

Not in scope: WithheldStatementMarker (Analysis) stays, since Notifications still uses it and StatementMarkerReaderTests pins it.

Test plan

• Case counts: the old class declared 24 test cases (9 [Fact] plus 15 [InlineData]) and a 315-pair corpus loop. The new class runs 60 cases: 9 facts, 15 inline, 15 hash-corpus rows, 21 text-corpus rows; the corpus loop is still 315 pairs. Nothing dropped.
• dotnet restore PerformanceMonitor.sln --locked-mode passes; git diff --name-only origin/dev...HEAD -- deprecated/ lists only the two packages.lock.json files.
• Builds: Lite.Tests, Darling.Tests, Darling Service, PerformanceMonitorLite, 0 warnings, 0 errors.
• Lite.Tests: QueryHashIdentityTests (60), SameStatementPileupDetectorTests, LongQueryUnknownHashTests, AttentionCollectorDefinitionTests, StatementMarkerReaderTests, and the sweep classes naming changed files (LiteRuntimePrerequisiteDocsTests, McpAnalysisFindingsCommandTests, StoryConfidenceTests), plus *Census* and *Ratchet*: 247 total, 0 failed.
• Darling.Tests: SameStatementPileupSourceCensusTests, LongQueryUnknownHashTests, StatementMarkerReaderTests, AnomalyThresholdsNAwareTests, LockedModeRestoreCoverageTests, McpPageContractTests, StatementCutSourceScanTests, plus *Census* and *Ratchet*: 788 total, 0 failed, 2 skipped.
• Full suites not run on this laptop; CI decides.

Note for the first test run: my first expectation for hash 0x was wrong (IsUnknown("0x") is false, since the all-zero test needs a digit after the prefix). That is existing behavior, now pinned as such (identity 0x), not a change.

Dev merge (Q2)

• Merged origin/dev into the branch as 4c3a3b4 (merge commit, no textual conflicts). Lock-file fix commit: 17f682c.
• tools/ui-smoke-wpf/packages.lock.json (dev's Dev CI: UiSmokeWpf lock file pins xunit.v3 4.0.2, and two live test classes skip without DARLING_TEST_PG (#5755) #5756 rewrote it; the project is not in the .sln): dotnet restore tools/ui-smoke-wpf/UiSmokeWpf.csproj --locked-mode failed with NU1004 (performancemonitor.analysis dependencies changed). Regenerated with --force-evaluate; the diff against origin/dev is only performancemonitor.analysis gaining "PerformanceMonitor.Common": "[3.10.0, )". Dev CI: UiSmokeWpf lock file pins xunit.v3 4.0.2, and two live test classes skip without DARLING_TEST_PG (#5755) #5756's pins are untouched.
• Locked-mode restores all exit 0: Darling.Tests, Lite.Tests, PerformanceMonitorLite, Installer.Tests, Dashboard.Tests, Darling Viewer, Darling Service, UiSmokeWpf, and PerformanceMonitor.sln. The solution restore rewrote no other lock file. git diff --name-only origin/dev...HEAD -- deprecated/ lists only the two packages.lock.json files.
• Lite.Tests and Darling.Tests rebuilt: 0 warnings, 0 errors. After the merge: Lite QueryHashIdentityTests 60/60, SameStatementPileupDetectorTests 41/41, *Census* 67/67; Darling SameStatementPileupSourceCensusTests 5/5, *Census* 549 (1 skipped, 0 failed), *Ratchet* 15/15 (Lite has no Ratchet class). 0 failures.

SECTION: None
ENTRY: none (internal: one statement-identity rule, no behavior change)

[claude]

Non-blocking, and the counterpart of this edge: PerformanceMonitor.Analysis/WithheldStatementMarker.cs:12-17 still carries the rationale this line refutes —

This is a copy of SensitiveStatements.PlaceholderText in PerformanceMonitor.Common, held here because the readers that must recognize it (the same-statement pileup detector, and the mute rule and incident grouper in PerformanceMonitor.Notifications) cannot reference Common without changing the packages.lock.json files of every project that references them.

That is now the only place in the tree asserting the constraint (grep -rn "cannot reference Common" → one hit), and it is false as of this diff: Analysis does reference Common, and the lock files changed. Two of its three named readers are now inside the closure that has Common — IncidentGrouping.cs:365 and MuteRule.cs:285 are in Notifications, which reaches Common through Analysis after this edge — so the sentence will mislead the next person who wants to alias the constant.

The body scopes the class itself out of this PR ("Notifications still uses it and StatementMarkerReaderTests pins it"), which I agree with: StatementMarkerReaderTests.cs:413 pins WithheldStatementMarker.Text against SensitiveStatements.PlaceholderText — the copy is pinned to the ORIGINAL, not to a sibling copy, so the #3684 "both must drift" shape does not apply to it. Only the stale reason sentence is worth a line.

[claude]

Non-blocking, but the body under-describes this regeneration. It says only that "Lite's lock drops a stale System.Diagnostics.EventLog entry"; the rewrite actually drops four things the body does not name:

• "System.Security.Cryptography.ProtectedData": "[10.0.11, )" out of this performancemonitor.plananalysis dependency list (this hunk) — in Lite/, deprecated/Dashboard/ and Darling/PerformanceMonitor.Darling.Viewer/;
• the System.Security.Cryptography.ProtectedData CentralTransitive entry (requested [10.0.12, ), resolved 10.0.11) in those same three;
• System.Diagnostics.EventLog out of Microsoft.Extensions.Logging.EventLog's dependency list and as a Transitive entry in deprecated/Dashboard/ too, not just Lite — and in Lite from the net10.0-windows7.0/win-x64 section as well.

The first two read oddly on their face, because PerformanceMonitor.PlanAnalysis/PerformanceMonitor.PlanAnalysis.csproj:26 still declares that package directly:

<PackageReference Include="System.Security.Cryptography.ProtectedData" VersionOverride="10.0.11" />

I do not think this is a defect: Lite.Tests/packages.lock.json, Darling/Darling.Tests/packages.lock.json, deprecated/Dashboard.Tests/packages.lock.json and tools/ui-smoke-wpf/packages.lock.json ALREADY carry the pruned shape on the base branch (their performancemonitor.plananalysis entries list only Microsoft.Data.SqlClient + PerformanceMonitor.Common, and neither package appears anywhere in them), so the pruning is what the pinned SDK's restore produces today and these three files were simply the stragglers — the same reason they were still on [3.9.0, ) while their siblings were on [3.10.0, ). Worth saying out loud in the body, though, since "drops a stale EventLog entry" reads as one incidental line and it is actually two packages across three files, one of them still declared in a csproj.

Nothing here is checkable from a read-only tree (no SDK in the review environment), so the lock-files job in build.yml:912-920 is the only witness.

[claude]

Verdict: LGTM

Claims

• VERIFIED — the ProjectReference and no-cycle claim — PerformanceMonitor.Analysis.csproj:18; Common's csproj has three PackageReferences and no ProjectReference.
• VERIFIED — StatementIdentity delegates, IsUnknownHash deleted — SameStatementPileupDetector.cs:466-467; grep -rn IsUnknownHash returns nothing, so no caller was orphaned.
• VERIFIED — NormalizeText / ShortHash stay live — ShortHash at :412 (ComputeIncidentId) and :524 (StoryPathHash), NormalizeText at :710 (Preview).
• VERIFIED — the correction that Common already nulls the withheld marker — QueryHashIdentity.cs:74-78 trim-ordinal-compares to SensitiveStatements.PlaceholderText (SensitiveStatements.cs:97), the same literal as WithheldStatementMarker.cs:22 and the same test as its IsMarker (:25). The removed check was redundant, not dropped.
• VERIFIED — "no input differs between the two definitions", read as two implementations rather than over a corpus: the deleted IsUnknownHash is character-for-character IsUnknown (same Length > 2 prefix strip, same all-zero scan); the deleted text path is the same trim / collapse / ToLowerInvariant loop, same SHA-256 first-16-hex, same text: prefix, same marker-before-fingerprint order; and QueryHash!.Trim().ToLowerInvariant() is KnownHash(h)?.ToLowerInvariant(). No input can diverge.
• VERIFIED — corpus and case counts — 15 hashes (QueryHashIdentityTests.cs:38-55) x 21 texts (:61-84) = the 315-pair loop at :109-131; 9 facts + 15 inline + 15 + 21 = 60 against the old 24. Nothing dropped.
• VERIFIED, stronger than claimed — the written-out fingerprints are arithmetically right, not just self-consistent: I recomputed all ten distinct normalized texts with sha256sum and every value at :66-83 matches SHA-256's first 16 hex (select 1 → 822ae07d4783158b, select 2 → 3cf988cc782b44bc, café → 850f7dc43910ff89, and the other seven). Case/whitespace-equal rows share a value because they normalize to one string.
• VERIFIED — the rename leaves nothing stale — 62%-similarity rename, identical corpus membership, and grep -rn QueryHashIdentityParityTests across the tree returns nothing.
• VERIFIED — lock bookkeeping — 3.9.0 → 3.10.0 matches Directory.Build.props:21 and no lock file still contains 3.9.0; under deprecated/ only the two lock files appear in gh pr diff --name-only; the whole ui-smoke hunk is performancemonitor.analysis gaining Common.
• VERIFIED — WithheldStatementMarker out of scope, and ENTRY: none — IncidentGrouping.cs:365 / MuteRule.cs:285 still call it, Darling.Tests/StatementMarkerReaderTests.cs:413 pins it to the Common original; and every identity, incident id and story fingerprint is byte-identical before and after, so no release note is owed. Only its stale rationale comment is worth a line (inline on the csproj).
• UNVERIFIED — the locked-mode restores all exiting 0 — no .NET SDK here, and the pruned entries are exactly the half of a lock file only a restore confirms.
• UNVERIFIED — "0 warnings, 0 errors" — needs a build. Common's NoWarn carries NU1701 for CredentialManagement (PerformanceMonitor.Common.csproj:13); Analysis's and Notifications' do not, and both gain that package transitively through this edge.
• UNVERIFIED — the test results (247 / 788 / 60 / 41 and the census counts) — a run, not a read; the 60 is verified above as a static count, the pass/fail is not.
• UNVERIFIED — the merge provenance (4c3a3b44a, "no textual conflicts", 17f682c4f) — this checkout is one merge commit, so the branch history is absent.

Riskiest lines

1. PerformanceMonitor.Analysis.csproj:18 — widest blast radius in the diff: seven lock files, plus ModelContextProtocol + CredentialManagement entering the restore graph of everything reaching Analysis (Notifications most of all, which reaches Common only through this edge). UNPINNED by anything runnable here; witnesses are build.yml:912-920 and Darling.Tests/LockedModeRestoreCoverageTests.
2. SameStatementPileupDetector.cs:467 — the value every group key, baseline match (:364) and ComputeIncidentId seed (:411) derives from; a divergence re-fingerprints recurring incidents instead of folding them. Pinned by Identity_AndTheDetectorsRowPath_FollowTheCorpus and the four Pileup_* facts.
3. Lite/packages.lock.json:790 and its twins in deprecated/Dashboard/ and the Viewer — performancemonitor.plananalysis now omits a package PerformanceMonitor.PlanAnalysis.csproj:26 still declares directly. UNPINNED from a read-only tree; the four lock files already regenerated on the base branch carry the identical pruned shape, which is the evidence it is right.

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." — applies, and this diff is the remedy, not the offence: the copy is gone and :467 calls the one definition. The surviving WithheldStatementMarker.Text is pinned against the ORIGINAL (StatementMarkerReaderTests.cs:413), not a sibling copy, so the both-must-drift shape misses it.
• "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)." — does not apply: nothing moved behind internal; StatementIdentity stays public (:466) and QueryHashIdentity is public (QueryHashIdentity.cs:33), so Darling.Tests/SameStatementPileupSourceCensusTests.cs:189 keeps compiling.
• "A pin that encodes the lie … Ask what a pin would fail ON (#3699)." — does not apply to the new pins: absolute values, recomputed independently (QueryHashIdentityTests.cs:66-83). The weaker shape was the OLD test, which compared two implementations to each other.
• "A roster-based census does not see the file it does not roster (#3699)." — does not apply: the detector's census derives from the tree and names the changed file by path (SameStatementPileupSourceCensusTests.cs:48, :161), and its assertions are negative — deleting code cannot add a Query Store token or a table.
• "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: the edit is in the shared libraries both SKUs compile, so there is no per-SKU copy to miss; Darling.Tests/LongQueryUnknownHashTests.cs reads the same definition Lite's does.
• "An expression-bodied member holding a { } property pattern is read SHORT by the … member walk (#3667)." — does not apply: :466-467 is expression-bodied but its body is a single call, no property pattern.

Divergence (blind read vs PR body)

One, an omission rather than a contradiction. The body accounts for the lock rewrite as version refreshes plus "Lite's lock drops a stale System.Diagnostics.EventLog entry". It also drops the System.Security.Cryptography.ProtectedData edge out of performancemonitor.plananalysis AND its CentralTransitive entry in three lock files (Lite, deprecated/Dashboard, Darling Viewer) while PerformanceMonitor.PlanAnalysis.csproj:26 still declares that package directly, and drops System.Diagnostics.EventLog from deprecated/Dashboard's lock too and from Lite's win-x64 section as well as its net10.0-windows7.0 one. My blind read flagged those as the one part of the diff the change itself does not explain; they are almost certainly correct regeneration output, since the base branch's already-regenerated lock files carry the same pruned shape. Detail inline on Lite/packages.lock.json. Everything else matches what I found before reading the body, including the self-reported 0x correction.

Reviewer checklist

• The lock-files job (build.yml:912-920): every locked-mode restore exits 0. Only a real restore under the global.json SDK pin confirms the pruned ProtectedData / EventLog entries — and deprecated/Installer*'s three lock files still carry both packages, so if the pruning is SDK-driven they are the next stragglers.
• Build Analysis and Notifications and look for a NEW NU1701 from CredentialManagement now that Common is in their restore graph. Warning-only, but the body claims 0 warnings and neither project carries Common's suppression.
• The full Lite.Tests and Darling.Tests suites, not run locally per the body — especially SameStatementPileupDetectorTests and both SKUs' LongQueryUnknownHashTests, which exercise the identity through a real detector/alert path rather than the corpus.

[claude]

Follow-up pass, range 17f682c4f..0bfc68b97.

This range is a single merge commit (0bfc68b97, parents 17f682c4f = previously-reviewed BASE, and 3f3b9c1d7 = origin/dev tip) plus the five dev commits it pulls in (3f3b9c1d7, 378fed75a, b66a39c40, 93f7ae606, cdf872759). None of those five were authored on this branch — they're pre-existing merged PRs from dev (#5705, #5757, #5762, #5751, #5740).

Checked whether the merge interacts with this PR's own change:

• git diff 3f3b9c1d7 0bfc68b97 --stat (dev-tip vs. merge-head) shows only the files this PR already touched before the merge (QueryHashIdentity.cs, SameStatementPileupDetector.cs, the renamed test file, PerformanceMonitor.Analysis.csproj, and the packages.lock.json set) — i.e. the merge-head differs from dev's tip only by this PR's own, already-reviewed content.
• None of those files appear in the dev-side commits' changed-file list (the 185-file BASE..HEAD diff), so there's no overlap between what the author changed and what the merge brought in — no conflict resolution, no interaction to re-review.

No new authored lines, no new public signatures, no new SQL/collector/alerting code in this range. Per the follow-up-pass instructions, I'm leaving the dev-sourced content alone and filing no findings against it.

[claude]

Verdict: LGTM

Claims

• UNVERIFIED — the PR body's "Dev merge (Q2)" section describes merge 4c3a3b44a and lock-fix commit 17f682c4f, both already inside BASE (17f682c4f) and graded by the prior full-pass review on this PR. The body makes no claim about this range's merge (0bfc68b97); there is nothing new to grade.

Riskiest lines

1. None — this range adds no authored lines. 0bfc68b97 is a merge commit; its only non-dev content is this PR's pre-existing, already-reviewed diff (confirmed via git diff 3f3b9c1d7 0bfc68b97 --stat, which shows only QueryHashIdentity.cs, SameStatementPileupDetector.cs, the renamed test, the .csproj edge, and the lock files — all already graded under head 17f682c4f).

Traps near this diff

• No trap applies: the range contributes zero authored SQL, collector, alerting, or analysis code. The five non-merge commits in range (3f3b9c1d7, 378fed75a, b66a39c40, 93f7ae606, cdf872759) are pre-existing dev commits (merged PRs #5705, #5757, #5762, #5751, #5740), not authored on this branch, and none of their changed files overlap this PR's own changed files — so the merge needed no conflict resolution and nothing in this PR's code interacts with what it brought in.

Divergence (blind read vs PR body)

None — the PR body does not describe this range's merge, so there is no claim to diverge from. The blind read confirms the merge is file-disjoint and content-trivial relative to this PR's own (already-reviewed) diff.

Reviewer checklist

• Confirm CI is green on 0bfc68b97 for the five dev-originated commits' own build/test coverage (already presumably verified when each landed on dev); nothing here is specific to this PR beyond a clean merge.