test(installer): prove every guard still catches its own bug (backend#2849) - #931
test(installer): prove every guard still catches its own bug (backend#2849)#931shujaatTracebloc wants to merge 20 commits into
Conversation
…ath (backend#2849) An unbounded external call against a SICK dependency does not fail, it BLOCKS — and this journey has now spent four runs learning that the hard way, twice (backend#2675's reboot prompt, backend#2849's empty exit code). Both were "the caller spent its whole budget and reported a timeout that named nothing". Auditing the never-executed Windows path for the same shape turned up three more, all on the route the e2e leg is about to walk for the first time. This file already has the rule — Invoke-BoundedProcess, "installer external-call timeout rule" — so each of these is a site that predates it, not a new policy. 1. THE ENGINE-UP PROBE (Install-DockerDesktop). The wait that exists to survive a bad Docker start read `docker info` natively, so a wedged daemon — a half-open \\.\pipe\docker_engine, which is what Docker Desktop leaves when it starts and gives up — blocked the probe itself. New Test-DockerEngineUp goes through Invoke-DockerCli at 15s. A timeout reads as "not up", never as up: mistaking a wedged daemon for a healthy one would send the install into Step 3 on an engine that is not there. 2. THAT WAIT'S DEADLINE WAS NOT A DEADLINE. `$maxWait = $waitMin * 20` assumed each pass costs exactly its 3s sleep, which stops being true the moment a probe blocks — so the 10-minute cap the code believed it had was an iteration count. Now wall-clock, checked in the loop guard. 3. THE STEP-3 PREFLIGHT READERS (Get-PfRuntimeMemGb/MemMib/Cpu) are the first thing to touch Docker after the engine wait, and had the same bare call. An install that got past Step 2 on a sick engine would hang at the ENTRY to Step 3, before anything printed, instead of falling back to the host reader as their own $null contract promises. 4. THE CLI INSTALLER CHILD had the only `WaitForExit()` in ~7400 lines with no bound. It runs `irm <cli install.ps1> | iex` — a network fetch we then execute — so a stalled handshake or a slow CDN parked the whole install with no output and nothing to kill. 10-minute deadline, then killed; a timeout is non-fatal because a failed CLI install already is (Step 4 falls back to the legacy credential flow). Scope, deliberately: this does NOT try to make Docker Desktop supported on Windows Server. It makes the answer ARRIVE — bounded and named — instead of arriving as a 30-minute silence. The field test (backend#1232) reached a working environment on Windows 11, so Steps 3-6 do work; what the e2e host adds is an unsupported SKU, and one run should be able to say so. The two Get-Pf* tests that mocked `docker` natively now mock Invoke-DockerCli — same assertions, seam moved out one layer. Nothing removed; a timed-out-probe case is added, which was previously unreachable. 958 Pester assertions pass on BOTH 5.5.0 and 6.0.1 (was 948). Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ess, not a failure (backend#2849) Finding 1 of backend#2849: on the Windows Server 2022 journey host Docker Desktop installed cleanly (full tree on disk, no error log) yet the run aborted. Docker's installer returns 3010 (ERROR_SUCCESS_REBOOT_REQUIRED) whenever the WSL2 backend adds Windows features -- our exact --backend=wsl-2 path, and expected success per Docker's enterprise-deployment docs -- but Invoke-TrackedInstall judged success as `$p.ExitCode -eq 0`, so a completed install was misfiled 'failed'. That is the client#611 idiom one layer up. Add an opt-in -SuccessExitCodes to the shared Invoke-TrackedInstall (default @(0)) and pass the documented reboot-pending SUCCESS codes at the three installer call sites: Win32 3010/1641 (a direct installer, e.g. Docker) and winget 0x8A150109/0x8A15010B as Int32 (-1978334967/-1978334965). winget's 0x8A15010A (REBOOT_REQUIRED_FOR_INSTALL, a real failure) is deliberately excluded. The real code is preserved in the return so a reboot-pending 'ok' stays visible in the log. Follows #913 (finding 2, the empty exit-code slot), which made the code readable in the first place. The WSL-update helper keeps its bare -eq 0 on purpose: its symptom was the null code (fixed by #913), it is non-fatal, and a reboot code there is speculative. Tests: reboot codes accepted end-to-end (incl. the negative winget HRESULTs), the winget failure reboot code stays 'failed', the default set stays @(0) so non-installer callers don't start tolerating 3010, and both Docker paths opt in. Full Pester suite 801/0/15; PSScriptAnalyzer 0 errors on install-k8s.ps1; scripts/manifest.sha256 regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the Docker install log (backend#2849) Review follow-up (shujaatTracebloc): the post-install log line claimed "continuing to bring up the engine" for BOTH reboot-pending families, but 1641 / winget 0x8A15010B (INSTALL_REBOOT_INITIATED) mean the installer has already started restarting the machine -- so on those codes that line is the last thing written before the box goes down, reading as a script that carried on when it didn't. Given this PR exists because a log said the wrong thing, the message should not. Add $script:INSTALLER_REBOOT_INITIATED_CODES = @(1641, -1978334965) (a subset of the OK codes) and branch the Docker log: an INITIATED code says the machine is restarting and the install resumes via the reboot handoff; a merely-required reboot (3010 / 0x8A150109, box still up) keeps "continuing to bring up the engine". Behavior is unchanged -- both remain 'ok' and press on; only the log text differs. Test: the INITIATED subset is exactly @(1641, -1978334965), is a subset of the OK codes, excludes 3010/0x8A150109, and the Docker handler branches on it. Pester 802/0/15; PSScriptAnalyzer 0 errors; scripts/manifest.sha256 regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-docker-probes # Conflicts: # scripts/manifest.sha256 # scripts/tests/install-k8s.Tests.ps1
…oth Docker paths (backend#2849) Bugbot (Medium): the reboot-INITIATED log line claimed "resumes via the reboot handoff", but Register-ResumeAfterReboot's RunOnce is armed only in Step 1 and is already spent by the Step-2 Docker install — so no handoff was actually armed. The winget path (tried first, the one that can return the winget HRESULT) never logged or stopped either, so an initiated reboot could drop the box mid-install and leave the run looking interrupted / the engine wait Err-ing. Replace the inline log with Invoke-PostInstallReboot, called from BOTH Docker install paths so the handling can't depend on which ran: - REQUIRED (3010 / winget 0x8A150109, box still up): log and continue, as before. - INITIATED (1641 / winget 0x8A15010B, already restarting): arm a FRESH resume-after-reboot and stop with the declared exit 2 — the same handoff Step 1 uses — so the install genuinely resumes instead of racing the reboot. The message no longer promises a handoff that isn't armed. Our flags never allow a reboot (--quiet; no winget --allow-reboot), so INITIATED stays the unexpected-but-safe branch; this makes it correct rather than merely quiet. Tests: the initiated branch arms a resume + exit 2 and both paths route through the handler (source, since exit 2 would end Pester); the REQUIRED/no-op branches never arm or exit (behavioral, Should -Invoke ... -Times 0). Pester 804/0/15; PSScriptAnalyzer 0 errors; manifest regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…xit-code' into fix/2849-bound-the-docker-probes # Conflicts: # scripts/manifest.sha256
… backend#2849) Reported from a real Windows machine testing from develop: "it just closed the PowerShell". #577 -- "the PowerShell installer must never terminate ungracefully, always show the user a clean what-happened" -- was fixed in install-k8s.ps1, which runs as a CHILD process where `exit` costs nothing. install.ps1 was missed, and it is the one that runs INSIDE the user's console via the documented `irm ... | iex`. A top-level `exit` there ends THEIR session: the window closes and takes the outcome with it, which is exactly the symptom #577 closed. Not only a failure path. `exit $LASTEXITCODE` after the child returns fires on EVERY run, so a perfectly successful install also slammed the window shut over its own summary. All three exit sites are affected (the child's code, the platform gate, the top-level catch). The exit CODE still propagates untouched. Swapping `exit` for `return` would have been the obvious fix and is wrong: `powershell.exe -Command` callers would then read 0 for every outcome, and the e2e harness reads this code to tell install-k8s.ps1's declared `exit 2` reboot handoff from a real failure -- a failed install would book as a pass. So Complete-Bootstrap holds the window open long enough to be read, then exits exactly as before. The hold is BOUNDED (60s) and gated on the same Test-CanPrompt predicate install-k8s.ps1 uses, so CI, a service and any piped/redirected stdin take the no-hold path -- an unbounded hold is the very class this ticket has spent its life removing. A host that cannot report keystrokes (ISE, a redirected console) throws on KeyAvailable and is caught. 977 Pester assertions pass on BOTH 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…es (client#917) Measured on a real Windows box, testing this very branch: Installation stopped: Ref 'fix/2849-bound-the-docker-probes' contains a path separator or '..' -- refusing to build a fetch URL from it. The belt-and-suspenders guard refused '/' outright, which broke the ONLY flow it exists for. Every real development branch is `fix/1234-thing` or `feat/...`, so the documented developer override could fetch `develop`, `staging` and `main` and nothing else -- while the whole point of the escape hatch is testing UNRELEASED code, which lives on feature branches. '..' is the actual traversal lever and is still refused on EVERY path, opt-in or not. The R8 property never rested on '/': a TAG still cannot carry one, because the vX.Y.Z shape check rejects it -- so the case that comment names, 'v1.2.3-../../heads/main', is refused twice over. A '/' is accepted only on the path that has already announced itself as an unverified branch install and printed the four-line warning. Multi-segment refs are held to exactly the shape a single segment is: validated segment by segment, so a leading, trailing or doubled slash (an empty segment) and a bare '.' are all refused. 985 Pester assertions pass on BOTH 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-docker-probes # Conflicts: # scripts/install-k8s.ps1 # scripts/manifest.sha256 # scripts/tests/install-k8s.Tests.ps1
…ff-Windows stall Addresses @LukasWodka's change-request and @aptracebloc's independent review on client#917, plus both open Bugbot findings. Each fix is mutation-proven below. 1. THE GUARD DEFENDED ONE VERB, THE TITLE CLAIMED THE CLASS (blocking) `Should -Not -Match '\(docker info'` has an input domain of one needle, so a new unbounded native call walked past it -- Lukas injected `docker ps -a` and watched all 38 tests stay green. Seven native calls were live at head, and not in a corner: `Set-ClusterAutostart` (MAIN install path), three cluster-REUSE inspects, and the two `-Diagnose` calls, i.e. the bundle a user collects BECAUSE Docker is wedged -- bare there hangs the one tool meant to explain the hang. All seven now go through `Invoke-DockerCli` with a 20s bound. Every timeout degrades in the safe direction: autostart logs and skips (k3d already sets unless-stopped), the advisories stay silent rather than inverting into a false warning, and the dataset-mount precondition is SKIPPED rather than fired -- inferring "no mount" from a wedged daemon would abort a correct cluster. The guard is now the class, asserted on the PowerShell AST rather than text: no native `docker` invocation is unbounded, where bounded means `Invoke-DockerCli` or a `Start-Job` reaped by `Wait-JobWithProgress`. AST is load-bearing -- ~10 Log/return strings in this file contain "docker run" / "docker build", so every text version either drowns in false positives or gets narrowed back into an instance guard. A second test requires the job sites actually be reaped on a deadline, closing "bounded-looking unbounded". Both fail closed on a parse error and refuse to pass vacuously. 2. A STALE KEYSTROKE COLLAPSED THE HOLD TO ZERO (install.ps1) `[Console]::KeyAvailable` reports what is QUEUED, not a press since the hold began, and nothing drained it. install-k8s.ps1 runs as a child on the same console with 11 Read-Host sites, so one extra Enter -- or the trailing newline of `irm | iex` -- made the wait false on its first evaluation and closed the window over the summary: the exact #577 failure this function exists to stop, under a line that had just promised to wait. Drained before the deadline wait, inside the same try. The test pins the ORDER, since draining after the wait would eat the user's real keypress and fix nothing. 3. THE WRONG-PLATFORM BAIL STALLED 60s WHERE NO WINDOW EXISTS `[Environment]::UserInteractive` is hardcoded $true on non-Windows .NET, so the predicate reduced to "stdin not redirected" and an instant, zero-cost bail-out held for a minute -- on the one branch that only runs off Windows, where `exit` from a child pwsh closes nothing. Now a bare `exit 1`. The `$bare.Count | Should -Be 1` assertion was a count standing in for a rule, which made this fix look like a regression. Replaced with the actual property, derived: no `exit` outside Complete-Bootstrap or the platform gate. 4. THE ELAPSED LABEL WAS THE LEAST HONEST THING ON SCREEN (Bugbot, both reviews) The deadline became wall-clock; the label still divided the iteration counter by 20. With the probe capped at 15s a wedged pass costs ~18s, so the loop exited at ~10 REAL minutes still reading "1 min elapsed", immediately before "didn't come up within 10 minutes". Derived from `$dockerStart`, the same clock as the deadline. `$maxWait` is gone entirely -- with the bound expressed as a deadline it had no job left, and keeping it invites the slip back in. 5. THE PROMPT PREDICATE WAS A COPY CHECKED AGAINST A COPY The old assertion hand-wrote the rule and tested install.ps1 only, so if Test-CanPrompt gained a condition the two would diverge silently and it would stay green. Now parsed out of BOTH files and required to agree, failing closed if either is unreadable. The doc comment no longer asserts the e2e journey takes the no-hold path -- an interactive desktop session satisfies both conjuncts, and that assumption was already measured false once on this journey. Correctness rests on the bound, not the predicate. Also: the two bare `Should -Throw` now pin `-ExpectedMessage` (the tag-shaped traversal case was satisfied by any of three refusals), and Set-ClusterAutostart's tests move their seam to `Invoke-DockerCli` -- same assertions, plus timeout and blank-line cases that were unreachable while the calls were bare. The Helm winget gap Lukas raised is fixed in develop: #921's merged version carried it, and develop's own invariant test (accepts == routes) now covers it. MUTATION-PROVEN, each reverted in isolation: - Lukas's injected `docker ps -a` -> both class guards redden (was green) - unbound the -Diagnose `docker ps -a` -> both class guards redden - remove the input-buffer drain -> drain-order test reddens - platform gate back to Complete-Bootstrap -> gate test reddens - diverge the two prompt predicates -> drift test reddens (was green) - elapsed label back to `$i / 20` -> same-clock test reddens 889 Pester assertions pass on BOTH 5.5.0 and 6.0.1, 0 failures. Both .ps1 parse clean, check-style.sh clean, manifest regenerated. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reported from a real dev install on Windows: a `CLIENT_ENV=dev` run told the operator to create a client and fetch credentials at https://ai.tracebloc.io/clients -- the PRODUCTION dashboard -- and credentials from there are then rejected by dev-api. The operator is sent to the wrong place at exactly the moment they need the right one. Hardcoded at THIRTEEN sites, while Get-BackendUrl sitting right above them was correctly env-aware. Both installers had it; this is the PowerShell half. The hosts are the BACKEND'S OWN per-environment settings, not a guess -- DEVICE_VERIFICATION_URI / RESET_PASSWORD_URL in xraybackend/settings/{dev,stg,prod}.py: dev -> https://dev.tracebloc.io stg -> https://stg.tracebloc.io prod -> https://ai.tracebloc.io Get-TraceblocDashboardUrl uses the SAME vocabulary and the same unknown->prod fallback as Get-BackendUrl, so the two can never disagree about which environment an install belongs to -- and a test pairs them per environment rather than asserting each alone, because the defect WAS precisely those two disagreeing. The guard that keeps it fixed: each of the three hosts must appear exactly once in the file (as its switch arm), and no live link may carry a path -- a hardcoded link always has one (/clients, /my-use-cases), while the bare host is only ever the mapping. Caught in review of my own change: the bare-host substitution rewrote the helper's OWN default arm into a call to itself, so the suite hung on infinite recursion instead of failing. Fixed, and the "appears exactly once" guard would now catch that shape too. 1019 Pester assertions pass on BOTH 5.5.0 and 6.0.1 (was 1013). Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…function @LukasWodka's follow-up on the reap guard, and he proved it rather than asserted it: the check matched `Wait-JobWithProgress -Job $x -TimeoutSec N` anywhere in the enclosing FUNCTION, so one compliant job vouched for its neighbours. He added a second, unreaped docker job to a function that already had a good one and watched all 41 tests stay green. The guard now walks from each native docker call out to its `Start-Job`, finds the variable that job is assigned to, and requires a reap naming THAT variable. A job with no assignment at all fails too -- nothing can reap what nothing names. Mutation-proven with his exact probe (an unreaped second job inside Test-ExistingClusterKubeletConfig, which already reaps $job correctly): [-] the Start-Job docker sites are actually reaped on a deadline, not merely in a job Test-ExistingClusterKubeletConfig starts docker job 'sneaky' but never reaps THAT job on a deadline Was Passed=41 Failed=0. The failure now names the offending job variable. 902 Pester assertions pass on both 5.5.0 and 6.0.1, 0 failures. check-style.sh clean, manifest regenerated. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…#2849) A fix is not fixed until a guard has been SEEN to fail. Both of this week's Windows defects were "already fixed", and nothing in CI could say otherwise: * #577 shipped a graceful-exit boundary and PR #588 DID touch install.ps1 -- but only its message. Every `exit` was untouched, so the bootstrap kept closing the user's console for another month. Reported from a real machine as "it just closed the PowerShell". * install-k8s.ps1's own comment promised WaitForExit() "guarantees ExitCode is populated for every caller". It did not, so every failure in Invoke-TrackedInstall rendered `exited ` with an empty slot -- and a Docker install that SUCCEEDED with 3010 was filed as a failure. Both were found by a human running the installer. The structural reason nothing else could: most installer guards read a property off the SOURCE TEXT, and such a guard passes forever once the string drifts -- "green" and "no longer looking" are the same colour. And every exit path sits behind `if (-not $env:TB_PESTER)` while every suite sets TB_PESTER=1, so NO test in this repo has ever executed an `exit`. mutation-check.ps1 reintroduces each fixed defect into a COPY of the tree and requires the claiming suite to go red. 11 registered, 11 caught. `-Dry` resolves markers only and says in as many words that it is not evidence anything bites. Four things it refuses to do, each because it has gone wrong somewhere: never mutates the working tree (nothing to restore, nothing a SIGKILL strands); never trusts a marker matching more than one line (a duplicated line becomes a loud STALE, and an `After` anchor disambiguates the two byte-identical Test-CanPrompt guards); never reports a catch without a green baseline; never lets a mutation be a no-op. A FRESH PROCESS PER RUN, found the hard way: the first version ran every suite in one pwsh session and reported two baseline failures that do not reproduce when the file is run alone -- Pester state survives between Invoke-Pester calls, so run N is not run 1. A harness whose verdicts depend on how many times it has already run cannot say whether a guard bit. It also now NAMES a failing baseline instead of printing a count, which is the same diagnostic gap these tickets are about. Wired into installer-tests.yaml, and .cursor/BUGBOT.md gains the six review rules this week produced -- fix-in-one-twin-only, guards behind TB_PESTER, comments asserting guarantees, source-text assertions with no mutation, numbers changed without their consumers, and preferring a pure function to a source grep. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The registry is the right instrument, and it is missing a site of a class it already tracks — found by a live E2E run rather than by the sweep, which is the useful part. The gapTwo markers here sit next to the CLI installer: The first covers the sites that render a code as blank — if ($p.ExitCode -eq 0) {
Test-TraceblocCli
} else {
Warn "Couldn't install the tracebloc CLI automatically -- you can still connect with existing client credentials."
Hint "Install it later: irm $TRACEBLOC_CLI_INSTALL_URL | iex"
}The block above it does everything right: redirects both streams, caches What it cost, measured todayE2E run 33395912890, The redirected streams were captured and replayed and hold nothing further, so the exit code was the only surviving evidence of the cause. The install correctly continues — a failed CLI install is not fatal — leaving a machine with no Why the sweep could not see it, which is the part worth encoding
That is a marker-shaped fact rather than a review note: mutate by stripping The stronger version, if it is cheap hereEvery marker in this registry names a specific defect, which means the registry can only be as complete as the enumeration behind it. For this class the invariant is checkable directly: every branch that handles a non-zero Not asking to widen this PR. But the sixth site is the thing to design against, and a marker alone will not catch it. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…e-installer-guards-bite # Conflicts: # scripts/install-k8s.ps1 # scripts/manifest.sha256 # scripts/tests/install-k8s.Tests.ps1
|
Conflicts resolved — and the diff against The conflicts were entirely #917's content, which has since merged and then been improved on develop. So develop's side is authoritative and I took it verbatim rather than re-litigating my own earlier text — two of those improvements are corrections to me:
One marker went STALE in the merge, which is the harness earning its place on its first real test. The CLI-wait mutation aimed at Re-verified after the merge:
— resolved with Claude Code |
… did (Bugbot on #931) Bugbot: the registry tracked the handle-cache / empty-ExitCode class only on Wait-ProcessWithDeadline, and the CLI wait only as a missing deadline -- so Install-TraceblocCli's own `$null = $p.Handle` and parameterless `WaitForExit()` flush were source-text assertions with nothing behind them. Correct, and the hole is exactly where this class keeps recurring: Bugbot found the missing flush on #917, INSIDE the fix for the same class. Worse than reported. The existing fakes give WaitForExit a no-arg ScriptMethod returning NOTHING, so `if ($p.WaitForExit($cliWaitMs))` is falsy and every one of them takes the TIMEOUT branch. "reports success only when the installer exits 0" was passing through the kill path -- the right verdict for the wrong reason -- and the success branch, where the flush lives, was never executed by any test. So the new fake models the REAL contract: WaitForExit(ms) returns $true, and ExitCode stays $null until the PARAMETERLESS overload has drained the streams. Two directions, so neither can pass by accident: with the flush present the install reports success and the flush is asserted to have happened; with ExitCode never readable it must WARN rather than credit an install whose verdict it cannot read (the client#611 shape -- Step 4 warns, Step 5 falls back to the legacy credential prompt). Both lines now carry registered mutations, disambiguated by `After` anchors since neither is unique in the file. 13/13 mutations caught. 1027 Pester assertions pass on BOTH 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 67b01c2. Configure here.
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
The harness can't fail for 11 of its 13 entries, so the thesis doesn't hold yet.
The sandbox copies only scripts/ (mutation-check.ps1:226), but
install-k8s.Tests.ps1 reads six paths above it — ../../docker/k3s-cuda/* and
../../client/templates/resource-monitor-daemonset.yaml among them. Run that suite
unmutated in a scripts-only sandbox and it's FAIL=5, none of them a guard. The predicate at
:234 is if ($fails -gt 0) { $caught++ }, so every install-k8s.ps1 mutation clears it on
inherited failures alone. Your own CI log agrees: every install-k8s entry reports >=8
failing, both install.ps1 entries report 2 and 3. The two install.ps1 mutations are sound —
that suite's floor is 0.
Proved it end to end rather than inferring it: made the dashboard guard vacuous (its regex ->
a string that never occurs), reintroduced the hardcoded link, ran the sandbox. FAIL=5, not
one of the five is the dashboard guard, and the harness prints caught. The only test that
bites that mutation is dead and the run stays green at 13/13.
That is the same denominator defect this PR exists to close, which is why I'd rather fix it
here than ticket it. Bugbot's "Mutation sandbox inherits unrelated failures" was right, and
that thread got resolved without the Copy-Item changing.
Two fixes: copy the whole tree (or those six paths), and assert the claiming guard reddened
rather than any failure — each entry naming the test it expects to see fail. The second is
the one that matters; it's what makes the count mean something as the suite grows.
Smaller, same family:
Get-MarkerIndex:149skips the uniqueness check wheneverAfteris set (-not $After -and $hits.Count -gt 1), so an anchored marker with two matches silently takes the first — the
exact thing the header says it refuses to do. 3 of 13 entries use anchors, and the anchored
search runs to EOF, soAfteris a lower bound rather than a scope.& pwsh ... 2>$nulldiscards why a run produced no RESULT line. Combined with
$ErrorActionPreference='Stop'it killed the harness outright on my machine right after
markers resolving: 13/13— exit 1, 14 bytes of truncated red, no diagnostic. A count
without a cause, which is what you're fixing everywhere else.Import-Module Pester -MinimumVersion 5.5.0takes the newest installed, and Pester 6 is
API-incompatible. Worth pinning the major before PSGallery's latest moves.
One I couldn't settle: whether mutation-check is a required check. The protection API
404s on my token, so that's an absence rather than a pass — worth confirming, because an
unrequired mutation tier proves nothing about future regressions.
Structure, registry shape, the fresh-process-per-run fix and the four explicit refusals are
all the right instincts, and the two Windows defects are well chosen. It's attribution that
has to land before it can claim "every".
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
Still the same blocker — nothing pushed since my last pass, so this is the new evidence
rather than a second ask.
Reproduced the denominator hole independently. Scripts-only sandbox, unmutated:
install-k8s.Tests.ps1 is FAIL=7, install.Tests.ps1 is FAIL=0. Then the control/treatment
pair on the dashboard entry — guard intact: FAIL=8 and the guard is in the failure list, so
caught is honest there. Guard's regex pointed at a string that never occurs: FAIL=7, guard
absent from the list, harness still prints caught (7 failing). Same result, different floor.
Two of the smaller items got sharper, and one is worse than I filed it.
The anchored-marker one is live, not theoretical. if (-not (Test-CanPrompt)) { return "" } matches twice below the Read-RebootChoice anchor — 1761 and 5656, the Read-ClientName
twin. :149 skips the uniqueness check because After is set, so it takes 1761. That's the
right line today by ordering luck only. Drop that guard from Read-RebootChoice in a refactor
— i.e. regress backend#2675, the thing the entry exists to detect — and the marker doesn't go
STALE: the search runs to EOF, silently retargets Read-ClientName, some other test reddens,
and the harness prints caught. That directly falsifies the header's second stated refusal.
Pester: -MinimumVersion 5.5.0 resolved to 6.0.1 on my box and killed the harness at
markers resolving: 13/13 with one truncated InvalidOperation line and exit 1 — the
diagnostic-free red I described. Worth knowing the workflow has the same unpinned shape
(Install-Module Pester -MinimumVersion 5.5.0), and your own new BUGBOT.md rule 6 says 6.x is
what Windows CI runs.
One more, and it's not on you: Bugbot's "Mutation sandbox inherits unrelated failures" High
shows resolvedBy: cursor[bot] with no reply and no code change — :226 and :234 are
untouched. So it self-cleared on a later clean pass. I'm filing that against the resolve
behaviour rather than this PR, because a High that auto-resolves is a hole in the gate the
release train reads.
…ny failure (@saadqbal on #931) Asad was right, and the finding is the one this PR exists to prevent, inside this PR: the harness could not fail for 11 of its 13 entries. The sandbox copied only `scripts/`, but install-k8s.Tests.ps1 reads six paths above it (../../docker/k3s-cuda/*, ../../client/templates/resource-monitor-daemonset.yaml). Unmutated in a scripts-only sandbox that suite is FAIL=7 -- I reproduced it -- and the predicate was `if ($fails -gt 0) { $caught++ }`, so every install-k8s.ps1 entry cleared on inherited failures alone. My own output said so and I did not read it: install-k8s entries all reported >=8 failing while the two install.ps1 entries reported 2 and 3. Asad proved the end state -- dashboard guard made vacuous AND the link reintroduced, run still 13/13. Five fixes, his order: 1. THE SANDBOX IS THE WHOLE TREE (minus .git, which is 106MB of 122 and which no suite reads). Both baselines are now 0 in the sandbox, not just in the repo. 2. ATTRIBUTION, which he called the one that matters. Each entry names the guard it EXPECTS to fail, only that guard's failure counts, and a mutation that reddens the suite via some OTHER test is MISATTRIBUTED -- reported as a registry failure, as loud as SURVIVED, never a catch. An entry with no `Expect` is refused at marker resolution, because without one it could only be scored on a bare count. Every catch now prints WHICH test bit. It found a real one immediately: removing the CLI handle cache is a semantic NO-OP under the mocks (`$p.StartTime` on a pscustomobject returns $null silently), so no behavioural test could see it and the mutation was reddening the suite through two unrelated flaky tests. Added the source-level guard the Wait-ProcessWithDeadline site already has, for the same reason: handle reaping is .NET behaviour no fake can reproduce. 3. `After` IS A SCOPE, NOT A LOWER BOUND. Uniqueness is enforced with an anchor too (it was skipped entirely), and the search is bounded to a `Within` window -- otherwise `After 'function Read-RebootChoice {'` still saw Read-ClientName's byte-identical guard 3900 lines later. That stricter check caught this the moment it was added. 4. THE CHILD'S STDERR IS KEPT. `2>$null` discarded why a run produced no RESULT line, and under $ErrorActionPreference='Stop' it killed the harness on Asad's machine with fourteen bytes of red and no cause. 5. Pester pinned to the 5 major; `-MinimumVersion` took the newest installed and 6 is API-incompatible. Still open, and I could not settle it either: whether mutation-check is a REQUIRED check. The protection API 404s on my token too, so that is an absence rather than a pass -- needs someone with admin scope. 13/13 caught BY the claiming guard. 1028 Pester assertions pass on both 5.5.0 and 6.0.1. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@saadqbal — you were right, and the finding is the defect this PR exists to prevent, inside this PR. I reproduced it before touching anything: unmutated in a scripts-only sandbox that suite is FAIL=7 (you measured 5; my branch had added two more guards since). With My own output said so and I did not read it. Every install-k8s entry reported ≥8 failing while the two install.ps1 entries reported 2 and 3. I noticed the asymmetry and treated it as a property of the suites. All five fixed, in your order: 1. The sandbox is the whole tree (minus 2. Attribution — the one you said matters. Each entry names the guard it expects to fail; only that guard's failure counts. A mutation that reddens the suite via some other test is MISATTRIBUTED, reported as loudly as SURVIVED and never as a catch. An entry with no It found a real one on its first run. Removing the CLI handle cache is a semantic no-op under the mocks — 3. 4. The child's stderr is kept, with the last lines quoted when no RESULT appears. Sorry it killed your run for fourteen bytes of red — that is precisely the shape I was claiming to fix elsewhere. 5. Pester pinned to the 5 major. Your open question, which I also could not settle: the protection API 404s on my token too, so 13/13 caught by the claiming guard. 1028 Pester assertions on both 5.5.0 and 6.0.1; style clean. The related Bugbot finding about the bash twins is now #946, where @LukasWodka caught the same class one layer further down — I'd defined |
|
bugbot run |
…e-installer-guards-bite
aptracebloc
left a comment
aptracebloc
left a comment
There was a problem hiding this comment.
Re-reviewed at ff7d479. The denominator defect is fixed, and both ways you asked for:
- The sandbox now copies the whole tree (
mutation-check.ps1:282-284), soinstall-k8s.Tests.ps1's reads ofdocker/k3s-cuda/*andclient/templates/…resolve and that suite's baseline floor is 0 — the baseline-green gate now actually protects it. - Attribution is per-guard (
:307): each entry names the test it expects to redden, and a redden via any other test is MISATTRIBUTED and as loud as SURVIVED. I traced the dashboard case that was demonstrated — a vacuous guard can no longer be credited on inherited failures; it now reports NOT caught. CI'smutation-checkis green at 13/13 by the claiming guard against the full tree. - Anchored markers are windowed and uniqueness is enforced inside the window (
Get-MarkerIndex:156-178), soAfter 'function Read-RebootChoice {'can no longer retarget theRead-ClientNametwin.
Two things before this can claim "every guard bites":
mutation-checkis still not a required check ondevelop— I confirmed the protection set (Unit tests, Lint, quality/*, version-bump-gate, Source-of-truth drift, chart-version, Helm unit tests); it isn't in it. An unrequired mutation tier proves nothing about future regressions. Worth adding to branch protection in this PR or a fast follow.- The two open Bugbot Mediums are both real and both about the harness silently going quiet: a crashed child (
FailedCount=-1) is scored SURVIVED with its stderr dropped (:312/:244) — and on:307a crash whose diagnostic contains the Expect token could even be miscredited CAUGHT; and Pester is installed min-only in the workflow while the import pins<6(:222), so a 6.x from PSGallery kills the baseline with no RESULT line. Both are the "a count without a cause" failure this harness exists to stop — worth closing here.
Structure and the attribution rewrite are right; clearing @saadqbal's thread and the two above gets it to "every".
— drafted with Claude Code
saadqbal
left a comment
saadqbal
left a comment
There was a problem hiding this comment.
The blocker's gone, and I checked it the way I broke it: made the dashboard guard's regex vacuous,
applied that entry's mutation, and the run now says MISATTRIB, 0/1, exit 1 where it used to say
caught. Whole-tree copy and Expect attribution both do what they claim. The anchor fix is better
than what I asked for — dropping the guard from Read-RebootChoice now gives STALE: no line matches instead of silently retargeting Read-ClientName 3900 lines down, and a duplicate inside
the window is refused too. Proven both ways.
One thing left, and it's the same class: the sandbox floor isn't 0, it's 2 — on every platform,
by construction. You create the sandbox at mutation-check.ps1:268 under
[System.IO.Path]::GetTempPath(), and Get-ElevationCommand (install-k8s.ps1:81-82) branches on
$ScriptPath -notlike "$temp*" off that same call. So inside the sandbox the durable -File branch
is unreachable and these two are red before any mutation lands:
Get-ResumeCommand (#420).carries -File, forwarded switches, and -Resume for a durable script path
Get-ElevationCommand (#421).re-runs an on-disk script with a QUOTED -File path + forwards the switches
That's what the new .Handle test's comment calls "unrelated flaky tests" — it isn't flake, it's
deterministic, so you've already met this and mis-diagnosed it.
Two consequences. A mutation that genuinely survives reports MISATTRIB ("2 other failure(s)")
rather than SURVIVED, so the message sends you at the registry when the answer is "the guard
doesn't bite". And a false green is still constructible: register an entry whose mutation only
rewords a comment, with Expect = 'durable script path', and you get caught / 1/1 mutations caught BY THE GUARD THAT CLAIMS THEM / exit 0. Nothing in today's registry collides, so 13/13 is
real — but it's a live trap for the next entry, and it's the third time this class has come round on
this PR. Run the unmutated suite once in a sandbox and require FAIL=0 — which is what your header
at :34 already promises — or put the sandbox somewhere that isn't temp.
The stderr fix doesn't survive $ErrorActionPreference = 'Stop'. With Stop set at :48, the first
ErrorRecord out of the 2>&1 pipeline at :238 terminates the parent, so the diagnostic block at
:241 is unreachable in exactly the case it was written for — RESULT: in red, exit 1, no cause.
Same fourteen bytes as before. A local Continue around the child call fixes it.
Bugbot's Pester thread is right and cheap to take: the workflow installs with -MinimumVersion 5.5.0 and no ceiling while the child now imports with a 5.x one, so the job is load-bearing on the
runner image's preinstalled 5.x. Pin the install too.
And I can finally close the absence I've reported twice: mutation-check is not a required
check — isRequired(pullRequestNumber:931) returns false, as it does for Pester and bats. The
20 required contexts are Lint, Unit tests, Helm unit tests, Source-of-truth drift, the chart-version
pair, and the four quality/* legs. So a red mutation-check won't stop a merge today, which seems
worth a ticket given what the job is for — not yours to fix here.
Tiny: the by: line's -replace '^.*?\.' cuts at the first period, so a Describe containing one
comes out mangled — visible in your own CI log.
…Pester pins agree (Bugbot on #931) Two findings, both mine, both the same class this harness exists to close. 1. A CRASHED CHILD WAS SCORED "SURVIVED". Invoke-Suite returns FailedCount -1 when the child dies before printing RESULT, and -1 is not `-gt 0`, so it fell through to SURVIVED -- reporting "this guard does not bite" for a run that never reached the guard, and discarding the very stderr the previous commit added to capture it. Now its own third outcome, printed with the cause and exiting non-zero: a crash means the entry proved NOTHING either way, and scoring it as anything softer would be the harness lying again. Proved both halves rather than reading the branch. A child that throws before RESULT during the BASELINE now reports the actual exception text instead of a bare count. Injected a sandbox-only death (baseline left green) and the per-mutation path reports CRASHED with its cause, where it previously said SURVIVED. 2. THE INSTALL AND THE IMPORT PINS DISAGREED. The workflow installed Pester with only a minimum, so the gallery hands the runner 6.x, while every suite child imports with a 5.x ceiling -- the child then cannot load Pester, prints no RESULT, and the job dies at the baseline having proven nothing. @saadqbal measured exactly that on his machine, twice. Both now carry the same ceiling, and the comment says they move together or not at all. @saadqbal's earlier items were already in ff7d479 (whole-tree sandbox, per-entry attribution, `After` as a bounded scope, kept stderr) -- his latest pass predates that push. His independent reproduction matches mine exactly: scripts-only sandbox is FAIL=7 for install-k8s.Tests.ps1 and FAIL=0 for install.Tests.ps1, and the control/treatment pair on the dashboard entry showed `caught` printed with the guard absent from the failure list. Still open, and NOT fixable from here: whether mutation-check is a REQUIRED check. The protection API 404s on both our tokens. 13/13 caught by the claiming guard, no SURVIVED/MISATTRIBUTED/CRASHED. 1064 Pester assertions pass. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@saadqbal — your latest pass predates my push (07:38Z vs 10:04), so the four main items were already in ff7d479. Your independent reproduction matches mine exactly: scripts-only sandbox is FAIL=7 for On the anchored marker being live rather than theoretical — you were right and your worked example is the one that matters: drop the guard from One item of yours I had only half-fixed, and Bugbot caught the rest: I pinned the import ceiling and left the workflow's Bugbot also found a third instance of this PR's own defect class, which is worth stating plainly: a child that died before printing RESULT returned Your point 4 stands and is not mine to close: a High that self-resolves via And the one neither of us can settle: the protection API 404s on my token too, so State now: 13/13 caught by the claiming guard, no SURVIVED / MISATTRIBUTED / CRASHED, both baselines green in the sandbox (890 + 57), 1064 Pester assertions passing, style clean. |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 537d36d. Configure here.
aptracebloc
left a comment
aptracebloc
left a comment
There was a problem hiding this comment.
Re-reviewed at 537d36d2. The two Bugbot Mediums I flagged last pass are both resolved — the crashed-child scoring and the Pester min-install-vs-<6-import mismatch — and the PR is now green, conflict-free, and Bugbot-clean. Combined with the denominator fix (whole-tree sandbox + per-guard attribution) that already landed, my side is clear.
One item from my list is still open, and it's the only thing keeping the header's "every guard bites" from being fully earned: mutation-check is not a required status check on develop (I re-confirmed against branch protection — the required set is Unit tests, Lint, quality/*, version-bump-gate, Source-of-truth drift, chart-version bump, Helm unit tests). Until it's added, a future guard-death still goes green on the required gate. Worth wiring in this PR or a fast follow.
Down to @saadqbal's gate from here.
— drafted with Claude Code
|
@saadqbal — asking for a re-approve when you have a moment. Everything you raised is addressed, and two things landed after your last pass that you should see before deciding. What changed since 07:38Z (your pass predates ff7d479, so the four main items were already in flight when you wrote):
Current state:
Worth noting the CI run is the first evidence the harness works on the runner rather than only on my machine — the whole-tree sandbox copy was the part I'd have expected to behave differently there. One thing I have not closed, and I don't want to pretend otherwise: your question about whether Your call either way — I'd just rather you make it knowing the question is still open than assume I'd quietly resolved it. |
|
@aptracebloc — thank you for settling that; you closed the one question @saadqbal and I both hit a 404 on. Recording what I found trying to act on it, because it changes which of your two options is available. I can't wire it in this PR. Branch protection here is not managed as code — I checked The wire-it-in-this-PR option exists but has a real cost, so I'd rather put it to you than take it:
Folding the mutation tier into And the obvious escape from that cost is a trap I'd want to avoid on principle: a So my read: land this as-is and add the required context separately, because an unrequired tier that exists is strictly better than no tier, and making it required needs no re-review of this code. If you'd rather it be gated before merging, say so and I'll fold it into Either way I'll note the gap where it can't be lost: the header of |
…ptracebloc on #931) Arturo settled the question @saadqbal and I both 404'd on: `mutation-check` is NOT a required status check on `develop`. The required set is Unit tests, Lint, quality/*, version-bump-gate, Source-of-truth drift, chart-version bump, Helm unit tests. So a guard-death reddens THIS job and still passes the gate the merge button reads -- and the header claimed "every guard must still catch the bug it was written for" without that qualifier. A file overstating its own coverage is precisely the class this harness exists to catch, so it should not be the first offender. The header now says it plainly: read the claim as "verified by CI-as-run", not "by CI-as-enforced", until the context is added. Adding it is not possible from a PR. Protection is not managed as code here -- I checked `client` for a settings/ruleset file and `.github`'s `repo-inventory.yml`, which carries only visibility and release_train for this repo -- so the required set lives in the API/UI and needs admin scope. The alternative, folding the tier into the required `Unit tests` (standard-checks.yml:149), is left to Arturo's call rather than taken: it gates by construction but roughly triples a required check every PR pays, including ones touching no installer code. And the obvious escape from that cost -- a `paths:` filter on a required context -- is a known trap: a skipped required check never reports, which is registered as a mutation in e2e-test-agent because it has bitten before. Comment-only; 13/13 markers still resolve, style clean. Part of tracebloc/backend#2849. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4 participants
A fix is not fixed until a guard has been seen to fail
Both of this week's Windows defects were already fixed, and nothing in CI could say otherwise:
install.ps1— but only its message. Everyexitwas untouched, so the bootstrap kept closing the user's console for another month. It was reported from a real machine as "it just closed the PowerShell".install-k8s.ps1's own comment promisedWaitForExit()"guarantees ExitCode is populated for every caller". It did not — so every failure inInvoke-TrackedInstallrenderedexitedwith an empty slot, and a Docker install that succeeded with 3010 was filed as a failure.Both were found by a human running the installer. The structural reason nothing else could:
if (-not $env:TB_PESTER)and every suite setsTB_PESTER=1, so no test in this repo has ever executed anexit.What this adds
scripts/tests/mutation-check.ps1reintroduces each fixed defect into a copy of the tree and requires the claiming suite to go red.Four things it refuses to do
Each because it has gone wrong somewhere:
git addstages.STALE.Test-CanPromptis the real case:Read-RebootChoiceandRead-ClientNamecarry the same guard line, so anAfteranchor disambiguates rather than silently hitting whichever came first.Findmust be present and differ fromReplace, checked before any suite runs.One thing I got wrong, and what it changed
The first version ran every suite inside one long-lived pwsh process and reported two baseline failures that do not reproduce when the same file runs alone — Pester state survives between
Invoke-Pestercalls, so run N is not run 1. A harness whose own verdicts depend on how many times it has already run cannot say whether a guard bit. Each run is now a fresh process, which is also how CI runs it.It also now names a failing baseline instead of printing a count — the same diagnostic gap these tickets are about, in my own tool.
Also
installer-tests.yamlas its own job..cursor/BUGBOT.mdgains the six review rules this week produced: a fix in one twin only, guards behindTB_PESTER, comments asserting guarantees with no test, source-text assertions with no mutation, a number changed without its consumers, and preferring a pure function to a source grep.Part of tracebloc/backend#2849.
🤖 Generated with Claude Code
Note
Low Risk
Changes are limited to test harness, CI workflow, and review docs; installer scripts are not modified in this diff.
Overview
Installer fixes were repeatedly marked done while CI stayed green because many guards only grep source text or never hit paths behind
TB_PESTER. This PR adds mutation testing so each registered defect is reintroduced in a sandbox copy of the repo and the claiming Pester suite must fail on the named guard—not merely on any failure.scripts/tests/mutation-check.ps1maintains a registry of known bugs (reboot/credential prompts, exit-code formatting, bounded Docker/CLI waits, dashboard URL, bootstrapexit, ref guard, etc.), validates unique markers (withAfter/Withinanchors), requires a green baseline per suite in isolatedpwshchildren (Pester 5.5–5.99 pinned to match CI), then scores caught / survived / misattributed / crashed via each entry’sExpectstring.installer-tests.yamlgains amutation-checkjob that runs that script.install-k8s.Tests.ps1adds behavioral tests forInstall-TraceblocCli: parameterlessWaitForExit()stream flush soExitCodeis readable on success, a negative case when streams never drain, plus a source assertion that$p.Handleis cached before the timed wait..cursor/BUGBOT.mddocuments six review rules (twin installers,TB_PESTER, comment “guarantees”, source-only guards, constant consumer drift, pure functions over greps).Reviewed by Cursor Bugbot for commit 8178750. Bugbot is set up for automated code reviews on this repo. Configure here.