Skip to content

feat: generic model selection via ACP session/set_model#150

Open
ironerumi wants to merge 6 commits intoopenclaw:mainfrom
ironerumi:main
Open

feat: generic model selection via ACP session/set_model#150
ironerumi wants to merge 6 commits intoopenclaw:mainfrom
ironerumi:main

Conversation

@ironerumi
Copy link
Contributor

Summary

  • --model <id> at session creation calls session/set_model when agent advertises models
  • acpx <agent> set model <id> routes through session/set_model instead of session/set_config_option
  • Model state tracking, caching, and status enrichment
  • Graceful degradation: agents without model support skip silently; ACP-coded rejections produce clear errors
  • Model cache only updated on successful set

Test plan

  • Integration tests: 8 model-related tests pass (spawn-time set, mid-session set, invalid params rejection, status enrichment)
  • Smoke tested against live Droid and Gemini CLI agents
  • Typecheck clean (tsc --noEmit)
  • Pre-commit hooks pass (lint, format, build)

Closes #148
Partially addresses #49

🤖 Generated with Claude Code

ironerumi and others added 5 commits March 17, 2026 02:36
Add agent-agnostic model selection that works across any ACP agent
implementing session/set_model (e.g. Droid, Gemini), not tied to
any agent's _meta conventions.

- --model <id> flag at spawn time calls session/set_model after
  session creation when agent advertises available models
- `acpx <agent> set model <id>` mid-session routes through
  session/set_model instead of session/set_config_option
- Model state tracking, caching, and status enrichment
- Graceful degradation: agents without model support skip silently;
  ACP-coded rejections produce clear error messages
- Model cache only updated on successful set (not on rejection)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@osolmaz
Copy link
Contributor

osolmaz commented Mar 26, 2026

Triage result

Human attention: ⚠️ Required
Recommendation: 🏁 escalate to a human
Human decision needed: confirm or refresh CI for latest head 847135c, then decide whether to merge as-is or request follow-up for the remaining non-blocking P2s.

Quick read

This PR is directionally right and the targeted feature validation passed.

Blocking Codex review findings were fixed locally and pushed; no P0/P1 findings remain for current head 847135c.

The remaining handoff reason is CI state: the stored CI snapshot is green, but it is for older head 29c6475, not the current PR head.

Intent

Make acpx support model selection in a generic ACP-native way across agents, both at session start and mid-session, while tracking and surfacing the active model.

Why

Before this PR, model selection was tied to Claude-specific _meta passthrough or session/set_config_option, so ACP agents expecting session/set_model did not get reliable model switching. Model state also was not consistently persisted, replayed, or shown in status.

Codex review

Local review for the current branch head is clear on blocking severity.

Handled in this lane:

  • Fixed json-strict leakage from model-switch warnings.
  • Fixed replay logic so reconnects only replay a user-requested model override, not an adapter-reported default model.
  • Added focused regression coverage for those paths.

Current review status:

  • No GitHub Codex review findings were present in the stored review state.
  • No local P0/P1 findings remain for head 847135c.
  • Two P2 items remain and were treated as non-blocking in this lane:
  • sessions ensure --model does not apply the model when reusing an existing session.
  • reconnect/load paths do not backfill returned model metadata into older saved records that lack those fields.

CI/CD

Targeted feature validation passed on the PR branch using:

  • pnpm run build:test
  • node --test dist-test/test/integration.test.js

Additional focused reruns were completed locally after fixes in this lane.

Stored CI snapshot:

  • Green across the recorded checks.
  • Not current for the latest PR head.
  • Stored green workflow/check data points to older head 29c6475.
  • Current local/pushed branch head is 847135c.

Workflow approval is not the blocker here. The blocker is that the available CI snapshot does not confirm the latest head.

Recommendation

Escalate to a human for the final merge decision.

Recommended next action:

  • Refresh or confirm CI for head 847135c.
  • If CI is green, decide whether to merge now or request a follow-up for the remaining P2s.

@osolmaz
Copy link
Contributor

osolmaz commented Mar 26, 2026

Triage result

Human attention: ⚠️ Required
Recommendation: 🏁 escalate to a human
Human decision needed: resolve the ambiguous merge conflict in src/cli-core.ts against origin/main and decide whether the PR should continue.

Quick read

  • Solution shape: good_enough
  • PR intent: generic ACP-native model selection via session/set_model
  • Current blocker: initial conflict check is ambiguous
  • Conflicted file: src/cli-core.ts
  • Conflict summary: auto-merge failed against origin/main; human judgment is needed before the flow can continue

Intent

This PR is trying to make acpx support choosing and changing an agent model in a generic ACP-native way across agents, including at session creation and during an existing session, while remembering and showing the selected model.

Why

The underlying problem is that acpx currently handles model selection through agent-specific metadata or generic config-option paths instead of ACP session/set_model. That makes model changes unreliable or unsupported for agents like Droid, Gemini CLI, and Codex, and it leaves model state inconsistently persisted, replayed, and surfaced in status.

Codex review

  • Read-only triage judged the solution good_enough.
  • The change appears to address the real protocol-layer gap rather than patching one adapter.
  • Evidence: src/client.ts applies session/set_model after session/new when models are advertised and routes set model through session/set_model.
  • Evidence: src/session-runtime.ts and src/session-runtime/connect-load.ts persist model state, enrich status, and replay saved model selection on fresh ACP sessions.
  • Evidence: test/integration.test.ts covers create-time selection, mid-session changes, reconnect replay, status output, and graceful handling when model support is absent or fails.
  • No validation, CI, or full review was run in this handoff step.

CI/CD

  • CI status: not available in this run state.
  • Validation path: not reached.
  • Final conflict gate: not reached because the initial conflict check already stopped the flow.
  • Initial conflict check: ambiguous
  • Merge attempt result: conflict in src/cli-core.ts

Recommendation

Escalate to a human. The implementation looks right-shaped, but the flow is blocked by an ambiguous merge conflict in src/cli-core.ts, so a human should resolve that conflict and decide whether the PR should proceed.

@osolmaz
Copy link
Contributor

osolmaz commented Mar 26, 2026

Triage result

Human attention: ⚠️ Required
Recommendation: 🏁 escalate to a human
Human decision needed: decide whether to rerun and complete the targeted feature validation before landing, or explicitly accept proceeding without that validation.

Quick read

  • The PR intent is to add generic ACP-native model selection via session/set_model.
  • The solution was judged good_enough and the change is feature-shaped rather than a bug-first repro path.
  • The current blocker is validation: targeted feature validation did not complete cleanly on the PR branch.
  • Planned targeted checks were pnpm run build:test and node --test dist-test/test/cli.test.js dist-test/test/integration.test.js dist-test/test/session-mode-preference.test.js.
  • This run does not include Codex review results or CI/CD results.

Intent

Make acpx handle model selection in a generic ACP-native way across different agents, both at session creation and mid-session, while preserving and showing the chosen model.

Why

acpx was relying on agent-specific config/meta paths for model selection instead of ACP session/set_model, so --model and set model were not working uniformly on agents like Droid, Gemini CLI, and Codex, and model state was not consistently tracked or replayed.

Codex review

  • No Codex review result is attached in this run.
  • The solution shape was previously judged good_enough: the shared ACP client/runtime is updated rather than patching a single harness, and the PR includes integration coverage for create-time set, mid-session set, replay, rejection handling, and status enrichment.

CI/CD

  • Validation status in this run: feature_not_validated.
  • Targeted feature validation did not complete cleanly on the PR branch.
  • CI/CD status is not attached in this run.

Recommendation

Escalate to a human to decide whether to require completion of the targeted feature validation before landing, or to make an explicit risk call and proceed without it.

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.

feat: generic model selection via ACP session/set_model

2 participants