feat(tools): share the native-command guard, and report which Copilot reviews are outstanding

[MikeGrier]

Stage 3 of peeling #56 apart, after
#79 (the probe report sink) and
#80 (the long-path probe pair).

Nine files, no crate touched. Everything here is tools/, its CI job, and the design note that
records the two decisions.

1. The native-command guard, shared

Under Windows PowerShell 5.1, a native command that writes to stderr while
$ErrorActionPreference is Stop raises a terminating error when its stderr is redirected with
2>&1. PowerShell 7 does not -- which is why this class of defect survives review and CI.

run-numa-spikes.ps1 had two such captures and no guard. The shape of that failure is what makes it
worth a PR rather than a one-line fix:

cargo --quiet writes nothing to stderr on a clean build, so under 5.1 the script ran to
completion for as long as every spike was healthy. It threw only when cargo did write there
-- a warning, or a failed compile -- which is exactly the case the script exists to report. The
throw landed before $buildExit was assigned, so the broken-instrument branch never ran: no
transcript, no summary, and the one artifact somebody downloads to diagnose a rotted spike was the
one case that never produced it.

Measured both ways against a deliberately uncompilable crate:

host	unguarded	guarded
Windows PowerShell 5.1	threw RemoteException, captured nothing	exit 101, all 7 diagnostic lines
PowerShell 7	fine	fine

Why it is dot-sourced and not a module

The guard already existed as three hand-copied copies, so this shares it -- and how it is shared
is a correctness requirement.

A scriptblock carries the session state it was created in. Invoke-Native { cargo build } builds
that scriptblock in the caller's scope, so & $Command runs it there; a .psm1 copy sets
$ErrorActionPreference in the module's scope and the flip never reaches the call. Measured with
the identical body in a module: under 5.1 seven of eight cases failed, while all eight passed under
PowerShell 7.

A module can be made to work via $PSCmdlet.SessionState.PSVariable.Set(...), and that was
measured working on both hosts. It is rejected because the guard would then rest on a subtlety that
looks removable -- anyone simplifying it back to a plain assignment reintroduces a defect that still
passes on PowerShell 7 and in CI.

test-common.ps1 runs its eight cases in the invoking host, re-invokes itself in the other, and
treats a missing host as a failure rather than a skip -- a single-host pass is not the claim it
exists to make. The sabotage job now runs under both pwsh and powershell, because it ran
pwsh-only, which is precisely why this was invisible.

run-mutants.ps1 was checked for the same defect and needs no change -- it redirects nothing,
confirmed by experiment on both hosts.

2. The Copilot review scanner

PR #56 has 197 Copilot reviews and nothing said which were dealt with. Findings arrive in two shapes
and only one has state:

• Inline comments become threads that can be resolved -- durable, visible, queryable.
• Suppressed comments exist only as prose in the review body's <details> block and create no
  thread. A review is not a reactable object either: POST /pulls/{n}/reviews/{id}/reactions
  returns 404 while the same call on an inline comment succeeds. So nothing anywhere records that
  a suppressed finding was read.

-MarkProcessed closes that gap with a marker comment:

<!-- copilot-review-processed: 5136043258 -->

An HTML comment, so it does not render; on the pull request rather than in a file or a session, so it
survives a new machine, contributor, or agent session; read back by the script, so a handled review
stops being reported. A -Summary is required alongside it, because a marker with no account of
what was done is a claim with no evidence.

Exercised end to end on #80: six threads resolved, one suppressed-only review marked, re-scan clean
at exit 0.

Deliberately not included

• Write-Report is not consolidated. Six scripts define one and they are not duplicates --
  they differ in level vocabulary (warn/warning, bad/error, plus good, note, detail,
  heading) and in rendering, with two emitting GitHub Actions annotations and four emitting console
  colours. Merging them would change six tools' output to remove a duplication that is only apparent.
• The M34.4 checklist bookkeeping. main has no M34 milestone -- it lives in the source branch's
  documentation cluster -- so the stub and its archive entry travel with that stage rather than being
  half-landed here.

Verification

Both hosts, run rather than reasoned about:

• test-common.ps1 -- 8 cases, passing on 7 and 5.1.
• test-run-sabotage.ps1 -- all shards pass under both hosts.
• run-numa-spikes.ps1 -- completes on both.
• Sabotage-verified: delivering the guard as a module turns the new suite red on 5.1 (7 of 8) while
  staying green on 7, so the suite detects the regression it was written for.
• check-encoding.ps1 615 files clean; check-workflow-refs.ps1 56 references resolve.

[Copilot]

🟢 Approval recommended

The changes are self-contained to tooling/CI, add cross-host test coverage for the 5.1-specific failure mode, and do not introduce correctness or safety issues in the reviewed diffs.

Pull request overview

This PR improves the reliability and maintainability of the repository’s PowerShell tooling by centralizing the Windows PowerShell 5.1 native-stderr capture guard (via dot-sourced shared support) and adding a script that reports which GitHub Copilot review findings remain outstanding, including suppressed-only reviews that otherwise have no durable “handled” state.

Changes:

• Introduce tools/common.ps1 as the shared, dot-sourced home for Invoke-Native / ConvertTo-OutputLines, and update multiple tools to use it for native command capture under Windows PowerShell 5.1.
• Add tools/test-common.ps1 and wire CI to validate the guard across both PowerShell hosts (pwsh + Windows PowerShell 5.1), plus run the sabotage harness tests under both shells.
• Add tools/scan-pr-reviews.ps1 and corresponding design notes to track unresolved Copilot review threads and to mark suppressed-only review bodies as processed via PR comments.

File summaries

File	Description
tools/common.ps1	New shared, dot-sourced helper implementing the 5.1 native stderr capture guard and output normalization.
tools/test-common.ps1	New cross-host test suite validating Invoke-Native behavior and refusing to pass if the other host is missing.
tools/test-run-sabotage.ps1	Dot-sources common.ps1 and routes native git/shell capture through Invoke-Native to avoid 5.1 terminating errors.
tools/soak-flush-barrier.ps1	Removes in-file guard copies and dot-sources common.ps1 to eliminate drift across capture sites.
tools/run-sabotage.ps1	Dot-sources common.ps1 and uses Invoke-Native for git calls that redirect stderr under Stop.
tools/run-numa-spikes.ps1	Dot-sources common.ps1 and uses Invoke-Native for cargo build/run captures to behave consistently on 5.1.
tools/scan-pr-reviews.ps1	New tool that scans Copilot reviews/threads and supports durable “processed” markers for suppressed-only findings.
DESIGN-NOTES.md	Records the dot-sourcing requirement for shared tools support and the Copilot review ledger convention.
.github/workflows/ci.yml	Adds test-common.ps1 and runs the sabotage harness tests under both pwsh and Windows PowerShell 5.1.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The new documentation in tools/common.ps1 and DESIGN-NOTES.md contradicts the intended "no restoration is needed/attempted" behavior and should be corrected to match the implementation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

scan-pr-reviews.ps1 should explicitly fail fast when gh is missing to avoid misclassifying a broken instrument as a JSON/parse error.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are confined to tools/CI/docs, add cross-host coverage for the 5.1-specific failure mode, and don’t alter crate code or public APIs.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes consistently centralize the measured 5.1-safe native-capture guard, add cross-host validation in both tests and CI, and introduce the review scanner without introducing observable correctness issues in the updated scripts.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The changes are cohesive, well-documented, and the added cross-host CI coverage directly validates the correctness-critical PowerShell 5.1 behavior the refactor is intended to enforce.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new review-marker mechanism in tools/scan-pr-reviews.ps1 can unintentionally honor quoted/example markers in normal PR comments, which can incorrectly suppress outstanding reviews unless marker parsing is tightened.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tools/scan-pr-reviews.ps1:186

• Marker parsing currently honors any <!-- copilot-review-processed: ... --> anywhere in a PR comment body, which can accidentally retire a review if someone with write access quotes a marker as an example. Consider bracketing tool-emitted markers with explicit begin/end sentinels so only intentional marker blocks are recognized.

This issue also appears on line 318 of the same file.

tools/scan-pr-reviews.ps1:319

• The scan treats any marker substring in an issue comment as authoritative. If a maintainer quotes a marker in normal discussion, it will be honored and suppress the review. After adding begin/end sentinels in the emitted marker comment, restrict recognition to markers inside that sentinel block (and ideally anchored to whole lines) to avoid accidental matches.

    $markers = [regex]::Matches((Get-Text $c.body), '<!--\s*copilot-review-processed:\s*(\d+)\s*-->')
    if ($markers.Count -eq 0) { continue }

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

scan-pr-reviews.ps1 currently discards gh stderr even on failure, making broken-instrument errors significantly harder to diagnose.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

[MikeGrier]

Both suppressed findings addressed in 4b0134d.

They were the same finding at two sites: marker recognition matched the marker text anywhere
in a comment body, which cannot distinguish a comment that IS a marker from one that merely
MENTIONS one. A maintainer quoting a marker in discussion, or pasting the example out of the
script's own documentation, would have silently retired a review -- and since a suppressed-only
review has no other state anywhere, that deletes the only record a finding was never read.

Markers are now emitted inside :begin / :end sentinels and honoured only on a line of their
own within such a block, with the sentinels defined once so emitter and reader cannot drift.
Both documentation examples now spell the id as a placeholder rather than digits, so copying an
example verbatim -- sentinels included -- matches nothing.

Verified 9 discrimination cases on both PowerShell hosts, 9 of 9: real marker and a two-id block
honoured; bare marker, quoted inline, quoted in a fenced block, the placeholder example, a marker
sharing a line with a sentinel, an unterminated block, and an empty body all rejected.

Not firing before the fix -- the only marker in the repository was the legitimate one on PR #80 --
but this branch publishes the literal marker text in its own PR body and DESIGN-NOTES, so the
material for an accidental retirement was already written. That PR #80 marker was bare and
correctly stopped counting after the change; it has been re-marked in the new format.

Worth noting these two findings were suppressed, so they created no review thread. Without the
scanner this pull request adds, nothing would have surfaced them.

[Copilot]

🟢 Approval recommended

The changes consistently apply the shared native-command guard across tools, add host-sensitive tests/CI coverage, and introduce the review-scanning tool without altering crate codepaths.

Review details

Suppressed comments (1)

tools/scan-pr-reviews.ps1:180

• Invoke-GitHubJson discards gh stderr via Invoke-NativeStdout, so on a failure (non-zero exit) the Exit-Broken message can end up missing the real diagnostic text (which gh typically prints to stderr). Consider capturing merged stdout+stderr only on the failing path so the error message is actionable without compromising the JSON parse on success.

    $text = Invoke-NativeStdout { gh @Arguments }
    $code = Get-LastExitCode
    if ($null -eq $code -or $code -ne 0) {
        Exit-Broken "gh $($Arguments -join ' ') failed (exit $code): $($text -join ' ')"
    }

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Addressed in 2cc009b. The approval's one suppressed finding was a regression I introduced two
commits earlier: routing gh through the stdout-only capture fixed JSON corruption and silently
emptied every broken-instrument message.

Measured which stream gh uses, because it is not uniform and that decides the fix. A REST 404
puts its JSON body on stdout (so that case still reported, which is what made it look fine),
but a network failure or a usage error leaves stdout EMPTY and says everything on stderr --
producing failed (exit 1): with nothing after the colon, blank exactly when the cause is
least guessable.

Invoke-NativeSplit now captures both streams separately: stdout stays clean for the parse,
stderr is available for the report, preferring stderr and falling back to stdout so the 404's
body is not lost either. The marker-post path is converted too -- it was merging, so a successful
post could have echoed a warning as though it were the comment URL.

Worth recording: the first implementation used a file redirect (2>$path) and was wrong on
Windows PowerShell 5.1, which writes the FORMATTED error record there -- source line, caret
ruler, CategoryInfo -- rather than raw stderr. Two of the new cases failed on 5.1 and passed on
7; the cross-host suite this PR adds is what caught it. The shipped version merges and partitions
by record type, which is lossless on both hosts.

Eighteen cases now, passing on both hosts, and the failure message is identical on each.

[MikeGrier]

Already fixed when this review ran. It reviewed 4b0134d and its one suppressed finding -- that
Invoke-GitHubJson discards gh stderr, leaving the Exit-Broken message without the real
diagnostic -- is the same one raised in review 5137310730 and addressed in 2cc009b, which was
pushed just after.

The exact line it quotes no longer exists: Invoke-GitHubJson now uses Invoke-NativeSplit,
which captures both streams separately so stdout stays clean for the JSON parse while stderr is
available for the failure message, preferring stderr and falling back to stdout so a REST 404's
JSON body is not lost either.

Recorded here rather than left open, because a suppressed finding creates no thread and would
otherwise keep being re-reported by this tool with nothing to say it was handled.

[Copilot]

🟢 Approval recommended

The changes are cohesive, well-scoped to tooling/CI/design-notes, and the only issue found is a small comment/name mismatch that doesn’t affect behavior.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tools/run-sabotage.ps1:219

• This comment says Get-RepoRoot goes through Invoke-Native, but the code now uses Invoke-NativeStdout. Keeping the helper name accurate matters here because the choice (merge vs stdout-only) is part of the contract being explained.

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Lite

[MikeGrier]

Addressed in c40a2cb.

The suppressed finding was restatement drift: converting Get-RepoRoot to the stdout-only
capture changed the call and left the comment above it still saying "Through Invoke-Native".
That distinction is exactly the contract the comment exists to explain -- merge for a
transcript, discard for output that is parsed -- so a wrong name there is worse than no name.
Two comment blocks had also stacked up with the older one first, which is how the stale name
survived being read; they are now one block that says why the call is guarded at all and
separately why it is stdout-only.

Swept the class rather than the reported line: all 37 Invoke-Native* call sites across
tools/, comparing the helper each calls against the helper named in its own preceding comment
block. Exactly one mismatch, the reported one. The other two converted sites read correctly.

No behaviour change; verified on both hosts anyway.

[Copilot]

🔵 Needs a closer look

The new review-scanner misclassifies non-collaborator permission checks (HTTP 404) as “unverifiable,” which can emit a misleading “token lacks push access” warning and should be corrected before approval.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

tools/scan-pr-reviews.ps1:347

• Get-RetireAuthority treats ANY non-zero gh api .../collaborators/<login>/permission result as unverifiable (and later prints the "token has no push access" warning). But the GitHub API returns 404 for a user who is not a collaborator, which is an ordinary "denied" outcome; classifying that as unverifiable will produce a misleading warning whenever a non-collaborator posts a marker comment.

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Lite