Skip to content

refactor(acp): extract provider prompt executor - #778

Merged
ewen-poch merged 1 commit into
mainfrom
refactor/acp-provider-prompt-executor
Aug 5, 2026
Merged

refactor(acp): extract provider prompt executor#778
ewen-poch merged 1 commit into
mainfrom
refactor/acp-provider-prompt-executor

Conversation

@ewen-poch

Copy link
Copy Markdown
Member

Problem

AcpRuntime still owned the external provider prompt lifecycle: provider probes, prompt/update racing, acceptance timing, supersession checks, notification routing, terminal capture, and provider-specific outcome normalization. This kept provider execution state coupled to application finalization and made the runtime harder to reason about and test independently.

Proposed change

  • add AcpProviderPromptExecutor.execute(...) as the owner of one external provider prompt attempt;
  • move provider probe setup/finalization and the prompt rejection/update race behind that interface;
  • preserve current-update ordering, provider acceptance timing, stale-turn draining, raw stop responses, terminal timestamps, and provider usage/model-turn normalization;
  • keep AcpRuntime responsible for durable application effects after an authoritative stop: Context, Artifact, events, usage reconciliation, and auto-compaction.
flowchart LR
  R["AcpRuntime: prepared interaction"] --> E["AcpProviderPromptExecutor"]
  E --> A["Provider-turn adapter probe"]
  E --> P["ACP prompt + update loop"]
  P --> N["Current notifications -> projector"]
  P --> O["Normalized provider outcome"]
  A --> O
  O --> F["AcpRuntime: app finalization"]
Loading

Scope and non-goals

  • Production raw is 476 lines (provider-prompt-executor.ts: 188; Runtime additions/deletions: 116/172), within the approved 550-line ceiling.
  • runtime.ts decreases from 2,941 to 2,885 lines on the current base.
  • No public API, IPC contract, persistence/data model, orchestration state, or user interaction changes.
  • Electron/Web/CLI/Task behavior and the existing Specialist/Permission/Compute capability asymmetries are unchanged.
  • Issue [Feature]: Provider-neutral multi-agent research orchestration #458 remains a forward-compatibility boundary only; this PR introduces no orchestration schema or state.
  • Provider adapters retain their existing semantics; this PR only selects and coordinates them.

Ownership and sequence

  1. The executor begins a provider-specific probe before dispatch.
  2. It rechecks turn authority, dispatches the ACP prompt, and races rejection against ordered updates.
  3. The first current update or stop signals provider acceptance once.
  4. Current notifications are routed in order; stale updates are drained without application writes.
  5. At provider stop, the executor captures terminal authority before potentially slow adapter finalization.
  6. It returns a normalized stopped, superseded, or not-dispatched outcome.
  7. Runtime performs application-owned finalization only for the authoritative stopped outcome.

Probe helpers and best-effort reporting remain fail-open. Prompt rejection is preserved, cancellation is issued once, late prompt settlement is consumed, and identity tokens prevent an older attempt from removing a replacement probe for the same provider Session ID.

Acceptance criteria and validation

All listed checks ran after the last material edit and again after rebasing onto dce57c9 (fix(acp): correct Claude refusal attribution (#774)).

Behavior Final check Result
Executor lifecycle, ordering, rejection, stale drain, async-probe supersession, terminal capture, fail-open helpers, observation cleanup npm test -- --run src/main/acp/provider-prompt-executor.test.ts ...provider-turn-adapter.test.ts ...claude-turn-adapter.test.ts ...codex-turn-adapter.test.ts ...opencode-turn-adapter.test.ts ...session-interaction-owner.test.ts ...session-update-projector.test.ts 7 files, 66 tests passed
Runtime provider usage/model turns, stop authority, cancellation, supersession/reset race, terminal event, reconnect deferral targeted src/main/acp/runtime.test.ts impact patterns 16 passed, 406 skipped
Consumer acceptance timing targeted src/main/acp/runtime-coordinator.test.ts continuation pattern 1 passed, 51 skipped
Node process contracts npm run typecheck:node passed
Touched linted sources targeted ESLint over the four changed files passed, no issues
Final ACP impact review including #774 independent Standards review 0 findings; additional 9 files / 157 tests passed
ARD-24 behavior and compatibility boundary independent Spec review 0 findings

The Test Impact Set includes the changed owner, all provider-turn adapters, terminal interaction ownership, notification projection, the Runtime consumer, and coordinator acceptance timing. Renderer/Web typecheck and platform/E2E lanes are excluded locally because no renderer, preload, shared IPC, persistence, platform process, or public interface changed; PR Gate remains authoritative for its selected lanes.

Review focus

  • acceptance occurs only after the first current provider update/stop and before routing it;
  • stale attempts drain without Context, Artifact, event, or usage writes;
  • terminal authority is captured before adapter finalization;
  • Claude refusal attribution from fix(acp): correct Claude refusal attribution #774 continues through framework-aware handleSessionUpdate projection;
  • Runtime performs durable finalization only for stopped outcomes.

Uncovered risks

  • There is no dedicated unit test with two overlapping executor calls using the same provider Session ID, although identity-token cleanup is covered by implementation review.
  • There is no dedicated test where the provider stop arrives before a later prompt rejection, although the rejection handler is attached immediately and consumed.
  • Full platform/network-listener coverage is delegated to the exact-head PR Gate and CodeQL/AI review.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defect introduced by this pull request.

@github-actions github-actions Bot added the ready-to-merge All completed AI reviewers found this pull request mergeable. label Aug 5, 2026
@ewen-poch
ewen-poch merged commit f8ff074 into main Aug 5, 2026
23 checks passed
@ewen-poch
ewen-poch deleted the refactor/acp-provider-prompt-executor branch August 5, 2026 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge All completed AI reviewers found this pull request mergeable.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant