Synced from GitHub
fix(acp): keep model discovery and selection on the same option#19567
ACP model discovery combined choices from multiple model-category options, while startup wrote only one option. A boolean option before that select could also receive the model ID, so a choice shown in the picker could fail session startup.
Use the first model-category select with a nonblank ID for discovery, current selection, capability checks, startup writes and custom-model validation. Flatten grouped choices from that same option. Grok uses the same selector while retaining main’s legacy model-state and context-window behavior.
Thanks @StiensWout for the reproduction and original fix in #19567. The maintainer takeover preserves the author’s commits and merges current main. Related work: #19575 and #19754. This stays focused on selecting the same first option; broader routing and picker behavior remain outside its scope.
Verification on Linux / Node 24:
- PR-added tests: 3 across 2 files → 2 integrated cases in the existing registry adapter suite. They prove that an advertised grouped choice reaches the correct wire write despite preceding boolean/blank-ID options, and that a later option’s value is rejected before writing. Both fail against main and pass on head. The separate probe-only case was removed; the combined probe/adapter suites have 23 passing tests.
- 244 tests pass across 8 focused ACP, registry and Grok suites. Four scoped typechecks pass (server, ACP, ACP registry and Grok). Focused lint, formatting and whitespace checks pass; one existing Grok unused-variable warning remains.
- One fresh parallel adversarial round (Claude Opus 5.5 and GPT-6 Sol) found no actionable defects in the full diff. The replay harness publishes its completion receipt before exposing the last reply, including trailing runtime-exit entries, so immediate startup rejection cannot race the transcript assertion.
- Before/after native ACP protocol proof: the real registry probe and adapter/runtime launch an independent local ACP peer. Main fails actual startup for an advertised choice in both multiple-select and preceding-boolean cases. Head passes both, sends
session/set_config_optionwithconfigId: providerandvalue: codex, and rejects a later-option choice without an extra write. All captured child processes exit after cleanup.
The peer uses synthetic option data; no authenticated fx, Cline or Grok account, browser picker flow or external model request was exercised. Those integrations still need a human check. macOS, Windows, iOS, Android and Electron desktop were not verified. There is no visual UI change; protocol output provides the evidence.
Tests: Codex GPT-6 Astra (high). Implementation and native verification: Codex GPT-6.1 Sol (high). Adversarial review: Claude Opus 5.5 and Codex GPT-6 Sol, through T3 Code.
Checks
22 successful · 12 skipped
Added 2 commits
Synced from GitHub
Approvability
Verdict: Approved at 5b07a62
Macroscope's review found this PR approvable — This is a focused ACP bug fix that makes model discovery, validation, and selection consistently use the same existing configuration option. Production changes are narrow and supported by integration coverage, while the remaining edits are test-harness or helper changes.
No code changes detected at 59c232b. Prior analysis still applies.
You can add or adjust custom eligibility rules. Learn more.
Synced from GitHub
Warning
Review limit reached
Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.
Next included review available in 15 minutes.
View limit details
Limit details: You’ve used all 10 included reviews currently available.
Review configuration:
⚙️ Run configuration
- Configuration used: Path: .coderabbit.config.ts
- Review profile: CHILL
- Plan: Advanced
- Run ID:
9b460d32-b590-4c81-9377-02671243a562
📥 Commits
Reviewing files that changed from the base of the PR and between 88c2eca46c631791801123ce5eb49cd8d8dba168 and 59c232b3de85b9f51fa3678a864800620afbe229.
📒 Files selected for processing: 8
apps/server/scripts/acp-replay-agent.tsapps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.test.tspackages/provider-acp-registry/src/server/probe.tspackages/provider-acp/src/server/AcpSessionRuntime.tspackages/provider-acp/src/server/adapter.tspackages/provider-acp/src/server/runtimeModel.tspackages/provider-acp/src/server/sessionConfig.tspackages/provider-grok/src/server/adapter.ts
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info
⚙️ Run configuration
- Configuration used: Path: .coderabbit.config.ts
- Review profile: CHILL
- Plan: Advanced
- Run ID:
95823970-d48f-43e5-9f1b-e4e62c88b2e2
📥 Commits
Reviewing files that changed from the base of the PR and between bf8ffe81a63c82d3de5fbaa9991fd5f30034a452 and 88c2eca46c631791801123ce5eb49cd8d8dba168.
📒 Files selected for processing: 9
apps/server/scripts/acp-replay-agent.tsapps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.test.tsapps/server/src/provider/AcpRegistryProbe.test.tspackages/provider-acp-registry/src/server/probe.tspackages/provider-acp/src/server/AcpSessionRuntime.tspackages/provider-acp/src/server/adapter.tspackages/provider-acp/src/server/runtimeModel.tspackages/provider-acp/src/server/sessionConfig.tspackages/provider-grok/src/server/adapter.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📝 Walkthrough
The changes standardize how ACP integrations identify model configuration options, update registry probing and stored-model selection tests, and change how the replay agent queues inbound frames and handles cursor and status updates.
Changes: ACP model selection
Layer / File(s) Summary Model-option selection contract
packages/provider-acp/src/server/runtimeModel.ts,packages/provider-acp/src/server/sessionConfig.tsA shared helper selects the first model-category select with a nonblank ID. Session configuration exports choice flattening and documents which model options appear in the dedicated picker. Model selection and validation
packages/provider-acp/src/server/AcpSessionRuntime.ts,packages/provider-acp/src/server/adapter.ts,packages/provider-grok/src/server/adapter.ts,apps/server/src/orchestration-v2/Adapters/AcpRegistryAdapterV2.test.tsRuntime validation and adapter model lookup use the shared selector. Tests cover stored model selection and rejection of a value outside the selected option’s allowed choices. Probe model discovery
packages/provider-acp-registry/src/server/probe.ts,apps/server/src/provider/AcpRegistryProbe.test.tsThe registry probe derives models and the current model ID from the selected option’s flattened choices. A test covers a grouped provider selector. ACP replay-agent frame handling
Layer / File(s) Summary Queue and flush inbound frames
apps/server/scripts/acp-replay-agent.tsInbound messages are queued before sending. The cursor advances as frames are queued, and status is written once before sending. Successful and cancelled runtime exits do not stop replay; other exits do. Outbound mismatches record failure before sending an error response. Priority: ➖ Normal
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Suggested reviewers:
juliusmarminge
Merge Risk: ⚪ Minimal · up to 88c2e
Model selection and replay handling have no identified issue requiring resolution before merge; proceed with normal checks.
Security architecture risk: 🔵 Low · up to 88c2e
The change makes advertised choices and configuration writes agree and narrows acceptance of unlisted values. No new access or privilege expansion was identified. Interrupted, overlapping or dynamically retargeted updates remain incompletely verified.
Retained concerns No architecture-level concerns identified.
Security review details
Security Blast Radius
- inferred — The changed decision propagates through shared model catalogs and model writes for the selected session across ACP registry and Grok consumers. The inspected paths do not establish any new cross-tenant, credential, infrastructure or tool-execution authority.
Trust Boundaries and Controls
- observed — Advertised option metadata and a requested model value meet at local validation before the remote configuration write. Listed values pass; unlisted values require explicit custom-model configuration and the canonical selected option. The added rejection case verifies that a value belonging only to a later option fails without a configuration request.
Resilience and Maintainability Implications
- inferred — Model writes retain a setup-time option ID while notifications refresh a separate live snapshot. Identity replacement, overlapping responses and interruption after remote application remain consistency uncertainties. The relevant snapshot and write mechanisms existed at the base, so these are not established PR-introduced concerns. Replay progress likewise records queued consumption, not acknowledged delivery after arbitrary interruption.
Pre-merge checks: Passed 4
Check name Status Explanation Linked Issues check Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request. Title check Passed The title clearly describes the primary change: keeping ACP model discovery and selection aligned to the same configuration option. Description check Passed The description explains the problem, change, scope, approval context, verification results, limitations, and test evidence. It is sufficiently complete for the template.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create a new PR
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Taking over this one. I’ll keep the smallest focused fix here: model discovery and selection consistently use the first model-category select. Related ACP model-option work in #19575 and #19754 remains outside this takeover. I’ll preserve the original commits and push the maintainer rebuild to this PR.
Added 2 commits
Synced from GitHub