fix(code): composer stop-wedge, worktree 403s, new-session modal brick (cave-kv8a)#3731
Merged
Conversation
…brick (cave-kv8a) 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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses three post-merge regressions in the Code surface’s composer + new-session flow: (1) Stop causing the composer to wedge due to an unhandled mid-stream abort rejection, (2) worktree-backed sessions 403’ing when the composer asserted an explicit unregistered projectRoot, and (3) the new-session modal staying “bricked” after a successful creation due to missing reset on prop-driven close.
Changes:
- Make Code composer sends abort-safe (Stop → partial reply preserved; phase returns to
done), and avoid sendingprojectRooton resume so the server derives cwd from the conversation/session record. - Extend
chatProjectAccessIdto map explicit roots under a registered project’s.worktrees/to the parent project’s grant (fail-closed otherwise), with regression tests. - Reset new-session modal state on successful session announcement and add a
.catchto avoid unhandled promise rejections.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib/chat-project-access.ts | Adds .worktrees/ parent-project mapping for explicit roots to avoid fail-closed 403s for worktree kickoffs. |
| src/lib/chat-project-access.test.ts | Adds regression coverage for .worktrees/ containment, traversal, and separator-exactness edge cases. |
| src/components/code-surface-mode.test.ts | Updates source-text pins to enforce “no projectRoot on resume” and abort-safe/error-safe behavior. |
| src/components/code-new-session.tsx | Ensures modal state resets on success and handles kickoff stream rejections. |
| src/components/code-composer.tsx | Removes projectRoot on resume sends and adds abort-safe try/catch around streaming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+78
to
+79
| const worktreeParent = worktreeParentProject(explicitRoot, args.projects); | ||
| if (worktreeParent) return worktreeParent.id; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three high-confidence findings from the post-merge review of #3724 (Code surface composer + new-session flow).
1. Stop wedged the composer (High)
send()awaitedstreamFamiliarTextwith no try/catch. A mid-stream abort rejects the SSE reader (contract peruse-quick-chat.ts:397), so Stop → unhandled rejection → phase stuck onstreaming, textarea disabled until remount. Now: abort keeps the partial reply and lands ondone; non-abort failures land onerrorwith the prompt restored for retry.2. Worktree sessions 403'd on composer sends (High)
The composer asserted
projectRoot: codeSessionWorkRoot(row). For.worktrees/sessions that's an explicit unregistered root →unregistered:<root>→ fail-closed inassertProjectAccess(same class as #2238). Two-part fix:projectRootrides — the send route derives the cwd from the conversation record (its own documented contract for continued turns).chatProjectAccessIdnow maps a root strictly below a registered project's.worktrees/to that project's grant — separator-exact and traversal-safe (path.resolvecollapses..), so sibling-dir evasion (cave-evil),.worktreesitself, and escapes still fail closed. The grant check still runs; nothing is exempted. This mirrors the Board task-card exemption's rationale ('worktrees are intentionally not separate project records') and fix(chat): reopened worktree chats keep their recorded .worktrees/ home #3725's client-side containment rule.3. New-session modal bricked after first success (Medium)
Parent closes the modal directly,
Modalnever firesonCloseon prop flip,reset()never ran → reopening showed disabled inputs stuck on 'Starting session…'. Success now resets before theonCreatedhandoff, and the kickoff promise has.catch(dropped stream ≠ unhandled rejection).Verification
chat-project-access.test.ts— 5 new chokepoint cases (parent map, sibling evasion,.worktreesitself, traversal escape, trailing slash) ✅code-surface-mode.test.ts— pins updated to the fixed behavior (no-projectRoot resume, catch-on-abort, reset-before-handoff, kickoff .catch) ✅harness-routing-host-session.test.ts,project-permissions.test.ts✅ (chokepoint consumers)pnpm typecheck,pnpm lint,pnpm build(bundle + standalone budgets) ✅Closes cave-kv8a.