fix(sandbox): destroy.ts held literal NUL bytes, so grep skipped the file#498
Merged
Conversation
…file The FailureLog fingerprints join their fields on a NUL separator. The source carried five LITERAL NUL bytes where escape sequences were intended - my own slip in #481, found incidentally when `grep -n "export function" destroy.ts` answered "Binary file destroy.ts matches". The runtime string is identical either way, which is why nothing caught it: the built bundle contains zero NUL bytes because esbuild re-encodes them, and the teardown tests pass on both forms. Proven rather than asserted - rebuilding sandbox.js after this change produces a BYTE-IDENTICAL bundle. What was actually broken is searchability, and on this file that matters. grep and ripgrep classify a file containing a NUL byte as binary and report only "binary file matches" instead of the lines, so every content search over the ca-sandbox teardown driver silently skipped it - including any security sweep for a pattern that happens to live there. Four exported functions were invisible to `grep`. A file that cannot be searched cannot be reviewed by search. Claude-Session: https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L
SUaDtL
added a commit
that referenced
this pull request
Jul 26, 2026
Both are now async, and every dockerRun call inside destroy.ts is awaited. Also carries the same NUL-byte source fix as #498, so the file is greppable on this branch too - it will merge cleanly with that PR since the change is identical. Remaining: registry.ts, cli.ts, claude-inside-cli.ts, and the test doubles. The error count RISES as each layer converts (220 now), because making a function async lights up every caller; it falls to zero only when the boundary reaches the CLI entry, which is already async and already awaited. Refs #479
SUaDtL
added a commit
that referenced
this pull request
Jul 26, 2026
destroy.ts conflicted as BINARY rather than textually: the merge base still carried the literal NUL bytes #498 removed, so git refused a line-level merge and left this branch's copy in place. That copy is the correct one - it holds both the NUL fix (applied here too, identically) and the async conversion. Verified rather than assumed: 0 NUL bytes, escapes present, destroySandbox and prune async, and the teardown/lifecycle/deadline suites green at 53/53.
SUaDtL
added a commit
that referenced
this pull request
Jul 26, 2026
#499) * wip(#479): async, cancellable docker chokepoint docker.ts only; the consumers do not compile yet. Pushed as recoverable WIP. spawnSync is uninterruptible by construction, so #394 could set a deadline but nothing could interrupt a call in flight - the thread sits inside the syscall with no point at which an AbortSignal could be observed. Now async spawn with an AbortSignal per CALL (not per runner: one runner serves several calls, and cancellation has to reach whichever is in flight). Three kills all arrive as close(null, "SIGKILL"), so the runner records WHICH killer fired before killing rather than inferring after. Abort is typed apart from timeout because they mean different things: a timeout is a wedged operation, an abort is the operator asking to stop, and logging one as the other would put "timed out" in an audit trail for a deliberate Ctrl-C. maxBuffer moved out of the spawn options bag into the runner, because async spawn has no such option - a caller still passing it inside `extra` would have silently lost its bound. That also fixes a latent mis-classification: spawnSync killed the child on a maxBuffer breach, which surfaced as status===null with a signal, the same shape the deadline check read, so an output overflow reported itself as a TIMEOUT with exit 124. An already-aborted signal now refuses to spawn at all, so a cancelled multi-step command cannot still create the next container. Refs #479 Claude-Session: https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L * wip(#479): thread the signal through cp, run, claude-inside, exec exec.ts carries AC-2: an ABORTED exec now runs the same bounded teardown a timed-out one does. The container is left running whether the deadline fired or the operator pressed Ctrl-C, and an orphaned box is the same leak either way - what differs is only what the report calls it, which is why docker.ts types the two apart rather than collapsing both into timedOut. The teardown stop deliberately carries NO signal. It is the cleanup for a call that was just cancelled, so handing it the same aborted signal would cancel the cleanup too and guarantee the orphan it exists to prevent. Also fixes a bound that the async conversion would otherwise have dropped silently: exec.ts passed maxBuffer inside the SPAWN options bag, which async spawn ignores. It is now the runner's own option, so the 256MB backstop still exists and capBytes stays the authoritative limit. Consumers still mid-conversion; the tree does not typecheck yet. Refs #479 Claude-Session: https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L * wip(#479): create.ts and destroy.ts teardown paths Both teardown paths deliberately carry NO signal. They run BECAUSE the operation failed or was cancelled, so handing them the aborted signal would cancel the cleanup and orphan exactly the containers and volumes they exist to reclaim. Still converging: destroySandbox and pruneSandboxes need the async boundary next, then registry.ts, cli.ts, and the test doubles. Refs #479 * wip(#479): destroySandbox and prune cross the async boundary Both are now async, and every dockerRun call inside destroy.ts is awaited. Also carries the same NUL-byte source fix as #498, so the file is greppable on this branch too - it will merge cleanly with that PR since the change is identical. Remaining: registry.ts, cli.ts, claude-inside-cli.ts, and the test doubles. The error count RISES as each layer converts (220 now), because making a function async lights up every caller; it falls to zero only when the boundary reaches the CLI entry, which is already async and already awaited. Refs #479 * wip(#479): registry.ts async, and the await sweep across consumers registry.ts was the last large module: 10 lookups, all of which either call dockerRun or delegate to one that does, now async. labelValue too - the per-object inspect behind listSandboxes. Production type errors: 50 -> 20. Remaining are cli.ts's dispatch, claude-inside, claude-inside-cli, and two stragglers in run/destroy; then the test doubles. Refs #479 * wip(#479): async keywords on runContainer, verifyScope, defaultShell Refs #479 * wip(#479): production code fully converted - 0 type errors The async boundary reached the CLI entry. Handlers is promise-returning across the board, runCli awaits each dispatch arm, and claude-inside-cli's entry now awaits its own promise rather than calling process.exit ON it - exiting on a pending promise would have reported 0 while the container start was still in flight. Production: 0 type errors. Test doubles remain (218): they return plain objects where DockerRun now returns a promise, and deadline.test.ts injects a spawnSync shape where an async child is expected. Refs #479 * wip(#479): convert the test doubles to async The injected fakes return a promise now, because DockerRun does. Two shapes needed converting: those annotated as DockerRun, and the inline ones passed straight as an argument, which carry no annotation to key off and were matched on their RunResult SHAPE instead. Every it/test callback became async. Harmless where nothing is awaited, and vitest treats the two identically - safer than deciding per-callback which bodies gained an await and getting one wrong silently. Test type errors 218 -> 44. Remaining: deadline.test.ts, whose spawn seam still injects a spawnSync-shaped return where an async child (an EventEmitter with stdout/stderr streams and a kill that emits close) is now expected. Refs #479 * wip(#479): async child double for deadline.test.ts The spawn seam injected a spawnSync-shaped RETURN; the runner now consumes a child process. The double is an EventEmitter with stdout/stderr streams and a kill that emits close(null, signal). That last detail is deliberate and load-bearing. A real SIGKILL surfaces as close(null, signal), IDENTICALLY whether the deadline fired, the caller aborted, or the output cap tripped - which is exactly why the runner records which killer fired rather than inferring it. A double that helpfully labelled its close event would let a broken runner pass, so this one reproduces the ambiguity faithfully. The deadline assertion changed shape with it: it used to read the SPAWN OPTION, which async spawn does not have. It is now asserted through observable behaviour - a child that never settles must still resolve, as a typed timeout, at the bound. Test type errors 218 -> 36. Refs #479 * wip(#479): rejection assertions and handler awaits in tests Two patterns the blanket await pass got wrong or left behind. `expect(() => await X).toThrow()` is invalid - an await inside a sync arrow - and the async equivalent asserts on the REJECTION, not on a synchronous throw. Handler calls through the defaultHandlers object are promises now. Test type errors 218 -> 29; production still 0. Refs #479 * wip(#479): the async conversion typechecks - 0 errors, production and tests Last shapes converted: vi.fn() handler mocks (they return promises now), the remaining sync docker fakes, and two await-precedence bugs the blanket pass introduced - `await findSandbox(id)!.containers` awaits the MEMBER, not the call, so it needed `(await findSandbox(id))!.containers`. That one would have typechecked as `any` in a looser config and failed at runtime. Two throw assertions became rejection assertions, which is a real semantic change rather than a syntactic one: `expect(() => await X).toThrow()` cannot work, and the async equivalent asserts the promise REJECTS. For the usage-error case the intent was "returns a code rather than throwing", so it now asserts the promise RESOLVES to a number. Typecheck is clean; the suite has not been run yet. Refs #479 * fix(#479): two real defects the first full test run exposed 1. `await expect(await createSandbox(...)).rejects.toThrow()` awaits the promise BEFORE expect can wrap it, so the rejection escaped as a test error instead of being asserted on. Six tests failed this way - my own blanket await pass put the await inside expect(). The .rejects form needs the promise itself. 2. A synchronous spawn failure rejected instead of returning a result. spawnSync reported a failed launch through r.error and the old runner mapped it to {code: 1}; async spawn normally EMITS an error event, but an invalid argument, a bad cwd, or a blocked exec can still throw straight out of spawn(). A throw inside a Promise executor rejects, and nothing in this driver catches - so an unusable docker would have surfaced as an unhandled rejection rather than the {code: 1} every caller is written against. Found by reading the diff, not by a failing test, which is why it is worth naming: no fixture spawns a real missing binary. Refs #479 * feat(#479): the cancellation contract, tested Eight tests covering the acceptance criteria that had none: AC-1 - an in-flight call aborts rather than running to its deadline; the child is KILLED rather than the promise merely resolving around it (resolving while leaving the process alive would look like cancellation and leak a docker client); and an ALREADY-aborted signal refuses to spawn at all, so a cancelled multi-step command cannot still create the next container. AC-2 - an aborted exec stops the container it targeted, exactly as a timed-out one does, and an ordinary non-zero exec still does NOT (escalation destroys a working box; doing it on exit 1 would tear one down every time a command failed). AC-4 - abort and timeout are typed apart. Both arrive as close(null, "SIGKILL"), so this is only true because the runner records which killer fired. Also declares ExecResult.aborted, which the new tests caught: exec.ts SET the flag but never declared it, so no caller could have read it. The pre-spawn guard is proven non-vacuous by mutation: disabling it fails exactly the already-aborted test and nothing else. deadline.test.ts 16/16. Full suite was 291/291 before these were added. Refs #479 * feat(#479): Ctrl-C cancels the in-flight call and lets teardown finish The last piece of the cancellation contract. Node's default SIGINT handling ends the CLI immediately, which leaves the box the operator was exec-ing into still running with no one left to reclaim it - the same orphan a timeout left before #394. The entry now aborts instead, so the runner kills its child, returns a typed aborted result, and the command module runs the SAME bounded teardown a timeout gets. A SECOND Ctrl-C exits immediately. The first already asked nicely, and a CLI that refuses to exit is how one earns a kill -9. The signal reaches the command modules through a HANDLERS FACTORY rather than a new runCli parameter, so the Handlers contract is unchanged and every test that injects its own handlers is untouched. Threading it through runCli would have changed that contract for every caller to serve exactly one of them. Installed at the entry, not in the runner: the signal is a property of this process, while docker.ts stays a primitive that observes a signal handed to it. That is what keeps the cancellation path testable without a daemon. Also threads the signal through createSandbox's own docker calls. It was declared on CreateOptions and unused - a dormant seam, which is the thing this issue exists to avoid shipping. Refs #479 * build(#479): rebuild the shipped bundles against the async runner Refs #479 * docs(#479): changelog entry for the cancellable docker boundary No version bump: ca-sandbox 0.1.5 carries no tag yet, so the payload gate accepts the change on the current version and this joins the open section. Closes #479 Closes #394 --------- Co-authored-by: SUaDtL <SUaDtL@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Five literal NUL bytes in
plugins/ca-sandbox/tools/destroy.tsbecome escape sequences. Same string at runtime; the file stops being classified as binary.How it was found
Incidentally, and it is my own slip from #481. A plain
grep -n "export function" destroy.tsanswered:The
FailureLogfingerprints join their fields on a NUL separator. The source was supposed to carry the escape; it carried the actual byte.Why it matters, given the runtime is unaffected
Searchability, on a file where that is a review control. grep and ripgrep classify any file containing a NUL byte as binary and print
binary file matchesinstead of the matching lines. So every content search over the ca-sandbox teardown driver silently skipped it — including any security sweep for a pattern that happens to live there. Four exported functions were invisible togrep.A file that cannot be searched cannot be reviewed by search. That is the whole defect; there is no runtime bug here.
Why nothing caught it
tools/is outside the shipped payload, so no version gate looks at itVerification
tsc --noEmit— 0 errorsteardown.test.ts+lifecycle.test.ts— 37/37 passedsandbox.jsSHA-256abce329a7ab2f079…before and after, which proves this is a source-representation change with no runtime effect rather than asserting itgrep -c "export function"now returns 4 instead of a binary-file noticeVersions
No bump.
plugins/ca-sandbox/tools/**is outside the shipped payload except for the committed bundles, andsandbox.jsis unchanged.payload_scope.py --plugin plugins/ca-sandboxreportsunchanged.https://claude.ai/code/session_01Y8UPz5RZwgnda3NbAVfd6L