Skip to content

D2: Add pinned restricted agent containers - #40

Merged
mchwang merged 17 commits into
mainfrom
codex/agent-isolation-d2
Sep 25, 2026
Merged

mchwang merged 17 commits into
mainfrom
codex/agent-isolation-d2

Conversation

@mchwang

@mchwang mchwang commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

D2 adds the pinned, restricted container boundary on top of D1. It builds Node 26.7.0 from a digest-pinned base with Codex 0.153.4 and Claude Code 2.1.281, prepares independent byte/inode-capped task and Git filesystems, and refuses to start unless Docker's resolved configuration and the in-container probe both match the required lockdown.

Tracking: #28. Owner: Codex Lane D. Build step 5 foundation, D2 / partial T1 and isolation T9. Base PR: #31 (codex/agent-isolation-d1), because D1 is not merge-authorized. D2 validated head: a4bd301442fa7574f015e7514fd143b0b016b049. Remote-main baseline observed before D2: b181d15 (guarded merge gate). Dependencies: D1 contract and isolated staging clone. Exact owned files: agents/container/**, git/clone.ts clone-capability registration, test/agent-container.test.ts, .github/workflows/agent-isolation.yml. No runner/store/UI/common-CI/package-manifest changes.

The profile defaults to --network=none; production callers cannot request bridge access. The two local credential probes use test-only Docker argv outside the exported production runner, with an empty fixture repository and one schema file. D3 must replace that temporary probe access with vendor-only egress before production invocation. Phase tool enforcement is also D3; adapters, cancellation settlement and output capture remain D4. The total storage-pool manager and combined hostile-input/common-CI completion remain D5/F. This PR does not claim full T1 or T9 completion.

Security behavior:

  • base image and both CLI versions are pinned and checked through image labels plus startup version probes;
  • daemon-resolved user, entrypoint, command, namespaces, devices, mounts, environment, resources and image identity are validated before start;
  • the root is read-only, all capability sets are empty, no-new-privileges is set, and normal profiles have no network;
  • /work and /work/.git are separate engine-owned tmpfs volumes with byte/inode ceilings; Git metadata is always read-only to the agent;
  • schema input contains one bounded regular file; Codex auth is a separate read-only file over bounded writable CLI state; Claude receives only its OAuth variable at launch;
  • the token is never retained or serialized in the profile object, and the host environment is rebuilt from an allowlist;
  • HOME, /tmp, and Codex state are bounded tmpfs mounts replaced for every invocation;
  • one overall deadline is budgeted across each multi-stage build, allocation and validation sequence.

Validation on exact pushed head a4bd301442fa7574f015e7514fd143b0b016b049:

  • authenticated real-Docker suite: 15/15 passed, including actual Codex and Claude reads of the hidden schema field;
  • npm run typecheck: passed;
  • npm test: 231/231 passed across 14 files; the ordinary CI path registers thirteen non-secret Docker tests and has no skipped tests;
  • the suite proves read-only planning mounts, writable execution persistence, fresh scratch/home, non-root/no-capability execution, absent source checkout and inherited secret, immutable tools/root, Git status, hard byte/inode failures with actual inode counts, read-only separate metadata, hard-link and mountpoint-replacement refusal, missing-lockdown refusal, exact tmpfs and namespace validation, unexpected-mount refusal, unbounded-volume refusal, immutable image capture, production network refusal and credential separation;
  • full current-diff self-review completed twice; the second complete pass found no remaining issues. The worktree is clean.

Observed failing-before cases fixed during development:

  • Claude's native binary was absent when its pinned postinstall was suppressed; the Dockerfile now explicitly allows only that pinned package's build-time script, and startup checks its exact version.
  • both authenticated probe fixtures initially failed to prove actual schema access; the schema mount is now explicitly container-readable and both providers must return the field value they read.
  • Docker Desktop rewrites bind sources through /host_mnt; validation now canonicalizes that engine representation while still checking the original host identity.
  • the Claude token was initially retained on the immutable profile; a regression now proves serialized profiles contain no token, and launch requires the credential separately.
  • Linux CI could not traverse a clone below a mode-0700 host ancestor, so every Docker test timed out while the same path passed on Docker Desktop. A bounded, root-only, no-network seeder now copies without executing repository content into volumes already held by the locked non-root keeper; the agent never receives those seeding capabilities.
  • the validator trusted a mutable image tag and a caller-selected network expectation. Agent, seeder and keeper launches now use the captured built digest, and production validation requires network none independently of caller data.
  • summary-only test concerns were made concrete: shell probes now fail fast, inode exhaustion counts actual files, tmpfs volume options are checked exactly, and PID/IPC modes plus the complete tmpfs mount set are validated.
  • profile cloning could replace both trusted fields and Docker argv, while schema/auth sources could change after capture. The exact frozen profile is now registered privately and both host inputs have stable metadata plus content identities rechecked across launch.
  • extra unconfined security policies and a worktree-first PATH could bypass intended probes. Security options are exact, container PATH must equal the image PATH, and the probe resets PATH before any utility lookup.
  • successful final Docker operations could cross the overall deadline. Image inspection, seeding, keeper/container validation, creation and attached execution now check the shared deadline after settlement before returning success.
  • CapDrop=ALL could coexist with an added capability, and subset option checks admitted conflicting filesystem flags. Agent/keeper CapAdd must be empty; every task-volume and tmpfs option set must match exactly with no duplicates.
  • a Linux mode-0600 host auth file is unreadable to container UID 10001. The builder reads it once through O_NOFOLLOW, writes a bounded mode-0444 snapshot under a mode-0700 runner directory, binds that snapshot read-only, and removes it when the one-shot profile settles.

Review rounds:

  1. Copilot reported three inline findings: mutable image identity and two forms of caller-authorized bridge networking. All were reproduced and fixed in 124800b; each thread has a regression, reply and resolution. Its six summary concerns were reviewed: Linux setup portability, fail-fast shell assertions, actual inode creation, exact tmpfs options/mount set, unexpected mounts and exact namespaces are covered by the same validated head. Nothing was declined. The first exact-head CI run also exposed the Linux mode-0700 traversal failure; both new real-Docker jobs pass after the fix.
  2. Copilot confirmed the three round-1 findings resolved and reported six new inline findings: mutable schema/auth/profile identity, permissive security-option checking, PATH shadowing, and a missing post-seed deadline check. All were reproduced or made concrete and fixed in e02995f; each inline thread has a regression or direct deadline guard, reply, and resolution. Two summary-only late-success concerns for final image and keeper inspection were fixed with post-operation deadline checks. Nothing was declined.
  3. Copilot confirmed all six round-2 findings resolved and reported two new inline findings: nonempty CapAdd was not rejected, and task-volume options were matched as subsets. Both were reproduced and fixed in 4b964a9 with pre-start capability refusal and exact duplicate-free option matching; both threads have replies and are resolved. Its summary also made Linux auth readability concrete. Codex auth is now staged once from a stable open file descriptor into a runner-owned mode-0444 snapshot under a mode-0700 directory and removed after the container settles. Nothing was declined.
  4. Copilot confirmed both round-3 findings resolved and reported two new inline findings: arbitrary image digests could reach privileged helpers, and credential staging was removed even when Docker cleanup failed. Both were fixed in 0845b6d: only builder-registered image digests can cross helper/profile boundaries, and cleanup retains staging plus surfaces an aggregate failure unless removal succeeds or absence is confirmed. Its summary-only cache-path concern was fixed by pinning both cache locations with a real-Docker mutation regression. Both threads have replies and are resolved. Nothing was declined.
  5. Copilot confirmed both round-4 findings resolved and reported two new inline ownership findings: filesystem tuples could cross task clones, and a duplicate attempt collision could remove the active original container. Both were fixed in 5ccd2aa with private clone-bound allocation capabilities and independently labeled container ownership; copied/cross-clone tuples and duplicate-name cleanup have real-Docker regressions. Its two summary-only cleanup concerns were fixed by one bounded cleanup deadline, explicit absence checks, stable allocation labels, aggregate failure reporting, and retention of the private capability after failure so cleanup can be retried. Both threads have replies and are resolved. Nothing was declined.
  6. Copilot confirmed both round-5 findings resolved and reported two new inline findings: remaining Docker namespace modes were not checked, and requested bind sources were checked without also checking daemon-resolved sources. Both were fixed in b975c7f with exact UTS/user/cgroup namespace validation, a host-UTS regression, and canonical comparison of both source representations. Its three summary-only cleanup concerns were fixed by deleting credential identity only after filesystem cleanup, recording planned volume names before create, and naming/labeling/removing the privileged seeder on timeout. The exact-head run then exposed only the default five-second duration of the expanded two-allocation regression; aa1bdbc gives that real-Docker test its existing 60-second boundary. Both threads have replies and are resolved. Nothing was declined.
  7. Copilot reported no inline findings and confirmed both round-6 findings resolved. Its summary made one remaining credential-path case concrete: CODEX_HOME was allowlisted without an exact value check. c438462 pins it to /run/codeboost-auth/codex before start and extends the real-Docker environment mutation regression. Nothing was declined.
  8. Copilot's overview said “Findings: None” but also opened four moderate threads. All four were fixed in a4bd301: descriptor reads are capped during concurrent growth; schema input is a sealed runner snapshot; Docker memory/swap/shm/CPU/block-I/O controls are pinned and validated; and D1-created clones carry a private capability checked before root seeding. Copied clones, source mutation, and unlimited swap have real-Docker regressions. All four threads have replies and are resolved. Nothing was declined.
  9. Copilot confirmed all four round-8 findings resolved and returned “Findings: None,” with no inline or summary-only actionable finding. The review loop converged. Nothing was declined.

Review-lesson audit: immutable identity and caller-independent safety invariants are covered by AGENTS.md “Guarded external actions” rules requiring fail-closed validation of every classification field and preserving stable identity. Linux tmpfs lifetime/mount-target behavior and the shell fixture changes are one-off Docker/test details, so no new general rule is added. No merge is authorized.

Copilot AI lite review requested due to automatic review settings September 24, 2026 15:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Three critical and six moderate unresolved findings affect image identity, network isolation, lockdown validation, and test reliability.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 3 High severity

Open (3)
What changed in this PR

Adds a pinned, restricted Docker boundary for agent execution with bounded filesystems, credential isolation, startup probes, and real-Docker CI tests.

Changes:

  • Builds pinned Node, Codex, and Claude agent images.
  • Adds isolated task/Git filesystems and lockdown validation.
  • Adds container isolation tests and CI coverage.
File Summary Findings
test/​agent-container.test.ts Tests Docker isolation, quotas, and metadata protection. Three moderate findings: shell assertions require fail-fast behavior and quota tests must prove actual inode creation.
agents/​container/​run.ts Allocates storage and validates/runs containers. One critical network-mode finding; three moderate findings covering tmpfs limits/options, unexpected mounts, and namespace validation.
agents/​container/​profile.ts Creates restricted container profiles. One critical finding: production-callable profiles permit bridge networking.
agents/​container/​probe.sh Performs in-container lockdown checks. No findings.
agents/​container/​image.ts Builds and verifies the agent image. One critical finding: validation must bind the built immutable digest rather than a mutable tag.
agents/​container/​Dockerfile Defines the pinned agent image. No findings.
.github/​workflows/​agent-isolation.yml Runs container isolation validation in CI. No findings.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread agents/container/image.ts
Comment thread agents/container/profile.ts Outdated
Comment thread agents/container/run.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 15:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical profile and container-validation gaps, along with deadline-handling issues, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 5 High severity · 1 Medium severity

Open (6)
Resolved since last review (3)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Missing deadline check after final image inspection

agents/​container/​image.ts:25

The final image-inspect operation is timed with remaining(), but its successful result is never checked against the deadline afterward. If the inspect returns after the overall build deadline, buildAgentImage can still return an image ID as a successful build.

Medium severity Missing deadline check after final keeper inspection

agents/​container/​run.ts:180

The keeper inspect is the final Docker operation in this validation sequence, but there is no remaining() check after it. If that inspect returns successfully after the deadline (the same late-success case this codebase tests for other multi-stage operations), the function can accept the container after its overall validation deadline has expired.

Comment thread agents/container/profile.ts Outdated
Comment thread agents/container/run.ts Outdated
Comment thread agents/container/run.ts
Comment thread agents/container/run.ts
Comment thread agents/container/run.ts
Comment thread agents/container/run.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 16:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Two critical and five moderate findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (6)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Mounted auth file may be unreadable by the container user

agents/​container/​profile.ts:124

The profile accepts a normal mode-0600 auth file without checking that UID 10001 can read it. A bind mount preserves the host file's ownership and mode, so on the Linux runner an auth file owned by the host user will be present but unreadable to the container user; the startup probe only checks -f and the provider then fails during authentication. Stage the credential into a runner-owned bounded file with controlled read permissions, or validate effective container readability before creating the container.

Comment thread agents/container/run.ts Outdated
Comment thread agents/container/run.ts
Copilot AI review requested due to automatic review settings September 24, 2026 16:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Pin npm and XDG cache paths to isolated scratch directories

agents/​container/​run.ts:220

The validator allowlists npm_config_cache and XDG_CACHE_HOME but never pins their values. A resolved configuration with either path redirected to /work is accepted, so execution can persist cache data in the task filesystem and consume its quota instead of using the fresh /tmp scratch mount. Require both values to remain /tmp/npm-cache and /tmp/xdg-cache (and keep the probe consistent).

Comment thread agents/container/profile.ts
Comment thread agents/container/run.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 16:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in profile validation and container lifecycle handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Unbounded allocation cleanup can hang and leak resources

agents/​container/​run.ts:111

Allocation-failure cleanup uses unbounded spawnSync calls and ignores both exit status and errors. If Docker is unavailable or hangs after a partial allocation, this catch can block indefinitely despite the overall deadline and can leave the keeper/volumes behind, allowing repeated failures to exhaust Docker storage. Bound cleanup and surface or aggregate cleanup failures instead of silently continuing.

Medium severity Cleanup ignores failures and can retain task storage

agents/​container/​run.ts:287

Both cleanup operations ignore their exit status, so a failed or timed-out keeper removal or volume deletion still reports success and can leave the task and Git data retained in Docker. Propagate cleanup failure (while handling already-absent resources explicitly) so callers cannot release the task state while its storage remains mounted or recoverable.

Comment thread agents/container/profile.ts
Comment thread agents/container/run.ts
Copilot AI review requested due to automatic review settings September 24, 2026 16:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved namespace and resolved-mount validation issues, plus cleanup and resource-leak risks, remain.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (2)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Delete identity only after credential directory cleanup succeeds

agents/​container/​profile.ts:100

The profile identity is deleted before the staged credential directory is removed. If rmSync fails (for example, during a filesystem or permission failure), the function throws but the WeakMap entry is already gone, so later cleanup calls become no-ops and the credential snapshot is orphaned. Remove the directory first and delete the identity only after removal succeeds, so a cleanup failure remains recoverable.

Medium severity Record volume names before creation to ensure cleanup

agents/​container/​storage.ts:113

The volume name is recorded only after docker volume create settles. If the daemon creates the volume but the client times out or otherwise throws before this push, the catch block does not include that known volume in cleanup, leaving an orphaned task filesystem. Record each name before issuing the create (cleanup already treats an absent volume as success), or otherwise inspect both planned names on failure.

Medium severity Track and clean up the privileged seeder container

agents/​container/​storage.ts:127

This privileged seeder is launched with --rm but without a name or a tracked handle. If the Docker CLI times out or is killed at the allocation deadline, the daemon can leave the seeder running; --rm only removes it after it exits. The catch path tracks only the keeper and volumes, so the root seeder can continue past the deadline and keep the volumes mounted, causing cleanup to fail and leak the allocation. Give the seeder an owned name and explicitly stop/remove it during cleanup.

Comment thread agents/container/run.ts Outdated
Comment thread agents/container/run.ts
Copilot AI review requested due to automatic review settings September 24, 2026 16:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Four unresolved moderate findings remain in profile and runtime validation.

Review effort: Lite
Findings: None

Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Enforce the resolved CODEX_HOME path before container creation

agents/​container/​run.ts:204

CODEX_HOME is allowlisted but its value is never checked here. A caller-mutated create argv with CODEX_HOME=/work therefore passes validateContainer and createValidatedContainer can return a container whose credential lookup is redirected; the probe only rejects this later at start. Require the daemon-resolved value to be exactly /run/codeboost-auth/codex so the pre-start validation is fail-closed.

Copilot AI review requested due to automatic review settings September 24, 2026 16:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate security findings remain in profile.ts and run.ts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)

Comment thread agents/container/profile.ts Outdated
Comment thread agents/container/profile.ts Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 16:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical and moderate findings remain unresolved in container resource enforcement, clone authorization, and cleanup ownership.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 High severity

Open (4)

Comment thread agents/container/run.ts
Comment thread agents/container/storage.ts
Copilot AI review requested due to automatic review settings September 24, 2026 17:10
mchwang and others added 8 commits September 24, 2026 23:31
- Seed Git metadata straight into its own volume so the work allocation
  never has to hold the worktree and history at once.
- Compare tmpfs byte ceilings against the page-rounded limit in the probe.
- Request private IPC and cgroup namespaces explicitly instead of relying
  on daemon defaults.
- Treat null Ulimits/Blkio fields from Docker inspect as empty.
- Run the isolation workflow on pushes to main, not the feature branch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Read Codex auth through one no-follow descriptor instead of lstat,
  realpath, then open, so the path cannot be swapped to another host file.
- Retain staged credentials and fail when the agent container name is held
  by another invocation, instead of treating it as cleaned up.
- Require Docker's default no-restart policy before start.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A `docker create` client killed by its deadline may still land in the
daemon. Cleanup now keeps checking for the container through a bounded
settle window and retains staged credentials if absence cannot be proven.
Also give the immutable-image test the same timeout as its neighbours.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Request Docker's builtin seccomp profile for the agent, keeper and seeder
  containers, since the daemon default can be unconfined (Docker Desktop
  reports profile=unconfined). Validation requires exactly
  no-new-privileges plus seccomp=builtin, and the startup probe refuses to
  exec unless /proc/self/status reports an active filter.
- Record the clone directory and .git identities at creation and re-verify
  them before and after seeding, so a replaced staging path is refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@mchwang
mchwang force-pushed the codex/agent-isolation-d2 branch from eccc116 to fb6577c Compare September 25, 2026 06:37
Copilot AI review requested due to automatic review settings September 25, 2026 06:37
@mchwang
mchwang changed the base branch from codex/agent-isolation-d1 to main September 25, 2026 06:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Four moderate issues remain around build reconciliation, restart-policy validation, cleanup reconciliation, and clone capability checks.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread agents/container/storage.ts Outdated
@mchwang mchwang closed this Sep 25, 2026
@mchwang mchwang reopened this Sep 25, 2026
- Track which allocation step's Docker client was killed by its deadline
  and give that volume, keeper or seeder a bounded settle window during
  cleanup, removing it if it lands late.
- Validate the task keeper's restart policy as well as the agent's.
- Re-verify the registered clone's staging directory identity whenever
  task filesystems are bound to an invocation clone.
- Clamp createValidatedContainer and runContainer to the captured
  invocation deadline and refuse to start once it has passed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 25, 2026 07:04
@mchwang

mchwang commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on findings that appeared only in Copilot's review summaries, with no inline thread (all in 162554e):

  • Invocation deadline discarded (review of eccc116): createValidatedContainer and runContainer now clamp their Docker budget to the captured InvocationInput.deadline and refuse to start once it has passed. A regression runs sleep 30 under a 4 s deadline with a 60 s timeout; it now stops by the deadline, where the previous code ran for about 33 s.
  • Restart-policy validation (review of fb6577c): the task keeper's restart policy is now validated as well as the agent container's. A regression runs docker update --restart=always on a live keeper and confirms validation refuses it.
  • Clone capability checks (review of fb6577c): assertTaskFilesystems now re-verifies the registered clone's staging-directory identity whenever filesystems are bound to an invocation clone, instead of re-resolving the path.
  • Build reconciliation (review of fb6577c): no change. Image trust is keyed on the digest that buildAgentImage inspects after a successful build, and a timed-out build registers nothing. A build that finishes later in the daemon can move the tag, but profiles and task storage accept only registered digests, never the tag.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved phase-authentication and captured-snapshot integrity issues remain, and the CI timeout does not cover the configured test budgets.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread agents/container/profile.ts
- captureInvocation registers each frozen request it returns, and
  createContainerProfile rejects any invocation it did not capture, so a
  copied request with an edited phase cannot get a writable workspace.
- Raise the agent isolation job timeout to cover the image build,
  teardown and per-test Docker budgets.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 25, 2026 07:12
@mchwang

mchwang commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on the summary-only items in Copilot's review of 162554e (both in 72fb064):

  • Captured-snapshot integrity: captured invocations are now authenticated as well as frozen. Only the exact object returned by captureInvocation is accepted at the profile boundary, so an edited copy cannot pass. This is the same fix as the phase thread.
  • CI timeout vs test budgets: the agent isolation job's timeout-minutes goes from 15 to 45, which covers the 10 min image build, the 2 min teardown and the per-test Docker budgets.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Two critical and one moderate unresolved security findings must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
Resolved since last review (1)

Comment thread agents/container/run.ts
Comment thread agents/contract.ts
- Request --runtime=runc for the agent, keeper and seeder containers and
  require it during validation, so an alternate configured runtime cannot
  bypass the checked isolation settings.
- captureInvocation refuses an attemptId it has already captured, so an
  existing request cannot be re-captured with an upgraded phase, deadline
  or command allowlist.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 25, 2026 07:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Two moderate findings remain unresolved, with one additional scope nit.

Review effort: Lite
Findings: None

Resolved since last review (2)

@mchwang
mchwang merged commit a02d9cc into main Sep 25, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants