Review findings on merged pull requests — the 2026-09-19 sweep

A review finding on a merged pull request is not exempt: the code shipped, and nobody answered. Twenty findings across nine merged PRs (#95, #96, #119, #147, #154, #158, #166, #183, #195) were swept on 2026-09-19 and each was replied to on its own thread. This page keeps what a reply cannot: the measurements, so the next sweep does not have to redo them, and the three defect shapes that collapsed a dozen findings into one fix each.

The denominator: twenty was a SUBSET, not the backlog

🚨 The sweep was briefed as "twenty findings across nine merged PRs". That is what was treated; it is not what exists. Measured 2026-09-19, after all twenty were answered, over every merged pull request in this repository:

merged PRs scanned: 186
untreated Copilot review threads still on merged PRs: 111

— i.e. 131 in total, of which this sweep closed 20. A thread counts as untreated when a top-level Copilot review comment carries no reply at all; the query is self-consistent, since all nine briefed PRs now report 0 remaining. The heaviest are #20 (6), #6 (5), #30 / #114 / #215 (4 each), and a long tail of one and two. By far the most common subject is scripts/resolve-platform.py — the vendored copy whose canonical lives in core — which is a single conversation spread over many PRs rather than many defects.

The number is here so the next sweep starts from a denominator instead of a hand-picked list, and so "we treat all findings" can be measured rather than asserted. The detector:

gh api "repos/$R/pulls/$n/comments?per_page=100" --jq '
  [.[]] as $all
  | [$all[] | select(.user.login | test("copilot"; "i"))] as $cop
  | [$all[] | select(.in_reply_to_id != null) | .in_reply_to_id] as $answered
  | [$cop[] | select(.in_reply_to_id == null)
           | select((.id|tostring) as $i | ($answered|map(tostring)|index($i)) == null)] | length'

🚨 And one trap in the posting half, which produced a false pass here: gh api -f "body=@file" sends the literal string @file. -f/--raw-field never reads a file; -F/--field does. All twenty replies posted as the bare filename and returned a perfectly normal comment id, so the POST looked like a success — the only thing that caught it was reading one comment back. Post with -F "body=@file", and verify by re-reading the comment and comparing it with the file.

What collapsed

Four measurements that a reply would have lost

1. A doubled <summary> emits NO compiler diagnostic

The finding text (and the fleet's instinct) says CS1571. It does not. Measured on .NET 10, GenerateDocumentationFile=true, -warnaserror, a method carrying two consecutive <summary> blocks: 0 Warning(s), 0 Error(s). The emitted XML carries both elements under one <member>:

<member name="M:C.M">
    <summary>Stale one.</summary>
    <summary>Real one.</summary>
</member>

So the cost is not a build signal, it is that every doc consumer shows the first summary — the stale one — and, in all eleven cases here, a second member was left with no documentation at all. That is the part Copilot's suggested remedy ("collapse to a single block") would have thrown away.

The root cause, and the repair rule. In eleven of the twelve sites a new member was inserted between an existing doc comment and the member it documented. So the orphan is the displaced member's summary, and the repair is to MOVE it down, not delete it. The tell is mechanical and worth keeping: each of the eleven files contained exactly one public/internal member with no doc comment, and its name matched the orphan's subject every time (Report_RendersMovements, ComplianceTable, MinRateOnLine, Economics_TowerDrawsEveryLayer, ClaimSnapshot_IsShapeIndependent, EveryString_ShipsGerman, Discounting_ReducesTheNominalAmount, Contractual_ChainReconcilesExactly, Files_RenderParseable, PlanYear_ReadsTheStoredFact, Mapping_IsReadOffTheDimension). Only the twelfth (EconomicsReport.OriginMarkdown) was a true stale duplicate of the same member's own summary, and only that one was deleted. Detector, and the control for the repair:

# 12 before the sweep, 0 after — and 11 undocumented members before, 0 after
python3 - <<'EOF'
import pathlib
for p in pathlib.Path('.').rglob('*.cs'):
    if str(p).startswith('legacy/'): continue
    lines = p.read_text(encoding='utf-8').splitlines(); i = 0
    while i < len(lines):
        if lines[i].strip().startswith('///'):
            j = i
            while j < len(lines) and lines[j].strip().startswith('///'): j += 1
            if sum('<summary>' in l for l in lines[i:j]) > 1: print(f"{p}:{i+1}")
            i = j
        else: i += 1
EOF

2. page.request.post() RESOLVES on 4xx — a .catch() after it tolerates everything

e2e/00-bootstrap.spec.ts seeded the demo coupon and ended the call .catch(() => undefined) under a comment promising that "a 401/403 is tolerated". Playwright's APIRequestContext rejects only on a network fault, so the catch never ran for an HTTP refusal and every status was tolerated — including the 400 an out-of-date CouponContent produces, which is exactly the fault #183 fixed by adding tier. The suite then failed four specs later as "the coupon does not work", i.e. as a product paywall fault rather than a broken fixture.

Measured against a local server returning 400 (@playwright/test 1.63):

RESOLVED -> status=400 ok=false body="schema validation failed: ..."
  the guard !ok && status not in {401,403} -> throws?  400: true   401: false   403: false   200: false

The seed now reads the status and throws with it and the body; 401/403 stay tolerated, and a network fault propagates (correct — the suite has already been driving that host with page.goto).

3. select: — only content is conditional

OriginMarkdown projected content on two live queries over a deal's whole subtree while reading only path / name / icon. Dropping content is safe, and the reason is in core: StorageAdapterMeshQueryProvider.DropUnprojectedContent — "Only content is conditional: every other MeshNode field is returned regardless of the select list, exactly as the SQL generators project every other column unconditionally." So a shell read keeps Icon without naming it, and Postgres projects NULL::jsonb AS content instead of hydrating the payload on every emission. The repo's rule is unchanged and now has both halves written down where the queries live: a content-bearing read names content; a chain you cannot prove content-free takes no select: at all.

4. An allow file with no stale detection is worse than no allow file

check-covers.py shipped cover-contrast.allow described as "seeded ABSENT, and it may only SHRINK" — with nothing enforcing the second half, and a docstring describing the prose allow file's format as Repo/Module while the code matched a bare module name. Both were unfalsifiable in the same way. Control, on the clean tree, with one bogus entry in each file:

Two rules now hold for both files: an entry whose finding is gone is stale, and an entry that names nothing in this repo is stale. --self-test drives the ratchet in both directions (a live finding is silenced and not called stale; a fixed one is called stale and reports nothing; a partly-fixed palette does both at once without either masking the other).

One thing found and NOT fixed here

🚨 scripts/run-node-tests.py cannot run at all locally. Line 390 calls cc.usings_union(sources, ai_available) and the platform's compile-check.py — the file scripts/platform-script.py fetches, and core's current copy too — defines no such function:

AttributeError: module 'compile_check' has no attribute 'usings_union'

CI is unaffected (the mesh gate runs the harness inside the platform image, and presentation runs only --self-test, which does not reach run_set), which is precisely why this went unnoticed: the one gate a developer is told to run before pushing exits on a traceback, and no green wall moves. The --self-test half still works and was used as the control for the declaration-regex fix below.

The declaration regex, for the record

collisions_within's duplicate-type detector reads source TEXT, so a form it cannot parse is a collision it cannot see — and the reader then gets the ~150-line CS0101 wall the check exists to replace, with nothing saying the detector missed it. Two bugs, both live since #119:

• readonly was not an accepted modifier, so public readonly record struct X(…) matched nothing — the form LossModelling/FrequencySeverityLossModel uses for DiscretePoint and DistributionStatistics, two live types the detector was blind to;
• record struct / record class captured the second keyword as the type name, so two unrelated record structs read as one duplicate type called struct, and the partial detector had the same bug — which made a legitimately split partial record struct look like a duplicate.

Controls: the self-test went from 7 failing cases to 0 (23 cases, nine declaration forms, each asserted both to be refused when duplicated and never to be reported under a keyword name), and a regex-vs-regex diff over all 711 .cs files outside legacy/ loses 0 names and gains exactly 2 — DiscretePoint and DistributionStatistics. The modifier set deliberately stops at the ones that may precede a top-level type: adding private/protected made ApplicabilityRules, a private sealed class nested in both Planning's and Ifrs17's ScopeApplicabilityResolver, look like a duplicate — a false refusal, since two same-named types under two different outer types are legal C#.