From 31d4f1e65f1afc777b05bb7426e51dfd96dd8ad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 7 Aug 2026 18:14:32 +0100 Subject: [PATCH] fix: hide empty shell output disclosure (#2378) --- .../messages/tool-execute-message.test.tsx | 60 +++++++--- .../chat/messages/tool-execute-message.tsx | 16 +-- .../task/chat/messages/turn-group-message.tsx | 10 +- apps/web/components/task/chat/types.ts | 8 ++ .../chat/mobile-tool-execute-output.spec.ts | 51 ++++++++ .../tests/chat/tool-execute-output.spec.ts | 31 +++++ .../plan.md | 110 ++++++++++++++++++ .../task-01-gate-empty-output-disclosure.md | 78 +++++++++++++ docs/specs/ui/acp-shell-command-output.md | 4 +- 9 files changed, 335 insertions(+), 33 deletions(-) create mode 100644 docs/plans/shell-output-disclosure-visibility/plan.md create mode 100644 docs/plans/shell-output-disclosure-visibility/task-01-gate-empty-output-disclosure.md diff --git a/apps/web/components/task/chat/messages/tool-execute-message.test.tsx b/apps/web/components/task/chat/messages/tool-execute-message.test.tsx index 274d34b7ad..209b084039 100644 --- a/apps/web/components/task/chat/messages/tool-execute-message.test.tsx +++ b/apps/web/components/task/chat/messages/tool-execute-message.test.tsx @@ -54,6 +54,10 @@ function executeMessage( }; } +function retainedOutputSummary(overrides: ShellOutputSummary = {}): ShellOutputSummary { + return { has_output: true, stdout_bytes: 1, ...overrides }; +} + function hookResult( overrides: Partial = {}, ): UseShellCommandOutputResult { @@ -71,14 +75,30 @@ function openOutput() { } describe("ToolExecuteMessage command row", () => { + it("hides the output disclosure when the projected summary is empty", () => { + render( + , + ); + + expect(screen.getByTestId("tool-execute-command")).toBeTruthy(); + expect(screen.queryByRole("button", { name: /show command output/i })).toBeNull(); + expect(useShellOutputMock).not.toHaveBeenCalled(); + }); + it("keeps the normalized command and working directory visible while output is collapsed", () => { render( , ); @@ -103,7 +123,8 @@ describe("ToolExecuteMessage command row", () => { it("renders loading and empty snapshot states only after opening", () => { useShellOutputMock.mockReturnValue(hookResult({ isLoading: true })); - const view = render(); + const outputSummary = retainedOutputSummary(); + const view = render(); expect(screen.queryByText("Loading command output...")).toBeNull(); openOutput(); @@ -119,7 +140,7 @@ describe("ToolExecuteMessage command row", () => { }, }), ); - view.rerender(); + view.rerender(); expect(screen.getByText("No command output yet.")).toBeTruthy(); expect(screen.queryByText(/Exit code/)).toBeNull(); }); @@ -142,7 +163,11 @@ describe("ToolExecuteMessage output states", () => { }, }), ); - render(); + render( + , + ); openOutput(); expect(screen.getByText("standard output")).toBeTruthy(); @@ -164,11 +189,10 @@ describe("ToolExecuteMessage output states", () => { ); render( , ); @@ -192,7 +216,7 @@ describe("ToolExecuteMessage output states", () => { retry, }), ); - render(); + render(); openOutput(); expect(screen.getByText("retained transcript")).toBeTruthy(); @@ -213,7 +237,7 @@ describe("ToolExecuteMessage output states", () => { error: new Error("network unavailable"), }), ); - render(); + render(); openOutput(); expect(screen.getByText("Command output unavailable.")).toBeTruthy(); @@ -233,7 +257,11 @@ describe("ToolExecuteMessage cancelled result", () => { }, }), ); - render(); + render( + , + ); openOutput(); expect(screen.getByText("Exit code 0").className).toContain("text-muted-foreground"); diff --git a/apps/web/components/task/chat/messages/tool-execute-message.tsx b/apps/web/components/task/chat/messages/tool-execute-message.tsx index 83bc253703..201956aa96 100644 --- a/apps/web/components/task/chat/messages/tool-execute-message.tsx +++ b/apps/web/components/task/chat/messages/tool-execute-message.tsx @@ -7,7 +7,7 @@ import { transformPathsInText } from "@/lib/utils"; import type { Message } from "@/lib/types/http"; import { ShellOutputDisclosure } from "./shell-output-disclosure"; import { normalizeToolCallStatus } from "./tool-status"; -import type { ToolCallMetadata } from "../types"; +import { hasProjectedShellOutput, type ToolCallMetadata } from "../types"; import { useTranslation } from "react-i18next"; type ToolExecuteMessageProps = { @@ -89,12 +89,14 @@ export const ToolExecuteMessage = memo(function ToolExecuteMessage({ )} - + {hasProjectedShellOutput(shellExec?.output) && ( + + )} ); diff --git a/apps/web/components/task/chat/messages/turn-group-message.tsx b/apps/web/components/task/chat/messages/turn-group-message.tsx index bdc7996457..15b2c535c3 100644 --- a/apps/web/components/task/chat/messages/turn-group-message.tsx +++ b/apps/web/components/task/chat/messages/turn-group-message.tsx @@ -7,7 +7,7 @@ import { GridSpinner } from "@/components/grid-spinner"; import { cn, transformPathsInText } from "@/lib/utils"; import type { Message } from "@/lib/types/http"; import type { TurnGroup } from "@/hooks/use-processed-messages"; -import type { ToolCallMetadata } from "@/components/task/chat/types"; +import { hasProjectedShellOutput, type ToolCallMetadata } from "@/components/task/chat/types"; import { MessageRenderer } from "@/components/task/chat/message-renderer"; import { isSubagentEffectivelyActive } from "@/components/task/chat/messages/tool-subagent-message"; @@ -167,14 +167,6 @@ function isZeroExitCode(shellExec: ShellExecPayload): boolean { return exitCode === 0; } -function hasProjectedShellOutput(output: ShellExecPayload["output"]): boolean { - return ( - Boolean(output?.has_output) || - (output?.stdout_bytes ?? 0) > 0 || - (output?.stderr_bytes ?? 0) > 0 - ); -} - function createShellExecSummary(message: Message, shellExec: ShellExecPayload): ShellExecSummary { const output = shellExec.output; return { diff --git a/apps/web/components/task/chat/types.ts b/apps/web/components/task/chat/types.ts index 3f0e6a25d2..8eaa9a799f 100644 --- a/apps/web/components/task/chat/types.ts +++ b/apps/web/components/task/chat/types.ts @@ -103,6 +103,14 @@ export type ShellExecOutputSummary = { truncated?: boolean; }; +export function hasProjectedShellOutput(output: ShellExecOutputSummary | undefined): boolean { + return ( + Boolean(output?.has_output) || + (output?.stdout_bytes ?? 0) > 0 || + (output?.stderr_bytes ?? 0) > 0 + ); +} + export type ShellExecPayload = { command?: string; work_dir?: string; diff --git a/apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts b/apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts index a6e023e158..63b7fa013b 100644 --- a/apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts @@ -13,6 +13,57 @@ async function expectInsideViewport( } test.describe("mobile: shell command output", () => { + test("does not show an output disclosure for a command with no output", async ({ + testPage, + apiClient, + seedData, + }) => { + const task = await apiClient.createTask(seedData.workspaceId, "Mobile Empty Command Output", { + description: "seeded mobile shell command without output", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + agent_profile_id: seedData.agentProfileId, + repository_ids: [seedData.repositoryId], + }); + const { session_id: sessionId } = await apiClient.seedTaskSession(task.id, { + state: "IDLE", + agentProfileId: seedData.agentProfileId, + }); + const command = "true"; + await apiClient.seedSessionMessage(sessionId, { + type: "tool_execute", + content: command, + metadata: { + status: "complete", + tool_call_id: "tool-mobile-empty-output", + normalized: { + shell_exec: { + command, + work_dir: "/workspace", + output: { exit_code: 0 }, + }, + }, + }, + }); + const shellOutputRequests: string[] = []; + testPage.on("request", (request) => { + if (request.url().endsWith("/shell-output")) shellOutputRequests.push(request.url()); + }); + + await testPage.goto(`/t/${task.id}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + const chat = session.activeChat(); + + await expect(chat.getByTestId("tool-execute-command").filter({ hasText: command })).toBeVisible( + { timeout: 15_000 }, + ); + await expect(chat.getByLabel("Command succeeded")).toBeVisible(); + await expect(chat.getByRole("button", { name: "Show command output" })).toHaveCount(0); + await expect(chat.getByTestId("tool-execute-output")).toHaveCount(0); + expect(shellOutputRequests).toHaveLength(0); + }); + test("keeps a long command and lazy output inside the viewport", async ({ testPage, apiClient, diff --git a/apps/web/e2e/tests/chat/tool-execute-output.spec.ts b/apps/web/e2e/tests/chat/tool-execute-output.spec.ts index 9bc8897094..3f5c521bb3 100644 --- a/apps/web/e2e/tests/chat/tool-execute-output.spec.ts +++ b/apps/web/e2e/tests/chat/tool-execute-output.spec.ts @@ -46,6 +46,37 @@ async function seedShellMessage( } test.describe("shell command output", () => { + test("does not show an output disclosure for a command with no output", async ({ + testPage, + apiClient, + seedData, + }) => { + const command = "true"; + const taskId = await seedShellMessage(apiClient, seedData, { + title: "Empty Command Output", + command, + status: "complete", + output: { exit_code: 0 }, + }); + const shellOutputRequests: string[] = []; + testPage.on("request", (request) => { + if (request.url().endsWith("/shell-output")) shellOutputRequests.push(request.url()); + }); + + await testPage.goto(`/t/${taskId}`); + const session = new SessionPage(testPage); + await session.waitForLoad(); + const chat = session.activeChat(); + + await expect(chat.getByTestId("tool-execute-command").filter({ hasText: command })).toBeVisible( + { timeout: 15_000 }, + ); + await expect(chat.getByLabel("Command succeeded")).toBeVisible(); + await expect(chat.getByRole("button", { name: "Show command output" })).toHaveCount(0); + await expect(chat.getByTestId("tool-execute-output")).toHaveCount(0); + expect(shellOutputRequests).toHaveLength(0); + }); + test("keeps full commands visible and fetches completed output only after expansion", async ({ testPage, apiClient, diff --git a/docs/plans/shell-output-disclosure-visibility/plan.md b/docs/plans/shell-output-disclosure-visibility/plan.md new file mode 100644 index 0000000000..71e12938bc --- /dev/null +++ b/docs/plans/shell-output-disclosure-visibility/plan.md @@ -0,0 +1,110 @@ +--- +spec: docs/specs/ui/acp-shell-command-output.md +created: 2026-08-07 +status: done +--- + +# Implementation Plan: Hide empty shell output disclosures + +## Root cause + +`ToolExecuteMessage` always mounts `ShellOutputDisclosure`, including when the +projected shell summary has no stdout or stderr. The disclosure formats a +zero-byte summary as `Output`, so users can expand a control that only reveals +the empty-output or exit-status state. The summary already carries the data +needed to decide whether an output body exists. + +## Overview + +Guard the shared shell output disclosure at the command-row boundary using the +browser-facing summary. Preserve the existing collapsed, on-demand behavior for +commands with output, then prove both paths in the existing component, desktop, +and mobile chat tests. No backend, API, persistence, or mobile composition +change is required. + +## Frontend + +### Command row disclosure guard + +Files: + +- `apps/web/components/task/chat/messages/tool-execute-message.tsx` +- `apps/web/components/task/chat/messages/tool-execute-message.test.tsx` + +Render `ShellOutputDisclosure` only when the normalized summary represents +retained stdout/stderr: `has_output` is true or either projected byte count is +positive. Keep the disclosure unmounted for zero-output commands so the +on-demand hook cannot fetch or poll accidentally. Commands with output keep +their existing collapsed default, endpoint fetch, polling, truncation, and exit +status behavior. + +### Mobile parity contract + +Desktop and mobile use the same `ToolExecuteMessage` data and disclosure guard; +mobile renders it inside the existing `TaskChatPanel` composition. The nearest +shipped mobile exemplar is +`apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts`, which already +proves the long-command/output path. The mobile hierarchy remains the command +row first and the optional output disclosure second; a zero-output command has +no secondary action because there is no content to inspect. The chat transcript +continues to own vertical scrolling, with no new drawer, overlay, viewport, or +safe-area behavior. + +## Tests + +- **What:** A zero-output summary renders the command/status context without a + `Show command output` control, while a summary with `has_output` or positive + byte counts retains the collapsed disclosure and lazy-fetch behavior. + **File:** `apps/web/components/task/chat/messages/tool-execute-message.test.tsx`. + **How:** Extend the existing React Testing Library fixtures with an explicit + zero-output case and keep the current output-state assertions. +- **What:** Desktop chat does not expose an empty output affordance, while a + real-output command still fetches only after expansion. + **File:** `apps/web/e2e/tests/chat/tool-execute-output.spec.ts`. + **How:** Seed a shell message with an empty projected output summary, assert + the command/status remain visible and the disclosure/request are absent, then + retain the existing lazy completed-output scenario. +- **What:** Mobile chat has the same no-affordance behavior and preserves the + existing long-command/output path inside the viewport. + **File:** `apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts`. + **How:** Add a no-output seeded command assertion alongside the existing + mobile output scenario. + +## E2E Tests + +- **Scenario:** GIVEN a projected shell command with no retained output, WHEN + desktop chat renders it, THEN the command/status are visible and no output + disclosure or shell-output request exists. + **File:** `apps/web/e2e/tests/chat/tool-execute-output.spec.ts`. +- **Scenario:** GIVEN the same no-output command on the mobile chat surface, + WHEN the transcript renders, THEN the empty disclosure is absent without + changing the existing mobile command wrapping or output behavior. + **File:** `apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts`. + +## Verification Results + +- `cd apps && pnpm install --frozen-lockfile` — passed. +- `cd apps && pnpm --filter @kandev/web test -- --run components/task/chat/messages/tool-execute-message.test.tsx` — passed, 9 tests. +- `cd apps/web && pnpm run typecheck` — passed. +- `cd apps/web && pnpm e2e:run --project chromium tests/chat/tool-execute-output.spec.ts` — passed, 4 tests. +- `cd apps/web && pnpm e2e:run --project mobile-chrome tests/chat/mobile-tool-execute-output.spec.ts` — passed, 2 tests. +- `pnpm exec prettier --check` on the changed TypeScript files — passed. +- `git diff --check` — passed. +- Managed desktop/mobile screenshot captures with synthetic data — passed; compressed assets are listed in `.pr-assets/manifest.json` and are not committed to the PR branch. + +## Implementation Waves + +Wave 1 (sequential): + +- [x] [task-01-gate-empty-output-disclosure](task-01-gate-empty-output-disclosure.md) (done) + +## Risks + +- Older or partially populated summaries may omit `has_output`; positive byte + counts must still keep the disclosure visible so valid output is not hidden. +- No new mobile interaction pattern is introduced; the only mobile change is + removing an unusable control when there is no output body. + +## Open Questions + +None. diff --git a/docs/plans/shell-output-disclosure-visibility/task-01-gate-empty-output-disclosure.md b/docs/plans/shell-output-disclosure-visibility/task-01-gate-empty-output-disclosure.md new file mode 100644 index 0000000000..3a2f738745 --- /dev/null +++ b/docs/plans/shell-output-disclosure-visibility/task-01-gate-empty-output-disclosure.md @@ -0,0 +1,78 @@ +--- +id: "01-gate-empty-output-disclosure" +title: "Gate empty output disclosure" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ui/acp-shell-command-output.md" +--- + +# Task 01: Gate empty output disclosure + +## Acceptance + +- Shell command rows with `has_output: false` and zero stdout/stderr byte + counts show the command, working directory, and status without a `Show +command output` control or an on-demand output request. +- Shell command rows with `has_output: true` or positive byte counts preserve + the existing collapsed disclosure and fetch output only after expansion. +- Desktop and mobile E2E coverage proves the no-output path while retaining + the existing real-output and viewport behavior. + +## Verification + +```bash +cd apps && pnpm install --frozen-lockfile +cd apps && pnpm --filter @kandev/web test -- --run components/task/chat/messages/tool-execute-message.test.tsx +cd apps/web && pnpm run typecheck +cd apps/web && pnpm e2e:run --project chromium tests/chat/tool-execute-output.spec.ts +cd apps/web && pnpm e2e:run --project mobile-chrome tests/chat/mobile-tool-execute-output.spec.ts +``` + +## Files likely touched + +- `apps/web/components/task/chat/messages/tool-execute-message.tsx` +- `apps/web/components/task/chat/messages/tool-execute-message.test.tsx` +- `apps/web/e2e/tests/chat/tool-execute-output.spec.ts` +- `apps/web/e2e/tests/chat/mobile-tool-execute-output.spec.ts` +- `docs/plans/shell-output-disclosure-visibility/plan.md` + +## Dependencies + +None. + +## Parallelism + +Sequential. Production code, component coverage, and desktop/mobile coverage +share the same summary predicate and must be verified together. + +## Inputs + +- `docs/specs/ui/acp-shell-command-output.md`, especially the disclosure and + no-output scenarios. +- `docs/plans/shell-output-disclosure-visibility/plan.md`. +- Existing `ShellOutputDisclosure` lazy-fetch behavior and the current desktop + and mobile shell-output fixtures. +- Mobile parity contract in the plan: reuse the shared command row and make no + new drawer or responsive surface. + +## Output contract + +Report the root cause addressed, files changed, the failing-then-passing +component regression, exact desktop/mobile E2E and typecheck results, any +artifacts or blockers, and synchronized task/plan status. + +## Results + +Implemented the shared command-row guard in `ToolExecuteMessage`. The output +disclosure now mounts only when `has_output` is true or a projected stdout or +stderr byte count is positive. Zero-output commands retain their command, +working directory, and status while never mounting the lazy output hook. + +The component regression was first run red against the unguarded row, then +passed after the guard and fixture updates (9 tests). The desktop E2E suite +passed all 4 tests, and the mobile E2E suite passed both tests. Web typecheck, +Prettier, and `git diff --check` also passed. Fresh synthetic desktop and +mobile screenshots were captured, inspected, compressed, and recorded in the +ignored PR asset manifest; no production artifacts or blockers remain. diff --git a/docs/specs/ui/acp-shell-command-output.md b/docs/specs/ui/acp-shell-command-output.md index 07fbe7fbbe..baddcf01f3 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-07-16 +updated: 2026-08-07 owner: cfl --- @@ -27,6 +27,7 @@ Agent chat shows that a shell command ran, but ACP agents encode terminal output - Each normalized output text field is bounded to 256 KiB. When a field exceeds the bound, Kandev retains its most recent valid UTF-8 content and sets `truncated: true`. - The full normalized command is always visible and wraps instead of truncating. The message content remains its fallback when a normalized command is absent. The working directory, when present, remains visible with the command. - Stdout and stderr render in a separate disclosure that is collapsed by default for both running and completed commands. Running commands never auto-expand their output. +- The output disclosure is offered only when the browser-facing summary indicates retained stdout or stderr (`has_output: true` or a positive stdout/stderr byte count). A command with no retained output shows its command, working directory, and status without an expandable output control. - Expanding the disclosure fetches the latest output snapshot on demand. A collapsed disclosure does not fetch or mount the output body. - While an expanded command is running, Kandev refreshes its output with non-overlapping polling until the command becomes complete, failed, or cancelled. Polling stops immediately when the disclosure collapses or unmounts. - The expanded disclosure shows combined output in a scrollable monospace region. On terminal completion it visibly shows `Exit code N` when known, or `Exit code unavailable` when unknown. Unknown is neutral, not success or failure. @@ -138,6 +139,7 @@ Live output updates use the existing tool-message update path and are persisted - **GIVEN** terminal output exceeds 256 KiB, **WHEN** live and final updates are normalized, **THEN** stored output remains within the bound, keeps the newest valid UTF-8 text, and the expanded row indicates truncation. - **GIVEN** a persisted shell message with a long command, **WHEN** chat opens on desktop or mobile, **THEN** the complete command wraps visibly while its output disclosure remains collapsed. - **GIVEN** a collapsed shell command with persisted output, **WHEN** task boot state, message REST/WS lists, and live message notifications reach the browser, **THEN** they contain byte counts and result metadata but no stdout or stderr body, and no output endpoint request occurs. +- **GIVEN** a shell command whose browser-facing summary has `has_output: false`, `stdout_bytes: 0`, and `stderr_bytes: 0`, **WHEN** its command row renders on desktop or mobile, **THEN** the command, working directory, and status remain visible but no output disclosure is offered and no shell-output request can be initiated. - **GIVEN** a persisted completed shell message, **WHEN** the user expands its output disclosure, **THEN** exactly one on-demand snapshot is fetched and its output, truncation, and exit status are readable without overlapping adjacent chat content. - **GIVEN** a running shell message with its disclosure expanded, **WHEN** output changes and the command later completes, **THEN** bounded non-overlapping polling refreshes the transcript and stops after the terminal snapshot. - **GIVEN** an expanded running shell message, **WHEN** the user collapses it or navigates away, **THEN** scheduled polling stops and an in-flight request cannot update the unmounted disclosure.