Repository navigation
Conversation
Port the harness ``read`` tool into ``strands.vended_tools`` as a sandbox-routed ``read``/``make_read`` pair. Text files return ``cat -n`` numbered lines with ``offset``/``limit`` paging; images and binary documents return media content blocks when ``media=True``, text placeholders otherwise. Validates absolute paths and rejects ``..`` traversal. Refs strands-agents#3236.
github-actions
Bot
added
the
area-catalog
GitHub Actions
Error
github-actions
Bot
added
enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Vended tools don't need their own catalog entries.
github-actions
Bot
removed
the
area-catalog
GitHub Actions
Error
Share the absolute-path / traversal check between read (this PR) and the upcoming write / edit tools so the three cannot drift on validation. Picks up the _paths module from strands-agents#5549.
GitHub Actions
Error
| count = limit if limit is not None else _READ_DEFAULT_LIMIT | ||
| window = lines[start : start + count] | ||
| if not window: | ||
| return f"[File has {len(lines)} lines; offset {offset} is past the end.]" |
There was a problem hiding this comment.
Issue: offset/limit have surprising behavior on non-positive inputs.
offset=0silently acts likeoffset=1(because0is falsy in(offset - 1) if offset else 0), even though the arg is documented as 1-indexed.- A negative
offsetcollapses tostart=0viamax(0, ...), so e.g.offset=-5is silently equivalent tooffset=1. limit=0returns the message"[File has N lines; offset None is past the end.]"even whenoffsetisNoneandoffsetis not actually past the end — the diagnostic is misleading and includes a literalNone.
Suggestion: Validate up front — reject offset < 1 and limit < 1 with a clear ValueError (or document a specific semantic). This matches file_editor's view-range validation and keeps the model from quietly getting the wrong window.
There was a problem hiding this comment.
Addressed in 96fa385 — non-positive offset/limit are now rejected with explicit ValueErrors (offset must be a positive 1-indexed line number / limit must be a positive line count), matching the spirit of file_editor's view-range validation. Verified by the new parametrized tests. ✅
| return read_tool | ||
|
|
||
|
|
||
| read = make_read() |
There was a problem hiding this comment.
Issue: make_file_editor is auto-vended by the built-in Docker and SSH sandboxes via get_tools() (as sandbox_file_editor), but no analogous sandbox_read is registered. A user who swaps in a Docker/SSH sandbox and imports read from strands.vended_tools gets a read bound to the agent's sandbox at call time (so it works), but a user following the Docker/SSH pattern of relying on get_tools() only sees a file editor, not a read tool.
Suggestion: Either (a) wire make_read(sandbox=self, name="sandbox_read", description=...) into docker.py/ssh.py's get_tools() for parity, or (b) call out explicitly in the module docstring / docs that read is NOT auto-vended by sandboxes and must be registered by the user.
There was a problem hiding this comment.
Addressed in 96fa385 — make_read(sandbox=self, name="sandbox_read", description=...) is now wired into both docker.py and ssh.py get_tools() alongside sandbox_file_editor, so the Docker/SSH auto-vending path now exposes a read tool at parity. ✅
| ("name", "data", "expected"), | ||
| [ | ||
| ("a.gif", b"GIF89a fake", "gif"), | ||
| ("a.gif", b"GIF87a fake", "gif"), |
There was a problem hiding this comment.
Issue: Per-field assertions here (and in test_pdf_returns_document_content) can silently miss regressions in sibling fields of the returned ToolResult.
Suggestion: Assert the whole shape at once, e.g.:
assert result == {
"toolUseId": "test-id",
"status": "success",
"content": [{"image": {"format": "png", "source": {"bytes": b"\x89PNG\r\n\x1a\nfake"}}}],
}The strands-py AGENTS guide (docs/TESTING.md) prefers whole-object equality for exactly this reason.
There was a problem hiding this comment.
Addressed in 96fa385 — test_png_returns_image_content and test_pdf_returns_document_content now assert the full ToolResult shape with a single equality, so a regression in any sibling field (toolUseId, status, extra content blocks) is caught. ✅
|
|
||
| Raises: | ||
| ValueError: If the path is not absolute, or contains a ``..`` segment. | ||
| """ |
There was a problem hiding this comment.
Issue: The commit title (refactor: extract shared validate_absolute_path) suggests this is a shared helper, but file_editor.py still carries its own _validate_path with different semantics (POSIX-only, plus an abspath(...) suggestion in the error, and a different .. error message with no trailing period). The two validators have drifted — a traversal rejected by one is rejected with a different error by the other.
Suggestion: Either (a) collapse file_editor._validate_path onto this shared helper (adjusting the error message or adding an optional suggest_abspath flag if the hint is worth preserving), or (b) rename this helper to something more scoped (e.g. validate_sandbox_path) and document in a comment why file_editor keeps a separate variant. As-is, future readers will have to re-derive which to use.
Unshure
left a comment
Unshure
left a comment
There was a problem hiding this comment.
Nice port — the structure matches the existing make_file_editor factory cleanly and the tests exercise the real sandbox seam rather than mocking it, which I appreciate. The byte-sniffing-before-trusting-the-extension logic (with the comment about poisoning history) is a strong defensive choice.
A few things to look at before merging:
Themes
- Input validation on
offset/limitis too lenient — non-positive values produce silently-wrong windows or a diagnostic that includes a literalNone. - Parity gap with
file_editor: Docker/SSH sandboxes auto-vendsandbox_file_editorviaget_tools()but don't vend an analogoussandbox_read; worth either wiring up or documenting. - The extracted
validate_absolute_pathis only used by the new tool;file_editor._validate_pathstill exists with different semantics, which the commit title suggests was not the intent. - A couple of tool-result assertions in the tests would catch more regressions as whole-object equality checks.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ], | ||
| } | ||
|
|
||
| content = await active.read_text(path) |
There was a problem hiding this comment.
Issue: Unlike file_editor (which caps reads at 1MB via _assert_within_size_limit), read has no size bound. For text it reads the whole file into memory and numbers every line before slicing to the offset/limit window; for images/documents (lines 97, 113) it loads the full bytes and ships them to the model. A multi-hundred-MB file could exhaust memory or blow past provider payload limits.
Suggestion: Mirror file_editor's size guard. For text, consider reading only the requested window where the sandbox seam allows, or at least assert a max size. For media, cap the byte size before building the content block and return a text placeholder/error when exceeded.
| import re | ||
|
|
||
|
|
||
| def validate_absolute_path(path: str) -> None: |
There was a problem hiding this comment.
Issue: This module's docstring says validation is "shared across sandbox-routed file tools," and the head commit is titled extract shared validate_absolute_path — but file_editor._validate_path still carries its own duplicate copy of the absolute/.. checks and was not migrated to this helper. The result is two validators to keep in sync, which is the opposite of what the extraction intended.
Suggestion: Migrate file_editor to call validate_absolute_path (reconciling the message/behavior differences noted below), or if the duplication is deliberate for now, say so in the PR description and drop the "shared" framing.
| "toolUseId": tool_context.tool_use["toolUseId"], | ||
| "status": "success", | ||
| "content": [ | ||
| {"document": {"format": extension, "name": _document_name(path), "source": {"bytes": data}}} |
There was a problem hiding this comment.
Issue: Images are signature-sniffed (with a comment noting that a mislabeled success image block "poisons history"), but documents are trusted purely by extension — a text or arbitrary file named report.pdf is sent as a success document block with no content check. If a malformed document block poisons history the same way a bad image does, this path has the exact risk the image path guards against.
Suggestion: Either sniff document magic bytes (e.g. %PDF, OLE/ZIP headers) before emitting the block, or document why documents don't need the same guard that images do.
|
|
||
| lines = content.split("\n") | ||
| start = max(0, (offset - 1) if offset else 0) | ||
| count = limit if limit is not None else _READ_DEFAULT_LIMIT |
There was a problem hiding this comment.
Issue: count is used directly in the slice without guarding non-positive values. limit=0 yields an empty window and falls into the "past the end" branch, producing a misleading message like offset None is past the end. even when offset was never supplied. A negative limit slices unexpectedly (lines[start:start+count] with count<0).
Suggestion: Guard limit/offset to be positive (reject or clamp) before slicing, and keep the "past the end" message reserved for a genuine offset-beyond-EOF case.
There was a problem hiding this comment.
Resolved in 96fa385 — offset < 1 and limit < 1 now raise a clear ValueError up front, so the empty-window branch is only reached on a genuine offset-past-EOF (no more literal None in the message). New parametrized tests (test_non_positive_offset_rejected, test_non_positive_limit_rejected) cover 0/-1/-100. ✅
|
|
||
| Raises: | ||
| ValueError: If the path is not absolute, or contains a ``..`` segment. | ||
| """ |
There was a problem hiding this comment.
Issue: The shared validator accepts Windows-absolute paths (ntpath.isabs, drive letters, UNC shares), but file_editor's own check is POSIX-only with the comment "Absolute means POSIX-absolute (leading /), matching the sandbox path model." So read will accept C:\foo as valid, then hand it to a sandbox whose path model is POSIX — a validation pass that fails confusingly downstream. The read tests even declare "POSIX path semantics assumed," yet test_paths.py asserts Windows paths are accepted.
Suggestion: Decide which path model is canonical for sandbox-routed tools and make both tools agree. If the sandbox is POSIX, keep the validator POSIX-only; if Windows paths are genuinely supported, update file_editor and document it.
|
Assessment: Comment Clean, well-documented port that closely follows the established Review Categories
Nice work on the |
- Reject non-positive offset/limit up front so offset=0/-N and limit=0/-N no longer silently collapse or emit a misleading "past the end" message. - Register sandbox_read in Docker and SSH sandboxes' get_tools() alongside sandbox_file_editor, matching the auto-vending pattern. - Tighten image/document tests to whole-object equality.
github-actions
Bot
added
strands-running
complexity/high
Documentation Preview ReadyYour documentation preview has been successfully deployed! Changed pages: Updated at: 2026-10-27T13:52:43.343Z |
|
Re-review of 96fa385 — thanks for the quick turnaround. Verified fixed (ran
Still open from the prior round (not touched in this commit):
None are blockers I'd insist on, but the size cap and the validator duplication/divergence are worth resolving before merge for consistency with |
Human Overview
Description
Adds a sandbox-routed
readvended tool to the Python SDK, ported from the harness'sstrands_harness.tools.file_tools.read. The tool returnscat -n-style numbered lines for text (withoffset/limitpaging so a large file can be read in pages), returns image and binary-document content blocks when the model supports media, and falls back to a text placeholder whenmedia=False. Routing goes throughtool_context.agent.sandbox, with optional pre-binding viamake_read(sandbox=...)to match thefile_editorfactory pattern.Non-absolute paths and
..segments are rejected up front; sandbox-level containment can take over once #5249 lands.Related Issues
Part of #3236. TypeScript port is deferred to a follow-up.
Documentation PR
Included in this PR under
site/: a new### Readsection invended-tools.mdxand aread.yamlcatalog entry.Type of Change
New feature
Testing
strands-py/tests/strands/vended_tools/test_read.pycover text paging, image sniffing and rejection, document rendering, media-off placeholders, path validation, and the agent-sandbox fallback. Exercised against the realNotASandboxLocalEnvironmenton the host filesystem.hatch check codepasses on the new files.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.