From 019ebb4730c5c0030c9312b9df1a9705d405c03a Mon Sep 17 00:00:00 2001 From: Serhii Vecherenko Date: Sun, 14 Jun 2026 17:04:57 -0700 Subject: [PATCH] fix: recover draft launch failures and qualify worktree refs - Re-enable the composer if `onStart` rejects so failed launches do not leave the draft spinner stuck. - Qualify bare remote-tracking start points before `git worktree add` and save the resolved source ref. - Cover both paths with new renderer and supervisor tests. --- .../thread/ThreadDraftComposerArea.tsx | 18 +- .../thread/ThreadDraftView.test.tsx | 29 ++++ .../components/thread/ThreadDraftView.tsx | 2 +- .../MainView/parts/AppContent/AppContent.tsx | 37 ++-- .../parts/AppContent/parts/DraftPane.tsx | 19 +-- src/supervisor/git.test.ts | 161 ++++++++++++++++++ src/supervisor/git/worktreeService.ts | 62 ++++++- 7 files changed, 280 insertions(+), 48 deletions(-) diff --git a/src/renderer/components/thread/ThreadDraftComposerArea.tsx b/src/renderer/components/thread/ThreadDraftComposerArea.tsx index 97140ab3..5b161ac2 100644 --- a/src/renderer/components/thread/ThreadDraftComposerArea.tsx +++ b/src/renderer/components/thread/ThreadDraftComposerArea.tsx @@ -193,7 +193,7 @@ export function ThreadDraftComposerArea(props: { onWorktreeModeChange: (worktreeMode: boolean) => void; onSwitchBranch: (branch: string, createNew: boolean) => void; onRememberPresentationMode: () => void; - onStart: (input: DraftStartInput) => void; + onStart: (input: DraftStartInput) => void | Promise; }) { const [prompt, setPrompt] = useState(""); const [hasContent, setHasContent] = useState(false); @@ -393,7 +393,7 @@ export function ThreadDraftComposerArea(props: { if (props.supportsModePicker) { props.onRememberPresentationMode(); } - props.onStart({ + const startResult = props.onStart({ agentKind: props.selectedAgent.kind, config: props.config, prompt: flatPrompt, @@ -415,6 +415,20 @@ export function ThreadDraftComposerArea(props: { } : {}), }); + // On success the draft pane unmounts (replaced by the launched thread), so + // this state never matters. On failure (e.g. worktree creation errored) the + // pane stays mounted — re-enable the composer instead of leaving it stuck on + // the launch spinner with the user's prompt trapped behind it. `onStart` may + // return void or a promise; Promise.resolve normalizes both. + void Promise.resolve(startResult).catch(() => { + submittedRef.current = false; + // resetDraftRefs() above cleared the snapshot the unmount-cleanup save + // reads. The prompt is still in the editor, so re-capture it — otherwise + // navigating away without another edit would silently drop it. + latestSegmentsRef.current = mentionRef.current?.serializeSegments() ?? []; + attachmentsRef.current = attachments.attachments; + setIsSubmitting(false); + }); } useLayoutEffect(() => { diff --git a/src/renderer/components/thread/ThreadDraftView.test.tsx b/src/renderer/components/thread/ThreadDraftView.test.tsx index 3b1bf46e..3047a4d7 100644 --- a/src/renderer/components/thread/ThreadDraftView.test.tsx +++ b/src/renderer/components/thread/ThreadDraftView.test.tsx @@ -528,6 +528,35 @@ describe("ThreadDraftView", () => { }); }); + it("re-enables the composer when onStart rejects (e.g. worktree creation fails)", async () => { + const onStart = vi.fn<(input: unknown) => void | Promise>(() => + Promise.reject(new Error("worktree creation failed")), + ); + + render( + , + ); + + await waitFor(() => { + const props = composerSpy.mock.lastCall?.[0] as { controls: Array<{ kind?: string }> }; + expect(props.controls.some((c) => c.kind === "provider-model")).toBe(true); + }); + + fireEvent.click(screen.getByText("set-prompt")); + await act(async () => { + fireEvent.click(screen.getByText("submit")); + }); + + expect(onStart).toHaveBeenCalledTimes(1); + + // Once the rejection settles the composer is interactive again rather than + // frozen on the launch spinner with the prompt trapped behind it. + await waitFor(() => { + const props = composerSpy.mock.lastCall?.[0] as { submitPending?: boolean }; + expect(props.submitPending).toBe(false); + }); + }); + it("defaults Home Codex drafts to provider defaults, same as any other project", async () => { const onStart = vi.fn<(input: unknown) => void>(); useSharedSettings.setState({ diff --git a/src/renderer/components/thread/ThreadDraftView.tsx b/src/renderer/components/thread/ThreadDraftView.tsx index 54b1bcdc..d0ca82e3 100644 --- a/src/renderer/components/thread/ThreadDraftView.tsx +++ b/src/renderer/components/thread/ThreadDraftView.tsx @@ -164,7 +164,7 @@ export function ThreadDraftView(props: { droppableRef?: React.RefObject; onClose?: (() => void) | undefined; dragHandleRef?: React.RefCallback; - onStart: (input: DraftStartInput) => void; + onStart: (input: DraftStartInput) => void | Promise; }) { const { project, diff --git a/src/renderer/views/MainView/parts/AppContent/AppContent.tsx b/src/renderer/views/MainView/parts/AppContent/AppContent.tsx index c114ea8f..24d24e8d 100644 --- a/src/renderer/views/MainView/parts/AppContent/AppContent.tsx +++ b/src/renderer/views/MainView/parts/AppContent/AppContent.tsx @@ -2,7 +2,6 @@ import { useShallow } from "zustand/shallow"; import { X } from "lucide-react"; import { toast } from "@heroui/react"; import type { - AgentStatus, ExtractContextResult, Project, PromptSegment, @@ -11,6 +10,7 @@ import type { ThreadPresentationMode, } from "@/shared/contracts"; import { getProjectAgentStatuses } from "@/shared/agentStatus"; +import { friendlyError } from "@/shared/messages"; import { isHomeProject } from "@/shared/homeScope"; import { buildWorktreeLocation } from "@/shared/worktree"; import { isDraftPaneId, parseDraftProjectId } from "@/shared/paneId"; @@ -34,6 +34,7 @@ import { closeAllPanels } from "@/renderer/actions/panelActions"; import { SplitPaneContainer, type Rect } from "@/renderer/components/layout/SplitPaneContainer"; import { macosTrafficLightPadClass } from "@/renderer/components/layout/sidebarChrome"; import { ThreadDraftView } from "@/renderer/components/thread/ThreadDraftView"; +import type { DraftStartInput } from "@/renderer/components/thread/ThreadDraftComposerArea"; import { normalizeShellScript, startShellWithToast, @@ -62,18 +63,7 @@ export function AppContent() { }); async function handleDraftStart( project: Project, - input: { - agentKind: AgentStatus["kind"]; - config: import("@/shared/contracts").ThreadConfig; - prompt: string; - segments?: PromptSegment[]; - existingWorktreePath?: string; - worktreeBranch?: string; - worktreeBaseBranch?: string; - worktreeIsNewBranch?: boolean; - worktreeTransferUncommitted?: boolean; - presentationMode?: import("@/shared/contracts").ThreadPresentationMode; - }, + input: DraftStartInput, replacePaneIdParam?: string, ) { const { @@ -127,7 +117,10 @@ export function AppContent() { } } catch (err) { console.error("[renderer] failed to create worktree:", err); - return; + // Surface the failure and propagate so the composer can re-enable + // itself — otherwise it stays stuck on the launch spinner forever. + toast.danger(friendlyError(err)); + throw err; } } @@ -288,7 +281,7 @@ export function AppContent() { void handleDraftStart(draftProject, input)} + onStart={(input) => handleDraftStart(draftProject, input)} /> ); @@ -333,7 +326,7 @@ export function AppContent() { paneAlign={paneAlign} headerNeedsTrafficLightPad={headerNeedsTrafficLightPad} onClose={() => closePane(paneId)} - onStart={(project, input) => void handleDraftStart(project, input, paneId)} + onStart={(project, input) => handleDraftStart(project, input, paneId)} /> ) : ( void; + onStart: (input: DraftStartInput) => void | Promise; }) { const { project, lastDraftConfig, onStart } = props; const projectAgentStatuses = useAgentStatusesStore( diff --git a/src/renderer/views/MainView/parts/AppContent/parts/DraftPane.tsx b/src/renderer/views/MainView/parts/AppContent/parts/DraftPane.tsx index 915a3565..2c9a435c 100644 --- a/src/renderer/views/MainView/parts/AppContent/parts/DraftPane.tsx +++ b/src/renderer/views/MainView/parts/AppContent/parts/DraftPane.tsx @@ -1,5 +1,5 @@ import { useRef } from "react"; -import type { AgentStatus, Project, PromptSegment } from "@/shared/contracts"; +import type { Project } from "@/shared/contracts"; import { getProjectAgentStatuses } from "@/shared/agentStatus"; import { isDetectingAgentsForLocation, @@ -10,6 +10,7 @@ import { useProjectWithoutDraftConfig, } from "@/renderer/state/useThread"; import { ThreadDraftView } from "@/renderer/components/thread/ThreadDraftView"; +import type { DraftStartInput } from "@/renderer/components/thread/ThreadDraftComposerArea"; import { useDraggable, useDroppable } from "@dnd-kit/react"; import { useShallow } from "zustand/shallow"; import { useIsDraggingPane, usePaneDropIndicatorState, type DragSourceData } from "@/renderer/dnd"; @@ -21,21 +22,7 @@ export function DraftPane(props: { paneAlign: "left" | "center" | "right"; headerNeedsTrafficLightPad?: boolean; onClose: () => void; - onStart: ( - project: Project, - input: { - agentKind: AgentStatus["kind"]; - config: import("@/shared/contracts").ThreadConfig; - prompt: string; - segments?: PromptSegment[]; - existingWorktreePath?: string; - worktreeBranch?: string; - worktreeBaseBranch?: string; - worktreeIsNewBranch?: boolean; - worktreeTransferUncommitted?: boolean; - presentationMode?: import("@/shared/contracts").ThreadPresentationMode; - }, - ) => void; + onStart: (project: Project, input: DraftStartInput) => void | Promise; }) { const project = useProjectWithoutDraftConfig(props.projectId); const initialLastDraftConfig = useInitialProjectDraftConfig(props.projectId); diff --git a/src/supervisor/git.test.ts b/src/supervisor/git.test.ts index 9c1a0368..96c7acfe 100644 --- a/src/supervisor/git.test.ts +++ b/src/supervisor/git.test.ts @@ -200,6 +200,167 @@ describe("GitService.addWorktree", () => { expect(configCall![1]).toContain("master"); }); + it("qualifies a bare remote-tracking branch start point with its remote", async () => { + // The composer passes a remote branch's short name (no "origin/" prefix) as + // the base. On its own that is not a valid object, so the worktree add must + // fork from the qualified `origin/` ref instead. + mockGitCommands((args) => { + if (args[0] === "worktree" && args[1] === "add") return { stdout: "" }; + if (args[0] === "remote") return { stdout: "origin\n" }; + if (args[0] === "rev-parse") { + const ref = args[args.length - 1]; + // The bare name does not resolve; only the qualified remote ref does. + if (ref === "refs/remotes/origin/lightcode/silver-meadow-abcd") return { stdout: "sha\n" }; + return { error: new Error("fatal: Needed a single revision") }; + } + if (args[0] === "config") return { stdout: "" }; + return { stdout: "" }; + }); + + await new GitService().addWorktree( + location, + "C:\\Users\\demo\\.lightcode\\worktrees\\lightcode-12345678\\lightcode-brave-heron", + "lightcode/brave-heron", + true, + "lightcode/silver-meadow-abcd", + ); + + const commands = execFileMock.mock.calls.map((c: unknown[]) => (c[1] as string[]).join(" ")); + expect( + commands.some((c) => + c.includes( + "worktree add -b lightcode/brave-heron " + + "C:\\Users\\demo\\.lightcode\\worktrees\\lightcode-12345678\\lightcode-brave-heron " + + "origin/lightcode/silver-meadow-abcd", + ), + ), + ).toBe(true); + + // The recorded source branch is the qualified ref, so diff bases line up. + const configCall = execFileMock.mock.calls.find( + (call: unknown[]) => + Array.isArray(call[1]) && + (call[1] as string[])[0] === "config" && + (call[1] as string[]).includes("branch.lightcode/brave-heron.lightcodeSource"), + ); + expect(configCall).toBeDefined(); + expect(configCall![1]).toContain("origin/lightcode/silver-meadow-abcd"); + }); + + it("leaves a start point untouched when it resolves locally", async () => { + const revParseRefs: string[] = []; + mockGitCommands((args) => { + if (args[0] === "worktree" && args[1] === "add") return { stdout: "" }; + if (args[0] === "rev-parse") { + revParseRefs.push(args[args.length - 1]!); + return { stdout: "sha\n" }; // resolves directly — no remote lookup needed + } + if (args[0] === "config") return { stdout: "" }; + return { stdout: "" }; + }); + + await new GitService().addWorktree( + location, + "C:\\Users\\demo\\.lightcode\\worktrees\\lightcode-12345678\\lightcode-brave-heron", + "lightcode/brave-heron", + true, + "main", + ); + + const commands = execFileMock.mock.calls.map((c: unknown[]) => (c[1] as string[]).join(" ")); + expect( + commands.some( + (c) => c.includes("worktree add -b lightcode/brave-heron") && c.endsWith("main"), + ), + ).toBe(true); + // A resolvable start point short-circuits before any `git remote` lookup. + expect(commands.some((c) => c === "remote")).toBe(false); + expect(revParseRefs).toEqual(["main"]); + }); + + it("qualifies with the single matching remote when several remotes are configured", async () => { + // Two remotes exist, but only `upstream` carries the branch — qualify with it. + mockGitCommands((args) => { + if (args[0] === "worktree" && args[1] === "add") return { stdout: "" }; + if (args[0] === "remote") return { stdout: "origin\nupstream\n" }; + if (args[0] === "rev-parse") { + const ref = args[args.length - 1]; + if (ref === "refs/remotes/upstream/feature/x") return { stdout: "sha\n" }; + return { error: new Error("fatal: Needed a single revision") }; + } + if (args[0] === "config") return { stdout: "" }; + return { stdout: "" }; + }); + + await new GitService().addWorktree( + location, + "C:\\Users\\demo\\.lightcode\\worktrees\\lightcode-12345678\\lightcode-brave-heron", + "lightcode/brave-heron", + true, + "feature/x", + ); + + const wtAdd = execFileMock.mock.calls + .map((c: unknown[]) => (c[1] as string[]).join(" ")) + .find((c) => c.startsWith("worktree add -b lightcode/brave-heron")); + expect(wtAdd?.endsWith("upstream/feature/x")).toBe(true); + }); + + it("prefers origin when the branch name exists on multiple remotes", async () => { + // Both `origin` and `upstream` carry the name — prefer origin, matching the + // module's origin-centric diff-base handling, rather than failing as ambiguous. + mockGitCommands((args) => { + if (args[0] === "worktree" && args[1] === "add") return { stdout: "" }; + if (args[0] === "remote") return { stdout: "upstream\norigin\n" }; + if (args[0] === "rev-parse") { + const ref = args[args.length - 1]; + if (ref === "refs/remotes/origin/feature/x" || ref === "refs/remotes/upstream/feature/x") { + return { stdout: "sha\n" }; + } + return { error: new Error("fatal: Needed a single revision") }; + } + if (args[0] === "config") return { stdout: "" }; + return { stdout: "" }; + }); + + await new GitService().addWorktree( + location, + "C:\\Users\\demo\\.lightcode\\worktrees\\lightcode-12345678\\lightcode-brave-heron", + "lightcode/brave-heron", + true, + "feature/x", + ); + + const wtAdd = execFileMock.mock.calls + .map((c: unknown[]) => (c[1] as string[]).join(" ")) + .find((c) => c.startsWith("worktree add -b lightcode/brave-heron")); + expect(wtAdd?.endsWith("origin/feature/x")).toBe(true); + }); + + it("forks a local branch directly when its name also exists on a remote", async () => { + // The bare name resolves locally, so the worktree forks the local branch and + // never consults remotes (no qualification, no `git remote` call). + mockGitCommands((args) => { + if (args[0] === "worktree" && args[1] === "add") return { stdout: "" }; + if (args[0] === "rev-parse") return { stdout: "sha\n" }; // resolves locally + if (args[0] === "config") return { stdout: "" }; + return { stdout: "" }; + }); + + await new GitService().addWorktree( + location, + "C:\\Users\\demo\\.lightcode\\worktrees\\lightcode-12345678\\lightcode-brave-heron", + "lightcode/brave-heron", + true, + "feature/x", + ); + + const commands = execFileMock.mock.calls.map((c: unknown[]) => (c[1] as string[]).join(" ")); + const wtAdd = commands.find((c) => c.startsWith("worktree add -b lightcode/brave-heron")); + expect(wtAdd?.endsWith("feature/x")).toBe(true); + expect(commands.some((c) => c === "remote")).toBe(false); + }); + it("resolves default WSL worktree paths through the bridge", async () => { const home = vi.fn<() => Promise<{ home: string }>>(async () => ({ home: "/home/demo" })); const bridgeMkdir = vi.fn<() => Promise>(async () => undefined); diff --git a/src/supervisor/git/worktreeService.ts b/src/supervisor/git/worktreeService.ts index b0f87b77..631942f8 100644 --- a/src/supervisor/git/worktreeService.ts +++ b/src/supervisor/git/worktreeService.ts @@ -188,9 +188,18 @@ export class GitWorktreeService { ) : undefined; + // A bare remote-tracking branch name (e.g. "feature/x" when only + // `origin/feature/x` exists locally) is not a valid object on its own, so + // `git worktree add -b feature/x` fails with "not a valid + // object name". Qualify it with its remote before forking. + const resolvedStartPoint = + startPoint && createBranch + ? await this.resolveStartPointRef(location, startPoint) + : startPoint; + const args = ["worktree", "add"]; if (createBranch && branch) { - args.push("-b", branch, resolvedPath, ...(startPoint ? [startPoint] : [])); + args.push("-b", branch, resolvedPath, ...(resolvedStartPoint ? [resolvedStartPoint] : [])); } else { args.push(resolvedPath, ...(branch ? [branch] : [])); } @@ -221,7 +230,9 @@ export class GitWorktreeService { } if (branch && createBranch) { - const sourceBranch = startPoint ?? (await this.getCurrentBranch(location)); + // Record the resolved (qualified) start-point so the diff base matches + // what we actually forked from — e.g. "origin/feature/x", not "feature/x". + const sourceBranch = resolvedStartPoint ?? (await this.getCurrentBranch(location)); if (sourceBranch && sourceBranch !== branch) { await this.writeWorktreeSourceBranch(location, branch, sourceBranch); } @@ -373,6 +384,53 @@ export class GitWorktreeService { return { sourceBranch, commitsAhead, sourceAhead }; } + /** + * Resolve a worktree start-point into a ref `git worktree add` can fork from. + * A bare remote-tracking branch name (e.g. "feature/x" when only + * `origin/feature/x` exists) is not a valid object on its own, so we qualify + * it with its remote. Local branches, tags, SHAs and already-qualified remote + * refs resolve directly and pass through unchanged. When the name lives on + * several remotes we prefer `origin` (matching this module's origin-centric + * defaults — fetch/resolveFetchedSourceRef); a genuinely origin-less clash + * stays ambiguous, so we leave it for git to report rather than guessing. + */ + private async resolveStartPointRef( + location: ProjectLocation, + startPoint: string, + ): Promise { + if (await this.refResolves(location, startPoint)) return startPoint; + + let remotesRaw: string; + try { + remotesRaw = await execGit(location, ["remote"], { timeout: GIT_STATUS_TIMEOUT }); + } catch { + return startPoint; + } + const remotes = remotesRaw + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean); + const matchingRemotes: string[] = []; + for (const remote of remotes) { + if (await this.refResolves(location, `refs/remotes/${remote}/${startPoint}`)) { + matchingRemotes.push(remote); + } + } + if (matchingRemotes.length === 1) return `${matchingRemotes[0]}/${startPoint}`; + if (matchingRemotes.includes("origin")) return `origin/${startPoint}`; + return startPoint; + } + + /** True when `ref` resolves to an object in this repository. */ + private async refResolves(location: ProjectLocation, ref: string): Promise { + try { + await execGit(location, ["rev-parse", "--verify", "--quiet", ref]); + return true; + } catch { + return false; + } + } + async resolveFetchedSourceRef(location: ProjectLocation, sourceBranch: string): Promise { await this.fetch(location, "origin", true); if (sourceBranch.startsWith("origin/")) return sourceBranch;