[codex] Fix ACP model selection methods#3688
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
🟢 Good taste — Elegant, simple solution that cleanly addresses the real problem.
Summary
This PR fixes a critical compatibility issue: Codex ACP conversations were failing with Method not found because the SDK was calling the deprecated session/set_model RPC instead of the current session/set_config_option method that modern ACP providers expose.
Analysis
Data Structure ✅
The addition of model_selection_method: Literal["set_session_model", "set_config_option"] to ACPProviderInfo is a clean, extensible abstraction. The default value preserves backward compatibility for external callers constructing the dataclass positionally.
Routing Logic ✅
The branching logic in set_acp_model() and _maybe_set_session_model() correctly dispatches to the provider's declared method. Unknown/custom providers fall back to set_config_option as a standard ACP protocol method.
Breaking Change Risk ✅
No breaking changes to the public API. The change is additive (new optional field with sensible default) and internal routing is opaque to callers.
Testing ✅
Comprehensive test coverage across provider metadata, model selection, resume reapply, and runtime switching. Tests verify both protocol methods are called correctly for their respective providers.
Minor Notes
- The error message change from
"rejected set_session_model"to"rejected model switch"(line ~4707) is appropriate since errors can now originate from either method. - The comment on line ~587 correctly identifies that
set_config_optionis a standard ACP protocol method for unknown providers.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The changes are focused on protocol method routing, well-tested, and introduce no breaking changes to the public API. Dependency bumps (agent-client-protocol 0.8.1→0.10.1, codex-acp 0.15.0→0.16.0) are justified and the newer package is 19 days old.
VERDICT:
✅ Worth merging — Core logic is sound, clean abstraction, comprehensive tests.
KEY INSIGHT:
Provider-specific protocol routing via an explicit model_selection_method field is a pragmatic solution that handles the fragmented ACP provider landscape without resorting to try/catch fallbacks or hardcoded provider checks.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
Taste Rating
🟡 Acceptable - Works but could be cleaner
Summary
This PR introduces a model_selection_method field to ACPProviderInfo to decouple the capability of supporting session/set_model from the actual method a provider uses for model selection. Codex ACP 0.16.0 changed its protocol to use session/set_config_option instead of session/set_model, which motivated this broader refactoring.
The changes are generally sound, but I have one concern worth discussing.
[CRITICAL ISSUES]
None identified. The core refactoring is well-structured.
[IMPROVEMENT OPPORTUNITIES]
-
[openhands-sdk/openhands/sdk/settings/acp_providers.py, ~Line 279] 🟠 Semantic Coupling: The existing
supports_set_session_modelfield now has a confusing dual meaning — it indicates the provider supports the literalsession/set_modelcall, but it's no longer the gate for which method to use. The newmodel_selection_methodfield is the authoritative selector, yetsupports_set_session_modelremains as legacy surface area. Consider:- Keeping
supports_set_session_modelas a derived property:model_selection_method == "set_session_model" - Or documenting more explicitly that
supports_set_session_modelis deprecated in favor ofmodel_selection_method
The current state isn't wrong, but it creates a trap: external callers checking
supports_set_session_modelto decide behavior will get surprising results for Codex. - Keeping
-
[openhands-sdk/openhands/sdk/agent/acp_agent.py, ~Line 574] 🟡 Naming: The new function
_apply_acp_model_selectiondoesn't match the pattern of_maybe_set_session_modeland_reapply_session_model_on_resume. Consider renaming to_apply_acp_modelfor consistency, or renaming the callers for clarity.
[STYLE NOTES]
- The docstring updates throughout (replacing "session/set_model" with "model-selection") are appropriate and maintainable. Good job generalizing without losing precision.
[TESTING GAPS]
The test updates look comprehensive. The mock assertions in TestSetACPModel and TestResumeACPModel correctly verify that Codex now uses set_config_option instead of set_session_model. ✅
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Key risk factors considered:
- Dependency bump:
agent-client-protocol0.8.1 → 0.10.1 (19 days old, well past the 7-day threshold). The bump is necessary — the newset_config_optionmethod comes from the updated protocol library. - Dockerfile bump: codex-acp 0.15.0 → 0.16.0 is required for the protocol change to work.
- Behavioral change: External code checking
ACPProviderInfo.supports_set_session_modelfor Codex will now seeFalseinstead ofTrue. This is a breaking change, but the field's semantic meaning is now more accurate.
VERDICT
✅ Worth merging — The core logic is sound. The model_selection_method abstraction is the right approach for supporting heterogeneous ACP providers with different protocol implementations.
KEY INSIGHT
The PR correctly identifies that "supports session/set_model" and "uses session/set_model for model selection" are different properties — and separates them cleanly. The model_selection_method field is the right abstraction.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: PR #3688 — [codex] Fix ACP model selection methods
🟡 Acceptable — The core fix is sound, but there are a few concerns worth addressing.
[CRITICAL ISSUES]
- [openhands-sdk/openhands/sdk/settings/acp_providers.py, Line ~435] Breaking Change: Changing
supports_set_session_modelfromTruetoFalsefor Codex breaks any external code that reads this field directly. While the docstring now clarifies it's "legacy capability metadata," there's no deprecation warning or migration path for external consumers. Consider adding a deprecation notice in the docstring or a runtime warning for direct field access.
[IMPROVEMENT OPPORTUNITIES]
- [openhands-sdk/openhands/sdk/agent/acp_agent.py, Line ~3455-3467] Complexity: The
set_acp_modelmethod now has nested conditionals to determine which model-selection call to make. Consider extracting this into a helper method that usesmodel_selection_methoddirectly, rather than duplicating the conditional logic. This would reduce the 10-line conditional block to a single line.
[STYLE NOTES]
- [openhands-sdk/openhands/sdk/agent/acp_agent.py] The PR introduces many comment changes that summarize what the code does rather than explaining why. These comments add noise without value — the reader can see what the code does. Consider removing redundant comment updates where the code is self-explanatory.
[TESTING GAPS]
- [tests/sdk/agent/test_acp_agent.py] The tests properly verify the new behavior (using
set_config_optionfor Codex/Claude instead ofset_session_model). ✅ - [tests/sdk/settings/test_acp_providers.py] New assertions for
model_selection_methodare present. ✅
No gaps identified — the test updates look comprehensive.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Key risk factors:
- Breaking API change:
supports_set_session_modelis a public field that external code may rely on. The change fromTruetoFalsefor Codex is intentional but breaks backward compatibility without a deprecation path. - Dependency upgrade: Bumping
agent-client-protocolfrom 0.8.1 to 0.10.1 is a minor version bump, but the lock file shows the upload-time is 2026-05-24 (about 19 days ago), so the supply chain risk is low. - Docker image update: The agent-server Dockerfile now pins
@zed-industries/codex-acp@0.16.0instead of0.15.0.
The changes are well-tested and the rationale is clearly documented. The breaking API concern is the main item to address before merge.
VERDICT
✅ Worth merging — The core fix correctly addresses the real problem (Codex ACP using the wrong RPC for model selection). The existing review comments have been addressed by the author. Consider adding a deprecation note for supports_set_session_model if this PR is targeting a minor release, or document the breaking change in the release notes if it's a major release.
KEY INSIGHT
The PR correctly routes model selection through a provider-specific method (set_config_option vs set_session_model), but the field rename from supports_set_session_model semantics to model_selection_method is a subtle breaking change that needs explicit documentation for external consumers.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in this agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 QA Report: PARTIAL
SDK-level verification confirms this PR changes Codex/Claude ACP model selection from session/set_model to session/set_config_option while preserving Gemini's session/set_model; live provider E2E could not be run without ACP provider credentials.
Does this PR achieve its stated goal?
Partially verified: yes for the SDK behavior that caused the reported bug. Running the SDK's ACP model-selection paths before/after shows the base branch sends Codex and Claude switches through set_session_model, while this PR sends both through set_config_option(config_id='model', value=...); Gemini still uses set_session_model. I also verified the Codex default command now resolves to @zed-industries/codex-acp@0.16.0, but I could not verify a live Codex/Claude/Gemini provider session because no ACP provider credentials/session are available in this QA environment.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv run created the project env and imported openhands.sdk/acp successfully. |
| CI Status | REST API (OpenAPI) was failing and several Agent Server/QA checks were still in progress. |
| Functional Verification | 🟡 Local SDK dispatch verified before/after; live provider E2E not verified due missing credentials. |
Functional Verification
Test 1: Runtime ACP model switching dispatch
Step 1 — Reproduce / establish baseline (without the fix):
Ran git switch --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/acp_runtime_probe.py; rc=$?; git switch codex/acp-config-option-model; test "$rc" -eq 0.
Relevant output:
codex-acp
set_config_option: None
set_session_model: call(model_id='gpt-5.4/low', session_id='sess-qa')
claude-agent-acp
set_config_option: None
set_session_model: call(model_id='claude-haiku-4-5', session_id='sess-qa')
gemini-cli
set_config_option: None
set_session_model: call(model_id='auto-gemini-2.5', session_id='sess-qa')
This confirms the old behavior: runtime model switching for Codex and Claude used session/set_model, matching the PR's stated failure mode for current Codex ACP.
Step 2 — Apply the PR's changes:
Checked out codex/acp-config-option-model at 9b1bccaad02774d8bbc7166ce8d96cfe32228f41.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/acp_runtime_probe.py.
Relevant output:
codex-acp
set_config_option: call(config_id='model', value='gpt-5.4/low', session_id='sess-qa')
set_session_model: None
current_model: gpt-5.4/low
claude-agent-acp
set_config_option: call(config_id='model', value='claude-haiku-4-5', session_id='sess-qa')
set_session_model: None
current_model: claude-haiku-4-5
gemini-cli
set_config_option: None
set_session_model: call(model_id='auto-gemini-2.5', session_id='sess-qa')
current_model: auto-gemini-2.5
This shows the runtime switch now uses session/set_config_option for Codex/Claude and keeps Gemini on session/set_model, which matches the PR goal.
Test 2: Initial ACP model selection and Codex default package version
Step 1 — Reproduce / establish baseline (without the fix):
Ran git switch --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/acp_initial_probe.py; rc=$?; git switch codex/acp-config-option-model; test "$rc" -eq 0.
Relevant output:
codex-acp
applied: True
set_config_option: None
set_session_model: call(model_id='gpt-5.4/low', session_id='sess-init')
claude-agent-acp
applied: True
set_config_option: None
set_session_model: call(model_id='claude-haiku-4-5', session_id='sess-init')
gemini-cli
applied: True
set_config_option: None
set_session_model: call(model_id='auto-gemini-2.5', session_id='sess-init')
codex default command: ['npx', '-y', '@zed-industries/codex-acp@0.15.0']
This shows initial model selection previously used set_session_model for all three built-in providers, and the Codex default still targeted 0.15.0.
Step 2 — Apply the PR's changes:
Checked out codex/acp-config-option-model.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/acp_initial_probe.py.
Relevant output:
codex-acp
applied: True
set_config_option: call(config_id='model', value='gpt-5.4/low', session_id='sess-init')
set_session_model: None
claude-agent-acp
applied: True
set_config_option: call(config_id='model', value='claude-haiku-4-5', session_id='sess-init')
set_session_model: None
gemini-cli
applied: True
set_config_option: None
set_session_model: call(model_id='auto-gemini-2.5', session_id='sess-init')
codex default command: ['npx', '-y', '@zed-industries/codex-acp@0.16.0']
This confirms the initial-selection path now follows the provider-specific method matrix and the SDK default resolves Codex ACP 0.16.0.
Test 3: Runtime switch capability exposed to clients
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python - <<'PY' ... to instantiate ACPAgent objects with active sessions and read supports_runtime_model_switch.
Observed:
codex-acp True
claude-agent-acp True
gemini-cli True
custom-acp-server False
This matches the changed behavior described by the PR: known switch-capable providers expose the live-switch control; custom/unknown providers do not.
Unable to Verify
I could not run a live ACP provider conversation against @zed-industries/codex-acp, claude-agent-acp, or Gemini CLI because this environment does not include the required provider credentials/session state. The local SDK probes exercised the actual OpenHands SDK dispatch code and recorded the outbound ACP calls, but they do not prove a real provider accepts and applies the model switch end-to-end.
Future QA would be stronger if AGENTS.md documented a credentialed ACP smoke-test command or a local no-auth ACP fixture server that can be driven by ACPAgent without using the pytest suite.
Issues Found
None from manual functional QA. CI currently has a REST API (OpenAPI) failure and several in-progress checks; I noted that status but did not rerun or investigate CI per QA scope.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Fix ACP model selection methods
🟡 Acceptable — The refactoring is sound, but a few items warrant attention before merging.
[CRITICAL ISSUES]
None identified. The breaking change to supports_set_session_model for codex is documented as legacy capability metadata, and model_selection_method is the authoritative dispatch gate going forward.
[IMPROVEMENT OPPORTUNITIES]
- [openhands-sdk/pyproject.toml, Line 8] Dependency Floor: Bumping
agent-client-protocolto>=0.10.1is appropriate, but verify that all features used in this PR (specificallyset_config_option) are present in 0.10.1 and not a later version. The current floor could allow an older 0.10.x that lacks needed functionality.
[TESTING NOTES]
The test suite appropriately updates mock assertions to verify the correct method is called per provider. Given the ACP protocol complexity, mock-based testing is the pragmatic choice here. The test coverage for the new model_selection_method dispatch logic is adequate.
[STYLE NOTES]
- The comment block at
acp_agent.pylines ~538-552 is verbose for what it documents. Consider whether the inline code is self-explanatory enough without the narrative setup. (Low priority)
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The PR modifies protocol-level model selection behavior, which is core functionality. However:
- The breaking change to
supports_set_session_modelis documented - The dependency bump targets a stable version (~19 days old)
- Tests are updated and cover the new dispatch logic
- The refactoring is internally consistent across all providers
Recommendation: Safe to merge once CI passes.
VERDICT:
✅ Worth merging: Core logic is sound. The new model_selection_method abstraction provides a clean, extensible way to handle provider-specific protocol differences.
KEY INSIGHT:
The introduction of model_selection_method transforms a boolean capability flag (supports_set_session_model) into a proper dispatch field, enabling providers like codex-acp 0.16.0 that use set_config_option instead of the legacy set_session_model call.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Fix ACP model selection methods
🟡 Acceptable — The core fix is sound. The new model_selection_method abstraction cleanly handles provider-specific protocol differences, but a few items warrant attention.
[CRITICAL ISSUES]
None identified. The breaking change to supports_set_session_model for Codex is documented as legacy capability metadata, and model_selection_method is the authoritative dispatch gate going forward.
[IMPROVEMENT OPPORTUNITIES]
-
[openhands-sdk/pyproject.toml, Line 8] Dependency Floor: Bumping
agent-client-protocolto>=0.10.1is appropriate. Per the lock file, 0.10.1 was published on 2026-05-24 (~19 days ago), which meets the 7-day threshold. ✓ -
[openhands-sdk/openhands/sdk/agent/acp_agent.py] Code Organization: The new
_apply_acp_modelfunction (lines ~591-622) is well-structured and reduces duplication effectively. Good extraction of the common dispatch logic.
[TESTING GAPS]
The test suite updates are comprehensive:
- Mock assertions correctly verify
set_config_optionis called for Codex/Claude instead ofset_session_model - The
model_selection_methodfield is tested intest_acp_providers.py - Error handling paths are covered
Given the ACP protocol complexity and the need to mock external protocol calls, mock-based testing is the pragmatic choice here.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The PR modifies core protocol-level model selection behavior:
- Breaking change:
supports_set_session_modelchanges fromTruetoFalsefor Codex — documented as legacy metadata - Dependency bumps:
agent-client-protocol0.8.1→0.10.1,codex-acp0.15.0→0.16.0 — both stable releases - Test coverage: Adequate for the protocol complexity
- Internal consistency: The refactoring is consistent across all providers and call sites
Recommendation: Safe to merge once CI passes.
VERDICT:
✅ Worth merging: The model_selection_method abstraction provides a clean, extensible pattern for handling provider-specific protocol differences. The breaking change is intentional and documented.
KEY INSIGHT:
The PR correctly identifies that ACP servers can use different protocol methods (set_session_model vs set_config_option) for model selection, and the new abstraction cleanly decouples the capability declaration from the dispatch logic.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Fix ACP model selection methods
🟡 Acceptable — The core logic is sound and the change addresses a real production problem (Codex ACP conversations failing with "Method not found"). Well-tested with 111 passing tests. Minor improvements suggested below.
[IMPROVEMENT OPPORTUNITIES]
-
[openhands-sdk/openhands/sdk/settings/acp_providers.py, ~Line 176] Redundant Field: The
supports_set_session_modelfield is now somewhat misleading. The new dispatch logic usesmodel_selection_methodinstead, making the old field a dangling capability flag that no longer gates dispatch. Consider either:- Removing it entirely (if no external callers depend on it), or
- Updating its docstring to explicitly state it's deprecated and only used for legacy compatibility
-
[openhands-sdk/openhands/sdk/agent/acp_agent.py, ~Line 591] Inconsistent Fallback: The
_apply_acp_modelfunction has two code paths for known providers (direct call with return) and one for unknown providers (best-effort with error handling). This asymmetry is fine, but the control flow could be clearer with early returns or a helper method. -
[openhands-agent-server/openhands/agent_server/models.py, ~Line 230] Documentation Gap: The updated docstring says unknown/custom ACP servers get
"False"forsupports_runtime_model_switchbecause "generic config writes are not guaranteed live-switch primitives." This is a behavioral change from the previous optimistic approach. Consider adding a note that this is intentional and explain the reasoning briefly.
[TESTING GAPS]
None identified. The PR has excellent test coverage:
- Unit tests for
_maybe_set_session_modelwith mocked connections - Tests for
_reapply_session_model_on_resume - Tests for
set_acp_modelruntime switching - Router tests for HTTP status code mapping (400 for protocol errors, 504 for timeouts)
- Integration-style tests for session resume behavior
[SUPPLY CHAIN]
agent-client-protocol: bumped from>=0.8.1to>=0.10.1. The target version (0.10.1) was published on 2026-05-24, which is 19 days ago — well past the 7-day threshold.@zed-industries/codex-acp: bumped from0.15.0to0.16.0. Same package family, no new supply chain concerns.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Key factors:
- Targeted fix to ACP model selection protocol — isolated scope
- Comprehensive test coverage (111 tests pass)
- Clear provider matrix: Codex and Claude use
set_config_option, Gemini stays onset_session_model - No breaking changes to public APIs — external callers constructing
ACPProviderInfopositionally get safe defaults - Dependency versions are mature (19+ days old)
VERDICT:
✅ Worth merging — This is a well-scoped bug fix that correctly addresses the "Method not found" failure in Codex ACP conversations. The new model_selection_method abstraction is clean and extensible. The minor documentation/redundancy issues noted above are suggestions, not blockers.
KEY INSIGHT:
The PR introduces an elegant provider-specific dispatch abstraction (model_selection_method) that cleanly handles the protocol divergence between ACP providers, replacing fragile hardcoded method calls with a declarative configuration approach.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 QA Report: PARTIAL
SDK-level ACP model-selection behavior works as intended, but a live Codex ACP turn could not be completed because this environment has no provider credentials.
Does this PR achieve its stated goal?
Yes for the changed SDK behavior I could exercise; full live-provider E2E remains unverified. Running the same SDK harness before/after shows the bug clearly: main calls session/set_model for Codex/Claude and fails when that method is missing, while this PR calls session/set_config_option with config_id="model" for Codex/Claude across initial selection, resume reapply, and runtime switching. Gemini still uses session/set_model, matching the PR’s provider matrix.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; no test/lint suite was run |
| CI Status | REST API (OpenAPI)), 8 pending, 3 skipped |
| Functional Verification | ✅ SDK dispatch/defaults verified before/after; |
Functional Verification
Test 1 — ACP model-selection dispatch when session/set_model is missing
Step 1 — Reproduce / establish baseline without the fix:
Ran OPENHANDS_SUPPRESS_BANNER=1 UV_PROJECT_ENVIRONMENT=/tmp/oh-sdk-main-venv uv run --project /tmp/oh-sdk-main python /tmp/qa_acp_model_selection.py on origin/main (c1001c86). The harness calls the real SDK model-selection paths with a connection that rejects session/set_model as Method not found and accepts session/set_config_option:
{
"codex-acp": {
"initial_when_set_model_missing": {"ok": false, "calls": [{"method": "session/set_model"}], "error": "RequestError: Method not found"},
"runtime_when_set_model_missing": {"ok": false, "calls": [{"method": "session/set_model"}], "error": "ValueError: ACP server rejected set_session_model(model=qa-model): Method not found"}
},
"claude-agent-acp": {
"initial_when_set_model_missing": {"ok": false, "calls": [{"method": "session/set_model"}], "error": "RequestError: Method not found"}
}
}This confirms the reported failure mode: known providers that no longer expose session/set_model cannot apply the requested model on the base branch.
Step 2 — Apply the PR changes:
Used the checked-out PR branch at 5fec2c0173787b66c287c31a0c53f3dd01676434 after make build.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_model_selection.py on the PR branch:
{
"codex-acp": {
"initial_when_set_model_missing": {"ok": true, "applied": true, "calls": [{"method": "session/set_config_option", "config_id": "model", "value": "qa-model"}]},
"resume_when_set_model_missing": {"ok": true, "applied": true, "calls": [{"method": "session/set_config_option", "config_id": "model", "value": "qa-model"}]},
"runtime_when_set_model_missing": {"ok": true, "calls": [{"method": "session/set_config_option", "config_id": "model", "value": "qa-model"}], "llm_model": "qa-model"}
},
"claude-agent-acp": {
"initial_when_set_model_missing": {"ok": true, "applied": true, "calls": [{"method": "session/set_config_option", "config_id": "model", "value": "qa-model"}]},
"runtime_when_set_model_missing": {"ok": true, "calls": [{"method": "session/set_config_option", "config_id": "model", "value": "qa-model"}], "llm_model": "qa-model"}
},
"gemini-cli": {
"initial_when_set_model_missing": {"ok": false, "calls": [{"method": "session/set_model"}], "error": "RequestError: Method not found"}
}
}This shows the fix works for Codex and Claude, including runtime switching updating the sentinel LLM model after a successful protocol call. The Gemini failure is expected in this simulated server because the PR intentionally keeps Gemini on session/set_model.
Test 2 — User-facing Codex ACP defaults and dependency version
Baseline:
Ran OPENHANDS_SUPPRESS_BANNER=1 UV_PROJECT_ENVIRONMENT=/tmp/oh-sdk-main-venv uv run --project /tmp/oh-sdk-main python /tmp/qa_acp_defaults.py:
{
"agent_client_protocol_version": "0.8.1",
"resolved_codex_command": ["npx", "-y", "@zed-industries/codex-acp@0.15.0"]
}PR branch:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_defaults.py:
{
"agent_client_protocol_version": "0.10.1",
"resolved_codex_command": ["npx", "-y", "@zed-industries/codex-acp@0.16.0"]
}This verifies the PR’s dependency/default command bump is visible through the SDK settings path a user would call.
Test 3 — Live Codex ACP startup attempt
Ran OPENHANDS_SUPPRESS_BANNER=1 timeout 60 uv run python /tmp/qa_codex_live_attempt.py, which creates an ACPAgent with npx -y @zed-industries/codex-acp@0.16.0, sends a real conversation message, and uses an empty temporary CODEX_HOME:
ACP server initialized: agent_name=codex-acp, agent_version=0.16.0
ACP server offers auth methods [chatgpt, codex-api-key, openai-api-key] but no matching env var is set — session creation may fail
LIVE_CODEX_RESULT: failed
RequestError: Authentication required
This confirms the actual provider package starts under the SDK, but the environment cannot complete a live provider turn or observe the provider accepting the model-selection RPC without Codex/OpenAI credentials.
Unable to Verify
A true live Codex/Claude/Gemini provider E2E model switch was not verified because this QA environment does not include ACP provider credentials (chatgpt, codex-api-key, openai-api-key, etc.). I attempted a real Codex ACP conversation startup and it failed at session creation with RequestError: Authentication required. Future QA guidance in AGENTS.md could document a safe non-production ACP credential setup or a local ACP fixture command for credential-free provider-level verification.
Issues Found
None from functional QA. Note that REST API (OpenAPI) was failing in CI at the time of this review, but I did not investigate CI because this QA run did not execute tests or linters.
This review was created by an AI agent (OpenHands) on behalf of the user.
Verdict: PARTIAL
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: [codex] Fix ACP model selection methods
Taste Rating
🟡 Acceptable — Works correctly but has some design considerations worth discussing.
Analysis
[CRITICAL ISSUES] (None identified)
[IMPROVEMENT OPPORTUNITIES]
-
[openhands-sdk/openhands/sdk/settings/acp_providers.py, Line 287] Semantic Drift: The
supports_set_session_modelfield's meaning has drifted without a rename. Previously it meant "uses set_session_model for initial selection"; now it means "supports the literal set_session_model RPC call" with a deprecation note in the docstring. A renamed field (e.g.,supports_legacy_set_session_modelor justhas_legacy_rpc) would make the semantics clearer for external consumers who constructACPProviderInfopositionally. -
[openhands-sdk/openhands/sdk/agent/acp_agent.py, Line 584-605] Suggestion: The
_apply_acp_modelfunction has two early-return paths (set_config_optionandset_session_model) that fall through toreturn Falseat the end. The control flow is correct but slightly confusing — consider an explicitelse: return Falseor restructuring to make the dispatch clearer. -
[openhands-sdk/openhands/sdk/agent/acp_agent.py, Line 593] Suggestion: The comment says "For unknown/custom providers, try set_config_option as a best-effort" but the code always calls it without catching exceptions (unlike the known-provider path). This is actually correct behavior, but the comment is slightly misleading since the outer
_maybe_set_session_modelalready has the try/except wrapper.
[STYLE NOTES]
- Most docstring updates are good updates — clarifying that "set_session_model" is no longer the only model-selection method. Well done.
- The comment cleanup in
conversation_router.pyandevent_service.pyis appropriate.
[TESTING GAPS]
- Tests appear comprehensive for the changed behavior. The mock updates correctly assert
set_config_optionfor codex-acp and claude-agent-acp instead ofset_session_model.
[DEPENDENCY CHANGES]
- [openhands-sdk/pyproject.toml] agent-client-protocol: Bumped from
>=0.8.1to>=0.10.1. This version was published on 2026-05-24, which is ~20 days ago — well beyond the 7-day safety threshold. No supply chain risk. - [openhands-agent-server/openhands/agent_server/docker/Dockerfile] codex-acp: Bumped from
0.15.0to0.16.0. This aligns with the code change that switches fromset_session_modeltoset_config_option.
Risk Assessment
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The changes are a clean refactor to support multiple model-selection methods across ACP providers. The logic is well-structured with proper error handling. The dependency upgrade (agent-client-protocol 0.8.1→0.10.1) is safe — published ~20 days ago. Codex-acp bump to 0.16.0 appears intentional and aligns with the protocol changes.
Verdict
✅ Worth merging — The refactor correctly addresses the codex-acp model selection issue while maintaining backward compatibility for other providers.
Key Insight
The introduction of model_selection_method is a pragmatic solution to support heterogeneous ACP server implementations, but the legacy supports_set_session_model field name now carries misleading semantics that should be addressed in a follow-up for clarity.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
The ACP Codex/Claude model-selection flow now completes through session/set_config_option where the base branch failed with Method not found during session startup.
Does this PR achieve its stated goal?
Yes. The PR set out to stop Codex/Claude ACP conversations from calling the obsolete session/set_model RPC and instead use session/set_config_option with configId="model"; I verified this by running an actual openhands.sdk.Conversation against a fake ACP stdio agent that accepts set_config_option and rejects set_model. On origin/main, the same user flow failed immediately after new_session with RequestError: Method not found; on this PR, the conversation initialized, sent a prompt, and runtime model switching updated the active model through set_config_option for both Codex and Claude provider names.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv sync --dev completed and installed PR dependencies, including agent-client-protocol==0.10.1. |
| CI Status | ⏳ Most checks are green; Build & Push (golang-arm64) and this QA check were still pending when queried. |
| Functional Verification | ✅ Base failure reproduced; PR Codex and Claude ACP flows completed through config-option model selection and runtime switching. |
Functional Verification
Test 1: Codex ACP initial model selection and runtime switch
Step 1 — Reproduce / establish baseline without the fix:
Checked out origin/main, synced dependencies, then ran the SDK as a user would with a fake ACP stdio agent named codex-acp. The fake agent supports session/set_config_option but rejects session/set_model with Method not found.
Ran:
git checkout --detach origin/main
uv sync --dev
QA_ACP_LOG=/tmp/qa_acp_base.log OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_user_flow.pyObserved:
{
"error": "Method not found",
"error_type": "RequestError",
"events": [
{"event": "initialize", "protocol_version": 1},
{"event": "new_session", "cwd": "/tmp", "kwargs": {}}
],
"status": "error"
}This confirms the pre-PR failure mode: after creating the ACP session, the SDK attempted a model-selection RPC the fake current-style Codex server did not implement, surfacing the same Method not found class of error described by the PR.
Step 2 — Apply the PR's changes:
Checked out codex/acp-config-option-model at 71c497e28c4bc6237a698561872a50470b1d3cd1 and re-synced dependencies.
Step 3 — Re-run with the fix in place:
Ran the same conversation flow and then switched the ACP model at runtime:
QA_ACP_LOG=/tmp/qa_acp_pr_switch.log QA_SWITCH_MODEL=gpt-5.5/high OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_user_flow.pyObserved:
{
"current_model_id": "gpt-5.5/high",
"events": [
{"event": "initialize", "protocol_version": 1},
{"event": "new_session", "cwd": "/tmp", "kwargs": {}},
{"event": "set_config_option", "config_id": "model", "value": "gpt-5.5/medium", "session_id": "qa-session"},
{"event": "set_session_mode", "mode_id": "full-access", "session_id": "qa-session"},
{"event": "prompt", "prompt_blocks": 1, "session_id": "qa-session"},
{"event": "set_config_option", "config_id": "model", "value": "gpt-5.5/high", "session_id": "qa-session"}
],
"status": "ok",
"supports_runtime_model_switch": true
}This shows the PR fixes the Codex path: initial selection and runtime switching both used set_config_option(config_id="model"), the prompt ran, and the SDK's active model reflected the switched model.
Test 2: Claude ACP initial model selection and runtime switch
Step 1 — Establish equivalent baseline:
The base-branch Codex run above established the failing behavior for a provider that does not implement session/set_model. I then reused the same fake ACP agent pattern with the provider name changed to claude-agent-acp on the PR branch to verify the new Claude dispatch path.
Step 2 — Apply the PR's changes:
PR branch remained checked out at 71c497e28c4bc6237a698561872a50470b1d3cd1.
Step 3 — Run with the fix in place:
Ran:
QA_ACP_LOG=/tmp/qa_acp_pr_claude.log QA_ACP_AGENT_NAME=claude-agent-acp QA_ACP_MODEL=claude-opus-4-8 QA_SWITCH_MODEL=claude-haiku-4-5-20251001 OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_user_flow.pyObserved:
{
"current_model_id": "claude-haiku-4-5-20251001",
"events": [
{"event": "initialize", "protocol_version": 1},
{"event": "new_session", "cwd": "/tmp", "kwargs": {"claudeCode": {"options": {"model": "claude-opus-4-8"}}}},
{"event": "set_config_option", "config_id": "model", "value": "claude-opus-4-8", "session_id": "qa-session"},
{"event": "set_session_mode", "mode_id": "bypassPermissions", "session_id": "qa-session"},
{"event": "prompt", "prompt_blocks": 1, "session_id": "qa-session"},
{"event": "set_config_option", "config_id": "model", "value": "claude-haiku-4-5-20251001", "session_id": "qa-session"}
],
"status": "ok",
"supports_runtime_model_switch": true
}This confirms the Claude path also uses session/set_config_option for both the authoritative initial model application and the live runtime switch, while preserving the claudeCode _meta payload and bypass-permissions session mode.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review: ACP Model Selection Methods
Taste Rating
🟡 Acceptable — Works correctly, some minor improvements possible.
Analysis
This PR addresses a real protocol compatibility issue with ACP model selection by introducing a model_selection_method dispatch field to handle different providers using different ACP protocol methods (set_session_model vs set_config_option). The refactoring is clean and the changes are well-tested.
[CRITICAL ISSUES]
None — no blocking issues found.
[IMPROVEMENT OPPORTUNITIES]
-
[
openhands-sdk/openhands/sdk/agent/acp_agent.py, Line 3462] Control Flow: Themodel_selection_callvariable is assigned before the try block but only used inside it. This creates a temporal coupling where the assignment must precede the try. Consider inlining it directly inrun_async()to make the dependency explicit and avoid a dangling variable if the code is refactored. -
[
openhands-sdk/openhands/sdk/settings/acp_providers.py, Line 45] Type Documentation: TheModelSelectionMethodLiteral type is well-defined but could benefit from a module-level docstring explaining the distinction between the two methods and when each should be used.
[TESTING GAPS]
✅ Tests updated comprehensively to cover the new dispatch logic. All three providers (claude-code, codex, gemini-cli) have explicit assertions verifying the correct method is called.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Key risk factors:
-
Behavioral change for built-in providers: This PR changes the ACP protocol method used by both
claude-codeandcodexfromset_session_modeltoset_config_option. While the tests verify the new behavior, this is a protocol-level change that depends on the ACP server implementations supporting the new method. The codex-acp version bump to 0.16.0 suggests the server side has been updated, but verify the claude-agent-acp 0.30.0 server actually supportsset_config_optionwithconfigId="model". -
External consumer migration: The
supports_set_session_modelfield is documented as legacy, but any external code using it for dispatch decisions will silently break. Consider a deprecation warning or a migration guide. -
Gemini CLI unchanged: Good that gemini-cli continues using
set_session_model— the Literal type and dispatch logic handles the variation correctly.
VERDICT:
✅ Worth merging — Core logic is sound, minor improvements suggested.
KEY INSIGHT:
The model_selection_method dispatch is a pragmatic solution to provider heterogeneity in the ACP protocol. The refactoring eliminates duplicated model-setting logic while maintaining backward compatibility through the legacy supports_set_session_model field.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 QA Report: PARTIAL
The ACP model-selection bug is fixed in the exercised SDK paths: Codex/Claude-style config-option selection works for initial selection and runtime switching, while Gemini remains on session/set_model; a full live Codex prompt still failed after successful model selection with an ACP provider Internal error.
Does this PR achieve its stated goal?
Yes for the stated model-selection fix, with one live-provider limitation. On origin/main, a Codex-like ACP session that does not expose session/set_model failed at startup with RequestError: Method not found. On this PR, the same SDK Conversation flow initialized, sent a prompt, and runtime-switched models; the ACP call log shows session/set_config_option with config_id="model" for both initial selection and switching. I also launched the real @zed-industries/codex-acp@0.16.0 package and observed OpenHands authenticate and successfully set gpt-5.5/medium via set_config_option; the subsequent provider prompt failed with Internal error, so I could not verify a complete live Codex response in this environment.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and installed the workspace with agent-client-protocol 0.10.1. |
| CI Status | 🟡 At last check, core checks/tests were successful; agent-server image builds and this QA check were still in progress. |
| Functional Verification | 🟡 Model-selection behavior verified through real SDK Conversation/ACP stdio flows; complete live Codex prompt response not verified due provider Internal error. |
Functional Verification
Test 1: Reproduce Codex Method not found on base, verify PR uses session/set_config_option
Step 1 — Reproduce baseline without the fix:
Ran git checkout --detach origin/main && FAKE_ACP_LOG=/tmp/acp-main.jsonl FAKE_ACP_REJECT_SET_MODEL=1 OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/run_acp_qa.py.
Relevant output:
{
"initial_status": "RequestError: Method not found",
"switch_status": "RuntimeError: ACP session is not initialized; the model can only be switched after the conversation has started (first run()).",
"calls": [
{"method": "initialize", "protocol_version": 1},
{"method": "session/new", "meta": null}
]
}This confirms the reported failure mode: a Codex-like ACP server that does not expose the old session/set_model path causes startup to fail with Method not found, so no prompt or runtime switch can happen.
Step 2 — Apply the PR's changes:
Checked out codex/acp-config-option-model at 263a782f5c3442d2a26e20669c9d0b8b745b47e4.
Step 3 — Re-run with the fix in place:
Ran FAKE_ACP_LOG=/tmp/acp-pr.jsonl FAKE_ACP_REJECT_SET_MODEL=1 OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/run_acp_qa.py.
Relevant output:
{
"initial_status": "ok",
"switch_status": "ok",
"calls": [
{"method": "initialize", "protocol_version": 1},
{"method": "session/new", "meta": null},
{"method": "session/set_config_option", "config_id": "model", "value": "gpt-qa-model"},
{"method": "session/set_mode", "mode_id": "full-access"},
{"method": "session/prompt", "blocks": 1},
{"method": "session/set_config_option", "config_id": "model", "value": "gpt-qa-model-2"}
]
}This shows the fix works for the affected path: initial model selection and runtime switching both use session/set_config_option with config_id="model", and the conversation proceeds through a prompt instead of failing at model selection.
Test 2: Verify Gemini still uses session/set_model
Ran FAKE_ACP_LOG=/tmp/acp-pr-gemini.jsonl FAKE_ACP_AGENT_NAME=gemini-cli FAKE_ACP_USE_UNSTABLE=1 OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/run_acp_qa.py.
Relevant output:
{
"initial_status": "ok",
"switch_status": "ok",
"calls": [
{"method": "session/new"},
{"method": "session/set_model", "model_id": "gpt-qa-model"},
{"method": "session/set_mode", "mode_id": "yolo"},
{"method": "session/prompt", "blocks": 1},
{"method": "session/set_model", "model_id": "gpt-qa-model-2"}
]
}This confirms the PR did not globally replace model selection with config options: Gemini-detected ACP sessions still use the unstable session/set_model method for initial selection and runtime switching.
Test 3: Launch the real Codex ACP package at the bumped version
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/run_real_codex_acp.py, which starts npx -y @zed-industries/codex-acp@0.16.0 through ACPAgent, passes the available LLM credentials as OpenAI-compatible env vars, and sends one real prompt.
Relevant output:
ACP server initialized: agent_name='codex-acp', agent_version='0.16.0'
Authenticating with ACP method: openai-api-key
Set model 'gpt-5.5/medium' on ACP provider codex via set_config_option
Setting ACP session mode: full-access
Sending ACP prompt (idle_timeout=45s, blocks=1)
ACP prompt failed with server error ... Internal error
{"status": "error", "error_type": "ConversationRunError", "error": "Conversation run failed ... Internal error"}
This verifies the real bumped Codex ACP package starts, authenticates, and accepts the PR's set_config_option model-selection path. The actual prompt did not complete due an ACP provider Internal error after selection, so the live package smoke test supports the model-selection fix but not full live response generation.
Test 4: Verify installed/default dependency state on the PR
Ran an SDK settings script that prints dependency and Codex default metadata.
Observed:
agent-client-protocol 0.10.1
CODEX_ACP_VERSION 0.16.0
resolved ['npx', '-y', '@zed-industries/codex-acp']
This confirms the PR environment is using the dependency floor needed for config-option support and the SDK default Codex version constant is bumped to 0.16.0.
Unable to Verify
I could not verify a complete live Codex provider turn end-to-end. The real @zed-industries/codex-acp@0.16.0 process initialized and accepted model selection through session/set_config_option, but the subsequent prompt returned ACP Internal error after retries. Future QA would benefit from AGENTS.md guidance describing a known-good ACP provider credential/model combination for live Codex/Claude smoke tests.
Issues Found
None.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Elegant, targeted fix with clean design.
This PR correctly handles Canvas Codex combined model IDs (model/reasoning_effort format) by splitting them into the two separate config options that codex-acp 0.16+ exposes. The use of rpartition("/") is the right choice — it naturally handles multi-segment paths like custom/provider/model without incorrectly splitting them, since effort would not match _CODEX_REASONING_EFFORTS. The agent_name or "" guard handles None cleanly. Test coverage is thorough and exercises real code paths.
One non-blocking observation: the sequential set_config_option loop (first model, then reasoning_effort) could leave a partial state if the second call fails while the first succeeded. In practice both options hit the same server so this is unlikely, and the surrounding ACPRequestError catch already surfaces failures gracefully — worth keeping in mind if more options are added later.
The supports_runtime_model_switch field description update (custom ACP servers → False) and the dependency bump to agent-client-protocol>=0.10.1 (published 2026-05-24, 25 days ago — well past the 7-day supply chain threshold) are both correct.
Evidence: screenshots in the PR description clearly show before/after behavior. ✅
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Well-scoped to Codex ACP model-selection logic. The only outward-facing behavior change issupports_runtime_model_switchflipping toFalsefor unknown ACP servers (removes an optimistic live-switch control that didn't actually work), which is intentional and evidenced by the screenshots. Good test coverage mitigates regression risk.
VERDICT:
✅ Worth merging — Core logic is sound, fix is correct, evidence provided.
KEY INSIGHT:
rpartition("/") elegantly handles both bare model IDs and combined model/effort IDs without a special-case branch — the effort membership check does all the discrimination.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 QA Report: PARTIAL
Verified the ACP model-selection/config-option behavior through real SDK execution paths and a live Codex ACP session initialization; full provider prompt completion could not be verified without real provider credentials.
Does this PR achieve its stated goal?
Partially verified, with positive evidence for the core model-selection behavior. Against main, the executable SDK paths for Codex combined Canvas IDs changed from one session/set_config_option(config_id="model", value="gpt-5.5/high") call to two config-option writes: model=gpt-5.5 and reasoning_effort=high, while Claude still uses session/set_config_option and Gemini still uses session/set_model. A real @zed-industries/codex-acp@0.16.0 session initialized successfully through the SDK with model_override_applied=true, but I could not verify a completed Codex prompt because real ACP provider credentials were unavailable.
| Phase | Result |
|---|---|
| Environment Setup | ✅ Dependencies were bootstrapped with the repo’s uv environment; QA scripts ran via uv run python. |
| CI Status | 🟡 Most checks are green; evaluate was still in progress and several label/dispatch workflows were skipped when checked. |
| Functional Verification | 🟡 ACP model-selection paths verified; full live provider prompt completion not verified due credential limits. |
Functional Verification
Test 1: SDK model-selection call shape for initial apply, resume reapply, and runtime switching
Step 1 — Reproduce / establish baseline on main:
Ran git checkout --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_model_paths.py.
Relevant baseline output:
{
"codex_combined": {
"initial_apply": {
"applied": true,
"calls": [
{
"method": "session/set_config_option",
"config_id": "model",
"value": "gpt-5.5/high",
"session_id": "sess-qa"
}
]
},
"resume_apply": {
"applied": true,
"calls": [
{
"method": "session/set_config_option",
"config_id": "model",
"value": "gpt-5.5/high",
"session_id": "sess-qa"
}
]
},
"runtime_switch": {
"calls": [
{
"method": "session/set_config_option",
"config_id": "model",
"value": "gpt-5.5/high",
"session_id": "sess-qa"
}
],
"current_model_id_after": "gpt-5.5/high",
"llm_model_after": "gpt-5.5/high"
}
},
"gemini_session_model": {
"runtime_switch": {
"calls": [
{
"method": "session/set_model",
"model_id": "gemini-2.5-pro",
"session_id": "sess-qa"
}
]
}
}
}This shows current main already uses session/set_config_option for Codex in this exercised path, but it sends the combined Canvas ID as the model config value.
Step 2 — Apply the PR's changes:
Checked out c0511f7bb581a7addd7927279624803bbdbd604d.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_acp_model_paths.py.
Relevant PR output:
{
"codex_combined": {
"initial_apply": {
"applied": true,
"calls": [
{
"method": "session/set_config_option",
"config_id": "model",
"value": "gpt-5.5",
"session_id": "sess-qa"
},
{
"method": "session/set_config_option",
"config_id": "reasoning_effort",
"value": "high",
"session_id": "sess-qa"
}
]
},
"resume_apply": {
"applied": true,
"calls": [
{"method": "session/set_config_option", "config_id": "model", "value": "gpt-5.5", "session_id": "sess-qa"},
{"method": "session/set_config_option", "config_id": "reasoning_effort", "value": "high", "session_id": "sess-qa"}
]
},
"runtime_switch": {
"calls": [
{"method": "session/set_config_option", "config_id": "model", "value": "gpt-5.5", "session_id": "sess-qa"},
{"method": "session/set_config_option", "config_id": "reasoning_effort", "value": "high", "session_id": "sess-qa"}
],
"current_model_id_after": "gpt-5.5/high",
"llm_model_after": "gpt-5.5/high"
}
},
"claude_config_option": {
"runtime_switch": {
"calls": [
{"method": "session/set_config_option", "config_id": "model", "value": "claude-opus-4", "session_id": "sess-qa"}
]
}
},
"gemini_session_model": {
"runtime_switch": {
"calls": [
{"method": "session/set_model", "model_id": "gemini-2.5-pro", "session_id": "sess-qa"}
]
}
}
}This shows the PR changes the actual initial, resume, and runtime model-selection paths to split Codex combined IDs into separate model and reasoning_effort config-option writes, while preserving the expected Claude and Gemini method choices.
Test 2: Real Codex ACP package session initialization through the SDK
Step 1 — Reproduce / establish baseline on main:
Ran git checkout --detach origin/main && OPENHANDS_SUPPRESS_BANNER=1 OPENAI_API_KEY=sk-qa-placeholder timeout 25s uv run python /tmp/qa_codex_session_init.py.
Relevant baseline output:
ACP server initialized: agent_name='codex-acp', agent_version='0.16.0'
Authenticating with ACP method: openai-api-key
Setting ACP session mode: full-access
{
"agent_name": "codex-acp",
"agent_version": "0.16.0",
"current_model_id": "gpt-5.5/high",
"model_override_applied": true,
"model_via_config_option": true,
"session_id_present": true
}
This establishes that the actual Codex ACP package can be started by the SDK in this environment with a dummy API-key placeholder and that session/model override initialization reaches a live session.
Step 2 — Apply the PR's changes:
Checked out c0511f7bb581a7addd7927279624803bbdbd604d.
Step 3 — Re-run with the fix in place:
Ran OPENHANDS_SUPPRESS_BANNER=1 OPENAI_API_KEY=sk-qa-placeholder timeout 25s uv run python /tmp/qa_codex_session_init.py.
Relevant PR output:
ACP server initialized: agent_name='codex-acp', agent_version='0.16.0'
Authenticating with ACP method: openai-api-key
Setting ACP session mode: full-access
{
"agent_name": "codex-acp",
"agent_version": "0.16.0",
"current_model_id": "gpt-5.5/high",
"model_override_applied": true,
"model_via_config_option": true,
"session_id_present": true
}
This confirms the PR still initializes a real Codex ACP 0.16.0 session through the SDK and applies the model override without a Method not found failure during session setup.
Unable to Verify
I could not verify a full live Codex prompt response with real provider credentials. I attempted:
OPENHANDS_SUPPRESS_BANNER=1 timeout 25s uv run python /tmp/qa_codex_live_attempt.pyIt failed during session creation as expected without credentials:
ACP server initialized: agent_name='codex-acp', agent_version='0.16.0'
ACP server offers auth methods ['chatgpt', 'codex-api-key', 'openai-api-key'] but no matching env var is set — session creation may fail
Failed to start ACP server: Authentication required
acp.exceptions.RequestError: Authentication required
I then retried with a dummy OPENAI_API_KEY=sk-qa-placeholder; the SDK authenticated to the local ACP server, created a session, and reached prompt execution, but the prompt itself failed/retried with provider-side internal errors and was stopped by the timeout:
ACP server initialized: agent_name='codex-acp', agent_version='0.16.0'
Authenticating with ACP method: openai-api-key
Setting ACP session mode: full-access
Sending ACP prompt (idle_timeout=10s, blocks=1)
ACP prompt failed with server error (attempt 1/4), retrying in 5s: [-32603] Internal error
A future QA run with real Codex/OpenAI/ChatGPT ACP credentials could verify the final provider response end-to-end.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
Verdict: PARTIAL
2 participants
HUMAN:
I have confirmed that this was broken before and fixed after.
before
after
AGENT:
This is a draft PR. I did not run a live provider end-to-end test because that requires real ACP provider credentials. I did verify the published provider packages' implemented methods and ran focused SDK/agent-server coverage plus pre-commit.
Why
Codex ACP conversations can fail with
Method not foundbecause the SDK was still calling the oldersession/set_modelRPC for model selection. Current@zed-industries/codex-acpexposes model changes throughsession/set_config_optionwithconfigId="model"instead.Provider compatibility checked from current npm packages:
@zed-industries/codex-acp@0.16.0: implements model selection viasession/set_config_option.@agentclientprotocol/claude-agent-acp@0.44.0, and the currently pinned0.30.0: implementsetSessionConfigOptionforconfigId="model".@google/gemini-cli@0.46.0: still implementsunstable_setSessionModel; its ACP agent class does not implementsetSessionConfigOption.Summary
set_config_optionand Gemini staying onset_session_model.agent-client-protocolto>=0.10.1and@zed-industries/codex-acpto0.16.0in SDK defaults and the agent-server Docker install.session/set_config_optionsupport. [skip-acp-check]Issue Number
Closes #3736
How to Test
Commands run:
Result:
111 passed in 0.38s.Result:
All checks passed!.Pre-commit results: YAML skipped, Ruff format passed, Ruff lint passed, pycodestyle passed, pyright passed, import dependency rules passed, tool registration passed.
Video/Screenshots
N/A. This is a backend/protocol compatibility fix. Live provider E2E was not run because it needs real ACP provider credentials.
Type
Notes
There is no known-provider method fallback in this change. The provider matrix is explicit: Codex and Claude use
session/set_config_option, Gemini usessession/set_modeluntil Gemini updates its ACP agent implementation.Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:c0511f7-pythonRun
All tags pushed for this build
About Multi-Architecture Support
c0511f7-python) is a multi-arch manifest supporting both amd64 and arm64c0511f7-python-amd64) are also available if needed