Uh oh!
There was an error while loading. Please reload this page.
release-train: develop -> staging - #783
Merged
Merged
Conversation
#776) Three PRs merged on 2026-08-20 — #762, #763 and tracebloc/release-train#94 — shipped ELEVEN tests that asserted the right property and proved nothing. Every one was written deliberately, reviewed, and green. That is the most frequent finding of the three PRs by a wide margin, more than every production defect in them combined, and the guide said nothing about it. The convention is that a finding recurring across PRs becomes a rule here, so: - the three shapes, ascending in subtlety — unreachable fixture, redundant mechanisms (two paths, one observable), and the inert mutation, where the MUTATION fails to express the defect. The third is the dangerous one: an anchor-resolution check cannot see it, because the anchor resolves perfectly. - a surviving mutation is a defect in the test, never a nuisance to annotate; and a green mutation log is evidence only if the run asserts the mutation APPLIED (backend#1729 rule 5). - a derived vocabulary must fail closed. Deriving beats restating, but a derivation that silently falls through returns the WRONG vocabulary and then agrees with itself — check-style.bats's `_brand_rgbs` fell back to the hex list when rule 1's RGB arm was deleted, so "every RGB triple is caught" passed with the RGB half gone. - the early-close hazard now has a CI gate, with diagnose.sh as the worked example of an instance that is correct AS a pipe. - a corollary on the `scripts/lib/*.sh` non-issue: they set no options but they RUN under both, so errexit/pipefail rules apply to them in full. A guard that asks only "does this file set the options" reads the whole lib tree as safe — the bug #763 fixed. - never resolve a review thread on "the reported case now passes": a fix for one spelling routinely leaves its sibling broken, and a resolved thread reads as handled to the next person. backend#1729 already required mutation-proving, deriving the input domain, and never testing a copy of the rule. All three were FOLLOWED in these PRs and the tests were still vacuous eleven times — the existing rules say to mutation-test but not what a surviving mutation means, nor that a fixture can be too thin for the property to be observable. That is the gap. Every claim verified against the tree before committing; two drafting errors caught that way (diagnose.sh carries no marker — the guard reads `set +e` — and `mutation-markers` is a release-train target, not a client one). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…aid it would (#775) * docs(telemetry): #1906 cannot drain these spools, and four comments said it would Comment-only. No behaviour change, and deliberately separate from the delivery work so the correction is not buried in it. `scripts/lib/telemetry.sh` said in four places that #1906's forwarder would read or drain the spool files this module writes. It cannot, and the reason is structural rather than a matter of sequencing: #1906 is an OpenTelemetry Collector running as a POD inside the customer's Kubernetes cluster, reading container stdout through a `filelog` receiver. Neither spool is container stdout and neither is reachable from a pod -- `$HOST_DATA_DIR/telemetry/pending.jsonl` and the `$TMPDIR` fallback are files on the operator's own machine. The pre-log failures the fallback exists for (`validate_config`, `early_data_dir_guard`) happen when there is often no cluster to run a Collector in at all. That is not a pedantic fix. The host transport went unowned after backend#1907 closed precisely because these comments pointed at a ticket that could never deliver it, and this file is the first place the next person looks. It now points at backend#2217 and says why #1906 is the wrong answer, so the same conclusion is not re-derived. Also records, at the trim, that `tail -n` is DROP-OLDEST on purpose and is deliberately not what RFC-BACKEND-1872 D7's overflow row says since rfcs#36. D7 was amended to drop-newest because `exporterhelper` sheds at the entrance and offers nothing else -- a constraint on the Collector's queue, not a preference. This spool is our own code and can do what D7 originally wanted. Without that note the next reader has a standing invitation to "fix" the disagreement in the wrong direction. The PowerShell twin needed no matching change: `install-k8s.ps1` contains no telemetry at all, so there is no drift between the two. Verified: `bash -n`, `shellcheck -S warning -x` clean, all 44 `scripts/tests/telemetry.bats` tests pass, and `scripts/tests/telemetry-vocabulary-agreement.sh` green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): regenerate manifest.sha256 — a comment edit IS a behaviour change here @saqlainsyed007 and Bugbot, and the finding falsifies this PR's own framing rather than just reddening CI. `scripts/lib/telemetry.sh` is bootstrap-FETCHED and hash-pinned, so editing a comment changes its digest: 38b2a2e9... -> d773c8a1..., exactly as reported. All three failures were the one cause -- `make drift` refusing the stale manifest, and `gen-manifest.bats` "a clean tree passes --check". The part that matters more than CI: at runtime `install.sh` verifies the fetched `telemetry.sh` against the pinned hash and ABORTS BEFORE THE PRIVILEGED INSTALL on a mismatch. So "comment-only, no behaviour change" was wrong -- for a hash-pinned bootstrap file the digest is part of the contract, and shipping this without the manifest would have broken every fresh install while every local check stayed green. The PR description is corrected too; leaving that claim standing would teach the next person the same wrong thing. Ran `scripts/gen-manifest.sh`. Exactly one line moves -- telemetry.sh's digest -- which is the check that this is the manifest catching a real edit rather than an unrelated drift riding along. Verified: `scripts/gen-manifest.sh --check` reports up to date; `make drift` green on all 6 guards; `scripts/tests/gen-manifest.bats` 20/20; `scripts/tests/telemetry.bats` 44/44. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… had the wrong one (#764) * test(installer): the k3s disable set had no guard, and metrics-server had the wrong one Both installers pass three `--disable=` flags to `k3d cluster create`. Two of the three components -- traefik and servicelb -- appeared NOWHERE in scripts/tests/ on either installer, so dropping one was silent: the install still succeeds, the cluster just runs an inbound component the chart has no use for. The third flag, local-storage, had two bats tests and nothing on Windows. The one that matters runs the other way. metrics-server must never be disabled: resource-monitor-daemonset.yaml `lookup`s the v1beta1.metrics.k8s.io APIService and `fail`s the release without it, which aborts the install and every later auto-upgrade tick. Adding `--disable=metrics-server` as a footprint saving is a plausible edit sitting right next to three legitimate ones, and nothing in either suite stopped it. Tests assert the EXACT disable set, derived from the argv each installer actually builds -- cluster.bats from the recorded k3d invocation, Pester from the $k3dArgs AST. Neither holds a second copy of the list. Exactness is what carries the metrics-server property: a `contains` test can only catch a REMOVED flag, so it would never see an added one, whatever it names. Adds scripts/tests/k3s-components-agreement.sh for what neither unit suite can see: the two installers agreeing with each other, that the chart coupling making metrics-server load-bearing still exists, and a tripwire on the bash/PowerShell divergence (cluster.sh gates local-storage on TB_STORAGE_MODE, install-k8s.ps1 disables it unconditionally -- correct only while node-local has no Windows path). Wired into `Source-of-truth drift`, which is REQUIRED on develop and main; `Pester (windows-latest)` is not a required context, so a guard living only there could advise but never block. Every assertion mutation-proven: flag removed / added / passed bare / variable renamed / chart `fail` removed, each confirmed to redden the intended test with the mutation anchor verified as applied. Also fixes stale comments this surfaced: * "the only in-cluster Service (mysql-client) is ClusterIP" (cluster.sh, install-k8s.ps1) -- there are four ClusterIP Services: mysql-client, jobs-manager, requests-proxy-service, egress-proxy-service. The conclusion held, the premise did not. * "crash-loops with 404s" (cluster.sh, values.yaml, resource-monitor-daemonset.yaml) -- it does not crash-loop, and that is the point. resource_monitor.py builds NodeUtilisation as the first statement in its `while True:` body and the loop handler catches Exception and sleeps 5s, so with no probes on the DaemonSet the pod stays Running and healthy-looking while send_heartbeat is never reached. Silent telemetry loss, not a crash-loop -- a crash-loop is the failure you would have noticed. * SECURITY.md already described the install-time failure correctly; adds the post-install one, since heartbeat freshness is the only thing that surfaces it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(chart): bump 1.9.48 -> 1.9.49 for the comment-only chart edits The required 'chart content ⇒ Chart.yaml version bump' gate fires on any change under client/templates/**, client/values.yaml or client/values.schema.json, and this branch edits comments in the resource-monitor DaemonSet template and in values.yaml. Rendered output is unchanged (helm template renders identically and the 482 helm-unittest cases pass), but the packaged .tgz content does change, and a Helm repo only ever publishes a NEW version -- so the bump is what makes the edit reachable rather than dark. #749 set the precedent for a docs-only chart change taking a patch bump. version and appVersion move in lockstep, as every bump in this chart's history has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(guard): strip Helm's chomping comment opener, and test the guard (Bugbot) The agreement guard's Helm-comment stripper matched `{{/*` but not `{{- /*`, so every whitespace-chomping comment block stayed in `ds_body`. This template already writes two of them. NOT reachable as written, and I checked before claiming either way: with the code removed and the header rewritten to `{{- /*`, the guard still exits 1 -- the surviving prose says `metrics.k8s.io` while check 1 greps the fully-qualified `v1beta1.metrics.k8s.io`, and it contains no `fail `. So the guard was protected by the WORDING of a comment, one legal edit away from letting prose stand in for the code it describes. That is not protection. Opener is now `/\{\{-?[[:space:]]*\/\*/`. AND THE GUARD NOW HAS A TEST SUITE. It runs in DRIFT_GUARDS -> `make drift` -> `Source-of-truth drift`, which is REQUIRED on develop and main, and the only thing exercising it was itself, against the real tree, printing green. A required gate whose sole evidence is its own green run is the #1729 shape. scripts/tests/k3s-components-agreement.bats, 9 cases driving the REAL script via a new TB_K3S_AGREEMENT_ROOT fixture hook -- never a re-implementation of its rules, which would drift from production and prove that a regex nobody runs would have caught the bug (#1729 rule 9): - the clean case exits 0, so every "catches X" below means something - a component in bash but not ps1, and the mirror - metrics-server disabled in either installer - the chart no longer looking up the APIService - THE REGRESSION: same removal with a `{{- /*` header - the chomping opener does not break the clean case - unreadable installer / unreadable daemonset -> exit 2, never green Mutation-proved: reverting the opener to `{{/*` reddens exactly test 6, and only test 6. Anchor asserted to match one line first, so an inert mutation cannot pass for coverage. Verified: make lint (44 parse, shellcheck clean), make drift all 7 green, gen-manifest --check up to date, check-style clean, bats-hygiene 18/18. The suite is auto-discovered -- BATS_TEST_COUNT is derived from `scripts/tests/*.bats`, now 1307. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(guard): the chomping closer too — opener and closer are one change (Bugbot) My previous commit taught the stripper Helm's chomping OPENER `{{- /*` and not the matching CLOSER `*/ -}}` (space before the hyphen). The real template uses both, so the block at line 136 opened and never closed: the stripper ate the rest of the file. Measured on the real template: 80 of 165 lines survived into ds_body, against 103 with the closer fixed. Check 3 passed anyway only because the `lookup` and `fail` sit ABOVE that block -- move them, or add a coupling below it, and the REQUIRED drift gate false-fails. This is a regression my own fix introduced, in the opposite direction from the one it fixed: the opener bug was fail-OPEN, this one is fail-CLOSED. Same root cause as the head-arm miss on client#777 an hour ago -- I changed one side of a paired construct and not the other. Twice in one day is a pattern, not bad luck. Closer is now /\*\/[[:space:]]*-?\}\}/. Two tests, 11 green. Bugbot also noted the existing fixtures never used this closer form, which is exactly why the suite did not catch it -- so the new fixture puts the coupling BELOW a `{{- /* … */ -}}` block, the arrangement that exposes it. The second test is the mirror that keeps the first honest: with the code genuinely absent below that same block, the guard must STILL exit 1, or "passes" could just mean the guard went blind. Mutation-proved, each half against its own test: revert the closer -> test 10 red, nothing else revert the opener -> test 6 red, nothing else Verified: make lint, make drift (7/7), gen-manifest --check, check-style, bats-hygiene 18/18. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… partial fix (#781) Three instances in one day, all caught by reviewers rather than by me, and all on constructs whose halves sat within twenty lines of each other: client#777 `||` neutralised to \001 for the grep arms, but \001 not added to the head terminator class -- `producer | head||die` silently dropped, though develop flagged it. FAIL-OPEN. client#764 Helm comment stripper taught the chomping opener `{{- /*` and not the closer `*/ -}}`, so a block using both never terminated and the stripper ate 23 of 165 lines. FAIL-CLOSED, in a required gate. client#777 the fix for the first shipped a second change whose mutation SURVIVED -- inert, reverted. The org convention is that a finding recurring across PRs becomes a rule here rather than being re-argued in comments, and this one recurred three times between two PRs. Two habits close it, both cheap, both stated in the rule: grep for the sibling before committing, and for any scanner or matcher diff the whole-tree output against the base -- if the base flags something you no longer do, that is a regression no unit test will show you, because the tests only cover the case you were already thinking about. Plus the corollary that explains why it kept getting through: a fixture set that only exercises the form the author had in mind. The k3s suite shipped nine cases in the SAME commit as a bug none of them could see, because none used the `*/ -}}` spelling the real template uses. Every claim re-checked against the tree before committing. check-style, gen-manifest --check and the early-close gate all clean. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* fix(shell-gate): `||` is not a pipe, and `|&` is (backend#2264)
The early-close gate flagged the output of its own remediation. Converting
the fleet for #2264 produces exactly this line:
cmd || grep -q needle <<<"$out"
There is no pipe there -- but the hazard regex matched the SECOND bar of the
`||`, so the scanner reported the form this repo recommends. A gate that
rejects its own fix is how a gate teaches people to switch it off, and it
would have blocked every cleanup PR in the #2264 wave.
Fix: neutralise `||` into \001\001 on a probe copy before the hazard test.
\001 cannot occur in a shell source line, and the ORIGINAL line is still
what gets printed, so offender output is unchanged.
Second, smaller gap found while writing that: `|&` (bash's pipe-both-streams)
is a pipe and carries the identical hazard, but the bar had to be followed by
space-or-name, so `noisy |& head -1` was missed. The bar now takes an
optional `&`.
Mutation-proved, 33 -> 33 green baseline, each mutation asserted to match
exactly one line first:
remove the || neutralisation -> 3 red (tests 27, 28, 29)
hazard test reads line not probe -> 1 red (test 28)
drop |& from the head arm -> 1 red (test 32)
drop |& from the grep -q arm -> 1 red (test 33)
Each mutation is caught by the test that NAMES the property, not by a shared
failure -- and the "a real pipe on a line that also contains ||" case is what
stops the three new spare-tests from passing if the hazard test were deleted
outright.
Verified: the gate reports zero over the whole tree; check-style clean;
gen-manifest --check up to date; 23 other bats suites green (0 failures).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(shell-gate): pin the quoted-fixture limitation with a test (backend#2264)
Measuring tracebloc/.github for the #2264 wave turned up its ONE reported
offender, and it is a false positive of a second, different kind:
house-rules-selftest.sh passes whole scripts as multi-line quoted arguments,
expect "a bare curl fires both rules" \
'#!/bin/bash
set -euo pipefail
curl -fsSL "$url" -o out' curl-timeout,curl-tls
and the scanner reads those `set -euo pipefail` lines as the FILE's options.
The file's real options are `set -uo pipefail` -- no errexit -- so the line
it flags carries no hazard at all.
I tried to fix it by tracking single-quoted regions and REJECTED the fix.
Shell has no escape inside '...', so counting quotes is exact in principle,
but apostrophes in prose ("the file's options") desynchronise the count --
measured, it is already out of phase by line 37 of that very file, before
any fixture appears. A desynchronised count HIDES real offenders, which is
strictly worse for a gate than reporting a false one.
So: documented in the header, and PINNED by a test asserting the current
(wrong) answer, so a future fix reddens it deliberately instead of changing
verdicts silently. A second test covers `# pipefail-guard: allow` as the
documented workaround for exactly this shape.
35 green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(shell-gate): the `||` stand-in must also be a boundary (Bugbot, client#777)
Bugbot caught the false positive one level deeper than my fix went, and it
is right. `grep[^|]*` used the `|` characters of a `||` as its STOP. Turning
them into something the class permitted let a plain `| grep` span the rest of
the line and reach the `-q` of a later `|| grep -q`:
producer | grep needle && cmd || grep -q x <<<"$y"
flagged again -- the same false positive my previous commit set out to remove.
Reproduced before fixing.
Fix: add \001 to the class, `grep[^|\001]*`, on both grep arms. Neutralising
the pipe was only half the job; the boundary has to survive it.
I also tried shrinking the stand-in from \001\001 to a single \001, on the
theory that the width mattered. Its mutation SURVIVED -- the class does all
the work -- so the change proved nothing and is reverted. Only the class
change ships.
Three tests added, 38 green. The third is the discrimination that makes the
other two mean something: on a line of that exact shape, a REAL `| grep -q`
is still flagged. Without it, widening the stand-in until everything is
spared would pass.
Mutation-proved:
drop \001 from the grep -q class -> test 36 red
drop \001 from the grep -m class -> test 37 red
shrink stand-in to one char -> nothing red (hence reverted)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(shell-gate): the head arm needs the same boundary as the grep arms (Arturo)
Arturo caught a FALSE NEGATIVE this PR introduced and develop did not have:
producer | head||die
`gsub` turns `||` into \001\001, so `head` is followed by \001 -- which was not
in the head terminator class, so the pipe was missed. Reproduced against both
scanners before fixing: develop flags the line, this PR silently dropped it.
It is the exact mirror of the boundary fix I made for the grep arms two commits
ago, and I only did half of it. `\001` went into `grep[^|\001]*` but not into
head's `[)"'`;|&]`, so the stand-in read as ordinary text on one arm and as a
boundary on the other. The neutralisation and the boundary are one change;
doing half moves the bug rather than fixing it -- which has now happened twice
on this same three-line edit.
Fix is his suggestion verbatim: add \001 to the head terminator class.
Four tests, 42 green. Two cover the glued form (`head||die`, `|& head||die`);
two are the discrimination that keeps them honest -- `head||true` and
`head|| :` must STILL be spared, or the fix would just be "flag anything with
head in it".
Mutation-proved: dropping \001 from the class reddens exactly tests 39 and 40,
and nothing else. Anchor asserted to match one line first.
Also checked the whole-tree parity that would have caught this earlier: running
develop's scanner and this one over every tracked .sh gives byte-identical
offender lists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>…terRole (#780) The last code item on backend#950. #731 narrowed this rule to the resources the image actually reads but left `watch` on both rules, and nothing watches. Every Kubernetes call in the resource-monitor image is a point read or a one-shot list — read_namespaced_pod, read_node, read_node_status, list_pod_for_all_namespaces, get_cluster_custom_object, list_namespaced_custom_object. There is no Watch(), no watch.stream, no watch=True anywhere in Node-deploy/, and the image shells out to nothing (CMD is `python /app/resource_monitor.py`; no kubectl is installed). The template's own comment already enumerated exactly this surface and never mentioned watch. Worth removing rather than shrugging at: this is a CLUSTER-scoped role, so `watch` on pods is a live streaming feed of every pod object in the cluster — other tenants' training pods included — held by an agent that only samples its own node. Same argument that removed `pods/log` under #950, one verb over. WHY IT SURVIVED, and the fix for that: resource_monitor_test.yaml pinned `resources` and left `verbs` unasserted, so the rule was narrowed on one axis while the other kept an unused grant. Both axes are asserted now, and the new test was checked against the unfixed template — re-adding `watch` fails it, so it is not vacuous. helm unittest 492 passed (was 488), helm lint clean. Refs backend#950 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 21, 2026
ContributorAuthor
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 38365cf. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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-stagingbranch (a mirror ofdevelop), so it never collides with a human PR. Merged only when the fr-gate is green.Note
Medium Risk
Touches cluster RBAC (verb reduction) and a required drift gate that can fail installs if metrics-server coupling or installer disable lists disagree. Behavior change is mostly tightening plus tests; still needs a careful look at the new parser/stripper.
Overview
Chart 1.9.51 tightens resource-monitor ClusterRole verbs to
get/listonly (drops unused cluster-widewatch) and pins that in helm-unittest. Comments/docs now describe the real metrics-server failure mode: pods stay Running and stop heartbeats rather than crash-looping.Adds a required drift guard (
k3s-components-agreement.sh) so bash and PowerShell installers disable the same k3s set, never disable metrics-server, and still hard-fail when the chart lookup/failcoupling is gone. Unit tests cover both installers and Helm comment-chomping stripper bugs.The pipefail early-close scanner no longer treats
||as a pipe, keeps\001as a boundary on both grep and head arms, and flags|&. Bugbot guide and telemetry comments are updated (host spool is backend#2217, not the in-cluster Collector).Reviewed by Cursor Bugbot for commit 38365cf. Bugbot is set up for automated code reviews on this repo. Configure here.