Skip to content

feat(envelope): record WHO chose the training envelope (backend#2220) - #768

Merged
shujaatTracebloc merged 8 commits into
developfrom
feature/2220-envelope-provenance
Aug 20, 2026
Merged

feat(envelope): record WHO chose the training envelope (backend#2220)#768
shujaatTracebloc merged 8 commits into
developfrom
feature/2220-envelope-provenance

Conversation

@shujaatTracebloc

@shujaatTraceblocshujaatTracebloc commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Follows #766 (merged as 3c40ef9). Rebased onto develop, so this diff is provenance only.

What

Scope bullet 4 of backend#2220"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 flagged as "the actual hard part of this ticket, not the consolidation." This adds the marker that makes it knowable going forward, and is honest about the edges where it never will be.

Why it can't be inferred

RESOURCE_* has no unset state once Helm has seen it — the fleet auto-upgrade CronJob, the installer reconcile path, and the CLI's own apply all use --reset-then-reuse-values, which re-applies stored user-supplied values indefinitely.

_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 already-pinned edge, or silently overrule operators who chose a size on purpose.

env.RESOURCE_PROVENANCE

ValueMeaning
installersized to this machine at install time
userexplicit resources set, or a TRACEBLOC_TRAINING_RESOURCES override
unknowncarried forward from before this key existed

unknown is a deliberate answer, not a placeholder. Guessing installer there would risk overruling a human, so consumers must treat unknown as user. Existing edges will report unknown and keep their current size until someone opts in explicitly — offering that opt-in is a product decision, not one P0 should make silently.

An existing marker is authoritative and survives 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 very ambiguity it exists to remove. Pinned in both bats and Pester.

The un-pin recipe, verified rather than assumed

Trap 2 in the issue's handover: "removing the key from the chart does not remove it from existing releases… the migration needs an explicit helm-side unset."

Helm deletes null-valued keys during value coalescing, so this drops all three vars and makes the derive path reachable — confirmed by rendering the chart both ways (6RESOURCE_* occurrences when set, 0 with null):

helm upgrade "$NAMESPACE" tracebloc/tracebloc -n "$NAMESPACE" \
--reset-then-reuse-values \
--set env.RESOURCE_LIMITS=null \
--set env.RESOURCE_REQUESTS=null \
--set env.RESOURCE_PROVENANCE=null

No chart change needed. MIGRATION.md carries it with a warning that matters: derivation is gated off (DERIVE_JOB_ENVELOPE, backend#2167), so clearing the keys today returns the edge to the fixed cpu=2,memory=8Gi literal — which on a sub-8GiB machine cannot schedule at all.

Implementation notes

  • _training_resources is now a thin 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.
  • Excluded from both generic env passthrough loops — otherwise it renders twice per container.

Verification

  • bats 172 ok, 8 new provenance tests. The 3 failures (ci: add advance-deploy-env caller workflow #64–66) are pre-existing on clean develop — re-confirmed by stashing; they need real helm
  • Pester 750 passed / 0 failed, 7 new
  • helm unittest 480 passed vs 477 on develop — 3 added, and no new failures among the 5 pre-existing ones (unrelated suites, schema-spec errors present on clean develop)
  • helm lint clean; PSScriptAnalyzer 0 errors; gen-envelope-embed.sh --check clean
  • Chart 1.9.48 → 1.9.49, version + appVersion in lockstep; manifest.sha256 regenerated (both installers moved)

What this does not do

It doesn't un-strand anything by itself, and it can't. Silent un-strand­ing is unsafe for exactly the reason above — for unknown edges we do not know whose choice we'd be overriding. What this buys is that from 1.9.49 the answer is recorded, so the ladder work behind #2167 can re-derive installer values and leave deliberate choices alone.

tracebloc resources set must also stamp user, or a user overriding an installer-marked edge would leave a stale installer marker on a deliberate human choice — the precise failure this marker exists to prevent. That's the companion cli PR.

Refs: RFC-BACKEND-664 §P0, client-runtime#358, client#766, cli#538, backend#2167

🤖 Generated with Claude Code


Note

Cursor Bugbot is generating a summary for commit 8b67114. Configure here.

Comment threadscripts/install-k8s.ps1 Outdated

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The premise is exactly right, and it's the part of backend#2220 that actually needed thinking about rather than typing. RESOURCE_* having no unset state once Helm has seen it — because the auto-upgrade CronJob, the installer reconcile path and the CLI apply all use --reset-then-reuse-values — means an installer-written envelope and a deliberate resources set are genuinely indistinguishable, and cpu=2,memory=8Gi being the only discriminator is a measurement, not a guess. Adding the marker going forward and being honest that pre-marker edges are unknown forever is the right call over inventing a heuristic.

Bugbot's Medium is real, and it's a fresh instance of the exact class #766 existed to remove. Read both sides:

install-k8s.ps1:4069-4092Get-TrainingProvenance performs its ownhelm get values, separate from the one Get-TrainingResources does immediately below, and the enclosing try ends in a bare catch {} falling through to:

 } catch {}
# 2. Sized to this machine, or 3. the static default -- both are OUR choice.return"installer"

The bash twin resolves both in one pass — _training_provenance() calls _resolve_training_size and reads $_TB_TRAINING_PROVENANCE, so size and provenance cannot disagree — and answers unknown when a carried size has no readable marker.

So on Windows, if the size path succeeds and carries a live RESOURCE_LIMITS but the provenance lookup then throws — a wedged API, a ConvertFrom-Json hiccup, anything the catch {} eats — the generated values pin that carried envelope as installer. A future ladder trusting that label re-derives and overrules what may well have been a human choice. unknown is the safe verdict and it's what the other installer already gives.

Two things make this worth fixing rather than noting:

  • It's the two installers diverging by construction, which is the whole reason #766 exists. Same file pair, three days later, in the fail-unsafe direction.
  • The bare catch {} is the standing hazard #766's own description flagged as "worth its own ticket" — "any exception on that path silently hands the customer the unschedulable literal with no diagnostic". It's now silently handing them a wrong provenance verdict too. Whether or not you widen that ticket here, this call site shouldn't fall through to the unsafe answer.

The shape that fixes both at once is the bash one: resolve size and provenance together in a single lookup, and let a failed read produce unknown rather than installer. That also drops a redundant helm get values per install.

Requesting changes on that; the Bugbot thread blocks the merge anyway.

Merge ordering, for whoever lands these: this and cli#539 are only correct together, per #539's description — the marker without the user side is worse than no marker. Note that #539 currently has its own open High (sameCeiling returns before BuildEnvSpec, so a same-size resources set keeps installer), so both need fixing before either is safe to land, not just sequencing.

shujaatTraceblocand others added 3 commits August 20, 2026 13:43
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>
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>
…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
shujaatTraceblocforce-pushed the feature/2220-envelope-provenance branch from 8b67114 to 516e30cCompareAugust 20, 2026 11:49
saadqbal
saadqbal previously approved these changes Aug 20, 2026

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Green (43 passing, 4 skipped, nothing failing or pending), no open threads, and the conflict from earlier is resolved.

My finding is fixed, and fixed at the right level.Get-CarriedTrainingValues is now one shared lookup returning @{ Size; Provenance } or $null, and I checked the consequence rather than the diff: Get-TrainingProvenance still answers installer when it gets $null, and that is now correct — because Get-TrainingResources receives the same $null from the same read and falls through to machine sizing. So installer is a true statement about a machine-sized envelope rather than a label pinned onto a carried one. The two answers can no longer disagree because there is only one answer.

The call site is what makes that hold, so I checked it too (:5023-5027):

$carried=Get-CarriedTrainingValues$trainingSize=Get-TrainingResources-Carried $carried-CarriedResolved
$trainingProvenance=Get-TrainingProvenance-Carried $carried-CarriedResolved

Resolved once, passed to both. And install-k8s.Tests.ps1:1690 pins exactly that property — "size and provenance come from ONE lookup and cannot disagree" — which is the invariant rather than a restatement of the code. That also drops a helm get values per install, and brings the ps1 into line with the bash twin's single _resolve_training_size pass, which is what I'd asked for.

Good catch on b204256 too — bumping to 1.9.50 because #767 took 1.9.49. develop is at 1.9.49 and this is at 1.9.50, so that's right. Worth noting that's the same collision I flagged on #764; git won't surface it when both sides say the same version identically, so it's easy to lose.

Merge ordering — please ship this with cli#539, not ahead of it. Both descriptions say the pair is only correct together, and that's accurate but worth stating precisely so the risk isn't over- or under-read:

  • Nothing consumes the marker yet — the comment at :5031 says it's bookkeeping and never changes the envelope — so landing this alone breaks nothing today.
  • But any edge the installer stamps installer and an operator then re-sizes by hand will keep saying installer until #539 is in, and that wrong value is silent and sticks. It only becomes damaging when a consumer trusts it, which is exactly what the future ladder will do.

And note cli#539 currently has its own open High — sameCeiling returns before BuildEnvSpec, so a same-size resources set doesn't stamp user either. So the pair isn't yet correct together; that one needs fixing before either lands, not just sequencing.

The premise remains the best part of this: RESOURCE_* having no unset state once Helm has seen it, with cpu=2,memory=8Gi as the only discriminator, is a measurement rather than a guess — and being honest that pre-marker edges are unknown forever beats inventing a heuristic that would be wrong quietly.

…ope-provenance
# Conflicts:
#	scripts/manifest.sha256
Comment threadscripts/lib/install-client-helm.sh
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>
…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>
Comment threadscripts/lib/install-client-helm.sh Outdated
shujaatTraceblocand others added 2 commits August 20, 2026 14:27
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>
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>
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

1 issue from previous review remains unresolved.

Fix All in Cursor

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit b34c13c. Configure here.

…(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>
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 25c3ca9. Configure here.

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving. Green (43 passing, 4 skipped, nothing failing or pending), no open threads.

Scope note first, since it changed:#769 has merged into this branch, so this is no longer provenance-only — it now carries the undersized-envelope work too, which I reviewed on #769 and found sound. Chart version checked because this is the fourth time I've seen that trap today: develop is at 1.9.49 and this is at 1.9.50. Correct, no collision.

The |unschedulable fix is my #769 note, and it turned out sharper than I framed it. I'd flagged the re-probe as a minor "second lookup that can disagree with the first". The real defect was worse: _machine_training_ceiling returns nothing for four different reasons and only one is "too small", so any listable node — one whose allocatable didn't parse, or one not yet Ready — tripped a hard "this machine is too small". Your own commit message names the contradiction better than I did: the comment said "warning about machine size would be a fabrication" while the code fabricated exactly that.

The resolution is the right one — a fourth distinct return rather than a smarter inference:

|viable -> sized to the machine
|undersized -> the honest remainder, warn
|unschedulable -> literal, warn (MEASURED, genuinely too small)
(fall-through) -> literal, SILENT (never measured, so any claim would be invented)

Two states that were one are now two, and the silent arm's comment says why. That's the same shape as the provenance fix earlier in this PR, which is presumably why you called it the third instance of the class.

And the bash twin now matches the ps1 — one _resolve_training_size pass returning size and marker together. The cases pin it directly: size and marker come from ONE pass and always agree, plus a carried size with no marker pairs with 'unknown' from the SAME read. 27 cases green across training-size and provenance, and the matrix is complete — only after measuring, stays SILENT when nothing parses, a failed values read carries NOTHING (never a stranding 'unknown'), a junk marker degrades to 'unknown', never to a guess, and an existing 'user' marker SURVIVES re-install.

That last group is the part worth keeping: every arm answers "what do we actually know" rather than "what's the most likely value", which is the whole premise of the marker.

Merge ordering still applies. This and cli#539 are only correct together — I've approved both. Ship them together rather than either alone.

@shujaatTracebloc
shujaatTracebloc merged commit f14a7be into developAug 20, 2026
47 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@shujaatTracebloc@saadqbal