fix: strip echoed command from persisted shell output (#1898)

This commit is contained in:
Clément Fiere
2026-07-24 10:48:00 +02:00
committed by GitHub
parent 29c86b7fa1
commit 93c6987bf5
6 changed files with 482 additions and 4 deletions
+29
View File
@@ -64,6 +64,9 @@ func executeCommand(e *emitter, fullPrompt, line string) {
case strings.HasPrefix(line, "e2e:monitor_end("):
executeMonitorEnd(e, line)
case strings.HasPrefix(line, "e2e:shell_result("):
executeSimulatedShellResult(e, line)
}
}
@@ -213,6 +216,32 @@ func executeSimulatedToolResult(e *emitter, line string) {
e.completeTool(acp.ToolCallId(state.lastToolID), map[string]any{"result": content})
}
// executeSimulatedShellResult emits an execute-kind (shell) tool call for
// the given command and completes it with the given raw stdout text. Used to
// reproduce a real provider whose captured stdout echoes the command before
// 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("<command>", "<stdout>")
//
// rawInputCommandKey/rawOutputKey mirror the "command"/"output" keys a real
// shell provider's tool_call/tool_call_update JSON uses.
const (
rawInputCommandKey = "command"
rawOutputKey = "output"
)
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))
toolID := nextToolID()
e.startTool(toolID, command, acp.ToolKindExecute, map[string]any{rawInputCommandKey: command})
e.completeTool(toolID, map[string]any{rawOutputKey: stdout})
}
// --- Argument parsing helpers ---
// extractStringArg extracts a quoted string argument from a command like prefix"text")
@@ -466,6 +466,44 @@ func TestExecuteScriptToolUseAndResult(t *testing.T) {
}
}
// TestExecuteScriptShellResult verifies e2e:shell_result emits an
// execute-kind tool call whose completion carries the given raw stdout,
// exercising the same wire shape a real provider whose captured stdout
// echoes the command would produce (see the ACP shell-output normalizer's
// leading-echo stripping in shell_output.go).
func TestExecuteScriptShellResult(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))
}
if !isToolCallUpdate(updates[0]) {
t.Fatal("expected tool_call update")
}
tc := updates[0].notification.Update.ToolCall
if tc.Kind != acp.ToolKindExecute {
t.Errorf("kind = %q, want %q", tc.Kind, acp.ToolKindExecute)
}
input, ok := tc.RawInput.(map[string]any)
if !ok || input["command"] != "cat file.txt" {
t.Errorf("raw input = %#v, want command %q", tc.RawInput, "cat file.txt")
}
if !isToolCallCompleteUpdate(updates[1]) {
t.Fatal("expected tool_call_update")
}
tcu := updates[1].notification.Update.ToolCallUpdate
output, ok := tcu.RawOutput.(map[string]any)
if !ok || output["output"] != "cat file.txt=== marker ===\n" {
t.Errorf("raw output = %#v, want the echoed stdout", tcu.RawOutput)
}
}
// TestExecuteScriptToolUseNoInput verifies tool_use with no input arg.
func TestExecuteScriptToolUseNoInput(t *testing.T) {
e, mock := newTestEmitter()
@@ -28,7 +28,7 @@ func applyFinalShellResult(payload *streams.ShellExecPayload, result any) {
}
output := ensureShellOutput(payload)
if normalized.hasStdout {
output.Stdout = normalized.stdout
output.Stdout = stripLeadingCommandEcho(payload.Command, normalized.stdout)
output.StdoutTruncated = normalized.stdoutTruncated
}
if normalized.hasStderr {
@@ -43,6 +43,51 @@ func applyFinalShellResult(payload *streams.ShellExecPayload, result any) {
}
}
// stripLeadingCommandEcho removes a leading terminal-prompt echo of the tool
// call's command from captured shell output. Some providers capture command
// output via a real terminal session whose input echo - rendered behind a
// "$ " shell prompt - is concatenated directly onto the actual output,
// duplicating the command the UI already renders above the output
// disclosure. Matching requires the literal "$ " prompt marker: a bare
// command-text prefix with no prompt is not stripped, because legitimate
// output can coincidentally start with text identical to the command (e.g.
// a file whose contents happen to begin with the command string).
//
// 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)
}
// stripPendingCommandEcho is the live-update counterpart of
// stripLeadingCommandEcho. Incremental terminal_output/terminal_output_delta
// chunks can split exactly between the echoed command and the separator that
// precedes real output (read() boundaries are arbitrary), so a buffer that
// 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 stripCommandEcho(command, stdout string, commitExactMatch bool) string {
if command == "" || stdout == "" {
return stdout
}
rest, ok := strings.CutPrefix(stdout, "$ "+command)
if !ok {
return stdout
}
if rest == "" && !commitExactMatch {
return stdout
}
if trimmed, ok := strings.CutPrefix(rest, "\r\n"); ok {
return trimmed
}
return strings.TrimPrefix(rest, "\n")
}
// 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.
@@ -57,6 +102,22 @@ func (n *Normalizer) NormalizeShellToolUpdate(
}
shell := payload.ShellExec()
recognized := false
// isFinal tracks whether THIS update carries a definitive completion
// signal - either a raw result payload or a reported exit code (the ACP
// terminal extension can finalize a command with only the latter, no
// rawOutput at all). Either signal means the strip must commit even if
// the buffer is nothing but the echo, instead of deferring forever with
// no further update ever arriving to re-trigger the strict path.
isFinal := rawOutput != nil
// finalStdoutCommitted tracks whether Stdout already went through a
// commit (strict) strip pass earlier in this same call, via
// applyFinalShellResult. Re-stripping an already-normalized value below
// would eat a second, legitimate occurrence of the command text (e.g.
// real output that happens to repeat "$ cmd"). A later terminal_output
// field that clobbers Stdout with the provider's raw, unstripped
// snapshot (the rawOutput-clobber path) clears the flag, since that raw
// value still needs exactly one strip.
finalStdoutCommitted := false
if data, ok := terminalOutputData(meta, "terminal_output_delta"); ok {
appendShellStdout(shell, data)
@@ -69,10 +130,12 @@ func (n *Normalizer) NormalizeShellToolUpdate(
if rawOutput != nil {
applyFinalShellResult(shell, rawOutput)
recognized = true
finalStdoutCommitted = true
}
if data, ok := terminalOutputData(meta, "terminal_output"); ok {
if rawOutput != nil {
replaceShellStdout(shell, data)
finalStdoutCommitted = false
} else {
replaceOrAppendTerminalOutput(shell, data)
}
@@ -81,6 +144,19 @@ func (n *Normalizer) NormalizeShellToolUpdate(
if exitCode, ok := terminalExitCode(meta); ok {
ensureShellOutput(shell).ExitCode = intPtr(exitCode)
recognized = true
isFinal = true
}
if shell.Output != nil {
switch {
case isFinal && !finalStdoutCommitted:
// 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)
case !isFinal:
shell.Output.Stdout = stripPendingCommandEcho(shell.Command, shell.Output.Stdout)
}
}
return recognized
@@ -103,6 +179,8 @@ func syncShellTruncation(output *streams.ShellExecOutput) {
func appendShellStdout(payload *streams.ShellExecPayload, data string) {
output := ensureShellOutput(payload)
rawBounded, _ := boundShellOutput(output.RawTerminalOutput + data)
output.RawTerminalOutput = rawBounded
bounded, truncated := boundShellOutput(output.Stdout + data)
output.Stdout = bounded
output.StdoutTruncated = output.StdoutTruncated || truncated
@@ -112,19 +190,29 @@ func appendShellStdout(payload *streams.ShellExecPayload, data string) {
func replaceShellStdout(payload *streams.ShellExecPayload, data string) {
output := ensureShellOutput(payload)
bounded, truncated := boundShellOutput(data)
output.RawTerminalOutput = bounded
output.Stdout = bounded
output.StdoutTruncated = truncated
syncShellTruncation(output)
}
// replaceOrAppendTerminalOutput merges a new cumulative terminal_output
// snapshot into the stored output. The prefix comparison uses
// RawTerminalOutput - a raw parallel of Stdout that mirrors the same bounded
// append/replace bookkeeping but is never echo-stripped - rather than the
// displayed Stdout. Stdout can already have had a leading command echo
// stripped by the end of a prior update; comparing a fresh raw cumulative
// frame against that stripped value would never find a matching prefix,
// misclassifying every later frame as a new chunk and duplicating output
// (plus reintroducing the very echo this file strips).
func replaceOrAppendTerminalOutput(payload *streams.ShellExecPayload, data string) {
current := ensureShellOutput(payload).Stdout
if current == "" || strings.HasPrefix(data, current) {
baseline := ensureShellOutput(payload).RawTerminalOutput
if baseline == "" || strings.HasPrefix(data, baseline) {
replaceShellStdout(payload, data)
return
}
// ACP does not identify cumulative terminal_output values. Treat values
// without the current prefix as new chunks so mixed update streams remain intact.
// without the baseline prefix as new chunks so mixed update streams remain intact.
appendShellStdout(payload, data)
}
@@ -289,6 +289,262 @@ func TestNormalizeShellToolUpdateCumulativeOutputRemainsCorrectAfterTruncation(t
require.True(t, payload.ShellExec().Output.Truncated)
}
func TestNormalizeShellToolResultStripsLeadingCommandEcho(t *testing.T) {
t.Parallel()
tests := []struct {
name string
command string
result any
wantStdout string
}{
{
name: "echoed command directly precedes output with no separator",
command: "cat file.txt",
result: "$ cat file.txt=== marker ===\n",
wantStdout: "=== marker ===\n",
},
{
name: "echoed command with shell prompt and newline separator",
command: "ls -la",
result: "$ ls -la\ntotal 0\n",
wantStdout: "total 0\n",
},
{
name: "multi-line command echoed verbatim before output",
command: "for i in 1 2; do\n echo $i\ndone",
result: "$ for i in 1 2; do\n echo $i\ndone\n1\n2\n",
wantStdout: "1\n2\n",
},
{
name: "command text appearing later in output is preserved",
command: "echo hi",
result: "unrelated preamble\necho hi appears mid-output\n",
wantStdout: "unrelated preamble\necho hi appears mid-output\n",
},
{
name: "output starting with a different command is preserved",
command: "echo hi",
result: "echo bye\n",
wantStdout: "echo bye\n",
},
{
// Regression for a review finding: without the "$ " prompt marker,
// a bare command-text prefix is ambiguous with legitimate output
// that happens to start with the same text (e.g. a file whose
// content begins with the command string, or a command that
// echoes its own argument). Only the evidenced "$ "-prefixed
// terminal-echo shape is stripped.
name: "bare command-text prefix without a shell prompt is preserved",
command: "cat file.txt",
result: "cat file.txt",
wantStdout: "cat file.txt",
},
{
name: "output that happens to start with the command text is preserved",
command: "echo hi",
result: "echo hi.txt contents follow\n",
wantStdout: "echo hi.txt contents follow\n",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": tt.command},
})
normalizer.NormalizeToolResult(payload, tt.result)
require.Equal(t, tt.wantStdout, payload.ShellExec().Output.Stdout)
})
}
}
func TestNormalizeShellToolUpdateStripsLeadingCommandEchoFromLiveOutput(t *testing.T) {
t.Parallel()
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": "tail -f log.txt"},
})
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output_delta": map[string]any{"data": "$ tail -f log.txt"}},
nil,
nil,
)
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output_delta": map[string]any{"data": "\nline one\n"}},
nil,
nil,
)
require.Equal(t, "line one\n", payload.ShellExec().Output.Stdout)
}
// TestNormalizeShellToolUpdateCumulativeTerminalOutputDoesNotDuplicateAfterEchoStrip
// is a regression for a review finding: stripping the echo from the
// displayed Stdout after the first cumulative terminal_output frame must not
// corrupt the prefix comparison used to classify the next raw cumulative
// frame. Comparing the provider's next full (unstripped) snapshot against an
// already-stripped Stdout never finds a matching prefix, so the frame gets
// misclassified as a new delta chunk - duplicating the prior output and
// reintroducing the very echo this file exists to strip.
func TestNormalizeShellToolUpdateCumulativeTerminalOutputDoesNotDuplicateAfterEchoStrip(t *testing.T) {
t.Parallel()
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": "cmd"},
})
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output": map[string]any{"data": "$ cmd\nfirst\n"}},
nil,
nil,
)
require.Equal(t, "first\n", payload.ShellExec().Output.Stdout)
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output": map[string]any{"data": "$ cmd\nfirst\nsecond\n"}},
nil,
nil,
)
require.Equal(t, "first\nsecond\n", payload.ShellExec().Output.Stdout)
}
// TestNormalizeShellToolUpdateCumulativeOversizedFrameStaysCorrectAfterEchoStrip
// documents that branch selection in replaceOrAppendTerminalOutput cannot
// desync the final Stdout once a cumulative frame's own length reaches the
// output bound: boundShellOutput always keeps just the trailing N bytes, so
// bound(anything + frame) == bound(frame) whenever len(frame) >= N,
// regardless of whether that frame was classified as a replace or an append.
// The short-output regime (the actually-reported bug shape, and the sibling
// test above) is where a misclassified branch is observable; this test
// pins the oversized regime to a concrete, content-bearing assertion rather
// than relying on that invariant by inference.
func TestNormalizeShellToolUpdateCumulativeOversizedFrameStaysCorrectAfterEchoStrip(t *testing.T) {
t.Parallel()
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": "cmd"},
})
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output": map[string]any{"data": "$ cmd\nfirst\n"}},
nil,
nil,
)
require.Equal(t, "first\n", payload.ShellExec().Output.Stdout)
oversized := "$ cmd\nfirst\n" + strings.Repeat("a", maxShellOutputBytes) + "DISTINGUISHABLE_TAIL"
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output": map[string]any{"data": oversized}},
nil,
nil,
)
want, truncated := boundShellOutput(oversized)
require.True(t, truncated)
require.Equal(t, want, payload.ShellExec().Output.Stdout)
require.Contains(t, payload.ShellExec().Output.Stdout, "DISTINGUISHABLE_TAIL")
require.NotContains(t, payload.ShellExec().Output.Stdout, "first\nfirst\n")
}
// TestNormalizeShellToolUpdateFinalRawOutputSurvivesTerminalOutputClobber is a
// regression for a review finding: when a single update carries both a final
// rawOutput and a terminal_output field, the terminal_output branch replaces
// Stdout with the provider's raw (unstripped) snapshot after
// applyFinalShellResult already stripped it. The end-of-update pass must
// re-commit the strip - not defer it - whenever rawOutput is present, so an
// update whose final content is nothing but the echoed command still
// collapses to empty instead of surfacing the raw echo.
func TestNormalizeShellToolUpdateFinalRawOutputSurvivesTerminalOutputClobber(t *testing.T) {
t.Parallel()
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": "cmd"},
})
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{"terminal_output": map[string]any{"data": "$ cmd"}},
nil,
"$ cmd",
)
require.Empty(t, payload.ShellExec().Output.Stdout)
}
// TestNormalizeShellToolUpdateFinalTerminalExitCommitsExactMatchEcho is a
// regression for a review finding: a command can also be finalized purely by
// a terminal_exit exit-code frame with no rawOutput at all (the ACP terminal
// extension's own exit reporting). An exit-code-only completion is just as
// final as a rawOutput-bearing one, so an exact-match echo must collapse to
// empty here too - not stay deferred forever with no further update ever
// arriving to re-trigger the strict path.
func TestNormalizeShellToolUpdateFinalTerminalExitCommitsExactMatchEcho(t *testing.T) {
t.Parallel()
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": "cmd"},
})
normalizer.NormalizeShellToolUpdate(
payload,
map[string]any{
"terminal_output": map[string]any{"data": "$ cmd"},
"terminal_exit": map[string]any{"exit_code": 0},
},
nil,
nil,
)
require.Empty(t, payload.ShellExec().Output.Stdout)
require.NotNil(t, payload.ShellExec().Output.ExitCode)
require.Equal(t, 0, *payload.ShellExec().Output.ExitCode)
}
// TestNormalizeShellToolUpdateCommitsEchoStripExactlyOnceForRawOutputOnlyResult
// is a regression for a review finding: applyFinalShellResult already
// commits the strict strip when it runs (rawOutput != nil), so the
// end-of-update pass must not strip a second time on top of an
// already-normalized value - a raw-only result whose real output legitimately
// repeats the echoed command text (e.g. "$ cmd\n$ cmd\n") would otherwise lose
// the second, legitimate occurrence to a redundant re-strip.
func TestNormalizeShellToolUpdateCommitsEchoStripExactlyOnceForRawOutputOnlyResult(t *testing.T) {
t.Parallel()
normalizer := NewNormalizer("")
payload := normalizer.NormalizeToolCall("execute", map[string]any{
"kind": "execute",
"raw_input": map[string]any{"command": "cmd"},
})
normalizer.NormalizeShellToolUpdate(payload, nil, nil, "$ cmd\n$ cmd\n")
require.Equal(t, "$ cmd\n", payload.ShellExec().Output.Stdout)
}
func shellOutputJSON(t *testing.T, output any) map[string]any {
t.Helper()
require.NotNil(t, output)
@@ -205,6 +205,12 @@ type ShellExecOutput struct {
// Internal stream state keeps the serialized combined flag accurate across replacements.
StdoutTruncated bool `json:"-"`
StderrTruncated bool `json:"-"`
// RawTerminalOutput mirrors Stdout's own bounded cumulative-append
// bookkeeping but is never echo-stripped. Cumulative terminal_output
// frames must be prefix-compared against the provider's own raw view of
// what it has sent so far, not against Stdout, which may already have
// had a leading command echo stripped for display.
RawTerminalOutput string `json:"-"`
}
// CodeSearchOutput contains the result of a code search operation.
@@ -0,0 +1,61 @@
import { test, expect } from "../../fixtures/test-base";
import { SessionPage } from "../../pages/session-page";
// Regression for a transcript bug: some providers capture shell output via a
// real terminal session whose input echo - rendered behind a "$ " shell
// prompt - is concatenated directly onto the real output, so the persisted
// stdout opens with a verbatim repeat of the command the chat already
// renders above the Output disclosure. The ACP shell-output normalizer
// strips that leading echo (unit-tested in shell_output_test.go); this file
// drives the mock-agent through the real ACP pipeline and asserts the chat
// UI never shows the command twice.
test.describe("shell command output echo stripping", () => {
test("does not repeat the command inside the expanded Output disclosure", async ({
testPage,
apiClient,
seedData,
}) => {
const command = "cat file.txt";
const echoedOutput = `$ ${command}=== marker ===\n`;
const script = [
`e2e:shell_result("${command}", "${echoedOutput.replace(/\n/g, "\\n")}")`,
'e2e:message("done")',
].join("\n");
const task = await apiClient.createTaskWithAgent(
seedData.workspaceId,
"Shell echo 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();
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("=== marker ===");
// Critical: the echoed command must not also appear inside the Output
// disclosure - it's already shown once in the command row above.
await expect(outputRegion).not.toContainText(command);
});
});