Follow-ups from the adversarial review of #429 (all LOW; none blocked that merge, which delivered the core property — failed Docker removals no longer vanish behind exit 0).
1. A single dead daemon is counted and reported four times
destroySandbox/prune record list-containers + list-volumes at discovery, then verifyScope re-runs the same two listings against the same dead daemon and records them again.
Empirically reproduced against the shipped artifact:
DOCKER_HOST=tcp://127.0.0.1:1 node sandbox.js prune
-> failureCount: 4, with the identical 'error during ...' diagnostic repeated
One unreachable daemon should read as one failure.
2. prune's post-sweep verification re-lists the GLOBAL scope
prune() verifies by re-listing the global ca.sandbox=1 scope. A sandbox created by another process or agent after discovery therefore lands in remainingContainers and makes prune exit 1 with These objects may be running UNTRUSTED code — remove them by hand, despite prune never having targeted it.
It fails safe (over-reports rather than under-reports), but a scary false positive teaches operators to ignore the signal — which defeats the point of #393. Verification should be scoped to what this invocation actually tried to remove.
The #429 author correctly did not add a remaining-empty assertion to the docker-gated prune test, so nothing is currently wrong in CI.
3. TEARDOWN_FAILURE_EXIT is never asserted
Every CLI exit-code assertion in teardown.test.ts is expect(code).not.toBe(0). The constant is exported and #429 explicitly claims it is "distinct from 2 (usage error)" — but nothing pins the value. It could regress to 2, collide with the usage-error code, and the whole suite stays green. Assert the exact value.
4. Bounded-diagnostic rendering is untested
formatTeardownDiagnostic's two elision branches (... and N more failure(s) not shown, and boundedRefs' ... and N more <label>(s)) plus MAX_DIAGNOSTIC_REFS have zero coverage. The "failure list is BOUNDED" test asserts only on the result object (failures.length <= 25), never on the rendered string — so the bounded-diagnostic claim is unverified where it actually matters.
5. --keep-volume can name a volume the operator asked to keep
destroySandbox (~L215): if the DISCOVERY volume listing fails (leaving keptVolumes empty) but the VERIFICATION listing succeeds, the deliberately-kept volume falls through the !keptVolumes.includes(v) filter and is reported in remainingVolumes. Exit is already non-zero from the listing failure so there is no false success — but the diagnostic names a volume the operator explicitly asked to keep, which is confusing at exactly the wrong moment.
Acceptance criteria
- AC-1: One unreachable daemon produces one failure entry, not four.
- AC-2:
prune's verification is scoped to the objects this invocation targeted; a concurrently created sandbox does not fail it.
- AC-3: A test pins the exact value of
TEARDOWN_FAILURE_EXIT and that it differs from the usage-error code.
- AC-4: Both elision branches are exercised against the rendered string.
- AC-5: A kept volume is never listed as remaining.
Follow-ups from the adversarial review of #429 (all LOW; none blocked that merge, which delivered the core property — failed Docker removals no longer vanish behind exit 0).
1. A single dead daemon is counted and reported four times
destroySandbox/prunerecord list-containers + list-volumes at discovery, thenverifyScopere-runs the same two listings against the same dead daemon and records them again.Empirically reproduced against the shipped artifact:
One unreachable daemon should read as one failure.
2.
prune's post-sweep verification re-lists the GLOBAL scopeprune()verifies by re-listing the globalca.sandbox=1scope. A sandbox created by another process or agent after discovery therefore lands inremainingContainersand makes prune exit 1 withThese objects may be running UNTRUSTED code — remove them by hand, despite prune never having targeted it.It fails safe (over-reports rather than under-reports), but a scary false positive teaches operators to ignore the signal — which defeats the point of #393. Verification should be scoped to what this invocation actually tried to remove.
The #429 author correctly did not add a remaining-empty assertion to the docker-gated prune test, so nothing is currently wrong in CI.
3.
TEARDOWN_FAILURE_EXITis never assertedEvery CLI exit-code assertion in
teardown.test.tsisexpect(code).not.toBe(0). The constant is exported and #429 explicitly claims it is "distinct from 2 (usage error)" — but nothing pins the value. It could regress to 2, collide with the usage-error code, and the whole suite stays green. Assert the exact value.4. Bounded-diagnostic rendering is untested
formatTeardownDiagnostic's two elision branches (... and N more failure(s) not shown, andboundedRefs'... and N more <label>(s)) plusMAX_DIAGNOSTIC_REFShave zero coverage. The "failure list is BOUNDED" test asserts only on the result object (failures.length <= 25), never on the rendered string — so the bounded-diagnostic claim is unverified where it actually matters.5.
--keep-volumecan name a volume the operator asked to keepdestroySandbox(~L215): if the DISCOVERY volume listing fails (leavingkeptVolumesempty) but the VERIFICATION listing succeeds, the deliberately-kept volume falls through the!keptVolumes.includes(v)filter and is reported inremainingVolumes. Exit is already non-zero from the listing failure so there is no false success — but the diagnostic names a volume the operator explicitly asked to keep, which is confusing at exactly the wrong moment.Acceptance criteria
prune's verification is scoped to the objects this invocation targeted; a concurrently created sandbox does not fail it.TEARDOWN_FAILURE_EXITand that it differs from the usage-error code.