From 302960d1c7cc2d9302c06f987d3c05454f62f1d3 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Wed, 17 Jun 2026 22:22:52 +0200 Subject: [PATCH 1/8] [AgentProfile][agent-server] active_agent_profile_id + /api/agent-profiles router + migration seed Expose the AgentProfile store over HTTP and add a separate active pointer, mirroring the LLM profiles router. AgentProfile activation is pointer-only: unlike the LLM /activate it must not write agent_settings (creation-time-only contract). Runs parallel to #3717; POST /{id}/materialize is a fast-follow once the resolver lands. - PersistedSettings.active_agent_profile_id (additive, defaults None; no schema bump) threaded through update(), SettingsResponse/Update, and the settings_router GET/PATCH passthrough. - /api/agent-profiles router: CRUD + POST /{id}/activate (pointer only) with FK error mapping (ProfileReferenced/FileExists -> 409, FileNotFound -> 404, Timeout -> 503, ValueError -> 400). - Lazy migration seed: first GET on an empty store seeds one default profile from agent_settings and sets the active pointer. - LLM profiles_router delete/rename now enforce the agent-profile FK (delete referenced LLM profile -> 409 naming referrers; rename cascades). Closes #3719 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 379 ++++++++++++++++++ .../openhands/agent_server/api.py | 2 + .../agent_server/persistence/models.py | 17 +- .../openhands/agent_server/profiles_router.py | 26 +- .../openhands/agent_server/settings_router.py | 8 +- .../openhands/sdk/settings/api_models.py | 8 + .../test_agent_profiles_router.py | 363 +++++++++++++++++ tests/agent_server/test_profiles_router.py | 93 ++++- tests/agent_server/test_settings_router.py | 50 ++- 9 files changed, 928 insertions(+), 18 deletions(-) create mode 100644 openhands-agent-server/openhands/agent_server/agent_profiles_router.py create mode 100644 tests/agent_server/test_agent_profiles_router.py diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py new file mode 100644 index 0000000000..b50b315006 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -0,0 +1,379 @@ +"""HTTP endpoints for managing named ``AgentProfile`` launch specs. + +Mirrors ``profiles_router.py`` (the LLM ``/api/profiles`` router) but serves the +reference-bearing :class:`~openhands.sdk.profiles.AgentProfile` union and keeps a +*separate* active pointer (``active_agent_profile_id``). Activation here is +pointer-only — unlike the LLM ``/activate`` it must **not** write +``agent_settings`` (the creation-time-only contract). + +``POST /{id}/materialize`` is a fast-follow once the resolver (#3717) lands; it +is deliberately not implemented here so this router ships independently. +""" + +from collections.abc import Iterator +from contextlib import contextmanager +from typing import Annotated, Any + +from fastapi import APIRouter, HTTPException, Path, Request, status +from pydantic import BaseModel, Field, ValidationError + +from openhands.agent_server._secrets_exposure import ( + build_expose_context, + get_cipher, + get_config, + parse_expose_secrets_header, + translate_missing_cipher, +) +from openhands.agent_server.persistence import ( + PersistedSettings, + get_settings_store, +) +from openhands.sdk.logger import get_logger +from openhands.sdk.profiles import ( + ACPAgentProfile, + AgentProfileStore, + OpenHandsAgentProfile, + ProfileLimitExceeded, + ProfileReferenced, + validate_agent_profile, +) +from openhands.sdk.profiles.agent_profile_store import PROFILE_NAME_PATTERN +from openhands.sdk.settings import AgentSettingsConfig + + +logger = get_logger(__name__) + +agent_profiles_router = APIRouter(prefix="/agent-profiles", tags=["Agent Profiles"]) + +MAX_AGENT_PROFILES = 50 + +# Name the lazily-seeded migration profile (and its LLM ref fallback). +SEED_PROFILE_NAME = "default" + +ProfileName = Annotated[ + str, + Path(min_length=1, max_length=64, pattern=PROFILE_NAME_PATTERN), +] +ProfileId = Annotated[str, Path(min_length=1, max_length=128)] + + +class AgentProfileInfo(BaseModel): + """Summary projection of a stored profile (no secret instantiation).""" + + id: str | None = None + name: str + agent_kind: str = "openhands" + revision: int | None = None + llm_profile_ref: str | None = None + mcp_server_refs: list[str] | None = None + + +class AgentProfileListResponse(BaseModel): + profiles: list[AgentProfileInfo] + active_agent_profile_id: str | None = None + + +class AgentProfileDetailResponse(BaseModel): + name: str + profile: dict[str, Any] + + +class AgentProfileMutationResponse(BaseModel): + name: str + message: str + + +class ActivateAgentProfileResponse(BaseModel): + id: str + message: str + agent_settings_applied: bool = False + + +class RenameAgentProfileRequest(BaseModel): + new_name: str = Field( + ..., + min_length=1, + max_length=64, + pattern=PROFILE_NAME_PATTERN, + ) + + +@contextmanager +def _store_errors() -> Iterator[None]: + """Map ``AgentProfileStore`` / FK errors to HTTP responses.""" + try: + yield + except ProfileReferenced as e: + # Names the referrers so the caller knows what to detach first. + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) + except FileExistsError as e: + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) + except FileNotFoundError as e: + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) + except TimeoutError: + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Agent profile store is busy. Please retry.", + ) + except ValueError as e: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) + + +def _build_seed_profile( + agent_settings: AgentSettingsConfig, active_llm_profile: str | None +) -> OpenHandsAgentProfile | ACPAgentProfile: + """Build one conservative ``AgentProfile`` from the current ``agent_settings``. + + Carries only the cleanly-overlapping fields; ``mcp_server_refs=None`` exposes + all of the user's MCP servers. An OpenHands profile references the active LLM + profile (falling back to ``"default"`` when none is set — a soft ref the + resolver checks at materialize time). + """ + if agent_settings.agent_kind == "acp": + return ACPAgentProfile( + name=SEED_PROFILE_NAME, + acp_server=agent_settings.acp_server, + acp_model=agent_settings.acp_model, + acp_session_mode=agent_settings.acp_session_mode, + acp_prompt_timeout=agent_settings.acp_prompt_timeout, + mcp_server_refs=None, + ) + return OpenHandsAgentProfile( + name=SEED_PROFILE_NAME, + llm_profile_ref=active_llm_profile or SEED_PROFILE_NAME, + agent=agent_settings.agent, + enable_sub_agents=agent_settings.enable_sub_agents, + tool_concurrency_limit=agent_settings.tool_concurrency_limit, + mcp_server_refs=None, + ) + + +def _seed_default_profile( + store: AgentProfileStore, request: Request, settings: PersistedSettings +) -> None: + """Persist one default profile and point ``active_agent_profile_id`` at it.""" + profile = _build_seed_profile(settings.agent_settings, settings.active_profile) + with _store_errors(): + store.save(profile, max_profiles=MAX_AGENT_PROFILES) + + profile_id = str(profile.id) + settings_store = get_settings_store(get_config(request)) + + def set_pointer(s: PersistedSettings) -> PersistedSettings: + s.active_agent_profile_id = profile_id + return s + + settings_store.update(set_pointer) + logger.info(f"Seeded default agent profile '{profile.name}' (id={profile_id})") + + +def _summary_id_for_name(store: AgentProfileStore, name: str) -> str | None: + """Return the stable id of the profile stored under ``name``, if present.""" + with _store_errors(): + for summary in store.list_summaries(): + if summary.get("name") == name: + sid = summary.get("id") + return str(sid) if sid is not None else None + return None + + +@agent_profiles_router.get("", response_model=AgentProfileListResponse) +async def list_agent_profiles(request: Request) -> AgentProfileListResponse: + """List all stored agent profiles and the active pointer. + + On the first call against an empty store with no active pointer, lazily + seeds one default profile from the current ``agent_settings`` and activates + it (the one-time migration that replaces a dedicated seed step). + """ + config = get_config(request) + settings_store = get_settings_store(config) + settings = settings_store.load() or PersistedSettings() + + store = AgentProfileStore() + with _store_errors(): + existing = store.list() + + if not existing and settings.active_agent_profile_id is None: + _seed_default_profile(store, request, settings) + settings = settings_store.load() or settings + + with _store_errors(): + summaries = store.list_summaries() + + return AgentProfileListResponse( + profiles=[AgentProfileInfo(**s) for s in summaries], + active_agent_profile_id=settings.active_agent_profile_id, + ) + + +@agent_profiles_router.get("/{name}", response_model=AgentProfileDetailResponse) +async def get_agent_profile( + request: Request, name: ProfileName +) -> AgentProfileDetailResponse: + """Get a stored profile. + + Use the ``X-Expose-Secrets`` header to control ``skills[].mcp_tools`` secret + exposure (``encrypted`` / ``plaintext``); absent, those values are masked. + """ + expose_mode = parse_expose_secrets_header(request) + cipher = get_cipher(request) + + store = AgentProfileStore() + with _store_errors(): + profile = store.load(name, cipher=cipher) + + context = build_expose_context(expose_mode, cipher) + with translate_missing_cipher(): + payload = profile.model_dump(mode="json", context=context) + + return AgentProfileDetailResponse(name=name, profile=payload) + + +@agent_profiles_router.post( + "/{name}", + response_model=AgentProfileMutationResponse, + status_code=status.HTTP_201_CREATED, +) +async def save_agent_profile( + request: Request, name: ProfileName, body: dict[str, Any] +) -> AgentProfileMutationResponse: + """Save an ``AgentProfile`` under ``name`` (overwriting a namesake). + + The path ``name`` is authoritative — it overrides any ``name`` in the body. + With ``OH_SECRET_KEY`` configured, ``skills[].mcp_tools`` secrets are + encrypted at rest; otherwise they are redacted. Returns 409 if creating a + new profile would exceed ``MAX_AGENT_PROFILES``. + """ + cipher = get_cipher(request) + try: + profile = validate_agent_profile({**body, "name": name}) + except (ValidationError, ValueError, TypeError) as e: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"Invalid agent profile: {type(e).__name__}", + ) + + store = AgentProfileStore() + try: + with _store_errors(): + store.save(profile, cipher=cipher, max_profiles=MAX_AGENT_PROFILES) + except ProfileLimitExceeded: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + f"Agent profile limit reached ({MAX_AGENT_PROFILES}). " + "Delete a profile before saving a new one." + ), + ) + + logger.info(f"Saved agent profile '{name}'") + return AgentProfileMutationResponse( + name=name, message=f"Agent profile '{name}' saved" + ) + + +@agent_profiles_router.delete("/{name}", response_model=AgentProfileMutationResponse) +async def delete_agent_profile( + request: Request, name: ProfileName +) -> AgentProfileMutationResponse: + """Delete a stored profile (idempotent). + + If the deleted profile was the active one, ``active_agent_profile_id`` is + cleared. + """ + store = AgentProfileStore() + deleted_id = _summary_id_for_name(store, name) + + with _store_errors(): + store.delete(name) + + if deleted_id is not None: + config = get_config(request) + settings_store = get_settings_store(config) + settings = settings_store.load() or PersistedSettings() + if settings.active_agent_profile_id == deleted_id: + + def clear_pointer(s: PersistedSettings) -> PersistedSettings: + s.active_agent_profile_id = None + return s + + settings_store.update(clear_pointer) + logger.info(f"Cleared active pointer for deleted profile '{name}'") + + logger.info(f"Deleted agent profile '{name}'") + return AgentProfileMutationResponse( + name=name, message=f"Agent profile '{name}' deleted" + ) + + +@agent_profiles_router.post( + "/{name}/rename", response_model=AgentProfileMutationResponse +) +async def rename_agent_profile( + name: ProfileName, body: RenameAgentProfileRequest +) -> AgentProfileMutationResponse: + """Rename a stored profile atomically. + + The stable ``id`` is preserved, so an active pointer (keyed on ``id``) + survives the rename untouched. Returns 404 if the source is missing, 409 if + ``new_name`` is taken. + """ + store = AgentProfileStore() + with _store_errors(): + store.rename(name, body.new_name) + + if name == body.new_name: + message = f"Agent profile '{name}' unchanged (same name)" + else: + message = f"Agent profile '{name}' renamed to '{body.new_name}'" + logger.info(message) + return AgentProfileMutationResponse(name=body.new_name, message=message) + + +@agent_profiles_router.post( + "/{profile_id}/activate", response_model=ActivateAgentProfileResponse +) +async def activate_agent_profile( + request: Request, profile_id: ProfileId +) -> ActivateAgentProfileResponse: + """Activate a profile by its stable ``id`` — pointer only. + + Sets ``active_agent_profile_id`` and nothing else: unlike the LLM + ``/activate``, this does **not** write ``agent_settings`` (the + creation-time-only contract). Returns 404 if no stored profile has that id. + """ + store = AgentProfileStore() + with _store_errors(): + known_ids = { + str(s["id"]) for s in store.list_summaries() if s.get("id") is not None + } + if profile_id not in known_ids: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Agent profile with id '{profile_id}' not found", + ) + + config = get_config(request) + settings_store = get_settings_store(config) + + def set_pointer(settings: PersistedSettings) -> PersistedSettings: + settings.active_agent_profile_id = profile_id + return settings + + try: + settings_store.update(set_pointer) + except (OSError, PermissionError): + logger.error("Failed to activate agent profile - file I/O error") + raise HTTPException(status_code=500, detail="Failed to activate agent profile") + except RuntimeError as e: + logger.error(f"Failed to activate agent profile: {e}") + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail="Settings file is corrupted or encrypted with a different key", + ) + + logger.info(f"Activated agent profile id '{profile_id}'") + return ActivateAgentProfileResponse( + id=profile_id, + message=f"Agent profile '{profile_id}' activated", + ) diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index 1ff882a20f..176d58ec09 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -16,6 +16,7 @@ from fastapi.staticfiles import StaticFiles from starlette.requests import Request +from openhands.agent_server.agent_profiles_router import agent_profiles_router from openhands.agent_server.auth_router import auth_router from openhands.agent_server.bash_router import bash_router from openhands.agent_server.bash_service import get_default_bash_event_service @@ -316,6 +317,7 @@ def _add_api_routes(app: FastAPI, config: Config) -> None: api_router.include_router(settings_router) api_router.include_router(workspaces_router) api_router.include_router(profiles_router) + api_router.include_router(agent_profiles_router) # /api/auth/* mints workspace cookies and requires the header to bootstrap, # so it lives under the header-only auth group. api_router.include_router(auth_router) diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index e5c1b6f714..2f51685e3d 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -55,6 +55,7 @@ class SettingsUpdatePayload(TypedDict, total=False): conversation_settings_diff: dict[str, Any] misc_settings_diff: dict[str, Any] active_profile: str | None + active_agent_profile_id: str | None def _deep_merge( @@ -140,6 +141,14 @@ class PersistedSettings(BaseModel): default=None, description="Name of the currently active LLM profile.", ) + active_agent_profile_id: str | None = Field( + default=None, + description=( + "Stable id of the currently active AgentProfile. Distinct from " + "active_profile (the active LLM profile name); additive with a " + "default, so older settings files load with this as None." + ), + ) misc_settings: dict[str, Any] = Field( default_factory=dict, description=( @@ -165,8 +174,8 @@ def llm_api_key_is_set(self) -> bool: def update(self, payload: SettingsUpdatePayload) -> None: """Apply a batch of changes from a nested dict. - Accepts ``agent_settings_diff``, ``conversation_settings_diff``, and - ``active_profile`` for partial updates. + Accepts ``agent_settings_diff``, ``conversation_settings_diff``, + ``active_profile``, and ``active_agent_profile_id`` for partial updates. ``agent_settings_diff`` is applied via :func:`apply_agent_settings_diff`: RFC 7386 merge-patch semantics with kind-switch awareness. When @@ -248,6 +257,10 @@ def update(self, payload: SettingsUpdatePayload) -> None: # Update active_profile if explicitly provided (including None to clear) if "active_profile" in payload: self.active_profile = payload["active_profile"] + + # Same for the AgentProfile pointer (including None to clear) + if "active_agent_profile_id" in payload: + self.active_agent_profile_id = payload["active_agent_profile_id"] finally: # Clear conv_merged to minimize plaintext exposure window if conv_merged is not None: diff --git a/openhands-agent-server/openhands/agent_server/profiles_router.py b/openhands-agent-server/openhands/agent_server/profiles_router.py index 5a2b5963ff..82c3124eeb 100644 --- a/openhands-agent-server/openhands/agent_server/profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/profiles_router.py @@ -26,6 +26,12 @@ ProfileLimitExceeded, ) from openhands.sdk.logger import get_logger +from openhands.sdk.profiles import ( + AgentProfileStore, + ProfileReferenced, + delete_llm_profile, + rename_llm_profile, +) logger = get_logger(__name__) @@ -227,10 +233,18 @@ async def save_profile( async def delete_profile( request: Request, name: ProfileName ) -> ProfileMutationResponse: - """Delete a saved profile (idempotent).""" + """Delete a saved profile (idempotent). + + Guarded by the agent-profile FK: returns 409 naming the referrers if any + ``AgentProfile`` still cites this LLM profile via ``llm_profile_ref``. + """ store = LLMProfileStore() - with _store_errors(): - store.delete(name) + agent_store = AgentProfileStore() + try: + with _store_errors(): + delete_llm_profile(agent_store, store, name) + except ProfileReferenced as e: + raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) if _set_active_profile_if_matches(request, name, None): logger.info(f"Cleared active_profile for deleted profile '{name}'") logger.info(f"Deleted profile '{name}'") @@ -249,12 +263,14 @@ async def rename_profile( exists. A same-name rename is a verified no-op (still 404s if missing). If the renamed profile is the currently active profile, the active_profile - setting is updated to the new name. + setting is updated to the new name. Any ``AgentProfile.llm_profile_ref`` + citing the old name is cascaded to the new name in lock-step. """ store = LLMProfileStore() + agent_store = AgentProfileStore() try: with _store_errors(): - store.rename(name, body.new_name) + rename_llm_profile(agent_store, store, name, body.new_name) except FileNotFoundError: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, diff --git a/openhands-agent-server/openhands/agent_server/settings_router.py b/openhands-agent-server/openhands/agent_server/settings_router.py index 5c3b410a58..1b7d816f3a 100644 --- a/openhands-agent-server/openhands/agent_server/settings_router.py +++ b/openhands-agent-server/openhands/agent_server/settings_router.py @@ -161,6 +161,7 @@ async def get_settings(request: Request) -> SettingsResponse: ), llm_api_key_is_set=settings.llm_api_key_is_set, active_profile=settings.active_profile, + active_agent_profile_id=settings.active_agent_profile_id, misc_settings=settings.misc_settings, ) @@ -204,8 +205,12 @@ async def update_settings( store = get_settings_store(config) update_data = payload.model_dump(exclude_none=True) + # exclude_none drops an explicit null, so re-add nullable pointers when the + # client set them (including to None) to allow clearing. if "active_profile" in payload.model_fields_set: update_data["active_profile"] = payload.active_profile + if "active_agent_profile_id" in payload.model_fields_set: + update_data["active_agent_profile_id"] = payload.active_agent_profile_id if not update_data: # No updates provided - this is a client error raise HTTPException( @@ -213,7 +218,7 @@ async def update_settings( detail=( "At least one of agent_settings_diff, " "conversation_settings_diff, misc_settings_diff, " - "or active_profile must be provided" + "active_profile, or active_agent_profile_id must be provided" ), ) @@ -269,6 +274,7 @@ def apply_update(settings: PersistedSettings) -> PersistedSettings: conversation_settings=settings.conversation_settings.model_dump(mode="json"), llm_api_key_is_set=settings.llm_api_key_is_set, active_profile=settings.active_profile, + active_agent_profile_id=settings.active_agent_profile_id, misc_settings=settings.misc_settings, ) diff --git a/openhands-sdk/openhands/sdk/settings/api_models.py b/openhands-sdk/openhands/sdk/settings/api_models.py index 52504941fe..c1e272f279 100644 --- a/openhands-sdk/openhands/sdk/settings/api_models.py +++ b/openhands-sdk/openhands/sdk/settings/api_models.py @@ -69,6 +69,10 @@ class SettingsResponse(BaseModel): default=None, description="Name of the currently active LLM profile, if one is selected.", ) + active_agent_profile_id: str | None = Field( + default=None, + description="Stable id of the currently active AgentProfile, if one is set.", + ) misc_settings: dict[str, Any] = Field(default_factory=dict) def get_agent_settings(self) -> AgentSettingsConfig: @@ -113,6 +117,10 @@ class SettingsUpdateRequest(BaseModel): pattern=PROFILE_NAME_PATTERN, description="Name of the active LLM profile to persist; null clears it.", ) + active_agent_profile_id: str | None = Field( + default=None, + description="Stable id of the active AgentProfile to persist; null clears it.", + ) # ── Secrets API Models ──────────────────────────────────────────────────── diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py new file mode 100644 index 0000000000..97c8596982 --- /dev/null +++ b/tests/agent_server/test_agent_profiles_router.py @@ -0,0 +1,363 @@ +"""Tests for agent_profiles_router endpoints. + +Mirrors the ``test_profiles_router`` (LLM) suite, plus the AgentProfile-specific +contracts: a separate ``active_agent_profile_id`` pointer, pointer-only +activation by id (no ``agent_settings`` write), and the lazy migration seed. +""" + +import tempfile +from pathlib import Path +from unittest.mock import patch + +import pytest +from fastapi.testclient import TestClient + +from openhands.agent_server import agent_profiles_router as router_module +from openhands.agent_server.api import create_app +from openhands.agent_server.config import Config +from openhands.agent_server.persistence import reset_stores +from openhands.sdk.profiles import AgentProfileStore, OpenHandsAgentProfile + + +@pytest.fixture +def temp_agent_profiles_dir(): + with tempfile.TemporaryDirectory() as tmpdir: + agent_dir = Path(tmpdir) / "agent-profiles" + agent_dir.mkdir(parents=True, exist_ok=True) + yield agent_dir + + +@pytest.fixture +def temp_settings_dir(): + with tempfile.TemporaryDirectory() as tmpdir: + settings_dir = Path(tmpdir) / "settings" + settings_dir.mkdir(parents=True, exist_ok=True) + yield settings_dir + + +@pytest.fixture +def client(temp_agent_profiles_dir, temp_settings_dir, monkeypatch): + """Test client with isolated agent-profile/settings dirs, no cipher.""" + reset_stores() + monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_settings_dir)) + config = Config(static_files_path=None, session_api_keys=[], secret_key=None) + app = create_app(config) + with patch( + "openhands.agent_server.agent_profiles_router.AgentProfileStore", + lambda: AgentProfileStore(base_dir=temp_agent_profiles_dir), + ): + yield TestClient(app) + reset_stores() + + +@pytest.fixture +def store(temp_agent_profiles_dir): + return AgentProfileStore(base_dir=temp_agent_profiles_dir) + + +# ── Lazy migration seed ───────────────────────────────────────────────────── + + +def test_first_list_seeds_default_profile(client): + """First GET on an empty store seeds exactly one default profile.""" + response = client.get("/api/agent-profiles") + + assert response.status_code == 200 + body = response.json() + assert len(body["profiles"]) == 1 + seeded = body["profiles"][0] + assert seeded["name"] == "default" + assert seeded["agent_kind"] == "openhands" + assert seeded["llm_profile_ref"] == "default" + assert seeded["mcp_server_refs"] is None + # The active pointer is set to the seeded profile's id. + assert body["active_agent_profile_id"] == seeded["id"] + + # And it is persisted into settings. + settings = client.get("/api/settings").json() + assert settings["active_agent_profile_id"] == seeded["id"] + + +def test_seed_is_idempotent(client): + """A second GET does not seed again.""" + first = client.get("/api/agent-profiles").json() + second = client.get("/api/agent-profiles").json() + + assert len(second["profiles"]) == 1 + assert second["active_agent_profile_id"] == first["active_agent_profile_id"] + + +def test_seed_references_active_llm_profile(client): + """The seed references the active LLM profile when one is set.""" + client.patch("/api/settings", json={"active_profile": "my-llm"}) + + body = client.get("/api/agent-profiles").json() + assert body["profiles"][0]["llm_profile_ref"] == "my-llm" + + +def test_seed_acp_when_settings_acp(client): + """ACP agent_settings seed an ACP profile (no llm_profile_ref).""" + client.patch( + "/api/settings", + json={"agent_settings_diff": {"agent_kind": "acp", "acp_server": "codex"}}, + ) + + body = client.get("/api/agent-profiles").json() + seeded = body["profiles"][0] + assert seeded["agent_kind"] == "acp" + assert seeded["llm_profile_ref"] is None + + detail = client.get("/api/agent-profiles/default").json() + assert detail["profile"]["acp_server"] == "codex" + + +def test_no_seed_when_store_nonempty(client, store): + """A non-empty store is never seeded.""" + store.save(OpenHandsAgentProfile(name="mine", llm_profile_ref="x")) + + body = client.get("/api/agent-profiles").json() + names = {p["name"] for p in body["profiles"]} + assert names == {"mine"} + assert body["active_agent_profile_id"] is None + + +# ── CRUD ───────────────────────────────────────────────────────────────────── + + +def test_save_creates_new(client, store): + response = client.post( + "/api/agent-profiles/new-profile", + json={"llm_profile_ref": "base-llm"}, + ) + + assert response.status_code == 201 + assert "saved" in response.json()["message"].lower() + loaded = store.load("new-profile") + assert loaded.llm_profile_ref == "base-llm" + + +def test_save_overwrites_existing(client, store): + store.save(OpenHandsAgentProfile(name="existing", llm_profile_ref="old")) + + response = client.post( + "/api/agent-profiles/existing", + json={"llm_profile_ref": "new"}, + ) + + assert response.status_code == 201 + assert store.load("existing").llm_profile_ref == "new" + + +def test_save_path_name_is_authoritative(client, store): + """The path name overrides any ``name`` in the body.""" + response = client.post( + "/api/agent-profiles/path-name", + json={"name": "body-name", "llm_profile_ref": "x"}, + ) + + assert response.status_code == 201 + assert store.load("path-name").name == "path-name" + with pytest.raises(FileNotFoundError): + store.load("body-name") + + +def test_save_acp_profile(client, store): + response = client.post( + "/api/agent-profiles/acp-one", + json={"agent_kind": "acp", "acp_server": "codex", "acp_model": "gpt-5.5"}, + ) + + assert response.status_code == 201 + loaded = store.load("acp-one") + assert loaded.agent_kind == "acp" + assert loaded.acp_server == "codex" + + +def test_save_missing_required_ref_returns_422(client): + """An OpenHands profile without llm_profile_ref is rejected.""" + response = client.post("/api/agent-profiles/bad", json={}) + assert response.status_code == 422 + + +def test_save_extra_field_returns_422(client): + """extra='forbid' rejects unknown fields.""" + response = client.post( + "/api/agent-profiles/bad", + json={"llm_profile_ref": "x", "bogus": 1}, + ) + assert response.status_code == 422 + + +def test_save_invalid_name_returns_422(client): + response = client.post( + "/api/agent-profiles/.hidden", + json={"llm_profile_ref": "x"}, + ) + assert response.status_code in (400, 404, 422) + + +def test_get_returns_profile(client, store): + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="base")) + + response = client.get("/api/agent-profiles/p") + + assert response.status_code == 200 + body = response.json() + assert body["name"] == "p" + assert body["profile"]["llm_profile_ref"] == "base" + assert body["profile"]["agent_kind"] == "openhands" + + +def test_get_not_found(client): + response = client.get("/api/agent-profiles/nonexistent") + assert response.status_code == 404 + + +def test_get_corrupted_returns_400(client, temp_agent_profiles_dir): + (temp_agent_profiles_dir / "broken.json").write_text("{ not valid json") + response = client.get("/api/agent-profiles/broken") + assert response.status_code == 400 + + +def test_delete_removes_existing(client, store): + store.save(OpenHandsAgentProfile(name="to-delete", llm_profile_ref="x")) + + response = client.delete("/api/agent-profiles/to-delete") + + assert response.status_code == 200 + with pytest.raises(FileNotFoundError): + store.load("to-delete") + + +def test_delete_idempotent(client): + response = client.delete("/api/agent-profiles/nonexistent") + assert response.status_code == 200 + + +def test_delete_clears_active_pointer(client, store): + """Deleting the active profile clears active_agent_profile_id.""" + store.save(OpenHandsAgentProfile(name="active-one", llm_profile_ref="x")) + profile_id = client.get("/api/agent-profiles/active-one").json()["profile"]["id"] + client.post(f"/api/agent-profiles/{profile_id}/activate") + assert client.get("/api/settings").json()["active_agent_profile_id"] == profile_id + + client.delete("/api/agent-profiles/active-one") + + assert client.get("/api/settings").json()["active_agent_profile_id"] is None + + +def test_rename_success(client, store): + store.save(OpenHandsAgentProfile(name="old-name", llm_profile_ref="x")) + + response = client.post( + "/api/agent-profiles/old-name/rename", + json={"new_name": "new-name"}, + ) + + assert response.status_code == 200 + assert "renamed" in response.json()["message"].lower() + with pytest.raises(FileNotFoundError): + store.load("old-name") + assert store.load("new-name").llm_profile_ref == "x" + + +def test_rename_not_found(client): + response = client.post( + "/api/agent-profiles/ghost/rename", + json={"new_name": "new-name"}, + ) + assert response.status_code == 404 + + +def test_rename_conflict(client, store): + store.save(OpenHandsAgentProfile(name="source", llm_profile_ref="a")) + store.save(OpenHandsAgentProfile(name="target", llm_profile_ref="b")) + + response = client.post( + "/api/agent-profiles/source/rename", + json={"new_name": "target"}, + ) + assert response.status_code == 409 + + +def test_rename_invalid_new_name_returns_422(client, store): + store.save(OpenHandsAgentProfile(name="valid", llm_profile_ref="x")) + response = client.post( + "/api/agent-profiles/valid/rename", + json={"new_name": "../etc/passwd"}, + ) + assert response.status_code == 422 + + +def test_rename_preserves_active_pointer(client, store): + """The id-keyed active pointer survives a rename (id is stable).""" + store.save(OpenHandsAgentProfile(name="before", llm_profile_ref="x")) + profile_id = client.get("/api/agent-profiles/before").json()["profile"]["id"] + client.post(f"/api/agent-profiles/{profile_id}/activate") + + client.post("/api/agent-profiles/before/rename", json={"new_name": "after"}) + + # Same id, still active. + assert client.get("/api/settings").json()["active_agent_profile_id"] == profile_id + assert client.get("/api/agent-profiles/after").json()["profile"]["id"] == profile_id + + +# ── Activate (pointer only, by id) ────────────────────────────────────────── + + +def test_activate_sets_pointer_without_mutating_agent_settings(client, store): + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="x")) + # Persist settings once first so the snapshot is already round-tripped + # (the default un-persisted vs persisted form differs harmlessly). + client.patch( + "/api/settings", + json={"agent_settings_diff": {"llm": {"model": "gpt-4o"}}}, + ) + before = client.get("/api/settings").json()["agent_settings"] + profile_id = client.get("/api/agent-profiles/p").json()["profile"]["id"] + + response = client.post(f"/api/agent-profiles/{profile_id}/activate") + + assert response.status_code == 200 + assert response.json()["agent_settings_applied"] is False + after = client.get("/api/settings").json() + assert after["active_agent_profile_id"] == profile_id + # agent_settings is untouched — the creation-time-only contract. + assert after["agent_settings"] == before + + +def test_activate_unknown_id_returns_404(client, store): + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="x")) + unknown = "00000000-dead-beef-0000-000000000000" + response = client.post(f"/api/agent-profiles/{unknown}/activate") + assert response.status_code == 404 + + +# ── Store errors → HTTP ───────────────────────────────────────────────────── + + +def test_list_timeout_returns_503(client, monkeypatch): + def boom(self): + raise TimeoutError("locked") + + monkeypatch.setattr(AgentProfileStore, "list", boom) + response = client.get("/api/agent-profiles") + assert response.status_code == 503 + + +def test_save_timeout_returns_503(client, monkeypatch): + def boom(self, profile, *, cipher=None, max_profiles=None): + raise TimeoutError("locked") + + monkeypatch.setattr(AgentProfileStore, "save", boom) + response = client.post("/api/agent-profiles/x", json={"llm_profile_ref": "y"}) + assert response.status_code == 503 + + +def test_save_at_limit_returns_409(client, store, monkeypatch): + monkeypatch.setattr(router_module, "MAX_AGENT_PROFILES", 1) + store.save(OpenHandsAgentProfile(name="first", llm_profile_ref="x")) + + response = client.post("/api/agent-profiles/second", json={"llm_profile_ref": "y"}) + assert response.status_code == 409 + assert "limit" in response.json()["detail"].lower() diff --git a/tests/agent_server/test_profiles_router.py b/tests/agent_server/test_profiles_router.py index 45ff6c2dc8..f2adcb53f9 100644 --- a/tests/agent_server/test_profiles_router.py +++ b/tests/agent_server/test_profiles_router.py @@ -14,6 +14,7 @@ from openhands.agent_server.persistence import reset_stores from openhands.sdk.llm import LLM from openhands.sdk.llm.llm_profile_store import LLMProfileStore +from openhands.sdk.profiles import AgentProfileStore, OpenHandsAgentProfile @pytest.fixture @@ -25,6 +26,15 @@ def temp_profiles_dir(): yield profiles_dir +@pytest.fixture +def temp_agent_profiles_dir(): + """Create a temporary directory for agent profiles (FK store).""" + with tempfile.TemporaryDirectory() as tmpdir: + agent_dir = Path(tmpdir) / "agent-profiles" + agent_dir.mkdir(parents=True, exist_ok=True) + yield agent_dir + + @pytest.fixture def temp_settings_dir(): """Create a temporary directory for settings.""" @@ -35,7 +45,7 @@ def temp_settings_dir(): @pytest.fixture -def client(temp_profiles_dir, temp_settings_dir, monkeypatch): +def client(temp_profiles_dir, temp_agent_profiles_dir, temp_settings_dir, monkeypatch): """Create test client with isolated profiles/settings directories, no cipher.""" # Reset store singletons to ensure clean state reset_stores() @@ -47,10 +57,17 @@ def client(temp_profiles_dir, temp_settings_dir, monkeypatch): config = Config(static_files_path=None, session_api_keys=[], secret_key=None) app = create_app(config) - # Patch LLMProfileStore to use temp directory - with patch( - "openhands.agent_server.profiles_router.LLMProfileStore", - lambda: LLMProfileStore(base_dir=temp_profiles_dir), + # Patch both stores to use temp directories (AgentProfileStore is hit by the + # FK guard on delete/rename). + with ( + patch( + "openhands.agent_server.profiles_router.LLMProfileStore", + lambda: LLMProfileStore(base_dir=temp_profiles_dir), + ), + patch( + "openhands.agent_server.profiles_router.AgentProfileStore", + lambda: AgentProfileStore(base_dir=temp_agent_profiles_dir), + ), ): yield TestClient(app) @@ -64,6 +81,52 @@ def store(temp_profiles_dir): return LLMProfileStore(base_dir=temp_profiles_dir) +@pytest.fixture +def agent_store(temp_agent_profiles_dir): + """Create the agent-profile store backing the FK guard.""" + return AgentProfileStore(base_dir=temp_agent_profiles_dir) + + +# ── FK Guard: deleting/renaming a referenced LLM profile ──────────────────── + + +def test_delete_referenced_llm_profile_returns_409(client, store, agent_store): + """Deleting an LLM profile cited by an AgentProfile returns 409 w/ referrers.""" + store.save("base-llm", LLM(model="gpt-4o")) + agent_store.save(OpenHandsAgentProfile(name="agent-a", llm_profile_ref="base-llm")) + + response = client.delete("/api/profiles/base-llm") + + assert response.status_code == 409 + assert "agent-a" in response.json()["detail"] + # The LLM profile is left intact. + assert store.load("base-llm").model == "gpt-4o" + + +def test_delete_unreferenced_llm_profile_succeeds(client, store, agent_store): + """An LLM profile no AgentProfile cites deletes normally.""" + store.save("lonely", LLM(model="gpt-4o")) + agent_store.save(OpenHandsAgentProfile(name="agent-a", llm_profile_ref="other-llm")) + + response = client.delete("/api/profiles/lonely") + + assert response.status_code == 200 + with pytest.raises(FileNotFoundError): + store.load("lonely") + + +def test_rename_llm_profile_cascades_to_agent_refs(client, store, agent_store): + """Renaming an LLM profile repoints citing AgentProfile.llm_profile_ref.""" + store.save("old-llm", LLM(model="gpt-4o")) + agent_store.save(OpenHandsAgentProfile(name="agent-a", llm_profile_ref="old-llm")) + + response = client.post("/api/profiles/old-llm/rename", json={"new_name": "new-llm"}) + + assert response.status_code == 200 + # The agent profile's FK was cascaded to the new name. + assert agent_store.load("agent-a").llm_profile_ref == "new-llm" + + # ── List Profiles ────────────────────────────────────────────────────────── @@ -659,7 +722,13 @@ def secret_key(): @pytest.fixture -def client_with_cipher(temp_profiles_dir, temp_settings_dir, secret_key, monkeypatch): +def client_with_cipher( + temp_profiles_dir, + temp_agent_profiles_dir, + temp_settings_dir, + secret_key, + monkeypatch, +): """Create test client with cipher configured.""" from pydantic import SecretStr @@ -676,9 +745,15 @@ def client_with_cipher(temp_profiles_dir, temp_settings_dir, secret_key, monkeyp ) app = create_app(config) - with patch( - "openhands.agent_server.profiles_router.LLMProfileStore", - lambda: LLMProfileStore(base_dir=temp_profiles_dir), + with ( + patch( + "openhands.agent_server.profiles_router.LLMProfileStore", + lambda: LLMProfileStore(base_dir=temp_profiles_dir), + ), + patch( + "openhands.agent_server.profiles_router.AgentProfileStore", + lambda: AgentProfileStore(base_dir=temp_agent_profiles_dir), + ), ): yield TestClient(app) diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index 01656eb3e2..ca64d7ef6e 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -522,6 +522,54 @@ def test_patch_settings_rejects_invalid_active_profile(client_with_settings): assert response.status_code == 422 +def test_patch_settings_active_agent_profile_id_independent(client_with_settings): + """active_agent_profile_id sets/clears independently of active_profile.""" + set_response = client_with_settings.patch( + "/api/settings", + json={"active_profile": "fast-profile", "active_agent_profile_id": "abc-123"}, + ) + assert set_response.status_code == 200 + body = set_response.json() + assert body["active_profile"] == "fast-profile" + assert body["active_agent_profile_id"] == "abc-123" + + # Clearing the agent pointer must leave the LLM profile pointer untouched. + clear_response = client_with_settings.patch( + "/api/settings", + json={"active_agent_profile_id": None}, + ) + assert clear_response.status_code == 200 + cleared = clear_response.json() + assert cleared["active_agent_profile_id"] is None + assert cleared["active_profile"] == "fast-profile" + + refetch = client_with_settings.get("/api/settings").json() + assert refetch["active_agent_profile_id"] is None + assert refetch["active_profile"] == "fast-profile" + + +def test_existing_settings_load_with_null_active_agent_profile_id( + temp_persistence_dir, config_with_settings +): + """A settings file predating the field loads with active_agent_profile_id=None.""" + _write_settings_file( + temp_persistence_dir, + { + "schema_version": PERSISTED_SETTINGS_SCHEMA_VERSION, + "agent_settings": {"agent_kind": "openhands"}, + "active_profile": "legacy-profile", + }, + ) + + client = TestClient(create_app(config_with_settings)) + response = client.get("/api/settings") + + assert response.status_code == 200 + body = response.json() + assert body["active_profile"] == "legacy-profile" + assert body["active_agent_profile_id"] is None + + def test_patch_settings_updates_condenser_config(client_with_settings): """PATCH /api/settings can update condenser constructor settings.""" response = client_with_settings.patch( @@ -642,7 +690,7 @@ def test_patch_settings_empty_payload_returns_400(client_with_settings): assert response.json()["detail"] == ( "At least one of agent_settings_diff, " "conversation_settings_diff, misc_settings_diff, " - "or active_profile must be provided" + "active_profile, or active_agent_profile_id must be provided" ) From 97a4fd15ddb30d46e2dfcd650ee50e509542c3ac Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Wed, 17 Jun 2026 22:41:37 +0200 Subject: [PATCH 2/8] Address review: faithful seed + mcp_tools secret round-trip Review bot flagged two correctness gaps: - Migration fidelity: the lazy seed now carries every cleanly-overlapping launch field from agent_settings (OpenHands: skills, system_message_suffix, condenser, verification subset; ACP: acp_command via shlex.join + acp_args), not just the LLM ref. Existing single-config users migrate faithfully once the resolver honors active_agent_profile_id. - Secret round-trip: skills[].mcp_tools env/headers are decrypted (Fernet -> plaintext) on GET, save, and seed via a shared helper. The store masks/ encrypts on save but has no symmetric load-time validator, so without this a GET(encrypted) -> POST edit double-encrypted the value. Plaintext now decrypts exactly once. Tests: cipher-backed GET(encrypted) -> POST round-trip + encrypted-at-rest, and seed-fidelity tests for non-default OpenHands and ACP settings. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 114 +++++++++++- .../test_agent_profiles_router.py | 168 ++++++++++++++++++ 2 files changed, 277 insertions(+), 5 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index b50b315006..7dd7201047 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -10,6 +10,8 @@ is deliberately not implemented here so this router ships independently. """ +import copy +import shlex from collections.abc import Iterator from contextlib import contextmanager from typing import Annotated, Any @@ -35,10 +37,14 @@ OpenHandsAgentProfile, ProfileLimitExceeded, ProfileReferenced, + ProfileVerificationSettings, validate_agent_profile, ) from openhands.sdk.profiles.agent_profile_store import PROFILE_NAME_PATTERN from openhands.sdk.settings import AgentSettingsConfig +from openhands.sdk.settings.model import VerificationSettings +from openhands.sdk.utils.cipher import Cipher +from openhands.sdk.utils.pydantic_secrets import decrypt_str_with_cipher_or_keep logger = get_logger(__name__) @@ -119,12 +125,82 @@ def _store_errors() -> Iterator[None]: raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) +def _decrypt_mcp_tools(tools: dict[str, Any], cipher: Cipher) -> dict[str, Any]: + """Return a copy of an ``mcp_tools`` dict with env/headers Fernet tokens + decrypted. Non-Fernet (plaintext) values pass through unchanged.""" + servers = tools.get("mcpServers") + if not isinstance(servers, dict): + return tools + out = copy.deepcopy(tools) + for server in out["mcpServers"].values(): + if not isinstance(server, dict): + continue + for key in ("env", "headers"): + mapping = server.get(key) + if isinstance(mapping, dict): + server[key] = { + k: decrypt_str_with_cipher_or_keep( + cipher, v, description="MCP env/headers" + ) + for k, v in mapping.items() + } + return out + + +def _decrypt_profile_mcp_tools( + profile: OpenHandsAgentProfile | ACPAgentProfile, cipher: Cipher | None +) -> OpenHandsAgentProfile | ACPAgentProfile: + """Decrypt Fernet-encrypted ``skills[].mcp_tools`` env/headers on a profile. + + ``AgentProfileStore`` masks/encrypts these on save but has no symmetric + load-time validator, so both the GET (at-rest) and the save (client + round-tripped) values carry ciphertext. Decrypting here gives the GET expose + serializer plaintext to re-mask, and stops the save path from + double-encrypting an already-encrypted value (the round-trip the resolver + would otherwise decrypt once and get a stale token). No-op without a cipher + or on ACP profiles (no skills). + """ + if cipher is None: + return profile + skills = getattr(profile, "skills", None) + if not skills: + return profile + new_skills = [ + skill.model_copy( + update={"mcp_tools": _decrypt_mcp_tools(skill.mcp_tools, cipher)} + ) + if skill.mcp_tools + else skill + for skill in skills + ] + return profile.model_copy(update={"skills": new_skills}) + + +def _profile_verification(v: VerificationSettings) -> ProfileVerificationSettings: + """Project the secret-free subset of ``VerificationSettings``. + + Drops ``critic_api_key`` — the profile is secret-free; the critic reuses + the resolved LLM profile's key. + """ + return ProfileVerificationSettings( + critic_enabled=v.critic_enabled, + critic_mode=v.critic_mode, + enable_iterative_refinement=v.enable_iterative_refinement, + critic_threshold=v.critic_threshold, + max_refinement_iterations=v.max_refinement_iterations, + critic_server_url=v.critic_server_url, + critic_model_name=v.critic_model_name, + ) + + def _build_seed_profile( agent_settings: AgentSettingsConfig, active_llm_profile: str | None ) -> OpenHandsAgentProfile | ACPAgentProfile: - """Build one conservative ``AgentProfile`` from the current ``agent_settings``. + """Build one ``AgentProfile`` faithfully from the current ``agent_settings``. - Carries only the cleanly-overlapping fields; ``mcp_server_refs=None`` exposes + Carries every cleanly-overlapping launch field so the migrated profile is a + stable representation of the user's current configuration (the active + pointer is otherwise just a lightweight id). ``mcp_server_refs=None`` exposes all of the user's MCP servers. An OpenHands profile references the active LLM profile (falling back to ``"default"`` when none is set — a soft ref the resolver checks at materialize time). @@ -136,12 +212,25 @@ def _build_seed_profile( acp_model=agent_settings.acp_model, acp_session_mode=agent_settings.acp_session_mode, acp_prompt_timeout=agent_settings.acp_prompt_timeout, + # settings store the command as a token list; the profile holds a + # single (re-parseable) string. Empty list => use the server default. + acp_command=( + shlex.join(agent_settings.acp_command) + if agent_settings.acp_command + else None + ), + acp_args=list(agent_settings.acp_args) or None, mcp_server_refs=None, ) + context = agent_settings.agent_context return OpenHandsAgentProfile( name=SEED_PROFILE_NAME, llm_profile_ref=active_llm_profile or SEED_PROFILE_NAME, agent=agent_settings.agent, + skills=list(context.skills), + system_message_suffix=context.system_message_suffix, + condenser=agent_settings.condenser, + verification=_profile_verification(agent_settings.verification), enable_sub_agents=agent_settings.enable_sub_agents, tool_concurrency_limit=agent_settings.tool_concurrency_limit, mcp_server_refs=None, @@ -149,12 +238,18 @@ def _build_seed_profile( def _seed_default_profile( - store: AgentProfileStore, request: Request, settings: PersistedSettings + store: AgentProfileStore, + request: Request, + settings: PersistedSettings, + cipher: Cipher | None, ) -> None: """Persist one default profile and point ``active_agent_profile_id`` at it.""" profile = _build_seed_profile(settings.agent_settings, settings.active_profile) + # Settings persist skills[].mcp_tools encrypted (and never decrypt on load), + # so decrypt before re-encrypting at save to avoid double-encryption. + profile = _decrypt_profile_mcp_tools(profile, cipher) with _store_errors(): - store.save(profile, max_profiles=MAX_AGENT_PROFILES) + store.save(profile, cipher=cipher, max_profiles=MAX_AGENT_PROFILES) profile_id = str(profile.id) settings_store = get_settings_store(get_config(request)) @@ -194,7 +289,7 @@ async def list_agent_profiles(request: Request) -> AgentProfileListResponse: existing = store.list() if not existing and settings.active_agent_profile_id is None: - _seed_default_profile(store, request, settings) + _seed_default_profile(store, request, settings, get_cipher(request)) settings = settings_store.load() or settings with _store_errors(): @@ -222,6 +317,10 @@ async def get_agent_profile( with _store_errors(): profile = store.load(name, cipher=cipher) + # The store leaves skills[].mcp_tools encrypted on load; decrypt to plaintext + # so the expose serializer can correctly redact / re-encrypt / reveal them. + profile = _decrypt_profile_mcp_tools(profile, cipher) + context = build_expose_context(expose_mode, cipher) with translate_missing_cipher(): payload = profile.model_dump(mode="json", context=context) @@ -253,6 +352,11 @@ async def save_agent_profile( detail=f"Invalid agent profile: {type(e).__name__}", ) + # A client editing a profile fetched with X-Expose-Secrets: encrypted posts + # back Fernet tokens; decrypt them so the save re-encrypts the original + # secret once rather than double-encrypting the token. + profile = _decrypt_profile_mcp_tools(profile, cipher) + store = AgentProfileStore() try: with _store_errors(): diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 97c8596982..04a63bb6e9 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -333,6 +333,174 @@ def test_activate_unknown_id_returns_404(client, store): assert response.status_code == 404 +# ── Seed fidelity (migration preserves the user's launch config) ──────────── + + +def test_seed_preserves_openhands_fields(client): + """The OpenHands seed carries the overlapping launch fields, not just refs.""" + client.patch( + "/api/settings", + json={ + "agent_settings_diff": { + "enable_sub_agents": True, + "tool_concurrency_limit": 3, + "agent_context": {"system_message_suffix": "be terse"}, + "verification": { + "critic_enabled": True, + "critic_model_name": "x-critic", + }, + } + }, + ) + client.get("/api/agent-profiles") # triggers the seed + + prof = client.get("/api/agent-profiles/default").json()["profile"] + assert prof["enable_sub_agents"] is True + assert prof["tool_concurrency_limit"] == 3 + assert prof["system_message_suffix"] == "be terse" + assert prof["verification"]["critic_enabled"] is True + assert prof["verification"]["critic_model_name"] == "x-critic" + # The profile verification is secret-free — no critic_api_key projected. + assert "critic_api_key" not in prof["verification"] + + +def test_seed_preserves_acp_fields(client): + """The ACP seed carries acp_server/model/args, not just the kind.""" + client.patch( + "/api/settings", + json={ + "agent_settings_diff": { + "agent_kind": "acp", + "acp_server": "codex", + "acp_model": "gpt-5.5", + "acp_args": ["--foo", "--bar"], + } + }, + ) + client.get("/api/agent-profiles") # triggers the seed + + prof = client.get("/api/agent-profiles/default").json()["profile"] + assert prof["agent_kind"] == "acp" + assert prof["acp_server"] == "codex" + assert prof["acp_model"] == "gpt-5.5" + assert prof["acp_args"] == ["--foo", "--bar"] + + +# ── Cipher: skills[].mcp_tools secret round-trip ──────────────────────────── + + +@pytest.fixture +def secret_key(): + from base64 import urlsafe_b64encode + + return urlsafe_b64encode(b"a" * 32).decode("ascii") + + +@pytest.fixture +def cipher(secret_key): + from openhands.sdk.utils.cipher import Cipher + + return Cipher(secret_key) + + +@pytest.fixture +def client_with_cipher( + temp_agent_profiles_dir, temp_settings_dir, secret_key, monkeypatch +): + from pydantic import SecretStr + + reset_stores() + monkeypatch.setenv("OH_PERSISTENCE_DIR", str(temp_settings_dir)) + config = Config( + static_files_path=None, session_api_keys=[], secret_key=SecretStr(secret_key) + ) + app = create_app(config) + with patch( + "openhands.agent_server.agent_profiles_router.AgentProfileStore", + lambda: AgentProfileStore(base_dir=temp_agent_profiles_dir), + ): + yield TestClient(app) + reset_stores() + + +def _profile_with_mcp_secret(header_value: str) -> dict: + return { + "llm_profile_ref": "base", + "skills": [ + { + "name": "leaky", + "content": "do stuff", + "mcp_tools": { + "mcpServers": { + "svc": { + "url": "https://x.test", + "headers": {"Authorization": header_value}, + } + } + }, + } + ], + } + + +def _mcp_auth(profile_payload: dict) -> str: + servers = profile_payload["skills"][0]["mcp_tools"]["mcpServers"] + return servers["svc"]["headers"]["Authorization"] + + +def test_mcp_tools_secret_encrypted_roundtrip(client_with_cipher, cipher): + """GET(encrypted) -> POST -> the secret still decrypts exactly once. + + Without decrypt-incoming-before-save the re-posted token would be encrypted + again and the stored value would decrypt to a stale token. + """ + secret = "Bearer ghp_roundtrip_secret" + + created = client_with_cipher.post( + "/api/agent-profiles/p", json=_profile_with_mcp_secret(secret) + ) + assert created.status_code == 201 + + # GET encrypted: a Fernet token of the ORIGINAL secret (not double-encrypted). + enc = client_with_cipher.get( + "/api/agent-profiles/p", headers={"X-Expose-Secrets": "encrypted"} + ).json() + token = _mcp_auth(enc["profile"]) + assert token != secret + assert cipher.decrypt(token).get_secret_value() == secret + + # Re-post the encrypted token (an ordinary client edit round-trip). + reposted = client_with_cipher.post( + "/api/agent-profiles/p", json=_profile_with_mcp_secret(token) + ) + assert reposted.status_code == 201 + + # Plaintext GET returns the original secret -> decrypted exactly once. + plain = client_with_cipher.get( + "/api/agent-profiles/p", headers={"X-Expose-Secrets": "plaintext"} + ).json() + assert _mcp_auth(plain["profile"]) == secret + + +def test_mcp_tools_secret_encrypted_at_rest( + client_with_cipher, temp_agent_profiles_dir, cipher +): + """A posted plaintext MCP secret is encrypted on disk, never stored raw.""" + import json + + secret = "Bearer ghp_at_rest_secret" + client_with_cipher.post( + "/api/agent-profiles/p", json=_profile_with_mcp_secret(secret) + ) + + raw = json.loads((temp_agent_profiles_dir / "p.json").read_text()) + stored = raw["skills"][0]["mcp_tools"]["mcpServers"]["svc"]["headers"][ + "Authorization" + ] + assert stored != secret + assert cipher.decrypt(stored).get_secret_value() == secret + + # ── Store errors → HTTP ───────────────────────────────────────────────────── From a48296eb1766e7c681ba72fa9d04aaa40051bb88 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Wed, 17 Jun 2026 23:59:21 +0200 Subject: [PATCH 3/8] Address review 2: keep AgentProfile id stable across overwrite + seed Review bot flagged two pointer-integrity gaps (active_agent_profile_id is keyed on the stable id): - save_agent_profile preserved nothing, so a create-style body (no id/revision) overwriting a namesake minted a fresh UUID and reset revision, dangling the active pointer. Now reuses the existing profile's id and bumps revision. - The lazy seed wasn't idempotent under concurrent first requests. It now holds the store lock across the empty-check + save + pointer write (double-checked), so concurrent first GETs seed exactly once and the pointer always matches the persisted profile id. Tests: overwrite-preserves-id-and-pointer + concurrent-first-list-seeds-once. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 68 +++++++++++++++---- .../test_agent_profiles_router.py | 42 ++++++++++++ 2 files changed, 97 insertions(+), 13 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index 7dd7201047..f4f1b4ea42 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -15,6 +15,7 @@ from collections.abc import Iterator from contextlib import contextmanager from typing import Annotated, Any +from uuid import UUID from fastapi import APIRouter, HTTPException, Path, Request, status from pydantic import BaseModel, Field, ValidationError @@ -243,23 +244,33 @@ def _seed_default_profile( settings: PersistedSettings, cipher: Cipher | None, ) -> None: - """Persist one default profile and point ``active_agent_profile_id`` at it.""" - profile = _build_seed_profile(settings.agent_settings, settings.active_profile) - # Settings persist skills[].mcp_tools encrypted (and never decrypt on load), - # so decrypt before re-encrypting at save to avoid double-encryption. - profile = _decrypt_profile_mcp_tools(profile, cipher) - with _store_errors(): + """Persist one default profile and point ``active_agent_profile_id`` at it. + + Holds the store lock across the empty-check + save + pointer write so + concurrent first requests seed exactly once (the loser sees a non-empty + store and returns); the pointer always matches the persisted profile id. + """ + with _store_errors(), store._acquire_lock(): + # Double-checked under the lock: a concurrent first request may have + # already seeded (the outer emptiness check in the list endpoint is + # unlocked). + if store.list(): + return + profile = _build_seed_profile(settings.agent_settings, settings.active_profile) + # Settings persist skills[].mcp_tools encrypted (and never decrypt on + # load), so decrypt before re-encrypting at save to avoid double-encrypt. + profile = _decrypt_profile_mcp_tools(profile, cipher) store.save(profile, cipher=cipher, max_profiles=MAX_AGENT_PROFILES) - profile_id = str(profile.id) - settings_store = get_settings_store(get_config(request)) + profile_id = str(profile.id) + settings_store = get_settings_store(get_config(request)) - def set_pointer(s: PersistedSettings) -> PersistedSettings: - s.active_agent_profile_id = profile_id - return s + def set_pointer(s: PersistedSettings) -> PersistedSettings: + s.active_agent_profile_id = profile_id + return s - settings_store.update(set_pointer) - logger.info(f"Seeded default agent profile '{profile.name}' (id={profile_id})") + settings_store.update(set_pointer) + logger.info(f"Seeded default agent profile '{profile.name}' (id={profile_id})") def _summary_id_for_name(store: AgentProfileStore, name: str) -> str | None: @@ -272,6 +283,29 @@ def _summary_id_for_name(store: AgentProfileStore, name: str) -> str | None: return None +def _existing_identity( + store: AgentProfileStore, name: str +) -> tuple[UUID | None, int | None]: + """Return the stable ``(id, revision)`` of the profile under ``name``. + + Used to keep ``id`` stable across an overwrite — the active pointer is keyed + on it — and to bump ``revision`` monotonically. Ignores a malformed stored + id (treated as no prior identity). + """ + with _store_errors(): + for summary in store.list_summaries(): + if summary.get("name") != name: + continue + sid = summary.get("id") + rev = summary.get("revision") + try: + parsed = UUID(str(sid)) if sid is not None else None + except (ValueError, TypeError): + parsed = None + return parsed, rev if isinstance(rev, int) else None + return None, None + + @agent_profiles_router.get("", response_model=AgentProfileListResponse) async def list_agent_profiles(request: Request) -> AgentProfileListResponse: """List all stored agent profiles and the active pointer. @@ -358,6 +392,14 @@ async def save_agent_profile( profile = _decrypt_profile_mcp_tools(profile, cipher) store = AgentProfileStore() + # The id is stable state, not a defaultable request field: overwriting a + # namesake keeps its id (so an active pointer never dangles) and bumps the + # revision, even when a create-style body omits both. + existing_id, existing_rev = _existing_identity(store, name) + if existing_id is not None: + profile = profile.model_copy( + update={"id": existing_id, "revision": (existing_rev or 0) + 1} + ) try: with _store_errors(): store.save(profile, cipher=cipher, max_profiles=MAX_AGENT_PROFILES) diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 04a63bb6e9..22a316a89b 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -5,6 +5,7 @@ activation by id (no ``agent_settings`` write), and the lazy migration seed. """ +import concurrent.futures import tempfile from pathlib import Path from unittest.mock import patch @@ -121,6 +122,25 @@ def test_no_seed_when_store_nonempty(client, store): assert body["active_agent_profile_id"] is None +def test_concurrent_first_list_seeds_once(client, store): + """Concurrent first GETs seed exactly one profile; the pointer is consistent. + + The seed holds the store lock across check + save + pointer write, so the + losing requests see a non-empty store and the active pointer always matches + the single persisted profile id (never a dangling/overwritten id). + """ + with concurrent.futures.ThreadPoolExecutor(max_workers=8) as ex: + codes = list( + ex.map(lambda _: client.get("/api/agent-profiles").status_code, range(8)) + ) + + assert all(code == 200 for code in codes) + summaries = store.list_summaries() + assert len(summaries) == 1 # seeded exactly once + pointer = client.get("/api/settings").json()["active_agent_profile_id"] + assert pointer == summaries[0]["id"] # pointer resolves to the real profile + + # ── CRUD ───────────────────────────────────────────────────────────────────── @@ -148,6 +168,28 @@ def test_save_overwrites_existing(client, store): assert store.load("existing").llm_profile_ref == "new" +def test_overwrite_preserves_id_and_pointer(client, store): + """Overwriting a profile keeps its id stable (and bumps revision). + + A create-style body that omits ``id``/``revision`` must not mint a fresh + UUID — that would dangle the active pointer keyed on the old id. + """ + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="base")) + pid = client.get("/api/agent-profiles/p").json()["profile"]["id"] + client.post(f"/api/agent-profiles/{pid}/activate") + assert client.get("/api/settings").json()["active_agent_profile_id"] == pid + + response = client.post("/api/agent-profiles/p", json={"llm_profile_ref": "changed"}) + assert response.status_code == 201 + + detail = client.get("/api/agent-profiles/p").json()["profile"] + assert detail["id"] == pid # stable id preserved + assert detail["revision"] == 1 # monotonically bumped + assert detail["llm_profile_ref"] == "changed" + # The active pointer still resolves to the (same-id) profile. + assert client.get("/api/settings").json()["active_agent_profile_id"] == pid + + def test_save_path_name_is_authoritative(client, store): """The path name overrides any ``name`` in the body.""" response = client.post( From eda4828a4b83706dfe2dd0771d623a6eefbab640 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Thu, 18 Jun 2026 00:15:06 +0200 Subject: [PATCH 4/8] Address review 3: 500 on settings corruption, UUID validation, comment trim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - activate: a corrupted/mis-keyed settings file (RuntimeError) is a server-side integrity failure → 500, not 409. Test added. - SettingsUpdateRequest.active_agent_profile_id gains a UUID format pattern so malformed pointers are rejected at the HTTP layer (mirrors active_profile). - Drop a diff-narrative comment in PersistedSettings.update(). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 6 ++++-- .../openhands/agent_server/persistence/models.py | 4 +--- .../openhands/sdk/settings/api_models.py | 9 +++++++++ tests/agent_server/test_agent_profiles_router.py | 15 +++++++++++++++ tests/agent_server/test_settings_router.py | 14 ++++++++++++-- 5 files changed, 41 insertions(+), 7 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index f4f1b4ea42..ae4b482910 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -512,10 +512,12 @@ def set_pointer(settings: PersistedSettings) -> PersistedSettings: logger.error("Failed to activate agent profile - file I/O error") raise HTTPException(status_code=500, detail="Failed to activate agent profile") except RuntimeError as e: + # A corrupted / mis-keyed settings file is a server-side integrity + # failure, not a client conflict. logger.error(f"Failed to activate agent profile: {e}") raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail="Settings file is corrupted or encrypted with a different key", + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Failed to activate agent profile", ) logger.info(f"Activated agent profile id '{profile_id}'") diff --git a/openhands-agent-server/openhands/agent_server/persistence/models.py b/openhands-agent-server/openhands/agent_server/persistence/models.py index 2f51685e3d..b3621510ab 100644 --- a/openhands-agent-server/openhands/agent_server/persistence/models.py +++ b/openhands-agent-server/openhands/agent_server/persistence/models.py @@ -254,11 +254,9 @@ def update(self, payload: SettingsUpdatePayload) -> None: if new_misc is not None: self.misc_settings = new_misc - # Update active_profile if explicitly provided (including None to clear) + # Update pointers if explicitly provided (including None to clear) if "active_profile" in payload: self.active_profile = payload["active_profile"] - - # Same for the AgentProfile pointer (including None to clear) if "active_agent_profile_id" in payload: self.active_agent_profile_id = payload["active_agent_profile_id"] finally: diff --git a/openhands-sdk/openhands/sdk/settings/api_models.py b/openhands-sdk/openhands/sdk/settings/api_models.py index c1e272f279..f3343e8dda 100644 --- a/openhands-sdk/openhands/sdk/settings/api_models.py +++ b/openhands-sdk/openhands/sdk/settings/api_models.py @@ -33,6 +33,14 @@ from openhands.sdk.llm.llm_profile_store import PROFILE_NAME_PATTERN +# An AgentProfile's stable id is a UUID (the pointer target); reject malformed +# values at the HTTP layer, mirroring ``active_profile``'s name pattern. +UUID_PATTERN = ( + r"^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-" + r"[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$" +) + + if TYPE_CHECKING: from .model import AgentSettingsConfig, ConversationSettings @@ -119,6 +127,7 @@ class SettingsUpdateRequest(BaseModel): ) active_agent_profile_id: str | None = Field( default=None, + pattern=UUID_PATTERN, description="Stable id of the active AgentProfile to persist; null clears it.", ) diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 22a316a89b..5eff06c321 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -375,6 +375,21 @@ def test_activate_unknown_id_returns_404(client, store): assert response.status_code == 404 +def test_activate_settings_corruption_returns_500(client, store, monkeypatch): + """A corrupted/mis-keyed settings file is a server-side failure (500).""" + from openhands.agent_server.persistence.store import FileSettingsStore + + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="x")) + profile_id = client.get("/api/agent-profiles/p").json()["profile"]["id"] + + def boom(self, *args, **kwargs): + raise RuntimeError("settings file corrupted") + + monkeypatch.setattr(FileSettingsStore, "update", boom) + response = client.post(f"/api/agent-profiles/{profile_id}/activate") + assert response.status_code == 500 + + # ── Seed fidelity (migration preserves the user's launch config) ──────────── diff --git a/tests/agent_server/test_settings_router.py b/tests/agent_server/test_settings_router.py index ca64d7ef6e..fe75e2f3b2 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -524,14 +524,15 @@ def test_patch_settings_rejects_invalid_active_profile(client_with_settings): def test_patch_settings_active_agent_profile_id_independent(client_with_settings): """active_agent_profile_id sets/clears independently of active_profile.""" + agent_id = "12345678-1234-1234-1234-1234567890ab" set_response = client_with_settings.patch( "/api/settings", - json={"active_profile": "fast-profile", "active_agent_profile_id": "abc-123"}, + json={"active_profile": "fast-profile", "active_agent_profile_id": agent_id}, ) assert set_response.status_code == 200 body = set_response.json() assert body["active_profile"] == "fast-profile" - assert body["active_agent_profile_id"] == "abc-123" + assert body["active_agent_profile_id"] == agent_id # Clearing the agent pointer must leave the LLM profile pointer untouched. clear_response = client_with_settings.patch( @@ -548,6 +549,15 @@ def test_patch_settings_active_agent_profile_id_independent(client_with_settings assert refetch["active_profile"] == "fast-profile" +def test_patch_settings_rejects_malformed_active_agent_profile_id(client_with_settings): + """A non-UUID active_agent_profile_id is rejected at the HTTP layer.""" + response = client_with_settings.patch( + "/api/settings", + json={"active_agent_profile_id": "not-a-uuid"}, + ) + assert response.status_code == 422 + + def test_existing_settings_load_with_null_active_agent_profile_id( temp_persistence_dir, config_with_settings ): From 94c6c5cf5cd2694018abd4a03722dbe4e4636977 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Thu, 18 Jun 2026 00:30:53 +0200 Subject: [PATCH 5/8] Address review 4: actionable+leak-safe save errors, stale-pointer test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - save_agent_profile: surface field locations + error types on a ValidationError (actionable), but never the input/msg (a nested mcp_tools MCPConfig error embeds the input, which may carry secrets). Any other validation failure — notably SkillValidationError from a malformed mcp_tools, which is NOT a ValueError and previously escaped to a 500 leaking the secret in the traceback — is now caught and returned as a generic 422. - Tests: 422 surfaces the offending field location; a malformed mcp_tools body never echoes its secret; empty store + non-null (stale) pointer is left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 20 ++++++++- .../test_agent_profiles_router.py | 41 ++++++++++++++++++- 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index ae4b482910..6a95ca0051 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -380,10 +380,26 @@ async def save_agent_profile( cipher = get_cipher(request) try: profile = validate_agent_profile({**body, "name": name}) - except (ValidationError, ValueError, TypeError) as e: + except ValidationError as e: + # Surface field locations + error types so the client can fix the body, + # but omit ``input``/``msg`` — a nested mcp_tools MCPConfig error embeds + # the input (which may carry secrets) in its message. raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=f"Invalid agent profile: {type(e).__name__}", + detail={ + "message": "Invalid agent profile", + "errors": [ + {"loc": err["loc"], "type": err["type"]} for err in e.errors() + ], + }, + ) + except Exception: + # Any other validation failure (e.g. SkillValidationError from a + # malformed mcp_tools, or a schema/migration error) is a client error, + # never a 500. Stay generic — these messages can embed the input. + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail="Invalid agent profile", ) # A client editing a profile fetched with X-Expose-Secrets: encrypted posts diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 5eff06c321..e6893b9374 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -122,6 +122,21 @@ def test_no_seed_when_store_nonempty(client, store): assert body["active_agent_profile_id"] is None +def test_no_seed_when_pointer_set_but_store_empty(client): + """An empty store with a non-null pointer is left as-is (no seed, no error). + + A stale pointer (e.g. after a failed delete) reflects user state, so the + seed condition deliberately requires both an empty store *and* a null + pointer. + """ + stale = "12345678-1234-1234-1234-1234567890ab" + client.patch("/api/settings", json={"active_agent_profile_id": stale}) + + body = client.get("/api/agent-profiles").json() + assert body["profiles"] == [] + assert body["active_agent_profile_id"] == stale + + def test_concurrent_first_list_seeds_once(client, store): """Concurrent first GETs seed exactly one profile; the pointer is consistent. @@ -216,9 +231,33 @@ def test_save_acp_profile(client, store): def test_save_missing_required_ref_returns_422(client): - """An OpenHands profile without llm_profile_ref is rejected.""" + """A missing required field is rejected and the field location is surfaced.""" response = client.post("/api/agent-profiles/bad", json={}) assert response.status_code == 422 + detail = response.json()["detail"] + # The discriminated union tags the location with the variant ("openhands"). + assert any("llm_profile_ref" in err["loc"] for err in detail["errors"]) + + +def test_save_invalid_body_does_not_leak_mcp_secret(client): + """A malformed profile body's 422 must not echo skills[].mcp_tools secrets.""" + secret = "ghp_should_never_appear_in_error" + response = client.post( + "/api/agent-profiles/bad", + json={ + "llm_profile_ref": "base", + "skills": [ + { + "name": "leaky", + "content": "x", + # Malformed: mcpServers must be an object, forcing a failure. + "mcp_tools": {"mcpServers": {"svc": {"headers": secret}}}, + } + ], + }, + ) + assert response.status_code == 422 + assert secret not in response.text def test_save_extra_field_returns_422(client): From 56b6023b61c8a5ad8893dcff501c1dbc6842fc1c Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Thu, 18 Jun 2026 00:44:48 +0200 Subject: [PATCH 6/8] Address review 5: mint a fresh id on create (global id uniqueness) The active pointer is keyed on AgentProfile.id, so ids must be globally unique, not just stable per file. save now mints a server id on create (ignoring any client-supplied id) and preserves it only on overwrite. Prevents a client from creating two names with the same id, which made the pointer ambiguous (deleting one could clear the active selection while a namesake id lives on). Test: creating 'b' with 'a's id yields a distinct id; activating b then deleting a leaves the pointer intact. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 12 +++++++---- .../test_agent_profiles_router.py | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index 6a95ca0051..1571ae015c 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -15,7 +15,7 @@ from collections.abc import Iterator from contextlib import contextmanager from typing import Annotated, Any -from uuid import UUID +from uuid import UUID, uuid4 from fastapi import APIRouter, HTTPException, Path, Request, status from pydantic import BaseModel, Field, ValidationError @@ -408,14 +408,18 @@ async def save_agent_profile( profile = _decrypt_profile_mcp_tools(profile, cipher) store = AgentProfileStore() - # The id is stable state, not a defaultable request field: overwriting a - # namesake keeps its id (so an active pointer never dangles) and bumps the - # revision, even when a create-style body omits both. + # The id is server-managed state, never a client-settable field — the active + # pointer is keyed on it. Overwriting a namesake keeps its id (so the pointer + # never dangles) and bumps the revision; creating a new name always mints a + # fresh id (ignoring any client-supplied one) so ids stay globally unique and + # the pointer is never ambiguous. existing_id, existing_rev = _existing_identity(store, name) if existing_id is not None: profile = profile.model_copy( update={"id": existing_id, "revision": (existing_rev or 0) + 1} ) + else: + profile = profile.model_copy(update={"id": uuid4()}) try: with _store_errors(): store.save(profile, cipher=cipher, max_profiles=MAX_AGENT_PROFILES) diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index e6893b9374..7d7867084b 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -205,6 +205,26 @@ def test_overwrite_preserves_id_and_pointer(client, store): assert client.get("/api/settings").json()["active_agent_profile_id"] == pid +def test_create_mints_fresh_id_ignoring_client_id(client): + """Creating a new name never reuses a client-supplied id (ids stay unique). + + Duplicate ids would make the id-keyed active pointer ambiguous — deleting + one profile could clear the active selection while a namesake id lives on. + """ + client.post("/api/agent-profiles/a", json={"llm_profile_ref": "x"}) + a_id = client.get("/api/agent-profiles/a").json()["profile"]["id"] + + # Try to create 'b' reusing a's id; the server must mint a fresh one. + client.post("/api/agent-profiles/b", json={"llm_profile_ref": "y", "id": a_id}) + b_id = client.get("/api/agent-profiles/b").json()["profile"]["id"] + assert b_id != a_id + + # Activate b, delete a: the pointer must survive (ids are distinct). + client.post(f"/api/agent-profiles/{b_id}/activate") + client.delete("/api/agent-profiles/a") + assert client.get("/api/settings").json()["active_agent_profile_id"] == b_id + + def test_save_path_name_is_authoritative(client, store): """The path name overrides any ``name`` in the body.""" response = client.post( From 1f22f61b1a8e6941b08d68008830a2ef9f86657b Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Thu, 18 Jun 2026 09:42:35 +0200 Subject: [PATCH 7/8] Align router conventions with the LLM profiles_router Cosmetic consistency, no behavior change to status codes: - _store_errors now maps the same set as profiles_router (TimeoutError -> 503, ValueError -> 400); FileNotFoundError/FileExistsError are handled inline per endpoint with clean, resource-specific messages (matching get/rename there). - save's 422 uses FastAPI's request-validation shape (detail = list of error objects), trimmed to loc/type to stay secret-safe. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 53 +++++++++++-------- .../test_agent_profiles_router.py | 10 +++- 2 files changed, 39 insertions(+), 24 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index 1571ae015c..35aa92fb8f 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -37,7 +37,6 @@ AgentProfileStore, OpenHandsAgentProfile, ProfileLimitExceeded, - ProfileReferenced, ProfileVerificationSettings, validate_agent_profile, ) @@ -107,16 +106,14 @@ class RenameAgentProfileRequest(BaseModel): @contextmanager def _store_errors() -> Iterator[None]: - """Map ``AgentProfileStore`` / FK errors to HTTP responses.""" + """Map ``AgentProfileStore`` errors to HTTP responses. + + Mirrors ``profiles_router._store_errors``: ``TimeoutError`` and + ``ValueError`` only. ``FileNotFoundError`` / ``FileExistsError`` are handled + inline per-endpoint so each gets a clean, resource-specific message. + """ try: yield - except ProfileReferenced as e: - # Names the referrers so the caller knows what to detach first. - raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) - except FileExistsError as e: - raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) - except FileNotFoundError as e: - raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e)) except TimeoutError: raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, @@ -348,8 +345,14 @@ async def get_agent_profile( cipher = get_cipher(request) store = AgentProfileStore() - with _store_errors(): - profile = store.load(name, cipher=cipher) + try: + with _store_errors(): + profile = store.load(name, cipher=cipher) + except FileNotFoundError: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Agent profile '{name}' not found", + ) # The store leaves skills[].mcp_tools encrypted on load; decrypt to plaintext # so the expose serializer can correctly redact / re-encrypt / reveal them. @@ -381,17 +384,12 @@ async def save_agent_profile( try: profile = validate_agent_profile({**body, "name": name}) except ValidationError as e: - # Surface field locations + error types so the client can fix the body, - # but omit ``input``/``msg`` — a nested mcp_tools MCPConfig error embeds - # the input (which may carry secrets) in its message. + # Match FastAPI's request-validation shape (``detail`` is a list of error + # objects), but surface only ``loc``/``type`` — a nested mcp_tools + # MCPConfig error embeds the input (which may carry secrets) in ``msg``. raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail={ - "message": "Invalid agent profile", - "errors": [ - {"loc": err["loc"], "type": err["type"]} for err in e.errors() - ], - }, + detail=[{"loc": err["loc"], "type": err["type"]} for err in e.errors()], ) except Exception: # Any other validation failure (e.g. SkillValidationError from a @@ -485,8 +483,19 @@ async def rename_agent_profile( ``new_name`` is taken. """ store = AgentProfileStore() - with _store_errors(): - store.rename(name, body.new_name) + try: + with _store_errors(): + store.rename(name, body.new_name) + except FileNotFoundError: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Agent profile '{name}' not found", + ) + except FileExistsError: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=f"Agent profile '{body.new_name}' already exists", + ) if name == body.new_name: message = f"Agent profile '{name}' unchanged (same name)" diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 7d7867084b..7543156923 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -251,12 +251,16 @@ def test_save_acp_profile(client, store): def test_save_missing_required_ref_returns_422(client): - """A missing required field is rejected and the field location is surfaced.""" + """A missing required field is rejected and the field location is surfaced. + + ``detail`` mirrors FastAPI's request-validation shape: a list of error + objects (here trimmed to loc/type to avoid leaking secret-bearing input). + """ response = client.post("/api/agent-profiles/bad", json={}) assert response.status_code == 422 detail = response.json()["detail"] # The discriminated union tags the location with the variant ("openhands"). - assert any("llm_profile_ref" in err["loc"] for err in detail["errors"]) + assert any("llm_profile_ref" in err["loc"] for err in detail) def test_save_invalid_body_does_not_leak_mcp_secret(client): @@ -312,6 +316,7 @@ def test_get_returns_profile(client, store): def test_get_not_found(client): response = client.get("/api/agent-profiles/nonexistent") assert response.status_code == 404 + assert "not found" in response.json()["detail"].lower() def test_get_corrupted_returns_400(client, temp_agent_profiles_dir): @@ -379,6 +384,7 @@ def test_rename_conflict(client, store): json={"new_name": "target"}, ) assert response.status_code == 409 + assert "already exists" in response.json()["detail"].lower() def test_rename_invalid_new_name_returns_422(client, store): From bfbe52a5a6a03e8de757b3e700c55d0557058331 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Thu, 18 Jun 2026 10:20:20 +0200 Subject: [PATCH 8/8] Address review 8: close save TOCTOU + cleanup - save_agent_profile now holds the store lock across the identity read + id mint + write, so two concurrent creates of the same new name converge on one id instead of clobbering each other (mirrors the seed path). Regression test added. - Comment agent_settings_applied as intentionally always-False (pointer-only; materialize #3717 is the path that resolves into settings). - Trim the _seed_default_profile / _decrypt_profile_mcp_tools docstrings to the non-obvious why. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/agent_profiles_router.py | 49 +++++++++---------- .../test_agent_profiles_router.py | 25 ++++++++++ 2 files changed, 49 insertions(+), 25 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py index 35aa92fb8f..9776020fe7 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -92,6 +92,9 @@ class AgentProfileMutationResponse(BaseModel): class ActivateAgentProfileResponse(BaseModel): id: str message: str + # Always False: activation is pointer-only by contract. The field documents + # that agent_settings was untouched; materialize (#3717) is the path that + # resolves a profile into settings. agent_settings_applied: bool = False @@ -148,15 +151,12 @@ def _decrypt_mcp_tools(tools: dict[str, Any], cipher: Cipher) -> dict[str, Any]: def _decrypt_profile_mcp_tools( profile: OpenHandsAgentProfile | ACPAgentProfile, cipher: Cipher | None ) -> OpenHandsAgentProfile | ACPAgentProfile: - """Decrypt Fernet-encrypted ``skills[].mcp_tools`` env/headers on a profile. - - ``AgentProfileStore`` masks/encrypts these on save but has no symmetric - load-time validator, so both the GET (at-rest) and the save (client - round-tripped) values carry ciphertext. Decrypting here gives the GET expose - serializer plaintext to re-mask, and stops the save path from - double-encrypting an already-encrypted value (the round-trip the resolver - would otherwise decrypt once and get a stale token). No-op without a cipher - or on ACP profiles (no skills). + """Decrypt Fernet ``skills[].mcp_tools`` env/headers (no-op without a cipher). + + The store masks/encrypts these on save but has no symmetric load-time + validator, so values arrive as ciphertext on both GET (at-rest) and save + (client round-trip). Decrypting here lets GET re-mask from plaintext and + stops save from double-encrypting an already-encrypted value. """ if cipher is None: return profile @@ -243,9 +243,8 @@ def _seed_default_profile( ) -> None: """Persist one default profile and point ``active_agent_profile_id`` at it. - Holds the store lock across the empty-check + save + pointer write so - concurrent first requests seed exactly once (the loser sees a non-empty - store and returns); the pointer always matches the persisted profile id. + The lock spans empty-check + save + pointer write so concurrent first + requests seed exactly once and the pointer matches the persisted id. """ with _store_errors(), store._acquire_lock(): # Double-checked under the lock: a concurrent first request may have @@ -406,20 +405,20 @@ async def save_agent_profile( profile = _decrypt_profile_mcp_tools(profile, cipher) store = AgentProfileStore() - # The id is server-managed state, never a client-settable field — the active - # pointer is keyed on it. Overwriting a namesake keeps its id (so the pointer - # never dangles) and bumps the revision; creating a new name always mints a - # fresh id (ignoring any client-supplied one) so ids stay globally unique and - # the pointer is never ambiguous. - existing_id, existing_rev = _existing_identity(store, name) - if existing_id is not None: - profile = profile.model_copy( - update={"id": existing_id, "revision": (existing_rev or 0) + 1} - ) - else: - profile = profile.model_copy(update={"id": uuid4()}) + # The id is server-managed (the active pointer is keyed on it): overwrite + # keeps the namesake's id and bumps revision; create mints a fresh id, + # ignoring any client-supplied one. The lock spans read + mint + save so two + # concurrent creates of the same new name can't both mint an id and clobber + # each other (the seed path guards the same window). try: - with _store_errors(): + with _store_errors(), store._acquire_lock(): + existing_id, existing_rev = _existing_identity(store, name) + if existing_id is not None: + profile = profile.model_copy( + update={"id": existing_id, "revision": (existing_rev or 0) + 1} + ) + else: + profile = profile.model_copy(update={"id": uuid4()}) store.save(profile, cipher=cipher, max_profiles=MAX_AGENT_PROFILES) except ProfileLimitExceeded: raise HTTPException( diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 7543156923..fcd76971c2 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -225,6 +225,31 @@ def test_create_mints_fresh_id_ignoring_client_id(client): assert client.get("/api/settings").json()["active_agent_profile_id"] == b_id +def test_concurrent_create_same_name_converges_on_one_id(client, store): + """Concurrent creates of the same new name yield one profile with one id. + + The save path holds the store lock across read + id-mint + write, so the + second writer sees the namesake and preserves its id instead of clobbering + it with a fresh one (which would dangle an active pointer). + """ + with concurrent.futures.ThreadPoolExecutor(max_workers=8) as ex: + codes = list( + ex.map( + lambda _: ( + client.post( + "/api/agent-profiles/dup", json={"llm_profile_ref": "x"} + ).status_code + ), + range(8), + ) + ) + + assert all(code == 201 for code in codes) + summaries = store.list_summaries() + assert len(summaries) == 1 + assert len({s["id"] for s in summaries}) == 1 + + def test_save_path_name_is_authoritative(client, store): """The path name overrides any ``name`` in the body.""" response = client.post(