feat(chat): runtime-compatibility AgentProfile picker#928
feat(chat): runtime-compatibility AgentProfile picker#928simonrosenberg wants to merge 5 commits into
Conversation
Reframe the in-conversation profile picker around AgentProfiles and their runtime compatibility with the running conversation (agent-canvas#669): incompatible profiles are shown but disabled with a reason, and a profile is only ever switched live when the whole profile applies — never a silent partial application. - src/utils/agent-profiles/runtime-plan.ts: pure `deriveProfileRuntimePlan` implementing the issue's compatibility matrix (current | switch-live | disabled+reason) over a normalized AgentProfile / ConversationRuntimeContext, plus `normalizeLlmProfile`. Fully unit-tested incl. ACP/non-runtime reasons. - src/utils/agent-profiles/reason-labels.ts: reason → i18n key (exhaustive). - src/hooks/use-profile-runtime-plans.ts: pairs each saved profile with a plan for the current conversation/home context (cloud-gated; ACP-aware). - switch-profile picker: renders plan-driven rows (current checked, switch-live actionable, disabled greyed + reason); switching is blocked unless the plan is switch-live. - ACP/cloud model menu: shows incompatible saved profiles disabled with "Requires a new conversation" instead of hiding them. - i18n: PROFILE_PICKER reason strings across all supported languages. Backend persistence for ACP profiles lands separately in software-agent-sdk#3433; until the typescript-client exposes the new profile fields, ACP profiles still surface here as disabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ofiles Pin @openhands/typescript-client to the typescript-client#195 commit (git SHA) so CI builds against the client that exposes the new profile fields, and wire the picker to actually use them: - normalizeLlmProfile maps `kind: "acp"` ProfileInfo → an ACP AgentProfile (provider + model from the list summary; command/args/env derive from the provider for built-ins, so the runtime-plan reduces to provider+model). - useProfileRuntimePlans: kind-aware `isActive` (an OpenHands profile can no longer be mislabeled "current" in an ACP conversation, and an ACP profile matching the running provider+model now reads as current); the ACP context is always switchable (useSwitchAcpModel handles both the home-page default and the in-conversation live swap), so home-page ACP profiles aren't wrongly greyed "session not initialized". - The ACP/cloud model menu now renders ALL profiles plan-driven: a same-provider model-only-diff ACP profile is switch-live (clickable → live acp_model swap), current is checked, incompatible ones stay disabled with the precise reason (different provider vs requires-new-conversation). Validated end-to-end against a local agent-server built from software-agent-sdk#3433: current / switch-live / "Different agent provider" / "Requires a new conversation" all render from real profile data. Co-Authored-By: Claude Opus 4.8 (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. Warning One or more snapshot tests crashed during generation — some snapshots below may be incomplete. ❌ 7 snapshots differ from the main branch baselines. Add the
🔴 Changed snapshots (7)
|
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
mcp-page — 5 snapshots
mcp-custom-server-1-editor-open
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
mcp-custom-server-editor
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
mcp-empty-installed
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
mcp-search-filtered
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
mcp-slack-install-1-marketplace
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
settings-page
settings-page
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
![]() |
![]() |
![]() |
✅ Unchanged snapshots (66)
archived-conversation
- conversation-panel-with-archived-badges
- conversation-view-archived
- conversation-view-sandbox-error
automations
- automations-delete-modal
- automations-list-active-inactive
- automations-no-automations
- automations-search-no-results
backends-extended
- backend-add-blank-disabled
- backend-add-cloud-advanced-open
- backend-add-cloud-no-key-disabled
- backend-add-cloud-with-key-enabled
- backend-add-form-partially-filled
- backend-add-invalid-url-disabled
- backend-add-local-ready
- backend-add-name-only-disabled
- backend-add-two-column-layout
- backend-add-whitespace-host-disabled
- backend-cancel-nothing-saved
- backend-dropdown-two-backends
- backend-edit-prefilled
- backend-manage-after-removal
- backend-manage-two-listed
- backend-remove-cancelled
- backend-remove-confirmation
- backend-switch-overlay
backends
- backend-add-modal
- backend-manage-modal
- backend-selector-open
changes-tab
- changes-deleted-file
- changes-diff-viewer
- changes-empty
collapsible-thinking
- reasoning-content-collapsed
- reasoning-content-expanded
- think-action-collapsed
- think-action-expanded
mcp-page
- mcp-custom-server-2-url-filled
- mcp-custom-server-3-all-filled
- mcp-custom-server-4-installed
- mcp-slack-install-2-modal
- mcp-slack-install-3-filled
- mcp-slack-install-4-installed
onboarding
- onboarding-step-0-choose-agent
- onboarding-step-1-check-backend
- onboarding-step-2-setup-llm
- onboarding-step-3-say-hello
projects-workspace-browser
- projects-workspace-browser
settings-page
- add-backend-modal
- analytics-consent-modal
- home-screen
- settings-app-page
settings-secrets
- secrets-add-form-filled
- secrets-add-form
- secrets-after-save
- secrets-delete-confirm
- secrets-list
settings-verification
- condenser-settings
- verification-settings-off
- verification-settings-on
sidebar
- sidebar-collapsed
- sidebar-conversation-panel
- sidebar-filter-menu
skills-page
- skills-empty
- skills-loaded
- skills-no-match
- skills-search-filtered
- skills-type-filter
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers.
…me compat On /conversations (the new-conversation / home surface) there is no running agent to be incompatible with, yet the picker applied the in-conversation live-switch compatibility matrix — so ACP profiles showed disabled with "Requires a new conversation" and couldn't be chosen to launch a conversation. - useProfileRuntimePlans: expose `inConversation`; when it's false, every profile is selectable (active one is `current`) and the grey-out matrix is skipped. Compat gating now applies only inside an existing conversation. - chat-input-model picker: selecting a profile on the home surface (no switchConversationId) activates the whole profile kind-aware (/activate) so picking an OpenHands profile flips the default away from ACP — rather than only swapping acp_model. In-conversation ACP selection still does the live session/set_model swap. Tests: home selection activates (not model-swap); existing in-conversation switch-live / disabled behavior unchanged. Allowlist the temporary typescript-client git pin in the package-library guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Settings → Agent is now the single surface for building an AgentProfile, and the standalone LLM page is folded into it. - The Agent page lists profiles and creates/edits them with a kind toggle: OpenHands shows the LLM config form; ACP shows a new AcpProfileForm (provider / command / model / env). Save persists the profile kind-aware via /api/profiles (llm for OpenHands, agent_settings for ACP) and activates it — "build a profile and save" is all that's needed. - New AcpProfileForm: controlled provider/command/model/env editor (ports the ACP fields from the old agent page; env is one KEY=value per line, stored encrypted at rest). - /settings/llm now redirects to /settings/agent and the standalone LLM nav item is removed (the named LlmSettingsScreen export stays for the OpenHands form / onboarding / cloud). Condenser and Verification remain separate, as the backend doesn't persist them inside a profile yet. - Adds SETTINGS$AGENT_ENV / _HINT strings (all locales); updates settings nav, route and editor tests for the merged IA. Validated end-to-end against a local agent-server built from software-agent-sdk#3433: create an ACP profile in the UI → it persists as agent_kind=acp and activates; activating an OpenHands profile flips the kind back. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ 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
There was a problem hiding this comment.
🟡 Acceptable — Sound design and clean separation of concerns. The pure-function runtime-plan.ts with exhaustive unit tests is the right approach. A few things need attention before merge.
1. @openhands/typescript-client: downgrade + git SHA dependency
package.json swaps from the published 1.24.3 (npm registry) to a git commit SHA that resolves to 1.24.2 — a version downgrade. Per the PR description this is intentional while kind/acp_* fields are in-flight, but a few operational risks remain:
- Blocks
npm ciin environments that can't reachgithub.comviagit://(restricted CI runners, air-gapped builds, Docker layer caches). npm audithas zero visibility into the git-pinned package; supply-chain checks are effectively bypassed for this dep.- The resolved version (
1.24.2) is older than what was previously there (1.24.3) — any fixes in1.24.3are lost.
Recommendation: keep the merge window tightly coupled to the publish of the new @openhands/typescript-client version. Track the registry bump as a follow-up to prevent this from lingering.
2. Profile always activates on save (behavior change)
ProfilesService.activateProfile() is now unconditional after every save. Previously it was only called when renaming the previously-active profile. A user editing a secondary/archive profile (e.g., to rotate its API key) will silently find it activated as their default. If "save = activate" is the intended UX, the save button copy ("Save & Activate") would set expectations clearly.
3. Profile row duplication
The CircuitIcon + name + checkmark + reason-label pattern is copy-pasted identically between switch-profile-context-menu.tsx and chat-input-model.tsx. A small shared <ProfileRow plan={...} profile={...} /> component would remove ~40 lines of duplication and keep future styling changes consistent across both surfaces.
🟢 Strengths
deriveProfileRuntimePlanis pure and exhaustively tested — right design. EveryRuntimeIncompatibilityReasonhas a test case, so adding a new reason without a label will immediately fail CI (via the exhaustiveness guard inreasonToI18nKey).isActiveguard prevents cross-kind mislabelling — thenormalized.kind === context.kindcheck before the name comparison is correct and the comment explains it well.- No silent partial application — disabling incompatible profiles at both the button and menu level is the right defence-in-depth.
normalizeLlmProfilecomment — the explanation of whyacpCommand/acpArgs/acpEnvare omitted from the ACP list summary is clear and prevents future confusion.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM — The git-SHA/downgraded dependency is the primary operational risk (CI portability, audit gap). Activate-on-save is a low-risk behavior change. Core compatibility logic and test coverage are solid.
VERDICT:
✅ Worth merging — with the git SHA dependency either resolved to a published registry version or explicitly tracked as a temporary state with a linked follow-up issue.
KEY INSIGHT:
The isActive-over-config-equality design is correct and relies on activeProfileName being reliably propagated; on cold loads where it's null, config-equality in deriveProfileRuntimePlan provides the correct fallback to current for matching profiles.
Improve this review? Add a
.agents/skills/custom-codereview-guide.mdto your branch with the/codereviewtrigger and any context the reviewer is missing. See customization docs.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
| "@monaco-editor/react": "4.7.0", | ||
| "@openhands/extensions": "git+https://github.com/OpenHands/extensions.git#39711065f53166c52608462f60a4c8507253ce56", | ||
| "@openhands/typescript-client": "1.24.3", | ||
| "@openhands/typescript-client": "git+https://github.com/OpenHands/typescript-client.git#c86ce065e9d4016a49f5a54a0856de35656cece3", |
There was a problem hiding this comment.
Dependency Downgrade + Git SHA: switching from 1.24.3 (npm registry) to a git commit SHA that resolves to 1.24.2 is a downgrade. npm ci will fail in environments without git access to github.com, and npm audit can no longer inspect this package. Per the PR description this is intentional while kind/acp_* fields are in-flight — worth tracking a follow-up to bump back to a published release so this doesn't linger.
| "version": "1.24.3", | ||
| "resolved": "https://registry.npmjs.org/@openhands/typescript-client/-/typescript-client-1.24.3.tgz", | ||
| "integrity": "sha512-4w5rGbgXSnRKm6DZGedGRMWa4nIyl8/1+lEzinBIjZbdE0MD6+89EAItkk6FTvKU0jdVpoCAtmLWBP3Y/r8ORA==", | ||
| "version": "1.24.2", |
There was a problem hiding this comment.
Confirms the downgrade: the resolved version is 1.24.2 (via git+ssh://) whereas the previous lock had 1.24.3 from the npm registry. Intentional per PR description; please track the version bump as a follow-up.
| } | ||
| await saveProfile.mutateAsync({ name: trimmedName, request }); | ||
| // "Save and that's it": the saved profile becomes the active one. | ||
| await ProfilesService.activateProfile(trimmedName); |
There was a problem hiding this comment.
Behavior change: activateProfile is now unconditional — every save switches the active profile. Previously it was only called when renaming the previously-active profile. A user editing a secondary profile (e.g., to rotate its API key) will find it silently activated. If "save = activate" is deliberate UX, consider clarifying the save button label or adding a note in the UI.
| // spurious disable. | ||
| llm: { | ||
| model: conversationModel ?? settings?.llm_model ?? null, | ||
| baseUrl: null, |
There was a problem hiding this comment.
The comment explains this correctly: since AppConversation doesn't expose base_url, two OpenHands profiles differing only in base_url will both evaluate against null here and thus both get switch-live (neither is blocked for a base-URL mismatch). The isActive guard by name prevents the current profile from being mislabelled, but a non-active profile with a different base URL will be offered as switchable. Fine if base-URL mismatches are intentionally permitted as live switches — just documenting the implication.





















What
Reframes the in-conversation profile picker around AgentProfiles and their runtime compatibility with the running conversation — the frontend half of #669. Incompatible profiles are visible but disabled with a reason, and a profile is only switched live when the whole profile applies (no silent partial application where only the model changes but condenser/tools/verification/provider/command are ignored).
Changes
src/utils/agent-profiles/runtime-plan.ts— purederiveProfileRuntimePlan()implementing the issue's compatibility matrix (current|switch-live|disabled+ the fullRuntimeIncompatibilityReasonset) over a normalizedAgentProfile/ConversationRuntimeContext, plusnormalizeLlmProfile(). No React/network → exhaustively unit-tested (incl. ACP and non-runtime-settings reasons).reason-labels.ts— reason → i18n key (exhaustiveness-guarded).use-profile-runtime-plans.ts— pairs each saved profile with a plan for the current conversation/home context (cloud-gated so it doesn't fetch profiles where they don't exist; ACP-aware).switch-profile-button/switch-profile-context-menu) — plan-driven rows: current is checked, switch-live is actionable, disabled is greyed with its reason; switching is blocked unless the plan isswitch-live.chat-input-model) — surfaces incompatible saved profiles disabled with "Requires a new conversation" instead of hiding them.PROFILE_PICKERreason strings across all supported languages.Scope / follow-ups
This is the runtime-compatibility slice. Backend persistence for ACP profiles lands separately in software-agent-sdk#3433. Until
@openhands/typescript-clientexposes the newkind/acp_*profile fields (and a create-ACP-profile UI exists), ACP profiles still surface here as disabled — consistent and safe, just not yet switchable.Validated locally against a real agent-server (incl. the local SDK#3433 build): OpenHands→OpenHands live switches work; in an ACP conversation saved profiles render disabled with the reason.
🤖 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.24.0-pythonopenhands-automation==1.0.0a50103fa455e273b7f31580aae01a004743a8879b4Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-0103fa4Run
All tags pushed for this build
About Multi-Architecture Support
sha-0103fa4) is a multi-arch manifest supporting both amd64 and arm64sha-0103fa4-amd64) are also available if needed