chore(make): one drift guard per line, and refuse an empty entry (backend#2626) - #933
Conversation
…kend#2626)
The DRIFT_GUARDS list is one `|`-separated line, so it conflicted every time two
branches each added a guard - three times on client#911 in a single day.
THE OBVIOUS CLAIM IS FALSE, and I checked instead of asserting it: one-per-line
does NOT make those appends auto-merge. Appending requires editing the previous
last line to add its `|\` continuation, so both sides still touch the same line
and git still conflicts. Measured both ways on the real 38-entry list.
What changes is whether the conflict can be RESOLVED BY READING it:
one line conflict hunk containing 1842-character lines. Finding which guard
each side added means diffing two 1842-char strings by eye, and the
resolution that looks right in a diff viewer is "take one side" -
which silently DELETES the other branch`s gate. A required check
stops existing and every run still reports green.
one each the differing entry is a 30-character line. You can see it.
So this buys legibility at the moment of highest risk, not automation. The comment
in the Makefile says exactly that rather than the tidier version.
AND IT CLOSES A FAIL-OPEN the new layout makes easy to type. A `||` in the middle
yields an EMPTY entry; `sh -c ""` exits 0 and `ran` still increments, so the count
check passes and the run reports "all N guards green" with one guard being the
empty string. Measured before the fix: `DRIFT_GUARDS=true||true` printed "all 3
guards green" and exited 0. A TRAILING `|` was already caught - the `for` drops a
trailing empty field so `ran` falls short - which is why only the middle case
needed a new guard. Entries are trimmed, so a whitespace-only entry is refused
too, and `==>` lines no longer carry the leading space Make`s continuations add.
NEW: scripts/tests/drift-list-integrity.bats, 8 cases. Until now the recipe`s
three fail-open guards were asserted by a COMMENT and had been found by hand
twice. Every case drives `make drift` itself with a crafted list rather than
re-implementing the splitting, so a copy of the rule cannot go on passing while
the recipe drifts. It also fails closed if the committed list ever shrinks below
20 entries, which would leave the crafted cases green while `make drift` gated
almost nothing.
Mutation-proved - each of the four refusals is independently load-bearing:
empty-entry guard removed -> 6 of 8 (the `||` and whitespace cases)
the trim removed -> 7 of 8
the count check removed -> 7 of 8
the empty-list check removed-> 7 of 8
NOT DONE, and named in the Makefile: deriving the list from `scripts/tests/*.sh`
would remove the conflict entirely and make an added guard FILE auto-merge. It is
left out because it changes which scripts RUN - a new .sh in that directory never
meant as a drift guard would start gating merges - so it needs its own change and
its own opt-out list.
VERIFIED: make drift 38/38 green; CI=true bats scripts/tests/*.bats 1603 passing,
0 failures; bats-hygiene green; shellcheck -S warning -x clean; manifest --check
clean. Makefile only - no chart content, so no version bump.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ugbot) DEMOTED, THEN HARDENED ANYWAY. The finding said GNU Make 4.x renders an exported simply-expanded variable as `export DRIFT_GUARDS :=`, so a `^DRIFT_GUARDS :=` matcher misses on Ubuntu CI and the floor never counts. Measured: this suite PASSED on Ubuntu CI as written - `bats (bash unit, mocked) = SUCCESS` on the very platform named - because Make records the assignment and the `export` directive separately. So it does not reproduce, and a miss would have failed CLOSED (the `-n` check) rather than passing silently. But the matcher WAS depending on which of two shapes a given Make emits, and one alternation removes that dependency. Local make here is 3.81; CI`s is 4.x; the test should not care. AND THE OTHER SHAPE IS NOW TESTED DIRECTLY, which is the part that would have stayed uncovered: the committed-list assertion can only ever exercise the shape THIS Make emits, so the alternative branch would have been untested until a Make upgrade made it live - exactly the gap the finding pointed at. All four forms are driven (with and without `export`, `:=` and `=`), plus a CONTROL that a line merely MENTIONING the name is not an assignment. Mutation-proved: the optional export prefix dropped -> 8 of 9 (the reported shape) the matcher made over-broad -> 8 of 9 (the control bites) make drift 38/38; bats-hygiene green; shellcheck -S warning -x clean; manifest --check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Demoted with evidence, then hardened anyway. It does not reproduceThe claim is that GNU Make 4.x renders an exported simply-expanded variable as Measured: this suite passed on Ubuntu CI as written — But the dependency was real, so it's goneThe matcher was depending on which of two shapes a given Make emits. Local make here is 3.81, CI's is 4.x, and the test should not care. One alternation removes it: The part that would genuinely have stayed uncovered is the other branch. The committed-list assertion can only ever exercise the shape this Make emits, so the alternative would have been untested until a Make upgrade made it live — which is the gap the finding was pointing at even though its stated failure was wrong. There's now a test driving all four forms directly ( Mutation-proved:
Also on this PRThe
|
|
bugbot run |
…f it (backend#2868) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
…#2868) Built on 5bf6002 rather than beside it - a co-driver and I fixed the same Bugbot finding on this branch at the same time, and 5bf6002 got there first. Its one-matcher-two-callers shape is kept; this is the delta. THE COUNT MADE THE STRIP UNTESTABLE. `awk -F"|" NF` returns the same number whether or not the assignment prefix was removed, because a leftover prefix just rides along inside field 1. Measured on 5bf6002 itself: narrowing the `sed` to `^DRIFT_GUARDS := ` left all NINE cases green. So the half of the matcher the finding was actually about - handling both shapes Make can print - was still only half covered. The function returns the value; callers count; the shape case asserts it BYTE-EXACT, which is the only thing a wrong strip changes. AND THE RENAME EXPOSED A VACUOUS CONTROL, which is the part worth reading. The control was `! printf ... | drift_guards_entries`. Renaming the function left that line pointing at nothing - `! missing_command` is non-zero, `!` inverts it, and the control went on passing while exercising NOTHING. Routing it through `bash -c` to capture the status made it worse: a bats-defined function is invisible there, so every run was 127. It now asserts the function EXISTS (`type -t`) and THEN that it refuses, so "it refused" can never be satisfied by "it is gone" (CLAUDE.md rule 10). Mutation-proved, all three reaching the live function: the sed narrowed -> red (the weakness this commit closes) the function renamed away -> red on TWO cases (the vacuous control, now caught) the grep narrowed -> red (the original finding shape, still covered) make drift 38/38; bats-hygiene green; shellcheck -S warning -x clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Built on The count made the strip untestable
The function returns the value now; callers count; the shape case asserts it byte-exact, which is the only thing a wrong strip changes. The rename exposed a vacuous control, and that's the part worth readingThe control was: ! printf '%s\n' "# DRIFT_GUARDS is documented above" | drift_guards_entriesRenaming the function left that line pointing at nothing — It now asserts the function exists ( Mutation-proved, all three reaching the live function:
Note on the two of usThree convergent fixes on this branch today (the develop merge, the rule-9 fix, and this). The per-PR lock only coordinates
|
|
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 31d7946. Configure here.
# Conflicts: # Makefile
aptracebloc
left a comment
There was a problem hiding this comment.
Verified against the code at 26a68e4. The one-per-line reformat preserves all 38 guards, and the new empty-entry refusal is genuinely fail-closed: each entry is trimmed then [ -z ]-checked, so a middle || or whitespace-only entry exits 1 rather than counting sh -c "" as a passing guard, while a trailing | stays caught by the existing ran != exp count check. The bats suite drives make drift itself rather than restating the split logic, floors the real list at 20 entries, and the mutation table shows each of the four refusals is independently load-bearing. CI is green (incl. make drift 38/38 and the +8 bats cases), both Bugbot threads are resolved, and there is no merge conflict. No defects found.
— drafted with Claude Code
Three conflicts, and two of them would have silently reverted work: * Makefile DRIFT_GUARDS -- develop moved to one-guard-per-line (#933) while this branch added `jobs-manager-waits-for-mysql.sh` to the old single-line form. UNION merged: develop format plus our guard as its own line. Taking either side alone drops a REQUIRED guard -- ours would have lost `hostpath-reads-guarded.sh`, theirs ours. Verified both are present: 40 green. * jobs-manager-deployment.yaml -- ours is this PR (unconditional initContainers), theirs is the nil-guarded hostPath read from #939/backend#2910. Adopted their guard inside our block; taking ours verbatim would have reverted a fix that keeps `--reuse-values` rendering when the hostPath key is absent entirely. Verified: 3 live hostPath reads, 0 unguarded, and the chart renders with hostPath omitted. * Chart.yaml -- 1.9.92, one patch above develop 1.9.91. Also reworded one comment of mine: `hostpath-reads-guarded.sh` reads RAW text, so my line describing the old gate tripped it by quoting the bare read. Prose breaking a check rather than satisfying one -- the guard cannot tell a comment from code, which is worth fixing in the guard rather than in every comment that needs to name the anti-pattern. Not doing that here: it is develop s guard and belongs in its own change. helm unittest 644/644, drift 40/40. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ctor (backend#2626) Both findings from @saadqbal review, plus the base merge. 1. CLS_ENV EMPTINESS WAS HALF-DEMOTED (Bugbot Medium; he reproduced it). The identity-env block already treated per-profile emptiness as legitimate and printed `ENVCLASS 0` -- but the STAYED empty-class arm set `fail = True` first, so that half could never rescue the run. Reproduced here on aks with autoUpgrade, imageRefresh and sealCheck.storageAssertions.enabled all off: 38 documents, and the ORIGINAL prints "[ERROR] found NO RELEASE_NAME ..." and "ENVCLASS 0" in ONE output. A required drift guard refusing a complete chart. AND THE ONE-LINE FIX WOULD HAVE BROKEN THE CROSS-PROFILE ASSERTION. Extending the demotion is right, but it must NOT also print `ENVCLASS 0` the way CLS_PATH prints `PATHCLASS 0`: the shell parses `grep -E ^ENVCLASS | head -1` and the identity-env block below prints the REAL count unconditionally, so a zero here would be read first and the true value discarded -- turning a cross-profile assertion into one that always sees 0. CLS_PATH can print its count because its two branches are mutually exclusive; CLS_ENV count is owned by the block below. Stated in the code. STILL LATENT, and I am not claiming a test covers it: all four CI profiles name 5 identity envs, so disabling the demotion does NOT redden the suite. The evidence is the live reproduction above, not a guard. Pinning it needs either a fifth profile or a specimen driving assert_stayed empty arm -- happy to add the latter. 2. THE REFUSAL DETECTOR HAD NO SPECIMENS AT ALL, which is the asymmetry that made his mutation possible: `_fallback_on_line` has three pinning lines, `_mitigations` had none. `return {"refusal"}` unconditionally left assertion 5 printing "all 2 routed Secret lookup(s) of 2 carry a mitigation" and exiting 0, both existing selftests green -- the detector deciding whether a routed credential Secret is mitigated could be DELETED and nothing in the tree would say so. Four specimens now, written down rather than hunted for in the chart: a real refusal, a negation that merely branches, a `fail` reached by negating an UNRELATED variable (the routed_vars discrimination), and a `fail` below the 4-line window. Mutations, all reddening: his exact `return {"refusal"}` (3 of 4 halves fire); widening the window 4 -> 40; dropping the per-var discrimination. A fourth came back INERT and its anchor assertion said so rather than reporting a pass. 3. BASE MERGED, both conflicts as he specified. Makefile: develop block plus one line, verified by comparing guard sets programmatically -- 39 on develop, 40 here, the single delta being fullname-override-completeness.sh -- rather than by eye, because `--ours` would delete #933 empty-entry refusal and `--theirs` would drop this guard from the list entirely. manifest.sha256 REGENERATED from the merged tree. Guard at head: rc=0, 60 OK, 0 ERROR, aks=5 bm=5 eks=5 oc=5 -- the 56/0 baseline he measured, plus the four new selftest lines. drift-list-integrity 9/9, gen-manifest --check clean. black is NOT run by this repo CI and the file was already non-black-clean (14 hunks before and after, none in the added code), so it is left alone rather than reformatted into an unrelated diff. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… it safe (backend#2626) (#911) * feat(chart): fullnameOverride, with the completeness guard that makes it safe (backend#2626) backend#2621 built this helper, proved the default render byte-identical, and REVERTED IT. The helper is the easy half. The release name appears ~174 times across these templates and is at least six different things, only two of which may follow an override -- and a partial routing produces `prod-auto-upgrade` beside `myrel-jobs-manager`, which is harder to reason about than a release that is merely badly named. So the guard is the deliverable and the helper rides along. WHAT MOVED (39 sites, 15 files): names of resources this chart creates, and two things that REFERENCE those names -- * `image-refresh-cronjob.yaml` DEPLOYMENT_NAME. It names a Deployment this chart creates; leaving it behind points `kubectl set image` at a workload that no longer exists. Not in the ticket's list. * `telemetry-collector-configmap.yaml`'s filelog glob, `{ns}_{release}-*/{container}/*.log`. It matches POD DIRECTORIES, and pods are named after the DaemonSet. Route the DaemonSet without the glob and the Collector runs, reports healthy, and collects nothing -- the exact failure that file already warns about. Also not in the ticket's list. WHAT STAYED: `app.kubernetes.io/instance` (Helm convention), the 27 `meta.helm.sh/release-name` annotations (Helm's bookkeeping), the 3 RELEASE_NAME/RELEASE envs (a HELM IDENTITY -- `helm status`, `helm rollback`), and 4 on-disk paths (a LOCATION: renaming orphans a tenant's data). THE TICKET PREDICTED THE BUG AND I WROTE IT ANYWAY. "A `sed` over `.Release.Name` catches it on the first pass; mine did." My first routing pass anchored the env-var exception with `$` against a CONCATENATED context string, so it never matched and the RELEASE_NAME env was routed -- backend#2620 re-introduced by the fix for backend#2621, exactly as written. Two more followed: `$.Release.Name` substituted as a plain string clipped `$.Release.Namespace` to `...$)space`, and routing ran over the helper's own body so `tracebloc.fullname` called itself until helm died. The third is now structural rather than guarded: the helper is inserted AFTER routing, so its body is never a candidate. The guard, `scripts/tests/fullname-override-completeness.sh` plus its assertions module, over EVERY platform profile in `client/ci/*-values.yaml`: 1. NO-OP override == release name renders identically to unset 1b. VERBATIM with the override unset, resource names carry the release name WHOLE 2. MOVED with a distinctive override, no resource name still carries the release name -- misses reported BY NAME 3. STAYED every exception still carries it, checked with the right predicate per class Assertions 2 and 3 are a pair: without 3, assertion 2 is satisfied by renaming Helm's own bookkeeping. PROFILES MATTER, and a single render hid it: `bm-values.yaml` sets `hostPath.enabled`, and the hostPath PVs and dataset directory exist in no other profile. One ad-hoc render checked ONE of the four release-scoped paths and called itself satisfied. ASSERTION 1b EXISTS BECAUSE A MUTATION SURVIVED. Assertion 1 diffs two renders that both pass through the helper, so `| trunc N` cancels out on both sides and is structurally invisible to it. The mutation was also inert for a three-letter release name. The release name used by the guard is now 38 characters, and 1b reads the default render directly. Non-determinism is MEASURED, not listed: secrets.yaml mints credentials with `randAlphaNum`, so the guard renders twice with identical inputs and excludes whatever differs. A hand-kept key list goes stale, and the template's variable names do not even map to the rendered keys (`$podTokenSecret` renders as `POD_TOKEN_SIGNING_SECRET`). Wired into `DRIFT_GUARDS`, which the REQUIRED `Source-of-truth drift` job runs -- a guard in a non-required job is advice. Evidence -------- helm unittest ./client (helm 3.15.4, CI's pin) -> 36 suites, 631 tests, OK 18 name-sensitive shell gates -> all pass helm lint / shellcheck -S warning -x -> clean the guard, 4 profiles -> 32 assertions OK mutations, each anchor asserted to have applied: route the RELEASE_NAME env -> caught (helm-identity) un-route one resource name -> caught (by name) route the dataset directory -> caught (on-disk path) trunc 20 on the default -> caught (verbatim) upper on the default -> caught (verbatim) restored -> green Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): the guard walks every scalar, NOTES is routed and checked (backend#2626) All four review points, and the first two were the same defect. THE GUARD DID NOT COVER THE SITES THIS PR SINGLED OUT. MOVED read doc-root `metadata.name` only, while STAYED enumerated the exception classes separately - so the two halves disagreed about what the exceptions were and every name-REFERENCE site fell through the gap. Un-routing DEPLOYMENT_NAME (which `kubectl set image` targets) or the Collector filelog glob (which globs pod directories) rendered both broken and left the guard green on all four profiles. Now ONE classifier, TWO callers: `classify()` walks every string scalar and labels each release-name hit with the exception that licenses it, or None. MOVED is "nothing unlicensed"; STAYED is "every class non-empty and correct". Adding a class cannot weaken MOVED without adding an obligation to STAYED. 1339-1422 scalars per profile, against a handful of metadata.name before. NOTES.txt: L6, L24 and L27 routed. It was half-routed seven lines apart - L6 printed the release name while L13 printed the override, in the first thing an operator reads. Assertion 4 now checks it, and needed its own render: `helm template` omits NOTES, `--show-only templates/NOTES.txt` answers "could not find template", and BOTH `--dry-run` and `--dry-run=client` need a cluster on 3.15.4 - with a reachable one, ownership validation against real objects made the verdict depend on whose kubeconfig ran it. So it renders a chart copy in which NOTES is an ordinary template, with --debug (helm refuses to emit output it cannot parse as YAML, and NOTES carries ANSI escapes) and KUBECONFIG=/dev/null. PyYAML: a named refusal with EXIT 2, distinct from the 1 that means "the chart is incomplete" - a missing dependency used to print a traceback and then "[ERROR] fullnameOverride is incomplete in 4 profile check(s)", sending the reader hunting un-routed names that do not exist. And `.py` is now in pyyaml-preflight.bats`s filter: `extract_python` finds python embedded in shell, so the first sidecar in this tree escaped the class rule entirely and every future one would have too. values.schema.json: DNS-1123 pattern + maxLength 53, the same constraints Helm enforces on the value this replaces. Verified - a bad value now fails at template time by name instead of object-by-object at the API server mid-install. VERIFIED: make drift 35/35 guards green on all four profiles; CI=true bats scripts/tests/*.bats 1568 passing, 0 failures; shellcheck -S warning -x clean. Mutation-proved eight ways, each anchor asserted applied - and two mutations exposed weaknesses in my own fix: a broadened path allowlist swallowed unrouted sites out of MOVED (the path class was the only one a non-path could satisfy by accident - it now asserts its members are paths), and deleting assertion 4`s "no NOTES supplied" refusal changed nothing because the loop always supplies it (there is now a self-check that invokes the assertions without it and requires a refusal). Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): refuse a fullnameOverride rename of a LIVE release (backend#2626) Bugbot High, and it is a data-loss-shaped bug rather than a naming one. fullnameOverride now routes tracebloc.secretName. That is correct for an INSTALL and unsafe for a RENAME: setting the override on a release that already exists moves the Secret name, so the lookup at the top of secrets.yaml misses and four of the six credentials fall to their tier-3 randAlphaNum and are MINTED FRESH. The MySQL PVC is keep-ed and still holds the old ones: helm upgrade -> STATUS: deployed mysql -> ERROR 1045 (28000): Access denied A successful upgrade that leaves the database unopenable, with no warning at any layer. The completeness guard this PR adds could not see it -- every rendered NAME follows the override exactly as designed; what did not follow was the DATA. secrets.yaml now looks for the Secret under the name it would have had WITHOUT the override. Present, plus an override that differs, means a live release is being renamed, and it fails with the migrate-deliberately remedy. A fresh install with an override, and a release that always had one, both see no old-name Secret and are untouched. MEASURED against a live cluster, not reasoned about -- k3d + `--dry-run=server`, which unlike `helm template` actually performs lookup: override + pre-existing un-overridden Secret -> REFUSED, by the named message override, no such Secret (fresh install) -> silent no override at all -> silent the dangerous case re-run after the controls -> still REFUSED The assertions go in client-credentials-have-a-secret-tier.sh rather than the chart suite, for that file own structural reason: this is a lookup, so helm-unittest renders it away. Three assertions -- the refusal exists, it derives the UN-overridden name from .Release.Name, and it gates on the two names differing. The middle one is the correctness of the whole thing: keyed on the overridden name it would compare a name against itself, never fire, and still read as a guard. Collapsed-run floor raised 13 -> 16. 3 mutations, all reddening, every anchor asserted applied. 631 chart tests pass, drift 35/35, completeness guard green, shellcheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): key the re-mint refusal on the persisted data, not on a name (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(chart): hold the CLASS behind the Secret-lookup finding, not just the instance (backend#2626) fae5441 fixed the instance, and better than my own attempt twice over. I had un-routed `tracebloc.secretName` so a rename could not lose the credentials - UNTESTED, because `lookup` is inert under every client-side renderer I have. ea6568d refused the dangerous case and MEASURED it with `--dry-run=server`, the only way to exercise a lookup; fae5441 then re-keyed that refusal on the persisted DATA after Arturo showed name-keying caught one of three rename directions and missed reinstall-over-a-kept-PVC entirely. That work is on the branch. Mine is discarded rather than layered on: with the Secret un-routed, the refusal would compare a name against itself and fire on every override-set upgrade. What nothing covers is the CLASS. A Secret `lookup` keyed on a name that follows the override misses on a rename, and a missed lookup is not an error - it silently takes the last resolution tier. This chart has TWO such sites: secrets.yaml a `fail` reached by the miss (fae5441, held by client-credentials-have-a-secret-tier.sh) tracebloc.telemetryTokenPresent safe ONLY because it ORs a lookup on the legacy fixed name - and nothing asserted that So the second member was one edit from the same silent shape with no check. Assertion 5 requires every routed Secret lookup to carry one of the two mitigations in its own file. Which mitigation a site needs is not this assertion`s call - it is a text-level check and could not have established what `--dry-run=server` did. It only requires that one is still there. THE REFUSAL IS DETECTED ON THE INVARIANT, NOT THE ARITHMETIC, and that is the lesson from writing this against a moving target. My first detector matched `printf "%s-secrets" .Release.Name` - the shape the refusal had while it keyed on the un-overridden name. fae5441 improved the refusal and my detector would have reported it as NO refusal. A detector that breaks when the thing it guards gets better is worse than none: it pushes back toward the shape it was written against. It now keys on the property that must hold however the other half is computed - a miss on THAT lookup reaches a `fail`. Which names follow the override is DERIVED transitively, closing `include` over the helper definitions, so a helper added tomorrow that wraps `fullname` is covered without anyone remembering. Fails closed three ways: a define parser that matches nothing, zero routed sites in a chart known to have two, and zero Secret lookups at all. Mutation-proved four ways, each anchor asserted applied: the data-keyed refusal removed -> flagged the refusal no longer negates that lookup -> flagged telemetry legacy-name fallback removed -> flagged (the unguarded member) routed-var detection stops resolving -> fail closed VERIFIED on helm v3.15.4 (the CI pin): make drift 35/35; helm unittest 631/631 in 36 suites; client-credentials-have-a-secret-tier 17 assertions OK; shellcheck clean. No chart files touched, so no version bump. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): read the client Secret under fullnameOverride, and parse helper bodies whole (backend#2626) Two Bugbot findings on a043e52, both Medium, both real. 1. THE INSTALLER MISSES AN OVERRIDDEN SECRET NAME. `tracebloc.secretName` follows `fullnameOverride`, so on a release installed with one the Secret is `<override>-secrets`. `detect_installed_client` and `Get-InstalledClientInfo` read `<release>-secrets`, find nothing, and a live client whose id lives only in the Secret (the backend#2571 shape the chart now recommends) reads as UNIDENTIFIABLE - so `diagnose` and `upgrade` treat it as having no id. The override is already in the values both callers have open, so the effective prefix costs one more read of the same file rather than a second API call. Absent -> the release name, which is the chart`s own `default .Release.Name .Values.fullnameOverride`. Four tests, two per language, and the CONTROL is the load-bearing half: without it the fix is satisfied by always using the override key, which would break every ordinary release. Mutation-proved in both directions, in both languages - reverting to the release name fails the override case, always using the override fails the control. 2. MY DEFINE PARSER STOPPED AT THE FIRST `end`. `{{- define "x" -}}(.*?){{- end -}}` is non-greedy, so a helper containing an inner `if`/`range` closed at the INNER end and its tail was silently dropped. Measured: 28 of 55 helper bodies truncated. IT CHANGED NO ANSWER TODAY - no helper`s `tracebloc.fullname` reference happens to sit in a dropped tail, so the routed-helper closure came out at 21 either way. That is why it was worth fixing rather than noting: the guard`s coverage depended on WHERE in a helper an include happened to sit, and one edit moving an include below an `if` would silently un-route it - after which a routed Secret lookup reads as unrouted and assertion 5 stops requiring a mitigation. Replaced with a balanced parse over the `{{ }}` ACTIONS, so an `end` inside a string or comment cannot close a block early. A permanent self-test feeds it the exact shape the regex dropped - a helper whose `fullname` reference sits after an inner `if`/`end` - because the chart contains no such helper today and a check only exercisable by a bug already present arrives too late. Both halves asserted: the routed probe must be seen, the unrouted probe must not. VERIFIED on helm v3.15.4 (the CI pin): make drift 35/35 (manifest.sha256 regenerated - the two installer files are hashed there); helm unittest 631/631; Pester 776 passed / 0 failed / 15 skipped; the two new bats cases green; shellcheck -S warning -x clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): assert the SPECIFIC refusal, gate the interpreter, and correct the prose the refusal falsified (backend#2626) Asad`s five remaining items, and two of them were defects in my own guard. 1. THE SELF-CHECK WAS SATISFIED BY ANY NON-ZERO EXIT. Written as `if …; then error; else OK; fi`, so with python3 absent (rc 127) the sidecar never ran and it printed [OK]. The one assertion whose stated purpose is "an unreachable refusal is one nobody notices has stopped refusing" reported green for a reason unrelated to what it checks - the bare `assertRaises(Exception)` shape, in shell (CLAUDE.md rule 10). It now requires EXIT 1 specifically, and treats any other code as a cannot-tell finding. 2. NO INTERPRETER GATE, where 23 siblings have one. python3 absent gave rc 127, missed the exit-2 branch, and ended on "fullnameOverride is incomplete in 4 profile check(s)" - a missing tool reported as a chart defect, the exact misdiagnosis the module half was fixed for. Added in the siblings` `fail_closed` idiom, for python3 AND helm. Measured: absent python3 now exits 2 with "THIS IS A MISSING TOOL, NOT A VERDICT ON THE CHART". And that work exposed a third: my PyYAML guard caught `ModuleNotFoundError`, which is a SUBCLASS of `ImportError` - so it missed the parent, and a plain `ImportError` is exactly what `pyyaml-preflight.bats` injects to simulate an absent PyYAML. Under the repo`s own simulation the guard gave a TRACEBACK and rc 1 while the bats class rule stayed green, because it reads the AST and accepts either name. Widened to `ImportError`, which also covers the real-world broken-install case. Verified: rc 2 with the named refusal. 3. `fullnameOverride: ""` was schema-INVALID. The pattern required at least one character, so the chart`s own commented example at values.yaml would have been rejected if uncommented verbatim, while the template treats "" as unset (Arturo). Pattern now accepts the empty string; Bad_Name and a 54-char value still fail at template time. 4. values.yaml said changing it on an existing install is "a migration, not a config tweak". It is REFUSED outright. That was the most misleading line in the file - it invited planning a migration the chart will not start. 5. values.yaml said the chart "ALWAYS emits `<release>-secrets`", with pre-create commands using that name. False under an override, and the consequence after the PVC re-key is a tier-3 hard fail on clientId for an operator who followed the doc correctly. It now says the name follows the override and gives a command to read it off the render rather than guess. 6. THE REFUSAL BLOCKED A DOCUMENTED RECOVERY PATH. docs/MIGRATIONS.md Option C is uninstall -> clear claimRef -> re-create PVCs -> install. Only the PVCs carry `resource-policy: keep`, so uninstall deletes the Secret and the re-install renders PVC-present + Secret-absent -> refused, with a primary remedy ("copy the credentials") that has nothing left to copy from. Option C now saves the Secret in a step 0 and restores it under the new effective name in a step 4, and the refusal message names `kubectl delete pvc mysql-pvc` as the accept-data-loss path so the failure is escapable from the failure itself. NOT CLAIMED: the `--dry-run=server` matrix was run against ea6568d, before the PVC re-key. Asad flagged that and he is right - it is not measured for this version and is not recorded as such. VERIFIED on helm v3.15.4: make drift 36/36; helm unittest 640/640; client-credentials-have-a-secret-tier 17 assertions; pyyaml-preflight 3/3; chart-pull-secret green; shellcheck clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): accept a renamed release`s token Secret, and scope the path-class check to the chart (backend#2626) Three Bugbot findings on f585de1. One demoted with evidence, two fixed. 1. HIGH, DEMOTED: "Path class fails on cloud profiles". It does not, at head. Measured on all four CI profiles: aks 1, bm 4, eks 1, oc 1 - the cloud profiles each render ONE release-scoped path, the Collector`s queue directory `/var/lib/tracebloc/<release>/telemetry`, because `telemetryCollector` is on by chart DEFAULT and no client/ci profile sets it. The required drift guard is green and does not refuse a complete chart. The HAZARD is real though, and the finding named the right one with the wrong scope: the gate is tri-state since backend#1906, so a profile that disabled the Collector AND hostPath would have no release-scoped path, and a PER-PROFILE emptiness check would then refuse a complete chart. So the emptiness assertion moved from per-profile to ACROSS profiles - a profile with none is a `[note]`, a CHART with none anywhere is still an error, because "no path followed the override" is equally true of a chart that stopped scoping paths by release. Proved both ways: a 5th profile disabling both keeps the guard green (the false positive that would otherwise have fired), and neutering the path class reddens it. 2. MEDIUM, FIXED: the token lookup missed a renamed release`s Secret. `telemetryTokenPresent` accepted the override-following name and the legacy FIXED name, but not `<release>-telemetry-token` - so on a renamed release `telemetryCollectorState` hard-FAILED for an operator who had explicitly enabled the Collector, naming two names that were never going to match while the token sat there under a third. Accepted now, and named in the refusal. Accepted rather than refused, which is deliberately the opposite call from the credentials Secret: there a name miss means silently minting a password against a datadir holding the old one. The token is server-side and re-derivable (jobs-manager writes it, backend#2274), so finding the existing one is safe and is what the operator meant. Nothing pinned the accepted-name set, so nothing would have caught it going away again. `telemetry-token-agreement.sh` now asserts an AGREEMENT rather than a list: every name the lookup accepts must be reported by the refusal. Placeholders are COUNTED against arguments, because the first cut checked that the name appeared on the line - and the format string and its args share one line, so deleting a `%q` left the substring matching while printf silently dropped the argument. Caught by mutation-proving; the counting lives in a `.py` sidecar, which is inside pyyaml-preflight`s class rule now that it covers `.py`. 3. MEDIUM, ALREADY FILED: the installer still builds `{namespace}-jobs-manager` in six places. That is backend#2888, filed with the measurement - six reconstructing sites, two that already DISCOVER by pattern and are override-safe, and the structural fix (select on `app.kubernetes.io/instance`). It is a change to the installer`s naming model, not a patch, and it should land before fullnameOverride is recommended to operators. VERIFIED on helm v3.15.4: make drift 36/36; helm unittest 640/640; telemetry-token-agreement green and mutation-proved 4 ways; pyyaml-preflight 3/3; shellcheck clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(chart): bump to 1.9.91 — develop reached 1.9.89 while this PR was open * fix(chart): make the token refusal escapable after an override-to-override rename (backend#2626) Bugbot, Medium: `telemetryTokenPreOverrideName` covers `<release>-telemetry-token`, so unset->A and mid-migration are handled, but A->B leaves the token under A`s name and the render hard-fails naming three names none of which will match. Correct, and the fix is NOT a fourth candidate name. WHY NOT ENUMERATE. Measured across the three consumers: the Collector`s volume (telemetry-collector-daemonset.yaml:197), the RBAC`s resourceNames (telemetry-token-rbac.yaml:102) and jobs-manager`s writer env (jobs-manager-deployment.yaml:351) ALL resolve `tracebloc.telemetryTokenSecretName` and nothing else. So a token discovered under any other name is one nothing is permitted to read - enumerating the namespace would turn a loud refusal into a green render with a Collector that cannot mount its token. That is strictly worse than the hard fail. What makes the hard fail acceptable is that it is ESCAPABLE, and it was not. The message now names the rename as the likely cause and gives the two ways out, neither of which is a reinstall: * copy the Secret to the name this render wants (the command is in the message) * leave telemetryCollector.enabled unset for one upgrade and let jobs-manager re-mint under the new name on its next re-authentication The second is safe because the Collector`s mount is `optional: true` and the daemonset says why in terms - "buffers until the token arrives" rather than "CrashLoopBackOff on every node" - so the window costs buffered telemetry, not node health. NO NEW PLACEHOLDERS, so telemetry-token-agreement.sh`s placeholder/argument arity check still holds; verified green. One design question left for the reviewer rather than decided here: whether the gate should skip the refusal entirely on a LIVE release, keying on the persisted MySQL PVC the way the credentials refusal now does. That covers every rename direction with no name arithmetic, and it costs the fresh-install protection on live releases. It changes the gate`s invariant, so it is the reviewer`s call and the evidence is on the thread. VERIFIED on helm v3.15.4: make drift 38/38; helm unittest 641/641; telemetry-token-agreement green; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(guard): make the fallback mitigation the property, not the arity (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(manifest): regenerate after the guard-mitigation change (backend#2626) `make drift` was red on `gen-manifest.sh --check`: 9eb3c7e changed scripts/lib/summary.sh and scripts/install-k8s.ps1 without regenerating scripts/manifest.sha256, so the R8 static-analysis gate saw two stale digests. Manifest-only. Verified: make drift 39/39, helm unittest 641/641, bats 69/69 over the manifest/hygiene/style suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): the rename remedy is incomplete on hostPath, and said otherwise (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): a sidecar exit outside {0,1} is could-not-run, not a chart defect (backend#2626) The last half of @saadqbal`s interpreter finding. 9eb3c7e/head fixed the fabricated path-class claim - assertion 6 now reports NOT CHECKED rather than "NO profile rendered a release-scoped on-disk path" - but the final verdict still blamed the chart: $ PATH=<python3 that exits 127> bash scripts/tests/fullname-override-completeness.sh [ERROR] fullnameOverride is incomplete in 5 profile check(s) # rc 1 `if [ "$rc" -eq 2 ]` handled exactly 2, and `[ "$rc" -eq 0 ] || failures++` swept every other code into "the chart is incomplete" - 127 from a stale pyenv shim or a dangling symlink, 126 from a non-executable interpreter, 137 from an OOM kill. Now classified: 0 clean, 1 a finding, ANYTHING ELSE exits 2 with the named refusal. $ PATH=<python3 that exits 127> … [ERROR] the assertions exited 127, which is not a verdict … NOTHING ABOUT fullnameOverride WAS CHECKED. # rc 2 AND THE PREFLIGHT I ADDED CANNOT COVER THIS, which the comment now says because it read as though it could: `command -v python3` answers "is there something on PATH by that name", not "does it run". A shim that execs a deleted interpreter passes it and exits 127 here. Mutation-proved: folding 127/126 back into `failures` restores the false "incomplete in N profile check(s)" claim and rc 1; the classification restores rc 2 and zero such claims. VERIFIED at head on helm v3.15.4: make drift 39/39; helm unittest 641/641; shellcheck -S warning -x clean; manifest --check clean. Part of tracebloc/backend#2626 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(guard): "not checked" is not a pass, and the fallback detector gets a fixture (backend#2626) Two from @saadqbal, and they are the same sentence twice: an absence is not an answer. 1. The pathclass_missing branch printed NOT CHECKED and fell through to exit 0. The two causes it enumerates both raise `failures` elsewhere, so the branch looked safe -- but a third does not: rename the PATHCLASS marker in the sidecar, a pure refactor with every assertion intact, and assertion 6 stops asserting while DRIFT_GUARDS reports a pass. Now `could_not_run` -> exit 2, the vocabulary the interpreter loop already owns; a confirmed finding still outranks it, so a real defect is reported as one and not as "cannot tell". Mutation-proved with the anchor asserted: PATHCLASS -> PATH_CLASS took the guard from rc=0 to rc=2. 2. _fallback_on_line was pinned by nothing -- regress it to the file-wide `count("lookup ") >= 2` and the real chart still renders green, because the chart does not contain the shape that separates the two. Written-down fixture, same reasoning as _SELFTEST_TEMPLATE: two lookups both routed (the half the old form gets wrong), a literal-keyed real fallback, a single routed lookup, and the $var indirection. Mutation-proved: the old form reddens the first and the $var case. `make drift`: all 39 guards green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(chart): a path is not an object, and the PV caveat outlived the refusal (backend#2626) Two Bugbot findings on the pvCaveat added in 91e95f4. High: the caveat rendered ONLY inside the credential refusal, which is skipped once a Secret exists under the new name -- so an operator on their second attempt read the copy-the-Secret remedy with the caveat missing. values.yaml made it worse by saying "the hostPath PVs keep the RELEASE name": true of the on-disk PATH, false of the PersistentVolume OBJECTS, which are named through tracebloc.fullname and carry no helm.sh/resource-policy: keep. Following that on bare metal deletes the PVs and leaves the retained PVCs Bound to them. Separated path from object in values.yaml, and NOTES.txt now carries the caveat on every hostPath render -- the path the refusal cannot reach. Gated on hostPath alone, not on fullnameOverride being set, because the A->unset rename is the same hazard and would have gone quiet. Medium: .Values.hostPath.enabled read without the (default dict ...) guard the rest of the chart uses. Guarded. Note the stated consequence does not hold as written: with hostPath nil the render already dies in shared-images-pvc.yaml:3, before secrets.yaml, both before and after this commit. Nine other unguarded sites remain and are filed separately -- one of ten does not fix the nil case. make drift: all 39 guards green. NOTES verified rendered under --set hostPath.enabled=true and absent when false. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(chart): the PV rename hazard gets a tripwire, and a refusal to go with the notice (backend#2626) Builds on 2cd99cd rather than replacing it. That commit fixed both Bugbot findings by correcting the values.yaml conflation and repeating the caveat in NOTES.txt; this adds the two things it left implicit. A REFUSAL, alongside the notice. NOTES.txt and this guard cover different halves and neither substitutes for the other. NOTES.txt renders on every hostPath install including dry-runs, so it is the half that is always visible -- but it prints after a successful render and cannot stop the upgrade that deletes the PVs. The new guard in secrets.yaml blocks that upgrade: it compares the retained claim's spec.volumeName against the volume this render names, and refuses when they disagree. It reads the claim rather than doing a PV lookup because spec.volumeName is written by the binding controller, needs no cluster-scoped RBAC, and cannot come back empty-and-look-renamed the way a forbidden lookup would. Scoped to hostPath because off hostPath the provisioner names volumes pvc-<uuid>, which would make it refuse every storage-class install. It names the observed pair and both causes -- a rename, or hostPath switched on over a dynamically provisioned claim -- rather than asserting which. A TRIPWIRE UNDER THE PROSE. Nothing tested any of this. hostpath_pv_rename_test pins the four facts both comments now assert, from render output rather than from a restatement: PV names follow the override, claim names do not, claims carry helm.sh/resource-policy keep, and the PVs do NOT. That last pair is the tripwire -- annotating the PVs keep is the real fix for this hazard, and when someone does it those cases go red and force the values.yaml paragraph and this guard to be revisited in the same PR. Each fact is asserted with the override both set and unset, since the set-only form would pass on a chart that named everything renamed-* unconditionally. Two things measured along the way. The claim's spec.volumeName is set by no template, so the chart test asserts the binding from the PV's claimRef end and says so; and on helm-unittest 0.5.2 a documentIndex is silently ignored when a suite scopes several templates and a test narrows with template:, evaluating against document 0 while reporting DocumentIndex 0 -- all six claim tests here ran against the PersistentVolume before switching to documentSelector. Also: a scoped regression test for the nil-guard, which had none. It is provable only template-scoped, because a whole-chart render under hostPath: null still dies at shared-images-pvc.yaml:3 either way. And 2cd99cd's new comment said the guard was "like every other hostPath read in this chart" -- measured 8 guarded, 9 bare on this tree, so the sentence is corrected and points at backend#2910. Filed a duplicate of that ticket (#2911) and closed it, moving its three additional measurements onto #2910. helm unittest 654/654 (was 641). make drift 39/39. Every claim mutation-proved: PV gains keep -> 1 red; PV name stops following -> 1 red; claim name starts following -> 3 red; host path starts following -> 1 red; nil-guard reverted to the bare read -> the secrets case reddens with the nil pointer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(guard): the identity-env class also has to name the right identity (backend#2626) Layered on 7dc86c2's `identity_env_sites`, which landed the same Bugbot Medium concurrently. Both of us reproduced the fail-open and fixed it differently; this keeps that commit's fix as the base and adds the two cases it cannot see, plus the chart-level backstop. WHAT 7dc86c2 ALREADY DOES, kept unchanged: derives the site set from the render with the override UNSET and requires every site to still be there with it set, so a routed site is a DISAPPEARANCE rather than an absence. That is the better half of the two approaches -- it catches a site vanishing for any reason, not only a routed value -- and replacing it with a same-shaped enumeration of my own would have been a discard, not a merge. WHAT IT CANNOT SEE, and both are now checked: * `identity_env_sites` accepts a value in (rel, ns) for ANY of the three names, so swapping RELEASE_NAME with RELEASE_NAMESPACE keys the same triple on both sides and passes -- while a consumer reading RELEASE_NAME gets the namespace. Each name is now checked against its own meaning. Assertable only because this profile deliberately makes rel and ns differ; under the installer's one-string-for-both convention (backend#2621) it would not be. * A `valueFrom` env has no literal to read, so it drops silently out of BOTH site sets and reads as agreement. It is now reported as a site the check cannot see, which is the finding rather than a pass (rule 3). AND PER-PROFILE EMPTINESS IS NOT A CHART FINDING. 7dc86c2 failed the guard when the default render named zero identity envs, which would refuse a complete chart on a profile rendering neither CronJob nor the storage-assertions Job -- the mistake the PATH class already made and had demoted after measuring. Replaced with the reviewed PATHCLASS shape: an `ENVCLASS` count out of the sidecar, asserted once ACROSS profiles by the shell (new assertion 7), where a missing marker exits 2 (could-not-run) instead of 0. Measured at head: aks 5, bm 5, eks 5, oc 5. Also took 7dc86c2's NOTES wording over mine -- same conclusion, tighter -- and added one clause it lacked: that the destructive upgrade is now refused at template time, so the notice tells the operator they are protected rather than only that a hazard exists. Mutation-proved, and the split is the point: routing one RELEASE_NAME -> exit 1 (both layers fire); swapping RELEASE_NAMESPACE -> exit 1 with ONLY the new per-name check firing, the site comparison silent; renaming the ENVCLASS marker -> exit 2 with NOT CHECKED; stripping the envs from all three templates -> exit 1 naming the chart-level cause. helm unittest 656/656. make drift 39/39. shellcheck clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(chart): force egress-enforcement and registry-Secret into the completeness denominator (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(chart): bump to 1.9.92 for the completeness denominator change (backend#2626) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(completeness): demote CLS_ENV emptiness, and pin the refusal detector (backend#2626) Both findings from @saadqbal review, plus the base merge. 1. CLS_ENV EMPTINESS WAS HALF-DEMOTED (Bugbot Medium; he reproduced it). The identity-env block already treated per-profile emptiness as legitimate and printed `ENVCLASS 0` -- but the STAYED empty-class arm set `fail = True` first, so that half could never rescue the run. Reproduced here on aks with autoUpgrade, imageRefresh and sealCheck.storageAssertions.enabled all off: 38 documents, and the ORIGINAL prints "[ERROR] found NO RELEASE_NAME ..." and "ENVCLASS 0" in ONE output. A required drift guard refusing a complete chart. AND THE ONE-LINE FIX WOULD HAVE BROKEN THE CROSS-PROFILE ASSERTION. Extending the demotion is right, but it must NOT also print `ENVCLASS 0` the way CLS_PATH prints `PATHCLASS 0`: the shell parses `grep -E ^ENVCLASS | head -1` and the identity-env block below prints the REAL count unconditionally, so a zero here would be read first and the true value discarded -- turning a cross-profile assertion into one that always sees 0. CLS_PATH can print its count because its two branches are mutually exclusive; CLS_ENV count is owned by the block below. Stated in the code. STILL LATENT, and I am not claiming a test covers it: all four CI profiles name 5 identity envs, so disabling the demotion does NOT redden the suite. The evidence is the live reproduction above, not a guard. Pinning it needs either a fifth profile or a specimen driving assert_stayed empty arm -- happy to add the latter. 2. THE REFUSAL DETECTOR HAD NO SPECIMENS AT ALL, which is the asymmetry that made his mutation possible: `_fallback_on_line` has three pinning lines, `_mitigations` had none. `return {"refusal"}` unconditionally left assertion 5 printing "all 2 routed Secret lookup(s) of 2 carry a mitigation" and exiting 0, both existing selftests green -- the detector deciding whether a routed credential Secret is mitigated could be DELETED and nothing in the tree would say so. Four specimens now, written down rather than hunted for in the chart: a real refusal, a negation that merely branches, a `fail` reached by negating an UNRELATED variable (the routed_vars discrimination), and a `fail` below the 4-line window. Mutations, all reddening: his exact `return {"refusal"}` (3 of 4 halves fire); widening the window 4 -> 40; dropping the per-var discrimination. A fourth came back INERT and its anchor assertion said so rather than reporting a pass. 3. BASE MERGED, both conflicts as he specified. Makefile: develop block plus one line, verified by comparing guard sets programmatically -- 39 on develop, 40 here, the single delta being fullname-override-completeness.sh -- rather than by eye, because `--ours` would delete #933 empty-entry refusal and `--theirs` would drop this guard from the list entirely. manifest.sha256 REGENERATED from the merged tree. Guard at head: rc=0, 60 OK, 0 ERROR, aks=5 bm=5 eks=5 oc=5 -- the 56/0 baseline he measured, plus the four new selftest lines. drift-list-integrity 9/9, gen-manifest --check clean. black is NOT run by this repo CI and the file was already non-black-clean (14 hunks before and after, none in the added code), so it is left alone rather than reformatted into an unrelated diff. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(completeness): the preflight does NOT make 127 unreachable (backend#2626) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
/fr-pass Functional review on staging — passed, with direct evidence. Journey (tier A), install via the real installer → client components healthy → CLI installed from its signed release and signed in → dataset ingested for every task type → use case published → model trained and the leaderboard read. The train leg, which is the one that matters: This repo's change is on the path that run exercised, so this is functional evidence rather than an inference from code review. Two things stated rather than glossed:
|
Part of tracebloc/backend#2626
DRIFT_GUARDSis one|-separated line, so it conflicted every time two branches each added a guard — three times on client#911 in a single day.The obvious claim is false, and I checked rather than asserting it
One-guard-per-line does not make those appends auto-merge. Appending requires editing the previous last line to add its
|\continuation, so both sides still touch the same line and git still conflicts. Measured both ways on the real 38-entry list:So what changes is whether the conflict can be resolved by reading it. At 1842 characters, finding which guard each side added means diffing two long strings by eye — and the resolution that looks right in a diff viewer is "take one side", which silently deletes the other branch's gate. A required check stops existing and every run still reports green, which is this file's own subject.
This buys legibility at the moment of highest risk, not automation. The Makefile comment says exactly that rather than the tidier version.
It also closes a fail-open the new layout makes easy to type
A
||in the middle of the list yields an empty entry.sh -c ""exits 0 andranstill increments, so the count check passes and the run reports green with one of its guards being the empty string. Measured before the fix:A trailing
|was already caught — thefordrops a trailing empty field, soranfalls short ofexp— which is why only the middle case needed a new guard. Entries are also trimmed, so a whitespace-only entry is refused too, and==>lines no longer carry the leading space Make's continuations insert.The recipe's guards were asserted by a comment
That's the part worth having. The three fail-open protections in the
driftrecipe were documented in prose and had been found by hand twice — #755's quote-collapse, and this doubled-|case. New:scripts/tests/drift-list-integrity.bats, 8 cases.Every case drives
make driftitself with a crafted list rather than re-implementing the splitting logic, so a copy of the rule cannot go on passing while the recipe drifts. It also fails closed if the committed list ever shrinks below 20 entries — otherwise the crafted cases would stay green whilemake driftgated almost nothing.Mutation-proved — each of the four refusals is independently load-bearing:
||and whitespace cases)Not done, and named in the Makefile
Deriving the list from
scripts/tests/*.shwould remove the conflict entirely and make an added guard file auto-merge — and it would stop the list being a second source of truth about which files are drift guards.It is left out because it changes which scripts run: a new
.shin that directory never meant as a drift guard would start gating merges. That needs its own change and its own opt-out list, and I would rather propose it than smuggle it in here.Test plan
make drift— 38/38 guards greenCI=true bats scripts/tests/*.bats— 1603 passing, 0 failures (was 1595; +8)bats-hygienegreen — every assertion in the new file is|| return 1hardenedshellcheck -S warning -x -s bashclean on the new filescripts/gen-manifest.sh --checkclean|, doubled|, and a quote-containing guard that must not collapse the listMakefile + one new test file only — no chart content, so no version bump.
🤖 Generated with Claude Code
Note
Low Risk
Changes only Makefile drift orchestration and new unit tests; no chart, auth, or runtime behavior.
Overview
Reformats
DRIFT_GUARDSin the Makefile to one guard per line (with|\continuations) so merge conflicts show a short differing line instead of a ~1800-character single line—reducing the risk of resolving conflicts by taking one side and silently dropping a guard. Thedriftrecipe now trims each entry (Make continuation spaces) and fails on empty or whitespace-only entries, closing a fail-open where||or| |madesh -c ""count as a passing guard while the run still reported green.Adds
scripts/tests/drift-list-integrity.bats: eight Bats cases that invokemake driftwith crafted lists (empty list, trailing|, doubled|, quotes, etc.) plus a floor that the committed list has at least 20 guards—machine-checking protections that were previously only documented in comments.Reviewed by Cursor Bugbot for commit 26a68e4. Bugbot is set up for automated code reviews on this repo. Configure here.