Conversation
… published When the Runtime rejected a settled external execution's result before publishing it (for example a completion over 256 KiB), the backend skipped abandonExecution because it required a published result. The ACP Session then stayed busy while inspection still reported ready. Abandon every unacknowledged execution; providers ignore turns they never settled. Fixes apache#5822 Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 70096678b1a1bd89ccbdc13d91435ccaa2ac0e52. The new regression covers the intended ACP failure window where the provider returned a terminal result but Runtime normalization rejected it. However, the implementation also calls the public abandonment hook for failures that occur before any result settles; see the inline P2.
Validation: clean npm ci; build:test; focused backend 16/16; full Runtime 3,669 pass / 13 skip; full typecheck, lint, format, ASF headers, model metadata, Windows test inventory, and git diff --check passed. A temporary production-path probe confirmed that a provider execute() rejection now invokes abandonExecution; restoring the previous returned-result gate made that probe fail as expected. The probe and mutation were removed and the worktree is clean.
Current main is 9265747de2b468158665f2e3d14f9c391c647106; this PR is 1 commit ahead and 2 behind, and the merge-tree is conflict-free. GitHub currently reports only the label check, so the hosted test workflow has not validated this head. I did not test third-party executor plugins, packaged Electron, or native Windows/macOS.
Conclusion: not ready to merge until abandonment is gated by actual provider settlement and a negative pre-settlement failure test is added.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // Abandon every unacknowledged execution, including one whose result the | ||
| // Runtime rejected before publishing it; the Plugin ignores turns it | ||
| // never settled. | ||
| if (this.#binding.acknowledgeExecution && !acknowledged && this.#binding.abandonExecution) |
There was a problem hiding this comment.
[P2] Preserve evidence that the provider actually returned a terminal result before calling abandonExecution. The public PluginExecutorProvider contract defines this hook for a settled result, but #produce returns false both when Runtime normalization rejects after settlement and when provider.execute() rejects before returning any result. Removing the returnedResult condition therefore calls abandonExecution for pre-settlement failures and cancellations too. ACP currently ignores a turn that is not awaiting acknowledgement, but the exported provider contract does not require every plugin to do that; another conforming provider may mark a history gap or tear down a retryable conversation. I reproduced this with a provider whose execute() throws transient transport failure: current head called abandonExecution, while restoring the old returned-result gate did not. Please carry explicit settlement provenance across the service/backend boundary and add a negative regression for pre-settlement rejection.
There was a problem hiding this comment.
Thanks, agreed: the provider contract scopes abandonExecution to settled results, and relying on ACP ignoring unrelated turns was wrong.
Fixed in 3a12963, following your suggestion:
PluginExecutorExecutionOptionsgainsonSettled.PluginExecutorServicecalls it when the provider returns a terminal result, beforenormalizeResult, in the same way it already forwardsonEvent.PluginExecutorBackend.#producereturns'published' | 'rejected' | 'unsettled'. The backend remains the single decision point, and every call still goes through the binding's scope and retirement guards: onlypublishedis acknowledged, and only a settled (publishedorrejected) execution can be abandoned.- Added the negative regression
a Plugin execution that fails before settling is neither acknowledged nor abandoned. The oversize regression now records on the provider and asserts a single abandonment.
Mutation checks: making the backend abandon unsettled executions fails the negative test, and suppressing onSettled fails the oversize test. Runtime test:dist 3684 pass / 0 fail / 14 skip; plugin executor service and ACP plugin suites 73/73; lint, format:check, and typecheck pass. End to end in Desktop with the fake Agent from #5822 on the current head: after the oversize turn, the composer shows the history-gap notice and blocks sending, the Plugin record is history_gap, and the Agent process is closed.
Carry settlement provenance from PluginExecutorService to the backend through a new onSettled execution option, called when the provider returns a terminal result and before the Runtime validates it. The backend remains the single place that acknowledges or abandons: it abandons a settled result that was rejected or not acknowledged, and leaves a provider failure before settlement untouched. Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head `3a12963594446f9d61f3c8cfa6f454f4ca2776bb`. I found no remaining P0-P3 issue in this revision.
The previous pre-settlement abandonment bug is fixed at the contract boundary. `PluginExecutorService` now reports settlement immediately after the provider returns and before Runtime result validation, while `PluginExecutorBackend` distinguishes published, rejected-after-settlement, and unsettled executions. A provider rejection before returning a terminal result is therefore neither acknowledged nor abandoned; a returned result rejected by normalization is abandoned exactly once.
I independently checked both regression directions. Suppressing the settlement callback makes the oversized-result test fail because no abandonment occurs. Removing the `settlement !== 'unsettled'` gate makes the pre-settlement rejection test fail because abandonment occurs. After restoring the exact head, the focused backend/service suites pass 29/29; the full Runtime suite passes 3,670 with 13 skipped; ACP executor and Antigravity adapter suites pass 61/61 and 9/9. `build:test`, full typecheck, lint, format, ASF headers, model metadata, Windows test inventory, and `git diff --check` also pass.
Current main is `0f98c2a488d3095df61eb184beb0030a240ad225`; the PR is 2 commits ahead and 3 behind, and the merge-tree is conflict-free. GitHub currently reports no hosted checks for this head. I did not validate the official Antigravity binary, packaged Electron, or native Windows/macOS behavior.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Summary
When an external executor turn settled but the Runtime rejected its result before publishing it (a completion over 256 KiB),
PluginExecutorBackendskippedabandonExecutionbecause it only abandoned published results. The ACP Session stayedacp_busyfor the rest of the Host lifetime while the executor picker still reported it as ready.The backend now learns whether the provider actually returned a terminal result, through a new
onSettledexecution option thatPluginExecutorServicecalls before validating the result. The backend stays the single place that acknowledges or abandons: a settled result that was rejected or not acknowledged is abandoned, and a provider failure before settlement is neither acknowledged nor abandoned, as before. The Session moves tohistory_gapright away, and the composer blocks sending and offers New Task. The task still cannot continue, as intended, because the transcript is missing the rejected reply.This closes the path left open after P2 #1 in Astro-Han's review on #5670.
Fixes #5822
Verification
a settled Plugin result that the Runtime rejects is abandoned onceanda Plugin execution that fails before settling is neither acknowledged nor abandoned. The first fails onmain(abandonedis[]); the second guards pre-settlement failures. Mutation checks: making the backend abandon unsettled executions fails the second, and suppressingonSettledfails the first.@maka/runtimetest:dist: 3684 passed, 0 failed, 14 skipped. Plugin executor service and ACP plugin suites: 73 passed.npm run lint,npm run format:check,npm run typecheckpass.npm run dev:worktree) with the fake ACP Agent from bug(runtime): ACP Session reports ready but stays busy after its result fails Runtime validation #5822 standing in for Antigravity:main): after the oversize turn, the picker showed Antigravity as ready and every send failed withAntigravity ACP Session is busy. The Plugin record stayedprompt_pendingand the Agent process kept running.history_gapand the Agent process is closed.Not run: the full workspace test suite,
knip, the official Antigravity binary, and Windows.Screenshots
Before: after the oversize turn, the next message fails with
Antigravity ACP Session is busy, although the picker still shows Antigravity as ready.After: the composer immediately shows the history-gap notice with New Task, and sending is blocked.
AI use
Tool(s) and scope: Claude Code diagnosed the defect, wrote the fix, the regression tests, and the fake ACP Agent used for end-to-end reproduction. Both commits carry a
Generated-by: Claude Codetrailer. I reproduced the behavior, reviewed the change, and ran the end-to-end check.Checklist
Does this PR entail a change in behavior?