feat(py): add unified storage interface - #3259
Conversation
Port the unified storage primitive from PR strands-agents#3099 (TypeScript SDK) into the Python SDK. Includes Storage Protocol, InMemoryStorage, LocalFileStorage, S3Storage, key normalization, and namespace support. Adapts the context offloader plugin to accept both the new unified Storage and the legacy offloader Storage via duck-type detection, with content framing and cycle-based eviction for unified storage.
The unified Storage is now the primary type; the legacy offloader storage is aliased as _LegacyStorage to signal deprecation.
… on namespaced views Two bugs fixed from TS PR strands-agents#3258: 1. Eviction now uses _storage_for_agent(agent) so deletes route through the agent's sandbox instead of bypassing it. 2. LocalFileStorage.namespace() returns _NamespacedLocalFileStorage which preserves for_sandbox, so pre-namespaced storage still binds to the sandbox correctly.
Make test_clear async instead of using deprecated asyncio.get_event_loop(). Fix import sorting flagged by ruff.
|
Assessment: Request Changes Clean, well-documented storage primitive with solid coverage of the happy paths, key normalization, and path-traversal rejection. The main concerns are around the interaction between per-agent (sandbox-bound) storage and the new eviction bookkeeping, binary-safety on the sandbox path, and API-review process — details are in the inline comments. Review Categories
Nice work porting this over — the interface is minimal and the docstrings are genuinely helpful. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Fix binary safety: pass raw bytes to sandbox.write_file instead of decoding to UTF-8 (which crashes on images/documents) - Simplify S3Storage.read dead-code fallback for NoSuchKey detection - Wrap all boto3 calls in asyncio.to_thread to avoid blocking the loop - Add tests for binary round-trip, sandbox binary handling, and namespace preserving for_sandbox
|
Issue (Testing): Codecov reports 69.76% patch coverage with 117 uncovered lines — concentrated exactly in the risk areas: Suggestion: Add tests for (1) the sandbox-bound |
|
Assessment: Comment (re-review) Thanks for the quick turnaround — the substantive items from the first pass are addressed: the sandbox path now round-trips raw bytes, Remaining (non-blocking) items
None of these are blockers on their own — the core design is solid. |
- Make ListQuery TypeVar contravariant for Protocol compatibility - Use builtins.list[str] to avoid shadowing by list() method name - Validate frame size in _unframe_content before slicing - Scope _stored_cycles per-agent to isolate eviction tracking - Log eviction failures at debug level instead of swallowing - Add tests for sandbox paths, binary round-trip, and error handling
- Skip chmod/nonexistent-path tests on Windows (os.chmod has no effect) - Add TestUnifiedStorage class covering: framing round-trip, binary framing, frame validation errors, offload+retrieve via unified storage, cycle-based eviction, per-agent eviction scoping, eviction disabled (None), debug logging on delete failure, auto-namespacing, pre-namespaced passthrough, and evict_after_cycles validation
- Add tests for S3 error paths (write/read/delete/list errors) - Add test for NoSuchKey via response code fallback - Add test for S3 pagination with continuation token - Add test for boto client config user_agent_extra merge - Add tests for LocalFileStorage prefix narrowing, nonexistent dir, atomic write cleanup on replace failure, delete/list error paths
|
Assessment: Approve (pending API-review process) Re-reviewed at Resolved
Sole remaining item (process, not code): this PR still lacks a Great iteration — the design and test coverage are in solid shape now. |
- Strip trailing slash in _NamespacedStorage.__init__ to prevent
double-slash prefix corruption (e.g. namespace("sessions/") producing
"sessions//" which truncates keys returned by list())
- Only remove eviction tracking entry when storage.delete() succeeds,
so transient failures retry on the next cycle instead of orphaning data
- Simplify _storage_for_agent by removing unreachable else-branch
- Collapse _NamespacedLocalFileStorage into _NamespacedStorage by adding
generic for_sandbox delegation, eliminating a single-purpose subclass
|
Re-review @ Reviewed the
No new code concerns. The only outstanding item remains the process one: applying |
|
Re-review @ Reviewed
253 storage + offloader tests still pass. No new concerns — the code side has been in solid shape for several rounds now, so the only thing gating merge is the |
Description
Ports the unified Storage primitive from PR #3099 (TypeScript SDK) to the Python SDK — a single persistence interface (
write/read/delete/listover opaque bytes) that all SDK subsystems can share. This eliminates per-subsystem persistence re-invention and gives every consumer the same backend options (local file, S3, in-memory, or custom) for free.The context offloader plugin is adapted to accept both the new unified
Storageand the legacy offloaderStorage, selecting the code path at runtime via duck-type detection. Unified storage gets content framing (2-byte BE content-type header) and cycle-based eviction; the legacy path is unchanged.Session integration is deferred — Python's
SessionRepositoryis architecturally different from TS's snapshot storage and deserves its own design.Public API
New module:
strands.storageStorageProtocol (generic onListQuery, defaults tostr)All keys are opaque, '/'-separated strings. All data is raw
bytes.readreturnsNonefor missing keys.deleteis a no-op for missing keys.listreturns keys sorted ascending; empty string matches all.InMemoryStorageThread-safe dict backend. Unbounded — consumers manage eviction.
LocalFileStorageMaps key segments to filesystem paths. Writes are atomic (temp file +
os.replace). Supports sandbox routing viafor_sandbox()— the returned view routes I/O through the sandbox's file API.namespace()returns a_NamespacedLocalFileStoragethat preservesfor_sandboxthrough the namespace layer.S3Storageasyncio.to_thread(non-blocking)list_objects_v2strands-agents)namespace()for key prefix scopingKey normalization
All implementations share
_normalize_key/_normalize_prefix://→/, strips leading/trailing/..segments → raisesStorageErrorNamespace support
Composable —
storage.namespace("a").namespace("b")scopes to"a/b/"._NAMESPACEDsentinel allows SDK constructs to detect pre-scoped storage and skip auto-prefixing.Context Offloader Changes
The
ContextOffloaderconstructor now acceptsstorage: Storage | _LegacyStorage:New parameters:
evict_after_cycles: int | None = 20— entries stored more than N cycles ago are deleted onBeforeModelCallEvent. Set toNoneto disable.Internal changes:
[2-byte BE content-type length][content-type UTF-8][content bytes]WeakKeyDictionary[Agent, dict[str, int]]hasattr(storage, "for_sandbox"))"offloader/"if not already scopedDeveloper Experience
Related Issues
#3099 (TS implementation), #3258 (TS bug fixes ported here)
Type of Change
New feature
Testing
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.