[trevoedwards]

Problem

After an ACP thread is rolled back (or ensureThread forces a new native thread), the replacement runtime's events are never consumed. flavor.onSessionEvent stops firing for that thread, the runtime's event queue grows for as long as it lives, and on agents with cancelBehavior: "wait-for-prompt" Stop waits on a drain barrier nobody acknowledges until the 15s cancel timeout kills the agent and Stop fails with "The ACP agent did not finish cancellation".

Cause, in packages/provider-acp/src/server/adapter.ts: wireAcpRuntimeHandlers forks the getEvents() consumer into runtimeScope ?? sessionScope. In the replacement path it is called while runtimeScope still points at the previous runtime's scope; runtimeScope = replacementScope is only assigned afterwards, and the same transition then closes previousScope, interrupting the consumer it just started.

Change

wireAcpRuntimeHandlers takes an optional scope, and the replacement path passes replacementScope, so the consumer lives as long as the runtime it reads. The initial start and restart paths already assign runtimeScope before wiring and are unchanged. I passed the scope explicitly rather than moving the assignment earlier because the replacement path only swaps runtimeScope after confirming the replacement has not already terminated; assigning it earlier would make a terminated replacement's scope the active one.

Scope and approval

A very small, focused fix for an obvious bug: the replacement path forks a fiber into a scope it closes a few lines later, unlike the two other paths that wire handlers. No behavior change beyond making the replacement runtime's event handling work like the original runtime's.

Verification

• Extended the existing test replaces the ACP session and clears conversation state on rollback (apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts) to count onSessionEvent calls and drain the replacement runtime's events after the post-rollback turn.
  • Before the fix: the test hangs in drainEvents (vp test run src/orchestration-v2/Adapters/AcpAdapterV2.test.ts -t "replaces the ACP session and clears conversation state on rollback" --testTimeout 20000 → Test timed out in 20000ms).
  • After the fix: it passes, as do all 114 tests in AcpAdapterV2.test.ts and AcpRegistryAdapterV2.test.ts.
• vp run check:changed passes (fmt, lint, knip, typecheck, affected tests).
• Not checked: a real ACP agent end to end; the test uses the repo's mock ACP agent.

Claude Opus 5.5 in cooperation with Trevor.

[macroscopeapp]

Approvability

Verdict: Approved at 31fab0f

Macroscope's review found this PR approvable — Replacement ACP runtimes now keep their event subscriptions in their own lifecycle scope, preventing old-runtime cleanup from stopping subsequent events. The change is narrowly contained and preserves existing cleanup and callback safeguards.

^{You can add or adjust custom eligibility rules. Learn more.}

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The ACP adapter now associates replacement-runtime event-stream fibers with the replacement scope. The rollback test checks that the replacement runtime emits session events after a second turn.

Changes: Replacement ACP runtime event handling

Layer / File(s)	Summary
Replacement scope wiring and regression test packages/provider-acp/src/server/adapter.ts, apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts	Handler wiring accepts an optional scope and uses it before the runtime or session scope. Replacement-runtime wiring supplies its scope. The rollback test counts session events and checks for an increase after the second turn.

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

Change: Bug fix

Suggested reviewers: juliusmarminge, maria-rcks

Merge Risk: Minimal · up to dd157

Replacement runtime event consumers now remain attached to the active replacement runtime across rollback and forced thread replacement.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title is concise, specific, and accurately describes the primary fix for ACP threads after rollback.
Description check		The description covers the problem, cause, fix, scope rationale, verification results, limitations, and agent attribution. It is sufficiently complete for review.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Recent review info

Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: b7ab02e
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: a068f888-a3c0-472c-8de7-cd74c84656c6

Commits

Reviewing files that changed from the base of the PR and between 7af52dc and dd157a2.

Files selected for processing: 2

• apps/server/src/orchestration-v2/Adapters/AcpAdapterV2.test.ts
• packages/provider-acp/src/server/adapter.ts

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

[maria-rcks]

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified head 7af52dc433cda93a7e56c70b07f21e0db72cd1fd on linux with repository playwright and /usr/bin/google-chrome. installed dependencies and ran main and the exact pr head sequentially, with isolated state and app-created projects and threads. no mocks or source changes.

native codex gpt-6.1-sol completed a turn on both revisions. native codex uses a different adapter, so the affected path was verified with real fx 0.0.16 acp turns using gpt-6.1-sol through the local proxy and fx's supported custom provider configuration.

reproduction: complete a turn, send another prompt, stop, choose "edit from here" and "revert and keep changes", resubmit, then stop the replacement runtime. server traces show:

main d8a829e5ec: initial stop          23.46 ms  success
main d8a829e5ec: stop after rollback 15003.58 ms interrupted at timeout
pr 7af52dc433:   stop after rollback   11.52 ms  success
pr 7af52dc433:   repeated rollback    14.10 ms  success

the main ui still displayed "stopped by user" despite the server timeout. server traces distinguish the failure from the pr result. after the pr rollback and stop, a follow-up real tool turn requested approval, ran git rev-parse --is-inside-work-tree, and returned ACP_TOOL_OK.

the existing rollback regression test passed on blacksmith: 1 passed, 84 filtered out. existing head ci checks pass.

short server verification transcript

Not verified

native electron, ios and android clients; antigravity and fx oauth; remote relay and tunnel connections; the separate forced-new-thread path through ensureThread. these were outside the available linux client and provider setup.