feat(#576): durable per-scope sandbox — P3 scope durability + RO-layer content hash + reaper - #779
Merged
Merged
Conversation
Introduces @omadia/sandbox (middleware/packages/harness-sandbox): the narrow Sandbox contract from issue #576 (qm competitive analysis) — provision/run/ read/write/list/teardown, optional capabilities (process-sessions/backup/ blob-staging) behind type guards rather than interface fields, and an AgentComputerProfile declaring persistence/egress/process-session posture. v1 backend is plain local Docker (DockerSandboxBackend), built on the same injectable-spawn pattern as src/plugins/builder/buildSandbox.ts's executeBuild seam (see dockerExec.ts's execDocker injection point) so the full backend logic is testable with zero real Docker. Wiring, not just declaration: - profile.egress === false becomes docker run --network none. Proven at two levels: a stub-level argv assertion (always runs) and, behind the opt-in SANDBOX_DOCKER_TEST=1 gate, a real container attempting an outbound wget and observing it fail — not just that the flag was passed. - read/write/list are traversal-hardened against a fixed sandbox root (pathGuard.ts's clampSandboxPathPosix), same discipline as the #772 broker and zipExtractor.ts's zip-slip guard: absolute paths, NUL bytes, and any ../ resolution outside the root are rejected before a single docker exec is issued (asserted directly — traversal tests check the stub recorded zero calls). - Container naming is a deterministic function of the scope key (sha256(scopeKey)[0:24]), so provision() re-attaches to an already-running container across backend-instance restarts without needing a DB-backed registry yet — that scope-durability bookkeeping (last-used timestamps for a reaper, RO-layer content-hash tracking, multi-backend routing) is P3's job, not a blocker for this backend working correctly today. No orchestrator touch (by design — P1 scope per the phase cut). No new runtime dependencies: the backend shells out to the docker CLI via node:child_process, mirroring dev-runner-shim's dockerd.ts and buildSandbox.ts's Node-builtins-only constraint. Tests: middleware/test/sandbox/{pathGuard,agentComputerProfile, dockerSandbox}.test.ts. 30 stub-tier tests (always run, no Docker required) + 2 real-Docker tests gated on SANDBOX_DOCKER_TEST=1 (both verified green locally against an actual daemon, including the egress block). Root npm test / typecheck / build / lint all green with this package included in the workspace chain. Mutation-checked: inverting the egress condition in dockerSandbox.ts and disabling the escape check in pathGuard.ts (with a full package rebuild between runs) both broke the corresponding tests; reverted and confirmed green again before committing.
…icy gate Adds the `execute` native tool (packages/harness-orchestrator/src/tools/ executeTool.ts), off by default behind operator config `sandbox_execute_enabled` (honest-inert, same convention as the #575 audience floor and #580 command policy — sharper here because this tool's entire job is running arbitrary commands). ## Security boundary — belt and braces, not a new mechanism orchestrator.ts's dispatchTool already runs every tool call through guardToolCommands (#580) at the ONE existing choke point, keyed on a top-level `command` field — execute's input schema uses exactly that key, so it is automatically gated by the EXISTING seam whenever a deployment installs a commandPolicy provider on the turn context. No deployment does that today (the seam is honest-inert until an operator config UI ships). Relying solely on that opt-in seam would mean execute ships fully open in every deployment that hasn't separately configured a policy — the #748 lesson (fail-open + no evidence = an invisible outage) applied to a much sharper edge than a security screener. So the handler ALSO runs its own command-policy check before ever touching a sandbox: resolveCommandPolicy defaults to defaultCommandPolicy() (the DEFAULT_ORG_FLOOR: recursive rm, force-push, destructive SQL, fork bombs, pipe-to-shell), and a throwing resolver is FAIL-CLOSED — refused, never run. require_approval is surfaced as an explicit refusal naming why, never silently escalated. No parallel policy mechanism: both checks call the same decideCommand/defaultCommandPolicy pure primitives from @omadia/channel-sdk. ## Count, don't just log (#749/#750 pattern) New commandPolicyMetrics.ts (mirrors securityScreenMetrics.ts): in-memory counters for allowed/denied/require_approval/truncated/resolve_failed, plus a per-rule-id tally. Wired into BOTH guardToolCommands (every existing #580 branch now records) and executeTool's own belt-and-braces check, so a broken policy resolver shows up as a resolveFailed streak instead of silence. ## Sandbox wiring execute resolves the calling turn's scope via ScopeId (parseSessionScope/formatSessionScope from @omadia/channel-sdk), falling back to a turn-unique key for an unscoped turn rather than a shared literal (the #445 lesson). Provisions/reuses that scope's sandbox via the injected SandboxBackend (DockerSandboxBackend in plugin.ts's wiring) and runs the command there. Registered in harness-orchestrator's plugin.ts — the SAME composition seam the existing ProcessMemory tools use (OB-76) — not src/index.ts or agentBuilder.ts, neither of which this PR touches. No writeCapabilities annotation: that contract is {dataClass, operation} for structured-write idempotency dedupe; execute's effects are arbitrary and untyped, and re-running the "same" shell command is not safely deduplicable the way replaying a structured write is. Deliberately absent, documented at the call site. ## Tests test/sandbox/executeTool.test.ts (17 tests) — every deny/require_approval/ truncated/resolve_failed path proven to never call SandboxBackend.provision (the security-boundary property), and the permitted path proven to reach provision with the correct scope key + run options. test/ commandPolicyMetrics.test.ts (9 tests) — counter module plus guardToolCommands wiring. All new + existing #580 tests green. ## Mutation-check evidence (dist rebuild between runs, per the phase-4b ## prompt's requirement now that @omadia/orchestrator imports @omadia/sandbox) Short-circuited the deny-decision branch in executeTool.ts (`if (false && decision.decision === 'deny')`), rebuilt @omadia/sandbox and @omadia/orchestrator, reran the suite: both the rm -rf and force-push denial tests failed as expected — the stub sandbox returned exit 0 for a command that should never have reached it. Reverted, rebuilt again, confirmed green. Full workspace build/typecheck/lint/test green after merging latest origin/main (7115 tests, 0 fail, 12 pre-existing skips).
…r content hash + reaper Migration 0044_sandbox_registry.sql (chosen with margin: 0040-0043 were occupied by concurrent credential-work PRs at authoring time, including an observed numbering collision at 0040 across two of them that has since been resolved upstream — noted in the migration's own header for the record, not something this PR touches). ## SandboxRegistry — the durable bookkeeping Docker doesn't give for free New SandboxRegistry interface (get/upsert/touch/delete/listAll) in @omadia/sandbox: InMemorySandboxRegistry (tests, and any deployment that hasn't wired a durable store) and PostgresSandboxRegistry (migration 0044's sandbox_registry table). DockerSandboxBackend gained an OPTIONAL registry constructor option — omitted (the default), the backend behaves byte-identical to what P1/P2 shipped (deterministic container naming, zero registry calls); provided, provision() records (scope, container name, profile, lastUsedAt) and re-attaches via the registry's STORED sandboxRef rather than recomputing the deterministic name, which is the seam a future non-deterministic backend needs. Regression-tested directly (dockerSandboxRegistry.test.ts's first suite asserts the no-registry path is unchanged). ## Reaper — orphaned (idle, non-persistent) sandbox cleanup reapOrphanedSandboxes takes "now" as a REQUIRED external parameter, never derived from the registry entries themselves — the #709/#710 clock-race lesson (an idle check must anchor to a clock independent of the row being checked). profile.persistent === true entries are never reaped regardless of idle time. A teardown failure leaves the registry row intact (for retry) and is reported in failedScopeKeys rather than silently dropped, and does not abort the sweep for the rest. Not wired to a scheduler in this PR — the function is a complete, directly callable, fully tested capability (same shape as DockerSandbox.teardown() itself, which also isn't auto-invoked by anything); hooking it to a cron/ routine is a scheduling-system integration, out of #576's substrate scope. ## RO-layer content-hash materialization computeContentHash (deterministic, order-independent, NUL-separated path/content encoding so shifted-boundary inputs can't collide) + syncReadOnlyLayer, which calls Sandbox.write only when the computed hash differs from the caller-supplied previousHash. #576 is deliberately the mechanism here, not the consumer: what actually constitutes a scope's RO layer (org files, skills) is the issue's OWN framing of what "builds on" this sandbox, not something #576 decides — inventing a content source would be scope creep into a separate concept. The primitive is fully exercised against a stub Sandbox (contentHash.test.ts proves the skip-when-unchanged behavior directly against the write call count). ## Tests - contentHash.test.ts (8), reaper.test.ts (6), dockerSandboxRegistry.test.ts (4) — all stub/pure, no Docker or Postgres needed, always run in npm test. - postgresSandboxRegistry.pg.test.ts (5) — gated on GRAPH_PG_TEST_URL/ MEMORY_PG_TEST_URL/DATABASE_URL, same probePgTest convention as postgresCredentialStore.pg.test.ts. Verified locally against a real postgres:16-alpine container (docker run, migration 0044 applied verbatim via psql, all 5 tests green) — not just trusted to work under CI's pg service. ## Mutation-check evidence (dist rebuild between runs) Removed the persistent-skip guard ("if (entry.profile.persistent) continue;") in reaper.ts, rebuilt @omadia/sandbox: the 'never reaps a persistent sandbox' test failed as expected (a persistent scope was reaped). Reverted, rebuilt again, confirmed green. (P1's egress/traversal mutations and P2's deny-bypass mutation were separately re-verified in their own PRs; this round targets the one new security-relevant branch P3 adds.) Full workspace build/typecheck/lint/test green after merging latest origin/main (7223 tests, 0 fail, 12 pre-existing skips).
…e-decoupling ratchet The comment referencing 'dev-runner-shim' by name matched the ratchet's 'dev-runner' literal pattern (scripts/check-core-decoupling.mjs), which counts references to the Dev Platform being extracted in epic #470 — an unrelated coincidence (dev-runner-shim is a real, unrelated package; the ratchet's pattern list is deliberately broad-literal). Reworded to drop the package name while keeping the same intent (Node-builtins-only constraint on this spawn seam). Confirmed the ratchet passes locally after this change: 'Dev Platform references held at 3296' (baseline unchanged, not lowered — this was never a real Dev Platform reference, just a name collision).
…orchestrator -> @omadia/sandbox npm install after the P1 merge regenerated this — the peerDependencies entry added to harness-orchestrator/package.json in the original P2 commit had not been reflected into package-lock.json's own copy of that block.
…-p3-scope-durability
Add/add conflicts in the three harness-sandbox files P3 evolved: main carries P1's squash (#776), this branch carries P1 plus P3's deliberate changes. Verified via git log that only the #776 squash ever touched these paths on main, so the branch side is the correct resolution. The lockfile conflict is resolved by taking main's and reconciling via npm install.
This was referenced Aug 20, 2026
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
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.
Summary
Issue #576, Phase 3 of 3 (interface+Docker = #776, execute tool+policy
gate = #777). Stacks on #777 — base branch is
feat/576-sandbox-p2-execute-tool, notmain. Once #776/#777 merge thisPR's diff against
mainwill shrink to just the P3 commit.Adds durability bookkeeping, an idle-sandbox reaper, and RO-layer
content-hash materialization to
@omadia/sandbox.Migration
0044_sandbox_registry.sqlOne table,
sandbox_registry(scope_keyPK,backend,sandbox_ref,profileJSONB,ro_layer_hash,created_at,last_used_at+ an indexon
last_used_atfor the reaper's sweep). Chosen with margin: atauthoring time
0040-0043were occupied by concurrent credential-workPRs, including an observed three-way collision at
0040across two ofthem — that has since resolved itself upstream (one of the
0040fileswas deleted and replaced by
0043_credential_asks.sqlin a later merge oforigin/maininto this branch). Noted in the migration's own header;nothing here touches the credentials/ namespace.
Verified by applying the migration file verbatim via
psqlagainst areal
postgres:16-alpinecontainer (not just eyeballing the SQL) — schemamatches what
postgresSandboxRegistry.pg.test.tsexpects.SandboxRegistry — the durable bookkeeping Docker doesn't give for free
SandboxRegistryinterface (get/upsert/touch/delete/listAll):InMemorySandboxRegistry— for tests and any deployment that hasn'twired a durable store.
PostgresSandboxRegistry— migration0044's table.DockerSandboxBackendgained an optionalregistryconstructoroption:
deterministic container naming, zero registry calls. Regression-tested
directly —
dockerSandboxRegistry.test.ts's first suite asserts theno-registry path is unchanged.
provision()records(scopeKey, containerName, profile, lastUsedAt)and re-attaches via the registry's storedsandboxRefrather than recomputing the deterministic name — the seam a future
non-deterministic backend (Fly/Firecracker/a Epic: Satellites — outbound-only edge nodes (Raspberry Pi class) that pair with a device code, run selected agents, and bridge internal-only systems securely into the main omadia #746 Satellite, where the
platform assigns the id) needs, exercised now even though this backend's
own name is always independently recomputable. A process-local cache hit
also touches the registry, so the reaper never sees an actively-reused
scope as idle just because Docker itself wasn't consulted that call.
Reaper — orphaned (idle, non-persistent) sandbox cleanup
reapOrphanedSandboxestakesnowas a required, externally suppliedparameter — never
new Date()computed inside the function, never derivedfrom the registry entries. This is the
#709/#710clock-race lessonapplied here: an idle-timeout check must anchor to a clock independent of
the row being checked, or a self-referential comparison becomes racy.
reaper.test.tshas a dedicated regression test for exactly this(
the anchor is the CALLER-supplied now, not anything derived from the entries).profile.persistent === trueentries are never reaped, regardlessof idle time.
sweep) and is reported in
failedScopeKeysrather than silentlydropped, and does not abort the sweep for the remaining entries.
Not wired to a scheduler in this PR. The function is a complete,
directly callable, fully tested capability — the same shape as
DockerSandbox.teardown()itself, which also isn't auto-invoked byanything in this codebase. Hooking it to a cron/routine is a
scheduling-system integration (Pulse/routines already exist for that);
#576's scope is the sandbox substrate, not a new scheduler.
RO-layer content-hash materialization
computeContentHash(deterministic, order-independent — sorted keys,NUL-separated path/content encoding so a shifted-boundary input like
{ab:'c'}vs{a:'bc'}can't collide, asserted directly) +syncReadOnlyLayer, which callsSandbox.writeonly when thecomputed hash differs from a caller-supplied
previousHash.#576 is deliberately the mechanism here, not the consumer: the issue's
own text frames "skills materialization" as a concept that builds on
this sandbox, not part of it — inventing an org-files/skills content
source in this PR would be scope creep into a separate, undecided concept.
contentHash.test.tsproves the skip-when-unchanged behavior directlyagainst a stub
Sandbox's write-call count, so the primitive itself isfully exercised even with no real consumer yet.
Tests
contentHash.test.ts(8),reaper.test.ts(6),dockerSandboxRegistry.test.ts(4) — stub/pure, no Docker or Postgresneeded, always run in
npm test.postgresSandboxRegistry.pg.test.ts(5) — gated onGRAPH_PG_TEST_URL/MEMORY_PG_TEST_URL/DATABASE_URL, sameprobePgTestconvention aspostgresCredentialStore.pg.test.ts.Verified locally against a real
postgres:16-alpinecontainer(
docker run, migration applied viapsql, all 5 green) — not justtrusted to pass under CI's pg service; not touching
ci.ymlsince theexisting
test:pgstep's recursive glob already picks uptest/sandbox/*.pg.test.ts.Mutation-check evidence (dist rebuild between runs)
Removed the persistent-skip guard
(
if (entry.profile.persistent) continue;) inreaper.ts, rebuilt@omadia/sandbox: the "never reaps a persistent sandbox" test failed asexpected (a persistent scope got reaped). Reverted, rebuilt again,
confirmed green. (P1's egress/traversal mutations and P2's deny-bypass
mutation were separately verified in their own PRs; this round targets
the one new security/data-loss-relevant branch P3 adds — the persistent
flag is the only thing standing between the reaper and tearing down a
sandbox a deployment explicitly asked to keep.)
Blast radius
contentHash.ts,sandboxRegistry.ts,postgresSandboxRegistry.ts,reaper.ts(all in@omadia/sandbox), migration0044, 4 new testfiles.
dockerSandbox.ts(additiveregistryoption — see theregression-tested no-op path above),
index.ts(barrel exports),package.json(addedpgpeerDependency).src/index.ts,agentBuilder.ts, credential, or skillnamespace changes.
path in this PR — both are complete, tested, standalone capabilities;
see the sections above for why that's the right cut rather than scope
creep.
Verification run locally (this branch, after merging origin/main —
currently at
6c9081fd, includes #578 P3 keychain-asks)npm run build(full workspace): greennpm run typecheck(full workspace incl. golden/adversarial): greennpm run lint: cleannpm run test: 7223 tests, 7211 pass, 0 fail, 12 skipped(pre-existing), 0 cancelled
postgresSandboxRegistry.pg.test.tsrun separately against a reallocal Postgres: 5/5 green (see above)
Base / stacking
Branched from
feat/576-sandbox-p2-execute-tool(#777). Do not mergebefore #776 and #777.
What's NOT in this PR (P4)
Admin view (web-ui) for the sandbox registry — explicitly optional per the
phase-4b plan ("nur wenn Zeit UND Konfidenz... Sonst sauber dokumentiert
weglassen"). Not attempted: web-ui work needs its own design pass (what
does an operator actually need to see/do — list live sandboxes, force-reap
one, inspect RO-layer sync state?) that I don't want to guess at without
that input, and it would be a fourth stacked PR on top of three already
awaiting review. Documenting it as deliberately deferred rather than
half-building a page nobody asked for the shape of yet.
Open questions for Marcel
(
idleThresholdMsis a parameter, there's no default and no scheduler).Worth a follow-up issue once there's a real operator surface to configure
it from, rather than me guessing a number now.
PostgresSandboxRegistry.upsert'sON CONFLICTclause usesCOALESCE(EXCLUDED.ro_layer_hash, sandbox_registry.ro_layer_hash)so are-provision that doesn't pass a hash doesn't clobber a previously
synced one — worth a second pair of eyes on that semantic, since it's
the one place this migration's upsert behavior isn't a straight
"last write wins."
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.