fix(sandbox): surface teardown failures and exit non-zero when removal fails

[SUaDtL]

Closes #393

The defect, verified against source before any edit

All four claims in the issue held on current origin/main (4ec1a8a, post-#425):

Site	Code
destroy.ts:69	if (r.code === 0) removedContainers.push(c);
destroy.ts:81	if (r.code === 0) removedVolumes.push(v);
destroy.ts:113	if (r.code === 0) removedContainers.push(c);
destroy.ts:119	if (r.code === 0) removedVolumes.push(v);
cli.ts:368-377	destroy and prune both return 0; unconditionally

DestroyResult / PruneResult carried no failure field at all, so a non-zero docker result was not merely unreported — it was unrepresentable. A daemon outage, a permission error, or an in-use object left untrusted sandbox containers and source volumes running while automation received exit 0. That is a containment failure: the teardown guarantee was false precisely on the path where cleanup matters.

The fix

Beyond "record the error", three properties:

1. Best-effort, never aborting. A failed removal no longer influences what else is attempted — every discovered container and volume is still targeted, so a partial teardown reclaims everything it can. Asserted directly: three containers, the first fails, all three are still attempted.
2. Verified, not assumed. After the removals both verbs re-list the same label scope and report what is still present (remainingContainers / remainingVolumes). A --keep-volume volume is a deliberate survivor and is excluded from that set, so keeping a volume is still a clean exit 0.
3. A failed listing is itself a failure. Discovery and verification route through new listContainersResult / listVolumesResult in registry.ts, which retain docker's exit code. Without this, a fully dead daemon discovers nothing, removes nothing, and reports a clean sweep — the exact false-negative the issue is about. listContainers / listVolumes become unchanged thin wrappers, so no read-only caller changes behavior.

Surfacing: failures is bounded (MAX_TEARDOWN_FAILURES = 25) while failureCount stays exact, so a wedged daemon cannot flood stdout/stderr yet the report never understates the damage. teardownIncomplete() is the verdict; formatTeardownDiagnostic() renders a bounded stderr report naming each failed op with docker's own exit code and message, then every object still present and the manual docker rm -f / docker volume rm needed. The full structured result still goes to stdout as JSON for scripting. New exit code TEARDOWN_FAILURE_EXIT = 1, distinct from 2 (usage error).

The verbs still return rather than throw — the exit-code decision belongs to the CLI, so a library caller can inspect a partial teardown instead of catching.

Red test first — verbatim failure output

teardown.test.ts was written and run before any source change. It drives the real destroySandbox / prune and the real runCli over an injected stateful fake docker (a successful rm actually removes the object from later label listings), so the post-teardown verification is exercised honestly rather than against a listing frozen in time.

$ cd plugins/ca-sandbox/tools && npx vitest run teardown.test.ts --reporter=basic

 RUN  v2.1.9 .../plugins/ca-sandbox/tools

 ❯ teardown.test.ts (16 tests | 13 failed) 12ms
   × destroySandbox — a failed removal is retained, not dropped (#393) > complete success reports zero failures and nothing remaining 4ms
     → expected undefined to deeply equal []
   × destroySandbox — a failed removal is retained, not dropped (#393) > a mixed run keeps the successes AND records the failure with its docker code 1ms
     → expected undefined to be 1 // Object.is equality
   × destroySandbox — a failed removal is retained, not dropped (#393) > keeps attempting EVERY discovered target after the first failure 0ms
     → expected undefined to be 2 // Object.is equality
   × destroySandbox — a failed removal is retained, not dropped (#393) > a dead daemon is a FAILURE, not an empty successful sweep 2ms
     → actual value must be number or bigint, received "undefined"
   × destroySandbox — a failed removal is retained, not dropped (#393) > --keep-volume: the deliberately kept volume is NOT reported as remaining 0ms
     → expected undefined to be +0 // Object.is equality
   × destroySandbox — a failed removal is retained, not dropped (#393) > the failure list is BOUNDED while the count stays truthful 0ms
     → expected undefined to be 50 // Object.is equality
   × prune — a failed reclaim is retained, not dropped (#393) > complete success reports zero failures and nothing remaining 0ms
     → expected undefined to deeply equal []
   × prune — a failed reclaim is retained, not dropped (#393) > a mixed run records the failure and names the object left behind 0ms
     → expected undefined to be 1 // Object.is equality
   × prune — a failed reclaim is retained, not dropped (#393) > a dead daemon is a FAILURE, not an empty successful sweep 0ms
     → actual value must be number or bigint, received "undefined"
   × runCli — teardown failure must not exit 0 (#393) > `destroy` exits NON-ZERO and names the object left behind 1ms
     → expected +0 not to be +0 // Object.is equality
   × runCli — teardown failure must not exit 0 (#393) > `destroy` on a dead daemon exits NON-ZERO 0ms
     → expected +0 not to be +0 // Object.is equality
   × runCli — teardown failure must not exit 0 (#393) > `prune` exits NON-ZERO and names every object left behind 0ms
     → expected +0 not to be +0 // Object.is equality
   × runCli — teardown failure must not exit 0 (#393) > `prune` on a dead daemon exits NON-ZERO 0ms
     → expected +0 not to be +0 // Object.is equality

⎯⎯⎯⎯⎯⎯ Failed Tests 13 ⎯⎯⎯⎯⎯⎯⎯

 FAIL  teardown.test.ts > destroySandbox — a failed removal is retained, not dropped (#393) > complete success reports zero failures and nothing remaining
AssertionError: expected undefined to deeply equal []

- Expected:
Array []

+ Received:
undefined

 ❯ teardown.test.ts:101:26
     99|     expect(res.removedContainers).toEqual(["c1"]);
    100|     expect(res.removedVolumes).toEqual(["ca-sbx-vol-id1"]);
    101|     expect(res.failures).toEqual([]);
       |                          ^
    102|     expect(res.failureCount).toBe(0);
    103|     expect(res.remainingContainers).toEqual([]);

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/13]⎯

 FAIL  teardown.test.ts > destroySandbox — a failed removal is retained, not dropped (#393) > a mixed run keeps the successes AND records the failure with its docker code
AssertionError: expected undefined to be 1 // Object.is equality

- Expected:
1

+ Received:
undefined

 ❯ teardown.test.ts:118:30
    116|     expect(res.removedVolumes).toEqual([]);
    117|
    118|     expect(res.failureCount).toBe(1);
       |                              ^
    119|     expect(res.failures).toHaveLength(1);
    120|     expect(res.failures[0].op).toBe("remove-volume");

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/13]⎯

 Test Files  1 failed (1)
      Tests  13 failed | 3 passed (16)

Each failure is on the defect itself — the missing failure surface (undefined where a failures / failureCount / remaining* field must be) and the exit-0 path (expected +0 not to be +0) — not on an import or a compile error. The 3 that passed pre-fix are the clean-teardown cases, which correctly exited 0 already; they are kept so the fix cannot be a blanket "always fail".

Before / after

Suite	Before (origin/main 4ec1a8a)	After
plugins/ca-sandbox/tools — npx vitest run (docker-gated)	18 files / 209 passed	18 files / 225 passed (+16 new in teardown.test.ts)
plugins/ca-sandbox/tools — npx tsc --noEmit	clean	clean
plugins/ca/hooks/tests — python -m unittest	1057 OK	1057 OK
.github/scripts — python -m unittest	1064 run, 4 errors/failures, 5 skipped	1064 run, same 4, 5 skipped (unchanged — see below)
python tools/build-host-packages.py --check --release-guard-base <merge-base>	pass	pass (no Pi payload change)
python3 .github/scripts/check-plugin-refs.py ca-sandbox	pass	pass
sandbox.js staleness gate (rebuild + git diff --quiet)	n/a	clean after rebuild

Baseline 209 is arithmetic-exact, not estimated: the only test file added is teardown.test.ts (16 cases); the edits to cli.test.ts and lifecycle.test.ts add assertions to existing cases, never new ones.

The 4 non-green .github/scripts results are pre-existing and environmental, identical before and after this branch: test_pi_benchmark requires plugins/ca-pi/tools/node_modules (the pinned esbuild), which is not installed in this worktree — the harness itself raises RuntimeError: Pi benchmark requires the installed pinned esbuild. Nothing in this diff touches Pi.

Test changes to pre-existing files, and why

lifecycle.test.ts's fakeDocker returned scripted stdout per arg-match, so its label listings were frozen in time: a removed object still appeared in every later listing. That was invisible while teardown never re-listed. Now that teardown verifies its own work, such a fake would report every successfully removed object as a leak and would lock in a physically impossible docker. Its fakeDocker now models removal (a successful rm drops the ref from later listings) — the same discipline the new suite uses. Existing assertions are untouched and still pass; three clean-teardown cases gained failureCount / remaining* assertions.

cli.test.ts's fakeHandlers gained the new success-shape fields (they are required by the widened result types).

Prose

commands/sandbox-destroy.md and skills/sandbox-lifecycle/SKILL.md state the new contract as a hard gate: a teardown may not report success when docker refused a removal or the verification could not confirm the scope is empty. The prose previously promised "zero labeled objects remain" with no statement of what happens when that promise cannot be kept.

Payload version

ca-sandbox 0.1.4 -> 0.1.5 with a new dated CHANGELOG section. Tag ca-sandbox-v0.1.4 is unpublished, so the CI guard would have tolerated staying on 0.1.4; a distinct section was chosen so this behavioral change is not filed under #424's "supply-chain hardening" heading.

NEEDS-TRIAGE (not in this diff)

• create.ts's clone-failure rollback has the same shape. On a failed clone it fires docker rm -f / volume rm -f for the half-built sandbox and ignores the exit codes (create.ts ~L451), so a rollback that docker refuses leaves a labeled volume behind and the caller only sees the clone error. Less severe than Surface and fail sandbox teardown when Docker removal fails #393 (the object is labeled, so prune can still reclaim it, and no container is running untrusted code yet) but it is the same silent-drop pattern. Deliberately out of scope — Surface and fail sandbox teardown when Docker removal fails #393 names destroy.ts and cli.ts only.
• registry.ts:labelValue swallows a failed inspect (if (r.code !== 0) return ""), so a container whose inspect fails is silently grouped under the leaked/no-id bucket in listSandboxes. Read-only surface, no containment impact, but it makes a docker error look like a legitimately unlabeled object.
• destroy/prune never re-attempt a failed removal. A volume that is momentarily in-use fails once and is reported as remaining. A bounded retry (or removing containers, waiting, then volumes) would cut the false-leak rate. Explicitly not added here: it is a behavior change beyond the issue, and a wrong retry policy on a wedged daemon is worse than an honest failure report.

https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L