From 4245929ae59cd8dea4d01079dd21d3021e235b2b Mon Sep 17 00:00:00 2001 From: Val Alexander Date: Thu, 23 Jul 2026 17:47:12 -0500 Subject: [PATCH] fix(code): composer stop-wedge, worktree 403s, and new-session modal brick (cave-kv8a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings from the #3724 merge: 1. Stop wedged the composer: a mid-stream abort REJECTS the streamFamiliarText reader (see use-quick-chat.ts), and send() had no try/catch — the unhandled rejection left phase stuck on 'streaming' with the textarea disabled until remount. Now caught: abort keeps the partial reply and lands on done; real failures surface as error with the prompt restored. 2. Worktree sessions 403'd on every composer send: asserting projectRoot=codeSessionWorkRoot(row) made resumes in .worktrees/ checkouts explicit unregistered-project requests, which fail closed (same class as #2238). Composer resumes now carry no projectRoot — the server derives the cwd from the conversation record. The fresh-worktree kickoff (an explicit root by necessity) is fixed at the chokepoint instead: chatProjectAccessId maps a root strictly below a registered project's .worktrees/ to THAT project's grant (separator-exact, traversal-safe via path.resolve), so the grant check still runs instead of deterministic denial. 3. New-session modal bricked after first success: the parent closes the modal without onClose firing, so reset() never ran and phase stayed 'starting' forever. Success now resets before handing off, and the kickoff promise has a .catch so a dropped stream can't reject unhandled. Pins updated in code-surface-mode.test.ts; chokepoint containment cases added to chat-project-access.test.ts (parent map, sibling-dir evasion, .worktrees itself, traversal escape, trailing slash). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/components/code-composer.tsx | 37 +++++++++++----- src/components/code-new-session.tsx | 38 ++++++++++------ src/components/code-surface-mode.test.ts | 24 +++++++++-- src/lib/chat-project-access.test.ts | 55 ++++++++++++++++++++++++ src/lib/chat-project-access.ts | 28 ++++++++++++ 5 files changed, 155 insertions(+), 27 deletions(-) diff --git a/src/components/code-composer.tsx b/src/components/code-composer.tsx index 638fe9fba..9492a876a 100644 --- a/src/components/code-composer.tsx +++ b/src/components/code-composer.tsx @@ -12,7 +12,6 @@ import { useRef, useState } from "react"; import { Button } from "@/components/ui/button"; import { streamFamiliarText } from "@/lib/familiar-stream"; -import { codeSessionWorkRoot } from "@/lib/code-surface"; import type { SessionRow } from "@/lib/types"; type Phase = { kind: "idle" } | { kind: "streaming"; runId: string } | { kind: "done" } | { kind: "error"; message: string }; @@ -45,15 +44,33 @@ export function CodeComposer({ setPhase({ kind: "streaming", runId }); setReply(""); setPrompt(""); - const result = await streamFamiliarText({ - familiarId: row.familiarId, - sessionId: row.id, - prompt: trimmed, - projectRoot: codeSessionWorkRoot(row), - runId, - signal: controller.signal, - onText: setReply, - }); + // No projectRoot rides on the resume: the server derives the cwd from the + // conversation record (or the daemon's session record), which is where the + // session actually lives — including `.worktrees/` checkouts. Asserting + // the worktree root here made the send an explicit unregistered-project + // request that fails closed (403), the same class #2238 fixed in Chat. + let result: Awaited>; + try { + result = await streamFamiliarText({ + familiarId: row.familiarId, + sessionId: row.id, + prompt: trimmed, + runId, + signal: controller.signal, + onText: setReply, + }); + } catch (err) { + // A mid-stream abort (Stop) rejects the reader — keep whatever streamed + // so far and only surface non-abort failures (see use-quick-chat.ts). + if (abortRef.current === controller) abortRef.current = null; + if (controller.signal.aborted) { + setPhase({ kind: "done" }); + } else { + setPhase({ kind: "error", message: err instanceof Error ? err.message : "Generation failed." }); + setPrompt(trimmed); // let the user retry without retyping + } + return; + } abortRef.current = null; if (controller.signal.aborted) { setPhase({ kind: "done" }); diff --git a/src/components/code-new-session.tsx b/src/components/code-new-session.tsx index bba897128..1bab98a73 100644 --- a/src/components/code-new-session.tsx +++ b/src/components/code-new-session.tsx @@ -99,6 +99,15 @@ export function CodeNewSession({ setPhase({ kind: "starting" }); let announced = false; + const announce = (sessionId: string) => { + if (announced) return; + announced = true; + // The component stays mounted after the parent closes the modal, so + // restore idle state here — Modal never fires onClose on a prop flip, + // and a phase stuck on "starting" bricks every later open. + reset(); + onCreated(sessionId); + }; // Fire-and-continue: the moment the session id arrives the rail can select // it; the stream keeps flowing server-side and Chat shows the transcript. void streamFamiliarText({ @@ -106,19 +115,22 @@ export function CodeNewSession({ prompt: prompt.trim(), projectRoot: cwd, runId: `code-new-session-${Date.now().toString(36)}`, - onSession: (sessionId) => { - if (announced) return; - announced = true; - onCreated(sessionId); - }, - }).then((result) => { - if (!announced && result.sessionId) { - announced = true; - onCreated(result.sessionId); - } else if (!announced && result.error) { - setPhase({ kind: "error", message: result.error }); - } - }); + onSession: announce, + }) + .then((result) => { + if (!announced && result.sessionId) { + announce(result.sessionId); + } else if (!announced && result.error) { + setPhase({ kind: "error", message: result.error }); + } + }) + .catch((err) => { + // A dropped stream rejects the reader; only surface it if the session + // id never arrived (afterwards the run lives server-side regardless). + if (!announced) { + setPhase({ kind: "error", message: err instanceof Error ? err.message : "Couldn’t start the session." }); + } + }); } function reset() { diff --git a/src/components/code-surface-mode.test.ts b/src/components/code-surface-mode.test.ts index 42d255c0b..19d639e5b 100644 --- a/src/components/code-surface-mode.test.ts +++ b/src/components/code-surface-mode.test.ts @@ -193,10 +193,16 @@ assert.match( /streamFamiliarText\(\{\s*familiarId: row\.familiarId,\s*sessionId: row\.id,/, "the composer RESUMES the selected session (sessionId rides) — never forks a new thread", ); +const composerSend = composer.match(/result = await streamFamiliarText\(\{[\s\S]*?\}\);/)?.[0] ?? ""; +assert.ok(composerSend.length > 0, "the composer resume send is present"); +assert.ok( + !composerSend.includes("projectRoot"), + "composer resumes assert NO projectRoot — the server derives the cwd from the conversation record; an explicit worktree root fails closed as unregistered (403, cave-kv8a)", +); assert.match( composer, - /projectRoot: codeSessionWorkRoot\(row\),/, - "composer turns run in the session's work root (worktree over shared checkout, cave-9q24)", + /catch \(err\) \{[\s\S]*?if \(controller\.signal\.aborted\) \{\s*setPhase\(\{ kind: "done" \}\);/, + "a mid-stream Stop rejects the reader — the catch keeps the partial reply and lands on done instead of wedging the streaming phase (cave-kv8a)", ); assert.match( composer, @@ -208,7 +214,7 @@ assert.match( /action: "create-worktree", branch: branch\.trim\(\)/, "fresh-worktree option provisions through the existing /api/changes action", ); -const kickoff = newSession.match(/void streamFamiliarText\(\{[\s\S]*?\}\)\.then/)?.[0] ?? ""; +const kickoff = newSession.match(/void streamFamiliarText\(\{[\s\S]*?\}\)\s*\.then/)?.[0] ?? ""; assert.ok(kickoff.length > 0, "the new-session kickoff send is present"); assert.ok( !kickoff.includes("sessionId:"), @@ -216,9 +222,19 @@ assert.ok( ); assert.match( newSession, - /onSession: \(sessionId\) => \{/, + /onSession: announce,/, "the rail learns the new session id the moment the bridge announces it", ); +assert.match( + newSession, + /const announce = \(sessionId: string\) => \{[\s\S]*?reset\(\);\s*onCreated\(sessionId\);/, + "success restores idle state before handing off — the mounted modal otherwise reopens bricked on 'Starting session…' (cave-kv8a)", +); +assert.match( + newSession, + /\.catch\(\(err\) => \{[\s\S]*?if \(!announced\) \{/, + "a kickoff stream failure surfaces as an error phase instead of an unhandled rejection (cave-kv8a)", +); assert.match( rail, /onNewSession\?: \(\) => void;/, diff --git a/src/lib/chat-project-access.test.ts b/src/lib/chat-project-access.test.ts index 067f158f1..f41fdc5f6 100644 --- a/src/lib/chat-project-access.test.ts +++ b/src/lib/chat-project-access.test.ts @@ -120,4 +120,59 @@ assert.equal( "a registered project wins over the workspace exemption", ); +// REGRESSION (cave-kv8a): the Code surface's fresh-worktree kickoff sends the +// just-provisioned `.worktrees/` checkout as an explicit projectRoot. +// Worktrees are intentionally not separate project records, so the request +// must authorize against the PARENT project's grant — not fail closed as an +// arbitrary unregistered directory (403 on every kickoff). +assert.equal( + chatProjectAccessId({ + projects, + requestedProjectRoot: "/Users/me/dev/cave/.worktrees/feat-x", + resolvedCwd: "/Users/me/dev/cave/.worktrees/feat-x", + }), + "proj-1", + "an explicit root under a registered project's .worktrees/ vets the parent project's grant", +); + +assert.equal( + chatProjectAccessId({ + projects, + requestedProjectRoot: "/Users/me/dev/cave-evil/.worktrees/feat-x", + resolvedCwd: "/Users/me/dev/cave-evil/.worktrees/feat-x", + }), + "unregistered:/Users/me/dev/cave-evil/.worktrees/feat-x", + "sibling-dir evasion (cave-evil) misses the containment check and fails closed", +); + +assert.equal( + chatProjectAccessId({ + projects, + requestedProjectRoot: "/Users/me/dev/cave/.worktrees", + resolvedCwd: "/Users/me/dev/cave/.worktrees", + }), + "unregistered:/Users/me/dev/cave/.worktrees", + "the .worktrees directory itself is not a worktree — fails closed", +); + +assert.equal( + chatProjectAccessId({ + projects, + requestedProjectRoot: "/Users/me/dev/cave/.worktrees/../../elsewhere", + resolvedCwd: "/Users/me/dev/cave/.worktrees/../../elsewhere", + }), + "unregistered:/Users/me/dev/cave/.worktrees/../../elsewhere", + "a traversal escape below .worktrees/ resolves outside and fails closed", +); + +assert.equal( + chatProjectAccessId({ + projects, + requestedProjectRoot: "/Users/me/dev/cave/.worktrees/feat-x/", + resolvedCwd: "/Users/me/dev/cave/.worktrees/feat-x", + }), + "proj-1", + "a trailing slash on the worktree root still maps to the parent project", +); + console.log("chat-project-access tests passed"); diff --git a/src/lib/chat-project-access.ts b/src/lib/chat-project-access.ts index 92c3c46e6..b8b2c7b75 100644 --- a/src/lib/chat-project-access.ts +++ b/src/lib/chat-project-access.ts @@ -19,6 +19,22 @@ function samePath(a: string, b: string): boolean { return path.resolve(a) === path.resolve(b); } +/** + * The registered project whose `.worktrees/` directory contains `root`, if + * any. Separator-exact and traversal-safe: the candidate is `path.resolve`d + * (collapsing `..` escapes) and must sit strictly BELOW + * `/.worktrees/`, so `/proj-evil/...`, `/proj/.worktrees` itself, + * and `/proj/.worktrees/../..` all miss. + */ +function worktreeParentProject(root: string, projects: CaveProject[]): CaveProject | null { + const resolved = path.resolve(root); + for (const project of projects) { + const prefix = path.resolve(project.root) + path.sep + ".worktrees" + path.sep; + if (resolved.startsWith(prefix) && resolved.length > prefix.length) return project; + } + return null; +} + /** * Resolve the project id a chat request must hold a grant for, or null when * the request is not project-scoped (no permission check applies). @@ -32,6 +48,15 @@ function samePath(a: string, b: string): boolean { * echo the recorded cwd back as an explicit projectRoot on later turns. * Fail-closing on it denied the familiar its own home ("project access * denied" 403 on turn 2 of every no-project chat). + * + * A second carve-out routes rather than skips the check: an explicit root + * sitting below a registered project's `.worktrees/` directory authorizes + * against THAT project. Worktrees are intentionally not separate project + * records (see the Board handoff exemption in the send route), so a + * `.worktrees/` checkout — e.g. the Code surface's fresh-worktree + * kickoff — must vet the familiar's grant on the parent project instead of + * fail-closing as an arbitrary unregistered directory. The grant check still + * runs; no access is conceded. */ export function chatProjectAccessId(args: ChatProjectAccessArgs): string | null { const explicitRoot = args.requestedProjectRoot?.trim() || undefined; @@ -50,5 +75,8 @@ export function chatProjectAccessId(args: ChatProjectAccessArgs): string | null return null; } + const worktreeParent = worktreeParentProject(explicitRoot, args.projects); + if (worktreeParent) return worktreeParent.id; + return `unregistered:${projectRoot}`; }