fix(agent): Fixed ShellSessionManager's indefinite waiting for a marker to appear.#4395
Open
codezkk wants to merge 1 commit intoalibaba:mainfrom
Open
fix(agent): Fixed ShellSessionManager's indefinite waiting for a marker to appear.#4395codezkk wants to merge 1 commit intoalibaba:mainfrom
codezkk wants to merge 1 commit intoalibaba:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes ShellSessionManager’s command-completion detection so it no longer waits indefinitely when the done-marker is appended to the end of a line (because command output didn’t end with a newline).
Changes:
- Update marker detection from
startsWith(marker)tocontains(marker)and extract pre-marker output + exit code from the same line. - Add a new
ShellSessionManagerTestsuite covering builder defaults, initialization, redaction, truncation, startup commands, restart, and uninitialized execution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
spring-ai-alibaba-agent-framework/src/main/java/com/alibaba/cloud/ai/graph/agent/tools/ShellSessionManager.java |
Adjusts output collection logic to detect the completion marker even when it appears mid-line and to preserve output preceding it. |
spring-ai-alibaba-agent-framework/src/test/java/com/alibaba/cloud/ai/graph/agent/tools/ShellSessionManagerTest.java |
Introduces tests for session lifecycle and command execution behavior (currently includes Windows-shell syntax issues and lacks a direct regression test for the no-newline marker case). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+357
to
+361
| private CommandResult collectOutput(String marker, long deadline, int maxOutputLines, Long maxOutputBytes) { | ||
| List<String> lines = new ArrayList<>(); | ||
| int totalLines = 0; | ||
| long totalBytes = 0; | ||
| boolean truncatedByLines = false; |
| String preMarker = line.substring(0, markerIndex); | ||
| if (!preMarker.isEmpty()) { | ||
| totalLines++; | ||
| totalBytes += preMarker.getBytes().length + 1; // +1 for newline |
| try { | ||
| String cmd; | ||
| if (isWindows()) { | ||
| cmd = "echo line1 & echo line2 & echo line3"; |
Comment on lines
+183
to
+185
| ShellSessionManager manager = ShellSessionManager.builder() | ||
| .addStartupCommand(isWindows() ? "set TEST_VAR=startup" : "export TEST_VAR=startup") | ||
| .build(); |
Comment on lines
+190
to
+192
| // In persistent shell, environment variables should persist | ||
| String cmd = isWindows() ? "echo %TEST_VAR%" : "echo $TEST_VAR"; | ||
| ShellSessionManager.CommandResult result = manager.executeCommand(cmd, config); |
| @Test | ||
| void testFailedStartupCommand() { | ||
| ShellSessionManager manager = ShellSessionManager.builder() | ||
| .addStartupCommand("nonexistentcommand_should_fail") |
Comment on lines
+220
to
+232
| Object firstSession = config.context().get("_SHELL_SESSION_"); | ||
| manager.restartSession(config); | ||
| Object secondSession = config.context().get("_SHELL_SESSION_"); | ||
|
|
||
| // Session object should be different if restarted (actually ShellSession is private, but let's see) | ||
| // Wait, restart() in ShellSession is internal. ShellSessionManager.restartSession calls session.restart() | ||
| // Looking at code: session.restart() stops and starts the SAME session object's process. | ||
| // So firstSession == secondSession. | ||
| assertSame(firstSession, secondSession); | ||
|
|
||
| String cmd = isWindows() ? "echo %TEST_VAR%" : "echo $TEST_VAR"; | ||
| ShellSessionManager.CommandResult result = manager.executeCommand(cmd, config); | ||
| assertTrue(result.getOutput().contains("startup")); |
Comment on lines
+230
to
+231
| String cmd = isWindows() ? "echo %TEST_VAR%" : "echo $TEST_VAR"; | ||
| ShellSessionManager.CommandResult result = manager.executeCommand(cmd, config); |
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe what this PR does / why we need it
ShellSessionManager uses a marker command (such as
printf '__LC_SHELL_DONE__ %s\n' $?) to detect when a command has finished executing and extracts the exit code. If the output of a command (such ascurl -s,printf, or some scripts) does not end with a newline character, the shell marker is appended directly to the same line, for example:{"status":"ok"}LC_SHELL_DONE... 0
Because the original code uses
line.startsWith(marker)(to check if the line starts with a marker),it cannot find the marker (because the marker is in the middle of the line, not at the beginning). The tool will wait indefinitely for the marker to appear until a timeout occurs, causing the program to hang and restart the session.
Does this pull request fix one issue?
Describe how you did it
ShellSessionManager.java has been updated:
Replaced line.startsWith(marker) with line.contains(marker), ensuring correct recognition even when the marker follows command output.
Correctly splits lines, extracts command output preceding the marker, and extracts exit codes.
Describe how to verify it
see ShellSessionManagerTest
Special notes for reviews