diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output.go index a8f08cc080..44f64677dc 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output.go @@ -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 { diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output_test.go index d525f7e9db..720cc74f67 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output_test.go @@ -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() diff --git a/apps/web/e2e/tests/chat/shell-output-echo.spec.ts b/apps/web/e2e/tests/chat/shell-output-echo.spec.ts index 5084b1c6f0..fb225ca853 100644 --- a/apps/web/e2e/tests/chat/shell-output-echo.spec.ts +++ b/apps/web/e2e/tests/chat/shell-output-echo.spec.ts @@ -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"); + }); });