Repository navigation
fix: Closing the gaps for the ACP v2 support - #744
KirillAlekhnovich wants to merge 8 commits into
Conversation
![jetbrains-air[bot]](https://avatars.githubusercontent.com/in/3314082?s=60&v=4){kind=small}
| const queue = this.getSteeringQueue(params.sessionId); | ||
| try { | ||
| return await queue.enqueue(params); | ||
| return await queue.enqueue(params, insertion); |
There was a problem hiding this comment.
Blocking: A v2 follow-up now waits in the session's SteeringQueue while it holds its turn-start slot. A steer that falls back to starting a turn takes its slot only inside the queue handler, in startNewTurnFromExternalPrompt. That covers _session/steering without promptRequired, and async-question answers, which make the same call through AsyncQuestionClient.sendReply. If such a steer is queued ahead of a v2 follow-up, it gets a slot behind that follow-up and waits for it. The follow-up is waiting for the steer to leave the queue, so the session deadlocks.
I reproduced this in a v2 harness test:
- A turn is running. Follow-up B arrives by
session/prompt, gets steered, and waits in the queue for insertion. - A
_session/steeringrequest S (an async-question answer works the same way) queues behind B. - Follow-up C arrives.
promptV2reserves C's slot, then C queues behind S. - The user sends
session/cancel. The turn endsinterruptedand B gets-32800. BecausecancelRequestedis set, S finds no steerable turn, reserves a slot behind C, and waits for C, which is still behind S in the queue.
What the client sees: S and C never get a response and no new turn starts. A later session/prompt D queues behind them and also hangs, so the session is stuck until it is closed. This PR introduced it: on main, promptV2 never enters the steering queue. The trigger needs a fallback steer queued behind a pending follow-up. That is more likely now, because async-question forms stay open after their turn ends.
Fix: make the steering queue and the turn-start FIFO put entries in the same order.
- In
executeOrQueueSteeringRequest, acquire the turn-start reservation when the steer is enqueued, for steers with noinsertionand nopromptRequired. - Pass it through
performSteeringRequest→startNewTurnFromSteering→startNewTurnFromExternalPromptinstead of acquiring a new one there. - When the steer injects or fails, release it in FIFO order with
void reservation.wait.then(() => reservation.release()), aspromptV2already does.
I applied this temporarily and checked:
- The reproduction settles: B and C get
-32800, S getsstartedNewTurn. steer-events.test.ts› "serializes concurrent late steering requests" still passes.- The full
npm testpasses: 2509 passed, 39 skipped.
Produced by Air Automations. Name: Codex ACP Code Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/deba358f-772c-4cff-bf03-c95d1670abe9?run=a2f48558-b6ac-4772-bdc5-c24f9612c3f2
Use native steering and track insertion separately from the active turn response. Keep internal plan approval prompts out of user message updates and restored history. Keep asynchronous questions alive until an answer, rejection, or cancellation. Preserve companion metadata so custom text remains beside its question. Type checking passed. The full suite passed 2468 tests and skipped 39 tests. Four failures also occur on the unchanged HEAD and are outside this change.
The current main sends the subagent capabilities before its state. It also combines terminal output with the completion update. Refresh the three snapshots without changing their expected content. Use unlink for a directory symlink so cleanup works on this Node version. Validation: 2502 tests pass. Type checking and the build pass.
Keep the AIR subagent and async task extension names on ACP v2. Air uses these events to show child sessions and background work. The previous conversion changed the names and hid those events from the client. Cover live updates, history replay, task stops, and the extension guard. The capability gates stay the same on both protocol versions. Tests: - npm run typecheck - npm test: 2507 passed, 39 skipped - npm run build
The subagent tests emitted live events before replay state publication finished. The scheduler then changed the update order or delayed terminal output until completion. Wait for the restored child directory before emitting more work. Drain the output delta before the completion event. Refresh the v1 and v2 snapshots for these explicit boundaries. Validation: - Unit tests: 2507 passed, 39 skipped, with retries disabled. - The live subagent suite passed ten iterations per test without retries. - Integration tests: 19 passed. - Type checking, the JavaScript build, and all six binary builds passed. - The checks used Node 24.22.2 and Bun 1.3.11.
b4ac1bf to
aba05c9
Compare
Reserve fallback turn slots before steering requests enter the queue. Release unused slots in order. Cover cancellation with a queued steer and with an asynchronous question answer.
Prefix the Air subagent and task update tags on v2. Keep the v1 tags unchanged. Reject unprefixed private updates in tests. Retain both form metadata locations for Air clients.
Extract the repeated user message insertion step. Keep the queue recovery scenario within the function length guideline. The affected steering and question suites pass.
…ejection Keep reservation cleanup in the handled prompt promise chain. A separate finally promise could reject after the caller had handled the original failure. Cover a rejected asynchronous answer in ACP v1 and v2. Check that the warning reaches the client and the next prompt can start.
c0ceda9 to
96c7820
Compare
| if (turnStillActive && !this.isNoActiveTurnToSteerError(err)) { | ||
| throw err; | ||
| } | ||
| if (turnStillActive && !this.isNoActiveTurnToSteerError(error)) throw error; |
There was a problem hiding this comment.
Problem: a v2 prompt sent while /review runs now fails instead of waiting for the review to finish.
How it happens:
- The user runs
/review(or/review-branch <x>,/review-commit <x>). The review turn is running and holds the turn-start slot. - The user sends an ordinary prompt.
promptV2seesneedsWaitand callssteerOrWaitForV2PromptwithcanSteer = true. tryInjectSteeringRequestwaits only forcompact(line 3122), so it sendsturn/steerinto the review turn.- Codex rejects it with
cannot steer a review turn(activeTurnNotSteerable;NonSteerableTurnKindis"review" | "compact"). - The turn is still active and this is not "no active turn to steer", so this line rethrows.
executeOrQueueSteeringRequestturns it into a v2 error.
Impact: the client gets -32600 for the follow-up, and it never runs. Any prompt during a review hits this. The PR introduced it: on the base commit the same prompt waits in the turn-start queue and starts a new turn once the review ends.
Fix: treat Codex's not-steerable error like "no active turn to steer" here, so the caller waits for its turn-start slot:
if (turnStillActive && !this.isNoActiveTurnToSteerError(error) && !this.isActiveTurnNotSteerableError(error)) throw error;This also covers review turns the adapter did not start itself. Async answers and _session/steering without promptRequired would then start their turn after the review, instead of failing. I checked it with a temporary v2 harness test: /review runs, turn/steer throws cannot steer a review turn, and a follow-up prompt arrives. Without the fix the prompt gets -32600. With the fix it starts a turn after the review and resolves with its messageId, and the full suite still passes. Please add that case next to compact-command-wire.test.ts › "holds the next prompt until the compact turn ends".
| Codex app-server owns shell commands that keep running after their tool call. | ||
| The adapter exposes them as async tasks when the client declares `asyncTasks`. | ||
| Without the capability, the adapter sends no async task update. | ||
| ACP v1 and v2 use the same `async_task_spawned`, `async_task_progress`, and `async_task_state_update` names and payloads. |
There was a problem hiding this comment.
This line and line 1004 ("AIR receives subagent_spawned and subagent_state_update with the same names ... on ACP v1 and v2") are no longer true after 447c97d. On v2, toV2SessionUpdate sends _async_task_* and _subagent_update, and readme-dev.md already says so. Please say that v2 uses the _-prefixed tags with the same payloads, or remove both lines.
| }); | ||
|
|
||
| if (protocolVersion === 2) { | ||
| it('delivers an async answer queued between cancelled follow-ups', async () => { |
There was a problem hiding this comment.
This repeats prompt-v2-steering.test.ts › "keeps fallback steering ahead of later prompts after cancellation" in the same harness. sendReply makes the same executeOrQueueSteeringRequest call (no insertion, no promptRequired) as _session/steering, and the steps are the same: a pending follow-up, a queued fallback, a later prompt, cancel, then the fallback starts turn 2. The idle-answer test above already covers sendReply starting a turn. Please drop this test, or make the steering test an it.each over the two sources of the fallback. That saves about 28 lines.
| const air = asRecord(asRecord(root[JETBRAINS_META_KEY])[AIR_META_KEY]); | ||
| const marker = air[AIR_CUSTOM_ANSWER_KEY] === undefined | ||
| ? root[LEGACY_AIR_CUSTOM_ANSWER_KEY] : air[AIR_CUSTOM_ANSWER_KEY]; | ||
| return marker === true || Object.keys(asRecord(marker)).length > 0; |
There was a problem hiding this comment.
The adapter only ever writes this marker as true (CodexElicitationHandler.ts, both the jetbrains.air and the legacy root spelling), so the object case is never produced. return marker === true; is enough.
| threadId: this.sessionId, | ||
| turnId, | ||
| /** Sends the reply through the session's shared steering and turn-start queue. */ | ||
| async sendReply(text: string): Promise<void> { |
There was a problem hiding this comment.
On ACP v1, an answer that arrives when the session is idle now starts a full turn with no session/prompt in flight. The v1 client then gets streamed output with no prompt it can show as busy. Turns that Codex starts by itself already stream like this on v1, so this may be fine. Can you confirm this is the intended v1 behavior for Air, and say so in docs/async-questions.md? It currently does not say how the two versions differ.
Why
Close ACP v2 gaps in Air input handling and agent lifecycle updates.
Related: IJAI-6021, IJAI-6023.
What
Validation
Notes
The full IDE manual checklist was not repeated for this revision.