Repository navigation
feat(agent-profiles): one launch pipeline for every conversation start - #5154
simonrosenberg wants to merge 7 commits into
Conversation
|
📁 PR Artifacts Notice This PR contains a |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR unifies every conversation-start path (stored profile, inline profile, raw agent, deprecated agent_settings, materialize preview, and the Docker runtime) behind a single prepare_agent_launch function. I reviewed the resolver, the agent-server launch glue, the Docker mediation double-resolve, the request model changes, and the test suite.
No material bugs found. The design is clean and achieves what it set out to do: each profile field is now implemented once instead of once-per-launch-path.
What holds up under scrutiny
- Exception hierarchy is correct.
UnresolvedProfileReferences -> AgentLaunchError -> ValueError, and both routers catchAgentLaunchErrorbeforeValueError, so the structured 422 detail (code/dangling_llm_profile_ref/dangling_mcp_server_refs) is preserved rather than collapsed to a plain string.ProfileNotFoundstays a separateException-> 404. - Docker double-resolve is sound.
prepare_startruns withbuild_agent=False(andbrowser_available=False) so a dangling ref fails before any container is provisioned;finish_startre-resolves the sameLaunchSource/catalog with the container's realbrowser_availablefromGET /server_info(usable_toolsfield -- verified correct). Skill discovery happens once in the catalog and is reused, not re-run. Container cleanup (registry.stop) is wired on every failure branch. - Secret scoping is enforced server-side in
apply_launch(plan.allowed_secretsfiltersrequest.secrets), andagent_settings_launch_sourcecorrectly setsallowed_secrets=None(unrestricted) to match the legacy path. The additions-cannot-widen-scope invariant is asserted by a real test. - Backward compatibility is preserved.
LaunchedAgentProfile.inline/llm_profile_refandAgentLaunchAdditions.llm_profile_refare additive fields with defaults;LaunchedAgentProfilehas noextra="forbid", so old persisted conversations load.agent_settingsis deprecated withdeprecated_in=1.50.0->removed_in=1.55.0(5 minor releases, meeting the policy) and is converted to an inline profile through the same pipeline rather than dropped. - Tests exercise real code paths, not mock wiring:
test_launch.pycovers runtime pieces, additions, provenance, dangling refs, and the deprecatedagent_settingsround-trip (verifying non-profile fields likecritic_api_key,user_message_suffix,agent_context.secretssurvive). The parity test diffs every launch path field-by-field.
Eval / benchmark risk -- flagging for a human maintainer
This PR changes agent launch behavior in ways that could plausibly move benchmark numbers, and there is no eval-monitor link or maintainer eval confirmation in the PR description or comments:
- The
agent_settingspath now goes through the unified pipeline, so it gains forcedstream=True,load_project_skills=True, browser injection (when the runtime has it andtoolsis null), and a freshcurrent_datetimeinstead of the saved timestamp. Canvas already sends these explicitly so it's unaffected, but a hand-rolled REST client relying on the old defaults would see a change. - ACP skill sourcing in Docker switched from the host's answer to the container's (
openhands_managed), so an ACP profile in Docker now gets managed skills where it previously got none.
Per the repo's review policy I'm leaving a COMMENT rather than approving. Recommend a maintainer run lightweight evals (or confirm Canvas-only impact) before merging.
Minor note (non-blocking)
warn_deprecated(..., deprecated_in="1.50.0") is called from the agent-server while the current SDK version is 1.49.1. _should_warn compares current >= deprecated_in, so the runtime warning won't actually fire until the SDK ships as 1.50.0 -- which is presumably the release this PR targets, so this is consistent, just worth knowing the warning is effectively inert until then.
Risk assessment: MEDIUM -- no correctness/security issues, but behavior changes on the agent_settings/ACP-in-Docker paths that warrant eval confirmation.
Verdict: Worth merging after a maintainer confirms no eval regression.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with thumbs up or thumbs down to give feedback.
|
Thanks — on the eval-risk flag, here is what I can evidence from this branch so a maintainer has the facts to decide. I have not run evals. Who actually sees the Cloud is not on this path. The enterprise app server builds a concrete ACP-in-Docker. Previously the host's Unchanged for stored-profile launches (the path evals exercise): the parity e2e diffs On the |
Collapse the several code paths that built a launch agent into prepare_agent_launch(), the single SDK function that resolves an Agent Profile's references and applies the runtime-dependent and per-launch pieces. conversation_service, the Docker runtime's mediation and the materialize preview all call it, so a profile named `default` and a named one build the same agent, and a preview can no longer disagree with a launch. Adds an inline `agent_profile` draft and a per-launch `llm_profile_ref` override, deprecates `agent_settings` (converted into an inline profile so it takes the same pipeline), and returns one structured error for dangling LLM/MCP references. Fixes #5141 Co-authored-by: openhands <openhands@all-hands.dev> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: openhands <openhands@all-hands.dev> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…agent Co-authored-by: openhands <openhands@all-hands.dev> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: openhands <openhands@all-hands.dev> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
97f40ab to
5786443
Compare
Co-authored-by: openhands <openhands@all-hands.dev> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#4287 added the routing settings to OpenHandsAgentSettings only, so route_task_to_model reached an agent_settings launch and not a profile launch. The profile now carries enable_classify_and_switch_llm_tool and meta_profile_ref, and the launch hydrates the meta-profile plus the LLMs it routes to, so a runtime without the store on disk can still route. Co-authored-by: openhands <openhands@all-hands.dev> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code review (xhigh) — head
|
- Pass meta_profile_ref to route_task_to_model by name instead of hydrating the meta-profile and its target LLMs (with decrypted keys) into the tool's untyped params at launch. The tool resolves it at call time again, so a missing meta-profile no longer fails the launch, and the meta-profile store is no longer created on every launch or materialize. - Keep explicit agent_settings opt-outs (current_datetime=null, load_project_skills=false); only unset values take the launch defaults. - Append the browser after the default tools, restoring the old tool order. - Serialize agent_settings through the settings model so secrets are masked unless expose_secrets/cipher context is given. - Turn a malformed inline agent_profile into a validation error (422) instead of an uncaught TypeError (500). - Run the browser probe in the worker thread, and skip it for raw agents. - Surface Docker build failures after the container starts as a 422 launch error with the real cause instead of a generic 502. - Dry run reuses the launch's resolved LLM instead of loading it twice. - Share ACPSkillSourcing and the server's skill-sourcing rule; drop the duplicate explicit-null filter in prepare_start; trim docstrings. Co-authored-by: openhands <openhands@all-hands.dev> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Fixes for the review above —
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Summary
I reviewed the current head dbd6b272 (25 files, +3074/-1280): the unified prepare_agent_launch pipeline, the agent_server/agent_launch.py adapter, the Docker double-resolve in mediation.py/routers.py, the StartConversationRequest changes, and the new/updated tests.
The code itself holds up. I ran the focused suites locally against this head and they pass:
tests/sdk/profiles— 181 passedtests/agent_server/test_agent_launch_parity.py,test_agent_launch_additions.py,test_acp_skill_sourcing.py— 25 passedtests/agent_server/test_agent_profile_conv_start.py,test_conversation_router.py,test_conversation_service.py,tests/agent_server/docker_runtime— 317 passed
CI for dbd6b272 is green (52 checks, all success). I also verified by hand that the deprecated agent_settings path carries through fields no profile models (critic_api_key, user_message_suffix, agent_context.secrets, current_datetime=null, load_project_skills=false), that a scoped profile's secret filter is applied in apply_launch, and that the inline-profile validator now rejects a malformed draft at parse time (422) instead of a 500.
I am leaving a COMMENT rather than an approval for one reason: the PR description on this head describes a meta_profile_ref implementation that the head no longer contains.
Finding — the PR description contradicts the shipped contract, and hides a real Docker routing gap
The current body says:
prepare_agent_launchtherefore loads the meta-profile and the LLM profiles it routes to; a caller that hydrated them itself (a cloud control plane, throughbase_settings) keeps its own copies.
and lists the 422 detail as:
{"code": "unresolved_profile_references", "message", "dangling_llm_profile_ref", "dangling_mcp_server_refs", "dangling_meta_profile_ref"}
Neither is true on dbd6b272. The final commit dbd6b272 ("address xhigh review of the unified launch") removed the hydration path and the dangling_meta_profile_ref key entirely. On this head:
OpenHandsAgentSettings.active_meta_profileis set toprofile.meta_profile_ref(name only);meta_profileandmeta_profile_llmsare left unset.UnresolvedProfileReferences.to_detail()emits onlydangling_llm_profile_refanddangling_mcp_server_refs; themeta_profile_refparameter no longer exists (grep confirms zero remaining references todangling_meta_profile_refin the tree).AgentLaunchCatalog.meta_profile_storeandMetaProfileLoaderare gone.
I verified the consequence for the case the body claims is fixed. A profile launch inside a Docker conversation container produces a route_task_to_model spec with params == {'active_meta_profile': 'pareto'} only, and the container's HOME/OH_PERSISTENCE_DIR is a per-conversation runtime dir (/var/openhands/.openhands, mounted from <runtime_dir>/persistence) that carries no meta-profiles/ directory and is never populated with the host's store. Reconstructing the tool the way the container does (ClassifyAndSwitchLLMTool.create(**spec["params"])) then fails at call time with:
FileNotFoundError: Active meta-profile 'pareto' could not be resolved from the
store or inline configuration. Available meta-profiles: none
So the routing tool a profile launch wires up cannot route inside Docker, which is exactly the failure the description says hydration was added to prevent. No test exercises a routing profile through the Docker prepare_start/finish_start path, so this is silent.
To be clear: this is not a regression against main — the base had no profile-level routing fields at all, and the author's 2026-09-28 comment discloses the Docker limitation as known. But the durable PR description still advertises hydration as the fix and still lists a 422 key that does not exist, and the REST detail schema is a public contract (the OpenAPI summary in the body is itself the artifact other clients read). A maintainer approving on that description would be approving a different change than the one at dbd6b272.
Please either (a) correct the description: drop "hydrates the meta-profile plus the LLM profiles it routes to" and dangling_meta_profile_ref, and state the Docker limitation plainly, or (b) if Docker routing is meant to work with a profile-carried meta_profile_ref, restore a store-independent path (e.g. hydrate into meta_profile/meta_profile_llms as agent_settings already does), with a test that drives it through the container launch.
Other notes (non-blocking)
warn_deprecated(..., deprecated_in="1.50.0")will not fire until the SDK ships 1.50.0, which is presumably the target release; the removal target1.55.0is similarly inert on this branch. Fine, just worth knowing.- The behavior changes the author enumerated for the
agent_settingspath (forcedstream=True,load_project_skills=True, freshcurrent_datetime, browser injection whentoolsis null) are real and intentional; I confirmed each against the base. Worth a maintainer eyeball for eval movement, as the earlier review flagged.
🔄 CHANGES REQUESTED
|
I'm closing this in favour of #5398, which replaces the design. A review of the head commit ( Bugs at head:
Design problems:
What #5398 does instead: it splits the launch into
Some pieces here can be reused, and #5398 lists them: the #5151 and #5315 refer to this PR. They should track #5398 instead. |
2 participants
HUMAN:
Filing the SDK half of #5141: the
defaultprofile and named profiles have to build the same agent from one pipeline, so new profile fields stop needing an implementation per launch path. Canvas and cloud adoption follow separately.AGENT:
Why
Launching the
defaultAgent Profile and launching a named one built different agents from the same stored settings, because the launch had several code paths and each set agent fields its own way: a Pydantic validator foragent_settings, theagent_profile_idbranch ofconversation_service, a second copy of that branch in the Docker runtime'sprepare_start, and a separate dry-run formaterialize. Every profile field had to be implemented, and kept correct, once per path — #3967, #4014, #4016 and #4542 were that cost paid one field at a time.This is step 1–3 of #5141 (the SDK steps). Canvas and cloud adoption are the follow-ups listed below.
Summary
openhands.sdk.profiles.prepare_agent_launch(source, *, catalog, runtime, additions, profile_origin, build_agent)resolves a profile'sllm_profile_ref/mcp_server_refs/disabled_skillsand owns the launch-time fields: tool defaults and browser injection, forced streaming, skill catalog and ACP skill sourcing, project-skill loading, suffix + additions,current_datetime,load_memory, and the secret scope. Runtime-dependent answers come in as an explicitAgentLaunchRuntime, so the Docker runtime can pass its container's answer instead of the host's.conversation_service, Docker mediation andmaterializeall call it through the newagent_server/agent_launch.py;_resolve_agent_from_profile,_with_load_memoryand_apply_acp_skill_sourcingare gone, as is the duplicated profile branch inprepare_start.materializeis the same call with side effects off (build_agent=False, which skipscreate_agent()— the only step that can refresh a subscription LLM's credentials over the network). Its verdict andresolved_settingsnow come from the launch itself.StartConversationRequestaccepts an inlineagent_profiledraft (resolved exactly like a stored one, never saved), andagent_settingsis deprecated (deprecated: truein OpenAPI, removal target v1.55.0). During the window the server converts it into an inline profile with the payload's own LLM/MCP/skills as its catalog, so it takes the same pipeline instead of the validator shortcut; fields no profile models (critic_api_key,user_message_suffix,agent_context.secrets,acp_isolate_data_dir, …) are carried through, not dropped.AgentLaunchAdditions.llm_profile_refgives the chat LLM picker a per-launch override, recorded inLaunchedAgentProfile(which also gainedinline). Additions stay additive: they carry no tools, MCP servers, skills or secrets, and a test asserts a scoped profile's tools/MCP/skills/secret scope are byte-identical with and without them.UnresolvedProfileReferencesand the start endpoint returns 422 with{"code": "unresolved_profile_references", "message", "dangling_llm_profile_ref", "dangling_mcp_server_refs", "dangling_meta_profile_ref"}— no silent fallback to a different path.OpenHandsAgentProfilegainsenable_classify_and_switch_llm_tool(behavior, like theenable_switch_llm_toolbeside it) andmeta_profile_ref(a reference resolved against the meta-profile store, likellm_profile_ref), and the launch hydrates the meta-profile plus the LLM profiles it routes to. See the note below for why hydration, not just the name.REST API contract changes
Compared with base OpenAPI
3311ba9eec50for public/api/**paths.Issue Number
Fixes #5141
How to Test
Unit tests (the meta-profile wiring is covered by 5 cases in
tests/sdk/profiles/test_launch.pyandtest_meta_profile_routing_reaches_a_profile_launchin the parity file):End-to-end against a real agent-server (this is the interesting one — it reproduces the table in #5141 without canvas):
It boots
python -m openhands.agent_serveron a temp persistence dir, stores two identically-configured profiles (defaultanddefault-copy), launches a conversation throughagent_profile_idfor each, through an inlineagent_profiledraft, and through the deprecatedagent_settings, then diffs the agents the server actually built against thematerializepreview of the same profile. Output in.pr/launch_parity_e2e_output.txt:Compared field by field:
llm(whole dump), tools, MCP keys, skills, suffix,disabled_skills,load_project_skills,load_memory, condenser, critic, concurrency, switch-LLM and whether a timestamp is present. The one deliberate exception is the skill catalog on theagent_settingspath: that payload carries the client's own catalog (canvas assembles one today), which is exactly what the canvas follow-up removes.Type
Notes
Rebased onto
mainon 2026-09-28 (was 11 days behind; 76 commits). The rebase was conflict-free, and both suites pass locally with the meta-profile wiring included:tests/agent_server2252 passed,tests/sdk6538 passed, ruff + pyright clean. Run them separately —tests/agent_serverandtests/sdkin one pytest process produce three unrelated ordering failures (CI runs them as separate jobs).Two interactions found while rebasing; the first is fixed in this PR:
Add Pareto prompt meta-profile routing #4287 (Pareto meta-profile routing, merged 09-24) had re-opened this exact divergence for four new fields — now wired through the profile. It added
enable_classify_and_switch_llm_tool,active_meta_profile,meta_profileandmeta_profile_llmstoOpenHandsAgentSettingsand not toOpenHandsAgentProfile, and_build_openhands_settingscomposes from an allow-list of profile fields. Measured on this branch before the fix:agent_settingslaunchenable_classify_and_switch_llm_toolTrueFalseactive_meta_profile'pareto'NoneThe legacy path survived only because it passes
base_settings; a profile launch silently lostroute_task_to_model. The split follows this issue's own rule — behavior on the profile, shared resources global — so the profile carries the toggle plus ameta_profile_ref, and the meta-profile store stays global like the MCP registry and the LLM profiles.Why the launch hydrates rather than just passing the name: the routing tool reads the store by name and only falls back to the inline
meta_profileblob when the store cannot resolve it. A conversation container'sHOMEis a per-conversation runtime dir with no meta-profile store, so a name alone would have left Docker routing failing at tool-call time.prepare_agent_launchtherefore loads the meta-profile and the LLM profiles it routes to; a caller that hydrated them itself (a cloud control plane, throughbase_settings) keeps its own copies. A danglingmeta_profile_reffails the launch only when the tool is enabled — an inert ref is never resolved, so it cannot fail a launch it has no effect on.Known limit: on the deprecated
agent_settingspath the routing targets are not hydrated, because that catalog's LLM loader only knows the payload's single inline LLM. It is{}there today as well, so nothing regresses, and a local runtime resolves them from its own store.fix(agent-server): enforce profile secret scope on runtime-launched conversations #5193's secret-scope gap is adjacent but not closed here.
apply_launchfiltersrequest.secretsonly when the source is a profile; a rawagentbound to a profile throughOH_RUNTIME_LAUNCHED_PROFILE(the in-container launch) is filtered on resume, not at create. I corrected the docstring that overclaimed this and left the fix to fix(agent-server): enforce profile secret scope on runtime-launched conversations #5193, where it belongs.Merge-order note: this PR supersedes
agent_server/profile_launch.pyfrom #5151 —agent_launch.pysubsumesgather_profile_launch_inputs, and probes the container's browser availability rather than the host's. #5151 is already conflicting with main independently (via #4287). Several open PRs touch functions this one rewrites (#5315, #4717, #5193, #4961, #5199, #5318, #4410, #5126); this PR itself merges cleanly withmain.Behavior changes reviewers should weigh:
code/dangling_llm_profile_ref(the oldmessage/dangling_mcp_server_refskeys are unchanged). Canvas never reads that status — it pre-checks the profile list and downgrades toagent_settings— and that rule is what the follow-up deletes.agent_settingsis no longer converted in the validator, soStartConversationRequest(agent_settings=...).agentisNoneuntil the server resolves it, and an invalid payload is rejected by the start endpoint (422) rather than at parse time. The field also lostexclude=Trueso it round-trips over the wire. An explicit"agent": nullalongside another source no longer crashes.agent_settingspath now gets the launch-owned fields too, because it goes through the same pipeline: streaming forced on,load_project_skills=True, browser added when the payload'stoolsis null and the runtime has it, and a freshcurrent_datetimeinstead of the saved one. Canvas already sendsstream: true,load_project_skills: trueand an explicit tool list, so its payload is unaffected; a hand-rolled REST client that relied on those staying off would see the change.build_agent=False, so a dangling ref still fails fast without paying for a container) and once after, with the container's own runtime answer (GET /server_infofor browser availability,openhands_managedskills). Previously the host's answers were used for a container agent, which meant an ACP profile in Docker got no managed skills./server_infoadvertisesunified_agent_launch_v1.Not fixed here, found while testing: a condenser's
max_tokensinheritance keys offmodel_fields_set, so a stored profile (loaded from JSON, every field "set") does not inherit the LLM's token limit while an in-memory one does. It is consistent across today's product paths (both read persisted JSON) and predates this PR, so I left it alone rather than widen the diff.Follow-ups, per #5141:
defaultname rule, the LLM-mismatch and dangling-ref fallbacks, theuse-llm-configuredcopy and client-side agent assembly, and send<RUNTIME_SERVICES>viaagent_launch_additions.prepare_agent_launch, stop overwriting fields the profile carries, surface resolution failures.default-profile refresh from globalagent_settingsbelongs with the canvas step: doing it before the settings pages edit the active profile would just let it drift again.profile_launch.pyis superseded byagent_launch.py.🤖 Generated with Claude Code
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:dbd6b27-pythonRun
All tags pushed for this build
About Multi-Architecture Support
dbd6b27-python) is a multi-arch manifest supporting both amd64 and arm64dbd6b27-python-amd64) are also available if needed