Fix keyless active LLM profile recovery#1346
Conversation
Detect local active LLM profiles whose API key is missing and guide users directly to the affected profile editor. Add unit and mock-LLM E2E coverage for the recovery path. Co-authored-by: openhands <openhands@all-hands.dev>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Resolve mock-LLM profile management E2E conflicts by retaining main's OpenHands provider base_url coverage and the issue #1344 keyless active profile recovery test. Co-authored-by: openhands <openhands@all-hands.dev>
✅ Mock-LLM E2E Tests55/55 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
🔶 Mock-LLM Docker E2E Test Results50/55 passed · 5 skipped Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. Outcome: APPROVE — see review comment. |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — clean, well-scoped fix.
The change closes a real UX gap in local mode: when the active LLM profile exists but its API key is missing, the home banner now explains the actual problem and deep-links straight into the profile editor instead of dumping the user on a generic setup screen. Approving.
What works well
- Right data structure. A small
LLM_CONFIGURATION_ISSUESenum + discriminated hook return (issue,activeProfileName) keeps the banner dumb and the classification centralized inuseLlmConfigured. Easy to extend if more recovery states show up. - Centralized route construction.
buildLlmSettingsRoute(profileName)insrc/constants/llm-settings.tsis the right place to own the URL shape and URL-encode the profile name — callers don't have to think about it. - Idempotent URL → editor effect.
requestedEditProfileNameRefis the correct guard for "respond to a query param on mount" in React: it survives profile-list refetches without re-firing, and correctly handles a later?profile=…change. - Local-only when it matters.
hasActiveProfileMissingApiKeyis gated onisLocal, matching the existinghasUsableLlmlocal-vs-cloud split — no spurious banners in cloud mode. - Test coverage. New unit test for the banner + a Playwright E2E that exercises the full home → settings editor flow against the real backend. CI confirms both pass (mock-LLM E2E: 55/55).
Minor notes (non-blocking)
- The
LLM_CONFIGURATION_ISSUESenum currently has only two values and one consumer. A plain boolean would suffice today, but keeping the enum is cheap and avoids a refactor if a third state appears. Keep as is. - The
useEffectdeps includehandleEditProfileandprofilesData?.profiles. The ref guard makes duplicate fires harmless, butprofilesData?.profileschanges reference on every query refetch — the effect will re-run frequently. Functionally correct; if it ever shows up in a profile, narrow the dep to a derived boolean (e.g.,profilesData ? "loaded" : "pending").
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Scoped to local-mode profile handling, no API or schema changes, opt-in?profile=query parameter, backward-compatible with existing URLs and cloud backends.
VERDICT:
✅ Worth merging. Core logic is sound, tests cover both unit and E2E paths, and the change follows the codebase's existing local/cloud split conventions.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
HUMAN:
AGENT:
This PR was created by an AI agent (OpenHands) on behalf of the user.
Verification performed:
Results: all commands above passed locally.
I also attempted the targeted mock-LLM E2E regression locally:
npm run test:e2e:mock-llm -- tests/e2e/mock-llm/mock-llm-profile-management.spec.ts -g "keyless active profile"The first attempt needed the Python
openhands-sdkdependency for the mock server, which I installed. Later Playwright webServer startup/teardown reset the terminal session, so I could not obtain a reliable local pass/fail result for that targeted E2E run. The mock-LLM E2E scenario is included in this PR for CI coverage.Why
Fixes #1344. In local mode, users could end up with an active LLM profile whose API key was missing (
api_key_set: false). The home/chat UI showed a generic blocked state even though the actionable fix was to re-enter the API key for the selected profile.Summary
useLlmConfigured./settings/llm?profile=<name>.Issue Number
Fixes #1344
How to Test
/settings/llm?profile=<profile-name>and opens that profile in edit mode.Automated checks run locally are listed in the AGENT section above.
Video/Screenshots
Not captured. A mock-LLM E2E regression was added to cover the browser scenario.
Type
Notes
No migrations or config changes required.
@malhotra5 can click here to continue refining the PR
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.28.1-pythonopenhands-automation==1.0.0a937306d437adca7af3580d890d4906f39e25cf75cPull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-37306d4Run
All tags pushed for this build
About Multi-Architecture Support
sha-37306d4) is a multi-arch manifest supporting both amd64 and arm64sha-37306d4-amd64) are also available if needed