Add the sabotage harness, sweeping a copy of the tree

[MikeGrier]

Peels the sabotage harness out of the PR #56 branch, because main needs it now
rather than whenever that branch lands.

Why this is not just tidying

PRs #59 and #63 landed crates/windows-waitable-queues/sabotage.json and
crates/windows-placement-probe/sabotage.json. Both open with:

Run with tools/run-sabotage.ps1; see tools/README-sabotage.md for the format
and for why the results are read the way they are.

Neither file was in main. So main has been shipping two manifests pointing
at a harness that is not in the tree -- one of them for a published crate.

What the harness does

It takes a manifest of deliberate defects and, for each: patches the source,
runs the suite, and records whether the suite noticed. That measures the claim a
green run does not make -- that the tests would fail if the code were wrong.

It exits 0 only when every entry behaves as the manifest declared, and reports
MANIFEST STALE when a pattern stops matching exactly one site, so a sabotage
that silently stopped applying cannot read as a pass. It is deliberately not
a CI gate.

It never touches your working tree

The sweep runs against a copy, refreshed from the working tree at the start
of each run, with its own cargo target directory -- the same approach
cargo mutants takes.

This is the PR's main design decision, and it was made late, after review had
worked through the in-place version. It is a premise rather than a precaution:
patching the developer's own files means every sabotage needs a backup, a
restore, a check that the restore worked, a dirty-tree guard, a containment
check, and a recovery path for when any of that is interrupted -- and each is a
chance to damage work that was never in a commit.

Of the eighteen defects found across seven review rounds here, eight were in
that machinery, and every data-loss risk was one of them. The last was the
sharpest: a byte-exact restore via File.Copy carried the original mtime, so
cargo judged the crate up to date against an artifact built from patched
source and left the sabotaged binary in the developer's build cache.

Working against a copy deletes the backup files, the restore, the restore
verification, the leftover-backup guard, the dirty-tree guard, the git
exit-code check it needed, the -AllowDirty switch, and exit code 3. Two
behaviour improvements fall out: a dirty tree is now swept exactly as it stands
(uncommitted edits are usually the code whose guards you are asking about), and
a bug in this tool now costs a scratch directory rather than your work.

Verification

• Real tree untouched: all 570 tracked files fingerprinted by content hash
  and mtime before and after a sweep; the sets match exactly.
• Copy is faithful: 570 files compared against the real tree, 0 differ.
• placement-probe (9 entries): 9/9 as declared, exit 0, on PowerShell 7 and
  Windows PowerShell 5.1. Parses clean on both.
• waitable-queues (39 entries): 34 as declared. The other 5 are
  MANIFEST STALE against main's own source -- see below.
• Uncommitted work is swept: an uncommitted edit that breaks a test turns
  the baseline red, proving the copy reflects the working tree, not HEAD.
• Cost: cold 69s, warm 49s for the 9-entry manifest; scratch holds 7 MB of
  tree and 379 MB of target.
• tools/check-encoding.ps1, which CI runs, passes: 570 files clean.

A pre-existing defect this found in main

Five of the 39 entries in main's windows-waitable-queues/sabotage.json have
find patterns that occur zero times in main's own
windows-waitable-queues source: the final drain returns nothing and four
reserving_mpsc: entries.

This is not a regression from this PR -- the copy is proven byte-identical to
the real tree, and the patterns were measured directly against the real files.
It means five guards on a published crate are currently unverified, and a
green sweep would never have said so. That the harness reports it as
MANIFEST STALE rather than counting those entries as caught is the whole
reason it distinguishes the two.

Repairing them needs judgement about what reserving_mpsc should now be
sabotaged at, which is a different piece of work from adding the tool, so it is
not fixed here.

Note for whoever lands PR #56

tools/run-sabotage.ps1 and tools/README-sabotage.md also exist on
mikegrier/deferred-namespace-ops. Once this merges, that branch's next merge
from main hits an add/add conflict on both -- the same thing that happened
with the placement probe. Deleting them there and taking main's copies is the
clean resolution; main's are substantially further along.

[Copilot]

🟡 Changes recommended

The harness has a couple of correctness/safety edge cases (cargo --no-run placement and restore-backup handling) that can yield misleading results or overwrite recovery data.

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

Pull request overview

Adds the missing sabotage sweep harness and documentation that existing shipped sabotage.json manifests already reference, so main can actually run those sweeps as intended.

Changes:

• Add tools/run-sabotage.ps1 to apply one sabotage patch at a time, run bounded build+test phases, and record outcomes.
• Add tools/README-sabotage.md documenting the sweep workflow, result interpretation, and manifest format.

File summaries

File	Description
tools/run-sabotage.ps1	Implements the sabotage harness: patch/apply/restore loop with bounded cargo build+test phases and per-sabotage transcripts/backups.
tools/README-sabotage.md	Documents how to run sweeps and author manifests, matching the harness behavior and outcomes.

Review details

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

---

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

[MikeGrier]

Fixed in 60e548d, and this one was a defect in the previous commit rather than in the peeled original -- that commit introduced the backup and asserted "a file left in that directory means a run was interrupted before it could restore", then did nothing to make the assertion hold. A documented claim the code does not enforce is exactly the failure mode, so thank you.

A sweep now refuses to start while any backup is present, names each file, and says what to do with it. Verified by planting one and confirming both halves:

Pre-patch backups from an earlier run are still present:
  ...\.scratch\sabotage\restore\pretend-interrupted.report.rs.bak
That run was interrupted before it could restore its target, so each of
these may be the only copy of the file it names -- under -AllowDirty,
including uncommitted work that is in no commit.
...
This sweep will not start while they are here, because it would overwrite them.
exit: 2

The planted file was still on disk at 21 bytes afterwards. Preserving it is the point; merely detecting it would not have been.

[MikeGrier]

Fixed in 60e548d. Latent rather than live -- I checked both shipped manifests, and all 39 waitable-queues names and all 9 placement-probe names reduce to distinct stems -- but worth closing, and I agree the backup half is the serious one: a shared transcript loses one entry's evidence, while a shared backup path puts two different files' recovery copies at one location.

I took the "compute the stem once" half of your suggestion but not the "fail fast if the path already exists" half, deliberately. Checking for an existing path reports a stale-file symptom, and the actual problem is a manifest containing two entries that cannot be told apart. So the stems are computed and cross-checked up front, and a collision names both offenders:

Two sabotages in this manifest reduce to the same file name stem,
so they would share a transcript and a restore backup:
  probe: colliding name
  probe - colliding name
Both become 'probe-colliding-name'. Rename one so the two differ by more than
punctuation.
exit: 2

The loop then consumes that precomputed stem rather than re-deriving it, so the sanitiser is spelled in one place and the guard cannot end up checking a name the writes do not use. Leftover backups from an interrupted run are covered separately by the guard in the other thread.

[MikeGrier]

Fixed in 60e548d. Real, and latent rather than live -- verified neither shipped manifest's testArgs contains a --, so nothing currently lands past a separator.

Agreed on the consequence, and it is worse than a wrong-argument error: with --no-run going to libtest, the build phase would actually run the tests, so the phase split that keeps ''the compiler rejected this'' apart from ''the tests caught this'' would have been measuring neither.

--no-run is now inserted before any separator via a small Add-CargoFlag helper. Exercised directly on four vectors:

in	out
test -p pkg --locked	test -p pkg --locked --no-run
test -p pkg -- --nocapture	test -p pkg --no-run -- --nocapture
test -- --test-threads=1	test --no-run -- --test-threads=1
-- --nocapture	--no-run -- --nocapture

The last case is why the helper uses Select-Object rather than a range: a -- at index 0 makes the obvious spelling \[0..(\-1)] into [0..-1], and a negative index counts from the end in PowerShell, so it would have silently reversed the vector instead of yielding nothing.

[Copilot]

🟡 Changes recommended

The harness has two concrete correctness hazards (git dirtiness check pathspec and a reserved transcript-name collision) that should be fixed before landing.

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

Review details

Suppressed comments (1)

tools/run-sabotage.ps1:405

• Transcript file names are derived from a sanitized sabotage name stem, but baseline.txt is also written into the same directory. A sabotage named (or sanitizing to) baseline would collide with the baseline transcript and corrupt the run's evidence.

Consider rejecting reserved stems like baseline during the preflight stem-collision check.

foreach ($sabotage in $selected) {
    $stem = $sabotage.name -replace '[^A-Za-z0-9]+', '-'
    if ($stemOwners.ContainsKey($stem)) {
        Exit-WithMessage (@(
                "Two sabotages in this manifest reduce to the same file name stem,"

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

[MikeGrier]

Fixed in 8a2797b, though not for the stated reason -- and the difference matters, so here is the evidence.

The reported mechanism does not reproduce. Git resolves an absolute pathspec against the repository root; it does not require a repo-relative one. With crates/windows-waitable-queues/src/spsc.rs modified, the exact pathspec this script builds matches, from the repo root and from an unrelated working directory alike:

target pathspec : Q:\github\windows-threadpool-sys\crates\windows-waitable-queues\src\spsc.rs
=== does git match the absolute pathspec? ===
MATCHED -> ' M crates/windows-waitable-queues/src/spsc.rs'
=== same query from an unrelated CWD ===
MATCHED -> ' M crates/windows-waitable-queues/src/spsc.rs'

(git 2.55.0.windows.4. Join-Path also normalises the manifest's forward slashes, so the mixed-separator form never actually reaches git.) There was already evidence for this: the guard fired correctly during the previous round's testing, which it could not have done if absolute paths failed to match.

But steelmanning it found a real defect one layer down, which is now fixed. The script read git's stdout and never checked its exit code. A query that genuinely fails prints to stderr and leaves $status empty -- indistinguishable from "the file is clean". The reachable case is a manifest whose root resolves outside the repository, since root may point anywhere:

Could not determine whether this sabotage target is clean in git:
  Q:\github\outside-repo-probe.txt
git exited 128 and said:
  fatal: ... is outside repository at 'Q:/github/windows-threadpool-sys'
Refusing to proceed: a failed check is not a clean result, and
treating it as one is how a sweep overwrites uncommitted work.

Before this commit that same case passed the guard silently and went on to patch a file whose cleanliness was never established. So: right instinct, wrong mechanism, real bug. Thanks.

[MikeGrier]

Also addressed the suppressed comment from this round (tools/run-sabotage.ps1:405, the reserved baseline stem) in 8a2797b -- it was a real collision and worth closing.

Transcripts are named from the sabotage's stem, and baseline.txt plus its .build / .build.err / .err variants are written into the same directory before the sweep starts. A stem of baseline would land on top of the record that the suite was green before any patching -- which is the premise every result in the run depends on, so losing it is worse than losing one entry's transcript.

Rejected up front alongside the existing collision check, case-insensitively, since the sanitiser preserves case while the filesystem does not:

This sabotage's name reduces to a file name stem this directory
already uses for its own transcripts:
  baseline
It becomes 'baseline', which would collide with 'baseline.txt'. Rename it.
exit: 2

One note from testing it: Baseline! sanitises to Baseline-, not baseline, and is correctly not rejected. Only an exact collision is refused, which is the intent -- the guard is about the one name that is actually taken, not about names that merely resemble it.

[Copilot]

🟡 Changes recommended

tools/run-sabotage.ps1 includes PowerShell 7-only syntax (? :) that will fail to parse in Windows PowerShell 5.1, so the tool may not run reliably in all expected environments.

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

Review details

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

[MikeGrier]

Fixed in f987320. Confirmed, and worse than reported -- then acting on it uncovered a second defect behind it that was worse still.

The reported defect, confirmed. It is not just that line: the ternary makes the whole script unparseable under 5.1, so it fails before line 1 executes. Measured with 5.1's own parser (5.1.26100.9168):

PS 5.1 PARSE ERRORS: 9
  line 191: Unexpected token '?' in expression or statement.
  line 191: Missing closing ')' in expression.
  line 191: The hash literal was incomplete.
  ... 6 more cascading

You were right about the sibling scripts too -- check-baseline.ps1, check-borrow-surface.ps1 and check-encoding.ps1 contain zero PowerShell 7 operators, so this one was the odd man out. Rewritten as an if/else.

But fixing only that would have been worse than leaving it. With the script parseable, I ran the full sweep under 5.1 and the baseline failed -- while its own transcript said Finished `test` profile in 2.36s. The build had succeeded and been reported as failed.

Cause: a Start-Process -PassThru Process object on 5.1 does not cache the native handle, so once the process exits .ExitCode reads back $null. Measured directly:

--- WITHOUT touching .Handle ---   ExitCode -> []   isNull=True
--- WITH .Handle touched first --- ExitCode -> [0]  isNull=False

$null -eq 0 is false, so every phase would classify as failed -- meaning the baseline can never pass and, had it passed, every sabotage would report caught. That is a clean bill of health that proves nothing, which is the one result this harness exists to make impossible. The ternary at least failed loudly; repairing it alone would have converted a parse error into a silent wrong answer.

Reading and discarding $process.Handle before the wait keeps the handle alive. It is a no-op on PowerShell 7.

Verified on both shells: parses clean under 5.1 and 7; full sweep under 5.1 now reports 9/9 as declared, exit 0, no leftover backups, clean tree (same invocation aborted at the baseline before this commit); full sweep under 7 unchanged at 9/9.

The README now states 5.1/7 support and why the difference bites here, so the constraint reads as deliberate to the next person editing this file. Good catch -- this one paid off well beyond the line it pointed at.

[Copilot]

🟡 Changes recommended

The new harness currently has a couple of operational footguns (over-broad output-dir cleanup and allowing out-of-repo mutation under -AllowDirty) that should be tightened before approval.

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

Review details

Suppressed comments (1)

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

tools/run-sabotage.ps1:348

• OutputDirectory cleanup deletes all files in the directory (Get-ChildItem ... -File | Remove-Item). Since -OutputDirectory is user-supplied, this can unintentionally remove unrelated files that aren’t transcripts. The docs only promise stale transcripts are cleared, so it’s safer to restrict deletion to the transcript naming convention this script writes (".txt") instead of every file in the directory.

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

[MikeGrier]

Fixed in bc689b8. Correct, and this one is a defect this PR introduced rather than inherited -- my round-2 exit-code check only guarded the -not $AllowDirty branch, so containment was never checked at all under the switch.

Agreed on the framing: -AllowDirty is documented as waiving the cleanliness requirement, and it was in effect waiving the boundary too. Those are different things, and the boundary is the one with no git checkout behind it. The check now runs for every target on every run, before the dirtiness query so an out-of-repo path is reported as what it is rather than as a git pathspec failure:

=== containment, default (no -AllowDirty) ===
Sabotage 'probe outside repo' names a file outside this repository:
  Q:\github\outside-repo-probe.txt
...refused whether or not -AllowDirty was passed -- that switch
waives the cleanliness check, not the boundary.
exit: 2

=== containment, WITH -AllowDirty (the reported gap) ===
[identical output]
exit: 2

=== was the out-of-repo file left untouched? ===
nonexistent

Before this commit the second case patched that file. The comparison is against a canonicalised root with a trailing separator, so a sibling directory whose name merely starts with the root's (...\repo-notes vs ...\repo) cannot pass as inside it.

[MikeGrier]

Also fixed the suppressed comment from this round (tools/run-sabotage.ps1:348, the over-broad -OutputDirectory cleanup) in bc689b8. Also a defect this PR introduced -- that blanket delete was my round-2 stale-transcript fix, and $OutputDirectory being a parameter makes it a genuinely destructive surprise.

I went further than the suggested *.txt* filter, because that filter is not actually safe: the tool's own transcripts and an unrelated notes.txt are indistinguishable by pattern. Instead it now deletes precisely the paths this invocation can write -- baseline plus each selected sabotage's stem, times the four transcript suffixes. That meets the original goal exactly (a transcript named in an error message is always from this run or absent) with no collateral whatsoever.

Verified by planting bystanders, including one that *.txt* would have taken:

=== before ===
baseline.txt           44   <- stale "STALE GREEN TRANSCRIPT from an earlier run"
important.dat          16
unrelated-notes.txt    26   <- matches *.txt*

=== after -List ===
[all three unchanged]

=== after a full sweep ===
baseline.txt        22314   <- replaced, stale text gone
important.dat          16   <- 'unrelated data'
unrelated-notes.txt    26   <- 'a caller kept notes here'

It also moved after the -List exit and after target validation, so neither listing a manifest nor being rejected by one deletes anything -- -List is now read-only, which it should always have been.

Both fixes re-verified with a full sweep on PowerShell 7 and Windows PowerShell 5.1: 9/9 as declared, exit 0, no leftover backups, clean tree.

[Copilot]

🔵 Needs a closer look

The new harness should handle malformed/invalid manifests via controlled Exit-WithMessage paths (parse + required-field validation) to match its stated behavior and avoid hard-to-diagnose failures.

Review details

Suppressed comments (2)

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

tools/run-sabotage.ps1:338

• Manifest JSON parse errors currently surface as a terminating PowerShell error (from ConvertFrom-Json) rather than a controlled Exit-WithMessage exit code. That contradicts the script's own intent to report a bad manifest without aborting the caller's session/script, and it also makes the failure mode less actionable (no consistent exit code 2).

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

tools/run-sabotage.ps1:378

• The script assumes package and sabotages are present in the manifest, but doesn't validate them. If package is missing (and testArgs isn't provided), the baseline will run cargo test -p --locked and fail later with a cargo error; if sabotages is missing/empty, the script reports a misleading "No sabotage ... matches name filter" even when the manifest is malformed. Validating these fields up front makes failures deterministic and actionable.

$package = $spec.package
$testArgs = @('test', '-p', $package, '--locked')
if ($spec.PSObject.Properties.Name -contains 'testArgs' -and $spec.testArgs) {

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

[MikeGrier]

Both findings from this round fixed in 7886a60. Taken as reported, and the class turned out to be six failure modes rather than two, so all six are fixed together.

The framing was right and is worth stating sharply: this was a contract the code stated and did not keep. Exit-WithMessage's own comment has said since the original file that "a diagnostic tool reporting a bad manifest must not take the caller's session down with it" -- while $ErrorActionPreference = 'Stop' meant an unguarded Resolve-Path or ConvertFrom-Json did exactly that.

Measured before, across both invocation paths:

missing manifest file  -List=1  sweep=1   raw Resolve-Path error
malformed JSON         -List=1  sweep=1   raw ConvertFrom-Json error
missing package        -List=1  sweep=1   "The property 'package' cannot be found"
missing sabotages      -List=1  sweep=1   "The property 'sabotages' cannot be found"
empty sabotages        -List=0  sweep=2   <-- -List reported SUCCESS
bad root               -List=1  sweep=1   raw Resolve-Path error

That -List=0 is the one I would not have predicted: listing a manifest with nothing in it exited 0, because the -List branch returns before the emptiness check. After the fix every mode is exit 2 on both paths:

No manifest at: .scratch\does-not-exist.json

This manifest is not valid JSON:
  ...\bad-json.json
The parser said:
  Conversion from JSON failed with error: Invalid character after parsing
  property name. Expected ':' but got: v. Path 'package', line 1, position 22.

This manifest declares no 'package':
  ...\no-package.json
It is required unless 'testArgs' supplies the cargo command instead.
Without it the sweep would run 'cargo test -p  --locked' and fail on
an empty package name well after the baseline started.

Your point about the misleading filter message is handled specifically: an empty sabotages now reports "this manifest's 'sabotages' array is empty" rather than "no sabotage matches name filter '*'", which sent the reader to look at a filter they never set.

I extended it to per-sabotage fields for the same reason -- every field in the README's format table is required, and under Set-StrictMode a missing one surfaced as a property error against a line of this script. expect is also checked to be caught/survives, since an unrecognised value is not inert: it is compared for equality when scoring, so it could never match and the entry would read as misbehaving on every run whatever the suite did.

One deliberate subtlety: the validator uses the same case-insensitive comparison the scorer uses, so it cannot reject a manifest that would in fact score correctly. expect: "Caught" is accepted by both; expect: "maybe" by neither. I checked both directions, because a validator stricter than the thing it guards is its own defect.

Verified: all eight modes exit 2 on both paths with actionable messages; a valid manifest still exits 0 on -List; both shipped manifests satisfy every newly-enforced rule (48 entries, zero violations), so nothing that already works is rejected; full sweep on PowerShell 7 and 5.1 still 9/9, exit 0, clean tree. The README now documents the exit codes as a table.

[Copilot]

🟡 Changes recommended

The new harness has a couple of confirmed behavioral/reporting issues (including a -List side-effect/behavior mismatch and an unhandled throw path) that should be corrected before approval.

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

Review details

Suppressed comments (1)

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

tools/run-sabotage.ps1:159

• Get-RepoRoot uses throw when not inside a git repository. With $ErrorActionPreference = 'Stop' this becomes an unhandled terminating error (stack trace) and can abort a calling script/session, which conflicts with the script's own goal of reporting invocation/manifest problems via Exit-WithMessage with a specific exit code.

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

[MikeGrier]

Fixed in ca638c0 -- and this one is the sharpest catch of the six rounds, because it is a claim I wrote one round ago that the code did not keep.

Round four moved the transcript clearing after the -List exit, and I added to the README: "Listing a manifest with -List writes and deletes nothing." That was false. The output directory and its restore/ subdirectory were still created before the listing path, and the leftover-backup guard also ran before it -- so -List both wrote to disk and could be refused outright because an earlier run had been interrupted. As you say, that is exactly when someone wants to read the manifest.

All of that setup now happens after the -List exit. Verified all three properties:

=== A. -List must create NOTHING ===
before: .scratch\sabotage exists = False
  -List exit=0
after : .scratch\sabotage exists = False

=== B. -List must work despite a leftover backup ===
  [manifest lists normally]
  -List exit=0

=== C. a real sweep must still refuse ===
  Pre-patch backups from an earlier run are still present: ...
  sweep exit=2
  backup preserved = True

The guard's message now also mentions that -List still works meanwhile.

[MikeGrier]

Both suppressed comments from this round are also fixed in 7d60e29.

tools/run-sabotage.ps1:155 -- timeout validation. Confirmed, and the zero case is worse than the negative one. Measured:

WaitForExit(0) on a running process -> False       [=> reported as HUNG]
WaitForExit(-1000)                  -> THREW MethodInvocationException

So -TimeoutSeconds 0 does not disable the bound, it makes every phase time out instantly -- and this tool reports a timed-out phase as caught. A sweep with a zero bound would therefore have returned every sabotage caught, exit 0: a perfectly green result proving nothing, which is the single outcome the harness exists to make impossible. The negative case is the report-don't-throw violation you name. Both now exit 2 before anything runs:

-TimeoutSeconds must be at least 1 second, and was 0.
A bound of zero does not disable the timeout; it makes every phase
time out instantly, which this tool would report as every sabotage
being caught -- a green sweep that proved nothing. Pass a real budget instead.

Verified for -TimeoutSeconds 0, -TimeoutSeconds -5, and -BuildTimeoutSeconds 0.

tools/run-sabotage.ps1:319 -- Format-Patch empty replacement. Confirmed, and it is live rather than hypothetical: splitting an empty replacement yields one empty element, so the old order printed + and then + (removed), and 5 of the 48 entries across the two shipped manifests use the deleting form. So this was every deletion sabotage's patch display. Now:

    --- injected patch ---
    - self.doorbell.signal();
    + (removed)

with the substitution case unchanged.

[Copilot]

🔵 Needs a closer look

Manifest validation still allows some invalid find/replace shapes to degrade into confusing outcomes or unintended destructive patches instead of failing fast with a clear exit-2 error.

Review details

Suppressed comments (1)

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

tools/run-sabotage.ps1:469

• Manifest-entry validation only checks that find/replace properties exist, not that they have a valid shape/value. In particular, an empty/null find produces a confusing MANIFEST STALE: pattern found ... times result, and a null replace is currently treated like deletion (because -join yields an empty string), which is likely to be an accidental destructive patch.

Consider rejecting empty find and null replace up-front with an exit-2 manifest error so invalid manifests fail fast with a clear message.

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

[Copilot]

🔵 Needs a closer look

It introduces a large new execution harness (process management + filesystem mutation in a working copy) that should get final human validation on Windows in addition to the small doc/message fixes noted.

Review details

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

[Copilot]

🟡 Changes recommended

The new README has a couple of concrete doc/schema mismatches (notably a fixed “60s” claim vs derived default timeout behavior) that should be corrected before merging.

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

Review details

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

[Copilot]

🔵 Needs a closer look

There are a few concrete documentation inconsistencies/ambiguities (and one misleading in-script comment) that should be corrected to avoid future misuse of the harness/manifest format.

Review details

Suppressed comments (4)

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

tools/README-sabotage.md:64

• The formula uses leading '-' characters (-TimeoutFloorSeconds, -TimeoutMultiplier) inside the max(...) expression. In Markdown this reads like subtraction, and max(15, 3x) is also ambiguous about what is being multiplied. Rephrasing the formula to use bare parameter names and an explicit baseline term is clearer.
  tools/run-sabotage.ps1:920
• The timeout-derivation comment says the baseline duration includes the build, but Invoke-Sabotaged returns the run/test phase seconds (build seconds are not included). This makes the comment contradict the very next paragraph and can mislead future edits about what baseline.Seconds represents.

tools/README-sabotage.md:218

• The manifest-format table lists timeoutSeconds twice with the same field name, once for the whole sweep and once for per-entry. This is ambiguous for readers and for anyone validating manifests manually.

| `timeoutSeconds` | no | Hang bound for this entry alone, when it legitimately runs far longer than the rest. Only ever raises the sweep's bound. |

tools/README-sabotage.md:217

• The docs say replace deletes with [''], but the shipped manifest also uses an empty array ("replace": []) to delete a matched block. Since the script joins the array with newlines, both spellings are equivalent; documenting both avoids confusing manifest authors.

| `replace` | yes | Replacement lines. `[""]` deletes. |

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

[Copilot]

🔵 Needs a closer look

The new README’s exit-code semantics don’t fully match the script’s behavior (baseline failures also exit 2), and there is also unused code that should be cleaned up.

Review details

Suppressed comments (2)

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

tools/README-sabotage.md:33

• The exit-code table says exit 2 only means “manifest or invocation is wrong”, but run-sabotage.ps1 also exits 2 when the baseline suite is red. The table should mention the baseline-failed case so callers don’t misinterpret exit 2 as strictly a manifest problem.
  tools/run-sabotage.ps1:870
• $treeRootPrefix is computed but never used. This looks like leftover code from an earlier version and adds noise to a script that already has a lot of moving parts.

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

[Copilot]

🔵 Needs a closer look

Manifest timeoutSeconds validation currently treats 0 as “not provided” due to truthiness checks, making the documented/reported “must be at least 1” behavior unenforced.

Review details

Suppressed comments (2)

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

tools/run-sabotage.ps1:597

• Manifest-level timeoutSeconds = 0 will currently bypass validation because the condition uses truthiness (-and $spec.timeoutSeconds). In PowerShell, 0 is $false, so the "must be at least 1" error path becomes unreachable and an explicitly invalid manifest value is silently ignored (contradicting the README text and the error message).

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

tools/run-sabotage.ps1:607

• Per-sabotage timeoutSeconds = 0 is also not rejected for the same reason: the guard uses $entry.timeoutSeconds as a truthiness check, so 0 skips validation. If 0 is meant to be invalid (as the error message says), the check should treat "present and 0" as an error rather than as "not provided".

foreach ($entry in @($spec.sabotages)) {
    if (($entry.PSObject.Properties.Name -contains 'timeoutSeconds') -and
        $entry.timeoutSeconds -and [int]$entry.timeoutSeconds -lt 1) {
        Exit-WithMessage (@(
                "The sabotage '$($entry.name)' sets timeoutSeconds = $($entry.timeoutSeconds)."
                "It must be at least 1; a bound of zero reports it as caught without running."
            ) -join "`n") 2
    }
}

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

[Copilot]

🟢 Approval recommended

The harness and documentation are internally consistent with the stated “copy-only” safety model, and the updated manifest patterns match the current source locations they target.

Review details

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