feat(isolation): container backend for folder projects (Phase B)#2153
Open
Wirasm wants to merge 4 commits into
Open
feat(isolation): container backend for folder projects (Phase B)#2153Wirasm wants to merge 4 commits into
Wirasm wants to merge 4 commits into
Conversation
Implement the Docker container isolation backend behind the Phase A kind-routed seam. Folder projects can opt into --container (or container.enabled config); the run executes inside a container over a read-only bind of the project root plus a writable overlayfs upper layer, mounted at the same absolute path as the host cwd. - docker-exec.ts: thin `docker` CLI wrapper + daemon/image preflight; new docker error patterns in the isolation classifier. - runner.Dockerfile + entrypoint.sh: debian-slim + claude native binary + git/bash/bun/uv; entrypoint mounts native overlay (fuse-overlayfs fallback) and signals a ready sentinel. Built via bun run build:runner-image. - ContainerBackend.prepare/destroy: per-run upper volume + labeled container, ready-poll, tracked isolation_environments row (branch_name sentinel). - Claude in-container spawn via the SDK spawnClaudeCodeProcess hook over docker exec -i; kill() crosses the boundary via in-container pkill. - Container env policy: the container receives only the Archon-managed env bag + a minimal base, never host process.env (the isolation boundary). - bash/script/loop until_bash nodes exec in-container via docker exec; host path stays byte-identical. - containerExec provider capability + pre-dispatch fail-fast: a container run whose AI nodes resolve to a non-Claude provider is rejected before any node runs (no silent host downgrade). - --container CLI flag, container.* config (repo>global), workflow container.enabled policy, repo-kind + --container hard error. - container_created/container_destroyed events across emitter, DB rows, CLI progress, and the console normalizer. - Fix: container metadata is a JSON string on SQLite (object on Postgres) — destroy() now parses both, else it skipped docker rm and leaked the container (caught by the live smoke). Phase C (pause/resume + approval-gated write-back) not included.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Docker-based container isolation for folder workflows, including runner image tooling, overlay-backed environments, CLI/configuration controls, container-aware workflow and provider execution, lifecycle events, cleanup, security documentation, and tests. ChangesContainer isolation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ContainerBackend
participant Docker
participant RunnerEntrypoint
participant DAGExecutor
participant ClaudeProvider
CLI->>ContainerBackend: prepare folder environment
ContainerBackend->>Docker: run preflight and runner container
Docker->>RunnerEntrypoint: start container
RunnerEntrypoint->>RunnerEntrypoint: mount overlay and create readiness sentinel
ContainerBackend->>Docker: poll readiness
ContainerBackend-->>CLI: return container execution context
CLI->>DAGExecutor: execute workflow with container context
DAGExecutor->>ClaudeProvider: dispatch container-compatible provider
ClaudeProvider->>Docker: execute Claude with docker exec
DAGExecutor-->>CLI: emit workflow lifecycle events
CLI->>ContainerBackend: destroy environment
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/providers/src/claude/provider.ts (1)
599-662: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEmpty
stderrLinesduring container execution breaks error classification.Bypassing the SDK by inheriting
stderrnatively inbuildContainerSpawnprevents the SDK from capturing Claude Code's stderr stream. Consequently, the inlinestderrcallback inprovider.tsis never executed, leavingstderrLinesempty. This breaksclassifyAndEnrichErrorduring container runs, leading to missing diagnostics for CLI errors like invalid API keys or rate-limits.Share the
stderrcallback with the spawn hook and switch back topipeto restore error classification context.
packages/providers/src/claude/provider.ts#L599-L662: Extract the inlinestderrcallback into a variable and pass it to bothbuildContainerSpawnand the returned SDK options.packages/providers/src/claude/container-spawn.ts#L110-L128: UpdatebuildContainerSpawnto accept anonStderrcallback, changestdioto['pipe', 'pipe', 'pipe'], and pipechild.stderr's data directly toonStderr.🛠️ Proposed fix to bridge `stderr` between the container and error classifier
In
packages/providers/src/claude/provider.ts:+ const stderrCallback = (data: string): void => { + const output = data.trim(); + if (!output) return; + stderrLines.push(output); + + const isError = + output.toLowerCase().includes('error') || + output.toLowerCase().includes('fatal') || + output.toLowerCase().includes('failed') || + output.toLowerCase().includes('exception') || + output.includes('at ') || + output.includes('Error:'); + + const isInfoMessage = + output.includes('Spawning Claude Code') || + output.includes('--output-format') || + output.includes('--permission-mode'); + + if (isError && !isInfoMessage) { + getLog().error({ stderr: output }, 'subprocess_error'); + } + }; + const containerExecContext = requestOptions?.execContext?.kind === 'container' ? requestOptions.execContext : undefined; const spawnOverride = containerExecContext - ? { spawnClaudeCodeProcess: buildContainerSpawn(containerExecContext) } + ? { spawnClaudeCodeProcess: buildContainerSpawn(containerExecContext, stderrCallback) } : {}; return { @@ -637,28 +660,7 @@ systemPrompt: requestOptions?.systemPrompt ?? { type: 'preset', preset: 'claude_code' }, settingSources: assistantDefaults.settingSources ?? ['project', 'user'], hooks: buildToolCaptureHooks(toolResultQueue), - stderr: (data: string): void => { - const output = data.trim(); - if (!output) return; - stderrLines.push(output); - - const isError = - output.toLowerCase().includes('error') || - output.toLowerCase().includes('fatal') || - output.toLowerCase().includes('failed') || - output.toLowerCase().includes('exception') || - output.includes('at ') || - output.includes('Error:'); - - const isInfoMessage = - output.includes('Spawning Claude Code') || - output.includes('--output-format') || - output.includes('--permission-mode'); - - if (isError && !isInfoMessage) { - getLog().error({ stderr: output }, 'subprocess_error'); - } - }, + stderr: stderrCallback, }; }In
packages/providers/src/claude/container-spawn.ts:export function buildContainerSpawn( execContext: Extract<ExecutionContext, { kind: 'container' }>, + onStderr: (data: string) => void, spawnFn: Spawner = spawn as unknown as Spawner ): (options: SpawnOptions) => SpawnedProcess { return (options: SpawnOptions): SpawnedProcess => { const dockerArgs = buildDockerExecArgs(execContext, options); getLog().debug( { containerId: execContext.containerId, argc: options.args.length }, 'claude.container_spawn_started' ); - // stderr inherited: the SpawnedProcess contract exposes only stdin/stdout, so - // the SDK can't read the child's stderr — inheriting surfaces container-side - // Claude errors in Archon's own stderr instead of swallowing them. The - // 'pipe','pipe','inherit' stdio makes stdin/stdout non-null. const child = spawnFn('docker', dockerArgs, { - stdio: ['pipe', 'pipe', 'inherit'], + stdio: ['pipe', 'pipe', 'pipe'], }); // The 'pipe','pipe' stdio guarantees stdin/stdout are present; guard rather // than assert so a misbehaving spawner fails loudly instead of NPE-ing later. if (!child.stdin || !child.stdout) { throw new Error('docker exec child is missing piped stdin/stdout'); } + + if (child.stderr) { + child.stderr.on('data', (chunk: Buffer | string) => { + onStderr(chunk.toString()); + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/providers/src/claude/provider.ts` around lines 599 - 662, Update packages/providers/src/claude/provider.ts lines 599-662 to extract the existing stderr handler into a reusable callback, pass it to buildContainerSpawn, and retain it in the returned SDK options. Update packages/providers/src/claude/container-spawn.ts lines 110-128 so buildContainerSpawn accepts onStderr, uses piped stdin/stdout/stderr, and forwards child.stderr data directly to onStderr, restoring stderrLines and error classification during container runs.
🧹 Nitpick comments (1)
packages/cli/src/commands/workflow.ts (1)
1147-1151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required structured log-event naming.
workflow.running_in_containerdoes not follow{domain}.{action}_{state}. Rename it to a concrete state event such asworkflow.container_backend_selected.As per coding guidelines, use structured Pino event names in
{domain}.{action}_{state}format.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/commands/workflow.ts` around lines 1147 - 1151, Update the structured log event in the container-running branch near the container image message to use the required {domain}.{action}_{state} naming format, renaming workflow.running_in_container to workflow.container_backend_selected while preserving its existing fields and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/workflow.ts`:
- Around line 1486-1496: The Phase B workflow must not retain paused containers
or expose unsupported Phase C lifecycle states. In
packages/cli/src/commands/workflow.ts:1486-1496, update the container teardown
flow to destroy paused containers rather than preserving them for resume. In
packages/workflows/src/store.ts:53-59, remove the container_stopped and
container_resumed states until Phase C implements suspend/resume behavior.
- Around line 1152-1161: Wrap the post-prepare workflow lifecycle in a
try/finally block so every failure after backend.prepare()—including
persistence, resume hydration, or executeWorkflow()—still invokes the container
backend’s destroy teardown. Apply the same acquisition-scoped cleanup to the
corresponding lifecycle at the additional location, preserving the existing
destroy behavior and execution flow.
- Around line 1134-1138: Update the folderConfig loading in the wantsContainer
selection flow to stop swallowing loadConfig errors: remove the fallback to
undefined and let loadConfig(codebase.default_cwd) propagate its failure or fail
explicitly. Preserve the existing container precedence and default behavior only
when configuration loads successfully.
In `@packages/isolation/docker/runner.Dockerfile`:
- Around line 33-46: Update the Claude, Bun, and uv installation RUN blocks to
use explicitly pinned installer or tool versions instead of mutable URLs, and
verify each downloaded installer or artifact against a trusted checksum or
signature before executing it as root. Preserve the existing executable presence
checks and fail the build when verification or installation fails.
In `@packages/isolation/src/backend-router.test.ts`:
- Around line 59-64: Update the test covering container backend rejection in
resolveFolderBackend to include a separate assertion where containerConfig is
provided but store is omitted, and verify it still throws the expected “not
wired up” error. Keep the existing assertion for both options being absent.
In `@packages/isolation/src/backends/container.ts`:
- Around line 274-278: Reduce workload container privileges in
packages/isolation/src/backends/container.ts by removing SYS_ADMIN and the
unconfined AppArmor security option while preserving conditional /dev/fuse
handling. In packages/isolation/docker/runner.Dockerfile, switch to a non-root
workload user after privileged overlay initialization, ensuring initialization
still runs with the required privileges.
- Around line 221-240: Update the teardown flow around the container and volume
removal catches so genuine removal failures do not proceed to persist destroyed
status; retain a recoverable state or propagate the failure, while preserving
successful cleanup behavior. In
packages/isolation/src/backends/container.test.ts lines 217-227, update the
assertion to expect the recoverable/error outcome instead of destroyed.
- Around line 133-180: The environment persistence step around store.create must
clean up resources on failure. Wrap the store.create call in a try/catch, call
forceRemove(containerName, volume), log the raw error with
isolation.container_prepare_failed, and rethrow the original error; preserve the
existing metadata and persistence behavior on success.
In `@packages/isolation/src/errors.ts`:
- Around line 45-50: Update the “no such image” error message in the preflight
error classification to avoid recommending the fixed archon-runner image tag.
Preserve the requested image reference when reporting the error, or use
tag-neutral guidance that also applies to versioned and custom container images.
In `@packages/providers/src/claude/container-spawn.ts`:
- Around line 153-157: Update the kill method in the child-process wrapper to
default its signal parameter to SIGTERM, matching ChildProcess.kill() behavior
and ensuring killInContainer receives a valid signal for parameterless calls.
In `@packages/workflows/src/store.ts`:
- Around line 53-59: Remove container_stopped and container_resumed from the
public lifecycle event list in packages/workflows/src/store.ts, leaving only the
Phase B-supported container_created and container_destroyed events so
unsupported suspend/resume inputs are not exposed.
---
Outside diff comments:
In `@packages/providers/src/claude/provider.ts`:
- Around line 599-662: Update packages/providers/src/claude/provider.ts lines
599-662 to extract the existing stderr handler into a reusable callback, pass it
to buildContainerSpawn, and retain it in the returned SDK options. Update
packages/providers/src/claude/container-spawn.ts lines 110-128 so
buildContainerSpawn accepts onStderr, uses piped stdin/stdout/stderr, and
forwards child.stderr data directly to onStderr, restoring stderrLines and error
classification during container runs.
---
Nitpick comments:
In `@packages/cli/src/commands/workflow.ts`:
- Around line 1147-1151: Update the structured log event in the
container-running branch near the container image message to use the required
{domain}.{action}_{state} naming format, renaming workflow.running_in_container
to workflow.container_backend_selected while preserving its existing fields and
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b06000b7-617c-4ea9-b22e-7e3c065a51b5
📒 Files selected for processing (38)
package.jsonpackages/adapters/src/chat/slack/workflow-bridge.tspackages/cli/src/cli.tspackages/cli/src/commands/workflow.tspackages/core/src/config/config-loader.tspackages/core/src/config/config-types.tspackages/isolation/docker/entrypoint.shpackages/isolation/docker/runner.Dockerfilepackages/isolation/package.jsonpackages/isolation/src/backend-router.test.tspackages/isolation/src/backend-router.tspackages/isolation/src/backends/container.test.tspackages/isolation/src/backends/container.tspackages/isolation/src/container/docker-exec.test.tspackages/isolation/src/container/docker-exec.tspackages/isolation/src/errors.tspackages/isolation/src/index.tspackages/isolation/src/types.tspackages/providers/package.jsonpackages/providers/src/claude/capabilities.tspackages/providers/src/claude/container-spawn.test.tspackages/providers/src/claude/container-spawn.tspackages/providers/src/claude/provider.tspackages/providers/src/codex/capabilities.tspackages/providers/src/codex/provider.test.tspackages/providers/src/community/copilot/capabilities.tspackages/providers/src/community/opencode/capabilities.tspackages/providers/src/community/pi/capabilities.tspackages/providers/src/registry.test.tspackages/providers/src/types.tspackages/server/src/adapters/web/workflow-bridge.tspackages/web/src/experiments/console/primitives/event.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.tspackages/workflows/src/event-emitter.tspackages/workflows/src/schemas/workflow.tspackages/workflows/src/store.tsscripts/build-runner-image.sh
Comment on lines
+33
to
+46
| # Claude Code native binary (self-contained). Installs to /root/.local/bin/claude. | ||
| RUN curl -fsSL https://claude.ai/install.sh | bash \ | ||
| && test -x /root/.local/bin/claude \ | ||
| || (echo "FATAL: claude binary not found after install" >&2 && exit 1) | ||
|
|
||
| # bun (script: nodes with runtime: bun) → /root/.bun/bin/bun | ||
| RUN curl -fsSL https://bun.sh/install | bash \ | ||
| && test -x /root/.bun/bin/bun \ | ||
| || (echo "FATAL: bun not found after install" >&2 && exit 1) | ||
|
|
||
| # uv (script: nodes with runtime: uv) → /root/.local/bin/uv | ||
| RUN curl -LsSf https://astral.sh/uv/install.sh | sh \ | ||
| && test -x /root/.local/bin/uv \ | ||
| || (echo "FATAL: uv not found after install" >&2 && exit 1) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Pin and verify all downloaded installers.
These mutable URLs execute arbitrary network content as root during builds. Pin tool versions and verify downloaded artifacts/checksums so the runner image is reproducible and a compromised installer endpoint cannot compromise the image.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/isolation/docker/runner.Dockerfile` around lines 33 - 46, Update the
Claude, Bun, and uv installation RUN blocks to use explicitly pinned installer
or tool versions instead of mutable URLs, and verify each downloaded installer
or artifact against a trusted checksum or signature before executing it as root.
Preserve the existing executable presence checks and fail the build when
verification or installation fails.
Address the prp-review findings on the Phase B container backend. - Mount strategy: try fuse-overlayfs (no CAP_SYS_ADMIN, closes the mount -o remount,rw /mnt/lower escape) FIRST, fall back to native overlay + CAP_SYS_ADMIN only when fuse can't mount. Mode dictated via ARCHON_OVERLAY_MODE; fuse fails fast (entrypoint exits, waitForReady detects the exit). Add SECURITY.md with the honest threat model: native mode grants the remount escape and is NOT a sandbox against a hostile agent (only rootless/userns daemons get the fuse path); also documents the docker-exec-secrets-in-host-ps and $ARTIFACTS_DIR caveats. - CLI teardown moved into finally so a throw from executeWorkflow before its own try/catch can't orphan a privileged container+volume. - destroy() distinguishes idempotent not-found (mark destroyed) from a genuine docker failure (leave row active, THROW); CLI surfaces a loud clean-up-manually message. Replace the swallow-everything test. - Runtime containerExec backstop in resolveNodeProviderAndModel so the hand-mirrored pre-scan isn't a single point of failure. - PID-scope the in-container kill (per-invocation pidfile via sh -c 'echo $$ > f; exec claude "$@"') so an abort doesn't kill sibling Claude nodes sharing the container. - Validate container.network (reject anything but bridge/none). - Env-isolation enforcement-point tests (process.env canary) for both the Claude provider env and the bash/script docker exec argv. - Wire classifyIsolationError on the CLI container-prepare path; add cleanup breadcrumbs.
Collaborator
Author
PR Review SummaryMulti-agent review of PR #2153 — feat(isolation): container backend for folder projects (Phase B) ( Critical Issues (5 found)
Important Issues (7 found)
Suggestions (11 found)
Strengths
Documentation Issues
VerdictCRITICAL ISSUES Recommended Actions
Multi-agent review (7 agents) at PR head |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/providers/src/claude/provider.ts (1)
1201-1210: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not resolve the Claude executable on the host for container runs.
resolveClaudeBinaryPath()is unconditional, so a container run fails when Claude is installed only in the runner image—even though the container spawn path does not use the resolved host executable.Proposed fix
-const resolvedCliPath = await resolveClaudeBinaryPath(assistantDefaults.claudeBinaryPath); +const resolvedCliPath = + requestOptions?.execContext?.kind === 'container' + ? undefined + : await resolveClaudeBinaryPath(assistantDefaults.claudeBinaryPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/providers/src/claude/provider.ts` around lines 1201 - 1210, Update the setup before the retry loop so resolveClaudeBinaryPath is called only for host execution, not container runs. Preserve the container spawn path without requiring a host-installed Claude executable, while retaining the existing resolved path behavior for non-container requests and continuing to build the subprocess environment as before.packages/providers/src/claude/container-spawn.ts (1)
164-170: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the abort race before the PID file exists.
For an already-aborted or immediately aborted signal, the killer can read the PID file before the shell writes it. The script then returns success without killing anything, allowing Claude to continue. Retry PID-file lookup for a short bounded interval and terminate the local
docker execchild as well; add a pre-aborted-signal regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/providers/src/claude/container-spawn.ts` around lines 164 - 170, Update the abort handling around onAbort in container-spawn.ts so cancellation waits briefly and retries PID-file lookup until the file exists or a short bounded timeout expires, then kills the in-container process. Also terminate the local docker exec child when abort occurs, including for already-aborted and immediately aborted signals, and add a regression test covering a pre-aborted signal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/workflow.ts`:
- Around line 1523-1531: Update the SIGINT/SIGTERM handling around the workflow
execution and its container teardown block so termination requests cancel the
active workflow, await the resulting cleanup in the existing finally path, and
only then exit with status 1. Ensure both signals use this
cancellation-and-teardown flow instead of calling process.exit immediately,
while preserving normal paused-run behavior.
- Around line 178-182: Update the pidsLimit validation in the workflow
configuration flow to reject finite non-integer values as well as undefined,
non-finite, and non-positive values. Preserve the existing error message and
handling for valid positive integers.
In `@packages/isolation/docker/SECURITY.md`:
- Around line 8-12: Update the native overlay wording in SECURITY.md to describe
native as the fallback mode rather than the default. In the one-line summary,
replace the “default native overlay mode” phrasing with “native fallback mode”
while preserving the existing limitation that it is not a sandbox against
malicious or prompt-injected agents.
In `@packages/isolation/src/backends/container.ts`:
- Around line 434-442: Update isRunning and its waitForReady caller so Docker
inspect failures remain an unknown/error state rather than returning false; only
an explicit stdout value of “false” should trigger the native-mode privilege
fallback. Preserve polling or propagate a clear infrastructure error for
timeouts and other inspect failures, without silently broadening permissions.
In `@packages/providers/src/claude/provider.ts`:
- Around line 147-150: Update the debug log in the environment-key mirroring
block to use a structured Pino event name matching the required
{domain}.{action}_{state} format, or remove the log if no appropriate event name
exists. Preserve the existing key assignment behavior.
---
Outside diff comments:
In `@packages/providers/src/claude/container-spawn.ts`:
- Around line 164-170: Update the abort handling around onAbort in
container-spawn.ts so cancellation waits briefly and retries PID-file lookup
until the file exists or a short bounded timeout expires, then kills the
in-container process. Also terminate the local docker exec child when abort
occurs, including for already-aborted and immediately aborted signals, and add a
regression test covering a pre-aborted signal.
In `@packages/providers/src/claude/provider.ts`:
- Around line 1201-1210: Update the setup before the retry loop so
resolveClaudeBinaryPath is called only for host execution, not container runs.
Preserve the container spawn path without requiring a host-installed Claude
executable, while retaining the existing resolved path behavior for
non-container requests and continuing to build the subprocess environment as
before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12af00e6-c8ad-4b89-b9de-7448adb54d25
📒 Files selected for processing (13)
packages/cli/src/commands/workflow.tspackages/isolation/docker/SECURITY.mdpackages/isolation/docker/entrypoint.shpackages/isolation/docker/runner.Dockerfilepackages/isolation/src/backends/container.test.tspackages/isolation/src/backends/container.tspackages/providers/package.jsonpackages/providers/src/claude/container-env.test.tspackages/providers/src/claude/container-spawn.test.tspackages/providers/src/claude/container-spawn.tspackages/providers/src/claude/provider.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/dag-executor.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/providers/package.json
- packages/isolation/docker/runner.Dockerfile
- packages/workflows/src/dag-executor.ts
- (A) Stop swallowing loadConfig errors in the container-selection path — a malformed config that would carry container.* policy now fails the run instead of silently downgrading to an in-place host run. - (B) Phase C lifecycle no longer leaks into Phase B: reject pause-capable workflows (approval/interactive nodes) BEFORE creating a container, and always destroy a container run in teardown (no more leaving a privileged container for a resume path that doesn't exist). Trim container_stopped/ container_resumed from the store event types + emitter phase union + console normalizer. - (C) Wrap store.create() so a row-insert rejection removes the container + volume before rethrowing (no orphan). - (D) Default SpawnedProcess.kill() signal to 'SIGTERM' so a no-arg kill() doesn't crash in toPosixSignal. - (E) Pin the runner image: base by digest, Claude/bun/uv by version (build args); document the TLS-fetched-but-not-checksummed installer residual in SECURITY.md. - (F) Image-missing classifier message is image-agnostic (points at bun run build:runner-image) so it can't recommend a tag that contradicts a custom container.image. - (G) backend-router test asserts the independently-missing-store case. - (H) verified destroy() applies the not-found-vs-genuine distinction to both the container and volume rm and skips updateStatus on a genuine failure. validate green; live smoke re-run (pause-block rejects approval workflow with no container created; non-pausing run creates + always-destroys, host untouched).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/commands/workflow.ts`:
- Around line 1193-1204: Update the wantsContainer handling in the workflow
execution path to reject --resume before creating a new container or overlay.
Check the resume state alongside the existing pause validation and throw a clear
error explaining that container-isolated workflows cannot resume because prior
filesystem changes are unavailable; preserve the existing non-resume container
behavior.
- Around line 1600-1615: Update the teardown error handling around the destroy()
catch to preserve the caught destroyErr rather than swallowing it; after the
finally block, when workflow execution otherwise succeeded, rethrow the
preserved error so the CLI does not report success or exit successfully. Keep
the existing console.error and getLog().error reporting unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c06c515-d2b4-419a-b294-0e5355159435
📒 Files selected for processing (12)
packages/cli/src/commands/workflow.tspackages/isolation/docker/SECURITY.mdpackages/isolation/docker/runner.Dockerfilepackages/isolation/src/backend-router.test.tspackages/isolation/src/backends/container.test.tspackages/isolation/src/backends/container.tspackages/isolation/src/container/docker-exec.test.tspackages/isolation/src/errors.tspackages/providers/src/claude/container-spawn.tspackages/web/src/experiments/console/primitives/event.tspackages/workflows/src/event-emitter.tspackages/workflows/src/store.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/isolation/src/backend-router.test.ts
- packages/isolation/src/container/docker-exec.test.ts
- packages/workflows/src/event-emitter.ts
- packages/isolation/src/errors.ts
- packages/isolation/docker/SECURITY.md
- packages/isolation/docker/runner.Dockerfile
- packages/providers/src/claude/container-spawn.ts
- packages/isolation/src/backends/container.test.ts
- (1) container.ts: a Docker inspect ERROR is now 'unknown', not 'stopped'
— waitForReady only fast-fails into the native+SYS_ADMIN fallback on an
EXPLICIT Running=false, so a transient inspect blip can't silently
escalate privilege. New tri-state containerState() replaces isRunning().
- (2) CLI signal handler: SIGINT/SIGTERM now awaits container teardown
before process.exit, so Ctrl-C doesn't orphan a privileged container
(the teardown finally is bypassed by process.exit).
- (3) reject --resume for container runs before creating a container — the
prior overlay was destroyed, so a resume would run downstream nodes
against a fresh empty overlay.
- (4) a container teardown failure after an otherwise-successful run now
FAILS the CLI (rethrow after finally) instead of printing 'completed
successfully' + exit 0 with a leaked privileged container.
- (5) container.memoryMb/pidsLimit require a positive INTEGER (Number.isInteger,
not isFinite) — docker --memory/--pids-limit reject fractions.
- (6) SECURITY.md: native is the FALLBACK mode (fuse is tried first), not
the default.
- (7) rename the api-key-mirror debug log to claude.api_key_mirrored
({domain}.{action}_{state}).
Tests: inspect-blip-no-fallback (backend); resolveContainerBackendConfig +
describeWorkflowPause (CLI, both now exported). validate green; live sanity
(resume-block creates no container; plain --container run + fuse→native +
clean destroy).
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.
Summary
--container(orcontainer.enabledconfig); the run executes inside a container over a read-only bind of the project root plus a writable overlayfs upper layer, mounted at the same absolute path as the host cwd. Claude,bash:, andscript:nodes all execute inside the container.docker stop/restart) and the approval-gated write-back of the overlay diff to the live root — is not in this PR. A Phase-B container run's overlay changes are discarded on teardown (documented); the milestone is "isolation works, live root is never touched mid-run."UX Journey
Before
After
Architecture Diagram
After
Connection inventory:
dockercontainer.*config merged repo>globalLabel Snapshot
risk: mediumsize: Lisolation|workflows|providers|cli|core|config|adapters|server|webisolation:container-backendChange Metadata
featuremulti(isolation + workflows + providers + cli)Linked Issue
Validation Evidence (required)
bun run validate # exit 0 — all 8 checks (type-check, lint, format, 4 generated-file checks, tests)bun run validategreen. New unit tests:isolation/src/container/docker-exec.test.ts,isolation/src/backends/container.test.ts(incl. SQLite string-metadata regression),providers/src/claude/container-spawn.test.ts,isolation/src/backend-router.test.ts(container routing),workflows/src/dag-executor.test.ts(containerExec fail-fast + container-command normalization).--container+ a bash-node workflow -> container created, bash ran INSIDE (hostname == container id, overlay mounted at the host cwd path),proof.txtlanded in the overlay only, host root unchanged, container + volume torn down cleanly (no leak),container_created/container_destroyedDB rows persisted. Non-Claude (codex) provider +--container-> capability fail-fast, no container leak. Image-missing -> actionable preflight error.Security Impact (required)
--cap-add SYS_ADMIN --security-opt apparmor=unconfined(native overlay mount) and in-container work runs as root underIS_SANDBOX=1. Scoped to the isolation container, which is the boundary (read-only lower bind + overlay upper on a VM-local volume + Archon-managed env only). Opt-in via--container/config; never the default.--network bridge|none; no new Archon-side calls). The runner image build pulls the Claude/bun/uv installers.requestOptions.env(managed bag) into the container viadocker exec -e; hostprocess.envis deliberately NOT forwarded (the isolation boundary).Compatibility / Migration
container: { image?, network?, memoryMb?, pidsLimit?, enabled? }in.archon/config.yaml(repo>global); new--containerCLI flag; new workflowcontainer.enabledpolicy. All optional.isolation_environments(provider'container',branch_namesentinel''); newworkflow_eventstypes are a TS const (the column has no CHECK constraint).bun run build:runner-image(ordocker build -t archon-runner:latest -f packages/isolation/docker/runner.Dockerfile packages/isolation/docker).Human Verification (required)
--containerbash-node run (isolation + teardown + events); non-Claude provider fail-fast + no leak; image-missing preflight error; native overlay mount on Docker Desktop macOS.Side Effects / Blast Radius (required)
runSubprocessrefactor (hostprocess.envmerge moved into the helper) touches all deterministic exec sites — mitigated by the host path being byte-identical + the full workflows suite green.diy.archon.managed) scope all future pruning.Rollback Plan (required)
3ac6f93a; container is opt-in so no host behavior depends on it. Any orphaned containers:docker ps -a --filter label=diy.archon.managed=true+docker rm -f.--container/ don't setcontainer.enabled-> in-place (default) is unchanged.isolation.container_*warn/error logs; preflight errors on missing daemon/image.Risks and Mitigations
$ARTIFACTS_DIRnot mounted into the container (a node writing directly to it fails).readContainerMetadataparses both) + regression-tested + throws loudly on unusable metadata; store-layer fix noted for follow-up.Summary by CodeRabbit
--containerand workflow configuration, including container backend settings and runner image defaults.