Skip to content

Commit 0a81231

Browse files
committed
fix(agent): only treat shell marker as completion when a valid exit code follows
Addresses review feedback on #4740: a command that reads from stdin (e.g. `cat` with no args, `read`, `head -n1`) can consume and echo the injected marker command before it runs. That echoed line contains the marker substring but no valid trailing exit status, so the in-line indexOf match would treat it as completion, leave exitCode null (reported as success), and let the real command keep owning the session. Only set completed when the text following the marker parses as the expected exit-code integer; otherwise keep reading (restoring the previous timeout/ restart behavior for stdin-consuming commands). Added a regression test that runs `cat` and asserts it times out instead of falsely completing.
1 parent 1132247 commit 0a81231

2 files changed

Lines changed: 64 additions & 15 deletions

File tree

spring-ai-alibaba-agent-framework/src/main/java/com/alibaba/cloud/ai/graph/agent/tools/ShellSessionManager.java

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -392,26 +392,29 @@ private CommandResult collectOutput(String marker, long deadline, int maxOutputL
392392
// follow-up echo command, so it usually arrives on its own line. But when the
393393
// command's output has no trailing newline (e.g. `cat` of a file without a final
394394
// newline, or `printf` without `\n`), the line reader merges that trailing output
395-
// and the marker into a single line. Searching for the marker anywhere in the line
396-
// keeps completion detection working, and any real output preceding the marker is
397-
// preserved. See #4740.
395+
// and the marker into a single line, so we look for the marker anywhere in the
396+
// line and preserve any real output that precedes it.
397+
//
398+
// We only treat the line as completion when the text following the marker parses
399+
// as the expected exit-status integer. A command that reads from stdin (e.g.
400+
// `cat`, `read`, `head -n1`) can consume and echo the injected marker command
401+
// itself before it ever runs; that echoed line contains the marker substring but
402+
// no valid trailing exit code, so it must NOT be treated as completion (otherwise
403+
// the command would be falsely reported as a successful run while still owning the
404+
// session). See #4740.
398405
if ("stdout".equals(outputLine.source)) {
399406
int markerIndex = line.indexOf(marker);
400407
if (markerIndex >= 0) {
401-
String afterMarker = line.substring(markerIndex + marker.length()).trim();
402-
if (!afterMarker.isEmpty()) {
403-
try {
404-
exitCode = Integer.parseInt(afterMarker.split("\\s+")[0]);
405-
} catch (NumberFormatException e) {
406-
// Ignore
408+
Integer parsedExitCode = parseExitCode(line.substring(markerIndex + marker.length()));
409+
if (parsedExitCode != null) {
410+
exitCode = parsedExitCode;
411+
// Keep any real output that was merged onto the marker line.
412+
line = line.substring(0, markerIndex);
413+
completed = true;
414+
if (line.isEmpty()) {
415+
break;
407416
}
408417
}
409-
// Keep any real output that was merged onto the marker line.
410-
line = line.substring(0, markerIndex);
411-
completed = true;
412-
if (line.isEmpty()) {
413-
break;
414-
}
415418
}
416419
}
417420

@@ -449,6 +452,24 @@ private CommandResult collectOutput(String marker, long deadline, int maxOutputL
449452
return new CommandResult(output, exitCode, timedOut, truncatedByLines,
450453
truncatedByBytes, totalLines, totalBytes);
451454
}
455+
456+
/**
457+
* Parse the exit status that the completion marker echoes after it (e.g. the {@code 0}
458+
* in {@code <marker> 0}). Returns {@code null} when the text is empty or its first token
459+
* is not an integer, which signals that this occurrence of the marker is not a genuine
460+
* completion line (for example the marker command echoed back by a stdin-reading command).
461+
*/
462+
private Integer parseExitCode(String afterMarker) {
463+
String trimmed = afterMarker.trim();
464+
if (trimmed.isEmpty()) {
465+
return null;
466+
}
467+
try {
468+
return Integer.parseInt(trimmed.split("\\s+")[0]);
469+
} catch (NumberFormatException e) {
470+
return null;
471+
}
472+
}
452473
}
453474

454475
/**

spring-ai-alibaba-agent-framework/src/test/java/com/alibaba/cloud/ai/graph/agent/tools/ShellSessionManagerTest.java

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
import static org.junit.jupiter.api.Assertions.assertEquals;
3030
import static org.junit.jupiter.api.Assertions.assertFalse;
31+
import static org.junit.jupiter.api.Assertions.assertTrue;
3132

3233
/**
3334
* Tests for {@link ShellSessionManager} command output collection.
@@ -111,4 +112,31 @@ void normalOutputWithTrailingNewlineStillWorks() throws Exception {
111112
}
112113
}
113114

115+
@Test
116+
@Timeout(value = 30, unit = TimeUnit.SECONDS)
117+
void stdinReadingCommandDoesNotFalselyCompleteOnEchoedMarker() throws Exception {
118+
Path workspace = Files.createTempDirectory("shell_session_test_");
119+
// Short timeout: this command can never complete normally, so keep the expected
120+
// timeout/restart fast.
121+
ShellSessionManager manager = ShellSessionManager.builder()
122+
.workspaceRoot(workspace)
123+
.commandTimeout(3000)
124+
.build();
125+
RunnableConfig config = RunnableConfig.builder().threadId("test-thread").build();
126+
manager.initialize(config);
127+
try {
128+
// `cat` with no argument reads from the session's stdin and consumes/echoes the
129+
// injected marker command before it ever runs. The echoed line contains the marker
130+
// substring but no valid trailing exit code, so it must NOT be treated as completion
131+
// — otherwise the command would be falsely reported as a successful (exitCode == null)
132+
// run while still owning the session. It should time out and restart instead.
133+
ShellSessionManager.CommandResult result = manager.executeCommand("cat", config);
134+
135+
assertTrue(result.isTimedOut(), "stdin-consuming command must not falsely complete on echoed marker");
136+
}
137+
finally {
138+
manager.cleanup(config);
139+
}
140+
}
141+
114142
}

0 commit comments

Comments
 (0)