Repository navigation
fix(acp): retain command streaming until the executor returns - #63097
paulovlobato wants to merge 5 commits into
Conversation
github-actions
Bot
added
the
needs:compliance
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes the case in #53914: with a long-running registered command executor, ACP now keeps streaming across execution cycles until the executor returns. I checked this with the wire fixture: the 3 new tests fail on the base and pass here, and the full ACP suite (105 tests) and session-activation.test.ts pass. I couldn't run the package typecheck scripts here.
The common case from #54371 is still broken. Markdown/template slash commands call session.prompt, which only admits the prompt and wakes the session, so /command returns before ACP has seen execution.started. execute then returns end_turn straight away, as before. A wire test that answers /command immediately and streams the execution after it still gets end_turn with no output. See the inline comment.
Public API (plugin), backwards compatible:
- Promise
CommandDefinition.execute(input)→execute(input, { signal }). Existing one-argument executors keep working. - New
sessionhook"interrupt"({ sessionID }) in both the Effect and PromiseSessionHooks. It runs from coreSession.interrupton every interrupt, including idle sessions, after waiting for plugin activation. Nothing in this PR uses it yet.
Other concerns (inline): windDown no longer interrupts the server execution, closes open tool calls or drains it when the transport goes away, and a command turn now starts on any delivered inbox item in the session. The two new ACP tests that rely on 20 ms setTimeout waits may be flaky. The "late request cancel" test is hard to follow and would be clearer as a smaller case.
Related: #54371
| if (prompt.command) { | ||
| yield* Deferred.succeed(turn.commandFinished, undefined) | ||
| const state = yield* Ref.get(turn.state) | ||
| if (state.executing) return yield* Fiber.join(events) |
There was a problem hiding this comment.
This depends on the event fiber having already folded session.execution.started when /command returns, and that isn't guaranteed. Markdown/template commands call session.prompt, which admits the prompt, wakes the session and returns. So the HTTP response usually arrives before inbox.delivered/execution.started reach the subscription: executing is false, and we return state.terminal ?? "succeeded" immediately. That's the #54371 symptom (end_turn in ~30ms, no output). A wire test that answers /command with 204 and then streams one execution reproduces it. A registered executor whose last cycle starts just before it returns can hit the same race. Could the command's completion be tied to something ordered on the event stream (e.g. waiting for the session to go idle after the executor returns), rather than a snapshot of state.executing?
| events: Fiber.Fiber<ACPTranslate.Terminal, ACPError.Failure>, | ||
| ) { | ||
| yield* Deferred.succeed(turn.cancelled, undefined) | ||
| if (!cancellationRequests.has(turn.ctx.sessionID)) return |
There was a problem hiding this comment.
This changes behaviour for every turn, not just commands. Before, any wind-down (including transport close or scope disposal) interrupted the server execution, drained it, and closed open tool calls through abandon. Now that only happens after an explicit session/cancel, $/cancel_request or close. When the client disconnects, the server execution keeps running. If that's intended, it should be stated in the description and covered by its own test. It's broader than the command-streaming fix.
| } | ||
| if (!sessionID || (sessionID !== ctx.sessionID && !child)) return { state, outputs: [] } | ||
| if (event.type === "session.inbox.delivered" && event.data.inboxID === ctx.start.id) | ||
| if (event.type === "session.inbox.delivered" && (event.data.inboxID === ctx.start.id || ctx.command)) |
There was a problem hiding this comment.
With ctx.command, any session.inbox.delivered in the session starts the turn. That includes a prompt another client (TUI, a second ACP connection) sends during the command, so its output gets streamed into this ACP turn. Can the command's own inbox items be matched more narrowly?
|
Addressed the review:
I did not change the existing 20 ms waits or the late-cancel test in this pass; the new test uses the same harness. |
There was a problem hiding this comment.
The commit fixes the start race I raised, but it breaks the multi-cycle case from #53914, and two of the PR's own tests now fail (bun test test/acp: 104 pass, 2 fail). The core session-activation tests pass. I couldn't type-check: bun check isn't in this Bun version.
Earlier points:
- Command returning before its prompt starts: resolved. Template commands now stream, and the new test passes.
windDownon transport close: resolved for turns that aren't commands, but the late-cancel test still expects the old behaviour and fails (inline).- Which delivered message starts a command: narrowed to the first one. Messages delivered after the command starts still stream into it. Not blocking given the #49208 direction.
New blocker: "streams every execution submitted by a still-running command" now hangs at await prompt (inline).
The public API changes from my last review are unchanged and backwards compatible: Promise CommandDefinition.execute gets a second { signal } argument, and there's a new session hook "interrupt". The 500 ms grace is a fixed wall-clock wait: a command whose prompt takes longer to start still ends silently with end_turn, and a command that never starts an execution now always waits 500 ms. That's acceptable if documented, but it's a timing heuristic, not ordering.
Related: #54371
| const started = yield* Deferred.await(turn.commandStarted).pipe( | ||
| Effect.timeoutOption(CommandStartGrace), | ||
| ) | ||
| if (Option.isSome(started)) return yield* Fiber.join(events) |
There was a problem hiding this comment.
This hangs when the command's last execution has already finished by the time the executor returns. The event loop hit that terminal while commandFinished wasn't set yet and continued, so it's now waiting for another event that never comes. commandStarted is already done, so we Fiber.join(events) forever. That's exactly "streams every execution submitted by a still-running command", which now times out (it passed on the previous commit). Something like: after commandStarted, return state.terminal when !state.executing, or have the event loop also finish when commandFinished completes while it's idle.
| expect((await prompt).stopReason).toBe("end_turn") | ||
| }) | ||
|
|
||
| test("a late request cancel cannot make the next transport close send a user interruption", async () => { |
There was a problem hiding this comment.
This test now fails (interrupts is ["ses_1"]). Its work prompt isn't a command, and you restored interrupt-on-close for non-command turns, so the expectation no longer matches the intended behaviour. Please update it to what you want now, or drop it. It also relies on a 20 ms sleep and is hard to follow, so a smaller case would be better.
d2386fe to
79060fc
Compare
|
I rewrote this as the focused command-streaming fix and removed the unrelated cancellation/plugin changes, so the
|
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
Three earlier points are fixed: windDown is back to the base behaviour, the multi-cycle hang is gone (its test passes again), and the failing late-cancel test is removed. The cancellation and plugin changes were dropped, so the public API is now unchanged. I ran the ACP suite 4 times: 105/105 in three runs; the first reported one failure I couldn't identify, so it may be a flake.
One problem is still there, and it's the #53914 case: when the executor starts another execution just before it returns, the prompt still resolves early with the previous execution's terminal (inline).
The 500 ms start grace is still a wall-clock heuristic. That's acceptable for now, but an execution that starts later than that is still dropped silently.
| if (prompt.command) { | ||
| yield* Deferred.succeed(turn.commandFinished, undefined) | ||
| const terminal = yield* Ref.get(turn.commandTerminal) | ||
| if (terminal !== undefined && (yield* Deferred.isDone(turn.commandStarted))) { |
There was a problem hiding this comment.
commandTerminal keeps the previous execution's terminal and is never cleared when a new session.execution.started arrives. Here's the sequence: execution 1 finishes, the executor starts execution 2 and then returns. We see terminal !== undefined and commandStarted already done, and return execution 1's terminal at once, so execution 2's output is dropped. A wire test reproduces it: started → text → succeeded, then delivered → started, then answer /command. The prompt resolves before execution 2's text and terminal arrive. Clearing commandTerminal on session.execution.started (around line 228) should be enough to make this fall through to Fiber.join(events). A small test for this order would cover it.
|
Fixed the last blocker: The 500 ms grace is still a wall-clock heuristic, as you noted; a prompt that starts later than that still settles. I've called that out in the description as the remaining #49208-direction limitation. |
There was a problem hiding this comment.
The stale-terminal problem is fixed: commandTerminal is now cleared when an execution starts, and the new wire test covers that order. All four new command tests fail on the base and pass here. The full ACP suite passes (106/106, two runs). I couldn't run the package typecheck scripts here (bun check isn't in this Bun version), and I didn't test with a real ACP client.
The public API is unchanged.
Nothing else blocks this. The 500 ms start grace and the start on the first delivered inbox are documented as known limits pending #49208.
Command prompts return end_turn immediately, before the turn's event stream is joined. Keep the prompt pending until the executor returns, wait for the command's admitted execution to start when it beats the executor, and continue across execution cycles. This is the focused command-streaming part of anomalyco#53913; the unrelated cancellation changes are not included. Original work by @danyel117 in anomalyco#53913. Fixes anomalyco#53914 Refs anomalyco#54371
An executor can start another execution just before it returns. The previous execution's terminal was still cached, so the prompt settled before the new execution's output. Clear it on session.execution.started and cover the order with a wire test.
756243a to
d515846
Compare
There was a problem hiding this comment.
This push is a rebase onto the latest dev. The command-streaming change is the same, adapted to the new background-turn code (optional start, a background follow-up that clears command). The ACP suite passes (108/108, two runs), and all four new command tests fail on the new base. I couldn't run the package typecheck scripts here (bun check isn't in this Bun version).
The public API is unchanged.
Nothing new blocks this. The 500 ms start grace and starting on the first delivered inbox remain the documented limits, pending #49208.
github-actions
Bot
added
the
needs:compliance
|
Confirmed — the 11:10 warning came from an accidental toggle of the checklist. Both items are checked now: |
|
Thanks for the update. The checklist and compliance check don't change my review: |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
Issue for this PR
Closes #53914
Related: #54371
Type of change
What does this PR do?
Rebased from #53913 (auto-closed by the stale-PR cleanup) onto
dev, narrowed to the command-streaming fix. Original work by @danyel117.Command prompts currently return
"succeeded"before the turn's event stream is joined, so ACP clients receiveend_turnimmediately and never see the command's output. This keeps the prompt pending until the executor returns, continues consuming command-owned events across execution cycles, and drains the final execution.A markdown/template command can return before its admitted prompt reaches the event stream. This waits on an
execution.startedsignal with a bounded grace instead of a snapshot, records the command's terminal event so an executor that returns after the last terminal settles instead of hanging, clears that terminal when a new execution starts, and starts the command turn on its first admitted inbox.The 500 ms start grace is a wall-clock bound. A command whose prompt starts later than that still settles; closing that gap needs the command outcome/inbox ID from core (the #49208 direction).
The unrelated cancellation/plugin changes from #53913 are not included.
User-facing report: #54371. Client evidence: stablyai/orca#40748.
How did you verify your code works?
/commandreturns beforesession.execution.started; it streams the expanded output and resolves after the terminal.bun run check: 34 tasks successful.Visual
N/A (no UI change).
Screenshots / recordings
N/A (no UI change).
Checklist
Added by the opencode bot (AI-generated).
Related: #54371
Public API: plugin: Promise command execute gets a second { signal } argument; adds a session "interrupt" hook (backwards compatible)