feat(sandbox): add Amazon Bedrock AgentCore Runtime backend - #937
Conversation
Adds `--sandbox agentcore`, running each rollout in an AgentCore Runtime microVM, and first refactors the transport layer the new backend would otherwise have had to copy. Refactor (behavior-preserving) Transport selection was an `if environment == "..."` chain in the ACP layer that re-derived the backend from a string while already holding the sandbox object, and sniffed private attributes (`env._strategy`) to tell Daytona's variants apart. Backends now answer `BaseSandbox.live_process(agent=...)` themselves, so the chain collapses to one call and a new backend adds no branch. This also makes an existing latent bug explicit: Modal fell through the chain's `else` and was handed a `DaytonaProcess`, failing deep inside Daytona SSH setup; it now raises an actionable NotImplementedError. `sandbox/process.py` had reached 1026 lines and is split into a package (import paths unchanged). `LiveProcess` becomes a pure ABC and the local subprocess implementation moves to `SubprocessLiveProcess`. That split is what the two WebSocket transports need: `DaytonaPtyProcess` already had to neutralize the base class with `_process = None # Not used`, and AgentCore is the second such transport. Per-backend capability facts move onto the provider registry, so the no-network gate reads `enforces_no_network` instead of growing a `sandbox == "<name>"` special case per backend that cannot isolate. Also drops a stale hand-copied provider list from a CLI docstring. AgentCore backend AgentCore takes no image per run — an image must be pushed to ECR and registered as an agent runtime. BenchFlow provisions per task: build for linux/arm64, push under a content-digest tag, reuse or create the runtime. The digest tag is what makes repeat runs cheap (measured: environment_setup 58.9s cold, 16.6s on reuse). Commands go through InvokeAgentRuntimeCommand; the ACP agent runs on the `open_shell` WebSocket, which shares a session and filesystem with exec. Three platform behaviors were established by experiment against the live service, not from docs, and are encoded here: - A container that does not answer `GET /ping` on :8080 reaches READY but 500s on every command. Task images know nothing about AgentCore, so a stdlib-only responder is appended to the image as its entrypoint. - READY describes the runtime definition, not a running session. The first command on a cold, never-pulled image 500s while the container boots, then succeeds. Warm-up retries with backoff so that race cannot surface as a scored-0 rollout. - The shell is a PTY (echo, bracketed paste, CRLF), the same shape as DaytonaPtyProcess, and is handled the same proven way. Gated rather than faked: snapshots (no primitive), multi-service compose (single container), and `network_mode = "no-network"` — `networkMode` offers only PUBLIC or VPC, so no-network tasks are refused instead of run unisolated. Throttling and quota errors map to SandboxStartupError so capacity problems stay attributable as infrastructure.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34382e6cc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dockerfile = self.environment_dir / "Dockerfile" | ||
| if not dockerfile.exists() and not self.task_env_config.docker_image: |
There was a problem hiding this comment.
When an environment contains both a Dockerfile and docker-compose.yaml, this validation accepts it, but _materialize_build_context() builds only the Dockerfile and AgentCore never launches the compose side services. Such tasks therefore run in a silently incomplete environment and can produce failures or scores attributed to the agent rather than the missing service; reject compose environments before launch as the documentation promises.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 935f67f66. Agreed this is the worst failure mode of the five — a missing side service produces a score attributed to the agent.
supports_compose now lives on the provider registry (true for docker and daytona-DinD, false for agentcore/modal/apple-container), so this is a declared capability rather than another per-name branch. Rejection happens twice: validate_task_runtime_support refuses during planning, before any image is built, and _validate_definition refuses at construction. Detection uses a shared compose_definition_path() covering docker-compose.yaml/yml and compose.yaml/yml.
Live: ### F4 compose rejected: True. A test also pins that docker still accepts compose tasks, so the gate cannot regress the backends that do support them.
| shim_path = context / ".benchflow_agentcore_shim.py" | ||
| shim_path.write_text(_PING_SHIM) | ||
| generated = context / "Dockerfile.benchflow-agentcore" | ||
| generated.write_text( |
There was a problem hiding this comment.
Concurrent trials of the same task write these two fixed paths in the shared environment directory and each _publish_image() later unlinks them, so one rollout can remove or replace files while another Docker build is reading them and intermittently fail setup. A task that already contains either filename is also overwritten and then deleted by a single run; use rollout-unique temporary files or serialize and restore the originals.
AGENTS.md reference: AGENTS.md:L30-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Still open at exact head 2b69b4b62, specifically the single-run collision in the original finding. materialized() writes both reserved paths into the caller-owned task environment, then unlinks them unconditionally. A repro with pre-existing Dockerfile.benchflow-agentcore and .benchflow_agentcore_shim.py showed generated contents inside the context and both original files absent afterward. The canonical digest also skips those names, so a colliding context can cache-hit as if the task files did not exist. Please fail closed before identity/build if either reserved path exists (and keep writes inside cleanup coverage), with byte-preservation regressions for both names.
There was a problem hiding this comment.
Fixed in 4ede269c4. Confirmed the single-run collision, including the cache-hit consequence you noted.
Both reserved paths are now refused in _validate_definition, before identity or build, so nothing is written or unlinked. Live, with a pre-existing .benchflow_agentcore_shim.py: refused: True | original preserved: True.
Byte-preservation regressions cover both names and assert the original content is intact after the refusal. Mutation-checked: removing the guard makes them fail (DID NOT RAISE).
I chose refusal over backup-and-restore deliberately — a restore path has its own failure mode (crash between write and restore still loses the file), and these names are ours by contract.
| if self.task_env_config.docker_image: | ||
| base = f"FROM {self.task_env_config.docker_image}\n" | ||
| else: | ||
| base = base_dockerfile.read_text() |
There was a problem hiding this comment.
When a task declares both a prebuilt docker_image and a local Dockerfile, calling start(force_build=True) still takes this branch and merely rebuilds a shim layer on the prebuilt image; the task Dockerfile is never used. This differs from the other sandbox backends and can run a stale environment despite the caller explicitly requesting a source rebuild, so the source choice must incorporate force_build rather than using it only for --no-cache.
Useful? React with 👍 / 👎.
| blob = "".join(staged) | ||
| assert "do not ship me" not in blob |
There was a problem hiding this comment.
This assertion cannot detect symlink exfiltration because _upload_via_tar() base64-encodes the archive before passing it to exec, while staged contains only those encoded shell commands; the space-containing plaintext do not ship me cannot appear even if the archive includes it. A regression that starts dereferencing symlinks would therefore still pass, so decode the staged chunks and inspect or extract the resulting tar before asserting the secret is absent.
AGENTS.md reference: AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 935f67f66. You were exactly right — the space-containing plaintext could never appear in base64-encoded printf commands, so the assertion was unfalsifiable.
The test now reassembles the staged base64 chunks, opens the resulting tar, and asserts on real member names and payloads, with a positive control (real.txt must be present) so an empty archive cannot pass.
I mutation-checked it rather than trusting that it passes: reverting iter_safe_tree to a plain rglob makes it fail:
VERDICT: CATCHES THE REGRESSION
E KeyError: "linkname 'workspace//.../host-secret.txt' not found"
Before the fix, the same mutation passed.
Makes AgentCore usable as a parallel backend and fixes a bug that would silently corrupt any multi-trial run. A rollout is a **session**, not a runtime. One registered runtime hosts many concurrent, filesystem-isolated microVMs (verified: 8 concurrent sessions on one runtime), and the account quotas are lopsided in exactly that direction — 5000 Active Session Workloads against 100 Total Agents, with CreateAgentRuntime and ListAgentRuntimes both limited to 5/s. The first cut got this backwards: it created and deleted a runtime per rollout and named it after the *task*. Three trials of one task therefore raced to create the same runtime, and whichever finished first deleted it out from under the other two. Two different tasks ran fine, which is why the initial testing missed it. Runtime identity now follows the image's content digest, computed from the build context before anything is built, and provisioning is single-flighted per key in a new `agentcore_provisioning` module. N concurrent rollouts of one task now perform exactly one build, one push, and one registration, then open N sessions. Verified live: 5 concurrent rollouts of one task produced 1 runtime and 5 isolated sessions in 17.7s, and the shared runtime survived teardown. Failures are deliberately not memoized so a transient throttle cannot poison the rest of a long run. The digest hashes file *contents*, not paths or mtimes: BenchFlow copies tasks into temp directories, so any location-based identity would change every run and defeat image reuse entirely. Because runtimes are now shared they outlive the run, like a built Docker image, and `bench sandbox cleanup` reclaims them by age. Only runtimes tagged benchflow-managed are touched, and unreadable tags fail closed — verified live against a runtime created outside BenchFlow, which was correctly skipped. Also fixed while wiring cleanup up: - `bench sandbox cleanup` aborted entirely when the Daytona SDK was installed but had no credentials, stranding AgentCore runtimes against a 100-per- account quota. Each backend's cleanup is now isolated. - Cleanup is gated on BENCHFLOW_AGENTCORE_ROLE_ARN rather than on an importable boto3, which several unrelated extras pull in. Without that gate the command reached out to AWS on any machine with credentials configured — including from the test suite. Adds an image-size preflight for AgentCore's hard, non-adjustable 2 GB limit, so an oversized environment fails with a message naming the quota instead of an opaque runtime error later.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
Update: parallel-safety rework (
|
| Quota | Default | Adjustable |
|---|---|---|
| Active Session Workloads / account | 5,000 | yes |
| Total Agents (runtimes) / account | 100 | yes |
CreateAgentRuntime / ListAgentRuntimes |
5/s | yes |
| Max image size | 2 GB | no |
So image + runtime are provisioned once per distinct task image (keyed by a content digest of the build context) and shared by every trial and skill arm; sessions are what scale out. New agentcore_provisioning module single-flights this per key.
The digest hashes file contents, not paths or mtimes — BenchFlow copies tasks into temp dirs, so any location-based identity would change every run and defeat reuse entirely.
Live verification
- 5 concurrent rollouts of one task → 1 runtime, 5 isolated sessions, 17.7s total; shared runtime survived teardown.
- Reaper: the tagged runtime was deleted, a runtime created outside BenchFlow was correctly skipped as unmanaged. Unreadable tags fail closed.
Also fixed while wiring up cleanup
bench sandbox cleanupaborted entirely when the Daytona SDK was installed without credentials, stranding AgentCore runtimes against the 100-per-account cap. Each backend's cleanup is now isolated.- Cleanup is gated on
BENCHFLOW_AGENTCORE_ROLE_ARNrather than an importableboto3(which several unrelated extras pull in). Without the gate the command reached AWS on any machine with credentials — including from the test suite, which is how I caught it.
Plus an image-size preflight for the hard 2 GB cap, so an oversized environment fails naming the quota instead of surfacing later as an opaque runtime error.
Gates
4989 passed, 10 skipped · ruff · ruff format · ty check src/ — all clean. 17 new tests in tests/test_agentcore_parallel.py.
Known limits for a full matrix
- 88 tasks × 2 skill arms = 176 distinct images, over the default 100-runtime cap. Adjustable, but needs raising before a big run.
- 2 GB image cap is not adjustable — heavy environments (LaTeX, Playwright, large snapshots) cannot run here; those stay on Docker/Daytona.
- Still requires local Docker to build. Removing that (CodeBuild remote build) is the next piece.
AgentCore only runs images that already exist in ECR, so something must build one — and until now that was a local Docker daemon. On a machine without the resources to run containers, which is precisely when a cloud sandbox is worth reaching for, the backend was unusable. Image building is now a strategy behind `agentcore_builder`: - LocalDockerBuilder: the existing path, native and fast on an arm64 host. - CodeBuildBuilder: zips the build context to S3 and builds it on an AWS CodeBuild Graviton worker, pushing straight to ECR. Nothing is required locally — no Docker, no arm64 host, no qemu — so this also works from CI and from Windows. Default is `auto`: use Docker when a daemon actually answers, otherwise build remotely. The check probes the daemon rather than the CLI, because an installed binary with a stopped daemon is the common laptop case and finding out at build time wastes the whole provisioning path. Override with BENCHFLOW_AGENTCORE_BUILDER=docker|codebuild|auto. Verified live end to end with no `docker` on PATH at all: preflight passes, auto-selects CodeBuild, builds arm64 remotely, and the resulting session shows the file baked by the task's own Dockerfile (43s). The archive is a ZIP, not a tar.gz. CodeBuild's S3 source only unpacks ZIP; a tarball is downloaded verbatim, leaving the build directory holding the archive and failing with a misleading "Dockerfile ... no such file or directory". That cost a live build to discover, so it is pinned by a test. The 2 GB image cap is enforced on the worker too, before the push, and an oversized remote build is reported as a size problem rather than a generic build failure — otherwise the user goes hunting the wrong bug. Also measured the parallel ceiling now that runtimes are shared: 60 concurrent rollouts of one task ran on a single runtime in ~30s wall clock, 60/60 clean, no throttling, median session start ~15s. Documented alongside the quotas.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
Update: no local Docker + measured scale (
|
| Concurrent rollouts (one task) | Result | Wall clock | Runtimes | Median start |
|---|---|---|---|---|
| 5 | 5/5 ok | 17.7s | 1 | — |
| 25 | 25/25 ok | 31.2s | 1 | 22.6s |
| 60 | 60/60 ok | 29.7s | 1 | 15.5s |
No throttling at 60 concurrent microVMs; latency improved as the image warmed. The first real limit is the session-creation rate (400/min, adjustable), well before the 5,000 concurrent-session ceiling — so this is comfortably past Daytona's ~100-VM cap.
Gates
5003 passed, 10 skipped · ruff · format · ty check src/ — all clean. 13 new tests in tests/test_agentcore_builder.py.
Setup delta for reviewers
Remote builds need one extra role:
export BENCHFLOW_AGENTCORE_CODEBUILD_ROLE_ARN="arn:aws:iam::<acct>:role/<build-role>"assumable by codebuild.amazonaws.com, able to push ECR + read the build bucket + write logs. BenchFlow creates the CodeBuild project and an expiring S3 bucket on first use. Docs updated.
Still open
- 2 GB image cap is not adjustable — heavy environments stay on Docker/Daytona.
- A full 88×2 matrix is 176 images vs the default 100-runtime cap; adjustable, needs raising first.
- Untested above 60 concurrent, and I have not run a full multi-task matrix end to end with agents.
| report.skipped_unmanaged += 1 | ||
| continue | ||
|
|
||
| created = runtime.get("createdAt") |
There was a problem hiding this comment.
[P1] Fail closed when runtime age is unavailable
A real ListAgentRuntimes response for a runtime created minutes earlier contains lastUpdatedAt, not createdAt. This branch therefore falls through to deletion whenever createdAt is absent. I reproduced it safely with dry_run=True, max_age_minutes=1440: scanned=1, deleted=1, skipped_recent=0 for the fresh runtime. The tests currently invent a createdAt field, so they cannot catch the real SDK shape. Please adapt the actual typed response (and fail closed on a missing/unparseable timestamp) before exposing this cleanup command; otherwise the documented one-day cleanup can delete every BenchFlow-managed runtime, including fresh ones.
There was a problem hiding this comment.
Fixed in 935f67f66, and you were right about the root cause — I mocked the response shape instead of checking it.
Verified against the service model: ListAgentRuntimes returns only lastUpdatedAt; there is no createdAt in the list shape (GetAgentRuntime has both). So the field was always absent, the age comparison was skipped entirely, and the reaper failed open.
_runtime_timestamp() now accepts either field, and an unresolvable age is treated as not stale — a runtime serving a live matrix is indistinguishable from an idle one, so the safe default is to keep it.
Your reproduction, live, after the fix:
### F1 fresh runtime kept: True (scanned=1 deleted=0 unmanaged=0 recent=1 errors=0)
I also added TestAwsResponseShapeConformance, which asserts against the real botocore service model that agentRuntimes[] has no createdAt and that GetAgentRuntime carries containerUri. That pins the invented fixtures to reality so this class of drift fails a test instead of production.
| materialized(request), | ||
| zipfile.ZipFile(buffer, "w", zipfile.ZIP_DEFLATED) as archive, | ||
| ): | ||
| for path in sorted(request.context_dir.rglob("*")): |
There was a problem hiding this comment.
[P1] Apply .dockerignore before uploading the CodeBuild context
The local Docker path honors .dockerignore, but this CodeBuild path recursively zips every regular file and uploads it to S3 first. A direct reproduction with .dockerignore excluding secret.env and ignored-cache/ still placed both in the ZIP (including the secret sentinel), and changing the ignored secret also changed build_context_digest. This is both a credential-exposure risk and a local/CodeBuild parity bug. Please use one canonical Docker-context walker for both packaging and identity, then add generated files explicitly so those two views cannot drift.
There was a problem hiding this comment.
Fixed in 935f67f66. Both the credential exposure and the identity bug were real.
There is now one canonical walker, agentcore_provisioning.iter_context_files(), used by both the digest and the CodeBuild upload, so the two cannot drift. It applies .dockerignore with Docker's semantics (comments, ! re-includes, directory prefixes, last match wins) and still skips symlinks. The generated Dockerfile and shim are excluded from the canonical walk — they must not affect image identity — and added to the archive explicitly, exactly as you suggested.
Live re-verification with .dockerignore excluding secret.env:
### F2 ignored secret absent from image: True
Plus unit coverage: ignored files absent from the ZIP with a positive control that keep.txt survives, the digest unchanged when the ignored secret is rotated, and !keep.log re-inclusion.
| ) | ||
| if existing is None: | ||
| raise | ||
| arn, runtime_id, _image = existing |
There was a problem hiding this comment.
[P1] Do not adopt a runtime that points at a different image
The conflict path retrieves the existing image URI into _image and then discards it. I reproduced this with an existing old.example/repo:tag and requested new.example/repo:tag: the old runtime was adopted and update_agent_runtime was never called. The runtime name is based only on local context bytes, so an ECR repository change, a mutable base-tag change, or force_build=True can all rebuild/push a different image while reusing the old runtime. Resolve the pushed ECR digest, bind/update the runtime to that immutable image identity, and verify role/network/lifecycle equivalence on every adoption.
There was a problem hiding this comment.
Fixed in 935f67f66, including the immutable-identity point.
Two changes:
- The conflict path now compares
current_imageagainst the requested URI, callsupdate_agent_runtimewhen they differ, and then re-reads the runtime via_verify_runtime_image()to fail closed if it still does not match. Adopting silently is gone. _publish_image()now returnsrepo@sha256:..., resolved from ECR viadescribe_images, instead ofrepo:tag. You were right that the name is derived from local context bytes only — a repository change, a mutable base tag, orforce_buildcan all move what a tag points at. Binding to the registry digest pins the exact bytes pushed.
Live: ### F3 immutable digest binding: True (the registered containerUri contains @sha256:).
Not yet done: I did not add role/network/lifecycle equivalence checks on adoption. Only the image is verified. Say the word if you want that in this PR rather than a follow-up.
There was a problem hiding this comment.
The image/lifecycle checks now pass, including my fresh live 600/7200 adoption probe, but the broader adoption contract is still open at 39e2f09f7.
A service-shaped GetAgentRuntime response with the expected image/lifecycle but a different roleArn, VPC network configuration, and serverProtocol=A2A is accepted. Because update runs only when the image differs, same-image drift is neither reconciled nor rejected. That can run with the wrong privileges/connectivity or an incompatible data-plane protocol. Please assert or reconcile role, network, and protocol on every adoption.
There was a problem hiding this comment.
Fixed in 2b69b4b62. Role, network mode, and protocol are now verified on adoption alongside image and lifecycle — all five.
A runtime with the right image but a different execution role, network mode, or protocol is a different sandbox contract than the run asked for (wrong permissions, wrong egress, or a shell that never answers), so it fails closed with the found/expected pair named. Parametrized regressions cover each of the three.
Thermo-nuclear review — exact head
|
bingran-you
added
status:blocked
…doption All five blocking findings from the PR #937 review, each re-verified against the live service rather than against invented response shapes. 1. Cleanup selected fresh runtimes (fail-open). `ListAgentRuntimes` returns only `lastUpdatedAt` — there is no `createdAt` in the list shape, though `GetAgentRuntime` has both. Reading the wrong field yielded None for every runtime, which skipped the age comparison entirely, so a one-day policy selected minutes-old runtimes. Age resolution now accepts either field and an unknown age is treated as not-stale. Verified live: the reviewer's reproduction now reports deleted=0, recent=1. 2. CodeBuild ignored `.dockerignore`. The local daemon honors it natively while the remote path zipped every regular file, uploading ignored files — including secrets — to S3, and letting an ignored file perturb the image digest. Both packaging and identity now walk one canonical context (`iter_context_files`) that applies `.dockerignore` (comments, `!` re-includes, directory prefixes), with the generated scaffolding added explicitly to the archive so the two views cannot drift. 3. Stale images could be adopted. The conflict path fetched the existing image URI and discarded it. It now compares, updates the runtime when it differs, and re-reads the runtime afterwards to fail closed if it still does not match. Runtimes are also bound to the immutable `repo@sha256:...` digest resolved from ECR rather than to a mutable tag, so what a runtime contains stays knowable after a rebuild or repository change. 4. Compose tasks were accepted but only one container was built, so the agent ran without its side services and the resulting failure was scored against the agent. `supports_compose` now lives on the provider registry; the capability gate refuses multi-service tasks during planning and the sandbox refuses them at construction. Docker and Daytona-DinD are unaffected. 5. The symlink regression test asserted against base64-encoded shell commands, where the plaintext could never appear. It now decodes the staged chunks and inspects the real tar, with a positive control. Mutation-checked: reverting the symlink skip makes it fail, which it did not before.
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
All five P1 findings fixed —
|
| # | Finding | Fix | Live proof |
|---|---|---|---|
| 1 | Cleanup selected fresh runtimes | ListAgentRuntimes has only lastUpdatedAt — no createdAt. Age resolves either field; unknown age = not stale |
deleted=0 recent=1 on your repro |
| 2 | CodeBuild ignored .dockerignore |
One canonical iter_context_files() drives both packaging and identity |
ignored secret absent from image |
| 3 | Stale image adopted | Compare → update → re-read and fail closed; runtimes bound to repo@sha256: |
containerUri contains @sha256: |
| 4 | Compose accepted, one container built | supports_compose on the registry; refused at planning and construction |
compose task rejected |
| 5 | Tautological symlink test | Decodes staged base64, inspects the real tar, positive control | mutation-checked |
On #5 specifically
I did not just make it pass — I mutation-tested it. Reverting iter_safe_tree to a plain rglob now fails the test (KeyError: linkname ... host-secret.txt); before the fix, the same mutation passed.
On the "mocked shapes" critique
Added TestAwsResponseShapeConformance, which asserts against the real botocore service model that agentRuntimes[] has no createdAt and that GetAgentRuntime exposes containerUri. Invented fixtures are now pinned to reality, so this class of drift fails a test rather than production.
Issue #935 — kept open
You are right that it is not resolved. I changed the PR body from a close keyword to Refs #935, so merging will not auto-close it. The SDK-link, product-confirmation, isolation-contract, and large-file-transfer questions remain open, as do compose, no-network, snapshots, images over 2 GB, and large directory transfers.
Known gaps I did not close
- Adoption verifies the image only — no role/network/lifecycle equivalence check. Happy to add here rather than as a follow-up if you prefer.
- Large directory transfers still stream base64 through
exec; no S3/EFS path. .dockerignorematching usesfnmatchplus a directory-prefix rule, not Docker's full pattern engine. It covers the patterns tasks actually use; exotic**cases could differ.- Your no-skill-artifact finding (OpenHands advertising its global skill catalog, and provider
highrather thanxhigh) is real but orthogonal — that logic is untouched here, as with thegpt-5.6-solbare-model-ID failure you already classified as pre-existing.
Gates
5016 passed, 10 skipped, 7 deselected · ruff · ruff format · ty check src/ · git diff --check — all clean. 24 tests added across the five findings.
Ready for another look.
| report.deleted.append(runtime_id) | ||
| continue | ||
| try: | ||
| control.delete_agent_runtime(agentRuntimeId=runtime_id) |
There was a problem hiding this comment.
[P1] Do not use runtime update age as a session-activity signal
The timestamp-shape fix keeps a fresh runtime, but it still does not protect an old runtime that is actively serving a matrix. On this exact head I opened a real AgentCore session, successfully executed printf active-session, and observed that the runtime lastUpdatedAt did not change. reap_stale_runtimes(..., max_age_minutes=0, dry_run=True) then selected that exact active runtime for deletion. AgentCore session invocations reset the per-session idle timer; they do not update the runtime definition timestamp. Cleanup therefore needs an explicit activity/lease contract and must fail closed when absence of active sessions cannot be proved, rather than calling the destructive API based only on control-plane age.
There was a problem hiding this comment.
Fixed in 39e2f09f7. You were right that the timestamp fix only covered the fresh case, and the diagnosis is exact: session invocations reset the per-session idle timer without touching the runtime definition.
I checked whether cleanup could query activity instead of inferring it — there is no such API. ListSessions is Memory-scoped (memoryId/actorId required), and nothing on the data plane enumerates a runtime's sessions. So activity has to be an explicit contract, as you said.
Added a lease: provisioning tags the runtime benchflow-lease-until with an expiry covering the longest a session started then could still live (max(maxLifetime, idleRuntimeSessionTimeout)). Cleanup refuses any runtime whose lease is unexpired, unparseable, or unreadable — all three are "cannot prove it is idle". It is written once per runtime per process (provisioning is single-flighted), so it adds no per-rollout control-plane traffic.
Your repro, live on the new head, with a session active:
### P1-1 active runtime protected from age-0 cleanup: True
(scanned=1 deleted=0 unmanaged=0 recent=0 active=1 errors=0)
Known limit, documented in the commit: the lease covers the configured session window from provisioning time, so a run exceeding BENCHFLOW_AGENTCORE_MAX_LIFETIME_SEC needs that raised or cleanup deferred.
There was a problem hiding this comment.
Still open at exact head 39e2f09f7 — the lease currently fails open in two independent ways.
_write_lease()swallowstag_resourcefailures, while_lease_is_active({})returns false. An exact fake-control repro withAccessDeniedon the tag write then selected a 30-day managed runtime with no lease:scanned=1 deleted=1 active=0. The comment claiming a missing lease is conservative is therefore backwards.- The write is inside the single-flighted factory. Two later
_ensure_runtime()calls for the same immutable image invoke the factory once, so a rollout starting after the original lease window does not renew it and can be reaped while active.
Please make lease-write failure fatal or make missing leases active/fail-closed, and refresh the lease for every new session after the memoized lookup. The no-renewal trade is not safe for long or staggered matrices.
There was a problem hiding this comment.
Fixed in 2b69b4b62. Both fail-open paths closed.
A failed tag_resource now raises SandboxStartupError instead of logging — starting a session on an unleased runtime is exactly the state cleanup is permitted to delete, so the launch aborts rather than racing another process. And a managed runtime with no lease is now treated as active, not idle: every runtime BenchFlow provisions is leased before any session runs and that write is fatal, so an unleased managed runtime is unexplained rather than safe.
Separately, the memoized-provisioning gap you identified: only the first rollout of an image reached the creation path, so later rollouts inherited an aging lease. _renew_lease() now runs on every rollout, throttled to a quarter of the lease window so it costs a few control-plane calls per run rather than one per rollout. Live, on a second sandbox sharing one runtime: cache-hit rollout renewed the lease: True, same runtime shared: True.
Still true and documented: there is no lease renewal by a background task, so a rollout that runs longer than the configured window without a subsequent rollout would still age out. Renewal is driven by rollout starts.
There was a problem hiding this comment.
Still open at exact head 2b69b4b622f5055510808a6711d94fea5cc59b58: renewal is fail-closed for the first caller, but fail-open for its immediate retry. lease_needs_renewal() stores _LEASE_RENEWED[runtime_arn] = now before TagResource runs. In a focused repro, the first _renew_lease() raised SandboxStartupError on AccessDenied; after clearing the AWS failure, a second call one second later made zero tag_resource calls because the failed write was already throttled as successful. That retry can start against the same expired lease. Record the throttle timestamp only after a successful write (or roll it back on error), and add a fail-then-immediate-retry regression.
There was a problem hiding this comment.
Fixed in 4ede269c4. Exactly as diagnosed — the throttle recorded the attempt, not the write.
lease_needs_renewal() is now a pure predicate that records nothing, and mark_lease_renewed() is called only after tag_resource actually lands. A renewal that raises leaves the window untouched, so the next rollout retries.
Live: after a forced AccessDenied, throttle untouched: True, and the following _renew_lease() reached AWS. The fail-then-retry regression you asked for asserts tag_resource.call_count == 2, and I mutation-checked it — restoring record-before-write makes it fail.
| def matches(relative: str) -> bool: | ||
| ignored = False | ||
| for pattern, negated in rules: | ||
| if fnmatch(relative, pattern) or relative.startswith(pattern + "/"): |
There was a problem hiding this comment.
[P1] Use Docker-compatible ignore matching before the S3 upload
The canonical walker fixes the simple literal case, but this matcher still leaks files that Docker excludes. On this exact head, .dockerignore containing /secret.env uploaded root secret.env, and **/*.pem uploaded root secret.pem; a plain secret.env rule was the only one of the three that worked. Leading/trailing slashes are ignored by Docker and ** matches zero or more directories, so both leaking patterns are valid. Because the CodeBuild ZIP is sent to S3, this remains a credential-exposure bug. Please use a Docker-compatible pattern matcher, or a fully tested compatible implementation, instead of claiming Docker semantics from fnmatch.
There was a problem hiding this comment.
Fixed in 39e2f09f7. You were right to reject the fnmatch claim — it does not implement Docker's semantics, and both of your patterns were valid.
Root causes: /secret.env never matched because the leading separator was kept while paths are relative, and **/*.pem could not span zero directories. Replaced with a real translator: separators stripped from patterns, * and ? bounded by /, ** spanning zero or more segments, and a match on an ancestor directory excluding its contents.
Live on the new head, with .dockerignore containing exactly /secret.env and **/*.pem:
### P1-2 root-anchored + ** patterns excluded: True
Unit coverage now pins /secret.env, **/*.pem, cache/, *.log with !keep.log, and that sub/secret.env is not excluded by a root-anchored rule.
There was a problem hiding this comment.
Still open at 39e2f09f7: the new translator handles the two reported patterns, but it does not implement Docker character classes. It escapes [ and ] literally.
Current-head repro: with .dockerignore containing secret[0-9].pem, BenchFlow's canonical walker includes secret7.pem; Docker excludes it. The same holds for secret[.]env. Because that walker feeds the S3 CodeBuild ZIP, valid ignore rules can still upload credentials. Please use a tested Docker-compatible matcher or implement Docker/Go character-class semantics and add parity regressions.
There was a problem hiding this comment.
Fixed in 2b69b4b62. You were right that the translator only covered the two reported patterns.
[...] character classes are now translated properly, including [!...]/[^...] negation and ranges, with an unterminated [ treated as a literal bracket as in shell globbing.
Live, with .dockerignore containing secret[0-9].pem and a Dockerfile that actually COPY . /ctx/s the context:
### P1-3 char class excluded: True (secret1.pem absent)
### P1-3 non-matching file still shipped: True (secretX.pem present)
The second line is the control — my first attempt at this probe used a Dockerfile with no COPY, so nothing from the context reached the image and neither result meant anything. I re-ran it before reporting.
| async def _create() -> tuple[str, str]: | ||
| return await self._create_or_adopt_runtime(name, image_uri) | ||
|
|
||
| arn, runtime_id = await provisioning.once(f"runtime:{name}", _create) |
There was a problem hiding this comment.
[P1] Include the immutable image URI in the runtime single-flight key
The adoption repair is bypassed by this process cache. I reproduced sequential _ensure_runtime(repo@sha256:old) and _ensure_runtime(repo@sha256:new) calls for the same context/name: _create_or_adopt_runtime ran only once and both calls returned the old ARN. This is reachable when force_build=True pushes new bytes under the same context identity. Key the single-flight on the immutable image URI as well as the runtime name, and add the two-URI regression so a rebuild actually reaches compare/update/verify.
There was a problem hiding this comment.
Fixed in 39e2f09f7. Correct — the memo key was the runtime name, which is derived from context bytes, so a force_build=True push under the same context identity hit the cache and skipped compare/update/verify entirely.
The key is now runtime:{name}:{image_uri} using the immutable digest URI, so different bytes always reach the adoption path. Added the two-URI regression you asked for: sequential _ensure_runtime(repo@sha256:old) then _ensure_runtime(repo@sha256:new) must invoke _create_or_adopt_runtime twice and rebind.
| }, | ||
| roleArn=self._require_role_arn(), | ||
| networkConfiguration={"networkMode": "PUBLIC"}, | ||
| protocolConfiguration=_PROTOCOL_CONFIGURATION, |
There was a problem hiding this comment.
[P1] Preserve the configured lifecycle when updating an adopted runtime
The create request supplies _lifecycle_configuration(), but this update omits it. I verified the effect live on this exact head: after setting an existing runtime to idle/max 600/7200, the current adoption path correctly rebound the immutable image but the runtime came back as the service defaults 900/28800. That silently discards explicit BENCHFLOW_AGENTCORE_IDLE_TIMEOUT / BENCHFLOW_AGENTCORE_MAX_LIFETIME settings and can reclaim long-running sessions earlier than the caller configured. Pass and verify lifecycle configuration on adoption, alongside the image.
There was a problem hiding this comment.
Fixed in 39e2f09f7. Confirmed and reproduced your finding — the update omitted lifecycleConfiguration, so AWS reset it to the service defaults.
The adoption update now passes _lifecycle_configuration(), and _verify_adopted_runtime() checks lifecycle alongside the image, failing closed on drift rather than logging it.
Live with BENCHFLOW_AGENTCORE_IDLE_TIMEOUT_SEC=600 / MAX_LIFETIME_SEC=7200:
### P1-4 lifecycle preserved: {'idleRuntimeSessionTimeout': 600, 'maxLifetime': 7200}
This also answers the open question from my previous reply about role/network/lifecycle equivalence on adoption: lifecycle and image are now verified. Role and network are sent on every update but not yet asserted — say the word if you want those verified too.
| running a build to discover it. | ||
| """ | ||
| digest = provisioning.build_context_digest( | ||
| self.environment_dir, self._generated_dockerfile_text() |
There was a problem hiding this comment.
[P1] Hash every byte and mode that determines the built image
The generated _PING_SHIM is added to the CodeBuild ZIP but excluded from this identity, and file permission bits are also omitted. Independent reproductions produced the same context digest for different shim bytes and for entrypoint.sh changed from 0644 to 0755, even though the resulting archives differed. Normal reuse can therefore keep an old runtime shim or stale executable permissions after an upgrade. Include the shim contents and relevant file mode bits in the image identity, with regressions proving either change invalidates the tag.
There was a problem hiding this comment.
Fixed in 39e2f09f7. Both omissions were real: the shim is copied into the image as its entrypoint, and a mode change alters the container while leaving every content byte identical.
build_context_digest() now takes the shim text and hashes it, and hashes the executable bit per file. Regressions prove either change invalidates the tag — different shim bytes produce a different digest, and entrypoint.sh flipped 0644 → 0755 does too.
There was a problem hiding this comment.
Still incomplete at 39e2f09f7. Hashing only executable/non-executable does not capture the file mode Docker copies: 0600 and 0644 produce the same BenchFlow digest even though the resulting image differs. That can reuse stale task bytes/permissions under the same ECR identity.
There is also an unframed boundary after the shim: shim="a" + first path "bc" collides structurally with shim="ab" + first path "c" when file bytes/mode are otherwise equal. Please hash the full relevant mode and frame the shim before the first path; add regressions for both cases.
There was a problem hiding this comment.
Fixed in 2b69b4b62. Both halves.
The digest now hashes the full permission bits (st_mode & 0o7777), so 0600, 0644, and 0755 are three distinct identities rather than two. And every field is length-prefixed (label + len + ":" + payload), which removes the framing collision — a file literally named shim, or content that reproduces a separator, can no longer imitate another field. A regression asserts digest(ctx, "DF", "abc") != digest(ctx, "DFabc", "").
| # Record rather than raise: this runs in a background task whose | ||
| # exception would otherwise be swallowed, leaving readline() to | ||
| # hang until its timeout instead of reporting the real cause. | ||
| self._failure = exc |
There was a problem hiding this comment.
[P1] Wake readline() immediately when the shell reader ends
This side channel is checked only after _line_buffer.get() times out. A reproduced reader exception was recorded immediately but surfaced only at the configured timeout; clean iterator EOF records no failure at all and leaves is_running=True. With the default this can turn an infrastructure disconnect into a 900-second experiment hang. Put an EOF/error sentinel on the queue in _drain_frames, consume it immediately in readline, and make liveness include the reader task state.
There was a problem hiding this comment.
Fixed in 39e2f09f7. Both halves were right, including that clean EOF recorded no failure at all and left is_running=True.
_drain_frames now enqueues an end sentinel in a finally, so it fires on exception and on clean iterator exit. readline() consumes the sentinel and raises immediately — a reader exception surfaces as remote_session_killed, a clean EOF as "closed by the remote session". is_running now includes reader state.
New tests/test_agentcore_transport.py drives this with a fake shell: reader error wakes readline within 2s (previously 900s), clean EOF likewise, is_running flips false once the reader ends, and buffered output is still delivered before the sentinel so ending the reader cannot drop already-framed lines.
There was a problem hiding this comment.
The common EOF case is fixed, but the startup race still reproduces the original 900-second hang at 39e2f09f7.
If the reader queues [marker, _READER_ENDED], _await_marker() consumes the marker and _clear_buffered_output() discards the sentinel. The next readline() sees _reader_done=True with an empty queue and waits the full configured timeout. A 50 ms repro timed out instead of raising TransportClosedError. EOF before the marker also calls .decode() on the sentinel and raises AttributeError.
Please preserve/consume the terminal sentinel across marker synchronization and cover both startup EOF orderings.
There was a problem hiding this comment.
Fixed in 2b69b4b62. Correct — I closed the steady-state EOF but left two startup paths that could consume the sentinel.
_await_marker now treats _READER_ENDED as a terminal transport failure instead of trying to decode it, and _clear_buffered_output re-posts the sentinel if it drained one (or if the reader has already finished), so the pre-agent drain cannot strand a later readline. Regression added: kill the shell during startup, run the drain, and readline must raise within 2s rather than waiting out the timeout.
There was a problem hiding this comment.
One residual half-close path remains at 2b69b4b62. After clean reader EOF, is_running correctly becomes false, but writeline() checks only _shell and _closed; a focused repro still called shell.send("lost-message\n") successfully after _reader_done=True. ContainerTransport.send() does not pre-check liveness, so ACP requests can be silently sent to a known-dead read side or surface an untyped SDK error. Gate writes on the same liveness state and add clean/error EOF tests asserting writeline() raises and no send occurs.
There was a problem hiding this comment.
Fixed in 4ede269c4. writeline() now gates on is_running, the same liveness state as readline(), so a half-closed transport refuses writes instead of handing an ACP request to a dead read side.
Tests assert it raises and that no send occurred (shell.sent == []) after both clean EOF and reader error, plus a positive control that writes still work while the reader is alive. Mutation-checked: reverting to the _shell/_closed check makes it fail.
| ) -> ReapReport: | ||
| """Delete BenchFlow-managed runtimes older than *max_age_minutes*.""" | ||
| report = ReapReport() | ||
| cutoff = (now or datetime.now(UTC)) - timedelta(minutes=max_age_minutes) |
There was a problem hiding this comment.
[P1] Reject non-positive cleanup ages before computing the cutoff
The public CLI accepts any integer, and a negative value moves cutoff into the future. A reproduction with a runtime timestamp equal to now and max_age_minutes=-1 selected that fresh managed runtime for deletion. Without --dry-run this reaches delete_agent_runtime. Constrain the Typer option and validate again in this library entry point so programmatic callers cannot turn a sign mistake into destructive cleanup.
There was a problem hiding this comment.
Fixed in 39e2f09f7. Guarded in both places, as you asked.
reap_stale_runtimes() raises ValueError for a negative age before computing the cutoff, so programmatic callers cannot turn a sign mistake into destructive cleanup, and the Typer option is constrained with min=0:
Invalid value for '--max-age': -5 is not in the range x>=0.
Live: ### P1-7 negative age rejected: True. A test also asserts delete_agent_runtime is never reached.
There was a problem hiding this comment.
Still open through the deprecated alias. The new bench sandbox cleanup option rejects negatives, but bench environment cleanup delegates directly to the shared function without the constraint.
Live safe repro on this head: bench environment cleanup --max-age -1 --dry-run exited 0, found 51 Daytona sandboxes, and marked three currently STARTED sandboxes as deletion candidates. The output explicitly says to omit --dry-run to delete them. Please validate centrally in sandbox_cleanup() (and constrain the hidden alias) so every entry point protects Daytona as well as AgentCore.
There was a problem hiding this comment.
Fixed in 2b69b4b62. This one was my regression — I guarded the new command and left the deprecated alias reaching the same destructive code.
Both bench sandbox cleanup and bench environment cleanup now carry min=0, and more importantly the guard moved into reap_stale_sandboxes itself, so every age knob (max_age_minutes, failed_max_age_minutes, min_idle_minutes) rejects negatives before the cutoff is computed. A CLI-only fix would have left every programmatic caller exposed.
$ bench environment cleanup --max-age -1 --dry-run
Invalid value for '--max-age': -1 is not in the range x>=0.
Tests cover the library guard, all three knobs, and both CLI spellings.
| 'SIZE=$(docker image inspect -f "{{.Size}}" $BF_IMAGE_URI)', | ||
| "MB=$((SIZE / 1048576))", | ||
| 'echo "benchflow: image size ${MB} MB"', | ||
| f'if [ "$MB" -gt {provisioning.MAX_IMAGE_MB} ]; then ' |
There was a problem hiding this comment.
[P2] Compare raw bytes so CodeBuild enforces the same 2 GiB cap
Integer division floors the size before the comparison. An image of 2 GiB + 1 byte produces MB=2048, passes this -gt 2048 gate, and is pushed even though image_size_error() correctly rejects the same size. The current test checks marker/order but not boundary parity. Compare SIZE against MAX_IMAGE_MB * 1048576 and add exact-cap plus cap-plus-one cases.
There was a problem hiding this comment.
Fixed in 39e2f09f7. The floor was a real hole — cap-plus-one-byte produced MB=2048 and passed a -gt 2048 gate.
The buildspec now compares raw bytes against MAX_IMAGE_MB * 1024 * 1024, so the remote gate and image_size_error() agree exactly. Added the boundary cases: the cap passes, cap+1 fails, and both gates are asserted to agree on that boundary.
There was a problem hiding this comment.
The remote byte gate is fixed, but the local-Docker gate still fails open. If docker image inspect -f {{.Size}} fails or returns non-numeric output, _reject_oversized() logs and returns; build_and_push() then logs in and pushes an unmeasured image.
A deterministic current-head repro reached the push path after an inspect failure, and no test invokes this branch. Please raise a startup error and assert login/push are not called when size cannot be established.
There was a problem hiding this comment.
Fixed in 2b69b4b62. The local gate is now fatal: if docker image inspect fails or returns something non-numeric, the push is refused with a message naming the cap, rather than proceeding with an unmeasured image that would resurface later as an opaque runtime error.
GitHub Actions
Inactive
2 P1 + 4 P2 + 1 P3 fixed —
|
| Sev | Finding | Fix | Mutation-checked |
|---|---|---|---|
| P1 | Failed renewal throttled as success | Predicate is pure; mark_lease_renewed() only after the write lands |
✅ |
| P1 | Reserved paths overwritten then deleted | Refused before identity/build; originals untouched | ✅ |
| P2 | Substring channel match | Exact match on the final enum component | ✅ |
| P2 | Writes accepted after reader EOF | writeline() gates on is_running |
✅ |
| P2 | Failure diagnostics blocked the loop | _build_failure via asyncio.to_thread |
✅ |
| P2 | Context-delete failure hidden at DEBUG | WARNING with bucket/key/error, build result preserved | — |
| P3 | Missing PR identifiers, mutation-blind tests | PR #937 added; blind cases rebuilt |
— |
The P3 was the most useful finding
You were right that the framing fixture proved nothing — it would have passed against the superseded scheme. It now pins an input pair that genuinely collided under the old framing (a shim containing the old \0shim\0 separator re-splits into a different dockerfile/shim pair) and asserts that collision explicitly before asserting the new framing separates them.
Because of that, I mutation-checked five guards this round instead of trusting green — half-close write, exact channel match, event-loop blocking, lease record-before-write, and reserved-path. All five fail against the reverted behavior. Two of my own checks were themselves broken on the first attempt (a bad anchor, and a mutation that still raised); I fixed both before reporting.
Live re-verification
### P1-reserved refused: True | original preserved: True
### P1-renewal fatal: True | throttle untouched: True
### P1-renewal retried after recovery: True
### exec still healthy: True
Gates
5070 passed, 10 skipped, 7 deselected · ruff · format · ty check src/ · git diff --check — clean.
Unchanged limitations
- Lease renewal is rollout-driven, not a background task.
- Large directory transfers still stream base64 through
exec. - Not a general Daytona replacement: compose, no-network, snapshots, >2 GiB.
- #935 stays open (
Refs). - No non-author approval at this head.
One design choice worth flagging: for reserved paths I chose refusal over backup-and-restore. A restore path has its own failure mode — a crash between write and restore still loses the file — and these two names are ours by contract. If you would rather they be namespaced out of the task tree entirely (e.g. built from a staged copy), that is a larger change I am happy to make instead.
bingran-you
left a comment
bingran-you
left a comment
There was a problem hiding this comment.
Exact-head follow-up review at 4ede269c43b5480657867c03b5db9ded8ab2f4db. I re-ran the original seven reproductions: those direct fixes hold. The new blockers below are adjacent contracts the patch still leaves open; full live E2E and disposition summary follow in the PR conversation.
| max(lifecycle["maxLifetime"], lifecycle["idleRuntimeSessionTimeout"]) | ||
| ) | ||
| now = time.monotonic() | ||
| if not provisioning.lease_needs_renewal(self.runtime_arn, window, now): |
There was a problem hiding this comment.
[P1] The retry fix is correct sequentially, but the lease is still unsafe at the throttle boundary and under concurrency. With window=28_800, a rollout at t=7_199 is throttled, may live until t=35_999, yet the tag written at t=0 expires at t=28_800; at t=28_801 the reaper selected that still-legal session. Separately, 20 concurrent due _renew_lease() calls produced 20 tag_resource calls because check/write/mark spans an await without single-flight; create/adopt also writes without marking, so the first rollout fan-out is immediately due again. Please make the expiry cover window + renewal_interval, serialize per ARN with a second due check, and mark successful create/adopt writes. Add boundary, gather() fan-out, and fail-first/retry tests.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: lease expiry now covers the service window plus renewal interval; renewal is single-flighted per runtime ARN with a second due check inside the lock; successful create/adopt writes mark the lease, while failed writes remain retryable. Boundary, concurrent gather() fan-out, and fail-first/retry regressions are included.
| context = request.context_dir | ||
| shim = context / provisioning.GENERATED_SHIM | ||
| dockerfile = context / provisioning.GENERATED_DOCKERFILE | ||
| shim.write_text(request.shim_text) |
There was a problem hiding this comment.
[P1] Refusing regular reserved files is reasonable, but writing fixed scaffolding into the caller's tree is still unsafe. A dangling reserved-name symlink passes Path.exists(), then write_text() follows it and creates/clobbers an external target; cleanup removes only the symlink. Two synchronized OS processes also overwrite each other's fixed Dockerfile/shim and one cleanup removes the other's inputs, so a build can use bytes that do not match its digest or fail mid-build. A second-write failure additionally leaves the shim behind because the try starts too late. Please materialize each build in an isolated, unique staging context (a process-local lock is insufficient), and test dangling symlinks, partial writes, and multiprocessing overlap.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: every build is materialized in a unique temporary staging context and the caller tree is never mutated. Reserved symlinks are rejected without following them; partial staging failures clean their isolated directory. Multiprocessing overlap, dangling-symlink, caller-immutability, and partial-write regressions are included.
| if self.task_env_config.docker_image: | ||
| base = f"FROM {self.task_env_config.docker_image}\n" | ||
| else: | ||
| base = (self.environment_dir / "Dockerfile").read_text() |
There was a problem hiding this comment.
[P1] The task Dockerfile can still escape the context through a symlink. _validate_definition() accepts a symlinked Dockerfile, and this read_text() follows it; an external marker then appeared in the generated Dockerfile and CodeBuild ZIP even though the canonical walker skips symlinks. Reject symlinked/non-regular Dockerfiles when no docker_image is configured and read with a no-follow/containment-safe path. The regression should prove external bytes never reach the generated Dockerfile or archive.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: a task Dockerfile must be a regular non-symlink file before it is read. The regression uses an external marker and proves those bytes reach neither the generated Dockerfile nor the archive.
| the CodeBuild upload, which is a credential-exposure bug because that | ||
| archive goes to S3. | ||
| """ | ||
| cleaned = pattern.strip().strip("/") |
There was a problem hiding this comment.
[P1] This is still not Docker-compatible enough for a secret-bearing CodeBuild upload. Against Docker 29.3.0, all three cases were excluded by Docker but included by BenchFlow/its ZIP: foo/../secret.env (Docker path cleaning), \!secret.env matching a literal !secret.env (backslash escaping), and Dockerfile.benchflow-agentcore.dockerignore: secret.env (Dockerfile-specific ignore precedence). Please replace the growing custom matcher with a tested Docker-compatible implementation and assert archive bytes are absent for each case. Also preserve directory entries: adding empty/ currently changes the real image but not the digest or ZIP.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: the custom matcher was replaced with pathspec, with Docker-style path cleaning, escaped !/#, Dockerfile-specific ignore precedence, negation, and empty-directory preservation. The same canonical staged tree drives the digest and CodeBuild ZIP, including executable modes and directory entries.
| ) | ||
| await self._await_marker(marker) | ||
| self._clear_buffered_output() | ||
| await self._shell.send(launch + "\n") |
There was a problem hiding this comment.
[P2] Startup still bypasses the new half-close guard. In a full fake-shell run that echoed the marker and then cleanly EOF'd, start() returned successfully with _reader_done=True and still sent exec bash -lc ... through this direct _shell.send. Route the launch through the liveness-checked send path and raise TransportClosedError with remote_session_killed; the current writeline() RuntimeError classifies as other and bypasses transport retry. Add a marker-then-EOF start() regression asserting no launch send.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: marker and launch writes both go through the liveness-checked _send; a marker-then-EOF startup raises TransportClosedError(remote_session_killed) and sends no launch. The real AWS PTY test also exposed command-echo and standalone bracketed-paste noise; exact marker matching and ANSI-only line filtering now pass live.
| back into the agent log. | ||
| """ | ||
| remote_path = f"/tmp/.benchflow_agent_env_{uuid.uuid4().hex[:16]}" | ||
| body = "".join(f"export {k}={shlex.quote(v)}\n" for k, v in env.items()) |
There was a problem hiding this comment.
[P2] AgentCore still accepts invalid env names. BAD.KEY generates an invalid export; sourcing fails and, because cleanup is the next && segment, the mode-0600 secret file remains in /tmp. Share the canonical POSIX identifier validation used by the other backends and make deletion unconditional (for example a shell trap). Test both refusal and cleanup without exposing the secret in terminal traffic.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: AgentCore reuses the canonical POSIX env-key validator before staging. The launch is an isolated subshell with an EXIT trap armed before cd/source, removes the file immediately after source, and best-effort cleans both partial staging and failed launch paths without typing secrets into the PTY.
| key = f"contexts/{uuid.uuid4().hex}.zip" | ||
| archive = await asyncio.to_thread(self._package, request) | ||
| await asyncio.to_thread(self._ensure_bucket) | ||
| await asyncio.to_thread( |
There was a problem hiding this comment.
[P2] Two remote cleanup paths remain open. If put_object succeeds server-side but the response is lost, this call raises before entering finally, so the credential-bearing archive is never deleted. If _run_build() times out or is cancelled, it raises without stop_build, so the paid build can continue and later push after BenchFlow has failed (and after the archive cleanup races it). Put the upload attempt inside the cleanup scope; on timeout/cancellation best-effort stop_build before deleting the context, while preserving the original error. Add ordering and ambiguous-upload tests.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: the upload attempt is inside the cleanup scope, ambiguous upload failures still delete the S3 key, and timeout/cancellation best-effort stops the CodeBuild build before archive deletion while preserving the original exception. Cleanup ordering/cancellation tests are included; a live 8-process CodeBuild/S3 canary also found and verified bounded jittered retries for concurrent bucket-hardening conflicts.
| """Guards PR #937: typed channels are matched exactly, not by substring.""" | ||
|
|
||
| @pytest.mark.parametrize("name", ["NOT_STDOUT", "STDOUT_METADATA", "METRICS"]) | ||
| def test_colliding_typed_names_are_not_stdout(self, name): |
There was a problem hiding this comment.
[P3] The regression-docstring claim is not fully satisfied: a class docstring does not populate each test function's __doc__, so it does not meet AGENTS.md's per-regression requirement. This and the adjacent regression functions at lines 199, 204, 225, 241, and 252 need their own Guards PR #937 docstrings; the same issue remains in test_agentcore_parallel.py at 953, 971, 978, 998, and 1081. The current suite stays green if all class-level attribution is removed, which is exactly why the convention is function-level.
There was a problem hiding this comment.
Fixed on current head 2f4de9f9c: each regression function now has its own Guards PR #937 docstring, and the reaper cases were split into test_agentcore_reaper.py so every new source/test module remains below the thermo-review 1,000-line threshold.
|
Re-reviewed exact head Live E2E at this headPinned SkillsBench
Both malformed AgentCore runs were nevertheless reported Remaining blockers
Gates / cleanup
Original four requirements
Labels remain |
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
GitHub Actions
Inactive
bingran-you
added
status:ready
Final thermo-nuclear review — exact head
|
| Layer | Evidence |
|---|---|
| Local full gate | 5098 passed, 19 skipped, 7 deselected; ty check src/, ruff lint, ruff format, and git diff --check all clean |
| GitHub | default-dependency suite 5106 passed, 11 skipped, 7 deselected; test, pip-audit, manifest parity, real rollout smoke, agent-judge, live oracle/Daytona parity, and reaper safety all green |
| AWS lifecycle without Docker | CodeBuild Graviton build/push, ECR digest, runtime create/adopt, command stdout/stderr/exit-code fidelity, nested upload/download, interactive shell, env/cwd/stdin/stdout, session stop all passed |
| Concurrency/cleanup | 8/8 independent concurrent CodeBuild/S3 processes succeeded; latest three CodeBuild runs are SUCCEEDED in every phase; build-context bucket is empty; cleanup dry-run scanned 3 managed runtimes and correctly protected all 3 active leases (deleted=0, errors=0) |
| Real model canaries | OpenHands + Azure Foundry gpt-5.5, AgentCore, real citation-check, both task-skill arms reward 1 with complete artifacts and provider usage |
The model canaries ran on production-code head 4cebc12ce; current head changes after that are only the optional-SDK test fixture and one cleanup CLI help string—no AgentCore runtime code changed.
- with task skill: reward 1; 15/15 LLM exchanges HTTP 200/completed; every response records
reasoning.effort=xhigh; 606,773 tokens; onecitation-managementinvocation; 9/9 verifier tests. - without task skill: reward 1; 13/13 HTTP 200/completed; every response records
reasoning.effort=xhigh; 362,155 tokens;citation-managementabsent and never invoked/read; 9/9 verifier tests. - The artifact validator marked both rollouts healthy with complete ACP/LLM trajectories, results, reward, timing, and provider usage. Two independent raw-trajectory audits found no verifier/oracle access, git-history leakage, answer lookup, test/reward mutation, monkey-patching, direct score writing, or other reward hacking.
- Precision caveat: the no-task-skill run still sees OpenHands' 55 unrelated global skills and the
invoke_skilltool, though it invokes none. It proves task-specific skill isolation, not a literal zero-skill-capability harness. This is existing OpenHands behavior, not an AgentCore regression.
Can AWS replace Daytona?
Yes for the supported subset: public-network, single-container, Linux/ARM64 tasks whose image is below AgentCore's hard 2 GB limit and whose transfers fit the bounded exec path. It needs AWS credentials plus the runtime and CodeBuild roles; it does not need a Daytona key or local Docker.
No as a general drop-in replacement: compose/multi-service tasks, enforced no-network, snapshots, images over 2 GB, and large-transfer workloads remain unsupported and are rejected/documented rather than silently degraded. Use Docker, Daytona, or Modal for those cases. Issue #935 therefore remains open by design for product confirmation, SDK linkage, isolation policy, and large-file-transfer design.
All eight review threads have current-head replies. Labels are now status:ready and review:pending; the only remaining merge gate is the required non-author human approval.
1 participant
Adds
--sandbox agentcore, running each rollout in an Amazon Bedrock AgentCore Runtime microVM.Refs #935 — the issue stays open. This lands a working backend for a useful subset (public-network, single-container arm64 tasks), but the issue's SDK-link, product-confirmation, isolation-contract, and large-file-transfer questions are not resolved here, and AgentCore cannot replace Daytona for compose, no-network, snapshots, or images over 2 GB.
The issue proposed reading a pre-made runtime ARN out of
BENCHFLOW_AGENTCORE_RUNTIME_ARN. That cannot run SkillsBench tasks: they ship their ownDockerfile, and a fixed ARN means every task silently executes in whatever image that one runtime was baked from — the verifier then fails for reasons unrelated to the agent. This PR provisions per task instead (build → ECR → runtime → isolated sessions, with lease-aware cleanup), so the task's declared environment is honored.PR-A (refactor) and PR-B (feature) are combined here at the author's request.
Refactor — behavior-preserving
Transport selection was an
if environment == "..."chain in the ACP layer that re-derived the backend from a string while already holding the sandbox object, and sniffed private attributes (env._strategy) to tell Daytona's variants apart. Backends now answerBaseSandbox.live_process(agent=...)themselves, so the chain collapses to one call and a new backend adds no branch there.This makes an existing latent bug explicit: Modal fell through that chain's
elseand was handed aDaytonaProcess, failing deep inside Daytona SSH setup with an unrelated error. It now raises an actionableNotImplementedError.sandbox/process.pyhad reached 1026 lines; it becomes a package (import paths unchanged).LiveProcessis now a pure ABC, with the local-subprocess implementation inSubprocessLiveProcess. That split is exactly what the WebSocket transports need —DaytonaPtyProcessalready had to neutralize the base class with_process = None # Not used, and AgentCore is the second such transport.Per-backend capability facts move onto the provider registry, so the no-network gate reads
enforces_no_networkrather than growing asandbox == "<name>"special case per backend that cannot isolate. Also removes a stale hand-copied provider list from a CLI docstring (the registry existed to prevent exactly that drift).Three platform behaviors found by experiment, not docs
READY, then everyInvokeAgentRuntimeCommandfails with a 500. ServingGET /pingon :8080 fixes it. Task images know nothing about AgentCore, so BenchFlow appends a stdlib-only responder as the image entrypoint.READYdescribes the runtime definition, not a running session. The first command on a cold, never-pulled image 500s while the container boots, then succeeds. Warm-up retries with backoff, so that race cannot surface as a scored-0 rollout.DaytonaPtyProcess, handled the same proven way (raw/no-echo, nonce marker sync, CR stripping).Two more measured facts shaped the code:
contentDeltakeepsstdout/stderrseparate (soExecResultdoes too), andexecandopen_shellshare one session filesystem (which is what lets the kernel stage files and the verifier read results around an agent living on the WebSocket).Gated rather than faked
BaseSandboxdefault raises.mainservices rejected.network_mode = "no-network"—networkConfiguration.networkModeaccepts onlyPUBLICorVPC. There is no isolated mode, so no-network tasks are refused rather than run unisolated.SandboxStartupError, so capacity problems stay attributable as infrastructure instead of being recorded as a failed command.Live validation
Real SkillsBench tasks via
bench eval run --sandbox agentcore --agent claude, against the live service:3d-scan-calcecon-detrending-correlationearthquake-plate-calculationerrors=0throughout; each task got its own runtime and cleaned it up. Image/runtime reuse measured:environment_setup58.9s cold → 16.6s on repeat. The one failure is the right failure mode — the harness worked, the model got the answer wrong.Also live-verified: cold never-pulled image (new base + 40 MB layer), exit-code fidelity (
rc=3), stdout/stderr separation, secret env injection, nestedupload_dir/download_dirround trip.Gates
Exact head
3d91c71619e03aedfd4ffb9ce026099a21f8c70b: local5098 passed, 19 skipped, 7 deselected;ruff check,ruff format --check,ty check src/, andgit diff --checkclean. GitHub default-dependency suite:5106 passed, 11 skipped, 7 deselected; test, pip-audit, manifest parity, real rollout smoke, agent-judge, live oracle/Daytona parity, and reaper-safety gates all green.Tests relocated rather than deleted: the six per-backend transport cases moved from
tests/test_acp.pytotests/test_sandbox_live_process.py, each keeping the PR regression it guards (#896, #921, #936) named in its docstring. The ACP layer keeps one test for the delegation contract it still owns.Reviewer notes
BENCHFLOW_AGENTCORE_ROLE_ARN(a role assumable bybedrock-agentcore.amazonaws.comthat can pull ECR and write CloudWatch logs), plusbedrock-agentcore:*and ECR push on the caller. Docs indocs/running-benchmarks.md.linux/arm64only.open_shell()harness-ARN limitation does not apply: the SDK requires aruntime/ARN, which is what per-task provisioning creates.BENCHFLOW_AGENTCORE_LIVE_TEST=1) so CI stays offline.