Skip to content

feat: add unified storage interface - #3099

Merged
lizradway merged 19 commits into
strands-agents:mainfrom
lizradway:storage
Jul 13, 2026
Merged

lizradway merged 19 commits into
strands-agents:mainfrom
lizradway:storage

Conversation

@lizradway

@lizradway lizradway commented Jul 6, 2026

Copy link
Copy Markdown
Member

Summary

Adds the unified Storage<ListQuery> interface — the single persistence primitive for the SDK — and wires it into existing subsystems (context offloader, session manager).

API

import type { Storage } from '@strands-agents/sdk/storage'

interface Storage<ListQuery = string> {
  write(key: string, data: Uint8Array): Promise<void>
  read(key: string): Promise<Uint8Array | null>
  delete(key: string): Promise<void>
  list(query: ListQuery): Promise<string[]>
}

Four operations over opaque Uint8Array values. Keys are /-separated path-like strings. The generic ListQuery defaults to string (prefix match) but can be widened by backends like DynamoDB.

Shipped implementations:

Class Backend Import path
InMemoryStorage Map @strands-agents/sdk/storage
LocalFileStorage Filesystem (atomic writes) @strands-agents/sdk/storage
S3Storage AWS S3 (lazy SDK import) @strands-agents/sdk/storage

Developer Experience

import { Agent } from '@strands-agents/sdk'
import { LocalFileStorage } from '@strands-agents/sdk/storage'
import { ContextOffloader } from '@strands-agents/sdk/vended-plugins/context-offloader'
import { SessionManager } from '@strands-agents/sdk/session'

// One storage instance backs everything
const storage = new LocalFileStorage('./.agent-data')

const agent = new Agent({
  model,
  plugins: [new ContextOffloader({ storage })],
  sessionManager: new SessionManager({ storage }),
})

// Or scope backends with .namespace()
const offloaderStorage = storage.namespace('offloader')
const sessionStorage = storage.namespace('sessions')

Changes

Storage primitive (src/storage/storage.ts):

  • Storage<ListQuery = string> interface with write, read, delete, list
  • namespace() helper for key-prefix scoping (composable)
  • Key normalization: collapses //, strips leading/trailing /, rejects .. segments
  • StorageError in src/errors.ts; ./storage subpath export

Implementations:

  • InMemoryStorage: Map-backed, for testing and serverless
  • LocalFileStorage: atomic writes (tmp + rename), recursive directory walking, sandbox-aware via forSandbox()
  • S3Storage: lazy @aws-sdk/client-s3 import, paginated listing, prefix namespacing

Context offloader adaptation:

  • Accepts unified Storage or legacy OffloaderStorage (duck-type detection)
  • Cycle-based eviction: tracks store-time via agent.metrics.cycleCount, deletes entries older than evictAfterCycles (default 20, null disables)
  • evictAfterCycles also flows to legacy storage when the user hasn't explicitly set evictAfterTurns
  • Content-type framed into stored bytes (2-byte length prefix + UTF-8 content-type + content)

Session manager adaptation:

  • SnapshotStorageAdapter bridges unified StorageSnapshotStorage
  • SessionManagerConfig.storage accepts Storage directly (or legacy { snapshot: SnapshotStorage })

Future Work

Phase 1 of the unified storage design. Planned next:

  • Memory store integration (LocalMemoryStore accepts optional Storage)
  • Context manager integration (v2 context strategy)
  • Transcripts (append-only audit log backed by Storage)
  • Remove deprecated classes (next major)

Testing

  • Unit tests for all storage implementations (InMemoryStorage, LocalFileStorage, S3Storage)
  • Unit tests for SnapshotStorageAdapter (CRUD round-trips, pagination, error wrapping)
  • Eviction tests for unified Storage path (cycle-based boundary conditions, null-disabled)
  • All existing offloader and session tests pass unchanged
  • Coverage: key normalization, path-traversal rejection, atomic writes, sandbox routing, duck-type detection, content-type framing round-trip

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

@github-actions github-actions Bot added size/xl area-persistence Session management or checkpointing typescript Pull requests that update typescript code area-hooks Features or requests that might be implementable via hooks enhancement New feature or request labels Jul 6, 2026
@lizradway
lizradway force-pushed the storage branch 2 times, most recently from cf9da16 to 7aac8bc Compare July 6, 2026 15:53
@lizradway
lizradway marked this pull request as ready for review July 6, 2026 15:58
@lizradway
lizradway requested a review from a team as a code owner July 6, 2026 15:58
@lizradway
lizradway requested a review from opieter-aws July 6, 2026 15:58
Comment thread strands-ts/src/vended-plugins/context-offloader/plugin.ts Outdated
Comment thread strands-ts/src/storage/local-file-storage.ts Outdated
Comment thread strands-ts/src/storage/local-file-storage.ts Outdated
Comment thread strands-ts/src/storage/local-file-storage.ts
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Assessment: Request Changes

Clean, well-documented storage primitive with strong unit coverage — but the adapters that wire it into existing subsystems have a correctness gap and some contract mismatches worth resolving before merge.

Review themes
  • Content-type loss across the offloader bridge (blocking): The unified Storage stores only opaque bytes, so bridging the ContextOffloader onto it drops the MIME type and breaks full retrieval + search for text/JSON — the exact "configure once, pass everywhere" flow the PR showcases. Needs a way to persist/reconstruct content type, plus a test through the unified path (none exists today). See inline on context-offloader/plugin.ts.
  • LocalFileStorage edge cases: keys ending in .tmp are silently excluded from list(); sandbox I/O branches swallow all errors (masking failures / data loss) and the atomicity guarantee doesn't hold on the sandbox path. See inline comments.
  • API review process: This adds a substantial new public surface (Storage, three implementations, StorageError, the Plugin.initStorage hook, AgentConfig.storage) but carries no needs-api-review label. Per team/API_BAR_RAISING.md this should go through API review before merge — e.g. whether persistence should be byte-only or metadata-aware directly affects the offloader gap above.

The backwards-compat approach (duck-typed detection, deprecation aliases, SnapshotStorageAdapter) is thoughtfully done and the storage unit tests are thorough.

Comment thread strands-ts/src/vended-plugins/context-offloader/plugin.ts Outdated
Comment thread strands-ts/src/storage/s3-storage.ts Outdated
Comment thread strands-ts/src/storage/local-file-storage.ts
Comment thread strands-ts/src/storage/local-file-storage.ts Outdated
Comment thread strands-ts/src/storage/local-file-storage.ts Outdated
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Issue (Important): This PR introduces a substantial new public surface — the Storage interface and three shipped implementations, the initStorage?(storage: Storage) hook on the Plugin interface, AgentConfig.storage, and a new ./storage subpath export. Per team/API_BAR_RAISING.md this is the kind of new primitive that should carry the needs-api-review label and go through API bar-raising before merge (it's explicitly positioned as "the single persistence primitive for the SDK"). I don't see that label on the PR.

Suggestion: Add needs-api-review and confirm sign-off. A few design points worth resolving there: (1) the Storage interface has no content-type/metadata concept, which is the root cause of the offloader regression noted separately; (2) whether forSandbox() belongs on the concrete classes vs. the interface. Separately, the "I have updated the documentation" box is unchecked and no site/ docs are included for a user-facing primitive — worth adding before this ships.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Summary

Assessment: Request Changes

Well-structured PR with a clean primitive, good normalization/path-traversal guards, and thorough tests for the storage implementations themselves. The main concern is that the adapter bridging the new Storage into the existing offloader loses content-type metadata, which regresses the offloader's core retrieve/search behavior — and this path isn't covered by tests.

Review Categories
  • Correctness (blocking): The unified→offloader adapter drops content type, so text/JSON offloaded through unified storage comes back as non-searchable binary. This is the "configure once, pass everywhere" path the PR promotes, so it needs a fix + test.
  • Error semantics: LocalFileStorage sandbox branches swallow all errors as "not found," diverging from the host path and masking real failures.
  • Edge cases: .tmp list filtering aliases legitimate keys ending in .tmp; sandbox writes silently drop the documented atomicity guarantee.
  • Standards: Hardcoding the S3 us-east-1 default overrides the AWS region provider chain.
  • Process: Large new public API without needs-api-review; user-facing docs not yet added.

The overall shape of the abstraction is solid — resolving the content-type round trip is the key thing before this is ready.

@lizradway
lizradway marked this pull request as draft July 6, 2026 16:49
Comment thread strands-ts/src/session/snapshot-storage-adapter.ts
Comment thread strands-ts/src/storage/storage.ts
@github-actions

Copy link
Copy Markdown
Contributor

Re-review of 5230716 + 6ff1275 (merge of main — clean, no conflict edits to strands-ts/src)

Assessment: Comment (downgraded from my prior Approve, pending one API-shape decision)

5230716 restores the instance storage.namespace(...) API advertised in the PR description — good intent — but re-opens the interface-vs-factory question that a prior commit had settled. The one substantive item is the optional declaration on the Storage interface (inline on storage.ts:105); resolving that (make it required, or bless the factory) is all that's needed.

Details
  • API shape (main item): namespace? is optional only on the base interface while backends + the factory return type make it required — breaking the documented storage.namespace(...) DX for Storage-typed values and leaving two co-equal ways to namespace. Recommend needs-api-review since this changes the public interface contract. See inline.
  • Offloader _bind (context-offloader/storage.ts): dropping the !== null guard now lets an explicit null propagate to _evictAfterTurns = null (disable eviction). This looks like an intended correctness fix — it matches the documented "null disables" behavior and the null-disabled tests — just calling it out since it's a behavior change not mentioned in the commit message. No action needed if intended.

Everything else from prior passes remains resolved — this is down to a single API decision.

Remove outdated evictAfterTurns doc, update snapshot-storage-adapter
to reference .namespace() method, simplify namespace() return type
now that the method is on the Storage interface.
Comment thread strands-ts/src/storage/storage.ts
@github-actions

Copy link
Copy Markdown
Contributor

Re-review of 823e36f ("fix stale comments and simplify namespace return type")

Assessment: Comment

This tidies docstrings and simplifies the factory return type to plain Storage, which resolves the earlier return-type-vs-interface mismatch. However, it doesn't address the root cause and now surfaces it as two doc-vs-type contradictions (compiler-verified — see inline on storage.ts:118):

Details
  • Optional namespace? (line 105): the adapter docstring now recommends storage.namespace('session'), but that doesn't typecheck for a Storage-typed value under strictNullChecks.
  • Chaining regression: dropping the Storage & { namespace(...) } return type breaks namespace(a).namespace(b), contradicting this function's own "Composable …" docstring (line 111).
  • Fix: make namespace required on the interface — one change makes both docstrings truthful, keeps the simplified return type composable, and matches all three shipped backends. Still worth a needs-api-review label since it settles the public interface contract.

Everything else remains resolved — this is down to the single interface-declaration decision.

@github-actions

Copy link
Copy Markdown
Contributor

Re-review of tip a0a3be9 — clean merge of main into storage.

The only change since 823e36f is this merge commit; its content diff is limited to two CI release workflows (release-python.yml, release-typescript.yml) pulled in from main (#3193). No PR code changed — storage.ts, snapshot-storage-adapter.ts, and the offloader are byte-for-byte identical to my last review, and the branch is a clean fast-forward (no rebase/force-push).

So all prior feedback still stands. The one remaining substantive item is unchanged: the Storage.namespace?() method is optional on the interface, which (1) breaks the storage.namespace('session') DX the adapter docstring recommends (strictNullChecks → possibly undefined) and (2) contradicts the "Composable — calling namespace() on the result nests prefixes" docstring at storage.ts:111, since the factory now returns plain Storage. Details + compiler evidence in my inline on storage.ts:118.

Assessment: Comment — no re-review needed for the merge itself; the open API decision (make namespace required, or drop the method and revert the docstrings to the factory form) is the only thing left, and it's worth a needs-api-review label since it settles the public interface contract.

Constructs like SessionManager auto-namespace with a default prefix
(e.g. 'session') only when the user hasn't already scoped the storage.
The NAMESPACED symbol on views returned by namespace() enables this
detection without adding to the public Storage interface.
Comment thread strands-ts/src/session/session-manager.ts
Comment thread strands-ts/src/storage/storage.ts
@github-actions

Copy link
Copy Markdown
Contributor

Re-review of 6b16e972 ("add NAMESPACED symbol for construct auto-prefix detection")

Assessment: Comment

Nice approach — the NAMESPACED symbol resolves my earlier storage.namespace(...) DX concern cleanly: SessionManager now uses the standalone namespace() factory and detects prior scoping via the symbol, so nothing relies on the optional interface method. I compiler-verified that NAMESPACED in storage narrows correctly, and confirmed the symbol is kept out of the public ./storage export surface (only imported via the internal path) — good hygiene. Using Symbol.for also makes detection robust across duplicate package copies.

Three items remain (details inline):

Open items
  • Testing gap (Important) — the detection branch's actual behavior (skip session/ when already-namespaced, apply otherwise) isn't tested at the SessionManager level; the new tests only assert symbol presence/absence on the view. Add a behavior test asserting resulting key prefixes for both paths. (session-manager.ts:128)
  • Cross-construct consistency (Important/Question) — the commit generalizes "constructs auto-namespace," but ContextOffloader writes at the root with no default prefix or NAMESPACED awareness, so sharing one backend scopes only session data. Apply the same pattern or document the asymmetry. (plugin.ts)
  • Type/docs loose ends (minor) — the return type erases the NAMESPACED brand and the "Composable" docstring (storage.ts:119) is still type-broken for callers (chaining errors under strict). Soften the wording or expose a required namespace on the view type. (storage.ts:129)

The core mechanism is solid; this is mostly about test coverage for the new branch and a consistency decision across constructs.

@opieter-aws

Copy link
Copy Markdown
Contributor

Thanks for the latest change, this enables migration! I would suggest to clearly document this in the docstrings of @deprecated classes, or reference there to a migration guide.

Reviewer bot called out a few gaps, but I'm fine to do that as follow-up

@lizradway

Copy link
Copy Markdown
Member Author

will update tests and doc strings in python/docs port

@lizradway

Copy link
Copy Markdown
Member Author

@strandly-the-agent port this to a python implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api/review-complete An API Bar-raiser reviewed and accepted the APIs area-hooks Features or requests that might be implementable via hooks area-persistence Session management or checkpointing enhancement New feature or request size/xl typescript Pull requests that update typescript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants