Keep ACP configuration notifications atomic

[boudra]

Keep plugin ACP configuration changes atomic when an agent sends vendor notifications alongside its configuration response. Successful changes publish one final snapshot; failed changes publish none.

Linked issue

Fixes the SDK test failure on main. No matching open issue or superseded PR found.

Type of change

• Bug fix

Reasoning

ACP SDK 1.4 dispatches extension notifications through an asynchronous handler chain while resolving responses independently. If a notification and response arrive together, the response can close our configuration transaction before the SDK invokes the notification callback. The resulting session.config event escapes rollback or duplicates a successful commit. Separate subprocess writes made this depend on pipe chunking.

Wire:   vendor/config → configuration response
Before: response → commit/rollback → vendor/config → extra snapshot
After:  vendor/config → response → commit/rollback

The adapter now consumes synchronous vendor transforms at its input stream boundary. Built-in ACP methods and all requests/responses still go through the SDK. This removes the deferred extension callback path; no sleeps, retries, or notification-count suppression are needed. Waiting for the existing notification queue would miss callbacks the SDK has not invoked yet.

Goals

• Publish exactly one transformer-derived snapshot for a successful configuration change.
• Discard notifications from both the failed change and its rollback.
• Verify single notifications and bursts coalesced with the response in one subprocess write.

Non-goals

• Change the public plugin API, configuration rollback policy, or ACP SDK version.
• Modify the built-in provider backend.

QA

Linux, Node 22.20.0, installed ACP SDK 1.4.0. Reproduction uses a real Node subprocess and SDK connection through runAcpProvider, without mocks.

• Repeated the original single-notification journey: reproduced an extra snapshot after initially passing runs. Temporary tracing showed the transaction closing before the SDK invoked the vendor callback.
• Strengthened the fixture to write notifications and their response together. Against unmodified main 47171b4, the initial rollback assertion failed 10/10 runs: expected ... length of 1 but got 2.
• With the fix, the targeted regression passed 20 consecutive runs / 40 cases, covering 1 and 64 notifications per response. Each case checks initial rollback, successful commit, and subsequent rollback.
• npx vitest run src/server/acp.test.ts --bail=1 in packages/plugin: 21 passed.
• npx vitest run src/boundaries.test.ts --bail=1 in packages/plugin: 44 passed.
• npx vitest run src/server/agent/plugin-provider.test.ts --bail=1 in packages/server: 3 passed.
• npm run build:server, npm run typecheck, npm run lint, and npm run format: passed.

Risk surface: delivery timing of vendor notifications in the plugin ACP adapter, shared by subprocess and connector transports. Existing ACP tests also cover SDK connectors, permission requests, configuration serialization, disconnects, and process shutdown. No live vendor session or macOS/Windows runtime was exercised locally.

Checklist

• Plugin changes follow the SDK import boundaries
• One focused change
• npm run typecheck passes
• npm run lint passes
• npm run format passes
• QA evidence
• Tests added or updated where it made sense

[greptile-apps]

Greptile Summary

The PR routes vendor ACP notifications synchronously at the input-stream boundary so configuration notifications remain inside the active transaction.

• Removes deferred SDK extension-notification handling while leaving built-in ACP messages with the SDK.
• Stages transformer-derived configuration during changes and publishes only the committed snapshot.
• Expands the subprocess regression test to cover one and 64 notifications coalesced with each response.

Confidence Score: 4/5

The atomicity fix appears behaviorally sound, but the broad error suppression violates an explicit repository requirement and should be resolved before merging.

No blocking behavioral failure was established in the routing or transaction logic; the remaining finding is the new catch-all path that hides unexpected notification-processing failures instead of handling or propagating them according to repository policy.

Files Needing Attention: packages/plugin/src/server/acp-internal/connection.ts

Important Files Changed

Filename	Overview
packages/plugin/src/server/acp-internal/connection.ts	Intercepts vendor notifications in wire order to preserve transaction atomicity, but broadly suppresses unexpected transformer failures.
packages/plugin/src/server/acp.test.ts	Strengthens caller-facing subprocess coverage for failed, successful, and rolled-back configuration changes with coalesced notification bursts.

Sequence Diagram

sequenceDiagram
    participant Agent
    participant Router as Vendor notification router
    participant SDK
    participant Transaction as Configuration transaction
    participant Events

    Agent->>Router: vendor/config notification
    Router->>Transaction: Run transformer synchronously
    Transaction->>Transaction: Stage derived configuration
    Agent->>Router: configuration response
    Router->>SDK: Forward response
    SDK-->>Transaction: Resolve configuration request
    alt Configuration succeeds
        Transaction->>Events: Publish one final session.config
    else Configuration or rollback fails
        Transaction->>Transaction: Discard staged notifications
    end

Loading

_{Reviews (1): Last reviewed commit: "fix(acp): keep vendor configuration noti..." | Re-trigger Greptile}