refactor(acp): source ACP model lists from typescript-client (closes #740)#775
Conversation
Replace the hand-mined CLAUDE_MODELS / CODEX_MODELS / GEMINI_MODELS lists (and the duplicated provider metadata) with the @openhands/typescript-client ACP registry, which mirrors the Python SDK source of truth (openhands.sdk.settings.acp_providers). acp-providers.ts becomes a thin adapter: it enriches each upstream record with Canvas-only UI fields (brand icon + onboarding description) and keeps the helper functions + public export surface unchanged, so no consumers change. - Bump the @openhands/typescript-client pin to the #187 merge commit (082d4d46), which adds available_models / default_model to the registry. - Delete the three hardcoded model lists; build ACP_PROVIDERS from getAcpProvider() + a small ACP_PROVIDER_UI map. - Incidentally corrects the Gemini default to auto-gemini-2.5 (the CLI's auto-router default), matching the merged SDK/client. Closes #740. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid architectural improvement but needs workflow cleanup
[IMPROVEMENT OPPORTUNITIES]
The refactor correctly eliminates hardcoded model lists in favor of the typescript-client registry (single source of truth). However, this change breaks the existing drift-detection infrastructure:
🟠 Important: .github/workflows/acp-providers-sync.yml and scripts/check-acp-providers-sync.mjs are now obsolete. The workflow expects to parse hardcoded CLAUDE_MODELS/CODEX_MODELS/GEMINI_MODELS arrays from the TypeScript file, but the refactor replaces that structure with Object.entries(ACP_PROVIDER_UI).map(([key, ui]) => { const info = getClientAcpProvider(key); ... }). The script will fail or produce incorrect results when triggered by this PR's changes to src/constants/acp-providers.ts.
Recommendation: Either remove the workflow and sync script (since Canvas now just uses typescript-client as the source of truth), or update the script to verify the typescript-client dependency pin is up-to-date. The former is simpler — typescript-client has its own drift check against the Python SDK.
[RISK ASSESSMENT]
Architectural improvement with good test coverage. The refactor moves maintenance burden upstream where it belongs. Side effect (Gemini default correction) is intentional and documented. Dependency is forward-only from v1.23.0.
VERDICT:
✅ Worth merging after addressing the stale workflow/script in a follow-up
KEY INSIGHT:
This refactor makes Canvas a consumer of the ACP registry rather than a mirror of it, which is the right architectural direction. The workflow cleanup is straightforward housekeeping.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26461411572
Now that typescript-client v1.23.2 is tagged/released (includes #187's ACP registry, mirroring SDK #3389), pin to the tag instead of the raw #187 merge SHA. v1.23.2 tracks the SDK's v1.23.2 patch line. Resolves to the same commit as the prior SHA, so no resolved-content change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The acp-providers-sync workflow + scripts/check-acp-providers-sync.mjs existed to keep Canvas's hand-kept ACP registry mirror in sync with the SDK source (agent-canvas#587). That mirror is gone — acp-providers.ts now sources its model data from @openhands/typescript-client, which carries its own SDK-drift check (check-acp-drift.py). So this canvas-side check is redundant and was failing on the refactored ACP_PROVIDERS (no longer a literal array). - Delete .github/workflows/acp-providers-sync.yml + the script. - Drop the docs-version-sync test case that asserted the script's example. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
2 participants
Summary
Stops hand-maintaining ACP model lists in Canvas. The three hardcoded arrays —
CLAUDE_MODELS/CODEX_MODELS/GEMINI_MODELS(the Claude one originally mined by string-scanning theclaudebinary) — are replaced by the@openhands/typescript-clientACP registry, which mirrors the Python source of truth (openhands.sdk.settings.acp_providers).Closes #740. Part of #769 (unify ACP model selection).
What changed
@openhands/typescript-client→v1.23.2(tagged release including feat(acp): mirror available_models / default_model from openhands-sdk typescript-client#187, which addedavailable_models/default_modelto the client's ACP registry).v1.23.2tracks the SDK's v1.23.2 patch line that contains feat(acp): add available_models / default_model to ACP provider registry software-agent-sdk#3389. Forward-only from the previousv1.23.0pin.acp-providers.tsis now a thin adapter:ACP_PROVIDERSis built by enriching each upstream registry record (viagetAcpProvider()) with a small Canvas-onlyACP_PROVIDER_UImap carrying the two fields the client registry doesn't have — the brandiconand the onboardingdescription_key.ACP_PROVIDERS,getAcpProvider,labelForAcpModel,buildAcpAgentSettingsDiff,ACPProviderConfig,ACPModelOption, …) is preserved, so the ~50 consumer call sites are untouched. Netacp-providers.ts: −132 / +54.Side effect (intended)
The Gemini default is now
auto-gemini-2.5(the CLI's auto-router default), corrected from the stalegemini-2.5-propin — it now comes from the client/SDK, matching the upstream fix.Validation
The #730 registry test (
__tests__/constants/acp-providers.test.ts) passes unchanged — it asserts behavior (display names,default_model ∈ available_models, diff seeding), which the client supplies identically.🤖 Generated with Claude Code
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.23.0-pythonopenhands-automation==1.0.0a5be787fd916a9121af69a2d2e1b083c27defb3f5cPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-be787fdRun
All tags pushed for this build
About Multi-Architecture Support
sha-be787fd) is a multi-arch manifest supporting both amd64 and arm64sha-be787fd-amd64) are also available if needed