fix(installer): make the Windows CLI resolvable in a fresh shell — copy it into the machine-wide tools dir (backend#2915) - #937
Conversation
…resh shells (backend#2904) The tracebloc CLI's own installer PATH-adds its bin dir at USER scope only. A fresh, non-interactive shell that sources no profile -- the SSM session the Windows e2e opens for its cli-windows step, which need not even run as the installing user -- never sees that entry, so `tracebloc` is not on PATH there even though every other client tool (installed to %ProgramFiles%\tracebloc\bin on the MACHINE PATH) is. After a successful CLI install, persist the CLI's bin dir onto the MACHINE PATH through one dedup-correct helper (Add-DirToMachinePath), and route Initialize-ToolDir through the same helper -- replacing its substring dedup, which both false-matched a prefix dir and mis-parsed a '[' in a path, and could append a ';;' empty (== current-directory) PATH entry. RefreshPath mirrors the value into the running process so the verify step sees it immediately. Pester covers the append/dedup/idempotency logic and the install wiring on Linux CI (the .NET Machine-scope setter is a no-op off-Windows, so the registry is simulated via mockable Get/Set-MachinePath wrappers); the self-hosted Windows e2e exercises the real path via Initialize-ToolDir. Part of tracebloc/backend#2904 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e (backend#2904) The R8 signed-installer manifest pins a SHA256 over each sub-script the bootstrap verifies before running privileged steps; editing install-k8s.ps1 changes its digest, so `make drift` (gen-manifest.sh --check) fails until the manifest is regenerated and committed. Part of tracebloc/backend#2904 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The persist-failure test asserted only `Should -Not -Throw`, which the function-wide catch in Install-TraceblocCli already guarantees — so it passed even with the load-bearing inner Add-DirToMachinePath try/catch deleted (Bugbot). Assert the discriminating behavior instead: a persist throw must be CONTAINED so Test-TraceblocCli still runs and the CLI is reported installed, not bounced to the function-wide catch that misreports the successful CLI install as failed. Verified by mutation: removing the inner try/catch now fails this test. Part of tracebloc/backend#2904 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
…li-persistent-path # Conflicts: # scripts/manifest.sha256
saadqbal
left a comment
There was a problem hiding this comment.
The scope-matched read is right, which was the thing I was most worried about in this
shape — Get-MachinePath reads 'Machine' and that's what feeds Set-MachinePath;
$env:PATH never touches the write. Dedup is exact per-segment, case-insensitive and
trailing-sep tolerant, so a second install really is a no-op, and elevation is
fail-closed long before either caller. Killing the old -like "*$Dir*" is a good
cleanup on its own.
One thing I want changed before this lands: the dir you persist is
$TRACEBLOC_CLI_INSTALL_DIR = %LOCALAPPDATA%\Programs\tracebloc (:7497), a per-user
user-writable path, and :7609 puts it on the Machine PATH. Two problems. It doesn't
fix the case your own summary names — in the elevate-to-a-different-admin flow this
script supports, $env:LOCALAPPDATA is the admin's, so the daily user gets a Machine
PATH entry pointing into a profile it can't read and tracebloc is still unresolvable.
And a user-writable directory on the system search path lets a non-admin plant a
docker.exe/kubectl.exe that any other user's elevated process then resolves
unqualified — CWE-426, a bigger version of the ;;-as-cwd case you already guard
against 30 lines up.
Cheapest fix I can see: $TOOL_DIR (%ProgramFiles%\tracebloc\bin) is already on the
Machine PATH from :661 and is admin-only-writable, so put the exe there (copy or shim)
rather than adding LOCALAPPDATA as a new entry — that solves the multi-user case and
adds no new PATH entry at all. Durable fix is upstream in tracebloc/cli installing
machine-wide when it's already elevated; I don't see a ticket, worth filing either way.
Minor: Add-DirToMachinePath guards -not $Dir so '' returns early, but ' ' gets
through, and Test-DirOnPath trims it to empty and returns false — so a whitespace dir
appends a junk entry every run. Not reachable from today's callers, but the guard and
the write disagree about what "empty" means, which is the same shape as the bug you're
fixing. A .Trim() in the guard closes it.
No length check anywhere, and I don't think it matters: this is a registry write, not
setx, so there's no silent 1024/2047 truncation — it throws, and both sites handle it
(top-level trap for Initialize-ToolDir, the inner catch for the CLI leg). Worth a
separate ticket that SetEnvironmentVariable at Machine scope writes REG_SZ, so a
write bakes any %SystemRoot%-style entries and flips PATH off REG_EXPAND_SZ —
pre-existing, Initialize-ToolDir already did it, just noting it now runs on a second
path.
Tests are good — the persist-failure one asserting the verify still ran, rather than
Should -Not -Throw, is the right answer to Bugbot and actually discriminates.
Two notes, neither on you: the red Helm unit tests is infra, not this branch — run
33403966198 was cancelled with zero steps and superseded at the identical SHA by
33404006297, which passed, so a re-run clears it. And backend#2904 is already closed,
so this needs a different closing ref.
…APPDATA on the Machine PATH (backend#2915) Review (saadqbal): adding %LOCALAPPDATA%\Programs\tracebloc to the Machine PATH is wrong twice over. It's a per-user, user-writable dir on the system search path (CWE-426 — a non-admin plants an exe an elevated process resolves unqualified), and in the elevate-to-a-different-admin flow it's the admin's profile, unreadable by the daily user, so the CLI is still unresolvable — the very case this was meant to fix. Instead, on a successful CLI install, COPY the exe into $TOOL_DIR (%ProgramFiles%\tracebloc\bin) — admin-only and already on the Machine PATH from Initialize-ToolDir — via Publish-TraceblocCliToToolDir. Resolvable machine-wide for any user, and no new PATH entry. Not a shim: a tb.cmd-style shim would bake the admin's LOCALAPPDATA path and reintroduce the unreadable-profile problem. Also (review): Add-DirToMachinePath's guard now rejects whitespace ([string]::IsNullOrWhiteSpace), matching Test-DirOnPath's trim — a ' ' dir no longer slips through to append a junk PATH entry. Add-DirToMachinePath stays the shared Machine-PATH helper for Initialize-ToolDir (exact per-entry dedup, ;;-collapse). Manifest regenerated for the install-k8s.ps1 change. Closes tracebloc/backend#2915 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — the multi-user + CWE-426 point is exactly right, and your framing (already-Machine-PATH'd admin-only Main ask — done. New Minor — done. Closing ref — fixed. You're right that #2904 is closed (Lukas closed it once e2e-test-agent#359 split the verify fix out, with #2906 for the exit-code half). I filed backend#2915 for the PATH half this PR actually fixes and retitled/re-pointed the PR at it ( Follow-ups filed, per your notes:
|
|
bugbot run |
…verdict on the machine-wide copy (backend#2915)
Bugbot: Copy-Item/New-Item raise NON-terminating errors under the installer's
default $ErrorActionPreference='Continue', so the try/catch around
Publish-TraceblocCliToToolDir never caught a failed copy — and Test-TraceblocCli
could still print "ready" off the CLI installer's User-scope LOCALAPPDATA entry
even though $TOOL_DIR never received tracebloc.exe, leaving a fresh non-interactive
shell unable to resolve it.
* Publish-TraceblocCliToToolDir: -ErrorAction Stop on Copy-Item/New-Item + a
post-copy Test-Path, so a failure THROWS and is logged by the caller.
* Test-TraceblocCli: the "ready" verdict now requires the MACHINE-WIDE artifact
($TOOL_DIR\tracebloc.exe, on the Machine PATH), not just `Has tracebloc`. When
the CLI resolves only via the installing user's own User PATH, it says so
honestly ("installed for you, but not machine-wide") instead of a false ready.
Tests: publish now propagates a copy failure and throws on a missing artifact;
Test-TraceblocCli asserts the machine-wide gate and the honest user-only branch.
Manifest regenerated.
Part of tracebloc/backend#2915
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
…-running repairs it (backend#2915) Bugbot: Test-TraceblocCli now treats the machine-wide $TOOL_DIR\tracebloc.exe as the "ready" condition, but the fast nothing-to-do path still gated only on Test-TraceblocCliCurrent — which returns true for a User-PATH-only CLI. A machine that completed a prior install with the CLI on the USER PATH only (an older installer, or a copy that failed) would shortcut past Install-TraceblocCli and never run Publish-TraceblocCliToToolDir, so re-running the installer — the documented repair — left a fresh/other-user shell unable to resolve tracebloc. Add Test-TraceblocCliMachineWide (is $TOOL_DIR\tracebloc.exe present?) and require it in the fast-path gate alongside Test-TraceblocCliCurrent, so a completed-but-User-only machine falls through and the copy is placed. Wiring test + unit tests added; manifest regenerated. Part of tracebloc/backend#2915 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
…ow an update (backend#2915)
Bugbot: the $TOOL_DIR copy sits on the Machine PATH, searched BEFORE the CLI
installer's updatable %LOCALAPPDATA% copy. Once the snapshot was at/above the 0.10.0
floor the fast path never republished, so a later `irm <cli>/install.ps1 | iex` or a
CLI self-update refreshed only %LOCALAPPDATA% while `tracebloc` kept running the stale
machine snapshot.
* Publish-TraceblocCliToToolDir now SKIPS the copy when the machine copy already
matches the source (SHA256), and re-copies when it differs (or the compare fails).
Being a cheap no-op when in sync makes it safe to call on every run.
* The fast nothing-to-do path now calls Publish-TraceblocCliToToolDir among its other
idempotent "re-run is a real remedy" repairs, so an out-of-band update to
%LOCALAPPDATA% is picked up on the next installer run instead of being shadowed
indefinitely.
Tests: publish skips-in-sync / re-copies-when-stale / re-copies-on-compare-error; a
wiring assertion that the fast path calls Publish. Manifest regenerated.
Part of tracebloc/backend#2915
Co-Authored-By: Claude Opus 4.8 <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 0128951. Configure here.
LukasWodka
left a comment
There was a problem hiding this comment.
Re-read at 0128951842e6739f3c3a4eaa4939ebaa968157fb (the head moved twice during this pass — 49c368a8 → 68d2d75b → 0128951). Not approving: @saadqbal's CHANGES_REQUESTED still stands and is his to clear. On the code alone I'd approve with the two follow-ups below.
His blocking point is fixed the way he asked. Add-DirToMachinePath has exactly one call site — Add-DirToMachinePath -Dir $TOOL_DIR at :667 — so no user-writable directory reaches the Machine PATH and the net PATH change from this PR is zero. The whitespace guard is now IsNullOrWhiteSpace, and the closing ref points at the open backend#2915. All five cursor threads are resolved.
One note on CI: 26 checks pass and 3 skip, but Helm unit tests is cancelled, not passing. Nothing failing and nothing pending, so it doesn't change anything here — worth a re-run before merge so the record is clean.
Fix-the-class checks out. kubectl.exe, k3d.exe and helm.exe already land directly in $TOOL_DIR (Machine PATH), and docker comes from Docker Desktop, so the CLI genuinely was the only member of the class. Test-ToolsPresent covers exactly docker/kubectl/k3d/helm, which is why nothing caught it.
Two things, neither blocking.
1. The staleness refresh is direction-blind. Publish-TraceblocCliToToolDir copies whenever the hashes differ, with no version check, and the fast path now calls it every run. So if %LOCALAPPDATA% holds an older build than the machine copy — a pinned older CLI, or a partially-failed reinstall — the fast path copies the older one over the newer and downgrades the machine-wide CLI for every user on the box, silently. Proven by calling the real function: both "source newer" and "source older" invoke Copy-Item. Narrow, since install state is per-user under $env:USERPROFILE\.tracebloc so a second admin reinstalls rather than fast-pathing — but the blast radius is machine-wide. Suggest copying only when the destination is missing or the source is actually newer.
Related: the new test is named "re-copies when the machine copy is STALE (source differs — an out-of-band update)", a directional claim, but it only asserts Copy-Item fired when hashes differ. It passes identically for the downgrade case, so it can't discriminate what its name asserts.
2. The -ErrorAction Stop guard isn't covered by its own test. Deleting -ErrorAction Stop from Copy-Item leaves the suite fully green — 879 passed, 0 failed on Pester 5.7.1, anchor confirmed applied. The test that claims to cover it, "propagates a copy failure so the caller can log it (not silently swallowed)", mocks Copy-Item { throw } — and a mocked throw is terminating regardless of -ErrorAction, so it can't tell the two apart. The code is right today; the guard protecting it is inert, so a future removal regresses past CI.
Concretely it would reopen Bugbot's "publish failures stay invisible" in narrower form: with a stale destination already present, a non-terminating copy failure passes the post-copy Test-Path (the old file exists), nothing throws, the caller logs success, and the box serves a stale CLI machine-wide. Driving it with a real read-only destination, or asserting on the emitted error record, would make it bite.
Minor, for a ticket rather than this PR: the CLI exe is the only binary landing in $TOOL_DIR with no integrity check. kubectl, k3d and helm are each SHA256-gated before they land; the CLI's only gate is the child installer's exit code, and the hash compare added here is source-vs-destination freshness, not authenticity. I also grepped for icacls / Set-Acl / FileSystemAccessRule across scripts/ and found none — the admin-only permission on %ProgramFiles%\tracebloc\bin is inherited rather than asserted, which is correct on default Windows but nothing verifies it. Bounded overall — the source is the elevated process's own %LOCALAPPDATA%, so this is not a non-admin escalation — but it's a real asymmetry, and the durable fix probably belongs upstream in tracebloc/cli.
Verified locally on macOS (pwsh 7.5.2 / Pester 5.7.1): 879 passed, 0 failed, 15 skipped, plus six mutations with the anchor confirmed applied each time — the fast-path gate, the whitespace guard, the exact-vs-substring dedup, the machine-wide ready verdict and the fast-path refresh all redden correctly; only -ErrorAction Stop does not. Windows-only and therefore untested here: the real registry write, %ProgramFiles% ACL inheritance, UAC elevation, and the SSM fresh-shell case the ticket exists for. Pester (windows-latest) is green, but no job exercises a real elevated machine-wide install.
…the -ErrorAction Stop guard a biting test (backend#2915) @LukasWodka, two non-blocking review findings: 1. The staleness refresh was direction-blind — Publish-TraceblocCliToToolDir copied on ANY hash difference, so a %LOCALAPPDATA% holding an OLDER build (a pinned CLI or a partially-failed reinstall) would let the fast path silently DOWNGRADE the machine-wide CLI for every user. Now directional: new Get-TraceblocExeVersion, and the refresh copies only when the machine copy is MISSING or the source is a strictly NEWER version (identical/older/unknown leaves it in place). Tests discriminate the downgrade case (mutation-verified: a direction-blind mutant reddens). 2. The -ErrorAction Stop guard test was inert — it mocked Copy-Item { throw }, and a thrown error is terminating regardless of -ErrorAction, so deleting the flag left the suite green. The test now emits a NON-terminating error via Write-Error, so it only throws when -ErrorAction Stop promotes it (mutation-verified: removing the flag reddens). Manifest regenerated. Part of tracebloc/backend#2915 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thank you — both were real and both are fixed in 1468cd2, mutation-verified. 1. Direction-blind refresh → no-downgrade. New 2. Inert Notes, acted on:
Appreciate the mutation-driven read; it caught two guards that looked covered but weren't. |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
1 issue from previous review remains unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1468cd2. Configure here.
…on an unreadable version (backend#2915)
Bugbot: the no-downgrade guard from the previous commit was too broad. When the
machine-wide tracebloc.exe exists but won't report a version (corrupt / wrong-arch),
Publish refused to replace it — and since Test-TraceblocCliMachineWide only checks
presence and Test-TraceblocCliCurrent fails open on an unreadable version, the fast
path printed nothing-to-do and re-running never repaired the corrupt snapshot that
shadows every user's CLI.
The two "unknown version" sides are NOT symmetric:
* SOURCE version unreadable -> can't vouch for it -> keep the machine copy (a
partially-failed reinstall must not clobber a working CLI), but
* source READABLE, DEST version unreadable -> the machine copy is broken and the
source is known-good -> REPAIR it (copy).
Both readable still copies only toward a strictly newer version (no downgrade).
Tests split into the two asymmetric cases (source-unreadable keeps; dest-unreadable
repairs; both-unreadable keeps); mutation-verified the repair path bites. Manifest
regenerated.
Part of tracebloc/backend#2915
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
…-run as Administrator" (backend#2915) Bugbot (Low): the "installed for you, but not machine-wide" branch told the operator to re-run as Administrator. The installer has already self-elevated, so another run hits the same no-op — the machine-wide copy is absent because the copy FAILED (in the log) or a custom INSTALL_PREFIX put the CLI where Publish never sees it. Name the real causes and point at the log instead of a fix that can't work. Test asserts the hint no longer says "as Administrator" and names the real cause. Part of tracebloc/backend#2915 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
… the per-user `tb` (backend#2915) Bugbot: the machine-wide "ready" branch preferred `tb`, but `Has tb` is true only because RefreshPath pulled in the installing user's User PATH, where the CLI installer dropped its per-user `tb.cmd` shim. We copy only tracebloc.exe into $TOOL_DIR, so a fresh or other-user shell — exactly what this verdict is about — has `tracebloc` on the Machine PATH but no `tb`. Naming `tb` promises a command that won't resolve in the shell the message says works. Name the machine-wide command, `tracebloc`. Test asserts the ready verdict says run 'tracebloc' and never run 'tb'. Part of tracebloc/backend#2915 Co-Authored-By: Claude Opus 4.8 <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 c9e96a7. Configure here.
LukasWodka
left a comment
There was a problem hiding this comment.
Both of my non-blocking findings are fixed, and — the part that matters — the fixes are mutation-proof. I ran the mutations myself rather than taking the commit messages for it.
Still not approving: @saadqbal's CHANGES_REQUESTED from 15:02:41 stands, and that is his to clear.
Verified locally (pwsh 7.5.2 / Pester 6.0.1, macOS)
Reviewed at 46100a98; the head has since moved to c9e96a77. That matters less than usual here — the two new commits touch only Test-TraceblocCli and its tests, and I checked rather than assumed: Publish-TraceblocCliToToolDir and Get-TraceblocExeVersion are byte-identical at both revisions (function-body sha 35aab5718f57 and 27cbad84ef14 at each), so everything below still describes the current head.
BASELINE P=14 F=0
MUT 1 -ErrorAction Stop deleted from Copy-Item
anchor confirmed applied (44 -> 43 occurrences, line 7594)
P=13 F=1
[-] promotes a NON-terminating copy error to a throw (so -ErrorAction Stop bites)
MUT 2 no-downgrade guard deleted
`if ($destVer -and ($srcVer -le $destVer)) { return }` -> `if ($false) { }`
anchor confirmed applied (guard occurrences 1 -> 0)
P=12 F=2
[-] does NOT downgrade when the source is an OLDER version (differs but older)
[-] does NOT copy at equal version even when the build (hash) differs
RESTORED byte-identical to the pristine download P=14 F=0
On the guard test specifically — this was my finding, so it is the one I owed you a real check on. Swapping the mock from throw to Write-Error is exactly the right correction: a mocked throw is terminating regardless of -ErrorAction and therefore cannot discriminate, whereas a non-terminating Write-Error only becomes a throw because the flag is there. It now reddens on deletion. The comment you wrote above it explains precisely that distinction, which is the part that keeps the next person from "simplifying" it back.
On the no-downgrade gate — the directionality is right and, importantly, so is the typing. Get-TraceblocExeVersion returns [version]$Matches[1], so $srcVer -le $destVer is a real version comparison rather than a string one; I checked because a string compare here would make 10.x sort below 9.x and silently reintroduce the downgrade for exactly the releases where it matters most. It doesn't.
The asymmetric treatment of the two unknowns is the best part of this and I'd have accepted a cruder fix. Unreadable source → keep the machine copy, because a partially-failed reinstall must not clobber a working CLI. Unreadable dest → repair, because otherwise a corrupt snapshot shadows every user and re-running never fixes it. Those pull in opposite directions and both are right; the comment says why, and both are pinned by their own test.
The elevation hint
c9e96a77 fixes it, and Bugbot was right — I'd checked before you pushed. The installer self-elevates (Start-Process -Verb RunAs, and the admin check at :112), so by the time Test-TraceblocCli runs the process is already elevated and "re-run as Administrator" cannot be the fix.
Worth noting the shape, because it is the class this PR keeps running into: the code comment three lines above the hint already named the real causes — "the $TOOL_DIR copy didn't land, e.g. an INSTALL_PREFIX override or a copy that failed" — while the operator-facing string said something else. The correct diagnosis was sitting in the file, addressed to the wrong reader. Naming the log path is a genuine improvement on top.
The tb → tracebloc fix in the sibling commit is the same shape again, and subtler: Has "tb" is true in-process only because RefreshPath pulled in the installing user's User PATH, so the verdict about fresh and other-user shells was naming a command that doesn't exist in those shells. Good catch.
Where this stands
CI at c9e96a77: 39 pass, 3 skip, 0 failing, 0 pending. Zero unresolved review threads — all six Bugbot threads resolved. MERGEABLE.
The only thing left on the board is @saadqbal's change-request. For what it's worth, every point in it reads as addressed to me, including the blocking CWE-426 one: Add-DirToMachinePath still has exactly one call site (-Dir $TOOL_DIR, :667), so the net PATH change from this PR remains zero.
Still unverifiable from here, and worth stating plainly: everything Windows-specific — the real registry write, %ProgramFiles% ACL inheritance, UAC elevation, and the SSM fresh-shell case this ticket exists for. Pester (windows-latest) is green, but no job exercises a real elevated machine-wide install, so the thing the PR is actually for is still tested only by proxy.
One for a follow-up ticket rather than this PR: the CLI exe remains the only binary landing in $TOOL_DIR without an integrity check — kubectl, k3d and helm are each SHA256-gated, while the CLI's only gate is the child installer's exit code, and the hash compare here is freshness, not authenticity. Bounded (the source is the elevated process's own %LOCALAPPDATA%), but a real asymmetry, and the durable fix probably belongs upstream in tracebloc/cli.
saadqbal
left a comment
There was a problem hiding this comment.
This is the right fix, and it's removed rather than supplemented — which is what I wanted
to see. One Add-DirToMachinePath call site left in production (-Dir $TOOL_DIR at :667),
$TRACEBLOC_CLI_INSTALL_DIR never reaches a PATH write, so net new PATH entries from this
PR is zero and the CWE-426 surface is gone. The different-admin case works now for the
reason that matters: the copy runs from the admin's profile into ProgramFiles, which the
daily user can actually read.
You were right to pick copy over shim, and for a better reason than I gave — a tb.cmd
shim would bake the admin's %LOCALAPPDATA%, which reintroduces the unreadable-profile
problem I was complaining about. Only one arm of my "copy or shim" was actually valid.
The staleness risk I raised is handled in three layers (hash-compare, version-directional
so a pinned older CLI can't downgrade the box, and publish on the fast path too, so a
re-run genuinely repairs), and Machine-before-User resolution is confirmed. The residual
window is worth a ticket rather than a change: between an out-of-band CLI self-update and
the next installer run, tb (User PATH -> LOCALAPPDATA) and tracebloc (Machine PATH ->
ProgramFiles) can report different versions for the same user — two commands our docs
treat as synonyms. No downgrade is possible and a re-run fixes it, but the support thread
that starts with "tb works and tracebloc doesn't" is worth pre-empting.
Two more for tickets, neither blocking. Copy-Item -Force can't replace a running exe on
Windows, and that failure currently surfaces as a green verdict: an admin re-runs while
someone has a long ingest open, the copy throws a sharing violation, the caller correctly
logs and continues — and Test-TraceblocCli then finds the stale exe present and prints
tracebloc CLI ready. Copy to a temp name and Move-Item over the target; a rename can
replace a running image where a write cannot. Note this is precisely what a mocked
Copy-Item can't produce, and Publish-TraceblocCliToToolDir has never run on a real
Windows box in CI — E2E last-mile journey is skipping, and the body's "runs on the
self-hosted Windows e2e" claim is about Initialize-ToolDir, not the new copy.
And the durable upstream fix — tracebloc/cli installing machine-wide when already
elevated — is named in backend#2915 as a separate follow-up, but 2915 is the ticket this PR
closes and I couldn't find a ticket of its own. Same for Lukas' point that the CLI exe is
the only binary landing in $TOOL_DIR with no SHA256 gate, unlike kubectl/k3d/helm.
Trivial: the comment at :664 says "Same persist-to-Machine-PATH the CLI dir now uses
(backend#2904)" — the CLI dir doesn't use it any more, that's the v1 approach, and 2904 is
closed. It'll send the next reader hunting a second call site.
Tests bite. The destination assertion is a real -ParameterFilter on $Destination plus a
separate post-copy artifact check, not a constructed string, and breaking the copy's
destination reddens. Nice arc across the eight commits — each Bugbot finding got a fix and
a discriminating test, and the two hardest calls (version-directional refresh, asymmetric
handling of unreadable source vs destination) are both right and both pinned.
Ignore the aggregate rollup reading FAILURE — two superseded Helm unit tests runs at this
SHA, with a third that passed. Latest-per-context is green, so there's nothing to re-run.
|
Thanks for running the mutations yourself rather than trusting the commit messages — and for checking the [version] typing on the no-downgrade compare; you're right that a string compare there would have quietly reintroduced the downgrade for exactly the 10.x-over-9.x releases that matter most. Your "correct diagnosis addressed to the wrong reader" framing is the sharpest description of the class this PR kept hitting — the code comment already naming the real causes while the operator string said something else. I'll keep that in mind. The integrity-check follow-up (CLI exe is the only $TOOL_DIR binary without a SHA256/authenticity gate at copy time; inherited-not-asserted ACL) is captured on backend#2915 as scope for the durable upstream fix in tracebloc/cli — agreed it belongs there, not here. And fully agree the Windows-specific behaviour (real registry write, %ProgramFiles% ACL, UAC, the SSM fresh-shell case) is still only tested by proxy; that's tracked as the journey-verification gap on backend#2915 too. Leaving @saadqbal's change-request for him to clear. |
…ed (backend#2906)
Bugbot Medium: the gate derived from `.ExitCode` and `$LASTEXITCODE` only, so
branches on `.Code` were invisible. `.Code` is the HOUSE result shape --
Invoke-BoundedProcess and Invoke-DockerCli return @{ Code; Output }, documented at
install-k8s.ps1:2307-2308 -- so this was the commonest wrapper spelling, not an
edge case.
DERIVED, NOT COUNTED BY HAND. Bugbot named 2 sites. Widening the gate and letting
the walk report found 31 `.Code` gates in the file, 7 of them carrying a
user-facing Warn/Err. The finding was 2 of 7.
TWO OF THE 7 ARE DISJUNCTIONS AND ARE NOW EXCLUDED, for the same reason the ELSE
and the CATCH already are: the guard cannot know WHICH disjunct fired, so
demanding the code demands a possibly-false cause. `if ($res.Code -ne 0 -or $out
-match "FAIL " -or $unconfirmed.Count -gt 0)` fails on a non-code disjunct with a
code of 0, and "exit 0" beside a failure is the wrong-cause-reads-as-information
outcome this Describe exists to refuse. @saadqbal flagged this hazard
pre-emptively for the bool-collapsed sites; it arrived here first.
THE FIVE REAL ONES NOW NAME THE CODE. All five are wrapper calls where 124 IS the
timeout, so the code is precisely what separates the two causes the old text made
the operator guess between -- ":1534 nvidia-smi failed or timed out" being the
clearest case.
AND THE GUARD WAS UNSATISFIABLE FOR ONE ITERATION, which is the finding worth
reading. Widening the GATE alone left compliance matching `.ExitCode` only: all
five sites kept failing with the code sitting in the message text. A guard that
cannot be satisfied is worse than one that does not check -- it trains the reader
to edit the guard instead of the code. The two sides now widen in one place, named
as such.
Floor 9 -> 14, MEASURED by raising it until it failed and reading the number back,
not by counting additions by hand -- which is how a floor and a walk start
disagreeing.
Base merged (#937 had landed; conflict was scripts/manifest.sha256 alone) and the
manifest REGENERATED rather than resolved by picking a side, per @saadqbal.
4 mutations, all reddening: drop the code from a fixed message; narrow the gate
back; narrow compliance back (the unsatisfiable state); drop the disjunction
exclusion. Full Pester suite: 1061 passed, 0 failed. gen-manifest --check clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ard the class (backend#2906) (#938) * fix(installer): name the exit code when the CLI install fails (backend#2906) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): derive the exit-code gate spelling, and name the code on the k3d path (backend#2906) Bugbot, Medium: the AST sweep matched only the dotted `.ExitCode -eq/-ne 0` spelling, so the house idiom after a wait -- copy the code into a local, branch on the local -- was invisible to it. `$k3dExitCode = $k3dProc.ExitCode` … `if ($k3dExitCode -ne 0) { Err … }` is exactly that, and it reported a k3d failure with no code while this guard called the class closed. Restating one spelling is what went wrong in client#913; restating two is the same mistake with a longer list. The variable names are now DERIVED from assignments whose right side reads `.ExitCode`. Compliance follows ONE HOP through the call arguments, which is what separates the two sites this newly sees. The GPU branch names no code in its Warn text, but $GPU_SKIP_REASON is assigned in that branch from Get-GpuBuildFailureReason -ExitCode $buildExit, whose fallback returns "docker build exit $ExitCode" -- the classifier is deliberately preferred over a bare number and the comment there says so. The k3d branch has no such hop: the code is read, tested and dropped. Fixed with the house Format-ExitCode helper, matching the sibling site at :4086. Mutation-proved, anchors asserted both times: revert the k3d fix -> 1 of 2 reddens -ExitCode $buildExit -> -ExitCode 0 -> 1 of 2 reddens The second mutation initially did NOT redden: the token regex matched the bare word ExitCode, which the PARAMETER NAME satisfies with a constant value, so the one-hop was a blanket pass. Tightened to the property read and the gate variable. Not widened to $LASTEXITCODE here: 28 uses, and the widening surfaces 7 further offenders. That is a red gate on arrival, so it is filed separately rather than landed (CLAUDE.md rule 4 -- arm while green). Pester: 868 total, 853 passed, 0 failed. make drift: all 38 guards green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): the gate misses $LASTEXITCODE, the commonest spelling (backend#2906) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): a stale exit code is worse than none, so do not demand one (backend#2906) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): follow BOTH exit-code spellings, and the site that exposed (backend#2906) @saadqbal's change-request, and he was right that the floor passing at 8 is what made the hole self-concealing. THE DERIVATION FOLLOWED ONE SPELLING. `$a.Right.Extent.Text -match '\.ExitCode'` never matched `$createRc = $LASTEXITCODE`, so `$createRc` never became a gate token and `if ($createRc -ne 0)` at install-k8s.ps1:5787 was invisible to the walk. `$LASTEXITCODE` had been added as a DIRECT token, which catches `if ($LASTEXITCODE -ne 0)` but not the copy-into-a-local idiom -- the same shape round one flagged for `.ExitCode`, fixed for one spelling. Widened to `'\.ExitCode|\$LASTEXITCODE'`, floor 8 -> 9. AND WIDENING IT IMMEDIATELY NAMED A LIVE SITE, which is the point: line 5789 gated on [$createRc -ne 0]: Err "Couldn't provision the client. Re-run to retry.", but got 1. Exactly the case he described -- `Print-CreateFailure` receives `-OutFile` and `-Location`, never the code, so an operator got "The client couldn't be provisioned." followed by "Couldn't provision the client. Re-run to retry." with no code at all. Fixed with the house pattern: Err "Couldn't provision the client (tracebloc exited $(Format-ExitCode $createRc)). Re-run to retry." So the two-line change is a three-line change: the guard widening is what makes the product fix findable, and leaving the floor at 8 would have let both sit. Pester: 853 passed, 0 failed, 15 skipped. drift 38/38. Manifest regenerated (install-k8s.ps1 moved). A note on my own process, since it bit me twice here: two earlier edits in this pass were silently lost because a later anchor miss in the same script aborted before `write_text`. Both edits are now applied one at a time and each re-read from disk to confirm it landed -- the widening in particular reported "ok" once without ever being written, and the Pester run that followed was measuring the unmodified file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): the THIRD spelling, and a guard that could not be satisfied (backend#2906) Bugbot Medium: the gate derived from `.ExitCode` and `$LASTEXITCODE` only, so branches on `.Code` were invisible. `.Code` is the HOUSE result shape -- Invoke-BoundedProcess and Invoke-DockerCli return @{ Code; Output }, documented at install-k8s.ps1:2307-2308 -- so this was the commonest wrapper spelling, not an edge case. DERIVED, NOT COUNTED BY HAND. Bugbot named 2 sites. Widening the gate and letting the walk report found 31 `.Code` gates in the file, 7 of them carrying a user-facing Warn/Err. The finding was 2 of 7. TWO OF THE 7 ARE DISJUNCTIONS AND ARE NOW EXCLUDED, for the same reason the ELSE and the CATCH already are: the guard cannot know WHICH disjunct fired, so demanding the code demands a possibly-false cause. `if ($res.Code -ne 0 -or $out -match "FAIL " -or $unconfirmed.Count -gt 0)` fails on a non-code disjunct with a code of 0, and "exit 0" beside a failure is the wrong-cause-reads-as-information outcome this Describe exists to refuse. @saadqbal flagged this hazard pre-emptively for the bool-collapsed sites; it arrived here first. THE FIVE REAL ONES NOW NAME THE CODE. All five are wrapper calls where 124 IS the timeout, so the code is precisely what separates the two causes the old text made the operator guess between -- ":1534 nvidia-smi failed or timed out" being the clearest case. AND THE GUARD WAS UNSATISFIABLE FOR ONE ITERATION, which is the finding worth reading. Widening the GATE alone left compliance matching `.ExitCode` only: all five sites kept failing with the code sitting in the message text. A guard that cannot be satisfied is worse than one that does not check -- it trains the reader to edit the guard instead of the code. The two sides now widen in one place, named as such. Floor 9 -> 14, MEASURED by raising it until it failed and reading the number back, not by counting additions by hand -- which is how a floor and a walk start disagreeing. Base merged (#937 had landed; conflict was scripts/manifest.sha256 alone) and the manifest REGENERATED rather than resolved by picking a side, per @saadqbal. 4 mutations, all reddening: drop the code from a fixed message; narrow the gate back; narrow compliance back (the unsatisfiable state); drop the disjunction exclusion. Full Pester suite: 1061 passed, 0 failed. gen-manifest --check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
/fr-pass Functional review on staging — passed, with direct evidence. Journey (tier A), install via the real installer → client components healthy → CLI installed from its signed release and signed in → dataset ingested for every task type → use case published → model trained and the leaderboard read. The train leg, which is the one that matters: This repo's change is on the path that run exercised, so this is functional evidence rather than an inference from code review. Two things stated rather than glossed:
|

Summary
After a fresh install, the
traceblocCLI was not on PATH in a fresh, non-interactive shell — the SSM session the Windows e2e opens forcli-windows. The CLI's own installer (tracebloc/cli) drops the binary in%LOCALAPPDATA%\Programs\traceblocand PATH-adds it at User scope only; a profile-less shell (which need not even run as the installing user) never sees it. Every other client tool lives in%ProgramFiles%\tracebloc\binon the Machine PATH and resolves fine — the CLI was the lone exception.What changed
Publish-TraceblocCliToToolDir— on a successful CLI install, copy the exe into the admin-only$TOOL_DIR(%ProgramFiles%\tracebloc\bin), whichInitialize-ToolDiralready put on the Machine PATH. Resolvable machine-wide for any user, and no new PATH entry. Best-effort / non-fatal.tb.cmd-style shim would bake the admin's%LOCALAPPDATA%path and reintroduce the unreadable-profile problem in the elevate-to-a-different-admin flow.%LOCALAPPDATA%to the Machine PATH: a user-writable dir on the system search path is CWE-426 (a non-admin plants an exe an elevated process resolves unqualified).Add-DirToMachinePath(shared helper, used byInitialize-ToolDir) — replaces the old inline substring dedup (-like "*$Dir*") with exact per-entry matching (Test-DirOnPath), collapses a trailing;before appending (no;;-as-cwd entry), and rejects whitespace dirs.Get-/Set-MachinePathwrappers make the append/dedup logic unit-testable off-Windows.Type of change
Test plan
Invoke-Pester scripts/tests/→ 1043 passed, 0 failed, 15 skipped. New/updated:Publish-TraceblocCliToToolDir(copy / create-dir / no-op paths),Add-DirToMachinePath(append/dedup/idempotency/;;-collapse/empty-and-whitespace),Test-DirOnPath(exact-vs-substring),Initialize-ToolDirwiring, andInstall-TraceblocClipublish-on-success / no-publish-on-failure / non-fatal-on-throw (the last proven to bite via mutation).Invoke-ScriptAnalyzer -Severity Error→ 0 (required Lint gate).Initialize-ToolDir+$TOOL_DIR.Deployment notes
Installer scripts only (no chart change) — the chart version-bump gate does not apply.
scripts/manifest.sha256regenerated for theinstall-k8s.ps1change (R8 signed-installer digest pin).Closes tracebloc/backend#2915
Note
Medium Risk
Changes Machine PATH handling and elevated installer behavior (security-sensitive PATH/CWE-426 concerns), though logic is defensive with extensive tests.
Overview
Fixes fresh / non-interactive shells (and other users) not finding
traceblocafter install: the upstream CLI installer only adds User PATH under%LOCALAPPDATA%, while client tools already live on the Machine PATH in%ProgramFiles%\tracebloc\bin.On successful CLI install—and on the fast path when already healthy—the installer copies
tracebloc.exeinto that admin-only tools dir viaPublish-TraceblocCliToToolDir(no shim, no user-writable dir on Machine PATH). Refresh is hash-no-op when identical and version-directional so a stale%LOCALAPPDATA%copy cannot downgrade the machine-wide binary.Initialize-ToolDirnow persists PATH throughAdd-DirToMachinePath/Test-DirOnPath(exact entry dedup, no;;cwd injection, whitespace guards) instead of substring-likematching. The fast-path gate addsTest-TraceblocCliMachineWide;Test-TraceblocCliproves machine-wide readiness and warns honestly when only User PATH works. Pester coverage andscripts/manifest.sha256updated accordingly.Reviewed by Cursor Bugbot for commit c9e96a7. Bugbot is set up for automated code reviews on this repo. Configure here.