Milestone 6: isolated execution and artifacts - #7
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughMilestone 6 adds lease-scoped sandbox execution, egress controls, programmatic tool bridging, durable artifact storage, artifactized tool output, lifecycle cleanup, security gates, and CircleCI coverage. ChangesSandbox execution and artifacts
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (30)
src/agent_core/application/artifact_writer.py-95-118 (1)
95-118: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftA crash between the store write and the metadata commit leaks an orphaned object.
The code writes content to the store at line 95, then persists metadata at line 114. The
except BaseExceptionhandler removes the object when the commit raises. It cannot help when the process dies between the two steps. The result is a file on disk with noartifactsrow.Nothing reclaims that file. The expiry sweep enumerates metadata rows, so an object without a row is invisible to it, and the tenant-scoped storage key cannot be rediscovered from the metadata side.
The PR objectives list artifact commit ordering as an ADR-0042 topic. Please confirm the ADR records this window, and add a reconciliation path that walks the store root and deletes objects that have no metadata row and are older than a safety margin.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/application/artifact_writer.py` around lines 95 - 118, The artifact write flow around _store.put and artifacts.create can leave orphaned objects after a process crash; update ADR-0042 to document this commit-ordering window and add reconciliation that walks the store root, resolves each object against artifact metadata, and deletes only unmatched objects older than a defined safety margin, while preserving tenant scoping and existing cleanup on commit failure.src/agent_core/adapters/persistence/memory.py-953-953 (1)
953-953: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDefine one meaning for
expires_at is Noneacross the artifact lifecycle. WideningArtifactRef.expires_attodatetime | Noneadded anis not Noneguard at each call site, but no site agrees on what a missing expiry means.PublicArtifactService._get_reftreats it as "never expires" and serves the artifact.TrajectoryExportService.readtreats it as "not found". The repository sweep and withdrawal paths treat it as "not eligible", so such a row is never reclaimed.artifact_to_domainrejects it as invalid. The combination creates a row that is readable, un-sweepable, and blocks re-export of its run.
src/agent_core/adapters/persistence/memory.py#L953-L953: inexpire_for_principal, setexpires_at = expired_atwhen it isNone, so consent withdrawal always makes the artifact due for the sweep.src/agent_core/application/trajectory_service.py#L195-L197: treat aNoneexpiry as "does not expire" and return the existing artifact, instead of raisingExportStateErrorfor a row the sweep will never remove.Pick the invariant first. If no artifact may ever lack an expiry, keep
expires_atnon-optional onArtifactRefand drop the guards. If a non-expiring tier is intended, record it in ADR-0042 and apply the same reading at every site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/persistence/memory.py` at line 953, Adopt the non-expiring interpretation for missing artifact expiry and apply it consistently: in src/agent_core/adapters/persistence/memory.py lines 953-953, update expire_for_principal so None is set to expired_at and becomes sweep-eligible; in src/agent_core/application/trajectory_service.py lines 195-197, update TrajectoryExportService.read to return the existing artifact when expires_at is None instead of raising ExportStateError. Ensure PublicArtifactService._get_ref and other lifecycle paths retain the same interpretation, and document the invariant in ADR-0042.src/agent_core/tools/executor.py-870-898 (1)
870-898: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe artifact is truncated at
hard_ceiling, but the marker calls it the full output.Line 871 slices
renderedtohard_ceiling, which is four timesmaximum_output_bytes. Only those bytes are streamed into the artifact. The marker at line 898 then tells the modelfull output: artifact:<id>.For any output larger than
hard_ceiling, that statement is wrong and the excess bytes are discarded with no record. Theelidedcount at line 897 measures the gap againstlen(rendered), so it silently reports discarded bytes as if they were preserved in the artifact.Pick one behavior and make the text match it:
- Store the entire
renderedpayload, and rely on the writer'smaximum_bytescap to reject genuinely oversized output.- Keep the ceiling, but record the stored size in the metrics and word the marker as a partial capture, for example
first N bytes: artifact:<id>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 870 - 898, Align artifact storage and marker semantics in the output handling around stream, writer.create, and marker: either stream the complete rendered payload so the artifact is genuinely full, or retain hard_ceiling while labeling the artifact as a partial first-N-byte capture and reporting the discarded size through the existing metrics. Ensure elided reflects bytes omitted from the displayed output without claiming discarded bytes are available in the artifact.src/agent_core/tools/executor.py-678-693 (1)
678-693: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftThe bridge retry loop polls the database until the tool timeout elapses.
When a bridged tool call requires approval,
_dispatch_oneraisesApprovalRequiredError, and this loop sleeps 200 ms and retries without a bound. Each iteration performs several database round trips:find_by_idempotency_key,approvals.get_by_action, and a policy evaluation.
asyncio.timeout(tool.spec.timeout_seconds)at line 776 wrapstool.execute, so the loop does terminate. It terminates by exhausting the sandbox tool's entire timeout at 5 iterations per second. A human approval will almost never arrive inside a tool timeout, so the common outcome is a sustained database poll followed by a timeout failure.Two changes would help:
- Apply exponential backoff with a cap instead of a fixed 200 ms interval.
- Return a refusal response to the sandbox as soon as an approval is required, so the script can react rather than block. The bridge response schema already carries
status,reason_code, andretryable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 678 - 693, Update the ApprovalRequiredError handling in the bridge retry loop around _dispatch_one to return an immediate refusal response to the sandbox using the existing status, reason_code, and retryable fields, instead of continuing to poll until tool.spec.timeout_seconds expires. If retry behavior remains necessary, replace the fixed asyncio.sleep(0.2) with exponential backoff capped at a bounded maximum, while preserving cancellation handling and the existing successful dispatch path.src/agent_core/tools/executor.py-1257-1261 (1)
1257-1261: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA malformed
artifact_idfrom a tool crashes the run.
UUID(raw_artifact_id)is unguarded.result.artifactsis tool-supplied data, and any tool, including a remote MCP tool, can return a non-UUID string.
_finishruns at line 841, outside thetry/exceptblock that normalizes tool exceptions intoToolFailurevalues. AValueErrorraised here therefore escapes the pipeline and fails the whole run with an internal error, instead of failing the single tool call.Ignore an unparsable value and continue.
🛡️ Proposed change
artifact_id: UUID | None = None if result.artifacts and isinstance(result.artifacts[0], dict): raw_artifact_id = result.artifacts[0].get("artifact_id") if isinstance(raw_artifact_id, str): - artifact_id = UUID(raw_artifact_id) + try: + artifact_id = UUID(raw_artifact_id) + except ValueError: + logger.warning( + "tool_artifact_id_malformed", + extra={"tool_name": tool.spec.name}, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 1257 - 1261, Update the artifact ID extraction in the executor’s _finish flow to catch UUID parsing failures from tool-supplied artifact_id strings. Ignore invalid values by leaving artifact_id as None, while preserving valid UUID conversion and allowing the tool result to continue through normal failure handling.src/agent_core/application/artifact_writer.py-143-153 (1)
143-153: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
for_rundropsretention_daysandmaximum_bytes, so neither is configurable.
BoundArtifactWriteracceptsretention_days=30andmaximum_bytes=512 * 1024 * 1024.ArtifactWriterFactorynever forwards them, so every writer uses the hardcoded values and deployment configuration cannot change retention or the size cap.The size cap is also duplicated.
FilesystemArtifactStore.__init__declares its ownmaximum_bytesdefault of512 * 1024 * 1024. If an operator lowers the store cap, the writer still spools the full payload first and the mismatch surfaces as a lateArtifactIntegrityErrorfromputrather than an early rejection.Accept both values on the factory and forward them, and source them from settings alongside
artifact_root.♻️ Proposed change
class ArtifactWriterFactory: def __init__( self, uow_factory: UnitOfWorkFactory, store: ArtifactStore, clock: Clock, ids: IdFactory, + *, + retention_days: int = 30, + maximum_bytes: int = 512 * 1024 * 1024, ) -> None: self._uow_factory = uow_factory self._store = store self._clock = clock self._ids = ids + self._retention_days = retention_days + self._maximum_bytes = maximum_bytes @@ session_id=session_id, run_id=run_id, origin=origin, + retention_days=self._retention_days, + maximum_bytes=self._maximum_bytes, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/application/artifact_writer.py` around lines 143 - 153, Update ArtifactWriterFactory.for_run to accept retention_days and maximum_bytes, source both values from settings alongside artifact_root, and forward them when constructing BoundArtifactWriter. Ensure FilesystemArtifactStore and the writer use the same configured maximum_bytes so oversized payloads are rejected before full spooling rather than failing later in put.src/agent_core/tools/executor.py-892-898 (1)
892-898: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe excerpt can exceed
maximum_output_bytes.
head_bytesis 60 percent of the budget andtail_bytesis 20 percent. Together with the marker, the resultingTextPartcan be larger than the budget the function exists to enforce. The marker alone is roughly 70 bytes. For a smallmaximum_output_bytes, the excerpt overshoots, and nothing re-checks the size after themodel_copy.Subtract the marker length from the budget before you split it between head and tail, and clamp the total.
🐛 Proposed change
budget = tool.spec.maximum_output_bytes - head_bytes = int(budget * 0.6) - tail_bytes = int(budget * 0.2) + marker_template = f"\n[... 0 bytes elided; full output: artifact:{ref.artifact_id} ...]\n" + excerpt_budget = max(0, budget - len(marker_template.encode("utf-8"))) + head_bytes = int(excerpt_budget * 0.75) + tail_bytes = excerpt_budget - head_bytes head = artifact_bytes[:head_bytes].decode("utf-8", errors="ignore") tail = artifact_bytes[-tail_bytes:].decode("utf-8", errors="ignore")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 892 - 898, Update the artifact excerpt sizing around maximum_output_bytes so the rendered TextPart, including the elision marker, never exceeds the budget. Reserve space for the marker before dividing the remaining budget between head and tail, clamp the available total for small budgets, and recheck or constrain the final assembled output after the model_copy path using the existing symbols budget, head_bytes, tail_bytes, and marker.src/agent_core/adapters/persistence/memory.py-1007-1009 (1)
1007-1009: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDisable sandbox reaping for in-memory storage or report active runs.
InMemoryMaintenanceRepository.live_run_leases()always returns an empty set, while the in-memory composition wiressandbox_manager.reapintoMaintenanceWorker. The fake reaper removes every environment whose(run_id, lease_epoch)is absent, so maintenance can destroy a sandbox used by an active in-process run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/persistence/memory.py` around lines 1007 - 1009, The in-memory composition must not let MaintenanceWorker reap active sandboxes: update InMemoryMaintenanceRepository.live_run_leases and its sandbox_manager.reap wiring to either report every active run’s (run_id, lease_epoch) or disable sandbox reaping for in-memory storage. Preserve normal maintenance behavior for non-active environments and ensure active in-process runs remain protected.src/agent_core/tools/bridge.py-120-136 (1)
120-136: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle the
StreamReaderline-limit error and unexpected dispatch errors.Two failure paths escape the
tryblock:
reader.readline()raisesValueErrorwhen a line exceeds theStreamReaderlimit, which defaults to 64 KiB. The in-sandbox caller can send an oversized line. The intended 64 KiB rejection at Line 54 never runs, and the handler aborts before the client receives a response.self._session.handlepropagates any non-BridgeProtocolErrorexception raised bydispatch. The client then blocks on a read that never completes until the socket closes.Catch both and return a denial frame.
🛠️ Proposed fix
async def _handle(self, reader: asyncio.StreamReader, writer: asyncio.StreamWriter) -> None: try: - while request := await reader.readline(): - try: - response = await self._session.handle(request) - except BridgeProtocolError as exc: - response = json.dumps( - { - "status": "denied", - "reason_code": "bridge.protocol_error", - "retryable": False, - "result": {"message": str(exc)}, - }, - separators=(",", ":"), - ).encode("utf-8") + while True: + try: + request = await reader.readline() + except ValueError: + writer.write(_denial("bridge.request_too_large") + b"\n") + await writer.drain() + return + if not request: + return + try: + response = await self._session.handle(request) + except BridgeProtocolError as exc: + response = _denial("bridge.protocol_error", str(exc)) + except Exception: + logger.exception("bridge_dispatch_failed") + response = _denial("bridge.internal_error") writer.write(response + b"\n") await writer.drain() finally: writer.close() await writer.wait_closed()Add the
_denialhelper and a module logger, and do not include exception detail for the internal-error case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/bridge.py` around lines 120 - 136, Update _handle to catch StreamReader.readline() ValueError for oversized lines and unexpected exceptions from self._session.handle, returning a denial frame for both cases instead of aborting. Add the _denial helper and module logger requested by the review, use the existing 64 KiB protocol rejection for oversized input, and return an internal-error denial without exposing exception details; log unexpected dispatch exceptions through the module logger.src/agent_core/tools/bridge.py-107-111 (1)
107-111: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClose the window where the bridge socket is not mode 0600.
asyncio.start_unix_servercreates and binds the socket at Line 110. Thechmodruns afterwards at Line 111, and it is additionally deferred to a worker thread. Between bind andchmod, the socket permissions follow the process umask. A local process can connect during that window and attempt bridge calls. Set the umask around the bind, or create the parent directory with mode 0700 so the socket is unreachable regardless of its own mode.🔒 Proposed fix
async def start(self) -> None: self._socket_path.parent.mkdir(parents=True, exist_ok=True) + os.chmod(self._socket_path.parent, 0o700) self._socket_path.unlink(missing_ok=True) - self._server = await asyncio.start_unix_server(self._handle, path=self._socket_path) - await asyncio.to_thread(os.chmod, self._socket_path, 0o600) + previous_umask = os.umask(0o177) + try: + self._server = await asyncio.start_unix_server(self._handle, path=self._socket_path) + finally: + os.umask(previous_umask) + os.chmod(self._socket_path, 0o600)Note that
os.umaskis process-global. If the sandbox lifecycle starts bridges concurrently in one process, prefer the directory-mode approach alone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/bridge.py` around lines 107 - 111, Update Bridge.start so the Unix socket is protected from creation through chmod, eliminating the bind-to-chmod window. Prefer ensuring the socket’s parent directory is created and retained with mode 0700, avoiding process-global os.umask changes when bridges may start concurrently; keep the final socket chmod as needed.src/agent_core/tools/bridge.py-62-64 (1)
62-64: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEncode both token values before comparison. A non-ASCII request token raises
TypeErrorinhmac.compare_digest, escapes_handle, and aborts the connection instead of returning an unauthorized response. Compare UTF-8 bytes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/bridge.py` around lines 62 - 64, Update the token validation in _handle to encode both the request token from loaded.get("token") and self._token as UTF-8 bytes before passing them to hmac.compare_digest, while preserving the existing type check and unauthorized response for invalid or mismatched tokens.src/agent_core/tools/artifact_export.py-68-69 (1)
68-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard against missing workspace and artifact writer collaborators.
castperforms no runtime check. Ifcontext.workspaceorcontext.artifactsisNone, line 71 or line 72 raisesAttributeError, which is not converted into aToolFailure.SandboxRunCommandTool._commandinsrc/agent_core/tools/sandbox_run_command.pyat Line 116 checkscontext.workspace is Nonebefore the cast. Apply the same guard here for both collaborators.🛡️ Proposed guard
- workspace = cast(WorkspaceHandle, context.workspace) - writer = cast(ArtifactWriter, context.artifacts) + if context.workspace is None or context.artifacts is None: + return ToolResult( + ok=False, + content=[], + failure=ToolFailure( + kind=ToolFailureKind.PRECONDITION_FAILED, + reason_code="tool.workspace_unavailable", + detail="artifact export requires a workspace and an artifact writer", + retryable=False, + ), + ) + workspace = cast(WorkspaceHandle, context.workspace) + writer = cast(ArtifactWriter, context.artifacts)Confirm the exact
ToolFailureKindmember name available inagent_core.domain.tools.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/artifact_export.py` around lines 68 - 69, In the artifact export flow before the casts to WorkspaceHandle and ArtifactWriter, validate that both context.workspace and context.artifacts are not None, following the guard pattern in SandboxRunCommandTool._command. When either collaborator is missing, raise or return a ToolFailure using the exact available ToolFailureKind member from agent_core.domain.tools, preventing later AttributeError calls.src/agent_core/tools/artifact_export.py-20-20 (1)
20-20: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftStream workspace files in bounded chunks.
read_boundedmaterializes the complete file, and_one_chunkonly re-yields that buffer. This can retain up to 512 MiB per export, multiplied across concurrent runs. Add a chunked workspace-read API and pass itsAsyncIterator[bytes]directly toArtifactWriter.create.
read_boundedraisesWorkspaceReadLimitExceededErrorwhen the file exceeds_MAX_ARTIFACT_BYTES;ArtifactExportTool.executedoes not catch it. Map this exception to a bounded-outputToolFailure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/artifact_export.py` at line 20, Replace the full-buffer workspace read used by ArtifactExportTool.execute with a chunked workspace-read API returning AsyncIterator[bytes], and pass that iterator directly to ArtifactWriter.create instead of routing it through _one_chunk. Preserve the _MAX_ARTIFACT_BYTES limit, catch WorkspaceReadLimitExceededError in execute, and convert it into a bounded-output ToolFailure.src/agent_core/tools/sandbox_run_command.py-137-151 (1)
137-151: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude
context.call_idin thescript_hashinput.
Parallel executions of the same command produce identical bridge call IDs. The executor includes these IDs in its idempotency keys, so identical bridged calls can share one persisted invocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/sandbox_run_command.py` around lines 137 - 151, Update the script_hash input constructed in the bridge dispatch block of sandbox_run_command to include context.call_id alongside the command arguments and working directory. Ensure the serialized source changes for distinct call IDs while preserving deterministic JSON serialization before hashing and constructing ProgrammaticBridgeSession.src/agent_core/bootstrap.py-433-434 (1)
433-434: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict
sandbox_passthroughto approved non-secret names.
AGENT_SANDBOX_PASSTHROUGHaccepts arbitrary names. Its denylist omitsDATABASE_URL,AUTH_TOKEN, and arbitrary*_API_KEYvariables. Validate the setting against an explicit non-secret allowlist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/bootstrap.py` around lines 433 - 434, Update the bootstrap configuration handling around sandbox_passthrough to validate names against an explicit allowlist of approved non-secret environment variables before passing them to the sandbox. Reject or exclude DATABASE_URL, AUTH_TOKEN, arbitrary *_API_KEY variables, and any other names not on the allowlist, while preserving passthrough for approved names.src/agent_core/runtime/worker.py-183-188 (1)
183-188: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProtect active sandboxes from stale lease snapshots.
SandboxManager.reaptreatslive_leasesas an allowlist. If a worker acquires a lease afterlive_run_leases()returns, the reaper can destroy its unexpired sandbox. Synchronize lease acquisition with reaping or add a minimum sandbox age/grace period.
InMemoryMaintenanceRepository.live_run_leases()returns an empty set. The current in-memory UOW hasqueue=None, soMaintenanceWorkerfails before invoking the reaper. Do not wire sandbox reaping to memory until it tracks leases or explicitly disables the sweep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/runtime/worker.py` around lines 183 - 188, Update the MaintenanceWorker sandbox-reaping flow around live_run_leases and _sweep_sandboxes to prevent stale lease snapshots from deleting newly leased active sandboxes, using synchronized lease acquisition or an appropriate minimum sandbox age/grace period. Also keep in-memory execution from invoking reaping while InMemoryMaintenanceRepository returns no leases and the UOW has queue=None; explicitly disable or guard the sweep until lease tracking is available.src/agent_core/bootstrap.py-426-435 (1)
426-435: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject a non-
fakeSANDBOX_MECHANISMwith memory storage.validate_settingsdoes not validatestorage. Memory storage therefore selectsFakeExecutionEnvironmentfor configureddocker,gvisor, ormicrovm, including production configurations. The effective sandbox does not match the configured isolation mechanism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/bootstrap.py` around lines 426 - 435, Update the bootstrap condition that selects FakeExecutionEnvironment so memory storage is accepted only when SANDBOX_MECHANISM is explicitly “fake”; otherwise preserve the configured sandbox mechanism, including docker, gvisor, and microvm. Add the necessary validation in validate_settings or the nearest settings-validation path so invalid memory-storage combinations are rejected before sandbox creation.tests/gates/test_sandbox_m6.py-37-48 (1)
37-48: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winResolve aliases and cover all host process APIs.
ast.unparse(node.func)producesaio.create_subprocess_execafterimport asyncio as aio, andspawnafterfrom os import system as spawn. Neither value matchesforbidden_calls.os.posix_spawnandos.exec*are also not listed. A runtime or tool change can spawn a host process without a gate finding. Resolve import aliases to canonical modules and reject the remaining process-spawn APIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_sandbox_m6.py` around lines 37 - 48, Update the AST gate’s call analysis to resolve module and symbol aliases from imports to canonical targets before comparing against forbidden_calls, including aliases such as asyncio and os.system. Expand forbidden_calls to cover os.posix_spawn and all os.exec* process APIs, while preserving findings for existing forbidden calls and reporting the original call location.tests/gates/test_sandbox_m6.py-53-60 (1)
53-60: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winResolve annotations before enforcing the host-path boundary.
A postponed or forward-reference annotation such as
workspace_root: Path | Nonecan be stored asPath | None. The checks at Lines 58 through 60 do not reject that value. A domain field can then expose a host path while this gate passes. Resolve type hints and recursively inspect aliases, unions, and generic arguments forpathlib.Path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_sandbox_m6.py` around lines 53 - 60, Update the annotation validation loop over EnvironmentSpec, EnvironmentHandle, ExecutionResult, and FileChange to resolve type hints before inspection, then recursively traverse unions, aliases, and generic arguments to detect pathlib.Path even when nested or expressed as Path | None. Preserve the existing host_path and container_id name checks while ensuring any resolved host-path type causes the gate to fail.tests/contract/test_programmatic_bridge_m6.py-19-42 (1)
19-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTest the bridge call cap and replay behavior.
Line 23 sets
maximum_calls=2, but the test sends only one authorized request. The test sends ordinal0only once. It passes if all ordinals map to zero, replay changes the call ID, or the call cap is ignored. Add a replay of ordinal0, an ordinal1request, and a request that exceeds the cap. Assert the stable replay behavior and the cap rejection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contract/test_programmatic_bridge_m6.py` around lines 19 - 42, Extend the test around ProgrammaticBridgeSession.handle to exercise maximum_calls=2: replay the authorized ordinal 0 request, send ordinal 1, then send a request beyond the cap. Assert that replaying ordinal 0 preserves the expected stable result and call identity behavior, ordinal 1 is accepted with its corresponding bridge_call_id, and the over-cap request is rejected with the established cap reason code.tests/security/test_sandbox_runtime_m6.py-204-238 (1)
204-238: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTest direct network denial in
ALLOWLISTmode.Lines 213 through 224 use proxy-aware
urllib.requestcalls. The raw-socket check at Lines 143 through 160 uses the default-deny policy, notEgressMode.ALLOWLIST. If allowlist mode retains an external route, sandbox code can use a raw socket or a proxy-disabled client to bypass host, port, and private-address policy while this test passes. Add an unproxied public connection probe under the allowlist policy and require it to fail.Proposed coverage
- import json,urllib.error,urllib.request + import json,socket,urllib.error,urllib.request out={} for name,url in { ... }.items(): try: out[name]=urllib.request.urlopen(url,timeout=5).status except Exception as exc: out[name]=type(exc).__name__ + try: + direct=socket.create_connection(('1.1.1.1',53),0.5) + direct.close() + out['raw_public']=True + except OSError: + out['raw_public']=False print(json.dumps(out,sort_keys=True)) ... assert outcomes["allowed"] == 200 + assert outcomes["raw_public"] is FalseADR-0042 requires the sandbox to have no direct route to the external bridge network.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/test_sandbox_runtime_m6.py` around lines 204 - 238, Extend test_egress_allowlisted with an unproxied raw-socket or proxy-disabled public connection probe while retaining the existing ALLOWLIST policy. Execute the probe in the sandbox and assert it fails, verifying direct external routing is denied independently of urllib proxy behavior; keep the existing allowlist outcome and egress-reason assertions intact.tests/gates/test_artifact_m6.py-175-243 (1)
175-243: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExercise the bounded artifact branch.
This fixture produces 2,527 serialized bytes with
maximum_output_bytes=1000. It stays below the 4,000-byte cap inToolPipeline._artifactize_large_output. When a result exceeds that cap,src/agent_core/tools/executor.pystores onlyrendered[:hard_ceiling]but labels the reference as"full output". Users can receive partial bytes under a full-output label.Use output larger than four times the limit. Update
ToolPipeline._artifactize_large_outputto store complete bytes or report a partial artifact. Assert that behavior here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_artifact_m6.py` around lines 175 - 243, Expand _LargeOutputTool’s fixture output beyond four times maximum_output_bytes so the bounded artifact branch in ToolPipeline._artifactize_large_output executes. Update _artifactize_large_output so artifacts storing rendered[:hard_ceiling] are identified as partial rather than “full output,” or otherwise ensure complete bytes are stored; then adjust test_large_tool_output_is_excerpted_and_artifactized assertions to verify the resulting label and bounded stored size.evals/gates/sandbox.yaml-62-67 (1)
62-67: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCover the workspace export path in this hard gate.
The registered
test_artifact_checksumcreates the artifact throughArtifactWriterFactory.create(). It does not use a workspace orArtifactExportTool.test_generated_workspace_file_exports_as_authorized_artifactcovers the workspace path, but it does not download through the API or compare the SHA-256. A failure between workspace reading and artifact persistence can therefore pass this registered gate.Add one registered test that covers workspace export, API download, and checksum validation in one flow. Keep the digest-mismatch assertion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@evals/gates/sandbox.yaml` around lines 62 - 67, Update the gate.sandbox.artifact_checksum registration and its coverage so one registered test exercises ArtifactExportTool with a generated workspace file, downloads the resulting artifact through the API, and compares its SHA-256 to the workspace bytes. Preserve the existing digest-mismatch assertion, and ensure the test is included in the registered gate rather than relying only on test_generated_workspace_file_exports_as_authorized_artifact.src/agent_core/execution/egress_core.py-10-24 (1)
10-24: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject all non-global and multicast destination addresses.
_DENIED_NETWORKSallows special-use addresses such as198.18.0.1,255.255.255.255, and::. Useaddress.is_globalafter IPv4-mapped normalization, and reject multicast addresses separately becauseff02::1hasis_global=True. Remove_DENIED_NETWORKSand add denial tests for special-use and multicast IPv4 and IPv6 addresses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/egress_core.py` around lines 10 - 24, The destination validation should reject every non-global or multicast address rather than relying on the incomplete _DENIED_NETWORKS list. Remove _DENIED_NETWORKS, normalize IPv4-mapped addresses before checking address.is_global, and add a separate multicast rejection so addresses such as ff02::1 are denied; add denial tests covering special-use and multicast IPv4 and IPv6 destinations.src/agent_core/execution/proxy.py-124-128 (1)
124-128: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict the proxy listener to the sandbox network.
The proxy binds
0.0.0.0:3128.src/agent_core/adapters/execution/docker.pystarts the proxy container on the defaultbridgenetwork and then connects it to the per-environment internal network. Any other container on the defaultbridgenetwork can therefore reach port 3128 and use the proxy to reach the allowlisted destinations, with the tenant and run identity of this sandbox attached to the audit log.Bind only the address on the internal per-environment network, or resolve the alias
egress-proxyat startup and pass that address toasyncio.start_server.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/proxy.py` around lines 124 - 128, Update main in src/agent_core/execution/proxy.py so asyncio.start_server binds only to the proxy’s address on the per-environment internal network, rather than 0.0.0.0. Resolve the egress-proxy alias at startup and pass the resolved address to start_server, preserving port 3128 and the existing request handler and policy flow.src/agent_core/execution/bridge_relay.py-25-30 (1)
25-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBoth stream readers cap bytes after the read, so the asyncio stream limit raises first. Each module defines a 64 KiB cap that equals the default
asynciostream limit and then checks the cap afterreadlineorreaduntilreturns. The stream layer raises before the check, so the cap branch is dead code and the oversized-input path produces an exception instead of the intended denial or error response. Pass an explicitlimitto the server and handle the stream exception in both places.
src/agent_core/execution/bridge_relay.py#L25-L30: passlimittoasyncio.start_unix_server, and catchValueErrorfromreadlineto returnbridge.request_too_large.src/agent_core/execution/proxy.py#L68-L70: passlimittoasyncio.start_server, and catchasyncio.LimitOverrunErrorfromreaduntilso the client receives the502response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/bridge_relay.py` around lines 25 - 30, The stream readers currently enforce their size limits only after the asyncio stream layer rejects oversized input. In src/agent_core/execution/bridge_relay.py lines 25-30, update the server created by the bridge relay handler to pass the explicit 64 KiB limit to asyncio.start_unix_server and catch ValueError from reader.readline, returning bridge.request_too_large. In src/agent_core/execution/proxy.py lines 68-70, pass the same limit to asyncio.start_server and catch asyncio.LimitOverrunError from reader.readuntil, preserving the existing 502 response.src/agent_core/execution/environment.py-10-21 (1)
10-21: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStrengthen the tier-0 refusal beyond a fixed name list.
TIER_ZERO_NAMESnames eight specific variables. Any other secret in the parent environment passes through when an operator lists it, for exampleGITHUB_TOKEN,AZURE_OPENAI_API_KEY, orDATABASE_PASSWORD.load_settingsinsrc/agent_core/config.pyalready treats every*_API_KEYvariable as a credential, which shows such names exist in the parent process environment. The explicit list also carries a deployment-specific entry,VEETBOT_OPENAI_KEY, which shows the list grows by discovery rather than by rule.Add a pattern rule on top of the explicit list, and refuse names that contain a secret-bearing token.
🛡️ Proposed fix
+_SECRET_NAME_FRAGMENTS = ("SECRET", "TOKEN", "PASSWORD", "CREDENTIAL", "_KEY", "KEY_") + + +def _is_secret_name(name: str) -> bool: + upper = name.upper() + return upper in TIER_ZERO_NAMES or any(part in upper for part in _SECRET_NAME_FRAGMENTS) + + def build_sandbox_environment( parent: Mapping[str, str], passthrough_names: Sequence[str] = (), *, working_directory: PurePosixPath = _WORKSPACE_ROOT, bridge: BridgeEndpoint | None = None, ) -> dict[str, str]: """Build, never filter, the environment visible to untrusted code.""" requested = set(passthrough_names) - forbidden = requested & TIER_ZERO_NAMES + forbidden = {name for name in requested if _is_secret_name(name)} if forbidden: raise ValueError( "tier-0 sandbox variables cannot be passed through: " + ", ".join(sorted(forbidden)) )Also applies to: 34-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/environment.py` around lines 10 - 21, Extend the tier-0 environment-variable refusal logic using the existing TIER_ZERO_NAMES rule: retain explicit names, and also reject variable names containing secret-bearing tokens such as API_KEY, TOKEN, PASSWORD, SECRET, or CREDENTIAL. Apply this predicate wherever tier-0 names are checked so undiscovered credentials are refused consistently.src/agent_core/adapters/execution/docker.py-688-691 (1)
688-691: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftInvalidate the environment after the adapter kills the container.
Both kill paths run
docker killon the long-lived sandbox container, butself._states[environment.environment_id]stays in place. The container is created without--rm, so it stops and stays stopped. Every laterdocker execfor the same lease then fails and surfaces asExecutionUnavailable("container runtime operation failed"), includingworkspace.read,listdir, and_snapshot. One command that exceeds the output cap or the disk cap therefore breaks the whole run lease with an opaque error.Choose one contract and make it explicit. Either kill only the command process and leave the container alive, or remove the state entry after the container kill so the next request provisions a new environment and callers receive
ExecutionRejected("execution environment is gone").Also applies to: 711-717
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 688 - 691, Invalidate the leased environment in both container-kill paths after calling _docker("kill", state.container_id). Remove the corresponding self._states entry using environment.environment_id so subsequent requests provision a new container and return ExecutionRejected("execution environment is gone") rather than attempting docker exec on the stopped container.src/agent_core/execution/manager.py-173-192 (1)
173-192: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAttempt every destroy during release and close.
release_runandcloseboth stop at the first failingdestroy. Inrelease_runthefinallyblock clears only the current key, then the exception propagates and the remaining environments for that run stay inself._handles. Inclosethe exception leavesself._handlesandself._lockspopulated. The container, volume, and network for those environments then survive until the reaper runs. Collect the failures and re-raise after all handles are processed.🛡️ Proposed fix
async def release_run(self, run_id: UUID) -> None: matches = [(key, handle) for key, handle in self._handles.items() if key[1] == run_id] + failures: list[BaseException] = [] for key, handle in matches: try: await self._environment.destroy(handle) + except Exception as exc: # noqa: BLE001 - continue releasing every lease + failures.append(exc) finally: self._handles.pop(key, None) self._locks.pop(key, None) + if failures: + raise ExceptionGroup("sandbox release failed", failures) @@ async def close(self) -> None: + failures: list[BaseException] = [] for handle in tuple(self._handles.values()): - await self._environment.destroy(handle) + try: + await self._environment.destroy(handle) + except Exception as exc: # noqa: BLE001 - close every environment + failures.append(exc) self._handles.clear() self._locks.clear() + if failures: + raise ExceptionGroup("sandbox close failed", failures)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/manager.py` around lines 173 - 192, Update release_run and close to attempt destroy for every matching/all tracked handle, collecting any exceptions instead of propagating immediately. Preserve per-handle cleanup in release_run and clear both _handles and _locks in close after processing, then re-raise the collected failure(s) only once all destroys have been attempted.src/agent_core/adapters/execution/docker.py-686-687 (1)
686-687: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not swallow
asyncio.CancelledError.The
except asyncio.CancelledErrorclause recordsKillReason.CANCELLEDand then continues to the code after thetryblock. It never re-raises. The coroutine therefore returns a normalExecutionResultafter the caller cancelled it, so the cancellation is lost and the caller sees a completed command. The followingawaitcalls (Line 692, Line 694, Line 697) also run inside a task that already carries a cancellation request, so they can raiseCancelledErroragain at an unpredictable point and leavedisk_task,stdout_task, andstderr_taskunfinished.Kill the container, clean up the helper tasks, then re-raise.
🛡️ Proposed fix
except asyncio.CancelledError: - killed_by = KillReason.CANCELLED + with suppress(ExecutionUnavailable): + await _docker("kill", state.container_id) + with suppress(ProcessLookupError): + process.kill() + disk_monitor_stop.set() + for task in (wait_task, exceeded_task, disk_task, stdout_task, stderr_task): + task.cancel() + raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 686 - 687, Update the asyncio.CancelledError handler in the execution flow around killed_by to kill the container, await cleanup of disk_task, stdout_task, and stderr_task, then re-raise the original cancellation instead of continuing to construct an ExecutionResult. Ensure cleanup completes without masking the propagated CancelledError.
🟡 Minor comments (5)
src/agent_core/adapters/persistence/memory.py-953-953 (1)
953-953: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConsent withdrawal now skips artifacts that never expire.
The previous comparison would raise a
TypeErrorforexpires_at is None, so the guard is an improvement. But the new branch means an artifact with no expiry survivesexpire_for_principaluntouched. If a non-expiring trajectory artifact ever exists, withdrawing consent will not schedule its deletion.Consider setting
expires_at = expired_atwhen it isNone, so withdrawal always produces a due artifact.🛡️ Proposed change
- if row.artifact.expires_at is None or row.artifact.expires_at <= expired_at: + if row.artifact.expires_at is not None and row.artifact.expires_at <= expired_at: continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/persistence/memory.py` at line 953, The expire_for_principal flow should schedule deletion for non-expiring artifacts instead of leaving them unchanged. Update the branch handling row.artifact.expires_at so a None value assigns expired_at, while preserving the existing comparison and expiration behavior for artifacts with an expiry.src/agent_core/tools/executor.py-901-904 (1)
901-904: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
stdout/stderrtruncation mixes byte and character units.Line 903 measures the value in UTF-8 bytes. Line 904 slices it by character count. For multibyte text,
value[: budget // 4]can produce up to four timesbudget // 4bytes, so the truncated field can be larger than the check intended to allow.Truncate on the encoded bytes and decode with
errors="ignore", which matches howheadandtailare produced above.🐛 Proposed change
value = structured.get(key) - if isinstance(value, str) and len(value.encode("utf-8")) > budget // 2: - structured[key] = value[: budget // 4] + "\n[TRUNCATED]" + if isinstance(value, str): + encoded = value.encode("utf-8") + if len(encoded) > budget // 2: + head_text = encoded[: budget // 4].decode("utf-8", errors="ignore") + structured[key] = head_text + "\n[TRUNCATED]"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 901 - 904, Update the stdout/stderr truncation loop to slice the UTF-8 encoded bytes rather than the original string, then decode the retained bytes with errors="ignore" before appending "[TRUNCATED]". Keep the existing budget thresholds and apply this change only to the structured stdout/stderr fields.src/agent_core/sandbox/limits.yaml-1-11 (1)
1-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd sandbox-specific overlay validation.
sandbox/limits.yamlis registered, and its top-level fields receive structural validation. The validator accepts malformed destinations, unsupportedegress.modevalues, and non-positive resource limits. Reject these values withConfigurationErrorbefore_composeconstructsResourceLimitsandEgressPolicy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/sandbox/limits.yaml` around lines 1 - 11, Extend the validation for the registered sandbox limits configuration before _compose constructs ResourceLimits and EgressPolicy: require positive values for each resource limit, restrict egress.mode to supported values, and validate every egress.destinations entry against the expected destination format. Raise ConfigurationError for any invalid value while preserving valid configurations.tests/contract/test_artifact_writer_provider_contract.py-10-30 (1)
10-30: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert all writer-provider identity fields.
ArtifactWriter.create()persistsprincipal_id,session_id, andoriginas well as tenant and run identity. This test only assertstenant_idandrun_id. A provider can discard one of the other values and still pass this contract.Store the received values by key. Assert the complete expected mapping.
Proposed test change
class _Provider: def __init__(self) -> None: - self.bound: tuple[object, ...] | None = None + self.bound: dict[str, object] | None = None def for_run(self, **values: object) -> object: - self.bound = tuple(values[key] for key in sorted(values)) + self.bound = dict(values) return object() ... assert provider.bound is not None - assert "tenant-a" in provider.bound - assert UUID(int=81) in provider.bound + assert provider.bound == { + "tenant_id": "tenant-a", + "principal_id": "user-a", + "session_id": UUID(int=80), + "run_id": UUID(int=81), + "origin": ArtifactOrigin.SANDBOX_EXPORT, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contract/test_artifact_writer_provider_contract.py` around lines 10 - 30, Update _Provider.for_run and test_artifact_writer_provider_binds_run_and_tenant to retain received values by key instead of an order-dependent tuple, then assert the complete expected mapping for tenant_id, principal_id, session_id, run_id, and origin.src/agent_core/adapters/execution/fake.py-60-77 (1)
60-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign
_FakeWorkspaceHandle.listdirwith the real adapters. RaiseNotADirectoryErrorfor a stored file or a path below one. RaiseFileNotFoundErrorfor other non-root paths with no stored file at or below them. Add contract cases for both errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/fake.py` around lines 60 - 77, The `_FakeWorkspaceHandle.listdir` method must validate the requested base before enumerating entries: raise `NotADirectoryError` when the path is a stored file or lies beneath one, and raise `FileNotFoundError` for non-root paths with no stored file at or below them. Preserve root and valid-directory listing behavior, and add contract cases covering both exceptions.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/agent_core/execution/manager.py (1)
169-175: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake bridge capability explicit in
SandboxManager.The built-in adapters support
execute_with_bridge, butSandboxManageraccepts any_WorkspaceEnvironment. The uncheckedcastcan still causeAttributeErrorfor an adapter without bridge support. Require_BridgeExecutionEnvironmentor return a defined execution failure when bridge support is unavailable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/manager.py` around lines 169 - 175, Update SandboxManager’s bridge execution path around execute_with_bridge to explicitly require bridge capability before invoking it: narrow the environment contract to _BridgeExecutionEnvironment or validate support and return the established execution failure when unavailable. Remove the unchecked cast-based assumption while preserving normal execution for environments without a bridge request.src/agent_core/adapters/execution/docker.py (3)
576-585: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the relay write in
_bridge_pump.The
tryat line 577 wraps onlyhandler.handle. The writes at lines 584-585 are unguarded. When the relay process exits,process.stdin.writefollowed bydrain()raisesBrokenPipeErrororConnectionResetError, and the_bridge_pumptask finishes with that exception.
execute_with_bridgethen awaits that task at line 654 and suppresses onlyasyncio.CancelledError, so the connection error propagates out of thefinallyblock and replaces theExecutionResultreturned at line 636. A completed sandbox command is reported as a failure.🛡️ Proposed fix
while request := await process.stdout.readline(): try: response = await handler.handle(request.rstrip(b"\n")) except Exception: response = ( b'{"status":"unavailable","reason_code":"bridge.internal_error",' b'"retryable":false}' ) - process.stdin.write(response + b"\n") - await process.stdin.drain() + try: + process.stdin.write(response + b"\n") + await process.stdin.drain() + except (BrokenPipeError, ConnectionResetError): + returnAlso widen the suppression at line 653 so no pump failure can mask the command result:
pump.cancel() with suppress(asyncio.CancelledError, OSError): await pump🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 576 - 585, Update `_bridge_pump` to guard the relay response write and `drain()` against `OSError` so a disconnected relay does not leave the task failed; preserve processing of subsequent requests only when the write succeeds. In `execute_with_bridge`, widen the cancelled pump suppression to include `OSError` when awaiting the pump, ensuring pump failures cannot replace the completed `ExecutionResult`.
794-811: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAn empty post-kill snapshot reports every workspace file as deleted.
Line 794 sets
after = {}wheneverkilled_byis notNone._changes(before, {})then emits aChangeKind.DELETEDentry for every path present inbefore.The kill path does not delete the workspace volume. It kills the container and restarts it at line 776, and the volume survives. So a timed-out or output-limited command on a populated workspace reports the whole workspace as deleted.
SandboxRunCommandTool.executeinsrc/agent_core/tools/sandbox_run_command.pycopiesfiles_changedstraight into its structured output, so the model receives false deletions.Line 795 also computes
workspace_sizeas0from the empty snapshot, which makes the guard at lines 796-799 unreachable in this state.Distinguish "no snapshot taken" from "empty workspace":
🐛 Proposed fix
- after = {} if killed_by is not None else await self._snapshot(state) + after = None if killed_by is not None else await self._snapshot(state) + workspace_size = 0 if after is None else sum(item[0] for item in after.values()) - workspace_size = sum(item[0] for item in after.values()) - if killed_by is None and ( + if after is not None and killed_by is None and ( workspace_size > state.specification.limits.workspace_bytes or len(after) > state.specification.limits.inodes_max ):Then compute changes only from a real snapshot:
- changes = self._changes(before, after) + changes = () if after is None else self._changes(before, after)Alternatively take the snapshot after the container restart at line 776, so a killed command still reports its true file changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 794 - 811, Update the post-command snapshot flow around _snapshot, killed_by, and _changes so a killed command does not substitute an empty mapping for the workspace snapshot. Preserve the distinction between no snapshot being available and a genuinely empty workspace, and ensure workspace_size limits are evaluated from a real snapshot when applicable. Compute files_changed only from a valid snapshot, or take the snapshot after the container restart, so surviving workspace files are not reported as deleted.
552-569: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winReduce the workspace-usage polling rate.
The loop runs one
docker execroughly every 100 ms for the entire command duration. Each invocation spawns a client process on the host and a Python process inside the container. For a command near the 300 s hard cap this reaches thousands of executions.The in-container probe also consumes a PID slot inside the same container, so it competes with the workload against
--pids-limit. The result is measurable CPU overhead plus a risk of skewing theKillReason.PIDSclassification at lines 804-810.Use a coarser interval, and derive it from a named constant so it is tunable.
♻️ Proposed interval change
+_WORKSPACE_POLL_SECONDS = 1.0 + `@staticmethod` async def _monitor_workspace_limits( state: _DockerState, stop: asyncio.Event ) -> KillReason | None: while not stop.is_set(): try: raw = await _docker( "exec", state.container_id, "python", "-c", _WORKSPACE_USAGE_SCRIPT ) except ExecutionUnavailable: if stop.is_set(): return None - await asyncio.sleep(0.1) + await asyncio.sleep(_WORKSPACE_POLL_SECONDS) continue size, inodes = (int(value) for value in raw.decode("ascii").split()) if ( size > state.specification.limits.workspace_bytes or inodes > state.specification.limits.inodes_max ): return KillReason.DISK with suppress(TimeoutError): - await asyncio.wait_for(stop.wait(), timeout=0.1) + await asyncio.wait_for(stop.wait(), timeout=_WORKSPACE_POLL_SECONDS)The post-command check at lines 796-803 already catches limit violations that the monitor misses, so a coarser interval does not weaken enforcement. Note that
tests/security/test_sandbox_runtime_m6.py::test_limits_enforcedassertsKillReason.DISKwithin an 8 s timeout; confirm the chosen interval keeps that test deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 552 - 569, Reduce the workspace-usage monitor polling frequency in the loop that invokes _docker and waits on stop by replacing the hard-coded 0.1-second timeout and retry delay with a named, tunable interval constant. Use the same interval consistently for asyncio.sleep and asyncio.wait_for, choosing a value that preserves deterministic detection within the test’s 8-second timeout while reducing container process overhead.
🧹 Nitpick comments (2)
src/agent_core/adapters/artifacts/filesystem.py (1)
132-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake claim restoration failure-tolerant so one bad entry does not abort the whole pass.
_restore_claimsuppresses onlyFileExistsError. Two other cases propagate out ofreconcile_orphansand abort the remaining entries:
os.linkraisesFileNotFoundErrorif another reconciliation pass already removed the claim.os.linkraisesOSError(for exampleEPERMorEOPNOTSUPP) on filesystems that do not support hard links.In both cases the reconciliation callback in
src/agent_core/bootstrap.py(Line 527) fails and later objects in the same pass are never examined. Restoration is best-effort by design, so handle these per entry and continue.♻️ Proposed change to keep the pass going
def _restore_claim(claim: Path, destination: Path) -> None: - with suppress(FileExistsError): + with suppress(OSError): os.link(claim, destination) claim.unlink(missing_ok=True)If you prefer to keep unexpected link errors visible, log them instead of suppressing them silently.
Also applies to: 153-157, 161-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/artifacts/filesystem.py` around lines 132 - 136, Update _restore_claim and the reconcile_orphans restoration path so FileNotFoundError and other OSError failures from os.link are handled per entry, allowing reconciliation to continue processing later claims. Preserve successful restoration and existing FileExistsError behavior; optionally log unexpected link errors rather than propagating them from the callback.src/agent_core/tools/executor.py (1)
924-932: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThe structured payload stays unbounded after artifactization.
_artifactize_large_outputboundscontentand truncates only thestdoutandstderrstring fields. Any other large field instructuredpasses through unchanged, and_finishpersists it asstructured_result(Line 1305). A tool that returns a large list, such asfiles_changedinsrc/agent_core/tools/sandbox_run_command.py, can therefore still write an unbounded row to the database even though the declared output budget was exceeded.Consider bounding the whole serialized
structuredpayload, rather than two known keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 924 - 932, The structured payload handling in _artifactize_large_output only truncates stdout and stderr, allowing other fields such as files_changed to exceed the output budget before _finish persists structured_result. Replace the field-specific truncation with whole-payload serialization and enforce the budget on the serialized structured data, preserving valid structured output while bounding its stored size.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agent_core/adapters/execution/docker.py`:
- Around line 879-898: Update the parsing logic in the reaper loop to treat an
empty or missing raw_created value as old enough to reap, while preserving the
existing grace-period calculation for valid timestamps. Keep malformed required
fields handled by the existing ValueError/TypeError path, and ensure legacy
containers without the created_at label continue through the cleanup checks
instead of being skipped.
- Around line 209-241: Update the Docker workspace read flow and _read to
prevent symlink traversal: resolve each path component beneath /workspace using
descriptor-based traversal with O_NOFOLLOW, rejecting any symlink in
intermediate or final components. Perform existence, regular-file, and size
validation on the resulting descriptor, then stream bytes from that descriptor
rather than using Path.exists/is_file/stat/open; preserve the existing mapped
exceptions and read-limit behavior.
In `@src/agent_core/adapters/execution/local_workspace.py`:
- Around line 128-141: Wrap each intermediate component open in the workspace
traversal logic around the directory iteration, translating OSError with
errno.ELOOP into WorkspaceEscape just like the final component path in the
existing exception handler. Preserve other OSError behavior and descriptor
cleanup, and add a contract test covering a symlinked directory component that
asserts WorkspaceEscape.
In `@src/agent_core/execution/bridge_relay.py`:
- Around line 55-60: The response relay around responses.readline() must prevent
oversized responses from leaving unread bytes that become the next response. Add
a response-size bound to the bridge relay, or terminate the relay/session
immediately when the readline overrun is detected, while preserving the existing
bridge.response_too_large handling and normal empty-response return behavior.
In `@src/agent_core/execution/manager.py`:
- Around line 178-190: Update release_run and close to coordinate teardown with
execute_for and delegated workspace operations by tracking a closing state and
active lease operations. Prevent new provisioning once close begins, ensure each
handle is destroyed only after its active operations finish, and make close
account for handles provisioned after its initial snapshot.
In `@src/agent_core/tools/artifact_export.py`:
- Around line 63-79: Update the collaborator guard before the WorkspaceHandle
and ArtifactWriter casts to handle _UnavailableCollaborator’s RuntimeError from
getattr(raw_writer, "create", None), treating it as an unavailable writer and
returning the existing INTERNAL tool.internal_error result instead of allowing
execution to raise. Preserve the current callable checks for available
collaborators.
In `@tests/contract/test_execution_environment_contract.py`:
- Around line 113-129: Update SandboxManager.release_run() and close() to
attempt bounded destruction of every handle even when a destroy() raises
asyncio.CancelledError, while retaining ordinary failures for retry and
re-raising cancellation after all cleanup completes. Extend the existing sandbox
cleanup tests with a regression case where the first destroy attempt raises
asyncio.CancelledError, verifying later handles are attempted and cancellation
is propagated.
In `@tests/contract/test_workspace_handle_contract.py`:
- Around line 27-39: Update DockerWorkspaceHandle path resolution to use
descriptor-relative traversal with dir_fd and O_NOFOLLOW for reads, writes,
streams, and directory listings; reject every symlink component with
WorkspaceEscape and map descendants beneath a non-directory ancestor to
NotADirectoryError. In tests/contract/test_workspace_handle_contract.py lines
27-39, run the symlink and FIFO assertions against DockerWorkspaceHandle; in
tests/contract/test_execution_environment_contract.py lines 94-110, run the
descendant-under-file assertion against DockerWorkspaceHandle.
---
Outside diff comments:
In `@src/agent_core/adapters/execution/docker.py`:
- Around line 576-585: Update `_bridge_pump` to guard the relay response write
and `drain()` against `OSError` so a disconnected relay does not leave the task
failed; preserve processing of subsequent requests only when the write succeeds.
In `execute_with_bridge`, widen the cancelled pump suppression to include
`OSError` when awaiting the pump, ensuring pump failures cannot replace the
completed `ExecutionResult`.
- Around line 794-811: Update the post-command snapshot flow around _snapshot,
killed_by, and _changes so a killed command does not substitute an empty mapping
for the workspace snapshot. Preserve the distinction between no snapshot being
available and a genuinely empty workspace, and ensure workspace_size limits are
evaluated from a real snapshot when applicable. Compute files_changed only from
a valid snapshot, or take the snapshot after the container restart, so surviving
workspace files are not reported as deleted.
- Around line 552-569: Reduce the workspace-usage monitor polling frequency in
the loop that invokes _docker and waits on stop by replacing the hard-coded
0.1-second timeout and retry delay with a named, tunable interval constant. Use
the same interval consistently for asyncio.sleep and asyncio.wait_for, choosing
a value that preserves deterministic detection within the test’s 8-second
timeout while reducing container process overhead.
In `@src/agent_core/execution/manager.py`:
- Around line 169-175: Update SandboxManager’s bridge execution path around
execute_with_bridge to explicitly require bridge capability before invoking it:
narrow the environment contract to _BridgeExecutionEnvironment or validate
support and return the established execution failure when unavailable. Remove
the unchecked cast-based assumption while preserving normal execution for
environments without a bridge request.
---
Nitpick comments:
In `@src/agent_core/adapters/artifacts/filesystem.py`:
- Around line 132-136: Update _restore_claim and the reconcile_orphans
restoration path so FileNotFoundError and other OSError failures from os.link
are handled per entry, allowing reconciliation to continue processing later
claims. Preserve successful restoration and existing FileExistsError behavior;
optionally log unexpected link errors rather than propagating them from the
callback.
In `@src/agent_core/tools/executor.py`:
- Around line 924-932: The structured payload handling in
_artifactize_large_output only truncates stdout and stderr, allowing other
fields such as files_changed to exceed the output budget before _finish persists
structured_result. Replace the field-specific truncation with whole-payload
serialization and enforce the budget on the serialized structured data,
preserving valid structured output while bounding its stored size.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db6e9a5d-9dac-46be-9cca-d29737806def
📒 Files selected for processing (37)
docs/adr/0042-milestone-6-sandbox-and-artifact-seams.mddocs/status/project-state.yamlsrc/agent_core/adapters/artifacts/filesystem.pysrc/agent_core/adapters/execution/docker.pysrc/agent_core/adapters/execution/fake.pysrc/agent_core/adapters/execution/local_workspace.pysrc/agent_core/adapters/persistence/memory.pysrc/agent_core/adapters/persistence/repositories.pysrc/agent_core/application/artifact_writer.pysrc/agent_core/application/public_services.pysrc/agent_core/bootstrap.pysrc/agent_core/config.pysrc/agent_core/execution/bridge_relay.pysrc/agent_core/execution/egress_core.pysrc/agent_core/execution/environment.pysrc/agent_core/execution/manager.pysrc/agent_core/execution/proxy.pysrc/agent_core/ports/execution.pysrc/agent_core/ports/repositories.pysrc/agent_core/runtime/worker.pysrc/agent_core/sandbox/limits.yamlsrc/agent_core/tools/artifact_export.pysrc/agent_core/tools/bridge.pysrc/agent_core/tools/executor.pysrc/agent_core/tools/sandbox_run_command.pytests/contract/test_artifact_repository_contract.pytests/contract/test_artifact_store_contract.pytests/contract/test_artifact_writer_provider_contract.pytests/contract/test_execution_environment_contract.pytests/contract/test_programmatic_bridge_m6.pytests/contract/test_workspace_handle_contract.pytests/gates/test_api_m5.pytests/gates/test_artifact_m6.pytests/gates/test_policy_m4.pytests/gates/test_sandbox_m6.pytests/security/test_sandbox_runtime_m6.pytests/unit/test_config.py
🚧 Files skipped from review as they are similar to previous changes (15)
- src/agent_core/sandbox/limits.yaml
- tests/contract/test_artifact_writer_provider_contract.py
- tests/contract/test_artifact_repository_contract.py
- tests/gates/test_artifact_m6.py
- docs/status/project-state.yaml
- src/agent_core/tools/sandbox_run_command.py
- src/agent_core/ports/execution.py
- src/agent_core/application/public_services.py
- src/agent_core/adapters/execution/fake.py
- src/agent_core/runtime/worker.py
- src/agent_core/adapters/persistence/repositories.py
- src/agent_core/application/artifact_writer.py
- tests/security/test_sandbox_runtime_m6.py
- src/agent_core/bootstrap.py
- src/agent_core/execution/egress_core.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agent_core/execution/manager.py`:
- Around line 249-259: Update release_run so run_id is added to _released_runs
under _condition before acquiring _teardown_lock, preventing new B operations
while release_run(B) waits on unrelated teardown for A. Preserve the existing
serialized active-operation drain, handle snapshot, and _destroy_matches
sequence.
- Around line 172-174: Make the provisioning handoff in _handle_for()
cancellation-safe by avoiding an await while transferring the newly provisioned
handle into _handles, or by ensuring cancellation cleans up the handle and
removes its uncached _locks entry before _operation() exits. Preserve correct
release_run() and close() behavior for handles returned by provision(),
including when provision() raises.
In `@tests/contract/test_execution_environment_contract.py`:
- Around line 157-183: Update SandboxManager.release_run() and close() to catch
and defer caller cancellation while waiting for active operations and completing
_destroy_matches(), then re-raise CancelledError only after all sandbox teardown
attempts finish. In tests/contract/test_execution_environment_contract.py lines
157-183, add blocked-execution cancellation tests covering both methods and
verify teardown completes before cancellation propagates. In
docs/adr/0042-milestone-6-sandbox-and-artifact-seams.md lines 45-49, remove or
revise the deferred-cancellation guarantee until the lifecycle implementation
enforces it.
In `@tests/gates/test_artifact_m6.py`:
- Around line 154-165: Split
test_artifact_export_normalizes_an_unavailable_writer into separate cases: one
with only workspace unavailable to verify workspace handling, and another with a
working workspace but unavailable artifacts collaborator to exercise the
artifacts.create path. Ensure each case asserts the expected normalized result
independently.
In `@tests/security/test_sandbox_runtime_m6.py`:
- Around line 227-230: Replace the hard-coded token argument in
ProgrammaticBridgeSession with a generated test token, or add a documented
targeted suppression for this intentional test credential so Ruff S106 passes
without changing the test’s behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81aa63d7-7fe8-462a-83c7-033ce8fb5556
📒 Files selected for processing (12)
docs/adr/0042-milestone-6-sandbox-and-artifact-seams.mddocs/status/project-state.yamlsrc/agent_core/adapters/execution/docker.pysrc/agent_core/adapters/execution/local_workspace.pysrc/agent_core/execution/bridge_relay.pysrc/agent_core/execution/manager.pysrc/agent_core/tools/artifact_export.pytests/contract/test_execution_environment_contract.pytests/contract/test_workspace_handle_contract.pytests/gates/test_artifact_m6.pytests/gates/test_sandbox_m6.pytests/security/test_sandbox_runtime_m6.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/contract/test_workspace_handle_contract.py
- src/agent_core/tools/artifact_export.py
- docs/status/project-state.yaml
- src/agent_core/execution/bridge_relay.py
- src/agent_core/adapters/execution/docker.py
- src/agent_core/adapters/execution/local_workspace.py
|
@coderabbitai review |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 8 minutes. |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agent_core/execution/manager.py`:
- Around line 157-191: Update _handle_for and the teardown flows in release_run,
close, and _discard_unused_locks to retain lock entries while their user count
is nonzero, including provisioning that outlives a timed drain. Reconcile
handles created after teardown snapshots by destroying late-provisioned handles
before the released or closing operation completes, and prevent _handle_for from
raising KeyError during its final ownership handoff. Add regressions covering
blocked provisioning followed by both release_run and close.
In `@tests/contract/test_programmatic_bridge_m6.py`:
- Around line 6-8: Update the test fixture’s literal token argument near the
token setup to generate the token at runtime instead of embedding a hard-coded
secret-like string. Preserve the fixture’s expected token behavior while
removing the Ruff S106 violation, reusing the existing imports or standard
runtime facilities as appropriate.
In `@tests/gates/test_sandbox_m6.py`:
- Around line 129-159: Update DockerWorkspaceHandle.stream in
src/agent_core/adapters/execution/docker.py to enforce the same timeout for
direct docker exec subprocesses, and on timeout kill the process and await
wait() before propagating the timeout failure. In tests/gates/test_sandbox_m6.py
lines 129-159, extend the hanging-process helper test to record and assert
wait() after kill(), and add regression coverage for the streaming path. In
docs/adr/0042-milestone-6-sandbox-and-artifact-seams.md lines 123-129, limit the
timeout claim to _docker() until all direct Docker subprocess paths are bounded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21c602ca-0f15-4a9e-aa90-18eb1d6bedb2
📒 Files selected for processing (19)
.circleci/config.ymldocs/adr/0042-milestone-6-sandbox-and-artifact-seams.mddocs/status/project-state.yamlexecution/sandbox.Dockerfilesrc/agent_core/adapters/execution/docker.pysrc/agent_core/execution/bridge_relay.pysrc/agent_core/execution/environment.pysrc/agent_core/execution/manager.pysrc/agent_core/execution/proxy.pysrc/agent_core/tools/artifact_export.pysrc/agent_core/tools/bridge.pysrc/agent_core/tools/executor.pytests/contract/test_artifact_store_contract.pytests/contract/test_artifact_writer_contract.pytests/contract/test_execution_environment_contract.pytests/contract/test_programmatic_bridge_m6.pytests/gates/test_artifact_m6.pytests/gates/test_sandbox_m6.pytests/security/test_sandbox_runtime_m6.py
💤 Files with no reviewable changes (1)
- src/agent_core/execution/environment.py
🚧 Files skipped from review as they are similar to previous changes (9)
- .circleci/config.yml
- execution/sandbox.Dockerfile
- src/agent_core/execution/proxy.py
- src/agent_core/execution/bridge_relay.py
- docs/status/project-state.yaml
- src/agent_core/tools/artifact_export.py
- src/agent_core/adapters/execution/docker.py
- tests/security/test_sandbox_runtime_m6.py
- src/agent_core/tools/executor.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/agent_core/adapters/execution/docker.py (2)
371-394: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle subprocess exit races during cleanup.
If the subprocess exits after the
returncodecheck,kill()orterminate()can raiseProcessLookupError. Suppress this exception and always awaitwait()withreturn_exceptions=Trueso cleanup does not mask the original result or skip relay-task cleanup.
src/agent_core/adapters/execution/docker.py#L371-L394src/agent_core/adapters/execution/docker.py#L779-L785🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 371 - 394, Update the subprocess cleanup in src/agent_core/adapters/execution/docker.py lines 371-394 and 779-785 to tolerate exit races: suppress ProcessLookupError around kill() or terminate() calls, and always await process.wait() with return_exceptions=True so cleanup cannot mask the original result or prevent relay-task cleanup. Apply the change to both affected cleanup sites.
366-390: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse one deadline for the complete workspace stream.
Each
asyncio.wait_for()starts a new 60-second timeout. Periodic output can keep the stream alive beyond 60 seconds. Compute one monotonic deadline before the loop and pass the remaining duration to each read and toprocess.wait(). Add a test with periodic data that exceeds the total deadline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 366 - 390, Update the workspace stream method containing the read loop to compute a single monotonic deadline before reading begins, then pass the remaining time to every stdout read and the final process.wait() call instead of restarting _DOCKER_COMMAND_TIMEOUT_SECONDS for each operation. Preserve the existing error mapping and convert deadline exhaustion to the current ExecutionUnavailable timeout behavior. Add coverage for periodic output that keeps arriving beyond the total deadline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/agent_core/adapters/execution/docker.py`:
- Around line 371-394: Update the subprocess cleanup in
src/agent_core/adapters/execution/docker.py lines 371-394 and 779-785 to
tolerate exit races: suppress ProcessLookupError around kill() or terminate()
calls, and always await process.wait() with return_exceptions=True so cleanup
cannot mask the original result or prevent relay-task cleanup. Apply the change
to both affected cleanup sites.
- Around line 366-390: Update the workspace stream method containing the read
loop to compute a single monotonic deadline before reading begins, then pass the
remaining time to every stdout read and the final process.wait() call instead of
restarting _DOCKER_COMMAND_TIMEOUT_SECONDS for each operation. Preserve the
existing error mapping and convert deadline exhaustion to the current
ExecutionUnavailable timeout behavior. Add coverage for periodic output that
keeps arriving beyond the total deadline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba91e6a4-02f2-4811-b784-50b1667ea888
📒 Files selected for processing (7)
docs/adr/0042-milestone-6-sandbox-and-artifact-seams.mddocs/status/project-state.yamlsrc/agent_core/adapters/execution/docker.pysrc/agent_core/execution/manager.pytests/contract/test_execution_environment_contract.pytests/contract/test_programmatic_bridge_m6.pytests/gates/test_sandbox_m6.py
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/status/project-state.yaml
- tests/contract/test_programmatic_bridge_m6.py
- tests/gates/test_sandbox_m6.py
- docs/adr/0042-milestone-6-sandbox-and-artifact-seams.md
- src/agent_core/execution/manager.py
|
Addressed both out-of-diff findings from CodeRabbit review 4852935691 in 0c2df0a. Docker workspace streaming now uses one monotonic 60-second deadline across all reads and process exit, with periodic-output regression coverage. Docker control, workspace-stream, and bridge-relay cleanup now suppresses ProcessLookupError exit races and always reaps subprocesses. Local make check, 68 PostgreSQL integration tests, 10 real Docker sandbox tests, and docs checks all pass. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 14 minutes. |
|
Addressed the focused local CodeRabbit finding in deb9ed6: Docker workspace streams now run an independent total-deadline task that kills and reaps docker exec even while the async iterator is suspended at yield; resumption reports the timeout. Added a regression that pauses after the first chunk and observes background kill/reap before resuming. All local gates pass. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent_core/runtime/worker.py (1)
165-190: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRevalidate leases before sandbox deletion.
SandboxManager.reaptreats the point-in-time lease set as authoritative. A lease claimed after the snapshot can therefore lose its sandbox. Docker’s creation grace only delays this race and can be disabled. Revalidate each candidate before deletion, and add a concurrent claim-and-reap regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/runtime/worker.py` around lines 165 - 190, Update the sandbox reaping flow invoked by MaintenanceWorker.run_once and implemented by SandboxManager.reap to revalidate each candidate’s lease immediately before deletion, rather than relying solely on live_run_leases captured at the initial snapshot. Preserve deletion for unclaimed candidates and ensure a concurrent claim prevents deletion. Add a regression test covering a lease claimed between snapshot and reap.
🧹 Nitpick comments (14)
src/agent_core/adapters/execution/fake.py (1)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject a negative
maximum_bytesto match the Docker adapter.
DockerWorkspaceHandle.read_boundedandDockerWorkspaceHandle.streamraiseValueErrorwhenmaximum_bytes < 0. The fake accepts a negative limit and then raisesWorkspaceReadLimitExceededErrorfor any non-empty file. Aligning the fake keeps the port contract testable against both adapters.♻️ Proposed alignment
async def read_bounded(self, path: str, maximum_bytes: int) -> bytes: + if maximum_bytes < 0: + raise ValueError("maximum_bytes must not be negative") data = await self.read(path) if len(data) > maximum_bytes: raise WorkspaceReadLimitExceededError("workspace file exceeds read limit") return data async def stream(self, path: str, maximum_bytes: int) -> AsyncIterator[bytes]: + if maximum_bytes < 0: + raise ValueError("maximum_bytes must not be negative") data = await self.read_bounded(path, maximum_bytes)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/fake.py` around lines 50 - 54, Update FakeWorkspaceHandle.read_bounded to raise ValueError immediately when maximum_bytes is negative, matching DockerWorkspaceHandle.read_bounded and stream; retain the existing size-limit check and return behavior for non-negative limits.src/agent_core/execution/manager.py (1)
242-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck for bridge support before the cast.
castperforms no runtime check. If a configured adapter does not implementexecute_with_bridge, Line 251 raisesAttributeError, which surfaces as an internal error instead of a domain rejection. A guarded check keeps the failure mode explicit.♻️ Proposed guard
if bridge is not None: + if not hasattr(self._environment, "execute_with_bridge"): + raise ExecutionRejected("execution environment does not support a tool bridge") bridge_environment = cast(_BridgeExecutionEnvironment, self._environment)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/manager.py` around lines 242 - 254, In the bridge execution branch of the operation manager, validate that the configured environment supports execute_with_bridge before casting it to _BridgeExecutionEnvironment. Reject unsupported bridge execution through the established domain-level failure path, and only construct the endpoint and invoke execute_with_bridge after the capability check.src/agent_core/execution/proxy.py (1)
122-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
suppressat module level.
__import__("contextlib").suppress(OSError)performs a dynamic import inside an exception handler. A top-levelfrom contextlib import suppressis clearer and avoids the per-call lookup.♻️ Proposed change
@@ import asyncio import json import os import socket import sys +from contextlib import suppress from urllib.parse import urlsplit @@ if not writer.is_closing(): writer.write(b"HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\n\r\n") - with __import__("contextlib").suppress(OSError): + with suppress(OSError): await writer.drain()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/proxy.py` around lines 122 - 125, Import suppress from contextlib at module scope, then update the exception-handling block in the proxy response path to use that imported symbol instead of dynamically importing contextlib via __import__. Preserve the existing OSError suppression and writer.drain behavior.src/agent_core/execution/bridge_relay.py (1)
60-74: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueExit the relay when the host response channel closes.
Line 68 returns from
handlewhenresponses.readline()yields an empty result. An empty result means the host closed stdout.response_overrunstays clear, so Line 83 keeps waiting and the relay stays alive until the host sendsSIGTERMthrough_BRIDGE_STOP_SCRIPT. Every later connection then completes with no response.Set the shutdown event on that path so the relay exits by itself.
Separately, Lines 61-62 write to
sys.stdout.buffersynchronously on the event loop. A single message can reach 65537 bytes, which exceeds the default pipe buffer, so the write can block the loop until the host drains the pipe.loop.connect_write_pipewith aStreamWriterwould keep the write non-blocking.♻️ Proposed exit on host EOF
if not response: + response_overrun.set() returnAlso applies to: 80-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/bridge_relay.py` around lines 60 - 74, Update handle so an empty result from responses.readline() sets response_overrun before returning, allowing the relay shutdown path to terminate without waiting for SIGTERM. Replace the synchronous sys.stdout.buffer.write/flush in the response relay with an asyncio StreamWriter created through loop.connect_write_pipe, and await its drain so large host responses do not block the event loop.src/agent_core/tools/sandbox_run_command.py (1)
124-131: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWire
output.hard_ceiling_multiplierinto both output paths. The executor bounds the model and persisted payload before_finish. However, the loaded configuration value is ignored, and bothsrc/agent_core/tools/sandbox_run_command.pyandsrc/agent_core/tools/executor.pyhard-code4.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/sandbox_run_command.py` around lines 124 - 131, The output-size multiplier is hard-coded as 4 in both the ExecutionCommand construction in sandbox_run_command and the corresponding output handling in executor.py. Load and reuse the configured output.hard_ceiling_multiplier in both paths, replacing the literals while preserving the existing bounds calculations and behavior.tests/contract/test_artifact_store_contract.py (1)
67-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
dataclasses.replaceinstead of positional construction.Both blocks pass 13 positional arguments to
ArtifactMetadataonly to override one or two fields. Any added or reordered field inArtifactMetadatasilently changes what these tests assert.replaceis already imported and used on line 48.♻️ Proposed refactor
- broken = ArtifactMetadata( - metadata.artifact_id, - metadata.tenant_id, - metadata.principal_id, - metadata.session_id, - metadata.run_id, - metadata.origin, - metadata.filename, - metadata.media_type, - metadata.size_bytes, - "0" * 64, - metadata.trust, - metadata.created_at, - metadata.expires_at, - ) + broken = replace(metadata, sha256="0" * 64)- replacement_metadata = ArtifactMetadata( - metadata.artifact_id, - ... - metadata.expires_at, - ) + replacement_metadata = replace( + metadata, + size_bytes=len(replacement), + sha256=hashlib.sha256(replacement).hexdigest(), + )Also applies to: 116-130
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contract/test_artifact_store_contract.py` around lines 67 - 81, Replace the positional `ArtifactMetadata` constructions in both affected test blocks with `dataclasses.replace`, reusing the existing metadata instance and overriding only the fields needed for each case. Preserve the current altered values and assertions, following the existing `replace` usage in the test.src/agent_core/application/artifact_writer.py (1)
25-29: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOffload the spool file I/O off the event loop.
SpooledTemporaryFilerolls over to a real file after 8 MiB, andmaximum_bytesallows up to 512 MiB._file_streamthen performs blockingsource.readcalls on the event loop, andcreateperforms blockingspool.writecalls on line 75.FilesystemArtifactStore.putalready offloads its writes withasyncio.to_thread, so the read and spool sides are the remaining blocking hops for large artifacts.Move both to a worker thread.
♻️ Proposed refactor
+import asyncio + async def _file_stream( source: _ReadableBytes, chunk_bytes: int = 64 * 1024 ) -> AsyncIterator[bytes]: - while chunk := source.read(chunk_bytes): + while chunk := await asyncio.to_thread(source.read, chunk_bytes): yield chunkApply the same treatment to the spool write:
- spool.write(chunk) + await asyncio.to_thread(spool.write, chunk)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/application/artifact_writer.py` around lines 25 - 29, Update _file_stream to perform each blocking source.read call via asyncio.to_thread before yielding the chunk, and update create’s spool.write path to use asyncio.to_thread as well. Preserve the existing chunking and streaming behavior while ensuring both file I/O operations run off the event loop.tests/gates/test_artifact_m6.py (1)
299-308: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise artifactization through the public dispatch path.
This gate calls the private
pipeline._artifactize_large_outputdirectly. The wiring in_execute_onceatsrc/agent_core/tools/executor.pyLines 785-790, thevalidate_outputordering, and the_finishpersistence ofartifact_id,output_bytes, andtruncatedat Lines 1288-1306 stay uncovered. A regression that stops calling_artifactize_large_outputfrom_execute_oncewould still pass this gate.The exact byte assertions add a second problem.
5027,4000, and1027derive from the JSON encoding of aTextPart. Any added field onTextPartchangesrenderedand breaks all three assertions for a reason unrelated to artifactization. Assert the relationships instead, for examplecaptured_bytes == tool.spec.maximum_output_bytes * 4anddiscarded_bytes == output_bytes - captured_bytes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_artifact_m6.py` around lines 299 - 308, Update the test to invoke artifactization through the public execution/dispatch path rather than calling the private pipeline._artifactize_large_output method directly, covering _execute_once, validate_output ordering, and _finish persistence of artifact_id, output_bytes, and truncated. Replace hard-coded byte totals with relationship-based assertions, including captured_bytes as tool.spec.maximum_output_bytes * 4 and discarded_bytes as output_bytes - captured_bytes, while preserving assertions that truncation and artifact metadata are persisted.src/agent_core/tools/executor.py (2)
754-764: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive the artifact origin from the tool target instead of hard-coding
SANDBOX_EXPORT.Every tool receives a writer bound to
ArtifactOrigin.SANDBOX_EXPORT, including tools whosetarget_kindis not"sandbox". A non-sandbox tool that writes throughcontext.artifactsthen persists an artifact row whoseoriginclaims a sandbox export._artifactize_large_outputalready selectsArtifactOrigin.TOOL_OUTPUTfor its own writer, so the two paths disagree on provenance labeling.Origin is stored provenance metadata that operators and the API surface read, so it should reflect the actual producer.
♻️ Proposed refactor
else self._artifact_writers.for_run( tenant_id=run.tenant_id, principal_id=principal.principal_id, session_id=run.session_id, run_id=run.id, - origin=ArtifactOrigin.SANDBOX_EXPORT, + origin=( + ArtifactOrigin.SANDBOX_EXPORT + if tool.spec.target_kind == "sandbox" + else ArtifactOrigin.TOOL_OUTPUT + ), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 754 - 764, Update the artifact writer construction in the executor flow to derive `ArtifactOrigin` from the tool’s `target_kind` instead of always using `ArtifactOrigin.SANDBOX_EXPORT`; preserve sandbox exports as sandbox-origin artifacts and assign the appropriate non-sandbox origin consistently with `_artifactize_large_output`’s `ArtifactOrigin.TOOL_OUTPUT` path.
882-884: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated trust-resolution rule.
These three lines repeat the rule at
_finishLines 1277-1279 exactly. Trust resolution decides whether tool output is treated asEXTERNAL_UNTRUSTED, so the two copies must not drift.Move the rule into one helper and call it from both sites.
♻️ Proposed refactor
+def _effective_trust(result: ToolResult, tool: Tool) -> TrustLevel: + if tool.spec.output_trust is TrustLevel.EXTERNAL_UNTRUSTED: + return TrustLevel.EXTERNAL_UNTRUSTED + return result.output_trust or tool.spec.output_trustThen at both call sites:
- trust = result.output_trust or tool.spec.output_trust - if tool.spec.output_trust is TrustLevel.EXTERNAL_UNTRUSTED: - trust = TrustLevel.EXTERNAL_UNTRUSTED + trust = _effective_trust(result, tool)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 882 - 884, Extract the trust-resolution logic into a shared helper near the existing executor utilities, preserving the rule that tool.spec.output_trust overrides result.output_trust when it is TrustLevel.EXTERNAL_UNTRUSTED. Replace the duplicated logic in the current site and _finish with calls to this helper so both paths use one implementation.tests/contract/test_artifact_repository_contract.py (1)
42-47: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAuthorization Bypass (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)
Reachability: Internal
Add a principal-boundary case.
Assert
NotFoundErrorfortenant-awithprincipal_id="user-b". This detects regressions that remove theprincipal_idpredicate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contract/test_artifact_repository_contract.py` around lines 42 - 47, Extend the contract test around repository.get to assert NotFoundError for the same artifact with Principal(tenant_id="tenant-a", principal_id="user-b"), while preserving the existing tenant-b case. This verifies lookup remains bound to both tenant_id and principal_id.tests/security/test_sandbox_runtime_m6.py (2)
125-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the local dictionary so it does not shadow the
secretsmodule.Line 8 imports
secrets. Line 125 binds a local dictionary to the same name. Inside this function the module becomes unreachable. A later edit that callssecrets.token_urlsafe()here raisesAttributeError.♻️ Proposed change
- secrets = { + parent_secrets = { "OPENAI_API_KEY": "synthetic-provider-value-7d951", "AGENT_DATABASE_URL": "synthetic-database-value-0be44", "AWS_SECRET_ACCESS_KEY": "synthetic-cloud-value-18c12", "PRIVATE_SERVICE_TOKEN": "synthetic-pattern-value-19d42", }Update the four later references (
parent=secrets,set(secrets),secrets.values()) toparent_secrets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/test_sandbox_runtime_m6.py` around lines 125 - 130, Rename the local dictionary in the affected test function from secrets to parent_secrets, and update all four subsequent references—including the parent argument, key-set access, and values access—so the imported secrets module remains available.
274-277: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPoll for proxy readiness instead of sleeping for a fixed interval.
Line 275 waits 0.25 s for the egress proxy to accept connections. On a loaded CI machine the proxy can still be unready. The
allowedassertion at Line 279 then fails for a timing reason, not a policy reason. A bounded readiness poll removes that flake.♻️ Proposed change
- await asyncio.sleep(0.25) + for _attempt in range(40): + probe = await _execute( + adapter, + handle, + "import urllib.request; urllib.request.urlopen('http://example.com:80',timeout=2)", + ) + if probe.exit_code == 0: + break + await asyncio.sleep(0.25) + else: + raise AssertionError("egress proxy did not become ready") result = await _execute(adapter, handle, script, timeout=12)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/security/test_sandbox_runtime_m6.py` around lines 274 - 277, Replace the fixed asyncio.sleep in the _environment test flow with a bounded poll that checks egress proxy readiness before calling _execute. Reuse the existing adapter/handle readiness mechanism if available, stop when the proxy accepts connections, and retain a timeout so failures remain deterministic rather than hanging.tests/contract/test_execution_environment_contract.py (1)
231-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBound the state busy-wait loops so a regression fails instead of hanging.
Several tests spin on private manager state with
while ...: await asyncio.sleep(0)(Lines 231-232, 256-257, 281-282, 307-308, 330-331). If a future change stops setting that state, the loop never exits and the test hangs until the CI job timeout. A bounded helper turns the same regression into a fast failure.♻️ Proposed helper
async def _await_condition(predicate: Callable[[], bool], *, timeout: float = 1.0) -> None: async def _spin() -> None: while not predicate(): await asyncio.sleep(0) await asyncio.wait_for(_spin(), timeout)- while run_id not in manager._released_runs: - await asyncio.sleep(0) + await _await_condition(lambda: run_id in manager._released_runs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contract/test_execution_environment_contract.py` around lines 231 - 233, Replace the unbounded state-polling loops in the affected contract tests with a shared bounded async helper such as _await_condition, using asyncio.wait_for around the existing sleep(0) spin and a short default timeout. Update each loop that waits on manager state, including _released_runs and the other listed conditions, while preserving the existing assertions and predicates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agent_core/adapters/artifacts/filesystem.py`:
- Around line 161-164: Update _restore_claim to suppress FileNotFoundError from
os.link in addition to FileExistsError, while preserving the existing
claim.unlink cleanup so concurrent reconcile_orphans passes do not abort when
the claim was already removed.
In `@src/agent_core/adapters/execution/docker.py`:
- Around line 680-707: Update _monitor_workspace_limits to use a substantially
longer polling interval with backoff, retaining prompt shutdown and limit
detection while reducing repeated docker exec calls. Configure workspace volume
quotas where the supported storage driver allows it, using monitoring only as a
safety net. Optimize _snapshot by skipping hashing for unchanged files based on
size and mtime, and avoid hashing files above the applicable workspace-size
bound.
In `@src/agent_core/adapters/execution/local_workspace.py`:
- Around line 216-217: Update LocalWorkspace.for_run to include lease_epoch in
the workspace handle cache key and generated host-directory path, ensuring
reclaimed runs receive distinct scoped workspaces. Add or update the
lease-handoff contract test to verify different lease epochs produce different
handles and directories.
In `@src/agent_core/application/public_services.py`:
- Around line 859-869: The artifact download flow should route using explicit
trajectory membership rather than artifact.origin, and eagerly open/validate the
backing object before creating the streaming response. Update the relevant
method around _artifacts.stream and _general_artifacts.open to use the
established trajectory-membership signal, translate FileNotFoundError to
NotFoundError, and preserve ArtifactIntegrityError while retaining streaming
after successful validation.
In `@src/agent_core/bootstrap.py`:
- Around line 666-668: Throttle store-wide orphan reconciliation by registering
a cadence-aware artifact maintenance service in src/agent_core/bootstrap.py
lines 666-668 instead of passing the raw reconcile_artifact_orphans callback. In
src/agent_core/runtime/worker.py lines 211-215, update
MaintenanceWorker.run_once to invoke orphan reconciliation only when its
separate coarse interval is due; preserve the existing frequent maintenance loop
for other tasks.
- Around line 518-525: Extend the maintenance flow associated with
ArtifactWriterFactory to add an idempotent expiry sweep for general artifacts:
remove expired metadata and their stored bytes based on retention_days, while
preserving reconcile_orphans for incomplete commits. Ensure repeated sweeps are
safe and locate the implementation through ArtifactWriterFactory and the
existing general-artifact maintenance methods.
In `@src/agent_core/execution/proxy.py`:
- Around line 91-109: Update _handle so plaintext HTTP traffic cannot relay
pipelined requests under the initial host authorization: parse each subsequent
request and run _resolved, evaluate_core, and _log for its Host, rejecting
unauthorized or changed hosts, or close both connections after completing only
one request. Preserve CONNECT tunneling behavior while ensuring every permitted
HTTP request is host-scoped and audited.
In `@src/agent_core/runtime/executor.py`:
- Line 64: Update RunCompleteCallback and the execute_claimed completion flow in
src/agent_core/runtime/executor.py (lines 64-64 and 97-118) to pass
claimed.lease alongside run_id; update the completion handler and
SandboxManager.release_run call in lines 225-247 to tear down only the matching
(run_id, lease_epoch), while keeping inline cleanup separate. Update the
callback wiring in src/agent_core/bootstrap.py (line 565) to accept and forward
the lease argument.
In `@src/agent_core/tools/artifact_export.py`:
- Around line 84-123: Update the exception handling in the artifact export flow
around workspace.stream and writer.create to catch ArtifactIntegrityError
alongside WorkspaceReadLimitExceededError, mapping it to the existing
OUTPUT_TOO_LARGE/tool.output_invalid failure response. Add the required
ArtifactIntegrityError import and preserve the current non-retryable detail
behavior.
In `@src/agent_core/tools/bridge.py`:
- Around line 96-113: Update the timeout handling in the bridge call method
around self._ordinal and bridge_call_id so the returned suspension cannot
advertise an invalid retry path: either mark bridge.approval_hold_expired as
non-retryable, or implement compatible ordinal exposure/rollback with explicit
duplicate-dispatch behavior. Preserve unique call-id and ordinal accounting for
subsequent invocations.
In `@src/agent_core/tools/executor.py`:
- Around line 922-930: Update the structured-output handling in _execute_once
and _artifactize_large_output so any stdout or stderr truncation remains
consistent with the declared output_schema and is visible to consumers; either
re-validate the mutated structured value before _finish persists it, or add an
explicit truncation indicator within structured and preserve that through the
returned structured_result.
In `@tests/contract/test_programmatic_bridge_m6.py`:
- Around line 26-31: Replace every literal token passed to
ProgrammaticBridgeSession in tests/contract/test_programmatic_bridge_m6.py at
lines 28, 77, 103, and 125 with a module-level secrets.token_urlsafe(16) value,
and reuse that value in the corresponding request payloads; also replace the
token literal in tests/security/test_sandbox_runtime_m6.py at lines 188-192 with
secrets.token_urlsafe(32).
In `@tests/gates/test_policy_m4.py`:
- Around line 335-338: Update the deadline-sensitive test around
active_run.model_copy to configure a fixed clock when building the application,
ensuring the 250 ms deadline remains stable while dispatch() computes
effective_timeout. Preserve the existing deadline and timeout assertions.
---
Outside diff comments:
In `@src/agent_core/runtime/worker.py`:
- Around line 165-190: Update the sandbox reaping flow invoked by
MaintenanceWorker.run_once and implemented by SandboxManager.reap to revalidate
each candidate’s lease immediately before deletion, rather than relying solely
on live_run_leases captured at the initial snapshot. Preserve deletion for
unclaimed candidates and ensure a concurrent claim prevents deletion. Add a
regression test covering a lease claimed between snapshot and reap.
---
Nitpick comments:
In `@src/agent_core/adapters/execution/fake.py`:
- Around line 50-54: Update FakeWorkspaceHandle.read_bounded to raise ValueError
immediately when maximum_bytes is negative, matching
DockerWorkspaceHandle.read_bounded and stream; retain the existing size-limit
check and return behavior for non-negative limits.
In `@src/agent_core/application/artifact_writer.py`:
- Around line 25-29: Update _file_stream to perform each blocking source.read
call via asyncio.to_thread before yielding the chunk, and update create’s
spool.write path to use asyncio.to_thread as well. Preserve the existing
chunking and streaming behavior while ensuring both file I/O operations run off
the event loop.
In `@src/agent_core/execution/bridge_relay.py`:
- Around line 60-74: Update handle so an empty result from responses.readline()
sets response_overrun before returning, allowing the relay shutdown path to
terminate without waiting for SIGTERM. Replace the synchronous
sys.stdout.buffer.write/flush in the response relay with an asyncio StreamWriter
created through loop.connect_write_pipe, and await its drain so large host
responses do not block the event loop.
In `@src/agent_core/execution/manager.py`:
- Around line 242-254: In the bridge execution branch of the operation manager,
validate that the configured environment supports execute_with_bridge before
casting it to _BridgeExecutionEnvironment. Reject unsupported bridge execution
through the established domain-level failure path, and only construct the
endpoint and invoke execute_with_bridge after the capability check.
In `@src/agent_core/execution/proxy.py`:
- Around line 122-125: Import suppress from contextlib at module scope, then
update the exception-handling block in the proxy response path to use that
imported symbol instead of dynamically importing contextlib via __import__.
Preserve the existing OSError suppression and writer.drain behavior.
In `@src/agent_core/tools/executor.py`:
- Around line 754-764: Update the artifact writer construction in the executor
flow to derive `ArtifactOrigin` from the tool’s `target_kind` instead of always
using `ArtifactOrigin.SANDBOX_EXPORT`; preserve sandbox exports as
sandbox-origin artifacts and assign the appropriate non-sandbox origin
consistently with `_artifactize_large_output`’s `ArtifactOrigin.TOOL_OUTPUT`
path.
- Around line 882-884: Extract the trust-resolution logic into a shared helper
near the existing executor utilities, preserving the rule that
tool.spec.output_trust overrides result.output_trust when it is
TrustLevel.EXTERNAL_UNTRUSTED. Replace the duplicated logic in the current site
and _finish with calls to this helper so both paths use one implementation.
In `@src/agent_core/tools/sandbox_run_command.py`:
- Around line 124-131: The output-size multiplier is hard-coded as 4 in both the
ExecutionCommand construction in sandbox_run_command and the corresponding
output handling in executor.py. Load and reuse the configured
output.hard_ceiling_multiplier in both paths, replacing the literals while
preserving the existing bounds calculations and behavior.
In `@tests/contract/test_artifact_repository_contract.py`:
- Around line 42-47: Extend the contract test around repository.get to assert
NotFoundError for the same artifact with Principal(tenant_id="tenant-a",
principal_id="user-b"), while preserving the existing tenant-b case. This
verifies lookup remains bound to both tenant_id and principal_id.
In `@tests/contract/test_artifact_store_contract.py`:
- Around line 67-81: Replace the positional `ArtifactMetadata` constructions in
both affected test blocks with `dataclasses.replace`, reusing the existing
metadata instance and overriding only the fields needed for each case. Preserve
the current altered values and assertions, following the existing `replace`
usage in the test.
In `@tests/contract/test_execution_environment_contract.py`:
- Around line 231-233: Replace the unbounded state-polling loops in the affected
contract tests with a shared bounded async helper such as _await_condition,
using asyncio.wait_for around the existing sleep(0) spin and a short default
timeout. Update each loop that waits on manager state, including _released_runs
and the other listed conditions, while preserving the existing assertions and
predicates.
In `@tests/gates/test_artifact_m6.py`:
- Around line 299-308: Update the test to invoke artifactization through the
public execution/dispatch path rather than calling the private
pipeline._artifactize_large_output method directly, covering _execute_once,
validate_output ordering, and _finish persistence of artifact_id, output_bytes,
and truncated. Replace hard-coded byte totals with relationship-based
assertions, including captured_bytes as tool.spec.maximum_output_bytes * 4 and
discarded_bytes as output_bytes - captured_bytes, while preserving assertions
that truncation and artifact metadata are persisted.
In `@tests/security/test_sandbox_runtime_m6.py`:
- Around line 125-130: Rename the local dictionary in the affected test function
from secrets to parent_secrets, and update all four subsequent
references—including the parent argument, key-set access, and values access—so
the imported secrets module remains available.
- Around line 274-277: Replace the fixed asyncio.sleep in the _environment test
flow with a bounded poll that checks egress proxy readiness before calling
_execute. Reuse the existing adapter/handle readiness mechanism if available,
stop when the proxy accepts connections, and retain a timeout so failures remain
deterministic rather than hanging.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ccc7fc16-8e40-4d63-bde3-c4fe0d1933f7
📒 Files selected for processing (66)
.circleci/config.ymlMakefiledocs/adr/0042-milestone-6-sandbox-and-artifact-seams.mddocs/adr/index.mddocs/plan/current-milestone.mddocs/status/project-state.yamlevals/gates/sandbox.yamlexecution/sandbox.Dockerfilemkdocs.ymlpyproject.tomlsrc/agent_core/adapters/artifacts/filesystem.pysrc/agent_core/adapters/execution/docker.pysrc/agent_core/adapters/execution/fake.pysrc/agent_core/adapters/execution/local_workspace.pysrc/agent_core/adapters/persistence/mappers.pysrc/agent_core/adapters/persistence/memory.pysrc/agent_core/adapters/persistence/repositories.pysrc/agent_core/adapters/persistence/unit_of_work.pysrc/agent_core/application/artifact_writer.pysrc/agent_core/application/public_services.pysrc/agent_core/bootstrap.pysrc/agent_core/config.pysrc/agent_core/domain/artifacts.pysrc/agent_core/domain/credentials.pysrc/agent_core/domain/errors.pysrc/agent_core/domain/execution.pysrc/agent_core/domain/tools.pysrc/agent_core/domain/trajectory.pysrc/agent_core/execution/__init__.pysrc/agent_core/execution/bridge_relay.pysrc/agent_core/execution/egress.pysrc/agent_core/execution/egress_core.pysrc/agent_core/execution/environment.pysrc/agent_core/execution/manager.pysrc/agent_core/execution/proxy.pysrc/agent_core/ports/artifacts.pysrc/agent_core/ports/credentials.pysrc/agent_core/ports/execution.pysrc/agent_core/ports/persistence.pysrc/agent_core/ports/repositories.pysrc/agent_core/runtime/executor.pysrc/agent_core/runtime/worker.pysrc/agent_core/sandbox/limits.yamlsrc/agent_core/tools/artifact_export.pysrc/agent_core/tools/bridge.pysrc/agent_core/tools/executor.pysrc/agent_core/tools/sandbox_run_command.pytests/contract/support.pytests/contract/test_artifact_repository_contract.pytests/contract/test_artifact_store_contract.pytests/contract/test_artifact_writer_contract.pytests/contract/test_artifact_writer_provider_contract.pytests/contract/test_credential_resolver_contract.pytests/contract/test_execution_environment_contract.pytests/contract/test_maintenance_repository_contract.pytests/contract/test_programmatic_bridge_m6.pytests/contract/test_workspace_handle_contract.pytests/gates/test_api_m5.pytests/gates/test_artifact_m6.pytests/gates/test_gate_registry.pytests/gates/test_policy_m4.pytests/gates/test_sandbox_m6.pytests/integration/test_artifact_persistence_m6.pytests/security/test_sandbox_runtime_m6.pytests/unit/test_config.pytests/unit/test_toolchain.py
|
Full-review remediation is pushed in 8b81ec9. The outside-diff lease-reaper finding is fixed: maintenance supplies a repository-backed live-lease recheck for each candidate immediately before deletion, and a regression covers a claim acquired after the initial snapshot. All 14 nitpicks are also addressed: negative fake read bounds; off-loop artifact spool I/O; relay EOF/backpressure; bridge capability checks; module-level suppress; configured hard-ceiling wiring; shared trust resolution; principal scoping; dataclasses.replace fixtures; bounded async test polling; public-dispatch artifactization coverage; non-shadowing secret names; and removal of the redundant fixed proxy sleep because provisioning already performs a bounded readiness poll. For artifact origin, the implementation preserves the normative plan rule that artifact.export is SANDBOX_EXPORT even though the tool is intentionally in_process. Origin selection is now centralized: artifact.export uses SANDBOX_EXPORT and other tool-created output uses TOOL_OUTPUT. Final local verification on this commit:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/agent_core/adapters/execution/docker.py (1)
1125-1130: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not let a live lease override environment expiry.
If
expiredis true andis_live()returns true, this branch skips cleanup. The container can then remain allocated after itsexpires_atdeadline and bypass the configured sandbox hard cap.Call
is_live()only for non-expired resources.Proposed fix
- if not expired and (run_id, lease_epoch) in live_leases: - continue - if not expired and not old_enough: - continue - if is_live is not None and await is_live(run_id, lease_epoch): - continue + if not expired: + if (run_id, lease_epoch) in live_leases: + continue + if not old_enough: + continue + if is_live is not None and await is_live(run_id, lease_epoch): + continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/execution/docker.py` around lines 1125 - 1130, Update the cleanup conditions around live_leases and is_live so is_live(run_id, lease_epoch) is evaluated only when expired is false; expired resources must proceed with cleanup regardless of the callback result, while preserving the existing handling for non-expired leases.src/agent_core/api/app.py (1)
440-451: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-525): Use of Web Browser Cache Containing Sensitive Information
Reachability: External · Exploitability: Moderate
Reachability path
● Entry src/agent_core/bootstrap.py:634 PublicArtifactService │ ▼ ● Hop src/agent_core/application/public_services.py:839 _get_ref: ArtifactRef requires an expiry, and persistence mapping rejects legacy null rows. │ ▼ ● Sink src/agent_core/api/app.pyAdd
Cache-Control: private, no-storeto both artifact responses.Authorization does not replace an explicit cache policy. The endpoint returns principal-scoped bytes, but user-agent caches can retain the response. Apply the header to both
200and304responses. No global middleware sets it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/api/app.py` around lines 440 - 451, Update the artifact response handling around the ETag match so both the 304 Response and the 200 StreamingResponse include Cache-Control: private, no-store in their headers, while preserving the existing ETag and response behavior.src/agent_core/execution/proxy.py (1)
94-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External · Exploitability: Moderate
Reachability path
● Entry src/agent_core/execution/bridge_relay.py:40 handle │ ▼ ● Sink src/agent_core/execution/proxy.pyReject non-
httpabsolute URIs in the non-CONNECTpath.For
httpstargets, this path opens a plain TCP connection and forwards headers without TLS. Reject non-httpschemes before dialing, or establish TLS with SNI and hostname validation. Add a regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/execution/proxy.py` around lines 94 - 100, Update the non-CONNECT proxy request path around urlsplit(raw_target) to reject absolute targets whose scheme is not http before opening the TCP connection; do not allow https or other schemes to proceed without TLS. Add a regression test covering rejection of a non-http absolute URI.
🧹 Nitpick comments (7)
tests/integration/test_artifact_persistence_m6.py (1)
88-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the unexpired artifact survives the sweep.
The test proves the expired
TOOL_OUTPUTartifact is removed. It does not prove the unexpiredSANDBOX_EXPORTartifact is retained.PostgresArtifactRepository.list_expiredanddelete_expiredare new SQL in this PR, and theirexpires_at <= nowpredicate is the guard that protects live artifacts. A predicate inversion would still pass this test.Add one assertion for the retained artifact.
💚 Proposed addition
assert await expiry.sweep_expired() == 1 assert await expiry.sweep_expired() == 0 async with composition.uow_factory() as uow: assert await uow.artifacts.exists(expired_ref.artifact_id) is False + assert await uow.artifacts.exists(ref.artifact_id) is True🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/test_artifact_persistence_m6.py` around lines 88 - 91, Add an assertion in the existing uow block after the expired artifact check to verify the unexpired SANDBOX_EXPORT artifact still exists, using its artifact ID and the same artifacts.exists method.src/agent_core/adapters/persistence/repositories.py (1)
1465-1489: 🚀 Performance & Scalability | 🔵 TrivialConsider a partial index for the general-artifact expiry sweep.
Both sweep queries filter on
origin != 'trajectory_export'together withexpires_at <= now. Onlyix_artifacts_expires_atexists today, so PostgreSQL must filter the origin predicate after the index scan. As the trajectory-export share of the table grows, each sweep reads more rows than it returns.A partial index on
expires_atrestricted to non-trajectory origins keeps the orderedLIMITscan tight.The predicate semantics look correct:
expires_atis nullable, and a NULL value makesexpires_at <= nowunknown, so legacy rows without an expiry are never swept.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/adapters/persistence/repositories.py` around lines 1465 - 1489, Add a PostgreSQL partial index on ArtifactRow.expires_at restricted to rows whose origin is not "trajectory_export", alongside the existing artifact indexes. Keep the predicate aligned with list_expired and delete_expired so nullable expires_at values remain excluded and the ordered expiry sweep can use the index efficiently.src/agent_core/tools/executor.py (1)
953-980: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueArtifactization replaces any artifact the tool already produced.
The
model_copyupdate setsartifactsto a single-element list holding the output artifact. It discards every entry the tool returned._finishthen readsresult.artifacts[0]at Line 1309 and persists that id asinvocation.artifact_id, so a tool that produced its own artifact loses that reference whenever its rendered output exceeds the budget.
artifact.exportis the only current artifact-producing tool, and its structured output is small, so the oversized branch is not reached today. Append instead of replace, so the contract stays correct for the next artifact-producing tool.♻️ Proposed change
- "artifacts": [ - { - "artifact_id": str(ref.artifact_id), - "sha256": ref.sha256, - "size_bytes": ref.size_bytes, - "media_type": ref.media_type, - } - ], + "artifacts": [ + *result.artifacts, + { + "artifact_id": str(ref.artifact_id), + "sha256": ref.sha256, + "size_bytes": ref.size_bytes, + "media_type": ref.media_type, + }, + ],If you apply this change, note that
_finishstill records only the first entry. Decide whetherinvocation.artifact_idshould track the tool's own artifact or the truncated-output artifact, and select that entry explicitly rather than by position.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/tools/executor.py` around lines 953 - 980, Update the oversized-output branch in the executor’s model_copy artifact handling to append the truncated-output artifact to the existing result.artifacts list instead of replacing it. Then update _finish to explicitly select the intended artifact entry for invocation.artifact_id, preserving the tool-produced artifact reference while consistently choosing whether the invocation tracks that artifact or the truncated-output artifact.tests/contract/test_trajectory_artifact_store_contract.py (1)
44-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the eager-failure contract of
open_verifieddirectly.
src/agent_core/api/app.pyawaitscontent.open()before it constructsStreamingResponse, so the API depends onopen_verifiedraising before any chunk is produced. This test only reaches that behavior throughstream, where iteration and verification are indistinguishable. A regression that moved verification back into the iterator would still pass.Add one assertion that awaiting
open_verifiedraises without iterating.💚 Proposed addition
with pytest.raises(ArtifactIntegrityError, match="digest or size"): _ = [chunk async for chunk in store.stream(stored)] + with pytest.raises(ArtifactIntegrityError, match="digest or size"): + await store.open_verified(stored)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/contract/test_trajectory_artifact_store_contract.py` around lines 44 - 58, Extend test_trajectory_artifact_store_rejects_integrity_drift to call and await open_verified on the tampered stored artifact, asserting ArtifactIntegrityError with the existing “digest or size” match before any iteration occurs. Keep the stream assertion as-is to continue covering chunked reads.src/agent_core/application/artifact_writer.py (2)
113-118: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe rollback delete can mask the original persistence failure.
If
uow.artifacts.createfails andself._store.deletethen raises, the storage error replaces the persistence error that the caller needs to see. Theexcept BaseExceptionclause also catchesasyncio.CancelledError, and anawaitduring cancellation is not guaranteed to complete, so the orphaned bytes may remain anyway.Suppress and log the rollback error, then re-raise the original exception.
♻️ Proposed change
try: async with self._uow_factory() as uow: await uow.artifacts.create(artifact) except BaseException: - await self._store.delete(stored, tenant_id=self._tenant_id) + try: + await self._store.delete(stored, tenant_id=self._tenant_id) + except Exception: + logger.exception( + "artifact_rollback_delete_failed", + extra={"artifact_id": str(artifact_id)}, + ) raiseThe orphan reconciliation sweep referenced in
runtime/worker.pyremains the backstop for bytes that survive a failed rollback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/application/artifact_writer.py` around lines 113 - 118, Update the exception handling around the UoW artifact creation in the artifact-writing method to preserve and re-raise the original persistence exception. Attempt self._store.delete as rollback, but catch, suppress, and log any rollback failure; avoid allowing cancellation handling to replace the original error, while leaving orphan cleanup to the existing reconciliation sweep.
178-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the broad
exceptas intentional for Ruff.Ruff reports BLE001 on Line 182. The broad catch is correct here, because the loop must continue and aggregate every per-artifact failure into the
ExceptionGroup. Add a scoped suppression with a reason so the lint gate stays green and the intent stays documented.♻️ Proposed change
- except Exception as exc: + # Aggregate every per-artifact failure; one bad artifact must not stop the sweep. + except Exception as exc: # noqa: BLE001 failures.append(exc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_core/application/artifact_writer.py` around lines 178 - 186, In the expired-artifact deletion loop around the broad exception handler in the artifact writer method, add a narrowly scoped Ruff BLE001 suppression with a clear reason documenting that failures are collected in failures so processing continues and the final ExceptionGroup reports them together.Source: Linters/SAST tools
tests/gates/test_artifact_m6.py (1)
395-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the hard-ceiling multiplier in the test instead of relying on the default.
Line 441 computes
captured_bytes = tool.spec.maximum_output_bytes * 4. The4mirrors the defaulthard_ceiling_multiplierinToolPipeline.__init__, but thisToolPipelinenever passes that argument. If the default changes, the test fails with an opaque byte-count mismatch rather than pointing at the multiplier.Pass the multiplier explicitly and derive the expected size from it.
💚 Proposed change
+ hard_ceiling_multiplier = 4 pipeline = ToolPipeline( registry, composition.uow_factory, composition.clock, SequenceIdFactory([UUID(int=31_000)]), artifact_writers=writers, + hard_ceiling_multiplier=hard_ceiling_multiplier, )- captured_bytes = tool.spec.maximum_output_bytes * 4 + captured_bytes = tool.spec.maximum_output_bytes * hard_ceiling_multiplierAlso applies to: 441-443
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/gates/test_artifact_m6.py` around lines 395 - 401, Update the ToolPipeline construction in the test to pass an explicit hard-ceiling multiplier, then use that same multiplier when computing captured_bytes instead of the literal 4. Keep the expected byte-count assertion aligned with the configured value so changes to ToolPipeline.__init__ defaults do not affect this test implicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agent_core/adapters/execution/docker.py`:
- Line 867: Update the provisioning flow around provision() and the before =
dict(state.snapshot) assignment to capture the initial filesystem snapshot after
/workspace/.agent-initialized is created and before the first command executes.
Ensure state.snapshot is populated from that baseline so the marker is not
reported as CREATED or assigned SANDBOX_WRITTEN provenance.
In `@src/agent_core/execution/proxy.py`:
- Around line 111-129: Update _handle() to reject repeated Content-Length
headers and accept only a single ASCII-decimal value before assigning
content_length, rather than forwarding multiple values or relying on int()
alone. Ensure invalid or duplicate headers raise ValueError before
asyncio.open_connection() is called, and add a regression test covering
rejection at that point.
---
Outside diff comments:
In `@src/agent_core/adapters/execution/docker.py`:
- Around line 1125-1130: Update the cleanup conditions around live_leases and
is_live so is_live(run_id, lease_epoch) is evaluated only when expired is false;
expired resources must proceed with cleanup regardless of the callback result,
while preserving the existing handling for non-expired leases.
In `@src/agent_core/api/app.py`:
- Around line 440-451: Update the artifact response handling around the ETag
match so both the 304 Response and the 200 StreamingResponse include
Cache-Control: private, no-store in their headers, while preserving the existing
ETag and response behavior.
In `@src/agent_core/execution/proxy.py`:
- Around line 94-100: Update the non-CONNECT proxy request path around
urlsplit(raw_target) to reject absolute targets whose scheme is not http before
opening the TCP connection; do not allow https or other schemes to proceed
without TLS. Add a regression test covering rejection of a non-http absolute
URI.
---
Nitpick comments:
In `@src/agent_core/adapters/persistence/repositories.py`:
- Around line 1465-1489: Add a PostgreSQL partial index on
ArtifactRow.expires_at restricted to rows whose origin is not
"trajectory_export", alongside the existing artifact indexes. Keep the predicate
aligned with list_expired and delete_expired so nullable expires_at values
remain excluded and the ordered expiry sweep can use the index efficiently.
In `@src/agent_core/application/artifact_writer.py`:
- Around line 113-118: Update the exception handling around the UoW artifact
creation in the artifact-writing method to preserve and re-raise the original
persistence exception. Attempt self._store.delete as rollback, but catch,
suppress, and log any rollback failure; avoid allowing cancellation handling to
replace the original error, while leaving orphan cleanup to the existing
reconciliation sweep.
- Around line 178-186: In the expired-artifact deletion loop around the broad
exception handler in the artifact writer method, add a narrowly scoped Ruff
BLE001 suppression with a clear reason documenting that failures are collected
in failures so processing continues and the final ExceptionGroup reports them
together.
In `@src/agent_core/tools/executor.py`:
- Around line 953-980: Update the oversized-output branch in the executor’s
model_copy artifact handling to append the truncated-output artifact to the
existing result.artifacts list instead of replacing it. Then update _finish to
explicitly select the intended artifact entry for invocation.artifact_id,
preserving the tool-produced artifact reference while consistently choosing
whether the invocation tracks that artifact or the truncated-output artifact.
In `@tests/contract/test_trajectory_artifact_store_contract.py`:
- Around line 44-58: Extend
test_trajectory_artifact_store_rejects_integrity_drift to call and await
open_verified on the tampered stored artifact, asserting ArtifactIntegrityError
with the existing “digest or size” match before any iteration occurs. Keep the
stream assertion as-is to continue covering chunked reads.
In `@tests/gates/test_artifact_m6.py`:
- Around line 395-401: Update the ToolPipeline construction in the test to pass
an explicit hard-ceiling multiplier, then use that same multiplier when
computing captured_bytes instead of the literal 4. Keep the expected byte-count
assertion aligned with the configured value so changes to ToolPipeline.__init__
defaults do not affect this test implicitly.
In `@tests/integration/test_artifact_persistence_m6.py`:
- Around line 88-91: Add an assertion in the existing uow block after the
expired artifact check to verify the unexpired SANDBOX_EXPORT artifact still
exists, using its artifact ID and the same artifacts.exists method.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b7f03d4-e57d-4fc3-82b0-cd9179451487
📒 Files selected for processing (40)
docs/adr/0042-milestone-6-sandbox-and-artifact-seams.mddocs/plan/current-milestone.mddocs/status/project-state.yamlsrc/agent_core/adapters/artifacts/filesystem.pysrc/agent_core/adapters/artifacts/local.pysrc/agent_core/adapters/execution/docker.pysrc/agent_core/adapters/execution/fake.pysrc/agent_core/adapters/execution/local_workspace.pysrc/agent_core/adapters/persistence/memory.pysrc/agent_core/adapters/persistence/repositories.pysrc/agent_core/api/app.pysrc/agent_core/application/artifact_writer.pysrc/agent_core/application/public_services.pysrc/agent_core/bootstrap.pysrc/agent_core/domain/views.pysrc/agent_core/execution/bridge_relay.pysrc/agent_core/execution/manager.pysrc/agent_core/execution/proxy.pysrc/agent_core/ports/artifacts.pysrc/agent_core/ports/repositories.pysrc/agent_core/runtime/executor.pysrc/agent_core/runtime/worker.pysrc/agent_core/tools/artifact_export.pysrc/agent_core/tools/bridge.pysrc/agent_core/tools/executor.pysrc/agent_core/tools/sandbox_run_command.pytests/contract/test_artifact_repository_contract.pytests/contract/test_artifact_store_contract.pytests/contract/test_execution_environment_contract.pytests/contract/test_maintenance_repository_contract.pytests/contract/test_programmatic_bridge_m6.pytests/contract/test_trajectory_artifact_store_contract.pytests/contract/test_workspace_factory_contract.pytests/gates/test_artifact_m6.pytests/gates/test_policy_m4.pytests/gates/test_proxy_m6.pytests/gates/test_sandbox_m6.pytests/integration/test_artifact_persistence_m6.pytests/integration/test_event_runtime_m2.pytests/security/test_sandbox_runtime_m6.py
🚧 Files skipped from review as they are similar to previous changes (16)
- docs/status/project-state.yaml
- src/agent_core/tools/sandbox_run_command.py
- src/agent_core/tools/artifact_export.py
- src/agent_core/ports/artifacts.py
- src/agent_core/execution/bridge_relay.py
- src/agent_core/tools/bridge.py
- tests/contract/test_artifact_repository_contract.py
- src/agent_core/adapters/persistence/memory.py
- src/agent_core/adapters/artifacts/filesystem.py
- src/agent_core/adapters/execution/fake.py
- docs/adr/0042-milestone-6-sandbox-and-artifact-seams.md
- src/agent_core/execution/manager.py
- src/agent_core/bootstrap.py
- tests/gates/test_policy_m4.py
- tests/contract/test_artifact_store_contract.py
- tests/security/test_sandbox_runtime_m6.py
|
Incremental-review remediation is pushed in 65daecd. All three outside-diff findings are fixed: expired environments now bypass lease preservation and honor the sandbox hard cap; both 200 and 304 artifact responses carry Cache-Control: private, no-store; and plaintext proxy requests reject every non-http absolute URI before resolution or dialing. All seven nitpicks are addressed as well: the PostgreSQL expiry test proves the unexpired artifact survives; revision f2a6d74b9c10 adds a partial general-artifact expiry index and round-trips cleanly; oversized output preserves tool-produced artifact references and explicitly selects the appended truncated-output artifact for invocation metadata; eager trajectory integrity failure is tested directly; rollback deletion can no longer mask the original metadata failure; the aggregate sweep catch is documented and scoped; and the hard-ceiling test pins and reuses its configured multiplier. Local verification on 65daecd:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
sandbox.run_command, the in-sandbox Unix tool bridge, tiered credential scrubbing, and bounded output artifactizationartifact.export, verified tenant-authorized downloads, and cleanupVerification
make check— 200 static and 78 contract tests, strict mypy/Ruff/docs, 116 cumulative gatesDATABASE_URL=... make migrate test-integration— 68 passed on PostgreSQL 16make test-sandbox— 8 passed against Docker, never fakeDesign decisions for owner review
Proposed ADR-0042 records the Docker-development/gVisor-production split, the stdio-backed in-sandbox bridge, active Docker workspace quota monitoring, artifact commit ordering, and one documentation conflict: the sandbox spec requires seven operator-set profile fields that the pre-existing exhaustive 106-knob inventory omits.
Summary by CodeRabbit
New Features
Security
Documentation
Tests