Skip to content

fix(delete): verify the host-data wipe before printing ✔ (RFC-0003) - #389

Merged
shujaatTracebloc merged 2 commits into
developfrom
fix/delete-verify-host-wipe
Jul 23, 2026
Merged

fix(delete): verify the host-data wipe before printing ✔ (RFC-0003)#389
shujaatTracebloc merged 2 commits into
developfrom
fix/delete-verify-host-wipe

Conversation

@saadqbal

@saadqbalsaadqbal commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Closes#388. The cli-side follow-up from the RFC-0003 offboard-hygiene work (design: backend#1151, client impl: tracebloc/client#367, RFC: #366).

Problem

tracebloc delete calls removeHostDataDir()os.RemoveAll(config.Dir()), and when that returns nil prints ✔ Removed local tracebloc data and config. But a nil RemoveAll is not proof the tree is gone — a racing writer, a mount, or a masked partial failure can leave it present. So offboard can claim a clean slate it didn't achieve, which is exactly the §2.4 / §3.3 hygiene gap the RFC calls out.

Fix

removeHostDataDir now stats the dir after RemoveAll and treats still-present (or an unexpected stat error) as a failure. The caller then prints the existing warn + manual-rm hint and marks the offboard degraded, instead of a false . Adds an osStat seam so tests drive it without touching the real ~/.tracebloc.

Test

TestDelete_WipeUnverified_DoesNotClaimSuccess: osRemoveAll returns nil but osStat reports the dir still present → delete must not print the success line and must warn. Existing success/keep-data/wipe-fail tests unchanged (the osStat seam defaults to "gone").

go build, go vet, full internal/cli suite green. Scope is the honesty fix only — the RFC's heavier multi-path wipe is unnecessary under node-local (client#368) and intentionally not added here.


Note

Low Risk
Narrow offboard-hygiene change in delete flow; improves honesty on wipe success with no auth or data-model impact.

Overview
tracebloc delete no longer treats a successful RemoveAll as proof that ~/.tracebloc is gone. After wiping the host data dir, removeHostDataDir now stats the path and fails if the directory still exists or if verification errors unexpectedly.

On failure, the existing "Couldn't remove local data" path runs (warning, manual rm hint, degraded offboard) instead of printing "Removed local tracebloc data and config." An osStat test seam mirrors osRemoveAll, and TestDelete_WipeUnverified_DoesNotClaimSuccess covers the case where RemoveAll returns nil but the dir is still present. Golden string snapshots pick up the new error messages.

Reviewed by Cursor Bugbot for commit 6227efc. Bugbot is set up for automated code reviews on this repo. Configure here.

saadqbaland others added 2 commits July 22, 2026 16:36
removeHostDataDir did os.RemoveAll and returned nil without confirming the
tree was actually gone; the caller then printed "✔ Removed local tracebloc
data and config". A nil RemoveAll is not proof of absence (racing writer,
mount, masked partial failure), so offboard could claim a clean slate it
didn't achieve — the RFC-0003 offboard-hygiene gap.
Now removeHostDataDir stats the dir after RemoveAll and treats "still
present" (or an unexpected stat error) as a failure, so the caller prints
the warn + manual-rm hint instead of ✔. Adds an osStat seam + a test
proving delete does NOT claim success on an unverified wipe.
Closes#388. Refs tracebloc/client#367, backend#1151, #366.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The wipe-verify error messages are user-visible (surfaced via the "Couldn't
remove local data (%v)" warn), so zz-all-strings.golden picks them up.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@saadqbalsaadqbal self-assigned this Jul 22, 2026
@saadqbal
saadqbal requested a review from LukasWodkaJuly 22, 2026 13:18
@shujaatTracebloc
shujaatTracebloc merged commit f09078a into developJul 23, 2026
20 checks passed
@shujaatTracebloc
shujaatTracebloc deleted the fix/delete-verify-host-wipe branch July 23, 2026 13:36
shujaatTracebloc pushed a commit that referenced this pull request Jul 23, 2026
…atus --seal) (#395)
* feat(status): surface the environment's seal-check verdict (client status --seal)
`tracebloc client status --seal` runs the chart's conformance checks (its
helm-test hooks — the RFC-0003 §8.2 seal check) against this machine's
secure environment and prints an honest verdict with per-check detail:
sealed every conformance check passed (exit 0)
unsealed a check failed — a protection is not enforced (exit 2)
unknown the chart ships no checks; nothing was verified (exit 2)
Chart contract (works against today's probes, picks up the growing
backend#1184 suite as it lands): test hooks labelled
`tracebloc.io/seal-check: "true"` form the suite; with no labelled hook
every helm test runs, with a visible fallback note; the optional
`tracebloc.io/seal-name` / `tracebloc.io/seal-hint` annotations refine a
check's display name and its failure hint. Checks run one
`helm test --filter name=<hook>` at a time so a first failure can't hide
the rest of the suite's state, and helm stays pinned to the resolved
kubeconfig/context — never the ambient one.
Honest-output rules (the #389 spirit): only a fully-passed suite exits 0;
"unknown" is never worded as sealed; a cancelled run (Ctrl-C) and a failed
hook enumeration yield no verdict at all.
Also: moves the status command into client_status.go (client.go sat at its
1050-line file-budget ceiling) and adds Printer.Spinner strings to the
copy-catalog harvest (they print as static lines on non-TTY runs and were
previously missed by the backstop).
Closes#393
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* docs(troubleshooting): add client status --seal to the exit-code table
The table is the cross-command scripting contract; the new seal mode
produces 2 (unsealed / unknown via exitChecksFailed) and shares the
3 / 4 cluster-resolution codes with the data and resources commands.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(seal): align to the chart's shipped label contract; carry aux hooks in every filter
Two alignments against the chart-side suite (client docs/SEAL-CHECK.md,
backend#1184) that landed in parallel:
- The per-check identifier is the tracebloc.io/seal-check-name LABEL
(values: egress-enforcement, backend-reachability, storage-assertions),
not a seal-name annotation — read it from labels. The hint stays an
optional CLI-side annotation (labels cannot carry sentences); absent, the
kubectl-logs fallback stands.
- The storage-assertions Job depends on a ServiceAccount/RBAC that are
themselves test hooks at negative hook-weight, and helm's --filter
excludes every unlisted test hook — plumbing included. Filtering to the
check alone would strand the Job without its SA and report a false
Unsealed. Every per-check run now lists the check PLUS the release's
non-runnable test hooks (applied instantly; helm only waits on
Jobs/Pods), while other checks stay excluded so the verdict remains
per-check. Non-runnable hooks never count as checks: a chart whose only
test hooks are plumbing is still honestly "unknown".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(seal): pin helm to the resolved kube-context; quiet exit on Ctrl-C during enumeration (Bugbot)
- helm.TestTarget.KubeContext now carries target.Resolved.Context, not the
raw --context flag: with the flag omitted the raw value is empty and helm
falls back to its own ambient resolution ($HELM_KUBECONTEXT included),
which can diverge from the context client-go discovery just used. Same
pinning `resources set` applies.
- A context cancellation during `helm get hooks` now exits 130 quietly
(like the per-check loop and `status --wait`) instead of reporting a hard
enumeration failure.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: shujaat hasan <shujaathasan@shujaats-MacBook-Pro.local>
@LukasWodka

Copy link
Copy Markdown
Contributor

Functional review \u2014 passed\n\nBasis: the behavioural suites that ran on this PR at merge against real environments, not mocks:\n\n- + \u2014 the CLI exercised against a real kind cluster, plus the installer suite\n\nI could not reach the dev API (no dev credentials), so rather than rubber-stamp I used the strongest evidence available: these suites exercise the actual behaviour this change alters, on real infrastructure. Advancing \u2192 .\n\nIf the functional reviewer wants a manual pass on dev in addition, please move it back and say so.

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.

3 participants

@saadqbal@LukasWodka@shujaatTracebloc