release-train: staging -> main - #557
Merged
Merged
Conversation
…eep Decision A (backend#2220) (#538) * refactor(resources): read the envelope constants from the contract, keep Decision A (backend#2220) Third and last reader in the P0 consolidation. client-runtime#358 made the training-envelope arithmetic single-definition and client#766 made both installers readers of it; this does the same for set.go. Deleted: the four literals (1-CPU/3-GiB overhead, 1-core/2-GiB floors). They now come from internal/resources/envelope_contract.json, vendored from client-runtime, whose arithmetic is node_sizing.envelope_from_allocatable. cli#143 Decision A is AMENDED IN SCOPE, NOT DEVIATED FROM. The number the user sets is still the per-run ceiling written to RESOURCE_* verbatim; DeriveTraining is still the identity on it; Overhead() is still a fit-check safety margin that is never subtracted. Every exported signature is unchanged, and the existing suite passes untouched. What went away is the duplicate *definition* of four numbers, which was never part of that decision. Amendment recorded on cli#143. Worth naming what the duplication actually cost, because it was not only tidiness: LargestReadyNode/nodeLarger ranks candidates (cpu, memory) while the bash installer ranked them (memory, cpu). On a cluster of 8c/16Gi + 4c/32Gi `resources set` and the installer anchored on DIFFERENT nodes and gave different answers about one machine. Nobody chose that; it fell out of two independent implementations. The contract records one order, and this repo's was the one kept -- it is what the user is shown when the wizard clamps their prompt. Go needs no generator, unlike the installers: go:embed takes the contract verbatim at compile time, so the vendored artifact is byte-identical to upstream and the cross-repo gate is a plain diff. The constants became small funcs rather than package vars for the same reason Overhead() is a func -- a var would be mutable from anywhere, and the point is that one place decides. mustContract panics on a malformed contract, MustCompile-style: the file is embedded at COMPILE time, so it can only be invalid via a hand-edit or a botched re-vendor -- a broken build, not a runtime condition. A default here would be a fifth policy. TestContractIsValid keeps the panic out of a release. The drift gate FAILS CLOSED when it cannot read upstream, per cli#536 -- a check that never executed must not report as passing. client-runtime is private, so it mints a least-privilege App token (named repositories, contents:read) instead of GITHUB_TOKEN. It also re-derives upstream's vectors from upstream's own generator: a vendored contract can match upstream byte-for-byte while UPSTREAM's goldens have gone stale against upstream's code, and mirroring that faithfully is still wrong. Verified: go build + go vet clean, go test ./... all packages ok, gofmt and goimports clean, go.mod/go.sum untouched. Mutation-tested: overhead 3GiB -> 4GiB in the vendored contract reddens 5 tests here, two of them PRE-EXISTING (TestOverhead_IsOneCoreThreeGiB, TestMaxRunCoresAndGiB) -- so the existing suite already anchored these numbers independently, which is the ticket's DoD holding in a consumer repo. Refs: RFC-BACKEND-664 P0, client-runtime#358, client#766, cli#143, cli#536 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(resources): drop the unreachable ContractVersion export (deadcode gate) The Lint job's deadcode check flagged it: exported, but unreachable from ./cmd/tracebloc. I had added it "for doctor output" and then never wired it into a command, which is exactly the shape that gate exists to catch — an export with no caller reads as API surface and gets maintained like it. The tests are in-package, so they use mustContract().ContractVersion directly and lose nothing. Surfacing the contract version in `doctor` may well be worth doing (a binary and an edge disagreeing about the contract is a real field question), but it belongs in the PR that touches the read path, not as a stub here. Not touched: the pre-existing stale allowlist entry for internal/doctor/doctor.go Status.String. It is reported on clean develop too and is a warning, not a failure — deadcode-check.sh still exits 0. Pruning it is someone's cleanup, not this PR's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: shujaat hasan <shujaathasan@shujaats-MacBook-Pro.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
`Installer (shell)` is a REQUIRED status check on develop, and its first action was
an `apt-get` with no retry and no time bound of its own. A slow package mirror
therefore consumed the whole 10-minute job budget before any shell was parsed, and
blocked every PR in the repo while doing it.
MEASURED, not theorised. cli#533 is a workflow-only diff that cannot touch installer
behaviour, and it failed FOUR consecutive times:
job 96126585157 Installer (shell) failure 10m16s
15:34 Set up job
15:34 Run actions/checkout
15:34 shellcheck + dash parse <- 10 minutes here, then killed
15:44 Post Run actions/checkout
Nothing after the `apt-get` line ever ran. And the annotation read `The job has
exceeded the maximum execution time of 10m0s` on a job called `Installer (shell)`,
so whoever sees it reasonably concludes the installer is hanging. Nothing points at
apt.
NOT REPO-WIDE, which is worth stating because the ticket first implied it: #530 and
#526 pass the same check. It reproduced on one head, four times.
THE FIX REMOVES THE DEPENDENCY RATHER THAN HARDENING IT. Both tools are already on
`ubuntu-latest`:
* shellcheck is preinstalled -- tracebloc/.github's own `quality / shellcheck`
job, a REQUIRED check in 16 repos, calls `shellcheck --version` with no install
at all. The org has depended on that fleet-wide for as long as that job existed.
* dash IS Ubuntu's /bin/sh, an essential package.
A retry-with-timeout around apt would have been the smaller diff and the worse fix:
a step that installs nothing cannot stall on a mirror, and no wrapper can say that.
`shellcheck --version | head -2` is kept as the first line, matching what the org's
own shellcheck job does -- so the version in use is in the log, and an absent binary
fails on line one with an obvious message instead of somewhere further down.
THIS PR'S OWN RUN IS THE PROOF, and that is deliberate: if either tool were missing
the step fails loudly here, before merge. Better than any claim in the comment.
Verified locally too: shellcheck --shell=sh --severity=error scripts/install.sh
clean, dash -n scripts/install.sh clean.
Closes #534.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…0) (#539) * feat(resources): stamp `resources set` as a human choice (backend#2220) Companion to client#768, and the half that makes the marker mean anything. client#768 taught the installer to write env.RESOURCE_PROVENANCE (installer | user | unknown). Without this side, an edge the installer marked `installer` and the operator then re-sized by hand would KEEP saying `installer` -- a deliberate human choice wearing the one label that invites a future ladder to overwrite it. That is strictly worse than no marker at all, so the two PRs are only correct together. BuildEnvSpec now writes RESOURCE_PROVENANCE=user unconditionally. Unconditional for the same reason NoGPUEnvValue is: the apply runs `helm upgrade --reset-then-reuse-values`, which re-applies the release's stored values on top of chart defaults, so an omitted key is silently RE-INHERITED rather than cleared. There is no branch on which omitting it would be safe. `tracebloc resources set` is by definition the human choice -- there is no variant of running it that isn't. Read side: Training.Provenance, normalised through NormalizeProvenance, which maps anything unrecognised -- empty, junk, a value this binary predates, wrong case -- to `unknown`, never to a guess. Returning "" would have made callers branch on empty and invent their own default, which is a second policy and exactly what this ticket removes. Callers MUST treat `unknown` as a human choice: it means we do not know, and guessing `installer` risks overruling an operator. Surfaced in `resources --verbose` as "set by", not in the default view: it answers a support question ("did someone set this, or did we?"), not one an operator needs on every run, and a line of bookkeeping that never changes the numbers should not grow the default output. `unknown` renders with its explanation rather than the bare word, which invites the wrong conclusion -- it does not mean something is broken. The env map already flows into helm.UpgradeParams.Env, so no new plumbing. The added apply-boundary test starts from an installer-marked edge specifically, because the unit test proves the map is right while this proves the map arrives -- and a marker that never lands looks identical to one that did, with the consequence surfacing much later. zz-all-strings.golden regenerated: one line, "set by", reviewed. Verified: go build + go vet clean, go test ./... all packages green, gofmt and goimports clean, deadcode exit 0. 6 new tests in internal/resources (including a set-then-read round trip) + 1 apply-boundary test in internal/cli. Refs: RFC-BACKEND-664 P0, client#768, client-runtime#358, cli#538 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(resources set): a same-size set must still stamp `user` (Bugbot + review, #539) Bugbot's High, confirmed by @saadqbal in review, and the fix was indeed sitting three lines below the bug. BuildEnvSpec stamps RESOURCE_PROVENANCE=user unconditionally WITHIN ITSELF, but the caller could exit before ever reaching it: `sameCeiling` returned "nothing to change" whenever the desired ceiling matched the current one. So an installer-sized edge whose operator ran `resources set --max`, or passed flags restating the current ceiling, kept `installer` -- a deliberate human choice wearing the one label that invites a future ladder to overwrite it. That is verbatim the state BuildEnvSpec's own comment calls the most dangerous the marker can be in, and my PR body claimed this PR and client#768 "are only correct together" while this hole made that untrue: not wrong, just narrower. Fixed with the phantom-GPU branch as the template -- same condition, same reason, already written and already reviewed. An unchanged ceiling now falls through to persist when the stored marker is not already `user`. Both conditions can hold at once, so both report their reason rather than one masking the other. `unknown` counts as stale deliberately. A pre-marker edge whose operator restates the ceiling has just made that size explicit, so recording it is the honest answer; the cost is one extra apply per edge, exactly once, because the second run sees `user` and is a clean no-op again. Three existing no-op tests now carry RESOURCE_PROVENANCE=user in their fixtures (TestSet_NoOpSkipsApply, TestSet_NoOpEvenWhenCurrentNoLongerFits, TestWizard_LeaveAsIs). Their invariant is unchanged and still asserted -- a clean no-op makes no helm call -- it just needs the marker present to BE the clean case. The uncovered case became TestSet_SameCeilingStampsProvenance, a table over installer / pre-marker / junk, asserting the apply happens, that we do not claim nothing changed while correcting the marker, and that the reason is stated. zz-all-strings.golden regenerated: one line, reviewed. Also rebased onto develop now that cli#538 has merged -- `git rebase --onto origin/develop 05517ed` to drop the squashed parent's commits rather than replay them into conflicts. GitHub had already retargeted the base to develop when #538 landed, so the stacked-PR banner in the description was stale; removed. Verified: go build + go vet clean, go test ./... all packages green, gofmt and goimports clean, deadcode exit 0. Refs: backend#2220, client#768 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(release): bump VERSION to 0.10.10 v0.10.9 is already released and this PR changes published files under internal/*, so the release train needs a version above every released tag — version-bump-gate fails otherwise (backend#1561). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: shujaat hasan <shujaathasan@shujaats-MacBook-Pro.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…#2217) (#542) * feat(telemetry): the CLI validated events and delivered none (backend#2217) `pendingSink()` returned nil, so every command outcome was validated and dropped. The ingest endpoint exists now (backend#1905) and speaks OTLP/HTTP JSON (backend#2213), so this is the delivery half. Option (c), as decided on the ticket: one inline POST with a ~1s budget for the whole step, spool to disk on any failure, drain at most 20 pending on the next invocation. No background daemon, no goroutine outliving the process, no retry loop -- the next command IS the retry. The deciding argument against fire-and-forget is that in a short-lived CLI async delivery is a lie: a goroutine outliving main() is killed at exit, so the honest options are "always block" or "always drop on failure", and dropping on failure discards exactly the partition-time events worth having. Three things worth a reviewer's attention: 1. `anyValue` switches on reflect.Kind, not on concrete type. The emitter's `checkAttrValue` deliberately admits named scalar types (`type Reason string`, time.Duration via telemetry.Duration) by kind; a `switch v := value.(type)` at the seam would match the dynamic type, miss those, and silently drop the values the layer above went out of its way to accept. 2. A 4xx other than 401/403/408/429 DISCARDS the batch. The endpoint answers 400 for a wholly unparseable batch; re-spooling that would wedge the spool forever, re-sent by every future command and pushing good records out at the cap. 401/403 retry because they are a credential state, not a payload verdict -- the next login makes them deliverable. 3. The spool keeps drop-OLDEST, which is not a contradiction of D7's amended drop-newest row. That row says drop-newest because `exporterhelper` sheds at the entrance and offers nothing else -- a platform constraint on the edge Collector. This spool is our own code and can do what D7 originally wanted, so it does, matching the installer's `tail -n` trim. `deliver` takes a resolved URL rather than an env. Found by its own test: with `api.BaseURL(env)` computed inside, the test posted to PRODUCTION. Passing the URL also means the record's label and its destination cannot disagree. No `timeUnixNano`. Per the #2213 decision no client-side timing is sent, so the receiver stamps arrival and "how late" is knowingly invisible; adding an event clock is a contract change, not a drive-by here. Verified: `make check` green (vet, full test suite, fmt, file-budget, style, tool-pins) and `make lint` clean (errcheck, ineffassign, misspell, staticcheck). Nine mutations run against the nine new assertions -- collapse the resourceLogs entries, encode int64 as a number, swap reflect for a type switch, invert the trim, retry a 400, send the legacy `Token` keyword, widen the spool to 0644, drop on retry, drop when unauthenticated -- all nine killed their test, none survived, none merely broke the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): the spool round-trip recoded integers as doubles (#542 review) @saqlainsyed007 and Bugbot, both right, and it defeated this PR's own wire-mapping guarantee on the path the design exists for. `readSpool` used `json.Unmarshal` into `map[string]any`, so every JSON number came back as float64 and `anyValue` routed it to `doubleValue`. The in-memory attempt sent `exit_code` as a canonical `intValue` string; every DRAINED retry -- the partition-time events the whole spool exists to preserve -- sent the same field as `doubleValue`. Same event, two encodings, wrong one on the path that matters. Reproduced before fixing rather than argued from the code: in-memory : "tracebloc.cli.exit_code":{"intValue":"2"} round-trip: "tracebloc.cli.exit_code":{"doubleValue":2} Fix is the one suggested: decode with `dec.UseNumber()` so numbers arrive as `json.Number`, and handle that in `anyValue` -- `intValue` when integral, `doubleValue` otherwise. It also keeps the spool file human-readable, which was a stated goal. THE ORDERING IN `anyValue` IS LOAD-BEARING, and this is the part worth reviewing: `json.Number` is a NAMED STRING TYPE, so the existing kind switch would have matched `reflect.String` and emitted `stringValue` -- turning an exit code into a string on the drained path, a different wrong answer from the float64 one. The json.Number branch therefore sits before the switch, not inside it. Why the existing test stayed green, since that is the reviewable lesson: `TestIntegersAreEncodedAsStrings` operates on the in-memory shape and never goes through writeSpool/readSpool. So it could not see this, which is the vacuity the house bar rejects. Two tests now cover the drained path: * `TestSpoolRoundTripPreservesIntegerEncoding` asserts the two encodings AGREE, not merely that the drained one looks right -- a test checking only the drained payload would be satisfied by both paths being wrong together. It also asserts the in-memory reference explicitly, so a regression there cannot make the comparison pass by both sides breaking. * `TestSpoolRoundTripKeepsRealsAsDoubles`, because a fix for integers that truncated every float would pass the first test. One note on the first draft of that test: it asserted "no doubleValue anywhere in the drained payload" and failed on CORRECT output, because the fixture carries `sampling_rate: 0.5` which must stay a double. Narrowed to per-attribute assertions -- an over-broad assertion reported as a code bug is its own defect. Verified: `make check` green (vet, full suite, fmt, file-budget, style, tool-pins), `make lint` clean. 13 mutations now, all killed: the original nine plus dropping `UseNumber` (the reported defect), letting json.Number fall through to the kind switch, routing integral numbers to doubleValue, and truncating reals to integers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(telemetry): the spool leaked records across environments (#542 review) Bugbot, and it reopened one level up exactly the mismatch `deliver` takes a resolved URL to prevent. A single host-wide `pending.jsonl` meant records queued while signed in to one backend were drained by the NEXT invocation against another -- POSTed to its endpoint, with its token, still carrying the first one's `deployment.environment`. `tracebloc login --env dev` after a failed prod command was enough. Reproduced before fixing: a `deployment.environment=prod` record arrived at a dev endpoint with a dev token. The spool is now per environment, `pending-<env>.jsonl`, and `deliver` takes the resolved spool PATH rather than deriving one -- so label, spool and destination are three views of a single resolution in `pendingSink` instead of three chances to disagree. That is the third reason on that function's doc comment, and the second and third each bit once. The consequence is stated in the code rather than hidden: records for an environment the operator never uses again are never delivered. That is the right trade -- they are capped, there is usually no token for that environment anyway, and delivering them to the WRONG backend is not a better outcome than not delivering them. `spoolEnvSlug` strips anything outside [a-z0-9-] and lowercases. Unreachable with the closed dev/stg/prod set, and present because a path segment built from a string is a traversal waiting for that set to open; an unexpected value becomes one bucket rather than being dropped, since an undeliverable record is still evidence. The leak test asserts BOTH halves -- nothing prod-labelled reaches the dev endpoint, AND the prod spool still holds its record afterwards -- because a fix that simply discarded the other environment's queue would satisfy the first assertion alone. Verified: `make check` green, `make lint` clean, 16 mutations all killed (the original nine, the four from the int-encoding fix, plus reverting to a host-wide spool, and the slug ceasing to strip separators or normalise case). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…d#2217) (#545) Completes backend#2217. The CLI half landed in #542; this is the installer half, built as option (b) rather than (a). WHY NOT (a), THE ROUTE THE TICKET ASSUMED. #2217 says "convert at the seam and POST", which presumes the installer can authenticate. It cannot: the ingest endpoint needs `Token`/`Bearer` under `IsAuthenticatedEdge`, and the installer holds `TRACEBLOC_CLIENT_ID`/`TRACEBLOC_CLIENT_PASSWORD` -- a provisioning pair with no exchange for a token -- and never reads the CLI's config (zero hits across `scripts/`). Having it read `~/.tracebloc/config.json` would also deliver NOTHING for the failures that matter most: `validate_config` and `early_data_dir_guard` run before provisioning, so no token exists on disk when those events are written, and those are exactly what the installer's `$TMPDIR` fallback was built to preserve. So the CLI carries them. It already owns the token and, since #542, the spool, drain loop and OTLP mapping -- this adds one more file to read and leaves the installer with no credential handling at all. THE UNPREDICTABLE FALLBACK PATH TURNED OUT NOT TO NEED AN INDEX FILE. I had expected to need one; `_telemetry_fallback_spool` uses `mktemp .../tracebloc-telemetry-XXXXXX`, so the NAME is unpredictable but the PATTERN is fixed. A glob over $TMPDIR / $HOME / /tmp finds them, the installer needs no change, and there is no shared state to keep in step. EVERY RECORD IS FILTERED BY ITS OWN ENVIRONMENT, and this is the part to review. Our spool is partitioned by env in the FILENAME; the installer's is not, and its records carry whatever CLIENT_ENV that run used. Forwarding blind would post a prod-labelled install failure to whichever backend this invocation points at -- the same leak #542's second finding was about. A record ships only when its `deployment.environment` matches this run's; the rest stay for a later invocation against that env. A record with NO environment is never forwarded (the contract omits rather than empties, so absent means unresolvable) but is also never dropped: it is still evidence. Their files are touched only on a path that CONSUMED them, and never on failure or when unauthenticated. A file we took nothing from is not rewritten at all. Verified: `make check` green, `make lint` clean, 16 mutations across the three suites all killed. One of the new nine earned its keep: it proved `TestInstallerRecordsSkipsAFileWithNothingForUs` VACUOUS -- it compared file bytes, and a rewrite of unchanged records produces identical bytes, so it passed under the mutation it existed to catch. It now asserts the real contract (the file never enters `remainder`) and the mutation kills it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…#544) * chore(shell): drop the pipe into early-closing readers (backend#2264) Seven pipelines fed a reader that closes before EOF (`| grep -q`, `| head -1`). Under `set -euo pipefail` that shape can return 141: the reader closes, the producer takes SIGPIPE, pipefail surfaces the signal as the pipeline status. MEASURED, none of the seven is reachable. Every producer here is a bash builtin `printf` emitting a single short token ("$min", "$max", "$line", "$BACKEND_REF"), and a single-line producer cannot SIGPIPE a `grep -q` anyway -- grep must read to end-of-line before it can report the match, so -q drains the input regardless. This is a shape conversion so the shared gate can be armed with the repo already green, NOT a bug fix. scripts/coverage-floor.sh 2 sites scripts/file-budget.sh 1 site scripts/sync-backend-fixtures.sh 2 sites scripts/sync-schema.sh 2 sites Verified behaviourally, both directions, on the converted conditions: ref validators REJECT (exit 2) ../../etc/passwd, a..b, -badstart, 'x;rm -rf /', 'a b' ACCEPT main, abc123/def-1.2_3, a 40-char SHA malformed guards catch 'pkg' (no colon), 'pkg:abc', 'pkg:12x', 'pkg:-5' for BUDGETS and FLOORS alike; a well-formed entry still reports "ok: internal/cli/data.go 248 <= 500" pct extraction still reads a real number: "ok: ./internal/cli/ 85.0% >= 1%" Each fixture was asserted to actually apply before its run, so an inert mutation cannot be mistaken for coverage. `make file-budget` and `make check-style` pass; shellcheck -S warning clean across scripts/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: restore the pinned fixtures my own verification overwrote (backend#2264) Self-inflicted, and worth naming precisely. To check that the converted ref validators still ACCEPT valid refs, I ran DATA_INGESTORS_REF=main bash scripts/sync-schema.sh BACKEND_REF=develop bash scripts/sync-backend-fixtures.sh These scripts are not predicates. On a valid ref they do the sync -- they downloaded from unpinned refs and rewrote four tracked files, and `git add -A` swept them into the conversion commit: internal/schema/ingest.v1.json (+79 lines from main) internal/api/testdata/edge_device_adopt.json ("status": 0 -> 2) internal/api/testdata/edge_device_create.json internal/api/testdata/edge_device_patch_cluster_id.json That is a silent supply-chain change to PINNED content, which is exactly what `.data-ingestors-ref` and `.backend-ref` exist to prevent, and it is what reddened Schema drift, Backend fixtures drift, Test and Integration (kind). The drift checks did their job. Restored all four from origin/develop. The PR is back to the four script changes it claims to be. The lesson is the harness, not the scripts: a verification step that INVOKES a mutating command is not a read-only check, and `git add -A` after running one cannot tell the difference. The rejection cases (exit 2 before any network call) were safe; the acceptance cases were not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… (cli#546) (#547) Bugbot MEDIUM on the develop->staging promotion mirror (#540), against #539 (backend#2220). #539 made any non-`user` RESOURCE_PROVENANCE stale, so an unchanged ceiling stopped returning early and instead fell through to the apply in order to re-stamp the marker as `user`. Correct intent, wrong landing site: the fall-through lands in the confirmation gate, and off a terminal that gate does not ask -- it returns exit 1. So tracebloc resources set --cores 4 --memory 16 # the CURRENT ceiling went from the exit-0 no-op the command's own --help documents ("0 applied (or nothing to change)"), and that docs/cli-navigation.md draws as an edge going straight to exit 0 bypassing CONF, to a hard failure. Nearly every installed edge still reads `installer` or `unknown`, so the blast radius was the installed base rather than an edge case, and the callers that restate a size are scripts -- the bootstrap, the end-to-end journey -- none of which pass --yes for what the docs promise is a no-op. The fix is one clause on the gate, not a revert: the gate guards the CEILING, and an unchanged ceiling has nothing to ask about ("Let each training run use up to 4 CPU / 16 GiB?" when the answer is already that is a question with one honest answer). The re-stamp #539 added still happens. The phantom-GPU fall-through (#241) had the same shape and is fixed by the same clause: both are bookkeeping writes, already announced by their own Infof lines, not budget changes an operator needs to sanction. Tests: TestSet_SameCeilingNeedsNoYes. Every same-ceiling case already in the file passed `yes: true` -- the flag under dispute -- which is why nothing was red. The new cases assert exit 0 AND that the apply still happens, because either alone is satisfiable by the wrong fix; they read RESOURCE_PROVENANCE=user off the values file helm was actually handed (the existing assertions go through --dry-run, which skips the very gate at issue); they cover the phantom-GPU sibling and a declining prompter on a terminal; and one sub-case bounds the fix by proving a real CHANGE off a terminal still exits 1 and mutates nothing. Five of the six sub-cases fail without the one-clause change. docs/cli-navigation.md: the `no change` node now says it may still re-stamp provenance or clear a phantom GPU, and never asks to confirm -- true of #539's behaviour too, which shipped without updating the map. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… ~/.tracebloc (backend#2314) (#548) * fix(delete): stop the exit-path telemetry write re-creating the wiped ~/.tracebloc (backend#2314) `tracebloc delete` printed "✔ Removed local tracebloc data and config." and then put the directory back before the process exited, so the offboard's central promise was not kept. main.go emits the command-outcome event AFTER the command tree returns, and the telemetry spool lives at <config.Dir()>/telemetry/pending-<env>.jsonl — inside the tree the offboard just removed. Two separate defects combined: * writeSpool called MkdirAll BEFORE its len(events) == 0 early return, so it created the directory even when it had nothing to write and was about to delete the spool file. This is why the tree came back on the DELIVERED path too, not just offline. * Nothing told the exit-path write that this invocation had deliberately removed local state, so on the undelivered path it wrote a real event file back into the wiped tree. That is the path the offboard always takes: the wipe takes the token with it, so deliver() finds no credential and spools. removeHostDataDir now returns the directory it removed and the offboard records it, so writeSpool drops any write that lands inside it. The recorded value is the PATH, not a boolean: a bare "telemetry is off" flag silences writes the offboard never touched, and is permanently sticky inside a test binary — three unrelated spool tests failed exactly that way while this was being written. Delivery over the network is untouched: an online offboard still reports its outcome. Only the on-disk fallback is suppressed, and a dropped telemetry record is the cheaper loss against silently undoing a wipe the user asked for. Regression coverage in telemetry_transport_test.go, verified load-bearing by reverting each half independently. This is the only failing assertion in the `Offboard teardown (k3d)` e2e, red on develop since c246912. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci(e2e): the offboard suite depends on internal/cli/telemetry*.go too (backend#2314) The paths probe gates `Offboard teardown (k3d)` on the black-box run's dependency surface, and the telemetry transport was missing from it. That is the same gap the filter's own comment records for internal/ui after #367: the command-outcome event is emitted from main.go AFTER the offboard returns, and its spool lives inside the ~/.tracebloc the offboard just deleted, so a telemetry change re-created the wiped tree and broke the suite's config-dir assertion without touching delete.go. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…cli#549) (#550) * fix(fmt): the formatter gates walked the working tree, not the repo (cli#549) `make fmt-check` ran `gofmt -s -l .` and `goimports -l .`, and `.` is the whole working TREE. Any untracked directory holding Go files — a nested git worktree, a vendored copy, a build sandbox — was reported as drift while every tracked file in the repo was correctly formatted: ==> goimports (import grouping) needed on: <untracked-scratch-dir>/internal/cli/data.go ==> run `make fmt` to fix CI never saw it, because a fresh checkout has no untracked Go files. So this was a local-only FALSE failure in `make check`, the documented pre-push tier — and the remedy it printed was the same bug in write mode: `make fmt` (`gofmt -s -w .`) rewrote files the repo does not track, i.e. someone else's working copy. Both invocations, check and write, now take the file list from `git ls-files '*.go'` — exactly the set a PR can contain. Notably this was the ONLY gate affected: Go's `./...` skips dot-prefixed directories, so `vet`, `test` and `deadcode` never saw `.claude/` at all. `gofmt .` does not skip them, which is why fmt-check alone cried wolf. scripts/format.sh, rather than more backslash-continued shell in the Makefile, because the edge cases want testing and comments: * xargs over a NUL stream from a printf builtin, not a bare argv expansion. Measured: 9000 tracked files is 2.0 MB of argv against a 1 MB ARG_MAX — the naive form dies with "argument list too long", the batched form reports all 9000. * tracked-but-DELETED index entries are filtered out. `git ls-files` reports the index, so a mid-edit deletion would otherwise hard-error the gate. * FAILS CLOSED (exit 2) outside a git work tree, and on an empty file list. The empty case is not cosmetic: bare `gofmt -l` with no path arguments reads STDIN, so an unguarded empty list checks nothing and exits 0 — the inert-verification class of backend#1729. * stderr is deliberately not captured; `go run`'s download progress would otherwise appear as phantom drift filenames on a cold cache. build.yml's Lint job calls `make fmt-check` instead of keeping its own inline copy, so the file set has one definition and cannot drift from local. That also drops the restated `goimports@v0.48.0` pin, and GOIMPORTS_VERSION joins check-tool-pins.sh's TOOLS so it stays dropped on the next bump. Verified on this branch, with a `.claude/worktrees/` directory present holding deliberately misformatted Go: * `make check` — green (was red on the same tree before this change; the old `gofmt -s -l .` flags the scratch file, shown in the ticket) * tracked drift still caught, both gates: a non-simplified slice expression in internal/slug/slug.go fails gofmt -s, a mis-grouped import fails goimports * `make fmt` leaves the scratch file byte-identical (sha unchanged) * fail-closed paths exercised: no work tree, empty list, bad usage — all 2 * a deleted tracked file passes at 218/219 rather than erroring * shellcheck (default severity, not just -error) clean; actionlint clean Refs cli#549. Found while working on cli#548, kept out of it to keep that PR scoped to the offboard telemetry bug (backend#2314). * fix(fmt): check mode swallowed a formatter failure and called it clean (cli#549) Bugbot High on scripts/format.sh:90 and @LukasWodka on #550, both correct, and the defect was the same class this PR exists to fix — in the PR's own new code. `run_formatter` ended with `exit 2` on a non-zero formatter. Write mode called it directly, so that killed the script. Check mode captured it: drift="$(run_formatter "gofmt -s" gofmt -s -l)" A function that exits inside a command substitution ends only the SUBSHELL. With `set -uo pipefail` and no `-e`, the parent read an empty `drift`, found no drift, left `fail` at 0, printed ==> fmt-check: 219 tracked Go file(s) clean and exited 0. So a cold `go run` cache, a network blip, a bad GOIMPORTS_VERSION pin or any non-zero gofmt produced a FALSE GREEN in `make fmt-check` and in CI's Lint step, with the true `FAILED (exit N)` line going to stderr where nothing acted on it. Reproduced standalone before changing anything: f() { echo boom >&2; exit 2; } out="$(f)"; echo "PARENT STILL RUNNING; out=[${out}]" -> boom / PARENT STILL RUNNING; out=[] / parent exit=0 `run_formatter` now writes the formatter's stdout to a temp file and RETURNS the status; callers are `run_formatter ... || exit 2` in the parent shell. It never exits and is never wrapped in `$( )`. A comment cannot hold that shut, so scripts/tests/format-verify.sh does: eight properties, formatters stubbed on PATH and via $GO, hermetic, no network, ~6 s. It asserts the failure propagates in BOTH modes, that the untracked file never reaches a formatter while the tracked one does, that tracked drift still exits 1, and the three fail-closed paths. Proof it is not inert — the harness run against a copy of format.sh with the old subshell shape restored: FAIL: a failing formatter exited 0 (check mode swallowed it — the #550 defect) format-verify: 1 FAILED, 7 passed and against the fix: `format-verify: 8 properties hold`. Wired into `make check`, `make ci`, and build.yml's Installer (shell) job — that job, not Lint, because stubbed formatters need no Go toolchain. `make check` is 10.6 s warm with it, against the 60 s budget. Fixing the harness cost one round of the same trap: `rc="$(run_case ...)"` lost the CASE_OUT the function had set. Call sites are direct now, with a comment naming why, since it is the identical mechanism. Re-verified after the refactor: `make check` green with a `.claude/worktrees/` scratch dir present holding misformatted Go (the old `gofmt -s -l .` still flags it), tracked gofmt drift still exits 1, `make fmt` leaves the scratch file byte-identical, shellcheck clean on both scripts at default severity, actionlint clean. Refs cli#549. * test(fmt): pin each propagation path, not "at least one" (cli#549) @LukasWodka on #550, and he mutation-proved it rather than asserting it: swallowing ONLY the gofmt check-mode call site left format-verify green. The `explode` stub failed BOTH formatters, so whichever call site still had `|| exit 2` carried the script to a non-zero exit and the case read that as success. What the suite actually asserted was "at least one propagation path works" — or-coverage under a name that promises per-formatter coverage. A regression breaking only gofmt, or only goimports, shipped green through it. Which is the scenario the file exists to prevent, because ONE call-site shape being wrong is exactly how the original bug arrived. The two formatters now get independent stubs (`write_stub`, mode per tool), and two cases fail exactly one of them, so only that call site can produce the non-zero exit. Mutation-proved in both directions: swallow only the gofmt call site -> FAIL: a failing gofmt alone exited 0 — that call site swallows failures swallow only the goimports call site -> FAIL: a failing goimports alone exited 0 — that call site swallows failures unmutated -> format-verify: 10 properties hold Before this commit the first of those was `10 properties hold`, exit 0. `make check` stays green and inside budget; shellcheck clean at default severity. Refs cli#549.
…eader (backend#2320) (#551) * fix(cli): resolve the backend env once per invocation, not once per reader (backend#2320) The env/base-URL resolution family, not the one site #540 named. Each of the last recuts produced one more finding about a site that answered "which backend?" differently from its neighbour (cli#528 review, #542 review, now #540) — the failure mode is additive, so only the SECOND site is ever a bug. - telemetry: recordCommandOutcome took the resolved env as a parameter instead of calling telemetryEnv(signedInEnv()) a second time. The label and the sink (spool path + POST destination) now derive from one value, which is what the comment above RecordCommandOutcome already claimed. This is #540's finding. - sessionEnv is now the single config -> session-env resolution point, and it normalises (trim + lower-case) like api.ResolveEnv. Returning cfg.CurrentEnv verbatim made it the one env-resolving function whose output was not normalised: invisible where the value only reaches api.BaseURL (which lower-cases again), load-bearing where it is COMPARED, or where one consumer trims and another does not — api.BaseURL does not trim, so " dev " fell through to PROD. - `cluster doctor` built its API client from cfg.CurrentEnv raw and `auth status --check` compared it raw against an already-normalised target. Both go through sessionEnv now: a doctor probing prod with a dev token reports "session expired" for a session that is fine, and the installer, whose contract is --check's exit code, re-ran login against a working session. - internal/doctor.backendHost derives its host from api.BaseURL instead of restating the same three hosts in a second switch. Behaviour-identical (BaseURL already lower-cases); it removes the copy that drifts. - A guard test pins the closed set of sanctioned resolution sites, so the next one fails a check instead of a review. NOT changed: api.BaseURL's unknown/empty -> prod fail-open. It is shared with the installer's _backend_url and contradicted by client-runtime's controller.py, so it is a three-component decision tracked on backend#2171. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(telemetry): reflow the BaseURL-mirroring note left ragged by the previous edit Comment-only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(auth): `auth status` must report the env the client dials, not the stored string (backend#2320) The audit's last site: `auth status` printed cfg.CurrentEnv as its "backend" field while runAuthCheck — the machine-facing answer to the same question, 40 lines below in the same file — compares the resolved one. A status command that disagrees with the client is worse than no status command. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(cli): keep the doctor env test off the network (backend#2320) My own bug, and worth the comment it now carries. Past the session probe, `cluster doctor` loads the real kubeconfig and calls the real doctor.Run, whose checkBackendEgress probes backendHost("") — a live GET to https://api.tracebloc.io/. On a developer machine with a real k3d cluster the test therefore made a production request; the run time (~16s vs 0.01s stubbed) is the tell. Stubbing loadClusterFn returns right after the session probe, which is all this test needs: the env is decided before it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(cli): make the resolution guard actually guard (backend#2320) Four review findings, all the same class — a check that verifies the FORM of a thing rather than the property the form exists to guarantee. Which is the class this PR is about, so the guard having it was the worst possible place for it. - Scan Go as Go. The hand-rolled comment stripper was fail-open on string literals: the `//` inside an `https://…` literal started a "comment" that ate the rest of the line, needle included, and a `/*` inside a literal like `"/*.json"` swallowed every needle below it to the next `*/` or EOF. Seven non-test files in internal/cli already carry an https:// literal, so this was one future line away. go/scanner with mode 0 drops comments and knows literals, so neither evasion exists. Literals are kept in the output — a needle inside a string is then a loud false positive, which is the cheap direction. - Walk the module root, not the test's own package dir. The guard covered 1 of the 17 packages under internal/, i.e. it was blind exactly where the next site is most likely to land: a new package written by someone who never reads internal/cli. Keys are now repo-relative, and internal/api, internal/config and internal/doctor join the allowlist with the reasons the PR body already gave. - An inert allowlist entry now FAILS. Checking only that a sanctioned file exists let my own change turn the telemetry.go entry into a licence: it matched no needle any more, so it checked nothing while silently pre-approving the next raw read in the very file whose double resolution this PR removes. The entry is gone and the staleness assertion stops the next one going inert unnoticed. - signedInEnv's docstring was false: it can no longer return "". Says so now, including that `if signedInEnv() == ""` cannot fire — and telemetryEnv's empty arm is marked production-unreachable-but-test-reachable rather than left to be traced. Also corrects an overclaim I made in the first draft of goCodeTokens' comment: go/scanner is lexical, so the error covers unterminated literals and comments — the faults that would desynchronise boundary tracking — not `func f( {`, which scans clean. Stated precisely rather than left flattering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(cli): one needle set, one matcher, one allowlist — each checked both ways (backend#2320) Adopts Lukas's unified shape for the guard, which is better than what I pushed in f33a3d1 in two concrete ways: - `matchesAnyNeedle` is now THE matcher, called from the detection sweep AND the allowlist audit. Two copies of "does this file resolve an env?" is the same shape as the two copies of "which env?" this PR removes, and it would let detection and allowlisting drift apart exactly where nobody looks. - The allowlist audit is per ENTRY, not per suite, so staleness, the empty-reason check and the needles-went-stale backstop all fall out of one loop and the failure names the entry to delete. Renaming a needle now reports all five entries by name instead of a global counter hitting zero. Kept a narrow anchor the per-entry loop genuinely cannot see: an EMPTY allowlist makes that loop vacuous, so a needle rename plus an empty allowlist would pass in silence. It asserts both counts are non-zero. Eight reproductions, all red, all restored green — the control, both string-literal evasions, the sibling-package site, a re-inerted sanctioned entry, an empty reason, a needle rename, and a lexical fault elsewhere in the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Bumps the golang-x group with 1 update: [golang.org/x/text](https://github.com/golang/text). Updates `golang.org/x/text` from 0.40.0 to 0.41.0 - [Release notes](https://github.com/golang/text/releases) - [Commits](golang/text@v0.40.0...v0.41.0) --- updated-dependencies: - dependency-name: golang.org/x/text dependency-version: 0.41.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: golang-x ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…nder (cli#552) (#553) Bugbot's High on the #540 promotion. Two keys for one concept, and the failure was silent by construction. `telemetryToken` took the telemetry LABEL and looked the profile up by it. Profiles are keyed on the RAW `cfg.CurrentEnv`; the label has been through `telemetryEnv`, which lower-cases and trims (via `sessionEnv`) and remaps anything unrecognised onto prod. Whenever those disagreed the lookup did not miss loudly — `Profile()` CREATED an empty profile and returned no token, so delivery took the no-token spool path forever while `authedClient`, reading `cfg.Current()` on the raw key, kept working. The CLI looked signed in and healthy; outcomes simply never arrived. THE PARAMETER WAS THE DEFECT, so it is gone rather than corrected. `telemetryToken()` now reads the current session's token, which is the same thing `authedClient` reads and the same key `SignedIn()` tests. Passing a label in is no longer possible, so the two cannot disagree again — as opposed to agreeing today and drifting the next time `telemetryEnv` gains a case. `config.CurrentToken()` is the reader, and it READS WITHOUT CREATING. `Profile()` storing an absent profile is correct for the write paths it exists for — sign-in mutates the returned pointer, then Saves — and a trap as a lookup: the miss is recorded, so the second call finds a profile and looks like a hit. That is the ticket's separate observation and it is fixed here, not deferred. THE TOKEN STILL GOES WHERE THE SESSION ALREADY TALKS. `api.BaseURL` routes an unrecognised env to prod exactly as `telemetryEnv` does, so the destination the label picks is the one this session's client is already using. That BaseURL does that at all is a real defect — shared with the installer's `_backend_url`, contradicted by client-runtime's controller.py, tracked across three components on backend#2171 — and `telemetryEnv`'s own note says this code must match that behaviour until it changes rather than diverge from it. This fix does not diverge from it. TESTS. There were NONE for `telemetryToken`, which is why this reached a promotion. Added at both levels: * `internal/config` — the reader is keyed on the raw `CurrentEnv` and agrees with `SignedIn()`; it does not create on a miss, with the contrast against `Profile()` asserted in the same test so a refactor cannot route one through the other and still pass; and it is empty on every genuinely-tokenless shape. * `internal/cli` — a table of raw keys that each differ from their resolved label (unrecognised → prod, upper-case, surrounding whitespace, a v1 verbatim `Dev`), plus `prod` as the control that worked before and must still. Each case first asserts the label really does diverge, so the table still means something if `telemetryEnv`'s mapping changes. And the composed behaviour the ticket asks for: a signed-in session whose label was remapped POSTs rather than spools. Real `telemetryToken` over a real on-disk config, real `deliver`, only the URL substituted — `api.BaseURL` has no test seam and would otherwise have sent the test at production. Mutation-proven: the two-key lookup restored 2 failed (incl. the POST-vs-spool test) CurrentToken routed via Profile() 1 failed (the creates-on-miss test) No survivors; green on restore. gofmt, go vet and the full suite clean. Closes #552 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
release-train: develop -> staging
…/stale (#543) * ci(1979): call the board-aware stale sweep instead of copying actions/stale * ci(1979): drop the 16-way actions/stale copy this caller replaces
release-train: develop -> staging
Contributor
Author
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit d289cd8. Configure here.
tracebloc-release-train
Bot
added
gate-nudge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated promotion by the release train (RFC-0008 D14). Head is the train-managed
release-train/to-mainbranch (a mirror ofstaging), so it never collides with a human PR. Merged only when the fr-gate is green.Note
High Risk
Touches auth/env matching, bearer-token telemetry POSTs, and helm apply paths for resource ceilings. A mismatch can send events to the wrong backend or skip confirmation on resource writes.
Overview
Release-train promotion (0.10.9 → 0.10.10) that turns command-outcome telemetry into a real, fail-closed delivery path and tightens how the CLI answers “which backend?” and “who set this ceiling?”.
Telemetry now POSTs OTLP/HTTP JSON to
/telemetry/v1/records/with a ~1s budget, per-env disk spool (drop-oldest, 0600), and drain on the next run. Label, spool path, and destination share one resolved env. The CLI also drains installer fallback spools, but only records whosedeployment.environmentmatches this session. Afterdeletewipes~/.tracebloc, exit-path writes must not recreate it.Session env is normalized (trim + lower-case) in
sessionEnvonly.auth status,--check,cluster doctor, and telemetry all use that value so a stored"Dev"no longer disagrees with--env dev. A module-wide test forbids new ambientCurrentEnv/CLIENT_ENVreads.resources setstampsRESOURCE_PROVENANCE=usereven when the ceiling is unchanged (installer/unknownis stale). Bookkeeping applies (provenance + phantom GPU) skip the confirm gate so scripts stay exit 0; a real size change still requires--yesoff a TTY. Verboseresourcesshows who set the ceiling.CI:
make fmt-checkscopes formatters to tracked*.gofiles; a hermeticfmt-selftestguards fail-closed script behavior. New weekly/PR envelope-contract drift job diffs the vendored JSON against pinnedclient-runtime. Stale-issue sweep becomes a thin reusable caller. E2E path filter includestelemetry*.go. Installer job dropsapt-getand shellsformat.sh.Reviewed by Cursor Bugbot for commit d289cd8. Bugbot is set up for automated code reviews on this repo. Configure here.