Skip to content

fix(storage): strip trailing slash from S3Storage prefix in Python - #5551

Open
lizradway wants to merge 1 commit into
strands-agents:mainfrom
lizradway:lizradway/py-s3-prefix-slash
Open

lizradway wants to merge 1 commit into
strands-agents:mainfrom
lizradway:lizradway/py-s3-prefix-slash

Conversation

@lizradway

Copy link
Copy Markdown
Member

Human Overview

Description

The Python S3Storage constructor appended / to the normalized prefix without first stripping a trailing one, so a prefix ending in / produced a double slash in every object key:

S3Storage("my-bucket", prefix="agents/").prefix  # before: "agents//"  after: "agents/"

write("key", ...) therefore stored agents//key instead of agents/key. The constructor now strips the trailing slash first, the same way namespace() already does. This makes it match the TypeScript S3Storage, which normalizes prefix: 'agents/' (and '/agents//a/') to agents/ (agents/a/).

Behavior change for existing data. S3Storage has shipped since python/v1.48.0, and its own docstring example (and the context offloader's) uses a trailing-slash prefix. Objects already written through a prefix that ends in / live under the double-slash key (agents//...). After this fix, a storage built with the same prefix reads and lists agents/..., so it no longer finds those objects. Nothing is deleted. To keep reading old data, move the objects to the single-slash keys, or construct the storage with the double-slash prefix explicitly until they are migrated. Prefixes without a trailing slash (prefix="agents") are unaffected.

Related Issues

None. Found while porting the S3 Vectors search strategy (#3967) to Python.

Documentation PR

None needed.

Type of Change

Bug fix

Testing

  • New test: prefix="agents/data/" and prefix="/agents//data//" both store under agents/data/key and list back as key. It fails on main and passes with the fix.

  • hatch test on storage, session, memory stores and context offloader: 807 passed. hatch fmt --formatter and hatch fmt --linter (ruff + mypy) clean.

  • 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.

S3Storage('b', prefix='agents/') stored objects under 'agents//key'
because the constructor appended '/' to a prefix that already ended in
one. Strip it first, as namespace() already does, so the prefix
normalizes to 'agents/' and matches the TypeScript SDK.
@lizradway
lizradway requested a review from a team as a code owner October 26, 2026 20:58
@lizradway
lizradway requested a review from mkmeral October 26, 2026 20:58
@github-actions github-actions Bot added area-persistence Session management or checkpointing bug Something isn't working python Pull requests that update python code strands-running complexity/low Touched functions have low cognitive complexity (<=10) size/xs labels Oct 26, 2026
@codecov

codecov Bot commented Oct 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Approve

Correct, minimal fix that brings the Python S3Storage prefix normalization into parity with TypeScript and matches the existing .rstrip("/") pattern already used in _NamespacedStorage.__init__ and S3Storage.namespace(). I verified the regression test fails against the pre-fix behavior (keys land under agents/data//key) and passes with the fix; the new parametrized test is a good round-trip + list() shape assertion.

Review notes
  • Correctness: _normalize_prefix(prefix).rstrip("/") yields agents/data/ for both agents/data/ and /agents//data//, and "" for ""/"/". Matches TS split('/').filter(Boolean).join('/')+'/'. ✅
  • Consistency: Now identical to _NamespacedStorage (storage.py:211) and namespace() (s3_storage.py:232). ✅
  • Tests: Parametrized, asserts observable behavior (object key + list()), not internal _prefix. ✅
  • Behavior change (the one thing to be aware of): As the description thoroughly documents, callers who shipped a trailing-slash prefix (including the docstring's own prefix="agents/" example) previously wrote under agents//... and will no longer find that data after this fix. This is the intended correction, but since it's a silent data-access change on a since-v1.48.0 API, please make sure it's called out in the release notes so existing users can migrate.

Nice catch while porting #3967 — good that the fix also folds in the leading/duplicate-slash cases.

This branch has not been deployed

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

Labels

area-persistence Session management or checkpointing bug Something isn't working complexity/low Touched functions have low cognitive complexity (<=10) python Pull requests that update python code size/xs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant