fix: send review to terminal respects repo default agent setting

scripts/screenshot/specs/send-review-to-terminal-default-agent.spec.tsx

@@ -0,0 +1,117 @@
+/**
+ * Verifies that "Send review to terminal" (the Plan/Edit buttons in the Finish
+ * Review popover) respects the repo-level default_agent setting.  Before this
+ * fix, a review was always sent to a Claude terminal even when the repo had
+ * codex set as its default agent.  After the fix, clicking "Plan" should open
+ * a Codex terminal (Sparkles icon) instead of a Claude terminal (Bot icon).
+ */
+
+import * as React from "react";
+import { expect, it } from "vitest";
+import userEvent from "@testing-library/user-event";
+import { createTestRepo, openRepo, resolveWorkspacePath, writeWorkspaceFile } from "../../../test/utils";
+import { render, screen, waitFor } from "../../../test/test-utils";
+import { Dashboard } from "../../../src/components/Dashboard";
+import {
+	createWorkspace,
+	getWorkspaces,
+	setRepoSetting,
+} from "../../../src/lib/api";
+import { captureDocument } from "../capture";
+
+const BRANCH_NAME = "feat/codex-review-agent";
+
+it("send review to terminal opens a codex terminal when repo default_agent=codex", async () => {
+	const { repoPath } = createTestRepo(false);
+	openRepo(repoPath);
+
+	// Background state: set repo default agent to codex BEFORE the review is
+	// submitted. handleCreateAgentWithReview reads this setting when the user
+	// clicks Plan/Edit, so it just needs to be set before that click.
+	await setRepoSetting(repoPath, "default_agent", "codex");
+
+	// Create workspace via API (incidental — not the behavior under test).
+	await createWorkspace(repoPath, BRANCH_NAME);
+	const workspaces = await getWorkspaces(repoPath);
+	const workspace = workspaces.find((w) => w.branch_name === BRANCH_NAME);
+	if (!workspace) throw new Error(`workspace ${BRANCH_NAME} not found`);
+
+	// Write an uncommitted file so the Review tab has a diff to show.
+	writeWorkspaceFile(
+		resolveWorkspacePath(repoPath, workspace.workspace_path),
+		"example.ts",
+		'export const answer = 42;\n',
+	);
+
+	const user = userEvent.setup();
+	render(<Dashboard />);
+
+	// Navigate to the workspace by clicking its name in the sidebar.
+	await user.click(await screen.findByText(BRANCH_NAME));
+	await screen.findByTestId("show-workspace-header");
+
+	// Open the Review tab.
+	await user.click(await screen.findByRole("tab", { name: /^Review/i }));
+	// The file appears in both sidebar and diff header — wait for any occurrence.
+	await screen.findAllByText("example.ts");
+
+	// The diff is already expanded; click "Add comment" on the first hunk line.
+	// Use findAll to handle one button per diff line without ambiguity.
+	const addCommentBtns = await screen.findAllByRole("button", {
+		name: /add comment/i,
+	});
+	await user.click(addCommentBtns[0]);
+
+	// Type a comment and submit it.
+	await user.type(
+		await screen.findByPlaceholderText(/add a comment/i),
+		"Fix this",
+	);
+	// The textarea is inside a form that has a submit button also labelled
+	// "Add Comment". Find the one with exact textContent to avoid ambiguity.
+	await waitFor(async () => {
+		const btns = screen.getAllByRole("button", { name: /add comment/i });
+		const submitBtn = btns.find(
+			(btn) => btn.textContent?.trim() === "Add Comment",
+		);
+		expect(submitBtn).toBeDefined();
+		await user.click(submitBtn!);
+	});
+	await screen.findByText("Fix this");
+
+	// Open the Finish Review popover.
+	await user.click(await screen.findByRole("button", { name: /finish review/i }));
+
+	await captureDocument(document, {
+		name: "send-review-codex-01-popover",
+		expectations: [
+			'The "Finish review" popover is open showing the "Finish your review" heading.',
+			'Three action buttons are visible: "Copy" on the left, "Plan" and "Edit" on the right.',
+			'The diff pane for example.ts is visible in the background.',
+		],
+	});
+
+	// Click "Plan" — this triggers handleCreateAgentWithReview which should
+	// pick up default_agent=codex and pass agent:"codex" to onSessionCreated.
+	await user.click(await screen.findByRole("button", { name: /^plan$/i }));
+
+	// Wait for the terminal pane to open with the new session. The pane
+	// uncollapse when a session is added, so any terminal-pane element appearing
+	// is the signal we want.
+	await waitFor(
+		async () => {
+			// The session tab for the review session should be present.
+			expect(screen.getByText("Code Review")).toBeInTheDocument();
+		},
+		{ timeout: 10000 },
+	);
+
+	await captureDocument(document, {
+		name: "send-review-codex-02-terminal",
+		expectations: [
+			'The terminal pane at the bottom is open.',
+			'A session tab labelled "Code Review" is visible in the terminal pane.',
+			'The "Code Review" tab has a Sparkles (✨) icon — the codex agent icon — not a Bot icon (which would indicate the claude agent was launched instead).',
+		],
+	});
+}, 60000);

src-tauri/tests/core_logging_test.rs

@@ -11,6 +11,15 @@ use tracing_subscriber::layer::SubscriberExt;
 use tracing_subscriber::Registry;
 use treq_lib::telemetry::{cleanup_old_logs, forward_log_record, FileLogExporter};
 
+/// True for an OTLP JSON record produced by one of this test's own
+/// `forward_log_record` calls, as opposed to a self-diagnostic record the
+/// OTel SDK emits through the same pipeline (e.g. on `LoggerProvider` drop).
+fn is_forwarded_record(v: &Value) -> bool {
+    v["resourceLogs"][0]["scopeLogs"][0]["logRecords"][0]["body"]["stringValue"]
+        .as_str()
+        .is_some_and(|body| body.starts_with("message at "))
+}
+
 /// Drive the real logging pipeline to produce a file on disk, then rewind its
 /// mtime to simulate age — much closer to what we ship than `fs::write` of
 /// arbitrary bytes.
@@ -175,8 +184,33 @@ fn forward_log_record_emits_otlp_json_per_level() {
         .find(|e| e.file_name().to_string_lossy().starts_with("treq."))
         .expect("rolling appender wrote a file");
 
-    let contents = std::fs::read_to_string(log_file.path()).expect("read log file");
-    let lines: Vec<&str> = contents.lines().filter(|l| !l.is_empty()).collect();
+    // `WorkerGuard`'s drop only waits a bounded amount of time for the
+    // non-blocking writer's background thread to flush; under heavy CI load
+    // that window can be missed even though the record was queued. Poll
+    // instead of assuming the flush already landed by the time we read.
+    //
+    // The SDK also emits its own self-diagnostic record ("Last reference of
+    // LoggerProvider dropped, initiating shutdown.") through this same
+    // pipeline when `provider` is dropped. It isn't one of the five records
+    // this test forwarded, so filter to lines carrying our own message body
+    // rather than asserting on the raw line count.
+    let is_own_record =
+        |line: &&str| serde_json::from_str::<Value>(line).is_ok_and(|v| is_forwarded_record(&v));
+
+    let mut contents = String::new();
+    let mut lines: Vec<&str> = Vec::new();
+    for _ in 0..50 {
+        contents = std::fs::read_to_string(log_file.path()).expect("read log file");
+        lines = contents
+            .lines()
+            .filter(|l| !l.is_empty())
+            .filter(is_own_record)
+            .collect();
+        if lines.len() >= 5 {
+            break;
+        }
+        std::thread::sleep(Duration::from_millis(20));
+    }
     assert_eq!(
         lines.len(),
         5,

src/components/ShowWorkspace.committed-toggle.test.tsx

@@ -35,6 +35,7 @@ vi.mock("../lib/api", async () => {
 	return {
 		...actual,
 		getSetting: vi.fn().mockResolvedValue(null),
+		getRepoSetting: vi.fn().mockResolvedValue(null),
 		lsWorkspace: vi.fn().mockResolvedValue([]),
 		getWorkspaceReadme: vi.fn().mockResolvedValue(null),
 		jjGetDefaultBranch: vi.fn().mockResolvedValue("main"),