Repository navigation
Conversation
github-actions
Bot
added
contributor
needs:compliance
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![opencode-agent[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I checked this with the new wire tests: both fail on the base and pass here. The ACP suite passes (104/104), the core activation tests pass, and cli and plugin typecheck cleanly. The approach works: streaming lasts as long as the executor runs, the final run is drained, the abort signal reaches Promise executors, and a metadata.acp.plan validated against a schema becomes ACP plan updates. Main concerns:
- After a command returns, deciding whether to drain depends on how far the event consumer has got. The final run's updates can be dropped (inline comment).
- Any inbox delivery now starts a command turn, so other prompts in the same session can show up in it (inline comment).
- The second new test relies on a 20 ms
setTimeoutbefore checking that nothing has responded yet. That kind of negative check with a sleep can pass by chance. Please tie it to an event instead, or drop that assertion.
| 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 reads the turn state once, right after session.command returns. Events still waiting in the subscription have not been folded in yet. If the executor's last run started and finished on the server but the consumer has not yet processed its session.execution.started, executing is still false. The turn then returns state.terminal ?? "succeeded" straight away and that run's text and tool updates are dropped (the outcome may also be out of date). The new test avoids this only by waiting for the first chunk before releasing the gate. Could the consumer catch up first, e.g. by draining events up to a marker, or by waiting for the matching terminal event, before checking executing?
| } | ||
| 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 set, any session.inbox.delivered for this session starts the turn, not only the command's own. A prompt from another client, or a queued one, delivered while the command is running would then be streamed into this ACP turn, and its terminal events would count toward the command's outcome. Can the inbox IDs created by the executor be linked to the command, e.g. by having session.command return or accept the ID?
danyel117
changed the title
This branch has not been deployed
1 participant
Issue for this PR
Closes #53914
Related report: prevalentWare/opencode-goal-plugin#59
Type of change
What does this PR do?
Registered command executors can submit several executions before returning. ACP currently correlates the wrong initial inbox and stops at the first terminal event. Continue consuming command-owned events until the executor ends, retain the latest outcome, and drain its last in-flight execution before replying.
Pass the per-command abort signal to Promise executors. Add an explicit session interruption hook so cooperating plugins can persist Cancel even between execution cycles. ACP transport teardown aborts local work without sending the explicit user interrupt API. Each cancellation request cleans its intent after interrupt completion, including cancellation during final usage reporting.
Plan metadata projection is independent in #53929 (issue #53928).
How did you verify your code works?
Required Bun 1.4.2: full upstream lint and all 36 package typechecks pass. This lifecycle slice's ACP suite: 103 pass, 0 fail. Core session activation suite: 4 pass, 0 fail, including cold idle interruption hooks and Promise executor signal isolation. The late-cancel regression fails before its fix and passes after it.
Native isolated ACP with a local SSE fixture: command remains pending across multiple automatic executions and a 311-second continuation interval. Cancel stops a foreground worker and cancels an idle goal with no later restart. Closing ACP and its standalone server preserves the active goal. No live user state or provider credentials used. The complete combined implementation was also verified with live/replayed plans before slicing.
Screenshots / recordings
Verified ACP JSON-RPC transcripts; no client UI changes.
Checklist
Chain Context
Independent host slice: 284 changed lines, starts at v2 434f7b2 and ends with complete command streaming and explicit cancellation. Plan projection follows independently under #53928. Plugin persistence/planning is outside this repository. Rollback restores the previous adapter and plugin SDK behavior.