Keep the review after a send and never hand a prompt to a shell pane (#1321)

Sending a review erased it while the agent received nothing. Two causes.

A resolved sendAgentMessageNow was treated as proof of delivery: it drops the
AgentPromptDelivery the backend returns, so the renderer wiped inlineComments on
every resolve. Even a tmux 'delivered' verdict only means the keystrokes were
accepted, not that the agent read them. Every send path now stamps sentAt and
keeps the text; clearing stays the confirmed Reset review.

resolveAgentPromptTargetPane fell through to the session's active pane whenever a
task had two or more live agent panes and no recorded focus, so a hand-off could
be typed into a focused shell split and still report success. The active pane now
wins only when it is itself an agent pane, otherwise the first live agent pane
does, and both fallbacks log the pane they picked.

Author: h0x91b

change-logs/2026/08/10/fix-review-send-never-loses-comments.md

@@ -0,0 +1,5 @@
+Short: Sending a review never loses it
+
+Pressing "Send to agent" on a diff review could erase the whole review while the agent received nothing — the send reported success as soon as the keystrokes left dev3, which is not the same as the agent reading them. Sent comments are now marked as sent and kept, exactly like a single comment sent on its own, and clearing them stays the explicit Reset button. A second cause is fixed underneath: on a task with several agent panes and no recorded focus, the hand-off could be typed into whatever pane you were looking at, including a plain shell — it now always goes to an agent pane, and the chosen pane is written to the log.
+
+Suggested by @diverru (Alexander Kiselyov)

decisions/2026/08/10/never-destroy-a-review-on-send.md

@@ -0,0 +1,58 @@
+# Never destroy an inline review on send, and never hand a prompt to a shell pane
+
+## Context
+
+A reviewer pressed "Send to agent" on a diff review: the agent received nothing and
+the whole review was erased. Intermittent — the next attempt worked. Reported through
+Alexander Kiselyov (`@diverru`) on 2026-08-10, against `v1.42.x`.
+
+## Investigation
+
+Two independent defects sat on the same path.
+
+1. **A resolved RPC was treated as proof of delivery.** `sendAgentMessageNow`
+   (`src/bun/rpc-handlers/pr-comments.ts`) discards the `AgentPromptDelivery` that
+   `sendMessageImmediately` returns and answers only `{ spilledPath }`. The renderer
+   therefore could not tell `delivered` from `unconfirmed`, and
+   `handleSendReviewToAgent` wiped `inlineComments` on every resolve — the auto-clear
+   added in #1236. Even a `delivered` verdict only means tmux accepted the keystrokes;
+   the agent's TUI can still drop a paste, which is exactly the intermittent shape of
+   the report.
+2. **A hand-off could be typed into a plain shell.** `resolveAgentPromptTargetPane`
+   (`src/bun/agent-prompt.ts`) fell through to the session's *active* pane whenever a
+   task had ≥2 live agent panes and no recorded last-focused agent. If the user was
+   looking at a shell or dev-server split, the review went there. tmux reports
+   `delivered` for any pane, so the send looked clean and the review then deleted
+   itself. The pre-existing test `ignores a recorded pane that is live but not a
+   registered agent pane` asserted this exact routing.
+
+## Decision
+
+- **The review is never destroyed by a send.** `handleSendReviewToAgent` and the
+  composer's `Send now` now stamp `sentAt` on the comments they handed over
+  (`markInlineCommentsSent`), the same sticky marking per-comment sends already used.
+  Sent comments grey out, leave the export payload, and disable `Copy`/`Send`; the
+  user clears them with the explicit, confirmed `Reset review`. This reverses the
+  auto-clear of #1236 — deliberately, with the user's word: an unprovable delivery may
+  not be allowed to delete the user's writing.
+- **A hand-off goes to an agent pane or to nothing.** With ≥2 live agent panes and no
+  recorded focus, the active pane is used only when it *is* one of them; otherwise the
+  first live agent pane wins. The legacy "no agent registry at all" fallback to the
+  active pane stays, and both fallbacks now log the pane they chose.
+
+## Risks
+
+- Reviewers who liked the self-clearing card must now press `Reset` once per round. It
+  is one click (the icon-only button in the card's title line) and it asks first.
+- Deterministically picking the first live agent pane can differ from the user's focus
+  in a multi-agent task where the focus hook never fired. Landing in the wrong *agent*
+  is recoverable; landing in a shell is not.
+
+## Alternatives considered
+
+- **Clear only on a proven `delivered`.** Would have required plumbing
+  `AgentPromptDelivery` to the renderer, and still deletes reviews in the common case,
+  because tmux says `delivered` even when the agent's input layer swallows the paste.
+- **Leave the routing alone and only fix the clearing.** The review would survive, but
+  the prompt would still silently land in a shell, so "I pressed send and the agent did
+  nothing" would keep happening with no signal.

docs/ux/PRODUCT_UX_BIBLE.md

@@ -200,8 +200,8 @@ Layout = left **Files aside** (collapsible, `22rem`) + right **diff stream**.
 - **Files aside** contains two cards: the **Review export card** (top) and the **Files card** (read-progress + expand/collapse-all + the file tree).
 - **Per-file header (diff stream):** status chip (A/M/D/R/C/T/?), path (click = expand/collapse), **copy-file-path** icon button (role `neutral`/icon), `+N/−N` stat pill, **mark-read** checkbox (success-tinted when read), expand/collapse caret. The path renders as **one truncating line at every width** (directory truncates, basename always survives) and the identity column carries a `basis-[15rem]` floor, so the trailing controls wrap onto a second row instead of squeezing the path into a one-character-per-line column.
 - **Inline comments:** drag across the gutter to select a line range (or use the hover `+` widget for a single line) → composer opens → comment is added to a per-file/per-side/per-line thread. Threads render inline and are editable/deletable in place.
-- **Composer actions (budget: 3):** `Cancel` (`neutral`), **`Send now`** (`secondary`), `Add comment` (`primary`). `Send now` is the one-shot lane: the comment is parked in the review, shipped to the agent, then dropped on success — a single remark costs one click instead of add → send → delete. A failed send leaves it in the review, so nothing typed is lost.
-- **Per-comment actions (budget: 3, no growth):** `Edit` (`neutral`), `Delete` (`destructive`), `Send to agent` (`secondary`) — the same pattern as the GitHub thread action, pushing that single comment into the task terminal. Sending is per comment and **sticky**: the comment is marked `Sent`, persisted with the review, and leaves the export payload, so `Copy review` and the batch `Send to Agent` cover **only unsent** comments. Sent comments stay visible and re-readable in the export card (greyed, `Sent` badge); editing one clears the mark so the edited text can be delivered again.
+- **Composer actions (budget: 3):** `Cancel` (`neutral`), **`Send now`** (`secondary`), `Add comment` (`primary`). `Send now` is the one-shot lane: the comment is parked in the review and shipped to the agent in one click instead of add → send. **No send ever deletes a comment** — a resolved send only proves the keys left dev3, so the text is marked `Sent` and kept; clearing stays the explicit, confirmed `Reset review` (decision `never-destroy-a-review-on-send`).
+- **Per-comment actions (budget: 3, no growth):** `Edit` (`neutral`), `Delete` (`destructive`), `Send to agent` (`secondary`) — the same pattern as the GitHub thread action, pushing that single comment into the task terminal. Sending is per comment and **sticky**: the comment is marked `Sent`, persisted with the review, and leaves the export payload, so `Copy review` and the batch `Send to Agent` cover **only unsent** comments. Sent comments stay visible and re-readable in the export card (greyed, `Sent` badge); editing one clears the mark so the edited text can be delivered again. The **batch** send marks its comments the same way rather than wiping the review — the pre-2026-08-10 auto-clear destroyed reviews whose delivery was never actually proven.
 
 **Review export card — action hierarchy (the one budgeted cluster):**
 

docs/ux/UX_DECISIONS.md

@@ -4,6 +4,12 @@ Compact index of UX architecture decisions — the *why* behind rules that live
 `PRODUCT_UX_BIBLE.md` / `ux-architecture.yaml`. Max ~5 lines per entry; details live in
 git history, PRs, and the records in `decisions/`. Newest first.
 
+## 2026-08-10 — A send marks the review, it never deletes it
+
+- **Rule:** No send path (per-comment, composer `Send now`, or batch) may clear inline review comments; they are stamped `Sent`, and only the confirmed `Reset review` destroys anything.
+- **Why:** A resolved send proves the keystrokes left dev3, not that the agent read them — a reviewer lost a whole review to a send that never landed; the rejected alternative, clearing only on a proven `delivered`, still deletes on the common tmux case where the agent's TUI swallows the paste.
+- **Status:** Implemented. Evidence: `src/mainview/components/TaskDiffViewer.tsx` (`markInlineCommentsSent`), `decisions/2026/08/10/never-destroy-a-review-on-send.md`.
+
 ## 2026-08-10 — Review export card is a title line; the composer owns the one-shot send
 
 - **Rule:** The export cluster (count + Copy + Send + icon-only Reset) sits on the card's title line, renders nothing when the review is empty, and the count is plain text — never a bordered box; the composer gains `Send now`, which delivers one remark and drops it.

src/bun/__tests__/agent-prompt.test.ts

@@ -166,14 +166,22 @@ describe("sendPromptToAgentPane — target resolution", () => {
 		expect(sentPane(0)).toBe("%1");
 	});
 
-	it("ignores a recorded pane that is live but not a registered agent pane", async () => {
-		// %3 is a live shell split, not in the agent registry.
+	it("never types into a focused shell split: falls back to a live agent pane", async () => {
+		// %3 is a live shell split, not in the agent registry. tmux would report the
+		// send as delivered either way, so aiming there loses the prompt silently.
 		vi.mocked(tmux.listPanes).mockResolvedValue([{ paneId: "%1" }, { paneId: "%2" }, { paneId: "%3" }] as never);
 		vi.mocked(tmux.showOption).mockResolvedValue("%3");
 		vi.mocked(tmux.activePaneId).mockResolvedValue("%3");
 		await runPrompt(sendPromptToAgentPane(TASK, "ping", TWO_AGENTS));
-		// No last-focused agent → ≥2 agents → active pane (%3, the focused shell).
-		expect(sentPane(0)).toBe("%3");
+		expect(sentPane(0)).toBe("%1");
+	});
+
+	it("keeps honouring the focus when the focused pane is itself an agent pane", async () => {
+		vi.mocked(tmux.listPanes).mockResolvedValue([{ paneId: "%1" }, { paneId: "%2" }, { paneId: "%3" }] as never);
+		vi.mocked(tmux.showOption).mockResolvedValue("");
+		vi.mocked(tmux.activePaneId).mockResolvedValue("%2");
+		await runPrompt(sendPromptToAgentPane(TASK, "ping", TWO_AGENTS));
+		expect(sentPane(0)).toBe("%2");
 	});
 
 	it("marks live agent panes with the focus-hook option (self-heal)", async () => {

src/bun/__tests__/rpc-handlers.test.ts

@@ -12885,8 +12885,9 @@ describe("handlers.createPullRequest", () => {
 		expect(sends[1]?.join(" ")).toContain("send-keys -t %5 Enter");
 	});
 
-	// With two or more agent panes the target is ambiguous, so respect focus.
-	it("routes to the active pane when there are two agent panes", async () => {
+	// With two or more agent panes the target is ambiguous — but a focused SHELL
+	// never wins it: tmux reports the send as delivered and the agent sees nothing.
+	it("routes to a live agent pane, not the focused shell, when there are two agent panes", async () => {
 		const project = makeProject();
 		const task = makeTask({
 			id: "task-1",
@@ -12904,10 +12905,10 @@ describe("handlers.createPullRequest", () => {
 
 		await handlers.createPullRequest({ taskId: "task-1", projectId: project.id });
 
-		expect(guardedSends()[0]?.join(" ")).toContain("send-keys -t %3 -H");
+		expect(guardedSends()[0]?.join(" ")).toContain("send-keys -t %5 -H");
 	});
 
-	it("routes to the active Codex main pane when its pane id is not persisted", async () => {
+	it("routes to the live agent pane when the Codex main pane id is not persisted", async () => {
 		const project = makeProject();
 		const task = makeTask({
 			id: "task-1",
@@ -12925,7 +12926,7 @@ describe("handlers.createPullRequest", () => {
 
 		await handlers.createPullRequest({ taskId: "task-1", projectId: project.id });
 
-		expect(guardedSends()[0]?.join(" ")).toContain("send-keys -t %3 -H");
+		expect(guardedSends()[0]?.join(" ")).toContain("send-keys -t %7 -H");
 	});
 
 	it("routes a legacy Codex main pane before a focused shell split", async () => {

src/bun/agent-prompt.ts

@@ -72,7 +72,9 @@ async function getLastFocusedAgentPane(tmuxSession: string, socket: string): Pro
  *    covers legacy tasks and the brief Codex pre-hook interval, when pane[0]'s
  *    ID has not been persisted yet but a shell split may be focused.
  *  - TWO OR MORE live agent panes with nothing recorded yet → respect the user's
- *    focus and use the session's active pane.
+ *    focus when the focused pane IS one of them, else the first live agent pane.
+ *    Never a shell split: tmux reports "delivered" for any pane, so a hand-off
+ *    typed into a shell reads as a clean send while the agent got nothing.
  *  - ZERO known agent panes (legacy tasks with no sessionState) → fall back to
  *    the active pane, preserving the historical behavior.
  *
@@ -112,9 +114,30 @@ export async function resolveAgentPromptTargetPane(
 		// recorded pane ID. Their session-state entry is pane[0], and tmux lists
 		// that initial pane first, so prefer it over an unrelated focused shell.
 		if (agentPanes?.length === 1 && hasUnresolvedAgentPane) return orderedLivePaneIds[0] ?? null;
-		// ≥2 or 0 live agent panes → fall through to the active pane below.
+		// ≥2 live agents and nothing recorded: honour the user's focus only when it
+		// IS one of them. Typing a hand-off into the focused shell used to look like
+		// a clean delivery (tmux takes the keys either way) while the agent never
+		// saw a word — and a review that clears itself on that "success" is gone.
+		if (liveAgentPanes.length > 0) {
+			if (activePane && liveAgentPanes.includes(activePane)) return activePane;
+			log.info("agent prompt routed to the first live agent pane", {
+				session: tmuxSession,
+				activePane: activePane ?? "none",
+				chosen: liveAgentPanes[0] ?? "none",
+				liveAgents: liveAgentPanes.length,
+			});
+			return liveAgentPanes[0] ?? null;
+		}
+		// No live agent pane at all → fall through to the active pane below.
 	}
 
+	// Legacy tasks with no agent registry: the active pane is the only guess there
+	// is. Logged, because it is also the one route that can land in a plain shell.
+	log.info("agent prompt falling back to the active pane (no live agent pane known)", {
+		session: tmuxSession,
+		activePane: activePane ?? "none",
+		registered: registeredIds.length,
+	});
 	return activePane;
 }
 

src/mainview/components/TaskDiffViewer.tsx

@@ -484,9 +484,9 @@ function hasAnyInlineComments(state: InlineDiffCommentsState): boolean {
 }
 
 /** Rewrites one comment anywhere in the review state, leaving every other entry untouched. */
-function mapInlineComment(
+function mapInlineComments(
 	state: InlineDiffCommentsState,
-	commentId: string,
+	matches: (comment: InlineDiffComment) => boolean,
 	transform: (comment: InlineDiffComment) => InlineDiffComment,
 ): InlineDiffCommentsState {
 	const nextState: InlineDiffCommentsState = {};
@@ -497,7 +497,7 @@ function mapInlineComment(
 				nextFileComments[side][lineNumber] = {
 					data: {
 						comments: thread.data.comments.map((comment) => (
-							comment.id === commentId ? transform(comment) : comment
+							matches(comment) ? transform(comment) : comment
 						)),
 					},
 				};
@@ -508,6 +508,12 @@ function mapInlineComment(
 	return nextState;
 }
 
+/** Stamp `sentAt` on every comment the caller just handed to the agent. */
+function markInlineCommentsSent(state: InlineDiffCommentsState, ids: ReadonlySet<string>): InlineDiffCommentsState {
+	const sentAt = new Date().toISOString();
+	return mapInlineComments(state, (comment) => ids.has(comment.id), (comment) => ({ ...comment, sentAt }));
+}
+
 function buildInlineReviewExportEntries(
 	files: TaskDiffFile[],
 	inlineComments: InlineDiffCommentsState,
@@ -2449,7 +2455,9 @@ function TaskDiffViewer({ task, project, request, onBack, navigationGuardRef }:
 		setSendingCommentIds((current) => ({ ...current, [commentId]: true }));
 		api.request.sendAgentMessageNow({ taskId: task.id, projectId: project.id, text: buildInlineReviewXml([entry]) })
 			.then((result) => {
-				deleteInlineComment(commentId);
+				// Marked, never deleted: a resolved RPC proves the keys were handed
+				// over, not that the agent ingested them, so the text must survive.
+				setInlineComments((current) => markInlineCommentsSent(current, new Set([commentId])));
 				toast.success(result?.spilledPath
 					? t("infoPanel.diffReviewSendCommentSuccessFile", { path: result.spilledPath })
 					: t("infoPanel.diffReviewSendCommentSuccess"), { taskId: task.id });
@@ -2495,10 +2503,7 @@ function TaskDiffViewer({ task, project, request, onBack, navigationGuardRef }:
 		setSendingCommentIds((current) => ({ ...current, [commentId]: true }));
 		api.request.sendAgentMessageNow({ taskId: task.id, projectId: project.id, text: prompt })
 			.then((result) => {
-				setInlineComments((current) => mapInlineComment(current, commentId, (comment) => ({
-					...comment,
-					sentAt: new Date().toISOString(),
-				})));
+				setInlineComments((current) => markInlineCommentsSent(current, new Set([commentId])));
 				toast.success(result?.spilledPath
 					? t("infoPanel.diffReviewSendCommentSuccessFile", { path: result.spilledPath })
 					: t("infoPanel.diffReviewSendCommentSuccess"), { taskId: task.id });
@@ -2528,13 +2533,15 @@ function TaskDiffViewer({ task, project, request, onBack, navigationGuardRef }:
 			return;
 		}
 		const snapshot = reviewExportXml;
+		const sentIds = new Set(pendingReviewExportEntries.filter((entry) => entry.origin === "local").map((entry) => entry.id));
 		setReviewSendState("sending");
 		api.request.sendAgentMessageNow({ taskId: task.id, projectId: project.id, text: snapshot })
 			.then((result) => {
 				setReviewSendState("sent");
-				// Delivered comments are dead weight: clear them here so the reviewer
-				// never has to run the destructive "Reset review" as a routine step.
-				setInlineComments({});
+				// Sent comments are marked, never destroyed. A resolved RPC only means
+				// the keys reached the terminal — the agent's TUI can still drop them —
+				// so wiping here silently ate reviews that never arrived.
+				setInlineComments((current) => markInlineCommentsSent(current, sentIds));
 				setEditingCommentId(null);
 				toast.success(result?.spilledPath
 					? t("infoPanel.diffReviewExportSendSuccessFile", { path: result.spilledPath })
@@ -2869,7 +2876,7 @@ function TaskDiffViewer({ task, project, request, onBack, navigationGuardRef }:
 
 		// Editing revives the comment: the agent got the old text, so the new text
 		// has to be deliverable again — via its own send or the next batch.
-		setInlineComments((current) => mapInlineComment(current, commentId, (comment) => ({
+		setInlineComments((current) => mapInlineComments(current, (comment) => comment.id === commentId, (comment) => ({
 			...comment,
 			body: trimmedBody,
 			sentAt: undefined,