Conversation
github-actions
Bot
added
vouch:unvouched
|
Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting). This review would cost an estimated $12.54, which exceeds your per-review limit of $10.00. The top 3 files driving up this estimate:
Tip To get this pull request reviewed, you can:
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Three convention violations in the Effect service code touched here: two new static-analysis suppressions without an explanation, and one error translation that discards its cause. Details inline.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
New @effect-diagnostics nodeBuiltinImport:off directive with no adjacent explanation of why the diagnostic must be disabled here. Consider stating the reason (a synchronous digest is needed to derive the id outside any Effect runtime), as other suppressions in this package do.
| // @effect-diagnostics nodeBuiltinImport:off | |
| // @effect-diagnostics nodeBuiltinImport:off - deterministic v5 ids need node:crypto's synchronous sha1 digest, which the Effect Crypto service does not expose. |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Fixed in f39fba0 — the suppression now records the reason (deterministic v5 ids need node:crypto's synchronous sha1 digest, which the Effect Crypto service does not expose).
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
| () => | ||
| new ProviderAdapterRequestError({ | ||
| provider: providerErrorLabelFromInstanceHint({ | ||
| instanceId: String(forkSource.providerInstanceId), | ||
| }), | ||
| method: "thread.turn.start", | ||
| detail: `Thread '${threadId}' cannot resolve fork capabilities for provider instance '${forkSource.providerInstanceId}'.`, | ||
| }), |
There was a problem hiding this comment.
This translation drops the underlying getCapabilities failure: the wrapper is built from a zero-arg lambda, so the original error and its stack are lost. ProviderAdapterRequestError already declares an optional cause; preserving it keeps the error chain intact.
| () => | |
| new ProviderAdapterRequestError({ | |
| provider: providerErrorLabelFromInstanceHint({ | |
| instanceId: String(forkSource.providerInstanceId), | |
| }), | |
| method: "thread.turn.start", | |
| detail: `Thread '${threadId}' cannot resolve fork capabilities for provider instance '${forkSource.providerInstanceId}'.`, | |
| }), | |
| (cause) => | |
| new ProviderAdapterRequestError({ | |
| provider: providerErrorLabelFromInstanceHint({ | |
| instanceId: String(forkSource.providerInstanceId), | |
| }), | |
| method: "thread.turn.start", | |
| detail: `Thread '${threadId}' cannot resolve fork capabilities for provider instance '${forkSource.providerInstanceId}'.`, | |
| cause, | |
| }), |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Fixed in f39fba0 — the mapError lambda now takes the failure and passes it as cause, so the original getCapabilities error and stack survive the translation.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
New @effect-diagnostics nodeBuiltinImport:off directive with no adjacent explanation. This file already acquires effect/Crypto (used for crypto.randomUUIDv4), so it is worth recording why the fork path reaches for node:crypto instead — the pure assembler's mintUuid needs a synchronous () => string.
| // @effect-diagnostics nodeBuiltinImport:off | |
| // @effect-diagnostics nodeBuiltinImport:off - assembleThreadFork's mintUuid is a synchronous callback, which the Effect Crypto service cannot satisfy. |
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
Fixed in f39fba0 — the suppression now records the reason (assembleThreadFork mints ids through a synchronous callback, which the Effect Crypto service cannot satisfy).
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
"git-fork" has no entry in ICON_PATHS (apps/web/src/contextMenuFallback.ts), which is the icon registry the browser context-menu fallback uses. createIconElement returns null for unknown names, so in the web build the new "Fork thread" row renders with no icon while every sibling row (new-thread-on-branch, pin, settle, rename, …) has one — and because the row is flex … gap-2 with a flex-1 label, its label shifts left out of the shared icon column in both the sidebar row menu and the chat header thread menu.
Smallest fix: register the Lucide git-fork paths alongside the other names in ICON_PATHS, e.g.
"git-fork": [
{ tag: "circle", attrs: { cx: "12", cy: "18", r: "3" } },
{ tag: "circle", attrs: { cx: "6", cy: "6", r: "3" } },
{ tag: "circle", attrs: { cx: "18", cy: "6", r: "3" } },
{ tag: "path", attrs: { d: "M18 9v2c0 .6-.4 1-1 1H7c-.6 0-1-.4-1-1V9" } },
{ tag: "path", attrs: { d: "M12 12v3" } },
],(alternatively reuse an already-registered name such as git-branch).
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Good catch — fixed in f39fba0 by registering the Lucide git-fork paths in ICON_PATHS, so the browser context-menu fallback renders the icon and the label stays in the shared icon column.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR adds a large, cross-cutting conversation-forking capability spanning UI, orchestration, provider adapters, persistence, filesystem/Git side effects, and schema migrations. It also enables the new behavior by default and adds static-analysis suppression directives, making human review necessary. Not approved because:
Review your spending limits in Billing settings, or comment |
Worktree removal and shared-worktree detection only consulted the active shell, so archived threads sharing a worktree were treated as gone: deleting a sibling could remove a worktree an archived thread still points at, and checkpoint branch-drift adoption ignored archived co-owners. Removal now refuses while any non-deleted thread references the path — resolved by a single atomic query so a thread can never fall between separate active and archived reads — drift adoption counts archived siblings, and the legacy sidebar's bulk delete no longer pre-seeds its deleted-set with the whole batch (which made the first deletion treat still-alive batch mates as gone). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds thread forking: any completed turn of a thread can be continued in a new thread that carries the conversation, workspace context, model and mode, and the linked pull request. The fork is a new aggregate — a server-materialized thread.fork command emits thread.created plus a new thread.forked event whose copied history lands as real projection rows (fresh ids, preserved timestamps, checkpoint refs aliased under the child) — and the provider session forks natively at the child's first message: Codex thread/fork, Claude resume+forkSession(+resumeSessionAt), OpenCode session.fork with an exclusive boundary. Cursor and Grok report the action as unsupported rather than starting an agent that has not seen the transcript. Per-turn provider anchors are recorded so mid-thread forks survive restarts; a T3CODE_THREAD_FORKING kill-switch drives the threadForking capability and is enforced server-side. Web gets fork-from-message on every hover rail plus thread menus, palette, /fork and an unbound chat.fork binding, a lineage chip and an in-timeline divider; mobile forks from thread menus and headers. Forks retitle themselves after their first reply. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9edfe97 to
f39fba0
Compare
The null-turn row cutoff compared ISO strings; canonical timestamps are UTC so this held, but an offset-form timestamp would misorder. Parse to epoch milliseconds instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| > | ||
| <GitForkIcon className="size-3" /> | ||
| </TooltipTrigger> | ||
| <TooltipPopup side="top">{ctx.forkWarning ?? "Fork from here"}</TooltipPopup> |
There was a problem hiding this comment.
ctx.forkWarning can never be shown here. It is non-null exactly when activeThread.session.status === "running" (ChatView), and activity.isWorking is phase === "running" || … with phase = derivePhase(session) — so whenever the warning exists this trigger is disabled, and Button carries disabled:pointer-events-none, which suppresses the hover that would open the tooltip. The "shares files with an agent that is still working" caution therefore never reaches the user, and the row action is silently unavailable instead.
Suggest picking one: keep the button hoverable while blocked (aria-disabled + aria-disabled:opacity-64 aria-disabled:cursor-not-allowed, guarding onClick) so the caution reads, or drop forkWarning and its plumbing if forking is meant to be simply unavailable while the agent works.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Fixed in ee806cd — and this caught a real behavior bug beyond the tooltip: forking earlier completed turns while the agent is working is intended (the per-row forkTurnId gating already excludes the running turn), so the button no longer disables on isWorking at all. The warning tooltip now renders on hover exactly when the caution applies, and the action goes through.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Fixed in ee806cd (this check run reviewed the previous commit). Went with the first option: the fork trigger only sets disabled on isRevertingCheckpoint, so earlier completed turns stay forkable while the agent works and the "shares files with an agent that is still working" warning is actually reachable on hover.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
| <Undo2Icon className="size-3" /> | ||
| </TooltipTrigger> | ||
| <TooltipPopup side="top">Revert to this message</TooltipPopup> | ||
| <TooltipPopup side="top">{ctx.revertDisabledReason ?? "Revert to this message"}</TooltipPopup> |
There was a problem hiding this comment.
revertDisabledReason is only rendered inside this tooltip, but the same non-null value also sets disabled on the trigger just above, and Button's base classes include disabled:pointer-events-none. A natively disabled button receives no hover or focus events, so this explanation can never appear — in an unsent fork the user just sees a greyed revert icon with no reason.
The repo's convention for explaining a blocked control on hover is to keep it hoverable: aria-disabled plus a dimmed style (see ThemeEditorPanel.tsx, whose comment states "a real disabled attribute would swallow the pointer events"), or wrap the trigger in <span className="inline-flex"> as ConnectionsSettings and the compact-context action in ChatView do.
- disabled={
- ctx.revertDisabledReason !== null ||
- activity.isRevertingCheckpoint ||
- activity.isWorking
- }
- onClick={() => ctx.onRevertUserMessage(messageId)}
+ aria-disabled={
+ ctx.revertDisabledReason !== null ||
+ activity.isRevertingCheckpoint ||
+ activity.isWorking
+ }
+ className="aria-disabled:cursor-not-allowed aria-disabled:opacity-64"
+ onClick={() => {
+ if (ctx.revertDisabledReason !== null) return;
+ if (activity.isRevertingCheckpoint || activity.isWorking) return;
+ ctx.onRevertUserMessage(messageId);
+ }}Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Fixed in ee806cd — the revert trigger now follows the ThemeEditorPanel convention: aria-disabled with a dimmed style and a guarded onClick instead of a native disabled attribute, so the explanation tooltip is reachable on an unsent fork.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Fixed in ee806cd (our pushes crossed with this check run — it reviewed the previous commit). The revert trigger now follows the ThemeEditorPanel pattern: aria-disabled={revertBlocked} with a dimmed class and a guarded onClick, no native disabled, so the tooltip explaining revertDisabledReason is reachable on hover/focus.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
| type="button" | ||
| aria-label={forkChipLabel} | ||
| onClick={openParentThread} | ||
| className="ml-2 inline-flex max-w-48 shrink-0 cursor-pointer items-center gap-1 rounded-full border border-border/70 px-1.5 py-px text-xs font-normal text-muted-foreground transition-colors hover:text-foreground focus-visible:outline-hidden focus-visible:ring-2 focus-visible:ring-ring" |
There was a problem hiding this comment.
This chip is a raw <button> that reconstructs Button size="micro" (small height, gap-1, muted text, pointer cursor, focus ring), but it does not inherit the primitive's pointer-coarse:after:min-h-11 pointer-coarse:after:min-w-11 hit target or focus-visible:ring-offset-1. On touch the chip is a ~18px tap target in the header, where every neighbouring control meets the 44px minimum.
A comparable tooltip-triggered chip already goes through the primitive — PullRequestDetailPanel.tsx renders <Button size="micro" variant="outline" className="… gap-1 font-normal text-muted-foreground" />. Suggest rendering this trigger as <Button size="micro" variant="ghost" className="ml-2 max-w-48 rounded-full border-border/70 font-normal text-muted-foreground …" /> (needs a Button import in this file) and keeping only the pill geometry/colour at the call site.
Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Fixed in ee806cd — the chip renders through the Button primitive (size="micro" variant="ghost", pill geometry kept at the call site), picking up the coarse-pointer 44px hit target and focus-ring behavior like the PullRequestDetailPanel chip.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Fixed in ee806cd (this check run reviewed the previous commit). The lineage chip now renders through the Button primitive (size="micro" variant="ghost"), inheriting the coarse-pointer 44px hit target and focus ring, with only the pill geometry/colour kept at the call site — mirroring the PullRequestDetailPanel chip.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
… lineage chip The fork button disabled itself whenever the agent was working, which both hid the shared-worktree warning tooltip (disabled buttons swallow pointer events) and blocked forking earlier completed turns mid-run — the per-row gating already excludes the running turn. The revert button now stays hoverable while blocked (aria-disabled, guarded click) so its reason reads, and the lineage chip renders through the Button primitive for the coarse pointer hit target and focus ring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| return ( | ||
| <Tooltip> | ||
| <TooltipTrigger | ||
| render={ |
There was a problem hiding this comment.
ctx.forkWarning can never reach the popup on line 1389: it is only non-null when the thread session is running, and derivePhase maps that status to phase === "running", which makes activity.isWorking true — so this disabled prop is set whenever the warning exists, and Button's disabled:pointer-events-none suppresses the hover that would open the tooltip. The row therefore always reads "Fork from here", and the "shares files with an agent that is still working" warning (which the docs promise) is unreachable dead plumbing.
Since forkTurnIdByMessageId already excludes the active running turn, earlier completed turns are still forkable while the agent works — which is the case the warning is for. Smallest fix is to stop gating on isWorking so the warning actually shows; otherwise drop the forkWarning prop chain.
- disabled={activity.isRevertingCheckpoint || activity.isWorking}
+ disabled={activity.isRevertingCheckpoint}Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
This is already the shipped state in ee806cd (the check-run appears to have analyzed the prior revision): the fork button's disabled prop is exactly disabled={activity.isRevertingCheckpoint} — no isWorking gate — so earlier completed turns fork while the agent works and the shared-worktree warning tooltip renders. See MessagesTimeline.tsx ~:1386 at this head.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Duplicate of the thread at line 1394 — fixed in ee806cd (this check run reviewed the previous commit). isWorking was dropped from the fork trigger's disabled, exactly as suggested, so the warning tooltip shows while the agent is running.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
revertDisabledReason is non-null exactly when this Button is disabled, and the shared Button base classes include disabled:pointer-events-none — so the trigger never receives hover or focus and the explanatory popup below can never open. On an unsent fork the user just sees a dimmed revert icon with no reason.
The established pattern here is to keep the control hoverable with aria-disabled and guard the handler (see ThemeEditorPanel.tsx: "A locked mode stays hoverable so the tooltip can say why it is off; a real disabled attribute would swallow the pointer events", and the aria-disabled hover popover in GitActionsControl.tsx):
- disabled={
- ctx.revertDisabledReason !== null ||
- activity.isRevertingCheckpoint ||
- activity.isWorking
- }
- onClick={() => ctx.onRevertUserMessage(messageId)}
+ aria-disabled={ctx.revertDisabledReason !== null || undefined}
+ className={
+ ctx.revertDisabledReason !== null ? "cursor-not-allowed opacity-64" : undefined
+ }
+ disabled={activity.isRevertingCheckpoint || activity.isWorking}
+ onClick={() => {
+ if (ctx.revertDisabledReason !== null) return;
+ ctx.onRevertUserMessage(messageId);
+ }}Posted via Macroscope — UI Consistency
There was a problem hiding this comment.
Already the shipped state in ee806cd: the revert trigger uses the ThemeEditorPanel pattern — revertBlocked computed once, aria-disabled={revertBlocked}, dimmed class, and a guarded onClick — so the reason tooltip is hoverable on an unsent fork. See MessagesTimeline.tsx ~:1343-1360 at this head; no native disabled attribute remains on this button.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Same finding as the thread at line 1367 — fixed in ee806cd (this check run reviewed the previous commit). The trigger is aria-disabled + guarded onClick rather than natively disabled, so the explanatory tooltip opens.
There was a problem hiding this comment.
Sorry, I'm unable to act on this request because you do not have permissions within this repository.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ee806cd. Configure here.
A later running or interrupted turn can already hold content in the provider's native session, so an anchorless whole-session fork taken "behind" it would give the child agent history its transcript does not show. atEnd is now true only when the fork turn is the source's final turn row; anchor-requiring providers reject otherwise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
wait for julius orchestration pr |
Sounds good, let me know if you have any questions. |
Merges the conversation-forking system (feat/thread-forking) into the fork's main. Upstream PR pingdotgg#9187 was parked pending an upstream orchestration change; Lecturn carries the feature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> # Conflicts: # apps/server/src/provider/Layers/CodexAdapter.ts # apps/server/src/provider/Layers/ProviderService.test.ts # apps/server/src/provider/Services/ProviderAdapter.ts
2 participants
Adds conversation forking: continue any completed point of a thread in a new thread, the way Claude Code, Codex and ChatGPT branch sessions. The fork is a perfect reconstruction of the parent at the fork point — conversation, workspace context, model and mode, linked PR — and deviates only through user/agent action afterwards.
How it works. A server-materialized
thread.forkcommand emitsthread.created+ a newthread.forkedevent on the child stream; the copied history lands as real projection rows (fresh ids, preserved timestamps) and the parent's checkpoint refs are aliased under the child, so diffs and revert work across the inherited turns. The provider session forks natively at the child's first message: Codexthread/fork {lastTurnId}, Clauderesume+forkSession(+resumeSessionAt, truncation verified against the live SDK), OpenCodesession.forkwith its exclusive message boundary. Cursor and Grok report the action as unsupported rather than starting an agent that hasn't seen the transcript. Per-turn provider anchors (providerRefs.providerTurnId→projection_turns.provider_turn_ref) make mid-thread forks durable across restarts; aT3CODE_THREAD_FORKINGkill-switch drives thethreadForkingcapability and is enforced server-side. Migration 045 adds the lineage/anchor columns (nullable, additive).Surfaces. Web: "Fork from here" on message hover (both rails), thread header/sidebar/legacy-sidebar menus, command palette,
/fork, an unboundchat.forkbinding, a lineage chip ("Forked from … · after turn N") and an in-timeline divider; revert is locked on a fork until its first send. Mobile: fork from all thread-list menu variants and the thread header, with a lineage subtitle. Forks title as " (fork)" and auto-retitle after their first reply. Docs:docs/user/forking-threads.md, glossary entries,docs/internals/thread-forking.md, provider capability table.Evidence (real app, real Codex; the fork answered from conversation memory — "hello" — while the file on disk had already been changed to "goodbye" by the parent's later turn):
The first commit here is #9185 (the fork flow makes shared worktrees a first-class case, so it rides in front). Verified with focused suites across contracts/server/web/mobile (~600 tests touching the feature), all six package typechecks, a live Claude truncation smoke, and the end-to-end browser pass above.
Built with Claude Fable 5 on Claude Code.
Note
Medium Risk
Fork touches orchestration projections, checkpoint aliasing, and attachment copying alongside new client navigation paths; mistakes could corrupt inherited history or lineage, though changes are heavily tested.
Overview
Adds mobile conversation forking at the latest completed turn, wired through shared
useForkThread/resolveThreadForkAvailabilitygates (connection,threadForkingcapability, nativeconversationFork, completed turn). Fork actions appear in compact and v2 thread-list menus, the iPad sidebar, and the thread screen header (iOS menu + Android action); success navigates to the child immediately via a narrowedonSelectThreadthat only needsenvironmentIdandid, with loading until the shell arrives. The open thread shows a “Forked from …” subtitle when lineage is known.On the server, this diff extends fork support beyond dispatch:
T3CODE_THREAD_FORKINGdrives the advertisedthreadForkingcapability;thread.forkedis projected into messages, turns, activities, plans, and thread lineage (forkedFrom/forkSource); session metadata now stampsprovider_turn_refon completed turns (including when diff-completion settled the turn first). Checkpoint refs can be aliased to child threads; attachments get deterministic child-thread copies viacopyClaimedAttachment. Snapshot queries expose fork context, full turn/activity lists for assembly, and worktree path lookups that include archived threads—used so branch-drift adoption does not ignore archived siblings on a shared worktree.Mobile outbox delivery passes a
titleSeedfor fork children still titled… (fork)so the first reply can auto-retitle. Tests cover fork menu behavior, attachment copy plans, checkpoint aliasing, projection replay/idempotency, and lineage on wire shapes.Reviewed by Cursor Bugbot for commit a814d61. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add conversation forking from any completed turn across server, web, and mobile
Adds a full-stack feature to fork a conversation thread from any completed turn, creating a child thread with inherited history, checkpoints, attachments, and workspace context.
thread.forkcommands over WebSocket: loads source projections, assembles inherited history (messages, turns, activities, plans), aliases checkpoint refs, copies attachments, and dispatches a materialized fork command to the orchestration engine. The normalizer rejects raw client-shaped fork commands so only the WebSocket path can create forks.ProviderAdapterValidationError. Claude requires a provider turn anchor; Codex and OpenCode do not.thread.created+thread.forkedevents. Projection pipeline copies inherited history into the child thread and stamps provider turn anchors on completed turns./forkslash command, and achat.forkkeybinding. Forked threads show a lineage chip and a timeline divider marking the fork boundary.forked_from_json,fork_source_json, andprovider_turn_refcolumns to the projection tables. Server config gains aT3CODE_THREAD_FORKINGkill switch (default enabled).normalizeDispatchCommandnow rejects rawthread.forkcommands withOrchestrationDispatchCommandError; any code path sending client-shaped fork commands outside the WebSocket dispatcher will fail.CheckpointReactorbranch-drift check now querieslistThreadIdsByWorktreePathinstead of scanning shells, so archived threads sharing a worktree path prevent branch adoption.Macroscope summarized a814d61.