feat(resources): stamp resources set as a human choice (backend#2220)

[shujaatTracebloc]

Stacked on #538 — base is feature/2220-cli-reads-contract. Merge #538 first, then retarget this to develop before merging.

What

Companion to client#768, and the half that makes the marker mean anything.

#768 taught the installer to write env.RESOURCE_PROVENANCE (installer / user / unknown). Without this side, the marker is actively harmful: an edge the installer marked installer, which 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's strictly worse than having no marker at all, so the two PRs are only correct together.

Write side

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, not cleared. There is no branch on which omitting it would be safe. And tracebloc resources set is by definition the human choice — there's no variant of running it that isn't.

Read side

Training.Provenance, normalised through NormalizeProvenance, which maps anything unrecognised — empty, junk, wrong case, a value this binary predates — 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 don't know, and guessing installer risks overruling an operator.

Surfacing

resources --verbose gains a set by line. Deliberately not 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 shouldn't grow the default output.

unknown renders with its explanation rather than the bare word — "predates provenance tracking, treated as an explicit choice" — because the bare word invites the wrong conclusion. It does not mean something is broken.

Notes

• The env map already flows into helm.UpgradeParams.Env, so no new plumbing — the key reaches the values file for free.
• The apply-boundary test starts from an installer-marked edge specifically. The unit test proves the map is right; this proves the map arrives. Worth separating because the failure is silent: a marker that never lands looks identical to one that did, and the consequence surfaces much later.
• zz-all-strings.golden regenerated — one line, "set by", reviewed rather than rubber-stamped.

Verification

• go build, go vet clean; go test ./... — all packages green
• gofmt, goimports clean; deadcode-check.sh exit 0
• 6 new tests in internal/resources (including a set-then-read round trip, the end-to-end invariant: what resources set writes, the read path reports as a human choice) + 1 apply-boundary test in internal/cli

Also in this branch

#538 needed a follow-up commit: the Lint job's deadcode gate flagged ContractVersion as an unreachable export — I'd added it "for doctor output" and never wired it up, which is exactly the shape that gate exists to catch. Removed; the in-package tests use the field directly. The pre-existing stale allowlist entry for doctor.go: Status.String is untouched — it's reported on clean develop too and is a warning, not a failure.

Refs: RFC-BACKEND-664 §P0, client#768, client-runtime#358, cli#538

🤖 Generated with Claude Code

---

Note

^{Cursor Bugbot is generating a summary for commit c947c29. Configure here.}

[saadqbal]

The design is right and the reasoning for unknown-never-a-guess is the part I'd have wanted argued: NormalizeProvenance mapping empty, junk, wrong case and future-unknown values all to unknown rather than returning "" and making every caller invent its own default is the correct shape.

Bugbot's High is real, and I confirmed it by reading — and the fix is already sitting three lines below the bug.

internal/cli/resources_set.go:240-244:

ceilingUnchanged:=sameCeiling(desired, current)
ifceilingUnchanged&&!phantomGPU {
p.Successf("Each training run already uses up to %s — nothing to change.", …)
returnnil// BuildEnvSpec is never reached
}
ifceilingUnchanged { // phantomGPU == true here// Don't treat it as a clean no-op — fall through to persist so BuildEnvSpec's// explicit-empty GPU override lands …
}

So BuildEnvSpec stamps RESOURCE_PROVENANCE=user unconditionally within itself, but the caller can exit before reaching it. An installer-sized edge where the operator runs resources set --max, or passes flags restating the current ceiling, exits "nothing to change" and keeps installer.

That's precisely the state BuildEnvSpec's own comment calls "the single most dangerous state the marker can be in — a human choice wearing a label that invites overwriting". And the phantom-GPU branch immediately below is the template: same condition, same reason, already written. Extending it to also fall through when the stored provenance isn't already user is a two-line change with the rationale pre-argued.

This matters for the pair's stated correctness condition. Your body says #768 and this "are only correct together", because the marker is actively harmful without the user side. With this hole open, they aren't correct together either — just narrower: the same-size set still leaves a deliberate choice labelled overwritable. Worth fixing before either lands rather than after both.

Two smaller notes:

• The stacked-PR banner is stale. It says base is feature/2220-cli-reads-contract and to retarget after #538 — but #538 merged at 10:57 and this PR's base already reads develop. Worth dropping the note so nobody retargets something already retargeted. (Also, for the record, the org standard is no stacked PRs on open ones — resolved here by #538 landing first, which is the right outcome.)
• BuildEnvSpec writing unconditionally is right, and the --reset-then-reuse-values reasoning is the same one NoGPUEnvValue rests on: an omitted key is silently re-inherited, not cleared. Good that it's stated rather than assumed.

Requesting changes on the sameCeiling path; the Bugbot thread blocks the merge regardless. Everything else here I'd own.

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 16aa63b. Configure here.}

[saadqbal]

My finding is fixed, and fixed the way I'd hoped — conditionally, not unconditionally:

staleProvenance:=current.Provenance!=resources.ProvenanceUserifceilingUnchanged&&!phantomGPU&&!staleProvenance { … returnnil }

So the clean no-op survives for an edge already marked user, and treating unknown as stale is the honest call with the cost stated ("one extra apply per edge, exactly once"). Good.

But Bugbot's new Medium is real, and I verified both halves across the two repos — it's worse than "one extra apply once".

The claim is that current.Provenance can never become user, so staleProvenance is stuck true. I checked the two things it depends on:

1. The CLI pins the chart version.internal/helm/upgrade.go:68 — "pin --version to the currently-installed chart version" — with the reasoning at resources_set.go:555 that omitting it would pull the latest chart and silently change the whole client. Correct behaviour, and load-bearing here.
2. Only chart 1.9.50 emits the key onto the pod.client#768 adds it at jobs-manager-deployment.yaml:477 (hasKey .Values.env "RESOURCE_PROVENANCE", else "unknown"), in the bump to 1.9.50.

Put those together and it doesn't converge. On any edge running ≤ 1.9.49:

• ParseTraining reads the pod env, which has no RESOURCE_PROVENANCE, so current.Provenance is unknown;
• staleProvenance is true, so the no-op never returns and every resources set runs a helm upgrade;
• that upgrade is pinned to the old chart, which still doesn't emit the key, so the next read is unknown again.

It's permanently non-idempotent on every existing edge, not once. And the tail Bugbot points at is the sharp end: off a TTY without --yes, that path now reaches the confirmation gate and exits 1 where it previously printed "nothing to change" and exited 0. Anything scripted around a restating resources set breaks.

The fix I'd reach for is to read the marker where it's written.resources set writes RESOURCE_PROVENANCE into Helm values; the pod env is a downstream projection gated on chart version. Reading it back from helm get values makes the round-trip self-consistent and independent of which chart is installed — and it's what the installer side already does (Get-CarriedTrainingValues reads values, not pod env).

Failing that, the two narrower options are to treat "installed chart predates the key" as its own state and no-op in it, or to gate staleProvenance on the installed chart version. Both work; both need the version comparison the first option avoids entirely.

Also: version-bump-gate / version-check is failing. Separate from the above and I haven't dug into it — worth a look since it's a required-looking gate rather than a flake.

Keeping changes-requested. My original ask is genuinely closed, so this is a new finding rather than a repeat; and per my note on client#768, that one is approved but shouldn't ship ahead of this.

[shujaatTracebloc]

@saadqbal ready for re-review.

Both points from the last pass are closed — one with a fix, one with a measurement.

version-bump-gate / version-check — fixed (5a1539f). Not a flake, a real stale-version gate: VERSION still said 0.10.9, v0.10.9 is already released, and this PR changes published files under internal/*. Bumped to 0.10.10, derived from the live release list rather than the log (highest released final is still 0.10.9, and no other open cli PR touches VERSION, so there's no collision). CI is now fully green.

The new Medium — I believe it's a false positive, and I checked it by rendering the chart rather than reading it. Full evidence is on the Bugbot thread; the short version:

Premise 2 ("only chart 1.9.50 emits the key onto the pod") doesn't hold. jobs-manager-deployment.yaml has the named hasKey blocks and a generic {{- range $key, $value := .Values.env }} passthrough whose filter is a denylist:

{{- if and (ne $key "CLIENT_ENV") (ne $key "RESOURCE_REQUESTS") (ne $key "RESOURCE_LIMITS") (ne $key "GPU_REQUESTS") ... $value }}

On charts ≤ 1.9.49RESOURCE_PROVENANCE is not in that list, so the generic loop emits it. helm template with env.RESOURCE_PROVENANCE=user:

1.9.49 (develop) container[0] api: RESOURCE_PROVENANCE x1 = ['user']
1.9.44 (released) container[0] api: RESOURCE_PROVENANCE x1 = ['user']
1.9.50 (#768) container[0] api: RESOURCE_PROVENANCE x1 = ['user']

container[0] is exactly what the read path consumes (JobsManagerEnv takes Containers[0]). I checked every released tag carrying the unified client/ chart — all have the generic passthrough, so there is no installed version that drops the key. values.schema.json also sets env.additionalProperties: {"type":"string"}, so an undeclared string key is accepted.

The neat inversion: #768 addsRESOURCE_PROVENANCE to the denylist (lines 544/636) to avoid double-emitting alongside its new named block — and that addition is precisely why 1.9.50 is the only version where the named block is the emission path. Rendered 1.9.50: exactly one entry, no duplicate.

So it converges on every chart: read is user on the second run, the clean no-op returns, and the off-TTY --yes-less path keeps exiting 0. Cost stays the documented one — a pre-marker edge pays one extra apply, once.

Your premise 1 (the CLI pins --version) is correct and I'm not disputing it; it just isn't load-bearing once the key travels by the generic path. I'd rather not switch the read to helm get values on a premise that measurement contradicts — happy to if you still see a hole, but it'd be trading a working path for a second source of truth.

Not changed: the stale stacked-PR banner. You're right that it should go (base already reads develop), but editing the PR description was blocked by a permission gate in my session — flagging it rather than claiming it done.

[saadqbal]

@shujaatTracebloc is right and I was wrong. Approving.

I verified the rebuttal against develop's chart rather than accepting it, and it holds. Chart 1.9.49 carries the generic passthrough at jobs-manager-deployment.yaml:514 and :611, and its denylist at :532/:612 is:

(ne $key "CLIENT_ENV") (ne $key "RESOURCE_REQUESTS") (ne $key "RESOURCE_LIMITS")
(ne $key "GPU_REQUESTS") (ne $key "GPU_LIMITS") (ne $key "RUNTIME_CLASS_NAME")
(ne $key "SINGLE_NODE") (ne $key "CLIENT_ID") (ne $key "IMAGE_PULL_SECRET_NAME")

No RESOURCE_PROVENANCE. So on the old chart the key rides the generic passthrough onto the pod, ParseTraining reads it, staleProvenance goes false after one apply, and the no-op returns. It converges.

Where I went wrong is worth naming precisely, because it wasn't a guess — it was a verification that stopped one step short. I checked #768's template, saw RESOURCE_PROVENANCE in its denylist, saw the named emit only in 1.9.50, and concluded older charts can't deliver the key. But #768 adds it to the denylist because it adds the named emit — otherwise the key would be emitted twice. The old chart doesn't exclude it, which is exactly why it gets through. Two charts, each self-consistent, both delivering the key by different routes. I confirmed the half that supported the finding and never tested the half that would have refuted it.

Rendering the chart rather than reading it is the right instinct and it's what settled this.

On the PR itself — my original sameCeiling finding is fixed the way I hoped, conditionally rather than unconditionally:

staleProvenance:=current.Provenance!=resources.ProvenanceUserifceilingUnchanged&&!phantomGPU&&!staleProvenance { … returnnil }

so the clean no-op survives for an edge already marked user, and treating unknown as stale is the honest call with the one-extra-apply cost stated rather than hidden. 5a1539f6 clears the version gate. 24 checks passing, nothing pending, no open threads.

The design I said I'd own still stands: NormalizeProvenance mapping empty, junk, wrong case and future-unknown all to unknown rather than returning "" and making each caller invent a default, and BuildEnvSpec writing the marker unconditionally for the --reset-then-reuse-values reason NoGPUEnvValue already rests on.

Merge ordering still applies, and it's now the only thing I'd flag: this and client#768 are only correct together, and I've approved both. Ship them together rather than either alone.

[LukasWodka]

/fr-pass