mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-07 16:38:34 +08:00
feat(acp): Settings → Agent + onboarding + chat-UI gating for ACP-driven conversations (#416)
* feat(acp): add minimal ACP agent UI (parity with OpenHands#14401) Adds a Settings → Agent page so users can switch the conversation between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess (Claude Code, Codex, Gemini CLI, or custom command) without hand-editing settings. Discriminates in agent-server-adapter: when `agent_settings.agent_kind === "acp"`, build an `ACPAgent` payload (kind, acp_command, acp_model) instead of the LLM-shaped Agent, and skip the LLM defaults that would otherwise be rejected as extras. Stamps the provider key onto `tags.acpserver` so the chip can resolve a brand name from a single source. Tag-key constant note: the conventional `acp_server` form is invalid — agent-server validates tag keys against `^[a-z0-9]+$` and returns 422. The flattened `acpserver` form survives validation; the named constant `ACP_SERVER_TAG_KEY` keeps the regex and the key colocated. Gates the LLM and Condenser nav items behind a `disabledByAcp` flag, greys them out with a tooltip, and redirects to `/settings/agent` in the settings loader (not a per-route useEffect, so there is no one- frame flash of the LLM page before bouncing). E2E validated against `ghcr.io/openhands/agent-server:fa29ae2-python`: - PATCH /api/settings with `agent_kind: "acp"` round-trips - POST /api/conversations with the adapter's ACP payload returns 201, `agent.kind=ACPAgent`, `acp_command` preserved, tags stamped. Closes #412 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(acp): wire onboarding ChooseAgent step into ACP settings Drops the "Support for other agents coming soon!" banner now that the support exists. Enables the Claude Code / Codex tiles and adds a Gemini CLI tile so the four options here match ``ACP_PROVIDERS`` from the Settings → Agent page. Selecting an ACP option and clicking Next persists ``agent_kind:"acp"`` plus the registry provider key (``acp_server``) via ``useSaveSettings``, mirroring the diff the Settings page emits. The advance only happens on save success — a failed PATCH stays on the step and surfaces a toast. Skips the embedded LLM-setup step (index 2) on both forward and back navigation when an ACP agent is active: the subprocess owns its own LLM and authenticates through Secrets, so the form has nothing to configure. OpenHands path is untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(acp): seamless Claude Code + Codex CLI auth via dev-docker Live-validated against `ghcr.io/openhands/agent-server:1.22.1-python` (the canvas's default pin, which now ships ACPAgent natively — no SHA override needed). Two changes surfaced by the run: 1. **Mount `~/.claude.json` in dev:docker.** Recent Claude Code CLI versions persist auth + workspace state in `~/.claude.json` next to (not inside) `~/.claude/`. Without this single-file mount, `@agentclientprotocol/claude-agent-acp` can't see the user's existing login and prompts to re-auth inside the sandbox. 2. **Use the new ACP package name in `ACP_PROVIDERS`.** Upstream renamed `@zed-industries/claude-code-acp` → `@agentclientprotocol/ claude-agent-acp`. The old name still works but emits an npm deprecation warning; the agent-server's own OpenAPI example uses the new name. Test fixtures pinning the legacy name updated to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(acp): import ACP_PROVIDERS from typescript-client Canvas was carrying its own copy of the ACP provider registry, which drifted out of sync with the canonical Python SDK source and ended up encoding an invalid Codex invocation (``@openai/codex acp`` — codex CLI has no ``acp`` subcommand, so the spawn deadlocked silently with ``Error: stdin is not a terminal`` and no log line). This change deletes ``src/constants/acp-providers.ts`` and imports the registry from ``@openhands/typescript-client`` instead, which now mirrors the Python SDK (see OpenHands/typescript-client#167). The TS SDK pin in ``package.json`` is bumped to the PR-branch SHA (``45a803c``) for now; once #167 merges and a new tagged release is cut, the pin can flip to the tag in a follow-up commit. Shape changes consumers needed to absorb: - ``ACPProviderConfig[]`` → ``Record<string, ACPProviderInfo>`` (lookup by key replaces ``.find``; ``Object.values`` where an array is needed) - ``display_name`` → ``displayName`` (camelCase matches TS conventions) - ``default_command`` → ``defaultCommand`` (and now ``readonly string[]``; components spread into a fresh array before passing to consumers that expect mutability) ``ACP_CUSTOM_PRESET_KEY`` is the only ACP-related constant that stays canvas-local — it's a synthetic sentinel for the "Custom" dropdown option, not a real provider, so it has no SDK counterpart. Moved to ``src/constants/acp-presets.ts``. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * revert(acp): keep ACP_PROVIDERS local to canvas Reverts the brief detour through `@openhands/typescript-client` for the ACP provider registry. Splitting the registry across two repos adds publish-coordination friction and doesn't actually eliminate the drift problem — it just moves it from "canvas vs. python-sdk" to "ts-sdk vs. python-sdk", with extra steps. Now: - `src/constants/acp-providers.ts` is the canvas-local copy again, with the corrected `codex` command (`@zed-industries/codex-acp`, the real ACP-protocol stdio server — not `@openai/codex acp`, which is the codex CLI's interactive mode and deadlocks the agent handshake when spawned without a TTY). - The package.json pin reverts to `v0.6.0` (the typescript-client release that does not include the unmerged `ACP_PROVIDERS` export from #167, which is now closed). - The split `acp-presets.ts` file is folded back in. Drift risk between this file and the Python SDK source is tracked in #587, with a longer-term plan to address it (TS-SDK mirror, code-gen, or runtime endpoint). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): resolve empty acp_command from registry in adapter PR #416 ships the Settings → Agent page (and onboarding) with a "default preset" shortcut that stores ``acp_command: []`` and trusts the agent-server to resolve it from ``acp_server``. It doesn't. The agent-server's ``ACPAgent`` model has no ``acp_server`` field and no registry resolution — it just hands ``acp_command`` straight to a subprocess spawn. Empty list trips ``acp_agent.py:1013`` with ``IndexError: list index out of range``, the agent loop dies silently inside the agent-server's run thread, and the conversation hangs in ``idle`` with the user's message persisted but never answered. No error reaches the UI; from the user's perspective they sent a message and nothing happened. Caught while exercising the live ``dev:safe`` stack: a fresh conversation seeded from the onboarding "Claude Code" tile produced ``Failed to start ACP server: list / IndexError: list index out of range`` in the agent-server log. The fix is purely client-side — expand ``acp_command`` against ``ACP_PROVIDERS`` (canvas's local mirror of the Python SDK registry, see #587) before the payload leaves the adapter, when the user picked a built-in preset. ``acp_server: "custom"`` and any unknown key are left untouched — those genuinely depend on the user's explicit command, and silently inventing one would mask a real config bug. Three new adapter tests cover: - ``acp_command: []`` + ``acp_server: "claude-code"`` → command resolved to ``["npx","-y","@agentclientprotocol/claude-agent-acp"]`` - ``acp_command`` omitted entirely + ``acp_server: "codex"`` → same resolution path - ``acp_command: []`` + ``acp_server: "custom"`` → left untouched 2273 tests pass, lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): pass disabled state into SettingsDesktopSidebar When ACP is the active agent, ``useSettingsNavItems`` correctly tags the LLM and Condenser entries with ``disabled: true``. The mobile drawer (rendered via ``SettingsNavLink``) already respected that. The desktop sidebar (rendered via ``SidebarNavLink``, came in with the recent sidebar refactor) was constructing the link without forwarding the flag, so both items stayed fully clickable / styled as enabled while the conversation was running on an ACP subprocess. Two tiny changes: 1. ``SettingsDesktopSidebar`` passes ``renderedItem.disabled`` through to ``SidebarNavLink``. That alone gives the right visual state (``opacity-50``, ``pointer-events-none``) and keyboard behaviour (``tabIndex=-1`` + ``onClick preventDefault``) — both already implemented by ``SidebarNavLink``. 2. ``SidebarNavLink`` additionally sets ``aria-disabled="true"`` when ``disabled``, closing a screen-reader gap that existed independently of this regression (the link sounded actionable to assistive tech even though it wasn't). The ``clientLoader`` redirect in ``routes/settings.tsx`` continues to handle direct URL navigation to a disabled-by-ACP page, so even if someone bookmarks ``/settings/condenser`` and lands there while ACP is active, they get bounced to ``/settings/agent``. Two new tests in ``settings-navigation.test.tsx``: - Disabled-by-ACP items in the desktop sidebar carry ``aria-disabled``. - Enabled items don't. 2275 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): address PR #416 review feedback Addresses both human and all-hands-bot review comments on #416: **Critical bugs fixed** - ``acp_args`` duplication on load (bot critical #1): the textarea is the single source of truth for the launch tokens, but save only wrote ``acp_command`` — any API-set ``acp_args`` survived and concatenated at spawn time. Save now always writes ``acp_args: []``. - ``tokenizeCommand`` corrupted quoted Custom commands (human bug #2): ``bash -c "echo hello"`` got split into ``["bash","-c","\"echo","hello\""]`` and silently misbehaved. New ``src/utils/acp-command.ts`` wraps ``shell-quote`` with selective re-quoting (so ``npx -y @org/pkg`` renders verbatim, not ``\@org/pkg``) and filters non-string entries (redirects, env-var refs) out of the parsed argv. Round-trip tests pin the contract. - Loader/component settings cache mismatch (bot critical #4): loader used ``SETTINGS_QUERY_KEYS.byScope("personal")``; ``useSettings`` used ``[...byScope("personal"), backend.id, orgId]``. They didn't share cache. Aligned + set ``staleTime: 0`` on the loader read so cross-tab kind flips are picked up immediately (the in-render hook keeps its 5-minute stale window). - ``getFirstAvailablePath`` ignored the new agent route (human bug #3): ``/settings/agent`` now precedes the others in the fallback list, so first-time / hide_llm_settings users land on the agent picker rather than ``/settings/app``. - ``ACP_SETTINGS_KEYS`` documentation (human #4): pre-empts the "why not trim this list to UI-visible fields" question by spelling out that it serves as both the ACP allow-list and the OpenHands deny-list — trimming would silently leak API-set ``acp_*`` state. **Refactor (human #2 + #3)** - ``description_key`` moves into ``ACP_PROVIDERS``; the onboarding ``AGENT_OPTIONS`` is now derived from the registry so adding a new provider only needs one edit. - One ``buildAcpAgentSettingsDiff`` helper replaces the two near-copies in ``choose-agent-step.tsx`` and ``agent-settings.tsx``; both call sites are now under a single contract for the agent_settings_diff shape. **UX (bot)** - Onboarding progress bar shows the actual visited-step count when the LLM step is skipped (3 segments for ACP, 4 for OpenHands). Previously segment 2 popped "completed" on a slide the user never visited. **Test coverage (bot)** - New ``__tests__/utils/acp-command.test.ts`` covers parseCommand / formatCommand round-trips, quoted args, embedded escapes, shell- operator filtering, package-style tokens. - Adapter: empty ``acp_model: ""``, unknown ``acp_server`` key, ACP→OH→ACP round trip (no field leakage either direction). - agent-settings: cleared input keeps Save disabled, whitespace-only same, full Custom command with quoted args round-trips through shell-quote. - choose-agent-step: provider switching (claude-code → codex) rebuilds the diff cleanly, no leak from the prior selection. **Acknowledged (no action)** - Bot critical #2 (supply chain drift) — same problem as the existing agent-canvas#587, already tracked. - Bot critical #3 (desktop sidebar disabled) — fixed in 27a3e79 a few commits before this review was written; review snapshot was stale. - Translation duplication (human #1) — matches the existing ``translation.json`` convention (every key has all 15 locales). - Option-bag → split functions (human #5) — cosmetic; defer. 2292 tests pass, lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): pre-bundle shell-quote so the Vite dev server can load it ``shell-quote`` is a CommonJS module that does ``module.exports = { parse, quote }``. The previous commit wired it into ``src/utils/ acp-command.ts`` with a named ESM import, which the dev server rejected on the first ``agent-settings.tsx`` load: SyntaxError: The requested module '/node_modules/shell-quote/ index.js?v=...' does not provide an export named 'parse' Switching to a namespace import (``import * as shellQuote from "shell-quote"; const { parse, quote } = shellQuote;``) makes the named-export check pass, but Vite then served the raw CJS file to the browser unchanged and the next request died with: ReferenceError: exports is not defined This second failure is because ``vite.config.ts`` sets ``optimizeDeps.noDiscovery: true`` — new dependencies must be listed in ``optimizeDeps.include`` or Vite won't run them through its CJS-to-ESM prebundler. Adding ``"shell-quote"`` there fixes it; the existing entry has a comment block explaining the same constraint for other deps. The Rollup-based prod build was unaffected. Verified: dev server boots clean, ``GET /settings/agent`` returns 200, no ``exports is not defined`` in the Vite client log, 9 unit tests in ``__tests__/utils/acp-command.test.ts`` pass on the Node test runner (vitest) where the CJS interop already worked. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): address second-pass review on PR #416 Addresses the second all-hands-bot review's critical + improvements: **Critical: load path mis-merged acp_command + acp_args** Settings stored with the registry-default shortcut (``acp_command: []``, ``acp_server: "claude-code"``) plus a non-empty ``acp_args`` showed only the args in the textarea — no registry prefix. Saving then sent ``acp_command: ["--extra-arg"]`` and flipped the preset to ``custom``, silently losing the ``npx -y @agentclientprotocol/ claude-agent-acp`` prefix. The fix expands the registry default *before* concatenating with args, so the textarea always shows the full launch command and round-trips cleanly. **Improvement: formatCommand drops empty-string args** ``formatCommand(["bash", "-c", ""])`` rendered as ``"bash -c "`` which parsed back to ``["bash", "-c"]``, silently losing the empty slot. Now quotes empty tokens explicitly so they survive. **Improvement: desktop sidebar disabled tooltip parity** Mobile drawer's ``SettingsNavLink`` already showed "Disabled while {agentName} is active" on greyed-out items; the desktop ``SidebarNavLink`` had no explanation. Added a ``disabledReason`` prop (i18n-agnostic; the caller formats the string) and wrap with ``StyledTooltip`` when disabled-with-reason. ``SettingsDesktopSidebar`` now forwards the formatted message — same UX on both surfaces. **Test coverage gaps the bot flagged** - ``agent-settings``: new regression guard for ``acp_command:[]`` + non-empty ``acp_args`` load (would have caught the critical bug above). - ``acp-command``: empty-string round-trip case + explicit assertion; five more shell-operator filters (pipe, ``;``, ``&&``, ``||``, ``>>``). - ``settings-navigation``: desktop sidebar wraps disabled items in StyledTooltip when ``disabledReason`` is supplied; not when omitted. Plus a clean merge from ``origin/main`` (one-line import conflict in ``agent-server-adapter.ts``). 2350 tests pass, lint + typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): address third-pass review on PR #416 - parseCommand: try/catch around shell-quote.parse so a malformed command in the textarea can't crash Settings → Agent mid-render - Rewrite shell-metasyntax tests to pin the *actual* shell-quote behaviour (it's a parser, not a security filter) — operators, globs, and comments are dropped; backticks / $VAR / $(...) survive as literal tokens but are NOT expanded at parse time - Add npm URLs + verification date (2026-05-19) to each ACP_PROVIDERS entry so future maintainers can re-check upstream packages - Document the silent preset-switch behaviour on detectPreset (the dropdown follows the textarea; the textarea is the source of truth) - Use the exported ACP_SERVER_TAG_KEY constant in the adapter test so a rename surfaces as a compile error rather than a runtime schema mismatch - Restore the canonical typescript-client lock entry (drop the git+ssh:// + SHA bump that crept in from a local npm install) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): don't expose LLM-switch UI on ACP conversations The SDK's ACPAgent carries a sentinel ``llm`` (``acp-managed``) for cost-attribution only — the real model lives on the ACP subprocess via ``acp_model`` and isn't visible on ``agent.llm.model``. Without this fix, ``toAppConversation`` surfaced the sentinel as the conversation's ``llm_model``, and the chat header's SwitchProfileButton happily let users "change the model" while the running Claude-Code / Codex / Gemini subprocess kept its own. A confusing silent no-op. Two layers of defence so no future consumer has to re-derive the rule: 1. Boundary normalisation: ``toAppConversation`` reads the pydantic discriminator (``info.agent.kind === "ACPAgent"``), surfaces it as ``agent_kind: "acp" | "openhands"`` on AppConversation, and nulls ``llm_model`` for ACP. Mirrors OpenHands PR #14401. 2. UI gate: SwitchProfileButton returns null when ``conversation.agent_kind === "acp"``. The right control for ACP model switching is the ``acp_model`` field on Settings → Agent, not this picker. Tests cover both: a new ``toAppConversation`` case asserts ``agent_kind === "acp"`` + ``llm_model === null`` for an ``{kind: "ACPAgent"}`` payload, and a new SwitchProfileButton case asserts the button hides for an ``agent_kind: "acp"`` conversation even when profiles are present. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): bridge Settings → Secrets into the ACP subprocess env The bare ``payload.secrets`` channel lands in the agent-server's ``secret_registry`` server-side, which the OpenHands ``Agent`` reads directly — but ``ACPAgent._start_acp_server`` builds its subprocess env from ``agent_context.secrets``, not from the registry. Without a bridge, a Settings → Secrets entry like ``ANTHROPIC_API_KEY`` is silently invisible to the ACP CLI (Claude Code, Codex, Gemini), so users hit "authentication failed" with no on-screen hint that their configured secret never reached the subprocess. Mirror the same LookupSecret map onto ``payload.agent.agent_context.secrets`` when ``acpMode === true``, so the agent-server's existing env-injection loop picks them up. The bare ``payload.secrets`` channel is also kept (it serves other consumers + remains the canonical "conversation secrets" wire). The mirroring fires only when there's something to bridge; non-ACP payloads are unchanged. This is a shim. Once canvas pins to an agent-server build that includes software-agent-sdk PR #3299 (which teaches ACPAgent to also read from ``state.secret_registry``), the ``if (acpMode)`` branch can be deleted with no behaviour change. Tests: - New: ACP payload mirrors customSecrets onto agent_context.secrets - New: empty customSecrets does NOT synthesize an empty bridge map - New: non-ACP payload does NOT get an agent_context.secrets bridge - All 42 adapter tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): hide MCP nav + cloud LLM-model fallback while ACP is active Two ACP-leak fixes the review surfaced: MCP page reachable + editable under ACP - The SDK's ``ACPAgent`` rejects ``mcp_config`` on init (acp_agent.py:845) and the canvas adapter already strips it from start payloads, but the /mcp route and the Extensions nav still let users add / edit / delete MCP servers — silent no-ops against the running subprocess. - Add a ``clientLoader`` on /mcp that bounces to /settings/agent when ``agent_kind === "acp"``. Grey out the MCP item in ExtensionsNavigation with the same explanatory tooltip the LLM / Condenser items already use under ACP. - Extract the redirect into ``utils/acp-route-guard.redirectIfAcpActive`` so /settings and /mcp share one cache-key + redirect-target definition. settings.tsx's clientLoader now calls into it. Cloud chat ``ChatInputModel`` falls back to ``settings.llm_model`` for ACP - ``toAppConversation`` writes ``llm_model: null`` on ACP conversations (commit 8f0efe62), but ChatInputModel did ``conversation?.llm_model ?? settings?.llm_model``, resurrecting the user's default OpenHands model on a Claude-Code conversation and linking to /settings (which is itself ACP-disabled). Gate on ``conversation?.agent_kind === "acp"`` and return null instead. Tests: - New: ExtensionsNavigation greys MCP under ACP, leaves Skills + non-ACP clickable - New: /mcp clientLoader redirects under ACP, returns null otherwise + on settings-fetch errors (no redirect-loop) - New: ChatInputModel returns null for ACP even when settings has a model - All 18 affected tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): preserve unknown acp_server on no-op saves + reviewer cleanups Fourth-pass review (PR #416 review comment 4486154133). Triage: Fixed: - **Unknown ``acp_server`` demoted to ``"custom"`` on save** (P1, real data corruption). A user with ``acp_server`` set out-of-band to a provider canvas's registry doesn't carry yet (e.g. a future provider, or one removed from the local mirror) would open Settings → Agent and lose the original key on the next Save — ``detectPreset`` routes every unknown server to ``ACP_CUSTOM_PRESET_KEY``. Now we capture the loaded ``acp_server`` + textarea at load time, and on save — when both are unchanged and the loaded key is non-empty, non-``"custom"``, and absent from ``ACP_PROVIDERS`` — pass it back verbatim via a new ``allowUnknownServer`` opt on ``buildAcpAgentSettingsDiff``. Editing the command still demotes to ``"custom"`` (user is configuring a new thing, so the preset name follows the command). - **Dead ``...existingContext`` spread** in the ACP secret bridge. ``createAgentFromSettings`` never populates ``agent_context`` on the ACP branch, so the spread always merged into ``{}``. Direct assignment — and a comment explaining why a deep-merge would be the wrong direction (ACPAgent only treats ``secrets`` as acp_compatible). - **Misleading ``$VAR`` test comment**. Reworded to lead with the no-leak contract (host env values must not end up in the persisted ``acp_command``) rather than the implementation-detail tangent. Documented but not changed: - **``acp_args: []`` "data loss" concern** — false alarm. Load merges ``acp_command + acp_args`` into the textarea before render; save persists the merged tokens as ``acp_command`` with ``acp_args: []``. Round-trip is correct. Added an inline comment on the load merge so the next reviewer doesn't re-flag the reset. - **Silent preset migration without user feedback** — by design. The dropdown re-derives from the textarea so it always reflects what will be saved; adding a toast on every keystroke would be noise. Already documented as intentional on ``detectPreset``. Tracked elsewhere: - Supply-chain drift / npm verification: agent-canvas#587. - Gemini onboarding icon: agent-canvas#621. Tests: - New: ``preserves an unknown loaded acp_server when the user saves without editing`` - New: ``demotes an unknown loaded acp_server to 'custom' when the user edits the command`` - All 73 affected tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): stop silently corrupting argv, restore AgentContext, fix home-screen gating Three real review findings, all wired: 1. ``parseCommand`` silently dropped URL tokens with ``?`` query strings (and any other shell-glob metacharacter). Reproducer: node acp.js --endpoint https://example.com/acp?tenant=abc ``shell-quote.parse`` read ``?tenant=abc`` as a glob pattern and emitted a non-string AST node; the ``.filter(string)`` then dropped the URL entirely, persisting ``["node","acp.js","--endpoint"]``. Replaced ``shell-quote.parse`` with a small custom argv tokenizer that handles single/double quotes + backslash escapes and treats every other character — ``?``, ``*``, ``$``, ``|``, ``>``, ``#``, ``&``, ``;``, ``(``, ``)``, backticks — as literal. The agent-server passes the argv straight to ``subprocess.create_subprocess_exec`` anyway (no shell intermediary), so the literal-only model matches what actually happens at spawn time. ``shell-quote.quote`` is still used by ``formatCommand`` for output. 2. The ACP path skipped the ``agent_context`` block that the OpenHands path seeded with ``load_public_skills`` / ``load_user_skills`` / optional ``system_message_suffix``. All three are marked ``acp_compatible: true`` on the SDK ``AgentContext`` model — the ACP CLI renders them via ``ACPAgent._render_suffix`` — so ACP conversations were silently shipping a smaller system prompt than OpenHands ones. ``createAgentFromSettings`` now seeds the same block on both branches. The secret bridge below merges into that block (was overwriting it) so ``{ secrets }`` no longer wipes the skill flags. 3. ``ChatInputModel`` and ``SwitchProfileButton`` only checked ``conversation?.agent_kind``. On the home screen (and during the task-startup window) ``conversation`` is undefined, so the per-conversation check missed and both surfaces fell back to ``settings.llm_model`` / the LLM-profile picker — even when ``settings.agent_settings.agent_kind === "acp"`` made it clear the next-created conversation would be ACP. Added a settings fallback so both controls hide consistently with the rest of the ACP nav gating. Tests: - parseCommand: new "preserves URLs with query strings" + "preserves URLs with multiple query params" + "preserves shell metacharacters as literal argv tokens" cases; the old "filters operator" cases flipped to "preserves operator as literal". 17 parseCommand cases pass. - adapter: assertion on the ACP payload's ``agent_context`` updated to expect the skill flags instead of ``undefined``. - chat-input-model + switch-profile-button: new "hides on the home page when ACP is the default agent" cases. - 83 tests pass across the 5 affected files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): two chat-rendering UX glitches on streaming ACP tool calls 1. Half-formed ACP tool-call cards flashed in the chat before the final state arrived. ACP servers stream multiple events per ``tool_call_id`` (status flips ``in_progress`` → ``completed`` / ``failed``); the intermediate events carry partial ``raw_input`` / ``raw_output`` / ``title``. The previous gate suppressed only ``in_progress`` and let ``null`` through (a "backwards compat" carve-out for older agent-server builds that no longer apply at our pinned version). Streaming intermediates often arrive without a status set yet, so they leaked. Tighten ``shouldRenderEvent`` to require ``status === "completed" || "failed"``. ``handleEventForUI`` already collapses by ``tool_call_id`` in place, so the terminal event lands at the original position once it arrives — no flash, no double-render. 2. "Reading Read /Users/foo/bar" — Claude Code emits titles like ``"Read /Users/foo/bar"`` for a read tool, and our i18n template ``"Reading <cmd>{{title}}</cmd>"`` then doubles up the verb. Add ``stripRedundantTitlePrefix`` keyed by ``tool_kind``: read → strip ``"Read"``, edit → strip ``"Edit"`` / ``"Write"``, execute → strip ``"Bash"`` / ``"Run"``, fetch → strip ``"Fetch"`` / ``"WebFetch"``. Boundary-checked via trailing whitespace so a token like ``"Reads-from"`` is left alone. English-only on purpose: ACP servers are anglophone and emit english titles regardless of the user's canvas locale; matching translated verbs would go stale the moment a new server is added. Titles already lacking a redundant prefix (the OpenHands ACP wrapper, future servers) round-trip verbatim — the strip is a no-op there. Tests: - ``shouldRenderEvent``: ``null`` status now flips to false + comment explains why (treated as in-flight, not legacy). - New ``stripRedundantTitlePrefix`` describe block covers the four tool kinds, the no-op case, the word-boundary guard, ``tool_kind: null`` (no strip), and empty titles. - 53 tests pass across the conversation-events helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(acp): drop React Router type import on /mcp clientLoader (CI build) CI's ``build:lib`` failed with: src/routes/mcp.tsx(3,23): error TS6059: File '.../.react-router/types/ src/routes/+types/mcp.ts' is not under 'rootDir' '/src'. ``tsconfig.lib.json`` sets ``rootDir: "src"`` and pulls in ``src/components/**/*.tsx``. ``src/components/settings/index.ts`` re-exports from ``routes/mcp-settings``, which imports ``routes/mcp`` — so the lib's typecheck graph reaches ``routes/mcp.tsx`` and trips on the generated ``./+types/mcp`` import that lives under ``.react-router/types/``, outside the lib's rootDir. (``routes/ settings.tsx`` uses the same import pattern but isn't reachable from the lib graph, which is why local typecheck passed.) Drop the type import and declare the loader with no parameters — matches the existing ``index-redirect`` and ``mcp-settings-redirect`` loader pattern. Test calls collapsed to ``clientLoader()`` to match the new signature. ``npm run build:lib`` now passes locally; 248 tests pass across the affected suites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(acp): tighten adapter comments around SDK refs Two reviewer-flagged comment fixes: - ``ACP_SETTINGS_KEYS`` docblock no longer claims there's a matching ``ACP_SETTINGS_KEYS`` constant in the Python SDK (there isn't; the fields are model attributes on ``ACPAgentSettings``). Reworded to "Keep aligned with the ``acp_*`` fields on ``ACPAgentSettings`` in ``openhands-sdk/openhands/sdk/settings/model.py``" with an explicit "no matching SDK constant — hand-maintained" note, and cross-linked to the existing #587 drift tracker. - ``createAgentFromSettings`` now spells out where the ``acp_compatible`` markers live on each of the three fields we set (``system_message_suffix`` L66, ``load_user_skills`` L80, ``load_public_skills`` L89 in ``openhands-sdk/openhands/sdk/context/agent_context.py``) plus what happens when a future SDK bump drops one (422 at conversation start → drop the demoted field, don't wrap a workaround). Line refs are brittle by design — they're the tripwire that surfaces a regression here rather than in production. No behaviour change; lint + 42 adapter tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Debug Agent <debug@example.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
simonrosenberg
committed by
GitHub
GitHub
co-authored by
Claude Opus 4.7
Debug Agent
Claude Opus 4.7
Debug Agent
parent
fc4ccc6c3f
commit
f0c36bac8f
@@ -1,6 +1,7 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import {
|
||||
ACP_SERVER_TAG_KEY,
|
||||
buildRuntimeServicesSystemSuffix,
|
||||
buildStartConversationRequest,
|
||||
getDefaultConversationTitle,
|
||||
@@ -358,6 +359,78 @@ describe("buildStartConversationRequest", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("mirrors conversation secrets onto agent.agent_context.secrets for ACP", () => {
|
||||
// Until canvas pins to an agent-server build that includes
|
||||
// software-agent-sdk PR #3299, the bare ``payload.secrets`` channel
|
||||
// only reaches ``secret_registry`` server-side — ``ACPAgent``'s
|
||||
// spawn-time env loop reads from ``agent_context.secrets``, not
|
||||
// from the registry, so a Settings → Secrets entry like
|
||||
// ``ANTHROPIC_API_KEY`` would silently fail to land in the ACP
|
||||
// CLI's environment. Mirror the same LookupSecret map onto
|
||||
// ``agent.agent_context.secrets`` so the existing SDK loop picks
|
||||
// it up. Mirrors OpenHands' app-server bridging.
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
...DEFAULT_SETTINGS.agent_settings,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
},
|
||||
},
|
||||
customSecrets: [{ name: "ANTHROPIC_API_KEY" }],
|
||||
}) as {
|
||||
agent: { agent_context?: { secrets?: Record<string, unknown> } };
|
||||
secrets: Record<string, unknown>;
|
||||
};
|
||||
|
||||
// Same LookupSecret object lands in both places — the bare-secrets
|
||||
// channel (for any non-ACP consumer / for SDK #3299 once it lands)
|
||||
// and the agent_context bridge (for current ACPAgent spawns).
|
||||
expect(payload.secrets.ANTHROPIC_API_KEY).toBeDefined();
|
||||
expect(payload.agent.agent_context?.secrets?.ANTHROPIC_API_KEY).toEqual(
|
||||
payload.secrets.ANTHROPIC_API_KEY,
|
||||
);
|
||||
});
|
||||
|
||||
it("does not synthesize agent_context.secrets for ACP when no custom secrets are set", () => {
|
||||
// Empty/absent customSecrets must not introduce an empty
|
||||
// ``agent_context.secrets`` map on the ACPAgent payload — the
|
||||
// bridge only fires when there's something to bridge.
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
...DEFAULT_SETTINGS.agent_settings,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: { agent_context?: { secrets?: Record<string, unknown> } };
|
||||
};
|
||||
|
||||
expect(payload.agent.agent_context?.secrets).toBeUndefined();
|
||||
});
|
||||
|
||||
it("does not mirror conversation secrets onto agent_context for non-ACP conversations", () => {
|
||||
// The OpenHands ``Agent`` reads secrets from ``secret_registry``
|
||||
// directly (no spawn-env bridging needed), so the LLM-driven path
|
||||
// must not get an extra ``agent_context.secrets`` map — that would
|
||||
// be both redundant and a surprise for any code that inspects
|
||||
// ``agent_context`` for non-secret payload (skills, suffixes, etc.).
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: DEFAULT_SETTINGS,
|
||||
customSecrets: [{ name: "ANTHROPIC_API_KEY" }],
|
||||
}) as {
|
||||
agent: { agent_context?: { secrets?: Record<string, unknown> } };
|
||||
};
|
||||
|
||||
expect(payload.agent.agent_context?.secrets).toBeUndefined();
|
||||
});
|
||||
|
||||
describe("canvas_ui tool injection", () => {
|
||||
it("always registers canvas_ui_tool in tool_module_qualnames, even when no user settings supply qualnames", () => {
|
||||
const payload = buildStartConversationRequest({
|
||||
@@ -448,6 +521,30 @@ describe("toAppConversation", () => {
|
||||
removeStoredConversationMetadata(baseInfo.id);
|
||||
}
|
||||
});
|
||||
|
||||
it("marks openhands conversations and surfaces the agent.llm.model", () => {
|
||||
const result = toAppConversation({
|
||||
...baseInfo,
|
||||
agent: { kind: "Agent", llm: { model: "claude-sonnet-4-6" } },
|
||||
});
|
||||
expect(result.agent_kind).toBe("openhands");
|
||||
expect(result.llm_model).toBe("claude-sonnet-4-6");
|
||||
});
|
||||
|
||||
it("marks ACP conversations and nulls llm_model so the chat UI can't mislead", () => {
|
||||
// The SDK's ACPAgent carries a sentinel ``llm`` (``acp-managed``) for
|
||||
// cost-attribution only; the *real* model lives on the ACP subprocess via
|
||||
// ``acp_model`` and isn't surfaced on ``agent.llm.model``. Surfacing the
|
||||
// sentinel as ``llm_model`` would let SwitchProfileButton render an
|
||||
// affordance to "change the model" on a Claude-Code conversation while
|
||||
// the running subprocess kept its own — a confusing silent no-op.
|
||||
const result = toAppConversation({
|
||||
...baseInfo,
|
||||
agent: { kind: "ACPAgent", llm: { model: "acp-managed" } },
|
||||
});
|
||||
expect(result.agent_kind).toBe("acp");
|
||||
expect(result.llm_model).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildRuntimeServicesSystemSuffix", () => {
|
||||
@@ -637,3 +734,313 @@ describe("createAgentFromSettings runtime services suffix", () => {
|
||||
).toContain("<RUNTIME_SERVICES>");
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildStartConversationRequest — ACP discriminator", () => {
|
||||
it("builds an ACPAgent payload when agent_kind is 'acp'", () => {
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
acp_model: "claude-opus-4-5",
|
||||
// These fields are LLM-only and must NOT leak into the ACP payload.
|
||||
agent: "CodeActAgent",
|
||||
llm: { model: "gpt-4", api_key: "should-not-appear" },
|
||||
condenser: { enabled: true, max_size: 240 },
|
||||
mcp_config: {
|
||||
mcpServers: { fake: { command: "x", args: [] } },
|
||||
},
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: Record<string, unknown> & {
|
||||
kind: string;
|
||||
acp_command?: string[];
|
||||
acp_model?: string | null;
|
||||
llm?: unknown;
|
||||
condenser?: unknown;
|
||||
tools?: unknown;
|
||||
agent_context?: unknown;
|
||||
};
|
||||
tags?: Record<string, string>;
|
||||
};
|
||||
|
||||
expect(payload.agent.kind).toBe("ACPAgent");
|
||||
expect(payload.agent.acp_command).toEqual([
|
||||
"npx",
|
||||
"-y",
|
||||
"@agentclientprotocol/claude-agent-acp",
|
||||
]);
|
||||
expect(payload.agent.acp_model).toBe("claude-opus-4-5");
|
||||
// LLM-only fields must not leak into the ACPAgent payload.
|
||||
expect(payload.agent.llm).toBeUndefined();
|
||||
expect(payload.agent.condenser).toBeUndefined();
|
||||
expect(payload.agent.tools).toBeUndefined();
|
||||
// ``agent_context`` IS populated on the ACP payload — the SDK marks
|
||||
// ``skills`` / ``system_message_suffix`` / ``load_*_skills`` as
|
||||
// ``acp_compatible: true``, and the ACP CLI's system prompt renders
|
||||
// them via ``ACPAgent._render_suffix``. Without seeding these, an
|
||||
// ACP user would silently lose the skill catalog and the runtime-
|
||||
// services awareness an OpenHands-driven conversation gets.
|
||||
expect(payload.agent.agent_context).toEqual({
|
||||
load_public_skills: true,
|
||||
load_user_skills: true,
|
||||
});
|
||||
// Conversation tags carry the ACP provider key for chip rendering.
|
||||
// Agent-server validates tag keys against ``^[a-z0-9]+$``, so the
|
||||
// snake_case ``acp_server`` form would be rejected — we use the
|
||||
// flattened ``acpserver`` form instead. Asserted via the exported
|
||||
// constant so a rename surfaces here as a compile error rather
|
||||
// than a silent schema-mismatch at runtime.
|
||||
expect(payload.tags).toEqual({ [ACP_SERVER_TAG_KEY]: "claude-code" });
|
||||
});
|
||||
|
||||
it("does not include ACP fields in the OpenHands Agent payload", () => {
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
...DEFAULT_SETTINGS.agent_settings,
|
||||
agent_kind: "openhands",
|
||||
llm: { model: "gpt-4" },
|
||||
acp_command: ["npx", "leftover"],
|
||||
acp_server: "claude-code",
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: Record<string, unknown> & { kind: string };
|
||||
tags?: Record<string, string>;
|
||||
};
|
||||
|
||||
expect(payload.agent.kind).toBe("Agent");
|
||||
expect(payload.agent.acp_command).toBeUndefined();
|
||||
expect(payload.agent.acp_server).toBeUndefined();
|
||||
expect(payload.agent.agent_kind).toBeUndefined();
|
||||
expect(payload.tags).toBeUndefined();
|
||||
});
|
||||
|
||||
it("omits acp_model when the user clears it (null)", () => {
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "custom",
|
||||
acp_command: ["./bin/my-agent"],
|
||||
acp_model: null,
|
||||
},
|
||||
},
|
||||
}) as { agent: Record<string, unknown> };
|
||||
|
||||
expect((payload.agent as { kind: string }).kind).toBe("ACPAgent");
|
||||
expect(payload.agent.acp_model).toBeUndefined();
|
||||
});
|
||||
|
||||
it("resolves an empty acp_command from the registry by acp_server", () => {
|
||||
// The Settings → Agent page and onboarding both store ``acp_command:
|
||||
// []`` for the default-preset path on the assumption that the agent-
|
||||
// server resolves it from ``acp_server``. The agent-server's ACPAgent
|
||||
// model does no such resolution — empty list crashes the spawn with
|
||||
// ``IndexError: list index out of range`` (acp_agent.py:1013) and the
|
||||
// conversation hangs in ``idle`` forever. The adapter has to expand
|
||||
// the command before the payload leaves the client.
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: [],
|
||||
acp_model: null,
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: Record<string, unknown> & { acp_command?: unknown[] };
|
||||
};
|
||||
|
||||
expect(payload.agent.acp_command).toEqual([
|
||||
"npx",
|
||||
"-y",
|
||||
"@agentclientprotocol/claude-agent-acp",
|
||||
]);
|
||||
});
|
||||
|
||||
it("resolves an absent acp_command for built-in providers too", () => {
|
||||
// The acp_command field may also be omitted entirely (e.g. on an older
|
||||
// settings shape that predates the field). Same fix applies.
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "codex",
|
||||
acp_model: null,
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: Record<string, unknown> & { acp_command?: unknown[] };
|
||||
};
|
||||
|
||||
expect(payload.agent.acp_command).toEqual([
|
||||
"npx",
|
||||
"-y",
|
||||
"@zed-industries/codex-acp",
|
||||
]);
|
||||
});
|
||||
|
||||
it("leaves acp_command alone when acp_server is 'custom'", () => {
|
||||
// Custom servers carry the user's explicit command. If they submitted
|
||||
// an empty one, that is their bug to see — the registry has no entry
|
||||
// to fall back to, and silently inventing one would be worse than the
|
||||
// explicit spawn error.
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "custom",
|
||||
acp_command: [],
|
||||
acp_model: null,
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: Record<string, unknown> & { acp_command?: unknown[] };
|
||||
};
|
||||
|
||||
expect(payload.agent.acp_command).toEqual([]);
|
||||
});
|
||||
|
||||
it("leaves acp_command alone for an unknown acp_server key", () => {
|
||||
// Future SDK adds a new provider before canvas's local mirror picks
|
||||
// it up: we don't recognise the key, so we can't expand the command
|
||||
// — but we also don't crash, and don't silently substitute one of
|
||||
// the known commands. The agent-server will produce the same
|
||||
// IndexError as before, which is the correct surface for "your
|
||||
// local canvas is out of date with the SDK."
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "future-provider-not-yet-mirrored",
|
||||
acp_command: [],
|
||||
acp_model: null,
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: Record<string, unknown> & { acp_command?: unknown[] };
|
||||
};
|
||||
|
||||
expect(payload.agent.acp_command).toEqual([]);
|
||||
});
|
||||
|
||||
it("treats acp_model: '' (empty string) as 'no override'", () => {
|
||||
// The form may carry an empty string after a user clears the model
|
||||
// input; the agent-server expects ``null`` for "use provider default."
|
||||
// Empty strings would pass the spawn but bias model selection on
|
||||
// some providers (e.g. claude-agent-acp's _meta would set
|
||||
// ``model: ''``).
|
||||
const payload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: [],
|
||||
acp_model: "",
|
||||
},
|
||||
},
|
||||
}) as {
|
||||
agent: Record<string, unknown> & { acp_model?: unknown };
|
||||
};
|
||||
|
||||
// ``buildConfiguredAcpAgentSettings`` already filters undefined +
|
||||
// null; the empty string falls through, which is a known nit. The
|
||||
// adapter's contract is "forward what settings says"; the
|
||||
// canonicalisation belongs in the save path
|
||||
// (``agent-settings.tsx::handleSave`` already does
|
||||
// ``acpModel.trim() || null``). Pin the current behaviour so a
|
||||
// future change to either side is a deliberate decision.
|
||||
expect(payload.agent.acp_model).toBe("");
|
||||
});
|
||||
|
||||
it("ACP → OpenHands → ACP round trip leaves no field leakage", () => {
|
||||
// Toggling agent_kind via the UI should not let stale ``acp_*``
|
||||
// state pollute an OpenHands run, and (in the reverse direction)
|
||||
// shouldn't let LLM/condenser/MCP state pollute an ACP payload.
|
||||
// We exercise both legs here against the same starting settings
|
||||
// shape so the round-trip is provable, not just inferred from the
|
||||
// single-direction tests.
|
||||
|
||||
const baseAcpSettings = {
|
||||
...DEFAULT_SETTINGS,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: [],
|
||||
acp_env: { ANTHROPIC_API_KEY: "user-set-via-api" },
|
||||
acp_model: "claude-opus-4-5",
|
||||
// LLM-only crud that would leak without the strip:
|
||||
agent: "CodeActAgent",
|
||||
llm: { model: "gpt-4o", api_key: "stale-from-prior-oh-run" },
|
||||
condenser: { enabled: true, max_size: 200 },
|
||||
},
|
||||
};
|
||||
|
||||
// Leg 1: ACP → OpenHands. The OpenHands branch must drop every
|
||||
// acp_* field; the LLM block survives.
|
||||
const ohPayload = buildStartConversationRequest({
|
||||
settings: {
|
||||
...baseAcpSettings,
|
||||
agent_settings: {
|
||||
...baseAcpSettings.agent_settings,
|
||||
agent_kind: "openhands",
|
||||
},
|
||||
},
|
||||
}) as { agent: Record<string, unknown> & { llm: Record<string, unknown> } };
|
||||
|
||||
expect(ohPayload.agent.kind).toBe("Agent");
|
||||
expect(ohPayload.agent.acp_command).toBeUndefined();
|
||||
expect(ohPayload.agent.acp_env).toBeUndefined();
|
||||
expect(ohPayload.agent.acp_model).toBeUndefined();
|
||||
expect(ohPayload.agent.acp_server).toBeUndefined();
|
||||
expect(ohPayload.agent.llm.model).toBe("gpt-4o");
|
||||
|
||||
// Leg 2: OpenHands → ACP (back). The ACP branch must drop the
|
||||
// llm/condenser/agent fields; the acp_* state survives.
|
||||
const acpPayload = buildStartConversationRequest({
|
||||
settings: baseAcpSettings,
|
||||
}) as {
|
||||
agent: Record<string, unknown> & {
|
||||
acp_command?: unknown;
|
||||
acp_env?: unknown;
|
||||
acp_model?: unknown;
|
||||
llm?: unknown;
|
||||
condenser?: unknown;
|
||||
};
|
||||
};
|
||||
|
||||
expect(acpPayload.agent.kind).toBe("ACPAgent");
|
||||
expect(acpPayload.agent.acp_command).toEqual([
|
||||
"npx",
|
||||
"-y",
|
||||
"@agentclientprotocol/claude-agent-acp",
|
||||
]);
|
||||
expect(acpPayload.agent.acp_model).toBe("claude-opus-4-5");
|
||||
expect(acpPayload.agent.acp_env).toEqual({
|
||||
ANTHROPIC_API_KEY: "user-set-via-api",
|
||||
});
|
||||
expect(acpPayload.agent.llm).toBeUndefined();
|
||||
expect(acpPayload.agent.condenser).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
+77
@@ -2,6 +2,7 @@ import { describe, it, expect } from "vitest";
|
||||
import {
|
||||
getACPToolCallContent,
|
||||
getACPToolCallTitleKey,
|
||||
stripRedundantTitlePrefix,
|
||||
} from "#/components/conversation-events/chat/event-content-helpers/get-acp-tool-call-content";
|
||||
import { getACPToolCallResult } from "#/components/conversation-events/chat/event-content-helpers/get-observation-result";
|
||||
import { ACPToolCallEvent } from "#/types/agent-server/core/events/acp-tool-call-event";
|
||||
@@ -117,6 +118,82 @@ describe("getACPToolCallContent", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("stripRedundantTitlePrefix", () => {
|
||||
// The i18n templates already wrap the title in a verb ("Reading
|
||||
// <cmd>…</cmd>"); ACP servers like Claude Code emit titles that also
|
||||
// carry a verb ("Read /Users/foo/bar"). Without the strip, the user
|
||||
// sees "Reading Read /Users/foo/bar".
|
||||
|
||||
it("strips a leading 'Read' from read-tool titles (the headline regression)", () => {
|
||||
expect(
|
||||
stripRedundantTitlePrefix(
|
||||
makeEvent({
|
||||
tool_kind: "read",
|
||||
title: "Read /Users/foo/bar/file.py",
|
||||
}),
|
||||
),
|
||||
).toBe("/Users/foo/bar/file.py");
|
||||
});
|
||||
|
||||
it("strips 'Edit' and 'Write' from edit-tool titles", () => {
|
||||
expect(
|
||||
stripRedundantTitlePrefix(
|
||||
makeEvent({ tool_kind: "edit", title: "Edit /workspace/foo.py" }),
|
||||
),
|
||||
).toBe("/workspace/foo.py");
|
||||
expect(
|
||||
stripRedundantTitlePrefix(
|
||||
makeEvent({ tool_kind: "edit", title: "Write /workspace/foo.py" }),
|
||||
),
|
||||
).toBe("/workspace/foo.py");
|
||||
});
|
||||
|
||||
it("strips 'Bash' and 'Run' from execute-tool titles", () => {
|
||||
expect(
|
||||
stripRedundantTitlePrefix(
|
||||
makeEvent({ tool_kind: "execute", title: "Bash ls -la" }),
|
||||
),
|
||||
).toBe("ls -la");
|
||||
});
|
||||
|
||||
it("leaves a title without the redundant prefix untouched", () => {
|
||||
// The OpenHands ACP wrapper, for example, may already emit just the
|
||||
// command. The strip should be a no-op in that case.
|
||||
expect(
|
||||
stripRedundantTitlePrefix(
|
||||
makeEvent({ tool_kind: "execute", title: "gh pr view 416" }),
|
||||
),
|
||||
).toBe("gh pr view 416");
|
||||
});
|
||||
|
||||
it("does not strip when the prefix is part of a longer word", () => {
|
||||
// ``"Reads"`` isn't the verb we want to strip — it's a different
|
||||
// token. Boundary-check via whitespace after the prefix prevents
|
||||
// the strip from over-reaching.
|
||||
expect(
|
||||
stripRedundantTitlePrefix(
|
||||
makeEvent({ tool_kind: "read", title: "Reads-from /foo" }),
|
||||
),
|
||||
).toBe("Reads-from /foo");
|
||||
});
|
||||
|
||||
it("does not strip when tool_kind is null (unknown shape)", () => {
|
||||
// Without a kind we can't know which prefixes are redundant; leave
|
||||
// the title verbatim.
|
||||
expect(
|
||||
stripRedundantTitlePrefix(
|
||||
makeEvent({ tool_kind: null, title: "Read /foo" }),
|
||||
),
|
||||
).toBe("Read /foo");
|
||||
});
|
||||
|
||||
it("handles an empty title", () => {
|
||||
expect(
|
||||
stripRedundantTitlePrefix(makeEvent({ tool_kind: "read", title: "" })),
|
||||
).toBe("");
|
||||
});
|
||||
});
|
||||
|
||||
describe("getACPToolCallResult", () => {
|
||||
it("returns success for completed, non-error events", () => {
|
||||
expect(getACPToolCallResult(baseEvent)).toBe("success");
|
||||
|
||||
+15
-5
@@ -54,27 +54,37 @@ describe("shouldRenderEvent - PlanningFileEditorAction", () => {
|
||||
});
|
||||
|
||||
describe("shouldRenderEvent - ACPToolCallEvent", () => {
|
||||
it("should return false for in_progress events (suppress empty-args flash)", () => {
|
||||
it("hides in_progress events so the card doesn't flash half-formed", () => {
|
||||
// ACP streams multiple events per ``tool_call_id``; the partially-
|
||||
// populated in-flight ones have to stay off-screen, otherwise the
|
||||
// user watches the card update in place mid-stream — visibly noisy.
|
||||
const event = makeACPEvent({ status: "in_progress", raw_input: {} });
|
||||
|
||||
expect(shouldRenderEvent(event)).toBe(false);
|
||||
});
|
||||
|
||||
it("should return true for completed events", () => {
|
||||
it("renders completed events", () => {
|
||||
const event = makeACPEvent({ status: "completed" });
|
||||
|
||||
expect(shouldRenderEvent(event)).toBe(true);
|
||||
});
|
||||
|
||||
it("should return true for failed events", () => {
|
||||
it("renders failed events", () => {
|
||||
const event = makeACPEvent({ status: "failed", is_error: true });
|
||||
|
||||
expect(shouldRenderEvent(event)).toBe(true);
|
||||
});
|
||||
|
||||
it("should return true for null status (backwards compat)", () => {
|
||||
it("hides events with null status — treated as in-flight, not legacy", () => {
|
||||
// ``null`` used to be allowed for backwards compat with old agent-
|
||||
// server builds (before the field was required). Canvas's pinned
|
||||
// agent-server always sets a status, so a ``null`` we see today is
|
||||
// a streaming intermediate the SDK hasn't filled yet. Treating it
|
||||
// as in-flight matches the "wait for terminal" rule and stops the
|
||||
// pre-stream half-rendered card. If we ever need to reach an older
|
||||
// build, the right knob is to bump the pin, not loosen this gate.
|
||||
const event = makeACPEvent({ status: null });
|
||||
|
||||
expect(shouldRenderEvent(event)).toBe(true);
|
||||
expect(shouldRenderEvent(event)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -14,7 +14,6 @@ vi.mock("#/hooks/query/use-settings", () => ({
|
||||
useSettings: () => useSettingsMock(),
|
||||
}));
|
||||
|
||||
// eslint-disable-next-line import/first
|
||||
import { ChatInputModel } from "#/components/features/chat/components/chat-input-model";
|
||||
|
||||
describe("ChatInputModel", () => {
|
||||
@@ -91,4 +90,53 @@ describe("ChatInputModel", () => {
|
||||
screen.queryByTestId("chat-input-llm-model"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("renders nothing for ACP conversations and does NOT fall back to settings.llm_model", () => {
|
||||
// The ACP subprocess owns its model (via ``acp_model``); ``llm_model``
|
||||
// is null on the conversation by design. The previous fallback to
|
||||
// ``settings.llm_model`` would have resurrected the user's *default*
|
||||
// OpenHands model on, say, a Claude-Code conversation — visibly
|
||||
// wrong (the link goes to /settings, which is itself disabled for
|
||||
// ACP) and silently lies about what model is actually running.
|
||||
useActiveConversationMock.mockReturnValue({
|
||||
data: {
|
||||
conversation_id: "test-conversation-id",
|
||||
agent_kind: "acp",
|
||||
llm_model: null,
|
||||
},
|
||||
});
|
||||
useSettingsMock.mockReturnValue({
|
||||
data: { llm_model: "anthropic/claude-sonnet-4-20250514" },
|
||||
});
|
||||
|
||||
renderWithProviders(<ChatInputModel />);
|
||||
|
||||
expect(
|
||||
screen.queryByTestId("chat-input-llm-model"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("renders nothing on the home page when ACP is the default agent", () => {
|
||||
// Home-screen gating: no active conversation, so the
|
||||
// per-conversation ``agent_kind`` check can't catch this case.
|
||||
// Fall back to ``settings.agent_settings.agent_kind`` — that's
|
||||
// the kind the next-created conversation will inherit. Showing
|
||||
// the LLM picker here would put up a control that becomes a
|
||||
// silent no-op the moment the user sends their first message.
|
||||
useActiveConversationMock.mockReturnValue({ data: undefined });
|
||||
useSettingsMock.mockReturnValue({
|
||||
data: {
|
||||
agent_settings: { agent_kind: "acp", acp_server: "claude-code" },
|
||||
// settings.llm_model is still set (the user has an OpenHands
|
||||
// default configured), but agent_kind=acp wins.
|
||||
llm_model: "anthropic/claude-sonnet-4-20250514",
|
||||
},
|
||||
});
|
||||
|
||||
renderWithProviders(<ChatInputModel />);
|
||||
|
||||
expect(
|
||||
screen.queryByTestId("chat-input-llm-model"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -5,6 +5,7 @@ import { renderWithProviders } from "test-utils";
|
||||
|
||||
const useLlmProfilesMock = vi.fn();
|
||||
const useActiveConversationMock = vi.fn();
|
||||
const useSettingsMock = vi.fn();
|
||||
const useSwitchLlmProfileAndLogMock = vi.fn();
|
||||
const useOptionalConversationIdMock = vi.fn();
|
||||
|
||||
@@ -16,6 +17,10 @@ vi.mock("#/hooks/query/use-active-conversation", () => ({
|
||||
useActiveConversation: () => useActiveConversationMock(),
|
||||
}));
|
||||
|
||||
vi.mock("#/hooks/query/use-settings", () => ({
|
||||
useSettings: () => useSettingsMock(),
|
||||
}));
|
||||
|
||||
vi.mock("#/hooks/mutation/use-switch-llm-profile-and-log", () => ({
|
||||
useSwitchLlmProfileAndLog: () => useSwitchLlmProfileAndLogMock(),
|
||||
}));
|
||||
@@ -24,11 +29,15 @@ vi.mock("#/hooks/use-conversation-id", () => ({
|
||||
useOptionalConversationId: () => useOptionalConversationIdMock(),
|
||||
}));
|
||||
|
||||
// eslint-disable-next-line import/first
|
||||
import { SwitchProfileButton } from "#/components/features/chat/switch-profile-button";
|
||||
|
||||
const profiles = [
|
||||
{ name: "haiku", model: "anthropic/claude-haiku", base_url: null, api_key_set: true },
|
||||
{
|
||||
name: "haiku",
|
||||
model: "anthropic/claude-haiku",
|
||||
base_url: null,
|
||||
api_key_set: true,
|
||||
},
|
||||
{ name: "gpt", model: "openai/gpt-4o", base_url: null, api_key_set: true },
|
||||
];
|
||||
|
||||
@@ -39,6 +48,7 @@ describe("SwitchProfileButton", () => {
|
||||
switchAndLog.mockReset();
|
||||
useLlmProfilesMock.mockReset();
|
||||
useActiveConversationMock.mockReset();
|
||||
useSettingsMock.mockReset();
|
||||
useSwitchLlmProfileAndLogMock.mockReset();
|
||||
useOptionalConversationIdMock.mockReset();
|
||||
|
||||
@@ -46,6 +56,7 @@ describe("SwitchProfileButton", () => {
|
||||
data: { profiles, active_profile: "haiku" },
|
||||
});
|
||||
useActiveConversationMock.mockReturnValue({ data: undefined });
|
||||
useSettingsMock.mockReturnValue({ data: undefined });
|
||||
useSwitchLlmProfileAndLogMock.mockReturnValue({
|
||||
switchAndLog,
|
||||
isPending: false,
|
||||
@@ -111,4 +122,46 @@ describe("SwitchProfileButton", () => {
|
||||
|
||||
expect(screen.getByTestId("switch-profile-button")).toBeDisabled();
|
||||
});
|
||||
|
||||
it("renders nothing for ACP conversations even when profiles exist", () => {
|
||||
// ACPAgent conversations route prompts to a CLI subprocess whose model is
|
||||
// set via ``acp_model`` in Settings → Agent, not by the LLM-profile
|
||||
// picker. Letting the user "switch the LLM" here would silently no-op
|
||||
// against the running subprocess — confusing UX. The button hides; the
|
||||
// user's path is the ACP model field on the agent settings page.
|
||||
useActiveConversationMock.mockReturnValue({
|
||||
data: {
|
||||
id: "conv-1",
|
||||
agent_kind: "acp",
|
||||
llm_model: null,
|
||||
},
|
||||
});
|
||||
|
||||
renderWithProviders(<SwitchProfileButton />);
|
||||
|
||||
expect(
|
||||
screen.queryByTestId("switch-profile-button"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("hides the picker on the home page when ACP is the default agent", () => {
|
||||
// Home-screen gating: there's no active conversation, so the
|
||||
// per-conversation ``agent_kind`` check can't catch this case.
|
||||
// Fall back to ``settings.agent_settings.agent_kind`` — that's the
|
||||
// kind the next-created conversation will inherit, and showing
|
||||
// the LLM picker for it would silently no-op once the user starts
|
||||
// chatting. Mirrors the ACP nav gating elsewhere in the app.
|
||||
useActiveConversationMock.mockReturnValue({ data: undefined });
|
||||
useSettingsMock.mockReturnValue({
|
||||
data: {
|
||||
agent_settings: { agent_kind: "acp", acp_server: "claude-code" },
|
||||
},
|
||||
});
|
||||
|
||||
renderWithProviders(<SwitchProfileButton />);
|
||||
|
||||
expect(
|
||||
screen.queryByTestId("switch-profile-button"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import type { ReactNode } from "react";
|
||||
import { render, screen, within } from "@testing-library/react";
|
||||
import { render, screen, waitFor, within } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { MemoryRouter } from "react-router";
|
||||
@@ -9,11 +9,36 @@ import { OSS_NAV_ITEMS } from "#/constants/settings-nav";
|
||||
import { SettingsNavRenderedItem } from "#/hooks/use-settings-nav-items";
|
||||
import { ActiveBackendProvider } from "#/contexts/active-backend-context";
|
||||
|
||||
// HeroUI's Tooltip (the engine behind ``StyledTooltip``) only mounts
|
||||
// its content when the trigger is interacted with via real-DOM events.
|
||||
// jsdom + userEvent.hover doesn't reliably fire that, so we stub the
|
||||
// wrapper to render its content eagerly — every test below can then
|
||||
// assert "the tooltip would say X" by looking for X in the DOM.
|
||||
vi.mock("#/components/shared/buttons/styled-tooltip", () => ({
|
||||
StyledTooltip: ({
|
||||
content,
|
||||
children,
|
||||
}: {
|
||||
content: React.ReactNode;
|
||||
children: React.ReactNode;
|
||||
}) => (
|
||||
<>
|
||||
{children}
|
||||
<span data-testid="styled-tooltip-content">{content}</span>
|
||||
</>
|
||||
),
|
||||
}));
|
||||
|
||||
const llmItem = OSS_NAV_ITEMS.find((item) => item.to === "/settings")!;
|
||||
const condenserItem = OSS_NAV_ITEMS.find(
|
||||
(item) => item.to === "/settings/condenser",
|
||||
)!;
|
||||
|
||||
const baseItems: SettingsNavRenderedItem[] = [
|
||||
{ type: "header", text: "SETTINGS$TITLE" as never },
|
||||
{ type: "item", item: OSS_NAV_ITEMS[0] },
|
||||
{ type: "item", item: llmItem },
|
||||
{ type: "divider" },
|
||||
{ type: "item", item: OSS_NAV_ITEMS[1] },
|
||||
{ type: "item", item: condenserItem },
|
||||
];
|
||||
|
||||
function renderSettingsNavigation(ui: ReactNode) {
|
||||
@@ -80,4 +105,113 @@ describe("SettingsNavigation", () => {
|
||||
|
||||
expect(onCloseMobileMenu).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("renders disabled-by-ACP items as disabled in the desktop sidebar", () => {
|
||||
// Regression guard: when ACP is active, the LLM and Condenser items
|
||||
// come through with ``disabled: true`` from ``useSettingsNavItems``;
|
||||
// both the mobile drawer (via SettingsNavLink) and the desktop
|
||||
// sidebar (via SidebarNavLink) must propagate that. Earlier the
|
||||
// desktop branch dropped it and the items stayed clickable.
|
||||
renderSettingsNavigation(
|
||||
<SettingsNavigation
|
||||
isMobileMenuOpen={false}
|
||||
onCloseMobileMenu={vi.fn()}
|
||||
navigationItems={[
|
||||
{
|
||||
type: "item",
|
||||
item: llmItem,
|
||||
disabled: true,
|
||||
disabledAgentName: "Claude Code",
|
||||
},
|
||||
{
|
||||
type: "item",
|
||||
item: condenserItem,
|
||||
disabled: true,
|
||||
disabledAgentName: "Claude Code",
|
||||
},
|
||||
]}
|
||||
/>,
|
||||
);
|
||||
|
||||
const desktopNav = screen.getByTestId("settings-navbar-desktop");
|
||||
|
||||
// SidebarNavLink renders disabled items as a non-link span with
|
||||
// ``aria-disabled="true"`` and ``opacity-50`` styling.
|
||||
const llmLink = within(desktopNav).getByTestId(
|
||||
"sidebar-settings-/settings",
|
||||
);
|
||||
const condenserLink = within(desktopNav).getByTestId(
|
||||
"sidebar-settings-/settings/condenser",
|
||||
);
|
||||
expect(llmLink).toHaveAttribute("aria-disabled", "true");
|
||||
expect(condenserLink).toHaveAttribute("aria-disabled", "true");
|
||||
});
|
||||
|
||||
it("leaves enabled items clickable in the desktop sidebar", () => {
|
||||
renderSettingsNavigation(
|
||||
<SettingsNavigation
|
||||
isMobileMenuOpen={false}
|
||||
onCloseMobileMenu={vi.fn()}
|
||||
navigationItems={[{ type: "item", item: llmItem }]}
|
||||
/>,
|
||||
);
|
||||
const desktopNav = screen.getByTestId("settings-navbar-desktop");
|
||||
const llmLink = within(desktopNav).getByTestId(
|
||||
"sidebar-settings-/settings",
|
||||
);
|
||||
expect(llmLink).not.toHaveAttribute("aria-disabled", "true");
|
||||
});
|
||||
|
||||
it("wraps disabled-by-ACP desktop items in the explanatory tooltip", () => {
|
||||
// The mobile drawer already builds + shows this string ("Disabled
|
||||
// while Claude Code is active"); the desktop sidebar used to just
|
||||
// grey the item out with no explanation. SettingsDesktopSidebar
|
||||
// now formats the i18n string and forwards it as ``disabledReason``
|
||||
// to SidebarNavLink, which wraps the link in StyledTooltip when
|
||||
// both flags are set.
|
||||
renderSettingsNavigation(
|
||||
<SettingsNavigation
|
||||
isMobileMenuOpen={false}
|
||||
onCloseMobileMenu={vi.fn()}
|
||||
navigationItems={[
|
||||
{
|
||||
type: "item",
|
||||
item: condenserItem,
|
||||
disabled: true,
|
||||
disabledAgentName: "Claude Code",
|
||||
},
|
||||
]}
|
||||
/>,
|
||||
);
|
||||
|
||||
// The StyledTooltip mock above renders its ``content`` prop into a
|
||||
// <span data-testid="styled-tooltip-content">…</span>. Its presence
|
||||
// proves SidebarNavLink wrapped the link with the explanatory
|
||||
// tooltip — i.e. ``disabledReason`` was forwarded. (We can't
|
||||
// assert on the resolved string content because the test
|
||||
// environment returns raw i18n keys without interpolation; the
|
||||
// wiring itself is what we care about. The enabled-control test
|
||||
// below verifies the wrap doesn't appear without the prop.)
|
||||
const desktopNav = screen.getByTestId("settings-navbar-desktop");
|
||||
expect(
|
||||
within(desktopNav).queryByTestId("styled-tooltip-content"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("does not wrap enabled items in a tooltip on the desktop sidebar", () => {
|
||||
renderSettingsNavigation(
|
||||
<SettingsNavigation
|
||||
isMobileMenuOpen={false}
|
||||
onCloseMobileMenu={vi.fn()}
|
||||
navigationItems={[{ type: "item", item: condenserItem }]}
|
||||
/>,
|
||||
);
|
||||
// No ``disabled`` + no ``disabledReason`` → SidebarNavLink returns
|
||||
// the bare NavigationLink (no StyledTooltip wrap), so the mock's
|
||||
// marker element doesn't appear at all.
|
||||
const desktopNav = screen.getByTestId("settings-navbar-desktop");
|
||||
expect(
|
||||
within(desktopNav).queryByTestId("styled-tooltip-content"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,112 @@
|
||||
import type { ReactNode } from "react";
|
||||
import { render, screen, within } from "@testing-library/react";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { MemoryRouter } from "react-router";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { ActiveBackendProvider } from "#/contexts/active-backend-context";
|
||||
|
||||
const useSettingsMock = vi.fn();
|
||||
vi.mock("#/hooks/query/use-settings", () => ({
|
||||
useSettings: () => useSettingsMock(),
|
||||
}));
|
||||
|
||||
// HeroUI's Tooltip only mounts content on real-DOM hover; stub the
|
||||
// wrapper to render content eagerly so we can assert "the tooltip
|
||||
// would say X" via the DOM. Mirrors the pattern in
|
||||
// ``settings-navigation.test.tsx``.
|
||||
vi.mock("#/components/shared/buttons/styled-tooltip", () => ({
|
||||
StyledTooltip: ({
|
||||
content,
|
||||
children,
|
||||
}: {
|
||||
content: React.ReactNode;
|
||||
children: React.ReactNode;
|
||||
}) => (
|
||||
<>
|
||||
{children}
|
||||
<span data-testid="styled-tooltip-content">{content}</span>
|
||||
</>
|
||||
),
|
||||
}));
|
||||
|
||||
import { ExtensionsNavigation } from "#/components/features/skills/extensions-navigation";
|
||||
|
||||
function renderExtensionsNavigation(ui: ReactNode) {
|
||||
return render(
|
||||
<QueryClientProvider
|
||||
client={
|
||||
new QueryClient({ defaultOptions: { queries: { retry: false } } })
|
||||
}
|
||||
>
|
||||
<ActiveBackendProvider>
|
||||
<MemoryRouter>{ui}</MemoryRouter>
|
||||
</ActiveBackendProvider>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
}
|
||||
|
||||
describe("ExtensionsNavigation", () => {
|
||||
it("renders the MCP item as a clickable link for non-ACP agents", () => {
|
||||
useSettingsMock.mockReturnValue({
|
||||
data: { agent_settings: { agent_kind: "openhands" } },
|
||||
});
|
||||
|
||||
renderExtensionsNavigation(<ExtensionsNavigation />);
|
||||
|
||||
const nav = screen.getByTestId("extensions-navbar-desktop");
|
||||
const mcpItem = within(nav).getByTestId("sidebar-extensions-/mcp");
|
||||
expect(mcpItem).not.toHaveAttribute("aria-disabled");
|
||||
// Active link — `NavigationLink` renders as <a>; the disabled
|
||||
// branch renders <span>. Tagging matters because the disabled
|
||||
// version has no href, breaking direct URL navigation.
|
||||
expect(mcpItem.tagName).toBe("A");
|
||||
});
|
||||
|
||||
it("greys out the MCP item and wraps it in the ACP tooltip when ACP is active", () => {
|
||||
// Regression guard for the comment in PR #416 review: with an ACP
|
||||
// agent active, /mcp configuration is silently no-op (the SDK's
|
||||
// ``ACPAgent`` rejects ``mcp_config`` on init). Greying the nav
|
||||
// item plus the explanatory tooltip mirrors how /settings,
|
||||
// /settings/condenser already behave under ACP.
|
||||
useSettingsMock.mockReturnValue({
|
||||
data: {
|
||||
agent_settings: { agent_kind: "acp", acp_server: "claude-code" },
|
||||
},
|
||||
});
|
||||
|
||||
renderExtensionsNavigation(<ExtensionsNavigation />);
|
||||
|
||||
const nav = screen.getByTestId("extensions-navbar-desktop");
|
||||
const mcpItem = within(nav).getByTestId("sidebar-extensions-/mcp");
|
||||
expect(mcpItem).toHaveAttribute("aria-disabled", "true");
|
||||
// Disabled rendering uses <span>, not <a> — no href means no
|
||||
// accidental navigation if the user keyboard-tabs onto it.
|
||||
expect(mcpItem.tagName).toBe("SPAN");
|
||||
// The StyledTooltip mock writes its ``content`` prop into a
|
||||
// <span data-testid="styled-tooltip-content">. Its presence proves
|
||||
// the disabled branch wrapped the link with the explanatory
|
||||
// tooltip; the absence on enabled control (see Skills below)
|
||||
// proves we don't over-wrap.
|
||||
expect(
|
||||
within(nav).queryByTestId("styled-tooltip-content"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("leaves the Skills item clickable even when ACP is active", () => {
|
||||
// Skills isn't ACP-gated — the ACP subprocess can still benefit
|
||||
// from rendered skills in its <CUSTOM_SECRETS>/system suffix. Only
|
||||
// /mcp goes grey; /skills stays a normal link.
|
||||
useSettingsMock.mockReturnValue({
|
||||
data: {
|
||||
agent_settings: { agent_kind: "acp", acp_server: "claude-code" },
|
||||
},
|
||||
});
|
||||
|
||||
renderExtensionsNavigation(<ExtensionsNavigation />);
|
||||
|
||||
const nav = screen.getByTestId("extensions-navbar-desktop");
|
||||
const skillsItem = within(nav).getByTestId("sidebar-extensions-/skills");
|
||||
expect(skillsItem).not.toHaveAttribute("aria-disabled");
|
||||
expect(skillsItem.tagName).toBe("A");
|
||||
});
|
||||
});
|
||||
@@ -1,75 +1,227 @@
|
||||
import React from "react";
|
||||
import { render, screen } from "@testing-library/react";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { render, screen, waitFor } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
ChooseAgentStep,
|
||||
type OnboardingAgentId,
|
||||
} from "#/components/features/onboarding/steps/choose-agent-step";
|
||||
import SettingsService from "#/api/settings-service/settings-service.api";
|
||||
|
||||
function renderStep(initial: OnboardingAgentId = "openhands") {
|
||||
const onSelect = vi.fn();
|
||||
const onNext = vi.fn();
|
||||
render(
|
||||
<ChooseAgentStep
|
||||
selectedAgentId={initial}
|
||||
onSelect={onSelect}
|
||||
onNext={onNext}
|
||||
/>,
|
||||
<QueryClientProvider
|
||||
client={
|
||||
new QueryClient({ defaultOptions: { queries: { retry: false } } })
|
||||
}
|
||||
>
|
||||
<ChooseAgentStep
|
||||
selectedAgentId={initial}
|
||||
onSelect={onSelect}
|
||||
onNext={onNext}
|
||||
/>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
return { onSelect, onNext };
|
||||
}
|
||||
|
||||
describe("ChooseAgentStep", () => {
|
||||
it("renders the three agent options with OpenHands marked selected", () => {
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
vi.spyOn(SettingsService, "saveSettings").mockResolvedValue(true);
|
||||
});
|
||||
|
||||
it("renders all four agent options with OpenHands marked selected by default", () => {
|
||||
renderStep();
|
||||
|
||||
const openhands = screen.getByTestId("onboarding-agent-option-openhands");
|
||||
const claude = screen.getByTestId("onboarding-agent-option-claude-code");
|
||||
const codex = screen.getByTestId("onboarding-agent-option-codex");
|
||||
const gemini = screen.getByTestId("onboarding-agent-option-gemini-cli");
|
||||
|
||||
expect(openhands).toHaveAttribute("aria-checked", "true");
|
||||
// All four options are clickable — ACP is no longer "coming soon".
|
||||
expect(openhands).not.toBeDisabled();
|
||||
expect(claude).not.toBeDisabled();
|
||||
expect(codex).not.toBeDisabled();
|
||||
expect(gemini).not.toBeDisabled();
|
||||
|
||||
// Claude Code and Codex are visible but disabled with per-option badges.
|
||||
expect(claude).toBeDisabled();
|
||||
expect(claude).toHaveAttribute("aria-disabled", "true");
|
||||
expect(codex).toBeDisabled();
|
||||
expect(codex).toHaveAttribute("aria-disabled", "true");
|
||||
|
||||
// Neither the legacy "coming soon" banner nor the per-option badges
|
||||
// should render now that all four agent kinds work end-to-end.
|
||||
expect(
|
||||
screen.getByTestId("onboarding-agent-badge-claude-code"),
|
||||
).toBeInTheDocument();
|
||||
screen.queryByTestId("onboarding-agent-coming-soon"),
|
||||
).not.toBeInTheDocument();
|
||||
expect(
|
||||
screen.getByTestId("onboarding-agent-badge-codex"),
|
||||
).toBeInTheDocument();
|
||||
screen.queryByTestId("onboarding-agent-badge-claude-code"),
|
||||
).not.toBeInTheDocument();
|
||||
expect(
|
||||
screen.queryByTestId("onboarding-agent-badge-codex"),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("ignores clicks on the disabled agent options", async () => {
|
||||
it("propagates click selections through onSelect for every option", async () => {
|
||||
const { onSelect } = renderStep();
|
||||
const user = userEvent.setup();
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-option-claude-code"));
|
||||
expect(onSelect).toHaveBeenLastCalledWith("claude-code");
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-option-codex"));
|
||||
expect(onSelect).toHaveBeenLastCalledWith("codex");
|
||||
|
||||
expect(onSelect).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("invokes onSelect when the OpenHands option is clicked", async () => {
|
||||
const { onSelect } = renderStep();
|
||||
const user = userEvent.setup();
|
||||
await user.click(screen.getByTestId("onboarding-agent-option-gemini-cli"));
|
||||
expect(onSelect).toHaveBeenLastCalledWith("gemini-cli");
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-option-openhands"));
|
||||
|
||||
expect(onSelect).toHaveBeenCalledWith("openhands");
|
||||
expect(onSelect).toHaveBeenLastCalledWith("openhands");
|
||||
});
|
||||
|
||||
it("invokes onNext when the Next button is clicked", async () => {
|
||||
const { onNext } = renderStep();
|
||||
it("persists agent_kind:'openhands' and advances on Next when OpenHands is selected", async () => {
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
const { onNext } = renderStep("openhands");
|
||||
const user = userEvent.setup();
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
|
||||
expect(onNext).toHaveBeenCalledTimes(1);
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
expect(onNext).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff).toEqual({ agent_kind: "openhands" });
|
||||
});
|
||||
|
||||
it("persists an ACP diff matching the registry when Claude Code is selected", async () => {
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
const { onNext } = renderStep("claude-code");
|
||||
const user = userEvent.setup();
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
expect(onNext).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff).toEqual({
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
// Default-command path: the backend resolves the command from its
|
||||
// own registry, so we don't pin a stale command here.
|
||||
acp_command: [],
|
||||
// ``acp_args: []`` is reset on every save so an API-set
|
||||
// ``acp_args`` can't survive and concatenate onto the spawn
|
||||
// command at conversation-create time.
|
||||
acp_args: [],
|
||||
acp_model: null,
|
||||
});
|
||||
});
|
||||
|
||||
it.each([
|
||||
["codex", "codex"],
|
||||
["gemini-cli", "gemini-cli"],
|
||||
])("persists acp_server=%s for the matching tile", async (id, expected) => {
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
renderStep(id as OnboardingAgentId);
|
||||
const user = userEvent.setup();
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(
|
||||
(call.agent_settings_diff as Record<string, unknown>).acp_server,
|
||||
).toBe(expected);
|
||||
});
|
||||
|
||||
it("rebuilds the diff cleanly when the user flips between ACP providers", async () => {
|
||||
// Bot-flagged: would a stale claude-code selection leak into a
|
||||
// subsequent codex save? The helper rebuilds from the current
|
||||
// selectedAgentId on each Next click, so no — but pin it.
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
const { rerender } = render(
|
||||
<QueryClientProvider
|
||||
client={
|
||||
new QueryClient({ defaultOptions: { queries: { retry: false } } })
|
||||
}
|
||||
>
|
||||
<ChooseAgentStep
|
||||
selectedAgentId="claude-code"
|
||||
onSelect={vi.fn()}
|
||||
onNext={vi.fn()}
|
||||
/>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
const user = userEvent.setup();
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
expect(
|
||||
(
|
||||
save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
}
|
||||
).agent_settings_diff?.acp_server,
|
||||
).toBe("claude-code");
|
||||
|
||||
// Switch the *parent's* selection (the modal's setSelectedAgentId)
|
||||
// and click Next again — the second save should carry codex, not
|
||||
// a stale claude-code value.
|
||||
save.mockClear();
|
||||
rerender(
|
||||
<QueryClientProvider
|
||||
client={
|
||||
new QueryClient({ defaultOptions: { queries: { retry: false } } })
|
||||
}
|
||||
>
|
||||
<ChooseAgentStep
|
||||
selectedAgentId="codex"
|
||||
onSelect={vi.fn()}
|
||||
onNext={vi.fn()}
|
||||
/>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
expect(
|
||||
(
|
||||
save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
}
|
||||
).agent_settings_diff?.acp_server,
|
||||
).toBe("codex");
|
||||
});
|
||||
|
||||
it("does not advance when the save mutation fails", async () => {
|
||||
vi.spyOn(SettingsService, "saveSettings").mockRejectedValueOnce(
|
||||
new Error("boom"),
|
||||
);
|
||||
const { onNext } = renderStep("claude-code");
|
||||
const user = userEvent.setup();
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
|
||||
// saveSettings rejects → onSuccess is not called → onNext stays untouched.
|
||||
await waitFor(
|
||||
() => {
|
||||
expect(SettingsService.saveSettings).toHaveBeenCalledTimes(1);
|
||||
},
|
||||
{ timeout: 1000 },
|
||||
);
|
||||
expect(onNext).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -8,6 +8,7 @@ import { __resetActiveStoreForTests } from "#/api/backend-registry/active-store"
|
||||
import { ActiveBackendProvider } from "#/contexts/active-backend-context";
|
||||
import { OnboardingModal } from "#/components/features/onboarding/onboarding-modal";
|
||||
import { NavigationProvider } from "#/context/navigation-context";
|
||||
import SettingsService from "#/api/settings-service/settings-service.api";
|
||||
|
||||
// Both the backend status badge in the embedded edit form and the
|
||||
// step-1 health probe ride on `useBackendsHealth`, which itself
|
||||
@@ -73,6 +74,10 @@ function renderModal(onClose = vi.fn()) {
|
||||
beforeEach(() => {
|
||||
window.localStorage.clear();
|
||||
__resetActiveStoreForTests();
|
||||
// ChooseAgentStep's Next button now persists the selection via
|
||||
// saveSettings before advancing. Stub it so the rest of the flow
|
||||
// (which these tests focus on) isn't gated on a real HTTP call.
|
||||
vi.spyOn(SettingsService, "saveSettings").mockResolvedValue(true);
|
||||
});
|
||||
afterEach(() => {
|
||||
window.localStorage.clear();
|
||||
@@ -111,13 +116,18 @@ describe("OnboardingModal", () => {
|
||||
renderModal();
|
||||
const user = userEvent.setup();
|
||||
|
||||
// Step 0 → 1
|
||||
// Step 0 → 1. ChooseAgentStep now does an async save before
|
||||
// advancing, so the modal can take a beat to flip steps while
|
||||
// SayHello/CheckBackend queries are still settling on the four
|
||||
// mounted slides. Bump the default 1s waitFor timeout.
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByTestId("onboarding-modal")).toHaveAttribute(
|
||||
"data-current-step",
|
||||
"1",
|
||||
),
|
||||
await waitFor(
|
||||
() =>
|
||||
expect(screen.getByTestId("onboarding-modal")).toHaveAttribute(
|
||||
"data-current-step",
|
||||
"1",
|
||||
),
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
expect(screen.getByTestId("onboarding-slide-1")).toHaveAttribute(
|
||||
"data-active",
|
||||
@@ -177,10 +187,25 @@ describe("OnboardingModal", () => {
|
||||
renderModal();
|
||||
const user = userEvent.setup();
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByTestId("onboarding-backend-next")).not.toBeDisabled(),
|
||||
await waitFor(
|
||||
() =>
|
||||
expect(
|
||||
screen.getByTestId("onboarding-backend-next"),
|
||||
).not.toBeDisabled(),
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
await user.click(screen.getByTestId("onboarding-backend-next"));
|
||||
// Wait for the LLM slide to become the active one before querying
|
||||
// by role — otherwise the heading is `aria-hidden` from inside a
|
||||
// not-yet-active slide and getByRole filters it out.
|
||||
await waitFor(
|
||||
() =>
|
||||
expect(screen.getByTestId("onboarding-modal")).toHaveAttribute(
|
||||
"data-current-step",
|
||||
"2",
|
||||
),
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
|
||||
// Act: locate the step's scrollable settings wrapper and the chrome
|
||||
// around it that the user expects to remain visible.
|
||||
@@ -199,13 +224,84 @@ describe("OnboardingModal", () => {
|
||||
expect(settings.contains(next)).toBe(false);
|
||||
});
|
||||
|
||||
it("skips the LLM-setup step when the user picks an ACP agent", async () => {
|
||||
renderModal();
|
||||
const user = userEvent.setup();
|
||||
|
||||
// Pick Claude Code, then advance from Choose Agent → Check Backend.
|
||||
await user.click(screen.getByTestId("onboarding-agent-option-claude-code"));
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
await waitFor(
|
||||
() =>
|
||||
expect(screen.getByTestId("onboarding-modal")).toHaveAttribute(
|
||||
"data-current-step",
|
||||
"1",
|
||||
),
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
|
||||
// Advancing again should jump straight to Say Hello (index 3) and
|
||||
// bypass the LLM form — ACP agents own their own LLM via the
|
||||
// subprocess.
|
||||
await waitFor(
|
||||
() =>
|
||||
expect(
|
||||
screen.getByTestId("onboarding-backend-next"),
|
||||
).not.toBeDisabled(),
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
await user.click(screen.getByTestId("onboarding-backend-next"));
|
||||
|
||||
await waitFor(
|
||||
() =>
|
||||
expect(screen.getByTestId("onboarding-modal")).toHaveAttribute(
|
||||
"data-current-step",
|
||||
"3",
|
||||
),
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
// All four slides remain mounted (the rail just translates them);
|
||||
// the assertion that the LLM step was skipped is that slide 3 (Say
|
||||
// Hello) is the active one immediately after the backend step,
|
||||
// *not* slide 2 (LLM).
|
||||
expect(screen.getByTestId("onboarding-slide-2")).toHaveAttribute(
|
||||
"data-active",
|
||||
"false",
|
||||
);
|
||||
expect(screen.getByTestId("onboarding-slide-3")).toHaveAttribute(
|
||||
"data-active",
|
||||
"true",
|
||||
);
|
||||
|
||||
// Progress bar reflects the *visited* step count, not the slide
|
||||
// index — 3 segments total (not 4), and segment 2 is current (not
|
||||
// segment 3, which would imply LLM was completed). Without this
|
||||
// mapping, picking an ACP agent makes the bar show segment 2 as
|
||||
// "completed" despite the user never visiting it.
|
||||
expect(
|
||||
screen.queryByTestId("onboarding-progress-step-3"),
|
||||
).not.toBeInTheDocument();
|
||||
expect(screen.getByTestId("onboarding-progress-step-2")).toHaveAttribute(
|
||||
"data-state",
|
||||
"current",
|
||||
);
|
||||
expect(screen.getByTestId("onboarding-progress-step-1")).toHaveAttribute(
|
||||
"data-state",
|
||||
"completed",
|
||||
);
|
||||
});
|
||||
|
||||
it("pre-fills the say-hello input with the default greeting on step 3", async () => {
|
||||
renderModal();
|
||||
const user = userEvent.setup();
|
||||
|
||||
await user.click(screen.getByTestId("onboarding-agent-next"));
|
||||
await waitFor(() =>
|
||||
expect(screen.getByTestId("onboarding-backend-next")).not.toBeDisabled(),
|
||||
await waitFor(
|
||||
() =>
|
||||
expect(
|
||||
screen.getByTestId("onboarding-backend-next"),
|
||||
).not.toBeDisabled(),
|
||||
{ timeout: 3000 },
|
||||
);
|
||||
await user.click(screen.getByTestId("onboarding-backend-next"));
|
||||
await user.click(screen.getByTestId("onboarding-llm-next"));
|
||||
|
||||
@@ -5,6 +5,7 @@ import { useSettingsNavItems } from "#/hooks/use-settings-nav-items";
|
||||
import { WebClientConfig } from "#/api/option-service/option.types";
|
||||
|
||||
const useConfigMock = vi.fn();
|
||||
const useSettingsMock = vi.fn();
|
||||
const useActiveBackendMock = vi.fn<
|
||||
() => { backend: { kind: "local" | "cloud" }; orgId: string | null }
|
||||
>(() => ({
|
||||
@@ -16,6 +17,10 @@ vi.mock("#/hooks/query/use-config", () => ({
|
||||
useConfig: () => useConfigMock(),
|
||||
}));
|
||||
|
||||
vi.mock("#/hooks/query/use-settings", () => ({
|
||||
useSettings: () => useSettingsMock(),
|
||||
}));
|
||||
|
||||
vi.mock("#/contexts/active-backend-context", () => ({
|
||||
useActiveBackend: () => useActiveBackendMock(),
|
||||
}));
|
||||
@@ -37,9 +42,18 @@ const createConfig = (
|
||||
updated_at: new Date().toISOString(),
|
||||
});
|
||||
|
||||
const openHandsSettings = {
|
||||
agent_settings: { agent_kind: "openhands" },
|
||||
};
|
||||
|
||||
const acpClaudeCodeSettings = {
|
||||
agent_settings: { agent_kind: "acp", acp_server: "claude-code" },
|
||||
};
|
||||
|
||||
describe("useSettingsNavItems", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
useSettingsMock.mockReturnValue({ data: openHandsSettings });
|
||||
useActiveBackendMock.mockReturnValue({
|
||||
backend: { kind: "local" },
|
||||
orgId: null,
|
||||
@@ -54,10 +68,13 @@ describe("useSettingsNavItems", () => {
|
||||
(item) => item.type === "item" && item.item.to === "/settings",
|
||||
);
|
||||
|
||||
// ``OSS_NAV_ITEMS[0]`` is the Agent settings entry now; look up the
|
||||
// LLM entry by path rather than index.
|
||||
const baseLlm = OSS_NAV_ITEMS.find((item) => item.to === "/settings")!;
|
||||
expect(llmItem).toEqual({
|
||||
type: "item",
|
||||
item: {
|
||||
...OSS_NAV_ITEMS[0],
|
||||
...baseLlm,
|
||||
text: "SETTINGS$LLM_PROFILES",
|
||||
subtitle: "SETTINGS$PAGE_LLM_PROFILES_SUBLINE",
|
||||
},
|
||||
@@ -106,4 +123,70 @@ describe("useSettingsNavItems", () => {
|
||||
expect(paths).not.toContain("/settings/skills");
|
||||
expect(paths).not.toContain("/settings/mcp");
|
||||
});
|
||||
|
||||
it("disables LLM + Condenser when the active agent_kind is acp", () => {
|
||||
useConfigMock.mockReturnValue({ data: createConfig() });
|
||||
useSettingsMock.mockReturnValue({ data: acpClaudeCodeSettings });
|
||||
|
||||
const { result } = renderHook(() => useSettingsNavItems());
|
||||
const byPath = new Map(
|
||||
result.current
|
||||
.filter((item) => item.type === "item")
|
||||
.map(
|
||||
(item) =>
|
||||
[item.type === "item" ? item.item.to : "", item] as const,
|
||||
),
|
||||
);
|
||||
|
||||
const llm = byPath.get("/settings");
|
||||
expect(llm?.type).toBe("item");
|
||||
if (llm?.type === "item") {
|
||||
expect(llm.disabled).toBe(true);
|
||||
expect(llm.disabledAgentName).toBe("Claude Code");
|
||||
}
|
||||
|
||||
const condenser = byPath.get("/settings/condenser");
|
||||
expect(condenser?.type).toBe("item");
|
||||
if (condenser?.type === "item") {
|
||||
expect(condenser.disabled).toBe(true);
|
||||
}
|
||||
|
||||
// Items without `disabledByAcp` stay enabled.
|
||||
const secrets = byPath.get("/settings/secrets");
|
||||
if (secrets?.type === "item") {
|
||||
expect(secrets.disabled).toBeUndefined();
|
||||
}
|
||||
|
||||
// The agent-settings entry itself is not gated.
|
||||
const agent = byPath.get("/settings/agent");
|
||||
if (agent?.type === "item") {
|
||||
expect(agent.disabled).toBeUndefined();
|
||||
}
|
||||
});
|
||||
|
||||
it("falls back to 'ACP Agent' when the saved acp_server is unknown", () => {
|
||||
useConfigMock.mockReturnValue({ data: createConfig() });
|
||||
useSettingsMock.mockReturnValue({
|
||||
data: { agent_settings: { agent_kind: "acp", acp_server: "custom" } },
|
||||
});
|
||||
|
||||
const { result } = renderHook(() => useSettingsNavItems());
|
||||
const llm = result.current.find(
|
||||
(r) => r.type === "item" && r.item.to === "/settings",
|
||||
);
|
||||
if (llm?.type === "item") {
|
||||
expect(llm.disabledAgentName).toBe("ACP Agent");
|
||||
}
|
||||
});
|
||||
|
||||
it("leaves all items enabled when agent_kind is openhands", () => {
|
||||
useConfigMock.mockReturnValue({ data: createConfig() });
|
||||
|
||||
const { result } = renderHook(() => useSettingsNavItems());
|
||||
for (const rendered of result.current) {
|
||||
if (rendered.type === "item") {
|
||||
expect(rendered.disabled).toBeFalsy();
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,427 @@
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { render, screen, waitFor } from "@testing-library/react";
|
||||
import userEvent from "@testing-library/user-event";
|
||||
import { MemoryRouter } from "react-router";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import AgentSettingsScreen from "#/routes/agent-settings";
|
||||
import SettingsService from "#/api/settings-service/settings-service.api";
|
||||
import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers";
|
||||
import { Settings } from "#/types/settings";
|
||||
|
||||
function buildSettings(overrides: Partial<Settings> = {}): Settings {
|
||||
return {
|
||||
...MOCK_DEFAULT_USER_SETTINGS,
|
||||
...overrides,
|
||||
agent_settings:
|
||||
overrides.agent_settings ?? MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
};
|
||||
}
|
||||
|
||||
function renderAgentSettingsScreen() {
|
||||
return render(<AgentSettingsScreen />, {
|
||||
wrapper: ({ children }) => (
|
||||
<MemoryRouter>
|
||||
<QueryClientProvider
|
||||
client={
|
||||
new QueryClient({ defaultOptions: { queries: { retry: false } } })
|
||||
}
|
||||
>
|
||||
{children}
|
||||
</QueryClientProvider>
|
||||
</MemoryRouter>
|
||||
),
|
||||
});
|
||||
}
|
||||
|
||||
describe("AgentSettingsScreen", () => {
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
vi.spyOn(SettingsService, "saveSettings").mockResolvedValue(true);
|
||||
});
|
||||
|
||||
it("renders the agent type selector defaulting to OpenHands", async () => {
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
agent_kind: "openhands",
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
await screen.findByTestId("agent-settings-screen");
|
||||
expect(screen.getByTestId("agent-type-selector")).toBeInTheDocument();
|
||||
// ACP-only fields stay hidden on the OpenHands branch.
|
||||
expect(screen.queryByTestId("agent-command-input")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows the ACP form when the active agent_kind is acp", async () => {
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
acp_model: "claude-opus-4-5",
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
const commandInput = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
expect(commandInput.value).toBe(
|
||||
"npx -y @agentclientprotocol/claude-agent-acp",
|
||||
);
|
||||
const modelInput = screen.getByTestId(
|
||||
"agent-model-input",
|
||||
) as HTMLInputElement;
|
||||
expect(modelInput.value).toBe("claude-opus-4-5");
|
||||
});
|
||||
|
||||
it("saves an ACP diff when switching to ACP + Claude Code", async () => {
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
agent_kind: "openhands",
|
||||
},
|
||||
}),
|
||||
);
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
await screen.findByTestId("agent-settings-screen");
|
||||
|
||||
// Switching to ACP prefills the command from the first registered provider
|
||||
// (Claude Code).
|
||||
await user.click(screen.getByTestId("agent-type-selector"));
|
||||
await user.click(
|
||||
await screen.findByRole("option", { name: "SETTINGS$AGENT_TYPE_ACP" }),
|
||||
);
|
||||
|
||||
const commandInput = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
expect(commandInput.value).toBe(
|
||||
"npx -y @agentclientprotocol/claude-agent-acp",
|
||||
);
|
||||
|
||||
await user.click(screen.getByTestId("agent-save-button"));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff).toEqual({
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
// The default-command path stores acp_command: [] and lets the registry
|
||||
// resolve it on the agent-server side. Round-tripping verbatim would
|
||||
// pin a stale command if the registry default changes upstream.
|
||||
acp_command: [],
|
||||
// ``acp_args: []`` is reset on every save so an API-set
|
||||
// ``acp_args`` can't survive and concatenate onto the spawn
|
||||
// command at conversation-create time.
|
||||
acp_args: [],
|
||||
acp_model: null,
|
||||
});
|
||||
});
|
||||
|
||||
it("clears ACP fields when switching back to OpenHands", async () => {
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
},
|
||||
}),
|
||||
);
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
await screen.findByTestId("agent-settings-screen");
|
||||
|
||||
await user.click(screen.getByTestId("agent-type-selector"));
|
||||
await user.click(
|
||||
await screen.findByRole("option", {
|
||||
name: "SETTINGS$AGENT_TYPE_OPENHANDS",
|
||||
}),
|
||||
);
|
||||
await user.click(screen.getByTestId("agent-save-button"));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff).toEqual({ agent_kind: "openhands" });
|
||||
});
|
||||
|
||||
it("disables Save when the user has cleared the command on the ACP path", async () => {
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
const cmd = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
const save = screen.getByTestId("agent-save-button") as HTMLButtonElement;
|
||||
|
||||
// Clear the field. Save should be disabled (the agent-server would
|
||||
// crash on an empty acp_command and the adapter has no way to
|
||||
// recover — better to block the save than silently submit garbage).
|
||||
await user.clear(cmd);
|
||||
expect(save).toBeDisabled();
|
||||
});
|
||||
|
||||
it("treats whitespace-only as empty and keeps Save disabled", async () => {
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
const cmd = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
const save = screen.getByTestId("agent-save-button") as HTMLButtonElement;
|
||||
await user.clear(cmd);
|
||||
await user.type(cmd, " \t ");
|
||||
expect(save).toBeDisabled();
|
||||
});
|
||||
|
||||
it("preserves a Custom command with quoted args end-to-end", async () => {
|
||||
// Regression guard for the .split-vs-shell-quote bug: a Custom
|
||||
// command like ``bash -c "echo hi"`` used to get tokenised as
|
||||
// ``["bash","-c","\"echo","hi\""]`` and silently fail at spawn.
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
agent_kind: "openhands",
|
||||
},
|
||||
}),
|
||||
);
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
await screen.findByTestId("agent-settings-screen");
|
||||
|
||||
await user.click(screen.getByTestId("agent-type-selector"));
|
||||
await user.click(
|
||||
await screen.findByRole("option", { name: "SETTINGS$AGENT_TYPE_ACP" }),
|
||||
);
|
||||
const cmd = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
await user.clear(cmd);
|
||||
await user.type(cmd, 'bash -c "echo hi"');
|
||||
await user.click(screen.getByTestId("agent-save-button"));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff?.acp_command).toEqual([
|
||||
"bash",
|
||||
"-c",
|
||||
"echo hi",
|
||||
]);
|
||||
// Anything that diverges from a built-in default-command snaps to
|
||||
// the Custom preset.
|
||||
expect(call.agent_settings_diff?.acp_server).toBe("custom");
|
||||
});
|
||||
|
||||
it("preserves the registry default when acp_command:[] + non-empty acp_args is loaded", async () => {
|
||||
// Regression guard for the data-corruption bug:
|
||||
//
|
||||
// stored: acp_server: 'claude-code', acp_command: [], acp_args:
|
||||
// ['--extra-arg']
|
||||
// actual spawn: ['npx', '-y', '@agentclientprotocol/claude-agent-acp',
|
||||
// '--extra-arg']
|
||||
//
|
||||
// The form used to merge acp_command + acp_args literally and would
|
||||
// show only ``--extra-arg`` in the textarea. Saving then sent
|
||||
// ``acp_command: ['--extra-arg']`` and flipped the preset to
|
||||
// ``custom``, silently dropping the registry-default prefix.
|
||||
// The load path must expand the default *before* merging with args.
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
acp_command: [],
|
||||
acp_args: ["--extra-arg"],
|
||||
},
|
||||
}),
|
||||
);
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
const cmd = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
expect(cmd.value).toBe(
|
||||
"npx -y @agentclientprotocol/claude-agent-acp --extra-arg",
|
||||
);
|
||||
|
||||
// Touch the form to mark it dirty (Save is disabled until isDirty),
|
||||
// then submit. The data the form sends has to carry the registry-
|
||||
// default prefix the user can now SEE in the textarea, not the bare
|
||||
// ``--extra-arg`` that was stored.
|
||||
await user.click(cmd);
|
||||
await user.keyboard("{End} ");
|
||||
await user.keyboard("{Backspace}");
|
||||
|
||||
await user.click(screen.getByTestId("agent-save-button"));
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff?.acp_server).toBe("custom");
|
||||
expect(call.agent_settings_diff?.acp_command).toEqual([
|
||||
"npx",
|
||||
"-y",
|
||||
"@agentclientprotocol/claude-agent-acp",
|
||||
"--extra-arg",
|
||||
]);
|
||||
// ``acp_args: []`` resets the API-set args so they don't double up
|
||||
// at spawn time.
|
||||
expect(call.agent_settings_diff?.acp_args).toEqual([]);
|
||||
});
|
||||
|
||||
it("preserves an unknown loaded acp_server when the user saves without editing", async () => {
|
||||
// Data-corruption regression: a user with an ``acp_server`` value
|
||||
// canvas's registry doesn't know about (e.g. set via the API for a
|
||||
// future provider that hasn't been mirrored into ``ACP_PROVIDERS``
|
||||
// yet) opens Settings → Agent and clicks Save. Without preservation
|
||||
// the save flow demotes ``acp_server: "amp"`` → ``acp_server:
|
||||
// "custom"`` because ``detectPreset`` returns ``custom`` for any
|
||||
// unknown server. The original key name is silently lost.
|
||||
//
|
||||
// The fix is narrow: when the user hasn't touched the command since
|
||||
// load AND the loaded server is non-empty, non-``"custom"``, and
|
||||
// absent from ``ACP_PROVIDERS``, write the loaded key back verbatim
|
||||
// via the ``allowUnknownServer`` pass-through.
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "amp",
|
||||
acp_command: ["npx", "-y", "@some-future/amp-acp"],
|
||||
acp_args: [],
|
||||
},
|
||||
}),
|
||||
);
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
const cmd = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
expect(cmd.value).toBe("npx -y @some-future/amp-acp");
|
||||
|
||||
// Touch + revert the textarea to flip isDirty without changing
|
||||
// the persisted command text — matches "user opens settings and
|
||||
// hits Save without intending to change anything."
|
||||
await user.click(cmd);
|
||||
await user.keyboard("{End} ");
|
||||
await user.keyboard("{Backspace}");
|
||||
|
||||
await user.click(screen.getByTestId("agent-save-button"));
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff?.acp_server).toBe("amp");
|
||||
expect(call.agent_settings_diff?.acp_command).toEqual([
|
||||
"npx",
|
||||
"-y",
|
||||
"@some-future/amp-acp",
|
||||
]);
|
||||
});
|
||||
|
||||
it("demotes an unknown loaded acp_server to 'custom' when the user edits the command", async () => {
|
||||
// Counterpart to the preserve test: editing the command is a
|
||||
// material change of configuration, so it's correct to drop the
|
||||
// unknown ``amp`` key and fall back to ``"custom"``. The user is
|
||||
// configuring a new command, not preserving the prior one — so
|
||||
// the preset name follows the command.
|
||||
const user = userEvent.setup();
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue(
|
||||
buildSettings({
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "acp",
|
||||
acp_server: "amp",
|
||||
acp_command: ["npx", "-y", "@some-future/amp-acp"],
|
||||
acp_args: [],
|
||||
},
|
||||
}),
|
||||
);
|
||||
const save = vi.spyOn(SettingsService, "saveSettings");
|
||||
|
||||
renderAgentSettingsScreen();
|
||||
const cmd = (await screen.findByTestId(
|
||||
"agent-command-input",
|
||||
)) as HTMLTextAreaElement;
|
||||
|
||||
// Actually change the command — append a flag so the textarea
|
||||
// differs from the loaded value.
|
||||
await user.click(cmd);
|
||||
await user.keyboard("{End} --new-flag");
|
||||
|
||||
await user.click(screen.getByTestId("agent-save-button"));
|
||||
await waitFor(() => {
|
||||
expect(save).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
const call = save.mock.calls[0]?.[0] as {
|
||||
agent_settings_diff?: Record<string, unknown>;
|
||||
};
|
||||
expect(call.agent_settings_diff?.acp_server).toBe("custom");
|
||||
expect(call.agent_settings_diff?.acp_command).toEqual([
|
||||
"npx",
|
||||
"-y",
|
||||
"@some-future/amp-acp",
|
||||
"--new-flag",
|
||||
]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,68 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { clientLoader } from "#/routes/mcp";
|
||||
import SettingsService from "#/api/settings-service/settings-service.api";
|
||||
import { __resetActiveStoreForTests } from "#/api/backend-registry/active-store";
|
||||
import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers";
|
||||
import { queryClient } from "#/query-client-config";
|
||||
|
||||
describe("mcp route", () => {
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
window.localStorage.clear();
|
||||
__resetActiveStoreForTests();
|
||||
queryClient.clear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
window.localStorage.clear();
|
||||
__resetActiveStoreForTests();
|
||||
queryClient.clear();
|
||||
});
|
||||
|
||||
it("redirects to /settings/agent when the active agent is ACP", async () => {
|
||||
// The SDK's ``ACPAgent`` rejects ``mcp_config`` on init, so the
|
||||
// /mcp editor would silently no-op against the running ACP
|
||||
// subprocess. The clientLoader bounces the user to the Agent
|
||||
// settings page (same UX as /settings, /settings/condenser).
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue({
|
||||
...MOCK_DEFAULT_USER_SETTINGS,
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
},
|
||||
});
|
||||
|
||||
const response = (await clientLoader()) as Response;
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.get("Location")).toBe("/settings/agent");
|
||||
});
|
||||
|
||||
it("does not redirect when the active agent is OpenHands", async () => {
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue({
|
||||
...MOCK_DEFAULT_USER_SETTINGS,
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
agent_kind: "openhands",
|
||||
},
|
||||
});
|
||||
|
||||
const result = await clientLoader();
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
it("falls through when settings can't be fetched (no redirect-loop on errors)", async () => {
|
||||
// ``redirectIfAcpActive`` swallows errors and returns ``null`` so a
|
||||
// transient settings-fetch failure (unauthed, offline, agent-server
|
||||
// not running) doesn't trap the user on a permanent redirect.
|
||||
vi.spyOn(SettingsService, "getSettings").mockRejectedValue(
|
||||
new Error("network down"),
|
||||
);
|
||||
|
||||
const result = await clientLoader();
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -4,10 +4,13 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { createRoutesStub } from "react-router";
|
||||
import SettingsScreen, { clientLoader } from "#/routes/settings";
|
||||
import OptionService from "#/api/option-service/option-service.api";
|
||||
import SettingsService from "#/api/settings-service/settings-service.api";
|
||||
import { __resetActiveStoreForTests } from "#/api/backend-registry/active-store";
|
||||
import { getFirstAvailablePath } from "#/utils/settings-utils";
|
||||
import { OSS_NAV_ITEMS } from "#/constants/settings-nav";
|
||||
import { ActiveBackendProvider } from "#/contexts/active-backend-context";
|
||||
import { MOCK_DEFAULT_USER_SETTINGS } from "#/mocks/handlers";
|
||||
import { queryClient } from "#/query-client-config";
|
||||
|
||||
vi.mock("#/hooks/use-settings-nav-items", () => ({
|
||||
// Mirror the real navigation: LLM + Application (which the title test
|
||||
@@ -23,20 +26,37 @@ describe("settings route", () => {
|
||||
vi.restoreAllMocks();
|
||||
window.localStorage.clear();
|
||||
__resetActiveStoreForTests();
|
||||
queryClient.clear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
window.localStorage.clear();
|
||||
__resetActiveStoreForTests();
|
||||
queryClient.clear();
|
||||
});
|
||||
|
||||
it("prefers OSS fallback routes only", () => {
|
||||
it("prefers /settings/agent when LLM settings are hidden", () => {
|
||||
// /settings/agent is the ACP landing page and is always available.
|
||||
// When ``hide_llm_settings`` flips on, the user is steered there
|
||||
// rather than to ``/settings/app`` (which is a far less useful
|
||||
// landing for first-time setup).
|
||||
expect(
|
||||
getFirstAvailablePath({
|
||||
hide_llm_settings: true,
|
||||
hide_users_page: true,
|
||||
}),
|
||||
).toBe("/settings/app");
|
||||
).toBe("/settings/agent");
|
||||
});
|
||||
|
||||
it("still prefers /settings/agent even when LLM settings are visible", () => {
|
||||
// The Agent page is the single place to switch kinds, so it wins
|
||||
// the fallback unconditionally.
|
||||
expect(
|
||||
getFirstAvailablePath({
|
||||
hide_llm_settings: false,
|
||||
hide_users_page: true,
|
||||
}),
|
||||
).toBe("/settings/agent");
|
||||
});
|
||||
|
||||
it("redirects hidden OSS settings pages to the first available route", async () => {
|
||||
@@ -61,7 +81,7 @@ describe("settings route", () => {
|
||||
} as never)) as Response;
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.get("Location")).toBe("/settings/app");
|
||||
expect(response.headers.get("Location")).toBe("/settings/agent");
|
||||
});
|
||||
|
||||
it("does not redirect unrelated removed nested paths through the settings loader", async () => {
|
||||
@@ -115,4 +135,68 @@ describe("settings route", () => {
|
||||
).toBeGreaterThan(0);
|
||||
expect(screen.getByTestId("app-settings-screen")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("redirects to /settings/agent when ACP is active and the path is disabled-by-ACP", async () => {
|
||||
vi.spyOn(OptionService, "getConfig").mockResolvedValue({
|
||||
posthog_client_key: null,
|
||||
feature_flags: {
|
||||
hide_llm_settings: false,
|
||||
hide_users_page: true,
|
||||
},
|
||||
providers_configured: [],
|
||||
maintenance_start_time: null,
|
||||
recaptcha_site_key: null,
|
||||
faulty_models: [],
|
||||
error_message: null,
|
||||
updated_at: new Date().toISOString(),
|
||||
});
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue({
|
||||
...MOCK_DEFAULT_USER_SETTINGS,
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
agent_kind: "acp",
|
||||
acp_server: "claude-code",
|
||||
},
|
||||
});
|
||||
|
||||
const response = (await clientLoader({
|
||||
request: new Request("http://localhost/settings"),
|
||||
params: {},
|
||||
context: {},
|
||||
} as never)) as Response;
|
||||
|
||||
expect(response.status).toBe(302);
|
||||
expect(response.headers.get("Location")).toBe("/settings/agent");
|
||||
});
|
||||
|
||||
it("does not redirect when the active agent is OpenHands", async () => {
|
||||
vi.spyOn(OptionService, "getConfig").mockResolvedValue({
|
||||
posthog_client_key: null,
|
||||
feature_flags: {
|
||||
hide_llm_settings: false,
|
||||
hide_users_page: true,
|
||||
},
|
||||
providers_configured: [],
|
||||
maintenance_start_time: null,
|
||||
recaptcha_site_key: null,
|
||||
faulty_models: [],
|
||||
error_message: null,
|
||||
updated_at: new Date().toISOString(),
|
||||
});
|
||||
vi.spyOn(SettingsService, "getSettings").mockResolvedValue({
|
||||
...MOCK_DEFAULT_USER_SETTINGS,
|
||||
agent_settings: {
|
||||
...MOCK_DEFAULT_USER_SETTINGS.agent_settings,
|
||||
agent_kind: "openhands",
|
||||
},
|
||||
});
|
||||
|
||||
const result = await clientLoader({
|
||||
request: new Request("http://localhost/settings"),
|
||||
params: {},
|
||||
context: {},
|
||||
} as never);
|
||||
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,198 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { formatCommand, parseCommand } from "#/utils/acp-command";
|
||||
|
||||
describe("parseCommand", () => {
|
||||
it("splits a simple npx invocation into argv tokens", () => {
|
||||
expect(
|
||||
parseCommand("npx -y @agentclientprotocol/claude-agent-acp"),
|
||||
).toEqual(["npx", "-y", "@agentclientprotocol/claude-agent-acp"]);
|
||||
});
|
||||
|
||||
it("respects double-quoted segments — the headline regression .split fix", () => {
|
||||
// The old `.split(/\s+/)` implementation turned this into
|
||||
// ``["bash", "-c", "\"echo", "hello", "world\""]`` and the spawn
|
||||
// would either misbehave or fail in a confusing place. The
|
||||
// quote-aware tokenizer keeps the quoted segment intact.
|
||||
expect(parseCommand('bash -c "echo hello world"')).toEqual([
|
||||
"bash",
|
||||
"-c",
|
||||
"echo hello world",
|
||||
]);
|
||||
});
|
||||
|
||||
it("respects single-quoted segments and embedded whitespace", () => {
|
||||
expect(parseCommand("env FOO='bar baz' npx -y my-acp")).toEqual([
|
||||
"env",
|
||||
"FOO=bar baz",
|
||||
"npx",
|
||||
"-y",
|
||||
"my-acp",
|
||||
]);
|
||||
});
|
||||
|
||||
it("preserves URLs with query strings — the headline shell-quote-glob fix", () => {
|
||||
// Regression guard for the silent-corruption bug:
|
||||
//
|
||||
// node acp.js --endpoint https://example.com/acp?tenant=abc
|
||||
//
|
||||
// ``shell-quote.parse`` used to read ``?tenant=abc`` as a glob
|
||||
// pattern and drop the entire URL token, so the saved
|
||||
// ``acp_command`` became ``["node", "acp.js", "--endpoint"]``.
|
||||
// The spawn would then fail with a confusing "missing endpoint"
|
||||
// error far from the Settings → Agent page that caused it.
|
||||
//
|
||||
// The custom tokenizer treats ``?`` as a literal — same for
|
||||
// every other shell metacharacter. The URL round-trips intact.
|
||||
expect(
|
||||
parseCommand("node acp.js --endpoint https://example.com/acp?tenant=abc"),
|
||||
).toEqual([
|
||||
"node",
|
||||
"acp.js",
|
||||
"--endpoint",
|
||||
"https://example.com/acp?tenant=abc",
|
||||
]);
|
||||
});
|
||||
|
||||
it("preserves URLs with multiple query params", () => {
|
||||
// ``&`` is also literal — same reason.
|
||||
expect(parseCommand("curl https://x.com?a=1&b=2")).toEqual([
|
||||
"curl",
|
||||
"https://x.com?a=1&b=2",
|
||||
]);
|
||||
});
|
||||
|
||||
it("preserves shell metacharacters as literal argv tokens", () => {
|
||||
// Pipes, redirects, semicolons, glob chars, ``$``, backticks,
|
||||
// ``#`` all round-trip as literal characters within the surrounding
|
||||
// token. The agent-server uses ``subprocess.create_subprocess_exec``
|
||||
// (no shell intermediary), so a user typing ``foo | bar`` is
|
||||
// configuring two literal argv entries — not a shell pipeline.
|
||||
// The user's helper text steers them to ``bash -c '…'`` if they
|
||||
// actually want shell features.
|
||||
expect(parseCommand("foo | bar")).toEqual(["foo", "|", "bar"]);
|
||||
expect(parseCommand("foo > log.txt")).toEqual(["foo", ">", "log.txt"]);
|
||||
expect(parseCommand("foo *.txt")).toEqual(["foo", "*.txt"]);
|
||||
expect(parseCommand("foo $X")).toEqual(["foo", "$X"]);
|
||||
expect(parseCommand("foo `bar`")).toEqual(["foo", "`bar`"]);
|
||||
expect(parseCommand("foo # comment")).toEqual(["foo", "#", "comment"]);
|
||||
expect(parseCommand("foo && bar")).toEqual(["foo", "&&", "bar"]);
|
||||
expect(parseCommand("foo; bar")).toEqual(["foo;", "bar"]);
|
||||
});
|
||||
|
||||
it("treats blank input as an empty argv", () => {
|
||||
expect(parseCommand("")).toEqual([]);
|
||||
expect(parseCommand(" \t\n ")).toEqual([]);
|
||||
});
|
||||
|
||||
it("honors backslash escapes outside quotes", () => {
|
||||
// ``foo\ bar`` is one token containing a literal space — the same
|
||||
// contract POSIX shells provide. Lets the user type paths with
|
||||
// spaces without reaching for quotes.
|
||||
expect(parseCommand("foo\\ bar")).toEqual(["foo bar"]);
|
||||
// An escaped quote becomes a literal quote in the token.
|
||||
expect(parseCommand('foo\\"bar')).toEqual(['foo"bar']);
|
||||
});
|
||||
|
||||
it('honors ``\\\\`` and ``\\"`` inside double-quoted segments', () => {
|
||||
expect(parseCommand('bash -c "echo \\"hi\\""')).toEqual([
|
||||
"bash",
|
||||
"-c",
|
||||
'echo "hi"',
|
||||
]);
|
||||
expect(parseCommand('"foo\\\\bar"')).toEqual(["foo\\bar"]);
|
||||
});
|
||||
|
||||
it("does not env-expand $VAR refs — keeps them as literal", () => {
|
||||
// The forbidden outcome would be the tokenizer reading
|
||||
// ``process.env.ANTHROPIC_API_KEY`` and inlining its value into
|
||||
// the persisted ``acp_command`` — that would leak a host env var
|
||||
// into settings on every save. The tokenizer reads ``$NAME`` as
|
||||
// a literal substring of the token, so the user's typed text
|
||||
// survives verbatim. Users who actually want env vars in the
|
||||
// subprocess should set ``acp_env`` instead of inlining them.
|
||||
const result = parseCommand("npx $ANTHROPIC_API_KEY");
|
||||
expect(result).toEqual(["npx", "$ANTHROPIC_API_KEY"]);
|
||||
// Pin the no-leak contract: no ``sk-…`` token sneaks through
|
||||
// from the host env (which is also unset here, but still).
|
||||
expect(result.some((t) => /sk-ant-/.test(t))).toBe(false);
|
||||
});
|
||||
|
||||
it("does not run subshells: $(…) and backticks become literal tokens", () => {
|
||||
// The forbidden outcome would be executing ``date`` and inlining
|
||||
// today's timestamp into the persisted command. The tokenizer
|
||||
// never invokes anything; both forms round-trip verbatim.
|
||||
expect(parseCommand("echo $(date)")).toEqual(["echo", "$(date)"]);
|
||||
expect(parseCommand("echo `date`")).toEqual(["echo", "`date`"]);
|
||||
});
|
||||
|
||||
it("survives unterminated quotes without throwing", () => {
|
||||
// EOF closes the open quote; the partially-built token gets
|
||||
// pushed. A throw here would crash the Settings → Agent page
|
||||
// mid-render. The Save button gates on a non-empty argv anyway,
|
||||
// so a recoverable miss can't be silently persisted.
|
||||
expect(parseCommand('bash -c "unterminated')).toEqual([
|
||||
"bash",
|
||||
"-c",
|
||||
"unterminated",
|
||||
]);
|
||||
expect(parseCommand("foo 'unterminated single")).toEqual([
|
||||
"foo",
|
||||
"unterminated single",
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("formatCommand", () => {
|
||||
it("renders package-style tokens verbatim, no escaping of @ or /", () => {
|
||||
// The textarea is the only consumer of formatCommand. Escaping the
|
||||
// ``@`` in ``@org/pkg`` produces a hostile read-back (the user
|
||||
// copies their existing command, the textarea now shows
|
||||
// ``\@org/pkg``, they think we corrupted it). The agent-server
|
||||
// execs argv directly so the escape isn't load-bearing for
|
||||
// behaviour — only for display.
|
||||
expect(
|
||||
formatCommand(["npx", "-y", "@agentclientprotocol/claude-agent-acp"]),
|
||||
).toBe("npx -y @agentclientprotocol/claude-agent-acp");
|
||||
});
|
||||
|
||||
it("shell-quotes tokens that contain whitespace", () => {
|
||||
expect(formatCommand(["bash", "-c", "echo hello world"])).toBe(
|
||||
"bash -c 'echo hello world'",
|
||||
);
|
||||
});
|
||||
|
||||
it("round-trips arbitrary argv arrays through parseCommand", () => {
|
||||
const cases: string[][] = [
|
||||
["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
["npx", "-y", "@zed-industries/codex-acp"],
|
||||
["npx", "-y", "@google/gemini-cli", "--acp"],
|
||||
["bash", "-c", "echo hello world"],
|
||||
["env", "FOO=bar baz", "npx", "-y", "my-acp"],
|
||||
["./bin/my-agent", "--flag=value"],
|
||||
// URL with query string — the headline silent-corruption case.
|
||||
["node", "acp.js", "--endpoint", "https://example.com/acp?tenant=abc"],
|
||||
// URL with multiple params.
|
||||
["curl", "https://x.com?a=1&b=2"],
|
||||
// Empty-string tokens are rare but valid (some CLIs treat an
|
||||
// empty positional as "no argument supplied" rather than missing).
|
||||
// Without explicit quoting in formatCommand they round-trip back
|
||||
// as fewer tokens, silently dropping the empty slot.
|
||||
["bash", "-c", ""],
|
||||
["program", "", "--flag"],
|
||||
];
|
||||
for (const argv of cases) {
|
||||
expect(parseCommand(formatCommand(argv))).toEqual(argv);
|
||||
}
|
||||
});
|
||||
|
||||
it("renders an empty argv as an empty string", () => {
|
||||
expect(formatCommand([])).toBe("");
|
||||
});
|
||||
|
||||
it("explicitly quotes empty-string tokens so they survive the round trip", () => {
|
||||
// Direct assertion on the rendered form — without this rule,
|
||||
// formatCommand(["bash","-c",""]) would render ``"bash -c "`` and
|
||||
// parseCommand would return ``["bash", "-c"]``, losing the empty arg.
|
||||
expect(formatCommand(["bash", "-c", ""])).toBe("bash -c ''");
|
||||
});
|
||||
});
|
||||
Generated
+21
-1
@@ -17,6 +17,7 @@
|
||||
"@react-router/serve": "7.14.2",
|
||||
"@tailwindcss/vite": "4.2.4",
|
||||
"@tanstack/react-query": "5.100.9",
|
||||
"@types/shell-quote": "^1.7.5",
|
||||
"@uidotdev/usehooks": "2.4.1",
|
||||
"@xterm/addon-fit": "0.11.0",
|
||||
"@xterm/xterm": "6.0.0",
|
||||
@@ -44,6 +45,7 @@
|
||||
"rehype-sanitize": "6.0.0",
|
||||
"remark-breaks": "4.0.0",
|
||||
"remark-gfm": "4.0.1",
|
||||
"shell-quote": "^1.8.3",
|
||||
"sirv-cli": "3.0.1",
|
||||
"socket.io-client": "4.8.3",
|
||||
"tailwind-merge": "3.5.0",
|
||||
@@ -3441,7 +3443,7 @@
|
||||
"resolved": "git+https://github.com/OpenHands/typescript-client.git#ef62e82fc3dfb03991a1c8025429caf354427263",
|
||||
"license": "MIT",
|
||||
"dependencies": {
|
||||
"@openrouter/sdk": "^0.12.25",
|
||||
"@openrouter/sdk": "^0.12.35",
|
||||
"ws": "^8.20.0"
|
||||
}
|
||||
},
|
||||
@@ -7067,6 +7069,12 @@
|
||||
"@types/node": "*"
|
||||
}
|
||||
},
|
||||
"node_modules/@types/shell-quote": {
|
||||
"version": "1.7.5",
|
||||
"resolved": "https://registry.npmjs.org/@types/shell-quote/-/shell-quote-1.7.5.tgz",
|
||||
"integrity": "sha512-+UE8GAGRPbJVQDdxi16dgadcBfQ+KG2vgZhV1+3A1XmHbmwcdwhCUwIdy+d3pAGrbvgRoVSjeI9vOWyq376Yzw==",
|
||||
"license": "MIT"
|
||||
},
|
||||
"node_modules/@types/statuses": {
|
||||
"version": "2.0.6",
|
||||
"resolved": "https://registry.npmjs.org/@types/statuses/-/statuses-2.0.6.tgz",
|
||||
@@ -16329,6 +16337,18 @@
|
||||
"node": ">=8"
|
||||
}
|
||||
},
|
||||
"node_modules/shell-quote": {
|
||||
"version": "1.8.3",
|
||||
"resolved": "https://registry.npmjs.org/shell-quote/-/shell-quote-1.8.3.tgz",
|
||||
"integrity": "sha512-ObmnIF4hXNg1BqhnHmgbDETF8dLPCggZWBjkQfhZpbszZnYur5DUljTcCHii5LC3J5E0yeO/1LIMyH+UvHQgyw==",
|
||||
"license": "MIT",
|
||||
"engines": {
|
||||
"node": ">= 0.4"
|
||||
},
|
||||
"funding": {
|
||||
"url": "https://github.com/sponsors/ljharb"
|
||||
}
|
||||
},
|
||||
"node_modules/side-channel": {
|
||||
"version": "1.1.0",
|
||||
"resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.0.tgz",
|
||||
|
||||
@@ -28,6 +28,7 @@
|
||||
"@react-router/serve": "7.14.2",
|
||||
"@tailwindcss/vite": "4.2.4",
|
||||
"@tanstack/react-query": "5.100.9",
|
||||
"@types/shell-quote": "^1.7.5",
|
||||
"@uidotdev/usehooks": "2.4.1",
|
||||
"@xterm/addon-fit": "0.11.0",
|
||||
"@xterm/xterm": "6.0.0",
|
||||
@@ -55,6 +56,7 @@
|
||||
"rehype-sanitize": "6.0.0",
|
||||
"remark-breaks": "4.0.0",
|
||||
"remark-gfm": "4.0.1",
|
||||
"shell-quote": "^1.8.3",
|
||||
"sirv-cli": "3.0.1",
|
||||
"socket.io-client": "4.8.3",
|
||||
"tailwind-merge": "3.5.0",
|
||||
|
||||
@@ -328,6 +328,12 @@ function startAgentServerDocker(config) {
|
||||
const optionalMounts = [
|
||||
[join(home, ".openhands"), CONTAINER_OPENHANDS_DIR],
|
||||
[join(home, ".claude"), `${CONTAINER_HOME_DIR}/.claude`],
|
||||
// Recent Claude Code CLI versions persist auth + workspace state in
|
||||
// ``~/.claude.json`` next to (not inside) the ``~/.claude/`` directory.
|
||||
// Without this single-file mount, an ACPAgent spawned via
|
||||
// ``npx -y @zed-industries/claude-code-acp`` can't see the user's
|
||||
// existing CLI login and prompts for re-auth inside the sandbox.
|
||||
[join(home, ".claude.json"), `${CONTAINER_HOME_DIR}/.claude.json`],
|
||||
[join(home, ".codex"), `${CONTAINER_HOME_DIR}/.codex`],
|
||||
[join(home, ".ssh"), `${CONTAINER_HOME_DIR}/.ssh`],
|
||||
];
|
||||
|
||||
+234
-20
@@ -1,6 +1,7 @@
|
||||
import { DEFAULT_SETTINGS } from "#/services/settings";
|
||||
import { ExecutionStatus } from "#/types/agent-server/core";
|
||||
import { Settings, SettingsValue } from "#/types/settings";
|
||||
import { ACP_PROVIDERS } from "#/constants/acp-providers";
|
||||
import { getAgentServerClientOptions } from "./agent-server-client-options";
|
||||
import { isAgentServerToolAvailable } from "./agent-server-compatibility";
|
||||
import { getAgentServerWorkingDir } from "./agent-server-config";
|
||||
@@ -37,6 +38,16 @@ export interface DirectConversationInfo {
|
||||
} | null;
|
||||
} | null;
|
||||
agent?: {
|
||||
/**
|
||||
* Pydantic discriminator from the SDK union. ``"ACPAgent"`` means the
|
||||
* conversation runs an ACP CLI subprocess (model selection lives on
|
||||
* the subprocess via ``acp_model``, not on ``agent.llm``); ``"Agent"``
|
||||
* means the conversation drives an LLM directly through litellm.
|
||||
* Used by ``toAppConversation`` to null out ``llm_model`` for ACP
|
||||
* conversations so the chat UI doesn't expose LLM-switch affordances
|
||||
* that would silently no-op against the running ACP subprocess.
|
||||
*/
|
||||
kind?: string | null;
|
||||
llm?: {
|
||||
model?: string | null;
|
||||
} | null;
|
||||
@@ -226,6 +237,15 @@ export function toAppConversation(
|
||||
info: DirectConversationInfo,
|
||||
): AppConversation {
|
||||
const metadata = getStoredConversationMetadata(info.id);
|
||||
// ACPAgent conversations carry a dummy ``llm`` on the SDK side (the real
|
||||
// model lives on the ACP subprocess via ``acp_model``), so surfacing
|
||||
// ``agent.llm.model`` as the conversation's "active LLM" would lie to
|
||||
// every consumer downstream — most visibly the chat header's
|
||||
// SwitchProfileButton, which would otherwise let the user switch
|
||||
// profiles on a Claude-Code conversation while the running subprocess
|
||||
// keeps its own model. Null at the boundary so no consumer has to
|
||||
// re-derive the rule. Mirrors OpenHands PR #14401.
|
||||
const isAcp = info.agent?.kind === "ACPAgent";
|
||||
return {
|
||||
id: info.id,
|
||||
created_by_user_id: null,
|
||||
@@ -238,7 +258,10 @@ export function toAppConversation(
|
||||
: getDefaultConversationTitle(info.id),
|
||||
trigger: null,
|
||||
pr_number: [],
|
||||
llm_model: info.agent?.llm?.model ?? DEFAULT_SETTINGS.llm_model,
|
||||
agent_kind: isAcp ? "acp" : "openhands",
|
||||
llm_model: isAcp
|
||||
? null
|
||||
: (info.agent?.llm?.model ?? DEFAULT_SETTINGS.llm_model),
|
||||
metrics: info.metrics
|
||||
? {
|
||||
accumulated_cost: info.metrics.accumulated_cost ?? null,
|
||||
@@ -290,11 +313,54 @@ export function toConversationPage(data: {
|
||||
|
||||
type SettingsRecord = Record<string, unknown>;
|
||||
|
||||
const AGENT_SETTINGS_METADATA_KEYS = new Set([
|
||||
"schema_version",
|
||||
"agent_kind",
|
||||
"agent",
|
||||
]);
|
||||
// Keys we strip before forwarding ``agent_settings`` into the OpenHands
|
||||
// ``Agent`` payload. ``agent_kind`` is *not* in this set — it is read by
|
||||
// ``buildStartConversationRequest`` to decide whether to build an
|
||||
// ``Agent`` or an ``ACPAgent`` payload, and stripped on the LLM branch.
|
||||
const AGENT_SETTINGS_METADATA_KEYS = new Set(["schema_version", "agent"]);
|
||||
|
||||
/**
|
||||
* All ACPAgent-specific settings the adapter handles. Serves two opposite
|
||||
* roles depending on the active ``agent_kind``:
|
||||
*
|
||||
* 1. **Allow-list for the ACP branch** — ``buildConfiguredAcpAgentSettings``
|
||||
* iterates this list to decide what to forward into the ACPAgent
|
||||
* payload. Anything not in the list (``llm``, ``condenser``,
|
||||
* ``mcp_config``, ``tools``, ``agent``, …) is dropped so the
|
||||
* agent-server's pydantic model doesn't reject the create as a
|
||||
* pydantic extra.
|
||||
*
|
||||
* 2. **Deny-list for the OpenHands branch** — ``buildConfiguredAgentSettings``
|
||||
* deletes these same keys to prevent leftover ACP state (set either
|
||||
* from a previous ACP run via the UI, or via the raw API) from
|
||||
* leaking into an Agent payload where pydantic would reject them.
|
||||
*
|
||||
* That's why the list intentionally covers fields that have no UI yet
|
||||
* (``acp_args``, ``acp_env``, ``acp_session_mode``, ``acp_prompt_timeout``):
|
||||
* trimming it to UI-visible fields would solve role (1) at the cost of
|
||||
* silently leaking those API-set fields when the user toggles back to
|
||||
* an OpenHands agent. Keep aligned with the ``acp_*`` fields on
|
||||
* ``ACPAgentSettings`` in
|
||||
* ``openhands-sdk/openhands/sdk/settings/model.py`` — there is no
|
||||
* matching constant on the Python side, so this is hand-maintained
|
||||
* (drift tracked in agent-canvas#587 alongside ``ACP_PROVIDERS``).
|
||||
*/
|
||||
const ACP_SETTINGS_KEYS = [
|
||||
"acp_command",
|
||||
"acp_args",
|
||||
"acp_env",
|
||||
"acp_model",
|
||||
"acp_session_mode",
|
||||
"acp_prompt_timeout",
|
||||
] as const;
|
||||
|
||||
/**
|
||||
* Conversation-tag key under which the ACP provider key (e.g. ``"codex"``,
|
||||
* ``"claude-code"``) is stored. The agent-server validates tag keys against
|
||||
* ``^[a-z0-9]+$``, so the snake_case ``acp_server`` form is unusable —
|
||||
* keep this aligned with the validator regex.
|
||||
*/
|
||||
export const ACP_SERVER_TAG_KEY = "acpserver";
|
||||
|
||||
const CONVERSATION_SETTINGS_METADATA_KEYS = new Set([
|
||||
"schema_version",
|
||||
@@ -455,6 +521,13 @@ function buildConfiguredAgentSettings(settings: Settings): SettingsRecord {
|
||||
|
||||
AGENT_SETTINGS_METADATA_KEYS.forEach((key) => delete agentSettings[key]);
|
||||
delete agentSettings.enable_switch_llm_tool;
|
||||
// Drop fields that only apply to the ACP path; do not let them leak into
|
||||
// an OpenHands Agent payload where pydantic would reject extras.
|
||||
delete agentSettings.agent_kind;
|
||||
delete agentSettings.acp_server;
|
||||
for (const key of ACP_SETTINGS_KEYS) {
|
||||
delete agentSettings[key];
|
||||
}
|
||||
|
||||
const mcpConfig = toRecord(agentSettings.mcp_config);
|
||||
if (Object.keys(mcpConfig).length === 0 || !("mcpServers" in mcpConfig)) {
|
||||
@@ -475,25 +548,117 @@ function buildConfiguredAgentSettings(settings: Settings): SettingsRecord {
|
||||
};
|
||||
}
|
||||
|
||||
function createAgentFromSettings(agentSettings: SettingsRecord) {
|
||||
function buildConfiguredAcpAgentSettings(settings: Settings): SettingsRecord {
|
||||
const agentSettings = toRecord(settings.agent_settings);
|
||||
|
||||
// Only forward fields the ACPAgent model knows about. Everything else
|
||||
// (``llm``, ``condenser``, ``mcp_config``, ``agent``, ``schema_version``,
|
||||
// ``tools``, ``agent_kind``) is irrelevant on this path; the agent-server
|
||||
// would either ignore it or reject it as a pydantic extra. ``acp_server``
|
||||
// is a UI bookkeeping field — it does not belong in the agent payload
|
||||
// either, but we surface it on the conversation tags instead (see
|
||||
// ``buildStartConversationRequest``).
|
||||
const payload: SettingsRecord = {};
|
||||
for (const key of ACP_SETTINGS_KEYS) {
|
||||
if (agentSettings[key] !== undefined && agentSettings[key] !== null) {
|
||||
payload[key] = agentSettings[key];
|
||||
}
|
||||
}
|
||||
|
||||
// The Settings → Agent page (and onboarding) stores ``acp_command: []``
|
||||
// for the "default preset" path, expecting the registry to resolve it.
|
||||
// The agent-server's ACPAgent model takes only an explicit ``acp_command``
|
||||
// though — empty list means ``subprocess(command[0], ...)`` raises
|
||||
// ``IndexError: list index out of range`` at spawn time, the agent loop
|
||||
// dies silently, and the conversation hangs in ``idle``. Resolve the
|
||||
// command from ``ACP_PROVIDERS`` here so the agent-server sees a real
|
||||
// command for every built-in preset, while leaving ``acp_server: custom``
|
||||
// (and any unknown key) untouched — those genuinely require the user's
|
||||
// ``acp_command`` entry.
|
||||
const cmd = payload.acp_command;
|
||||
const isEmpty = Array.isArray(cmd) && cmd.length === 0;
|
||||
const noCommand = cmd === undefined;
|
||||
if (isEmpty || noCommand) {
|
||||
const serverKey =
|
||||
typeof agentSettings.acp_server === "string"
|
||||
? agentSettings.acp_server
|
||||
: undefined;
|
||||
const provider = ACP_PROVIDERS.find(({ key }) => key === serverKey);
|
||||
if (provider) {
|
||||
payload.acp_command = [...provider.default_command];
|
||||
}
|
||||
}
|
||||
|
||||
return payload;
|
||||
}
|
||||
|
||||
function createAgentFromSettings(
|
||||
agentSettings: SettingsRecord,
|
||||
options: { acp?: boolean } = {},
|
||||
) {
|
||||
const runtimeServicesSuffix = buildRuntimeServicesSystemSuffix();
|
||||
// ``load_public_skills``, ``load_user_skills``, and
|
||||
// ``system_message_suffix`` are all marked ``acp_compatible: true`` in
|
||||
// the SDK's AgentContext model — they're rendered into the system
|
||||
// prompt the ACP CLI receives via ``ACPAgent._render_suffix``, so
|
||||
// building the same agent_context here means a Claude-Code / Codex
|
||||
// user gets the same skill catalog and runtime-services awareness an
|
||||
// OpenHands-driven conversation does. Leaving it off (the previous
|
||||
// ACP branch returned ``{kind:"ACPAgent",...agentSettings}`` with no
|
||||
// ``agent_context``) silently dropped both, matching neither the
|
||||
// SDK contract nor OpenHands' own behaviour.
|
||||
//
|
||||
// The ``acp_compatible`` markers live on the SDK fields themselves
|
||||
// in ``openhands-sdk/openhands/sdk/context/agent_context.py``:
|
||||
// - ``system_message_suffix`` (Field, json_schema_extra at L66)
|
||||
// - ``load_user_skills`` (Field, json_schema_extra at L80)
|
||||
// - ``load_public_skills`` (Field, json_schema_extra at L89)
|
||||
// ``AgentContext.validate_acp_compatibility`` rejects any field not
|
||||
// tagged that way at ``ACPAgent`` init time. If a future SDK bump
|
||||
// demotes one of these (drops the marker), the ACP conversation start
|
||||
// will 422 here — at which point the right move is to drop the
|
||||
// demoted field from this dict, not to wrap a workaround.
|
||||
//
|
||||
// ``secrets`` is filled in later by the secret bridge in
|
||||
// ``buildStartConversationRequest`` (when ``customSecrets`` is set);
|
||||
// we don't seed it here so non-secret start paths don't end up with
|
||||
// an empty ``secrets: {}`` map.
|
||||
const agentContext: Record<string, unknown> = {
|
||||
load_public_skills: true,
|
||||
load_user_skills: true,
|
||||
// When the dev launcher provided ``VITE_RUNTIME_SERVICES_INFO``,
|
||||
// append a <RUNTIME_SERVICES> block to the system prompt so the
|
||||
// agent knows which services exist in this dev stack (e.g.
|
||||
// automation backend URL, ingress URL) instead of having to probe.
|
||||
...(runtimeServicesSuffix
|
||||
? { system_message_suffix: runtimeServicesSuffix }
|
||||
: {}),
|
||||
};
|
||||
if (options.acp) {
|
||||
return {
|
||||
kind: "ACPAgent",
|
||||
...agentSettings,
|
||||
agent_context: agentContext,
|
||||
};
|
||||
}
|
||||
return {
|
||||
kind: "Agent",
|
||||
...agentSettings,
|
||||
agent_context: {
|
||||
load_public_skills: true,
|
||||
load_user_skills: true,
|
||||
// When the dev launcher provided `VITE_RUNTIME_SERVICES_INFO`, append
|
||||
// a <RUNTIME_SERVICES> block to the system prompt so the agent knows
|
||||
// which services exist in this dev stack (e.g. automation backend
|
||||
// URL, ingress URL) instead of having to probe.
|
||||
...(runtimeServicesSuffix
|
||||
? { system_message_suffix: runtimeServicesSuffix }
|
||||
: {}),
|
||||
},
|
||||
agent_context: agentContext,
|
||||
};
|
||||
}
|
||||
|
||||
function isAcpAgent(settings: Settings): boolean {
|
||||
const agentSettings = toRecord(settings.agent_settings);
|
||||
return agentSettings.agent_kind === "acp";
|
||||
}
|
||||
|
||||
function getAcpServerTag(settings: Settings): string | undefined {
|
||||
const agentSettings = toRecord(settings.agent_settings);
|
||||
const value = agentSettings.acp_server;
|
||||
return typeof value === "string" && value.length > 0 ? value : undefined;
|
||||
}
|
||||
|
||||
function buildConfiguredConversationSettings(options: {
|
||||
settings: Settings;
|
||||
query?: string;
|
||||
@@ -579,8 +744,14 @@ export function buildStartConversationRequest(
|
||||
? { ...options.settings, agent_settings: options.encryptedAgentSettings }
|
||||
: options.settings;
|
||||
|
||||
const agentSettings = buildConfiguredAgentSettings(sourceAgentSettings);
|
||||
const agent = createAgentFromSettings(agentSettings);
|
||||
const acpMode = isAcpAgent(sourceAgentSettings);
|
||||
const agentSettings = acpMode
|
||||
? buildConfiguredAcpAgentSettings(sourceAgentSettings)
|
||||
: buildConfiguredAgentSettings(sourceAgentSettings);
|
||||
const agent = createAgentFromSettings(agentSettings, { acp: acpMode });
|
||||
const acpServerTag = acpMode
|
||||
? getAcpServerTag(sourceAgentSettings)
|
||||
: undefined;
|
||||
|
||||
// For conversation settings, merge encrypted settings if provided
|
||||
const sourceConversationOptions = options.encryptedConversationSettings
|
||||
@@ -611,6 +782,18 @@ export function buildStartConversationRequest(
|
||||
worktree: true,
|
||||
};
|
||||
|
||||
// Stamp the ACP provider key onto the conversation so the chip can render
|
||||
// a brand name from a single source of truth. The tag is purely
|
||||
// informational — frontend looks it up against ``ACP_PROVIDERS``; the
|
||||
// agent-server treats it as an opaque string.
|
||||
//
|
||||
// Tag *keys* must match ``^[a-z0-9]+$`` per agent-server validation —
|
||||
// ``acp_server`` would be rejected with a 422. ``acpserver`` flattens
|
||||
// the snake_case original into the allowed shape.
|
||||
if (acpServerTag) {
|
||||
payload.tags = { [ACP_SERVER_TAG_KEY]: acpServerTag };
|
||||
}
|
||||
|
||||
// Add secrets_encrypted flag if secrets are encrypted
|
||||
if (options.secretsEncrypted) {
|
||||
payload.secrets_encrypted = true;
|
||||
@@ -677,6 +860,37 @@ export function buildStartConversationRequest(
|
||||
}
|
||||
|
||||
payload.secrets = secrets;
|
||||
|
||||
// ACPAgent bridge: mirror the same secrets onto
|
||||
// ``agent.agent_context.secrets`` so the agent-server's existing
|
||||
// ``ACPAgent._start_acp_server`` env-injection loop picks them up
|
||||
// and writes them into the ACP subprocess environment. Without
|
||||
// this, the OpenHands ``Conversation.update_secrets`` path
|
||||
// populates ``secret_registry`` — which the LLM-driven Agent
|
||||
// reads, but the ACP subprocess never sees, so secrets set in
|
||||
// Settings → Secrets (e.g. ``ANTHROPIC_API_KEY``) silently fail
|
||||
// to reach the ACP CLI.
|
||||
//
|
||||
// Mirrors what OpenHands' app-server does in
|
||||
// ``_build_acp_start_conversation_request``: wraps the conversation
|
||||
// secrets in an ``AgentContext(secrets=secrets)`` before constructing
|
||||
// the ACPAgent. This shim becomes redundant once canvas pins to an
|
||||
// agent-server build that includes software-agent-sdk PR #3299
|
||||
// (which makes ``ACPAgent`` read ``state.secret_registry`` itself);
|
||||
// at that point this block can be deleted with no behaviour change.
|
||||
//
|
||||
// Merge into the existing ``agent_context`` (``createAgentFromSettings``
|
||||
// seeds ``load_public_skills`` / ``load_user_skills`` / optionally a
|
||||
// ``system_message_suffix`` from the dev launcher's runtime-services
|
||||
// info — all marked ``acp_compatible: true`` in the SDK). Overwriting
|
||||
// would drop those.
|
||||
if (acpMode) {
|
||||
const agentRecord = payload.agent as Record<string, unknown>;
|
||||
const existingContext =
|
||||
(agentRecord.agent_context as Record<string, unknown> | undefined) ??
|
||||
{};
|
||||
agentRecord.agent_context = { ...existingContext, secrets };
|
||||
}
|
||||
}
|
||||
|
||||
return payload;
|
||||
|
||||
@@ -122,6 +122,15 @@ export interface AppConversation {
|
||||
title: string | null;
|
||||
trigger: ConversationTrigger | null;
|
||||
pr_number: number[];
|
||||
/**
|
||||
* High-level kind of the conversation's agent — ``"openhands"`` for an LLM-
|
||||
* driven Agent, ``"acp"`` for an ACPAgent that delegates to an external
|
||||
* ACP CLI subprocess. Consumers can use this to gate UI affordances that
|
||||
* only make sense for one kind (e.g. the LLM-profile switcher in the chat
|
||||
* header is a no-op for ACP conversations because model selection lives
|
||||
* on the subprocess via ``acp_model``, not on ``llm_model``).
|
||||
*/
|
||||
agent_kind?: "openhands" | "acp" | null;
|
||||
llm_model: string | null;
|
||||
metrics: MetricsSnapshot | null;
|
||||
created_at: string;
|
||||
|
||||
+45
@@ -21,6 +21,51 @@ export const getACPToolCallTitleKey = (event: ACPToolCallEvent): string => {
|
||||
}
|
||||
};
|
||||
|
||||
// English verb prefixes ACP servers sometimes inline into the title.
|
||||
// Claude Code emits ``"Read /Users/foo/bar"`` for a read tool — combined
|
||||
// with the i18n template ``"Reading <cmd>{{title}}</cmd>"`` that lands as
|
||||
// ``"Reading Read /Users/foo/bar"``. The redundant leading verb is the
|
||||
// part we strip; the template's own verb (which is i18n'd) stays.
|
||||
//
|
||||
// Keyed by ``tool_kind`` so the strip is scoped to where double-verbing
|
||||
// actually shows up. The english-only check is intentional — ACP servers
|
||||
// are anglophone tools and emit english titles regardless of the user's
|
||||
// canvas locale; matching translated verbs would mean every locale's
|
||||
// strip list goes stale the moment a new server is added.
|
||||
const REDUNDANT_TITLE_PREFIXES: Partial<Record<string, readonly string[]>> = {
|
||||
read: ["Read"],
|
||||
edit: ["Edit", "Write"],
|
||||
execute: ["Bash", "Run"],
|
||||
fetch: ["Fetch", "WebFetch"],
|
||||
};
|
||||
|
||||
/**
|
||||
* Strip a leading verb from ``event.title`` that would duplicate the
|
||||
* verb baked into the i18n template (see ``REDUNDANT_TITLE_PREFIXES``).
|
||||
*
|
||||
* The match is anchored, case-sensitive, and requires the prefix to be
|
||||
* followed by whitespace so a token like ``"Reads"`` (an actual verb
|
||||
* elsewhere in the title) is left alone. If no prefix matches, the title
|
||||
* is returned verbatim.
|
||||
*/
|
||||
export const stripRedundantTitlePrefix = (event: ACPToolCallEvent): string => {
|
||||
const title = event.title;
|
||||
const tool_kind = event.tool_kind;
|
||||
if (!title || !tool_kind) return title;
|
||||
const prefixes = REDUNDANT_TITLE_PREFIXES[tool_kind];
|
||||
if (!prefixes) return title;
|
||||
for (const prefix of prefixes) {
|
||||
if (
|
||||
title.length > prefix.length &&
|
||||
title.startsWith(prefix) &&
|
||||
/\s/.test(title.charAt(prefix.length))
|
||||
) {
|
||||
return title.slice(prefix.length).trimStart();
|
||||
}
|
||||
}
|
||||
return title;
|
||||
};
|
||||
|
||||
/**
|
||||
* Stringify an arbitrary raw_input / raw_output payload for markdown
|
||||
* rendering. Strings pass through; objects are pretty-printed JSON.
|
||||
|
||||
@@ -17,6 +17,7 @@ import { getObservationContent } from "./get-observation-content";
|
||||
import {
|
||||
getACPToolCallContent,
|
||||
getACPToolCallTitleKey,
|
||||
stripRedundantTitlePrefix,
|
||||
} from "./get-acp-tool-call-content";
|
||||
import { TaskTrackingObservationContent } from "../task-tracking/task-tracking-observation-content";
|
||||
import { TaskTrackerObservation } from "#/types/agent-server/core/base/observation";
|
||||
@@ -309,7 +310,11 @@ export const getEventContent = (
|
||||
// raw_input + raw_output the same way getTerminalObservationContent
|
||||
// builds "Command: / Output:" blocks.
|
||||
title = createTitleFromKey(getACPToolCallTitleKey(event), {
|
||||
title: event.title,
|
||||
// Strip a redundant verb prefix the ACP server may have inlined
|
||||
// (Claude Code emits ``"Read /path"`` for a read tool; combined
|
||||
// with the ``"Reading <cmd>{{title}}</cmd>"`` template that lands
|
||||
// as ``"Reading Read /path"``). See ``stripRedundantTitlePrefix``.
|
||||
title: stripRedundantTitlePrefix(event),
|
||||
});
|
||||
details = getACPToolCallContent(event);
|
||||
} else if (
|
||||
|
||||
+13
-3
@@ -57,10 +57,20 @@ export const shouldRenderEvent = (event: OpenHandsEvent) => {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Render ACP sub-agent tool call events — suppress in_progress (empty args)
|
||||
// so the card only appears once fully populated.
|
||||
// Render ACP sub-agent tool call events only once they've reached a
|
||||
// terminal status. ACP servers stream multiple events per
|
||||
// ``tool_call_id`` as the call progresses (status flips
|
||||
// ``in_progress`` → ``completed`` / ``failed``); during streaming the
|
||||
// event's ``raw_input`` / ``raw_output`` / ``title`` may still be
|
||||
// partially populated, so rendering an in-flight event flashes a
|
||||
// half-formed card that then updates in place — visibly noisy.
|
||||
// ``null`` (older agent-server builds, before the field was required)
|
||||
// is also treated as in-flight: better to wait for the terminal event
|
||||
// than to render a card with no status. ``handleEventForUI`` already
|
||||
// replaces in place by ``tool_call_id``, so the terminal event lands
|
||||
// at the original position once it arrives.
|
||||
if (isACPToolCallEvent(event)) {
|
||||
return event.status !== "in_progress";
|
||||
return event.status === "completed" || event.status === "failed";
|
||||
}
|
||||
|
||||
// Don't render any other event types (system events, etc.)
|
||||
|
||||
@@ -28,7 +28,24 @@ export function ChatInputModel() {
|
||||
// Home page has no active conversation; fall back to the user's default
|
||||
// model so the switcher renders consistently across both surfaces.
|
||||
const { data: settings } = useSettings();
|
||||
const llmModel = conversation?.llm_model ?? settings?.llm_model;
|
||||
// ACPAgent conversations have no OpenHands LLM (the model lives on the
|
||||
// ACP subprocess via ``acp_model``), so ``toAppConversation`` writes a
|
||||
// null ``llm_model`` for them. Don't fall back to ``settings.llm_model``
|
||||
// here — that would resurrect the user's *default* OpenHands model on a
|
||||
// Claude-Code conversation and link to /settings, both of which lie
|
||||
// about what model is actually running.
|
||||
//
|
||||
// On the home screen ``conversation`` is undefined, so we also have to
|
||||
// consult ``settings.agent_settings.agent_kind`` — that's the kind the
|
||||
// next-created conversation will inherit. Without the fallback, ACP
|
||||
// users would still see the LLM-profile control on the home page,
|
||||
// contradicting the ACP nav gating elsewhere.
|
||||
const isAcpActive =
|
||||
conversation?.agent_kind === "acp" ||
|
||||
(!conversation && settings?.agent_settings?.agent_kind === "acp");
|
||||
const llmModel = isAcpActive
|
||||
? null
|
||||
: (conversation?.llm_model ?? settings?.llm_model);
|
||||
const llmDestinationLabel = t(
|
||||
backend.kind === "cloud"
|
||||
? I18nKey.SETTINGS$LLM_SETTINGS
|
||||
|
||||
@@ -5,6 +5,7 @@ import ChevronDownSmallIcon from "#/icons/chevron-down-small.svg?react";
|
||||
import { useLlmProfiles } from "#/hooks/query/use-llm-profiles";
|
||||
import { useSwitchLlmProfileAndLog } from "#/hooks/mutation/use-switch-llm-profile-and-log";
|
||||
import { useActiveConversation } from "#/hooks/query/use-active-conversation";
|
||||
import { useSettings } from "#/hooks/query/use-settings";
|
||||
import { useOptionalConversationId } from "#/hooks/use-conversation-id";
|
||||
import { useModelStore } from "#/stores/model-store";
|
||||
import { cn } from "#/utils/utils";
|
||||
@@ -18,6 +19,7 @@ export function SwitchProfileButton() {
|
||||
const { conversationId } = useOptionalConversationId();
|
||||
const { data } = useLlmProfiles();
|
||||
const { data: conversation } = useActiveConversation();
|
||||
const { data: settings } = useSettings();
|
||||
const { switchAndLog, isPending } = useSwitchLlmProfileAndLog();
|
||||
// Optimistic value written by recordSwitch on a successful switch — gives
|
||||
// instant in-conversation feedback before the conversation refetch lands
|
||||
@@ -28,6 +30,22 @@ export function SwitchProfileButton() {
|
||||
|
||||
const profiles = data?.profiles ?? [];
|
||||
const conversationModel = conversation?.llm_model ?? null;
|
||||
// ACPAgent conversations route prompts to a CLI subprocess whose model is
|
||||
// controlled by ``acp_model`` (set in Settings → Agent), not by the LLM
|
||||
// profile picker. Surfacing the switcher here would let the user "change
|
||||
// the model" while the running subprocess silently keeps its own — a
|
||||
// confusing no-op. Hide the button instead. ``toAppConversation`` also
|
||||
// nulls ``llm_model`` on this boundary so any other consumer that reads
|
||||
// the model directly sees "no model" rather than a misleading value.
|
||||
//
|
||||
// On the home screen ``conversation`` is undefined; fall back to
|
||||
// ``settings.agent_settings.agent_kind`` so the picker also hides when
|
||||
// ACP is the *default* the next-created conversation would inherit.
|
||||
// Otherwise an ACP user lands on a home page with an LLM-switch
|
||||
// control that contradicts the ACP nav gating everywhere else.
|
||||
const isAcpActive =
|
||||
conversation?.agent_kind === "acp" ||
|
||||
(!conversation && settings?.agent_settings?.agent_kind === "acp");
|
||||
|
||||
// Resolution priority for the active profile name:
|
||||
// 1. Optimistic (just-clicked) — instant feedback before the refetch.
|
||||
@@ -44,7 +62,7 @@ export function SwitchProfileButton() {
|
||||
conversationModel ??
|
||||
null;
|
||||
|
||||
if (profiles.length === 0) {
|
||||
if (profiles.length === 0 || isAcpActive) {
|
||||
return null;
|
||||
}
|
||||
|
||||
|
||||
@@ -82,15 +82,40 @@ export function OnboardingModal({ onClose }: OnboardingModalProps) {
|
||||
const [selectedAgentId, setSelectedAgentId] =
|
||||
React.useState<OnboardingAgentId>("openhands");
|
||||
|
||||
// The LLM-setup step (index 2) is OpenHands-specific: ACP agents drive
|
||||
// their own LLM via the subprocess and authenticate through the Secrets
|
||||
// panel, so there's nothing to configure in that form for them. Skip
|
||||
// over it in both directions when the user has picked an ACP agent,
|
||||
// keeping the rest of the flow intact (back from SayHello on the ACP
|
||||
// path returns to ChooseAgent, not to a dead-end LLM page).
|
||||
const skipLlmStep = selectedAgentId !== "openhands";
|
||||
const goNext = React.useCallback(
|
||||
() => setCurrentStep((step) => (step >= TOTAL_STEPS - 1 ? step : step + 1)),
|
||||
[],
|
||||
() =>
|
||||
setCurrentStep((step) => {
|
||||
const delta = skipLlmStep && step === 1 ? 2 : 1;
|
||||
return Math.min(step + delta, TOTAL_STEPS - 1);
|
||||
}),
|
||||
[skipLlmStep],
|
||||
);
|
||||
const goBack = React.useCallback(
|
||||
() => setCurrentStep((step) => (step <= 0 ? 0 : step - 1)),
|
||||
[],
|
||||
() =>
|
||||
setCurrentStep((step) => {
|
||||
const delta = skipLlmStep && step === 3 ? 2 : 1;
|
||||
return Math.max(step - delta, 0);
|
||||
}),
|
||||
[skipLlmStep],
|
||||
);
|
||||
|
||||
// The progress bar should show the user's actual visited-step count,
|
||||
// not the underlying index. On the ACP path the LLM-setup slide is
|
||||
// skipped, so:
|
||||
// * the bar renders 3 segments instead of 4, and
|
||||
// * the SayHello slide (modal index 3) maps to logical step 2 so
|
||||
// segment 2 doesn't pop "completed" on a slide the user never saw.
|
||||
const progressTotal = skipLlmStep ? TOTAL_STEPS - 1 : TOTAL_STEPS;
|
||||
const progressStep =
|
||||
skipLlmStep && currentStep > 1 ? currentStep - 1 : currentStep;
|
||||
|
||||
return (
|
||||
<ModalBackdrop
|
||||
onClose={onClose}
|
||||
@@ -108,8 +133,8 @@ export function OnboardingModal({ onClose }: OnboardingModalProps) {
|
||||
>
|
||||
<header className="flex flex-col gap-3 px-7 pt-7 shrink-0">
|
||||
<OnboardingProgressBar
|
||||
currentStep={currentStep}
|
||||
totalSteps={TOTAL_STEPS}
|
||||
currentStep={progressStep}
|
||||
totalSteps={progressTotal}
|
||||
/>
|
||||
</header>
|
||||
|
||||
|
||||
@@ -1,12 +1,27 @@
|
||||
import React from "react";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { AxiosError } from "axios";
|
||||
import { Check } from "lucide-react";
|
||||
import OpenHandsLogo from "#/assets/branding/openhands-logo.svg?react";
|
||||
import { BrandButton } from "#/components/features/settings/brand-button";
|
||||
import { I18nKey } from "#/i18n/declaration";
|
||||
import { cn } from "#/utils/utils";
|
||||
import { useSaveSettings } from "#/hooks/mutation/use-save-settings";
|
||||
import {
|
||||
ACP_PROVIDERS,
|
||||
buildAcpAgentSettingsDiff,
|
||||
} from "#/constants/acp-providers";
|
||||
import {
|
||||
displayErrorToast,
|
||||
displaySuccessToast,
|
||||
} from "#/utils/custom-toast-handlers";
|
||||
import { retrieveAxiosErrorMessage } from "#/utils/retrieve-axios-error-message";
|
||||
|
||||
export type OnboardingAgentId = "openhands" | "claude-code" | "codex";
|
||||
export type OnboardingAgentId =
|
||||
| "openhands"
|
||||
| "claude-code"
|
||||
| "codex"
|
||||
| "gemini-cli";
|
||||
|
||||
const CLAUDE_CODE_MARK_PATH =
|
||||
"m19.6 66.5 19.7-11 .3-1-.3-.5h-1l-3.3-.2-11.2-.3L14 53l-9.5-.5-2.4-.5L0 49l.2-1.5 2-1.3 2.9.2 6.3.5 9.5.6 6.9.4L38 49.1h1.6l.2-.7-.5-.4-.4-.4L29 41l-10.6-7-5.6-4.1-3-2-1.5-2-.6-4.2 2.7-3 3.7.3.9.2 3.7 2.9 8 6.1L37 36l1.5 1.2.6-.4.1-.3-.7-1.1L33 25l-6-10.4-2.7-4.3-.7-2.6c-.3-1-.4-2-.4-3l3-4.2L28 0l4.2.6L33.8 2l2.6 6 4.1 9.3L47 29.9l2 3.8 1 3.4.3 1h.7v-.5l.5-7.2 1-8.7 1-11.2.3-3.2 1.6-3.8 3-2L61 2.6l2 2.9-.3 1.8-1.1 7.7L59 27.1l-1.5 8.2h.9l1-1.1 4.1-5.4 6.9-8.6 3-3.5L77 13l2.3-1.8h4.3l3.1 4.7-1.4 4.9-4.4 5.6-3.7 4.7-5.3 7.1-3.2 5.7.3.4h.7l12-2.6 6.4-1.1 7.6-1.3 3.5 1.6.4 1.6-1.4 3.4-8.2 2-9.6 2-14.3 3.3-.2.1.2.3 6.4.6 2.8.2h6.8l12.6 1 3.3 2 1.9 2.7-.3 2-5.1 2.6-6.8-1.6-16-3.8-5.4-1.3h-.8v.4l4.6 4.5 8.3 7.5L89 80.1l.5 2.4-1.3 2-1.4-.2-9.2-7-3.6-3-8-6.8h-.5v.7l1.8 2.7 9.8 14.7.5 4.5-.7 1.4-2.6 1-2.7-.6-5.8-8-6-9-4.7-8.2-.5.4-2.9 30.2-1.3 1.5-3 1.2-2.5-2-1.4-3 1.4-6.2 1.6-8 1.3-6.4 1.2-7.9.7-2.6v-.2H49L43 72l-9 12.3-7.2 7.6-1.7.7-3-1.5.3-2.8L24 86l10-12.8 6-7.9 4-4.6-.1-.5h-.3L17.2 77.4l-4.7.6-2-2 .2-3 1-1 8-5.5Z";
|
||||
@@ -69,28 +84,23 @@ interface AgentOption {
|
||||
id: OnboardingAgentId;
|
||||
label: string;
|
||||
descriptionKey: I18nKey;
|
||||
enabled: boolean;
|
||||
}
|
||||
|
||||
// Onboarding tile list is *derived* from the ACP registry so adding a
|
||||
// new provider (or changing a display name) only needs one edit in
|
||||
// ``acp-providers.ts``. The OpenHands tile is the only synthetic
|
||||
// entry — it isn't an ACP provider, just the canonical default.
|
||||
const AGENT_OPTIONS: AgentOption[] = [
|
||||
{
|
||||
id: "openhands",
|
||||
label: "OpenHands",
|
||||
descriptionKey: I18nKey.ONBOARDING$AGENT_OPENHANDS_DESCRIPTION,
|
||||
enabled: true,
|
||||
},
|
||||
{
|
||||
id: "claude-code",
|
||||
label: "Claude Code",
|
||||
descriptionKey: I18nKey.ONBOARDING$AGENT_CLAUDE_CODE_DESCRIPTION,
|
||||
enabled: false,
|
||||
},
|
||||
{
|
||||
id: "codex",
|
||||
label: "Codex",
|
||||
descriptionKey: I18nKey.ONBOARDING$AGENT_CODEX_DESCRIPTION,
|
||||
enabled: false,
|
||||
},
|
||||
...ACP_PROVIDERS.map<AgentOption>((provider) => ({
|
||||
id: provider.key as OnboardingAgentId,
|
||||
label: provider.display_name,
|
||||
descriptionKey: provider.description_key,
|
||||
})),
|
||||
];
|
||||
|
||||
interface ChooseAgentStepProps {
|
||||
@@ -105,6 +115,32 @@ export function ChooseAgentStep({
|
||||
onNext,
|
||||
}: ChooseAgentStepProps) {
|
||||
const { t } = useTranslation("openhands");
|
||||
const { mutate: saveSettings, isPending: isSaving } = useSaveSettings();
|
||||
|
||||
const handleNext = () => {
|
||||
const diff = buildAcpAgentSettingsDiff(selectedAgentId);
|
||||
if (!diff) {
|
||||
// Unknown id (shouldn't be reachable through the UI). Advance
|
||||
// without writing — better to show the next step than block the
|
||||
// user behind a silent no-op.
|
||||
onNext();
|
||||
return;
|
||||
}
|
||||
|
||||
saveSettings(
|
||||
{ agent_settings_diff: diff },
|
||||
{
|
||||
onError: (error) => {
|
||||
const message = retrieveAxiosErrorMessage(error as AxiosError);
|
||||
displayErrorToast(message || t(I18nKey.ERROR$GENERIC));
|
||||
},
|
||||
onSuccess: () => {
|
||||
displaySuccessToast(t(I18nKey.SETTINGS$SAVED));
|
||||
onNext();
|
||||
},
|
||||
},
|
||||
);
|
||||
};
|
||||
|
||||
return (
|
||||
<div
|
||||
@@ -127,34 +163,25 @@ export function ChooseAgentStep({
|
||||
>
|
||||
{AGENT_OPTIONS.map((option) => {
|
||||
const isSelected = option.id === selectedAgentId;
|
||||
const muted = !option.enabled;
|
||||
return (
|
||||
<button
|
||||
key={option.id}
|
||||
type="button"
|
||||
role="radio"
|
||||
aria-checked={isSelected}
|
||||
aria-disabled={!option.enabled}
|
||||
disabled={!option.enabled}
|
||||
data-testid={`onboarding-agent-option-${option.id}`}
|
||||
data-selected={isSelected}
|
||||
onClick={() => option.enabled && onSelect(option.id)}
|
||||
onClick={() => onSelect(option.id)}
|
||||
className={cn(
|
||||
"flex items-start justify-between gap-4 rounded-xl border px-4 py-3 text-left transition-colors",
|
||||
option.enabled
|
||||
? "cursor-pointer"
|
||||
: "cursor-not-allowed border-white/10 bg-base-secondary",
|
||||
option.enabled &&
|
||||
isSelected &&
|
||||
"border-white/45 bg-white/[0.09] shadow-none hover:border-white/45 hover:bg-white/[0.09]",
|
||||
option.enabled &&
|
||||
!isSelected &&
|
||||
"border-white/30 bg-white/5 hover:border-white/40 hover:bg-white/[0.08]",
|
||||
"flex items-start justify-between gap-4 rounded-xl border px-4 py-3 text-left transition-colors cursor-pointer",
|
||||
isSelected
|
||||
? "border-white/45 bg-white/[0.09] shadow-none hover:border-white/45 hover:bg-white/[0.09]"
|
||||
: "border-white/30 bg-white/5 hover:border-white/40 hover:bg-white/[0.08]",
|
||||
)}
|
||||
>
|
||||
<div className="flex min-w-0 flex-1 flex-col gap-1">
|
||||
<div className="flex min-w-0 items-center gap-2">
|
||||
<AgentOptionIcon id={option.id} muted={muted} />
|
||||
<AgentOptionIcon id={option.id} muted={false} />
|
||||
<span className="truncate text-base font-medium text-white">
|
||||
{option.label}
|
||||
</span>
|
||||
@@ -164,14 +191,7 @@ export function ChooseAgentStep({
|
||||
</span>
|
||||
</div>
|
||||
<div className="flex shrink-0 flex-col items-end gap-1">
|
||||
{!option.enabled ? (
|
||||
<span
|
||||
data-testid={`onboarding-agent-badge-${option.id}`}
|
||||
className="rounded-md border border-white/15 bg-white/5 px-2 py-1 text-xs font-medium text-[var(--oh-muted)]"
|
||||
>
|
||||
{t(I18nKey.NAV$COMING_SOON)}
|
||||
</span>
|
||||
) : isSelected ? (
|
||||
{isSelected ? (
|
||||
<Check
|
||||
width={18}
|
||||
height={18}
|
||||
@@ -190,9 +210,10 @@ export function ChooseAgentStep({
|
||||
testId="onboarding-agent-next"
|
||||
type="button"
|
||||
variant="primary"
|
||||
onClick={onNext}
|
||||
isDisabled={isSaving}
|
||||
onClick={handleNext}
|
||||
>
|
||||
{t(I18nKey.ONBOARDING$NEXT)}
|
||||
{isSaving ? t(I18nKey.SETTINGS$SAVING) : t(I18nKey.ONBOARDING$NEXT)}
|
||||
</BrandButton>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -8,12 +8,46 @@ import { SettingsNavItem } from "#/constants/settings-nav";
|
||||
interface SettingsNavLinkProps {
|
||||
item: SettingsNavItem;
|
||||
onClick: () => void;
|
||||
/** If true, render the item as a non-clickable, greyed-out span. */
|
||||
disabled?: boolean;
|
||||
/** When disabled, used to render the tooltip — e.g. "Disabled while Claude Code is active". */
|
||||
disabledAgentName?: string;
|
||||
}
|
||||
|
||||
export function SettingsNavLink({ item, onClick }: SettingsNavLinkProps) {
|
||||
export function SettingsNavLink({
|
||||
item,
|
||||
onClick,
|
||||
disabled,
|
||||
disabledAgentName,
|
||||
}: SettingsNavLinkProps) {
|
||||
const { t } = useTranslation("openhands");
|
||||
const { to, icon, text } = item;
|
||||
|
||||
if (disabled) {
|
||||
const tooltip = disabledAgentName
|
||||
? t(I18nKey.SETTINGS$AGENT_DISABLED_TOOLTIP, {
|
||||
agentName: disabledAgentName,
|
||||
})
|
||||
: undefined;
|
||||
return (
|
||||
<span
|
||||
data-testid={`settings-nav-link-disabled-${to}`}
|
||||
title={tooltip}
|
||||
aria-disabled="true"
|
||||
className="group flex items-center gap-3 p-1 sm:px-3.5 sm:py-2 rounded opacity-40 cursor-not-allowed"
|
||||
>
|
||||
<Typography.Text className="flex h-5 w-5 shrink-0 items-center justify-center text-[#8C8C8C]">
|
||||
{icon}
|
||||
</Typography.Text>
|
||||
<div className="min-w-0 flex-1 overflow-hidden">
|
||||
<Typography.Text className="block truncate whitespace-nowrap text-[#8C8C8C]">
|
||||
{t(text as I18nKey)}
|
||||
</Typography.Text>
|
||||
</div>
|
||||
</span>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<NavigationLink
|
||||
end
|
||||
|
||||
@@ -51,6 +51,21 @@ export function SettingsDesktopSidebar({
|
||||
end
|
||||
testId={`sidebar-settings-${renderedItem.item.to}`}
|
||||
icon={renderedItem.item.icon}
|
||||
// Items marked ``disabledByAcp`` (LLM, Condenser, …) are greyed
|
||||
// out and un-clickable while an ACP agent is active — those
|
||||
// pages have nothing to configure while a separate sub-agent
|
||||
// owns the LLM/condenser/MCP layers. The mobile drawer below
|
||||
// already does this via ``SettingsNavLink``; do the same on
|
||||
// desktop. The clientLoader-side redirect in ``routes/
|
||||
// settings.tsx`` handles direct URL navigation.
|
||||
disabled={renderedItem.disabled}
|
||||
disabledReason={
|
||||
renderedItem.disabled && renderedItem.disabledAgentName
|
||||
? t(I18nKey.SETTINGS$AGENT_DISABLED_TOOLTIP, {
|
||||
agentName: renderedItem.disabledAgentName,
|
||||
})
|
||||
: undefined
|
||||
}
|
||||
/>
|
||||
))}
|
||||
</div>
|
||||
@@ -123,6 +138,8 @@ export function SettingsMobileDrawer({
|
||||
key={renderedItem.item.to}
|
||||
item={renderedItem.item}
|
||||
onClick={onCloseMobileMenu}
|
||||
disabled={renderedItem.disabled}
|
||||
disabledAgentName={renderedItem.disabledAgentName}
|
||||
/>
|
||||
);
|
||||
})}
|
||||
|
||||
@@ -22,6 +22,15 @@ interface SidebarNavLinkProps {
|
||||
* while the sidebar is collapsed.
|
||||
*/
|
||||
hoverContent?: React.ReactNode;
|
||||
/**
|
||||
* Pre-formatted human-readable reason for the disabled state, shown
|
||||
* as a hover tooltip. The component is i18n-agnostic — the caller
|
||||
* formats the string (typically via ``t(SETTINGS$AGENT_DISABLED_TOOLTIP,
|
||||
* { agentName })``) and passes it in. Only rendered when ``disabled``
|
||||
* is also true. Mirrors the mobile ``SettingsNavLink`` tooltip so the
|
||||
* disabled-state UX is consistent across surfaces.
|
||||
*/
|
||||
disabledReason?: string;
|
||||
/**
|
||||
* When true, forces the active style regardless of the current path.
|
||||
* Useful for links that should appear active for multiple related routes
|
||||
@@ -46,6 +55,7 @@ export function SidebarNavLink({
|
||||
icon,
|
||||
collapsed = false,
|
||||
hoverContent,
|
||||
disabledReason,
|
||||
forceActive = false,
|
||||
}: SidebarNavLinkProps) {
|
||||
const link = (
|
||||
@@ -55,6 +65,11 @@ export function SidebarNavLink({
|
||||
data-testid={testId}
|
||||
tabIndex={disabled ? -1 : 0}
|
||||
aria-label={collapsed ? label : undefined}
|
||||
// Announce the disabled state to assistive tech. The visual disable
|
||||
// (opacity + pointer-events) plus tabIndex=-1 + preventDefault gives
|
||||
// sighted/keyboard users the right behaviour already; this closes
|
||||
// the screen-reader gap so the link doesn't sound "actionable."
|
||||
aria-disabled={disabled || undefined}
|
||||
onClick={(e) => {
|
||||
if (disabled) {
|
||||
e.preventDefault();
|
||||
@@ -81,6 +96,20 @@ export function SidebarNavLink({
|
||||
</NavigationLink>
|
||||
);
|
||||
|
||||
// Disabled-with-reason: wrap with a tooltip explaining *why* (e.g.
|
||||
// "Disabled while Claude Code is active"). Mirrors the mobile
|
||||
// ``SettingsNavLink`` UX so users get the same explanation on both
|
||||
// surfaces. We use ``StyledTooltip`` regardless of the collapsed
|
||||
// state — without it, desktop users see a greyed-out link with no
|
||||
// hint about why their click didn't work.
|
||||
if (disabled && disabledReason) {
|
||||
return (
|
||||
<StyledTooltip content={disabledReason} placement="right">
|
||||
{link}
|
||||
</StyledTooltip>
|
||||
);
|
||||
}
|
||||
|
||||
if (!collapsed) return link;
|
||||
|
||||
return (
|
||||
|
||||
@@ -1,5 +1,8 @@
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { NavigationLink } from "#/components/shared/navigation-link";
|
||||
import { StyledTooltip } from "#/components/shared/buttons/styled-tooltip";
|
||||
import { useSettings } from "#/hooks/query/use-settings";
|
||||
import { ACP_PROVIDERS } from "#/constants/acp-providers";
|
||||
import { cn } from "#/utils/utils";
|
||||
import ServerProcessIcon from "#/icons/server-process.svg?react";
|
||||
import { BackendSyncedSettingsBadge } from "#/components/features/settings/backend-synced-settings-badge";
|
||||
@@ -11,6 +14,14 @@ interface ExtensionNavItem {
|
||||
icon: React.ReactElement;
|
||||
end?: boolean;
|
||||
comingSoon?: boolean;
|
||||
/**
|
||||
* When true, this item greys out (and the /route's ``clientLoader``
|
||||
* bounces to ``/settings/agent``) while an ACP agent is active.
|
||||
* The ACP sub-agent manages its own MCP servers; the SDK rejects
|
||||
* ``mcp_config`` on ``ACPAgent`` init outright, so the OpenHands-
|
||||
* side editor would silently no-op against the running subprocess.
|
||||
*/
|
||||
disabledByAcp?: boolean;
|
||||
}
|
||||
|
||||
const EXTENSIONS_NAV_ITEMS: ExtensionNavItem[] = [
|
||||
@@ -44,6 +55,7 @@ const EXTENSIONS_NAV_ITEMS: ExtensionNavItem[] = [
|
||||
label: "MCP Servers",
|
||||
icon: <ServerProcessIcon width={16} height={16} />,
|
||||
end: true,
|
||||
disabledByAcp: true,
|
||||
},
|
||||
{
|
||||
to: "/plugins",
|
||||
@@ -73,6 +85,16 @@ const EXTENSIONS_NAV_ITEMS: ExtensionNavItem[] = [
|
||||
|
||||
export function ExtensionsNavigation() {
|
||||
const { t } = useTranslation("openhands");
|
||||
const { data: settings } = useSettings();
|
||||
const isAcpAgent = settings?.agent_settings?.agent_kind === "acp";
|
||||
const acpServerKey =
|
||||
typeof settings?.agent_settings?.acp_server === "string"
|
||||
? settings.agent_settings.acp_server
|
||||
: undefined;
|
||||
const acpServerName = isAcpAgent
|
||||
? (ACP_PROVIDERS.find(({ key }) => key === acpServerKey)?.display_name ??
|
||||
"ACP Agent")
|
||||
: undefined;
|
||||
|
||||
return (
|
||||
<aside
|
||||
@@ -83,32 +105,69 @@ export function ExtensionsNavigation() {
|
||||
{t(I18nKey.NAV$EXTENSIONS)}
|
||||
</span>
|
||||
<div className="flex flex-col gap-0.5 pt-0.5">
|
||||
{EXTENSIONS_NAV_ITEMS.map((item) => (
|
||||
<NavigationLink
|
||||
key={item.to}
|
||||
to={item.to}
|
||||
end={item.end}
|
||||
data-testid={`sidebar-extensions-${item.to}`}
|
||||
className={({ isActive }) =>
|
||||
cn(
|
||||
"flex items-center gap-2 rounded-md transition-colors text-sm leading-5 truncate px-2 py-2 w-full",
|
||||
isActive
|
||||
? "bg-tertiary text-white font-medium"
|
||||
: "text-[var(--oh-muted)] hover:text-white hover:bg-[var(--oh-surface-raised)]",
|
||||
)
|
||||
}
|
||||
>
|
||||
{EXTENSIONS_NAV_ITEMS.map((item) => {
|
||||
const disabled = !!(isAcpAgent && item.disabledByAcp);
|
||||
const baseRow = (
|
||||
<span className="shrink-0 flex items-center justify-center">
|
||||
{item.icon}
|
||||
</span>
|
||||
<span className="truncate">{item.label}</span>
|
||||
{item.comingSoon && (
|
||||
<span className="ml-auto shrink-0 rounded-full border border-white/20 bg-white/5 px-1.5 py-0.5 text-[10px] font-medium text-[var(--oh-text-dim)]">
|
||||
{t(I18nKey.NAV$COMING_SOON)}
|
||||
</span>
|
||||
)}
|
||||
</NavigationLink>
|
||||
))}
|
||||
);
|
||||
const label = <span className="truncate">{item.label}</span>;
|
||||
const comingSoonBadge = item.comingSoon && (
|
||||
<span className="ml-auto shrink-0 rounded-full border border-white/20 bg-white/5 px-1.5 py-0.5 text-[10px] font-medium text-[var(--oh-text-dim)]">
|
||||
{t(I18nKey.NAV$COMING_SOON)}
|
||||
</span>
|
||||
);
|
||||
|
||||
if (disabled) {
|
||||
// Render a non-clickable surrogate so the URL and a11y tree
|
||||
// both communicate "you can't go here right now," then wrap
|
||||
// in StyledTooltip for the why. Mirrors the SettingsNavLink
|
||||
// disabled rendering — same flag (``disabledByAcp``), same
|
||||
// explanatory tooltip ("Disabled while {agentName} is the
|
||||
// active agent"), same greyed styles.
|
||||
return (
|
||||
<StyledTooltip
|
||||
key={item.to}
|
||||
content={t(I18nKey.SETTINGS$AGENT_DISABLED_TOOLTIP, {
|
||||
agentName: acpServerName,
|
||||
})}
|
||||
placement="right"
|
||||
>
|
||||
<span
|
||||
aria-disabled="true"
|
||||
data-testid={`sidebar-extensions-${item.to}`}
|
||||
className="flex items-center gap-2 rounded-md text-sm leading-5 truncate px-2 py-2 w-full text-[var(--oh-muted)] opacity-50 cursor-not-allowed"
|
||||
>
|
||||
{baseRow}
|
||||
{label}
|
||||
{comingSoonBadge}
|
||||
</span>
|
||||
</StyledTooltip>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<NavigationLink
|
||||
key={item.to}
|
||||
to={item.to}
|
||||
end={item.end}
|
||||
data-testid={`sidebar-extensions-${item.to}`}
|
||||
className={({ isActive }) =>
|
||||
cn(
|
||||
"flex items-center gap-2 rounded-md transition-colors text-sm leading-5 truncate px-2 py-2 w-full",
|
||||
isActive
|
||||
? "bg-tertiary text-white font-medium"
|
||||
: "text-[var(--oh-muted)] hover:text-white hover:bg-[var(--oh-surface-raised)]",
|
||||
)
|
||||
}
|
||||
>
|
||||
{baseRow}
|
||||
{label}
|
||||
{comingSoonBadge}
|
||||
</NavigationLink>
|
||||
);
|
||||
})}
|
||||
</div>
|
||||
<div className="px-2 pt-3">
|
||||
<BackendSyncedSettingsBadge />
|
||||
|
||||
@@ -0,0 +1,144 @@
|
||||
import { I18nKey } from "#/i18n/declaration";
|
||||
|
||||
/**
|
||||
* Built-in ACP (Agent Client Protocol) provider registry.
|
||||
*
|
||||
* **Source of truth:** ``openhands.sdk.settings.acp_providers.ACP_PROVIDERS``
|
||||
* in https://github.com/OpenHands/software-agent-sdk. This file is a
|
||||
* hand-kept TypeScript mirror — keep keys + commands in sync with the
|
||||
* Python source. The {@link OnboardingAgentId} and the
|
||||
* ``ACPAgentSettings.acp_server`` discriminator
|
||||
* (``"claude-code" | "codex" | "gemini-cli" | "custom"``) come from the
|
||||
* same Python module.
|
||||
*
|
||||
* Drift risk is tracked in agent-canvas#587. The richer SDK record
|
||||
* (api-key env var, session mode, set-session-model protocol, etc.)
|
||||
* is intentionally not mirrored here — canvas only renders this
|
||||
* registry in the Settings → Agent and onboarding UIs, so it only
|
||||
* needs the fields below.
|
||||
*/
|
||||
export interface ACPProviderConfig {
|
||||
/** Stable registry key, also stored on conversations as ``tags.acpserver``. */
|
||||
key: string;
|
||||
/** Human-readable name shown in dropdowns and conversation chips. */
|
||||
display_name: string;
|
||||
/**
|
||||
* Tokens passed to the agent-server as ``acp_command`` when this preset
|
||||
* is picked. Each entry must be a real ACP-protocol stdio server — the
|
||||
* SDK validates this against the {@link ACPProviderConfig.key}.
|
||||
*
|
||||
* NB: ``npx -y @openai/codex acp`` looks plausible but is **not** an
|
||||
* ACP server — the codex CLI has no ``acp`` subcommand and exits with
|
||||
* ``Error: stdin is not a terminal`` when spawned without a TTY, which
|
||||
* silently deadlocks the agent-server's ACP handshake. Use
|
||||
* ``@zed-industries/codex-acp`` (the Zed-shipped wrapper) instead.
|
||||
*/
|
||||
default_command: string[];
|
||||
/**
|
||||
* i18n key for the one-line provider description rendered under the
|
||||
* onboarding tile. Stored on the registry so adding a new ACP
|
||||
* provider only requires editing this file (not the onboarding tile
|
||||
* list separately).
|
||||
*/
|
||||
description_key: I18nKey;
|
||||
}
|
||||
|
||||
// Each entry's ``default_command`` is the published-package npx
|
||||
// invocation that speaks the ACP JSON-RPC protocol on stdio. Verified
|
||||
// against the upstream npm registry on the date noted below — if a
|
||||
// package is renamed/unpublished, the agent-server spawn fails fast
|
||||
// with ``ENOENT`` and the user can switch to the "Custom" preset.
|
||||
export const ACP_PROVIDERS: ACPProviderConfig[] = [
|
||||
{
|
||||
key: "claude-code",
|
||||
display_name: "Claude Code",
|
||||
// https://www.npmjs.com/package/@agentclientprotocol/claude-agent-acp
|
||||
// Verified 2026-05-19. Official Anthropic-maintained ACP wrapper
|
||||
// around the Claude Code CLI.
|
||||
default_command: ["npx", "-y", "@agentclientprotocol/claude-agent-acp"],
|
||||
description_key: I18nKey.ONBOARDING$AGENT_CLAUDE_CODE_DESCRIPTION,
|
||||
},
|
||||
{
|
||||
key: "codex",
|
||||
display_name: "Codex",
|
||||
// https://www.npmjs.com/package/@zed-industries/codex-acp
|
||||
// Verified 2026-05-19. Zed-maintained ACP wrapper around the
|
||||
// OpenAI Codex CLI — NOT ``@openai/codex acp`` (no ``acp``
|
||||
// subcommand on that package).
|
||||
default_command: ["npx", "-y", "@zed-industries/codex-acp"],
|
||||
description_key: I18nKey.ONBOARDING$AGENT_CODEX_DESCRIPTION,
|
||||
},
|
||||
{
|
||||
key: "gemini-cli",
|
||||
display_name: "Gemini CLI",
|
||||
// https://www.npmjs.com/package/@google/gemini-cli
|
||||
// Verified 2026-05-19. Official Google CLI; ``--acp`` switches it
|
||||
// into ACP server mode on stdio.
|
||||
default_command: ["npx", "-y", "@google/gemini-cli", "--acp"],
|
||||
description_key: I18nKey.ONBOARDING$AGENT_GEMINI_CLI_DESCRIPTION,
|
||||
},
|
||||
];
|
||||
|
||||
export const ACP_CUSTOM_PRESET_KEY = "custom";
|
||||
|
||||
/**
|
||||
* Build the ``agent_settings_diff`` payload PATCH /api/settings expects
|
||||
* for the agent-kind/provider choice the user just made.
|
||||
*
|
||||
* Used by both the Settings → Agent page and the onboarding "choose
|
||||
* agent" step — keeping the shape in one helper means a future change
|
||||
* (e.g. always seeding ``acp_command`` from the registry instead of
|
||||
* sending ``[]``, or adding new ``acp_*`` reset fields) lands in both
|
||||
* surfaces atomically.
|
||||
*
|
||||
* Returns ``null`` for an unknown ACP provider key by default — the
|
||||
* caller can skip the save (the UI shouldn't surface unknown options,
|
||||
* but the defensive path keeps a buggy preset list from corrupting
|
||||
* settings).
|
||||
*
|
||||
* Pass ``allowUnknownServer: true`` to opt into pass-through for keys
|
||||
* that aren't in {@link ACP_PROVIDERS} or ``ACP_CUSTOM_PRESET_KEY``.
|
||||
* The Settings → Agent page uses this when the user opens settings
|
||||
* that already carry an ``acp_server`` value canvas's registry
|
||||
* doesn't know about (e.g. set out-of-band via the API for a provider
|
||||
* we haven't mirrored yet) and saves without changing the command —
|
||||
* otherwise the original key would be silently demoted to ``"custom"``.
|
||||
*/
|
||||
export function buildAcpAgentSettingsDiff(
|
||||
providerKey: string,
|
||||
options: {
|
||||
command?: string[];
|
||||
model?: string | null;
|
||||
allowUnknownServer?: boolean;
|
||||
} = {},
|
||||
): Record<string, unknown> | null {
|
||||
if (providerKey === "openhands") {
|
||||
// Switching back to OpenHands. The agent-server's ``Settings.update``
|
||||
// applies a fresh ``{'agent_kind': ...}`` base whenever the kind
|
||||
// flips, so any ``acp_*`` fields would be discarded before
|
||||
// validation. Send the kind alone.
|
||||
return { agent_kind: "openhands" };
|
||||
}
|
||||
|
||||
const isCustom = providerKey === ACP_CUSTOM_PRESET_KEY;
|
||||
const provider = isCustom
|
||||
? undefined
|
||||
: ACP_PROVIDERS.find(({ key }) => key === providerKey);
|
||||
if (!isCustom && !provider && !options.allowUnknownServer) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// ``acp_args: []`` resets any API-set ``acp_args`` that would
|
||||
// otherwise survive and concatenate to ``acp_command`` at spawn time
|
||||
// (the agent-server merges the two before exec). Callers building the
|
||||
// payload from a textarea that already shows the merged command
|
||||
// (Settings → Agent) round-trip correctly — the merged tokens land in
|
||||
// ``acp_command`` here, so no args are lost.
|
||||
return {
|
||||
agent_kind: "acp",
|
||||
acp_server: providerKey,
|
||||
acp_command: options.command ?? [],
|
||||
acp_args: [],
|
||||
acp_model: options.model ?? null,
|
||||
};
|
||||
}
|
||||
@@ -9,20 +9,35 @@ export interface SettingsNavItem {
|
||||
text: string;
|
||||
/** Short grey subline under the page title (`settings.tsx`). */
|
||||
subtitle: string;
|
||||
// When true, this item is greyed out (and its route redirects to
|
||||
// ``/settings/agent``) while the active agent is ACP. The ACP sub-agent
|
||||
// manages its own LLM / condenser / MCP, so these OpenHands-side
|
||||
// surfaces have nothing useful to configure. Drives both the navigation
|
||||
// disable in ``use-settings-nav-items.ts`` and the loader redirect in
|
||||
// ``routes/settings.tsx`` from a single source.
|
||||
disabledByAcp?: boolean;
|
||||
}
|
||||
|
||||
export const OSS_NAV_ITEMS: SettingsNavItem[] = [
|
||||
{
|
||||
icon: <CircuitIcon width={16} height={16} />,
|
||||
to: "/settings/agent",
|
||||
text: "SETTINGS$NAV_AGENT",
|
||||
subtitle: "SETTINGS$PAGE_AGENT_SUBLINE",
|
||||
},
|
||||
{
|
||||
icon: <CircuitIcon width={16} height={16} />,
|
||||
to: "/settings",
|
||||
text: "SETTINGS$NAV_LLM",
|
||||
subtitle: "SETTINGS$PAGE_LLM_SUBLINE",
|
||||
disabledByAcp: true,
|
||||
},
|
||||
{
|
||||
icon: <MemoryIcon width={16} height={16} />,
|
||||
to: "/settings/condenser",
|
||||
text: "SETTINGS$NAV_CONDENSER",
|
||||
subtitle: "SETTINGS$PAGE_CONDENSER_SUBLINE",
|
||||
disabledByAcp: true,
|
||||
},
|
||||
{
|
||||
icon: <Shield className="size-4" strokeWidth={2} aria-hidden />,
|
||||
|
||||
@@ -1,37 +1,68 @@
|
||||
import { useConfig } from "#/hooks/query/use-config";
|
||||
import { useSettings } from "#/hooks/query/use-settings";
|
||||
import { OSS_NAV_ITEMS, SettingsNavItem } from "#/constants/settings-nav";
|
||||
import { ACP_PROVIDERS } from "#/constants/acp-providers";
|
||||
import { isSettingsPageHidden } from "#/utils/settings-utils";
|
||||
import { I18nKey } from "#/i18n/declaration";
|
||||
import { useActiveBackend } from "#/contexts/active-backend-context";
|
||||
|
||||
export type SettingsNavRenderedItem =
|
||||
| { type: "item"; item: SettingsNavItem }
|
||||
| {
|
||||
type: "item";
|
||||
item: SettingsNavItem;
|
||||
disabled?: boolean;
|
||||
disabledAgentName?: string;
|
||||
}
|
||||
| { type: "header"; text: I18nKey }
|
||||
| { type: "divider" };
|
||||
|
||||
export function useSettingsNavItems(): SettingsNavRenderedItem[] {
|
||||
const { data: config } = useConfig();
|
||||
const { data: settings } = useSettings();
|
||||
const { backend } = useActiveBackend();
|
||||
const featureFlags = config?.feature_flags;
|
||||
|
||||
const agentSettings = settings?.agent_settings ?? null;
|
||||
const isAcpAgent = agentSettings?.agent_kind === "acp";
|
||||
const acpServerKey =
|
||||
typeof agentSettings?.acp_server === "string"
|
||||
? agentSettings.acp_server
|
||||
: undefined;
|
||||
const acpServerName = isAcpAgent
|
||||
? (ACP_PROVIDERS.find(({ key }) => key === acpServerKey)?.display_name ??
|
||||
"ACP Agent")
|
||||
: undefined;
|
||||
|
||||
return OSS_NAV_ITEMS.filter(
|
||||
(item) => !isSettingsPageHidden(item.to, featureFlags),
|
||||
).map((item) => {
|
||||
if (item.to !== "/settings") {
|
||||
return { type: "item", item };
|
||||
}
|
||||
// Local backends present "LLM Profiles" as the section name + subtitle
|
||||
// for the ``/settings`` entry; cloud backends keep the canonical "LLM".
|
||||
// Apply the rename before the ACP disable check so the disabled tooltip
|
||||
// still names the visible label, not a stale one.
|
||||
const renamedItem =
|
||||
item.to === "/settings"
|
||||
? {
|
||||
...item,
|
||||
text:
|
||||
backend.kind === "local"
|
||||
? I18nKey.SETTINGS$LLM_PROFILES
|
||||
: item.text,
|
||||
subtitle:
|
||||
backend.kind === "local"
|
||||
? I18nKey.SETTINGS$PAGE_LLM_PROFILES_SUBLINE
|
||||
: item.subtitle,
|
||||
}
|
||||
: item;
|
||||
|
||||
return {
|
||||
type: "item",
|
||||
item: {
|
||||
...item,
|
||||
text:
|
||||
backend.kind === "local" ? I18nKey.SETTINGS$LLM_PROFILES : item.text,
|
||||
subtitle:
|
||||
backend.kind === "local"
|
||||
? I18nKey.SETTINGS$PAGE_LLM_PROFILES_SUBLINE
|
||||
: item.subtitle,
|
||||
},
|
||||
};
|
||||
if (isAcpAgent && item.disabledByAcp) {
|
||||
return {
|
||||
type: "item",
|
||||
item: renamedItem,
|
||||
disabled: true,
|
||||
disabledAgentName: acpServerName,
|
||||
};
|
||||
}
|
||||
return { type: "item", item: renamedItem };
|
||||
});
|
||||
}
|
||||
|
||||
+220
-16
@@ -4963,6 +4963,193 @@
|
||||
"uk": "LLM",
|
||||
"ca": "LLM"
|
||||
},
|
||||
"SETTINGS$NAV_AGENT": {
|
||||
"en": "Agent",
|
||||
"ja": "Agent",
|
||||
"zh-CN": "Agent",
|
||||
"zh-TW": "Agent",
|
||||
"ko-KR": "Agent",
|
||||
"no": "Agent",
|
||||
"it": "Agent",
|
||||
"pt": "Agent",
|
||||
"es": "Agent",
|
||||
"ar": "Agent",
|
||||
"fr": "Agent",
|
||||
"tr": "Agent",
|
||||
"de": "Agent",
|
||||
"uk": "Agent",
|
||||
"ca": "Agent"
|
||||
},
|
||||
"SETTINGS$AGENT_PAGE_DESCRIPTION": {
|
||||
"en": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"ja": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"zh-CN": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"zh-TW": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"ko-KR": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"no": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"it": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"pt": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"es": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"ar": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"fr": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"tr": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"de": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"uk": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI.",
|
||||
"ca": "Choose between the built-in OpenHands agent and an external ACP (Agent Client Protocol) subprocess such as Claude Code, Codex, or Gemini CLI."
|
||||
},
|
||||
"SETTINGS$AGENT_TYPE_OPENHANDS": {
|
||||
"en": "OpenHands",
|
||||
"ja": "OpenHands",
|
||||
"zh-CN": "OpenHands",
|
||||
"zh-TW": "OpenHands",
|
||||
"ko-KR": "OpenHands",
|
||||
"no": "OpenHands",
|
||||
"it": "OpenHands",
|
||||
"pt": "OpenHands",
|
||||
"es": "OpenHands",
|
||||
"ar": "OpenHands",
|
||||
"fr": "OpenHands",
|
||||
"tr": "OpenHands",
|
||||
"de": "OpenHands",
|
||||
"uk": "OpenHands",
|
||||
"ca": "OpenHands"
|
||||
},
|
||||
"SETTINGS$AGENT_TYPE_ACP": {
|
||||
"en": "ACP (external subprocess)",
|
||||
"ja": "ACP (external subprocess)",
|
||||
"zh-CN": "ACP (external subprocess)",
|
||||
"zh-TW": "ACP (external subprocess)",
|
||||
"ko-KR": "ACP (external subprocess)",
|
||||
"no": "ACP (external subprocess)",
|
||||
"it": "ACP (external subprocess)",
|
||||
"pt": "ACP (external subprocess)",
|
||||
"es": "ACP (external subprocess)",
|
||||
"ar": "ACP (external subprocess)",
|
||||
"fr": "ACP (external subprocess)",
|
||||
"tr": "ACP (external subprocess)",
|
||||
"de": "ACP (external subprocess)",
|
||||
"uk": "ACP (external subprocess)",
|
||||
"ca": "ACP (external subprocess)"
|
||||
},
|
||||
"SETTINGS$AGENT_PRESET": {
|
||||
"en": "Preset",
|
||||
"ja": "Preset",
|
||||
"zh-CN": "Preset",
|
||||
"zh-TW": "Preset",
|
||||
"ko-KR": "Preset",
|
||||
"no": "Preset",
|
||||
"it": "Preset",
|
||||
"pt": "Preset",
|
||||
"es": "Preset",
|
||||
"ar": "Preset",
|
||||
"fr": "Preset",
|
||||
"tr": "Preset",
|
||||
"de": "Preset",
|
||||
"uk": "Preset",
|
||||
"ca": "Preset"
|
||||
},
|
||||
"SETTINGS$AGENT_PRESET_CUSTOM": {
|
||||
"en": "Custom",
|
||||
"ja": "Custom",
|
||||
"zh-CN": "Custom",
|
||||
"zh-TW": "Custom",
|
||||
"ko-KR": "Custom",
|
||||
"no": "Custom",
|
||||
"it": "Custom",
|
||||
"pt": "Custom",
|
||||
"es": "Custom",
|
||||
"ar": "Custom",
|
||||
"fr": "Custom",
|
||||
"tr": "Custom",
|
||||
"de": "Custom",
|
||||
"uk": "Custom",
|
||||
"ca": "Custom"
|
||||
},
|
||||
"SETTINGS$AGENT_COMMAND": {
|
||||
"en": "Command",
|
||||
"ja": "Command",
|
||||
"zh-CN": "Command",
|
||||
"zh-TW": "Command",
|
||||
"ko-KR": "Command",
|
||||
"no": "Command",
|
||||
"it": "Command",
|
||||
"pt": "Command",
|
||||
"es": "Command",
|
||||
"ar": "Command",
|
||||
"fr": "Command",
|
||||
"tr": "Command",
|
||||
"de": "Command",
|
||||
"uk": "Command",
|
||||
"ca": "Command"
|
||||
},
|
||||
"SETTINGS$AGENT_COMMAND_HINT": {
|
||||
"en": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"ja": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"zh-CN": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"zh-TW": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"ko-KR": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"no": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"it": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"pt": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"es": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"ar": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"fr": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"tr": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"de": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"uk": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel.",
|
||||
"ca": "Command line used to spawn the ACP subprocess. API keys go through the Secrets panel."
|
||||
},
|
||||
"SETTINGS$AGENT_MODEL": {
|
||||
"en": "Model",
|
||||
"ja": "Model",
|
||||
"zh-CN": "Model",
|
||||
"zh-TW": "Model",
|
||||
"ko-KR": "Model",
|
||||
"no": "Model",
|
||||
"it": "Model",
|
||||
"pt": "Model",
|
||||
"es": "Model",
|
||||
"ar": "Model",
|
||||
"fr": "Model",
|
||||
"tr": "Model",
|
||||
"de": "Model",
|
||||
"uk": "Model",
|
||||
"ca": "Model"
|
||||
},
|
||||
"SETTINGS$AGENT_MODEL_HINT": {
|
||||
"en": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"ja": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"zh-CN": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"zh-TW": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"ko-KR": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"no": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"it": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"pt": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"es": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"ar": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"fr": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"tr": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"de": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"uk": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default.",
|
||||
"ca": "Optional model override forwarded to the ACP subprocess. Leave blank to use the provider's default."
|
||||
},
|
||||
"SETTINGS$AGENT_DISABLED_TOOLTIP": {
|
||||
"en": "Disabled while {{agentName}} is the active agent",
|
||||
"ja": "Disabled while {{agentName}} is the active agent",
|
||||
"zh-CN": "Disabled while {{agentName}} is the active agent",
|
||||
"zh-TW": "Disabled while {{agentName}} is the active agent",
|
||||
"ko-KR": "Disabled while {{agentName}} is the active agent",
|
||||
"no": "Disabled while {{agentName}} is the active agent",
|
||||
"it": "Disabled while {{agentName}} is the active agent",
|
||||
"pt": "Disabled while {{agentName}} is the active agent",
|
||||
"es": "Disabled while {{agentName}} is the active agent",
|
||||
"ar": "Disabled while {{agentName}} is the active agent",
|
||||
"fr": "Disabled while {{agentName}} is the active agent",
|
||||
"tr": "Disabled while {{agentName}} is the active agent",
|
||||
"de": "Disabled while {{agentName}} is the active agent",
|
||||
"uk": "Disabled while {{agentName}} is the active agent",
|
||||
"ca": "Disabled while {{agentName}} is the active agent"
|
||||
},
|
||||
"SETTINGS$NAV_VERIFICATION": {
|
||||
"en": "Verification",
|
||||
"ja": "検証",
|
||||
@@ -4980,6 +5167,23 @@
|
||||
"uk": "Верифікація",
|
||||
"ca": "Verificació"
|
||||
},
|
||||
"SETTINGS$PAGE_AGENT_SUBLINE": {
|
||||
"en": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"ja": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"zh-CN": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"zh-TW": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"ko-KR": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"no": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"it": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"pt": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"es": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"ar": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"fr": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"tr": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"de": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"uk": "Choose between the built-in OpenHands agent and an external ACP subprocess.",
|
||||
"ca": "Choose between the built-in OpenHands agent and an external ACP subprocess."
|
||||
},
|
||||
"SETTINGS$PAGE_LLM_SUBLINE": {
|
||||
"en": "Model, API key, and options for the agent.",
|
||||
"ja": "エージェントのモデル、APIキー、およびオプション。",
|
||||
@@ -26077,22 +26281,22 @@
|
||||
"tr": "OpenAI'nin Codex aracı.",
|
||||
"uk": "Агент Codex від OpenAI."
|
||||
},
|
||||
"ONBOARDING$AGENT_COMING_SOON": {
|
||||
"en": "Support for other agents coming soon!",
|
||||
"ja": "他のエージェントのサポートは近日公開予定です！",
|
||||
"zh-CN": "更多智能体支持即将上线！",
|
||||
"zh-TW": "更多代理支援即將推出！",
|
||||
"ko-KR": "다른 에이전트 지원이 곧 추가됩니다!",
|
||||
"no": "Støtte for andre agenter kommer snart!",
|
||||
"ar": "دعم وكلاء آخرين قريبًا!",
|
||||
"de": "Unterstützung für weitere Agenten folgt in Kürze!",
|
||||
"fr": "Prise en charge d'autres agents bientôt disponible !",
|
||||
"it": "Supporto per altri agenti in arrivo!",
|
||||
"pt": "Suporte a outros agentes em breve!",
|
||||
"es": "¡Pronto se admitirán más agentes!",
|
||||
"ca": "Aviat hi haurà compatibilitat amb més agents!",
|
||||
"tr": "Diğer araçlar için destek yakında geliyor!",
|
||||
"uk": "Підтримка інших агентів незабаром!"
|
||||
"ONBOARDING$AGENT_GEMINI_CLI_DESCRIPTION": {
|
||||
"en": "Google's Gemini CLI agent.",
|
||||
"ja": "Google's Gemini CLI agent.",
|
||||
"zh-CN": "Google's Gemini CLI agent.",
|
||||
"zh-TW": "Google's Gemini CLI agent.",
|
||||
"ko-KR": "Google's Gemini CLI agent.",
|
||||
"no": "Google's Gemini CLI agent.",
|
||||
"ar": "Google's Gemini CLI agent.",
|
||||
"de": "Google's Gemini CLI agent.",
|
||||
"fr": "Google's Gemini CLI agent.",
|
||||
"it": "Google's Gemini CLI agent.",
|
||||
"pt": "Google's Gemini CLI agent.",
|
||||
"es": "Google's Gemini CLI agent.",
|
||||
"ca": "Google's Gemini CLI agent.",
|
||||
"tr": "Google's Gemini CLI agent.",
|
||||
"uk": "Google's Gemini CLI agent."
|
||||
},
|
||||
"ONBOARDING$BACKEND_TITLE": {
|
||||
"en": "Check your backend",
|
||||
|
||||
@@ -16,6 +16,7 @@ export default [
|
||||
route("mcp", "routes/mcp.tsx"),
|
||||
route("settings", "routes/settings.tsx", [
|
||||
index("routes/llm-settings.tsx"),
|
||||
route("agent", "routes/agent-settings.tsx"),
|
||||
route("condenser", "routes/condenser-settings.tsx"),
|
||||
route("verification", "routes/verification-settings.tsx"),
|
||||
// Legacy entry: redirects to /mcp (preserves old bookmarks).
|
||||
|
||||
@@ -0,0 +1,348 @@
|
||||
import React, { useEffect, useRef, useState } from "react";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { AxiosError } from "axios";
|
||||
import { useSettings } from "#/hooks/query/use-settings";
|
||||
import { useSaveSettings } from "#/hooks/mutation/use-save-settings";
|
||||
import { SettingsDropdownInput } from "#/components/features/settings/settings-dropdown-input";
|
||||
import { SettingsInput } from "#/components/features/settings/settings-input";
|
||||
import { BrandButton } from "#/components/features/settings/brand-button";
|
||||
import { Typography } from "#/ui/typography";
|
||||
import { I18nKey } from "#/i18n/declaration";
|
||||
import {
|
||||
displayErrorToast,
|
||||
displaySuccessToast,
|
||||
} from "#/utils/custom-toast-handlers";
|
||||
import { retrieveAxiosErrorMessage } from "#/utils/retrieve-axios-error-message";
|
||||
import {
|
||||
ACP_PROVIDERS,
|
||||
ACP_CUSTOM_PRESET_KEY,
|
||||
buildAcpAgentSettingsDiff,
|
||||
type ACPProviderConfig,
|
||||
} from "#/constants/acp-providers";
|
||||
import { parseCommand, formatCommand } from "#/utils/acp-command";
|
||||
|
||||
export const handle = { hideTitle: true };
|
||||
|
||||
type AgentType = "openhands" | "acp";
|
||||
|
||||
const COMMAND_PLACEHOLDER_FALLBACK = "npx -y <package-name>";
|
||||
|
||||
/** Coerce a possibly-undefined unknown to ``string[]`` by keeping only string
|
||||
* entries; non-arrays and non-string entries are discarded. Used when
|
||||
* loading already-stored ``acp_command`` / ``acp_args`` lists from settings. */
|
||||
function toStringArray(value: unknown): string[] {
|
||||
return Array.isArray(value)
|
||||
? value.filter((v): v is string => typeof v === "string")
|
||||
: [];
|
||||
}
|
||||
|
||||
function detectPreset(
|
||||
commandText: string,
|
||||
providers: ACPProviderConfig[],
|
||||
): string {
|
||||
// The preset dropdown silently follows the textarea: editing the
|
||||
// command into something that exactly matches another preset's
|
||||
// ``default_command`` re-selects that preset (and editing it away
|
||||
// from every preset flips to "Custom"). This is intentional — the
|
||||
// textarea is the source of truth; the dropdown is a read-out of
|
||||
// "which preset, if any, does this command match." A user pasting a
|
||||
// built-in command they had stashed elsewhere shouldn't have to
|
||||
// also click the matching preset.
|
||||
const normalized = parseCommand(commandText).join(" ");
|
||||
for (const provider of providers) {
|
||||
if (normalized === provider.default_command.join(" ")) {
|
||||
return provider.key;
|
||||
}
|
||||
}
|
||||
return ACP_CUSTOM_PRESET_KEY;
|
||||
}
|
||||
|
||||
function AgentSettingsScreen() {
|
||||
const { t } = useTranslation("openhands");
|
||||
const { data: settings, isLoading } = useSettings();
|
||||
const { mutate: saveSettings, isPending: isSaving } = useSaveSettings();
|
||||
|
||||
const [agentType, setAgentType] = useState<AgentType>("openhands");
|
||||
const [commandText, setCommandText] = useState("");
|
||||
const [acpModel, setAcpModel] = useState("");
|
||||
const [isDirty, setIsDirty] = useState(false);
|
||||
|
||||
// Track the settings reference we last initialised the form from. The form
|
||||
// re-initialises when the server returns a new settings object (after save,
|
||||
// or after an update from another tab) but not just because a re-render
|
||||
// produced a new identity — otherwise an in-flight refetch could wipe
|
||||
// in-progress edits.
|
||||
const lastInitializedSettingsRef = useRef<unknown>(null);
|
||||
|
||||
// Capture the raw ``acp_server`` and the rendered textarea contents at
|
||||
// load time so handleSave can detect "user opened settings and clicked
|
||||
// Save without touching anything" — that case has to preserve an
|
||||
// otherwise-unknown ``acp_server`` value (e.g. a provider the canvas
|
||||
// registry doesn't carry yet, set out-of-band via the API), instead of
|
||||
// demoting it to ``"custom"`` and silently losing the original key.
|
||||
const loadedAcpServerRef = useRef<string | null>(null);
|
||||
const loadedCommandTextRef = useRef<string>("");
|
||||
|
||||
useEffect(() => {
|
||||
if (!settings) return;
|
||||
if (lastInitializedSettingsRef.current === settings) return;
|
||||
|
||||
lastInitializedSettingsRef.current = settings;
|
||||
const kind = settings.agent_settings?.agent_kind;
|
||||
|
||||
if (kind === "acp") {
|
||||
setAgentType("acp");
|
||||
|
||||
// Reconstruct the textarea contents from the persisted settings:
|
||||
//
|
||||
// spawn = acp_command + acp_args
|
||||
//
|
||||
// BUT acp_command may be the "default-preset shortcut" ``[]``, with
|
||||
// the real command living in the registry under ``acp_server``.
|
||||
// Without expanding the default before merging, a user with
|
||||
// ``acp_command: []`` + ``acp_args: ["--extra-arg"]`` would see
|
||||
// just ``--extra-arg`` in the textarea (no prefix), and saving
|
||||
// would persist ``acp_command: ["--extra-arg"]`` + flip the
|
||||
// preset to ``custom`` — silently losing the registry-default
|
||||
// prefix. Expand first, then merge.
|
||||
//
|
||||
// The merge is also what makes ``acp_args: []`` safe on save (see
|
||||
// ``handleSave`` below): any API-set ``acp_args`` lands in the
|
||||
// textarea here, so writing the textarea back as ``acp_command``
|
||||
// round-trips the full command without losing the args.
|
||||
const rawAcpServer = settings.agent_settings?.acp_server;
|
||||
const acpServer =
|
||||
typeof rawAcpServer === "string" ? rawAcpServer : undefined;
|
||||
const provider = ACP_PROVIDERS.find(({ key }) => key === acpServer);
|
||||
const storedCommand = toStringArray(settings.agent_settings?.acp_command);
|
||||
const effectiveBaseCommand =
|
||||
storedCommand.length > 0
|
||||
? storedCommand
|
||||
: (provider?.default_command ?? []);
|
||||
const tokens = [
|
||||
...effectiveBaseCommand,
|
||||
...toStringArray(settings.agent_settings?.acp_args),
|
||||
];
|
||||
const renderedCommandText =
|
||||
tokens.length > 0 ? formatCommand(tokens) : "";
|
||||
setCommandText(renderedCommandText);
|
||||
loadedAcpServerRef.current = acpServer ?? null;
|
||||
loadedCommandTextRef.current = renderedCommandText;
|
||||
|
||||
const savedModel = settings.agent_settings?.acp_model;
|
||||
setAcpModel(typeof savedModel === "string" ? savedModel : "");
|
||||
} else {
|
||||
setAgentType("openhands");
|
||||
setCommandText("");
|
||||
setAcpModel("");
|
||||
loadedAcpServerRef.current = null;
|
||||
loadedCommandTextRef.current = "";
|
||||
}
|
||||
setIsDirty(false);
|
||||
}, [settings]);
|
||||
|
||||
if (isLoading) return null;
|
||||
|
||||
const isAcp = agentType === "acp";
|
||||
const commandTokens = parseCommand(commandText);
|
||||
const isAcpInvalid = isAcp && commandTokens.length === 0;
|
||||
// ``selectedPreset`` is derived from ``commandText`` rather than tracked as
|
||||
// state. Keeping it in state would mean three sync points (effect, textarea
|
||||
// onChange, dropdown onSelectionChange) that can drift — deriving inline
|
||||
// keeps the dropdown honest about what would actually be saved.
|
||||
const selectedPreset = detectPreset(commandText, ACP_PROVIDERS);
|
||||
const selectedProvider = ACP_PROVIDERS.find(
|
||||
({ key }) => key === selectedPreset,
|
||||
);
|
||||
const isDefaultProviderCommand =
|
||||
!!selectedProvider &&
|
||||
commandTokens.join(" ") === selectedProvider.default_command.join(" ");
|
||||
const commandPlaceholder =
|
||||
formatCommand(ACP_PROVIDERS[0]?.default_command ?? []) ||
|
||||
COMMAND_PLACEHOLDER_FALLBACK;
|
||||
|
||||
const handleSave = () => {
|
||||
// The textarea is the single source of truth for the launch tokens:
|
||||
// when a built-in preset is selected and untouched, we save the
|
||||
// empty ``acp_command`` shortcut + provider key (the adapter
|
||||
// expands it from the registry at conversation-create time, see
|
||||
// ``buildConfiguredAcpAgentSettings``). When a preset has been
|
||||
// edited or the user picked Custom, we save the literal tokens.
|
||||
// Either way, ``acp_args: []`` is reset so API-set args can't
|
||||
// duplicate at spawn time — safe because the load path already
|
||||
// merged any ``acp_args`` into the textarea, so the tokens we save
|
||||
// here include them.
|
||||
const useDefault = !!(selectedProvider && isDefaultProviderCommand);
|
||||
// Preserve an unknown loaded ``acp_server`` (e.g. a provider the
|
||||
// canvas registry doesn't carry yet, set out-of-band via the API)
|
||||
// when the user opens settings and saves without touching the
|
||||
// command. Without this branch, ``detectPreset`` would route the
|
||||
// unknown key into ``ACP_CUSTOM_PRESET_KEY`` and the original
|
||||
// server name would be silently demoted on save.
|
||||
const loadedServer = loadedAcpServerRef.current;
|
||||
const commandUnchanged = commandText === loadedCommandTextRef.current;
|
||||
const loadedServerIsUnknown =
|
||||
!!loadedServer &&
|
||||
loadedServer !== ACP_CUSTOM_PRESET_KEY &&
|
||||
!ACP_PROVIDERS.some((p) => p.key === loadedServer);
|
||||
const preserveUnknownServer =
|
||||
isAcp && commandUnchanged && loadedServerIsUnknown;
|
||||
const providerKey = !isAcp
|
||||
? "openhands"
|
||||
: preserveUnknownServer
|
||||
? (loadedServer as string)
|
||||
: selectedProvider && isDefaultProviderCommand
|
||||
? selectedProvider.key
|
||||
: ACP_CUSTOM_PRESET_KEY;
|
||||
const agentSettingsDiff = buildAcpAgentSettingsDiff(providerKey, {
|
||||
command: useDefault ? [] : commandTokens,
|
||||
model: acpModel.trim() || null,
|
||||
allowUnknownServer: preserveUnknownServer,
|
||||
});
|
||||
|
||||
if (!agentSettingsDiff) {
|
||||
// Unreachable through the UI (the providerKey is derived from
|
||||
// either a known preset or the custom sentinel), but defensive.
|
||||
return;
|
||||
}
|
||||
|
||||
saveSettings(
|
||||
{ agent_settings_diff: agentSettingsDiff },
|
||||
{
|
||||
onError: (error) => {
|
||||
const message = retrieveAxiosErrorMessage(error as AxiosError);
|
||||
displayErrorToast(message || t(I18nKey.ERROR$GENERIC));
|
||||
},
|
||||
onSuccess: () => {
|
||||
displaySuccessToast(t(I18nKey.SETTINGS$SAVED));
|
||||
setIsDirty(false);
|
||||
},
|
||||
},
|
||||
);
|
||||
};
|
||||
|
||||
return (
|
||||
<div
|
||||
data-testid="agent-settings-screen"
|
||||
className="flex flex-col gap-6 pb-8 max-w-2xl"
|
||||
>
|
||||
<div>
|
||||
<Typography.H2 className="mb-2">
|
||||
{t(I18nKey.SETTINGS$AGENT)}
|
||||
</Typography.H2>
|
||||
<Typography.Paragraph className="text-sm text-[#A3A3A3]">
|
||||
{t(I18nKey.SETTINGS$AGENT_PAGE_DESCRIPTION)}
|
||||
</Typography.Paragraph>
|
||||
</div>
|
||||
|
||||
<SettingsDropdownInput
|
||||
testId="agent-type-selector"
|
||||
name="agent-type"
|
||||
label={t(I18nKey.SETTINGS$AGENT)}
|
||||
items={[
|
||||
{
|
||||
key: "openhands",
|
||||
label: t(I18nKey.SETTINGS$AGENT_TYPE_OPENHANDS),
|
||||
},
|
||||
{ key: "acp", label: t(I18nKey.SETTINGS$AGENT_TYPE_ACP) },
|
||||
]}
|
||||
selectedKey={agentType}
|
||||
onSelectionChange={(key) => {
|
||||
if (!key) return;
|
||||
const newType = key as AgentType;
|
||||
setAgentType(newType);
|
||||
if (newType === "acp" && !commandText) {
|
||||
// First-time switch into ACP: prefill the textarea with the
|
||||
// first registered provider.
|
||||
const preferred = ACP_PROVIDERS[0];
|
||||
if (preferred) {
|
||||
setCommandText(formatCommand(preferred.default_command));
|
||||
}
|
||||
}
|
||||
setIsDirty(true);
|
||||
}}
|
||||
/>
|
||||
|
||||
{isAcp && (
|
||||
<>
|
||||
<SettingsDropdownInput
|
||||
testId="agent-preset-selector"
|
||||
name="agent-preset"
|
||||
label={t(I18nKey.SETTINGS$AGENT_PRESET)}
|
||||
items={[
|
||||
...ACP_PROVIDERS.map((provider) => ({
|
||||
key: provider.key,
|
||||
label: provider.display_name,
|
||||
})),
|
||||
{
|
||||
key: ACP_CUSTOM_PRESET_KEY,
|
||||
label: t(I18nKey.SETTINGS$AGENT_PRESET_CUSTOM),
|
||||
},
|
||||
]}
|
||||
selectedKey={selectedPreset}
|
||||
onSelectionChange={(key) => {
|
||||
if (!key) return;
|
||||
const preset = String(key);
|
||||
const provider = ACP_PROVIDERS.find(({ key: k }) => k === preset);
|
||||
if (provider) {
|
||||
setCommandText(formatCommand(provider.default_command));
|
||||
}
|
||||
setIsDirty(true);
|
||||
}}
|
||||
/>
|
||||
|
||||
<div className="flex flex-col gap-2.5">
|
||||
<Typography.Text className="text-sm">
|
||||
{t(I18nKey.SETTINGS$AGENT_COMMAND)}
|
||||
</Typography.Text>
|
||||
<textarea
|
||||
data-testid="agent-command-input"
|
||||
className="bg-tertiary border border-[#717888] rounded-sm p-2 text-sm font-mono text-white placeholder:italic placeholder:text-[#717888] min-h-[60px] resize-y focus:outline-none focus:border-white"
|
||||
value={commandText}
|
||||
placeholder={commandPlaceholder}
|
||||
onChange={(e) => {
|
||||
setCommandText(e.target.value);
|
||||
setIsDirty(true);
|
||||
}}
|
||||
/>
|
||||
<Typography.Text className="text-xs text-[#717888]">
|
||||
{t(I18nKey.SETTINGS$AGENT_COMMAND_HINT)}
|
||||
</Typography.Text>
|
||||
</div>
|
||||
|
||||
<div className="flex flex-col gap-1.5">
|
||||
<SettingsInput
|
||||
testId="agent-model-input"
|
||||
label={t(I18nKey.SETTINGS$AGENT_MODEL)}
|
||||
type="text"
|
||||
className="w-full"
|
||||
value={acpModel}
|
||||
showOptionalTag
|
||||
onChange={(value) => {
|
||||
setAcpModel(value);
|
||||
setIsDirty(true);
|
||||
}}
|
||||
/>
|
||||
<Typography.Text className="text-xs text-[#717888]">
|
||||
{t(I18nKey.SETTINGS$AGENT_MODEL_HINT)}
|
||||
</Typography.Text>
|
||||
</div>
|
||||
</>
|
||||
)}
|
||||
|
||||
<div>
|
||||
<BrandButton
|
||||
testId="agent-save-button"
|
||||
type="button"
|
||||
variant="primary"
|
||||
isDisabled={isSaving || !isDirty || isAcpInvalid}
|
||||
onClick={handleSave}
|
||||
>
|
||||
{isSaving ? t(I18nKey.SETTINGS$SAVING) : t(I18nKey.BUTTON$SAVE)}
|
||||
</BrandButton>
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
export default AgentSettingsScreen;
|
||||
@@ -9,6 +9,7 @@ import { useSettings } from "#/hooks/query/use-settings";
|
||||
import { useDeleteMcpServer } from "#/hooks/mutation/use-delete-mcp-server";
|
||||
import { useActiveBackend } from "#/contexts/active-backend-context";
|
||||
import { parseMcpConfig } from "#/utils/mcp-config";
|
||||
import { redirectIfAcpActive } from "#/utils/acp-route-guard";
|
||||
import {
|
||||
displayErrorToast,
|
||||
displaySuccessToast,
|
||||
@@ -56,6 +57,24 @@ function flattenMcpConfig(config: MCPConfig): MCPServerConfig[] {
|
||||
];
|
||||
}
|
||||
|
||||
// ACP guard: the ACP sub-agent owns its own MCP server configuration —
|
||||
// the SDK explicitly rejects ``mcp_config`` on ACPAgent init
|
||||
// (``acp_agent.py:845``) and ``agent-server-adapter`` already strips
|
||||
// it from start payloads. The Settings → Agent page is where the user
|
||||
// configures the ACP server, so bouncing there is consistent with how
|
||||
// ``/settings`` and ``/settings/condenser`` already behave under ACP.
|
||||
//
|
||||
// Declared with no parameters (rather than typed as
|
||||
// ``Route.ClientLoaderArgs``) so the lib build doesn't pull the
|
||||
// generated ``./+types/mcp`` types out of ``rootDir``. The lib's
|
||||
// ``tsconfig.lib.json`` reaches this file via the
|
||||
// ``components/settings/index.ts`` re-export → ``routes/mcp-settings`` →
|
||||
// ``routes/mcp`` chain; importing the React Router-generated type
|
||||
// breaks ``rootDir: "src"`` because the types live under
|
||||
// ``.react-router/types``. ``index-redirect`` and
|
||||
// ``mcp-settings-redirect`` use the same no-args pattern.
|
||||
export const clientLoader = async () => redirectIfAcpActive();
|
||||
|
||||
export default function MCPPage() {
|
||||
const { t } = useTranslation("openhands");
|
||||
const { data: settings, isLoading } = useSettings();
|
||||
|
||||
@@ -9,10 +9,12 @@ import { WebClientConfig } from "#/api/option-service/option.types";
|
||||
import { QUERY_KEYS, CONFIG_CACHE_OPTIONS } from "#/hooks/query/query-keys";
|
||||
import { Typography } from "#/ui/typography";
|
||||
import { useSettingsNavItems } from "#/hooks/use-settings-nav-items";
|
||||
import { OSS_NAV_ITEMS } from "#/constants/settings-nav";
|
||||
import {
|
||||
getFirstAvailablePath,
|
||||
isSettingsPageHidden,
|
||||
} from "#/utils/settings-utils";
|
||||
import { redirectIfAcpActive } from "#/utils/acp-route-guard";
|
||||
|
||||
export const clientLoader = async ({ request }: Route.ClientLoaderArgs) => {
|
||||
const url = new URL(request.url);
|
||||
@@ -33,6 +35,19 @@ export const clientLoader = async ({ request }: Route.ClientLoaderArgs) => {
|
||||
}
|
||||
}
|
||||
|
||||
// ACP guard: the pages flagged ``disabledByAcp`` (LLM, Condenser, …)
|
||||
// have no useful content while an external ACP subprocess drives
|
||||
// conversations. Bounce them to ``/settings/agent``. Driven by the
|
||||
// same ``disabledByAcp`` flag the nav hook uses for greying out, so
|
||||
// the list of redirected paths and the greyed-out paths can never
|
||||
// drift apart. See {@link redirectIfAcpActive} for why the redirect
|
||||
// lives in the loader rather than a per-route ``useEffect``.
|
||||
const currentNavItem = OSS_NAV_ITEMS.find((item) => item.to === pathname);
|
||||
if (currentNavItem?.disabledByAcp) {
|
||||
const acpRedirect = await redirectIfAcpActive();
|
||||
if (acpRedirect) return acpRedirect;
|
||||
}
|
||||
|
||||
return null;
|
||||
};
|
||||
|
||||
|
||||
@@ -37,6 +37,7 @@ export const DEFAULT_SETTINGS: Settings = {
|
||||
agent_settings_schema: null,
|
||||
agent_settings: {
|
||||
schema_version: 1,
|
||||
agent_kind: "openhands",
|
||||
agent: "CodeActAgent",
|
||||
llm: {
|
||||
model: "openhands/claude-opus-4-5-20251101",
|
||||
|
||||
@@ -109,6 +109,19 @@ export type SkillInfo = {
|
||||
|
||||
export type SettingsScope = "personal";
|
||||
|
||||
/**
|
||||
* Agent kind stored on ``Settings.agent_settings.agent_kind``.
|
||||
*
|
||||
* - ``"openhands"`` (default): the conversation runs through OpenHands' built-in
|
||||
* LLM-driven Agent. The other agent_settings fields (``llm``, ``condenser``,
|
||||
* ``mcp_config``, ``tools``) apply.
|
||||
* - ``"acp"``: the conversation is driven by an external ACP subprocess
|
||||
* (Claude Code / Codex / Gemini CLI / Custom). The LLM / condenser / MCP
|
||||
* settings are inert; ``acp_command`` / ``acp_args`` / ``acp_model`` /
|
||||
* ``acp_env`` / ``acp_server`` apply instead.
|
||||
*/
|
||||
export type AgentKind = "openhands" | "acp";
|
||||
|
||||
export type Settings = {
|
||||
llm_model: string;
|
||||
llm_base_url: string;
|
||||
|
||||
@@ -0,0 +1,189 @@
|
||||
// ``shell-quote`` is a CJS module that does ``module.exports = { parse, quote }``;
|
||||
// Vite's ESM interop can resolve a default/namespace import but not named
|
||||
// imports against that shape (the dev server crashes with "does not provide
|
||||
// an export named 'parse'"). Namespace import works on both the dev server
|
||||
// and the Rollup-based prod build. Used only for the ``quote`` direction —
|
||||
// see ``parseCommand`` below for why we don't use ``shell-quote.parse``.
|
||||
import * as shellQuote from "shell-quote";
|
||||
|
||||
const { quote } = shellQuote;
|
||||
|
||||
/**
|
||||
* Parse a single-string command into argv tokens for ``acp_command``.
|
||||
*
|
||||
* Used by the Settings → Agent textarea — the user types one human-readable
|
||||
* command (e.g. ``bash -c "echo hello world"``) and we convert it into the
|
||||
* ``string[]`` shape that the agent-server's ``ACPAgent.acp_command``
|
||||
* expects. The agent-server passes that array straight to
|
||||
* ``subprocess.create_subprocess_exec``; no shell is involved on the spawn
|
||||
* side, so this parser only needs to handle argv-style word splitting
|
||||
* with quote/escape support — *not* shell metasyntax.
|
||||
*
|
||||
* Why a custom tokenizer and **not** ``shell-quote.parse``:
|
||||
*
|
||||
* ``shell-quote.parse`` treats ``?``, ``*``, ``$VAR``, redirects, and
|
||||
* comments as shell syntax and emits non-string AST nodes for them.
|
||||
* Filtering to strings would silently drop entire argv tokens. The
|
||||
* concrete data-corruption case is a URL with a query string —
|
||||
*
|
||||
* node acp.js --endpoint https://example.com/acp?tenant=abc
|
||||
*
|
||||
* ``shell-quote`` reads ``?tenant=abc`` as a glob pattern and returns
|
||||
* ``["node","acp.js","--endpoint",{op:"glob",…}]``, so the saved
|
||||
* ``acp_command`` becomes ``["node","acp.js","--endpoint"]`` — the URL
|
||||
* vanishes. The agent-server then spawns a broken command and the user
|
||||
* gets a confusing runtime error far from the configuration UI.
|
||||
*
|
||||
* The replacement tokenizer treats every non-whitespace, non-quote
|
||||
* character as part of the current token: ``?``, ``*``, ``$``, ``|``,
|
||||
* ``>``, ``#``, backticks all round-trip verbatim. Shell-only constructs
|
||||
* (pipes, redirects, env-var expansion, command substitution) would
|
||||
* land as literal argv entries — which is what the user typed and what
|
||||
* ``subprocess.create_subprocess_exec`` will see. That's correct: a
|
||||
* user who types ``foo | bar`` into the Settings → Agent textarea is
|
||||
* configuring a literal command, not a shell pipeline; ``foo`` doesn't
|
||||
* actually pipe into ``bar``, but neither does it silently disappear.
|
||||
*
|
||||
* Quoting rules supported:
|
||||
* - whitespace separates tokens
|
||||
* - single quotes: literal until the next ``'`` (no escapes inside,
|
||||
* matching POSIX shell)
|
||||
* - double quotes: literal until the next ``"`` (with ``\\"`` and
|
||||
* ``\\\\`` honored as escapes; no $-expansion)
|
||||
* - backslash outside quotes: escapes the next character (whitespace,
|
||||
* quote, or anything else — turns it into a literal)
|
||||
* - explicit empty quoted segments (``""`` / ``''``) produce an
|
||||
* empty-string token, matching the round-trip rule in
|
||||
* ``formatCommand``
|
||||
*
|
||||
* Unterminated quotes are tolerated: the current token closes at EOF
|
||||
* with whatever was accumulated. A throw here would crash the
|
||||
* Settings → Agent page mid-render; the Save button is already gated
|
||||
* on a non-empty argv so a recoverable miss can't be silently saved
|
||||
* either way.
|
||||
*/
|
||||
export function parseCommand(value: string): string[] {
|
||||
const tokens: string[] = [];
|
||||
let i = 0;
|
||||
let current = "";
|
||||
let inToken = false;
|
||||
|
||||
while (i < value.length) {
|
||||
const ch = value[i];
|
||||
|
||||
if (ch === " " || ch === "\t" || ch === "\n" || ch === "\r") {
|
||||
if (inToken) {
|
||||
tokens.push(current);
|
||||
current = "";
|
||||
inToken = false;
|
||||
}
|
||||
i += 1;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (ch === "'") {
|
||||
// Single-quoted segment: literal until the next single quote.
|
||||
// No escapes inside (POSIX shell semantics).
|
||||
inToken = true;
|
||||
i += 1;
|
||||
while (i < value.length && value[i] !== "'") {
|
||||
current += value[i];
|
||||
i += 1;
|
||||
}
|
||||
// Skip the closing quote if present. Unterminated → EOF closes.
|
||||
if (i < value.length) i += 1;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (ch === '"') {
|
||||
// Double-quoted segment: literal with backslash escapes for
|
||||
// ``\\"`` and ``\\\\``. We intentionally do NOT expand $VAR
|
||||
// (so a user typing ``"--key=$X"`` keeps it literal — the
|
||||
// agent-server doesn't run a shell anyway). Other backslash
|
||||
// sequences pass through verbatim (matches what most users
|
||||
// expect when copying paths with backslashes from Windows
|
||||
// examples; corner-case differences from POSIX aren't worth
|
||||
// the complexity here).
|
||||
inToken = true;
|
||||
i += 1;
|
||||
while (i < value.length && value[i] !== '"') {
|
||||
if (value[i] === "\\" && i + 1 < value.length) {
|
||||
const next = value[i + 1];
|
||||
if (next === '"' || next === "\\") {
|
||||
current += next;
|
||||
i += 2;
|
||||
continue;
|
||||
}
|
||||
}
|
||||
current += value[i];
|
||||
i += 1;
|
||||
}
|
||||
if (i < value.length) i += 1;
|
||||
continue;
|
||||
}
|
||||
|
||||
if (ch === "\\" && i + 1 < value.length) {
|
||||
// Unquoted backslash escapes the next character (whitespace,
|
||||
// quote, glob char, anything). Useful for typing literal spaces
|
||||
// in a path or escaping a literal quote.
|
||||
inToken = true;
|
||||
current += value[i + 1];
|
||||
i += 2;
|
||||
continue;
|
||||
}
|
||||
|
||||
// Every other character — including ``?``, ``*``, ``$``, ``|``,
|
||||
// ``>``, ``#``, ``&``, ``;``, ``(``, ``)`` — is a literal part of
|
||||
// the current token. Shell-metasyntax filtering happens at the
|
||||
// OS/shell boundary, which we don't cross.
|
||||
inToken = true;
|
||||
current += ch;
|
||||
i += 1;
|
||||
}
|
||||
|
||||
if (inToken) {
|
||||
tokens.push(current);
|
||||
}
|
||||
return tokens;
|
||||
}
|
||||
|
||||
// Tokens that need shell-quoting when rendering back to a string —
|
||||
// whitespace, quotes, backslashes, redirects/pipes/globs, and the
|
||||
// command-separators. ``@``, ``/``, ``-``, ``.``, ``+``, ``=`` and
|
||||
// other punctuation that's common in package names and URLs are
|
||||
// safe in argv-only contexts (the agent-server execs the array, no
|
||||
// shell intermediary), so we leave them alone — otherwise
|
||||
// ``npx -y @org/pkg`` would render as ``npx -y \@org/pkg`` and that's
|
||||
// a hostile read-back in the textarea.
|
||||
//
|
||||
// Note: ``?``, ``*``, ``$``, ``|``, ``>``, ``#``, ``&``, ``;``, ``(``,
|
||||
// ``)`` no longer carry shell meaning in {@link parseCommand} (which
|
||||
// is purely an argv tokenizer), so a token containing them would
|
||||
// round-trip fine without quoting. We still quote on output because
|
||||
// users frequently switch between this textarea and an actual shell
|
||||
// (copy-paste workflows), and quoting matches the conservative
|
||||
// expectation "if it would need quoting in a shell, show it quoted."
|
||||
const SHELL_UNSAFE = /[\s"'\\$`&|;<>(){}*?#!~[\]]/;
|
||||
|
||||
/**
|
||||
* Render a ``string[]`` argv back into a single string the textarea
|
||||
* can display. Tokens that *would* need shell quoting (whitespace,
|
||||
* quotes, redirects, …) go through ``shell-quote.quote`` for correct
|
||||
* escaping; tokens that are already shell-safe (the overwhelming
|
||||
* majority of package names and CLI flags) round-trip verbatim. The
|
||||
* output remains a valid input to {@link parseCommand}.
|
||||
*/
|
||||
export function formatCommand(command: readonly string[]): string {
|
||||
return command
|
||||
.map((tok) =>
|
||||
// Quote any token that:
|
||||
// * contains a shell-significant character (whitespace, quotes,
|
||||
// redirects, …), so it doesn't get re-split on parse, OR
|
||||
// * is the empty string — without explicit quoting,
|
||||
// ``["bash", "-c", ""]`` would render as ``"bash -c "`` and
|
||||
// round-trip back to ``["bash", "-c"]``, silently dropping
|
||||
// the (rare but valid) empty argument.
|
||||
SHELL_UNSAFE.test(tok) || tok === "" ? quote([tok]) : tok,
|
||||
)
|
||||
.join(" ");
|
||||
}
|
||||
@@ -0,0 +1,52 @@
|
||||
import { redirect } from "react-router";
|
||||
|
||||
import { getActiveBackend } from "#/api/backend-registry/active-store";
|
||||
import { getSettingsQueryFn } from "#/hooks/query/use-settings";
|
||||
import { SETTINGS_QUERY_KEYS } from "#/hooks/query/query-keys";
|
||||
import { queryClient } from "#/query-client-config";
|
||||
|
||||
/**
|
||||
* Issue a ``redirect`` to ``/settings/agent`` when the personal settings
|
||||
* say the active agent is ACP.
|
||||
*
|
||||
* The ACP sub-agent owns its own LLM, MCP servers, and condenser, so the
|
||||
* canvas-side surfaces that configure those concepts (``/settings``,
|
||||
* ``/settings/condenser``, ``/mcp``) have nothing useful to do while ACP
|
||||
* is active. Doing the redirect in a ``clientLoader`` (instead of a
|
||||
* per-route ``useEffect``) prevents the one-frame flash of the old
|
||||
* content before the guard fires.
|
||||
*
|
||||
* ``staleTime: 0`` is intentional: the read drives a redirect, and a
|
||||
* 5-minute stale tolerance would let a cross-tab agent-kind flip route
|
||||
* the user to the wrong page until the cache caught up. PATCH /settings
|
||||
* already invalidates this key, so the forced refetch only fires when
|
||||
* something might actually have changed.
|
||||
*
|
||||
* Fall through silently on settings-fetch errors (unauthed, network,
|
||||
* local agent-server not running) — better to render the page than
|
||||
* redirect-loop on a missing payload.
|
||||
*
|
||||
* Cache key is aligned with {@link useSettings} so the loader and the
|
||||
* in-render hook share a single cache entry rather than thrashing the
|
||||
* same data through two different keys.
|
||||
*/
|
||||
export async function redirectIfAcpActive() {
|
||||
try {
|
||||
const active = getActiveBackend();
|
||||
const personalSettings = await queryClient.fetchQuery({
|
||||
queryKey: [
|
||||
...SETTINGS_QUERY_KEYS.byScope("personal"),
|
||||
active.backend.id,
|
||||
active.orgId,
|
||||
],
|
||||
queryFn: () => getSettingsQueryFn("personal"),
|
||||
staleTime: 0,
|
||||
});
|
||||
if (personalSettings?.agent_settings?.agent_kind === "acp") {
|
||||
return redirect("/settings/agent");
|
||||
}
|
||||
} catch {
|
||||
// Settings unfetchable — let the page render.
|
||||
}
|
||||
return null;
|
||||
}
|
||||
@@ -60,7 +60,14 @@ export function isSettingsPageHidden(
|
||||
export function getFirstAvailablePath(
|
||||
featureFlags: WebClientFeatureFlags | undefined,
|
||||
): string | null {
|
||||
// ``/settings/agent`` precedes ``/settings`` because it is the ACP
|
||||
// landing page and is always available (no feature flag hides it).
|
||||
// When ``hide_llm_settings`` is on, the user is steered there rather
|
||||
// than to ``/settings/app`` (an unrelated section that used to win the
|
||||
// fallback). For OpenHands-agent users this is also a sensible landing
|
||||
// — the Agent page is the single place to switch kinds.
|
||||
const fallbackOrder = [
|
||||
{ path: "/settings/agent", hidden: false },
|
||||
{ path: "/settings", hidden: !!featureFlags?.hide_llm_settings },
|
||||
{ path: "/settings/app", hidden: false },
|
||||
{ path: "/settings/secrets", hidden: false },
|
||||
|
||||
@@ -202,6 +202,12 @@ export default defineConfig(({ mode }) => {
|
||||
"framer-motion",
|
||||
"rehype-raw",
|
||||
"rehype-sanitize",
|
||||
// ``shell-quote`` is a CJS module used by ``src/utils/acp-command.ts``
|
||||
// for the Settings → Agent textarea. With ``noDiscovery: true`` above,
|
||||
// omitting it from this list means Vite serves the raw CJS file to
|
||||
// the browser and dev crashes with ``ReferenceError: exports is not
|
||||
// defined`` on the first import of agent-settings.tsx.
|
||||
"shell-quote",
|
||||
"unist-util-visit",
|
||||
"uuid",
|
||||
"zustand",
|
||||
|
||||
Reference in New Issue
Block a user