fix(resources): a same-ceiling resources set must not require --yes (cli#546)

[saadqbal]

Fixes#546. Bugbot MEDIUM on the develop→staging promotion mirror #540, against #539 (backend#2220). Fixed on develop, per the promotion-findings rule — nothing was pushed to #540.

The regression

#539 made any non-userRESOURCE_PROVENANCE stale, so an unchanged ceiling stopped returning early and fell through to the apply in order to re-stamp the marker as user. Right intent, wrong landing site: the fall-through lands in the confirmation gate, and off a terminal that gate does not ask — it returns exit 1.

$ tracebloc resources set --cores 4 --memory 16 # restating the CURRENT ceiling
refusing to change the ceiling without confirmation: pass --yes, or run on a terminal
$ echo $?
1

That is a breaking change for scripted callers. The command's own --help documents 0 applied (or nothing to change), and docs/cli-navigation.md draws the no change edge going straight to exit 0, bypassing CONF entirely. Both became false. Nearly every installed edge still reads installer or unknown, so the blast radius is the installed base, not an edge case — and the callers that restate a size are scripts (the bootstrap, the end-to-end journey), none of which pass --yes for what the docs promise is a no-op.

The fix

One clause on the gate, not a revert:

if!req.yes&&!req.dryRun&&!ceilingUnchanged {

The gate guards the ceiling. An unchanged ceiling has nothing to ask about — "Let each training run use up to 4 CPU · 16 GiB?" when the answer is already 4 CPU · 16 GiB is a question with one honest answer. #539's re-stamp still happens; the marker still flips to user.

The phantom-GPU fall-through (#241) has the same shape and is fixed by the same clause. Both are bookkeeping writes, already announced by their own Infof lines above the gate, not budget changes an operator needs to sanction.

Tests

TestSet_SameCeilingNeedsNoYes. The finding's sharpest point was that the regression is untested: every same-ceiling case already in the file passes yes: true — the flag under dispute — which is why nothing was red.

• Asserts exit 0 AND that the apply still happens. Either alone is satisfiable by the wrong fix: reverting the staleness treatment gives exit 0 with no re-stamp; feat(resources): stamp resources set as a human choice (backend#2220) #539 as merged gives the re-stamp only to callers who pass --yes.
• Reads RESOURCE_PROVENANCE=useroff the values file helm was actually handed (new fakeHelmValues helper). The existing provenance assertions all go through --dry-run, and --dry-run skips the very gate at issue — so they cannot prove this path.
• Covers the phantom-GPU sibling, and a declining prompter on a terminal (the only observable proof the gate was not entered: entering it would cleanCancel and shell no helm).
• One sub-case bounds the fix: a real ceiling change off a terminal still exits 1 and mutates nothing.

Five of the six sub-cases fail without the one-clause change — verified by reverting just that line:

--- FAIL: TestSet_SameCeilingNeedsNoYes/installer-sized_edge
restating the current ceiling must not need --yes, got: refusing to change the ceiling without confirmation: pass --yes, or run on a terminal
--- FAIL: TestSet_SameCeilingNeedsNoYes/pre-marker_edge
--- FAIL: TestSet_SameCeilingNeedsNoYes/junk_marker
--- FAIL: TestSet_SameCeilingNeedsNoYes/phantom_GPU_cleanup_needs_no_--yes
--- FAIL: TestSet_SameCeilingNeedsNoYes/not_even_asked_on_a_terminal

The sixth ("a real change still needs --yes") passes both ways by design — it is the guard, not the proof.

Docs

docs/cli-navigation.md: the no change node now says it may still re-stamp provenance or clear a phantom GPU, and never asks to confirm. That was already true of #539's behaviour, which shipped without updating the map. The --help exit-code table needed no change — 0 applied (or nothing to change) becomes true again. docs/troubleshooting.md does not list resources set under exit 1, so it stays correct.

Verification

go vet ./... clean · gofmt -s -l + goimports -l clean over tracked files · go test ./... all packages ok · make lint (errcheck, ineffassign, misspell, staticcheck) clean · make lint-full → 0 issues. · make file-budget / make check-style / make check-tool-pins ok.

_{Note for the reviewer: make check's fmt-check step flags three files under an untracked, stale nested git worktree (.claude/worktrees/practical-maxwell-4338c6/) left by an earlier local session. It is unrelated to this change and invisible to CI; I ran the fmt gate scoped to git ls-files instead, which is clean.}

🤖 Generated with Claude Code

---

Note

Medium Risk
Touches the resources set confirmation gate and helm apply path used by scripts. The change is a narrow skip for unchanged ceilings; actual budget changes still require confirmation.

Overview
Restores the documented exit-0 path for resources set when the per-run ceiling is unchanged. After #539, restating the current size (to re-stamp RESOURCE_PROVENANCE=user or clear a phantom GPU) fell through into the confirmation gate and failed off a TTY without --yes, breaking scripted callers.

The gate now also requires !ceilingUnchanged. Bookkeeping helm applies still run; a real size change still refuses without --yes or a terminal.

Tests cover installer/unknown/junk markers, phantom-GPU cleanup, a declining TTY prompter, and the guard that a real change still exits 1. The navigation map notes that the no-change path may re-stamp or clear GPU and never asks to confirm.

^{Reviewed by Cursor Bugbot for commit 24447b2. Bugbot is set up for automated code reviews on this repo. Configure here.}

[saadqbal]

@LukasWodka — could you approve this one too if it looks right? Blocked on review.

It's the resources set regression from #539: treating any non-userRESOURCE_PROVENANCE as stale meant the unchanged-ceiling path stopped returning early and fell into the --yes gate, so a non-interactive restatement of the current ceiling now exits 1 instead of the documented no-op. Nearly every installed edge is still unknown or installer, so it hits almost the whole fleet — and the e2e journey drives the CLI non-interactively, so it's the kind of thing that breaks automation rather than a person.

Fix is one clause (the gate guards the ceiling; an unchanged ceiling has nothing to ask about) and #539's provenance re-stamp still happens. It also fixes the docs #539 made false — docs/cli-navigation.md drew no change → exit 0 bypassing the confirm node.

Six new sub-cases; reverting just the fix line reddens five of them.

[LukasWodka]

Approving. I verified the claims locally rather than reading them approvingly, including the one that decides whether this fix is safe.

The mutation claim holds exactly. Reverting just the clause (anchor match asserted, so I know the mutation landed and isn't inert):

--- FAIL: TestSet_SameCeilingNeedsNoYes/installer-sized_edge
--- FAIL: TestSet_SameCeilingNeedsNoYes/pre-marker_edge
--- FAIL: TestSet_SameCeilingNeedsNoYes/junk_marker
--- FAIL: TestSet_SameCeilingNeedsNoYes/phantom_GPU_cleanup_needs_no_--yes
--- FAIL: TestSet_SameCeilingNeedsNoYes/not_even_asked_on_a_terminal

Five sub-cases, and a_real_change_still_needs_--yes passes both ways — precisely as you described it, the guard rather than the proof. Restored → green. go vet ./... clean, go test ./... all packages ok.

The thing I actually went looking for, since it's what makes this a one-line change instead of a revert: adding && !ceilingUnchanged means any case where ceilingUnchanged is wrongly true becomes a silent confirmation bypass on a real ceiling change. So the fix is only as exact as sameCeiling. It compares CPU/Mem via Cmp (quantity-aware, so unit spellings normalise), HasGPU, and GPUName+GPU when a GPU is present. Against resources.Training it omits two fields:

• Provenance — correctly, and deliberately: it "never affects the numbers", and treating a provenance difference as a ceiling change is exactly the bug #539 created. It's handled separately by staleProvenance.
• HasCPUMem — not compared. The only way that could matter is HasCPUMem=true with CPU=0, Mem=0 comparing equal to an unset current, which the floors validation should make unreachable.

Neither is introduced here — sameCeiling is untouched by this PR (0 lines in the diff), and it already gates the more consequential no-op early return above, where imprecision would skip the apply entirely rather than just a prompt. So the blast radius of any residual imprecision is strictly smaller on your new path than on the existing one. Not a blocker; noting it because "one clause" changes what sameCeiling is load-bearing for, and that's worth someone having checked once.

Two things in the tests I'd have asked for if they weren't there:

• Asserting exit 0 AND that the apply still happened. You're right that either alone is satisfiable by a wrong fix, and that's the distinction between fixing this and reverting #539.
• Reading RESOURCE_PROVENANCE off the values file helm was actually handed, via the new fakeHelmValues helper, because the existing provenance assertions all run through --dry-run — which skips the very gate in dispute. A test that cannot reach the path it names is the failure mode this whole class keeps hitting; good that you built the helper rather than reusing an assertion that looked adjacent.

Docs: the no change edge in docs/cli-navigation.md now says it may re-stamp provenance or clear a phantom GPU and never asks to confirm — which was already true of #539's shipped behaviour, so this corrects a map that was silently stale rather than documenting the new clause. And the --help table's 0 applied (or nothing to change) becomes true again rather than needing an edit, which is the right direction: the code moved back to the contract, not the contract to the code.

Noted your untracked-worktree aside on make check's fmt-check — scoping the gate to git ls-files is the right call and CI agrees. Gate: 10 checks pass (Bugbot included), 3 skipping, MERGEABLE, zero unresolved threads.

[LukasWodka]

/fr-pass