diff --git a/scripts/screenshot/specs/send-review-to-terminal-default-agent.spec.tsx b/scripts/screenshot/specs/send-review-to-terminal-default-agent.spec.tsx new file mode 100644 index 00000000..288b3bc8 --- /dev/null +++ b/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(); + + // 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); diff --git a/src-tauri/tests/core_logging_test.rs b/src-tauri/tests/core_logging_test.rs index db049a29..e0c4383a 100644 --- a/src-tauri/tests/core_logging_test.rs +++ b/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::(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, diff --git a/src/components/ShowWorkspace.committed-toggle.test.tsx b/src/components/ShowWorkspace.committed-toggle.test.tsx index 0188463e..6119e1cf 100644 --- a/src/components/ShowWorkspace.committed-toggle.test.tsx +++ b/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"), diff --git a/src/components/ShowWorkspace.review-agent.test.tsx b/src/components/ShowWorkspace.review-agent.test.tsx new file mode 100644 index 00000000..e1c97c41 --- /dev/null +++ b/src/components/ShowWorkspace.review-agent.test.tsx @@ -0,0 +1,229 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import { render, screen, waitFor } from "../../test/test-utils"; +import { ShowWorkspace } from "./ShowWorkspace"; +import type { Workspace } from "../lib/api"; +import type { SessionCreationInfo } from "../types/sessions"; + +// Capture the onCreateAgentWithReview callback so tests can invoke it directly. +let capturedOnCreateAgentWithReview: + | ((review: string, mode: "plan" | "acceptEdits") => Promise) + | undefined; + +vi.mock("./FileBrowser", () => ({ + FileBrowser: () =>
, +})); + +vi.mock("./LinearCommitHistory", () => ({ + LinearCommitHistory: () =>
, +})); + +vi.mock("./ChangesDiffViewer", () => ({ + ChangesDiffViewer: ({ + onCreateAgentWithReview, + }: { + onCreateAgentWithReview?: ( + review: string, + mode: "plan" | "acceptEdits", + ) => Promise; + }) => { + capturedOnCreateAgentWithReview = onCreateAgentWithReview; + return
; + }, +})); + +vi.mock("./TargetBranchSelector", () => ({ + TargetBranchSelector: () =>
, +})); + +vi.mock("../lib/api", async () => { + const actual = + await vi.importActual("../lib/api"); + 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"), + listConflictedFiles: vi.fn().mockResolvedValue([]), + jjGetBranches: vi.fn().mockResolvedValue([]), + setWorkspaceTargetBranch: vi.fn().mockResolvedValue(undefined), + jjGetChangedFiles: vi.fn().mockResolvedValue([]), + createSession: vi.fn().mockResolvedValue(42), + ptyCreateSession: vi.fn().mockResolvedValue(undefined), + ptyWrite: vi.fn().mockResolvedValue(undefined), + checkAndRebaseWorkspaces: vi.fn().mockResolvedValue({ + rebased: false, + success: true, + has_conflicts: false, + conflicted_files: [], + message: "No rebase needed", + bookmark_conflicts: [], + }), + resolveBookmarkConflict: vi.fn().mockResolvedValue({ + success: true, + message: "Resolved", + }), + listCommits: vi.fn().mockResolvedValue({ + commits: [], + target_branch: "main", + workspace_branch: "feature-one", + }), + }; +}); + +const workspace: Workspace = { + id: 7, + repo_path: "/Users/test/repo", + workspace_name: "feature-one", + workspace_path: "/Users/test/repo/.treq/workspaces/feature-one", + branch_name: "feature-one", + title: "feature-one", + created_at: new Date().toISOString(), + not_on_remote: false, +}; + +function renderWorkspace(onSessionCreated: (s: SessionCreationInfo) => void) { + capturedOnCreateAgentWithReview = undefined; + return render( + , + ); +} + +async function openReviewTab() { + const user = userEvent.setup(); + const reviewTab = await screen.findByRole("tab", { name: /review/i }); + await user.click(reviewTab); + await screen.findByTestId("changes-viewer"); + await waitFor(() => expect(capturedOnCreateAgentWithReview).toBeDefined()); +} + +describe("Send review to terminal respects default agent setting", () => { + beforeEach(() => { + vi.clearAllMocks(); + capturedOnCreateAgentWithReview = undefined; + }); + + it("passes agent=undefined when no default agent is configured", async () => { + const api = await import("../lib/api"); + vi.mocked(api.getRepoSetting).mockResolvedValue(null); + vi.mocked(api.getSetting).mockResolvedValue(null); + + const onSessionCreated = vi.fn(); + renderWorkspace(onSessionCreated); + + await openReviewTab(); + + await capturedOnCreateAgentWithReview!("review text", "plan"); + + expect(onSessionCreated).toHaveBeenCalledOnce(); + const [sessionInfo] = onSessionCreated.mock.calls[0] as [ + SessionCreationInfo, + ]; + expect(sessionInfo.agent).toBeUndefined(); + }); + + it("passes agent=codex when repo default_agent is codex", async () => { + const api = await import("../lib/api"); + vi.mocked(api.getRepoSetting).mockResolvedValue("codex"); + vi.mocked(api.getSetting).mockResolvedValue(null); + + const onSessionCreated = vi.fn(); + renderWorkspace(onSessionCreated); + + await openReviewTab(); + + await capturedOnCreateAgentWithReview!("review text", "plan"); + + expect(onSessionCreated).toHaveBeenCalledOnce(); + const [sessionInfo] = onSessionCreated.mock.calls[0] as [ + SessionCreationInfo, + ]; + expect(sessionInfo.agent).toBe("codex"); + }); + + it("passes agent=cursor when repo default_agent is cursor", async () => { + const api = await import("../lib/api"); + vi.mocked(api.getRepoSetting).mockResolvedValue("cursor"); + vi.mocked(api.getSetting).mockResolvedValue(null); + + const onSessionCreated = vi.fn(); + renderWorkspace(onSessionCreated); + + await openReviewTab(); + + await capturedOnCreateAgentWithReview!("review text", "acceptEdits"); + + expect(onSessionCreated).toHaveBeenCalledOnce(); + const [sessionInfo] = onSessionCreated.mock.calls[0] as [ + SessionCreationInfo, + ]; + expect(sessionInfo.agent).toBe("cursor"); + }); + + it("repo setting takes precedence over app-level default_agent", async () => { + const api = await import("../lib/api"); + vi.mocked(api.getRepoSetting).mockResolvedValue("codex"); + vi.mocked(api.getSetting).mockResolvedValue("cursor"); + + const onSessionCreated = vi.fn(); + renderWorkspace(onSessionCreated); + + await openReviewTab(); + + await capturedOnCreateAgentWithReview!("review text", "plan"); + + expect(onSessionCreated).toHaveBeenCalledOnce(); + const [sessionInfo] = onSessionCreated.mock.calls[0] as [ + SessionCreationInfo, + ]; + expect(sessionInfo.agent).toBe("codex"); + }); + + it("falls back to app-level default_agent when no repo setting", async () => { + const api = await import("../lib/api"); + vi.mocked(api.getRepoSetting).mockResolvedValue(null); + vi.mocked(api.getSetting).mockResolvedValue("codex"); + + const onSessionCreated = vi.fn(); + renderWorkspace(onSessionCreated); + + await openReviewTab(); + + await capturedOnCreateAgentWithReview!("review text", "plan"); + + expect(onSessionCreated).toHaveBeenCalledOnce(); + const [sessionInfo] = onSessionCreated.mock.calls[0] as [ + SessionCreationInfo, + ]; + expect(sessionInfo.agent).toBe("codex"); + }); + + it("includes the review markdown in the pending prompt", async () => { + const api = await import("../lib/api"); + vi.mocked(api.getRepoSetting).mockResolvedValue("codex"); + vi.mocked(api.getSetting).mockResolvedValue(null); + + const onSessionCreated = vi.fn(); + renderWorkspace(onSessionCreated); + + await openReviewTab(); + + const reviewText = "## Code Review\n\nPlease fix the bug."; + await capturedOnCreateAgentWithReview!(reviewText, "plan"); + + const [sessionInfo] = onSessionCreated.mock.calls[0] as [ + SessionCreationInfo, + ]; + expect(sessionInfo.pendingPrompt).toBe(reviewText); + expect(sessionInfo.permissionMode).toBe("plan"); + }); +}); diff --git a/src/components/ShowWorkspace.tsx b/src/components/ShowWorkspace.tsx index 017560dd..7092055e 100644 --- a/src/components/ShowWorkspace.tsx +++ b/src/components/ShowWorkspace.tsx @@ -40,6 +40,8 @@ import { type DirectoryEntry, discardWorkspaceChanges, dryRunHomeRepoRebase, + getRepoSetting, + getSetting, getWorkspaceReadme, getWorkspaceStatus, type HomeRebaseDryRunResult, @@ -861,6 +863,38 @@ export const ShowWorkspace = memo( try { const sessionName = "Code Review"; + // Resolve the default agent from repo-level then app-level settings, + // so "send review to terminal" honours the configured default agent. + let resolvedAgent: "claude" | "codex" | "cursor" | undefined; + const repoPathForSettings = effectiveRepoPath || workingDirectory; + try { + let repoDefault: string | null = null; + let appDefault: string | null = null; + try { + repoDefault = await getRepoSetting( + repoPathForSettings, + "default_agent", + ); + } catch { + // repo may not be initialized yet + } + try { + appDefault = await getSetting("default_agent"); + } catch { + // ignore + } + const defaultAgent = repoDefault || appDefault; + if ( + defaultAgent === "codex" || + defaultAgent === "cursor" || + defaultAgent === "claude" + ) { + resolvedAgent = defaultAgent; + } + } catch { + // fall back to undefined (Dashboard will default to claude) + } + // Create new database session const dbSessionId = await createSession( effectiveRepoPath, @@ -869,7 +903,7 @@ export const ShowWorkspace = memo( ); const sessionRepoPath = effectiveRepoPath || workingDirectory; - // Notify parent with pending prompt to be sent after Claude initializes + // Notify parent with pending prompt to be sent after agent initializes // (ConsolidatedTerminal will create the PTY session when it mounts) onSessionCreated?.({ sessionId: dbSessionId, @@ -879,6 +913,7 @@ export const ShowWorkspace = memo( repoPath: sessionRepoPath, pendingPrompt: reviewMarkdown, permissionMode: mode, + agent: resolvedAgent, }); } catch (error) { addToast({ diff --git a/test/ShowWorkspace.test.tsx b/test/ShowWorkspace.test.tsx index a8629743..7aae11c5 100644 --- a/test/ShowWorkspace.test.tsx +++ b/test/ShowWorkspace.test.tsx @@ -60,6 +60,7 @@ vi.mock("../src/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"),