Skip to content

feat(telemetry): the Windows installer emits an outcome event (backend#2268) - #782

Merged
LukasWodka merged 6 commits into
developfrom
feat/2268-windows-installer-telemetry
Aug 21, 2026
Merged

feat(telemetry): the Windows installer emits an outcome event (backend#2268)#782
LukasWodka merged 6 commits into
developfrom
feat/2268-windows-installer-telemetry

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Closes tracebloc/backend#2268. Parent epic: tracebloc/backend#1872.

The gap

scripts/install-k8s.ps1 was 6,646 lines containing zero telemetry — no outcome event, no spool, nothing for any transport to carry. Not "the transport is missing": the emitter was missing, while backend#1907 sat closed titled "CLI and installer telemetry" and RFC-BACKEND-1872 listed client installer as a single row. Every statement of the form "the installer now emits" was true of one platform and false of the other.

scripts/lib/telemetry.ps1 is a port of telemetry.sh's contract, not of its lines: same closed vocabularies, resource/record split, attribute discipline, --help latch, declared-exit-2 handoff, "a signal on a skipped run is still skipped", and the never-create-HOST_DATA_DIR rule. Its comments aren't re-argued — they're implemented, with pointers to the twin. What is documented is every place the platform forced a real difference, since that's what a reviewer can't check against the other file.

Wired, because an emitter nothing calls is this epic's dominant defect class. The finally at install-k8s.ps1:6638 already described itself as mirroring bash's install_cleanup, so that's the emit point. The status comes from the classifying sites (PowerShell gives finally no access to an exit's code), and the interrupted case is derived from $script:OutcomeReported — the installer's own existing Ctrl-C signal — rather than a second mechanism that could disagree with it.

Four real bugs, all found by running the tests

Worth reading as a list of traps the next PowerShell port will hit.

1. Set-StrictMode -Version Latest is not file-scoped. Dot-sourcing applied it to the caller, imposing it on all 6,600 lines of install-k8s.ps1 — code written without it, where reading an absent property returns $null. 560 sibling assertions failed with "The property 'override' cannot be found", nowhere near this file. In the field it would have surfaced as the installer dying somewhere unrelated to telemetry: the observer breaking the thing it observes, which is the one outcome this feature must never have.

2. , binds tighter than +.@( 'a:' + $x + '"', 'b:' + $y + '"' ) is not a two-element array — the , binds to the adjacent string operands, + gets an array, and the whole thing collapses into one string joined by $OFS. The record rendered as {"resource":{"service.name":"installer" "tracebloc... — not JSON, entirely plausible in a log, and every regex assertion passed. Only the ConvertFrom-Json round-trip caught it.

3. -match is case-insensitive. The key regex [a-z][a-z0-9_]* accepted A.b, so the closed key vocabulary wasn't closed. Every shape test is -cmatch now, which is what [[ =~ ]] does on the bash side.

4. A Windows path opens with a colon. The bash twin takes ${1%%:*} for the source file because a POSIX path has no colon; C:\...\install-k8s.ps1:12 made that return C, which is in no vocabulary — so the location was dropped on every real Windows failure.

Two more worth flagging: the shape regexes anchor \A..\z, not ^..$, because .NET's $ matches before a trailing newline even without Multiline — the same line-vs-string hole telemetry.sh left grep over, arrived at from the other direction. And Get-Command -Name 'Log' resolved to /usr/bin/log, the macOS system logger, which the emitter then shelled out to; every lookup is -CommandType Function.

Delivery

A hash-pinned fetched sub-script like every other, added to install.ps1's $Files and gen-manifest.sh's WINDOWS_FILES (which check against each other).

⚠️It is the first scripts/lib/ entry on the Windows side, and Invoke-WebRequest -OutFile does not create directories — so the bootstrap now creates the parent first, as install.sh has always done with mkdir -p. Without that, the Windows bootstrap would die on its first fetch. No integrity property changes: every file is still verified against the signed manifest.

The duplication is guarded

A .ps1 cannot read bash declarations at runtime, so the closed sets exist twice — the restated-not-derived shape that goes stale silently. telemetry-vocabulary-agreement.sh now parses both twins and compares five vocabularies, holding no list of its own. Not the source vocabulary: bash names its eighteen files and ps1 its three, and those should differ.

Writing that parser surfaced a third failure mode worth naming: the one-line = @('A','B') form made the range scan run away and return every quoted string in the file, so a non-empty wrong answer sailed through the emptiness check. Bounded now.

Tests

82 new (75 emitter + 7 wiring). 848 passing, 0 failed against 766 on develop.

MutationResult
emit call deleted from the finally1 failed
run-started latch deleted1 failed
latch moved afterConfirm-Config1 failed ← order, not presence
an exit-2 handoff declaration dropped1 failed
a Step loses its phase letter1 failed
failure status no longer recorded1 failed — see below
lib dropped from the bootstrap's $Files1 failed
a value added to one twin's vocabularyguard exit 1
a value removed from one twin's vocabularyguard exit 1
the ps1 twin deletedguard exit 2 (fails closed)
a declaration reformatted past the parserguard exit 2 (fails closed)

The sixth row survived at first, and mutation testing is the only reason it doesn't still: the assertion was '$script:TbExitCode = 1', which is a prefix of = 130, so the interrupted line satisfied it and deleting the failure line changed nothing. It asserts = 1; now. A failed install would have reported exit_code: 0 — the one wrong answer that looks entirely fine.

Two existing source-text guards in install-k8s.Tests.ps1 asserted lines this change edits. Both updated to the new text rather than loosened, and both now also assert the telemetry half, so the wiring can't be removed silently.

Not in scope

Delivery. The host transport is backend#2217, and #1906's Collector is a pod that can reach neither of these files. This produces the records.

The RFC correction is rfcs#38 — a separate repo, so necessarily a separate PR.

Test plan

  • Invoke-Pester scripts/tests/ — 848 passed, 0 failed, 13 skipped (develop baseline: 766/0/13)
  • make check — parse, shellcheck -S warning, 8 drift guards, helm-lint, helm-vocab
  • bats gen-manifest install-bootstrap installer-parity check-style — 72 passed (check-style scans .ps1)
  • scripts/gen-manifest.sh --check — manifest current
  • telemetry-vocabulary-agreement.sh — 5 cross-twin vocabularies agree
  • Mutation matrix above; every anchor asserted applied
  • Windows CI (installer-tests.yaml) — the suites run on real Windows there as well as Linux pwsh

🤖 Generated with Claude Code


Note

Medium Risk
Touches the Windows bootstrap integrity surface (new signed fetch, parent-dir creation) and installer exit/finally paths, but telemetry is fail-soft and must not fail an install.

Overview
The Windows installer now emits the same closed-vocabulary outcome event as bash (install.run.succeeded|failed|cancelled|skipped), closing the gap where install-k8s.ps1 had no telemetry at all.

Adds scripts/lib/telemetry.ps1 as a fail-soft, hash-pinned twin of telemetry.sh. install-k8s.ps1 sources it optionally, tracks shared phase letters af, records exit status for finally (PowerShell cannot read exit codes), and emits once from that closer. Failures get error.type plus a basename/line from Err/Show-FatalError; help/diagnose stay unlatched; healthy re-runs are skipped; declared reboot exit 2 is cancelled.

install.ps1 fetches the new lib, creates parent dirs for nested $Files entries, and exports TRACEBLOC_INSTALL_REF so service.version is not permanently unknown. Vocabularies are compared across twins; Pester covers the emitter, privacy boundary, and wiring.

Reviewed by Cursor Bugbot for commit 25d540e. Bugbot is set up for automated code reviews on this repo. Configure here.

…d#2268)
`scripts/install-k8s.ps1` was 6,646 lines containing zero telemetry — no outcome
event, no spool, nothing for any transport to carry. Not "the transport is
missing": the EMITTER was missing, while backend#1907 sat closed titled "CLI and
installer telemetry" and RFC-BACKEND-1872 listed `client installer` as a single
row. Every statement of the form "the installer now emits" was true of one
platform and false of the other.
A PORT OF telemetry.sh's CONTRACT, not of its lines. Same closed vocabularies,
same resource/record split, same attribute discipline, same `--help` latch, same
declared-exit-2 handoff, same "a signal on a skipped run is still skipped", same
never-create-HOST_DATA_DIR rule. Its comments are not re-argued here; they are
implemented, with pointers to the twin. What IS documented is every place the
platform forced a real difference, because that is what a reviewer cannot check
against the other file.
WIRED, because an emitter nothing calls is this epic's dominant defect class. The
`finally` at install-k8s.ps1:6638 already described itself as mirroring bash's
`install_cleanup`, so it is the emit point. The exit status comes from the
classifying sites (PowerShell gives `finally` no access to an exit's code), and
the interrupted case is DERIVED from `$script:OutcomeReported` — the installer's
own existing Ctrl-C signal — rather than from a second mechanism that could
disagree with it.
FOUR REAL BUGS, ALL FOUND BY RUNNING THE TESTS RATHER THAN BY READING THE CODE.
Recording them because each is a trap the next PowerShell port will hit:
1. `Set-StrictMode -Version Latest` is NOT file-scoped. Dot-sourcing applied it
to the CALLER, imposing it on all 6,600 lines of install-k8s.ps1 — code
written without it, where reading an absent property returns $null. 560
sibling assertions failed with "The property 'override' cannot be found",
nowhere near this file. In the field it would have surfaced as the installer
dying somewhere unrelated to telemetry: the observer breaking the thing it
observes, the one outcome this feature must never have. Removed.
2. `,` BINDS TIGHTER THAN `+`. `@( 'a:' + $x + '"', 'b:' + $y + '"' )` is not a
two-element array — the `,` binds to the adjacent string operands, `+` gets
an array, and the whole thing collapses to ONE string joined by $OFS. The
record rendered as `{"resource":{"service.name":"installer" "tracebloc...` —
not JSON, entirely plausible in a log, and every regex assertion passed. Only
the ConvertFrom-Json round-trip caught it. Each element is parenthesised now,
and the field count is checked before the record is built.
3. `-match` IS CASE-INSENSITIVE. The key regex `[a-z][a-z0-9_]*` accepted
`A.b`, so the closed key vocabulary was not closed. Every shape test is
`-cmatch` now, which is what `[[ =~ ]]` does on the bash side.
4. A WINDOWS PATH OPENS WITH A COLON. The bash twin takes `${1%%:*}` for the
source file because a POSIX path has no colon; `C:\...\install-k8s.ps1:12`
made that return `C`, in no vocabulary, so the location was dropped on every
real Windows failure. Splits on the LAST colon now.
Also: the shape regexes anchor \A..\z, not ^..$. telemetry.sh moved off `grep`
because grep matches a LINE; .NET reintroduces that hole in a subtler spelling —
`$` matches before a trailing newline even without Multiline, so "abc\n" passes
'^[a-z]+$'. Tested directly, so a tidy-up back to ^..$ reddens.
And `Get-Command -Name 'Log'` resolved to /usr/bin/log, the macOS system logger,
which the emitter then shelled out to. Every lookup is -CommandType Function.
DELIVERY. A hash-pinned fetched sub-script like every other, added to install.ps1's
$Files and gen-manifest.sh's WINDOWS_FILES (which the two check against each
other). It is the first `scripts/lib/` entry on the Windows side, and
Invoke-WebRequest -OutFile does not create directories — so the bootstrap now
creates the parent first, as install.sh has always done with `mkdir -p`. Without
that the Windows bootstrap would die on its first fetch. No integrity property
changes: every file is still verified against the signed manifest.
THE DUPLICATION IS GUARDED. A ps1 cannot read bash declarations at runtime, so the
closed sets exist twice — the restated-not-derived shape that goes stale silently.
telemetry-vocabulary-agreement.sh now parses BOTH twins and compares five
vocabularies, holding no list of its own. Not the source vocabulary: bash names its
eighteen files and ps1 its three, and those SHOULD differ. Writing that parser
surfaced a third failure mode worth naming — the one-line `= @('A','B')` form made
the range scan run away and return every quoted string in the file, so a
non-empty WRONG answer passed the emptiness check. Bounded now.
TESTS. 82 new (75 emitter + 7 wiring), 848 total passing against 766 on develop.
Mutation-proven, anchor asserted applied every time:
emit call deleted from the finally 1 failed
run-started latch deleted 1 failed
latch moved AFTER Confirm-Config 1 failed <- order, not presence
an exit-2 handoff declaration dropped 1 failed
a Step loses its phase letter 1 failed
failure status no longer recorded 1 failed (see below)
lib dropped from the bootstrap's $Files 1 failed
a value added to one twin's vocabulary guard exit 1
a value removed from one twin's vocabulary guard exit 1
the ps1 twin deleted guard exit 2 (fails closed)
a declaration reformatted past the parser guard exit 2 (fails closed)
The sixth row SURVIVED at first, and mutation testing is the only reason it does
not still: the assertion was `'$script:TbExitCode = 1'`, which is a PREFIX of
`= 130`, so the interrupted line satisfied it and deleting the failure line
changed nothing. It asserts `= 1;` now. A failed install would have reported
exit_code 0 — the one wrong answer that looks entirely fine.
Two existing source-text guards in install-k8s.Tests.ps1 asserted lines this
change edits. Both updated to the new text rather than loosened, and both now
also assert the telemetry half, so the wiring cannot be removed silently.
NOT IN SCOPE. Delivery — the host transport is backend#2217, and #1906's Collector
is a pod that can reach neither of these files. This produces the records.
831 lines of it are the emitter and its tests; the installer diff is 60.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment threadscripts/install-k8s.ps1
Comment threadscripts/install-k8s.ps1
Comment threadscripts/install-k8s.ps1
Comment threadscripts/install-k8s.ps1
Comment threadscripts/lib/telemetry.ps1

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big, careful port — the "contract not lines" framing is right, and the four bugs-found-by-running writeup (, binding tighter than +, -match case-insensitivity, the colon-in-Windows-path source split, Set-StrictMode leaking to the caller) is exactly the kind of thing that makes the next PowerShell port cheaper. But the emitter has a set of wiring defects that defeat its own purpose, and I confirmed the two High ones against the code. Requesting changes so they're fixed as one class rather than one line at a time.

Bugbot #2Err leaves the exit code at 0 (High, confirmed).Err (install-k8s.ps1:148) sets $script:OutcomeReported = $true but never sets $script:TbExitCode before exit 1. finally can't read the exit code, so Send-TelemetryOutcome -Code $script:TbExitCode gets 0 and records install.run.succeeded for the installer's primary failure helper. The last-resort trap { Show-FatalError $_; exit 1 } at :6456 has the identical hole. Only Test-InstallSucceeded (:6691) and the catch (:6697) set the code — every failure routed through Err or the trap reports success. Fix: set $script:TbExitCode = 1 in Err and in the trap before exit 1 (the same way :6691/:6697 already do).

Bugbot #1 — the run-started latch fires before Help/Diagnose (High, confirmed).Set-TelemetryRunStarted runs at :6468, before the if ($Help) / if ($Diagnose) dispatch at :6471:6472. So a -Help or -Diagnose run latches as started, reaches finally with code 0, and emits install.run.succeeded for a run that never touched the machine — the client#747 failure the latch comment says it exists to prevent. And telemetry.Tests.ps1:405 asserts $latch -lt $help, so the test locks in the inverted order rather than catching it. Fix: move Set-TelemetryRunStarted after the Help/Diagnose terminal dispatch (or gate the emit on a latch those paths don't set), and flip the test to assert the latch comes after help.

The three Medium threads are the same shape and worth fixing in the same pass — the twin defines a latch/phase the Windows side never wires:

  • #3 fast path exits 0 after the run-started latch without Set-TelemetryRunSkipped, so "nothing to do" maps to succeeded not skipped.
  • #4Wait-ForClientReady has no Start-TelemetryPhase -Letter 'f', so connect time folds into helm and connect failures misclassify as helm_install_failed.
  • #5Add-Content/Set-Content -Encoding utf8 on PS 5.1 writes a UTF-8 BOM, so the first JSONL record starts EF BB BF and isn't valid JSON to a byte consumer — the install log already dodges this with UTF8Encoding($false); the spool should too.

Common thread: the emitter is present and the vocabularies are faithfully ported, but several outcomes reach finally with a status that doesn't match what actually happened — succeeded-on-failure, succeeded-on-help, succeeded-on-skip. For a feature whose whole job is to report the truth of a run, those are the ones that matter most. Also note CI isn't green yet (Prereqs — almalinux:9 pending) — I'll re-review once the threads are closed and it lands green.

…tatus (backend#2268)
@saqlainsyed007's review of #782, and every one of the five is real. They share a
shape worth naming: the emitter was present and the vocabularies faithfully
ported, but the STATUS arriving at it did not match what happened — succeeded on
failure, succeeded on help, succeeded on skip. For a feature whose only job is to
report the truth of a run, that is the worst class of defect it can have, and it
is invisible from every angle except reading the record.
1. ERR REPORTED SUCCESS FOR EVERY FAILURE IT HANDLED (High). `Err` is the
installer's primary failure helper — it sets `$script:OutcomeReported` and
exits 1, and never touched `$script:TbExitCode`. `finally` cannot read an
exit's code, so the emitter got 0 and wrote `install.run.succeeded`. Only
`Test-InstallSucceeded` and the `catch` set the status; everything routed
through `Err` lied. The last-resort `trap` had the identical hole, so a
terminating error outside the try lied too. Both set it now.
2. THE LATCH FIRED BEFORE THE TERMINAL FLAGS (High), so `-Help` and `-Diagnose`
latched as started, reached the `finally` with code 0, and emitted
`install.run.succeeded` for a run that never touched the machine. That is
exactly the client#747 bug, and the comment beside the latch said it was there
to prevent it while the code did the opposite. The wiring test asserted
`$latch -lt $help` and so PINNED the inversion rather than catching it — a test
agreeing with the code instead of with the requirement, which is the class this
epic keeps finding, appearing in the change that cites it. Both bounds are
asserted now (after the dispatch, before Confirm-Config) so neither can drift.
3. THE FAST PATH MAPPED "NOTHING TO DO" TO SUCCEEDED. It exits 0 after the latch
without `Set-TelemetryRunSkipped`, so re-runs on an already-healthy machine
inflated the success count. `skipped` is a registered verb and the bash twin
already reports it from assess.sh's gate — so the two platforms were answering
the same question differently.
4. THE CONNECT PHASE DID NOT EXIST. `Wait-ForClientReady` had no
`Start-TelemetryPhase -Letter 'f'`, so the readiness wait was timed inside
`helm` and a client that never became Ready classified as
`helm_install_failed` when helm had in fact succeeded — a FABRICATED helm
failure in the exact rate this feature exists to produce. My Step-letters test
asserted a..e and read as complete; it now says explicitly that `f` comes from
the readiness gate, not a numbered step, and asserts it separately.
5. THE SPOOL BEGAN WITH A UTF-8 BOM. `Add-Content`/`Set-Content -Encoding utf8`
writes one on PowerShell 5.1 — the PowerShell a stock Windows install has — so
the first JSONL record started EF BB BF and no byte consumer could parse it.
The install log already avoids this with `UTF8Encoding($false)`; the spool uses
the same idiom through two writers rather than three call sites. Asserted on
the BYTES, because every string-level read strips a BOM invisibly and would
pass either way.
Mutation-proven, each fix reverted individually:
Err stops recording the status 1 failed
the trap stops recording it 1 failed
latch moved back before the flags 1 failed <- the shipped bug
fast path stops marking skipped 1 failed
connect phase removed 1 failed
BOM reintroduced (UTF8Encoding($true)) 2 failed
No survivors; green on restore.
One existing guard asserted the old `trap` line; updated to the full new line so
neither half can be dropped, not loosened.
853 Pester passing (0 failed), manifest regenerated for both changed ps1 files,
make check green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

All five fixed in dc8de73, each replied to on its thread. Your framing was the useful part — they are one class, and fixing them one at a time would have missed that.

The common thread, restated because it's the thing to watch for in the next emitter: the mechanism was present and the vocabularies faithfully ported, but the status arriving at the emitter didn't match what happened. Succeeded-on-failure, succeeded-on-help, succeeded-on-skip. All three are invisible from every angle except reading the emitted record — the installer behaves correctly, the tests pass, the log looks fine, and the metric is wrong in the direction that flatters us.

Two things I'd single out as my errors rather than oversights:

  • The latch comment was right and the code was wrong. It said "set after the terminal flags have had their chance to dispatch" while sitting above if ($Help). I wrote the reasoning and then implemented the opposite, and never read the two against each other.
  • My own test pinned the inversion ($latch -lt $help). A test agreeing with the code instead of the requirement — the exact class this epic exists to hunt, inside the change that cites it. Both bounds are asserted now.

Same shape on the connect phase: the Step-letters test asserted ae and read as "the phase vocabulary is wired", when f was never emitted by anything. It now states that f comes from the readiness gate rather than a numbered step, and asserts it separately, so the a–e list can't be misread as complete.

Verification: each fix mutation-reverted individually — Err → 1 failed; trap → 1 failed; latch moved back → 1 failed; fast path → 1 failed; connect phase → 1 failed; BOM reintroduced → 2 failed. No survivors, green on restore. 853 Pester passing (0 failed), make check green, manifest regenerated for both changed .ps1 files.

One existing guard asserted the old trap one-liner; updated to the full new line rather than loosened, so none of its three parts can be dropped silently.

On CI: agreed it wasn't green — Prereqs — almalinux:9 was still pending when you looked. Re-requesting review now that the threads are closed; I'll watch it to green rather than hand it to you red.

Also seconding a note from #779: Cursor Bugbot reported neutral on both PRs today with Highs open underneath. A neutral conclusion isn't a blocking check, so it's worth confirming the required-status mapping actually gates on it — otherwise a High can merge red. Happy to file that separately.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment threadscripts/lib/telemetry.ps1 Outdated
Comment threadscripts/lib/telemetry.ps1
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Filed the Bugbot-gating point as tracebloc/backend#2284 — measured rather than asserted: Cursor Bugbot reported NEUTRAL on #779 with a High open, is not in develop's required contexts, and NEUTRAL would satisfy a required context anyway. So its verdict gates nothing today; what actually blocks is unresolved-conversation protection, which doesn't distinguish a High from a nit. Nothing has merged red because of it — the human reviews caught everything on both PRs — so it's the missing gate, not damage done.

Comment threadscripts/lib/telemetry.ps1
Comment threadscripts/install-k8s.ps1
…r's state (backend#2268)
Two Bugbot findings with one cause, and it is the only place this port was wrong
as a CLASS rather than a line.
In bash, a sourced lib and its caller share one variable namespace, so
telemetry.sh reads `$CLIENT_STATE` and `$HOST_DATA_DIR` straight out of the
environment. install-k8s.ps1 does not work that way: it RESOLVES those into script
variables — `$script:ClientState` (set by Wait-ForClientReady) and
`$script:HOST_DATA_DIR` (line 661, defaulting to `$env:USERPROFILE\.tracebloc`
when the env var is unset, which is the normal case) — and never exports them.
Ported literally, the emitter read names that are empty on every ordinary Windows
install:
* `client_state` was always absent, so a connect-phase failure could never
classify as bad_credentials / image_pull_failed / image_pull_untrusted_ca /
crash_loop and collapsed to the phase-based `not_ready`. The state-based half
of the classifier was dead code in production.
* the data-dir spool was never used, so every record went to the one-off
fallback file — which backend#2217's transport does not look for. Telemetry
that emits correctly and delivers nowhere.
Both are silent by construction: §1.2 omits an absent value, so the record stays
well-formed. Hence one helper with an explicit precedence — the installer's
resolved value, then the environment — rather than two one-line fixes.
AND A DERIVED TEST FOR THE CLASS, which is the part that pays. Every name the
emitter reads as a script variable must be a name install-k8s.ps1 actually
assigns; the names are parsed out of the emitter's own Get-InstallerValue calls, so
no list lives in the test. It immediately caught my own over-application:
TB_VERSION IS NOT A SCRIPT VARIABLE — IT IS NOTHING AT ALL. Nothing in the ps1
pair set it, by either mechanism, so `service.version` on every Windows record
was permanently `0.0.0-unknown` — the field that says WHICH installer failed, on
the platform this feature was added for. Bugbot did not flag it and neither did
I; the derived test did. install.ps1 now exports the resolved ref exactly as
install.sh:247 does, and install-k8s.ps1 derives TB_VERSION from it, mirroring
common.sh:1128. install-k8s.ps1 runs as a CHILD process, so the export is
inherited — asserted, along with the ordering, because an export after the launch
would be silently useless.
Mutation-proven, anchors asserted applied:
client state back to env-only 3 failed
HOST_DATA_DIR back to env-only 1 failed
the ref export removed from install.ps1 1 failed
The new tests set NO environment variable, which is the point: with env fallback
alone they fail. The pre-existing tests all set env vars and so passed either way —
they could not have caught this, and adding assertions that distinguish the two
sources was the actual work.
860 Pester passing (0 failed), manifest regenerated, make check green,
cross-twin vocabulary agreement green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Both threads fixed in 737c2e7 — one cause, and naming it as a class is what made a third instance findable.

Bugbot found two, and the derived test found a third neither of us flagged. I added a check that every name the emitter reads as a script variable must be one install-k8s.ps1 actually assigns, with the names parsed out of the emitter's own Get-InstallerValue calls so the test holds no list. It failed immediately on TB_VERSION — and the reason is worse than a wrong scope:

Nothing in the ps1 pair set TB_VERSION at all, by either mechanism. So service.version on every Windows record was permanently 0.0.0-unknown — the field that tells you which installer version failed, on the platform this whole feature was added for. install.ps1 now exports the resolved ref exactly as install.sh:247 does, and install-k8s.ps1 derives TB_VERSION from it, mirroring common.sh:1128. The child-process launch means the export is inherited — asserted, along with the ordering, since an export placed after the launch would be silently useless.

That's the argument for the derived check over three more careful readings: I'd read these files closely enough to write 500 lines against them and still missed it.

On the pre-existing tests: every one of them set env vars, so they passed either way and could not have caught this. Adding assertions that distinguish the two sources was the real work — the new tests set no environment variable at all, and there's one on the consequence rather than the plumbing ($script:ClientState = 'image_pull_ca' must yield "error.type":"image_pull_untrusted_ca").

Mutations: client state back to env-only → 3 failed; HOST_DATA_DIR back to env-only → 1 failed; ref export removed → 1 failed.

860 Pester passing (0 failed), make check green, manifest regenerated, cross-twin vocabulary agreement green. bats also ran clean locally on this branch (1325 tests, exit 0).

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Bugbot's find, and the honest description is dead code that reads as a feature.
The emitter carried `Get-TelemetrySourceBasename` / `Get-TelemetrySourceLine` —
colon-splitting careful enough for Windows drive letters, with a test asserting
`C:\Users\...\install-k8s.ps1:12` parses — and NOTHING in the ps1 pair ever set
the value. So every real Windows failure omitted source attribution while the
parser and its tests sat there looking finished.
Both failure sites now supply it, and they need different mechanisms:
* `Show-FatalError` has an ErrorRecord, whose `InvocationInfo` knows exactly
where the throw came from.
* `Err` is a hand-raised failure with no ErrorRecord, so `$MyInvocation` — which
describes the CALL to `Err` — gives the line that actually failed rather than a
line inside `Err`.
Only the file NAME survives, never the path that reached it, and the emitter
closes the basename against its own source vocabulary besides — so a location from
anything that is not one of our scripts still drops BOTH halves, which is the
existing rule: a line number with no file is a confident wrong answer, not a
partial one.
Read through the same precedence helper as the rest of the installer's state, so
this is the fourth instance of the class the previous commit named — and the third
one Bugbot found rather than me. The derived ScriptVar test now covers it for free:
`TbErrLoc` is read as a script variable, so install-k8s.ps1 must assign it.
Mutation-proven, anchors asserted applied:
Err stops recording the location 1 failed
Show-FatalError stops recording it 1 failed
the emitter reads TB_ERR_LOC only again 1 failed
The new test sets no environment variable and asserts the username and drive
letter do not reach the record.
866 Pester passing (0 failed), manifest regenerated, make check green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed on 737c2e74. All five items from my last review are addressed — I verified each against the code:

  • Err / trap exit code (was High #2) — fixed.Err now sets $script:TbExitCode = 1 before exit 1 (with a comment naming exactly why), and the last-resort trap at :6484 sets it too. The primary failure paths no longer reach the emitter as 0.
  • Run-started latch before Help/Diagnose (was High #1) — fixed.Set-TelemetryRunStarted is now at :6500, after the -Help (:6487) and -Diagnose (:6488) dispatch, so those runs no longer latch as started and emit succeeded.
  • Fast-path skipped, connect phase, UTF-8 BOM (the three Mediums) — threads resolved.

One new Bugbot finding is open and it's real — I confirmed it:

install-k8s.ps1:165 — the source location is parsed but never set.telemetry.ps1:477 reads $loc = $env:TB_ERR_LOC, and the emitter carefully splits on the last colon so a C:\...\file.ps1:12 drive letter survives — with tests for exactly that. But grep TB_ERR_LOC finds only that one read: nothing in Err, the trap, or Show-FatalError ever sets$env:TB_ERR_LOC. So tracebloc.install.source / source_line are empty on every real Windows failure, and the drive-letter parser you built (and fixed) is never exercised outside its unit tests. Same "machinery present but not wired" shape as the latch findings. Fix: set $env:TB_ERR_LOC = "$($_.InvocationInfo.ScriptName):$($_.InvocationInfo.ScriptLineNumber)" (or the Err-call equivalent) on those failure paths before the emit.

CI is also still mid-flight (Unit tests, bats, several Prereqs pending), so this isn't approvable yet regardless. Close the :165 thread and once it's green I'll approve — the emitter's correctness is in much better shape now.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment threadscripts/lib/telemetry.ps1 Outdated
Bugbot's find, and the same class as this PR's `-match` -> `-cmatch` fix one
operator down: `-in` and `-notin` are CASE-INSENSITIVE in PowerShell. So
`'STG' -in @('dev','stg','prod')` was True and `STG` reached the record verbatim
as `deployment.environment`. A query keyed on `stg` misses that row — a wrong
label, which is worse than no record. Same hole on `client_state` via `-notin`.
FIXED BY NORMALISING, NOT BY DROPPING, AND THAT DIVERGES FROM BUGBOT'S SUGGESTED
REMEDY DELIBERATELY. It proposed matching the bash twin, which drops an
unrecognised environment under §3.2. That is right for bash, where `case` is
case-sensitive throughout, so `STG` genuinely is not a valid environment there.
It is valid here. PowerShell's `switch` is case-insensitive, so install-k8s.ps1's
own Get-TraceblocClientEnv/Get-BackendUrl send `CLIENT_ENV=STG` to the STAGING
backend — measured, not assumed:
stg -> env=stg url=stg-api
STG -> env=STG url=stg-api <- a real staging install
Staging -> env=stg url=stg-api
So a run with `CLIENT_ENV=STG` is a correctly configured staging install that
worked. Dropping its record to match the twin would discard telemetry for an
install that succeeded — the opposite of what this feature is for. Folding to the
canonical spelling first, then requiring an exact match, fixes the wrong-label
defect Bugbot identified while keeping the run countable: `STG` is emitted as
`stg`, and a genuine non-member like `staging-2` still drops.
Alias folding (`Staging` -> `stg`) stays delegated to Get-TraceblocClientEnv, so
the emitter holds no second copy of that mapping (backend#1745).
FILENAMES ARE THE ONE EXCEPTION and it is now stated rather than inherited from an
operator default: a file's case is a filesystem artifact on Windows, not a
contract value, so `Install-K8s.PS1` must still be attributed. Folded through the
same helper, which also makes the emitted value canonical.
There is now a grep test asserting NO bare `-in`/`-notin`/`-match`/`-notmatch`
remains in the emitter, comments excluded. This class has been introduced twice in
one PR; the third time should fail a test rather than reach review.
MY OWN TESTS HIT THE SAME TRAP A THIRD TIME. `Should -Match` in Pester is also
case-insensitive, so `Should -Not -Match 'STG'` was satisfied by the correct output
`stg` and my first draft failed against a working emitter. Every assertion here now
parses the JSON and uses `-BeExactly` instead of a regex, which is both
case-exact and a better test.
Mutation-proven, anchors asserted applied:
membership back to `-in` 1 failed
canonicalisation dropped (raw returned) 6 failed
the fold removed (strict, loses STG runs) 6 failed
862 Pester passing (0 failed), manifest regenerated, make check green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saqlainsyed007 — thanks for the re-review. The :165 source-location item you flagged was already fixed in 518708c, which landed after the 737c2e74 you reviewed; that thread is closed with the detail.

Since then one more Bugbot High came in and is also fixed (c2bb8ce): -in/-notin are case-insensitive in PowerShell, the same class as the -cmatch fix. Worth a look at the reply on that thread, because I did not take the suggested remedy — dropping the record as the bash twin does would have discarded telemetry for a legitimately configured staging install, since PowerShell's switch makes CLIENT_ENV=STG resolve to the staging backend. Measured that before deciding; folding to canonical was the fix instead.

Head is now c2bb8ce. All threads resolved, 874 Pester passing, 0 failed, make check green, and bats ran clean locally (1325 tests). CI is down to one pending check.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed on c2bb8ce2. The :165 source-location finding is now fixed too, and cleanly: rather than the unset $env:TB_ERR_LOC, the failure paths set a script-scoped $script:TbErrLocErr records "$($MyInvocation.ScriptName):$($MyInvocation.ScriptLineNumber)" (with the comment noting it's the failed line, not a line inside Err), and the trap/Show-FatalError reads $err.InvocationInfo. The emitter reads it via Get-InstallerValue -ScriptVar 'TbErrLoc' -EnvVar 'TB_ERR_LOC', so the drive-letter parser is finally fed on real Windows failures, with the bash-style env var kept as a fallback. Nicely done.

Every thread on this PR is now resolved and all my earlier asks are addressed and verified. The only thing between here and approval is the green gate: the full CI matrix just re-kicked on this push and is still running. Nothing further needed from you on my account — I'll approve as soon as it lands green.

Comment threadscripts/lib/telemetry.ps1
Bugbot's find, and a REGRESSION FROM MY OWN PREVIOUS FIX — worth saying plainly,
because the direction it moved in is the interesting part.
install-k8s.ps1 SEEDS `$script:ClientState = "starting"` at load (:772), long
before anything has diagnosed the client. The bash twin leaves `CLIENT_STATE=""`
(summary.sh:29) and fills it only at the readiness gate (:59/:61) for exactly this
reason. `Get-TelemetryErrorClass` prefers state over phase, so once the emitter
started reading the script variable, EVERY failure in preflight, tools, cluster,
register or helm reported `error.type: not_ready` with `client_state: starting`
instead of the phase-based class.
Two commits ago the state never worked and the attribute was absent. One commit
ago it always said `starting`. The second is worse: an absent attribute omits
information, a seeded one ASSERTS something false — and it did so on the paths this
feature primarily exists to measure, while looking more complete than before.
DERIVED, not a new marker variable. Both real writers of ClientState live inside
Wait-ForClientReady (:5387/:5388), which is also where phase `f` opens — so "we
are in the connect phase" IS "the gate has run", and there is no second thing to
keep in step. A failure during connect with the value still `starting` is honestly
`not_ready`: we waited, and it did not become ready.
THE GUARD I WROTE LAST COMMIT TO CATCH THIS CLASS HAD THE SAME HOLE AS THE CODE.
It banned bare `-in`/`-notin`/`-match`/`-notmatch` and said nothing about
`-eq`/`-ne`, which are equally case-insensitive — so a mutation swapping `-ceq`
for `-eq` on the new gate survived it. A guard against a class that knows only two
members of the class is the shape this epic keeps finding, and it was mine.
Now it flags the whole family (`eq ne in notin match notmatch contains
notcontains like notlike`) when compared against a STRING LITERAL — scoped that
way on purpose, because `-eq 0` and `-eq $null` have no case and a blanket ban
would be noise that gets switched off. Four existing literal comparisons converted
to `-ceq` to satisfy it. A false-positive mutation (numeric `-eq 0`) confirms the
scoping holds.
Mutation-proven, anchors asserted applied:
gate removed, state read unconditionally 5 failed
gate on the wrong phase 8 failed
gate made case-insensitive (-eq) 1 failed <- survived before the widening
one literal comparison reverted to -eq 1 failed
numeric -eq introduced 0 failed <- correctly NOT flagged
Tests Passed: 880, Failed: 0, Skipped: 13, Inconclusive: 0, NotRun: 0
manifest regenerated, make check green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick triage on the red CI at 25d540eaall three failures are transient, not your change, so don't go debugging the diff:

  • Template render (aks) and Template render (eks) both died at Install kubeconform with curl: (22) ... error: 504 — GitHub's release CDN 504'd on the pinned-binary download, three retries, step failed before any template was rendered.
  • quality / gitleaks is the same 504, installing the gitleaks binary — so the scanner never ran. To be clear: this is not a secret finding, it's a download timeout. No leak was reported.

A re-run (or the next push) should clear all three. Content-wise you're done from my side — every thread is resolved and all my earlier asks are verified fixed. Still holding only because the green gate counts any red check; I'll approve as soon as CI is actually green.

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 25d540e. Configure here.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saqlainsyed007 CI is green now — 43 passed, 4 skipped, 0 failures on 25d540e.

The two red checks were both the pinned-binary download, not the checks themselves: quality / gitleaks failed in "Install gitleaks (version + SHA-256 pinned)" with its scan step skipped, and Template render (aks) took a 504 fetching kubeconform. Re-ran the failed jobs and both passed unchanged — no code touched.

All threads resolved, including the last Bugbot High (the seeded ClientState, which was a regression from my own earlier fix — detail on that thread). Over to you whenever.

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on 25d540ea — CI is green (43 pass, 4 skip, 0 fail), every review thread is resolved, and all my earlier findings are verified fixed. As diagnosed last pass, the three red checks were transient release-CDN 504s installing pinned tools (gitleaks included, which never ran a scan), and the re-run cleared them cleanly.

Recapping what this PR closed, all confirmed against the code:

  • Exit-code truthfulnessErr and the last-resort trap now set $script:TbExitCode = 1, so the primary failure paths no longer reach the emitter as 0 and mislabel a failed install install.run.succeeded.
  • Help/Diagnose no longer latchSet-TelemetryRunStarted moved after that dispatch, so runs that never touched the machine don't emit succeeded.
  • Fast-path skip, connect phase, UTF-8 BOM — all resolved.
  • Source location is fed$script:TbErrLoc is set from $MyInvocation in Err and from $err.InvocationInfo in the trap, read by the emitter with the bash-style env var as fallback, so the drive-letter parser is exercised on real Windows failures.

The port is faithful to telemetry.sh's contract, the "four bugs found by running" writeup is a real gift to the next PowerShell port, and the emitter now reports both the truth of a run and where it failed. Nicely done — LGTM.

@LukasWodka
LukasWodka merged commit 335bcf2 into developAug 21, 2026
60 of 63 checks passed
@LukasWodka
LukasWodka deleted the feat/2268-windows-installer-telemetry branch August 21, 2026 10:58
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saqlainsyed007