Skip to content
Open
Show file tree
Hide file tree
Changes from all 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 @@ -255,30 +255,50 @@ public EditResult edit(
Base64.getEncoder()
.encodeToString(payload.getBytes(java.nio.charset.StandardCharsets.UTF_8));

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"
// Edit script is assembled with real newlines, then base64-encoded and piped via stdin
// to `python3 -`; the payload is written to a temp file first (to avoid ARG_MAX limits),
// then passed as argv[1].
//
// Do NOT revert to `python3 -c "...\n..."` inline form: under `bash -lc`, \n inside
// double quotes is a literal backslash+n (not a newline), so the entire script collapses
// into one line and Python fails with:
// SyntaxError: unexpected character after line continuation character
// This makes edit_file 100% non-functional in sandbox environments (the model falls back
// to write_file, which refuses to overwrite existing files, so no file can be modified).
// Using temp file + pipe avoids all quoting/escaping issues and ARG_MAX limits.
String script =
"import sys, os, base64, json\n"
+ "payload = json.loads(base64.b64decode(open(sys.argv[1],"
+ " \"rb\").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"
+ " 1)\n"
+ "with open(path, 'wb') as f: f.write(result.encode('utf-8'))\n"
+ "print(json.dumps({'count': count}))\n";
String scriptB64 =
Base64.getEncoder()
.encodeToString(script.getBytes(java.nio.charset.StandardCharsets.UTF_8));

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.

[Info] echo is only guaranteed to pass a single long argument through verbatim by bash's builtin; some echo implementations/versions wrap very long output, and a wrapped base64 stream decodes to a truncated script (manifesting as a Python SyntaxError, i.e. the same symptom this PR is fixing). This repo already hit the mirror-image problem on the read path — see SandboxBackedFilesystem#L209 ("MIME decoder tolerates wrapped base64 output from GNU base64"). printf '%s\n' avoids the ambiguity, and it would be worth extending edit_usesBase64PipedScript with a large-payload case asserting the emitted command is a single line.


String cmd =
"cat > /tmp/.agentscope-edit-$$ <<'__EDIT_EOF__'\n"
+ payloadB64
+ "\n__EDIT_EOF__\n";
+ "\n__EDIT_EOF__\n"
+ "printf '%s\\n' "
+ scriptB64
+ " | base64 -d | python3 - /tmp/.agentscope-edit-$$ 2>&1\n"
+ "rm -f /tmp/.agentscope-edit-$$";

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.

The payload lands in /tmp under a name whose only uniqueness token is $$, i.e. the PID of the shell running this one command line (verified: within a single bash login invocation all three $$ expansions resolve identically, so the command does work). Two residual hazards follow from relying on that. (1) cat > creates the file with the process umask — typically 0644 — so the plain-text payload of an edit (the old text plus the new text, which in practice often include connection strings or API keys copied out of the file being edited) is readable by every other user/process in a shared sandbox or container until the final rm -f runs. (2) If the execution backend ever reuses a persistent shell session, $$ is constant across calls and two edits within one turn overwrite each other's temp file. Suggest deriving one unique name per call in Java (or T=$(mktemp /tmp/.agentscope-edit.XXXXXX) and then writing/reading/removing "$T"), which fixes both the permission and the collision question in one move.


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.

The base64 alphabet is newline-free, so the quoted heredoc delimiter can never be terminated early by the payload — that part is safe. What is not safe is the error path: if the temp file ever goes missing or base64 -d fails, the 2>&1 redirect puts a raw Python traceback into output, which matches neither the "error" nor the "count" branch and falls through to the generic "unexpected server response" message. That is exactly how the previous revision's breakage surfaced, so a cheap guard (detect Traceback / No such file or directory in the output and report an execution failure rather than a model-visible edit result) would make the next regression obvious instead of mysterious.

ExecuteResponse result = execute(runtimeContext, cmd, null);
String output = result.output() != null ? result.output().strip() : "";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,57 @@ void glob_executeFailure_shouldFailInsteadOfErrorAsPaths() {
assertFalse(result.isSuccess(), "glob should fail when the command never ran");
assertTrue(result.error().contains("status=504"), "error should carry the cause");
}

@Test
void edit_usesBase64PipedScript() {
FakeSandboxFilesystem fs = new FakeSandboxFilesystem();

// edit() constructs the command and calls execute().
// FakeSandboxFilesystem.execute() returns empty output, so edit() will
// return an error result, but we only care about verifying the command shape.
var result = fs.edit(RT, "/workspace/test.txt", "old", "new", false);

// Verify command uses temp file + printf + base64 pipe (not echo/argv)
assertTrue(
fs.lastCommand.contains("cat > /tmp/.agentscope-edit-$$"),
"edit should write payload to temp file, got: " + fs.lastCommand);

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.

These new assertions still only check the string shape, which is why the previous revision passed CI while being fully broken at runtime — the base64 -d | python3 - /tmp/.agentscope-edit-$$ containment check would also have been satisfied by the heredoc-eats-stdin version. Please add one test that actually runs the assembled command against a temp workspace on a POSIX shell and asserts the edited file content plus the JSON on stdout. I know both tests here were added in the earlier revision rather than this one; the reason I am asking again is that this PR's whole failure mode is shell semantics that no string comparison can distinguish.

assertTrue(
fs.lastCommand.contains("printf '%s\\n'"),
"edit should use printf for script base64, got: " + fs.lastCommand);
assertTrue(
fs.lastCommand.contains("base64 -d | python3 - /tmp/.agentscope-edit-$$"),
"edit should pipe script and pass temp file as argv, got: " + fs.lastCommand);
assertTrue(
fs.lastCommand.contains("<<'__EDIT_EOF__'"),
"edit should use heredoc to write payload, got: " + fs.lastCommand);
assertFalse(
fs.lastCommand.contains("python3 -c"),
"edit should NOT use python3 -c inline form");
assertFalse(
fs.lastCommand.contains("echo "),
"edit should NOT use echo (may wrap long base64), got: " + fs.lastCommand);
}

@Test
void edit_largePayloadUsesHeredocNotArgv() {
FakeSandboxFilesystem fs = new FakeSandboxFilesystem();

// Simulate a large edit (multi-KB payload) to verify it goes through
// temp file (via heredoc), not argv[1] (which would hit ARG_MAX limits).
String largeString = "x".repeat(100_000);
fs.edit(RT, "/workspace/large.txt", "old", largeString, false);

// Payload must NOT appear directly as argv[1]
// It should be written to temp file via heredoc
assertTrue(
fs.lastCommand.contains("<<'__EDIT_EOF__'"),
"large payload should use heredoc to write temp file, got command length: "
+ fs.lastCommand.length());
// Verify the payload is after the heredoc marker, not before it (i.e., not in argv)
int heredocPos = fs.lastCommand.indexOf("<<'__EDIT_EOF__'");
int editEofPos = fs.lastCommand.indexOf("__EDIT_EOF__", heredocPos + 1);
assertTrue(editEofPos > heredocPos, "payload should be between heredoc markers");
}
}

// ================================================================
Expand Down
Loading