diff --git a/apps/backend/cmd/mock-agent/script.go b/apps/backend/cmd/mock-agent/script.go index 81627bbed0..d5c64b9e8b 100644 --- a/apps/backend/cmd/mock-agent/script.go +++ b/apps/backend/cmd/mock-agent/script.go @@ -222,12 +222,20 @@ func executeSimulatedToolResult(e *emitter, line string) { // the actual output - the ACP shell-output normalizer must strip that // leading echo so the transcript's command header and Output disclosure // don't show the command twice. -// Format: e2e:shell_result("", "") +// Format: e2e:shell_result("", ""[, ""]) // -// rawInputCommandKey/rawOutputKey mirror the "command"/"output" keys a real -// shell provider's tool_call/tool_call_update JSON uses. +// The optional third argument reproduces a provider that resolves a +// relative file-path argument in the command to an absolute (cwd-joined) +// one before actually invoking the real terminal, while the reported +// "command" keeps the original relative form - the normalizer's workDir- +// aware echo stripping must still recognize that echo. +// +// rawInputCommandKey/rawInputCwdKey/rawOutputKey mirror the +// "command"/"cwd"/"output" keys a real shell provider's +// tool_call/tool_call_update JSON uses. const ( rawInputCommandKey = "command" + rawInputCwdKey = "cwd" rawOutputKey = "output" ) @@ -235,10 +243,17 @@ func executeSimulatedShellResult(e *emitter, line string) { inner := extractParenContent(line, "e2e:shell_result(") command, rest := extractFirstStringArg(inner) rest = strings.TrimPrefix(strings.TrimSpace(rest), ",") - stdout, _ := extractFirstStringArg(strings.TrimSpace(rest)) + stdout, rest := extractFirstStringArg(strings.TrimSpace(rest)) + rest = strings.TrimPrefix(strings.TrimSpace(rest), ",") + cwd, _ := extractFirstStringArg(strings.TrimSpace(rest)) + + rawInput := map[string]any{rawInputCommandKey: command} + if cwd != "" { + rawInput[rawInputCwdKey] = cwd + } toolID := nextToolID() - e.startTool(toolID, command, acp.ToolKindExecute, map[string]any{rawInputCommandKey: command}) + e.startTool(toolID, command, acp.ToolKindExecute, rawInput) e.completeTool(toolID, map[string]any{rawOutputKey: stdout}) } diff --git a/apps/backend/cmd/mock-agent/script_test.go b/apps/backend/cmd/mock-agent/script_test.go index da67121b44..b90b19ba35 100644 --- a/apps/backend/cmd/mock-agent/script_test.go +++ b/apps/backend/cmd/mock-agent/script_test.go @@ -504,6 +504,59 @@ func TestExecuteScriptShellResult(t *testing.T) { } } +// TestExecuteScriptShellResultWithCwd verifies the optional third +// e2e:shell_result argument populates rawInput["cwd"], reproducing a +// provider that resolves a relative file-path argument in the command to +// an absolute (cwd-joined) one before actually invoking the real terminal +// (see the ACP shell-output normalizer's workDir-aware echo stripping in +// shell_output.go). +func TestExecuteScriptShellResultWithCwd(t *testing.T) { + e, mock := newTestEmitter() + resetState() + + executeScript(e, "", `e2e:shell_result("cat notes.txt", "$ cat /work/notes.txt\nhello\n", "/work")`) + + updates := mock.getUpdates() + if len(updates) != 2 { + t.Fatalf("expected 2 updates, got %d", len(updates)) + } + + tc := updates[0].notification.Update.ToolCall + input, ok := tc.RawInput.(map[string]any) + if !ok || input["command"] != "cat notes.txt" || input["cwd"] != "/work" { + t.Errorf("raw input = %#v, want command %q and cwd %q", tc.RawInput, "cat notes.txt", "/work") + } + + tcu := updates[1].notification.Update.ToolCallUpdate + output, ok := tcu.RawOutput.(map[string]any) + if !ok || output["output"] != "$ cat /work/notes.txt\nhello\n" { + t.Errorf("raw output = %#v, want the echoed stdout", tcu.RawOutput) + } +} + +// TestExecuteScriptShellResultWithoutCwdOmitsCwdKey confirms the +// two-argument form (no cwd) keeps working exactly as before: rawInput +// carries no "cwd" key at all. +func TestExecuteScriptShellResultWithoutCwdOmitsCwdKey(t *testing.T) { + e, mock := newTestEmitter() + resetState() + + executeScript(e, "", `e2e:shell_result("cat file.txt", "cat file.txt=== marker ===\n")`) + + updates := mock.getUpdates() + if len(updates) != 2 { + t.Fatalf("expected 2 updates, got %d", len(updates)) + } + tc := updates[0].notification.Update.ToolCall + input, ok := tc.RawInput.(map[string]any) + if !ok { + t.Fatalf("raw input = %#v, want a map", tc.RawInput) + } + if _, present := input["cwd"]; present { + t.Errorf("raw input = %#v, want no cwd key", tc.RawInput) + } +} + // TestExecuteScriptToolUseNoInput verifies tool_use with no input arg. func TestExecuteScriptToolUseNoInput(t *testing.T) { e, mock := newTestEmitter() 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 246de5a8f4..fc8c9fea89 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 @@ -28,7 +28,7 @@ func applyFinalShellResult(payload *streams.ShellExecPayload, result any) { } output := ensureShellOutput(payload) if normalized.hasStdout { - output.Stdout = stripLeadingCommandEcho(payload.Command, normalized.stdout) + output.Stdout = stripLeadingCommandEcho(payload.Command, payload.WorkDir, normalized.stdout) output.StdoutTruncated = normalized.stdoutTruncated } if normalized.hasStderr { @@ -56,8 +56,8 @@ func applyFinalShellResult(payload *streams.ShellExecPayload, result any) { // Used for final/terminal results, where the buffer is known to be complete: // a stdout consisting of nothing but the echoed command commits to an empty // result rather than leaving the redundant echo visible. -func stripLeadingCommandEcho(command, stdout string) string { - return stripCommandEcho(command, stdout, true) +func stripLeadingCommandEcho(command, workDir, stdout string) string { + return stripCommandEcho(command, workDir, stdout, true) } // stripPendingCommandEcho is the live-update counterpart of @@ -67,18 +67,54 @@ func stripLeadingCommandEcho(command, stdout string) string { // currently equals nothing but the echo is left untouched - the separator or // real output may still be in flight - instead of collapsing to empty and // orphaning a later chunk's leading newline. -func stripPendingCommandEcho(command, stdout string) string { - return stripCommandEcho(command, stdout, false) +func stripPendingCommandEcho(command, workDir, stdout string) string { + return stripCommandEcho(command, workDir, stdout, false) } -func stripCommandEcho(command, stdout string, commitExactMatch bool) string { +func stripCommandEcho(command, workDir, stdout string, commitExactMatch bool) string { if command == "" || stdout == "" { return stdout } - rest, ok := strings.CutPrefix(stdout, "$ "+command) - if !ok { + if rest, ok := strings.CutPrefix(stdout, "$ "+command); ok { + return finishCommandEchoStrip(rest, stdout, commitExactMatch) + } + return stripCommandEchoWithWorkDir(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 line then no +// longer matches literally. Retry against just the first echoed line 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 } + // 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. + firstLine, remainder, hasMore := cutFirstLine(stdout) + workDirPrefix := strings.TrimRight(workDir, "/") + "/" + if strings.ReplaceAll(firstLine, workDirPrefix, "") != "$ "+command { + return stdout + } + if !hasMore { + if !commitExactMatch { + return stdout + } + return "" + } + return remainder +} + +func finishCommandEchoStrip(rest, stdout string, commitExactMatch bool) string { if rest == "" && !commitExactMatch { return stdout } @@ -88,6 +124,16 @@ func stripCommandEcho(command, stdout string, commitExactMatch bool) string { return strings.TrimPrefix(rest, "\n") } +// cutFirstLine splits s at its first newline, reporting whether one was +// found. The returned line excludes both the newline and any preceding "\r". +func cutFirstLine(s string) (line, remainder string, hasMore bool) { + idx := strings.IndexByte(s, '\n') + if idx < 0 { + return s, "", false + } + return strings.TrimSuffix(s[:idx], "\r"), s[idx+1:], true +} + // NormalizeShellToolUpdate merges live and final ACP shell result fields into // the active normalized payload. It reports whether the update contained a // recognized shell output field so statusless updates can be persisted. @@ -153,9 +199,9 @@ func (n *Normalizer) NormalizeShellToolUpdate( // A definitive completion signal arrived, and Stdout hasn't // already been committed-stripped this call: commit now, even // if the buffer is nothing but the echo. - shell.Output.Stdout = stripLeadingCommandEcho(shell.Command, shell.Output.Stdout) + shell.Output.Stdout = stripLeadingCommandEcho(shell.Command, shell.WorkDir, shell.Output.Stdout) case !isFinal: - shell.Output.Stdout = stripPendingCommandEcho(shell.Command, shell.Output.Stdout) + shell.Output.Stdout = stripPendingCommandEcho(shell.Command, shell.WorkDir, shell.Output.Stdout) } } 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 d2fca96a0b..58c3a66ef2 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 @@ -365,6 +365,192 @@ func TestNormalizeShellToolResultStripsLeadingCommandEcho(t *testing.T) { } } +// TestNormalizeShellToolResultStripsLeadingCommandEchoWithWorkDirResolvedPath +// is a regression for a real-world report: PR #1898's fix only strips the +// echo when the captured terminal line matches the tool call's reported +// "command" byte-for-byte. Some providers resolve a relative file-path +// argument to an absolute one (workDir-joined) before actually invoking the +// real terminal, while the "command" field reported on the tool call - the +// same text the chat header renders - keeps the original relative form. +// The literal prefix match then never fires, and the full echoed line +// (with the resolved absolute path) leaks into the persisted Output. +func TestNormalizeShellToolResultStripsLeadingCommandEchoWithWorkDirResolvedPath(t *testing.T) { + t.Parallel() + + command := `grep -n "^const STRATEGY\|STRATEGY =" apps/web/components/task/chat/message-list.tsx` + workDir := "/home/clem/.kandev/tasks/new-message-line_0vc/kdlbs-kandev" + absolutePath := workDir + "/apps/web/components/task/chat/message-list.tsx" + resolvedCommand := strings.Replace(command, "apps/web/components/task/chat/message-list.tsx", absolutePath, 1) + + normalizer := NewNormalizer("") + payload := normalizer.NormalizeToolCall("execute", map[string]any{ + "kind": "execute", + "raw_input": map[string]any{"command": command, "cwd": workDir}, + }) + + normalizer.NormalizeToolResult(payload, "$ "+resolvedCommand+"\n24:const STRATEGY = \"native\";\n") + + require.Equal(t, "24:const STRATEGY = \"native\";\n", payload.ShellExec().Output.Stdout) +} + +// TestNormalizeShellToolUpdateStripsLeadingCommandEchoWithWorkDirResolvedPath +// covers the same workDir-resolved-path mismatch on the live-update path +// (terminal_output plus a terminal_exit completion, no rawOutput). +func TestNormalizeShellToolUpdateStripsLeadingCommandEchoWithWorkDirResolvedPath(t *testing.T) { + t.Parallel() + + command := "cat notes.txt" + workDir := "/repo" + resolvedCommand := "cat /repo/notes.txt" + + normalizer := NewNormalizer("") + payload := normalizer.NormalizeToolCall("execute", map[string]any{ + "kind": "execute", + "raw_input": map[string]any{"command": command, "cwd": workDir}, + }) + + normalizer.NormalizeShellToolUpdate( + payload, + map[string]any{ + "terminal_output": map[string]any{"data": "$ " + resolvedCommand + "\nhello\n"}, + "terminal_exit": map[string]any{"exit_code": float64(0)}, + }, + nil, + nil, + ) + + require.Equal(t, "hello\n", payload.ShellExec().Output.Stdout) +} + +// TestNormalizeShellToolUpdateWorkDirFallbackDefersPendingEchoWithoutNewline +// covers the pending (non-final) counterpart of the workDir fallback: a +// buffer that is exactly the resolved echo with no trailing newline yet +// must be left untouched, since the real output (or even just the +// separator newline) may still be in flight. +func TestNormalizeShellToolUpdateWorkDirFallbackDefersPendingEchoWithoutNewline(t *testing.T) { + t.Parallel() + + normalizer := NewNormalizer("") + payload := normalizer.NormalizeToolCall("execute", map[string]any{ + "kind": "execute", + "raw_input": map[string]any{"command": "cat notes.txt", "cwd": "/repo"}, + }) + + normalizer.NormalizeShellToolUpdate( + payload, + map[string]any{"terminal_output": map[string]any{"data": "$ cat /repo/notes.txt"}}, + nil, + nil, + ) + + require.Equal(t, "$ cat /repo/notes.txt", payload.ShellExec().Output.Stdout) +} + +// TestNormalizeShellToolUpdateWorkDirFallbackCommitsPendingEchoWithNewline +// covers the fallback's other pending-path branch: once a trailing newline +// confirms the echo line is complete (hasMore == true), the strip commits +// immediately even though commitExactMatch is false - the same behavior +// finishCommandEchoStrip already applies once its literal-match "rest" +// carries a newline. +func TestNormalizeShellToolUpdateWorkDirFallbackCommitsPendingEchoWithNewline(t *testing.T) { + t.Parallel() + + normalizer := NewNormalizer("") + payload := normalizer.NormalizeToolCall("execute", map[string]any{ + "kind": "execute", + "raw_input": map[string]any{"command": "cat notes.txt", "cwd": "/repo"}, + }) + + normalizer.NormalizeShellToolUpdate( + payload, + map[string]any{"terminal_output": map[string]any{"data": "$ cat /repo/notes.txt\n"}}, + nil, + nil, + ) + + require.Equal(t, "", payload.ShellExec().Output.Stdout) +} + +// TestNormalizeShellToolResultWorkDirFallbackNeverCorruptsNearMissOutput is a +// safety regression for the workDir-collapse fallback added alongside the +// above test: ReplaceAll(firstLine, workDirPrefix, "") strips every +// occurrence of that prefix from the echoed line, including one embedded in +// a quoted argument that isn't a resolved file path (e.g. a grep pattern +// that happens to contain workDir-looking text). If the collapsed line +// still doesn't reconstruct "$ "+command exactly - because the real +// terminal echo genuinely differs (different args, or the workDir text was +// incidental rather than a resolved path) - the fallback must leave stdout +// untouched rather than mangling legitimate output. +func TestNormalizeShellToolResultWorkDirFallbackNeverCorruptsNearMissOutput(t *testing.T) { + t.Parallel() + + command := `grep -n "/repo/notes" apps/foo.txt` + workDir := "/repo" + // The real terminal echo resolves the file argument to an absolute path + // (workDir-joined) AND happens to also carry a coincidental "/repo/" + // substring inside the quoted pattern - unrelated to path resolution. + // Collapsing every "/repo/" occurrence therefore also eats the pattern's + // own "/repo/" text, so the result can never exactly reconstruct + // "$ "+command: the fallback must decline to strip rather than guess. + stdout := `$ grep -n "/repo/notes" /repo/apps/foo.txt` + "\n1:a match\n" + + normalizer := NewNormalizer("") + payload := normalizer.NormalizeToolCall("execute", map[string]any{ + "kind": "execute", + "raw_input": map[string]any{"command": command, "cwd": workDir}, + }) + + normalizer.NormalizeToolResult(payload, stdout) + + require.Equal(t, stdout, payload.ShellExec().Output.Stdout) +} + +// TestNormalizeShellToolResultStripsLeadingCommandEchoWithTrailingSlashWorkDir +// is a regression for a Codex review finding on this PR: a provider can +// report "cwd" already "/"-terminated (e.g. "/repo/"). Appending "/" +// unconditionally would then search for "/repo//" - a doubled separator +// the actually-joined path ("/repo/notes.txt") never contains - silently +// declining to strip a real echo. +func TestNormalizeShellToolResultStripsLeadingCommandEchoWithTrailingSlashWorkDir(t *testing.T) { + t.Parallel() + + command := "cat notes.txt" + workDir := "/repo/" + resolvedCommand := "cat /repo/notes.txt" + + normalizer := NewNormalizer("") + payload := normalizer.NormalizeToolCall("execute", map[string]any{ + "kind": "execute", + "raw_input": map[string]any{"command": command, "cwd": workDir}, + }) + + normalizer.NormalizeToolResult(payload, "$ "+resolvedCommand+"\nhello\n") + + require.Equal(t, "hello\n", payload.ShellExec().Output.Stdout) +} + +// TestNormalizeShellToolResultStripsLeadingCommandEchoWithRootWorkDir covers +// the same Codex finding's other reported case: the root directory "/" is +// itself already "/"-terminated, so the joined path is single- not +// double-slashed ("/notes.txt", not "//notes.txt"). +func TestNormalizeShellToolResultStripsLeadingCommandEchoWithRootWorkDir(t *testing.T) { + t.Parallel() + + command := "cat notes.txt" + workDir := "/" + resolvedCommand := "cat /notes.txt" + + normalizer := NewNormalizer("") + payload := normalizer.NormalizeToolCall("execute", map[string]any{ + "kind": "execute", + "raw_input": map[string]any{"command": command, "cwd": workDir}, + }) + + normalizer.NormalizeToolResult(payload, "$ "+resolvedCommand+"\nhello\n") + + require.Equal(t, "hello\n", 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 871dc4c9c0..bb01f90afd 100644 --- a/apps/web/e2e/tests/chat/shell-output-echo.spec.ts +++ b/apps/web/e2e/tests/chat/shell-output-echo.spec.ts @@ -58,4 +58,63 @@ test.describe("shell command output echo stripping", () => { // disclosure - it's already shown once in the command row above. await expect(outputRegion).not.toContainText(command); }); + + test("does not repeat a workDir-resolved absolute-path echo inside the expanded Output disclosure", async ({ + testPage, + apiClient, + seedData, + }) => { + // Some providers resolve a relative file-path argument in the command + // to an absolute (cwd-joined) one before actually invoking the real + // terminal, while the reported "command" - the same text the chat + // header renders - keeps the original relative form. The captured echo + // line then no longer matches the command byte-for-byte. + const command = "cat notes.txt"; + const cwd = "/workspace/example-repo"; + const resolvedCommand = `cat ${cwd}/notes.txt`; + const echoedOutput = `$ ${resolvedCommand}\nhello\n`; + + const script = [ + `e2e:shell_result("${command}", "${echoedOutput.replace(/\n/g, "\\n")}", "${cwd}")`, + 'e2e:message("done")', + ].join("\n"); + + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + "Shell echo workDir 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: command }); + await expect(commandRow).toBeVisible(); + // The header keeps rendering the original relative command, unaffected + // by the provider's internal absolute-path resolution. + 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("hello"); + // Critical: neither the workDir-prefixed absolute path nor a repeat of + // the resolved command may leak into the Output disclosure. + await expect(outputRegion).not.toContainText(cwd); + await expect(outputRegion).not.toContainText(resolvedCommand); + }); });