[AgentProfile][agent-server] agent_profile_id at conversation start + LaunchedProfile provenance#3784
Conversation
… LaunchedProfile provenance (#3720) Accept `agent_profile_id: UUID | None` on `StartConversationRequest` (mutually exclusive with `agent`/`agent_settings`, carry-only `exclude=True`). The SDK validator enforces exclusivity; resolution runs server-side so stores and the cipher are available. Resolution path (`_resolve_agent_from_profile`): - look up profile name from stable UUID via `AgentProfileStore.list_summaries()` - load and decrypt the profile (`AgentProfileStore.load`) - pull global `mcp_config` from `PersistedSettings` (already-decrypted by the file-store cipher context) - call `resolve_agent_profile()` → `AgentSettingsConfig.create_agent()` - return the built `AgentBase` and a `LaunchedProfile(id, revision)` provenance `_start_conversation` calls this via `asyncio.to_thread()` before the agent- requirement check, injects the resolved agent onto the request, and stamps the provenance on `StoredConversation`. `_compose_conversation_info` projects it onto `ConversationInfo`. `ProfileNotFound` maps to 404; `DanglingMcpServerRef` maps to 422 in the router. `LaunchedProfile` is a new declared field on both `StoredConversation` and `ConversationInfo` so ts-client `deriveSwitchPlan` can identify the active profile without fragile settings-comparison. Tests (20): SDK mutual-exclusivity, `_resolve_agent_from_profile` helper (OpenHands, ACP, unknown-id, dangling-ref), service-layer propagation, router HTTP mapping, and `LaunchedProfile` JSON round-trip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Profile-based conversation start works end-to-end through the real agent-server API, including provenance stamping, persistence across restart, validation, and profile-resolution error responses.
Does this PR achieve its stated goal?
Yes. The PR sets out to allow agent_profile_id at conversation start, resolve it server-side into an agent, and expose launched_profile provenance on conversation info; I verified this with real HTTP requests against a running local agent-server. On main, the same profile-start payload is rejected with 422 because no agent/agent_settings is present; on this PR it returns 201 with an Agent built from the saved profile and launched_profile: {profile_id, revision} in both create and GET responses, and the same provenance survives a server restart.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; local agent-server started with isolated temp HOME/profile stores. |
| CI Status | |
| Functional Verification | ✅ Real /api/conversations requests verified profile launch, direct launch compatibility, 404/422 error paths, and restart persistence. |
Functional Verification
Test 1: Before/after profile-based conversation start
Step 1 — Establish baseline without the fix:
Checked out main at b0be8a52, started python -m openhands.agent_server on localhost, and sent the same real API request a client would send:
curl -sS -w '
HTTP_STATUS:%{http_code}
' -H 'Content-Type: application/json' --data @/tmp/start_profile_payload.json http://127.0.0.1:18385/api/conversationsObserved:
{
"http_status": 422,
"detail": [{
"msg": "Value error, Either `agent` or `agent_settings` must be provided"
}]
}This confirms the old behavior: agent_profile_id alone could not start a conversation.
Step 2 — Apply the PR's changes:
Checked out agent-profile-conv-start at ae6e1150, saved a real LLMProfileStore entry and OpenHandsAgentProfile in an isolated temp HOME, then started the PR agent-server.
Step 3 — Re-run with the fix in place:
Ran the same POST against the PR server:
curl -sS -w '
HTTP_STATUS:%{http_code}
' -H 'Content-Type: application/json' --data '{"agent_profile_id":"5bb6063d-4bf1-442b-93ad-c99c3ff5f9a5","workspace":{"kind":"LocalWorkspace","working_dir":"/tmp/ohqa-workspace-pr-3784"}}' http://127.0.0.1:18384/api/conversationsObserved relevant response fields:
{
"http_status": 201,
"id": "21195fd5-9619-43ad-a02d-ccbfb0523901",
"execution_status": "idle",
"launched_profile": {
"profile_id": "5bb6063d-4bf1-442b-93ad-c99c3ff5f9a5",
"revision": 7
},
"agent_kind": "Agent",
"agent_model": "gpt-4o-mini",
"agent_usage_id": "qa"
}This shows the server accepted agent_profile_id, resolved it into an OpenHands Agent, and returned the promised launched-profile provenance.
Test 2: ConversationInfo exposes provenance and persists across restart
Baseline: prior to the PR, the profile-start request cannot create a conversation, so there is no ConversationInfo.launched_profile to retrieve.
With the PR: after the 201 response above, I fetched the conversation via HTTP:
curl -sS -w '
HTTP_STATUS:%{http_code}
' http://127.0.0.1:18384/api/conversations/21195fd5-9619-43ad-a02d-ccbfb0523901Observed:
{
"http_status": 200,
"id": "21195fd5-9619-43ad-a02d-ccbfb0523901",
"launched_profile": {
"profile_id": "5bb6063d-4bf1-442b-93ad-c99c3ff5f9a5",
"revision": 7
}
}I then created a fresh PR conversation, stopped the PR server, restarted it, and fetched that conversation again:
{
"http_status": 200,
"id": "f8bd172c-689b-4b13-a7a8-63dcec4046eb",
"launched_profile": {
"profile_id": "5bb6063d-4bf1-442b-93ad-c99c3ff5f9a5",
"revision": 7
}
}This confirms the provenance is exposed through the API and survives normal server reload/persistence.
Test 3: Error and compatibility paths
Unknown profile ID:
{
"http_status": 404,
"detail": "Agent profile with id '00000000-0000-0000-0000-000000000001' not found"
}agent_profile_id combined with agent_settings:
{
"http_status": 422,
"detail": [{
"msg": "Value error, `agent_profile_id` is mutually exclusive with `agent` and `agent_settings`"
}]
}Profile referencing missing MCP server:
{
"http_status": 422,
"detail": {
"message": "MCP server ref(s) not present in the user's MCP config: 'missing-mcp-server'",
"dangling_mcp_server_refs": ["missing-mcp-server"]
}
}Existing direct agent_settings start path:
{
"http_status": 201,
"execution_status": "idle",
"launched_profile": null,
"agent_kind": "Agent",
"agent_model": "gpt-4o-mini",
"agent_usage_id": "direct"
}These responses match the PR description's intended 404/422 mappings and show the existing direct-start path still works with launched_profile: null.
Issues Found
None from functional QA.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
…ered id→name scans Three callers open-coded the same O(n) list_summaries scan to resolve a stable UUID to a profile name. Add a single store method so callers don't each re-implement it: store.name_for_id(profile_id) -> str | None Update _resolve_agent_from_profile in conversation_service to use it, and fix test mocks accordingly (mock name_for_id instead of list_summaries). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
⚠️ QA Report: PASS WITH ISSUES
The agent-server API path works end-to-end for starting from agent_profile_id, but the documented SDK model_dump() usage for a profile-only request currently crashes.
Does this PR achieve its stated goal?
Mostly yes. I started a real local agent-server, seeded a real LLM profile + agent profile in the profile store, and POST /api/conversations with only agent_profile_id returned 201 with the expected launched_profile.profile_id and revision; GET returned the same provenance even after a server restart. The documented error mappings also worked (ProfileNotFound → 404, dangling MCP ref → 422). However, the PR description's SDK carry-only serialization example does not work: a profile-only StartConversationRequest(...).model_dump(mode="json") raises PydanticSerializationError because agent remains None.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and local server launched on both main and PR branch |
| CI Status | 🟡 GitHub checks: 34 successful, 3 skipped, 1 in-progress qa-changes check when observed |
| Functional Verification |
Functional Verification
Test 1: Conversation start from agent_profile_id (before/after)
Step 1 — Establish baseline on origin/main:
Seeded a real temporary profile store, started python -m openhands.agent_server --host 127.0.0.1 --port 8765, then ran:
curl -sS -w "\nHTTP_STATUS:%{http_code}\n" -H "Content-Type: application/json" --data @/tmp/qa-profile-payload.json http://127.0.0.1:8765/api/conversationsObserved:
{"detail":[{"type":"value_error","msg":"Value error, Either `agent` or `agent_settings` must be provided"}]}
HTTP_STATUS:422This confirms the pre-PR server did not accept agent_profile_id as a conversation-start source.
Step 2 — Apply the PR's changes:
Checked out agent-profile-conv-start at ae6e1150bc9abdcb6af5a55b5216689e4f1672c7 and restarted the same local server with the same seeded profile store.
Step 3 — Re-run with the PR:
Ran the same POST /api/conversations request with only agent_profile_id and workspace. Observed:
{
"id": "10a35fef-b6c1-47d4-bffc-b533463631dd",
"execution_status": "idle",
"launched_profile": {
"profile_id": "3a4261cd-4915-4075-84c7-928074268de7",
"revision": 3
},
"agent": {"llm": {"model": "gpt-4o-mini", "usage_id": "qa-llm"}}
}
HTTP_STATUS:201This shows the server resolved the stored profile into an agent and stamped the expected LaunchedProfile provenance.
Test 2: Provenance is readable and persists across restart
After the successful create, ran:
curl -sS -w "\nHTTP_STATUS:%{http_code}\n" http://127.0.0.1:8765/api/conversations/10a35fef-b6c1-47d4-bffc-b533463631ddObserved the same launched_profile with profile_id=3a4261cd-4915-4075-84c7-928074268de7 and revision=3 with HTTP_STATUS:200.
Then stopped and restarted the PR server and ran the same GET again. The response still included:
"launched_profile": {
"profile_id": "3a4261cd-4915-4075-84c7-928074268de7",
"revision": 3
}
HTTP_STATUS:200This confirms the provenance is stored, not only held in memory.
Test 3: Profile resolution error mappings
Unknown profile ID:
curl -sS -w "\nHTTP_STATUS:%{http_code}\n" -H "Content-Type: application/json" --data '{"agent_profile_id":"11111111-1111-4111-8111-111111111111","workspace":{"kind":"LocalWorkspace","working_dir":"/tmp/oh-qa-workspace.qBzIQJ"}}' http://127.0.0.1:8765/api/conversationsObserved:
{"detail":"Agent profile with id '11111111-1111-4111-8111-111111111111' not found"}
HTTP_STATUS:404Dangling MCP server ref profile:
{"detail":{"message":"MCP server ref(s) not present in the user's MCP config: 'missing-mcp-server'","dangling_mcp_server_refs":["missing-mcp-server"]}}
HTTP_STATUS:422This matches the PR's claimed 404 and 422 behavior.
Test 4: SDK request object behavior
Ran the PR description's direct SDK usage pattern:
req = StartConversationRequest(
agent_profile_id=profile_id,
workspace=LocalWorkspace(working_dir="/tmp/oh-qa-sdk"),
)
print("profile-only request valid:", req.agent is None, req.agent_profile_id == profile_id)
print(req.model_dump(mode="json"))Observed:
profile-only request valid: True True
model_dump succeeded: False
model_dump error type: PydanticSerializationError
model_dump error excerpt: Error calling function `_serialize_with_mcp_handling`: AttributeError: 'NoneType' object has no attribute 'model_dump'
mutual exclusivity enforced: True
This confirms profile-only validation works, but the documented carry-only serialization behavior does not.
Issues Found
- 🟠 Issue: Profile-only
StartConversationRequest.model_dump(mode="json")crashes withPydanticSerializationError, so the SDK "How to Test" / carry-only serialization path is not functional even though the raw HTTP API path works.
This review was created by an AI agent (OpenHands) on behalf of the user.
…est is dumped
`StartConversationRequest` with `agent_profile_id` set holds `agent=None`
(the server resolves it). Pydantic's `model_dump` calls the `AgentBase`
model_serializer with `self=None`, crashing. The `@field_serializer("agent",
mode="wrap")` short-circuits before the model_serializer fires and returns
`None` directly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified with real local agent-server HTTP requests that agent_profile_id conversation start works end-to-end and preserves launched_profile provenance.
Does this PR achieve its stated goal?
Yes. On main, the same POST /api/conversations payload with only agent_profile_id was rejected with HTTP 422 because the server still required agent/agent_settings; on this PR branch, the server accepted the profile ID, resolved the stored profile into an agent, returned HTTP 201, and exposed launched_profile.profile_id + revision in the response. I also restarted the PR server and confirmed GET /api/conversations/{id} still returned the provenance, and verified the promised 404/422 error mappings.
| Phase | Result |
|---|---|
| Environment Setup | ✅ uv environment was usable; local agent-server started on both base and PR branches. |
| CI Status | ⏳ At check time: 21 successful, 7 pending, 1 skipped, 0 failing. I did not run tests/linters locally. |
| Functional Verification | ✅ Profile launch, provenance response/persistence, error mappings, and direct-agent regression path all behaved as claimed. |
Functional Verification
Test 1: Before/after profile launch through POST /api/conversations
Step 1 — Establish baseline on main without the fix:
Started the local agent-server from main, created an LLM profile and an OpenHands agent profile through the HTTP API, then posted a conversation start request containing only agent_profile_id:
POST /api/conversations
{"agent_profile_id":"50bc6860-88cf-4311-8a0f-2265114add00","workspace":{"working_dir":"/tmp/oh-qa-main-work","kind":"LocalWorkspace"}}Observed:
{"detail":[{"type":"value_error","loc":["body"],"msg":"Value error, Either `agent` or `agent_settings` must be provided"}]}
HTTP:422
This confirms the previous behavior did not allow starting a conversation from an agent profile ID alone.
Step 2 — Apply the PR's changes:
Switched back to agent-profile-conv-start at 474950f7f3ed87825ebb39d3b4197045c816c967, started the local agent-server, and used the same API flow.
Step 3 — Re-run with the PR branch:
POST /api/conversations
{"agent_profile_id":"50bc6860-88cf-4311-8a0f-2265114add00","workspace":{"working_dir":"/tmp/oh-qa-work","kind":"LocalWorkspace"}}Observed selected response fields:
{
"id": "cdc30258-e0ab-49c3-b162-faeae0185692",
"execution_status": "idle",
"launched_profile": {
"profile_id": "50bc6860-88cf-4311-8a0f-2265114add00",
"revision": 0
},
"agent.llm.usage_id": "qa-llm"
}
HTTP:201This shows the PR resolves the stored profile server-side, starts the conversation, and projects the launched profile provenance in the response.
Test 2: launched_profile persists across server restart
Started a fresh PR-branch conversation from the profile ID and captured the response:
{
"id": "03b9a2c4-0402-4211-97fc-457e9759189b",
"launched_profile": {
"profile_id": "50bc6860-88cf-4311-8a0f-2265114add00",
"revision": 1
}
}
HTTP:201Then stopped and restarted the PR server and fetched the same conversation:
GET /api/conversations/03b9a2c4-0402-4211-97fc-457e9759189bObserved selected fields:
{
"id": "03b9a2c4-0402-4211-97fc-457e9759189b",
"launched_profile": {
"profile_id": "50bc6860-88cf-4311-8a0f-2265114add00",
"revision": 1
}
}
HTTP:200This confirms the provenance is stored, not just present in the initial in-memory response.
Test 3: Error mappings for bad profile references
Unknown profile ID:
{"detail":"Agent profile with id '00000000-0000-0000-0000-000000000999' not found"}
HTTP:404
Profile with missing MCP server reference:
{"detail":{"message":"MCP server ref(s) not present in the user's MCP config: 'missing-server'","dangling_mcp_server_refs":["missing-server"]}}
HTTP:422
These match the PR's claimed ProfileNotFound → 404 and DanglingMcpServerRef → 422 behavior.
Test 4: Direct agent start regression check
Started a conversation with a concrete agent payload and no agent_profile_id:
{
"id": "af45ed77-55c3-4c46-aa8c-9b2dde63b4fd",
"execution_status": "idle",
"launched_profile": null
}
HTTP:201This confirms existing direct-agent starts still work and do not get false profile provenance.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable - The overall data flow is straightforward and the provenance model is useful, but one configuration/cipher bug in the profile-resolution path keeps this from approval.
[CRITICAL ISSUES]
- See inline comment in
conversation_service.py: profile launch readsPersistedSettingsthrough an unconfigured settings-store singleton, so MCP server refs/global settings can come from the wrong persistence dir or fail to decrypt if this is the first settings-store access after boot. - Existing unresolved thread: the profile-only
StartConversationRequestserialization path still needs a clear answer; at current headmodel_dump(mode="json")dropsagent_profile_idand emitsagent: null, so that dump is not a usable start-conversation payload. I’m not opening a duplicate inline thread.
[TESTING]
- Ran
uv run pytest tests/agent_server/test_agent_profile_conv_start.py -qlocally: 20 passed.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This touches conversation start, server-side profile resolution, persisted metadata, and MCP/secret-bearing configuration. Existing direct-agent starts appear unaffected, but profile launches need to reliably use the server-configured settings/cipher path.
VERDICT:
❌ Needs rework: Fix the settings-store initialization/cipher path before this should be approved; keep the existing serialization review thread resolved or addressed as well.
KEY INSIGHT:
The new agent_profile_id field is only a pointer; all correctness depends on resolving that pointer against the same configured stores/cipher the rest of the server uses.
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 (e.g., "Security concerns about X do not apply here because Y"). 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 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
VascoSch92
left a comment
VascoSch92
left a comment
There was a problem hiding this comment.
Profile-only StartConversationRequest can't be serialized model_dump()/model_dump_json() raise PydanticSerializationError.
The agent field is typed non-optional AgentBase but defaults to None, and AgentBase's @model_serializer (base.py:341) calls self.model_dump() which Pydantic invokes even when the value is None. Before this PR, _require_agent guaranteed agent was never None, so the path was unreachable; making agent=None valid (profile-only) now exposes it.
This means the PR's own "How to Test" snippet (req.model_dump(mode="json")) crashes. The live feature isn't broken — the TS client posts raw JSON and the server resolves agent before its own model_dump (conversation_service.py:687 runs before :751) — but it's a real footgun for any Python caller, and the "carry-only" framing implies dump should work. Note test_agent_profile_id_is_excluded_from_model_dump only dumps an agent request, so it hides this.
Suggest guarding the AgentBase serializer against a None/non-agent self (or omitting agent when None), plus a test that dumps a profile-only request.
…ngleton The free function _resolve_agent_from_profile called get_settings_store() without a Config, so if it ran before any other handler had initialised the singleton the cipher was missing and MCP config values stayed encrypted. Extract _load_mcp_config() which builds a short-lived FileSettingsStore from the ConversationService's conversations_dir + cipher — exactly mirroring _get_persistence_dir() in the store module — and pass the result in as an explicit mcp_config parameter, removing the singleton dependency from the resolver entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🔍 Review in progress…\n\nWe are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Verified the agent-server API now starts conversations from agent_profile_id, exposes launched_profile, preserves it across restart, and maps profile-resolution errors as described.
Does this PR achieve its stated goal?
Yes. On main, the same POST /api/conversations payload with only agent_profile_id is rejected before profile resolution with 422 and the message "Either agent or agent_settings must be provided". On this PR branch, the server accepted an existing profile UUID, resolved it into an Agent, returned launched_profile.profile_id + revision from both POST and GET, preserved that provenance after server restart, and returned the promised 404 / 422 responses for unknown profile and dangling MCP references.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; no test suite, linter, formatter, or pre-commit run by QA |
| CI Status | qa-changes were still pending at last refresh |
| Functional Verification | ✅ Real agent-server processes were started and exercised through HTTP requests plus SDK object construction |
Functional Verification
Test 1: Conversation start from an existing profile UUID
Step 1 — Reproduce / establish baseline without the fix:
Ran git switch main && bash /tmp/qa_agent_profile_baseline.sh && git switch agent-profile-conv-start. The script created a real LLM profile + agent profile, started python -m openhands.agent_server, then posted to /api/conversations with only agent_profile_id:
--- POST existing profile on base branch ---
HTTP/1.1 422 Unprocessable Content
{"detail":[{"type":"value_error","loc":["body"],"msg":"Value error, Either `agent` or `agent_settings` must be provided",...}]}
This confirms the old behavior: even with an existing profile UUID, the API could not start a conversation from agent_profile_id alone.
Step 2 — Apply the PR's changes:
Switched back to agent-profile-conv-start at 5b3e6dd83558561e7eb097c19b1fed453052c19f.
Step 3 — Re-run with the fix in place:
Ran bash /tmp/qa_agent_profile_api.sh, which created real persisted profiles, started the agent-server, posted the same shape of request, then fetched the conversation:
PROFILE_ID=93e92b89-5944-44d5-a1ad-26c86f4fee13
--- POST existing profile ---
{"id":"0f4bb216-e3a0-46b7-8738-92f6569ed36f", ... "launched_profile":{"profile_id":"93e92b89-5944-44d5-a1ad-26c86f4fee13","revision":11}, ... "agent":{..."kind":"Agent"}}
--- GET conversation info ---
{"id":"0f4bb216-e3a0-46b7-8738-92f6569ed36f", ... "launched_profile":{"profile_id":"93e92b89-5944-44d5-a1ad-26c86f4fee13","revision":11}, ...}
--- validate launched_profile values ---
{'profile_id_matches': True, 'revision': 11, 'agent_kind': 'Agent', 'execution_status': 'idle'}
This shows the new API behavior works end-to-end: the profile ID resolves server-side, a conversation is created idle, and ConversationInfo includes the expected provenance.
Test 2: Profile resolution error mapping
Step 1 — Baseline:
The base-branch command in Test 1 rejects agent_profile_id before profile resolution, so profile-specific 404/422 behavior cannot be reached there.
Step 2 — Apply the PR's changes:
Used the PR branch server.
Step 3 — Re-run with error inputs:
The existing-profile script also posted an unknown UUID, and a separate script created a profile with mcp_server_refs=["missing-mcp-server"]:
--- POST unknown profile ---
HTTP/1.1 404 Not Found
{"detail":"Agent profile with id '00000000-0000-0000-0000-000000000001' not found"}
--- POST profile with missing MCP server ref ---
HTTP/1.1 422 Unprocessable Content
{"detail":{"message":"MCP server ref(s) not present in the user's MCP config: 'missing-mcp-server'","dangling_mcp_server_refs":["missing-mcp-server"]}}
This confirms the router maps ProfileNotFound to 404 and DanglingMcpServerRef to 422 with the missing-ref list exposed.
Test 3: LaunchedProfile survives stored conversation reload
Step 1 — Baseline:
Before the PR, conversations cannot be launched from agent_profile_id, so no profile provenance is written for this path.
Step 2 — Apply the PR's changes:
Used the PR branch server with isolated HOME and OH_PERSISTENCE_DIR.
Step 3 — Re-run after restart:
Ran bash /tmp/qa_agent_profile_restart.sh, which started the server, created the conversation from a profile, stopped the exact server PID, restarted the server against the same persistence directory, and fetched the conversation by ID:
PROFILE_ID=29358fa0-63b9-4f04-8818-6647af1d19af
CONV_ID=e16474a1-325b-4dfc-a322-9a607522fbd5
--- GET conversation after restart ---
{"id":"e16474a1-325b-4dfc-a322-9a607522fbd5", ... "launched_profile":{"profile_id":"29358fa0-63b9-4f04-8818-6647af1d19af","revision":17}, ...}
--- validate restarted response ---
{'profile_id_matches_after_restart': True, 'revision_after_restart': 17, 'execution_status': 'idle'}
This confirms the provenance is not only returned from the live service; it is stored and loaded back after restart.
Test 4: SDK request behavior
Ran a short user-style Python script constructing StartConversationRequest directly:
profile_id accepted: True
agent is None before server resolution: True
agent_profile_id in model_dump json: False
mutual exclusion error contains expected text: True
This confirms the SDK carries agent_profile_id for local construction/validation, leaves agent unset for server resolution, excludes the profile ID from serialized request data, and enforces mutual exclusivity with a direct agent.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Solid, focused change. The carry-only agent_profile_id (exclude=True) plus the wrap-serializer guard at request.py:297-301 makes the profile-only round-trip work end-to-end — I confirmed locally that StartConversationRequest(agent_profile_id=…, workspace=…).model_dump(mode="json") produces a clean dict with agent: null and no agent_profile_id key. Validation (mutually exclusive / agent_profile_id) is concise; the HTTP status mapping (404 / 422) is well-isolated in the router; new tests cover the persistence round-trip and ConversationInfo propagation.
One small clean-up: the two except … : raise blocks in conversation_service.py:321-324 are no-ops. ProfileNotFound and DanglingMcpServerRef inherit from Exception, not ValueError/TypeError, so neither is caught by the (TypeError, ValueError) arm below. Drop them and let the exceptions propagate naturally.
See inline for details.
- Drop dead `except ProfileNotFound: raise` / `except DanglingMcpServerRef: raise` blocks — these exceptions are not ValueError/TypeError so they were no-ops; let them propagate naturally. - Drop `_load_mcp_config` helper and its duplicated persistence-dir derivation. Instead, initialise the settings-store singleton with the server cipher in `ConversationService.get_instance(config)` (one-time, before any request can run), then call `get_settings_store()` in `_start_conversation` — no duplication, correct cipher guaranteed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable - the server-side profile resolution flow is straightforward, but the API request model currently hides the new field from normal serialization.
[CRITICAL ISSUES]
- See the inline comment on
agent_profile_id: clients usingStartConversationRequest.model_dump(...)will drop the UUID before makingPOST /api/conversations, so the new start-from-profile path is not usable through the SDK request model.
[TESTING GAPS]
- Add a regression test for serializing a profile-start request as an outgoing API payload; it should prove
agent_profile_idremains present for transport while still being omitted from persistedStoredConversationmetadata.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This touches the public conversation-start REST contract and server-side agent construction. The implementation is mostly additive and well-scoped, but losing the profile UUID at the SDK serialization boundary would break the new feature for Python/API clients that rely on the shared request model.
VERDICT:
❌ Needs follow-up before approval: Keep the field serializable for API transport and exclude it only on the persistence path.
KEY INSIGHT:
The same Pydantic model is serving both as the inbound API contract and the persisted conversation base, so persistence-only exclusions must not be encoded on the API field itself.
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 (e.g., "Security concerns about X do not apply here because Y"). 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 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Profile-based conversation start works end-to-end: I created real stored profiles, started the actual agent-server, POSTed /api/conversations with agent_profile_id, and verified provenance survives a server restart.
Does this PR achieve its stated goal?
Yes. The PR set out to accept a stable agent profile UUID at conversation start, resolve it server-side, stamp LaunchedProfile provenance, expose it through conversation info, and map resolution errors. I verified that a real HTTP POST with an existing profile UUID returns 201 Created with launched_profile.profile_id and revision, the same data is returned by GET after restarting the server, unknown profiles return 404, and dangling MCP refs return 422 with dangling_mcp_server_refs.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; no test suite, linter, formatter, or pre-commit run was executed. |
| CI Status | 🟡 At observation time: 22 completed checks successful, 7 skipped, 7 still in progress (qa-changes + Docker image builds); no failures observed. |
| Functional Verification | ✅ Real SDK and HTTP API behavior matched the PR description, including persistence and error paths. |
Functional Verification
Test 1: Baseline vs PR SDK request behavior
Step 1 — Reproduce / establish baseline without the fix:
Checked out main (cf563075) and ran a real SDK construction with only agent_profile_id:
git checkout main
uv run python - <<'PY'
from uuid import uuid4
from pydantic import ValidationError
from openhands.sdk.conversation.request import StartConversationRequest
from openhands.sdk.workspace import LocalWorkspace
try:
StartConversationRequest(agent_profile_id=uuid4(), workspace=LocalWorkspace(working_dir='/tmp/oh-qa-baseline'))
except ValidationError as exc:
print('BASELINE_VALIDATION_ERROR')
print(str(exc).split('\n')[1])
PYObserved:
BASELINE_VALIDATION_ERROR
Value error, Either `agent` or `agent_settings` must be provided
This confirms the prior SDK entry point could not represent the profile-only start request.
Step 2 — Apply the PR's changes:
Checked out agent-profile-conv-start at 9074b800.
Step 3 — Re-run with the fix in place:
uv run python - <<'PY'
from uuid import UUID
from pydantic import ValidationError
from openhands.sdk import LLM, Agent
from openhands.sdk.conversation.request import StartConversationRequest
from openhands.sdk.workspace import LocalWorkspace
profile_id = UUID('11111111-1111-4111-8111-111111111111')
req = StartConversationRequest(agent_profile_id=profile_id, workspace=LocalWorkspace(working_dir='/tmp/oh-qa-workspace'))
print('PR_PROFILE_ONLY_VALID', req.agent_profile_id, req.agent)
print('PR_MODEL_DUMP_HAS_AGENT_PROFILE_ID', 'agent_profile_id' in req.model_dump(mode='json'))
try:
StartConversationRequest(agent_profile_id=profile_id, agent=Agent(llm=LLM(model='gpt-4o-mini'), tools=[]), workspace=LocalWorkspace(working_dir='/tmp/oh-qa-workspace'))
except ValidationError as exc:
print('PR_MUTUAL_EXCLUSIVITY_ERROR')
print(str(exc).split('\n')[1])
PYObserved:
PR_PROFILE_ONLY_VALID 11111111-1111-4111-8111-111111111111 None
PR_MODEL_DUMP_HAS_AGENT_PROFILE_ID False
PR_MUTUAL_EXCLUSIVITY_ERROR
Value error, `agent_profile_id` is mutually exclusive with `agent` and `agent_settings`
This shows the PR enables profile-only request construction, keeps agent_profile_id carry-only for dumps, and enforces the claimed exclusivity.
Test 2: End-to-end HTTP start from stored profile + persisted provenance
Step 1 — Establish baseline:
Before this PR, Test 1 shows the SDK could not construct the profile-only start request; no server-side profile resolution/provenance path was available through the conversation start contract.
Step 2 — Apply the PR's changes and create real profile data:
Using isolated HOME=/tmp/oh-qa-profile-home, I saved a real LLM profile and agent profile through the public stores:
llm_base_dir /tmp/oh-qa-profile-home/.openhands/profiles
agent_base_dir /tmp/oh-qa-profile-home/.openhands/agent-profiles
summaries [{'id': '11111111-1111-4111-8111-111111111111', 'name': 'qa-profile', 'agent_kind': 'openhands', 'revision': 7, 'llm_profile_ref': 'qa-llm', 'mcp_server_refs': None}]
name_for_id qa-profile
Then I started the actual server:
HOME=/tmp/oh-qa-profile-home OH_CONVERSATIONS_PATH=/tmp/oh-qa-conversations OH_SESSION_API_KEYS_0=qa-key uv run python -m openhands.agent_serverThe server responded on http://127.0.0.1:8000/server_info with HTTP/1.1 200 OK.
Step 3 — Re-run with the fix in place:
curl -i -X POST http://127.0.0.1:8000/api/conversations \
-H 'Content-Type: application/json' \
-H 'X-Session-API-Key: qa-key' \
--data '{"agent_profile_id":"11111111-1111-4111-8111-111111111111","workspace":{"kind":"LocalWorkspace","working_dir":"/tmp/oh-qa-workspace"}}'Observed key response fields:
HTTP/1.1 201 Created
"launched_profile":{"profile_id":"11111111-1111-4111-8111-111111111111","revision":7}
"agent":{"llm":{"model":"gpt-4o-mini", ... "usage_id":"qa-profile-llm"}}
I then restarted the exact server process and retrieved the same conversation:
curl -i http://127.0.0.1:8000/api/conversations/8c9e9f39-bf33-46ca-805a-a597af17a936 \
-H 'X-Session-API-Key: qa-key'Observed:
HTTP/1.1 200 OK
AFTER_RESTART_LAUNCHED_PROFILE {'profile_id': '11111111-1111-4111-8111-111111111111', 'revision': 7}
AFTER_RESTART_AGENT_MODEL gpt-4o-mini
This confirms the server resolved the UUID to a stored profile, built the agent from the referenced LLM profile, returned the LaunchedProfile in conversation info, and persisted it across process restart.
Test 3: HTTP error mappings for resolution failures
Step 1 — Establish baseline:
The PR description claims new profile resolution errors should map to HTTP 404 and 422. These paths are new to the profile-start API.
Step 2 — Apply the PR's changes:
With the same running PR server, I POSTed an unknown profile UUID and a real profile with mcp_server_refs=['missing-server'].
Step 3 — Run error cases:
--- missing profile ---
HTTP/1.1 404 Not Found
{"detail":"Agent profile with id '33333333-3333-4333-8333-333333333333' not found"}
--- dangling mcp ---
HTTP/1.1 422 Unprocessable Content
{"detail":{"message":"MCP server ref(s) not present in the user's MCP config: 'missing-server'","dangling_mcp_server_refs":["missing-server"]}}
This confirms the user-facing API maps ProfileNotFound to 404 and DanglingMcpServerRef to 422 with the expected missing-reference list.
Test 4: Existing direct-agent conversation start still works
Step 1 — Establish baseline:
Direct agent starts are existing behavior and should not stamp launched-profile provenance.
Step 2 — Apply the PR's changes:
Generated a normal StartConversationRequest(agent=Agent(...)) payload from the SDK and POSTed it to the same real server.
Step 3 — Run direct-agent start:
DIRECT_PAYLOAD_HAS_AGENT_PROFILE_ID False
HTTP/1.1 201 Created
DIRECT_STATUS_LAUNCHED_PROFILE None
DIRECT_AGENT_MODEL gpt-4o-mini
This confirms existing direct agent starts still succeed and leave launched_profile as null.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
…3784) Remove exclude=True from agent_profile_id on the API request model so model_dump() includes it for HTTP transport. Override the field with exclude=True on StoredConversation (the persistence model) instead, and exclude it explicitly when building the request_data dict so the mutual-exclusivity validator is not triggered after resolution. Add regression tests covering both the transport and persistence paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Fixed in 7726004: removed |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Started the agent-server on both main and this PR branch, then verified via real HTTP requests that conversations can now launch from an agent_profile_id, expose launched_profile provenance, and return the promised resolver errors.
Does this PR achieve its stated goal?
Yes. On main, the same POST /api/conversations payload with an existing profile UUID is rejected because only agent or agent_settings are accepted. On this PR branch, the same user flow returns 201 Created, resolves the profile into an OpenHands agent using the stored LLM profile, and both POST/GET conversation responses include launched_profile.profile_id + revision; the persisted meta.json also stores launched_profile without re-persisting raw agent_profile_id. I also verified the two claimed error mappings over HTTP: unknown profile UUID returns 404, and dangling MCP refs return 422 with dangling_mcp_server_refs.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed and the server started from the documented uv run python -m openhands.agent_server command. |
| CI Status | ⏳ 21 checks passing, 8 pending at observation time (mostly Agent Server image builds plus this QA workflow), 3 skipped. |
| Functional Verification | ✅ Real API requests confirmed the new profile-launch behavior and error handling. |
Functional Verification
Test 1: Start a conversation from an existing agent profile UUID
Step 1 — Establish baseline on origin/main:
Started the server with isolated persistence on port 8124, created an LLM profile and agent profile through the real API, then ran:
curl -i -X POST http://127.0.0.1:8124/api/conversations \
-H 'Content-Type: application/json' \
--data-binary @start_conversation.jsonPayload:
{"agent_profile_id":"955e4d88-1368-4c76-a14c-341629185592","workspace":{"kind":"LocalWorkspace","working_dir":"/tmp/oh-qa-main-C30xFZ/workspace"},"autotitle":false}Observed:
HTTP/1.1 422 Unprocessable Content
{"detail":[{"type":"value_error","loc":["body"],"msg":"Value error, Either `agent` or `agent_settings` must be provided", ...}]}This confirms the old behavior: even with a real stored profile UUID, conversation start could not use agent_profile_id.
Step 2 — Apply the PR's changes:
Checked out agent-profile-conv-start at 7726004067ab9b2b20536b4951f20c6bdd15fdbf, re-synced the environment, and started the same server on port 8123 with isolated persistence.
Step 3 — Re-run with the fix in place:
Created the same kind of LLM profile and agent profile through /api/profiles/qa-llm and /api/agent-profiles/qa-agent, then ran:
curl -i -X POST http://127.0.0.1:8123/api/conversations \
-H 'Content-Type: application/json' \
--data-binary @start_conversation.jsonPayload:
{"agent_profile_id":"955e4d88-1368-4c76-a14c-341629185592","workspace":{"kind":"LocalWorkspace","working_dir":"/tmp/oh-qa-pr-GekZwc/workspace"},"autotitle":false}Observed:
HTTP/1.1 201 Created
{"id":"8acdc193-75d5-4671-8023-6b117c3a6cfd", ..., "launched_profile":{"profile_id":"955e4d88-1368-4c76-a14c-341629185592","revision":2}, "agent":{"llm":{"model":"openai/gpt-4o-mini", ...}}}This confirms the PR resolves the profile server-side and starts a stored conversation with provenance.
Test 2: Conversation info and persistence expose provenance correctly
After the PR-created conversation, ran:
curl -f http://127.0.0.1:8123/api/conversations/8acdc193-75d5-4671-8023-6b117c3a6cfdSelected response fields:
{
"id": "8acdc193-75d5-4671-8023-6b117c3a6cfd",
"launched_profile": {
"profile_id": "955e4d88-1368-4c76-a14c-341629185592",
"revision": 2
},
"agent_model": "openai/gpt-4o-mini",
"agent_profile_id_present_in_response": false
}Then inspected the persisted conversation metadata side effect:
{
"launched_profile": {
"profile_id": "955e4d88-1368-4c76-a14c-341629185592",
"revision": 2
},
"agent_profile_id_present_in_metadata": false
}This shows clients get the intended LaunchedProfile provenance, while the carry-only agent_profile_id is not persisted back into stored conversation metadata.
Test 3: Profile resolution errors map to the claimed HTTP statuses
With the PR server running, ran an unknown-profile launch request:
curl -i -X POST http://127.0.0.1:8123/api/conversations \
-H 'Content-Type: application/json' \
--data-binary @start_unknown.jsonObserved:
HTTP/1.1 404 Not Found
{"detail":"Agent profile with id '11e46f0b-02bb-44fa-91ab-b13978a30f7a' not found"}Then created an agent profile referencing missing-mcp-server and started a conversation with its UUID:
HTTP/1.1 422 Unprocessable Content
{"detail":{"message":"MCP server ref(s) not present in the user's MCP config: 'missing-mcp-server'","dangling_mcp_server_refs":["missing-mcp-server"]}}This confirms the promised ProfileNotFound → 404 and DanglingMcpServerRef → 422 behavior through the real router.
Issues Found
None.
This QA review was created by an AI agent (OpenHands) on behalf of the user.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Re-review of PR #3784 (HEAD 7726004, on top of fixes 9074b80 and 474950f):
This PR cleanly addresses all three concerns flagged in my earlier review:
-
Profile-only request serialization (request.py L289). The new
@field_serializer("agent", mode="wrap")onStartConversationRequestshort-circuits whenvalue is None, soStartConversationRequest(agent_profile_id=..., workspace=...).model_dump(mode="json")no longer raisesPydanticSerializationError. I verified this end-to-end in a fresh venv: the dump returnsagent=None, includesagent_profile_id, and round-trips throughjson.dumps. -
Settings-store singleton initialization order.
start_conversationnow callsget_settings_store()(parameterless), whichget_instance()routes to the lazily-created singleton that has already been bound to the mergedConfigduring deferred-init (init_router.py L209). No more risk of anunconfigured singletonfor the cipher. -
No more duplicated persistence-dir derivation. The review-feedback commit 9074b80 dropped the helper that was recreating the settings-store path; the service now uses
get_settings_store().load()uniformly, and themcp_configderivation commit 5b3e6dd pulls the cipher from the same singleton. Both fixes share one source of truth.
Additional observations:
StoredConversation(StartConversationRequest)withagent_profile_idexcluded keeps the persisted JSON shape backwards-compatible (confirmed viamodel_dump(..., exclude={"agent_profile_id"})).name_for_idis only invoked after the store returns the profile, so it is safe against None UUIDs from the request.- CI on HEAD is green for all functional checks (tests, breakage checks, persisted-settings compatibility, agent-server multi-OS builds). The only "in-progress" jobs are multi-arch image pushes, which are post-merge artifacts.
Approving. Nice iteration cycle.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
@VascoSch92 good catch — fixed in 474950f. Added a |
|
@enyst it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
|
@enyst it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
|
@enyst it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
|
@enyst it looks like you haven't created an OpenHands account yet. Please sign up at OpenHands Cloud and try again. |
4 participants
HUMAN:
Agent profile ID resolution at conversation start: accepts a stable UUID, resolves server-side via the profile store and cipher, stamps LaunchedProfile provenance on the stored conversation, and maps resolution errors (ProfileNotFound → 404, DanglingMcpServerRef → 422).
AGENT:
20/20 new tests pass locally. Full agent-server suite: 1241 passed, 1 pre-existing flake (TestAutoTitle — confirmed failing on main before this branch).
Why
ts-client
deriveSwitchPlan(epic #3713) needs to know which agent profile launched a conversation without doing fragile settings-comparison. The resolution logic also needs cipher + store access that the SDK layer doesn't have.Summary
agent_profile_id: UUID | Noneadded toStartConversationRequest(carry-onlyexclude=True, mutually exclusive withagent/agent_settings; SDK enforces exclusivity only)_resolve_agent_from_profile(profile_id, cipher): UUID→name vialist_summaries(), load profile, pull globalmcp_configfromPersistedSettings, callresolve_agent_profile(), return builtAgentBase+LaunchedProfile(id, revision)_start_conversationruns resolution viaasyncio.to_thread(), injects resolved agent into request, stampslaunched_profileonStoredConversation;LaunchedProfileprojected ontoConversationInfoProfileNotFound → 404,DanglingMcpServerRef → 422Issue Number
Closes #3720
How to Test
End-to-end: start a conversation with
agent_profile_idset to an existing profile UUID viaPOST /api/conversations;GET /api/conversations/{id}should showlaunched_profile.profile_idandlaunched_profile.revisionin the response.Video/Screenshots
(unit tests cover all paths; no UI changes)
Type
Notes
request.secretschannel — resolver is untouched._resolve_agent_from_profileuses lazy imports to avoid circular imports betweenagent_serverandsdk.profiles; test patch targets are the source modules (e.g.openhands.sdk.profiles.agent_profile_store.AgentProfileStore), not the service module.Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang: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:7726004-pythonRun
All tags pushed for this build
About Multi-Architecture Support
7726004-python) is a multi-arch manifest supporting both amd64 and arm647726004-python-amd64) are also available if needed