Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,8 @@ public WriteResult write(RuntimeContext runtimeContext, String filePath, String
"if [ -e "
+ escapedPath
+ " ]; then echo 'EXISTS'; exit 1; fi; "
+ "mkdir -p \"$(dirname "
+ escapedPath
+ ")\" 2>&1";
+ FilesystemUtils.mkdirParent(filePath)
+ " 2>&1";

ExecuteResponse checkResult = execute(runtimeContext, checkCmd, null);
if (checkResult.exitCode() != null && checkResult.exitCode() != 0) {
Expand All @@ -211,7 +210,7 @@ public WriteResult write(RuntimeContext runtimeContext, String filePath, String
+ " because it already exists. Read and then make an"
+ " edit, or write to a new path.");
}
return WriteResult.fail("Failed to write file '" + filePath + "'");
return WriteResult.fail(executeFailureMessage(checkResult, "writing", filePath));
}

List<FileUploadResponse> responses =
Expand Down Expand Up @@ -255,30 +254,36 @@ public EditResult edit(
Base64.getEncoder()
.encodeToString(payload.getBytes(java.nio.charset.StandardCharsets.UTF_8));

// The program travels on stdin and the payload as a base64 environment value, so the
// generated command contains no double quote at all. Passing it through `python3 -c "..."`
// would nest double quotes inside the command and lose them on a Windows host; see
// FilesystemUtils#mkdirParent.
String cmd =
"python3 -c \"import sys, os, base64, json\\n"
+ "payload ="
+ " json.loads(base64.b64decode(sys.stdin.read().strip()).decode('utf-8'))\\n"
+ "path, old, new = payload['path'], payload['old'], payload['new']\\n"
+ "replace_all = payload.get('replace_all', False)\\n"
+ "if not os.path.isfile(path):\\n"
+ " print(json.dumps({'error': 'file_not_found'}))\\n"
+ " sys.exit(0)\\n"
+ "with open(path, 'rb') as f: text = f.read().decode('utf-8')\\n"
+ "count = text.count(old)\\n"
+ "if count == 0:\\n"
+ " print(json.dumps({'error': 'string_not_found'}))\\n"
+ " sys.exit(0)\\n"
+ "if count > 1 and not replace_all:\\n"
+ " print(json.dumps({'error': 'multiple_occurrences', 'count': count}))\\n"
+ " sys.exit(0)\\n"
+ "result = text.replace(old, new) if replace_all else text.replace(old, new,"
+ " 1)\\n"
+ "with open(path, 'wb') as f: f.write(result.encode('utf-8'))\\n"
+ "print(json.dumps({'count': count}))\\n"
+ "\" 2>&1 <<'__EDIT_EOF__'\n"
"SAA_EDIT_PAYLOAD='"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Warning] Moving the payload into a single environment-variable assignment puts it on the command line, where Linux's per-string execve limit (MAX_ARG_STRLEN, 128 KiB) applies — the previous heredoc-on-stdin form had no such bound. edit() calls whose base64 payload exceeds ~128 KiB (roughly 96 KiB of JSON content) will now fail with Argument list too long, which is a new failure mode for large edits. Suggest keeping the payload on stdin (and single-quoting / escaping the python3 - program) so the command stays quote-free without reintroducing a size ceiling; if an env-var carrier is intended, please cap the payload and return a clear error above the threshold.

+ payloadB64
+ "\n__EDIT_EOF__\n";
+ "' python3 - 2>&1 <<'__EDIT_PY__'\n"
+ "import base64, json, os, sys\n"
+ "payload ="
+ " json.loads(base64.b64decode(os.environ['SAA_EDIT_PAYLOAD']).decode('utf-8'))\n"
+ "path, old, new = payload['path'], payload['old'], payload['new']\n"
+ "replace_all = payload.get('replace_all', False)\n"
+ "if not os.path.isfile(path):\n"
+ " print(json.dumps({'error': 'file_not_found'}))\n"
+ " sys.exit(0)\n"
+ "with open(path, 'rb') as f: text = f.read().decode('utf-8')\n"
+ "count = text.count(old)\n"
+ "if count == 0:\n"
+ " print(json.dumps({'error': 'string_not_found'}))\n"
+ " sys.exit(0)\n"
+ "if count > 1 and not replace_all:\n"
+ " print(json.dumps({'error': 'multiple_occurrences', 'count':"
+ " count}))\n"
+ " sys.exit(0)\n"
+ "result = text.replace(old, new) if replace_all else text.replace(old,"
+ " new, 1)\n"
+ "with open(path, 'wb') as f: f.write(result.encode('utf-8'))\n"
+ "print(json.dumps({'count': count}))\n"
+ "__EDIT_PY__\n";

ExecuteResponse result = execute(runtimeContext, cmd, null);
String output = result.output() != null ? result.output().strip() : "";
Expand Down Expand Up @@ -430,7 +435,8 @@ public WriteResult move(RuntimeContext runtimeContext, String fromPath, String t
AbstractFilesystem.validatePath(toPath);
String escapedFrom = FilesystemUtils.shellQuote(fromPath);
String escapedTo = FilesystemUtils.shellQuote(toPath);
String cmd = "mkdir -p $(dirname " + escapedTo + ") && mv " + escapedFrom + " " + escapedTo;
String cmd =
FilesystemUtils.mkdirParent(toPath) + " && mv " + escapedFrom + " " + escapedTo;
ExecuteResponse result = execute(runtimeContext, cmd, null);
if (result.exitCode() != 0) {
return WriteResult.fail(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,38 @@ public static String shellQuote(String s) {
}
return "'" + s.replace("'", "'\\''") + "'";
}

/**
* Parent directory of a sandbox path, computed in Java instead of by a shell {@code dirname}.
*
* <p>Sandbox paths are POSIX-style regardless of host OS, so the split is on {@code '/'} only.
*/
public static String parentDir(String path) {
if (path == null || path.isEmpty()) {
return ".";
}
int lastSlash = path.lastIndexOf('/');
if (lastSlash < 0) {
return ".";
}
if (lastSlash == 0) {
return "/";
}
return path.substring(0, lastSlash);
}

/**
* Build {@code mkdir -p} for the parent of {@code path} using single quotes only.
*
* <p>A whole shell program reaches the sandbox as a single command-line argument, and on a
* Windows host Java must serialize that argument into one {@code CreateProcess} command line
* before {@code docker.exe} re-parses it. Single-quoted text survives that round trip, but
* nested double quotes are dropped: {@code mkdir -p "$(dirname 'a b.txt')"} arrives as
* {@code mkdir -p $(dirname 'a} {@code b.txt')"}, the quoted space becomes an argument
* separator, and the shell aborts on an unbalanced parenthesis. Deriving the parent here keeps
* the generated command free of double quotes.
*/
public static String mkdirParent(String path) {
return "mkdir -p " + shellQuote(parentDir(path));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
import io.agentscope.harness.agent.filesystem.model.GrepResult;
import io.agentscope.harness.agent.filesystem.model.LsResult;
import io.agentscope.harness.agent.filesystem.model.ReadResult;
import io.agentscope.harness.agent.filesystem.model.WriteResult;
import io.agentscope.harness.agent.filesystem.util.FilesystemUtils;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
Expand Down Expand Up @@ -364,6 +366,98 @@ void ls_filePath_shouldReturnFail() throws IOException {
// Test helpers
// ================================================================

/**
* Sandbox commands travel to the shell as one command-line argument. Two consequences are
* covered here: an unquoted command substitution word-splits on any host, and nested double
* quotes do not survive a Windows host serializing that argument for {@code CreateProcess}
* (issue #3262 reports {@code 'file1.txt')} arriving intact while the quoting around
* {@code $(dirname ...)} is lost).
*/
@Nested
class CommandConstructionTests {

@Test
@EnabledOnOs({OS.LINUX, OS.MAC})
void move_destinationParentWithSpace_renamesAndCreatesExactlyThatDirectory()
throws IOException {
Path root = Files.createTempDirectory("saa-move").toRealPath();
Path sourceDir = Files.createDirectories(root.resolve("src dir"));
Path source = sourceDir.resolve("a b.txt");
Files.writeString(source, "hello");
Path destination = root.resolve("dst dir").resolve("c d.txt");

WriteResult result =
new LocalShellSandboxFilesystem()
.move(RT, source.toString(), destination.toString());

assertTrue(
result.isSuccess(),
"an unquoted $(dirname ...) word-splits a spaced parent into two words, so mv"
+ " aborts and leaves garbage directories behind: "
+ result.error());
assertEquals("hello", Files.readString(destination));
assertFalse(Files.exists(source), "source should have been renamed away");
assertFalse(Files.exists(root.resolve("dst")), "no directory may come from splitting");
assertFalse(Files.exists(root.resolve("dir")), "no directory may come from splitting");
}

@Test
void generatedCommands_avoidNestedDoubleQuotes() {
FakeSandboxFilesystem capture = new FakeSandboxFilesystem();

capture.write(RT, "/workspace/my dir/note.txt", "content");
assertFalse(
capture.lastCommand.contains("\""),
"write() must not nest double quotes in the command it transports: "
+ capture.lastCommand);

capture.edit(RT, "/workspace/note.txt", "one", "two", false);
assertFalse(
capture.lastCommand.contains("\""),
"edit() must not nest double quotes in the command it transports: "
+ capture.lastCommand);

capture.move(RT, "/workspace/a.txt", "/workspace/new dir/b.txt");
assertFalse(
capture.lastCommand.contains("\""),
"move() must not nest double quotes in the command it transports: "
+ capture.lastCommand);
}

@Test
void write_sandboxFailure_surfacesCommandOutput() {
FixedResponseFilesystem failing =
new FixedResponseFilesystem(
new ExecuteResponse(
"sh: syntax error: end of file unexpected (expecting \")\")",
2,
false));

WriteResult result = failing.write(RT, "/workspace/note.txt", "content");

assertFalse(result.isSuccess());
assertTrue(
result.error().contains("syntax error"),
"a failed write must carry the reason rather than only naming the file, got: "
+ result.error());
}

@Test
void parentDir_coversAbsoluteRelativeBareAndMissingPaths() {
assertEquals("/workspace/docs", FilesystemUtils.parentDir("/workspace/docs/a.md"));
assertEquals("/workspace", FilesystemUtils.parentDir("/workspace/a.md"));
assertEquals("/", FilesystemUtils.parentDir("/a.md"));
assertEquals(".", FilesystemUtils.parentDir("a.md"));
assertEquals(".", FilesystemUtils.parentDir(""));
assertEquals(".", FilesystemUtils.parentDir(null));
}

@Test
void mkdirParent_usesOnlySingleQuotes() {
assertEquals("mkdir -p 'my dir'", FilesystemUtils.mkdirParent("my dir/file.txt"));
}
}

private static final class FakeSandboxFilesystem extends BaseSandboxFilesystem {

String lastCommand;
Expand Down
Loading