From 56338772e74a67067f454333a825ce20bf248c45 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Mon, 7 Sep 2026 21:56:54 -0400 Subject: [PATCH 01/11] feat(tools): share the native-command guard, and report which Copilot reviews are outstanding Two pieces of tooling, peeled off the long-running deferred-namespace-ops branch so they can be reviewed on their own. Neither touches any crate. ## The native-command guard 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 survives review and CI. `run-numa-spikes.ps1` had two such captures and no guard, and the shape of that failure is what makes it dangerous. `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: unguarded, 5.1 threw and captured nothing; guarded, exit 101 with all seven diagnostic lines. The guard already existed as three hand-copied copies, so this shares it -- and HOW it is shared is a correctness requirement, not a style choice. A scriptblock carries the session state it was created in, so `Invoke-Native { cargo build }` runs in the CALLER's scope while a `.psm1` copy flips the preference in the MODULE's scope; 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. Dot-sourcing puts the function in the caller's own scope, where the plain assignment does reach it. A module can be made to work through `$PSCmdlet.SessionState.PSVariable.Set`, measured working on both hosts, and is rejected: the guard would rest on a subtlety that looks removable, and simplifying it back reintroduces a defect that still passes on PowerShell 7 and in CI. `test-common.ps1` covers it on BOTH hosts -- it runs its cases in the invoking host, re-invokes itself in the other, and treats a missing host as a failure rather than a skip. The sabotage job now runs under `pwsh` and `powershell` for the same reason: 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. ## The review scanner PR #56 has accumulated 197 Copilot reviews and nothing said which had been dealt with. Findings arrive in two shapes and only one has state: inline comments become threads that can be RESOLVED, while suppressed comments exist only as prose in the review body 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. `scan-pr-reviews.ps1` reports both, and `-MarkProcessed` closes the gap with a marker comment on the pull request: 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 PR #80: six threads resolved, one suppressed-only review marked, re-scan clean at exit 0. ## Deliberately not included The `Write-Report` functions are NOT consolidated. Six scripts define one and they are not duplicates -- they differ in level vocabulary 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 is not here either: `main` has no M34 milestone, which 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. Verified on both hosts: `test-common.ps1` passes, the sabotage suite passes under 7 and 5.1, `run-numa-spikes.ps1` completes, and the encoding and workflow-reference checks pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/ci.yml | 16 +++ DESIGN-NOTES.md | 88 +++++++++++++ tools/common.ps1 | 92 +++++++++++++ tools/run-numa-spikes.ps1 | 35 +++-- tools/run-sabotage.ps1 | 9 +- tools/scan-pr-reviews.ps1 | 248 +++++++++++++++++++++++++++++++++++ tools/soak-flush-barrier.ps1 | 55 ++------ tools/test-common.ps1 | 179 +++++++++++++++++++++++++ tools/test-run-sabotage.ps1 | 43 +++--- 9 files changed, 696 insertions(+), 69 deletions(-) create mode 100644 tools/common.ps1 create mode 100644 tools/scan-pr-reviews.ps1 create mode 100644 tools/test-common.ps1 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 46fc4c80..211c59e3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -86,9 +86,25 @@ jobs: runs-on: windows-latest steps: - uses: actions/checkout@v7 + # The shared native-command guard, before the suites that depend on it: a + # failure here explains a failure below rather than being diagnosed twice. + # This one step covers both hosts on its own -- it re-invokes itself under + # the other shell and fails if it cannot find one. + - name: Run test-common.ps1 + shell: pwsh + run: ./tools/test-common.ps1 - name: Run test-run-sabotage.ps1 shell: pwsh run: ./tools/test-run-sabotage.ps1 + # And again under Windows PowerShell 5.1, which is not a duplicate of the + # step above. The harness and its suite both capture native output, and the + # rule that makes that capture terminate under `Stop` exists ONLY on 5.1 -- + # so a defect in that path is invisible to a pwsh-only job, which is + # exactly how one reached `main` and survived review. `powershell` is the + # Windows PowerShell 5.1 that ships on every windows runner. + - name: Run test-run-sabotage.ps1 (Windows PowerShell 5.1) + shell: powershell + run: ./tools/test-run-sabotage.ps1 # The edition, MSRV, and pinned channel are declared once in Cargo.toml and # rust-toolchain.toml, then restated a dozen times -- in this file's `msrv` diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index 16ff0f4b..c16c9bda 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1645,3 +1645,91 @@ the caller's own parsing mode -- `GetFullPathNameW`'s output, or it after normalisation preserves what the path meant; doing it before silently reinterprets it. Both crates arrived at this independently, which is why it is written down once. + +## Shared support in `tools/` is dot-sourced, never a module + +`tools/common.ps1` holds the support every script there shares, and it is delivered by +dot-sourcing (`. (Join-Path $PSScriptRoot 'common.ps1')`). **Converting it to a `.psm1` and +importing it silently breaks it on Windows PowerShell 5.1, which is the only host it exists +to protect**, so the delivery mechanism is a correctness requirement rather than a +preference. + +The single thing it carries today is `Invoke-Native`, the guard against 5.1's treatment of +native stderr: there, 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. The guard flips the preference to `Continue` around the call and +restores it afterwards. + +**Why a module fails.** A scriptblock carries the session state it was created in. +`Invoke-Native { cargo build }` builds that scriptblock in the *caller's* script scope, so +`& $Command` runs it there, not in the module's scope. A module copy sets +`$ErrorActionPreference` in the module's own scope, the flip never reaches the scriptblock, +and the native call still runs under `Stop`. Measured with the identical body in a `.psm1`: +under 5.1 seven of the eight cases in [tools/test-common.ps1](tools/test-common.ps1) failed, +while **all eight passed under PowerShell 7**. Dot-sourcing puts the function in the caller's +own scope, where the plain assignment does reach the call. + +A module *can* be made to work by reaching into the caller's session state +(`$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. Dot-sourcing makes the property hold by construction. + +**The test crosses hosts, and refuses to pass vacuously.** +[tools/test-common.ps1](tools/test-common.ps1) runs its cases in the invoking host and then +re-invokes itself in the other one, failing if it cannot find it. A single-host suite is +worthless for this defect class -- the whole hazard is that it is invisible on the host most +people run, and CI ran `shell: pwsh` only, which is how the original defect reached `main` +and survived review. The `sabotage harness tests` job now runs `test-run-sabotage.ps1` under +both shells for the same reason. + +**What is deliberately NOT shared: `Write-Report`.** Six scripts define a function by that +name, and they are *not* duplicates -- they differ in level vocabulary (`warn` against +`warning`, `bad` against `error`, plus `good`, `note`, `detail`, `heading`) and in rendering: +[run-numa-spikes.ps1](tools/run-numa-spikes.ps1) and +[soak-flush-barrier.ps1](tools/soak-flush-barrier.ps1) emit GitHub Actions annotations +(`::warning::`), the other four emit console colours. Consolidating them would mean unifying +those vocabularies, which changes the output of six tools to remove a duplication that is +only apparent. The shared name is a naming convention -- the repository's one-output-sink +rule -- not shared code, and it stays that way until some script needs another's rendering. + +## Which Copilot reviews are dealt with: resolve threads, and mark suppressed-only reviews + +A long-lived pull request accumulates hundreds of Copilot reviews -- PR #56 reached 196 -- and +GitHub records "this was dealt with" for only some of them. [tools/scan-pr-reviews.ps1](tools/scan-pr-reviews.ps1) +reports what is genuinely outstanding, and the convention below is what makes that report mean +something. + +**A review's findings arrive in two shapes, and only one of them has state.** + +- **Inline comments** become review threads, which can be **resolved**. That flag is durable, + visible, and queryable, so it is the tag -- there is nothing to invent. Resolve a thread when + its finding is dealt with, including when it is *refuted*: "we measured this and it is not a + defect" is a disposition, not an open question. +- **Suppressed comments** exist only as prose inside the review body's `
` block. They + create **no thread**, so there is nothing to resolve, and a review is not a reactable object + either: `POST /pulls/{n}/reviews/{id}/reactions` returns 404 while the same call against an + inline comment succeeds. Nothing anywhere records that a suppressed finding was read. + +**So suppressed-only reviews are tagged with a marker comment**, posted by the script: + +``` + +``` + +It is an HTML comment, so it does not render; it lives on the pull request rather than in a file +or a session, so it survives a new machine, a new contributor, and a new agent session; and the +script reads it back, which is what keeps the report from re-raising a review already handled. +The script requires a `-Summary` alongside it, because a marker with no account of what was done +is a claim with no evidence. + +**Reading the report.** It separates unresolved threads into *current* and *outdated*. Outdated +means the anchored line has since changed, which usually means the finding was fixed and the +thread simply never resolved -- so those are the cheap ones to clear, and they are listed only +under `-IncludeOutdated` to keep the default output about work that is actually open. + +**A marker asserts the review was read, so do not back-fill in bulk.** PR #56 carries 131 +reviews with suppressed comments and no marker. Almost all were addressed during the rounds +that followed them, but "almost all" is not evidence, and marking them wholesale would convert +an honest absence of information into a false record. Mark a historical review only when +somebody has actually read it. diff --git a/tools/common.ps1 b/tools/common.ps1 new file mode 100644 index 00000000..5b9ccda5 --- /dev/null +++ b/tools/common.ps1 @@ -0,0 +1,92 @@ +# Copyright (c) Mike Grier. +<# +.SYNOPSIS + Shared support for the scripts in `tools/`. Dot-source it; do not run it. + +.DESCRIPTION + Dot-sourced rather than imported as a module, and that is a correctness + requirement rather than a style choice. See "Why not a module" below before + converting this to a `.psm1`. + + Every script here that captures a native command's output needs the same + guard against Windows PowerShell 5.1's treatment of stderr, and that guard + had been copied into three scripts by the time it was written down. This is + the one copy. + +.NOTES + Usage, from any script in this directory: + + . (Join-Path $PSScriptRoot 'common.ps1') + + `$PSScriptRoot` is populated in a script body on both hosts. It is NOT + populated while evaluating a parameter default on a `[CmdletBinding()]` + script under 5.1, so keep this call in the body, as the scripts here do for + their own `-OutputDirectory` defaults. +#> + +# Render a captured record as plain text. +# +# `2>&1` wraps a native command's stderr in ErrorRecords. One of those +# stringifies to the literal text `System.Management.Automation.RemoteException`, +# which lands in the middle of a captured compiler diagnostic and makes a +# transcript less legible than the console output of the same failure. +function ConvertTo-OutputLines { + param([Parameter(ValueFromPipeline = $true)] $Record) + process { + if ($Record -is [System.Management.Automation.ErrorRecord]) { + $Record.Exception.Message + } + else { + "$Record" + } + } +} + +# Run a native command, capturing merged stdout+stderr as plain strings. +# +# 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. Flipping the preference to +# `Continue` around the call is what makes the capture work on both, and +# restoring it afterwards keeps `Stop` for everything that is not a native call. +# +# `$LASTEXITCODE` is global, so a caller still reads the command's exit code +# after this returns. That matters here: these scripts distinguish a broken +# instrument from a finding by exactly that code. +# +# ## Why not a module +# +# Moving this function into a `.psm1` and importing it SILENTLY BREAKS IT under +# 5.1, which is the only host it exists to protect. +# +# A scriptblock carries the session state it was created in. `Invoke-Native +# { cargo build }` builds that scriptblock in the CALLER's script scope, so +# `& $Command` runs it there -- not in the module's scope. A module copy of this +# function sets `$ErrorActionPreference` in the module's own scope, the flip +# never reaches the scriptblock, and the native call still runs under `Stop`. +# +# Measured, because the failure is invisible on the host most people run: with +# the identical body in a `.psm1`, 5.1 threw `RemoteException` and captured +# nothing while PowerShell 7 succeeded. Dot-sourcing lands the function in the +# caller's own scope, where the plain assignment below does reach the call, and +# both hosts then behave identically. +# +# A module CAN be made to work by reaching into the caller's session state +# (`$PSCmdlet.SessionState.PSVariable.Set(...)`), and that was measured working +# too. It is rejected because the guard would then depend on a subtlety that +# looks removable: anyone simplifying it back to a plain assignment would +# reintroduce a defect that still passes on PowerShell 7 and in CI. Dot-sourcing +# makes the property hold by construction instead of by counter-measure. +# +# [test-common.ps1](test-common.ps1) asserts this on both hosts. +function Invoke-Native { + param([Parameter(Mandatory = $true)][scriptblock] $Command) + $previous = $ErrorActionPreference + $ErrorActionPreference = 'Continue' + try { + & $Command 2>&1 | ConvertTo-OutputLines + } + finally { + $ErrorActionPreference = $previous + } +} diff --git a/tools/run-numa-spikes.ps1 b/tools/run-numa-spikes.ps1 index e94faf9d..4ea4f4a9 100644 --- a/tools/run-numa-spikes.ps1 +++ b/tools/run-numa-spikes.ps1 @@ -68,6 +68,23 @@ function Write-Report { } } +# `Invoke-Native` and `ConvertTo-OutputLines`, which every capture below goes +# through. Dot-sourced rather than imported: a module copy of that guard does +# not reach the scriptblock it is handed, and silently fails on 5.1 alone. The +# full argument, and the measurement behind it, is in that file. +# +# What it costs THIS script, recorded here because the shape is specific to the +# spikes: `cargo --quiet` writes nothing to stderr on a clean build, so under +# 5.1 this 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 this 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. Confirmed both ways +# against a deliberately uncompilable crate: unguarded, 5.1 threw and captured +# nothing; guarded, it returned exit 101 with all seven lines of diagnostic. +. (Join-Path $PSScriptRoot 'common.ps1') + $repoRoot = Resolve-Path (Join-Path $PSScriptRoot '..') $spikeDir = Join-Path $repoRoot 'crates\windows-ioring-sys\design-sessions\spikes' @@ -138,20 +155,20 @@ windows-sys = { version = "0.61.2", default-features = false, features = [$featu Write-Report "=== building $($spike.Name) ===" Push-Location $work try { - $build = & cargo build --quiet 2>&1 + $build = Invoke-Native { cargo build --quiet } $buildExit = $LASTEXITCODE $buildOutput = ($build | Out-String) if ($buildExit -ne 0) { # A build failure is a defect in the instrument, and is one of the # two things here worth failing over. Write-Report "spike $($spike.Name) failed to build" -Level error - # Echo the Out-String rendering rather than the raw objects. `2>&1` - # turns cargo's stderr into ErrorRecords, and one of those - # stringifies to the literal text `System.Management.Automation. - # RemoteException` in the middle of the compiler diagnostic. Piping - # the already-rendered text keeps the log and the transcript - # identical, instead of the artifact being the more legible of the - # two records of the same failure. + # Echo the same text the transcript gets, so the log and the + # artifact are two renderings of one capture rather than two + # records of one failure that a reader has to reconcile. + # `Invoke-Native` has already flattened the ErrorRecords `2>&1` + # produces into plain strings, so neither carries the stray + # `System.Management.Automation.RemoteException` this used to + # splice into the middle of a compiler diagnostic. $buildOutput.TrimEnd() -split "`n" | ForEach-Object { Write-Report $_.TrimEnd() } $instrumentFailures++ $sections.Add("### $($spike.Name)`n`n**FAILED TO BUILD** -- the instrument is broken, not the machine.`n") @@ -159,7 +176,7 @@ windows-sys = { version = "0.61.2", default-features = false, features = [$featu else { $built = $true Write-Report "=== running $($spike.Name) ===" - $output = & cargo run --quiet 2>&1 | Out-String + $output = Invoke-Native { cargo run --quiet } | Out-String $runExit = $LASTEXITCODE Write-Report $output } diff --git a/tools/run-sabotage.ps1 b/tools/run-sabotage.ps1 index ecf4d939..e2661478 100644 --- a/tools/run-sabotage.ps1 +++ b/tools/run-sabotage.ps1 @@ -171,6 +171,13 @@ param( Set-StrictMode -Version Latest $ErrorActionPreference = 'Stop' +# `Invoke-Native`, which the git calls below go through. Dot-sourced rather than +# imported: a module copy of that guard does not reach the scriptblock it is +# handed, and silently fails on Windows PowerShell 5.1 alone -- see +# [common.ps1](common.ps1), and [test-common.ps1](test-common.ps1) for the +# cross-host proof. +. (Join-Path $PSScriptRoot 'common.ps1') + $utf8NoBom = [System.Text.UTF8Encoding]::new($false) # Script scope so Invoke-Bounded reads it without threading it through three @@ -837,7 +844,7 @@ New-Item -ItemType Directory -Force -Path $OutputDirectory | Out-Null # without bound and looks like nothing until it does. $outputFull = [System.IO.Path]::GetFullPath($OutputDirectory) if ($outputFull.StartsWith($repoRootPrefix, [System.StringComparison]::OrdinalIgnoreCase)) { - git -C $repoRoot check-ignore -q -- $outputFull 2>&1 | Out-Null + Invoke-Native { git -C $repoRoot check-ignore -q -- $outputFull } | Out-Null if ($LASTEXITCODE -ne 0) { Exit-WithMessage (@( "-OutputDirectory is inside the repository but git does not ignore it:" diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 new file mode 100644 index 00000000..20857062 --- /dev/null +++ b/tools/scan-pr-reviews.ps1 @@ -0,0 +1,248 @@ +# Copyright (c) Mike Grier. +<# +.SYNOPSIS + Report which Copilot reviews on a pull request still need attention, and + record the ones that have been dealt with. + +.DESCRIPTION + A long-lived pull request accumulates hundreds of Copilot reviews, and + nothing in the GitHub UI says which have been dealt with. Judging by eye + does not scale, and judging by "did a commit follow it" is guesswork. This + reports the two signals that are real, and adds the one GitHub does not. + + A review carries findings in one of two shapes, and they need different + treatment because GitHub models only one of them: + + INLINE COMMENTS become review threads, which can be RESOLVED. That flag is + durable, visible in the UI, and queryable, so it is the tag for this shape + -- there is nothing to invent. A thread also reports `isOutdated`, meaning + the line it was anchored to has since changed; those are reported + separately, because an outdated finding is usually one that was fixed and + never resolved rather than one still waiting. + + SUPPRESSED COMMENTS exist only as prose inside the review body's `
` + block. They create no thread, so there is nothing to resolve; and a review + is not a reactable object either -- `POST /pulls/{n}/reviews/{id}/reactions` + is 404, while the same call on an inline comment succeeds. A review whose + findings were ALL suppressed therefore has no state anywhere saying it was + read, which is exactly the gap this script closes. + + For those, `-MarkProcessed` posts a pull-request comment carrying a marker: + + + + The marker is an HTML comment, so it does not render, and it lives on the + pull request rather than in a file or a session, which is what makes it + survive a new machine, a new contributor, and a new agent session. A later + run of this script reads those markers back and stops reporting the review. + +.PARAMETER Pr + The pull request number. + +.PARAMETER MarkProcessed + One or more review ids to record as processed. Posts a single comment + carrying a marker for each, with the summary as its visible text. + +.PARAMETER Summary + The visible text of the marker comment. Required with -MarkProcessed: + a marker with no account of what was done is a claim with no evidence. + +.PARAMETER IncludeOutdated + Also list unresolved threads whose anchor line has since changed. + +.EXAMPLE + .\tools\scan-pr-reviews.ps1 -Pr 56 + +.EXAMPLE + .\tools\scan-pr-reviews.ps1 -Pr 56 -MarkProcessed 5136043258 ` + -Summary 'Both suppressed findings were measured and refuted; see commit abc1234.' +#> +[CmdletBinding()] +param( + [Parameter(Mandatory = $true)][int] $Pr, + [long[]] $MarkProcessed, + [string] $Summary, + [switch] $IncludeOutdated +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +. (Join-Path $PSScriptRoot 'common.ps1') + +$script:Owner = 'MikeGrier' +$script:Name = 'windows-threadpool-sys' + +# The single output sink, per the repository's one-output-sink rule. +function Write-Report { + param( + [Parameter(Mandatory = $true)][AllowEmptyString()][string] $Message, + [ValidateSet('info', 'detail', 'heading', 'warn')][string] $Level = 'info' + ) + switch ($Level) { + 'detail' { Write-Host $Message -ForegroundColor DarkGray } + 'heading' { Write-Host $Message -ForegroundColor Cyan } + 'warn' { Write-Host $Message -ForegroundColor Yellow } + default { Write-Host $Message -ForegroundColor Gray } + } +} + +function Invoke-GitHubJson { + param([string[]] $Arguments) + $text = Invoke-Native { gh @Arguments } + if ($LASTEXITCODE -ne 0) { + throw "gh $($Arguments -join ' ') failed: $($text -join ' ')" + } + return ($text -join "`n") | ConvertFrom-Json +} + +# --- marking ----------------------------------------------------------------- + +if ($MarkProcessed) { + if (-not $Summary) { + throw 'Summary is required with -MarkProcessed: a marker with no account of what was done is a claim with no evidence.' + } + $lines = @($Summary, '') + foreach ($id in $MarkProcessed) { $lines += "" } + $file = Join-Path ([System.IO.Path]::GetTempPath()) ("mark-" + [guid]::NewGuid().ToString('N') + '.md') + [System.IO.File]::WriteAllText($file, ($lines -join "`n"), [System.Text.UTF8Encoding]::new($false)) + try { + $url = Invoke-Native { gh pr comment $Pr --repo "$script:Owner/$script:Name" --body-file $file } + if ($LASTEXITCODE -ne 0) { throw "posting the marker comment failed: $($url -join ' ')" } + Write-Report "marked processed: $($MarkProcessed -join ', ')" + Write-Report ($url -join ' ') -Level detail + } + finally { Remove-Item $file -ErrorAction SilentlyContinue } + exit 0 +} + +# --- scanning ---------------------------------------------------------------- + +# `??` is PowerShell 7 only, and these tools run on 5.1 too. +function Get-Text { + param($Value) + if ($null -eq $Value) { return '' } + return [string]$Value +} + +Write-Report "scanning pull request #$Pr" -Level heading + +$reviews = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/pulls/$Pr/reviews?per_page=100", '--paginate') +$issueComments = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/issues/$Pr/comments?per_page=100", '--paginate') + +# Reviews already recorded as processed, by marker. +$processed = @{} +foreach ($c in $issueComments) { + foreach ($m in [regex]::Matches((Get-Text $c.body), '')) { + $processed[[long]$m.Groups[1].Value] = $true + } +} + +# Unresolved threads, which is the authoritative state for inline findings. +$query = @' +query($owner:String!, $name:String!, $pr:Int!, $cursor:String) { + repository(owner:$owner, name:$name) { + pullRequest(number:$pr) { + reviewThreads(first:100, after:$cursor) { + pageInfo { hasNextPage endCursor } + nodes { + isResolved isOutdated path line + comments(first:1) { + nodes { body author { login } pullRequestReview { databaseId } } + } + } + } + } + } +} +'@ + +$threads = @() +$cursor = $null +do { + $arguments = @('api', 'graphql', '-f', "query=$query", + '-F', "owner=$script:Owner", '-F', "name=$script:Name", '-F', "pr=$Pr") + if ($cursor) { $arguments += @('-F', "cursor=$cursor") } + $page = (Invoke-GitHubJson $arguments).data.repository.pullRequest.reviewThreads + foreach ($t in $page.nodes) { + $c = $t.comments.nodes[0] + if ($c.author.login -notmatch '[Cc]opilot') { continue } + $threads += [pscustomobject]@{ + Review = [long]$c.pullRequestReview.databaseId + IsResolved = $t.isResolved + IsOutdated = $t.isOutdated + Path = $t.path + Line = $t.line + Body = ($c.body -replace '\s+', ' ') + } + } + $cursor = if ($page.pageInfo.hasNextPage) { $page.pageInfo.endCursor } else { $null } +} while ($cursor) + +$copilotReviews = @($reviews | Where-Object { $_.user.login -match '[Cc]opilot' }) + +# A review's suppressed count is only in its body, as rendered prose. +function Get-SuppressedCount { + param([string] $Body) + if ($Body -match 'Suppressed comments?\s*\((\d+)\)') { return [int]$Matches[1] } + return 0 +} + +$openThreads = @($threads | Where-Object { -not $_.IsResolved }) +$current = @($openThreads | Where-Object { -not $_.IsOutdated }) +$outdated = @($openThreads | Where-Object { $_.IsOutdated }) + +$suppressedOnly = @() +foreach ($r in $copilotReviews) { + $count = Get-SuppressedCount (Get-Text $r.body) + if ($count -eq 0) { continue } + if ($processed.ContainsKey([long]$r.id)) { continue } + # A review whose inline threads are all resolved may still carry suppressed + # findings nobody read, so this is judged on the marker alone. + $suppressedOnly += [pscustomobject]@{ + Id = [long]$r.id + When = ([datetime]$r.submitted_at).ToString('yyyy-MM-dd HH:mm') + Suppressed = $count + Inline = @($threads | Where-Object { $_.Review -eq [long]$r.id }).Count + } +} + +Write-Report '' +Write-Report "Copilot reviews: $($copilotReviews.Count)" +Write-Report "unresolved threads: $($openThreads.Count) ($($current.Count) current, $($outdated.Count) outdated)" +Write-Report "reviews with suppressed: $(@($copilotReviews | Where-Object { (Get-SuppressedCount (Get-Text $_.body)) -gt 0 }).Count)" +Write-Report " of those, unprocessed: $($suppressedOnly.Count)" +Write-Report '' + +Write-Report '=== unresolved threads on current lines ===' -Level heading +if ($current.Count -eq 0) { Write-Report ' none' -Level detail } +foreach ($t in ($current | Sort-Object Path, Line)) { + Write-Report (" review {0} {1}:{2}" -f $t.Review, $t.Path, $t.Line) + Write-Report (' ' + $t.Body.Substring(0, [Math]::Min(200, $t.Body.Length))) -Level detail +} + +if ($IncludeOutdated) { + Write-Report '' + Write-Report '=== unresolved threads whose anchor line has changed ===' -Level heading + Write-Report ' Usually fixed and never resolved; resolve them to clear this list.' -Level detail + foreach ($t in ($outdated | Sort-Object Path, Line)) { + Write-Report (" review {0} {1}:{2}" -f $t.Review, $t.Path, $t.Line) + } +} + +Write-Report '' +Write-Report '=== reviews with suppressed comments and no processed marker ===' -Level heading +Write-Report ' Suppressed findings create no thread, so nothing else records that they' -Level detail +Write-Report ' were read. Mark one with -MarkProcessed once it has been dealt with.' -Level detail +if ($suppressedOnly.Count -eq 0) { Write-Report ' none' -Level detail } +foreach ($r in ($suppressedOnly | Sort-Object When)) { + Write-Report (" {0} {1} suppressed={2} inline={3}" -f $r.Id, $r.When, $r.Suppressed, $r.Inline) +} + +Write-Report '' +if ($current.Count -gt 0 -or $suppressedOnly.Count -gt 0) { + Write-Report 'Outstanding items above.' -Level warn + exit 1 +} +Write-Report 'Nothing outstanding.' +exit 0 diff --git a/tools/soak-flush-barrier.ps1 b/tools/soak-flush-barrier.ps1 index 35ff2d8e..a67f073d 100644 --- a/tools/soak-flush-barrier.ps1 +++ b/tools/soak-flush-barrier.ps1 @@ -134,49 +134,22 @@ function Write-Report { } } -# Normalise one cargo invocation's merged output into plain strings. +# `Invoke-Native` and `ConvertTo-OutputLines`, which every capture site here goes +# through. Dot-sourced rather than imported: a module copy of that guard does not +# reach the scriptblock it is handed, and silently fails on 5.1 alone. The full +# argument, and the measurement behind it, is in that file. # -# With 2>&1, native stderr arrives as ErrorRecord objects. For the *empty* lines -# cargo emits between diagnostics the message is "" while ToString() falls back -# to the type name, so a bare "$_" renders those as -# "System.Management.Automation.RemoteException" scattered through the output -- -# which then reaches both the saved log and the Select-String that extracts the -# detail column. +# What it costs THIS script, recorded here because the shape is specific to the +# soak: cargo writes to stderr routinely -- "Compiling ...", and the "did not +# finalize incremental compilation session directory" notes this workspace emits +# constantly. Measured: under 5.1 the script died in the build step with +# NativeCommandError before running a single instrument, having written only the +# CSV header. Under 7 the same script completed. # -# Defined once and used by every capture site on purpose: this was originally -# fixed at the build step alone, leaving the per-instrument run with the same -# defect, which is how two copies of one rule drift apart. -function ConvertTo-OutputLines { - param([Parameter(ValueFromPipeline = $true)] $Record) - process { - if ($Record -is [System.Management.Automation.ErrorRecord]) { - $Record.Exception.Message - } else { - "$Record" - } - } -} - -# Run a native command, capturing merged stdout+stderr as plain strings. -# -# The ErrorActionPreference dance is what makes this work on Windows PowerShell -# 5.1. There, a native command writing to stderr under `Stop` raises a -# TERMINATING error, and cargo writes to stderr routinely -- "Compiling ...", -# and the "did not finalize incremental compilation session directory" notes -# this workspace emits constantly. Measured: under 5.1 the script died in the -# build step with NativeCommandError before running a single instrument, having -# written only the CSV header. Under 7 the same script completed. Restoring the -# preference afterwards keeps `Stop` for everything that is not a native call. -function Invoke-Native { - param([Parameter(Mandatory = $true)][scriptblock] $Command) - $previous = $ErrorActionPreference - $ErrorActionPreference = 'Continue' - try { - & $Command 2>&1 | ConvertTo-OutputLines - } finally { - $ErrorActionPreference = $previous - } -} +# Used by every capture site rather than one: this was originally fixed at the +# build step alone, leaving the per-instrument run with the same defect, which is +# how two copies of one rule drift apart. +. (Join-Path $PSScriptRoot 'common.ps1') # The rotation, in the order described above. $instruments = @( diff --git a/tools/test-common.ps1 b/tools/test-common.ps1 new file mode 100644 index 00000000..aa222212 --- /dev/null +++ b/tools/test-common.ps1 @@ -0,0 +1,179 @@ +# Copyright (c) Mike Grier. +<# +.SYNOPSIS + Tests for [common.ps1](common.ps1), run on BOTH PowerShell hosts. + +.DESCRIPTION + Runs its cases in the host that invoked it, then re-invokes itself in the + other host and requires that run to pass too. + + The cross-host run is the point of this file, not a nicety. The defect + `Invoke-Native` exists to prevent appears ONLY under Windows PowerShell 5.1: + PowerShell 7 captures a native command's stderr under `Stop` without + complaint. A suite that tested one host would report green while the guard + was broken on the only host that needs it -- which is how the original + defect reached `main` and survived review, since CI runs `shell: pwsh`. + + It refuses to pass vacuously: if the other host cannot be found, that is a + FAILURE rather than a skip, because "tested one host" is not the claim this + file is here to make. + +.PARAMETER SingleHost + Run the cases in this process only, without re-invoking the other host. + Used internally for the child run; also useful when debugging one host. +#> +[CmdletBinding()] +param( + [switch] $SingleHost +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +. (Join-Path $PSScriptRoot 'common.ps1') + +$script:Failures = 0 +$script:Host51 = 'C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe' + +# The single output sink, per the repository's one-sink rule. +function Write-Report { + param( + [Parameter(Mandatory = $true)][AllowEmptyString()][string] $Message, + [ValidateSet('info', 'good', 'bad', 'heading')][string] $Level = 'info' + ) + switch ($Level) { + 'good' { Write-Host $Message -ForegroundColor Green } + 'bad' { Write-Host $Message -ForegroundColor Red } + 'heading' { Write-Host $Message -ForegroundColor Cyan } + default { Write-Host $Message -ForegroundColor Gray } + } +} + +function Test-Case { + param([string] $Name, [scriptblock] $Body) + try { + & $Body + Write-Report " PASS $Name" -Level good + } + catch { + $script:Failures++ + Write-Report " FAIL $Name" -Level bad + Write-Report " $($_.Exception.Message)" + } +} + +function Assert-Equal { + param($Expected, $Actual, [string] $What) + if ("$Expected" -ne "$Actual") { + throw "$What -- expected '$Expected', got '$Actual'" + } +} + +Write-Report "common.ps1 tests on $($PSVersionTable.PSVersion)" -Level heading + +# The guard's whole purpose. A bare `& { ... } 2>&1` here would throw on 5.1. +Test-Case 'a native command writing to stderr is captured, not thrown' { + $out = Invoke-Native { cmd /c "echo to-stderr 1>&2" } + Assert-Equal 'to-stderr' ("$out".Trim()) 'captured text' +} + +Test-Case 'stdout and stderr are merged in one capture' { + $out = Invoke-Native { cmd /c "echo on-out & echo on-err 1>&2" } + $text = ($out -join ' ') + if ($text -notmatch 'on-out' -or $text -notmatch 'on-err') { + throw "both streams must appear, got '$text'" + } +} + +# The reason the script that motivated this cares: it tells a broken instrument +# from a finding by the exit code, so the guard must not swallow it. +Test-Case 'the exit code survives the capture' { + $null = Invoke-Native { cmd /c "echo boom 1>&2 & exit 3" } + Assert-Equal 3 $LASTEXITCODE 'LASTEXITCODE after a failing native command' +} + +Test-Case 'a failing command still yields its diagnostic text' { + $out = Invoke-Native { cmd /c "echo diagnostic-line 1>&2 & exit 1" } + if ("$out" -notmatch 'diagnostic-line') { + throw "the diagnostic must be captured, got '$out'" + } +} + +# Flattening is what keeps a transcript readable; without it a captured stderr +# line can stringify to `System.Management.Automation.RemoteException`. +Test-Case 'captured records are plain strings, not ErrorRecords' { + $out = @(Invoke-Native { cmd /c "echo to-stderr 1>&2" }) + foreach ($line in $out) { + if ($line -is [System.Management.Automation.ErrorRecord]) { + throw 'a raw ErrorRecord escaped ConvertTo-OutputLines' + } + } + if ("$out" -match 'RemoteException') { + throw "a record stringified to RemoteException: '$out'" + } +} + +Test-Case 'the caller''s ErrorActionPreference is restored afterwards' { + $before = $ErrorActionPreference + $null = Invoke-Native { cmd /c "echo to-stderr 1>&2" } + Assert-Equal $before $ErrorActionPreference 'ErrorActionPreference after the call' +} + +# The guard must not disarm `Stop` for anything that is not the native call. +Test-Case 'Stop still terminates a non-native error after the call' { + $null = Invoke-Native { cmd /c "echo to-stderr 1>&2" } + $threw = $false + try { Get-Item 'Q:\no\such\path\at\all.txt' | Out-Null } catch { $threw = $true } + if (-not $threw) { throw 'Stop was left disarmed for cmdlet errors' } +} + +# Restoration must survive the native call throwing for some other reason, or a +# later failure would run with the preference still flipped. +Test-Case 'ErrorActionPreference is restored even when the command throws' { + $before = $ErrorActionPreference + try { $null = Invoke-Native { throw 'deliberate' } } catch { } + Assert-Equal $before $ErrorActionPreference 'ErrorActionPreference after a throwing command' +} + +if (-not $SingleHost) { + # The other host, which is the claim this file exists to make. + $isSeven = $PSVersionTable.PSVersion.Major -ge 6 + $other = if ($isSeven) { $script:Host51 } else { 'pwsh' } + $otherName = if ($isSeven) { 'Windows PowerShell 5.1' } else { 'PowerShell 7' } + + $resolved = if ($isSeven) { + if (Test-Path $other) { $other } else { $null } + } + else { + $command = Get-Command $other -ErrorAction SilentlyContinue + if ($command) { $command.Source } else { $null } + } + + if (-not $resolved) { + $script:Failures++ + Write-Report '' + Write-Report "FAIL $otherName was not found, so the cross-host claim is untested." -Level bad + Write-Report ' This suite exists to prove the guard on BOTH hosts: the defect it' + Write-Report ' guards against appears only on 5.1, so a single-host pass is not' + Write-Report ' the result this file reports. Install the missing host or run it' + Write-Report ' there by hand rather than treating this as a skip.' + } + else { + Write-Report '' + Write-Report "=== re-running under $otherName ===" -Level heading + & $resolved -NoProfile -File $PSCommandPath -SingleHost + if ($LASTEXITCODE -ne 0) { + $script:Failures++ + Write-Report "FAIL the $otherName run reported failures." -Level bad + } + } +} + +Write-Report '' +if ($script:Failures -gt 0) { + Write-Report "$($script:Failures) failure(s)." -Level bad + exit 1 +} + +Write-Report 'All cases passed.' -Level good +exit 0 diff --git a/tools/test-run-sabotage.ps1 b/tools/test-run-sabotage.ps1 index 4e5a90c4..9b711a2d 100644 --- a/tools/test-run-sabotage.ps1 +++ b/tools/test-run-sabotage.ps1 @@ -74,6 +74,13 @@ param( Set-StrictMode -Version Latest $ErrorActionPreference = 'Stop' +# `Invoke-Native`, which every native call below goes through. Dot-sourced +# rather than imported: a module copy of that guard does not reach the +# scriptblock it is handed, and silently fails on 5.1 alone -- see +# [common.ps1](common.ps1), and [test-common.ps1](test-common.ps1) for the +# cross-host proof. +. (Join-Path $PSScriptRoot 'common.ps1') + $script:Harness = Join-Path $PSScriptRoot 'run-sabotage.ps1' $script:Passed = 0 $script:Failed = 0 @@ -153,12 +160,12 @@ function New-Fixture { [System.IO.File]::WriteAllText((Join-Path $root '.gitignore'), ".scratch/`n") [System.IO.File]::WriteAllText((Join-Path $root 'src\lib.rs'), $Source) - git -C $root init --quiet 2>&1 | Out-Null + Invoke-Native { git -C $root init --quiet } | Out-Null if ($null -ne $Manifest) { Set-Manifest -Root $root -Spec $Manifest } # Added to the index but not committed: `git ls-files` reads the index, # which is all the harness needs, and committing would demand identity # configuration this fixture has no reason to care about. - git -C $root add -A 2>&1 | Out-Null + Invoke-Native { git -C $root add -A } | Out-Null return $root } @@ -166,7 +173,7 @@ function Set-Manifest { param([string] $Root, $Spec) $json = $Spec | ConvertTo-Json -Depth 8 [System.IO.File]::WriteAllText((Join-Path $Root 'sabotage.json'), $json) - git -C $Root add -A 2>&1 | Out-Null + Invoke-Native { git -C $Root add -A } | Out-Null } # The default manifest: patches the fixture's marker line, expecting it caught. @@ -234,24 +241,24 @@ function Invoke-Harness { param([string] $Root, [string[]] $Arguments) Push-Location $Root - # $ErrorActionPreference is dropped to Continue for the call, and this is - # load bearing on Windows PowerShell 5.1. There, a native command's stderr - # redirected with 2>&1 arrives as an ErrorRecord, which under Stop is a - # TERMINATING error -- so every case testing a rejection path threw on the - # harness's own message instead of reading its exit code, and reported the - # harness's text as the failure. The harness writes to stderr deliberately - # (that is what Exit-WithMessage is for), so its output is data here, not a - # fault. PowerShell 7 does not do this, which is why the suite passed there - # and failed on 5.1 until it was run on both. - $previous = $ErrorActionPreference - $ErrorActionPreference = 'Continue' + # Through Invoke-Native, and that is load bearing on Windows PowerShell 5.1. + # The harness runs here as a CHILD PROCESS, so its `Exit-WithMessage` writes + # -- which go straight to the process stderr handle -- are native stderr to + # this script. Redirected with 2>&1 under Stop they arrive as ErrorRecords, + # which is a TERMINATING error: every case testing a rejection path threw on + # the harness's own message instead of reading its exit code, and reported + # the harness's text as the failure. The harness writes to stderr + # deliberately, so its output is data here, not a fault. PowerShell 7 does + # not do this, which is why the suite passed there and failed on 5.1 until + # it was run on both. try { $shell = if ($PSVersionTable.PSVersion.Major -ge 6) { 'pwsh' } else { 'powershell' } - $text = & $shell -NoProfile -File $script:Harness @Arguments 2>&1 | Out-String + $text = Invoke-Native { + & $shell -NoProfile -File $script:Harness @Arguments + } | Out-String return [pscustomobject]@{ ExitCode = $LASTEXITCODE; Output = $text } } finally { - $ErrorActionPreference = $previous Pop-Location } } @@ -800,7 +807,7 @@ Test-Case 'copies a file whose name git quotes, and drops it when the source doe try { $odd = Join-Path $root ('src\caf' + [char]0xE9 + '.rs') [System.IO.File]::WriteAllText($odd, "// unicode`n") - git -C $root add -A 2>&1 | Out-Null + Invoke-Native { git -C $root add -A } | Out-Null $stub = New-Stub -Behaviour 'fail' -Root $root Invoke-Harness -Root $root ` @@ -810,7 +817,7 @@ Test-Case 'copies a file whose name git quotes, and drops it when the source doe Assert-True (Test-Path -LiteralPath $copied) 'the copy must not silently omit it' Remove-Item -LiteralPath $odd -Force - git -C $root add -A 2>&1 | Out-Null + Invoke-Native { git -C $root add -A } | Out-Null Invoke-Harness -Root $root ` -Arguments @('-Manifest', 'sabotage.json', '-CargoCommand', $stub) | Out-Null From 4e0ee3f76fce2222be025fe16b85ba52fc886578 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Mon, 7 Sep 2026 22:18:56 -0400 Subject: [PATCH 02/11] fix(tools): remove a dead restoration in Invoke-Native, and make its tests able to fail Code review found that `Invoke-Native`'s `try/finally` restoration was dead code and that three cases in `test-common.ps1` claimed to cover it while being unable to fail. Both hold, and verifying it turned up a second defect the review did not reach. **The restoration was a no-op.** `$ErrorActionPreference = 'Continue'` assigns to a FUNCTION-LOCAL variable -- PowerShell assignment always writes to the current scope -- so the caller's value was never modified and the local one is discarded on return. `& $Command` still inherits `Continue` because the scriptblock runs in a child of that scope, which is the reach the guard needs. Measured on both hosts: a variant with the `try/finally` deleted outright leaves the caller reading `Stop` immediately after the call, identically to the version that had it. Removed, with the scoping rule stated where the assignment is. **The three cases were aimed at the wrong property.** They are not worthless, which is worth being precise about: a mutant that writes `$script:ErrorActionPreference` instead leaves the CALLER running under `Continue` for everything afterwards, and that is the mutation actually worth guarding. They are now described as testing that the flip does not ESCAPE, which is what they establish. **The second defect: they could be defeated by their own contamination.** Each captured `$before = $ErrorActionPreference` rather than establishing a known value. Under a `$script:`-scoped mutant the leak happens on the FIRST call, so a later case captured `Continue`, compared it against `Continue` afterwards, and passed -- the contamination hiding itself. Measured before the fix: that mutant was caught by only ONE of the three, the behavioural case that observes `Stop` through a failing cmdlet rather than through the variable. Each case now sets `$script:ErrorActionPreference = 'Stop'` first, and all three catch it on both hosts. **And a real gap the review's finding exposed: PowerShell 7 had no coverage at all that the guard did anything.** Removing the `Continue` flip fails loudly on 5.1, but 7 captures stderr either way, so every case passed there against a guard that did nothing. Added `the flip reaches the scriptblock it is handed`, which observes the preference from inside the passed scriptblock and is therefore host-independent. It is the only case that fails on 7 against a removed flip. Sabotage-verified, both mutants on both hosts: escape to $script: 7 : 3 cases fail (was 1 before the isolation fix) escape to $script: 5.1 : 3 cases fail flip removed 7 : 1 case fails (was 0 before the new case) flip removed 5.1 : 8 cases fail Nine cases now, passing on both hosts. The sabotage suite passes on both, the NUMA spike runner completes, and the encoding check is clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/common.ps1 | 30 ++++++++++++++++++++------ tools/test-common.ps1 | 50 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/tools/common.ps1 b/tools/common.ps1 index 5b9ccda5..144c8312 100644 --- a/tools/common.ps1 +++ b/tools/common.ps1 @@ -50,6 +50,26 @@ function ConvertTo-OutputLines { # `Continue` around the call is what makes the capture work on both, and # restoring it afterwards keeps `Stop` for everything that is not a native call. # +# **No restoration is needed, and none is attempted.** `$ErrorActionPreference = +# 'Continue'` here creates a FUNCTION-LOCAL variable: PowerShell assignment +# always writes to the current scope, so the caller's own value is untouched and +# the local one is discarded when this returns. `& $Command` runs the scriptblock +# in a child of this scope, so it inherits `Continue` -- which is exactly the +# reach the guard needs -- while nothing outside sees it. +# +# An earlier version wrapped the call in `try { } finally { $ErrorActionPreference +# = $previous }`. That restored the local copy nobody could observe, so it was +# dead code, and worse: three cases in `test-common.ps1` claimed to cover it and +# could not fail. Measured on both hosts -- with the `try/finally` deleted +# outright, the caller still reads `Stop` immediately after the call, identically +# to the version that had it. +# +# The property that DOES need a test is the other direction: that the flip never +# escapes into the caller. Writing `$script:` or `$global:` here would leave the +# caller running under `Continue` for everything afterwards, and `test-common.ps1` +# covers that (a `$script:`-scoped mutant leaves the caller at `Continue` and +# fails those cases). +# # `$LASTEXITCODE` is global, so a caller still reads the command's exit code # after this returns. That matters here: these scripts distinguish a broken # instrument from a finding by exactly that code. @@ -81,12 +101,8 @@ function ConvertTo-OutputLines { # [test-common.ps1](test-common.ps1) asserts this on both hosts. function Invoke-Native { param([Parameter(Mandatory = $true)][scriptblock] $Command) - $previous = $ErrorActionPreference + # Function-local by construction -- see the note above on why there is no + # restoration to do. Never `$script:` or `$global:` here. $ErrorActionPreference = 'Continue' - try { - & $Command 2>&1 | ConvertTo-OutputLines - } - finally { - $ErrorActionPreference = $previous - } + & $Command 2>&1 | ConvertTo-OutputLines } diff --git a/tools/test-common.ps1 b/tools/test-common.ps1 index aa222212..8fabb87f 100644 --- a/tools/test-common.ps1 +++ b/tools/test-common.ps1 @@ -113,26 +113,58 @@ Test-Case 'captured records are plain strings, not ErrorRecords' { } } -Test-Case 'the caller''s ErrorActionPreference is restored afterwards' { - $before = $ErrorActionPreference +# The flip must REACH the scriptblock, and this is the only case that shows it +# directly. The stderr case above shows it too, but only on 5.1 -- PowerShell 7 +# captures either way, so on 7 nothing else here distinguishes a guard that works +# from one that does nothing. Observing the preference from inside the passed +# scriptblock is host-independent. +Test-Case 'the flip reaches the scriptblock it is handed' { + $seen = Invoke-Native { $ErrorActionPreference } + Assert-Equal 'Continue' ("$seen".Trim()) 'ErrorActionPreference as seen inside the command' +} + +# The three cases below are the OTHER direction: the flip must not escape. +# +# They are deliberately not described as testing a "restoration". `Invoke-Native` +# assigns to a function-local `$ErrorActionPreference`, so the caller's value is +# never modified and there is nothing to restore -- an earlier version wrapped +# the call in a `try/finally` that restored a copy nobody could observe, and +# these three cases could not fail against deleting it. Measured on both hosts. +# +# What they do catch is real and is the mutation worth guarding: writing +# `$script:ErrorActionPreference` or `$global:` in `Invoke-Native` would leave +# the CALLER running under `Continue` for everything afterwards, silently +# disarming `Stop` for the rest of the script. A `$script:`-scoped mutant leaves +# the caller at `Continue` and fails all three. +# Each of these three sets `$script:ErrorActionPreference` to a known value +# first, rather than capturing whatever it happens to be. That is not ceremony: +# a mutant that escapes to script scope leaks `Continue` on its FIRST call, so a +# later case reading "before" would capture `Continue`, compare it against +# `Continue` afterwards, and pass -- the contamination hiding itself. Measured: +# without this reset, a `$script:`-scoped mutant was caught only by the +# behavioural case below, and the two variable-observing cases passed. +Test-Case 'the flip does not escape into the caller' { + $script:ErrorActionPreference = 'Stop' $null = Invoke-Native { cmd /c "echo to-stderr 1>&2" } - Assert-Equal $before $ErrorActionPreference 'ErrorActionPreference after the call' + Assert-Equal 'Stop' $ErrorActionPreference 'the caller''s ErrorActionPreference after the call' } -# The guard must not disarm `Stop` for anything that is not the native call. +# The same property observed through behaviour rather than through the variable: +# `Stop` must still terminate on something that is not the native call. Test-Case 'Stop still terminates a non-native error after the call' { + $script:ErrorActionPreference = 'Stop' $null = Invoke-Native { cmd /c "echo to-stderr 1>&2" } $threw = $false try { Get-Item 'Q:\no\such\path\at\all.txt' | Out-Null } catch { $threw = $true } if (-not $threw) { throw 'Stop was left disarmed for cmdlet errors' } } -# Restoration must survive the native call throwing for some other reason, or a -# later failure would run with the preference still flipped. -Test-Case 'ErrorActionPreference is restored even when the command throws' { - $before = $ErrorActionPreference +# And on the path where the command throws, which is where a scope-escaping +# assignment would be least likely to be noticed by hand. +Test-Case 'the flip does not escape when the command throws' { + $script:ErrorActionPreference = 'Stop' try { $null = Invoke-Native { throw 'deliberate' } } catch { } - Assert-Equal $before $ErrorActionPreference 'ErrorActionPreference after a throwing command' + Assert-Equal 'Stop' $ErrorActionPreference 'the caller''s ErrorActionPreference after a throw' } if (-not $SingleHost) { From e5ad08f96287fdbc1c5ed6da2129e71f27bd8602 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Mon, 7 Sep 2026 22:46:56 -0400 Subject: [PATCH 03/11] fix(tools): separate a broken instrument from a finding, and only trust markers from write access Three defects from a review round on this branch. Two were the reviewer's; the third was found by running the tool this branch adds against its own pull request, which reported it. **scan-pr-reviews.ps1 exited 1 for both "there are findings" and "the tool broke".** Every error path was an unhandled `throw`, and an unhandled throw from a script also exits 1, so a caller gating on the code could not tell "2 unresolved threads" from "gh is not authenticated" or "no such pull request". That is the instrument-versus-finding distinction `run-numa-spikes` and `run-sabotage` already make by exit code. Error paths now leave through `Exit-Broken` with code 2; 1 stays the finding. A `ConvertFrom-Json` failure is covered too, which is the realistic shape of a proxy returning an HTML error page to a `gh` that still exited 0. Verified on both hosts: nonexistent PR -> 2, missing -Summary -> 2, PR with open threads -> 1, clean PR -> 0. **Processed markers were honoured from any comment author.** This repository is public, so anyone able to comment could post `` and permanently retire a review from every future scan. Because a suppressed-only review has no state anywhere else -- the whole reason the tool exists -- that would silently delete the only record that a finding was never read, and the summary would just report a smaller number. Only OWNER, MEMBER and COLLABORATOR are now honoured; CONTRIBUTOR means merely "has had a pull request merged" and is not enough. `author_association` arrives on every comment in the same request, so this costs no extra call. Markers from other authors are counted and reported rather than dropped in silence, because such a marker is either an honest mistake or an attempt to retire a finding and both are worth seeing. Verified on both hosts across all six association values, including a missing field, which fails closed; the real OWNER marker on PR #80 is still honoured, so the legitimate path is unaffected. **Restatement drift: two places still said the guard "restores" the preference.** The previous commit removed that restoration as dead code and corrected the implementation comment, but left the claim standing in `common.ps1`'s own summary paragraph and in DESIGN-NOTES. Both now state what the code does -- the flip is function-local rather than restored -- and why the property worth testing is that it does not escape. Swept `restor` across `tools/` and DESIGN-NOTES: 49 matches, 2 stale and corrected, the rest about file restoration in the sabotage harness and unrelated crate behaviour. Found by running `scan-pr-reviews.ps1 -Pr 81`, which is the tool this branch adds; it reported both stale sites as unresolved Copilot threads. The instrument catching a defect in its own change set is the outcome it was written for. Verified: `test-common.ps1` passes on both hosts, the sabotage suite passes on both, encoding 615 files clean, workflow references resolve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- DESIGN-NOTES.md | 11 +++++- tools/common.ps1 | 5 ++- tools/scan-pr-reviews.ps1 | 79 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 85 insertions(+), 10 deletions(-) diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index c16c9bda..05951f2d 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1657,8 +1657,15 @@ preference. The single thing it carries today is `Invoke-Native`, the guard against 5.1's treatment of native stderr: there, 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. The guard flips the preference to `Continue` around the call and -restores it afterwards. +PowerShell 7 does not. The guard flips the preference to `Continue` for the duration of the +call, and **that flip is function-local rather than restored afterwards**: PowerShell +assignment always writes to the current scope, so the caller's value is never modified and +the local one is discarded on return, while `& $Command` still inherits it through the child +scope. An earlier version wrapped the call in a `try`/`finally` that restored a copy nobody +could observe; it was dead code, and three cases in +[tools/test-common.ps1](tools/test-common.ps1) claimed to cover it while being unable to +fail. The property that does need testing is the opposite one -- that the flip never +*escapes* into the caller, which is what a `$script:`-scoped mistake would do. **Why a module fails.** A scriptblock carries the session state it was created in. `Invoke-Native { cargo build }` builds that scriptblock in the *caller's* script scope, so diff --git a/tools/common.ps1 b/tools/common.ps1 index 144c8312..0b4ce65c 100644 --- a/tools/common.ps1 +++ b/tools/common.ps1 @@ -47,8 +47,9 @@ function ConvertTo-OutputLines { # 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. Flipping the preference to -# `Continue` around the call is what makes the capture work on both, and -# restoring it afterwards keeps `Stop` for everything that is not a native call. +# `Continue` for the duration of the call is what makes the capture work on both, +# and keeping that flip function-local is what leaves `Stop` in force for +# everything that is not a native call. # # **No restoration is needed, and none is attempted.** `$ErrorActionPreference = # 'Continue'` here creates a FUNCTION-LOCAL variable: PowerShell assignment diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 index 20857062..f69b2b77 100644 --- a/tools/scan-pr-reviews.ps1 +++ b/tools/scan-pr-reviews.ps1 @@ -36,6 +36,17 @@ survive a new machine, a new contributor, and a new agent session. A later run of this script reads those markers back and stops reporting the review. + Only markers written by someone with write access (author_association OWNER, + MEMBER or COLLABORATOR) are honoured. This repository is public, so anyone who + can comment could otherwise retire a finding that has no other state anywhere. + Markers from other authors are counted and reported, not silently dropped. + +.OUTPUTS + Exit code 0 when nothing is outstanding, 1 when there are findings, and 2 when + the tool could not run (gh unauthenticated, no such pull request, the marker + could not be posted). 1 and 2 are kept distinct so a caller can tell a finding + from a broken instrument. + .PARAMETER Pr The pull request number. @@ -70,6 +81,24 @@ $ErrorActionPreference = 'Stop' . (Join-Path $PSScriptRoot 'common.ps1') +# Exit codes, kept distinct on purpose. `1` is a FINDING -- the scan ran and +# there is outstanding work -- while `2` is a BROKEN INSTRUMENT: `gh` is not +# authenticated, the network is down, the pull request does not exist, the +# marker could not be posted. A caller gating on the exit code has to be able to +# tell those apart, or "no findings" and "the tool never ran" look identical, +# which is the same instrument-versus-finding separation `run-numa-spikes.ps1` +# and `run-sabotage.ps1` already make. Every error path below leaves through +# `Exit-Broken` rather than through a bare `throw`, because an unhandled throw +# from a script also exits 1 and would collide with the finding code. +$script:ExitFindings = 1 +$script:ExitBroken = 2 + +function Exit-Broken { + param([Parameter(Mandatory = $true)][string] $Message) + [Console]::Error.WriteLine($Message) + exit $script:ExitBroken +} + $script:Owner = 'MikeGrier' $script:Name = 'windows-threadpool-sys' @@ -91,16 +120,24 @@ function Invoke-GitHubJson { param([string[]] $Arguments) $text = Invoke-Native { gh @Arguments } if ($LASTEXITCODE -ne 0) { - throw "gh $($Arguments -join ' ') failed: $($text -join ' ')" + Exit-Broken "gh $($Arguments -join ' ') failed: $($text -join ' ')" + } + try { + return ($text -join "`n") | ConvertFrom-Json + } + catch { + # Reached when gh succeeds but returns something that is not JSON -- a + # proxy's HTML error page is the realistic case. Still the instrument, + # not a finding. + Exit-Broken "gh $($Arguments -join ' ') returned unparsable output: $($_.Exception.Message)" } - return ($text -join "`n") | ConvertFrom-Json } # --- marking ----------------------------------------------------------------- if ($MarkProcessed) { if (-not $Summary) { - throw 'Summary is required with -MarkProcessed: a marker with no account of what was done is a claim with no evidence.' + Exit-Broken 'Summary is required with -MarkProcessed: a marker with no account of what was done is a claim with no evidence.' } $lines = @($Summary, '') foreach ($id in $MarkProcessed) { $lines += "" } @@ -108,7 +145,9 @@ if ($MarkProcessed) { [System.IO.File]::WriteAllText($file, ($lines -join "`n"), [System.Text.UTF8Encoding]::new($false)) try { $url = Invoke-Native { gh pr comment $Pr --repo "$script:Owner/$script:Name" --body-file $file } - if ($LASTEXITCODE -ne 0) { throw "posting the marker comment failed: $($url -join ' ')" } + if ($LASTEXITCODE -ne 0) { + Exit-Broken "posting the marker comment failed: $($url -join ' ')" + } Write-Report "marked processed: $($MarkProcessed -join ', ')" Write-Report ($url -join ' ') -Level detail } @@ -131,9 +170,34 @@ $reviews = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/pulls/$P $issueComments = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/issues/$Pr/comments?per_page=100", '--paginate') # Reviews already recorded as processed, by marker. +# +# **Only markers from someone with write access are honoured.** This repository +# is public, so anyone able to comment on a pull request can post a marker; and +# because a suppressed-only review has no state anywhere else -- which is the +# whole reason this tool exists -- an unauthenticated marker would permanently +# and silently delete the only record that a finding was never read. The counter +# below would simply report a smaller number, with nothing to indicate why. +# +# `author_association` comes back on every comment from the same request, so +# this costs no extra call. GitHub sets it per comment from the author's +# relationship to the repository at the time of writing: OWNER, MEMBER and +# COLLABORATOR are the ones that imply write access. CONTRIBUTOR means only +# "has had a pull request merged", and NONE is any passer-by; neither is +# sufficient to retire a finding. +$trustedAssociations = @('OWNER', 'MEMBER', 'COLLABORATOR') $processed = @{} +$ignoredMarkers = 0 foreach ($c in $issueComments) { - foreach ($m in [regex]::Matches((Get-Text $c.body), '')) { + $markers = [regex]::Matches((Get-Text $c.body), '') + if ($markers.Count -eq 0) { continue } + if ($trustedAssociations -notcontains (Get-Text $c.author_association)) { + # Counted and reported rather than dropped in silence: a marker from an + # untrusted author is either an honest mistake or an attempt to retire a + # finding, and both are worth seeing. + $ignoredMarkers += $markers.Count + continue + } + foreach ($m in $markers) { $processed[[long]$m.Groups[1].Value] = $true } } @@ -212,6 +276,9 @@ Write-Report "Copilot reviews: $($copilotReviews.Count)" Write-Report "unresolved threads: $($openThreads.Count) ($($current.Count) current, $($outdated.Count) outdated)" Write-Report "reviews with suppressed: $(@($copilotReviews | Where-Object { (Get-SuppressedCount (Get-Text $_.body)) -gt 0 }).Count)" Write-Report " of those, unprocessed: $($suppressedOnly.Count)" +if ($ignoredMarkers -gt 0) { + Write-Report "ignored markers: $ignoredMarkers (author lacks write access)" -Level warn +} Write-Report '' Write-Report '=== unresolved threads on current lines ===' -Level heading @@ -242,7 +309,7 @@ foreach ($r in ($suppressedOnly | Sort-Object When)) { Write-Report '' if ($current.Count -gt 0 -or $suppressedOnly.Count -gt 0) { Write-Report 'Outstanding items above.' -Level warn - exit 1 + exit $script:ExitFindings } Write-Report 'Nothing outstanding.' exit 0 From 5a9b35e01d8b53c6a2ec4d4dcbf496b4af423e48 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Mon, 7 Sep 2026 22:55:44 -0400 Subject: [PATCH 04/11] fix(tools): fail fast when gh is missing, instead of reporting a false all-clear Copilot raised that `scan-pr-reviews.ps1` should preflight `gh`. Measuring it found the consequence is worse than either the report or my own expectation. The report's stated mechanism was a stale `$LASTEXITCODE` surfacing as a misleading JSON-parse failure. That is not what happens. `Invoke-Native { gh ... }` with `gh` absent throws CommandNotFoundException on both hosts, and the script then died on something else entirely: `$LASTEXITCODE` is UNSET until a native command has run in the session, and under `Set-StrictMode -Version Latest` reading an unset variable is itself a terminating error. So the failure was a StrictMode violation about `$LASTEXITCODE`, describing the wreckage rather than the missing tool. And the exit code was the real damage. Measured before and after, both hosts, with `gh` stripped from PATH: pre-fix exit 0 <- "Nothing outstanding." post-fix exit 2 <- broken instrument Exit 0 is this tool's "nothing outstanding" signal, so a machine without the GitHub CLI got a silent false all-clear from the one tool whose entire purpose is to stop findings being lost. That is strictly worse than the exit-1 collision the previous commit fixed for the other error paths, and it is the same class: a broken instrument reporting as a result. Two changes, because the preflight alone would leave the landmine for any other path that reads the variable first: - `gh` is checked once, up front, and its absence exits through `Exit-Broken` with a message naming the cause and the fix. - `$LASTEXITCODE` is read through `Get-LastExitCode`, which uses `Get-Variable -ErrorAction SilentlyContinue` and treats unset as failure rather than throwing. Both call sites now distinguish "unset" from "non-zero" and report the code they saw. Verified on both hosts: without `gh`, exit 2 with the intended message; with `gh`, the contract still holds -- findings 1, clean 0, nonexistent PR 2. `test-common.ps1` and the sabotage suite pass on both hosts; encoding clean. A note on measurement, since it bit twice while checking this: `pwsh -Command "& script.ps1"` does NOT propagate the script's exit code, and `Select-Object -First N` stops the pipeline in a way that makes `$LASTEXITCODE` unreliable. Both produced wrong readings that looked plausible. The numbers above come from `-Command "...; exit $LASTEXITCODE"` with no truncation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/scan-pr-reviews.ps1 | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 index f69b2b77..eba852fe 100644 --- a/tools/scan-pr-reviews.ps1 +++ b/tools/scan-pr-reviews.ps1 @@ -99,6 +99,31 @@ function Exit-Broken { exit $script:ExitBroken } +# `$LASTEXITCODE` is UNSET until a native command has run in the session, and +# under `Set-StrictMode -Version Latest` reading an unset variable throws. That +# is not a hypothetical: with `gh` absent from PATH, the call below fails with +# CommandNotFoundException before ever setting it, and the script then died on +# the StrictMode violation rather than on the missing tool -- exiting 1, the +# code that means "there are findings". Reading it through `Get-Variable` +# removes the landmine wherever the code path reaches it. +function Get-LastExitCode { + $variable = Get-Variable -Name LASTEXITCODE -Scope Global -ErrorAction SilentlyContinue + if ($null -eq $variable -or $null -eq $variable.Value) { return $null } + return [int]$variable.Value +} + +# `gh` is the whole instrument here, so its absence is checked once, up front, +# rather than being discovered as a confusing symptom further in. Without this, +# a machine without the CLI reported either a StrictMode error about +# `$LASTEXITCODE` or a JSON parse failure -- both describing the wreckage rather +# than the cause, and both exiting 1 as though the scan had found something. +if (-not (Get-Command gh -ErrorAction SilentlyContinue)) { + Exit-Broken @' +gh was not found on PATH, so this tool cannot read the pull request at all. +Install the GitHub CLI (https://cli.github.com) and run `gh auth login`. +'@ +} + $script:Owner = 'MikeGrier' $script:Name = 'windows-threadpool-sys' @@ -119,8 +144,9 @@ function Write-Report { function Invoke-GitHubJson { param([string[]] $Arguments) $text = Invoke-Native { gh @Arguments } - if ($LASTEXITCODE -ne 0) { - Exit-Broken "gh $($Arguments -join ' ') failed: $($text -join ' ')" + $code = Get-LastExitCode + if ($null -eq $code -or $code -ne 0) { + Exit-Broken "gh $($Arguments -join ' ') failed (exit $code): $($text -join ' ')" } try { return ($text -join "`n") | ConvertFrom-Json @@ -145,8 +171,9 @@ if ($MarkProcessed) { [System.IO.File]::WriteAllText($file, ($lines -join "`n"), [System.Text.UTF8Encoding]::new($false)) try { $url = Invoke-Native { gh pr comment $Pr --repo "$script:Owner/$script:Name" --body-file $file } - if ($LASTEXITCODE -ne 0) { - Exit-Broken "posting the marker comment failed: $($url -join ' ')" + $code = Get-LastExitCode + if ($null -eq $code -or $code -ne 0) { + Exit-Broken "posting the marker comment failed (exit $code): $($url -join ' ')" } Write-Report "marked processed: $($MarkProcessed -join ', ')" Write-Report ($url -join ' ') -Level detail From fb91b678f87ab1b7dc419aa66a9ba9240cff1566 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Mon, 7 Sep 2026 23:28:43 -0400 Subject: [PATCH 05/11] fix(tools): enforce the marker-trust claim, and stop nullable API fields exiting as findings Two findings from a review round, both confirmed by measurement on both hosts. **Nullable API fields would have exited 1 -- the "findings" code.** Several property chains over GraphQL and REST data are nullable BY SCHEMA rather than by accident: `author` and `pullRequestReview` are null for a deleted account, REST `user` likewise, `submitted_at` is null on a PENDING review, and a thread's `comments.nodes` can be empty. Under `Set-StrictMode -Version Latest` every one of those is a TERMINATING error, and an unhandled throw from a script exits 1 -- which the exit contract added two commits ago defines as "there are outstanding findings". A contributor deleting their GitHub account would have silently converted this tool into a false positive. Measured on both hosts: `$null.login`, `@()[0]` and `[datetime]$null` all throw under StrictMode, and a JSON-sourced null behaves identically. Added `Get-Path`, which walks a property chain yielding `$null` instead of throwing, and routed every such access through it; a thread that cannot be attributed to a review is skipped rather than attributed to review 0, and a PENDING review renders as `pending` rather than being cast. **The marker-trust check was wider than it claimed.** It trusted `author_association` in OWNER/MEMBER/COLLABORATOR and described those as "the ones that imply write access". They are not: GitHub reports `COLLABORATOR` for anyone invited to collaborate, with no permission qualifier, so a collaborator with `read` or `triage` could post a marker and permanently retire a suppressed-only finding that has no other state anywhere -- without even being counted as ignored. Replaced with an actual permission lookup against the collaborators endpoint, accepting only `admin` or `write`. One call per DISTINCT marker author, cached, and markers are rare, so this is a call or two per scan. It **fails closed**: a 404 for a non-collaborator, a 403 because the account running the scan cannot query permissions, or any network failure leaves the marker unhonoured. That direction is deliberate -- over-reporting a handled finding is visible and recoverable, while wrongly honouring a marker silently deletes the only record that a finding was never read. The lookup deliberately does not go through `Invoke-GitHubJson`, because a non-zero exit there is the ordinary answer for a non-collaborator rather than a broken instrument. Verified against real accounts on both hosts: the owner may retire, `octocat` and `torvalds` may not, an empty login may not; and the existing OWNER marker on PR #80 is still honoured, so the legitimate path is unchanged. The claim is corrected in both places that stated it -- the script header and DESIGN-NOTES -- since the old wording described an enforcement the code did not perform. Swept for the stale phrasing; no occurrences remain. Verified: `test-common.ps1` passes on both hosts, the sabotage suite passes on both, exit contract intact (findings 1, clean 0, nonexistent PR 2), encoding 615 files clean, workflow references resolve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- DESIGN-NOTES.md | 9 +++ tools/scan-pr-reviews.ps1 | 138 ++++++++++++++++++++++++++++++-------- 2 files changed, 119 insertions(+), 28 deletions(-) diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index 05951f2d..e7c6c5df 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1735,6 +1735,15 @@ means the anchored line has since changed, which usually means the finding was f thread simply never resolved -- so those are the cheap ones to clear, and they are listed only under `-IncludeOutdated` to keep the default output about work that is actually open. +**A marker is only honoured from an account with `admin` or `write` permission**, checked +against the collaborators permission endpoint rather than inferred from the comment's +`author_association`. That field reports `COLLABORATOR` for a read-only collaborator as well +as a writer, so trusting it would enforce something weaker than the control claims. The check +fails closed -- a 404, a 403 because the account running the scan cannot query permissions, or +any network failure leaves the marker unhonoured -- because over-reporting a finding that was +in fact handled is visible and recoverable, while wrongly honouring a marker silently deletes +the only record that a finding was never read. Unhonoured markers are counted and reported. + **A marker asserts the review was read, so do not back-fill in bulk.** PR #56 carries 131 reviews with suppressed comments and no marker. Almost all were addressed during the rounds that followed them, but "almost all" is not evidence, and marking them wholesale would convert diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 index eba852fe..18cd5684 100644 --- a/tools/scan-pr-reviews.ps1 +++ b/tools/scan-pr-reviews.ps1 @@ -36,10 +36,14 @@ survive a new machine, a new contributor, and a new agent session. A later run of this script reads those markers back and stops reporting the review. - Only markers written by someone with write access (author_association OWNER, - MEMBER or COLLABORATOR) are honoured. This repository is public, so anyone who - can comment could otherwise retire a finding that has no other state anywhere. - Markers from other authors are counted and reported, not silently dropped. + Only markers written by someone whose repository permission is `admin` or + `write` are honoured, checked against the collaborators permission endpoint + rather than inferred from `author_association` -- GitHub reports + COLLABORATOR for a read-only collaborator too, so that field is weaker than + this control claims to be. This repository is public, so anyone who can + comment could otherwise retire a finding that has no other state anywhere. + The check fails closed, and markers that were not honoured are counted and + reported rather than silently dropped. .OUTPUTS Exit code 0 when nothing is outstanding, 1 when there are findings, and 2 when @@ -184,6 +188,32 @@ if ($MarkProcessed) { # --- scanning ---------------------------------------------------------------- +# Walk a property chain over API-supplied data, yielding $null rather than +# throwing when any link is absent or null. +# +# Needed because `Set-StrictMode -Version Latest` turns `$x.a.b` into a +# TERMINATING error the moment `a` is null, and several fields these queries +# return are nullable BY SCHEMA rather than by accident: GraphQL `author` and +# `pullRequestReview` are null for a deleted account, REST `user` likewise, and +# `submitted_at` is null on a PENDING review. Any one of those would leave this +# script through an unhandled throw -- which exits 1, the code that means +# "there are findings". A contributor deleting their GitHub account would +# silently turn this tool into a false positive. +# +# Verified on both hosts that all of these throw under StrictMode without it: +# `$null.login`, `@()[0]`, and `[datetime]$null`. +function Get-Path { + param($Object, [string[]] $Names) + $current = $Object + foreach ($name in $Names) { + if ($null -eq $current) { return $null } + $property = $current.PSObject.Properties[$name] + if ($null -eq $property) { return $null } + $current = $property.Value + } + return $current +} + # `??` is PowerShell 7 only, and these tools run on 5.1 too. function Get-Text { param($Value) @@ -196,31 +226,63 @@ Write-Report "scanning pull request #$Pr" -Level heading $reviews = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/pulls/$Pr/reviews?per_page=100", '--paginate') $issueComments = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/issues/$Pr/comments?per_page=100", '--paginate') -# Reviews already recorded as processed, by marker. +# Whether this author may retire a finding, by ACTUAL repository permission. +# +# `author_association` is the cheap answer and it is the wrong one. GitHub sets +# `COLLABORATOR` for anyone *invited to collaborate*, with no permission +# qualifier -- a collaborator with `read` or `triage` gets `COLLABORATOR` too -- +# so trusting that value would enforce something weaker than the control claims. +# The permission endpoint answers the question actually being asked. # -# **Only markers from someone with write access are honoured.** This repository -# is public, so anyone able to comment on a pull request can post a marker; and -# because a suppressed-only review has no state anywhere else -- which is the -# whole reason this tool exists -- an unauthenticated marker would permanently -# and silently delete the only record that a finding was never read. The counter -# below would simply report a smaller number, with nothing to indicate why. +# One call per DISTINCT author who posted a marker, cached, and markers are +# rare, so this is a call or two per scan rather than one per comment. # -# `author_association` comes back on every comment from the same request, so -# this costs no extra call. GitHub sets it per comment from the author's -# relationship to the repository at the time of writing: OWNER, MEMBER and -# COLLABORATOR are the ones that imply write access. CONTRIBUTOR means only -# "has had a pull request merged", and NONE is any passer-by; neither is -# sufficient to retire a finding. -$trustedAssociations = @('OWNER', 'MEMBER', 'COLLABORATOR') +# **Fails closed.** Anything other than a confirmed `admin` or `write` -- a 404 +# because the author is not a collaborator, a 403 because the caller running +# this scan lacks push access and may not query permissions, a network failure +# -- leaves the marker unhonoured. That direction is deliberate: an unhonoured +# marker over-reports a finding that was in fact handled, which is visible and +# recoverable, while wrongly honouring one silently deletes the only record that +# a finding was never read. +$script:PermissionCache = @{} +function Test-CanRetireFinding { + param([string] $Login) + if (-not $Login) { return $false } + if ($script:PermissionCache.ContainsKey($Login)) { return $script:PermissionCache[$Login] } + + # Deliberately NOT through Invoke-GitHubJson: a non-zero exit here is the + # ordinary answer for a non-collaborator, not a broken instrument, so it must + # not exit 2. + $text = Invoke-Native { + gh api "repos/$script:Owner/$script:Name/collaborators/$Login/permission" --jq '.permission' + } + $code = Get-LastExitCode + $permission = if ($null -eq $code -or $code -ne 0) { '' } else { (Get-Text ($text -join '')).Trim() } + + $allowed = @('admin', 'write') -contains $permission + $script:PermissionCache[$Login] = $allowed + return $allowed +} + +# Reviews already recorded as processed, by marker. +# +# This repository is public, so anyone able to comment on a pull request can post +# a marker; and because a suppressed-only review has no state anywhere else -- +# which is the whole reason this tool exists -- an unauthorised marker would +# permanently and silently delete the only record that a finding was never read. +# The counter would simply report a smaller number, with nothing to indicate why. $processed = @{} $ignoredMarkers = 0 foreach ($c in $issueComments) { $markers = [regex]::Matches((Get-Text $c.body), '') if ($markers.Count -eq 0) { continue } - if ($trustedAssociations -notcontains (Get-Text $c.author_association)) { - # Counted and reported rather than dropped in silence: a marker from an - # untrusted author is either an honest mistake or an attempt to retire a - # finding, and both are worth seeing. + + $login = Get-Text (Get-Path $c @('user', 'login')) + if (-not (Test-CanRetireFinding $login)) { + # Counted and reported rather than dropped in silence: a marker that was + # not honoured is either an honest mistake, a permissions problem with + # the account running the scan, or an attempt to retire a finding, and + # all three are worth seeing. $ignoredMarkers += $markers.Count continue } @@ -256,21 +318,36 @@ do { if ($cursor) { $arguments += @('-F', "cursor=$cursor") } $page = (Invoke-GitHubJson $arguments).data.repository.pullRequest.reviewThreads foreach ($t in $page.nodes) { - $c = $t.comments.nodes[0] - if ($c.author.login -notmatch '[Cc]opilot') { continue } + # A thread with no comments is not a shape this query should produce, but + # indexing an empty array is an error rather than $null under StrictMode, + # so it is checked rather than assumed. + $comments = Get-Path $t @('comments', 'nodes') + if ($null -eq $comments -or @($comments).Count -eq 0) { continue } + $c = @($comments)[0] + + $login = Get-Path $c @('author', 'login') + if ((Get-Text $login) -notmatch '[Cc]opilot') { continue } + + # Null for a deleted review; without it there is nothing to attribute the + # thread to, so the thread is skipped rather than attributed to review 0. + $reviewId = Get-Path $c @('pullRequestReview', 'databaseId') + if ($null -eq $reviewId) { continue } + $threads += [pscustomobject]@{ - Review = [long]$c.pullRequestReview.databaseId + Review = [long]$reviewId IsResolved = $t.isResolved IsOutdated = $t.isOutdated Path = $t.path Line = $t.line - Body = ($c.body -replace '\s+', ' ') + Body = ((Get-Text (Get-Path $c @('body'))) -replace '\s+', ' ') } } $cursor = if ($page.pageInfo.hasNextPage) { $page.pageInfo.endCursor } else { $null } } while ($cursor) -$copilotReviews = @($reviews | Where-Object { $_.user.login -match '[Cc]opilot' }) +$copilotReviews = @($reviews | Where-Object { + (Get-Text (Get-Path $_ @('user', 'login'))) -match '[Cc]opilot' + }) # A review's suppressed count is only in its body, as rendered prose. function Get-SuppressedCount { @@ -290,9 +367,14 @@ foreach ($r in $copilotReviews) { if ($processed.ContainsKey([long]$r.id)) { continue } # A review whose inline threads are all resolved may still carry suppressed # findings nobody read, so this is judged on the marker alone. + # Null on a PENDING review, and `[datetime]$null` is an error rather than a + # zero date, so the absence is rendered rather than cast. + $submitted = Get-Path $r @('submitted_at') + $when = if ($submitted) { ([datetime]$submitted).ToString('yyyy-MM-dd HH:mm') } else { 'pending ' } + $suppressedOnly += [pscustomobject]@{ Id = [long]$r.id - When = ([datetime]$r.submitted_at).ToString('yyyy-MM-dd HH:mm') + When = $when Suppressed = $count Inline = @($threads | Where-Object { $_.Review -eq [long]$r.id }).Count } From 17051928454be83ccac5030ca10aeec484c8fbe9 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Mon, 7 Sep 2026 23:50:22 -0400 Subject: [PATCH 06/11] fix(tools): report why a marker was not honoured, instead of accusing its author A review round found that the marker-trust check collapsed two different outcomes into one message, and the message asserted the one the run had not established. That is the same defect this whole tool exists to prevent, so it is worth naming precisely. `GET /repos/{owner}/{repo}/collaborators/{user}/permission` requires the CALLER to have push access. An account without it gets a flat 403 for every login it asks about -- including the maintainer who wrote the markers. The old code mapped every non-zero exit to `$false` and printed: ignored markers: N (author lacks write access) In that case the author has `admin`. A reader following the message goes and checks the marker author's collaborator role, which is fine, rather than their own token, which is the actual cause. Measured against the live API, there are three outcomes and the old code had two. The third is not an error at all, which is what made the conflation easy: exit 0, admin|write -> allowed exit 0, read|none -> denied (a statement about the author) exit 1, HTTP 403 -> unverifiable (a statement about the caller) `octocat` on this repository returns exit 0 with `read`, and `github-actions[bot]` returns exit 0 with `none` -- both successful answers, not failures. Only the 403 is a failure, and only it is unverifiable. `Get-RetireAuthority` now returns those three, counted and reported separately. The unverifiable message says whose problem it is and that every marked review is being re-reported as a consequence. All three still fail closed -- only `allowed` honours a marker -- because over-reporting a handled finding is visible and recoverable, while wrongly honouring one silently deletes the only record that a finding was never read. The bot result is worth its own note, and is now in the script header and DESIGN-NOTES: a marker posted from a workflow using GITHUB_TOKEN is written by `github-actions[bot]`, whose permission reads `none`, so it can never be honoured. Mark from a real account. Measured rather than assumed. Verified on both hosts against real accounts -- owner allowed, non-collaborator denied, bot denied, a repository where this account lacks push unverifiable, and a comment with no author denied -- 5 of 5 correct. Exit contract intact (PR #80 0, PR #81 0, nonexistent PR 2), `test-common.ps1` and the sabotage suite pass on both hosts, encoding 615 files clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- DESIGN-NOTES.md | 13 +++++- tools/scan-pr-reviews.ps1 | 97 +++++++++++++++++++++++++++++---------- 2 files changed, 86 insertions(+), 24 deletions(-) diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index e7c6c5df..34fc67c5 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1742,7 +1742,18 @@ as a writer, so trusting it would enforce something weaker than the control clai fails closed -- a 404, a 403 because the account running the scan cannot query permissions, or any network failure leaves the marker unhonoured -- because over-reporting a finding that was in fact handled is visible and recoverable, while wrongly honouring a marker silently deletes -the only record that a finding was never read. Unhonoured markers are counted and reported. +the only record that a finding was never read. + +Unhonoured markers are counted and reported **separately by cause**, because the two causes +send a reader to different places. A *denied* marker is a statement about its author: the +endpoint answered, and the answer was `read` or `none`. An *unverifiable* one is a statement +about the account running the scan: that endpoint requires the caller to have push access, so +an account without it gets a flat 403 for every login it asks about -- including a maintainer +whose markers are perfectly valid -- and reporting that as "the author lacks write access" +would be an accusation the run never established. Two consequences worth knowing: scanning +from an account without push access re-reports every marked review as outstanding, and a +marker posted from a workflow using `GITHUB_TOKEN` is written by `github-actions[bot]`, whose +permission reads `none`, so it can never be honoured. Both measured. **A marker asserts the review was read, so do not back-fill in bulk.** PR #56 carries 131 reviews with suppressed comments and no marker. Almost all were addressed during the rounds diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 index 18cd5684..57fd2560 100644 --- a/tools/scan-pr-reviews.ps1 +++ b/tools/scan-pr-reviews.ps1 @@ -43,7 +43,18 @@ this control claims to be. This repository is public, so anyone who can comment could otherwise retire a finding that has no other state anywhere. The check fails closed, and markers that were not honoured are counted and - reported rather than silently dropped. + reported rather than silently dropped -- separately by cause, because "the + author has no write access" and "this account cannot query permissions at + all" send a reader to look in different places. + + Two consequences worth knowing before using -MarkProcessed: + + - Running the scan from an account WITHOUT push access to this repository + makes every marker unverifiable (the endpoint 403s for every login), so + every marked review is re-reported as outstanding. + - Posting a marker from a workflow using GITHUB_TOKEN writes it as + github-actions[bot], whose permission reads `none`, so that marker can + never be honoured. Measured, not assumed. Mark from a real account. .OUTPUTS Exit code 0 when nothing is outstanding, 1 when there are findings, and 2 when @@ -244,24 +255,53 @@ $issueComments = Invoke-GitHubJson @('api', "repos/$script:Owner/$script:Name/is # marker over-reports a finding that was in fact handled, which is visible and # recoverable, while wrongly honouring one silently deletes the only record that # a finding was never read. +# Three outcomes, not two, because two of them have different causes and only one +# of them is a statement about the author: +# +# 'allowed' the endpoint answered `admin` or `write`. +# 'denied' the endpoint answered, and it was `read` or `none`. Measured: +# a plain non-collaborator returns exit 0 with `read`, and a bot +# account returns exit 0 with `none` -- neither is an error. +# 'unverifiable' the call failed. The endpoint requires the CALLER to have push +# access, so an account without it gets a flat 403 for every +# login, including the maintainer who wrote the markers. +# +# Collapsing the last two was a defect of exactly the kind this whole tool exists +# to prevent: the run reported "author lacks write access" in a case where it had +# established nothing about the author, and the reader would go and check the +# author's role rather than their own token. +# +# All three still fail closed -- only 'allowed' honours a marker -- because +# over-reporting a handled finding is visible and recoverable, while wrongly +# honouring one silently deletes the only record that a finding was never read. $script:PermissionCache = @{} -function Test-CanRetireFinding { +function Get-RetireAuthority { param([string] $Login) - if (-not $Login) { return $false } + # No author at all (a deleted account leaves `user` null). Nothing to + # attribute the marker to, which is itself a decided answer rather than an + # unanswerable one. + if (-not $Login) { return 'denied' } if ($script:PermissionCache.ContainsKey($Login)) { return $script:PermissionCache[$Login] } - # Deliberately NOT through Invoke-GitHubJson: a non-zero exit here is the - # ordinary answer for a non-collaborator, not a broken instrument, so it must - # not exit 2. + # Deliberately NOT through Invoke-GitHubJson: a non-zero exit here is an + # ordinary answer rather than a broken instrument, so it must not exit 2. $text = Invoke-Native { gh api "repos/$script:Owner/$script:Name/collaborators/$Login/permission" --jq '.permission' } $code = Get-LastExitCode - $permission = if ($null -eq $code -or $code -ne 0) { '' } else { (Get-Text ($text -join '')).Trim() } - $allowed = @('admin', 'write') -contains $permission - $script:PermissionCache[$Login] = $allowed - return $allowed + $authority = if ($null -eq $code -or $code -ne 0) { + 'unverifiable' + } + elseif (@('admin', 'write') -contains (Get-Text ($text -join '')).Trim()) { + 'allowed' + } + else { + 'denied' + } + + $script:PermissionCache[$Login] = $authority + return $authority } # Reviews already recorded as processed, by marker. @@ -272,22 +312,23 @@ function Test-CanRetireFinding { # permanently and silently delete the only record that a finding was never read. # The counter would simply report a smaller number, with nothing to indicate why. $processed = @{} -$ignoredMarkers = 0 +$deniedMarkers = 0 +$unverifiableMarkers = 0 foreach ($c in $issueComments) { $markers = [regex]::Matches((Get-Text $c.body), '') if ($markers.Count -eq 0) { continue } $login = Get-Text (Get-Path $c @('user', 'login')) - if (-not (Test-CanRetireFinding $login)) { - # Counted and reported rather than dropped in silence: a marker that was - # not honoured is either an honest mistake, a permissions problem with - # the account running the scan, or an attempt to retire a finding, and - # all three are worth seeing. - $ignoredMarkers += $markers.Count - continue - } - foreach ($m in $markers) { - $processed[[long]$m.Groups[1].Value] = $true + # Counted by CAUSE rather than lumped together, and never dropped in silence: + # a denied marker says something about its author, an unverifiable one says + # something about the account running this scan, and telling a reader the + # wrong one sends them to look in the wrong place. + switch (Get-RetireAuthority $login) { + 'allowed' { + foreach ($m in $markers) { $processed[[long]$m.Groups[1].Value] = $true } + } + 'denied' { $deniedMarkers += $markers.Count } + default { $unverifiableMarkers += $markers.Count } } } @@ -385,8 +426,18 @@ Write-Report "Copilot reviews: $($copilotReviews.Count)" Write-Report "unresolved threads: $($openThreads.Count) ($($current.Count) current, $($outdated.Count) outdated)" Write-Report "reviews with suppressed: $(@($copilotReviews | Where-Object { (Get-SuppressedCount (Get-Text $_.body)) -gt 0 }).Count)" Write-Report " of those, unprocessed: $($suppressedOnly.Count)" -if ($ignoredMarkers -gt 0) { - Write-Report "ignored markers: $ignoredMarkers (author lacks write access)" -Level warn +if ($deniedMarkers -gt 0) { + Write-Report "markers not honoured: $deniedMarkers (author has no write access here)" -Level warn +} +if ($unverifiableMarkers -gt 0) { + # Says whose problem it is. This fires when the ACCOUNT RUNNING THE SCAN + # cannot query collaborator permissions, which is a 403 for every login it + # asks about -- including a maintainer whose markers are perfectly valid -- + # so pointing at the author would send the reader to check the wrong thing. + Write-Report "markers unverifiable: $unverifiableMarkers" -Level warn + Write-Report " This account could not query collaborator permissions, so no marker" -Level warn + Write-Report " could be confirmed and every marked review is re-reported above. That" -Level warn + Write-Report " is this token's push access, not a statement about who wrote them." -Level warn } Write-Report '' From f06b1bae62532d4c3d10ee43aa3888dfe900f1e6 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Tue, 8 Sep 2026 00:14:19 -0400 Subject: [PATCH 07/11] fix(tools): route the last two redirected native captures through the guard A review round found a native capture this branch had missed. The miss was a sweep failure on my part, and the correction is a wider sweep rather than one line: I had grepped for `2>&1` and converted those, but `2>$null` is the same hazard and I never looked for it. Measured, both spellings, Windows PowerShell 5.1 under `Stop`: git rev-parse ... 2>$null THREW RemoteException git rev-parse ... 2>&1 THREW RemoteException git rev-parse ... OK, LASTEXITCODE=128 **`run-sabotage.ps1`'s `Get-RepoRoot` was the live one, and it inverted this script's own exit contract.** Its whole failure mode IS stderr -- outside a working tree git says `fatal: not a git repository` -- so on 5.1 the deliberate `Exit-WithMessage ... 2` line beneath it was unreachable. The harness died with NativeCommandError and exited 1, which in that script means "sabotages did not behave as declared": a broken instrument reported as a finding, which is the exact separation this branch exists to make. PowerShell 7 reached the intended path either way, which is why it looked fine. Nothing tested it, because the suite only ever ran the harness INSIDE a fixture repository. Added a case that runs it from a non-git directory and asserts exit 2, and sabotage-verified it by reverting the guard: guard reverted, PS 7 -> PASS (the bug is invisible here) guard reverted, PS 5.1 -> FAIL That is the host-specific signal this branch's new `shell: powershell` CI step exists to surface. **`check-workflow-refs.ps1`'s `cargo metadata ... 2>$null` was latent**, and is converted too. Cargo writes to stderr routinely; it happens to stay silent on a warm workspace, which is what kept this hidden. A cold one printing "Downloading", or any warning, would have killed the check on 5.1 while passing on 7. That script does run on 5.1, so the fix is live there. **`check-encoding.ps1` carries the same shape and is deliberately NOT changed.** Converting it looked right and would have been theatre: measured on both this branch and origin/main, the file does not parse under Windows PowerShell 5.1 at all. It has no BOM and contains UTF-8 mojibake fixture strings, which 5.1 reads as the ANSI code page, so it fails at parse time long before reaching any git call. The hazard cannot manifest there. That is a real pre-existing defect -- a repository check that cannot run on one of the two hosts -- but it is a different one, in a file this branch does not otherwise touch, and fixing it means changing that file's encoding rather than its git call. Verified: the new case passes on both hosts and fails on 5.1 against the reverted guard; `test-common.ps1` and the full sabotage suite pass on both; `check-workflow-refs.ps1` passes on both; encoding 615 files clean; the review scanner self-reports 0 on this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/check-workflow-refs.ps1 | 12 +++++++++++- tools/run-sabotage.ps1 | 11 ++++++++++- tools/test-run-sabotage.ps1 | 23 +++++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/tools/check-workflow-refs.ps1 b/tools/check-workflow-refs.ps1 index acd44c10..e18e0274 100644 --- a/tools/check-workflow-refs.ps1 +++ b/tools/check-workflow-refs.ps1 @@ -47,6 +47,11 @@ param( $ErrorActionPreference = 'Stop' Set-StrictMode -Version Latest +# `Invoke-Native`, for the cargo call below. Dot-sourced rather than imported: a +# module copy of that guard does not reach the scriptblock it is handed and +# silently fails on 5.1 alone -- see [common.ps1](common.ps1). +. (Join-Path $PSScriptRoot 'common.ps1') + # Resolved here rather than as a param default: Windows PowerShell 5.1 does not # populate $PSScriptRoot while evaluating a default on a [CmdletBinding()] # script, so the default form fails outright under 5.1 while working under 7. @@ -63,7 +68,12 @@ $repoRoot = Resolve-Path (Join-Path $PSScriptRoot '..') # One `cargo metadata` call answers every crate/bin/example question. `--no-deps` # keeps it to workspace members, which is the only thing a workflow can name. -$metadataJson = & cargo metadata --no-deps --format-version 1 2>$null +# Through `Invoke-Native`: cargo writes to stderr routinely, and any stderr +# redirect makes that a TERMINATING error on Windows PowerShell 5.1 under +# `Stop`. It does not fire on a warm workspace, which is what kept it latent -- +# a cold one that prints "Downloading" or a warning would have killed the check +# on 5.1 while passing on 7. +$metadataJson = Invoke-Native { cargo metadata --no-deps --format-version 1 } if ($LASTEXITCODE -ne 0) { Write-Host "::error::cargo metadata failed, so workflow references cannot be resolved" exit 2 diff --git a/tools/run-sabotage.ps1 b/tools/run-sabotage.ps1 index e2661478..5cb195f9 100644 --- a/tools/run-sabotage.ps1 +++ b/tools/run-sabotage.ps1 @@ -216,7 +216,16 @@ function Exit-WithMessage { } function Get-RepoRoot { - $root = git rev-parse --show-toplevel 2>$null + # Through `Invoke-Native`, and `2>$null` is exactly why. Any stderr redirect + # -- not just `2>&1` -- makes a native command's stderr a TERMINATING error + # on Windows PowerShell 5.1 under `Stop`, and this is the one call in this + # script whose failure mode IS stderr: outside a working tree git says + # `fatal: not a git repository`. Measured on 5.1, unguarded, the line below + # was unreachable -- the script died with NativeCommandError and exited 1, + # which in this script means "sabotages did not behave as declared" rather + # than "you ran me in the wrong directory". PowerShell 7 reached it either + # way, which is why the deliberate exit-2 path looked fine. + $root = Invoke-Native { git rev-parse --show-toplevel } if ($LASTEXITCODE -ne 0) { # Reported, not thrown. Under $ErrorActionPreference = 'Stop' a `throw` # here is a terminating error that prints a stack trace and propagates diff --git a/tools/test-run-sabotage.ps1 b/tools/test-run-sabotage.ps1 index 9b711a2d..1199ed27 100644 --- a/tools/test-run-sabotage.ps1 +++ b/tools/test-run-sabotage.ps1 @@ -463,6 +463,29 @@ Test-Case 'rejects an output directory inside the repository that git does not i finally { Remove-Fixture $root } } +Test-Case 'rejects being run outside a git repository, on both hosts' { + # This one is host-sensitive, and it went unnoticed because the suite only + # ever ran the harness INSIDE a fixture repository. `Get-RepoRoot` captured + # git with `2>$null`, and on Windows PowerShell 5.1 any stderr redirect makes + # a native command's stderr a TERMINATING error under `Stop` -- so outside a + # working tree, where git's whole answer is `fatal: not a git repository` on + # stderr, the deliberate exit-2 path was unreachable. The harness died with + # NativeCommandError and exited 1, which in this script means "sabotages did + # not behave as declared": a broken instrument reported as a finding. + # + # PowerShell 7 reached the intended path either way, which is exactly why + # this needs a test rather than a reading. + $outside = Join-Path ([System.IO.Path]::GetTempPath()) ('nogit-' + [guid]::NewGuid().ToString('N')) + New-Item -ItemType Directory -Force -Path $outside | Out-Null + [System.IO.File]::WriteAllText((Join-Path $outside 'sabotage.json'), (New-Spec | ConvertTo-Json -Depth 8)) + try { + $result = Invoke-Harness -Root $outside -Arguments @('-Manifest', 'sabotage.json', '-List') + Assert-Equal 2 $result.ExitCode $result.Output + Assert-Match 'Not inside a git repository' $result.Output + } + finally { Remove-Item -Recurse -Force $outside -ErrorAction SilentlyContinue } +} + Test-Case 'validates a manifest timeoutSeconds even when -TimeoutSeconds overrides it' { # The check used to be gated on "we are about to use this", so a bogus # manifest value was diagnosed only on the runs that did NOT override it -- From 34025514816e76f1b89b71eba6c6b3b9970ef399 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Tue, 8 Sep 2026 00:23:39 -0400 Subject: [PATCH 08/11] fix(tools): add a stdout-only capture, because merging stderr corrupts parsed output The previous commit routed `cargo metadata` through `Invoke-Native` and turned CI red while passing locally. The cause is a semantic difference between the two redirects that I had treated as equivalent, and it is worth stating plainly because the whole point of that commit was that any stderr redirect is a hazard on 5.1: 2>$null DISCARDS stderr Invoke-Native MERGES stderr into the capture (2>&1) For a transcript, merging is what you want. For output that is PARSED, it is corruption. `cargo metadata`'s stdout is JSON, and a cold CI runner emits rustup's `info: syncing channel updates` on stderr, so the capture became `info: ... {json}` and `ConvertFrom-Json` failed with "Unexpected character encountered while parsing value: i". A warm workspace writes nothing to stderr, which is exactly why it passed here and failed there. Reproduced on both hosts against a stand-in that writes both streams, with the identical error message, and confirmed the fix on both. Added `Invoke-NativeStdout`, which keeps the `Continue` flip -- `2>$null` is still a redirect and still terminating on 5.1 under `Stop` -- while discarding stderr instead of merging it. Swept every `Invoke-Native` call site and converted the four whose output is parsed rather than shown: check-workflow-refs.ps1 cargo metadata -> JSON run-sabotage.ps1 git rev-parse -> a path scan-pr-reviews.ps1 gh api -> JSON scan-pr-reviews.ps1 gh api --jq -> a permission string The last two matter beyond tidiness: a notice on `gh`'s stderr would have corrupted every API response, and in the permission case would have made a genuine writer read as denied -- the marker-trust control failing in the direction that looks like a legitimate refusal. The remaining call sites are transcripts or piped to Out-Null, where merging is intended. Four cases added to test-common.ps1, including the reduced form of the exact CI failure: stderr discarded rather than merged, JSON parseable under stderr noise, the exit code preserved, and no throw on 5.1 despite the redirect. Thirteen cases now, passing on both hosts. Verified: check-workflow-refs, the sabotage suite and the review scanner all pass on both hosts; encoding 615 files clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/check-workflow-refs.ps1 | 13 +++++++------ tools/common.ps1 | 29 +++++++++++++++++++++++++++++ tools/run-sabotage.ps1 | 4 +++- tools/scan-pr-reviews.ps1 | 9 +++++++-- tools/test-common.ps1 | 33 +++++++++++++++++++++++++++++++++ 5 files changed, 79 insertions(+), 9 deletions(-) diff --git a/tools/check-workflow-refs.ps1 b/tools/check-workflow-refs.ps1 index e18e0274..a37b2613 100644 --- a/tools/check-workflow-refs.ps1 +++ b/tools/check-workflow-refs.ps1 @@ -68,12 +68,13 @@ $repoRoot = Resolve-Path (Join-Path $PSScriptRoot '..') # One `cargo metadata` call answers every crate/bin/example question. `--no-deps` # keeps it to workspace members, which is the only thing a workflow can name. -# Through `Invoke-Native`: cargo writes to stderr routinely, and any stderr -# redirect makes that a TERMINATING error on Windows PowerShell 5.1 under -# `Stop`. It does not fire on a warm workspace, which is what kept it latent -- -# a cold one that prints "Downloading" or a warning would have killed the check -# on 5.1 while passing on 7. -$metadataJson = Invoke-Native { cargo metadata --no-deps --format-version 1 } +# Through `Invoke-NativeStdout`, NOT `Invoke-Native`, and the difference is the +# whole point here: this output is parsed as JSON, so stderr must be DISCARDED +# rather than merged into it. Cargo writes to stderr routinely -- a cold runner +# emits rustup's `info: syncing channel updates` -- and merging that in makes +# `ConvertFrom-Json` fail on the `i`. The guard is still needed because any +# stderr redirect is a terminating error on Windows PowerShell 5.1 under `Stop`. +$metadataJson = Invoke-NativeStdout { cargo metadata --no-deps --format-version 1 } if ($LASTEXITCODE -ne 0) { Write-Host "::error::cargo metadata failed, so workflow references cannot be resolved" exit 2 diff --git a/tools/common.ps1 b/tools/common.ps1 index 0b4ce65c..10a589d7 100644 --- a/tools/common.ps1 +++ b/tools/common.ps1 @@ -107,3 +107,32 @@ function Invoke-Native { $ErrorActionPreference = 'Continue' & $Command 2>&1 | ConvertTo-OutputLines } + +# Run a native command and capture ONLY its stdout, discarding stderr. +# +# **Use this whenever the output is PARSED rather than shown.** `Invoke-Native` +# above merges stderr into the capture, which is right for a transcript and +# wrong for data: a command that writes progress or warnings to stderr while +# succeeding on stdout produces a capture with the two interleaved, and the +# parse then fails on text that was never part of the answer. +# +# That is not hypothetical. Routing `cargo metadata --no-deps --format-version 1` +# through the merging helper turned green locally and red in CI, because a warm +# workspace writes nothing to stderr while a cold runner emits rustup's +# `info: syncing channel updates` -- so `ConvertFrom-Json` failed with +# "Unexpected character encountered while parsing value: i". Reproduced on both +# hosts against a stand-in that writes both streams. +# +# The redirect is still what makes this need the same `Continue` flip: on +# Windows PowerShell 5.1 ANY stderr redirect, `2>$null` included, turns a native +# command's stderr into a terminating error under `Stop`. Measured -- both +# spellings throw, an unredirected call does not. +# +# `$LASTEXITCODE` survives, so a caller still distinguishes success from +# failure; what it loses is the diagnostic text, which is the trade a parsed +# command is making anyway. +function Invoke-NativeStdout { + param([Parameter(Mandatory = $true)][scriptblock] $Command) + $ErrorActionPreference = 'Continue' + & $Command 2>$null +} diff --git a/tools/run-sabotage.ps1 b/tools/run-sabotage.ps1 index 5cb195f9..7d4ad829 100644 --- a/tools/run-sabotage.ps1 +++ b/tools/run-sabotage.ps1 @@ -225,7 +225,9 @@ function Get-RepoRoot { # which in this script means "sabotages did not behave as declared" rather # than "you ran me in the wrong directory". PowerShell 7 reached it either # way, which is why the deliberate exit-2 path looked fine. - $root = Invoke-Native { git rev-parse --show-toplevel } + # Stdout only: this is parsed as a PATH, and git can warn on stderr while + # succeeding, which merging would splice into the repository root. + $root = Invoke-NativeStdout { git rev-parse --show-toplevel } if ($LASTEXITCODE -ne 0) { # Reported, not thrown. Under $ErrorActionPreference = 'Stop' a `throw` # here is a terminating error that prints a stack trace and propagates diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 index 57fd2560..e2949d08 100644 --- a/tools/scan-pr-reviews.ps1 +++ b/tools/scan-pr-reviews.ps1 @@ -158,7 +158,10 @@ function Write-Report { function Invoke-GitHubJson { param([string[]] $Arguments) - $text = Invoke-Native { gh @Arguments } + # Stdout only: this is parsed as JSON, so a notice or warning `gh` writes to + # stderr must not be spliced into it. The failure is still reported, from the + # exit code rather than from the text. + $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 ' ')" @@ -285,7 +288,9 @@ function Get-RetireAuthority { # Deliberately NOT through Invoke-GitHubJson: a non-zero exit here is an # ordinary answer rather than a broken instrument, so it must not exit 2. - $text = Invoke-Native { + # Stdout only: the answer is compared against `admin`/`write`, so anything + # `gh` writes to stderr would make a genuine writer read as denied. + $text = Invoke-NativeStdout { gh api "repos/$script:Owner/$script:Name/collaborators/$Login/permission" --jq '.permission' } $code = Get-LastExitCode diff --git a/tools/test-common.ps1 b/tools/test-common.ps1 index 8fabb87f..58ed7499 100644 --- a/tools/test-common.ps1 +++ b/tools/test-common.ps1 @@ -113,6 +113,39 @@ Test-Case 'captured records are plain strings, not ErrorRecords' { } } +# --- Invoke-NativeStdout: the same guard, but for output that is PARSED ------- +# +# The distinction is not cosmetic and these cases exist because getting it wrong +# shipped: routing `cargo metadata` through the MERGING helper passed locally and +# failed in CI, where a cold runner emits rustup's `info:` line on stderr and +# `ConvertFrom-Json` then choked on the `i`. + +Test-Case 'stdout-only capture discards stderr instead of merging it' { + $out = Invoke-NativeStdout { cmd /c "echo noise 1>&2 & echo {`"ok`":true}" } + $text = ("$out").Trim() + Assert-Equal '{"ok":true}' $text 'only stdout may be captured' + if ($text -match 'noise') { throw "stderr leaked into a parsed capture: $text" } +} + +Test-Case 'stdout-only capture still yields parseable JSON under stderr noise' { + # The exact shape that failed in CI, reduced. + $out = Invoke-NativeStdout { cmd /c "echo info: syncing channel updates 1>&2 & echo {`"ok`":true}" } + $parsed = $out | ConvertFrom-Json + Assert-Equal $true $parsed.ok 'the JSON must survive stderr noise' +} + +Test-Case 'stdout-only capture preserves the exit code' { + $null = Invoke-NativeStdout { cmd /c "echo boom 1>&2 & exit 4" } + Assert-Equal 4 $LASTEXITCODE 'LASTEXITCODE after a failing parsed command' +} + +Test-Case 'stdout-only capture does not throw on 5.1 despite redirecting' { + # `2>$null` is a redirect too, so it hits the same 5.1 rule the merging + # helper guards. Without the Continue flip this throws there and passes on 7. + $out = Invoke-NativeStdout { cmd /c "echo only-stderr 1>&2" } + Assert-Equal '' ("$out").Trim() 'a command writing only to stderr yields nothing, and does not throw' +} + # The flip must REACH the scriptblock, and this is the only case that shows it # directly. The stderr case above shows it too, but only on 5.1 -- PowerShell 7 # captures either way, so on 7 nothing else here distinguishes a guard that works From 4b0134dd82428baf56a26b06bed0b8d0e4ab2c37 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Tue, 8 Sep 2026 00:28:58 -0400 Subject: [PATCH 09/11] fix(tools): honour a review marker only inside sentinels, not wherever the text appears Copilot review 5137260817 on PR #81, whose two findings were both suppressed -- so they created no thread and, without this branch's own scanner, nothing would have surfaced them at all. Marker recognition matched `` ANYWHERE in a comment body. That cannot tell a comment that IS a marker from one that merely MENTIONS one, so a maintainer quoting a marker in discussion -- or pasting the example out of this script's own documentation -- would silently retire a review. Since a suppressed-only review has no other state anywhere, retiring it deletes the only record that a finding was never read. Not currently firing: the sole marker in the repository is 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 is already written. Markers are now emitted bracketed: and honoured only on a line of their own inside such a block. The sentinels are defined once so emitter and reader cannot drift, which is the failure this whole tool is about. 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 hosts, 9 of 9: a real marker and a two-id block are honoured; a bare marker, one quoted inline in prose, one inside a fenced block, the placeholder example, a marker sharing a line with a sentinel, an unterminated block, and an empty body are all rejected. The existing marker on PR #80 was bare, so it correctly stopped counting -- the scan went back to exit 1 there, which is the fail-closed direction. Re-marked in the new format and confirmed exit 0 again; its finding is unchanged. Verified: exit contract intact on both hosts (PR #80 0, nonexistent PR 2), test-common.ps1 13 cases pass on both, the sabotage suite passes on both, encoding 615 files clean, workflow references resolve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- DESIGN-NOTES.md | 4 ++- tools/scan-pr-reviews.ps1 | 58 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 5 deletions(-) diff --git a/DESIGN-NOTES.md b/DESIGN-NOTES.md index 34fc67c5..2418f1aa 100644 --- a/DESIGN-NOTES.md +++ b/DESIGN-NOTES.md @@ -1721,7 +1721,9 @@ something. **So suppressed-only reviews are tagged with a marker comment**, posted by the script: ``` - + + + ``` It is an HTML comment, so it does not render; it lives on the pull request rather than in a file diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 index e2949d08..963f0e1b 100644 --- a/tools/scan-pr-reviews.ps1 +++ b/tools/scan-pr-reviews.ps1 @@ -29,7 +29,14 @@ For those, `-MarkProcessed` posts a pull-request comment carrying a marker: - + + + + + A marker is honoured only on a line of its own inside that begin/end block, + so a comment that merely MENTIONS one does not retire anything. The id is + written here as a placeholder rather than digits on purpose: copying this + example verbatim, sentinels included, still matches nothing. The marker is an HTML comment, so it does not render, and it lives on the pull request rather than in a file or a session, which is what makes it @@ -108,6 +115,11 @@ $ErrorActionPreference = 'Stop' $script:ExitFindings = 1 $script:ExitBroken = 2 +# The sentinels that make a marker a marker. Written once so the emitter and the +# reader cannot drift, which is the failure this whole tool is about. +$script:MarkerBegin = '' +$script:MarkerEnd = '' + function Exit-Broken { param([Parameter(Mandatory = $true)][string] $Message) [Console]::Error.WriteLine($Message) @@ -183,8 +195,15 @@ if ($MarkProcessed) { if (-not $Summary) { Exit-Broken 'Summary is required with -MarkProcessed: a marker with no account of what was done is a claim with no evidence.' } - $lines = @($Summary, '') + # Bracketed by sentinels, and the reader below honours a marker ONLY inside + # such a block, on a line of its own. Without that, any occurrence of the + # marker text anywhere in any comment counted -- so a maintainer quoting one + # in ordinary discussion, or pasting an example out of the documentation, + # would silently retire a review. The block is what distinguishes "this + # comment IS a marker" from "this comment MENTIONS one". + $lines = @($Summary, '', $script:MarkerBegin) foreach ($id in $MarkProcessed) { $lines += "" } + $lines += $script:MarkerEnd $file = Join-Path ([System.IO.Path]::GetTempPath()) ("mark-" + [guid]::NewGuid().ToString('N') + '.md') [System.IO.File]::WriteAllText($file, ($lines -join "`n"), [System.Text.UTF8Encoding]::new($false)) try { @@ -309,6 +328,37 @@ function Get-RetireAuthority { return $authority } +# The review ids a comment genuinely marks as processed. +# +# A marker counts only when it sits on a line of its own INSIDE a +# begin/end sentinel block. Matching the marker text anywhere in the body -- what +# this did first -- cannot tell "this comment IS a marker" from "this comment +# MENTIONS one", so quoting one in discussion, or pasting the example out of this +# script's own documentation, would silently retire a review. +# +# The documentation deliberately spells its example with a placeholder id rather +# than digits, so even a verbatim copy of the example, sentinels and all, matches +# nothing here. +function Get-MarkerIds { + param([string] $Body) + + $ids = @() + if (-not $Body) { return $ids } + + # `(?m)` so `^`/`$` bind to lines rather than the whole body, and the + # sentinels are matched literally. + $blockPattern = '(?ms)^\s*' + [regex]::Escape($script:MarkerBegin) + + '\s*$(.*?)^\s*' + [regex]::Escape($script:MarkerEnd) + '\s*$' + + foreach ($block in [regex]::Matches($Body, $blockPattern)) { + foreach ($m in [regex]::Matches($block.Groups[1].Value, + '(?m)^\s*\s*$')) { + $ids += [long]$m.Groups[1].Value + } + } + return $ids +} + # Reviews already recorded as processed, by marker. # # This repository is public, so anyone able to comment on a pull request can post @@ -320,7 +370,7 @@ $processed = @{} $deniedMarkers = 0 $unverifiableMarkers = 0 foreach ($c in $issueComments) { - $markers = [regex]::Matches((Get-Text $c.body), '') + $markers = @(Get-MarkerIds (Get-Text $c.body)) if ($markers.Count -eq 0) { continue } $login = Get-Text (Get-Path $c @('user', 'login')) @@ -330,7 +380,7 @@ foreach ($c in $issueComments) { # wrong one sends them to look in the wrong place. switch (Get-RetireAuthority $login) { 'allowed' { - foreach ($m in $markers) { $processed[[long]$m.Groups[1].Value] = $true } + foreach ($id in $markers) { $processed[$id] = $true } } 'denied' { $deniedMarkers += $markers.Count } default { $unverifiableMarkers += $markers.Count } From 2cc009beae5fce3b09f653acd2504a78c6e61e7c Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Tue, 8 Sep 2026 00:36:17 -0400 Subject: [PATCH 10/11] fix(tools): keep the gh diagnostic on failure without merging it into parsed output Copilot review 5137310730, whose finding is a regression I introduced two commits ago: routing `gh` through the stdout-only capture fixed the JSON corruption and silently emptied every broken-instrument message. Measured which stream `gh` actually uses, because it is not uniform and the difference decides the fix: REST 404 stdout has the JSON error body, stderr `gh: Not Found` network failure stdout EMPTY, stderr `error connecting to ...` usage error stdout EMPTY, stderr the usage text So the 404 case still reported -- which is what made this look fine locally -- while a bad host, a bad flag or an auth problem produced `failed (exit 1): ` with nothing after the colon. Blank exactly when the cause is least guessable, which is the shape a broken-instrument message exists to explain. Added `Invoke-NativeSplit`, which captures both streams separately: stdout stays clean for the parse, stderr is available for the report. Both `gh` call sites use it, preferring stderr and falling back to stdout, so the 404's JSON body is not lost either. The marker-post path is converted too -- it was merging, so a successful post could echo a warning as though it were the comment URL. **The first implementation was wrong on 5.1, and the cross-host suite caught it.** Redirecting stderr to a file (`2>$path`) looks like the clean way to keep the streams apart, and on PowerShell 7 it is. Windows PowerShell 5.1 writes the FORMATTED error record there instead -- `cmd.exe : to-err`, the offending source line, a caret ruler, CategoryInfo and FullyQualifiedErrorId -- so the diagnostic would have arrived wrapped in a stack trace of the helper itself. Two of the new cases failed on 5.1 and passed on 7. The shipped version merges with `2>&1` and partitions by RECORD TYPE, which is lossless on both: PowerShell wraps native stderr in ErrorRecords and leaves stdout as plain strings. Five cases added, including the one that names the defect -- a diagnostic survives when stdout is empty -- plus the 5.1 redirect hazard and an empty stderr arriving as `''` rather than `$null`, which would throw at the call sites under StrictMode. Eighteen cases now, passing on both hosts, and the `-Pr 999999` message is identical on each. Verified: exit contract intact on both hosts, sabotage suite passes on both, encoding 615 files clean, workflow references resolve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/common.ps1 | 53 +++++++++++++++++++++++++++++++++++++++ tools/scan-pr-reviews.ps1 | 38 ++++++++++++++++++++-------- tools/test-common.ps1 | 47 ++++++++++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 10 deletions(-) diff --git a/tools/common.ps1 b/tools/common.ps1 index 10a589d7..4c7397d8 100644 --- a/tools/common.ps1 +++ b/tools/common.ps1 @@ -136,3 +136,56 @@ function Invoke-NativeStdout { $ErrorActionPreference = 'Continue' & $Command 2>$null } + +# Run a native command and capture its streams SEPARATELY. +# +# For the case `Invoke-NativeStdout` cannot serve: output that is parsed on +# success, but whose stderr is the diagnostic worth reporting on failure. +# Discarding stderr keeps the parse clean and throws away the only explanation +# of what went wrong, and merging keeps the explanation and corrupts the parse; +# this keeps both by not choosing. +# +# Measured, because which stream carries the message is NOT uniform and the +# obvious assumption is wrong for the common case. `gh`: +# +# REST 404 stdout carries the JSON error body, stderr `gh: Not Found` +# network failure stdout EMPTY, stderr `error connecting to ...` +# usage error stdout EMPTY, stderr the usage text +# +# So a failure reported from stdout alone is blank exactly when the cause is +# least guessable -- an unreachable host, a bad flag, an auth problem -- which +# is the shape a broken-instrument message exists to explain. +# +# Returns an object with `Stdout`, `Stderr` and `ExitCode`, both texts already +# flattened to plain strings. `$LASTEXITCODE` is also left set, so a caller that +# only wants the code need not unpack anything. +function Invoke-NativeSplit { + param([Parameter(Mandatory = $true)][scriptblock] $Command) + $ErrorActionPreference = 'Continue' + + # Merged with `2>&1`, then partitioned by RECORD TYPE: PowerShell wraps a + # native command's stderr in ErrorRecords and leaves stdout as plain + # strings, so the merge is losslessly separable even though it looks like a + # mixed stream. + # + # A file redirect (`2>$path`) is the obvious alternative and is WRONG on + # Windows PowerShell 5.1. There it writes PowerShell's *formatted* error + # record to the file -- `cmd.exe : to-err`, then the offending source line, a + # caret ruler, CategoryInfo and FullyQualifiedErrorId -- rather than the raw + # stderr text, so the diagnostic would arrive wrapped in a stack trace of + # this helper. PowerShell 7 writes the raw text, so that version passed there + # and failed on 5.1; the cross-host suite caught it. + $merged = & $Command 2>&1 + $code = $LASTEXITCODE + + $stdout = @($merged | Where-Object { $_ -isnot [System.Management.Automation.ErrorRecord] }) + $stderr = @($merged | + Where-Object { $_ -is [System.Management.Automation.ErrorRecord] } | + ForEach-Object { $_.Exception.Message }) -join "`n" + + return [pscustomobject]@{ + Stdout = $stdout + Stderr = $stderr + ExitCode = $code + } +} \ No newline at end of file diff --git a/tools/scan-pr-reviews.ps1 b/tools/scan-pr-reviews.ps1 index 963f0e1b..c1e1d2c9 100644 --- a/tools/scan-pr-reviews.ps1 +++ b/tools/scan-pr-reviews.ps1 @@ -170,13 +170,22 @@ function Write-Report { function Invoke-GitHubJson { param([string[]] $Arguments) - # Stdout only: this is parsed as JSON, so a notice or warning `gh` writes to - # stderr must not be spliced into it. The failure is still reported, from the - # exit code rather than from the text. - $text = Invoke-NativeStdout { gh @Arguments } - $code = Get-LastExitCode + # Streams kept separate, because this needs both and for different reasons: + # stdout is parsed as JSON on success and must not have stderr spliced into + # it, while on failure stderr is usually the ONLY explanation. Measured on + # `gh`: a REST 404 puts its JSON body on stdout, but a network failure or a + # usage error leaves stdout EMPTY and says everything on stderr -- so + # reporting from stdout alone was blank exactly when the cause was least + # guessable. + $result = Invoke-NativeSplit { gh @Arguments } + $text = $result.Stdout + $code = $result.ExitCode if ($null -eq $code -or $code -ne 0) { - Exit-Broken "gh $($Arguments -join ' ') failed (exit $code): $($text -join ' ')" + # stderr first, falling back to stdout: whichever carried the message. + $detail = ("$($result.Stderr)").Trim() + if (-not $detail) { $detail = ("$($text -join ' ')").Trim() } + if (-not $detail) { $detail = '(no output on either stream)' } + Exit-Broken "gh $($Arguments -join ' ') failed (exit $code): $detail" } try { return ($text -join "`n") | ConvertFrom-Json @@ -207,13 +216,22 @@ if ($MarkProcessed) { $file = Join-Path ([System.IO.Path]::GetTempPath()) ("mark-" + [guid]::NewGuid().ToString('N') + '.md') [System.IO.File]::WriteAllText($file, ($lines -join "`n"), [System.Text.UTF8Encoding]::new($false)) try { - $url = Invoke-Native { gh pr comment $Pr --repo "$script:Owner/$script:Name" --body-file $file } - $code = Get-LastExitCode + # Split for the same reason as the API calls: on success stdout is the + # comment URL and is echoed as such, while on failure the explanation is + # on stderr. Merging would have reported the two as one string, so a + # successful post could echo a warning as though it were the URL. + $result = Invoke-NativeSplit { + gh pr comment $Pr --repo "$script:Owner/$script:Name" --body-file $file + } + $code = $result.ExitCode if ($null -eq $code -or $code -ne 0) { - Exit-Broken "posting the marker comment failed (exit $code): $($url -join ' ')" + $detail = ("$($result.Stderr)").Trim() + if (-not $detail) { $detail = ("$($result.Stdout -join ' ')").Trim() } + if (-not $detail) { $detail = '(no output on either stream)' } + Exit-Broken "posting the marker comment failed (exit $code): $detail" } Write-Report "marked processed: $($MarkProcessed -join ', ')" - Write-Report ($url -join ' ') -Level detail + Write-Report ($result.Stdout -join ' ') -Level detail } finally { Remove-Item $file -ErrorAction SilentlyContinue } exit 0 diff --git a/tools/test-common.ps1 b/tools/test-common.ps1 index 58ed7499..b528566c 100644 --- a/tools/test-common.ps1 +++ b/tools/test-common.ps1 @@ -146,6 +146,53 @@ Test-Case 'stdout-only capture does not throw on 5.1 despite redirecting' { Assert-Equal '' ("$out").Trim() 'a command writing only to stderr yields nothing, and does not throw' } +# --- Invoke-NativeSplit: parse stdout, report stderr -------------------------- +# +# For output parsed on success whose stderr is the diagnostic on failure. +# Discarding stderr keeps the parse clean and loses the only explanation of what +# went wrong; merging keeps the explanation and corrupts the parse. These pin +# that it does neither. + +Test-Case 'split capture keeps the two streams apart' { + $r = Invoke-NativeSplit { cmd /c "echo to-err 1>&2 & echo to-out" } + Assert-Equal 'to-out' ("$($r.Stdout)").Trim() 'stdout must not carry stderr' + if (("$($r.Stderr)").Trim() -ne 'to-err') { + throw "stderr must be captured separately, got '$($r.Stderr)'" + } +} + +Test-Case 'split capture parses stdout as JSON under stderr noise' { + $r = Invoke-NativeSplit { cmd /c "echo info: syncing 1>&2 & echo {`"ok`":true}" } + $parsed = $r.Stdout | ConvertFrom-Json + Assert-Equal $true $parsed.ok 'the JSON must survive stderr noise' +} + +# The case the whole helper exists for, and the one a stdout-only capture got +# wrong: measured on `gh`, a network failure or a usage error leaves stdout EMPTY +# and puts the entire explanation on stderr. Reporting from stdout alone was +# blank exactly when the cause was least guessable. +Test-Case 'split capture still has a diagnostic when stdout is empty' { + $r = Invoke-NativeSplit { cmd /c "echo only-on-stderr 1>&2 & exit 5" } + Assert-Equal 5 $r.ExitCode 'the exit code must survive' + Assert-Equal '' ("$($r.Stdout)").Trim() 'this command writes nothing to stdout' + if (("$($r.Stderr)").Trim() -ne 'only-on-stderr') { + throw "the diagnostic must survive on stderr, got '$($r.Stderr)'" + } +} + +Test-Case 'split capture does not throw on 5.1 despite redirecting' { + # A file redirect is still a redirect, so this hits the same 5.1 rule. + $r = Invoke-NativeSplit { cmd /c "echo x 1>&2" } + Assert-Equal 0 $r.ExitCode 'a command that only writes to stderr still succeeds' +} + +Test-Case 'split capture reports an empty stderr as empty, not null' { + # StrictMode: a `$null` here would make `.Trim()` at the call sites throw. + $r = Invoke-NativeSplit { cmd /c "echo quiet" } + Assert-Equal '' ("$($r.Stderr)").Trim() 'a silent command yields empty stderr' + if ($null -eq $r.Stderr) { throw 'Stderr must be an empty string rather than $null' } +} + # The flip must REACH the scriptblock, and this is the only case that shows it # directly. The stderr case above shows it too, but only on 5.1 -- PowerShell 7 # captures either way, so on 7 nothing else here distinguishes a guard that works From c40a2cb6f36279bcc07ba7d98d949eee0b0c0686 Mon Sep 17 00:00:00 2001 From: Mike Grier Date: Tue, 8 Sep 2026 00:43:50 -0400 Subject: [PATCH 11/11] docs(tools): name the right helper in Get-RepoRoot's comment Restatement drift, from an approving review. Converting `Get-RepoRoot` to the stdout-only capture changed the call and left the comment above it still saying "Through `Invoke-Native`", so the explanation named the merging helper while the code used the discarding one. That distinction is the contract being explained -- 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, the older one first, which is how the stale name survived being read. Merged into one that says why the call is guarded at all (any stderr redirect is terminating on 5.1, and this call's failure mode IS stderr) and separately why it is stdout-only (the output becomes the repository root, and a git warning merged into it would corrupt the path). Swept the class rather than the reported line: all 37 `Invoke-Native*` call sites across `tools/`, comparing the helper each one 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 anyway on both hosts: the outside-a-git-repository case passes, the full sabotage suite passes, test-common passes, encoding and workflow-reference checks pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/run-sabotage.ps1 | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/tools/run-sabotage.ps1 b/tools/run-sabotage.ps1 index 7d4ad829..b1f65898 100644 --- a/tools/run-sabotage.ps1 +++ b/tools/run-sabotage.ps1 @@ -216,17 +216,22 @@ function Exit-WithMessage { } function Get-RepoRoot { - # Through `Invoke-Native`, and `2>$null` is exactly why. Any stderr redirect - # -- not just `2>&1` -- makes a native command's stderr a TERMINATING error - # on Windows PowerShell 5.1 under `Stop`, and this is the one call in this - # script whose failure mode IS stderr: outside a working tree git says - # `fatal: not a git repository`. Measured on 5.1, unguarded, the line below - # was unreachable -- the script died with NativeCommandError and exited 1, - # which in this script means "sabotages did not behave as declared" rather - # than "you ran me in the wrong directory". PowerShell 7 reached it either - # way, which is why the deliberate exit-2 path looked fine. - # Stdout only: this is parsed as a PATH, and git can warn on stderr while - # succeeding, which merging would splice into the repository root. + # Through `Invoke-NativeStdout`, and BOTH halves of that name are load + # bearing here. + # + # Guarded at all, because any stderr redirect -- `2>$null` as much as `2>&1` + # -- makes a native command's stderr a TERMINATING error on Windows + # PowerShell 5.1 under `Stop`, and this is the one call in this script whose + # failure mode IS stderr: outside a working tree git says `fatal: not a git + # repository`. Measured on 5.1, unguarded, the line below was unreachable -- + # the script died with NativeCommandError and exited 1, which in this script + # means "sabotages did not behave as declared" rather than "you ran me in + # the wrong directory". PowerShell 7 reached it either way, which is why the + # deliberate exit-2 path looked fine. + # + # Stdout-only rather than the merging `Invoke-Native`, because this output is + # PARSED -- it becomes the repository root. Git can warn on stderr while + # succeeding, and merging would splice that warning into the path. $root = Invoke-NativeStdout { git rev-parse --show-toplevel } if ($LASTEXITCODE -ne 0) { # Reported, not thrown. Under $ErrorActionPreference = 'Stop' a `throw`