feat(web): fork from a selected assistant response#4248
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
vouch:unvouched
github-actions
Bot
added
the
size:XL
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| yield* directory.getBinding(requestedForkFrom.threadId), | ||
| ); | ||
| if (!sourceBinding) { | ||
| if (requestedForkFrom.sourceTurnId === undefined) { |
There was a problem hiding this comment.
🟠 High Layers/ProviderService.ts:573
When forkFrom is requested but the source thread has no persisted provider binding and forkFrom.sourceTurnId is absent, startSession silently drops the fork request and starts a fresh, unrelated session — even when forkFrom.sourceTurnIndex is provided. An index-based fork without a sourceTurnId is a valid request shape, so the fork is silently ignored and the resulting session has no relationship to the intended source thread. The fallback to undefined at line 573 should only apply when neither sourceTurnId nor sourceTurnIndex is present; if either boundary field is specified, the missing-binding condition should produce a validation error instead.
Also found in 1 other location(s)
apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:516
forkSource.threadIdis always set to the immediate parent fork (forkedFrom.threadId). A fork can itself be forked from one of its inherited completed responses before any follow-up is sent, and the PR explicitly delays provider-session creation until that first follow-up. In that case the immediate parent has no persisted provider binding, soProviderService.startSessionrejects the nested fork with “no persisted provider binding” even though the original ancestor has valid native resume state. The nested side chat therefore cannot send its first message; this needs to resolve the ancestor provider source/boundary rather than requiring a binding on the unstarted intermediate fork.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/ProviderService.ts around line 573:
When `forkFrom` is requested but the source thread has no persisted provider binding and `forkFrom.sourceTurnId` is absent, `startSession` silently drops the fork request and starts a fresh, unrelated session — even when `forkFrom.sourceTurnIndex` is provided. An index-based fork without a `sourceTurnId` is a valid request shape, so the fork is silently ignored and the resulting session has no relationship to the intended source thread. The fallback to `undefined` at line 573 should only apply when neither `sourceTurnId` nor `sourceTurnIndex` is present; if either boundary field is specified, the missing-binding condition should produce a validation error instead.
Also found in 1 other location(s):
- apps/server/src/orchestration/Layers/ProviderCommandReactor.ts:516 -- `forkSource.threadId` is always set to the immediate parent fork (`forkedFrom.threadId`). A fork can itself be forked from one of its inherited completed responses before any follow-up is sent, and the PR explicitly delays provider-session creation until that first follow-up. In that case the immediate parent has no persisted provider binding, so `ProviderService.startSession` rejects the nested fork with “no persisted provider binding” even though the original ancestor has valid native resume state. The nested side chat therefore cannot send its first message; this needs to resolve the ancestor provider source/boundary rather than requiring a binding on the unstarted intermediate fork.
| function sessionUserMessageStartsHumanTurn(message: SessionMessage): boolean { | ||
| if (message.type !== "user") { | ||
| return false; | ||
| } | ||
| const content = (message.message as { readonly content?: unknown } | null)?.content; | ||
| if (!Array.isArray(content)) { | ||
| return true; | ||
| } | ||
| return content.some( | ||
| (block) => | ||
| typeof block !== "object" || | ||
| block === null || | ||
| (block as { readonly type?: unknown }).type !== "tool_result", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟠 High Layers/ClaudeAdapter.ts:607
sessionUserMessageStartsHumanTurn treats nested subagent user messages that contain text as new human turns, but those messages have no corresponding top-level visible turn. This inflates terminalAssistantUuids with phantom entries, shifting the array relative to sourceTurnIndex so resolveClaudeAssistantForkPoint returns the wrong assistant UUID (or hits an out-of-range index and returns undefined). The function should return false for any user message with a non-null parent_tool_use_id, since those belong to nested agent invocations rather than top-level human turns.
| function sessionUserMessageStartsHumanTurn(message: SessionMessage): boolean { | |
| if (message.type !== "user") { | |
| return false; | |
| } | |
| const content = (message.message as { readonly content?: unknown } | null)?.content; | |
| if (!Array.isArray(content)) { | |
| return true; | |
| } | |
| return content.some( | |
| (block) => | |
| typeof block !== "object" || | |
| block === null || | |
| (block as { readonly type?: unknown }).type !== "tool_result", | |
| ); | |
| } | |
| function sessionUserMessageStartsHumanTurn(message: SessionMessage): boolean { | |
| if (message.type !== "user") { | |
| return false; | |
| } | |
| const rawMessage = message.message as { readonly content?: unknown; readonly parent_tool_use_id?: unknown } | null; | |
| if (rawMessage?.parent_tool_use_id != null) { | |
| return false; | |
| } | |
| const content = rawMessage?.content; | |
| if (!Array.isArray(content)) { | |
| return true; | |
| } | |
| return content.some( | |
| (block) => | |
| typeof block !== "object" || | |
| block === null || | |
| (block as { readonly type?: unknown }).type !== "tool_result", | |
| ); | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/ClaudeAdapter.ts around lines 607-621:
`sessionUserMessageStartsHumanTurn` treats nested subagent `user` messages that contain text as new human turns, but those messages have no corresponding top-level visible turn. This inflates `terminalAssistantUuids` with phantom entries, shifting the array relative to `sourceTurnIndex` so `resolveClaudeAssistantForkPoint` returns the wrong assistant UUID (or hits an out-of-range index and returns `undefined`). The function should return `false` for any `user` message with a non-null `parent_tool_use_id`, since those belong to nested agent invocations rather than top-level human turns.
| "applyThreadMessagesProjection", | ||
| )(function* (event, attachmentSideEffects) { | ||
| switch (event.type) { | ||
| case "thread.forked": |
There was a problem hiding this comment.
🟠 High Layers/ProjectionPipeline.ts:843
The thread.forked message projection copies inherited attachments unchanged via [...message.attachments], so the forked thread's attachment IDs still contain the source thread's segment. When the source thread is deleted or reverted past those messages, runAttachmentSideEffects deletes files by that source segment, destroying attachment files that the fork still references. Consider materializing independent attachments for the fork (as thread.message-sent does via materializeAttachmentsForProjection) or making cleanup account for cross-thread references.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/orchestration/Layers/ProjectionPipeline.ts around line 843:
The `thread.forked` message projection copies inherited `attachments` unchanged via `[...message.attachments]`, so the forked thread's attachment IDs still contain the source thread's segment. When the source thread is deleted or reverted past those messages, `runAttachmentSideEffects` deletes files by that source segment, destroying attachment files that the fork still references. Consider materializing independent attachments for the fork (as `thread.message-sent` does via `materializeAttachmentsForProjection`) or making cleanup account for cross-thread references.
2 participants
What Changed
thread/fork, OpenCodesession.forkwith the selected message ID, and Claude resume/fork at the selected assistant UUID.Why
Forking from the chat header could only mean "fork the latest state." When a user wants to explore an earlier answer, the visible transcript and the provider session must both stop at that exact response.
Putting the action beneath each completed response makes the boundary explicit and matches the existing message-action interaction model.
Scope
This PR contains only the user-facing "fork from this message" feature. It includes the orchestration, persistence, and provider plumbing required to preserve the selected boundary end to end. It does not expose agent thread-control tools or add agent coordination instructions.
Verification
vp test runon the ten affected orchestration, migration, provider-adapter, message-timeline, and side-panel test files: 244 tests passed.UI Evidence
Before: latest-state action in the header
After: action beneath the selected response
Forked side chat and native provider follow-up
Interaction
The 15-second recording keeps the real cursor visible, holds the complete message action row and Fork to side chat tooltip before the click, then shows the forked panel accepting an independent native-provider follow-up.
15-second real-cursor selected-response fork and native follow-up recording
Checklist
Note
Add fork-from-assistant-response UI and backend support for Claude, Codex, and OpenCode providers
thread.forkcommand that creates a new thread with inherited messages up to the selected turn.thread.forkcommand andthread.forkedevent in the contracts layer, with decider logic that validates the source turn and copies non-streaming messages, and projector/pipeline logic that persists the forked thread and its inherited messages.forkSession/resumeSessionAt), Codex (viathread/forkRPC), and OpenCode (viasession.fork) adapters; addssessionFork: 'native'capability advertisement to each.ChatViewtab (with a fork icon) so users can view both branches side by side; the embedded view suppresses the top header and terminal drawers.forked_from_thread_idandforked_from_turn_idcolumns (plus an index) to theprojection_threadstable.📊 Macroscope summarized f44ff03. 26 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.