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..9776020fe7 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -0,0 +1,555 @@ +"""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. +""" + +import copy +import shlex +from collections.abc import Iterator +from contextlib import contextmanager +from typing import Annotated, Any +from uuid import UUID, uuid4 + +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, + 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__) + +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 + # 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 + + +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`` 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 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 _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 ``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 + 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 ``AgentProfile`` faithfully from the current ``agent_settings``. + + 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). + """ + 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, + # 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, + ) + + +def _seed_default_profile( + store: AgentProfileStore, + request: Request, + settings: PersistedSettings, + cipher: Cipher | None, +) -> None: + """Persist one default profile and point ``active_agent_profile_id`` at it. + + 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 + # 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)) + + 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 + + +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. + + 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, get_cipher(request)) + 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() + 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. + 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) + + 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 as e: + # 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=[{"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 + # 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() + # 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(), 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( + 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() + 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)" + 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: + # 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_500_INTERNAL_SERVER_ERROR, + detail="Failed to activate agent profile", + ) + + 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 371e6f711d..b0aae95734 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 @@ -352,6 +353,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..b3621510ab 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 @@ -245,9 +254,11 @@ 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"] + 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..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 @@ -69,6 +77,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 +125,11 @@ 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, + pattern=UUID_PATTERN, + 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..fcd76971c2 --- /dev/null +++ b/tests/agent_server/test_agent_profiles_router.py @@ -0,0 +1,678 @@ +"""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 concurrent.futures +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 + + +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. + + 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 ───────────────────────────────────────────────────────────────────── + + +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_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_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_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( + "/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): + """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) + + +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): + """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 + assert "not found" in response.json()["detail"].lower() + + +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 + assert "already exists" in response.json()["detail"].lower() + + +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 + + +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) ──────────── + + +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 ───────────────────────────────────────────────────── + + +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..fe75e2f3b2 100644 --- a/tests/agent_server/test_settings_router.py +++ b/tests/agent_server/test_settings_router.py @@ -522,6 +522,64 @@ 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.""" + agent_id = "12345678-1234-1234-1234-1234567890ab" + set_response = client_with_settings.patch( + "/api/settings", + 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"] == agent_id + + # 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_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 +): + """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 +700,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" )