Uh oh!
There was an error while loading. Please reload this page.
refactor(resources): read the envelope constants from the contract, keep Decision A (backend#2220) - #538
Conversation
…eep Decision A (backend#2220) Third and last reader in the P0 consolidation. client-runtime#358 made the training-envelope arithmetic single-definition and client#766 made both installers readers of it; this does the same for set.go. Deleted: the four literals (1-CPU/3-GiB overhead, 1-core/2-GiB floors). They now come from internal/resources/envelope_contract.json, vendored from client-runtime, whose arithmetic is node_sizing.envelope_from_allocatable. cli#143 Decision A is AMENDED IN SCOPE, NOT DEVIATED FROM. The number the user sets is still the per-run ceiling written to RESOURCE_* verbatim; DeriveTraining is still the identity on it; Overhead() is still a fit-check safety margin that is never subtracted. Every exported signature is unchanged, and the existing suite passes untouched. What went away is the duplicate *definition* of four numbers, which was never part of that decision. Amendment recorded on cli#143. Worth naming what the duplication actually cost, because it was not only tidiness: LargestReadyNode/nodeLarger ranks candidates (cpu, memory) while the bash installer ranked them (memory, cpu). On a cluster of 8c/16Gi + 4c/32Gi `resources set` and the installer anchored on DIFFERENT nodes and gave different answers about one machine. Nobody chose that; it fell out of two independent implementations. The contract records one order, and this repo's was the one kept -- it is what the user is shown when the wizard clamps their prompt. Go needs no generator, unlike the installers: go:embed takes the contract verbatim at compile time, so the vendored artifact is byte-identical to upstream and the cross-repo gate is a plain diff. The constants became small funcs rather than package vars for the same reason Overhead() is a func -- a var would be mutable from anywhere, and the point is that one place decides. mustContract panics on a malformed contract, MustCompile-style: the file is embedded at COMPILE time, so it can only be invalid via a hand-edit or a botched re-vendor -- a broken build, not a runtime condition. A default here would be a fifth policy. TestContractIsValid keeps the panic out of a release. The drift gate FAILS CLOSED when it cannot read upstream, per cli#536 -- a check that never executed must not report as passing. client-runtime is private, so it mints a least-privilege App token (named repositories, contents:read) instead of GITHUB_TOKEN. It also re-derives upstream's vectors from upstream's own generator: a vendored contract can match upstream byte-for-byte while UPSTREAM's goldens have gone stale against upstream's code, and mirroring that faithfully is still wrong. Verified: go build + go vet clean, go test ./... all packages ok, gofmt and goimports clean, go.mod/go.sum untouched. Mutation-tested: overhead 3GiB -> 4GiB in the vendored contract reddens 5 tests here, two of them PRE-EXISTING (TestOverhead_IsOneCoreThreeGiB, TestMaxRunCoresAndGiB) -- so the existing suite already anchored these numbers independently, which is the ticket's DoD holding in a consumer repo. Refs: RFC-BACKEND-664 P0, client-runtime#358, client#766, cli#143, cli#536 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… gate) The Lint job's deadcode check flagged it: exported, but unreachable from ./cmd/tracebloc. I had added it "for doctor output" and then never wired it into a command, which is exactly the shape that gate exists to catch — an export with no caller reads as API surface and gets maintained like it. The tests are in-package, so they use mustContract().ContractVersion directly and lose nothing. Surfacing the contract version in `doctor` may well be worth doing (a binary and an edge disagreeing about the contract is a real field question), but it belongs in the PR that touches the read path, not as a stub here. Not touched: the pre-existing stale allowlist entry for internal/doctor/doctor.go Status.String. It is reported on clean develop too and is a warning, not a failure — deadcode-check.sh still exits 0. Pruning it is someone's cleanup, not this PR's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
saadqbal
left a comment
There was a problem hiding this comment.
Reviewed as the third reader in the chain, and the cross-repo claim checks out end to end. I have all three repos on disk, so I verified byte-identity rather than trusting it:
4b37dde48131e34470f12cf981ce7abd5ca745d6d344183edb69f89a1fd6b7cb client-runtime@6293d15:envelope_contract.json
4b37dde48131e34470f12cf981ce7abd5ca745d6d344183edb69f89a1fd6b7cb client#766:scripts/tests/fixtures/envelope_contract.json
4b37dde48131e34470f12cf981ce7abd5ca745d6d344183edb69f89a1fd6b7cb cli#538:internal/resources/envelope_contract.json
Same digest all three, and both readers pin the identical upstream SHA. That's the property the whole consolidation rests on, and it holds.
go test ./internal/resources/... green, and mutation-tested both halves of the claim:
- Tamper the vendored overhead (
cpu_millicores1000 → 1500) →TestContractIsValid,TestGoldenVectorsReplay, plus the pre-existingTestOverhead_IsOneCoreThreeGiB,TestFitsNode_IncludesOverheadAndGPUandTestMaxRunCoresAndGiBall fail. So the vectors are derived and the old suite is genuinely still load-bearing. - Make
DeriveTrainingsubtract overhead (Decision A violated) →TestDecisionAIsIntactand the pre-existingTestDeriveTraining_IdentityUnderDecisionAboth fail.
On "every exported signature is unchanged" — accurate, and I checked the surface rather than the diff summary. The only delta in set.go's exported funcs is minRunCPUMilli → minRunCPUMilli() inside two one-line bodies; the signatures are identical. Constants becoming funcs rather than package vars is the right call for the reason given: a var is mutable from anywhere in the process, and the point of this ticket is that one place decides.
mustContract panicking is correct here and the reasoning is the part I'd have wanted written down — the file is embedded at compile time, so invalidity means a hand-edit or a botched re-vendor, i.e. a broken build rather than a user-reachable condition. A silent default really would be a fifth policy. TestContractIsValid plus TestContractJSONIsCanonicalFormatting between them keep both the panic and a formatting-only re-vendor out of a release, which is more thorough than I expected.
Keeping this repo's (cpu, memory) order as the contract's is the right way round, for the reason stated — it's the number the user is shown when the wizard clamps their prompt, so the installers should move to it rather than the reverse.
Holding the approval only on CI:Lint, Test and Cursor Bugbot are all still in progress as I write (the scan had caught an earlier Lint failure; the current run is mid-flight). Nothing here needs another round from me — I'll approve next pass once they land green.
saadqbal
left a comment
There was a problem hiding this comment.
Approving as promised. Head is still 05517ed4 — the commit I reviewed — and CI is clean (24 passing, 3 skipped, nothing failing or pending), no open threads. Re-ran go test ./internal/resources/... on the current head: green.
Checked that drop the unreachable ContractVersion export didn't take the validation with it — contract.go:85 still panics on ContractVersion < 1, and contract_test.go:30 still asserts it, so what went was the redundant accessor rather than the guard.
Everything from my earlier pass stands: all three repos carry a byte-identical contract (4b37dde4…) at the same pinned client-runtime SHA, tampering the vendored overhead reddens five tests including the pre-existing ones, and making DeriveTraining subtract overhead reddens both Decision-A tests.
Uh oh!
There was an error while loading. Please reload this page.
… review, #539) Bugbot's High, confirmed by @saadqbal in review, and the fix was indeed sitting three lines below the bug. BuildEnvSpec stamps RESOURCE_PROVENANCE=user unconditionally WITHIN ITSELF, but the caller could exit before ever reaching it: `sameCeiling` returned "nothing to change" whenever the desired ceiling matched the current one. So an installer-sized edge whose operator ran `resources set --max`, or passed flags restating the current ceiling, kept `installer` -- a deliberate human choice wearing the one label that invites a future ladder to overwrite it. That is verbatim the state BuildEnvSpec's own comment calls the most dangerous the marker can be in, and my PR body claimed this PR and client#768 "are only correct together" while this hole made that untrue: not wrong, just narrower. Fixed with the phantom-GPU branch as the template -- same condition, same reason, already written and already reviewed. An unchanged ceiling now falls through to persist when the stored marker is not already `user`. Both conditions can hold at once, so both report their reason rather than one masking the other. `unknown` counts as stale deliberately. A pre-marker edge whose operator restates the ceiling has just made that size explicit, so recording it is the honest answer; the cost is one extra apply per edge, exactly once, because the second run sees `user` and is a clean no-op again. Three existing no-op tests now carry RESOURCE_PROVENANCE=user in their fixtures (TestSet_NoOpSkipsApply, TestSet_NoOpEvenWhenCurrentNoLongerFits, TestWizard_LeaveAsIs). Their invariant is unchanged and still asserted -- a clean no-op makes no helm call -- it just needs the marker present to BE the clean case. The uncovered case became TestSet_SameCeilingStampsProvenance, a table over installer / pre-marker / junk, asserting the apply happens, that we do not claim nothing changed while correcting the marker, and that the reason is stated. zz-all-strings.golden regenerated: one line, reviewed. Also rebased onto develop now that cli#538 has merged -- `git rebase --onto origin/develop 05517ed` to drop the squashed parent's commits rather than replay them into conflicts. GitHub had already retargeted the base to develop when #538 landed, so the stacked-PR banner in the description was stale; removed. Verified: go build + go vet clean, go test ./... all packages green, gofmt and goimports clean, deadcode exit 0. Refs: backend#2220, client#768 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…0) (#539) * feat(resources): stamp `resources set` as a human choice (backend#2220) Companion to client#768, and the half that makes the marker mean anything. client#768 taught the installer to write env.RESOURCE_PROVENANCE (installer | user | unknown). Without this side, an edge the installer marked `installer` and the operator then re-sized by hand would KEEP saying `installer` -- a deliberate human choice wearing the one label that invites a future ladder to overwrite it. That is strictly worse than no marker at all, so the two PRs are only correct together. BuildEnvSpec now writes RESOURCE_PROVENANCE=user unconditionally. Unconditional for the same reason NoGPUEnvValue is: the apply runs `helm upgrade --reset-then-reuse-values`, which re-applies the release's stored values on top of chart defaults, so an omitted key is silently RE-INHERITED rather than cleared. There is no branch on which omitting it would be safe. `tracebloc resources set` is by definition the human choice -- there is no variant of running it that isn't. Read side: Training.Provenance, normalised through NormalizeProvenance, which maps anything unrecognised -- empty, junk, a value this binary predates, wrong case -- to `unknown`, never to a guess. Returning "" would have made callers branch on empty and invent their own default, which is a second policy and exactly what this ticket removes. Callers MUST treat `unknown` as a human choice: it means we do not know, and guessing `installer` risks overruling an operator. Surfaced in `resources --verbose` as "set by", not in the default view: it answers a support question ("did someone set this, or did we?"), not one an operator needs on every run, and a line of bookkeeping that never changes the numbers should not grow the default output. `unknown` renders with its explanation rather than the bare word, which invites the wrong conclusion -- it does not mean something is broken. The env map already flows into helm.UpgradeParams.Env, so no new plumbing. The added apply-boundary test starts from an installer-marked edge specifically, because the unit test proves the map is right while this proves the map arrives -- and a marker that never lands looks identical to one that did, with the consequence surfacing much later. zz-all-strings.golden regenerated: one line, "set by", reviewed. Verified: go build + go vet clean, go test ./... all packages green, gofmt and goimports clean, deadcode exit 0. 6 new tests in internal/resources (including a set-then-read round trip) + 1 apply-boundary test in internal/cli. Refs: RFC-BACKEND-664 P0, client#768, client-runtime#358, cli#538 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(resources set): a same-size set must still stamp `user` (Bugbot + review, #539) Bugbot's High, confirmed by @saadqbal in review, and the fix was indeed sitting three lines below the bug. BuildEnvSpec stamps RESOURCE_PROVENANCE=user unconditionally WITHIN ITSELF, but the caller could exit before ever reaching it: `sameCeiling` returned "nothing to change" whenever the desired ceiling matched the current one. So an installer-sized edge whose operator ran `resources set --max`, or passed flags restating the current ceiling, kept `installer` -- a deliberate human choice wearing the one label that invites a future ladder to overwrite it. That is verbatim the state BuildEnvSpec's own comment calls the most dangerous the marker can be in, and my PR body claimed this PR and client#768 "are only correct together" while this hole made that untrue: not wrong, just narrower. Fixed with the phantom-GPU branch as the template -- same condition, same reason, already written and already reviewed. An unchanged ceiling now falls through to persist when the stored marker is not already `user`. Both conditions can hold at once, so both report their reason rather than one masking the other. `unknown` counts as stale deliberately. A pre-marker edge whose operator restates the ceiling has just made that size explicit, so recording it is the honest answer; the cost is one extra apply per edge, exactly once, because the second run sees `user` and is a clean no-op again. Three existing no-op tests now carry RESOURCE_PROVENANCE=user in their fixtures (TestSet_NoOpSkipsApply, TestSet_NoOpEvenWhenCurrentNoLongerFits, TestWizard_LeaveAsIs). Their invariant is unchanged and still asserted -- a clean no-op makes no helm call -- it just needs the marker present to BE the clean case. The uncovered case became TestSet_SameCeilingStampsProvenance, a table over installer / pre-marker / junk, asserting the apply happens, that we do not claim nothing changed while correcting the marker, and that the reason is stated. zz-all-strings.golden regenerated: one line, reviewed. Also rebased onto develop now that cli#538 has merged -- `git rebase --onto origin/develop 05517ed` to drop the squashed parent's commits rather than replay them into conflicts. GitHub had already retargeted the base to develop when #538 landed, so the stacked-PR banner in the description was stale; removed. Verified: go build + go vet clean, go test ./... all packages green, gofmt and goimports clean, deadcode exit 0. Refs: backend#2220, client#768 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(release): bump VERSION to 0.10.10 v0.10.9 is already released and this PR changes published files under internal/*, so the release train needs a version above every released tag — version-bump-gate fails otherwise (backend#1561). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: shujaat hasan <shujaathasan@shujaats-MacBook-Pro.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
commented
Aug 22, 2026
/fr-pass |
What
Third and last reader in the
backend#2220/ RFC-BACKEND-664 §P0 consolidation. client-runtime#358 made the arithmetic single-definition; client#766 made both installers readers; this does the same forset.go.Deleted: the four literals — the 1-CPU / 3-GiB overhead and the 1-core / 2-GiB floors. They now come from
internal/resources/envelope_contract.json, vendored from client-runtime, whose arithmetic isnode_sizing.envelope_from_allocatable.cli#143 is amended in scope, not deviated from
Decision A stands, word for word:
RESOURCE_*verbatimDeriveTrainingis still the identity on itOverhead()is still a fit-check safety margin that is never subtractedEvery exported signature is unchanged and the existing suite passes untouched. What went away is the duplicate definition of four numbers, which was never part of that decision. The amendment is recorded on cli#143.
What the duplication actually cost
Not just tidiness.
LargestReadyNode/nodeLargerranks candidates(cpu, memory); the bash installer ranked them(memory, cpu). On a cluster of 8c/16Gi + 4c/32Gi those pick different nodes, sotracebloc resources setand the installer gave different answers about the same machine. Nobody chose that — it fell out of two independent implementations. The contract records one order, and this repo's is the one kept: it's what the user is shown when the wizard clamps their prompt.Why this repo needs no generator
Unlike bash and PowerShell, Go can read JSON.
go:embedtakes the contract verbatim at compile time, so the vendored artifact is byte-identical to upstream and the cross-repo gate is a plaindiff. No embed step, no regeneration, nothing to forget.Two small design notes:
Overhead()is a func: a var would be mutable from anywhere in the process, and the whole point is that one place decides these numbers.mustContractpanics on a malformed contract,MustCompile-style. The file is embedded at compile time, so it can only be invalid via a hand-edit or a botched re-vendor — a broken build, not a runtime condition a user could hit. A silent default here would be a fifth policy, which is exactly what this ticket removes.TestContractIsValidkeeps the panic out of a release.The drift gate fails closed
Per cli#536 (landed today): a check that never executed must not report as passing. client-runtime is private, so
GITHUB_TOKENcan't read it — the job mints a least-privilege App token (repositories: client-runtime,contents: read, per the backend#2157 sweep) and fails rather than warning-and-skipping if that doesn't work.It also re-derives upstream's vectors from upstream's own generator. A vendored contract can match upstream byte-for-byte while upstream's goldens have gone stale against upstream's code — mirroring that faithfully is still mirroring a lie.
Pinned via
scripts/.client-runtime-ref, the same pin-don't-float convention this repo already uses for.client-refand.data-ingestors-ref.Verification
go build,go vetclean;go test ./...— all packages okgofmtandgoimportsclean;go.mod/go.sumuntouched3GiB → 4GiBin the vendored contract reddens 5 tests here — two of them pre-existing (TestOverhead_IsOneCoreThreeGiB,TestMaxRunCoresAndGiB). The existing suite already anchored these numbers independently, so the DoD holds in a consumer repo without me having written the test that proves it.Where #2220 stands after this
RESOURCE_*unconditionallyset.gokeeps its contract, drops its arithmeticresources setOn the third bullet. If the installer stops writing
RESOURCE_*, every edge falls toboth-unset→ the derive path → which is flag-off (DERIVE_JOB_ENVELOPE, backend#2167) → back to a flatcpu=2,memory=8Gi. That would undo backend#1236 fleet-wide and reinstate the unschedulable-8Gi bug on sub-8GiB machines. Turning derivation on needs #2167's concurrency answer, which is explicitly P4/L0. Recommend re-framing that bullet as half-P0 / half-P4 on the ticket rather than leaving it silently unticked.Refs: RFC-BACKEND-664 §P0, client-runtime#358, client#766, cli#143, cli#536, backend#2167
🤖 Generated with Claude Code
Note
Cursor Bugbot is generating a summary for commit aa83178. Configure here.