dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

fix(acp): strip CRLF multi-line command echoes with no reported cwd (#2334)

Browse Source
This commit is contained in:
Clément Fiere
2026-08-07 10:57:26 +02:00
committed by GitHub
parent 078ca80562
commit 0c2dcf2e78
3 changed files with 211 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output.go
+35 -25
View File
@@ -78,35 +78,45 @@ func stripCommandEcho(command, workDir, stdout string, commitExactMatch bool) st
if rest, ok := strings.CutPrefix(stdout, "$ "+command); ok {
return finishCommandEchoStrip(rest, stdout, commitExactMatch)
}
return stripCommandEchoWithWorkDir(command, workDir, stdout, commitExactMatch)
return stripCommandEchoMultiline(command, workDir, stdout, commitExactMatch)
}
// stripCommandEchoWithWorkDir is stripCommandEcho's fallback for providers
// that resolve a relative file-path argument in the command to an absolute
// (workDir-joined) one before actually invoking the real terminal, while the
// tool call's reported "command" field - the same text the chat header
// renders - keeps the original relative form. The captured echo then no
// longer matches literally. Retry against exactly as many leading echoed
// lines as the command itself spans - a command can carry an embedded
// newline (e.g. a multi-line script or commit message passed as a single
// tool-call argument), and a real terminal echoes that verbatim across
// multiple lines, not just the first - with the workDir prefix collapsed
// back out, so real output later in stdout (which may legitimately contain
// workDir-prefixed paths of its own) is never touched.
func stripCommandEchoWithWorkDir(command, workDir, stdout string, commitExactMatch bool) string {
if workDir == "" {
return stdout
}
// stripCommandEchoMultiline is stripCommandEcho's fallback for echoed lines
// that don't match the command byte-for-byte on the first try. It compares
// exactly as many leading echoed lines as the command itself spans - a
// command can carry an embedded newline (e.g. a multi-line script or commit
// message passed as a single tool-call argument), and a real terminal
// echoes that verbatim across multiple lines, not just the first - after
// two independent normalizations that a real terminal's echo can apply
// without the tool call's reported "command" field (the same text the chat
// header renders) changing to match:
//
// 1. CRLF line endings: cutLines collapses "\r\n" back to "\n" in the
// echoed chunk. A command's embedded newlines are always plain "\n" (the
// literal bytes of the reported argument), but a real terminal echoes
// canonical-mode input with "\r\n" - so a multi-line command with no
// workDir at all (the common case) would otherwise never match past its
// first embedded newline, leaking the entire echo verbatim.
// 2. A resolved relative-to-absolute path: some providers resolve a
// relative file-path argument to a workDir-joined absolute one before
// actually invoking the real terminal. Only applied when workDir is
// known, and scoped to the compared chunk, so real output later in
// stdout (which may legitimately contain workDir-prefixed paths of its
// own) is never touched.
func stripCommandEchoMultiline(command, workDir, stdout string, commitExactMatch bool) string {
echoLines := strings.Count(command, "\n") + 1
chunk, remainder, hasMore := cutLines(stdout, echoLines)
// workDir's trailing slashes are collapsed to exactly one first: a
// provider can report cwd already "/"-terminated (e.g. "/repo/"), and
// the root directory "/" is inherently "/"-terminated. Appending "/"
// unconditionally would search for a doubled separator ("/repo//" or
// "//") that the actually-joined path never contains, silently
// declining to strip a real echo.
workDirPrefix := strings.TrimRight(workDir, "/") + "/"
if strings.ReplaceAll(chunk, workDirPrefix, "") != "$ "+command {
if workDir != "" {
// workDir's trailing slashes are collapsed to exactly one first: a
// provider can report cwd already "/"-terminated (e.g. "/repo/"), and
// the root directory "/" is inherently "/"-terminated. Appending "/"
// unconditionally would search for a doubled separator ("/repo//" or
// "//") that the actually-joined path never contains, silently
// declining to strip a real echo.
workDirPrefix := strings.TrimRight(workDir, "/") + "/"
chunk = strings.ReplaceAll(chunk, workDirPrefix, "")
}
if chunk != "$ "+command {
return stdout
}
if !hasMore {
apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output_test.go
+116
View File
@@ -578,6 +578,122 @@ func TestNormalizeShellToolResultStripsLeadingCommandEchoWithWorkDirResolvedPath
require.Equal(t, "start\nhello\n", payload.ShellExec().Output.Stdout)
}
// TestNormalizeShellToolResultStripsLeadingCommandEchoWithCRLFMultilineCommandNoWorkDir
// is a regression for a live report: a multi-line command with no reported
// cwd (the common case - most commands don't need an explicit cwd), run
// through a real terminal that echoes canonical-mode input with "\r\n" line
// endings while the command's own embedded newlines - the text the tool
// call actually reports - stay plain "\n". stripCommandEcho's exact-match
// prefix check then fails at the very first embedded newline, and until now
// stripCommandEchoWithWorkDir bailed out immediately whenever workDir was
// empty, without ever reaching the CRLF-normalizing cutLines comparison it
// already applies for the workDir-resolved-path case - so the entire
// multi-line echo leaked into the persisted Output verbatim, directly
// butted against the real output with no separator.
func TestNormalizeShellToolResultStripsLeadingCommandEchoWithCRLFMultilineCommandNoWorkDir(t *testing.T) {
t.Parallel()
command := "for i in 1 2; do\n echo $i\ndone"
stdout := "$ for i in 1 2; do\r\n echo $i\r\ndone\r\n1\n2\n"
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": command},
})
normalizer.NormalizeToolResult(payload, stdout)
require.Equal(t, "1\n2\n", payload.ShellExec().Output.Stdout)
}
// TestNormalizeShellToolResultCRLFMultilineFallbackNeverCorruptsNearMissOutput
// is a safety regression for the above: the CRLF-normalizing comparison must
// still decline to strip when the echoed lines genuinely diverge from the
// reported command (e.g. a stale echo, or a provider that actually ran
// something else), rather than guessing.
func TestNormalizeShellToolResultCRLFMultilineFallbackNeverCorruptsNearMissOutput(t *testing.T) {
t.Parallel()
command := "for i in 1 2; do\n echo $i\ndone"
stdout := "$ for i in 1 2; do\r\n echo $i\r\ndone; echo extra\r\n1\n2\n"
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": command},
})
normalizer.NormalizeToolResult(payload, stdout)
require.Equal(t, stdout, payload.ShellExec().Output.Stdout)
}
// TestNormalizeShellToolUpdateStripsLeadingCommandEchoWithCRLFMultilineCommandNoWorkDir
// covers the same CRLF-normalizing multiline fallback as
// TestNormalizeShellToolResultStripsLeadingCommandEchoWithCRLFMultilineCommandNoWorkDir,
// but on the live-update path (terminal_output plus a terminal_exit
// completion, no rawOutput) rather than the final/committed one - a review
// finding that NormalizeShellToolUpdate exercises the same
// stripCommandEchoMultiline fix but had no test pinning it directly.
func TestNormalizeShellToolUpdateStripsLeadingCommandEchoWithCRLFMultilineCommandNoWorkDir(t *testing.T) {
t.Parallel()
command := "for i in 1 2; do\n echo $i\ndone"
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": command},
})
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{
"terminal_output": map[string]any{"data": "$ for i in 1 2; do\r\n echo $i\r\ndone\r\n1\n2\n"},
"terminal_exit": map[string]any{"exit_code": float64(0)},
},
nil,
nil,
)
require.Equal(t, "1\n2\n", payload.ShellExec().Output.Stdout)
}
// TestNormalizeShellToolUpdateCRLFMultilineNoWorkDirDefersPendingEchoWithoutNewline
// is the pending (non-final, deferred) counterpart of
// TestNormalizeShellToolUpdateStripsLeadingCommandEchoWithCRLFMultilineCommandNoWorkDir,
// added per review: stripCommandEchoMultiline's own deferred branch
// ("!hasMore" with commitExactMatch still false) was only exercised through
// the workDir-resolved-path variant
// (TestNormalizeShellToolUpdateWorkDirFallbackDefersPendingEchoWithoutNewline),
// never through the CRLF-normalizing no-cwd fallback this file added. A
// buffer that echoes exactly the command's own embedded newlines - with no
// cwd, and with no trailing newline yet confirming the echo's last line is
// complete - must be left untouched, since the real output (or even just
// the separator newline) may still be in flight.
func TestNormalizeShellToolUpdateCRLFMultilineNoWorkDirDefersPendingEchoWithoutNewline(t *testing.T) {
t.Parallel()
command := "for i in 1 2; do\n echo $i\ndone"
echo := "$ " + command
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": command},
})
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output": map[string]any{"data": echo}},
nil,
nil,
)
require.Equal(t, echo, payload.ShellExec().Output.Stdout)
}
func TestNormalizeShellToolUpdateStripsLeadingCommandEchoFromLiveOutput(t *testing.T) {
t.Parallel()
apps/web/e2e/tests/chat/shell-output-echo.spec.ts
+60
View File
@@ -182,4 +182,64 @@ test.describe("shell command output echo stripping", () => {
fullPage: true,
});
});
test("does not repeat a CRLF multi-line echo with no cwd inside the expanded Output disclosure", async ({
testPage,
apiClient,
seedData,
}) => {
// Regression for a live report: a multi-line command with no reported
// cwd (the common case - most commands don't need one), echoed by a
// real terminal using canonical-mode "\r\n" line endings, while the
// command's own embedded newlines - the literal bytes of the reported
// "command" field - stay plain "\n". The workDir-resolved-path fix
// above only reached its CRLF-normalizing comparison when a cwd was
// present, so a multi-line command with no cwd at all still leaked its
// entire echo verbatim into the Output disclosure.
const command = "echo start\necho hello";
const echoedOutput = "$ echo start\r\necho hello\r\nstart\nhello\n";
const script = [
`e2e:shell_result("${command.replace(/\n/g, "\\n")}", "${echoedOutput.replace(/\n/g, "\\n")}")`,
'e2e:message("done")',
].join("\n");
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Shell echo CRLF multiline regression",
seedData.agentProfileId,
{
description: script,
workflow_id: seedData.workflowId,
workflow_step_id: seedData.startStepId,
repository_ids: [seedData.repositoryId],
},
);
await testPage.goto(`/t/${task.id}`);
const session = new SessionPage(testPage);
await session.waitForLoad();
await session.waitForChatIdle({ timeout: 30_000 });
const chat = session.activeChat();
const commandRow = chat.getByTestId("tool-execute-command").filter({ hasText: "echo start" });
await expect(commandRow).toBeVisible();
await expect(commandRow).toHaveText(command);
const disclosure = chat.getByRole("button", { name: "Show command output" });
const responsePromise = testPage.waitForResponse(
(response) => response.url().endsWith("/shell-output") && response.status() === 200,
);
await disclosure.click();
await responsePromise;
const outputRegion = chat.getByTestId("tool-execute-output");
await expect(outputRegion).toContainText("start");
await expect(outputRegion).toContainText("hello");
// Critical: the CRLF-echoed command lines must not also appear inside
// the Output disclosure - they're already shown once in the command
// row above.
await expect(outputRegion).not.toContainText("echo start");
await expect(outputRegion).not.toContainText("$ echo");
});
});