feat(acp): save concrete model defaults in Canvas#730
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solves a real UX problem with clean implementation, but requires verification of model defaults
[EVAL-RISK CONCERN]
This PR changes what model is saved for new ACP conversations:
- Before:
acp_model: null→ falls back to provider's default - After:
acp_model: "claude-sonnet-4-6"(or other Canvas default)
Question: Do the Canvas defaults match the actual provider defaults?
The hardcoded defaults are:
- Claude Code:
claude-sonnet-4-6 - Codex:
gpt-5.5/medium - Gemini CLI:
gemini-2.5-pro
If these don't match what the providers would use when acp_model is null, this PR changes agent behavior and could affect benchmarks. The test name suggests they should match ("seeds built-in ACP diffs with the provider default model"), but the test only verifies that buildAcpAgentSettingsDiff uses provider.default_model - it doesn't verify that value actually matches the real provider's default.
Recommendation: Before merging, verify that these Canvas defaults match the actual provider defaults (what model would run if acp_model were null). If they don't match, this needs eval validation.
[IMPROVEMENT OPPORTUNITIES]
The implementation is solid, but a few suggestions for future work:
-
Model list maintenance: The hardcoded model lists in
acp-providers.tswill need manual updates when providers change their offerings. Consider documenting this maintenance requirement or adding a script to detect drift. -
Evidence for UI changes: This PR modifies three user-facing surfaces (agent settings dropdown, onboarding, chat input display) but includes no screenshots or videos. While the test coverage is good, visual verification would help confirm the UI renders correctly.
-
Agent settings complexity: The
agent-settings.tsxfile grew significantly with the dropdown/custom model logic. The state management (switching between dropdown and text input, syncing with provider selection) is clear but could be extracted into a separate component in the future.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Changes user-facing model display and what model is saved for new ACP conversations. Well-tested frontend implementation with comprehensive coverage. Main risk is the uncertainty about whether Canvas defaults match provider defaults, which could affect agent behavior. No security, dependency, or breaking change concerns. Recommend verifying model defaults before merge.
VERDICT:
❓ Needs verification: Confirm Canvas model defaults match provider defaults to ensure no unintended behavior change
KEY INSIGHT:
The fallback chain (runtime fields → configured model → sentinel check) is elegant and future-proof, but the initial seed values need verification to avoid silent behavior changes.
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/26296373486
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ❌ 1 snapshot differ from the main branch baseline. Add the
✅ Unchanged snapshots (72)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |



Summary
acp_modelvalues instead of leaving the model blankacp_modelwhile keeping ACP conversations out of the OpenHands LLM profile switcherValidation
npm cinpm run typechecknpm test -- __tests__/constants/acp-providers.test.ts __tests__/routes/agent-settings.test.tsx __tests__/components/onboarding/choose-agent-step.test.tsx __tests__/api/agent-server-adapter.test.ts __tests__/components/features/chat/components/chat-input-model.test.tsx __tests__/components/features/chat/components/chat-input-actions.test.tsx __tests__/components/features/chat/switch-profile-button.test.tsxnpm run buildgit diff --check && git diff --cached --checkNotes
This avoids the common Canvas-created ACP path showing
default (recommended)by never saving an empty model for built-in providers. SDK runtime model fields are still preferred when present, then Canvas falls back to the configuredacp_model.🐳 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.0a39d1b7c1700f5172ed8f57bbec6e712be4f2f4a4dPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-9d1b7c1Run
All tags pushed for this build
About Multi-Architecture Support
sha-9d1b7c1) is a multi-arch manifest supporting both amd64 and arm64sha-9d1b7c1-amd64) are also available if needed