Uh oh!
There was an error while loading. Please reload this page.
fix(installer): strip SS3 escapes and floor escape-only input (cli#516) - #736
Conversation
_strip_paste_garbage and ConvertTo-SanitizedInput handled CSI (ESC '[' … final) only. SS3 (ESC 'O' final) is what the same arrow / Home / End / F-keys emit once the terminal is in DECCKM application-cursor mode — the state vim, less or tmux leave behind on an unclean exit. That residue was worse than the CSI residue fixed in client#362 / cli#364 (2026-07-21, not re-litigated here): CSI cleans to empty and the name prompt re-prompts, while 'O' and the final byte are printable, so ESC OD ×3 ESC OA ×3 survived as the plausible name "ODODODOAOAOA" and minted the permanent namespace "odododoaoaoa". Nothing downstream can refuse it — the backend validates DNS-1123 form by idempotence against the slug rule, and form is exactly what this input preserves. Two changes in each of the two copies here: 1. The strip matches CSI and SS3 in one pattern. 2. A post-sanitise floor. If an ESC SURVIVES the strip the value carries an escape family we do not recognise — which is precisely how SS3 got here — so it must show one alphanumeric that did not come from an escape final byte, probed with a greedier pattern whose output is never returned. Nothing but residue emits empty, which every caller already treats as "no answer" (re-prompt, or auto-name in the CLI). Scoped to "an ESC survived" so a clean value never reaches it and real content beside an unknown escape is kept. The bash floor tests for content with `tr`, not `=~ [[:alnum:]]`: bash's regex engine is locale-dependent and under the C locale the installer often runs in, [[:alnum:]] does not match a UTF-8 letter. That was caught by a test, not by review — see the mutation evidence below. Tests: 9 new bats cases for _strip_paste_garbage (SS3 arrows / Home-End / F-keys / mixed with CSI / truncated / a bare O is not an escape; the floor with SS2 standing in for "the next family", including the non-Latin-content case). Mutation-proven, three anchors, each applied and each detected: • SS3 dropped from the strip pattern -> 2 cases red (SS3 around content, mixed) • floor short-circuited to false -> 2 cases red (truncated SS3, unknown family) • tr check swapped for [[:alnum:]] -> 1 case red (non-Latin content) The "SS3 arrows only" case is green under anchor 1 because the floor also covers it; anchor 1 is carried by the mixed-content cases, which the floor cannot mask. The PowerShell peer's behaviour was verified against the same 17-case corpus out of tree (17/17), but its committed tests live in scripts/tests/install-k8s.Tests.ps1, which is outside this change's scope — see the PR body. scripts/manifest.sha256 regenerated (common.sh and install-k8s.ps1 are both in the bootstrap's integrity surface). The Go peer gets the same two changes in tracebloc/cli. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4ce3c2d. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
Self-review finding on the commit before this one, caught before a reviewer saw
it: the floor's probe copied the strip's `while [[ $s =~ $pat ]]; do
s="${s/${BASH_REMATCH[0]}/}"; done` shape, and pattern substitution treats
BASH_REMATCH as a GLOB, not a literal.
That is safe for the CSI loop by construction — its match is ESC '[' [0-9;]*
<final>, which can never contain a `]`, so it can never form a complete bracket
expression and the glob always degrades to the literal. The floor's probe
pattern has `[^A-Za-z0-9~]*` in the middle, which CAN swallow a `]`. On the
input ESC [ ; ] A the regex matches the whole value, the glob `<ESC>[;]A` then
means ESC ';' 'A' — not present in the string — the substitution removes
nothing, and the loop never terminates. A hang at the installer's name prompt,
on a value the floor exists to refuse.
Replaced with a single `LC_ALL=C sed -E` pass: no glob semantics, no loop, same
result on all 17 corpus cases.
Mutation-proven: restoring the loop turns the new test red (status 142 under a
local timeout stand-in — the call never returns), and the sed version passes it.
The test is bounded the way common.bats bounds its recursion guard; macOS ships
no timeout(1), so Linux CI is the authority on the hang half.
scripts/manifest.sha256 regenerated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>Bugbot, Medium, on tracebloc/client#736: the floor's probe used an unbounded `[A-Za-z~]+` after an unrecognised ESC, so every ASCII letter following the escape was swallowed into the probe and the value read as residue-only. It is right, and the sharper half of it is the part I had not seen: `\x1bNChello` was refused while `\x1bNC日本` was kept, which makes keep-vs-reject depend on the script the user's name is written in. I had accepted the over-strictness on purpose; I had not noticed it was inconsistent. Bounded to `{1,2}`. Two, not one and not unbounded: one leaves the 'D' of an unrecognised SS3-shaped pair behind and the floor stops firing on the exact family shape this ticket is about, while unbounded eats a whole name. An escape final is one byte, an intro plus a final is two, and every keyboard-input escape family (SS2, SS3, the 7-bit C1 forms) fits in that — so the bound is a statement about escapes rather than a tuning constant. Every case the floor is meant to catch is unaffected: ESC N B / ESC N C, a truncated ESC O, and ESC [ ; ] A all still collapse to empty. Applied to all three copies so the rule stays one rule. Mutation-proven: reverting `{1,2}` to `+` turns the new case red in Go ("\x1bNChello" -> "") and in bats. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bugbot, Medium, on #736: the floor's probe used an unbounded `[A-Za-z~]+` after an unrecognised ESC, so every ASCII letter following the escape was swallowed into the probe and the value read as residue-only. It is right, and the sharper half of it is the part I had not seen: `\x1bNChello` was refused while `\x1bNC日本` was kept, which makes keep-vs-reject depend on the script the user's name is written in. I had accepted the over-strictness on purpose; I had not noticed it was inconsistent. Bounded to `{1,2}`. Two, not one and not unbounded: one leaves the 'D' of an unrecognised SS3-shaped pair behind and the floor stops firing on the exact family shape this ticket is about, while unbounded eats a whole name. An escape final is one byte, an intro plus a final is two, and every keyboard-input escape family (SS2, SS3, the 7-bit C1 forms) fits in that — so the bound is a statement about escapes rather than a tuning constant. Every case the floor is meant to catch is unaffected: ESC N B / ESC N C, a truncated ESC O, and ESC [ ; ] A all still collapse to empty. Applied to all three copies so the rule stays one rule. Mutation-proven: reverting `{1,2}` to `+` turns the new case red in Go ("\x1bNChello" -> "") and in bats. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…520) * fix(sanitize): strip SS3 escapes and floor escape-only names (cli#516) sanitizeClientName handled CSI (ESC '[' … final) only. SS3 (ESC 'O' final) is what the same arrow / Home / End / F-keys emit once the terminal is in DECCKM application-cursor mode — the state vim, less or tmux leave behind on an unclean exit. That residue was worse than the CSI residue fixed in cli#364 / client#362 (2026-07-21, not re-litigated here): CSI cleans to empty and re-prompts, while 'O' and the final byte are printable, so ESC OD ×3 ESC OA ×3 survived as the plausible name "ODODODOAOAOA" and minted the permanent namespace "odododoaoaoa". Nothing downstream can refuse it: is_dns1123_label validates by idempotence against the slug rule, so escape-derived garbage is a perfectly canonical label. Form is exactly what this input preserves. Two changes, both in sanitizeClientName — deliberately NOT in internal/slug, which must stay a faithful mirror of backend/common/utils/slug.py: 1. escSequence now matches CSI and SS3 in one pattern. 2. A post-sanitise floor. If an ESC SURVIVES step 1 the value carries an escape family we do not recognise — which is precisely how SS3 got here — so it must show one alphanumeric that did not come from an escape final byte, probed with a greedier pattern whose output is never returned. Nothing but residue returns "", the same path an omitted --name takes. Scoped to "an ESC survived" so a clean name never reaches it and real content beside an unknown escape is kept; the failure it chooses is the recoverable one. Tests: 10 new cases in the table (SS3 arrows / Home-End / F-keys / mixed with CSI / truncated / a bare O is not an escape; the floor with SS2 standing in for "the next family", including the non-Latin-content case) plus a test pinning the ticket's exact repro and the slug it used to mint. Mutation-proven, three anchors, each applied and each detected: • SS3 dropped from escSequence -> 2 cases red ("na\x1bODme", SS3+CSI mixed) • floor short-circuited to false -> 2 cases red (truncated SS3, unknown family) • hasAlphanumeric made ASCII-only -> 1 case red (non-Latin content) The "SS3 arrows only" case is green under anchor 1 because the floor also covers it; anchor 1 is carried by the mixed-content cases, which the floor cannot mask. The bash and PowerShell peers get the same two changes in tracebloc/client. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(release): VERSION 0.10.8 -> 0.10.9 (cli#516) version-bump-gate is a required check and it refuses a PR that touches internal/* while VERSION still names an already-released version: v0.10.8 is out, so shipping this fix under it would put different bytes under an existing release. 0.10.9 is untagged and above every released final version, and it is the same target the other two open PRs on develop bump to — identical one-line changes merge without conflict, and all three then ship under the pending 0.10.9. Not a hand-cut release: the release train still reads this file and cuts the tag from it at the prod hop. The gate's own message is explicit that it never bumps for you, and that a stale VERSION fails days later on somebody else's hop (backend#1561) rather than here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(sanitize): bound the floor's probe to two final bytes (cli#516) Bugbot, Medium, on tracebloc/client#736: the floor's probe used an unbounded `[A-Za-z~]+` after an unrecognised ESC, so every ASCII letter following the escape was swallowed into the probe and the value read as residue-only. It is right, and the sharper half of it is the part I had not seen: `\x1bNChello` was refused while `\x1bNC日本` was kept, which makes keep-vs-reject depend on the script the user's name is written in. I had accepted the over-strictness on purpose; I had not noticed it was inconsistent. Bounded to `{1,2}`. Two, not one and not unbounded: one leaves the 'D' of an unrecognised SS3-shaped pair behind and the floor stops firing on the exact family shape this ticket is about, while unbounded eats a whole name. An escape final is one byte, an intro plus a final is two, and every keyboard-input escape family (SS2, SS3, the 7-bit C1 forms) fits in that — so the bound is a statement about escapes rather than a tuning constant. Every case the floor is meant to catch is unaffected: ESC N B / ESC N C, a truncated ESC O, and ESC [ ; ] A all still collapse to empty. Applied to all three copies so the rule stays one rule. Mutation-proven: reverting `{1,2}` to `+` turns the new case red in Go ("\x1bNChello" -> "") and in bats. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Conflict was scripts/manifest.sha256 only — a generated file. Resolved by re-running scripts/gen-manifest.sh over the merged tree rather than hand-merging two sets of hashes, which would produce a manifest matching neither side and make the bootstrap refuse its own scripts. client#735 (the Windows-on-ARM cosign asset) landed on develop and regenerated the same file; both sides also touch install-k8s.ps1, which merged cleanly.
saqlainsyed007
left a comment
There was a problem hiding this comment.
Approving — reviewed for correctness.
Verified empirically (sourced the PR-head scripts/lib/common.sh and ran _strip_paste_garbage):
- All in-PR bats vectors reproduce exactly (SS3 strip, floor rejection of residue-only, non-Latin content kept,
ESC [ ; ] Aterminates via thesedpass rather than hanging). - Own adversarial vectors also pass: CSI
ESC[3~, bracketed paste, pure-CSI→empty, legitna~me, 3-byte unknown run.
Manifest integrity: recomputed SHA-256 of both touched bootstrap files (common.sh, install-k8s.ps1) at the PR head — both match scripts/manifest.sha256. Good, since this is the installer's Tier-0 integrity surface.
Caller contract: confirmed the floor's empty return is treated as "no answer" by every consumer — provision.sh re-prompts (×3) then errors, cluster.sh's _read_sanitized yields an empty var, install-client-helm.sh's _sanitize_credential warns. No path mints an empty namespace.
Logic: the combined CSI+SS3 strip stays glob-safe in the ${s/${BASH_REMATCH[0]}/} loop (a match can never contain ], so no infinite substitution), and the floor's sed-not-glob-loop + LC_ALL=C tr choices are correct and locale-independent (checked the 日本 / C-locale case directly).
All 40 CI checks are green (Bugbot, Pester on Windows + Ubuntu, bats, static analysis, gitleaks). The one Bugbot thread (floor over-sweep → {1,2} bound) is resolved. The bash \200-\377 vs PowerShell [\p{L}\p{Nd}] content definitions differ only for symbol-only input beside an unknown escape — never a valid name — and it's documented intent, so not a blocker.
The two disclosed follow-ups (committed PowerShell test for the new behaviour; the shared cross-repo fixture) are correctly deferred to backend#2084.
Uh oh!
There was an error while loading. Please reload this page.
`develop` gained #735 (amd64 cosign bootstrap on Windows-on-ARM) and #736 (SS3 escape stripping), both of which touch `scripts/lib/common.sh` — the file this branch also edits, since the binfmt probe moved there so both arch gates read one probe. `common.sh` and `install-client-helm.bats` auto-merged. `manifest.sha256` was the only conflict and was REGENERATED with `scripts/gen-manifest.sh` rather than hand-merged: the manifest is a derived artifact, and a hand-resolved one records hashes for a tree that never existed. Re-running the generator now produces no diff, so the committed manifest matches the merged tree. Verified on the merged result, not on either side: the full bats suite exits 0 with 0 failures (1119+ tests), so #739's `_pf_arch` / `_assert_engine_runs_on_this_arch` changes still hold against develop's `common.sh`.
… no test (#743) #736 extended ConvertTo-SanitizedInput with the SS3 family and the unknown-family floor, and shipped both with zero PowerShell coverage. The four cases in that Describe block are all CSI, so on Windows the entire SS3 half and the whole floor were unverified while bash and Go both had cases. That asymmetry is the mechanism behind the bug it was fixing. The rule is hand-copied into three languages, only one shape was ever tested in one of them, and SS3 went missing from all three at once (tracebloc/cli#516) — exactly as the CSI gap had before it (cli#364 / client#362). A rule with coverage in two of three implementations is a rule that drifts in the third. Eight It blocks, mirroring the bats corpus in install-client-helm.bats case-for-case: SS3 around content, SS3-only (arrows, Home/End, F1/F2) to empty, SS3+CSI mixed, a bare O is not an escape, truncated ESC O, the unknown family alone and beside content, and the floor counting non-Latin letters as content. The non-Latin case is asserted as a PAIR with the ASCII one. Either alone proves nothing: the point is that keep-vs-reject does not depend on the script a name is written in (Bugbot, #736), and only the two together say that. Mutation-proven, each anchor confirmed to apply, 12 cases in the block: drop SS3 from the strip (the cli#516 bug) -> 2 fail remove the floor entirely -> 2 fail floor uses [A-Za-z] instead of \p{L} -> 1 fail match O<final> without requiring ESC -> 4 fail Full Pester suite: 671 passed, 0 failed, 13 skipped. This is backend#2084's carved-out sub-task, not the ticket. The structural half — one corpus file all three suites derive from, with a cross-repo drift check — is untouched here, and deliberately: a fixture wired into two of the three implementations is the same defect wearing a different hat. Refs backend#2084
… path (backend#1907) (#747) * feat(telemetry): one outcome event per install, with nowhere to put a path (backend#1907) The installer is the highest-variance, least-observed step in the product: it runs on machines we have never seen, under package managers, proxies and shells we do not control, and it reports to nobody. Each of the backend#736 failures — the CLI landing in ~/.local/bin with PATH advice only printed, apt-get appearing hung because unattended-upgrades held the dpkg lock — was invisible until a customer happened to mention it. scripts/lib/telemetry.sh emits one contract-shaped event per run from install_cleanup, the EXIT trap, so it fires on every path including the interrupted and the fatal one. It carries the phase reached, per-phase durations, the exit code, the client state, OS/arch, the version, an error class, and — for the #736 PATH case specifically — TB_CLI_ON_FRESH_PATH, which install-cli.sh has always computed and only ever printed advice about. "NO ARGUMENTS, NO PATHS, NO DATA" IS A SHAPE, NOT A RULE. Every value goes through _telemetry_attr, which admits a string only if it matches ^[A-Za-z0-9._-]{1,64}$ and an integer only if it is one. A path contains '/', a proxy credential contains ':' and '@', a token is longer than 64 characters, a name contains a space. Values that fail are dropped, never trimmed: a redactor has to imagine what it is stripping, and a shape only admits what it was told to. The phase and the client state are additionally checked against their closed sets at the render boundary — a canary assigned straight to TB_TELEMETRY_PHASE reached the record before that line existed, and it was shaped exactly like a legal value, so the token regex waved it through. The vocabularies are DERIVED, and a new guard proves it. scripts/tests/telemetry-vocabulary-agreement.sh parses install-k8s.sh's step_header calls, summary.sh's CLIENT_STATE writers, gen-manifest.sh's FILES array and install.sh's release-tag regex, and compares each to telemetry.sh's declaration; the error classes have no second declaration, so it exercises the classifier over the full cross-product and checks both that every answer is registered and that every registered class is reachable. It runs in drift-checks' `Source-of-truth drift` job, which is required — a guard in a job nobody must wait for is advice. It found one thing on its first run: summary.sh's own CLIENT_STATE docstring had been missing image_pull_ca since #424. TWO REAL BUGS THE TESTS CAUGHT, both of the same shape and both fatal: `printf | grep -q` returns 141 on a match under `set -o pipefail`, and so does `tr -dc < /dev/urandom | head -c 16` — the latter at SOURCE time, which killed the whole installer before it printed a line. Every unit-level test passed throughout; only the test that runs install_cleanup for real under the installer's own shell options went red. That test stays. install-bootstrap.bats held a hand-written second copy of install.sh's FILES array, in two places, so adding a lib turned ten unrelated supply-chain tests red. It now derives the list, and fails closed on an inert parse. WHAT IS NOT CONNECTED: the transport. The 17 Aug decision (rfcs#28) replaced the Collector gateway with an ingest endpoint on the backend — backend#1905, which does not exist yet — so _telemetry_deliver writes the install log and a bounded 0600 local spool that #1906's forwarder can drain, and posts nothing. Opt-out (default on) via TRACEBLOC_NO_TELEMETRY or DO_NOT_TRACK, documented in --help — and that promise is itself checked, because a user who exports a stale name believes they have opted out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(telemetry): drop the literal `curl -u user:pass` from a canary fixture (backend#1907) gitleaks' curl-auth-user rule fired on the TB_ERR_CMD fixture, and it was right to: a source file containing that spelling is a finding whatever the surrounding test claims, and a reviewer scanning the diff has no way to tell a canary from the real thing at a glance. The fixture's purpose is unchanged — TB_ERR_CMD holds the failing command UNEXPANDED, which is free text carrying a path, and must not be emitted. It now carries a path instead of a credential. The credential half of the same test is already covered by HTTPS_PROXY, which encodes user:pass in a proxy URL and is what a hospital network actually configures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): a --help run is not a successful install (backend#1907) install_cleanup is the EXIT trap, so it fires for every exit of install-k8s.sh — including the terminal commands that touch no machine. `--help` exits 0 and was emitting a full install.run.succeeded with phase `bootstrap`. Reproduced: $ HOST_DATA_DIR=$T bash scripts/install-k8s.sh --help {"attributes":{"event.name":"install.run.succeeded", "tracebloc.install.phase":"bootstrap","tracebloc.install.exit_code":0,…}} That is the worst bug this feature could have. `--help` is the command people run MOST while a real install is broken, so a free success lands in the denominator of the failure RATE — the one number the ticket exists to produce — and moves it in the direction that hides the problem. Found by Bugbot on client#747. The fix is a latch, not a phase test: main() calls telemetry_run_started once --help / --diagnose / prepare-host have had their chance to dispatch, and telemetry_emit_outcome returns early without it. A phase test would have been wrong — a genuine failure IN the bootstrap phase (the leftover-data guard, validate_config) is an install attempt and must still be reported, which is now pinned by a test that drives the real entrypoint into a validate_config rejection. Found while fixing it: the assess handoff exits 0 having run no step, so counting it as succeeded would grow the success count with re-runs on machines nothing happened to. It now emits install.run.skipped — a registered outcome verb (contract §6.4), so no new vocabulary — which also makes "how often is the installer re-run on a machine that was already done" answerable. prepare-host is deliberately still not reported: it is a different command with its own registry component (§10.1), and filing it under tracebloc.component=install would be mislabelling it rather than measuring it. TWO OF THE SIX NEW MUTATIONS CAME BACK INERT on the first pass — "main stops setting the latch" and "assess.sh stops marking the handoff" — because the tests set those flags themselves and so could not see the WIRING disappear. That is the same class as the bug Bugbot found: a unit test of the emitter cannot observe which exits reach the trap. Both are now driven end to end, through the real install-k8s.sh and the real _assess_handoff, and both mutations redden. Bugbot's second finding (the source lookup aborting the whole event under set -e) is reported as unreachable with evidence rather than fixed: a command substitution in an ARGUMENT position does not propagate its status to the enclosing command, measured on bash 3.2.57 and 5.x. The invariant is pinned anyway — an unrecognised source location drops the field, never the event. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): the phase that was still running had no duration (backend#1907) telemetry_phase_begin only closes a phase when the NEXT one starts, and nothing closed the active phase before the event rendered. That lost the most important number in the file, and it lost it in exactly the case the ticket names: * on every SUCCESSFUL install, phase_connect_ms was absent — the readiness wait, up to READY_TIMEOUT (600s), the single longest phase; * on every failure and every cancel, the phase named by tracebloc.install.phase had no duration. The dpkg-lock case in its likeliest real form is stuck twenty minutes in `prerequisites` and then killed or given up on, never reaching step c. Reproduced before fixing: "tracebloc.install.phase":"prerequisites" "tracebloc.install.phase_preflight_ms":0 <no prerequisites duration at all> * `bootstrap` had no key at all, because the loop iterated the letter map and bootstrap has no step letter. So the download + verify + leftover-guard + assess time was an unnamed remainder — which is also why subtracting the other keys from duration_ms could not recover the missing active phase. Found by Bugbot on client#747. My own "a slow phase is visible" test passed throughout, because it only ever measured a phase a later step_header had closed — the exact shape of a test that proves the easy half. The live delta is added at READ time rather than by a "close the phase" call in the emit path, so render stays idempotent: the tests call it repeatedly, and a render that mutated the accumulators would report different numbers each time. The clock is now read once per event, so the per-phase numbers and the total are exactly consistent — which is an invariant a test asserts. That test needed a FAKE CLOCK. The obvious fixture is wrong: winding _TB_TELEMETRY_PHASE_STARTED_MS backwards after the step_headers have already attributed that time invents milliseconds that never elapsed, and the first version failed for precisely that reason (sum 1200000 vs total 900000). Four mutations run. Three redden — dropping the live delta, going back to the letter map, and counting the delta for every phase. The fourth (reading the clock per attribute instead of once) is INERT and is reported as inert rather than counted: _telemetry_now_ms has second resolution, so two reads inside the same second are identical, and the guard only matters across a second boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): the spool must not create the data dir the installer refused (backend#1907) _telemetry_deliver ran from the EXIT trap and did `mkdir -p "$HOST_DATA_DIR/telemetry"` unconditionally — including on the path where early_data_dir_guard had just REFUSED that directory for being on a network filesystem and called `error`. That guard deliberately skips an existing directory ("an EXISTING data dir has no at-risk mkdir here", client#441), so anything creating the directory behind its back disarms it for every later run: run 1: guard refuses (dir absent) -> error -> EXIT trap -> telemetry creates it run 2: guard sees the dir, returns 0 -> MySQL installs onto NFS which is exactly the InnoDB corruption client#432 exists to prevent, reintroduced by the telemetry that was only supposed to watch. Reproduced before fixing: guard exit=1 HOST_DATA_DIR created on the REJECTED volume? YES ./nfs-volume/.tracebloc/telemetry An observer that changes the install's own preconditions is not an observer. The spool now only writes INTO a data dir that already exists; a run that dies before that still reports through the install log, which is what a support bundle collects and which _choose_log_file has already placed somewhere safe (falling back to $TMPDIR). Found by Bugbot on client#747 — the third real finding of three rounds, and the most serious: the other two corrupted the metric, this one corrupted a customer's database. Both directions mutation-proven: removing the existence check reddens the new test, and disabling delivery outright reddens four others, so the fix cannot pass by simply turning the feature off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): a pre-log failure had nowhere to go, so it went nowhere (backend#1907) Fallout from the previous commit, and the worst kind: the fix that stopped telemetry disarming the NFS guard also made the NFS refusal itself invisible. _telemetry_deliver's comment claimed "the install log always gets it". It does not. `log` is a no-op until setup_log_file sets LOG_FILE, and setup_log_file runs AFTER validate_config and early_data_dir_guard — deliberately, because #432 refuses a network data dir BEFORE logging starts. So on exactly those paths there was no log AND (correctly, since the previous commit) no data dir, and the rendered event was discarded: $ early_data_dir_guard # target reads as nfs guard exit=1 any telemetry written anywhere? 0 A run refused for being on NFS is a real, actionable field failure, and it was the single case producing no record at all — invisible to the very failure rate this feature exists to produce. Those pre-log failures are also precisely the class the run-started latch was built to preserve, so losing them undid that too. Found by Bugbot on client#747, which also spotted that this file's own NFS test masked the bug by setting LOG_FILE=/dev/null. That test now leaves LOG_FILE unset, as the real path does, and asserts the refusal IS reported. Fix: when there is no data dir, spool to a mktemp'd file in $TMPDIR. mktemp and not a fixed name — /tmp is world-writable on Linux and the installer runs privileged steps, so a predictable path is a symlink target for an append that may be running under sudo; mktemp creates with O_EXCL. This mirrors _choose_log_file's own fallback, so an early-failure run leaves one small file beside the install log it already leaves there, rather than a new class of litter. #1906's forwarder reads both locations. A comment that claims something untrue is itself the defect (workspace CLAUDE.md rule 7), so the false claim is replaced with what actually holds and why. Four mutations, all reddening: removing the fallback, giving it a predictable shared path, dropping its 0600 mode, and dropping the data-dir existence test (which would disarm the NFS guard again). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): the record went into the directory the bootstrap deletes (backend#1907) Four findings from @saadqbal's review, each reproduced before it was fixed, plus the nit. The first one is the significant one: it silently undid round 4 on the primary macOS path. 1. The fallback spool landed inside the bootstrap's own scratch dir. install.sh:238 does `TMPDIR="$(mktemp -d)"` and :239 traps `rm -rf "$TMPDIR"`. A plain assignment to a name that is ALREADY EXPORTED keeps the export attribute — and TMPDIR is always exported on macOS — so install-k8s.sh inherited the doomed directory and `mktemp "${TMPDIR:-/tmp}/…"` wrote the record into it. Reproduced end-to-end: the spooled file was gone the moment the bootstrap returned. So the NFS refusal, and every other pre-setup_log_file failure, still produced no record anywhere on macOS — the exact hole the fallback closed. _telemetry_fallback_dir now disqualifies TMPDIR when the running installer is inside it, which is true precisely when TMPDIR is the bootstrap's scratch dir, and falls back to $HOME (never $HOME/.tracebloc — telemetry must not create HOST_DATA_DIR) and then /tmp. DERIVED rather than agreed: asking install.sh to export its original TMPDIR under another name would work only when the bootstrap is new, and install.sh is served from a URL a user may have curl'd months ago. Both sides of that comparison are resolved with `pwd -P`. The first cut compared them as written and missed every Mac, because /var is a symlink to /private/var — caught by re-running the reproduction against the fix, not by reading it. 2. grep is line-based, so a value with a newline passed the shape check. The one input shape "nowhere for a path to go" does not cover, because what lands is not a path — it is a second line: TB_VERSION=$'v1.9.3\n","tracebloc.install.injected":"yes' → "service.version":"v1.9.3 ","tracebloc.install.injected":"yes",… A forged attribute AND one record split across two lines of a .jsonl spool, so #1906's forwarder reads two malformed events. Reproduced on all four checks — the key, str and int shapes in _telemetry_attr, _telemetry_version, and _telemetry_source_line's inline regex. All now `[[ =~ ]]`, which anchors at end of string. As a bonus it removes the external process, so the backend#1778 SIGPIPE class the here-strings were working around cannot recur here at all. The agreement check proved the two version regexes were byte-identical while they behaved differently, and reported that as "the service.version shape is install.sh's own release-tag gate" — a claim about behaviour that byte-identity does not support, because each side was matched with a different operator. It now checks both: byte-identity, then verdict agreement over a corpus, each side evaluated the way the file that owns it evaluates it. The corpus is written down independently of either matcher and the check fails closed if it contains no embedded-newline input, since without one it degenerates into the byte check. 3. exit 2 is the "complete this step and re-run" handoff, not a failure. gpu-nvidia.sh:55 exits 2 after install_nvidia_drivers SUCCEEDED, to ask for a reboot. That call sits under step_header b, so every unattended GPU host's first install booked an `install.run.failed` with error.type=prerequisites_failed — a fabricated prerequisite failure in the rate this ticket exists to produce. Same shape as the --help bug, opposite direction; install_cleanup has treated 2 as its own outcome ("Re-run required") since client#681. It now renders install.run.cancelled and carries no error.type. It rides an existing verb rather than a new one because §6.4's outcome list is closed and adding to it is a PR against the contract, not an emitter's unilateral call; of the registered verbs, `cancelled` is the only terminal one that is true here. exit_code stays on the record, so 2 (handoff) and 130/143 (Ctrl-C) remain separable — which is why the exit code is an attribute in the first place. Event names are now a declared closed set with a guard. The guard derives the emitted names two ways — the literals in the case statement, and what the function actually renders over the installer's exit codes — rather than reading the declaration twice, and checks §6.1's grammar. It cannot check the §6.4 half from this repo: the verb registry is in rfcs, and a hand-copied second list of verbs would be the defect rather than the fix. 4. The chmod ran before the trim replaced the file. `tail > "${spool}.tmp"` creates under the process umask and `mv` keeps the tmp file's mode, so 0600 did not survive. common.sh's `umask 077` normally covers it, but _install_userspace_tools (setup-linux.sh:893) and its macOS twin set `umask 022` and restore it only afterwards. Reproduced: spool 644. The chmod now runs on the inode that survives, before the mv — one chmod, not two, because a second one on the spool afterwards is unreachable belt and braces that no test can redden. The test pinning 600 could not see any of this, because load_lib sources common.sh first and every test therefore ran under 077. There is now one that sets umask 022 and asserts the umask actually took. 5. nit: the comments claimed coverage the file does not have. `bootstrap` means "install-k8s.sh before step a", not "everything before step a" — download and verify happen in install.sh, which never sources this file and whose EXIT trap is `rm -rf "$TMPDIR"`. install.sh in TB_TELEMETRY_SOURCES is unreachable for the same reason: TB_ERR_LOC has exactly one writer, common.sh's _record_err. Both comments now say so. No bootstrap telemetry added. Also noted on install.run.skipped, which reads wider than it is: install.sh:132 reaches a healthy machine and `exec tracebloc`s at :144 before install-k8s.sh is fetched, so on curl|bash the assess gate is never reached at all. Tests: 4 new bats tests (36 in telemetry.bats, 1161 across the suite, all green), 3 new checks in the agreement guard. Every fix mutation-proved: 11 mutations, 11 reddened, each with its anchor asserted. Two first-pass mutations came back inert and are fixed rather than counted — one removed a redundant chmod nothing could observe (the redundancy is now gone), the other rewrote the guard's own detector alongside its corpus so the detector matched its mutated needle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(installer): regenerate the manifest after the handoff-marker change scripts/gen-manifest.sh output, required by the Static analysis R8 gate after any installer script changes (backend#1907). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): the fallback was unreachable in the case it was built for (backend#1907) Two Bugbot findings, both reproduced before either was touched. 1. HIGH — a failed data-dir spool write dropped the outcome event. `_telemetry_deliver` had `|| return 0` on both the spool's `mkdir -p` and its append, so a data dir that EXISTS but refuses the write ended the function with the record nowhere — never reaching the $TMPDIR fallback added in 46a33de. That is not an exotic path: HOST_DATA_DIR present but unwritable is precisely when `_choose_log_file` (common.sh:769-775) has already fallen back to a mktemp log, and on curl|bash that log sits in the bootstrap's own doomed TMPDIR, so the `log` line kept nothing either. The fallback existed and was unreachable in exactly the case it was written for. Reproduced both halves, each with an anchor proving the fixture was not inert: * a 0500 data dir with no telemetry/ yet. Anchor A: `_choose_log_file` really does fall back out of that dir (called, not asserted). Anchor B: telemetry/ was not created afterwards, so the mkdir really failed. Result: no data-dir spool, no fallback file. The event was gone. * pending.jsonl replaced by a directory, so the append cannot open it. Same result, and it holds for root too. Both now fall through. The single-write guarantee comes from control flow rather than a flag: the `return 0` sits after a SUCCESSFUL append and nowhere else, so the fallback is reachable only on a path that wrote nothing. Everything after the append — chmod, trim, mv — may fail freely, because the line is already on disk and re-filing it would turn one install into two rows. The control case still writes exactly one spool line and zero fallback files. The trim moved into `_telemetry_trim_spool` so the append's success is the last thing in that branch. Inline, the trim sat between the append and the return, which is what made it easy to write the `|| return` that skipped the fallback decision in the first place. Also fixed on the same line: `2>/dev/null` now precedes the `>>`. Redirections apply left to right and a failing `>>` is reported by the SHELL, not by printf, so the old order printed `…/pending.jsonl: Is a directory` — with the customer's path — out of an EXIT trap. 2. LOW — a line number was emitted with no file to attach it to. `tracebloc.install.source` and `tracebloc.install.source_line` were two independent gates over one fact. A location whose basename is outside TB_TELEMETRY_SOURCES dropped the file and kept the number. Reproduced: `/home/someone/evil.sh:9` rendered `"tracebloc.install.source_line":9` with no source key, and so did `?:118` — the shape the ERR trap produces whenever BASH_SOURCE is empty, which is a real installer path, not a synthetic one. A line with no file is not a partial answer; it is a confident wrong one. The gate is now `_telemetry_source_basename`'s own exit status, so TB_TELEMETRY_SOURCES stays the single declaration of what counts as one of our files. Deliberately not gated the other way: TB_ERR_LOC has exactly one writer in the tree (install-k8s.sh:118) and it always appends `:${LINENO}`, so source-without-line is unreachable — a branch for it would be belt and braces no test could redden. A file with no line is honest information anyway. Mutation results — 10 mutations, each asserting its own anchor applied first: M1 both halves return instead of falling through RED M2 only the mkdir half returns RED M3 only the append half returns RED M4 a successful append no longer returns (double file) RED M5 source and line gated independently again RED M5b the same mutation vs the PRE-EXISTING source test RED (it now covers the line) M6 common.sh dropped from TB_TELEMETRY_SOURCES GREEN — survivor, by design M6b cluster.sh dropped (the anchor names it) RED M7a the trim's else-branch cleanup removed RED M7b a failed trim treated as success RED M7c the trim is never called RED M7d a failed mv leaves the trimmed copy behind RED M6 is a deliberate survivor and is reported rather than hidden: the pairing test derives its expectation FROM the vocabulary, so a consistent removal moves both sides. Verified that the gate which owns the vocabulary catches it instead — telemetry-vocabulary-agreement.sh goes red with "source basenames disagrees with gen-manifest.sh's FILES array". The independent anchor at the end of the pairing test also names cluster.sh in its own right, which is why M6b reddens. M7 found a defect in this commit's own first draft. The trim-failure fixture broke `tail` with a PATH shim, and common.sh:8 PREPENDS the system directories to PATH, so the real `tail` ran and both assertions passed against a trim that had worked perfectly. Rewritten to use a shell function — which beats PATH lookup outright — plus a behavioural anchor: SPOOL_MAX=3 against 7 spooled lines must leave 7, or the trim ran after all. scripts/manifest.sha256 regenerated (Static analysis R8). Full suite: 1168 bats tests green. `make lint` clean; telemetry.sh clean under `shellcheck -S warning -x` and `bash -n` on bash 3.2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): a Ctrl-C on the already-set-up screen cancelled nothing (backend#1907) Third Bugbot finding on this branch, reproduced before it was touched. `_assess_handoff` marks the run skipped and then hands the user the interactive `tracebloc` home screen before `exit 0`, with install-k8s.sh:122's `trap 'exit 130' INT` live. The 130/143 branch of telemetry_render_event booked `cancelled` without consulting `_TB_TELEMETRY_SKIPPED`, so Ctrl-C on that screen — the most ordinary thing a user does there — filed a cancelled install for a run that installed nothing. Reproduced through the real INT and EXIT traps, not just the render function, with the skipped latch set identically in both arms: skipped run, ordinary exit 0 -> install.run.skipped skipped run, then Ctrl-C (130) -> install.run.cancelled <- the defect `cancelled` asserts that an install was cancelled. On this path there was no install to cancel, and the row would land in the denominator of "how often do installs not complete" — inflating it with runs that never attempted anything. Same class and same direction as the `--help` bug fixed earlier on this branch: it makes the product look worse while telling nobody anything actionable. No contract change. §6.4's verb list is untouched — no verb added, removed or redefined. Both `skipped` and `cancelled` were already registered and already emitted by this file; only which of the two a given run books changes, and `skipped` is the true one here. A wrong verb from a closed list is an ordinary defect. SKIPPED DOES NOT WIN OVER A FAILURE, and the asymmetry is deliberate. The flag is consulted only on the exits that mean nothing was installed — 0 and the two signals. A skipped run that then dies with a real non-zero stays `failed` and keeps its error.type, so the `*)` branch does not look at the flag. The shorter spelling — hoisting a blanket "skipped wins" ahead of the case — would have hidden a genuine failure, which is why it is not used and why there is a test for it. The comment claiming 130/143 were "unconditional" is now false and was rewritten rather than left to mislead the next reader. Mutation results, each asserting its anchor applied first: N1 130/143 unconditional again (the reported defect) RED N2 130/143 ALWAYS skipped (deletes the cancelled signal) RED <- positive control N3 the flag test inverted RED N4 only 130 consults the flag, 143 forgotten RED N5 blanket skipped-wins hoisted ahead of the case RED N5b only the failure branch consults the flag RED N2 is the control that matters: without it, "renders skipped" would be satisfied by a change that never renders cancelled at all, which would silently delete the interrupted-install signal. N5b exists because N5 tripped an earlier assertion (exit_code) before reaching the failure-swallowing one — it mutates only the `*)` branch, leaving exit_code untouched, and proves that assertion is live rather than decorative. telemetry-vocabulary-agreement.sh still passes unchanged: the case statement gains no new event-name literal, and its (b) sweep already exercised 130 x skipped=1, so the input domain was already derived from the producer. scripts/manifest.sha256 regenerated (Static analysis R8). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(telemetry): allowlist the canary curl -u fixture in .gitleaks.toml (backend#1907) The telemetry canary tests feed a fabricated `curl -u $CANARY:hunter2` through the redaction guards to prove they strip credentials. The value is never a real secret, but a git-mode range scan keeps re-finding it in earlier commits of this branch (commit 72af29e) even after the fixture was refactored, so a code change cannot clear it. Per code-quality.yml, a deliberate false positive belongs in .gitleaks.toml (commit-independent), not the baseline. Scoped to the exact canary match; default rules extended. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 19, 2026
/fr-pass Advanced at @LukasWodka's explicit direction. Held in the automated pass for a verification gap (interactive/TTY, GPU-only, or journey-dependent while e2e journey is red — backend#2206); Lukas is accepting that gap for this card. |

The bash + PowerShell half of tracebloc/cli#516. Companion PR (the Go copy): tracebloc/cli#520 — land both, they are one rule.
The bug
_strip_paste_garbage(scripts/lib/common.sh) andConvertTo-SanitizedInput(scripts/install-k8s.ps1) handled CSI (ESC [ … final) only. SS3 (ESC O final) is what the same arrow / Home / End / F-keys emit once the terminal is in DECCKM application-cursor mode — the state vim, less or tmux leave behind on an unclean exit.SS3 residue was worse than the CSI residue fixed in #362 / tracebloc/cli#364 on 2026-07-21 (not re-litigated here — that fix is correct and stays). CSI residue cleans to empty, so the name prompt's non-empty check re-prompts. SS3 does not:
ESCis dropped as a control byte butOand the final byte are printable, soand the namespace is immutable. Nothing downstream can refuse it — the backend validates DNS-1123 form by idempotence against the slug rule, so escape-derived garbage is a perfectly canonical label. Form is exactly what this input preserves.
The fix — two changes, in each of the two copies here
1. The strip matches CSI and SS3 in one pattern.
2. A post-sanitise floor. The strip knows CSI, SS3 and the paste markers; it cannot know the family nobody has reported yet — and that is exactly how SS3 got here. So: if an ESC survives the strip, the value carries a shape we do not recognise, and it must show one alphanumeric that did not come from an escape final byte. The probe is
ESC+ intermediates + at most two final-class bytes; its output is a yes/no and is never returned. Nothing but residue emits empty — which every caller here already treats as "no answer":provision.shre-prompts,cluster.sh's_read_sanitizedyields an empty var,_sanitize_credentialwarns.Scoped to "an ESC survived the known families" rather than the ticket's "cleaned ≠ raw and remainder under N chars": "cleaned ≠ raw" fires on every ordinary paste and every stray tab, so an N large enough to catch residue also rejects short real names. This trigger needs no magic number — a clean value never reaches it, real content beside an unknown escape is kept, and only a value that is nothing but residue is refused.
Three things the tests caught that review did not
The C locale. The bash floor checks for content with
LC_ALL=C tr -dc '0-9A-Za-z\200-\377', not[[ "$probe" =~ [[:alnum:]] ]]. Bash's regex engine is locale-dependent, and under the C locale the installer often runs in,[[:alnum:]]does not match a UTF-8 letter — so the first draft auto-named a perfectly good日本the moment an unknown escape sat next to it. Keeping every byte ≥ 0x80 makes the question locale-independent. (.NET's regex is Unicode-aware, so the PowerShell copy uses[\p{L}\p{Nd}].)A hang. The probe's first draft copied the strip's
while [[ $s =~ $pat ]]; do s="${s/${BASH_REMATCH[0]}/}"; doneshape. Pattern substitution treatsBASH_REMATCHas a glob, not a literal. That is safe for the CSI loop by construction — its match isESC '[' [0-9;]* <final>, which can never contain a], so it can never form a complete bracket expression. The probe's pattern has[^A-Za-z0-9~]*in the middle, which can swallow a]: onESC [ ; ] Athe regex matches the whole value, the glob<ESC>[;]Athen meansESC ';' 'A'— not present — the substitution removes nothing, and the loop never terminates. A hang at the installer's name prompt, on a value the floor exists to refuse. Replaced with a singleLC_ALL=C sed -Epass: no glob semantics, no loop.An encoding-dependent verdict (Bugbot, Medium, resolved). The probe's final-byte run was unbounded, so
\x1bNChellohad the whole name swallowed and was refused while\x1bNC日本was kept — keep-vs-reject depending on the script the user's name is written in. Now{1,2}: one is too few (it leaves theDof an unrecognised SS3-shaped pair behind, and the floor stops firing on the very shape this PR is about), unbounded is too many. An escape final is one byte, an intro plus a final is two, and every keyboard-input family fits in that.Mutation evidence
Five anchors, each applied to
common.sh, each confirmed to redden, each restored:if false && …)trcheck swapped for=~ [[:alnum:]]sedpass reverted to the glob loopESC [ ; ] Atest (status 142 under a localtimeoutstand-in — the call never returns){1,2}→+Worth stating rather than hiding: the "SS3 arrows only → empty" case stays green under the first mutation, because the floor catches it too. It is kept as the ticket's documented repro; the SS3 strip itself is carried by the mixed-content cases, which the floor cannot mask.
Tests
scripts/tests/install-client-helm.bats: 13_strip_paste_garbagecases (was 2). The floor cases use SS2 (ESC N <final>) as a stand-in for "the next family" — genuinely not matched by the strip, so they exercise the floor and nothing else. The hang test is bounded the waycommon.batsbounds its recursion guard; macOS ships notimeout(1), so Linux CI is the authority on that half.make check— greenmake bats— 1074 ok, 0 not okmake check-all— green (adds helm-template + 465 helm-unittest)pwsh -NoProfile -Command "Invoke-Pester scripts/tests/ -Output Normal"— 672 passed, 0 failed, 13 skippedscripts/gen-manifest.shre-run —common.shandinstall-k8s.ps1are both in the bootstrap's integrity surface, soscripts/manifest.sha256is updated here.Two things this PR does not do, stated plainly
1. No committed PowerShell test for the new behaviour.
ConvertTo-SanitizedInput's SS3 + floor behaviour was verified against the same 18-case corpus as the other two implementations (18/18), and the existing Pester suite is green with the change in — but its committed cases still cover only CSI, becausescripts/tests/install-k8s.Tests.ps1was outside this change's scope. That is a real hole, not an oversight, and it is the first sub-task on the follow-up below.2. No shared fixture (tracebloc/cli#516 item 4). The ticket is right that three hand-maintained copies with no shared corpus is why all three missed SS3 at once, and I built exactly that corpus while doing this. I did not land it, for two reasons: it spans two repos, so it needs a vendoring + drift-check mechanism (the shape of
scripts/tests/check-drift.sh) that is larger and riskier than the fix; and a fixture wired into two of three implementations, with the third still hand-maintained, is precisely the "appears to verify something, is not connected to what it claims to check" pattern. Ship it to three or not at all.Follow-up: tracebloc/backend#2084. Until it lands, all three copies now carry a "change all three together" pointer to the other two.
Note
Medium Risk
Changes provisioning/name sanitization on the install path—wrong behavior could block valid names or still mint bad namespaces—but scope is localized to escape stripping with broad bats coverage and empty-input handling already wired in callers.
Overview
Fixes cli#516 by aligning
_strip_paste_garbageincommon.shandConvertTo-SanitizedInputininstall-k8s.ps1with the Go sanitizer: the strip regex now removes CSI and SS3 (ESC O+ final) sequences, not CSI alone. SS3 is what arrow/Home/F-keys emit after vim/less/tmux leave DECCKM mode; leftoverO/D/Abytes had produced plausible garbage names (e.g.ODOA) and immutable namespaces, unlike CSI residue that cleaned to empty and re-prompted.Adds a post-strip floor: if an
ESCstill remains, the value is probed (bounded{1,2}final-byte removal) for real alphanumeric content. Bash uses a singlesedpass plusLC_ALL=C trsoESC [ ; ] Acannot hang the name prompt via glob-basedBASH_REMATCHsubstitution, and non-Latin letters still count as content. Escape-only input returns empty so existing callers re-prompt or fail closed.Tests in
install-client-helm.batsgrow from 2 to 13 cases (SS3, floor, hang, locale/encoding).manifest.sha256is updated for the touched bootstrap scripts.Reviewed by Cursor Bugbot for commit b29d549. Bugbot is set up for automated code reviews on this repo. Configure here.