[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button - #4200

Closed
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start
Closed

[Bug]: Unsupported attachment (e.g. HEIC) permanently wedges the turn in "working" with a dead stop button#4200
dmstoykov wants to merge 8 commits into
pingdotgg:mainfrom
dmstoykov:fix/adapter-validate-before-turn-start

Conversation

@dmstoykov

@dmstoykovdmstoykov commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What Changed

sendTurn now validates attachments and builds the user message before mutating any turn state, in both adapters that had the wrong order.

  • ClaudeAdapter.sendTurn: buildUserMessageEffect (which validates attachment mime types) is hoisted above session.status = "running", the activeTurnId assignment, and the turn.started emission.
  • CursorAdapter.sendTurn: same hoist for its attachment-id and empty-prompt validation.
  • A validation failure now leaves the session completely untouched — no status change, no activeTurnId, no turn.started — so ProviderCommandReactor's existing recoverTurnStartFailure path produces a clean ready state with the error surfaced.
  • Codex, OpenCode, and Grok adapters were audited and already validate before emitting (or handle failure synchronously in-fiber), so they are unchanged.
  • Tests: regression tests in both adapter suites asserting that an unsupported attachment yields the request error, emits noturn.started, and leaves the session ready with no activeTurnId — plus that a subsequent valid turn emits exactly one turn.started with the correct turn id. ClaudeAdapter.test.ts 60/60, CursorAdapter.test.ts 19/19.

Why

Pasting an image the provider rejects (e.g. a HEIC screenshot from an iPhone, which passes the composer's image/* check) permanently wedges the thread: it shows "working" forever and the stop button does nothing.

The cause is an ordering race, not the validation itself. Because turn.started is emitted before validation runs, a validation failure leaves an event already in flight. recoverTurnStartFailure correctly resets the session to ready, but ProviderRuntimeIngestion processes that stale turn.started asynchronously; when it lands after the recovery it flips the session back to running with an activeTurnId for a turn that can never finish — the message was never queued, so no turn.completed can ever arrive, and query.interrupt() has nothing to interrupt.

Reordering is the smallest fix that removes the race at its source: if the failure happens before any state is published, there is no stale event to lose the race to, and no new recovery machinery is needed. The alternative — emitting a compensating turn-end event on failure — adds a second event path to keep correct and still leaves a window where the UI shows a phantom running turn.

UI Changes

No UI code changed. The user-visible difference is that a rejected attachment now surfaces its error and returns the thread to ready, instead of leaving it stuck in "working" with an unresponsive stop button.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Medium Risk
Changes core provider turn lifecycle, event ordering, and concurrent sendTurn settlement in Cursor—high user impact if wrong, but scope is adapter-layer with extensive new regression tests.

Overview
Fixes threads stuck in working when a provider rejects an attachment (e.g. HEIC): validation used to run afterturn.started and session turn state were published, so async ingestion could flip the session back to running with no matching turn.completed.

ClaudeAdapter moves buildUserMessageEffect (including mime-type checks) before marking the session running, setting activeTurnId, or emitting turn.started.

CursorAdapter does the same for prompt/attachment validation, and expands turn handling for overlapping sendTurn calls: early turnId reservation while promptsInFlight is tracked, turn.started only after validation (first surviving prompt announces), and an onExit drain that emits a matching turn.completed (including failed) when the last prompt exits without a normal settle—covering steer-during-prep, reserver prep failure, interrupt, and first-prompt ACP failure.

The ACP mock agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS and T3_ACP_FAIL_FIRST_PROMPT; adapter tests lock in no phantom turn.started, clean session state after validation errors, and merged-turn boundaries.

Reviewed by Cursor Bugbot for commit dd18ff4. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Fix unsupported attachments wedging turns in 'working' state with a dead stop button

  • In ClaudeAdapter.ts, reorders sendTurn to build and validate the user message before emitting turn.started or mutating session state, so unsupported attachments (e.g. HEIC) return an error without leaving a stuck turn.
  • In CursorAdapter.ts, reserves the active turn ID and increments promptsInFlight immediately so concurrent sendTurn calls steer into the same turn, and defers turn.started emission until after validation succeeds.
  • CursorAdapter now tracks activeTurnStarted, activeTurnSettled, and activeTurnLastStopReason on session context to guarantee exactly one turn.started per turn and always settle an announced turn even when the last prompt holder exits without resolving.
  • acp-mock-agent.ts gains T3_ACP_FAIL_FIRST_PROMPT and T3_ACP_SET_CONFIG_OPTION_DELAY_MS env vars to support testing these failure modes.
  • Behavioral Change: failures during message construction no longer emit turn.started, so the UI stop button will not appear and the turn will not appear to be running when attachment validation fails.

Macroscope summarized dd18ff4.

ClaudeAdapter.sendTurn (and CursorAdapter.sendTurn, which had the same
ordering) marked the session running, set activeTurnId, and emitted
turn.started before validating/building the outgoing user message. When
validation failed (e.g. an unsupported Claude image mime type, or an
unresolvable Cursor attachment id), the already-emitted turn.started
could race the error-recovery path and leave the session stuck
"running" with a turn that can never complete — a dead stop button.
Reorder both adapters so message/prompt validation runs before any
turn-state mutation or event emission, so a validation failure leaves
the session untouched and the existing recovery path produces a clean
"ready" state with the error surfaced.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitaiBot commented Jul 20, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5c613fe2-dc84-49a2-b6be-da0b133359d3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Jul 20, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
@macroscopeapp

macroscopeappBot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This bug fix introduces significant turn lifecycle state management changes in CursorAdapter with subtle edge case handling. An unresolved review comment identifies a potential bug where turn completion events may fire after session exit during stopSession, potentially corrupting session state.

You can customize Macroscope's approvability policy. Learn more.

A concurrent sendTurn arriving while the first prompt awaited session
configuration or attachment I/O saw promptsInFlight > 0 but activeTurnId
still unset, so it minted a second turn id and emitted a second
turn.started. Only the last prompt to resolve emits turn.completed, so
the other turn id never settled.
Reserving the turn id in the same synchronous step as the counter that
gates steering closes the window. The reservation is adapter-internal;
the session snapshot and turn.started emission stay after validation.
Reported by Macroscope on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…endTurns
Reserve the turn id in the same synchronous step as the in-flight counter
that gates steering: a concurrent sendTurn arriving while the first prompt
awaits session configuration or attachment I/O previously minted a second
turn id and emitted a second turn.started that never settled.
The reservation alone reintroduces two hazards when the reserving prompt
fails preparation after a steer has joined its turn:
- The steered prompt used to skip turn.started (it saw itself as steering
an already-announced turn), completing a turn that never started. Now
whichever prompt survives preparation first announces the turn.
- The steered prompt could resolve while the failing prompt still held the
in-flight count, so neither emitted turn.completed and the turn wedged.
The last prompt to drain now settles an announced-but-unsettled turn
with the last known stop reason.
A reservation that never announced a turn is dropped on drain so the next
sendTurn opens a fresh turn instead of steering into a dead one.
The mock ACP agent gains T3_ACP_SET_CONFIG_OPTION_DELAY_MS to hold a
prompt inside its preparation window; a same-value model selection is
skipped runtime-side, so the regression tests request a model change to
force a real session/set_config_option round-trip.
Addresses the concurrent-steer race and failed-prep findings on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@github-actionsgithub-actionsBot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Jul 21, 2026
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts
The drain-time settle emitted turn.completed with state "completed"
whenever the last prompt exited, including when every prompt of the
announced turn failed and no stop reason was ever recorded. That
reported a failed turn as successful: ingestion moved the session to
ready, racing the caller's turn-start failure recovery and masking the
real error.
Gate the drain settle on a recorded stop reason. When no prompt of the
merged turn resolved, the error keeps propagating to each caller and
failure recovery settles the thread, as before the drain settle existed.
The mock ACP agent gains T3_ACP_FAIL_FIRST_PROMPT so the regression test
can fail the announced turn's only prompt while the follow-up turn
succeeds.
Addresses cursor bot's drain-settle finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…mpt fails
The drain-time settle previously chose between two bad outcomes when all
prompts of an announced turn failed: emitting state "completed" reported
the failure as success (flipping the session to ready and masking the
error), while emitting nothing left the turn.started unanswered and
wedged the thread in "working" — the turn-start failure recovery can
lose that race when turn.started is ingested after it runs.
Settle as state "failed" with the drained prompt's error message
instead. The emission trails the turn.started already in the event
queue, so ingestion deterministically ends in the error state regardless
of how the recovery interleaves. Interrupt-only drains (session
teardown) still skip settling and leave it to session lifecycle events.
Addresses cursor bot's re-wedge finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment threadapps/server/src/provider/Layers/CursorAdapter.ts Outdated
…esult on interrupt
The interrupt-only drain guard returned before consulting the recorded
stop reason, so interrupting the last in-flight prompt after a sibling
of the merged turn had already resolved skipped settling entirely and
left the announced turn open.
Scope the interrupt guard to the no-recorded-result branch: an
interrupted drain with no result is session teardown and stays with
session lifecycle events, but a recorded sibling result is the turn's
outcome and now settles regardless of how the last holder exited.
Addresses cursor bot's interrupt-skip finding on PR pingdotgg#4200.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

});
return;
}
ctx.activeTurnSettled = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interrupt settle resurrects stopped session

Medium Severity

The new onExit path settles an announced turn with a recorded sibling result even when the last holder exits via interrupt-only, and it never checks ctx.stopped. During stopSession, scope close can interrupt a still-preparing holder after a sibling already recorded a result, so turn.completed can publish after session.exited. Ingestion then maps that completion to ready, undoing the stopped session.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit dd18ff4. Configure here.

@juliusmarminge

Copy link
Copy Markdown
Member

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

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

Labels

size:L100-499 changed lines (additions + deletions).vouch:unvouchedPR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@dmstoykov@juliusmarminge@foldimitar