Skip to content

feat(context-offloader): auto-namespace unified Storage under offloader - #3258

Merged
lizradway merged 5 commits into
strands-agents:mainfrom
lizradway:lizradway/context-offloader-auto-namespacing
Jul 14, 2026
Merged

lizradway merged 5 commits into
strands-agents:mainfrom
lizradway:lizradway/context-offloader-auto-namespacing

Conversation

@lizradway

Copy link
Copy Markdown
Member

Description

When a unified Storage is passed to ContextOffloader, keys are now auto-scoped under 'offloader/' to prevent collisions with other constructs sharing the same Storage (e.g. session manager uses 'session/'). Pre-namespaced storage is respected as-is.

LocalFileStorage passed directly is also auto-routed through the agent's sandbox via forSandbox, matching the legacy FileStorage behavior.

Related Issues

Documentation PR

Type of Change

Bug fix
New feature
Breaking change
Documentation update
Other (please describe):

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.

  • I ran hatch run prepare

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

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

When a unified Storage is passed to ContextOffloader, keys are now
auto-scoped under 'offloader/' to prevent collisions with other
constructs sharing the same Storage (e.g. session manager uses 'session/').
Pre-namespaced storage is respected as-is.

LocalFileStorage passed directly is also auto-routed through the agent's
sandbox via forSandbox, matching the legacy FileStorage behavior.
@lizradway
lizradway requested a review from a team as a code owner July 14, 2026 18:52
@lizradway
lizradway requested a review from notowen333 July 14, 2026 18:52
@github-actions github-actions Bot added area-context Session or context related area-persistence Session management or checkpointing enhancement New feature or request strands-running size/m labels Jul 14, 2026
Comment thread strands-ts/src/vended-plugins/context-offloader/__tests__/plugin.test.ts Outdated
Comment thread strands-ts/src/vended-plugins/context-offloader/plugin.ts
Comment thread strands-ts/src/storage/local-file-storage.ts
Comment thread strands-ts/src/vended-plugins/context-offloader/plugin.ts
@github-actions

Copy link
Copy Markdown
Contributor

Issue (cross-SDK parity): This introduces the offloader/ auto-namespace convention only in the TypeScript SDK. The Python ContextOffloader (strands-py/.../context_offloader/plugin.py) still assigns self._storage = storage with no auto-scoping. AGENTS.md asks that conventions/constants shared by both SDKs stay in sync.

Suggestion: Either mirror this behavior (and the offloader/ prefix string) in the Python SDK, or record why the two intentionally differ (e.g. Python's offloader doesn't yet accept a unified Storage) in a team/DECISIONS.md entry so the gap is tracked. Also worth a note in the PR description — it currently reads as Python-oriented but the diff is entirely TypeScript.

@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment (a few things to confirm before merge)

Auto-scoping unified storage under offloader/ and reusing the sandbox-routing path for LocalFileStorage is a sensible, well-tested direction. My main questions are around the eviction/sandbox delete path and cross-SDK parity; the rest are refinements.

Review themes
  • Correctness: Eviction deletes go through the non-sandbox storage view while writes route through the sandbox — worth confirming file-backed artifacts still get evicted.
  • Design/robustness: Sandbox routing keys off the structural presence of a forSandbox method; a more explicit marker would avoid surprising custom Storage implementations. Nested .namespace() on LocalFileStorage silently drops sandbox routing.
  • Cross-SDK parity: The offloader/ convention lands only in TS — mirror it in Python or record the intentional divergence.
  • Testing: New tests assert length + prefix regex where the key is deterministic; a full-array equality would guard against unexpected keys (consistent with the existing test above them).

Nice touch keeping pre-namespaced storage untouched via the NAMESPACED symbol and covering both the raw and pre-namespaced paths in tests.

- Use exact toEqual assertions in auto-namespacing tests
- Route eviction deletes through per-agent sandbox storage
- Document forSandbox contract for sandbox routing
- Document single-level namespace limitation on LocalFileStorage
@github-actions

Copy link
Copy Markdown
Contributor

Re-review (updated commits)

Thanks for the quick turnaround — the new commits address the substantive feedback from the last round:

  • Eviction correctness: _evict now routes deletes through _storageForAgent(agent), matching the write path for sandboxed storage.
  • Test assertions: auto-namespacing tests now use full-array toEqual equality.
  • Nested-namespace contract: documented on LocalFileStorage.namespace().
  • Sandbox-routing contract: documented on the storage config option (structural forSandbox check retained by design).

One item from the last review is still open and non-blocking:

  • Cross-SDK parity: the offloader/ auto-namespace convention still only exists in the TS SDK. Please either mirror it in the Python ContextOffloader or record the intentional divergence in team/DECISIONS.md so the gap is tracked. A short note in the PR description (which currently reads Python-oriented while the diff is all TS) would also help.

Assessment: Approve once the cross-SDK parity item is either resolved or explicitly tracked. The code changes themselves look good to merge.

lizradway added a commit to lizradway/harness-sdk that referenced this pull request Jul 14, 2026
… 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.
opieter-aws
opieter-aws previously approved these changes Jul 14, 2026
Replace /\/+/g and /\/+$/ regex patterns in normalizeKey,
normalizePrefix, LocalFileStorage, and S3Storage with split/filter/join
string operations to avoid polynomial backtracking on uncontrolled input.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-context Session or context related area-persistence Session management or checkpointing enhancement New feature or request size/m

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants