Repository navigation
Conversation
GitHub Actions
Error
github-actions
Bot
added
area-tool
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Assessment: Comment Reconciliation of two previously-approved PRs (find_line + undo_edit, media/document view, configurable size caps, confinement root). Code quality is high — the implementation carries clear design-rationale comments and the test suite is thorough. I ran the full Review Categories
Nicely done — the inline design comments and edge-case test coverage make this easy to review. |
Documentation Preview ReadyYour documentation preview has been successfully deployed! Changed pages: Updated at: 2026-10-26T20:45:59.801Z |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Assessment: Comment Re-reviewed at New observations on this commit
Still unresolved from the previous review
|
|
Assessment: Request Changes Re-reviewed at One import-time issue should be fixed before merge. Findings
The |
|
Assessment: Request Changes Re-reviewed at Findings
Only the format-check failure blocks merge; the rest is unchanged from my prior review and the new |
github-actions
Bot
added
the
complexity/low
|
Assessment: Comment Re-reviewed at
No new issues introduced by the rebase. |
|
Assessment: Approve (one optional nit remaining) Re-reviewed at
Only remaining item is the earlier nit (non-blocking): Nice iteration on the lazy-loading fix — the deprecation is now correctly scoped to actual |
|
Assessment: Approve (suggestions, non-blocking) Re-reviewed at
Two non-blocking items: Suggestion (testing): There's no regression test locking in this deprecation contract (silent Minor (behavior tradeoff): Using the default Nit (still open): Solid handling of the sandbox suppression with explanatory comments. |
|
Assessment: Approve Re-reviewed at
That's exactly the regression protection needed given how many times this behavior flip-flopped. Gates green: Nit (still open, non-blocking): Nice addition of the deprecation regression tests — happy to approve. |
|
Assessment: Request Changes Re-reviewed at What changed
Net effect: any user who vends a sandbox's default tools now receives a Fix: restore the suppression in both Minor (unchanged from prior round): |
Introduce `strands.vended_tools.edit`, a focused edit tool that replaces
an exact, unique string in a text file. Signature matches the harness
`edit` tool byte-for-byte so a harness migration is a drop-in swap:
`edit(path, old_str, new_str, tool_context) -> "Edited {path}."`.
Sandbox-routed via the same construction-time bind / call-time resolve
pattern as the existing shell and file_editor tools; `make_edit` lets
custom sandbox implementations mount it with a bound sandbox.
Mark `file_editor` and `make_file_editor` deprecated. The warning fires
when a consumer calls `make_file_editor` directly. The two internal
callers — the module-level default `file_editor` instance and
`DockerSandbox` / `SshSandbox` `get_tools` (which continues to vend
`sandbox_file_editor` until the companion `read`/`write` tools land) —
suppress the warning via `warnings.catch_warnings`, so
`import strands.vended_tools` and `DockerSandbox(...).get_tools()` do
not emit the deprecation warning. (A pure lazy `__getattr__` lazy-load
in the parent package was explored but is unreliable here: `file_editor/`
is a subpackage name, so any `from strands.vended_tools.file_editor import X`
auto-binds `vended_tools.file_editor` to the subpackage and bypasses
`__getattr__`.)
Path validation accepts either a POSIX or a Windows absolute path (the
path belongs to the sandbox, not the host) and rejects `..` on either
separator, matching the harness `edit` tool.
Also add two `test_backslash_in_filename_is_literal` tests covering the
POSIX sandbox contract that `\\` in a filename is a literal character,
not a separator — orthogonal to any tool above the sandbox.
|
Assessment: Request Changes Re-reviewed at Status
Gates otherwise green: Fix: restore the suppression in both |
| occurrences = content.count(old_str) | ||
| if occurrences == 0: | ||
| raise ValueError(f"old_str did not appear verbatim in {path}.") | ||
| if occurrences > 1: |
There was a problem hiding this comment.
I think requiring the target string to appear exactly once is too stringent for ordinary file edits. Why don't we add replace_all as a tool parameter like what we did for the reconciled file_editor?
| await edit(path="\\\\server\\share\\file.txt", tool_context=ctx, old_str="a", new_str="b") | ||
|
|
||
|
|
||
| class TestFileEditorDeprecation: |
There was a problem hiding this comment.
I think this test class belongs in test_file_editor rather than test_edit.
| Returns: | ||
| A decorated tool that performs file operations through the sandbox. | ||
| """ | ||
| warnings.warn( |
There was a problem hiding this comment.
Docker and SSH sandbox users now get a deprecation warning they cannot act on, since they still instantiate file_editor tools. We should file an issue where the sandboxes use the new tools after they are all merged in. Until then, consider suppressing the sandbox warnings with with warnings.catch_warnings().
Also, this line in the PR description is inaccurate:
DockerSandbox.get_tools() / SshSandbox.get_tools() — now vend sandbox_edit + sandbox_shell instead of sandbox_file_editor + sandbox_shell
There was a problem hiding this comment.
Was going to try and merge all of the tools before the next release to avoid this issue, but I think you are right and its safer to just catch the warning.
| Returns: | ||
| A decorated tool that performs file operations through the sandbox. | ||
| """ | ||
| warnings.warn( |
There was a problem hiding this comment.
Other deprecations use logger.warning("tool_name=<...> | %s", ...) instead of a bare warnings.warn. Consider switching to that style for consistency.
There was a problem hiding this comment.
We have guidance where we use warnings.warn for deprecation like messages: #5317
| class TestSandboxBinding: | ||
| @pytest.mark.asyncio | ||
| async def test_bound_sandbox_overrides_agent_sandbox(self, tmp_path): | ||
| bound = NotASandboxLocalEnvironment() |
There was a problem hiding this comment.
This doesn't test the override because the test agent already defaults to a NotASandboxLocalEnvironment. Perhaps we can make the agent's sandbox raise in this test.
| new_str: str, | ||
| tool_context: ToolContext, | ||
| ) -> str: | ||
| """Replace an exact string in a file. ``old_str`` must appear exactly once. |
There was a problem hiding this comment.
This is missing the insert operation promised in the PR description. Is that intended?
| @@ -0,0 +1,101 @@ | |||
| """Sandbox-routed edit tool. | |||
|
|
|||
| Replaces an exact, unique string in a text file through a | |||
There was a problem hiding this comment.
I noticed that we're dropping the file size cap in this tool. Are we doing the same in the write tool? I ask because the model can get around a possible size check in the write tool by using the edit tool to replace a string with a bigger string.
There was a problem hiding this comment.
good call, ill make sure these are consistent
| if occurrences == 0: | ||
| raise ValueError(f"old_str did not appear verbatim in {path}.") | ||
| if occurrences > 1: | ||
| raise ValueError(f"old_str appears {occurrences} times in {path}; make it unique.") |
There was a problem hiding this comment.
There's no check for an empty old_str, so this produces a misleading error message: "appears N times; make it unique"
| A decorated tool that performs file operations through the sandbox. | ||
| """ | ||
| warnings.warn( | ||
| "file_editor is deprecated and will be removed in a future release. " |
There was a problem hiding this comment.
test_file_editor now emits many DeprecationWarnings. Should we mark the module with filterwarnings?
| raise ValueError(f"old_str did not appear verbatim in {path}.") | ||
| if occurrences > 1: | ||
| raise ValueError(f"old_str appears {occurrences} times in {path}; make it unique.") | ||
| await active.write_text(path, content.replace(old_str, new_str, 1)) |
There was a problem hiding this comment.
Noting that we have a race condition where 2 concurrent edits can drop the first. The reordering trick we used in the reconciled file_editor doesn't work here because we have 2 separate awaits. Fixing this would require a lock shared between file tools, since the write tool can presumably overwrite edits.
| @@ -0,0 +1,101 @@ | |||
| """Sandbox-routed edit tool. | |||
There was a problem hiding this comment.
I see that we're splitting edit, read, and write into their own modules. Is that for more intuitive imports and organization? Noting that this makes it harder to share file-related helpers and deviates from harness.
There was a problem hiding this comment.
While I was working on write and read, I noticed the same thing and moved path validation to a common utility function. Why do separate modules make it harder to share file-related helpers?
There was a problem hiding this comment.
Sharing file-related helpers across separate modules requires us to write a vended tool helper file, which is the first of its kind. It's not hard to implement, but it introduces a new pattern.
Human overview
Implement an
edittool, and deprecate thefile_editortool. This better aligns with the teams work done in the strands harness and vends the same tools it uses.Summary
Shift direction on vended file-editing tools. Instead of growing
file_editorinto a reconciled multi-command tool (view/create/str_replace/insert/find_line/undo_edit), introduce three focused tools —read,write, andedit— that match the harness's existingstrands_harness.toolsshape and letfile_editorbe retired. This PR lands the first of the three,edit.Tracks issue #3235 ("Vended tools: edit tool"). Companion issues #3236 (
read) and #3237 (write) land in follow-up PRs offagent-tasks/3236-3237-read-write-baseline.What's in this PR
strands.vended_tools.edit— bespoke edit tool: replaces an exact, uniqueold_strwithnew_str(errors on 0 or >1 occurrences). Signature matches the harnessedittool byte-for-byte so a harness migration is a drop-in swap.make_shellandmake_file_editor: pass asandbox=to bind it (as the Docker/SSH sandboxes do), otherwise it reads fromtool_context.agent.sandbox...rejected). Will delegate to the sandbox once Sandbox: own path validation and containment (root/jail) #5249 lands.file_editormarked deprecated — docstring note +warnings.warn(DeprecationWarning)inmake_file_editor. The module stays importable and functional; nothing else changes. Will be removed oncereadandwriteland.DockerSandbox.get_tools()/SshSandbox.get_tools()— unchanged in this PR: still vendsandbox_file_editor+sandbox_shell. The sandbox-internalmake_file_editor(...)call is wrapped inwarnings.catch_warnings()so constructing a sandbox does not emit the deprecation; a user-facingmake_file_editor()call still warns.test_backslash_in_filename_is_literaltests document a sandbox-level contract (POSIX\is a legal filename character). Kept because they're orthogonal to the tool above the sandbox.### Editsection insite/.../vended-tools.mdx;### File editormarked deprecated;sourceLinksaddsedit/edit.py.What's intentionally not in this PR
readandwritevended tools — tracked separately in Vended tools: read tool (py + ts) #3236 / Vended tools: write tool (py + ts) #3237.find_line,undo_edit, media view, root confinement, LRU undo state, oversize caps,replace_all, binary-document handling). The baseline for that work is preserved onUnshure/harness-sdk @ agent-tasks/3236-3237-read-write-baseline(at commit527af4bd5) for the read-tool port, but it does not ship through this PR.strands-ts/src/vended-tools/file-editorpivot (and the newedittool in TS) ships in a follow-up.file_editor. It stays importable with a deprecation warning untilreadandwriteare vended.Test plan
tests/strands/vended_tools/test_edit.py— 18 tests covering both commands, path validation, sandbox-binding behavior, and ambiguous/missing-match errors.tests/strands/vended_tools/test_file_editor.py::TestFileEditorDeprecation— regression tests covering:import strands.vended_toolsis quiet;make_file_editor()warns;DockerSandbox.get_tools()/SshSandbox.get_tools()are quiet.test_file_editor.pysuite still passes (deprecation-only change to the module).ruff check+ruff format --check+mypy ./src ./tests_typingall green.