Uh oh!
There was an error while loading. Please reload this page.
test(installer): the bats host is an input — declare it, don't inherit it (backend#2208) - #759
Merged
Merged
Conversation
…t it (backend#2208) Three install_client_helm flow tests passed on CI and failed on every Apple Silicon Mac: install_client_helm: same client re-run is allowed (upgrade in place) install_client_helm: same client in a different namespace -> upgrades in place, no duplicate install_client_helm: different-namespace reconcile works WITHOUT jq (Bugbot #284) MySQL 5.7 is required here, but amd64 emulation could not be verified on this Apple Silicon Mac — enable Rosetta and re-run (...) client#756 made _assert_engine_runs_on_this_arch run on macOS as a fail-closed backstop, and it probes the REAL host: `uname -m`, plus a live Rosetta/Docker smoke on Darwin. setup() pinned neither OS nor ARCH, so those tests inherited the developer's CPU. Only these three, because only these three mock `helm list` into an EXISTING release — the one path that resolves the engine to 5.7 and therefore engages the gate. Every other flow test resolves fresh -> 8.4 and returns early. CI is Linux/amd64, so the gate returns early at the arch check and Unit tests stays green. The one platform the gate exists FOR is the one CI never runs, so CI cannot see this by construction — a red local suite that CI calls green. Pin OS=Linux / ARCH=x86_64 in setup(), i.e. to what CI actually is, so a local run reproduces CI. This keeps the gate ARMED: TRACEBLOC_ALLOW_ARM64 would have switched off the very behaviour client#756 added, which is why it is not used. Every arch-sensitive test in the file already sets OS/ARCH for itself (_arch_gate_ctx and the engine tests), so they override this and still drive the real rule. No new gate tests: client#756 already covers both arms (emulation ok -> proceeds, missing -> refuses, helper absent -> refuses fail-closed, 8.4 -> skips, escape hatch -> skips). Adding more would duplicate, not strengthen. Also fixes a comment this exposed: the block above the function still read "macOS is NOT covered here: this gate is Linux-only ... deliberately out of scope", directly above the Darwin arm client#756 added. Prose that contradicts the code teaches the bypass. Mutation-proved on an arm64 host: - drop the setup() pin -> the same 3 tests redden again - break the gate's Darwin arm -> 2 of its own tests redden (gate still armed, so the pin fixed the tests without disarming it) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saadqbal
previously approved these changes
Aug 20, 2026
saadqbal
left a comment
Contributor
There was a problem hiding this comment.
Verified the reasoning against the source rather than the description, and it holds:
_assert_engine_runs_on_this_archreads${ARCH:-$(uname -m)}and${OS:-}, so the pin returns at thex86_64arm before the OS probe is reached — that's why the three existing-release reconcile tests were the only ones affected (they're the only flow tests whosehelm listmock resolves the engine to 5.7).- The gate stays armed:
_arch_gate_ctxsets its ownOS=Linux; ARCH=aarch64, and the engine tests at ~1245-1584 each setARCHexplicitly, so they overridesetup()and keep driving the real rule. UsingTRACEBLOC_ALLOW_ARM64=1instead would have disarmed it — good call not to. - The comment fix is the real find. The old prose said "this gate is Linux-only … deliberately out of scope" while sitting directly above the
Darwin)arm #756 added. That's the kind of stale comment that gets trusted over the code.
Pinning the host in setup() is the right fix — a unit test's verdict depending on the developer's CPU is the bug, not the symptom.
# Conflicts: # scripts/manifest.sha256
aptracebloc
approved these changes
Aug 20, 2026
aptracebloc
left a comment
Contributor
There was a problem hiding this comment.
Approving.
The diagnosis holds: _assert_engine_runs_on_this_arch (client#756) probes the real host, and setup() pinned neither OS nor ARCH, so the three flow tests whose helm mock reports an existing release — the only path that resolves the engine to 5.7 and engages the gate — inherited the developer's CPU and failed on Apple Silicon, invisibly to a Linux/amd64 CI that returns early at the arch check.
The fix is the right shape: pin OS=Linux/ARCH=x86_64 so a local run reproduces CI, and pointedly notTRACEBLOC_ALLOW_ARM64, which would disarm the very gate #756 added — the arch-sensitive tests still set their own OS/ARCH, so they keep exercising the real rule. It also corrects the inverted comment that claimed the gate was Linux-only / macOS-uncovered when #756 had made macOS the one path where this is the sole late gate.
Test-only + a comment; CI green, no conflict, no open threads.
— drafted with Claude Code
Uh oh!
There was an error while loading. Please reload this page.
shujaatTracebloc added a commit
that referenced
this pull request
Aug 20, 2026
…iew, #768) Bugbot's Medium, confirmed by @saadqbal, and it was a fresh instance of the exact class client#766 exists to remove: the two installers diverging by construction, same file pair, days later, in the fail-unsafe direction. Get-TrainingProvenance performed its OWN `helm get values`, separate from the one Get-TrainingResources did immediately below, each wrapped in its own bare `catch {}`. So on Windows, if the size read succeeded and carried a live RESOURCE_LIMITS but the provenance read then threw -- a wedged API, a ConvertFrom-Json hiccup, anything the catch eats -- the generated values pinned that carried envelope as `installer`. A future ladder trusting that label would re-derive and overrule what may well have been a deliberate human choice. `unknown` is the safe verdict there, and the bash twin already gave it. Fixed by adopting the bash shape rather than patching the symptom: Get-CarriedTrainingValues does the lookup ONCE and returns @{Size; Provenance} or $null. That $null is the point -- either the read succeeds and both resolvers see the same carried pair, or it fails and NEITHER takes the carry path, so the size is machine-derived and `installer` is then correct. Size and provenance can no longer disagree, structurally, rather than by two functions happening to agree. The values generation now does that one lookup and hands it to both via -Carried/-CarriedResolved, which also drops the redundant `helm get values` per install that @saadqbal noted. Both params are optional, so every existing caller and test that invokes the resolvers bare is unaffected. Four Pester tests added for the fail-unsafe path specifically: a values read that returns unparseable JSON must carry nothing and answer `installer` (not pin a carried size as ours); one lookup handed to both resolvers must agree; the historic literal is still treated as the absence of a choice even with a marker beside it; an unreadable namespace carries nothing without throwing. Also rebased onto develop to clear the DIRTY merge state. The only conflict was scripts/manifest.sha256 -- a GENERATED file, so it was regenerated rather than hand-merged. Chart stays at 1.9.50, still ahead of develop's 1.9.49. Verified: Pester 754 passed / 0 failed (+4); bats 175 ok / 0 failed -- develop's #759 fixed the three helm-dependent tests that were failing before, so this branch is now clean rather than clean-except-three; helm lint clean; helm unittest 486 passed with the same 5 pre-existing failures develop has; PSScriptAnalyzer 0 errors; make lint clean over 47 files; embed + manifest checks clean. Refs: backend#2220, client#766, cli#539 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shujaatTracebloc added a commit
that referenced
this pull request
Aug 20, 2026
…end#2220)
A machine below the training floor was handed cpu=2,memory=8Gi. On a box with
~4 GiB allocatable that envelope is LARGER THAN THE MACHINE, so every training
pod stays Pending forever -- precisely the failure install-k8s.ps1's own header
says the machine sizing exists to remove ("dead on arrival on nodes under 8 GiB,
the WSL2 field case"), recreated by its own fallback. And
--reset-then-reuse-values then re-applies that value on every upgrade, forever.
Not a hypothetical band: preflight hard-fails below 5 GB on Linux and only WARNS
on macOS/Windows (PF_MIN_MEM_GB=5, PF_WARN_MEM_GB=8), while its own comment notes
"16 GiB is needed to train locally (a job's limit is ~8 GiB+)". The permitted
band and the unschedulable band overlap.
Root cause was that two situations produced one answer: "I cannot read the
machine" and "I read it and it is too small" both returned empty and both fell
through to the literal. Now separated:
unreadable -> the literal, unchanged. We cannot do better, and warning
about machine size would be a fabrication.
below the floor -> the honest remainder (>= 1 core and >= 1 GiB), which FITS.
A run can be scheduled and fail for a REASON instead of
hanging, and the operator is warned.
below even that -> the literal plus a hard warning. cpu=0 is not a training
request, and refusing the install would break the
run-but-do-not-train-locally case preflight allows.
The warning lives in the CALLER, never in the resolver: _training_resources and
_training_provenance are captured with $(...) and their tests compare the whole
output, so a warn emitted from the resolver would land inside RESOURCE_LIMITS.
Pinned by a test that captures the value and asserts it is clean.
_machine_training_resources keeps its exact old semantics (empty when
non-viable), so its callers and its golden replay are untouched; the new
viability-aware _machine_training_ceiling sits beside it.
Two expectations CHANGED rather than added, and both encoded the bug -- the bats
and Pester "below-floor machine falls back to the static default" tests each
asserted the unschedulable literal as if it were the contract. Neither loses
coverage: the unreadable-cluster tests still pin the case where the literal is
genuinely right, now a distinct branch rather than the same one.
The Pester golden replay is strengthened, not just repaired. It mapped EVERY
non-viable vector onto the literal, which is what let this bug live inside a
passing replay; it now checks them against the contract's own render_gi. Those
numbers already matched -- wsl2-too-small-4gi renders 3c/1Gi, exactly what the
installer now writes -- so the accessor and the installer agree on small machines
too, and that agreement is asserted.
Rebuilt on top of the current #768 rather than merged into it. #768 has since
taken review fixes and a develop merge that restructured the same two sizing
functions, so a replay produced seven conflict hunks across both installers --
the serialize-don't-parallelize hazard the ticket itself warns about. Re-applying
this self-contained delta was safer than hand-resolving them; the test files
applied from the original patch unchanged.
Verified: bats 179 ok / 0 failed (develop's #759 fixed the three helm-dependent
tests that used to fail); Pester 758 passed / 0 failed; manifest + envelope-embed
checks clean.
Refs: RFC-BACKEND-664 P0, client-runtime#358, client#766, client#768, backend#2167
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>shujaatTracebloc added a commit
that referenced
this pull request
Aug 20, 2026
…end#2220) (#769) A machine below the training floor was handed cpu=2,memory=8Gi. On a box with ~4 GiB allocatable that envelope is LARGER THAN THE MACHINE, so every training pod stays Pending forever -- precisely the failure install-k8s.ps1's own header says the machine sizing exists to remove ("dead on arrival on nodes under 8 GiB, the WSL2 field case"), recreated by its own fallback. And --reset-then-reuse-values then re-applies that value on every upgrade, forever. Not a hypothetical band: preflight hard-fails below 5 GB on Linux and only WARNS on macOS/Windows (PF_MIN_MEM_GB=5, PF_WARN_MEM_GB=8), while its own comment notes "16 GiB is needed to train locally (a job's limit is ~8 GiB+)". The permitted band and the unschedulable band overlap. Root cause was that two situations produced one answer: "I cannot read the machine" and "I read it and it is too small" both returned empty and both fell through to the literal. Now separated: unreadable -> the literal, unchanged. We cannot do better, and warning about machine size would be a fabrication. below the floor -> the honest remainder (>= 1 core and >= 1 GiB), which FITS. A run can be scheduled and fail for a REASON instead of hanging, and the operator is warned. below even that -> the literal plus a hard warning. cpu=0 is not a training request, and refusing the install would break the run-but-do-not-train-locally case preflight allows. The warning lives in the CALLER, never in the resolver: _training_resources and _training_provenance are captured with $(...) and their tests compare the whole output, so a warn emitted from the resolver would land inside RESOURCE_LIMITS. Pinned by a test that captures the value and asserts it is clean. _machine_training_resources keeps its exact old semantics (empty when non-viable), so its callers and its golden replay are untouched; the new viability-aware _machine_training_ceiling sits beside it. Two expectations CHANGED rather than added, and both encoded the bug -- the bats and Pester "below-floor machine falls back to the static default" tests each asserted the unschedulable literal as if it were the contract. Neither loses coverage: the unreadable-cluster tests still pin the case where the literal is genuinely right, now a distinct branch rather than the same one. The Pester golden replay is strengthened, not just repaired. It mapped EVERY non-viable vector onto the literal, which is what let this bug live inside a passing replay; it now checks them against the contract's own render_gi. Those numbers already matched -- wsl2-too-small-4gi renders 3c/1Gi, exactly what the installer now writes -- so the accessor and the installer agree on small machines too, and that agreement is asserted. Rebuilt on top of the current #768 rather than merged into it. #768 has since taken review fixes and a develop merge that restructured the same two sizing functions, so a replay produced seven conflict hunks across both installers -- the serialize-don't-parallelize hazard the ticket itself warns about. Re-applying this self-contained delta was safer than hand-resolving them; the test files applied from the original patch unchanged. Verified: bats 179 ok / 0 failed (develop's #759 fixed the three helm-dependent tests that used to fail); Pester 758 passed / 0 failed; manifest + envelope-embed checks clean. Refs: RFC-BACKEND-664 P0, client-runtime#358, client#766, client#768, backend#2167 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
shujaatTracebloc added a commit
that referenced
this pull request
Aug 20, 2026
…#768) * feat(envelope): record WHO chose the training envelope (backend#2220) Scope bullet 4 -- "migration for edges already pinned by resources set: they must not be stranded". The blocker was never the migration mechanics, it was that provenance is currently unknowable, which Lukas called the actual hard part of the ticket. This adds the marker that makes it knowable from now on, and is honest about the edges where it never will be. Why it cannot be inferred: RESOURCE_* has no unset state once helm's --reset-then-reuse-values has seen it (fleet auto-upgrade CronJob, installer reconcile, and the CLI's own apply all use it). _existing_training_resources carries any value forward unless it equals the historic cpu=2,memory=8Gi, and that is the ONLY discriminator available -- so an installer-written envelope and a deliberate `tracebloc resources set` are indistinguishable. Without a marker, future automatic sizing must either strand every pinned edge or silently overrule operators who chose a size on purpose. env.RESOURCE_PROVENANCE, three states: installer sized to this machine at install time user explicit `resources set`, or a TRACEBLOC_TRAINING_RESOURCES override unknown carried forward from before this key existed `unknown` is deliberate, not a placeholder. Guessing `installer` there would risk overruling a human, so consumers MUST treat unknown as user. Existing edges report unknown and keep their size until someone opts in explicitly -- offering that opt-in is a product decision, not something P0 should make silently. An existing marker is authoritative and is preserved across re-install, in both installers. Without that, a re-install would quietly downgrade a `user` choice to `unknown`, and the marker would be worse than useless -- it would decay toward the ambiguity it exists to remove. Pinned in bats and Pester. _training_resources is now a wrapper over _resolve_training_size, which returns the size AND the verdict from one pass. Deriving them separately would mean either running the bounded cluster probes twice or re-implementing the branch logic beside itself -- the exact duplication this ticket deletes. All 15 pre-existing size tests pass unmodified. The marker rides inside the same template gate as the pair it describes: a provenance marker with no envelope to describe is noise, and gating them together means one explicit unset clears all three. Also excluded from both generic env passthrough loops, or it would render twice per container. MIGRATION.md carries the un-pin recipe, VERIFIED rather than assumed: helm deletes null-valued keys during value coalescing, so `--set env.RESOURCE_*=null` drops all three vars and makes the derive path reachable -- confirmed by rendering the chart both ways (6 occurrences set, 0 with null). That is the explicit helm-side unset the ticket's trap 2 asks for, and it needs no chart change. The recipe carries a warning: derivation is gated OFF (backend#2167), so clearing the keys today returns the edge to the fixed literal, which on a sub-8GiB machine cannot schedule at all. Chart 1.9.48 -> 1.9.49, version + appVersion in lockstep. manifest.sha256 regenerated: both installers moved. Verified: bats 172 ok (same 3 pre-existing failures clean develop has, #64-66 -- they need real helm), 8 new provenance tests; Pester 750 passed / 0 failed, 7 new; helm unittest 480 passed vs 477 on develop, so 3 added and no new failures among the 5 pre-existing; helm lint clean; PSScriptAnalyzer 0 errors; gen-envelope-embed --check clean. Refs: RFC-BACKEND-664 P0, client-runtime#358, client#766, cli#538, backend#2167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(chart): bump to 1.9.50 — 1.9.49 was taken by #767 The chart-version guard failed against the new base: client#767 landed on develop and claimed 1.9.49 while this branch was open, so relative to that base this PR bumped nothing. Same collision shape as two migrations picking the same number off one parent — the version has to be re-checked after develop advances, not chosen once when the branch is cut. Rebased onto develop (3c40ef9 -> 16adfc7) and moved to 1.9.50, version + appVersion in lockstep. Verified after the rebase: my provenance env block and #767's GPU_LIMITS gate coexist cleanly in jobs-manager-deployment.yaml; helm lint clean; helm unittest 486 passed vs 477 on develop with the SAME 5 pre-existing failures and 5 errors, so 9 added and none broken; embed check, manifest check, chart-version-guard 23/23 and the provenance/envelope/training-size bats all green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): one shared lookup for size + provenance (Bugbot + review, #768) Bugbot's Medium, confirmed by @saadqbal, and it was a fresh instance of the exact class client#766 exists to remove: the two installers diverging by construction, same file pair, days later, in the fail-unsafe direction. Get-TrainingProvenance performed its OWN `helm get values`, separate from the one Get-TrainingResources did immediately below, each wrapped in its own bare `catch {}`. So on Windows, if the size read succeeded and carried a live RESOURCE_LIMITS but the provenance read then threw -- a wedged API, a ConvertFrom-Json hiccup, anything the catch eats -- the generated values pinned that carried envelope as `installer`. A future ladder trusting that label would re-derive and overrule what may well have been a deliberate human choice. `unknown` is the safe verdict there, and the bash twin already gave it. Fixed by adopting the bash shape rather than patching the symptom: Get-CarriedTrainingValues does the lookup ONCE and returns @{Size; Provenance} or $null. That $null is the point -- either the read succeeds and both resolvers see the same carried pair, or it fails and NEITHER takes the carry path, so the size is machine-derived and `installer` is then correct. Size and provenance can no longer disagree, structurally, rather than by two functions happening to agree. The values generation now does that one lookup and hands it to both via -Carried/-CarriedResolved, which also drops the redundant `helm get values` per install that @saadqbal noted. Both params are optional, so every existing caller and test that invokes the resolvers bare is unaffected. Four Pester tests added for the fail-unsafe path specifically: a values read that returns unparseable JSON must carry nothing and answer `installer` (not pin a carried size as ours); one lookup handed to both resolvers must agree; the historic literal is still treated as the absence of a choice even with a marker beside it; an unreadable namespace carries nothing without throwing. Also rebased onto develop to clear the DIRTY merge state. The only conflict was scripts/manifest.sha256 -- a GENERATED file, so it was regenerated rather than hand-merged. Chart stays at 1.9.50, still ahead of develop's 1.9.49. Verified: Pester 754 passed / 0 failed (+4); bats 175 ok / 0 failed -- develop's #759 fixed the three helm-dependent tests that were failing before, so this branch is now clean rather than clean-except-three; helm lint clean; helm unittest 486 passed with the same 5 pre-existing failures develop has; PSScriptAnalyzer 0 errors; make lint clean over 47 files; embed + manifest checks clean. Refs: backend#2220, client#766, cli#539 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): never write an envelope larger than the machine (backend#2220) (#769) A machine below the training floor was handed cpu=2,memory=8Gi. On a box with ~4 GiB allocatable that envelope is LARGER THAN THE MACHINE, so every training pod stays Pending forever -- precisely the failure install-k8s.ps1's own header says the machine sizing exists to remove ("dead on arrival on nodes under 8 GiB, the WSL2 field case"), recreated by its own fallback. And --reset-then-reuse-values then re-applies that value on every upgrade, forever. Not a hypothetical band: preflight hard-fails below 5 GB on Linux and only WARNS on macOS/Windows (PF_MIN_MEM_GB=5, PF_WARN_MEM_GB=8), while its own comment notes "16 GiB is needed to train locally (a job's limit is ~8 GiB+)". The permitted band and the unschedulable band overlap. Root cause was that two situations produced one answer: "I cannot read the machine" and "I read it and it is too small" both returned empty and both fell through to the literal. Now separated: unreadable -> the literal, unchanged. We cannot do better, and warning about machine size would be a fabrication. below the floor -> the honest remainder (>= 1 core and >= 1 GiB), which FITS. A run can be scheduled and fail for a REASON instead of hanging, and the operator is warned. below even that -> the literal plus a hard warning. cpu=0 is not a training request, and refusing the install would break the run-but-do-not-train-locally case preflight allows. The warning lives in the CALLER, never in the resolver: _training_resources and _training_provenance are captured with $(...) and their tests compare the whole output, so a warn emitted from the resolver would land inside RESOURCE_LIMITS. Pinned by a test that captures the value and asserts it is clean. _machine_training_resources keeps its exact old semantics (empty when non-viable), so its callers and its golden replay are untouched; the new viability-aware _machine_training_ceiling sits beside it. Two expectations CHANGED rather than added, and both encoded the bug -- the bats and Pester "below-floor machine falls back to the static default" tests each asserted the unschedulable literal as if it were the contract. Neither loses coverage: the unreadable-cluster tests still pin the case where the literal is genuinely right, now a distinct branch rather than the same one. The Pester golden replay is strengthened, not just repaired. It mapped EVERY non-viable vector onto the literal, which is what let this bug live inside a passing replay; it now checks them against the contract's own render_gi. Those numbers already matched -- wsl2-too-small-4gi renders 3c/1Gi, exactly what the installer now writes -- so the accessor and the installer agree on small machines too, and that agreement is asserted. Rebuilt on top of the current #768 rather than merged into it. #768 has since taken review fixes and a develop merge that restructured the same two sizing functions, so a replay produced seven conflict hunks across both installers -- the serialize-don't-parallelize hazard the ticket itself warns about. Re-applying this self-contained delta was safer than hand-resolving them; the test files applied from the original patch unchanged. Verified: bats 179 ok / 0 failed (develop's #759 fixed the three helm-dependent tests that used to fail); Pester 758 passed / 0 failed; manifest + envelope-embed checks clean. Refs: RFC-BACKEND-664 P0, client-runtime#358, client#766, client#768, backend#2167 Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(installer): one shared lookup in the bash twin too (Bugbot #768) I fixed this on the PowerShell side last push and left the bash side split -- which is precisely the two-installers-diverge class client#766 exists to remove. Bugbot caught the half I missed, and its failure mode is the MIRROR of the ps1 one: ps1 a failed provenance read reported `installer` for a carried size, inviting a future ladder to overrule a human choice bash a failed provenance read reported `unknown`, which consumers treat as a human pin -- so an installer-sized edge was PERMANENTLY STRANDED as a deliberate choice The second is worse, and it is the exact outcome scope bullet 4 exists to prevent: stranding is the defect, and a fail-unsafe read was manufacturing it. _existing_training_values now does ONE `helm get values` and echoes "<size>|<provenance>", or nothing. Nothing is the important case: a failed or empty read carries NOTHING, so the caller machine-sizes and `installer` is then the correct verdict, keeping the edge eligible for a future ladder. Provenance is normalised in that one place -- anything unrecognised, including absent, is `unknown`, never a guess. _existing_training_resources and _existing_training_provenance are now thin readers over it, so they cannot disagree, and _resolve_training_size takes both fields from a single call. All 15 pre-existing size/provenance tests pass UNMODIFIED, and the sizing path is down to exactly one `helm get values` (the other in the file is the unrelated reconcile path). Five tests added on the fail-unsafe path specifically: a failed read and an empty read must each carry nothing; a failed read must leave the verdict at `installer` rather than a stranding `unknown`; one lookup returns both fields together; and a carried size with genuinely no marker still pairs with `unknown` -- because `unknown` is correct when the read SUCCEEDS and there is no marker. The bug was reporting it when the read failed. Verified: bats 180 ok / 0 failed (+5), bats-hygiene 18/18, shellcheck -S warning -x clean on the edited script, manifest + envelope-embed checks clean. Refs: backend#2220, client#766, client#769 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(manifest): regenerate for the shared-lookup change The digest for scripts/lib/install-client-helm.sh was stale in 1d0b72d: I regenerated the manifest in the working tree AFTER committing, then reverted that working-tree change instead of committing it. The rebase had resolved the manifest conflict with --ours, which is never right for a generated file. Caught by running gen-manifest.sh --check against the pushed tree rather than trusting the earlier "up to date" line, which had been printed after a regeneration that never made it into a commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): only call a machine too small once we've measured it (Bugbot #768) Third instance of the same divergence class in this PR, and this one I wrote into my own commit message and then contradicted in the code: "warning about machine size would be a fabrication" -- while the code fabricated exactly that. _TB_TRAINING_UNSCHEDULABLE was INFERRED by re-probing `kubectl get nodes -o name` whenever the ceiling helper returned nothing. But the helper returns nothing for three different reasons, and only one of them is "too small": no kubectl, an unreadable/failed node read, nothing parseable, OR a parsed node whose remainder is not a requestable shape. So any listable node -- one whose allocatable would not parse, or one not Ready yet -- tripped a hard "this machine is too small" warning plus the cpu=2,memory=8Gi literal, about a machine we had never successfully measured. The PowerShell twin only flagged it after a PARSED node ($seen), so the two installers disagreed on the same cluster. Fixed at the source rather than at the caller: _machine_training_ceiling now returns "|unschedulable" as its own verdict when it HAS parsed a node and the remainder is not requestable, and stays silent only when it genuinely could not measure. Silence now means one thing -- "I could not read the cluster" -- and the resolver keys off the verdict instead of guessing from a second probe. That also drops the redundant `kubectl get nodes` call. Three tests, the first of which is the bug: readable-but-unparseable nodes must NOT be called too small; the helper reports |unschedulable only after measuring; the helper stays silent when nothing parses. The existing "too small for even a 1c/1Gi run" test still passes -- that node parses, so it is a measurement. Verified: bats 187 ok / 0 failed (+3), bats-hygiene 18/18, shellcheck -S warning -x clean, manifest + envelope-embed checks clean. Refs: backend#2220, client#766 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
Fixes tracebloc/backend#2208
The bug
Three
install_client_helmflow tests inscripts/tests/install-client-helm.batspass on CI and fail on every Apple Silicon Mac:same client re-run is allowed (upgrade in place)same client in a different namespace -> upgrades in place, no duplicatedifferent-namespace reconcile works WITHOUT jq (Bugbot #284)#756 made
_assert_engine_runs_on_this_archrun on macOS as a fail-closed backstop, and it probes the real host:uname -m, plus a live Rosetta/Docker smoke on Darwin.setup()pinned neitherOSnorARCH, so these tests inherited the developer's CPU.Only these three, because they are the only flow tests whose
helm listmock reports an existing release — the one path that resolves the engine to5.7and therefore engages the gate. Every other flow test resolvesfresh→8.4and returns early at the engine check.CI cannot see this by construction. CI is Linux/amd64, so the gate returns early at the arch check and
Unit testsstays green. The one platform the gate exists for is the one CI never runs — so it is a red local suite that CI calls green, for every Mac developer.The fix
Pin
OS=Linux/ARCH=x86_64in that file'ssetup()— to what CI actually is — so a local run reproduces CI and no test's verdict depends on the machine it runs on.This keeps the gate armed.
TRACEBLOC_ALLOW_ARM64=1would also turn the suite green, and that is precisely why it is not used here: it switches off the behaviour #756 added. Every arch-sensitive test in the file already setsOS/ARCHfor itself (_arch_gate_ctx, and the engine tests at lines ~1306-1567), so they override this and keep driving the real rule.No new gate tests.#756 already covers both arms — emulation ok → proceeds, missing → refuses, helper absent → refuses fail-closed, 8.4 → skips, escape hatch → skips. More would duplicate, not strengthen.
A comment this exposed
The block above
_assert_engine_runs_on_this_archstill read:sitting directly above the
Darwin)arm #756 added. Anyone trusting the prose over the code concludes macOS is ungated. Rewritten to describe what the function does, and to say that it probes the real host so callers under test must declareOS/ARCH.Mutation proof (run on an arm64 host)
setup()pinDarwinarm (return 0unconditionally)The second row is the one that matters: it shows the gate is still armed after this change, so the pin fixed the tests without disarming the thing they were tripping over. A fix that quietly disabled the gate would satisfy row 1 alone and look identical.
Verification (arm64 macOS, no
TRACEBLOC_ALLOW_ARM64)bats scripts/tests/install-client-helm.bats→ 159/159, was 156/159bats scripts/tests/setup-macos-arch.bats scripts/tests/preflight.bats→ 157/157scripts/gen-manifest.sh --check,scripts/check-facts.sh --check,scripts/check-style.sh,scripts/tests/check-drift.sh→ cleanbash -n+shellcheck -S erroron the changed lib → cleanReproduced beforehand on a clean
git archive origin/developcheckout at526fc49and at tipc238e5f, so this was never branch-specific.🤖 Generated with Claude Code
Note
Low Risk
Test harness and documentation only; installer behavior unchanged aside from comment accuracy and checksum manifest.
Overview
Fixes local-only failures in three
install_client_helmflow tests (same-client upgrade / cross-namespace reconcile) on Apple Silicon: those mocks report an existing Helm release, which resolves MySQL to 5.7 and runs_assert_engine_runs_on_this_archagainst the real host (Rosetta/uname), not the test harness.install-client-helm.batssetup()now setsOS=LinuxandARCH=x86_64(CI’s platform) so default flow tests don’t inherit the developer CPU. Arch-sensitive cases still override via_arch_gate_ctxand engine tests; this is notTRACEBLOC_ALLOW_ARM64, so the gate stays armed.Comment above
_assert_engine_runs_on_this_archis updated to state that macOS is gated (client#756) and that callers under test must declareOS/ARCH.scripts/manifest.sha256is refreshed for the touched lib file.Reviewed by Cursor Bugbot for commit 9f2b9f6. Bugbot is set up for automated code reviews on this repo. Configure here.