Count files the reviewer actually read, and gate the manifest so it stops rotting - #1137
Open
Alexandre Zollinger Chohfi (azchohfi) wants to merge 15 commits into
Open
Count files the reviewer actually read, and gate the manifest so it stops rotting#1137Alexandre Zollinger Chohfi (azchohfi) wants to merge 15 commits into
Alexandre Zollinger Chohfi (azchohfi) wants to merge 15 commits into
Conversation
manifest.json mixed two conventions: 280 entries were repo-root-relative (tests/, tools/) while 206 project entries were src/-relative. No entry resolved under both bases, so the split was invisible to anything testing a single base -- auditing all 633 against repo-root alone reports 280 hits and 353 false deaths. That ambiguity is not just an auditing hazard. A two-base resolver silently accepts a path that is wrong but happens to resolve under the other base, so the invariant a CI gate could enforce would be weaker than it looks. Prefixing the 206 makes "every manifest path resolves from the repo root" a statement that cannot drift. Mechanical: only a leading "src/" is inserted, and only on lines inside a "files" array, so scope.included/excluded are left alone. Dead paths are left untouched here; they are retargeted separately. Verified with a differential oracle rather than a count: the ordered (batch, resolved-path) set is byte-identical before and after -- 486 entries either way. A count would have been vacuous, and mutation-checking the oracle confirms it: repointing one entry at a different real file leaves the count at 486/486 and is caught only by the set comparison; dropping an entry is caught by both. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
138 of the manifest's 147 dead entries point at files that still exist under a different path. Left alone the reviewer hands each one to an agent that cannot open it, and still counts it as reviewed -- so this is silently unreviewed code, not merely a miscount. Resolved by replaying the manifest's own history rather than by matching file names. manifest.json is tracked, and its first version (227fb77, then at reviewer/manifest.json) holds 645 paths of which all 645 really existed. Each original path was walked forward through git's rename graph to where it lives today, then cross-checked with `git log --follow` ancestry on the candidate. Name matching was tried first and rejected as unsound: it maps PropertyGrid/Factories.cs onto Reactor.Advanced/Factories.cs, which is simply a different file. Replay walks rename chains and never consults leaf names, so it gives the provable answer, PropertyGrid/PropertyGridFactories.cs. Nine retargets do change file name across the move -- TreeChartDsl.cs to Charts.Tree.cs, Md4cEnums.cs to MarkdownEnums.cs, EaseTests.cs to EaseChartingExtraTests.cs and so on -- and each was confirmed against an explicit R-record. Two targets share a leaf with an unrelated file (Reactor/Animation/Curve.cs, a gallery Treemap.cs sample); replay distinguishes them. Verified: every target asserted present on disk before the write, JSON re-parsed after it, no duplicate path introduced within a batch, no batch emptied. Dead entries drop 147 -> 9, and the 9 that remain are genuine judgement calls handled separately. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
These are the residue after the mechanical retargets: paths whose file was deleted rather than moved, or where more than one live file could plausibly be meant. Each needed a decision about where coverage went, so they are separated from the provable retargets and recorded individually. Retargeted on identity evidence: Md4cTypes.cs -> Reactor.Advanced/Markdown/MarkdownTypes.cs (x3) WinAppDriverHelper.cs -> Infrastructure/WinAppUi.cs Neither shows a git rename link, because in both cases the content changed too much for the similarity detector. Both are nonetheless a delete and an add in a single commit -- deafff9 for the Md4c public API rename, e523b4b for the Appium/WinAppDriver to winapp ui migration -- and in each changeset a sibling file moved the same way and *was* recorded as a rename (Md4cEnums.cs -> MarkdownEnums.cs). A sibling where the detector did fire is what raises a delete/add pair from coincidence to evidence. Retargeted on coverage, not identity -- no rename link and none implied: SelfTestBatch.cs -> tests/Reactor.SelfTests/SelfTestBatch.cs ReactorCharting.Tests/CurveTests.cs -> tests/Reactor.Tests/D3/CurveTests.cs CurveTests.cs had two live candidates; the D3 one is chosen because all 25 of its sibling DuctD3.Tests files resolve there. A reader should treat these two as weaker than the four above. Dropped, coverage already represented in the same batch: PropertyGrid/PropertyDescriptor.cs (x2) -- deleted with no successor; TypeMetadata.cs, TypeRegistry.cs and ReflectionTypeMetadataProvider.cs remain and carry the type-system concern those batches describe. ReactorCharting.Tests.csproj -- project folded into Reactor.Tests, whose csproj is already listed in the same batch. Also fixes the one stale batch description: test-quality-batch-11 still advertised "WinAppDriver helpers" after e523b4b removed WinAppDriver in June. Note on scope: e523b4b replaced WinAppDriverHelper.cs with six new infrastructure files, and the manifest lists none of them. Retargeting restores one of six, which is better than zero but is not full coverage of that migration. The other five are deliberately not added -- batch membership encodes human judgement about what belongs together, and widening a batch silently is the same objection that ruled out generating the manifest. Called out in the PR description instead. Verified: 630 entries, 0 dead paths, no batch emptied, no duplicate within a batch. The only batches that shrank are general-batch-13 and general-batch-27, 4 files to 3. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
run-review.ps1 never opens the files it reviews -- it interpolates the manifest
list into a prompt and asks an agent to read them -- yet it wrote the report
header straight from the manifest:
**Files reviewed**: $($BatchObj.files.Count)
So a path that resolves to nothing was handed to an agent that could not open
it, and the batch still reported it as reviewed. The metric counted intent, not
outcome. On the manifest as it stood before this branch that was 147 of 633
entries: a full run claimed 633 files reviewed while roughly a quarter could not
be opened at all.
Each batch now resolves its paths first, prompts with only the files that exist,
and reports "N of M" with any shortfall named in the report body and warned to
the console. A run-level coverage line is printed up front so an operator learns
the manifest is stale before spending an LLM run rather than after.
Deliberately not fail-fast. A stale path aborting all 91 batches would turn a
reporting defect into an availability one, and would punish the legitimate case
of a file deleted between manifest edits. Strictness belongs in CI, where
blocking is cheap; that gate is added separately.
Also fixes $RepoRoot, which was Split-Path -Parent $PSScriptRoot and so
evaluated to <repo>/tools rather than the repo root. Harmless while it only fed
a Write-Host, but this commit makes it load-bearing for path resolution.
The three near-identical report/progress blocks are now one definition. They
were duplicated for a real reason -- ForEach-Object -Parallel runspaces do not
inherit the caller's functions, which is also why the script re-inlines prompt
building instead of calling its own Build-AgentPrompt -- so the shared helpers
are held as source text and dot-sourced into each runspace via $using:.
Verified by mutation rather than by reading: pointing one entry at a
non-existent file turns "8 of 8" into "7 of 8", names the file in the report and
in a console warning, and the run still completes. Both the specialist and the
general parallel paths were exercised, and a clean full dry run reports
630 of 630.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixing the 147 stale paths was a point-in-time repair. The manifest rots
continuously -- from ordinary file movement between the rare occasions anyone
edits it, not from any single bad commit -- so without a gate it is stale again
within a couple of refactors and nobody finds out. That is what happened last
time: the reviewer reported full coverage for months while a quarter of its
paths pointed at nothing.
Four invariants, each with a message that says what to do rather than what
failed:
every path resolves from the repo root
no batch is empty (an empty batch still burns a run and reports clean)
no duplicate path in a batch (inflates the count; across batches is fine and
intentional -- several agents review one file)
paths are repo-root-relative, forward-slash
A fifth test asserts the manifest loads and is non-trivial. The other four all
have the shape "no offenders in this collection", which an empty collection
satisfies for free, so if the manifest ever failed to load they would go green
while checking nothing.
Blocking lives here rather than in run-review.ps1 on purpose: aborting a
91-batch LLM run over one stale path trades a reporting defect for an
availability one. CI is where stopping is cheap, so CI is where it is strict and
the run merely reports its shortfall.
Headless: JsonDocument plus File.Exists, no WinUI types. Reuses
GallerySources.RepoRoot() rather than copying the walk-up-to-Reactor.slnx logic.
Mutation-checked rather than assumed -- a green run against a correct manifest
proves nothing on its own. Each assertion was independently broken and confirmed
to redden: a non-existent path fails the resolve test naming batch and path, a
repeated entry fails the duplicate test, emptying general-batch-25 fails the
empty-batch test, and a backslash path fails the form test. 5 tests, non-zero
matched count, so the filter is not passing vacuously.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Two pieces of documentation asserted things the repo stopped doing, which is the same class of defect as the stale paths themselves -- text nobody re-checked. manifest.json's scope.included was left inconsistent by the normalization: it still named Reactor/ and Reactor.Cli/ in the old src-relative form while every file entry is now repo-root-relative, so the declared scope no longer described the actual entries. It also listed tests/ReactorCharting.Tests/ (folded into Reactor.Tests), Reactor/Charting/ (now src/Reactor.Advanced/), *.sln (the repo is .slnx) and excluded selfhost/ (no longer exists), while omitting Reactor.Advanced, Reactor.Devtools, Reactor.SelfTests and Reactor.Markdown.TestRenderer, all of which the entries actually cover. Rewritten from the prefixes the 630 entries really use. README claimed "485 files across 91 batches". The real figure is 477 distinct files across 630 entries -- entries exceed files because a file may sit in several batches on purpose, so different agents review it from different angles, which the old wording gave no way to tell. Also fixes a scope bullet list that still advertised markdown as part of core and ReactorCharting as a separate library, and an example fix-list path in the superseded convention. Documents the coverage reporting and the CI gate, including why the run reports a shortfall while the test blocks, so the next person does not "fix" the run script by making it fail fast. Per-agent batch counts in the README were checked against the manifest and are correct (safety 5, lifecycle 12, interop 3, security 4, test-quality 35, general 32); left alone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Alexandre Zollinger Chohfi (azchohfi)
requested a review
from Chris Anderson (codemonkeychris)
as a code owner
August 27, 2026 04:27
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 04:28
View session
Contributor
📦 Build metricsArtifact sizes for Packages (compressed .nupkg)
Assemblies in Microsoft.UI.Reactor
Assemblies in Microsoft.UI.Reactor.Advanced
Assemblies in Microsoft.UI.Reactor.Devtools
✅ smaller / |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes the reviewer tooling’s “files reviewed” metric reflect what can actually be opened (resolved paths), fixes manifest drift by retargeting stale entries, and adds a CI gate to prevent the manifest from silently accumulating dead paths again.
Changes:
- Update
run-review.ps1to resolve manifest paths against the repo root, prompt agents with only existing files, and report “N of M” coverage (including a missing-path list). - Normalize and retarget
tools/reviewer/manifest.jsonpaths (repo-root-relative) and align scope/docs with current repo layout. - Add
ReviewerManifestTeststo fail CI if any manifest entry is stale, malformed, duplicated within a batch, or if any batch is empty.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/reviewer/run-review.ps1 | Resolves batch file paths against repo root; reports honest coverage and warns on missing paths; shares helpers across parallel runspaces. |
| tools/reviewer/README.md | Documents the new coverage reporting behavior and updates scope/path examples. |
| tools/reviewer/manifest.json | Retargets/normalizes batch paths to repo-root-relative; updates included scope to current directories/projects. |
| tests/Reactor.Tests/Tooling/ReviewerManifestTests.cs | Adds CI gate ensuring all manifest batch paths resolve and remain well-formed/non-vacuous. |
Suppressed comments (1)
tools/reviewer/run-review.ps1:251
- This report placeholder line includes a Unicode em dash (—) inside a here-string. For the same PowerShell 5.1 CP1252-decoding reason as above, non-ASCII characters inside string literals can make the whole script fail to parse. Use ASCII punctuation here.
**Status**: DRY RUN — no findings generated
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review flagged em dashes in two string literals as a PowerShell 5.1 hazard: BOM-less UTF-8 decoded as CP1252 can turn the byte into a string delimiter and break parsing. The decoding mechanism is real, but replacing those characters would not have made the script run under 5.1, and would have been churn on lines this branch did not touch. Both flagged strings are pre-existing and identical on main, and the file holds 7 em dashes both before and after this branch -- the change is em-dash-neutral. More to the point, the script calls ForEach-Object -Parallel (lines 292 and 405), which is PowerShell 7.0+ only, so 5.1 cannot run it whatever the string literals say. README has always listed PowerShell 7+ as a prerequisite; nothing enforced it. So the requirement is now declared rather than assumed. Under 5.1 the script stops with "cannot be run because it contained a #requires statement for Windows PowerShell 7.0" instead of a confusing parse error pointing at a quote character -- which addresses the failure the reviewer was actually protecting against, and does so for every 7.0-only construct in the file rather than two punctuation marks. Verified both directions: parses clean and still runs under pwsh 7 (dry run reports "8 of 8"), and Windows PowerShell 5.1 now refuses it with the explicit version message. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 04:41
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
tools/reviewer/run-review.ps1:131
- Resolve-BatchFiles currently treats directories as “resolved” because it uses Test-Path without constraining the path type. Since batches are supposed to list files, and ReviewerManifestTests uses File.Exists (leaf-only), this can make the script’s runtime coverage reporting disagree with the CI gate if a directory ever slips into the manifest.
$resolved = [System.Collections.Generic.List[string]]::new()
$missing = [System.Collections.Generic.List[string]]::new()
foreach ($f in $Files) {
if (Test-Path -LiteralPath (Join-Path $Root $f)) { $resolved.Add($f) } else { $missing.Add($f) }
}
tools/reviewer/README.md:185
- This scope bullet says
src/vscode-reactor/, but the manifest’s scope.included issrc/vscode-reactor/src/and the batches currently reference onlysrc/vscode-reactor/src/extension.ts. As written, the README overstates what’s actually covered (e.g. package.json/tsconfig.json aren’t in scope per the manifest).
- `src/Reactor/` — Core framework (reconciler, elements, hosting, flex, yoga, animation, property grid)
- `src/Reactor.Advanced/` — Charting/D3 and Markdown
- `src/Reactor.Cli/` — CLI tool
- `src/Reactor.Devtools/` — Devtools and preview server
- `src/Reactor.Localization.Generator/` — Source generator
- `src/vscode-reactor/` — VS Code extension
- `tests/` — All test projects (reviewed for quality, not just correctness)
tests/Reactor.Tests/Tooling/ReviewerManifestTests.cs:50
- This asserts that a 'batches' property exists, but not that it’s actually an array. If the manifest shape changes to make 'batches' a non-array value, EnumerateArray() will throw and the failure won’t carry the intended actionable message.
Assert.True(
doc.RootElement.TryGetProperty("batches", out var batches),
$"{ManifestRelativePath} has no 'batches' array — the manifest shape changed and this gate needs updating.");
return batches.EnumerateArray()
Contributor
🧪 Merged coverageCoverage for
No coverage change beyond the noise floor. ✅ ✅ higher / |
All three were suppressed comments rather than blocking ones, but each is real. Resolve-BatchFiles used bare Test-Path, which returns true for a directory. The CI gate uses File.Exists, which does not. So a directory in the manifest would have been counted as reviewed by the run and rejected by the gate -- the two instruments meant to agree about coverage would have disagreed, which is the same class of defect this branch exists to fix. Now -PathType Leaf. Confirmed by mutation: replacing an entry with src/Reactor/Core (a real directory) makes the run report 7 of 8 and name it as missing, matching what the gate would do. ReviewerManifestTests asserted that a 'batches' property exists but not that it is an array, so a shape change would surface as an InvalidOperationException out of EnumerateArray() instead of the actionable message the assertion was written to give. Now checks ValueKind explicitly. Mutation-checked: with batches set to a string, all five tests fail with "has a 'batches' property of kind String, expected an array" rather than an exception. README overstated the VS Code extension scope as src/vscode-reactor/ when scope.included is src/vscode-reactor/src/ and the batches reference only extension.ts -- so package.json and tsconfig.json read as covered when they are not. That is the same "text asserting something the repo does not do" problem the rest of this branch is about, so it is worth the one-line fix. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Collaborator
Author
|
Picked up the three suppressed comments from round two — all three were real. Pushed as 1. 2. 3. README overstated the VS Code extension scope. Right again — For the record on the earlier em-dash thread: I didn't apply that one as written, because both flagged strings are pre-existing on |
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 04:56
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/Reactor.Tests/Tooling/ReviewerManifestTests.cs:160
PathsUseRepoRootRelativeForwardSlashFormflags any path containing ".." anywhere. That can create false positives if a filename legitimately contains ".." (even though it’s rare) because it’s not actually a parent-directory traversal. It’s safer to detect..as a path segment (e.g. starts with../, contains/../, or ends with/..).
var malformed = Entries()
.Where(e => e.Path.Contains('\\')
|| e.Path.StartsWith('/')
|| e.Path.StartsWith("./")
|| e.Path.Contains(".."))
The path-form check flagged any entry containing "..", which is broader than the rule it is meant to express. The hazard is a path escaping the repo root; a file legitimately named Md4cParser..Block.cs is not that, and would have been rejected for a problem it does not have. Now splits on "/" and looks for ".." as a whole segment. Worth keeping precise rather than deleting, because this is the only check that can catch traversal at all: src/Reactor/Core/../Core/Element.cs passes File.Exists, so the resolve test goes green on it and the form test is the sole line of defence. Verified both directions. The three traversal shapes -- leading ../, embedded /../, trailing /.. -- are all still caught, and the mutation above reddens the form test while the resolve test stays green, confirming which assertion is doing the work. Md4cParser..Block.cs no longer matches, where the substring check said it did. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 05:17
View session
Two wording fixes and one gate tightening from review round four.
The pre-flight coverage line said "630 of 630 files resolve", but that total
counts manifest entries, and a file may sit in several batches on purpose --
630 entries across 477 distinct files. Reporting entries as files is the same
kind of imprecision this branch exists to remove, particularly now that the
README spells the distinction out. The per-batch "Files reviewed" header is left
alone: within one batch the two counts cannot diverge, because the gate forbids
a batch listing a path twice.
The path-form check now rejects rooted paths via Path.IsPathRooted, which also
subsumes the leading-slash test it replaces.
On that last one the review's stated mechanism did not hold, and the comment
records why. Rooted paths were said to escape the repo root because
Join-Path/Path.Join would honour them; measured, both concatenate --
Path.Join("C:\repo", "C:/Windows/...") yields "C:\repo\C:/Windows/...", which
fails File.Exists. Escaping is Path.Combine's behaviour, and this gate does not
use it, so the invariant was never unenforced. What was true is that such a path
got diagnosed by the resolve test as a missing file rather than by the form test
as a malformed path, and a backslash-rooted path was already caught while a
forward-slash one was not. Worth fixing for the diagnosis, not for a hole.
Mutation-checked: C:/Windows/System32/notepad.exe now fails the form test naming
batch and path, and the dry run reports "630 of 630 entries resolve".
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 05:33
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/Reactor.Tests/Tooling/ReviewerManifestTests.cs:76
- This test currently reads/parses the manifest more than necessary: it calls Batches() and then Entries(), and Entries() calls Batches() again (another file read + JSON parse). Deriving entries from the already-loaded batches keeps the guard test fast and avoids redundant I/O.
var batches = Batches();
Assert.True(batches.Count >= 50, $"expected the reviewer manifest to define many batches, found {batches.Count}");
Assert.True(Entries().Count >= 400, $"expected the reviewer manifest to list many files, found {Entries().Count}");
ManifestLoadsAndIsNonTrivial read and parsed manifest.json three times: once via Batches(), then twice more because Assert.True evaluates its message argument eagerly, so Entries() ran for both the condition and the message -- and Entries() calls Batches() internally. Now derives the entry count from the batches already in hand. Also says "entries" rather than "files" in that message, for the same reason the pre-flight line changed: a file may appear in several batches, so the total is entries. Behaviour is unchanged, and mutation-checked to confirm the guard still does its job: truncating the manifest to a single batch still fails with "expected the reviewer manifest to define many batches, found 1". That guard is the one protecting the other four tests from passing vacuously, so it is worth re-proving after touching it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 05:51
View session
All three catch blocks wrote their own minimal header, so a batch that threw produced a report with no "Files reviewed" line and no missing-path list. The coverage metric vanished exactly when a run went wrong -- which is the same defect this branch is about, just relocated to the error path, and arguably worse there: a failed batch is precisely when someone needs to know how much of it was even openable. All three now go through New-ReportHeader, so a FAILED report carries the same "N of M" and shortfall list as a successful one. The two parallel catch blocks also gain -Encoding UTF8, which they were missing while every other write in the file had it -- the em dash in a batch description would have been mangled on the error path only. Exercised rather than reasoned about: claude is not installed in this environment, so running a batch without -DryRun takes the real catch path. With a stale path injected as well, the failed report now reports "7 of 8" and names src/Reactor/Core/GhostFile.cs. Before this change that report carried neither. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 06:10
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/Reactor.Tests/Tooling/ReviewerManifestTests.cs:159
- The claim about PowerShell Join-Path always concatenating even when the child path is rooted is misleading (Join-Path’s behavior with rooted ChildPath is provider/version dependent, and docs commonly describe rooted ChildPath as overriding the base). Since this test already rejects rooted paths via Path.IsPathRooted, it’s safer to avoid asserting Join-Path semantics here and just explain why rooted paths are rejected.
/// A rooted path such as <c>C:/x.cs</c> does not actually escape the repo root here — both
/// <c>Path.Join</c> and PowerShell's <c>Join-Path</c> concatenate rather than letting a rooted
/// second argument win, so it fails <c>File.Exists</c> and the resolve test catches it. (That
/// is <c>Path.Combine</c>'s behaviour, which this gate deliberately does not use.) It is
/// rejected here anyway so the diagnosis names the real defect — a malformed path — instead of
/// reporting a missing file.
tools/reviewer/run-review.ps1:245
- Invoke-ReviewAgent builds the prompt before resolving the file scan, which forces a second Resolve-BatchFiles call and can make the prompt list and the report header drift. After updating Build-AgentPrompt to accept a precomputed scan, pass the scan in and build the prompt from it.
$prompt = Build-AgentPrompt -BatchObj $BatchObj
$scan = Resolve-BatchFiles -Files $BatchObj.files -Root $RepoRoot
Invoke-ReviewAgent resolved the batch twice: once inside Build-AgentPrompt to build the prompt's file list, and again for the report header. Two resolutions of the same batch can disagree -- if the working tree changes between them, or if the resolution rule is edited later and one call site is missed -- and the disagreement would be exactly the thing this branch exists to prevent: a header claiming coverage the prompt did not ask for. The scan is now computed once and threaded through, with -Scan mandatory on Build-AgentPrompt so a future caller cannot reintroduce a second resolution by omission. The remarks on PathsUseRepoRootRelativeForwardSlashForm also claimed PowerShell's Join-Path concatenates a rooted child path. I had measured that, but on one platform and one version, and then stated it as general behaviour -- the review is right that it is provider- and version-dependent. The claim was also out of place: this is a C# test, and it composes paths with Path.Join, whose behaviour is documented and is what actually matters here. Narrowed to Path.Join, keeping the warning against switching to Path.Combine, which would introduce the escape the check is accused of missing. Verified the prompt and header still agree on the same numbers: with a stale path injected, pre-flight reports "7 of 8 entries", the progress line reports "7 of 8 files -- 1 missing", and the report header reports "7 of 8" naming GhostFile.cs. Gate still 5/5. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 06:29
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/Reactor.Tests/Tooling/ReviewerManifestTests.cs:59
- Batches() guards the top-level 'batches' property shape, but then assumes every batch object has a string 'id' and an array 'files' (GetProperty + EnumerateArray). If the manifest shape drifts, these will throw (KeyNotFoundException / InvalidOperationException) and the failure won’t explain what changed.
Consider adding explicit TryGetProperty/ValueKind checks for 'id' and 'files' (including the batch id in the error) so CI failures remain actionable.
return batches.EnumerateArray()
.Select(b => (
Id: b.GetProperty("id").GetString()!,
Files: b.GetProperty("files").EnumerateArray().Select(f => f.GetString()!).ToArray()))
.ToList();
Batches() guarded the top-level 'batches' array but then assumed each element
was an object carrying a string 'id' and an array 'files'. A manifest that drifts
from that shape would have surfaced as a bare KeyNotFoundException from
GetProperty or an InvalidOperationException from EnumerateArray -- an exception
in CI that tells the next person a test blew up, not what to fix. This gate's
entire value is the second thing.
Each batch is now read through a helper that checks element kind, id, files, and
every file entry, naming the batch id once it is known so the message points at
a specific batch rather than an index. Same reasoning as the batches ValueKind
check already here; this just extends it one level down.
Mutation-checked all three, since an error path nobody triggers is an error path
nobody has tested:
batch missing id -> "batch at index 0 has no string 'id'"
files not an array -> "batch 'safety-batch-1' has no 'files' array"
non-string entry -> "batch 'safety-batch-1' has a non-string entry at index 0
(kind Number)"
No behaviour change on a well-formed manifest: still 5/5, 630 entries, 0 dead.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve-BatchFiles passed every manifest entry straight to Join-Path, so the run accepted path shapes the CI gate rejects. Demonstrated, not theorised: with the worktree at .../azchohfi-bookish-fiesta, src/Reactor/../../../azchohfi-bookish-fiesta/README.md satisfies Test-Path and was counted as reviewed, while ReviewerManifestTests fails the build on it for containing a ".." segment. Two instruments meant to agree about coverage, disagreeing -- the same defect as the directory case fixed earlier, and the thing this branch exists to prevent. Resolve-BatchFiles now applies the gate's rule -- repo-root-relative, forward-slash, no traversal -- before touching the filesystem, and treats a malformed entry as Missing so it is reported rather than silently dropped. Rooted paths are rejected by the same check. On this platform they already failed to resolve, because Join-Path concatenated rather than letting the rooted child win, but review correctly pointed out that behaviour is provider- and version-dependent, so this deliberately does not depend on it. Verified in both directions: the traversal entry above now reports 7 of 8 and is named in the missing list, and the real manifest still resolves 630 of 630 with no false rejections. Gate 5/5. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Alexandre Zollinger Chohfi (azchohfi)
August 27, 2026 06:55
View session
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tools/reviewer/run-review.ps1reported**Files reviewed**: $($BatchObj.files.Count)— a number taken straight frommanifest.json. The script never opens those files; it interpolates the list into an agent prompt and asks the agent to read them. So the metric counted intent, not outcome: 147 of 633 manifest entries resolved to no file, and every one was still reported as reviewed. Because nearly all of those files still exist elsewhere, that was silently unreviewed code, not a miscount.This fixes the data (147 → 0 dead paths), makes the metric honest (
N of M, naming the misses), and adds a CI gate so it cannot rot unnoticed again.Linked issue / spec
N/A — follow-up carved out of #1129 to keep that PR scoped.
Test plan
tests/Reactor.Tests/Tooling/ReviewerManifestTests.cs(5 tests)dotnet test tests/Reactor.Tests— 13,610 total, 0 failed, 64 skippedrun-review.ps1 -DryRunbefore/after, exercising both the specialist and general parallel pathsdotnet build Reactor.slnx -c Release— could not run locally, see RiskMutation evidence
A green run on a correct manifest proves nothing, so each check was broken on purpose and confirmed to redden:
general-batch-258 of 8→7 of 8, file named in report and console warning, run completesThe first row is the reason the normalization is gated on set equality rather than a count — a count would have waved it through. A fifth test asserts the manifest loads and is non-trivial, because the other four all have the shape "no offenders in this collection", which an empty collection satisfies for free.
Risk / breaking changes
No public API change. No product code touched — the only compiled artifact is one new test file.
dotnet build Reactor.slnx -c Releaseis unverified locally. It fails here with 234NU1900+ 24NU1301, all "unable to load the service index for nuget.org". It fails at restore, so nothing compiled — a green Release is therefore not something I'm in a position to assert. What I can say: a filtered scan forCS|MSB|WMC|IL|PRIerrors returns 0, and Debug compiles and runs the full suite clean. CI is the real gate here.run-review.ps1 -DryRunis not side-effect-free. Its Phase 3 writesreports/fix-list.mdunconditionally, and a dry run during this work overwrote that tracked file. It was restored and verified byte-identical toorigin/main(blob43bca78f), and it appears in none of these commits — but the trap is real for the next person.Why it rotted — drift, not renames
My first diagnosis blamed the repo-wide naming passes that rewrote manifest paths. That was wrong, and the correction matters because it changes what the fix should be.
manifest.jsonis tracked. Its first version (227fb776) holds 645 paths, all 645 of which really existed. Measuring each subsequent manifest edit against a fixed tree — its own commit, so the repo can't drift underneath the measurement:ea0cb9bfDuct→Reactora2ed3e50monaco movedeafff9anaming cleanup99b608adpre-launch1aa6e021slnx158eab4d(#1129)Every delta is ≤ 0. No manifest edit ever introduced rot —
ea0cb9bfrepaired 450 paths and left 88 imperfect. The naming passes under-fixed; they didn't damage.The dominant channel is ordinary repo drift between the rare occasions anyone touches the manifest. Same manifest blob (
1aa6e021), two different trees:This is why the durable fix is a CI gate rather than a smarter rename-following tool: the gate is cause-agnostic and catches drift from any source. A rename-specific fix would have missed the bigger channel entirely.
How the 147 were retargeted
By replaying the manifest's own history, not by matching file names. Each original (real) path was walked forward through git's rename graph to where it lives today, then cross-checked with
git log --followancestry on the candidate.Name matching was tried first and rejected as unsound: it maps
PropertyGrid/Factories.csontoReactor.Advanced/Factories.cs, which is simply a different file. Replay never consults leaf names and gives the provable answer,PropertyGrid/PropertyGridFactories.cs. Nine retargets legitimately change filename across the move (TreeChartDsl.cs→Charts.Tree.cs,Md4cEnums.cs→MarkdownEnums.cs,EaseTests.cs→EaseChartingExtraTests.cs, …); each was confirmed against an explicitRrecord. Two targets share a leaf with an unrelated file (Reactor/Animation/Curve.cs; a galleryTreemap.cs) — replay distinguishes them.138 resolved this way. The remaining 9 needed a judgement call, and the two kinds are kept separate because a reader six months out needs to know which are provable:
Identity — no rename link (content changed past git's similarity threshold) but a delete and an add in a single commit, with a sibling in the same changeset where the detector did fire:
Md4cTypes.cs→Reactor.Advanced/Markdown/MarkdownTypes.cs(×3) —deafff9a, siblingMd4cEnums.cs→MarkdownEnums.csWinAppDriverHelper.cs→Infrastructure/WinAppUi.cs—e523b4bcCoverage, not identity — treat these two as weaker; no rename link and none implied:
SelfTestBatch.cs→tests/Reactor.SelfTests/SelfTestBatch.csReactorCharting.Tests/CurveTests.cs→tests/Reactor.Tests/D3/CurveTests.cs(chosen because all 25 siblingDuctD3.Testsfiles resolve there)Dropped, coverage already represented in the same batch —
PropertyGrid/PropertyDescriptor.cs(×2) andReactorCharting.Tests.csproj. Onlylifecycle-batch-8(6→5),general-batch-13(4→3) andgeneral-batch-27(4→3) shrink; nothing is emptied.Known gap: orphaned files (the second failure mode)
e523b4bcreplacedWinAppDriverHelper.cswith six new infrastructure files —InputInjector.cs,Keys.cs,UiElement.cs,UiaPropertyReader.cs,WinAppException.cs,WinAppUi.cs. The manifest covers none of them. Retargeting restores one of six.The other five are deliberately not added. Batch membership encodes human judgement about which files are worth reviewing together; widening a batch silently is the same objection that ruled out auto-generating the manifest.
AppTestBase.csandTestSession.csare already in that batch and were modified by the same migration, so the harness isn't unreviewed.This is the manifest's second failure mode, and this gate is structurally blind to it: the gate proves every listed path resolves, but says nothing about files that exist and are listed nowhere. Worth a follow-up asking whether a future gate should flag orphaned sources. Out of scope here.
Design decision
Report the shortfall in the script; fail fast in CI.
Runtime fail-fast was considered and rejected: aborting a 91-batch parallel LLM run because one path rotted trades a reporting defect for an availability one, punishes the legitimate case of a file deleted between manifest edits, and is awkward to surface coherently from inside a
ForEach-Object -Parallelrunspace. Strict where blocking is cheap (CI), resilient where it's expensive (a run).Also in these commits:
src/-relative, with no entry resolving under both — so any single-base audit reported hundreds of false misses, which is exactly how this bug survived. A two-base resolver would also silently accept a path that's wrong but happens to resolve under the other base.$RepoRoot, which wasSplit-Path -Parent $PSScriptRootand evaluated to<repo>/tools, not the repo root. Harmless while it only fed aWrite-Host; load-bearing now that paths are resolved against it.ForEach-Object -Parallelrunspaces don't inherit the caller's functions, which is also why the script re-inlines prompt building instead of calling its ownBuild-AgentPrompt. The shared helpers are now held as source text and dot-sourced into each runspace via$using:.scope.includedand the README with reality: the scope block still namedReactor/in the superseded form, listedtests/ReactorCharting.Tests/andselfhost/(both gone) and*.sln(repo is.slnx), and omitted four projects the entries actually cover. README's "485 files" is now 477 distinct across 630 entries, with the entries-exceed-files distinction spelled out.