Skip to content

fix(contracts): side chats open after an abandoned worktree setup - #34686

Merged
juliusmarminge merged 4 commits into
mainfrom
factory/issue-34672
Nov 12, 2026
Merged

juliusmarminge merged 4 commits into
mainfrom
factory/issue-34672

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Nov 12, 2026 •

Copy link
Copy Markdown
Member

The review item was correct: the previous head's ordinal-based exemption disagreed with the server, which decides whether a thread is waiting for its worktree from latestExecutedRun/runRanAfter (completion order, unfinished runs latest; Orchestrator worktreeMissingRun). A setup retry on run 1 that failed again after run 2 completed was exempted, so web, mobile and thread.fork allowed a side chat while the server held the parent's messages for the worktree. orchestrationV2WorktreePending now returns true when worktreePath is null and the latest executed run is a worktree-preparation run in preparing, failed or interrupted. That covers the reported bug (a later completed run supersedes the abandoned setup), a retry in progress, and a retry that fails again. Because latestExecutedRun lives in @t3tools/shared (which depends on contracts), the helper moved from packages/contracts/src/orchestrationV2.ts to packages/shared/src/orchestrationV2ThreadError.ts, and the three callers (web ChatView, mobile use-thread-composer-state, server Orchestrator.dispatchThreadFork) import it from there. In the test fixture, finished runs now carry completedAt as real ones do. The repro test now also covers retry-preparing and retry-failed-again (TestClock advances so the retry finishes after run 2). The failed-again case fails with the previous head's helper and passes now. No new regex or parser.

Fixes #34672

Reproduction on main

Commit a6d676f (origin/main). Created a local git repo on branch dev whose origin pointed at a local TCP port that accepts and never answers, so worktree setup hangs on 'Fetch base branch'. Started T3CODE_DEV_SEED=0 vp run dev (fresh DB), paired the web client, added the repo as a project via Add project > Local folder. In a new thread chose Workspace 'New worktree' (From dev (remote)), Codex GPT-6-Luna, sent 'Say hello'. While it showed 'Setting up worktree… / Fetch base branch' clicked Stop: the thread showed 'Worktree setup cancelled' and 'Run interrupted · Run interrupted before provider start'. Sent 'Reply with the word ok': it ran in the project checkout (branch dev) and replied 'ok'. Typed /side in the composer: the /side command showed 'Available once the thread's worktree is set up.' and could not be used. The dev DB showed run 1 interrupted with workspacePreparation {type:worktree, baseRef:dev, startFromOrigin:true}, run 2 completed with no workspacePreparation, and the thread worktreePath null, which matches the reporter's evidence. The server's refusal is covered by a new test in ThreadFork.execution.test.ts ('a side chat opens once a later run has worked in the project checkout'), which fails on main with 'Thread … has no worktree yet. Start the side chat once its worktree is set up.'

Evidence

Same scripted steps on fresh databases: create a dev repository with an unresponsive remote, start New worktree from dev (remote), cancel at Fetch base branch using the setup card's Cancel action, complete a replay turn in the project checkout, then type /side. On head, submit a side-chat message and verify the pane opens. Chromium, 1280×800, light theme. The replay returns recorded content rather than literal ok.

Proven: the scenario fails on base and passes on head; scenarios:prove exited 0.

Before — a6d676fd9a After — a0e8182841
Result Side command remains blocked after the checkout run completes. Side command is enabled and opens a side chat.
Fetch base branch Before: fetching base branch After: fetching base branch
Side command after checkout run Before: side command blocked After: side command enabled
Failure capture Before: blocked command remains visible
Side chat open After: side chat opened
Recording Before recording After recording

Scenario source. Recordings redact local paths during replay output; the after recording ends before fixture cleanup.

The exact Stop-button route remains unverified: the initial base attempt interrupted the run but left setup active, so the successful comparison uses setup Cancel on both commits.

AI-assisted; verified with the tests and tools listed above.

Not shown:

  • Exact Stop-button reproduction: the initial base attempt interrupted the run but left the setup card active and blocked the follow-up. The successful comparison uses the setup card's Cancel action on both commits.
  • Mobile and desktop-specific behavior was not exercised; this proof covers the real web client and server.

Tests

apps/server: vp test run src/orchestration-v2/ThreadFork.execution.test.ts -t 'project checkout' with the previous helper: fails (fork accepted after retry failed again). With the change: vp test run ThreadFork.execution.test.ts runtimeLayer.test.ts ThreadLaunchService.test.ts: 216/216 pass. apps/web: vp test run src/components/ChatView.logic.test.ts: 51/51 pass. apps/mobile: vp test run src/features/threads/worktree-setup-state.test.ts: 4/4 pass. vp run check:changed: first run failed formatting only, fixed with vp fmt; rerun: format, lint, knip, graphql, typecheck (37 packages), tests (11 files), react compiler: all pass.

Not in this change

  • Orchestrator.worktreeSetupPending has the same any-run shape, but changing it breaks deliberate, tested queue behaviour: messages queued for the worktree stay held even after the user starts a run in the project checkout (runtimeLayer.test.ts 'keeps held agent messages while the thread's worktree setup is unresolved', ThreadLaunchService.test.ts 'holds a message queued during setup that arrives after the setup failed/interrupted'). Whether those held messages should be released once a later run has worked in the checkout is a separate product decision.
  • Orchestrator.worktreeSetupPending keeps its any-run shape: existing tests (runtimeLayer 'keeps held agent messages while the thread's worktree setup is unresolved', ThreadLaunchService 'holds a message queued during setup that arrives after the setup failed/interrupted') deliberately keep worktree-bound queued messages held after the user continues in the project checkout; changing that is a separate product decision.
  • Orchestrator.worktreeSetupPending (the queue hold) keeps its any-run shape. Existing tests (runtimeLayer 'keeps held agent messages while the thread's worktree setup is unresolved', ThreadLaunchService 'holds a message queued during setup that arrives after the setup failed/interrupted') deliberately keep worktree-bound queued messages held after the user continues in the project checkout; changing that is a separate product decision.

AI-assisted; verified with the tests and tools listed above.

The parent's first run asked for a new worktree and was interrupted during
setup; a later run completed in the project checkout. The side chat fork is
still refused with "has no worktree yet".

AI-assisted; verified with the tests and tools listed above.
A first run stopped or failed during worktree setup kept
orchestrationV2WorktreePending true for the life of the thread, even after
later runs worked in the project checkout, so web, mobile and the server's
thread.fork refused side chats forever. The setup request now stops counting
once a later run has started.

AI-assisted; verified with the tests and tools listed above.
Retrying a failed setup puts the same run back into preparing; a later run
that worked in the project checkout must not exempt it.

AI-assisted; verified with the tests and tools listed above.
A setup retry can fail again after a later run completed in the project
checkout. Order runs the way the server does (latestExecutedRun) instead
of by ordinal, so side chats stay refused exactly while the server holds
messages for the worktree. The helper moves next to latestExecutedRun.

AI-assisted; verified with the tests and tools listed above.
@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Nov 12, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at a0e8182

Macroscope's review found this PR approvable — Side chats can open once later execution has superseded an abandoned worktree setup, rather than remaining blocked by historical failures. Client and server checks stay aligned, and the pending-worktree guard remains in place for the latest relevant run.

You can add or adjust custom eligibility rules. Learn more.

@github-actions

Copy link
Copy Markdown
Contributor

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 21.5 KiB 21.5 KiB 0 B (0.0%) 24.4 KiB ✅
Codex Live turn messages 1 1 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.9 KiB 21.9 KiB 0 B (0.0%) 24.4 KiB ✅
Claude Live turn messages 1 1 0 (0.0%) 8 ✅

Baseline: a21b43f · PR result: a0e8182 · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@coderabbitai

coderabbitai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The pending-worktree helper moved from contracts to shared orchestration code. It now checks the latest executed run. Mobile, server, and web callers use the shared helper, and side-chat tests cover subsequent runs after interrupted worktree setup.

Changes: Worktree pending-state behavior

Layer / File(s) Summary
Shared helper and caller updates
packages/contracts/src/orchestrationV2.ts, packages/shared/src/orchestrationV2ThreadError.ts, apps/mobile/src/state/use-thread-composer-state.ts, apps/server/src/orchestration-v2/Orchestrator.ts, apps/web/src/components/ChatView.tsx
The helper was removed from contracts and added to shared orchestration code. It checks the latest executed run for worktree preparation and a preparing, failed, or interrupted status when no worktree path exists. The mobile, server, and web callers now import it from the shared module.
Side-chat tests
apps/server/src/orchestration-v2/ThreadFork.execution.test.ts
Tests cover a successful fork after an interrupted setup is followed by a completed checkout run, and rejection when a later run is preparing or failed. The test fixture now sets completion timestamps for failed and interrupted runs.

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Medium

Possibly related issues

Possibly related PRs

Suggested labels: bugfix, size:S

Suggested reviewers: t3dotgg, macroscopeapp

Merge Risk: Minimal · up to a0e81

Side chats are no longer blocked after a later project-checkout run, and retried worktree setups that are still preparing or have failed again remain pending. No merge-blocking risk was identified.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description is thorough and follows the template's sections. It states the problem (side chat refused after a later project-checkout run, caused by a helper that disagreed with the server's latest… 1. Add the Scope and approval justification. Link the triaged issue #34672 and cite the maintainer approval comment for this direction. If no approval exists, explain why this fix qualifies as a very small, focused fix of an obvious bug.
2.…
Passed Passed checks: 3 passed

Check name Status Explanation
Title check Passed The title names the fix area (contracts) and the symptom (side chats opening after an abandoned worktree setup). It is conventional-commit style and matches the changeset's core behavior. However, the…
Linked Issues check Passed Issue #34672 requires that a side chat is not refused permanently when a later run has used the project checkout. The helper at head, packages/shared/src/orchestrationV2ThreadError.ts, returns true on…
Out of Scope Changes check Passed The changes relate to the issue. The helper moved from packages/contracts to packages/shared because it depends on latestExecutedRun, which lives in @t3tools/shared. The three callers change only thei…
Full details: Title check

Explanation

The title names the fix area (contracts) and the symptom (side chats opening after an abandoned worktree setup). It is conventional-commit style and matches the changeset's core behavior. However, the change mainly touches the shared helper and its callers across web, mobile, and server. It moves the helper out of contracts, so the scope label "contracts" is only partly accurate. The title still describes the main bug fix, so it passes.

Full details: Linked Issues check

Explanation

Issue #34672 requires that a side chat is not refused permanently when a later run has used the project checkout. The helper at head, packages/shared/src/orchestrationV2ThreadError.ts, returns true only when worktreePath is null and the latest executed run (chosen by latestExecutedRun/runRanAfter) is preparing, failed, or interrupted with workspacePreparation.type === "worktree". Case 1 from the issue (run 1 interrupted, later run 2 completed in the project checkout) now returns false, so the side chat is allowed. Case 2 (a retry still preparing, or failed again after a later completed run) still returns true, so the side chat remains refused. The old any-run check in packages/contracts was removed, and its three callers (web ChatView, mobile use-thread-composer-state, server Orchestrator.dispatchThreadFork) now import the new helper. The PR adds a ThreadFork.execution.test.ts case for the reported scenario; the author reports it fails on the previous head and passes at this head. Queue-hold behaviour in Orchestrator.worktreeSetupPending is excluded by the PR and by the issue scope, so it is not required here. I did not re-run the tests; the pass result rests on the source evidence and the reported test results.

Full details: Out of Scope Changes check

Explanation

The changes relate to the issue. The helper moved from packages/contracts to packages/shared because it depends on latestExecutedRun, which lives in @t3tools/shared. The three callers change only their import path. The ThreadFork.execution.test.ts fixture sets completedAt for finished runs, which the new ordering logic needs. The new tests cover the reported case and the retry cases. No unrelated behaviour change appears in the summary. The Orchestrator.worktreeSetupPending queue hold is deliberately left unchanged and is documented as out of scope. The 'Evidence' section is a placeholder, which is documentation and not a code change.

Full details: Description check

Explanation

The description is thorough and follows the template's sections. It states the problem (side chat refused after a later project-checkout run, caused by a helper that disagreed with the server's latestExecutedRun ordering), describes the change (new orchestrationV2WorktreePending in packages/shared, moved from contracts, with callers updated), and links the issue #34672. It gives reproduction steps on main, before/after evidence with screenshots and recordings, and a list of tests run with results. The Scope and approval section is missing: it does not link a triaged issue with maintainer approval or explain an exemption. The description also says it covers mobile behavior that was not exercised, which is disclosed. The 'Not in this change' section repeats the same bullet three times and should be cleaned up. The closing AI-assistance note does not name the model and harness, as the template asks. The Problem section is not a separate heading, though the content is present in the body.

Resolution

  1. Add the Scope and approval justification. Link the triaged issue #34672 and cite the maintainer approval comment for this direction. If no approval exists, explain why this fix qualifies as a very small, focused fix of an obvious bug.
  2. Add explicit '## Problem' and '## Change' headings. Move the problem statement and the change explanation under them.
  3. Add the model and harness used for the AI-assisted work at the end of the Verification section, as the template requires.
  4. Remove the two duplicate 'AI-assisted; verified with the tests and tools listed above.' lines and the three duplicate entries in 'Not in this change'. Keep one entry.
  5. Keep the existing reproduction and evidence sections. They satisfy the UI screenshot and verification requirements.
Finishing Touches

Generate unit tests (beta)

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between a21b43f and a0e8182.

Files selected for processing: 6

  • apps/mobile/src/state/use-thread-composer-state.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/ThreadFork.execution.test.ts
  • apps/web/src/components/ChatView.tsx
  • packages/contracts/src/orchestrationV2.ts
  • packages/shared/src/orchestrationV2ThreadError.ts
Files with no reviewable changes: 1

  • packages/contracts/src/orchestrationV2.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

@juliusmarminge
juliusmarminge merged commit d63db06 into main Nov 12, 2026
37 checks passed
@juliusmarminge
juliusmarminge deleted the factory/issue-34672 branch November 12, 2026 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Side chat permanently blocked after a thread's first worktree setup is interrupted

1 participant