Repository navigation
refactor(sandbox): object-store names for sandbox object copies; read handed-in urls from the store that holds them - #97
Conversation
…read handed-in urls from the store that holds them Sandbox.write_file_from_object and VmSandbox.load_object_file replace write_file_from_s3 and load_s3_file, which stay as deprecated aliases. The implementations were already store-neutral; only the names, the s3_url parameters and the /tmp/_s3_ staging prefix said S3. Every core call site moves to the new names. The sites that read, sign or key a url they were handed now resolve the store that holds it (get_object_store_at) instead of the configured one: the VM download and image signing, the Modal VM download, ModalSandbox's object write, the rubrics verifier's trajectory reads and snapshot_agent_state's per-service capture. Under namespace routing outside an @Local run, a url the local store holds was signed or keyed by the configured store, which for S3 raised "is not an object in s3://<bucket>". Sites that mint new urls keep the configured store. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| async def write_file_from_object(self, object_url: str, destination_path: str) -> None: | ||
| """Write the object store's object at ``object_url`` into the agent process's filesystem at | ||
| ``destination_path``.""" | ||
| raise NotImplementedError(f"{self.__class__.__name__} does not support write_file_from_object") |
There was a problem hiding this comment.
If a plugin sandbox implements write_file_from_s3 but not write_file_from_object, core file-loading code now calls the new method and hits this NotImplementedError. The artifact never reaches the sandbox. The old alias helps callers using the old name, but not plugins that implement it. VmSandbox subclasses that override only load_s3_file have the same problem. Keep a path that calls those existing implementations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent_env/providers/sandbox_providers/sandbox.py
Line: 166-169
Comment:
**Existing plugin copies fail**
If a plugin sandbox implements `write_file_from_s3` but not `write_file_from_object`, core file-loading code now calls the new method and hits this `NotImplementedError`. The artifact never reaches the sandbox. The old alias helps callers using the old name, but not plugins that implement it. `VmSandbox` subclasses that override only `load_s3_file` have the same problem. Keep a path that calls those existing implementations.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
The aliases are deliberately caller-side: code that calls write_file_from_s3 / load_s3_file keeps working through them. A sandbox that only implements the old name has to rename to write_file_from_object / load_object_file. We found none: the only implementation was ModalSandbox's, renamed here, and no downstream sandbox we know of implements either name, while the one downstream caller we know of is covered by the alias.
The S3-named methods are going away in a later breaking (!) release, so this PR doesn't add compatibility code for old-name implementations.
There was a problem hiding this comment.
Thanks for the clarification. Given that the aliases are intentionally caller-side compatibility only, and there are no downstream sandboxes implementing the old names, this finding is not actionable for this PR.
The renamed methods are the intended implementation contract, and compatibility for old-name implementations can be addressed in the planned breaking release. No change is needed here; I’m withdrawing the finding.
…-file-names # Conflicts: # src/agent_env/providers/sandbox_providers/modal_vm_sandbox.py # tst/unit/providers/sandbox_providers/vm_sandbox_test.py
Comments Outside DiffThese findings could not be posted inline.
|
Summary
Object-store names for the sandbox object copies.
Sandbox.write_file_from_object(object_url, destination_path)andVmSandbox.load_object_file(object_url, destination_path)replacewrite_file_from_s3andload_s3_file.DeprecationWarningand delegate.s3_urlparameters and the/tmp/_s3_*staging prefix (now/tmp/_obj_*) said S3.ModalSandboxoverrideswrite_file_from_object;ModalVmSandboxinherits both.Handed-in URLs are read from the store that holds them. These sites resolve
get_config().get_object_store_at(url)instead of the configured store:VmSandbox._download_object_to_vmand_signed_image_urls(per image);ModalVmSandbox._download_object_to_vm;ModalSandbox.write_file_from_object;_filter_trajectoryread and_read_trajectory_text;snapshot_agent_state._capture_universe_state, which keys and writes each service's state beside the workspace capture, in the store that holds it.Under namespace routing, outside an
@localrun, a URL the local store holds used to be signed or keyed by the configured store. For S3 that raisedis not an object in s3://<bucket>. Inside an@localrun nothing changes, because the run's routed store already routed by URL. Sites that mint new URLs, like the compacted trajectory, keep the configured store.snapshot_agent_state's docstrings describe the state read as it is now (data/get, else/export-state) and no longer say S3.Not breaking: the aliases remain. Removing them later is a break that the plugin API check won't flag, since
Sandboxis reached throughSandboxProviderrather than listed as a base, so that change needs a hand-marked!.Testing
ModalSandbox.write_file_from_object, the verifier's trajectory reads (the compacted copy is still minted in the configured store), and the snapshot capture (the services land beside a local capture).tst/unit6,072 passed, 13 skipped. The existing fakes and calls moved to the new names.snapshot_agent_state's per-service capture equalled each service's/export-state: 687k Slack messages, 53k files. The round-trip export was equal as well.Labelled
bump-version.🤖 Generated with Claude Code
The PR is not ready to merge because plugins that implement only the old sandbox methods still cannot load files through core callers.
Fix with agent prompt
Summary
Sandbox object-copy methods now use store-neutral names, with the old S3 names kept as deprecated aliases. Handed-in object URLs are read from the store that owns them, and snapshot service files are saved beside their workspace capture.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR URL[Object URL] --> Store[Owning store] Store --> Sign{Can sign a URL?} Sign -->|Yes| Download[VM downloads object] Sign -->|No| Stream[Framework streams object to VM] Stream --> Check[Check copied bytes]Reviews (2) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." · Reviewed by Greptile