Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions src/components/code-composer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down Expand Up @@ -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<ReturnType<typeof streamFamiliarText>>;
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" });
Expand Down
38 changes: 25 additions & 13 deletions src/components/code-new-session.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -99,26 +99,38 @@ 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({
familiarId,
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() {
Expand Down
24 changes: 20 additions & 4 deletions src/components/code-surface-mode.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -208,17 +214,27 @@ 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:"),
"the kickoff send carries NO sessionId — a fresh thread, saved like any chat",
);
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;/,
Expand Down
55 changes: 55 additions & 0 deletions src/lib/chat-project-access.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/<branch>` 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");
28 changes: 28 additions & 0 deletions src/lib/chat-project-access.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
* `<project>/.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).
Expand All @@ -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/<branch>` 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;
Expand All @@ -50,5 +75,8 @@ export function chatProjectAccessId(args: ChatProjectAccessArgs): string | null
return null;
}

const worktreeParent = worktreeParentProject(explicitRoot, args.projects);
if (worktreeParent) return worktreeParent.id;
Comment on lines +78 to +79

return `unregistered:${projectRoot}`;
}
Loading