fix(cluster): verify the mutating target from the API and fail closed when it can't be (backend#2983)

[aptracebloc]

Fixes tracebloc/backend#2983.

The bug

A machine with no recorded cluster anchor warned

⚠ … the target couldn't be verified before changing anything. Run `tracebloc
  client create` to record it. Proceeding against https://127.0.0.1:8444.

…and then mutated anyway. The endpoint it named is a localhost port-forward — tbcluster allocates 8443/8444/8445 across fleets, so the same command shape (same flags/--namespace/--context) reaches a different fleet depending on which tunnel is up on which port. The operator's only signal about which environment was about to change was a port number, and the success panel is identical whichever fleet it hit.

Worst of all, the run carried --no-input — whose contract is "disable prompts; fail on missing required values" — and it did not fail closed. A CI job ingesting into the wrong fleet got a green exit and a correct-looking summary. data delete (drops a table + PVC files) prints the identical line, so the same fail-open sat on a destructive, irreversible path.

This is the gap backend#2863 left: it added a local anchor to compare against, but the no-anchor path stayed warn-and-proceed.

The fix (both directions from the issue)

guardActiveClientCluster now:

1. Verifies the target identity from the API. It asks the backend which client owns the cluster actually reached — keyed on the live kube-system UID — and prints that identity (name (namespace)) instead of a port. This does not depend on local state, which is exactly what was missing in the field.
2. Fails closed when the target is unverifiable. If the API can't confirm the target and no recorded anchor matches it, the command exits non-zero without writing, unless the operator passes the new --i-know-the-target. A scripted --no-input caller has no human to ask, so this honours its contract; a human gets the same named escape hatch.

Kept from backend#2863: an anchor mismatch is still a hard refusal (--i-know-the-target overrides an unknown target, never a known-wrong one), and an anchor that matches the live cluster still proceeds offline.

--i-know-the-target is added to every command that mutates through resolveClusterTarget — data ingest, data delete, resources set, client status --seal — so the safety check is uniform and never a dead end. The anti-rot call-site sweep is updated for the new argument.

Out of scope (deliberate): tracebloc delete (offboard) keeps its documented fail-open — a dead/unreachable cluster is the main reason to offboard, and blocking it would leave a machine unremovable (TestDelete_UnreachableCluster_StillOffboards, TestDelete_NoAnchor_StillOffboards). The OTEL_EXPORTER_OTLP_ENDPOINT is unset line is also untouched (backend#2145 NOT_PLANNED / backend#2751 COMPLETED).

Tests

• data ingest and the destructive data delete path fail closed under an unverifiable target (before any staging / before the dataset list is read).
• The printed identity comes from the API, not local state (decoy local name; API name is what's printed).
• The --i-know-the-target escape hatch proceeds with a warning.
• Existing backend#2863 mismatch / unreadable-id / right-cluster tests still hold; the anti-rot mutation-intent sweep is updated to the new call-site arity.

Full go test ./... green, golangci-lint 0 issues, gofmt clean, goldens regenerated.

Behaviour change to note for review

An unreadable config, or a not-signed-in machine with no anchor, now fails closed where it previously warned-and-proceeded — this is the intended #2983 tightening (can't verify ⇒ don't mutate), with --i-know-the-target as the escape hatch. Flagged here so it's a conscious call.

Versioning

VERSION 0.10.20 → 0.10.21: v0.10.20 is already released, so this first published-path PR of the cycle carries the pending bump the version-bump-gate requires (same pattern as backend#2863's own #608: 0.10.17 → 0.10.18).

🤖 Generated with Claude Code

---

Note

High Risk
Changes safety behavior for destructive mutating CLI paths (ingest/delete/resources); scripted --no-input runs without anchor or API reachability will now error instead of mutating, though --i-know-the-target remains the escape hatch.

Overview
Mutating cluster commands (data ingest, data delete, resources set, client status --seal) no longer warn and proceed when the target cluster cannot be verified. They fail closed unless the operator passes --i-know-the-target.

guardActiveClientCluster now confirms identity with the backend (kube-system UID → account client) and prints API-backed client name/namespace when verification succeeds. With no local anchor and no API confirmation, mutations are refused instead of continuing against an ambiguous endpoint (e.g. localhost port-forwards). --i-know-the-target only overrides unknown targets—it does not bypass a recorded anchor mismatch or HTTP 426 (upgrade required). Ctrl-C during guard steps exits quietly (130).

The flag is wired through resolveClusterTarget (new ackTarget parameter), shared via addKnowTargetFlag / knowTargetFlag in flags.go, and help goldens are updated. VERSION bumps 0.10.20 → 0.10.21.

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

[LukasWodka]

Reviewed at high effort. This is a clean, well-reasoned fix — the six guard paths (nil target · unreadable ID ±ack · anchor mismatch · API-verified · anchor-match-offline · unverifiable ±ack) are all covered, the mismatch stays a hard refusal that --i-know-the-target can't override, the anti-rot arity sweep is updated to 8 args, and the API-identity-not-local-state point is proven by the decoy-name test. The --no-input fail-closed at the data ingest / data delete boundaries (before staging / before the dataset list) is exactly the field bug closed.

The code is correct as written — nothing below is a defect. Three things, all test-hardening or a conscious-call question, not blockers:

1. Pin the security asymmetry with a test (recommended). The whole safety story rests on "--i-know-the-target overrides an unknown target, never a known-wrong one." That invariant is stated in the comments and holds structurally (the mismatch branch at clusterguard.go returns without consulting ackTarget), but every mismatch test runs ack=false. A future edit adding if ackTarget { return nil } to the mismatch branch "for symmetry" would silently turn the flag into a known-wrong-cluster bypass and nothing would redden. A WrongCluster + ack=true case asserting it still refuses is the house-rules mutation-proof for this guard, and it's cheap.

2. The unreadable-ID + ack branch is untested. guardActiveClientCluster's idErr && ackTarget → warn+proceed path has no coverage (UnreadableID_Refuses only exercises ack=false). A minor twin of #1.

3. --dry-run now fails closed on an unverifiable target — intended? The guard runs inside resolveClusterTarget (mutates=true) before the dry-run branch, so on a no-anchor machine data delete --dry-run / resources set --dry-run against an unverifiable cluster now refuses instead of showing the preview — a change from the old warn-and-proceed that the body flags generally but not for dry-run specifically. Defensible (the preview needs to know the cluster anyway), just want it to be a conscious call rather than a side effect.

Not approving yet only because CI is still running (builds/tests/lint/Bugbot all pending) — I'll approve as soon as it lands green; #1–#3 are yours to take or leave with a reason.

[LukasWodka]

CI went green, but I'm holding the approval — Bugbot's Medium ("verifier swallows upgrade-required errors") is a real one, and I verified it against the tree rather than taking it on faith:

verifyTargetFromAPI does a blanket if err != nil { return nil, false }, folding a 426 *api.UpgradeRequiredError into plain "unverifiable." But 426 is a typed error the rest of the CLI treats as a hard stop — auth.go, delete.go, doctor.go, client_status.go, client.go all errors.As it to tell the operator to upgrade, and .cursor/BUGBOT.md records that as the convention ("every caller" branches on it). Swallowing it here means:

• an anchored machine on a too-old CLI skips the API verdict and proceeds offline via the anchor-match path (step 5) — it mutates when the backend just told it not to;
• an unanchored one fails closed with "run tracebloc client create", pointing at the wrong fix when the real one is tracebloc upgrade.

Suggest errors.As-ing the ListClients error for *UpgradeRequiredError in verifyTargetFromAPI (or its caller) and surfacing it as the hard upgrade error the other call sites already return, instead of collapsing it to unverifiable. A small test that a 426 from listAccountClientsFn hard-stops (rather than proceeding on a matching anchor) would pin it.

Holding until that's addressed or you resolve the thread with a reason. Everything else from my review above still stands — the core guard is correct, this is the one path that leaks a signal it shouldn't.

[aptracebloc]

@LukasWodka thanks for the careful review. All three taken:

1. Security-asymmetry mutation-proof — added TestResolveClusterTarget_Mutating_WrongCluster_AckDoesNotOverride: a mismatch with --i-know-the-target set still refuses. If a future edit makes the mismatch branch consult ackTarget, that test reddens.

2. Unreadable-ID + ack — added TestResolveClusterTarget_Mutating_UnreadableID_AckProceeds covering the idErr && ackTarget → warn+proceed branch.

3. --dry-run fails closed on an unverifiable target — conscious call, yes. The guard runs in resolveClusterTarget (mutates=true) before the dry-run branch on purpose: a dry-run that could not verify the target has not actually checked out the secure environment, so reporting "checks out" would be the same dishonesty the issue is about — and it is strictly safer (no write either way), with --i-know-the-target as the escape hatch. Better to surface it at dry-run than to pass dry-run and fail the real run.

Also fixed a real one Bugbot caught in the same push (8462704): the verifier was swallowing a 426 UpgradeRequiredError into "\unverifiable'", so a too-old CLI on an anchored machine would have proceeded. It now hard-stops with the tracebloc upgrade message before the anchor-match, and --i-know-the-target does not override it.

Re-requesting review.

[aptracebloc]

bugbot run

[LukasWodka]

Still holding — Bugbot landed a second Medium on 8462704 ("offline lookup reported as no client"), and the message-accuracy half is real (I checked it against the code, not just the summary):

verifyTargetFromAPI's own return-contract comment says (nil, nil) covers both "the API answered, no client anchored here" and "couldn't be reached (offline / not signed in / transient)" — explicitly "Both are 'couldn't confirm'." But the fail-closed refusal then asserts one of them — "tracebloc has no client registered for it" — and points at client create. So an offline, no-anchor operator is told a fact the CLI never actually established, and nudged toward a client create that (the #515 shape Bugbot cites) can mint a duplicate rather than adopt — most sharply against a legacy client whose ClusterID is still empty.

The guard itself is still correct: it refuses to mutate, which is the part that matters. This is about the refusal wording not overstating what's known — worth carrying the couldn't-confirm-vs-confirmed-absent distinction out of verifyTargetFromAPI (e.g. a third return state, or a typed "unreachable" error) so the message can say "couldn't reach tracebloc to check" instead of "no client registered" when that's the real situation.

Holding until it's addressed or you resolve the thread with a reason. Everything from 8462704 still checks out — the 426 hard-stop and the three added tests (mismatch+ack, unreadable-ID+ack, 426-hard-stops) are all correct.

[aptracebloc]

bugbot run

[aptracebloc]

@LukasWodka that is exactly the fix — landed in 932d817 (pushed a few minutes after your review, so you likely reviewed 8462704).

verifyTargetFromAPI now returns the third state you suggested: (client, reached bool, err), mirroring DiscoverInClusterClientIDs three-valued contract and clientSurvey.looked. The guard branches on reached:

• reached=false (offline / not signed in / transient) → "tracebloc couldn't be reached to confirm the target", points at tracebloc login / --context. It no longer asserts absence, and no longer nudges toward client create on an unconfirmed cluster — closing the A wrong active-client pointer has no supported recovery — and doctor then says "no secure environment" over a healthy install #515 mint-vs-adopt half.
• reached=true, no match → confirmed absence, where client create is honest (and the wording now notes it adopts an existing client on the cluster, covering the legacy empty-ClusterID row).
• 426 still hard-stops; the anchor-match and mismatch paths are unchanged.

The return-contract comment you quoted is rewritten to describe the three states. Two tests pin the split: an unreachable case asserting the refusal does not say "no client registered" / "client create", and a confirmed-absence case asserting it does. Bugbot thread resolved with the same note; re-requesting your review.

[LukasWodka]

Third Bugbot Medium, and it's a real residual of the same theme (I checked it). After the reached/not-reached split, the reached && anchoredClient==nil branch still can't tell "the API confirmed no client here" from "the API returned a legacy client whose ClusterID is empty, so it couldn't match this UID." Bugbot's sharpest tell is the giveaway: this guard only fires after resolveClusterTarget already resolved a jobs-manager / release on the cluster — so in the legacy case the refusal asserts "tracebloc has no client registered for it" about a cluster the CLI literally just found a client release on, and can still misfire the #515 client create mint.

It's a narrower edge than the first two — it needs a legacy empty-ClusterID record — so whether to close it now or carry it as a follow-up is your call. If you close it: either treat an empty-ClusterID match-miss as couldn't-confirm rather than confirmed-absence, or cross-check the release resolveClusterTarget already resolved before claiming absence. Either way the gate holds my approval until the thread's resolved (fix or a reasoned resolve). Everything from 932d817 — the three-valued (client, reached, err) split and its two branch tests — is otherwise correct.

[aptracebloc]

bugbot run

[aptracebloc]

@LukasWodka closed in 171eeb3 (landed a couple minutes after this review). I took the message-accuracy route of your two options: the reached && anchoredClient==nil branch no longer asserts absence at all. Since — as you and Bugbot both note — the guard only fires after resolveClusterTarget resolved a release on the cluster, the refusal now says the on-cluster client "isn't linked to any client in your account (it may be newly installed, or created before cluster anchoring)" rather than "no client registered."

On the #515 mint worry: client create here is safe to recommend precisely because it's the adopt path — it discovers the in-cluster CLIENT_ID and adopts + backfills the anchor (R7), so on a legacy empty-ClusterID client it adopts rather than mints. The wording now says so ("adopts the client already on this cluster and records it").

Test renamed ReachedButUnmatched, seeds a legacy empty-ClusterID record in the account list, and asserts the refusal does not contain "has no client registered". Bugbot thread resolved. Re-requesting review.

[LukasWodka]

Fourth Bugbot Medium, and it's a real (narrow) one — I traced the flow. verifyTargetFromAPI folds context.Canceled into the generic "couldn't ask → reached=false" bucket. So a Ctrl-C during the 8s lookup becomes reached=false: with a matching anchor the guard then proceeds (the operator's abort is swallowed), and without one it exits 3 with "couldn't be reached / run login" instead of a clean 130 — then the next cluster call runs under the already-cancelled context and fails with a misleading kubeconfig/workspace error.

The distinction worth drawing: the WithTimeout's own DeadlineExceeded (the 8s fallback) should stay "couldn't ask" — that's the intended behaviour — but a parent context.Canceled (operator abort) should propagate. An errors.Is(err, context.Canceled) check that returns the cancellation (so the command aborts / exits 130) rather than reclassifying it as unreachable would close it.

It needs a Ctrl-C inside that 8s window, so it's narrow — fix-now vs a follow-up ticket is your call, and the gate holds my approval until the thread's resolved either way. Everything from 171eeb3 (the "not linked to your account" wording + the legacy empty-ClusterID test) is correct.

[aptracebloc]

bugbot run

[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 c49d1e6. Configure here.}

[LukasWodka]

Fifth Bugbot — this one Low, and a small regression the #4 fix introduced: interrupted attaches ctx.Err() to the exitError, so IsSilentError is false and a Ctrl-C during the new verify steps now prints Error: context canceled instead of the quiet exit-130 the other interrupt paths give. One-liner — return the exitError with a nil inner error (or whatever silent variant those other paths use) so 130 stays silent.

Genuinely minor and a perfectly fine fast-follow if you'd rather land this now — your call. Nothing else is outstanding: the four earlier findings and my three original notes are all fixed and tested, so this cosmetic exit line is the only thing between here and my approve once CI's green.

[aptracebloc]

bugbot run

[aptracebloc]

@LukasWodka green and clean on 8360670: CI all passing and Cursor Bugbot is a clean pass. Bugbot ran five rounds on the new API-verification path, all now resolved:

1. 426 UpgradeRequiredError swallowed → hard-stop before the anchor-match.
2. couldn't-reach vs confirmed-absence conflated → three-valued (client, reached, err).
3. legacy empty-ClusterID reported as absence → "isn't linked to your account" wording (no false absence; client create framed as adopt).
4. Ctrl-C during the ~8s lookup could authorize the mutation on a matching anchor → interrupted(ctx) returns 130 before the anchor-match.
5. that interrupt exit wasn't silent → bare &exitError{code: exitInterrupted}.

The guard's fail-closed core was unchanged throughout — these hardened the edges of the new backend lookup. Ready for your approval whenever you are.

[LukasWodka]

Approving. CI is green, Bugbot's re-analysis of 8360670 landed clean (no new finding), and all five of its threads are resolved — I re-checked the head so this verdict lands on the commit I reviewed.

This went through an unusually deep hardening cycle, and every round closed with a real fix and a test. For the record, what I verified across the chain:

• Core guard (backend#2983): verifies the mutating target's identity from the backend, prints it, and fails closed when it can't be confirmed — closing the --no-input fail-open that could mutate the wrong fleet with a green exit. Anchor mismatch stays a hard refusal --i-know-the-target can't override.
• Bugbot #1 (426): verifyTargetFromAPI now propagates only UpgradeRequiredError and hard-stops before the offline anchor-match — a too-old CLI can't mutate on a stale anchor.
• Bugbot #2/#3 (offline vs absence, legacy empty ClusterID): the three-valued (client, reached, err) split distinguishes "couldn't reach the API" from "reached, no client here," and the reached-but-unmatched refusal says "not linked to your account" rather than falsely asserting absence — no more mis-directing an operator into a client create that mints a duplicate.
• Bugbot #4/#5 (Ctrl-C): interrupted(ctx) checks the command's context (not the lookup error, so the inner 8s timeout is unaffected) and exits a silent 130 — an operator abort during the check is a clean abort, not a swallowed proceed or a noisy refusal.
• My three earlier notes (the ack-vs-mismatch asymmetry, the unreadable-ID+ack branch, the 611→613 comment) are all in too.

Each of those carries a targeted test that reddens if the behaviour regresses. Nice work seeing a long chain of findings all the way through. ✅

(Landing this is a /ship / release-train action, not mine — this is a code approval only.)

[LukasWodka]

/fr-pass

Functional review on staging — passed, with direct evidence.

Tier-A journey, staging · amd64, run 33548672373 — green end to end against the deployed staging backend (d87e7847, docker-build success). Train + leaderboard leg:

experiment ejtt4m3h: COMPLETED (terminal, 27 poll(s))
leaderboard: found after 22 poll(s), running_score=0.9, cycle=2
submissions: our run is on the board (accuracy=0.9 loss=0.3456 captured=True); 1 row(s) total

This repo's change is on the path that run exercised (install / ingest / use-case / train / leaderboard), so this is functional evidence, not an inference from review.