Repository navigation
feat: copy the run's agent profile into its cloud sandbox - #614
simonrosenberg wants to merge 2 commits into
Conversation
|
Warning Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it. |
|
The cloud path checks out on Beta, so I've added the results under How to Test → Cloud path on Beta.
Everything created on Beta has been deleted. Marking this ready for review. I'm an AI agent (Claude Code) helping @simonrosenberg. |
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
Moves plugin-spec conversation startup from the sandbox's own agent-server to the profile's owner at dispatch: a new ExecutionBackend.create_conversation (local POST /api/conversations, cloud POST /api/v1/app-conversations + start-task polling), a dispatcher step that fails the run when launched_agent_profile is not the requested profile, and a runner that attaches instead of creating. Fixes the OHE 404 in #601.
What I checked on head 3302d12
- Scope belongs here: no cross-repo move; this is the automation service's own dispatch/runner path.
POST /api/conversationsbodies built by the local backend validate against the pinned SDK'sStartConversationRequest(checked directly with the workspace venv), including the subject-derivedconversation_id,user_id, plugins (withrepo_path), and observability fields.- Provenance read is sound:
launched_agent_profileis projected ontoConversationInfoby the pinned agent-server's_compose_conversation_info, and the paired agent-server (theopenhands-sdk==1.56.0pin) exposes it._check_launched_profilecorrectly treats a present-but-different or absent profile as a failure and does not classify it transient. - Error classification is grounded: 429 is retried (
_retry), 401/403 re-mints (_with_auth_retry), other 4xx becomes a non-transientConversationLaunchErrorwith its status code, and 5xx/transport fall through torun_status_detail_from_exception. Failure path releases the context before_fail, consistent with the existing dispatch-failure contract. - Script runners are untouched (
create_conversationis only reached forrunner == "conversation"), andAUTOMATION_CONVERSATION_IDis still only set for such runs. - Tests:
tests/test_backends.py(43 passed) andtests/test_plugin_specs.pypass locally. The newTestExecuteRunPluginSpecConversationcases need Postgres via testcontainers, which this environment cannot start (no Docker socket); GitHub Actionsunit-testsandbackendare green on the exact head.
Residual, explicitly deferred (not blocking this PR)
The runner still calls workspace.get_secrets(agent_profile_id=...) against the sandbox agent-server. On OHE the enterprise profile id is not in that server's store, so the call 404s and the runner injects no secrets; profile secret_refs/tool allowlists are not enforced there. This is unchanged behavior from the base and is acknowledged in the PR's Notes and in #601/#600 as pending OpenHands/enterprise#763. It is not over-exposure, so I am not treating it as a defect of this PR, but the corresponding #601 acceptance criterion is not met by this change alone.
Head is at the requested SHA, CI is green, and the change is focused and well tested. No material correctness, security, or compatibility defect found on the current head.
✅ APPROVED
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ConversationLaunch: |
There was a problem hiding this comment.
can we be defined in the sdk layer so we can dedup potentially across the other services
There was a problem hiding this comment.
Gone in the rework (5ddbf46). The service no longer starts the conversation, so ConversationLaunch and the backends' create_conversation are removed.
I'm an AI agent (Claude Code) helping @simonrosenberg.
| if not automation_conversation_id: | ||
| raise RuntimeError( | ||
| "Plugin-spec conversation runner requires AUTOMATION_CONVERSATION_ID" | ||
| ) |
There was a problem hiding this comment.
can we just generate one if its not provided, seems like better ux than hard failing
There was a problem hiding this comment.
Gone in the rework (5ddbf46). The runner uses RemoteConversation.create again, and AUTOMATION_CONVERSATION_ID is back to its meaning on main: set only for continue_conversation runs, and the runner creates the conversation either way.
I'm an AI agent (Claude Code) helping @simonrosenberg.
| print(f" plugins loaded: {len(plugin_sources)}") | ||
| # attach() does not register the conversation, and the completion | ||
| # callback reports only a registered one. | ||
| workspace.register_conversation(str(conversation.id)) |
There was a problem hiding this comment.
we intentionally don't manually register the conversation this way, the sdk layer should be responsible for doing it
out of curiosity, why aren't we using RemoteConversation.create? If this runs in sandbox in cloud, there should still be an agent server there to handle the conversation requests
There was a problem hiding this comment.
Agreed. The rework (5ddbf46) drops the manual call: the runner uses RemoteConversation.create again, which registers the conversation itself. Your question is answered in the cloud.py thread: the service now copies the profile into the sandbox instead.
I'm an AI agent (Claude Code) helping @simonrosenberg.
| if ctx.sandbox_id and ctx.api_url and ctx.api_key: | ||
| await delete_sandbox(client, ctx.api_url, ctx.api_key, ctx.sandbox_id) | ||
|
|
||
| async def create_conversation( |
There was a problem hiding this comment.
Ah so the issue is that the agent server, when started in a remote sandbox, doesn't already contain the agent profiles. So we need to go through the app servers conversation creation flow, instead of interacting with the agent server directly
Seems like the fix may be to instantiate the agent server with the correct profiles from the start?
There was a problem hiding this comment.
The reason I say this is because RemoteConversation in the SDK is how we prescribe users start conversations, and having them reimplement this API in their custom scripts seems inconvenient
There was a problem hiding this comment.
I guess new profiles can be added after the agent server started, so that's a bit unclear.
There was a problem hiding this comment.
ah actually this agent-server only lives during the automation run so no problem with user creating profiles
There was a problem hiding this comment.
Agreed, and it's simpler. An automation's sandbox lives for one run, so a profile copied in at dispatch can't go stale. Here's how I'd rework this PR:
- Cloud backend, once the sandbox is ready: if the run has an agent profile, copy it into the sandbox's agent-server.
- The profile:
GET /api/agent-profiles/{name}on the app server, thenPOST /api/agent-profiles/{name}on the sandbox. - Its
llm_profile_ref, with the key, fromGET /api/v1/users/me?expose_secrets=true, thenPOST /api/profiles/{name}. This is what_seed_sandbox_profilesdoes when the app server starts a conversation. - The MCP servers in its
mcp_server_refs, from the same response.
- The profile:
- Runner: back to
RemoteConversation.create(StartConversationRequest(agent_profile_id=…)). Custom scripts in the run can do the same. - Removed: the backends'
create_conversation,ConversationLaunch, the dispatcher step, the provenance check andAUTOMATION_CONVERSATION_ID. This also settles your other three comments, sincecreate()registers the conversation itself. - Local mode: unchanged from
main; the profile is already in the agent-server.
One wrinkle: the sandbox's agent-server assigns a new id when it creates a profile. The service reads that id back and passes it as AUTOMATION_AGENT_PROFILE_ID.
Side effects:
- The sandbox's agent-server resolves the profile itself, so on OHE the profile's tools and
secret_refsapply without fix(agent-profiles): align Enterprise launches with the SDK enterprise#763. A missing profile, LLM profile or MCP server fails the launch instead of silently falling back ([Agent Profile] Fail closed when a scoped launch profile cannot be resolved enterprise#759). - The app server no longer starts the conversation. Its record comes from the agent-server's webhook, as for preset runs.
Does this match what you had in mind?
I'm an AI agent (Claude Code) helping @simonrosenberg.
malhotra5
left a comment
malhotra5
left a comment
There was a problem hiding this comment.
Marking as request changes till comments are resolved, imho there is an alternate solution we can use
3302d12 to
5ddbf46
Compare
simonrosenberg
changed the title
On OHE/Cloud a run's sandbox starts with an empty agent-profile store, so `RemoteConversation.create(agent_profile_id=...)` returns 404 there. Before the run's command starts, the cloud backend now copies the automation's agent profile from the app server into the sandbox's agent server, along with the LLM profile and MCP servers it references, and passes the id the agent server assigned as AUTOMATION_AGENT_PROFILE_ID. A profile that can't be copied fails the run at dispatch. Local mode is unchanged. The stock plugin-spec runner passes its secrets in the start request, so the agent server applies the profile's secret_refs to them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s conversation The agent-server image ships without its built-in sub-agent definitions (OpenHands/software-agent-sdk#5380), so on OHE a profile with task_tool_set offered task but every call failed with "none registered". Forward them in the start request, as enterprise does for the conversations it starts. Also restore update_secrets for the run's secrets: the agent server applies the launched profile's secret_refs there too, so moving them into the start request changed nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
360cff9 to
669cddb
Compare
⏳ Review waiting on checksThe current head
No action is needed. The scheduled scan retries once every check on the head reports a conclusion. This is an automated check - no AI was used to generate this comment. |
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).
Reviewed the live head 669cddba26ce (the PR title/body have been reworked since the review request in the trigger: the approach is now "the service copies the run's agent profile into the sandbox's agent server before the run starts", not "the service starts the conversation"). The change fits this repository: dispatch/scheduling and backend orchestration live here, and the runner-side change is a single start-request field.
What I verified against the workspace code and the pinned dependencies (openhands-sdk/openhands-workspace 1.56.0):
- Secret scoping actually holds. The runner's
get_secrets(agent_profile_id=...)still passes the parameter to an endpoint that ignores it, but the filtering that matters happens on the sandbox's agent server:event_service.update_secretsfilters incoming secrets bystored.launched_agent_profile.allows_secret(...)at 1.56.0, and the request path that launches fromagent_profile_idpopulateslaunched_agent_profile.AUTOMATION_SESSION_URLgoes through the same filter. So a profile'ssecret_refsis enforced regardless of what the app-server secrets endpoint returns. - The resolved-LLM-key read is guarded.
GET /api/v1/users/me?expose_secrets=truecallsvalidate_session_key_ownershipagainst the sandbox session key the backend sends, so the copy cannot read another user's key. - The app-server contract the new code depends on exists.
/api/agent-profileslist items carryid/name,GET /api/agent-profiles/{name}returns the SDKAgentProfilebody, andsettings.agent_settings.mcp_configis thedict[str, MCPServer]shape thatPATCH /api/settings(agent_settings_diff) accepts on the sandbox agent server. - The runner's
agent_definitionsis valid and idempotent. It is a field ofStartConversationRequestat 1.56.0, is forwarded byRemoteConversation.create, and the server registers definitions withregister_agent_if_absent, so re-sending the built-ins is a no-op once the image ships them.openhands-tools(which providesdiscover_builtin_agents) is installed by the presetsetup.shat the same version the runner is generated against. - Failure handling is on the existing dispatch path:
release_contextthen_fail, so a refused copy leaves no sandbox and no command; the new tests assert exactly that (bash_command_id is None,operation=seed_agent_profile). - CI on this head:
unit-tests,backend,Build and Push Automation Image, and the PR-title/description checks are allsuccess. The new backend tests exercise real request/response paths viahttpx.MockTransportrather than asserting on mocks of the code under test.
Non-blocking observation, demonstrated on this head: the new step rewrites only the env var, so env_vars["AUTOMATION_AGENT_PROFILE_ID"] becomes the sandbox's id while resolved_runtime["agent_profile_id"] (set in step 5 and persisted) keeps the app-server id. The runner reads only the env var, so launch behavior is correct, but the stored run record is inconsistent for anyone debugging a run.
No material correctness, security, compatibility, or acceptance-criteria defect found on this head.
✅ APPROVED
This branch has not been deployed
3 participants
Why
A plugin-spec conversation run starts with
RemoteConversation.create(agent_profile_id=…)on its workspace's agent server. That works locally, where the agent server holds the user's profiles. On OHE/Cloud the profiles live on the app server and the run's new sandbox starts with an empty profile store, so the launch returns 404Agent profile with id '…' not found(#600). Profile-based automations, and with them sub-agents (#557), have no working path on OHE. Custom scripts that start a conversation from the run's profile hit the same 404.Summary
seed_agent_profile. Once the sandbox is running, the service copies the automation's agent profile into the sandbox's agent server, together with what the profile references:GET /api/agent-profilesandGET /api/agent-profiles/{name}on the app server, thenPOST /api/agent-profiles/{name}on the sandbox;llm_profile_ref, key included, from the app server'sGET /api/v1/users/me?expose_secrets=true(which requires the sandbox's session key), thenPOST /api/profiles/{name}. This is what the app server's_seed_sandbox_profilesdoes when it starts a conversation itself;mcp_server_refs(all of them when it's null, as the resolver does), viaPATCH /api/settings.seed_agent_profilebefore the run's command starts and passes the returned id asAUTOMATION_AGENT_PROFILE_ID. The sandbox's agent server assigns its own id when it creates a profile, so the backend reads that id back.RemoteConversation.create, and now forwards the SDK's built-in sub-agent definitions (agent_definitions) in the start request. Agent-server images ship without them (Docker conversation runtime: built-in sub-agents are not registered (task tool fails with "none registered") software-agent-sdk#5380), so on OHE a profile withtask_tool_setwas offeredtaskbut every call failed withUnknown agent 'general-purpose'. Available types: none registered. Enterprise forwards the same definitions for the conversations it starts.Issue Number
Fixes #601
Design
FAILEDat dispatch (operation=seed_agent_profile), or the launch fails on the dangling reference. No silent fallbackWhy the service copies the profile, rather than the app server starting the conversation. This keeps
RemoteConversation.create, the SDK's way to start a conversation, working in the sandbox for the stock runner and for custom scripts alike. A sandbox lives for one run, so the copy can't go stale. And because the sandbox's agent server resolves the profile itself, the profile's tools andsecret_refsapply on OHE today, without waiting for OpenHands/enterprise#763.Run state. A failed copy goes through the existing dispatch-failure path:
release_contextdeletes the sandbox, then_failrecords astatus_detailclassified byrun_status_detail_from_exception. App-server calls retry 429s and re-mint the key on 401/403, using the backend's existing helpers.Secrets. Unchanged: the runner still sends them with
update_secrets. The sandbox's agent server applies the launched profile'ssecret_refsthere too, so a profile only gets the secrets it names. One consequence, the same onmain: a profile whosesecret_refsdoesn't nameAUTOMATION_SESSION_URLdoesn't get it.Code:
seed_agent_profileinbackends/cloud.pyandbackends/base.py, step 6 indispatcher.py, and the runner's start request inpresets/plugin_spec/sdk_main.py.How to Test
Unit tests.
uv run python -m pytest tests/on this branch, rebased on main 1.23.1: 2125 passed, 7 skipped. Pre-commit is clean. New tests:TestCloudSeedAgentProfileintests/test_backends.py: what's copied and where;mcp_server_refsnull copies every server; an unknown profile or LLM profile writes nothing to the sandbox; local returns the id unchanged.TestExecuteRunSeedsAgentProfileintests/test_dispatcher.py: the command gets the sandbox's id; a failed copy fails the run before any command and releases the sandbox; a run without a profile copies nothing.tests/test_plugin_specs.py: the generated runner forwardsdiscover_builtin_agents()asagent_definitions.agent_definitions. In each case one test failed.End-to-end, local mode. Native processes, no Docker: this branch stacked with #615,
openhands-agent-server==1.56.0, a mock LLM, and theprompt-taskplugin (OpenHands/extensions#799) on a profile withtask_tool_set.COMPLETED, launched from the chosen profile.terminal, file_editor, task_tracker, task_tool_set. The model is offeredtask. A mock LLM doesn't exercise the delegation itself; the OHE run below does.FAILEDatpreflight_plugin, with no command and no conversation.End-to-end on OHE Replicated (fleet instance shared-8, 2026-10-20).
simon/qa-automation-plugin-spec-profiles.qa-deepwikiandqa-unused;QA_SECRET_ALLOWEDandQA_SECRET_BLOCKED;terminal, file_editor, task_tracker, task_tool_set, the install'sDefaultLLM profile,mcp_server_refs: [qa-deepwiki]andsecret_refs: [QA_SECRET_ALLOWED].prompt-task(feat(plugins): add prompt-task, the stock plugin for prompt automations extensions#799) with that profile,keep_alive: trueso the sandbox could be read afterwards, dispatched manually. The prompt asks the agent to start one sub-agent that repliesPONG.COMPLETED,conversation_idrecorded; enterprise lists the conversation with all its eventsdefaultplus the copied profile, under the sandbox's own id; the conversation launched from that idDefault, with its key: the conversation runsopenhands/claude-sonnet-4-5-20250929qa-deepwikionly; the model was offered its 3 toolsQA_SECRET_ALLOWEDonlytaskbash-runner,code-explorer,general-purpose; the model delegated togeneral-purpose, which answeredPONGFAILEDat dispatch,operation=seed_agent_profile,AgentProfileSeedError; sandbox deleted, no conversationFAILEDatpreflight_plugin, no sandboxThe first OHE build of this approach (without 669cddb) passed every row except
task. Its description read "None: every registered agent uses tools this agent does not have", and both of the model's calls failed withUnknown agent 'general-purpose'. Available types: none registered. In that sandbox,POST /api/sub-agentswith only built-ins requested returned an empty list. With the definitions forwarded in a start request, the same sandbox delegated and gotPONG.Notes
meta_profile_ref. The launch fails on the dangling reference rather than falling back.launched_agent_profile: null, since enterprise learns about the conversation only through the webhook. The sandbox's record has the launched profile.I'm an AI agent (Claude Code) helping @simonrosenberg.
🤖 Generated with Claude Code