Skip to content

fix(fork): resolve P0/P1 bugs — protocol, atomicity, streaming guard, button#4377

Open
waleedlatif1 wants to merge 5 commits intostagingfrom
feat/fork-fixes
Open

fix(fork): resolve P0/P1 bugs — protocol, atomicity, streaming guard, button#4377
waleedlatif1 wants to merge 5 commits intostagingfrom
feat/fork-fixes

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Fix root P0 bug: change fork protocol from ID-based (upToMessageId) to position-based (keepCount) so assistant messages can be forked correctly — Go's FindMessageIndex only matches user messages by MessageID, making assistant-message forks impossible with the old protocol
  • Add streaming guard: reject fork requests when parent.conversationId is non-null (active stream in progress)
  • Fix atomicity: delete the Sim copilotChats row if the downstream Go fork call fails, preventing orphaned chat rows
  • Include workflowId in forked chat row so the fork inherits the parent's workflow link
  • Enable fork button in MessageActions by wiring isStreamActive prop through from mothership-chat.tsx (was hardcoded false)
  • Pass isStreamActive from MothershipChat so the fork button is disabled during active streams

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

… button

- Send keepCount instead of upToMessageId to Go; the Sim persisted-message
  array index is used directly so there is no ID-matching between Sim and Go.
- Add streaming guard: reject fork if parent.conversationId is set (active
  stream in progress), matching the /chat/stop pattern.
- Add workflowId to INSERT so forked chats inherit the parent workflow link.
- Make Go failure a hard error with compensating delete: if the copilot call
  returns non-OK or throws, the orphaned Sim row is deleted before returning
  500 instead of silently succeeding.
- Move taskPubSub and captureServerEvent into the success path so analytics
  and sidebar updates only fire when both services confirmed the fork.
- Enable the fork button: canFork = Boolean(messageId && !isStreamActive).
- Add isStreamActive prop to MessageActions; mothership-chat passes it down
  so the button is suppressed during active generations.
@vercel
Copy link
Copy Markdown

vercel Bot commented May 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 1, 2026 7:12pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 1, 2026

PR Summary

Medium Risk
Changes the fork workflow to call the downstream Go service first (with a new keepCount protocol) and adds a server-side guard against forking while a stream is active, which could affect task creation behavior and error handling.

Overview
Fixes mothership task forking by changing the Sim→Go fork call from upToMessageId to a position-based keepCount and treating Go-fork failure as a hard error.

Reorders the fork flow to fork in Go first, then insert the Sim copilotChats row to avoid orphaned UI entries, and rejects forks when conversationId indicates an active stream; the forked chat now also copies workflowId.

Re-enables the fork UI action by adding an isStreamActive prop to MessageActions and wiring it from MothershipChat so the fork button is unavailable while streaming. Updates the API validation baseline route count to 717.

Reviewed by Cursor Bugbot for commit c90c2fc. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 1, 2026

Greptile Summary

This PR fixes a set of fork-related bugs: switches the Sim→Go protocol from upToMessageId to keepCount, inverts the operation order so Go is forked before the Sim row is inserted (eliminating the orphaned-row problem), adds a streaming guard, propagates workflowId, and finally wires isStreamActive through so the fork button is correctly disabled during active streams. The changes are well-scoped and the atomicity trade-offs are clearly documented in code comments.

Confidence Score: 5/5

Safe to merge — no P0/P1 issues found; single P2 suggestion on the validation error message.

All four changes are narrow and correct. The atomicity inversion (Go first, then DB) is sound given the acknowledged trade-off that a Go orphan without a Sim row is permanently inaccessible. The streaming guard, workflowId propagation, and button wiring are straightforward. No new security surface or data-loss risk introduced.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/app/api/mothership/chats/[chatId]/fork/route.ts Core fork logic refactored: Go called before DB insert for atomicity, protocol switched to keepCount, streaming guard added, workflowId propagated.
apps/sim/app/workspace/[workspaceId]/components/message-actions/message-actions.tsx Enables fork button by replacing hardcoded false with chatId && messageId && !isStreamActive guard; isStreamActive prop added to interface.
apps/sim/app/workspace/[workspaceId]/home/components/mothership-chat/mothership-chat.tsx Single-line change wiring isStreamActive down to MessageActions — straightforward and correct.
scripts/check-api-validation-contracts.ts Baseline counters bumped from 716 to 717 to account for the new fork route — housekeeping only.

Sequence Diagram

sequenceDiagram
    participant Client
    participant ForkRoute as POST /fork
    participant DB as Sim DB
    participant Go as Go Copilot Service

    Client->>ForkRoute: POST {upToMessageId}
    ForkRoute->>DB: SELECT parent chat
    DB-->>ForkRoute: parent row
    ForkRoute->>ForkRoute: Check conversationId (streaming guard)
    ForkRoute->>ForkRoute: Resolve upToMessageId → keepCount
    ForkRoute->>Go: POST /api/chats/fork {keepCount}
    alt Go fork fails
        Go-->>ForkRoute: non-OK / error
        ForkRoute-->>Client: 500 (no Sim row created)
    else Go fork succeeds
        Go-->>ForkRoute: OK
        ForkRoute->>DB: INSERT forked chat row
        alt Insert fails
            DB-->>ForkRoute: error
            ForkRoute-->>Client: 500 (Go state orphaned, inaccessible)
        else Insert succeeds
            DB-->>ForkRoute: newChat row
            ForkRoute->>Go: publishStatusChanged (PubSub)
            ForkRoute-->>Client: {success: true, id: newId}
        end
    end
Loading

Reviews (3): Last reviewed commit: "refactor(fork): remove self-explanatory ..." | Re-trigger Greptile

Comment thread apps/sim/app/api/mothership/chats/[chatId]/fork/route.ts Outdated
Comment thread apps/sim/app/api/mothership/chats/[chatId]/fork/route.ts Outdated
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 4228b6e. Configure here.

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit c90c2fc. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant