fix: normalize OMP shell command output (#2480) (#2486)

2026-08-10 15:42:19 +01:00
parent 7968691dc1
commit 152e5a34c0
5 changed files with 208 additions and 5 deletions
@@ -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 {
@@ -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")
@@ -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 `$ <command>` 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 `$ <command>` line.
+- Add an adapter-level regression test that seeds OMP's `execute` tool call and completes it with two adjacent text content items (`$ <command>` 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
+  `$ <command>` 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 `$ <command>` 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.
@@ -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 `$ <command>` 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 `$ <command>` 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 `$ <command>` 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 `$ <command>` 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.
@@ -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 `<output>`, `<stderr>`, and `<return-code>` fields.
+  - OMP: keep the reported command visible once in the command row, omit the matching leading `$ <command>` 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 `<return-code>`. 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 `<return-code>0</return-code>`, **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 `$ <command>` 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.