From 040dab2ada43bef97621a4ae6b4701ec418a00c7 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Sun, 3 May 2026 12:21:36 +0800 Subject: [PATCH] =?UTF-8?q?fix(cli):=20address=20PR=203801=20review=20?= =?UTF-8?q?=E2=80=94=20exhaustive=20switch=20consistency=20+=20path-agnost?= =?UTF-8?q?ic=20hint?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four Suggestions from the latest /review pass: - `statusLabel` rewritten as a single top-level switch with a `never`-typed default, matching `taskLabel` / `taskId` / `taskOutputPath`. The previous `if`/`if`/fallthrough form would silently apply monitor formatting to a future 4th kind. - `taskOutputPath` gained the same exhaustive default — was the only per-kind helper still relying on implicit fallthrough; would silently omit a 4th-kind output path while the adjacent helpers flip to compile errors. - Hint wording de-specifies the exact keystroke count: `'Tip: focus the Background tasks pill in the footer (use ↓ from an empty composer) and press Enter ...'`. Previous "press ↓ then Enter" phrasing was wrong when the Arena agent tab bar is present — `InputPrompt`'s focus chain routes Down through the tab bar first, so a single Down lands there, not on the bg pill. - Test pin tightened: `[mon_fail] failed: spawn ENOENT (0 events)` is now a full-string assertion instead of a prefix match, so a regression that drops the `(N events)` suffix from monitor's failed branch fails loudly. --- .../cli/src/ui/commands/tasksCommand.test.ts | 8 +- packages/cli/src/ui/commands/tasksCommand.ts | 132 ++++++++++-------- 2 files changed, 83 insertions(+), 57 deletions(-) diff --git a/packages/cli/src/ui/commands/tasksCommand.test.ts b/packages/cli/src/ui/commands/tasksCommand.test.ts index 325170b0f..104285ea1 100644 --- a/packages/cli/src/ui/commands/tasksCommand.test.ts +++ b/packages/cli/src/ui/commands/tasksCommand.test.ts @@ -232,7 +232,13 @@ describe('tasksCommand', () => { expect(result.content).toContain( '[mon_auto] completed (Max events reached, 1000 events)', ); - expect(result.content).toContain('[mon_fail] failed: spawn ENOENT'); + // Pin the full string (not just the prefix) so a regression that + // drops the `(N events)` suffix from monitor's failed branch fails + // loudly. The other status branches above already have full-string + // assertions; this one was incidentally shorter. + expect(result.content).toContain( + '[mon_fail] failed: spawn ENOENT (0 events)', + ); // No on-disk output file for monitors — events stream via // task_notification, so the "output:" line should not appear for // any monitor entry. diff --git a/packages/cli/src/ui/commands/tasksCommand.ts b/packages/cli/src/ui/commands/tasksCommand.ts index a4ac8917f..553b0ab68 100644 --- a/packages/cli/src/ui/commands/tasksCommand.ts +++ b/packages/cli/src/ui/commands/tasksCommand.ts @@ -26,56 +26,62 @@ type MonitorTaskEntry = MonitorEntry & { kind: 'monitor' }; type TaskEntry = AgentTaskEntry | ShellTaskEntry | MonitorTaskEntry; function statusLabel(entry: TaskEntry): string { - if (entry.kind === 'agent') { - switch (entry.status) { - case 'completed': - return 'completed'; - case 'failed': - return `failed: ${entry.error ?? 'unknown error'}`; - case 'cancelled': - return 'cancelled'; - case 'paused': - return entry.resumeBlockedReason - ? `paused (resume blocked): ${entry.resumeBlockedReason}` - : 'paused'; - case 'running': - default: - return 'running'; + switch (entry.kind) { + case 'agent': + switch (entry.status) { + case 'completed': + return 'completed'; + case 'failed': + return `failed: ${entry.error ?? 'unknown error'}`; + case 'cancelled': + return 'cancelled'; + case 'paused': + return entry.resumeBlockedReason + ? `paused (resume blocked): ${entry.resumeBlockedReason}` + : 'paused'; + case 'running': + default: + return 'running'; + } + case 'shell': + switch (entry.status) { + case 'completed': + return `completed (exit ${entry.exitCode ?? '?'})`; + case 'failed': + return `failed: ${entry.error ?? 'unknown error'}`; + case 'cancelled': + return 'cancelled'; + case 'running': + return 'running'; + default: + return 'running'; + } + case 'monitor': { + // Append eventCount as a glanceable signal for activity. error (set + // on `failed` and on auto-stopped `completed`) is included verbatim. + const events = `${entry.eventCount} event${entry.eventCount === 1 ? '' : 's'}`; + switch (entry.status) { + case 'completed': + return entry.error + ? `completed (${entry.error}, ${events})` + : `completed (exit ${entry.exitCode ?? '?'}, ${events})`; + case 'failed': + return `failed: ${entry.error ?? 'unknown error'} (${events})`; + case 'cancelled': + return `cancelled (${events})`; + case 'running': + return `running (${events})`; + default: + return `running (${events})`; + } } - } - - if (entry.kind === 'shell') { - switch (entry.status) { - case 'completed': - return `completed (exit ${entry.exitCode ?? '?'})`; - case 'failed': - return `failed: ${entry.error ?? 'unknown error'}`; - case 'cancelled': - return 'cancelled'; - case 'running': - return 'running'; - default: - return 'running'; + default: { + const _exhaustive: never = entry; + throw new Error( + `statusLabel: unknown TaskEntry kind: ${JSON.stringify(_exhaustive)}`, + ); } } - - // monitor — append eventCount as a glanceable signal for activity. error - // (set on `failed` and on auto-stopped `completed`) is included verbatim. - const events = `${entry.eventCount} event${entry.eventCount === 1 ? '' : 's'}`; - switch (entry.status) { - case 'completed': - return entry.error - ? `completed (${entry.error}, ${events})` - : `completed (exit ${entry.exitCode ?? '?'}, ${events})`; - case 'failed': - return `failed: ${entry.error ?? 'unknown error'} (${events})`; - case 'cancelled': - return `cancelled (${events})`; - case 'running': - return `running (${events})`; - default: - return `running (${events})`; - } } function taskLabel(entry: TaskEntry): string { @@ -113,11 +119,22 @@ function taskId(entry: TaskEntry): string { } function taskOutputPath(entry: TaskEntry): string | undefined { - if (entry.kind === 'agent') return entry.outputFile; - if (entry.kind === 'shell') return entry.outputPath; - // Monitors stream to the agent via task_notification rather than a - // file on disk — no output path to surface here. - return undefined; + switch (entry.kind) { + case 'agent': + return entry.outputFile; + case 'shell': + return entry.outputPath; + case 'monitor': + // Monitors stream to the agent via task_notification rather than a + // file on disk — no output path to surface here. + return undefined; + default: { + const _exhaustive: never = entry; + throw new Error( + `taskOutputPath: unknown TaskEntry kind: ${JSON.stringify(_exhaustive)}`, + ); + } + } } export const tasksCommand: SlashCommand = { @@ -174,13 +191,16 @@ export const tasksCommand: SlashCommand = { // Soft redirect: in interactive mode the dialog is richer (per-entry // detail view, live updates, cancel keybinding). Don't show the hint // in non_interactive / acp — those consumers have no dialog to point - // at and the noise just clutters their output. Note: the dialog - // opens via Down arrow + Enter on the footer pill (NOT Ctrl+T — - // that's bound to the MCP tool descriptions toggle elsewhere). + // at and the noise just clutters their output. The wording avoids + // pinning a single-key path because Down may pass through the Arena + // agent tab bar first when subagents are present (`InputPrompt` + // focus chain: agent tab bar → bg pill); calling it "the footer + // Background tasks pill" lets the user reach it however the focus + // chain routes them today. if (context.executionMode === 'interactive') { lines.push( t( - 'Tip: press ↓ from an empty composer then Enter to open the interactive Background tasks dialog with detail view + live updates.', + 'Tip: focus the Background tasks pill in the footer (use ↓ from an empty composer) and press Enter for the interactive dialog with detail view + live updates.', ), '', );