[chcodex]

Summary

Rewrite BaseSandboxFilesystem.edit() to perform the string replacement natively inside the sandbox using a static inline Python script, instead of downloading the whole file to Java, replacing, and uploading it back.

old/new cross the boundary base64-encoded on the command line — the base64 alphabet has no shell metacharacters, so user content cannot break out of its argument, and the file itself never crosses the boundary. The script is a fixed heredoc (real newlines, quoted delimiter) whose only output is one __RESULT__{json} line, parsed with Jackson.

When python3 is unavailable — or the command would exceed the size bound, or execute throws, or the native write fails (write_failed, target untouched) — edit() degrades to the existing download → FilesystemUtils.stringReplacement() → upload path.

Why the design changed on the way to review

Worth recording, because the head differs from the first reviewed version by more than a refactor. The first revision staged old/new as files under /tmp/agentscope-edit-<uuid>/ and uploaded them via uploadFiles(). That looked reasonable until the transfer path was traced:

• uploadFiles refuses any path outside the sandbox workspace — resolveArchivePath throws for the tar-hydrate fallback, DockerSandbox.requireTransferPath requires the path under workspaceRoot, and on Kubernetes the runtime itself confines /upload//download to WORKSPACE_DIR (agentscope-runtime, sandbox/box/shared/routers/workspace.py, ensure_within_workspace, HTTP 403 outside).
• So the staging upload could never succeed: edit() fell back to the transfer path on every call, and the native path this PR exists to provide never ran in production. Opening /tmp on the backends would not have helped — the boundary is enforced server-side.

Rather than widen that boundary (per-backend changes, an exec fallback with its own size cap, and workspace staging that would leak user content into workspace snapshots), the params now travel base64-encoded in the command. That removes the staging round trip entirely and makes the native path portable across backends. Two consequences are deliberate:

• User content does enter the command, as base64. The base64 alphabet contains no shell metacharacters, so the injection surface is still zero and no escaping layer is needed, but the payloads are decodable from logs. This weakens — does not break — the "raw content never reaches the command line" property.

The deprecated performStringReplacement() bridge deliberately retains the published v2.0.x exact-line-ending behavior; only the new typed stringReplacement() normalizes CRLF/lone-CR to LF. This avoids silently changing semantics for downstream users of the old API while allowing the filesystem implementations to converge on the new behavior.

• Oversized arguments fail rather than degrade. See the size bound below.

Bugs Fixed (#2397)

1. \n not recognized by python3 -c — the old inline script embedded Java \\n literally, causing SyntaxError. The new script is a text block with real newlines and a quoted heredoc delimiter.
2. jsonEscape() incomplete — the hand-built JSON missed control characters, so a detail string could break json.loads(). The result payload is now produced by json.dumps inside the script and parsed with Jackson; user content never takes part in building it.
3. Hard python3 dependency — removed as a hard requirement: missing python3 falls back to the transfer path.

Why native edit

• No staging files: old/new ride in the command as base64, so there is no /tmp directory to create, clean up, or race on, and no dependency on uploadFiles accepting paths outside the sandbox workspace (the Docker/Kubernetes transfer APIs refuse those, which is what kept the native path unreachable).
• The command string is bounded: the whole command is one argv element, so the kernel's 128 KiB MAX_ARG_STRLEN applies to it in total. Oversized arguments are rejected with a "split the edit" diagnostic rather than silently transferred — old/new are edit fragments, so a request that large should be split.
• All three filesystems share one replacement path (FilesystemUtils.stringReplacement): same occurrence counting, same error messages, and the same CRLF/lone-CR → LF normalization — applied identically in the native script and in the fallback, so an edit can neither match on one backend and not the other, nor succeed on one path and fail on the other within BaseSandboxFilesystem. Line endings are written back as LF, matching LocalFilesystem's long-standing behaviour.
• Atomic write-back: realpath + same-directory temp + os.replace preserves mode, follows symlinks, never crosses a mount. There is no non-atomic write: if the temp file cannot be created or the rename fails, the target is untouched and write_failed retries through the transfer path (whose shell redirection needs only file write permission).
• python exit status survives the trailing cleanup via $?/exit $__ec, so python3-missing detection works even on backends that do not relay stderr.

Documentation and compatibility follow-up

• The sandbox image contract in both docs/v2 locales now marks python3 as optional for edit_file: it enables the native fast path; without an executable interpreter the transfer fallback is used. The docs also describe the 112 KiB total-command limit and split-edit guidance.
• The filesystem docs in both locales now document that oldString must be non-empty, newString cannot be null ("" means deletion), and CRLF/lone-CR content is normalized to LF.
• The deprecated performStringReplacement() bridge preserves the published v2.0.x exact-line-ending behavior; only the new typed stringReplacement() normalizes line endings. This keeps the bridge's behavior truthful, not just its JVM signature.
• A hard kill between temp creation and atomic rename may leave a UUID-named sibling temp file visible to listings/snapshots. This is documented in the edit Javadoc and script comment. No age-based sweep is used because it could race with a slow concurrent edit.

Changes

File	Change
BaseSandboxFilesystem.java	Rewrite edit() as native inline script + editViaTransfer() fallback; remove jsonEscape(); add Logger, $? exit passthrough, Jackson-based result parsing; document rare temp-file residue after a hard kill
FilesystemUtils.java	Add ReplacementResult record + stringReplacement() (CRLF/lone-CR → LF); keep @Deprecated performStringReplacement (Object[]) as a compatibility bridge preserving the published exact-line-ending behavior; add shared argument validation and strict UTF-8 decoding
LocalFilesystem.java	Adapt to ReplacementResult; reject null newString / empty oldString; drop its now-shared line-ending normalization
RemoteFilesystem.java	Adapt to ReplacementResult; reject null newString / empty oldString
BaseSandboxFilesystemTest.java	Spy-based unit tests for the native path and every fallback branch, plus real sh/python3 integration tests; real downloadFiles/uploadFiles in the local-shell helper

Tests

164 tests passing, 0 failures in the filesystem suites; 1363 tests passing, 0 failures, 10 skipped in the whole agentscope-harness module (includes upstream regression tests):

• BaseSandboxFilesystemTest: canned unit (native success/multiline params present only base64-encoded/exact error mapping/write_failed fallback/python3-missing via 126 and 127 and via stderr-only/oversized-argument rejection/invalid UTF-8 rejection/newString == null) + FilesystemUtilsTest (typed replacement, CRLF/LF/CR normalization and LF write-back, deprecated-bridge contract including exact CRLF behavior, argument validation, strict UTF-8 decoder) + real-shell CRLF parity between the native and fallback paths + Linux integration via real sh + python3 (simple, special chars, replace-all, not-found, string-not-found, multiple-occurrences, symlink preserved (native), CRLF parity native-vs-fallback, heredoc quoting edges — delimiter-shaped path, single-quoted path (with a decoy sibling), backticked path, params ending in a newline — fallback symlink write-through vs replace, native/fallback replaceAll parity, malformed result JSON)
• FilesystemToolTest, LocalFilesystemModeTest, CompositeFilesystemTest, SandboxBackedFilesystemTest

Fixes #2397

---

Note on the branch name

The head branch is still fix/sandbox-edit-python-to-java, which was chosen when the first revision removed the inline Python script in favour of a pure download-edit-upload flow. The current revision reverses that direction: it runs a static inline Python script inside the sandbox (with the download-edit-upload path kept only as a fallback), so the branch name no longer describes the change.

Renaming a PR's head branch is not possible on GitHub — renaming the source branch does not redirect a cross-fork PR and instead closes it, losing the review history. The name is therefore left as-is; the PR title and this description reflect the actual approach.

[codecov]

Codecov Report

❌ Patch coverage is 80.79470% with 29 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...gent/filesystem/sandbox/BaseSandboxFilesystem.java	76.19%	12 Missing and 13 partials ⚠️
...arness/agent/filesystem/local/LocalFilesystem.java	80.00%	1 Missing and 1 partial ⚠️
...ness/agent/filesystem/remote/RemoteFilesystem.java	77.77%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

✅ Approved — Major improvement over the inline Python approach.

The old python3 -c approach was fragile (JSON escaping gaps, \n literal issues, hard Python dependency). The new download → FilesystemUtils.performStringReplacement() → upload path:

• Eliminates ~90 lines of string building and parsing
• Reuses the same replacement logic as LocalFilesystem and RemoteFilesystem
• Removes the python3 runtime dependency
• The ReplacementResult record is a nice improvement over the Object[] return type

Good test coverage with both unit tests (spy-based) and integration tests (real download/upload).

[AgentScopeJavaBot]

🤖 AI Review

Refactoring of the filesystem edit() pipeline: replaced untyped Object[] return with type-safe ReplacementResult record, and rewrote BaseSandboxFilesystem.edit() from remote Python-script to download-replace-upload pattern. Note: title says A2A server but diff contains only filesystem refactoring.

(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)

[AgentScopeJavaBot]

🤖 AI Review

Refactoring of the filesystem edit() pipeline: replaced untyped Object[] return with type-safe ReplacementResult record, and rewrote BaseSandboxFilesystem.edit() from remote Python-script to download-replace-upload pattern. Note: title says A2A server but diff contains only filesystem refactoring.

(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)

[AgentScopeJavaBot]

🤖 AI Review

Refactoring of the filesystem edit() pipeline: replaced untyped Object[] return with type-safe ReplacementResult record, and rewrote BaseSandboxFilesystem.edit() from remote Python-script to download-replace-upload pattern. Note: title says A2A server but diff contains only filesystem refactoring.

(inline comments could not be attached — line numbers fell outside PR hunks. See archived report.)

[oss-maintainer]

⚠️ Merge conflict detected

This PR currently conflicts with main and cannot be merged. Please rebase (or merge main in) and resolve the conflicts:

git fetch upstream
git checkout fix/sandbox-edit-python-to-java
git rebase upstream/main
# resolve conflicts, then:
git push --force-with-lease

This is a one-time reminder. Once the conflicts are resolved, comment @oss-maintainer please re-review and I will pick it up again.

---

Automated notification by github-manager-bot

[oss-maintainer]

Summary

Re-review at 325fdca0. The branch was force-pushed onto current main, so the previous APPROVED (recorded at d13ac57d) no longer applies to any commit that exists on this PR — this is effectively a full review of a rebase plus a design change: the approved plan was download → FilesystemUtils → re-upload, and the head now runs a static Python script inside the sandbox with the Java transfer path demoted to a fallback.

The core design is right, and better than what was approved in July:

• User content never enters the command string. old/new cross the boundary as files and sys.argv carries paths only, which structurally removes the escaping/python3 -c injection surface the old implementation relied on. shellQuote is applied to every path argument, and the edit_native_multilineParams_notInCommandString test asserts the leakage property directly rather than just the outcome.
• parseErrorToken reads the error field exactly instead of substring-matching the payload, and edit_native_writeFailedDetailWithErrorLikePath_notMisclassified proves the case that motivated it (a path containing string_not_found). mapNativeResult covers every script error token plus the unknown-error default.
• Exit-code passthrough (__ec=$?; ...; exit $__ec) is what makes the 127 detection trustworthy, and isPythonMissing additionally handles backends that report only on stderr with exit 0 — both are tested.
• ReplacementResult replaces the Object[] return with a typed record across LocalFilesystem/RemoteFilesystem/sandbox, and newString == null is now a documented deletion rather than an "null"-literal or NPE.
• 269 new test lines, including six real LocalShellSandboxFilesystem end-to-end cases (special characters, replaceAll, not-found, multiple occurrences). CI green (6/6), license/cla signed, and the merge conflict flagged on 2026-09-17 is resolved cleanly (MERGEABLE/CLEAN).

Findings (4, one warning):

• [Warning] tmp_out = real + ".agentscope-edit-tmp-" + str(os.getpid()) is not unique per edit when a sandbox backend reuses a long-lived shell process, so two concurrent edits of the same target can share the temp file and one silently loses its update while still reporting count success — the param dir right below already uses a UUID for exactly this reason (BaseSandboxFilesystem.java:287).
• [Info] The transfer fallback decodes with new String(bytes, UTF_8), which replaces malformed bytes instead of failing like the Python path does, so on a python3-less sandbox a legacy-encoding text file gets edited successfully and silently re-encoded (BaseSandboxFilesystem.java:535).
• [Info] The non-writable-directory fallback reopens the target with "wb", so a mid-write I/O error truncates the file instead of leaving it unchanged (BaseSandboxFilesystem.java:307).
• [Info] os.path.realpath is the right call for workspace symlinks and mount locality; a test asserting the symlink is still a symlink after an edit would lock that behavior in (BaseSandboxFilesystem.java:285).

Commenting rather than re-approving because the head changed design after the previous approval; the items above are small and the temp-file uniqueness one is worth fixing before merge.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] PID is not a unique temp-file suffix inside a sandbox.

tmp_out = real + ".agentscope-edit-tmp-" + str(os.getpid()) is only unique per process. Two concurrent edit(...) calls on the same target file that are routed into the same sandbox container (same-key concurrent calls are a real scenario here — see PR #2846, which had to serialize them for the persisted-state path) share the PID namespace value only if they are the same process, but they do share the target path, so:

• both write to <real>.agentscope-edit-tmp-<pid> with different PIDs → fine;
• but with a forked/exec'd shell pool or a reused sh process (some sandbox backends run every command in one long-lived shell), both edits land on the same tmp_out: A writes content-A, B overwrites with content-B, A's os.replace moves the shared path (now content-B) onto the target, B's os.replace then fails with FileNotFoundError and falls into the in-place fallback — so A's edit is silently lost and reported as success ({"count": ...}).

The param dir right below already uses a UUID for exactly this reason; the same trick costs nothing here (e.g. uuid.uuid4().hex[:8], or tempfile.mkstemp(dir=os.path.dirname(real))). A count that is reported from a lost update is worse than a visible failure, because the agent believes the file is edited.

[oss-maintainer]

[Info] The transfer fallback silently re-encodes non-UTF-8 text files, unlike the native path.

The Python script opens in binary and decode("utf-8")s inside a try, so a Latin-1 / GBK text file returns {"error": "read_failed"} and the edit fails cleanly. The fallback does new String(contentBytes, StandardCharsets.UTF_8), which replaces malformed byte sequences with U+FFFD, then re-uploads — so on a sandbox without python3, editing a legacy-encoding file succeeds while corrupting every non-ASCII byte outside the replaced region.

Extending FilesystemUtils to decode with CharsetDecoder.onMalformedInput(CodingErrorAction.REPORT) (surfacing read_failed-equivalent error) would keep the two paths honest to each other. This is pre-existing behavior on the Java side, but this PR is what makes the two paths observable alternatives for the same call, so it is the right time to align them.

[oss-maintainer]

[Info] In-place fallback can truncate the file on failure.

When the directory is not writable, the script re-opens the target with "wb" and writes — so an I/O error halfway (disk full, quota) leaves the file truncated rather than unchanged, and the {"count": ...} success is never reported. The os.replace path is atomic precisely to avoid this. Consider f.write(out_bytes); f.flush(); os.fsync(f.fileno()) at minimum, or a one-line note that the non-atomic fallback is best-effort. Not blocking — the primary path is atomic and the fallback matches the previous behavior of this method.

[oss-maintainer]

[Info] Nice catch on realpath, one consequence worth documenting.

Resolving os.path.realpath(target) before writing is the right call — it edits through workspace symlinks instead of replacing them with a regular file, and keeps the temp file on the same mount so os.replace cannot cross devices. One side effect to pin in the javadoc: because os.stat(real)/os.chmod(tmp_out, st.st_mode) act on the resolved target, an edit that arrives via a symlink now mutates the referenced file's content while the symlink itself is preserved. That is the intended behavior; a test asserting "symlink still a symlink after edit" would lock it in (the LocalShellSandboxFilesystem tests cover plain files only).

[chcodex]

Addressed the four re-review findings at 352ec152.

[Warning] Temp-file uniqueness — tmp_out now uses uuid.uuid4().hex instead of os.getpid(), so concurrent edits sharing a long-lived shell process can no longer collide on the same temp file.

[Info] Non-atomic in-place fallback (chosen fix) — removed the open(real, "wb") branch entirely. Because "wb" carries O_TRUNC, it emptied the target before writing, so any mid-write failure (ENOSPC, EIO, OOM-kill) destroyed data — and this branch could only trigger when no temp copy existed. A write failure now always happens before the target is touched, so failure ⇒ file unchanged holds unconditionally. edit() retries a write_failed result through editViaTransfer, whose shell redirection needs only file write permission and succeeds where a read-only directory blocked the temp file. Verified end-to-end: in a chmod 555 directory the native path returns write_failed with the target still intact, then the transfer path writes it successfully.

[Info] Silent re-encode in fallback — the fallback now decodes with a strict UTF-8 CharsetDecoder (CodingErrorAction.REPORT) and fails with file is not valid UTF-8 instead of byte-substituting, matching the Python path's strict decode.

[Info] Symlink test — added edit_symlinkTarget_preservesLinkAndWritesThrough, asserting the link is still a symlink after the edit and the content landed in the real target.

109 tests passing. This also removes the last non-atomic write in the native path.

[chcodex]

@oss-maintainer please re-review

Follow-up at 648fe7bf, addressing the latest review points:

1. Removed the shutil.copyfile fallback — it was the same truncate-on-failure risk as the in-place "wb" write removed in 352ec152 (copyfile opens the destination with O_TRUNC). The temp file lives in the target's own directory, so os.replace has no cross-device failure mode; when it still fails (immutable/NFS), the script now returns write_failed with the target untouched and Java retries through the transfer path. The native script now writes the target only via temp + atomic rename — no non-atomic write remains.
2. Documented the edit() javadoc contract — editing a symlink writes through to the resolved target and preserves the link itself; write_failed implies the file is unchanged and is retried via transfer.
3. Synced the PR description — test count updated to 109, and the write-back bullet now reflects the no-non-atomic-write behavior.

109 tests passing; rebased on current main (6a056809).

[oss-maintainer]

Summary

The rework is a clear improvement: user content no longer travels inside the command line (paths only, shellQuoted), the Python helper is a static heredoc so it cannot be broken by `` sequences in the payload, the atomic same-directory temp + os.replace write with the read-only-directory fallback is the right shape, result parsing is now anchored on an explicit `RESULT` marker with exact-field error extraction, and the fallback decodes UTF-8 strictly instead of silently re-encoding a legacy-encoding file. Coverage on the fallback matrix is thorough. Holding at COMMENT for the symlink-semantics divergence between the two paths and one public-API compatibility question.

Findings

• [Warning] BaseSandboxFilesystem.java:538 - the transfer fallback re-uploads via uploadFiles(filePath, ...), which does not carry the native path's "writes through the symlink, never replaces it" guarantee; the two paths end up with different invariants for the same operation.
• [Warning] FilesystemUtils.java:72 - performStringReplacement return type change is a source/binary-incompatible change for downstream callers of a published public utility; needs either a deprecated bridge or a migration-note entry.
• [Info] BaseSandboxFilesystem.java:337 - cross-reference with #3269, which removes a quoted-space write command on these same backends; make sure the two are converging on one command-construction story.

Suggestions

• If the symlink asymmetry is intentional, one line in the editViaTransfer javadoc plus a fake that distinguishes write-through from replace would make it explicit.
• Consider asserting EditResult.occurrences() parity between native and fallback paths for the replaceAll case in one parameterised test, so the two implementations cannot drift.

Notes

• CI is green (5/5 checks) on the current head and license/cla is signed; the two new commits are both from the same push, so this review covers the combined delta since my previous COMMENTED review.

---

Automated review by github-manager-bot

[oss-maintainer]

[Warning] The native path deliberately resolves realpath(target) and uses os.replace so a workspace symlink keeps pointing where it did (there is a test for it, edit_symlinkTarget_preservesLinkAndWritesThrough). This fallback does not have that property: it re-uploads through uploadFiles(filePath, ...), so on backends where upload creates/replaces the file rather than writing through the link, a sandbox that edited fine before python3 went missing can come back as a regular file - the same class of breakage the real dance above exists to avoid, just on the rarer path.

Two options: (1) note the asymmetry in the method javadoc and mirror it with a test that asserts the fallback's symlink behaviour on a fake that distinguishes the two, or (2) make the fallback preserve the link by downloading/uploading against the realpath instead of filePath. Worth deciding explicitly rather than leaving the two paths with different invariants.

[oss-maintainer]

[Warning] Replacing Object[] with a record is clearly the right API (the result.length == 1 discriminant was a foot-gun), but FilesystemUtils is a public utility of a published module, so this is a source- and binary-incompatible change for any extension or downstream subclass calling performStringReplacement(...) - callers get a NoSuchMethodError at runtime rather than a compile error. agentscope-extensions does not call it in-tree, which is why CI stays green.

Since the release is pre-2.0 this may be acceptable, but it should be a deliberate call: either keep the old signature as a @Deprecated bridge returning Object[], or list it in the migration/release notes alongside the SandboxState accessors removed in the sibling PR #2194. Same question for the BaseSandboxFilesystem protected surface if any of it changed shape.

[oss-maintainer]

[Info] Cross-reference with PR #3269, which also edits this file and removes a quoted-space write command while surfacing its error output. Both changes are about how user-controlled strings reach the sandbox shell, so it is worth a sentence here (or there) saying whether the heredoc + shellQuoted-args form introduced here is meant to become the single command-construction path for this class, or whether #3269's write path stays as it is. That saves the next reader working out which of the two conventions applies where.

[oss-maintainer]

Follow-up on 648fe7bf (pushed while my review was being written): dropping the non-atomic shutil.copyfile fallback is the right call - as the new comment says, a copy fallback reintroduces exactly the truncate-on-failure risk the temp-file + os.replace dance exists to avoid, and the uuid4 temp name is a real fix over os.getpid() for concurrent edits sharing a long-lived shell process.

Two corrections to my review above, in case the diff view is confusing:

• My summary mentioned "the read-only-directory fallback". On this head that in-script fallback is gone; a write_failed now falls through to the Java transfer path only. The [Warning] on editViaTransfer still applies unchanged (it still re-uploads against filePath, so symlink preservation is native-path-only).
• The [Info] about temp-dir naming was anchored to the older head; nothing there needs action beyond what is already in the thread.

No new issues found in 648fe7bf. The FilesystemUtils return-type compatibility question is still the open one.

---

Automated review by github-manager-bot

[oss-maintainer]

Re-checked against 648fe7bf, which landed while my review was in flight.

Two things in it are outright improvements: dropping the non-atomic shutil.copyfile fallback (a copy fallback reintroduces exactly the truncate-on-failure window the temp-file + os.replace dance exists to close), and switching the temp name from os.getpid() to uuid4, which is the correct uniqueness domain for concurrent edits that share a long-lived shell process.

That does not change my two [Warning]s, and the new javadoc actually sharpens the first one: the documented contract is now "editing a symlink writes through to the file it points at and the symlink itself is preserved", but editViaTransfer reaches the file through downloadFiles/uploadFiles on filePath rather than the resolved target, so on a python3-less backend the contract is exactly what the native path was written to protect. One sentence stating the fallback's symlink behaviour, or resolving the target before the re-upload, closes it.

The FilesystemUtils.performStringReplacement return-type question is unchanged and is the only thing I'd like an explicit decision on before this merges.

---

Automated review by github-manager-bot

[chcodex]

@oss-maintainer please re-review

Follow-up at 8c955060 addressing the two [Warning]s from your review on 648fe7bf:

1. Fallback symlink semantics (decided: document the divergence, option a)

• editViaTransfer javadoc now states explicitly that it re-uploads against filePath as given (no realpath resolution), so symlink preservation depends on the backend's upload semantics — write-through keeps the link, replace breaks it — while the native path guarantees preservation. Doing the same realpath resolution in Java would mean an extra exec round-trip on every fallback edit and would make the two paths pretend to be identical while the backend's upload behaviour still decides the outcome; documenting is the honest fix.
• Added the requested fake that distinguishes the two semantics: edit_fallback_writeThroughUploadPreservesSymlink and edit_fallback_replacingUploadReplacesSymlink, both through a forced-transfer backend with real files.
• Also added edit_replaceAll_occurrencesParityBetweenNativeAndFallback (your suggestion): replaceAll through native and fallback must report the same count and final content.

2. FilesystemUtils.performStringReplacement return type (decided: internal, no bridge)

• Object[] → ReplacementResult is intentional and stays. All four callers are inside this repository (agentscope-harness: LocalFilesystem, RemoteFilesystem, BaseSandboxFilesystem, and the utility's own tests); the class is not part of a published API surface. Added an @apiNote recording the change and stating that no deprecated bridge is provided, so the decision is discoverable from the Javadoc rather than only in this thread.

3. #3269 cross-reference — that PR removes a double-quoted span containing a space from write()'s existence check; edit()'s command builds every path argument with shellQuote (single-quoted) and carries no double-quoted spans, so the two are already on the same quote-free-spread story; no change needed here.

112 tests passing; PR description updated (test count and coverage list).

[oss-maintainer]

Summary

This PR rewrites BaseSandboxFilesystem.edit() to run a static inline Python script inside the sandbox for string replacement, with old/new crossing the boundary as files (paths only on the command line) and a download-edit-upload fallback when Python is unavailable or the native write fails. It also refactors FilesystemUtils.performStringReplacement() from an untyped Object[] to a type-safe ReplacementResult record and adapts LocalFilesystem/RemoteFilesystem accordingly. The change is a clear improvement over the previous python3 -c + hand-rolled JSON escaping approach and has thorough spy-based and real-shell test coverage. Overall: no critical issues, but a few actionable gaps around fallback completeness and cross-filesystem consistency remain.

Findings

• [Warning] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:367 — isPythonMissing() only treats exit code 127 and a few English substrings as "python3 missing"; exit code 126 (command found but not executable) is missed, so the transfer fallback is skipped in that case.
• [Warning] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:355 — newString == null is treated as deletion in the sandbox filesystem, but LocalFilesystem/RemoteFilesystem still throw NPE on null, creating an inconsistent cross-filesystem API.
• [Info] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:431 — Result JSON is parsed with hand-rolled parseErrorToken()/parseCount() although Jackson is available in the module; using a proper JSON parser would be more robust.
• [Info] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:357 — The native edit path hardcodes /tmp/agentscope-edit-<uuid> as the staging directory, which loses the fast path on backends without a writable /tmp.

Suggestions

1. Extend isPythonMissing() to also recognize exit code 126 so a non-executable python3 degrades gracefully to the transfer path.
2. Apply the same newString == null deletion semantics (or an explicit rejection) to LocalFilesystem.edit() and RemoteFilesystem.edit() so all filesystem implementations behave consistently.
3. Replace the hand-rolled JSON extraction with a small Jackson ObjectMapper read of a result POJO; this removes the brittle substring parsing and makes the code easier to maintain.
4. Make the sandbox staging directory configurable or backend-derived instead of hardcoding /tmp.

---

Automated review by github-manager-bot

[oss-maintainer]

isPythonMissing() only treats exit code 127 and a few English substrings as 'python3 missing'. Exit code 126 (command found but not executable) is not handled, so a sandbox where python3 exists but lacks execute permission will surface 'unexpected server response' instead of falling back to the transfer path. Add exitCode == 126 to the fallback condition.

[oss-maintainer]

BaseSandboxFilesystem.edit() now treats newString == null as deletion (empty replacement), but LocalFilesystem.edit() (line 370) and RemoteFilesystem.edit() (line 307) still throw NPE on a null newString because they pass it straight to String.replace()/FilesystemUtils.performStringReplacement(). Since this PR touches all three filesystem implementations, unify the null semantics or document that only the sandbox filesystem supports deletion via null.

[oss-maintainer]

mapNativeResult()/parseErrorToken()/parseCount() implement a hand-rolled JSON parser even though Jackson is already on the module classpath (e.g., HarnessSandboxJacksonModule). Using a small ObjectMapper or JsonParser would make the result parsing robust to field reordering, whitespace, and future error values that contain escaped quotes.

[oss-maintainer]

The native edit path hardcodes /tmp/agentscope-edit- as the parameter staging directory. Sandbox backends that do not expose a writable /tmp (or map /tmp differently) will silently lose the fast native path and always fall back to download-edit-upload. Consider making the staging prefix configurable or deriving it from the backend's workspace temp location.

[oss-maintainer]

Summary

This PR rewrites BaseSandboxFilesystem.edit() to run a static inline Python script inside the sandbox for string replacement, with old/new crossing the boundary as files (paths only on the command line) and a download-edit-upload fallback when Python is unavailable or the native write fails. It also refactors FilesystemUtils.performStringReplacement() from an untyped Object[] to a type-safe ReplacementResult record and adapts LocalFilesystem/RemoteFilesystem accordingly. The change is a clear improvement over the previous python3 -c + hand-rolled JSON escaping approach and has thorough spy-based and real-shell test coverage. Overall: no critical issues, but a few actionable gaps around fallback completeness and cross-filesystem consistency remain.

Findings

• [Warning] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:367 — isPythonMissing() only treats exit code 127 and a few English substrings as "python3 missing"; exit code 126 (command found but not executable) is missed, so the transfer fallback is skipped in that case.
• [Warning] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:355 — newString == null is treated as deletion in the sandbox filesystem, but LocalFilesystem/RemoteFilesystem still throw NPE on null, creating an inconsistent cross-filesystem API.
• [Info] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:431 — Result JSON is parsed with hand-rolled parseErrorToken()/parseCount() although Jackson is available in the module; using a proper JSON parser would be more robust.
• [Info] agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystem.java:357 — The native edit path hardcodes /tmp/agentscope-edit-<uuid> as the staging directory, which loses the fast path on backends without a writable /tmp.

Suggestions

1. Extend isPythonMissing() to also recognize exit code 126 so a non-executable python3 degrades gracefully to the transfer path.
2. Apply the same newString == null deletion semantics (or an explicit rejection) to LocalFilesystem.edit() and RemoteFilesystem.edit() so all filesystem implementations behave consistently.
3. Replace the hand-rolled JSON extraction with a small Jackson ObjectMapper read of a result POJO; this removes the brittle substring parsing and makes the code easier to maintain.
4. Make the sandbox staging directory configurable or backend-derived instead of hardcoding /tmp.

---

Automated review by github-manager-bot

[oss-maintainer]

isPythonMissing() only treats exit code 127 and a few English substrings as 'python3 missing'. Exit code 126 (command found but not executable) is not handled, so a sandbox where python3 exists but lacks execute permission will surface 'unexpected server response' instead of falling back to the transfer path. Add exitCode == 126 to the fallback condition.

[oss-maintainer]

BaseSandboxFilesystem.edit() now treats newString == null as deletion (empty replacement), but LocalFilesystem.edit() (line 370) and RemoteFilesystem.edit() (line 307) still throw NPE on a null newString because they pass it straight to String.replace()/FilesystemUtils.performStringReplacement(). Since this PR touches all three filesystem implementations, unify the null semantics or document that only the sandbox filesystem supports deletion via null.

[oss-maintainer]

mapNativeResult()/parseErrorToken()/parseCount() implement a hand-rolled JSON parser even though Jackson is already on the module classpath (e.g., HarnessSandboxJacksonModule). Using a small ObjectMapper or JsonParser would make the result parsing robust to field reordering, whitespace, and future error values that contain escaped quotes.

[oss-maintainer]

The native edit path hardcodes /tmp/agentscope-edit- as the parameter staging directory. Sandbox backends that do not expose a writable /tmp (or map /tmp differently) will silently lose the fast native path and always fall back to download-edit-upload. Consider making the staging prefix configurable or deriving it from the backend's workspace temp location.

[chcodex]

@oss-maintainer please re-review

Follow-up at 7040a75a, addressing the findings from your review on 8c955060/c55d57f8:

1. [Warning] exit 126 — fixed. isPythonMissing() now treats 126 (command found but not executable — e.g. python3 present with the execute bit cleared) the same as 127, and also matches permission denied / not executable output. New test edit_pythonNotExecutable_exit126_fallsBackToTransfer.

2. [Warning] newString == null — decision: reject it everywhere (option b). A null newString is not a deletion; "" is, and it keeps the normal replacement semantics and occurrence count. Validation lives in one shared place, FilesystemUtils.validateEditArguments(), applied by LocalFilesystem, RemoteFilesystem and BaseSandboxFilesystem alike, so all three now fail identically instead of NPE-ing (local/remote) or silently deleting (sandbox). The same guard also rejects an empty oldString, which would otherwise make countOccurrences loop forever — a hang reachable today on LocalFilesystem/RemoteFilesystem. Covered in LocalFilesystemModeTest#edit_rejectsNullNewString_andEmptyOldString and RemoteFilesystemCASTest#edit_rejectsNullNewString_andEmptyOldString, asserting the file is untouched on rejection and that "" still deletes with a correct count.

3. [Info] Jackson parsing — done. parseErrorToken()/parseCount() are gone; the __RESULT__ payload is read into a NativeScriptResult record with ObjectMapper. Unparseable payloads are logged and surfaced verbatim (new test edit_malformedResultJson_failsWithRawPayload).

4. [Info] hardcoded /tmp staging — no change, agreed. /tmp is the standard temp dir on the Linux images all sandbox backends run, and the failure mode is graceful: a non-writable /tmp only costs the fast path, since param upload failure already falls back to the transfer path (covered by edit_native_paramUploadFails_fallsBackToTransfer). Adding a configuration knob would widen the API surface without a failing case today.

125 tests in the filesystem suites, 1090 in the module, all passing; rebased on current main; PR description updated.

[oss-maintainer]

Summary

Re-review at 7040a75a. This round moves the sandbox edit onto a static heredoc script (old/new cross the boundary as files, only paths on the command line), adds strict UTF-8 decoding, a JSON-parsed result channel instead of substring sniffing, and a transfer fallback when python3 is unavailable or the atomic write fails. The previous findings are addressed — the payload no longer flows through the command string, result parsing is robust against user-controlled text, and all three filesystem backends now share one argument validator. Tests are extensive (1053-line extension of BaseSandboxFilesystemTest). CLA signed; builds green on ubuntu/windows.

Findings

• [Warning] BaseSandboxFilesystem.java:372 — the staging path is hard-coded to /tmp, outside the default sandbox transfer root (/workspace) for the Docker/K8s backends: the native path would never be used in production sandboxes, silently keeping the fallback as the default.
• [Warning] FilesystemUtils.java:52 — public performStringReplacement return type changed with no bridge; source/binary break for downstream users of the published artifact.
• [Info] BaseSandboxFilesystem.java:372 — 32 bits of UUID entropy for a shared /tmp staging dir, and the trailing rm -rf can clobber a concurrent edit's payload.
• [Info] BaseSandboxFilesystem.java:405 — constant heredoc delimiter with an agent-controlled path lets a path containing __AGENTSCOPE_EDIT_PY__ end the heredoc early.
• [Info] FilesystemUtils.java:79 — empty oldString is reported as "string not found" rather than a validation error.

The staging-path point is the one that affects whether this change delivers its goal; the rest are non-blocking.

---

Automated review by github-manager-bot

[oss-maintainer]

/tmp/agentscope-edit-... is outside the sandbox transfer root used by the Docker/Kubernetes backends (/workspace by default), so the two param uploads are rejected there and every edit silently degrades to the download/replace/re-upload fallback even when python3 is available — the native path never runs in exactly the environments it was written for. Please stage the old/new files under the same root uploadFiles accepts (or probe the root), and emit a log line when the native path is skipped.

[oss-maintainer]

8 hex characters (32 bits) of UUID are weak if two concurrent edits ever share a sandbox, and the trailing rm -rf of the staging directory can delete the peer's still-unexecuted payload. uuid.randomUUID().toString() costs nothing and removes the collision class.

[oss-maintainer]

The heredoc delimiter is a constant while filePath is agent-controlled: a path containing __AGENTSCOPE_EDIT_PY__ at line start terminates the heredoc early and the remainder of the path becomes shell input. shellQuote covers the arguments but not the body — deriving a per-call delimiter (as the unique tmp dir already does) would close it.

[oss-maintainer]

performStringReplacement is a public method of the published harness artifact and its return type changed from Object[] to ReplacementResult with no bridge — source- and binary-incompatible for downstream callers. The javadoc notes this is intentional; please make sure it is called out in the release notes / changelog so the change is not a surprise.

[oss-maintainer]

An empty oldString is rejected with "String not found in file", which reads as a content error rather than a validation error, and differs from the sandbox script's own old == "" path. A dedicated message ("oldString must not be empty") would help the model self-correct.

[chcodex]

@oss-maintainer please re-review

Follow-up at 929828e2. This round resolves the staging-path Warning at the root and lands the deprecated bridge you asked for a decision on.

1. [Warning] /tmp staging made the native path unreachable — fixed by removing the staging entirely.

I traced why before changing anything. uploadFiles rejects any path outside the sandbox workspace: resolveArchivePath throws for the tar-hydrate fallback, and both native implementations refuse it too — DockerSandbox.requireTransferPath requires the path under workspaceRoot, and on Kubernetes the runtime itself confines /upload//download to WORKSPACE_DIR (agentscope-runtime sandbox/box/shared/routers/workspace.py, ensure_within_workspace, HTTP 403 otherwise). So the two param files could never be staged, edit() fell back to the transfer path on every call, and the native path never ran in production. Opening /tmp in either backend would not have worked — the server side is the boundary.

Rather than widen that boundary (which would have needed per-backend changes, an exec fallback with its own size cap, and a snapshot-pollution argument about workspace staging), the params now travel base64-encoded in the command:

• base64 has no shell metacharacters, so user content cannot break out of its single-quoted argument; the file itself never crosses the boundary.
• No staging directory, no cleanup command, no concurrent-cleanup race, and no dependency on the transfer APIs at all — which is what makes this portable across Docker, Kubernetes, and the backends that have no dedicated transfer channel.

2. Size bound is on the whole command, not per parameter (your earlier point, which I had wrong). The command is a single argv element, so the kernel MAX_ARG_STRLEN (128 KiB; measured E2BIG at exactly 128 KiB here) applies to the script body, the target path and both payloads combined. MAX_INLINE_COMMAND_BYTES is 112 KiB. Over that, the call is rejected with a "split the edit into smaller pieces" diagnostic and nothing is sent to the sandbox — not a silent switch to the transfer path. old/new are edit fragments, so a request that large indicates the edit should be split anyway.

3. [Warning] performStringReplacement — decision: deprecated bridge. agentscope-harness is deployed to Sonatype (distributionManagement) and the method shipped as Object[] in v2.0.1/v2.0.2/v2.0.3, so the break is real. Since Java cannot overload on return type, the old name keeps the Object[] signature and is now @Deprecated, delegating to the new stringReplacement() that returns ReplacementResult; all three implementations use the new name. FilesystemUtilsTest pins the legacy contract (two-element array on success, single-element on error) and asserts the bridge and the typed result agree.

4. Other items from your review

• [Info] isPythonMissing() also treats 126 (found but not executable) — done.
• [Info] Jackson result parsing replacing the hand-rolled parseErrorToken/parseCount — done.
• [Info] empty oldString now reports a validation error, not "String not found"; it is also rejected on the local and remote filesystems, where it used to hang countOccurrences forever.
• [Info] heredoc delimiter in a path cannot truncate the command: a newline inside single quotes stays inside the argument, and the heredoc body is read only after the command line is complete. Added an integration test with a delimiter-shaped path.

138 tests in the filesystem suites, 1103 in the module; rebased on current main; description updated.

[chcodex]

Why the approach changed during review

Since the head now differs from the first reviewed version by a design decision rather than a refactor, recording the reasoning so it does not have to be re-derived. (Also added to the description.)

The first revision staged old/new as files under /tmp/agentscope-edit-<uuid>/ and uploaded them via uploadFiles(). It looked fine — the staging directory is a temp dir, the payload is small — until the transfer path was actually traced:

• uploadFiles rejects any path outside the sandbox workspace. The tar-hydrate fallback throws in resolveArchivePath; DockerSandbox.requireTransferPath requires the path under workspaceRoot; and on Kubernetes the boundary is enforced server-side — agentscope-runtime, sandbox/box/shared/routers/workspace.py, ensure_within_workspace() returns HTTP 403 outside WORKSPACE_DIR.
• Therefore the staging upload could never succeed. edit() fell back to the transfer path on every call, which means the native path this PR exists to provide was unreachable in production. The tests passed because the local-shell fake implements uploadFiles directly, with no workspace boundary — the failure was invisible from the test suite.

Options considered, and why the chosen one:

Option	Why not
Allow /tmp on Docker	docker cp allows it, but fixes only one backend, and the K8s boundary is server-side — it cannot be opened client-side.
Write staging inside the workspace	persistWorkspace tars the workspace root, so the transient directory (containing raw old/new) lands in session snapshots, is visible to ls/glob, and can be caught by a concurrent git add -A.
Workspace + mv into /tmp	Same snapshot exposure, plus a second channel and two failure modes.
base64 in the command (chosen)	Removes the staging round trip, the cleanup command, the concurrent-cleanup race, and the dependency on the transfer APIs entirely. One command, one semantics, portable across every backend.

Two consequences, both deliberate and worth your scrutiny:

1. User content now does enter the command string, as base64. The alphabet has no shell metacharacters, so the injection surface stays zero and no escaping layer is needed — but the payloads are decodable from logs, which weakens the "raw content never reaches the command line" property that earlier reviews highlighted. It is a weakening, not a break.
2. Oversized arguments fail instead of degrading. The whole command is one argv element, so the kernel MAX_ARG_STRLEN (128 KiB, E2BIG verified at exactly that size here) covers the script, the path and both payloads together. Past 112 KiB the call is rejected with a "split the edit into smaller pieces" diagnostic and nothing is sent to the sandbox.

One process note for my own record: I initially proposed bounding old and new separately, which would have allowed ~192 KiB combined and broken at 128 KiB. The bound has to be on the assembled command, and the diagnostic has to name the real limit — both are now covered by tests.

[oss-maintainer]

Summary

PR #2398 rewrites BaseSandboxFilesystem.edit() to run a static inline-Python edit natively inside the sandbox (old/new crossing as base64-encoded command-line arguments, heredoc script, Jackson-parsed RESULT line) with a download → stringReplacement → re-upload fallback, plus a typed ReplacementResult refactor across all three filesystems. Follow-up commit 929828e genuinely closes all three earlier findings: the /tmp staging is removed at the root rather than relocated (verified against DockerSandbox.requireTransferPath and resolveArchivePath, which would have rejected it — the native path now performs zero file transfers), exit 126 now triggers the transfer fallback alongside 127, and performStringReplacement keeps its exact published Object[] signature as a @deprecated bridge; I additionally verified the generated command end-to-end with real sh/python3 (simple/special-char edits, symlink write-through with the link preserved, exit-code passthrough, and no heredoc break-out from a delimiter-shaped or newline-bearing path). Remaining issues are minor: a stale class-level javadoc still asserting the old "command line carries paths only" property, os.stat outside the script's try (a TOCTOU race yields a raw traceback instead of a mapped error, though the atomic temp+os.replace write itself is sound), the transfer fallback mislabeling any download failure as "file not found", and a CRLF-normalization divergence that undercuts the claimed three-way parity.

Re-reviewed at head 929828e2e7; CLA: unknown.

---

Automated review by github-manager-bot

[oss-maintainer]

editViaTransfer reports "Error: File 'X' not found" for any downloadFiles failure, discarding the real reason the response carries. In the write_failed → fallback flow the file provably exists (the native script just read and edited it), so a transient transfer failure — backend 504 / exec error / truncated output, all of which SandboxBackedFilesystem.downloadFiles reports via FileDownloadResponse.fail(path, actualMessage) — is misdiagnosed as the file being absent. An agent that trusts that message may abandon the edit or try to recreate the file. The upload branch ~30 lines below surfaces its error ("Error writing edited file ...: " + err); mirror it using downloaded.get(0).error(), keeping "not found" only for downloaded.isEmpty() if that distinction is wanted.

[oss-maintainer]

The class-level javadoc still describes the removed staging design: "old/new cross the boundary as files, the command line carries paths only". The head actually sends them base64-encoded on the command line (nativeEditCommand), which the PR description itself flags as a deliberate weakening of exactly this property. Since the whole review history of this PR turned on the "user content never enters the command string" claim, leaving the old (now false) assertion here will mislead the next auditor of the injection surface. One-line fix: align it with the accurate method-level javadoc ("base64-encoded on the command line; the alphabet has no shell metacharacters").

[oss-maintainer]

TOCTOU gap in the script: st = os.stat(real) sits outside the try that guards the write. If the resolved target is unlinked (or its permissions change) between the open(target, "rb") read and this stat — e.g. a concurrent command in the same sandbox — the script dies with an uncaught traceback: no __RESULT__ line, exit 1, and Java surfaces "unexpected server response: Traceback..." instead of a mapped read_failed/write_failed, and skips the transfer retry. Nothing is corrupted (no write has happened yet), so this is diagnostics-only, but moving the realpath/stat/tmp_out block inside the try and reporting write_failed keeps every failure on the designed result channel. Note the atomic temp+os.replace write itself is sound — on failure the target is untouched, verified live.

[oss-maintainer]

The PR claims "Replacement semantics match FilesystemUtils.stringReplacement exactly … so all three filesystems behave identically", but LocalFilesystem still normalizes \r\n→\n for content, old and new (and writes the whole file back with LF endings), while the sandbox script and RemoteFilesystem do exact-byte matching. On a CRLF file, an LF-style oldString succeeds locally — silently converting every remaining CRLF to LF — but returns string_not_found on the sandbox path (verified: the inline script matches old as exact bytes). This is the same class of cross-filesystem inconsistency the PR fixed for null newString via the shared validateEditArguments; either normalize in all three implementations or document the divergence.

[chcodex]

@oss-maintainer please re-review

Follow-up at d5456636, addressing the four minor findings from your review at 929828e2. Thanks also for verifying the generated command end-to-end with real sh/python3 — that is what the local fake cannot tell you.

1. Stale class javadoc — fixed. The class-level text still claimed the command line carries paths only; it now describes the base64-encoded params and points at stringReplacement.

2. os.stat outside the script's try — fixed. stat now shares the write try block, so a target that disappears between the read and the stat maps to write_failed (which the Java side correctly reads as "target untouched" and retries via transfer) instead of printing a raw traceback with no __RESULT__ line and surfacing as "unexpected server response".

3. Transfer fallback mislabeled every download failure as file_not_found — fixed. It now distinguishes them: a backend-side read failure reports Could not read file '<path>' for editing: <cause> with the underlying reason (permissions, transport, truncated response) attached. Reporting that as "not found" would send the caller off to re-read a file it already has. The native path's file_not_found still means the path genuinely does not exist. Tests cover both the empty-response and the explicit-error case.

4. CRLF divergence, and the claimed three-way parity — fixed, by unifying rather than by softening the claim. FilesystemUtils.stringReplacement now normalizes CRLF and lone CR to LF on the content and on both patterns, and LocalFilesystem drops its own pre-normalization as redundant. Rationale: CRLF is a defect on a Unix-style filesystem, so normalizing on write is the intended outcome, and it is exactly what LocalFilesystem has always done — the other two were the outliers. A side effect worth flagging explicitly: editing a CRLF file through the remote or sandbox path now rewrites it with LF endings. That is a behavior change on those backends, made in the direction of the local filesystem's established semantics rather than inventing a third option (preserving CRLF would have been the "safe" choice but would have kept the two implementations disagreeing about what \n means). Tests cover CRLF↔LF matching in both directions, lone CR, mixed line endings under replaceAll, LF-only write-back, and — importantly — that the >1 occurrence guard still fires on CRLF content, since otherwise the same edit would be accepted on one backend and rejected on another.

147 tests in the filesystem suites, 1112 in the module; javadoc:jar clean; rebased on current main; description updated.

[oss-maintainer]

Summary

Commit d545663 unifies LF normalization by moving it into FilesystemUtils.stringReplacement, closes the os.stat TOCTOU gap by moving the stat inside the Python script's write try-block, fixes the transfer-fallback error to report the real download failure cause, and updates the stale class-level javadoc; the prior review's four findings are therefore fully resolved except for the CRLF parity item, which is only half-fixed. The sandbox native Python path still performs exact-byte replacement, while the fallback now normalizes via FilesystemUtils.stringReplacement, so the same edit can succeed or fail depending on whether python3 is available. This reintroduces an inconsistency within BaseSandboxFilesystem itself and leaves the claimed three-way filesystem parity incomplete.

---

Automated review by github-manager-bot

[oss-maintainer]

The moved line-ending normalization makes FilesystemUtils.stringReplacement LF-normalize content and both patterns, but the sandbox native Python script (unchanged in this diff) still performs exact-byte replacement. That creates a new native-vs-fallback divergence inside BaseSandboxFilesystem: an edit of a CRLF file with an LF pattern succeeds via the python3-missing fallback but fails on the native path. To fully unify semantics, the Python helper should apply the same CRLF/lone-CR→LF normalization before matching and writing, or the fallback should be kept exact-byte to match the native path.

[chcodex]

@oss-maintainer please re-review

02797dfe — conflict resolved (state is CLEAN again) and the remaining CRLF parity finding is closed.

1. Conflict with #3226. It rewrote the same edit() on the same class, so ours was kept and its substance preserved:

• Its three canned tests are carried over. The one asserting python3 -c is retargeted at the heredoc form. I narrowed that assertion on purpose: the old assertFalse(cmd.contains("\\n")) can no longer hold, because the CRLF normalizer legitimately uses a backslash-n inside a Python string literal. The invariant behind [Bug]: BaseSandboxFilesystem.edit fails when replaceAll=false because the generated Python command contains literal \\n sequences` #2571 is that program statements are separated by real line feeds, so the test now asserts that the first and a later statement each occupy their own line.
• Its two executeFailure tests now pass because edit() reports execution-layer failures through the same executeFailureMessage() convention as ls/read/grep/glob — edit() had been the last method in the class still emitting "unexpected server response".
• Its three integration tests are behaviourally identical to the edit_simpleReplacement / edit_multipleOccurrencesWithoutReplaceAll / edit_replaceAll tests already on this branch. The one assertion they added (a rejected ambiguous edit leaves the file untouched) is folded into ours rather than kept as a near-duplicate. Verified by diffing the test-name sets against main: nothing on main is lost.

2. CRLF parity — you were right that it was only half-fixed. The native script still replaced bytes exactly while the fallback normalized, so the same edit could succeed with python3 and fail without it, and the three-way parity claim was not true within BaseSandboxFilesystem itself. The script now applies the same LF normalization as FilesystemUtils.stringReplacement, so both paths accept and reject identically. Tests cover: a CRLF file matching an LF pattern, native-vs-fallback parity on the same input, and — the one that would otherwise have slipped through — the >1 occurrence guard still firing on CRLF content on the native path, with the file left untouched.

One implementation note, since it is easy to trip over again: the normalization snippet inside the Java text block has to escape as \\r\\n. Written as \r\n the text block emits real CR/LF bytes, which land inside the Python string literal and split it across lines — that is a SyntaxError, and it took out 12 integration tests before I caught it. The doubled form is commented in place.

156 tests in the filesystem suites, 1118 in the module; javadoc:jar clean; spotless applied; rebased onto current main; description updated.

[oss-maintainer]

Summary

Re-review of 02797dfe (responding to your request). The conflict with #3226 is resolved correctly and the CRLF parity finding is closed: the program is now fed through python3 - with a quoted heredoc so statements are separated by real line feeds, user content only reaches the sandbox as base64 arguments, no literal \n is emitted into the generated program, and the retargeted assertions pin the real invariant instead of the over-broad assertFalse(cmd.contains("\\n")). The no-marker failure path also now reports through executeFailureMessage(...), consistent with ls/read. One test-coverage gap remains, so this is a comment rather than a blocking change request.

Findings

• [Warning] BaseSandboxFilesystemTest.java:252 — the new 98 lines cover CRLF parity, failure reporting and one write-failed fallback case, but nothing pins the heredoc-quoting edges (single quotes / backticks in the path, trailing-newline content).

Suggestions

The file path is the one remaining piece of user input that still goes through shell quoting here — a case with ' or a backtick in the path, plus a content case ending in a newline, would lock in the command construction you just reworked. Apart from that, thanks for keeping #3226's canned tests and re-targeting them rather than dropping them.

---

Automated review by github-manager-bot

Additional notes (not anchored to a changed line)

• [WARNING] agentscope-harness/src/test/java/io/agentscope/harness/agent/filesystem/sandbox/BaseSandboxFilesystemTest.java:252 — The 98 new test lines verify CRLF parity, executeFailure reporting, and one write_failed fallback case, but do not add explicit tests for heredoc-quoting edge cases. Add tests for a file path containing single quotes (the only user input still shell-quoted), backticks, and a trailing-newline pattern in file content to ensure the python3 - <<'...' command construction stays robust. (line outside diff)

[chcodex]

@oss-maintainer please re-review

c23053b2d — closes the remaining test-coverage finding. State is CLEAN again (merged current main; upstream's a2cbea5c3 touches SandboxBackedFilesystem, which does not overlap this branch's edit() path).

Heredoc quoting edges. The target path is the last piece of user input that still passes through shell quoting, since old/new are base64 — so that is what the new tests pin:

• edit_pathContainingSingleQuote_editsTheRightFile — a path containing a quote, next to a decoy file one substitution away (it's a file.txt vs its a file.txt). If the '\'' escaping were wrong the edit could land on the wrong file; the test asserts the intended file changed and the decoy did not.
• edit_pathContainingBacktick_doesNotTriggerCommandSubstitution — a backticked path, asserting no substitution runs.
• edit_paramsEndingWithNewline_surviveEncodingAndHeredoc — old/new ending in a newline, which would split the command into extra lines if the base64 encoding did not protect them.

Each asserts the resulting file content, not just the EditResult — the failure mode being guarded is landing on the wrong path, which a success/failure check alone would not catch.

159 tests in the filesystem suites, 1303 in the module; javadoc:jar clean; spotless applied; description updated. As far as I can see this closes every finding raised so far, so this should be the last round unless you see something I have missed.

[Buktal]

docs/v2/en/docs/harness/sandbox.md:208 and docs/v2/zh/docs/harness/sandbox.md:207 still list python3 as a required image-contract item for edit_file. This PR deliberately makes it optional — a missing or non-executable python3 now degrades edit_file to the transfer path — so both rows now mislead image selection. Please update both locales: mark python3 as optional for edit_file (native fast path / transfer fallback), and while there record the other user-visible changes this PR introduces — null newString and empty oldString are now rejected uniformly by all three filesystems, and an edit rewrites CRLF/lone-CR content back with LF endings on the remote and sandbox paths (previously exact-byte there). The contribution workflow requires matching docs/v2 EN+ZH updates for user-visible behavior changes; this PR currently contains none.

FilesystemUtils.performStringReplacement (agentscope-harness/src/main/java/io/agentscope/harness/agent/filesystem/util/FilesystemUtils.java:101) keeps the published v2.0.x Object[] signature but now silently LF-normalizes the content and both patterns by delegating to stringReplacement (FilesystemUtils.java:125-127), where v2.0.1–v2.0.3 did exact-byte matching. Its @Deprecated javadoc states stringReplacement "returns the same result" (FilesystemUtils.java:96), which is no longer true relative to the versions the bridge shipped in. Either restore exact-byte semantics inside the bridge, or state the normalization explicitly in the bridge javadoc and in the release notes.

Non-blocking notes:

• decodeUtf8Strict (BaseSandboxFilesystem.java:553) duplicates the strict UTF-8 decoder in RemoteFilesystem.java:591-597; a single shared implementation in FilesystemUtils would serve both.
• Public FilesystemUtils.stringReplacement still loops forever on an empty oldString (via countOccurrences, FilesystemUtils.java:168); the guard lives only in validateEditArguments, so a one-line guard inside the utility would protect any future caller.
• validateEditArguments never reads its filePath parameter (FilesystemUtils.java:78).
• A hard kill between temp-file creation and os.replace leaves .agentscope-edit-tmp-<uuid> files in the workspace (BaseSandboxFilesystem.java:301) — visible to ls/glob and captured by snapshots. A stale-file sweep on edit, or a documented note, would close it.

[mintlify]

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit bb8671a.

Approve preview

[chcodex]

@Buktal please re-review

Follow-up at bb8671a5b, addressing your 2026-10-21 review:

1. Docs v2 EN+ZH updated. The sandbox image contract now marks python3 optional for edit_file (native fast path; download/replace/upload fallback when missing or non-executable), describes the 112 KiB whole-command limit and split-edit guidance, and notes the fallback may be slower. The filesystem guides now document null/empty argument behavior and CRLF/lone-CR → LF normalization.
2. Published bridge semantics preserved. performStringReplacement still has the v2.0.x Object[] signature and now also keeps its exact line-ending matching behavior. It calls a shared exact-string helper; only the new typed stringReplacement normalizes line endings. The bridge Javadoc says this explicitly, and a regression test distinguishes CRLF behavior of the legacy bridge vs the new API.
3. UTF-8 decoding centralized. Sandbox fallback and RemoteFilesystem now share FilesystemUtils.decodeUtf8Strict; invalid UTF-8 still fails rather than being silently substituted.
4. Empty needle guarded at the public utility. stringReplacement rejects empty/null oldString directly; countOccurrences also returns 0 for an empty needle instead of looping. Removed the unused filePath parameter from validateEditArguments.
5. Temporary-file hard-kill caveat documented. A SIGKILL before os.replace can leave a UUID-named sibling temp file visible to listings/snapshots. I did not add an age-based sweep: it could delete a slow concurrent edit's live temp file. This limitation is now stated in the edit Javadoc and script comment.

Checks: 164 filesystem tests pass; agentscope-harness 1363 pass, 0 fail, 10 skipped; docs tests, docs validation/build, broken-links, and javadoc:jar all pass. Merged current main; PR head is bb8671a5b.

[chcodex]

@Buktal please re-review

Follow-up at bb8671a5b addressing your 2026-10-21 review:

1. EN/ZH docs updated. python3 is now marked optional for edit_file: present/executable selects the native fast path; absent/non-executable uses the transfer fallback. Both sandbox guides describe the whole-command 112 KiB cap and split-edit diagnostic. Both filesystem guides document that oldString must be non-empty, newString cannot be null ("" means deletion), and CRLF/lone-CR is normalized to LF.

2. Deprecated bridge preserves the published semantics, not only the signature. performStringReplacement() now calls a separate exact-string replacement helper and keeps v2.0.x line endings unchanged. The new typed stringReplacement() alone normalizes CRLF/lone-CR to LF. Its Javadoc says this explicitly, and performStringReplacement_bridgePreservesExactLineEndingSemantics pins the distinction.

3. Shared strict UTF-8 decoder. Extracted FilesystemUtils.decodeUtf8Strict() and use it from both sandbox fallback and RemoteFilesystem; test covers valid UTF-8 and malformed bytes.

4. Empty oldString guard in the utility. stringReplacement() itself rejects null/empty old strings; countOccurrences() defensively returns zero for an empty needle rather than looping. validateEditArguments no longer accepts the unused filePath parameter.

5. Temp-file hard-kill caveat documented. A SIGKILL between creation and os.replace can leave the UUID-named sibling temp file visible in listings/snapshots. Added to the edit Javadoc and script comment. No age-based sweep: it could race with a slow concurrent edit.

Checks: 164 filesystem tests pass; agentscope-harness 1363 pass, 0 fail, 10 skipped; docs tests 13/13, docs validation/build and broken-links pass; javadoc:jar passes. Merged current main; PR description updated.