Skip to content

fix: defer startup interactions until session setup is delivered - #714

Merged
benbrandt merged 1 commit into
mainfrom
fix/setup-interaction-ordering
Oct 30, 2026
Merged

benbrandt merged 1 commit into
mainfrom
fix/setup-interaction-ordering

Conversation

@benbrandt

Copy link
Copy Markdown
Member

Summary

  • Hold session-scoped permission requests and elicitations for a freshly created or forked session until its successful setup response has actually been written. Both the shared v1-shaped path and direct v2 requests use the same delivery barrier.
  • Make MCP startup reporting wait on that barrier before starting OAuth. The optional MCP startup wait still only waits for native startup status or its timeout, not for a client dialog, so setup cannot deadlock on sign-in.
  • Hold fresh standard-child announcements behind the parent creation response using the router's existing readiness mechanism. Creation only releases readiness; it does not run resume's child snapshot refresh.
  • Preserve caller cancellation and discard unsent interactions on close or disconnect. Track URL completions only after the interaction passes the startup barrier, and fence stale release callbacks from later session lifetimes.
  • Extend the existing response-write observer to distinguish success from error responses. Error responses and failed writes do not release startup UI.
  • Update the existing MCP startup documentation. No review/checklist Markdown file is added.

The ACP v2 session-setup recommendation says Agents SHOULD NOT send session-scoped requests before the new-session response. Checked against ACP d5e9949c. SDK reference e23cc640 processes incoming requests asynchronously, so handler completion alone is not a response-delivery boundary. The adapter now uses the existing AcpResponseDelivery observer for this policy on both protocol versions.

Scope

This does not globally pause interactions for already-active sessions on resume. Existing replay and current-state synchronization ordering remain intact. Active-resume environment replacement, complete configuration-change notifications, and remaining session/entity metadata reconstruction are separate follow-ups.

Validation

  • npm run typecheck
  • npm run build
  • npm test: 2,310 passed, 39 skipped
  • npm run test:integration: all 19 passed on the final run
  • npm run codex-test -- -p "Reply with exactly OK. Do not use tools or modify any files." -o summary: completed with end_turn
  • git diff --check
  • Opt-in E2E tests were not run.

New wire tests exercise both ACP versions with held response writes, MCP sign-in with and without the startup timeout, fresh child announcements, and fork interactions. Delivery tests cover unrelated sessions, cancellation/close/disconnect before sending, stale release callbacks, and unsent URL completions. Existing response-boundary tests cover failed writes and error responses.

The first integration run failed the existing shell-approval assertion about output in the completed tool-call update. Two subsequent full integration runs passed without changing that test.

@benbrandt
benbrandt merged commit ad993bb into main Oct 30, 2026
6 checks passed
@benbrandt
benbrandt deleted the fix/setup-interaction-ordering branch October 30, 2026 18:16
benbrandt added a commit that referenced this pull request Oct 30, 2026
## Summary

- Extend the existing recovery path's before/after configuration
comparison into one shared configuration-change helper. Publish a
complete `config_option_update` when the advertised list changes, rather
than duplicating publication in individual callers.
- Use it for `session/set_config_option`, the legacy mode/model setters,
command-driven changes, approved-plan implementation, and
provider/recovery model refreshes. The requesting client's duplicate
notification is written before the setter response.
- Do not publish for unchanged lists, rejected changes, the existing
JetBrains 2026.1 disabled-config workaround, or a session state
superseded by replay.
- Build setter responses from the current session state after
publication. If replay replaces the session while a native settings
request is pending, the old request does not advertise stale options in
its notification or response.
- Keep v2 field conversion in the existing conversion layer. Both ACP
versions receive full lists, including dependent changes to reasoning
choices and removal of fast mode after a model switch.

Checked against ACP commit `854e6003`.
`docs/protocol/v2/session-config-options.mdx:375-379` recommends
configuration notifications whenever configuration changes, including
changes requested through `session/set_config_option`, with the
requesting client's duplicate before its response. Responses and
notifications replace the complete list.

## Scope

Based on the merged #714, not stacked on its feature branch. No
review/checklist Markdown file is added. This covers configuration
changes owned by the adapter; it does not add new native settings-event
synchronization or change active-resume environment replacement.
Remaining session/entity metadata reconstruction is separate.

## Validation

- `npm run typecheck`
- `npm run build`
- `npm test`: 2,332 passed, 39 skipped
- `npm run test:integration`: all 19 passed
- `npm run codex-test -- -p "Reply with exactly OK. Do not use tools or
modify any files." -o summary`: completed with `end_turn`
- `git diff --check`
- Opt-in E2E tests were not run.

New wire tests cover notification-before-response ordering for every
root config option on both protocol versions, complete dependent model
changes, no-op and rejected/native-failure cases, one notification for
`/plan`, legacy setters, and replay replacing the session during a
pending native settings request. Existing command, plan,
provider/recovery, and child configuration tests remain passing without
snapshot updates.
EugeneTheDev pushed a commit that referenced this pull request Nov 8, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.6.0](v2.5.0...v2.6.0)
(2026-11-06)


### Features

* expose configured permission profiles as modes
([#395](#395))
([caf2e87](caf2e87))
* predict the follow-up prompt with thread/sample on the session thread
([#740](#740))
([f9cb470](f9cb470))
* show async Codex questions as elicitations
([#707](#707))
([cc57c7e](cc57c7e))
* update ACP SDK to 2.4 and align protocol behavior
([#727](#727))
([ab513aa](ab513aa))


### Bug Fixes

* clear stale entity metadata during ACP v2 replay
([#716](#716))
([580d8b9](580d8b9))
* defer startup interactions until session setup is delivered
([#714](#714))
([ad993bb](ad993bb))
* fence turn interrupt retries to the original prompt
([#723](#723))
([9c33f86](9c33f86))
* match vendor-prefixed gateway model ids such as OpenRouter's
([#731](#731))
([29195ec](29195ec))
* preserve sibling session metadata namespace fields
([#718](#718))
([3720341](3720341))
* publish complete configuration updates for session changes
([#715](#715))
([b7ac0bf](b7ac0bf))
* release the prompt suggestion thread and keep it minimal
([#709](#709))
([82ad58f](82ad58f))
* report verified standard subagent directories
([#722](#722))
([7d7e95a](7d7e95a))
* resynchronize current session metadata on ACP v2 resume
([#717](#717))
([927c480](927c480))
* scope authentication cancellation and initial status delivery
([#720](#720))
([e059ab7](e059ab7))
* show commands in Zed approval prompts
([#735](#735))
([cca2dca](cca2dca))
* skip the app-server restart when providers/set repeats the applied
config
([#728](#728))
([3b35338](3b35338))
* start the Codex app-server with the provider of the gateway routing
([#733](#733))
([cbcd446](cbcd446))
* suppress session messages after close
([#712](#712))
([85bf932](85bf932))
* synchronize resumed titles and reset replayed message metadata
([#713](#713))
([ef45a6d](ef45a6d))
* tear down session interactions and background processes on close
([#711](#711))
([1e89efa](1e89efa))
* update ACP SDK to 2.2 and align protocol behavior
([#710](#710))
([6f96fbe](6f96fbe))
* update codex to 0.166.0
([#734](#734))
([e53f23e](e53f23e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: acp-release-bot[bot] <246668977+acp-release-bot[bot]@users.noreply.github.com>
mrmans0n added a commit to mrmans0n/codex-acp that referenced this pull request Nov 8, 2026
Upstream v2.6.0 moves the app-server initialize of initialize before the capability reads (agentclientprotocol#714),
adds request cancellation to authenticate (agentclientprotocol#720), skips the restart for a repeated provider
routing (agentclientprotocol#728) and restarts the app-server for a new routing even before a session is open
(agentclientprotocol#733). The patch's code lines are unchanged: the hook review is configured after upstream's
initialize call (no hook runs during initialize) and the authenticate invalidation sits before
upstream's new try block. A repeated routing keeps the process and configuration, so the review
stays valid as upstream returns early.

Tests: the provider-change tests now cover both cases. Disabling OpenAI keeps the process and
must fail the stale review with -32044; a new routing restarts the app-server and ends the
request with 1001. Neither dispatches thread/start or writes trust. The router test's mock agent
implements upstream's new startup publication hooks.
Adapted from 2341b05 (original 4d43405).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant