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 44f64677dc..ebff5e1123 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 @@ -21,10 +21,10 @@ type normalizedShellResult struct { stderrTruncated bool } -func applyFinalShellResult(payload *streams.ShellExecPayload, result any) { +func applyFinalShellResult(payload *streams.ShellExecPayload, result any) bool { normalized := normalizeFinalShellResult(result) if !normalized.hasStdout && !normalized.hasStderr && normalized.exitCode == nil { - return + return false } output := ensureShellOutput(payload) if normalized.hasStdout { @@ -41,6 +41,7 @@ func applyFinalShellResult(payload *streams.ShellExecPayload, result any) { if normalized.exitCode != nil { output.ExitCode = normalized.exitCode } + return normalized.hasStdout } // stripLeadingCommandEcho removes a leading terminal-prompt echo of the tool @@ -198,9 +199,8 @@ func (n *Normalizer) NormalizeShellToolUpdate( recognized = true } if rawOutput != nil { - applyFinalShellResult(shell, rawOutput) + finalStdoutCommitted = applyFinalShellResult(shell, rawOutput) recognized = true - finalStdoutCommitted = true } if data, ok := terminalOutputData(meta, "terminal_output"); ok { if rawOutput != nil { diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/tool_call_update_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/tool_call_update_test.go index b59fe166aa..b64b15a9d8 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/tool_call_update_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/tool_call_update_test.go @@ -225,6 +225,41 @@ func TestConvertToolCallResultUpdate_ReplacesCumulativeContent(t *testing.T) { require.Equal(t, "first\nsecond\n", payload.ShellExec().Output.Stdout) } +func TestConvertToolCallResultUpdate_OMPStructuredFinalOutputStripsCommandContent(t *testing.T) { + a := newTestAdapter() + seedExecuteToolCall(t, a, "tc-omp") + + command := "/usr/bin/gh pr view --json files --jq .files[]" + realOutput := "README.md\nsrc/main.go\n" + completed := acp.ToolCallStatus("completed") + event := a.convertToolCallResultUpdate("session-1", &acp.SessionToolCallUpdate{ + ToolCallId: "tc-omp", + Title: &command, + Status: &completed, + RawInput: map[string]any{"command": command}, + Content: []acp.ToolCallContent{ + {Content: &acp.ToolCallContentContent{ + Content: acp.TextBlock("$ " + command), + Type: "content", + }}, + {Content: &acp.ToolCallContentContent{ + Content: acp.TextBlock(realOutput), + Type: "content", + }}, + }, + RawOutput: map[string]any{ + "metadata": map[string]any{"exit": float64(0)}, + }, + }) + + require.NotNil(t, event) + require.Equal(t, command, event.NormalizedPayload.ShellExec().Command) + require.Equal(t, realOutput, event.NormalizedPayload.ShellExec().Output.Stdout) + require.NotContains(t, event.NormalizedPayload.ShellExec().Output.Stdout, "$ "+command) + require.NotNil(t, event.NormalizedPayload.ShellExec().Output.ExitCode) + require.Equal(t, 0, *event.NormalizedPayload.ShellExec().Output.ExitCode) +} + func TestConvertToolCallResultUpdate_FinalOutputReplacesLiveWithoutDuplication(t *testing.T) { a := newTestAdapter() seedExecuteToolCall(t, a, "tc-final") diff --git a/docs/plans/acp-shell-command-echo/plan.md b/docs/plans/acp-shell-command-echo/plan.md new file mode 100644 index 0000000000..b098df6bb2 --- /dev/null +++ b/docs/plans/acp-shell-command-echo/plan.md @@ -0,0 +1,78 @@ +--- +spec: docs/specs/ui/acp-shell-command-output.md +created: 2026-08-10 +status: implemented +issue: https://github.com/kdlbs/kandev/issues/2480 +--- + +# Implementation Plan: ACP Shell Command Echo Normalization + +## Overview + +OMP sends the visible `$ ` text and the command result as adjacent ACP content items, while its structured final `rawOutput` does not expose a stdout field recognized by Kandev. Kandev currently treats any non-nil final `rawOutput` as proof that stdout was already normalized, so it skips the final command-echo strip and persists the two content items glued together. The repair makes that state reflect whether stdout was actually recognized and adds an adapter-level regression test for OMP's exact event shape. + +## Backend + +### Track actual final-stdout normalization + +Files: + +- `apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output.go` +- `apps/backend/internal/agentctl/server/adapter/transport/acp/tool_call_update_test.go` + +Changes: + +- Make `applyFinalShellResult` report whether the final result contained a recognized stdout field and therefore committed the leading-command normalization. +- In `NormalizeShellToolUpdate`, derive `finalStdoutCommitted` from that result instead of setting it for every non-nil `rawOutput`. +- Preserve the existing single-strip guarantee when the final result does contain recognized stdout, including real output that legitimately starts with a second `$ ` line. +- Add an adapter-level regression test that seeds OMP's `execute` tool call and completes it with two adjacent text content items (`$ ` and real output) plus its structured `rawOutput` container. + +No persistence, API, frontend, or provider-specific branch is added. The existing provider-neutral ACP adapter remains the normalization boundary established by ADR-0036. + +## Tests + +- **What:** OMP's structured final shell update removes the leading command presentation content and preserves the complete adjacent real output. + **File:** `apps/backend/internal/agentctl/server/adapter/transport/acp/tool_call_update_test.go`. + **How:** Adapter integration test through `convertToolCallUpdate` and `convertToolCallResultUpdate` using the current OMP mapper shape. +- **What:** Recognized final stdout is normalized exactly once and cumulative/live shell paths keep their existing behavior. + **File:** existing tests in `apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output_test.go`. + **How:** Run the full ACP adapter package after the focused red/green regression. + +No browser E2E is added because the browser contract and renderer do not change; the defect occurs before persistence at the ACP adapter boundary, and the adapter integration test exercises the complete changed path. + +## Verification Results + +Diagnostic reproduction completed before implementation: + +- The throwaway OMP regression failed as expected because Kandev retained + `$ ` directly before `/usr/bin/gh`. +- The throwaway test was removed after confirming the root cause. +- Focused regressions passed: 2 tests. +- Full ACP adapter package passed: 764 tests. + +PR fixup verification: + +- Review remediation commit `59f10560c8563a3fd68a5885cb555ecddaa302fc` added + exit-code assertions for the metadata-only OMP result, clarified the + recognized-stdout wording, and changed the task verification to use the + repository backend test target. +- Focused regressions passed: 2 tests. +- `rtk make -C apps/backend test` passed. +- At the exact PR head above, CI reported 36 passed, 0 failed, and two E2E + shards still in progress: `E2E Shard 2/14` and `E2E Shard 6/14`. +- The review thread was replied to and resolved; no unresolved review threads + or actionable issue comments remain. GitHub reports the PR as mergeable with + `mergeStateStatus=UNSTABLE` while those checks run. + +## Implementation Waves And Parallel Candidates + +- [x] [Task 01: Normalize OMP structured final shell output](task-01-normalize-omp-shell-output.md) (`done`) + +The task is sequential. It owns the complete repair and its regression coverage; no subagent delegation is authorized by this plan. + +## Risks And Out Of Scope + +- The fix must distinguish “final result present” from “final stdout recognized” without stripping an already-normalized value twice. +- This repair does not infer or synthesize a separator between arbitrary content items. It removes only the already-evidenced leading `$ ` presentation content. +- Provider-side output missing before it reaches Kandev remains out of scope. +- Installing or changing OMP is out of scope; the captured public mapper shape and failing Kandev adapter reproduction are sufficient to own this repair. diff --git a/docs/plans/acp-shell-command-echo/task-01-normalize-omp-shell-output.md b/docs/plans/acp-shell-command-echo/task-01-normalize-omp-shell-output.md new file mode 100644 index 0000000000..9e2af5119c --- /dev/null +++ b/docs/plans/acp-shell-command-echo/task-01-normalize-omp-shell-output.md @@ -0,0 +1,87 @@ +--- +id: "01-normalize-omp-shell-output" +title: "Normalize OMP structured final shell output" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/acp-shell-command-output.md" +parallelism: sequential +--- + +# Task 01: Normalize OMP Structured Final Shell Output + +## Intent + +Ensure a structured final ACP result marks stdout as committed only when Kandev actually recognized and normalized a stdout field. This lets the existing final command-echo strip run for OMP's cumulative content without adding an OMP-specific production branch. + +## Acceptance + +- OMP's adjacent `$ ` and real-output content persists only the complete real output, including its first byte when no separator exists. +- A recognized final stdout value is still normalized exactly once; legitimate real output beginning with `$ ` is not stripped a second time. +- The complete ACP adapter package passes. + +## Verification + +Run the new regression before the production change and confirm it fails for the echoed-command mismatch. After the minimal fix, run the focused regression and the repository backend test target: + +```bash +cd apps/backend && rtk go test ./internal/agentctl/server/adapter/transport/acp -run 'TestConvertToolCallResultUpdate_OMPStructuredFinalOutputStripsCommandContent|TestNormalizeShellToolUpdateCommitsEchoStripExactlyOnceForRawOutputOnlyResult' -count=1 +rtk make -C apps/backend test +``` + +## Files Likely Touched + +- `apps/backend/internal/agentctl/server/adapter/transport/acp/shell_output.go` +- `apps/backend/internal/agentctl/server/adapter/transport/acp/tool_call_update_test.go` +- `docs/specs/ui/acp-shell-command-output.md` +- `docs/plans/acp-shell-command-echo/plan.md` +- `docs/plans/acp-shell-command-echo/task-01-normalize-omp-shell-output.md` + +## Dependencies + +None. + +## Parallelism + +`sequential`. The production change and regression test exercise the same adapter state transition. + +## Inputs + +- OMP's ACP event mapper emits `$ ` as presentation content on command updates and merges result text after it. +- `NormalizeShellToolUpdate` currently sets `finalStdoutCommitted` for any non-nil `rawOutput`, even when `applyFinalShellResult` recognizes no stdout. +- ADR-0036 keeps provider output normalization at the ACP adapter boundary. +- The OMP regression scenario in `docs/specs/ui/acp-shell-command-output.md`. + +## Output Contract + +Report the root-cause branch changed, files modified, the red/green regression result, the full ACP package result, remaining risks, and synchronized task/plan statuses in this conversation. + +## Results + +Implemented the provider-neutral normalization-state repair. + +- Changed `applyFinalShellResult` to report whether it recognized stdout. +- Used that result for `finalStdoutCommitted`, so metadata-only structured final + results still receive the final command-echo strip. +- Added an adapter-level OMP regression for adjacent `$ ` and real + output content with a structured metadata-only `rawOutput`. +- Red: the new regression failed because the persisted output retained the + command echo directly before `README.md`. +- Green: the focused regression and existing exactly-once regression passed. +- Full ACP adapter package: 764 tests passed. + +Remaining risk is limited to provider output that is absent before it reaches +the adapter; this change does not synthesize missing output or separators. + +PR fixup verification: + +- Review remediation commit `59f10560c8563a3fd68a5885cb555ecddaa302fc` added + exit-code assertions, clarified the recognized-stdout wording, and aligned + the verification command with the repository backend test target. +- Focused regressions passed: 2 tests. +- `rtk make -C apps/backend test` passed. +- At the exact PR head, CI reported 36 passed, 0 failed, and only `E2E Shard + 2/14` and `E2E Shard 6/14` still running. The review thread was resolved and + the PR was mergeable; GitHub reported `UNSTABLE` only because CI was still + in progress. diff --git a/docs/specs/ui/acp-shell-command-output.md b/docs/specs/ui/acp-shell-command-output.md index baddcf01f3..af62d040fb 100644 --- a/docs/specs/ui/acp-shell-command-output.md +++ b/docs/specs/ui/acp-shell-command-output.md @@ -1,7 +1,7 @@ --- status: shipped created: 2026-07-14 -updated: 2026-08-07 +updated: 2026-08-10 owner: cfl --- @@ -21,6 +21,7 @@ Agent chat shows that a shell command ran, but ACP agents encode terminal output - Claude: replace the displayed output with `_meta.terminal_output.data` and read `_meta.terminal_exit.exit_code`. Plain final `rawOutput` remains a fallback for agents or versions that do not emit the extension. - OpenCode: replace the displayed output from cumulative text `content`; on completion prefer `rawOutput.output` and read `rawOutput.metadata.exit`. - Auggie: parse `rawOutput.output`, including its ``, ``, and `` fields. + - OMP: keep the reported command visible once in the command row, omit the matching leading `$ ` presentation content from the output body, and preserve real output that immediately follows it without a separator. A structured final `rawOutput` that has no recognized stdout field does not bypass this normalization. - A final authoritative output replaces the accumulated live output rather than appending it a second time. If a final update omits output or one explicit stream, the accumulated value for each omitted field remains visible. - Exit-code precedence is `_meta.terminal_exit.exit_code`, provider-native structured exit fields, then Auggie's ``. An absent or unparseable exit status remains unknown; it MUST NOT become exit `0`. - Terminal text is treated as a combined stream unless an agent explicitly supplies separate stdout and stderr fields. Kandev does not infer stream separation from line ordering. @@ -116,6 +117,7 @@ The extension is additive: agents that ignore it continue through their current - Malformed provider output is retained as combined terminal text when possible; malformed exit metadata remains unknown. - A statusless update with recognized terminal output is treated as an in-progress tool update so it is not dropped by the existing persistence path. - Repeated cumulative output replaces the previous cumulative value; delta output appends once. Final aggregate output replaces either form and cannot duplicate the visible text. +- A structured final result without a recognized stdout field does not mark cumulative shell content as already normalized. Matching leading command presentation content is still removed before persistence. - Output exceeding the bound is truncated deterministically and remains valid UTF-8. - Unknown exit status never renders a success check or a failure cross solely because the value is absent. - ACP `failed` and `cancelled` statuses terminate active-call tracking even when no exit code is available. @@ -133,6 +135,7 @@ Live output updates use the existing tool-message update path and are persisted - **GIVEN** Claude receives the advertised terminal-output capability and emits `terminal_output` followed by `terminal_exit`, **WHEN** the command completes, **THEN** its output is visible and the exact exit code is shown. - **GIVEN** OpenCode emits cumulative `content` values followed by `rawOutput.metadata.exit: 7` while ACP status is `completed`, **WHEN** the command completes, **THEN** the latest output is not duplicated and the row shows `Exit code 7` as failure. - **GIVEN** Auggie returns XML-like output with `0`, **WHEN** the command completes, **THEN** the parsed output is visible and the row shows `Exit code 0` as success. +- **GIVEN** OMP completes a shell tool with cumulative text content containing `$ ` immediately followed by real output and a structured `rawOutput` without a recognized stdout field, **WHEN** Kandev normalizes the terminal update, **THEN** the command remains visible once in the command row and the persisted output starts with the complete real output. - **GIVEN** an agent returns plain output with no structured or embedded exit status, **WHEN** the command completes, **THEN** the output is visible and the row shows `Exit code unavailable` with neutral status. - **GIVEN** a command is cancelled without an exit code, **WHEN** its terminal update is persisted, **THEN** the transcript remains expandable and shows `Exit code unavailable`. - **GIVEN** a command's turn has settled and the provider later emits a terminal tool update, **WHEN** the update is persisted, **THEN** the existing transcript is reconciled without creating a turn, waking the session or task, or producing an empty-output warning.