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 9a448c05e7..8dccfd4445 100644 --- a/openhands-agent-server/openhands/agent_server/agent_profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/agent_profiles_router.py @@ -39,6 +39,7 @@ SEED_PROFILE_NAME, AgentProfileDiagnostics, AgentProfileStore, + OpenHandsAgentProfile, ProfileLimitExceeded, build_seed_profile, resolve_agent_profile_dry_run, @@ -72,6 +73,7 @@ class AgentProfileInfo(BaseModel): revision: int | None = None llm_profile_ref: str | None = None mcp_server_refs: list[str] | None = None + llm_profile_ref_matches_active: bool | None = None class AgentProfileListResponse(BaseModel): @@ -82,6 +84,7 @@ class AgentProfileListResponse(BaseModel): class AgentProfileDetailResponse(BaseModel): name: str profile: dict[str, Any] + llm_profile_ref_matches_active: bool | None = None class AgentProfileMutationResponse(BaseModel): @@ -242,6 +245,36 @@ def set_pointer(s: PersistedSettings) -> PersistedSettings: logger.info(f"Seeded default agent profile '{profile.name}' (id={profile_id})") +def _llm_profile_ref_matches_active( + agent_kind: str, + llm_profile_ref: str | None, + active_profile: str | None, +) -> bool | None: + """Detect drift between a profile's ``llm_profile_ref`` and the active LLM + profile (#4338). + + Activating an LLM profile (``POST /api/profiles/{name}/activate``) only + ever updates ``settings.active_profile`` — it never touches any stored + ``AgentProfile.llm_profile_ref`` — so the two can silently point at + different LLM profiles with no error surfaced anywhere. This is a + read-only diagnostic: it never repairs the drift, only reports it. + + Tri-state, not boolean: ``None`` means "not applicable / unknown" and + must never be read as "stale" — + - an ACP profile carries no ``llm_profile_ref`` at all, so comparison is + meaningless (``agent_kind != "openhands"``); + - no LLM profile is currently active (``active_profile is None``), so + there is nothing to compare against. + Only an explicit ``False`` means the ref has drifted from the active LLM + profile; only ``True`` means it is current. + """ + if agent_kind != "openhands": + return None + if active_profile is None: + return None + return llm_profile_ref == active_profile + + 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(): @@ -275,14 +308,30 @@ async def list_agent_profiles(request: Request) -> AgentProfileListResponse: with store_errors(): summaries = store.list_summaries() + # AgentProfileInfo(**s) can't pick up llm_profile_ref_matches_active on its + # own: `s` is a raw store summary dict (`{id, name, agent_kind, revision, + # llm_profile_ref, mcp_server_refs}`) with no such key, so the computed + # value must be injected explicitly rather than relying on the splat. return AgentProfileListResponse( - profiles=[AgentProfileInfo(**s) for s in summaries], + profiles=[ + AgentProfileInfo( + **s, + llm_profile_ref_matches_active=_llm_profile_ref_matches_active( + s.get("agent_kind", "openhands"), + s.get("llm_profile_ref"), + settings.active_profile, + ), + ) + 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(name: ProfileName) -> AgentProfileDetailResponse: +async def get_agent_profile( + request: Request, name: ProfileName +) -> AgentProfileDetailResponse: """Get a stored profile. A profile is secret-free at rest (#4017), so there is nothing to mask or @@ -299,8 +348,32 @@ async def get_agent_profile(name: ProfileName) -> AgentProfileDetailResponse: detail=f"Agent profile '{name}' not found", ) + settings_store = get_settings_store(get_config(request)) + # The staleness flag below is an optional diagnostic (#4338) riding along + # on the primary read — it must never turn a successful profile load into + # a 500. FileSettingsStore.load() deliberately re-raises PermissionError/ + # OSError for a genuinely unreadable settings file; degrade to + # active_profile=None (the flag's own "unknown" state) instead of + # propagating. + try: + settings = settings_store.load() or PersistedSettings() + active_profile = settings.active_profile + except OSError: + active_profile = None + payload = profile.model_dump(mode="json") - return AgentProfileDetailResponse(name=name, profile=payload) + llm_profile_ref = ( + profile.llm_profile_ref if isinstance(profile, OpenHandsAgentProfile) else None + ) + return AgentProfileDetailResponse( + name=name, + profile=payload, + llm_profile_ref_matches_active=_llm_profile_ref_matches_active( + profile.agent_kind, + llm_profile_ref, + active_profile, + ), + ) @agent_profiles_router.post( diff --git a/openhands-agent-server/openhands/agent_server/profiles_router.py b/openhands-agent-server/openhands/agent_server/profiles_router.py index cea019baee..80540d04b9 100644 --- a/openhands-agent-server/openhands/agent_server/profiles_router.py +++ b/openhands-agent-server/openhands/agent_server/profiles_router.py @@ -30,6 +30,7 @@ ProfileReferenced, delete_llm_profile, rename_llm_profile, + sync_seed_llm_ref, ) @@ -283,6 +284,7 @@ class ActivateProfileResponse(BaseModel): name: str message: str llm_applied: bool = True + llm_profile_ref_synced: bool = False @profiles_router.post("/{name}/activate", response_model=ActivateProfileResponse) @@ -295,6 +297,10 @@ async def activate_profile( 1. Loads the named profile's LLM configuration 2. Applies it to the current agent settings (updates ``agent_settings.llm``) 3. Records the profile name as the active profile for frontend tracking + 4. Best-effort: repoints the seeded default AgentProfile's + ``llm_profile_ref`` to track this activation, if still eligible + (#4338) — reported via ``llm_profile_ref_synced``; this step never + fails the activation itself Returns 404 if the profile does not exist. @@ -317,8 +323,13 @@ async def activate_profile( # Apply the LLM config to settings and record active profile settings_store = get_settings_store(config) + previous_active_profile: str | None = None def apply_profile(settings: PersistedSettings) -> PersistedSettings: + # Captured under the settings-store lock, so this is an exact + # pre-update read (not a separate load() -> TOCTOU race). + nonlocal previous_active_profile + previous_active_profile = settings.active_profile settings.agent_settings = settings.agent_settings.model_copy( update={"llm": llm} ) @@ -338,8 +349,41 @@ def apply_profile(settings: PersistedSettings) -> PersistedSettings: ) logger.info(f"Activated profile '{name}'") + + # Repoint the seeded default AgentProfile's llm_profile_ref to track this + # activation, if it is still eligible (#4338). This runs *after* + # settings_store.update() has returned and released the settings-store + # lock: _seed_default_profile already nests agent-profile-lock -> + # settings-lock, so acquiring the agent-profile lock while the settings + # lock is still held here would invert that order and deadlock. + # profile_store.list_summaries() below acquires and releases the + # LLM-profile lock entirely before sync_seed_llm_ref acquires the + # agent-profile lock, so this call site as a whole still resolves + # agent-before-llm — the same order rename_llm_profile and + # delete_llm_profile depend on. Keep it that way if this ever gets + # inlined/reordered. It is also best-effort — activation has already + # succeeded and been persisted by this point, so nothing from this + # diagnostic side effect may escape and turn a successful activation + # into a failed request. + llm_profile_ref_synced = False + try: + known_llm_profiles = {s["name"] for s in profile_store.list_summaries()} + llm_profile_ref_synced = sync_seed_llm_ref( + get_agent_profile_store(), + old_ref=previous_active_profile, + new_ref=name, + known_llm_profiles=known_llm_profiles, + ) + except Exception as e: + logger.warning( + f"Failed to sync seed profile llm_profile_ref after activating " + f"'{name}': {e}", + exc_info=True, + ) + return ActivateProfileResponse( name=name, message=f"Profile '{name}' activated and applied to current settings", llm_applied=True, + llm_profile_ref_synced=llm_profile_ref_synced, ) diff --git a/openhands-sdk/openhands/sdk/profiles/__init__.py b/openhands-sdk/openhands/sdk/profiles/__init__.py index 1dc2bf6b24..a603c6db80 100644 --- a/openhands-sdk/openhands/sdk/profiles/__init__.py +++ b/openhands-sdk/openhands/sdk/profiles/__init__.py @@ -24,6 +24,7 @@ delete_llm_profile, find_referrers, rename_llm_profile, + sync_seed_llm_ref, ) from openhands.sdk.profiles.resolver import ( AgentProfileDiagnostics, @@ -64,5 +65,6 @@ "resolve_agent_profile_dry_run", "safe_validation_error_detail", "save_profile_preserving_identity", + "sync_seed_llm_ref", "validate_agent_profile", ] diff --git a/openhands-sdk/openhands/sdk/profiles/profile_refs.py b/openhands-sdk/openhands/sdk/profiles/profile_refs.py index fb8d82f1bf..1ee36f6b73 100644 --- a/openhands-sdk/openhands/sdk/profiles/profile_refs.py +++ b/openhands-sdk/openhands/sdk/profiles/profile_refs.py @@ -2,7 +2,7 @@ An ``OpenHandsAgentProfile.llm_profile_ref`` is a soft FK onto an LLM-profile store key. ``find_referrers`` / ``cascade_rename`` / ``delete_llm_profile`` / -``rename_llm_profile`` keep that FK from dangling. +``rename_llm_profile`` / ``sync_seed_llm_ref`` keep that FK from dangling. Store-agnostic: these touch the agent-profile store only through :class:`~openhands.sdk.profiles.agent_profile_store.AgentProfileStoreProtocol`, @@ -20,9 +20,12 @@ from openhands.sdk.logger import get_logger from openhands.sdk.profiles.agent_profile_store import PROFILE_NAME_REGEX +from openhands.sdk.profiles.seed import SEED_PROFILE_NAME if TYPE_CHECKING: + from collections.abc import Collection + from openhands.sdk.llm.llm_profile_store import LLMProfileMutator from openhands.sdk.profiles.agent_profile_store import AgentProfileStoreProtocol @@ -147,3 +150,104 @@ def rename_llm_profile( with agent_store.lock(): llm_store.rename(old_name, new_name) return _rewrite_refs(agent_store, old_name, new_name) + + +def sync_seed_llm_ref( + store: AgentProfileStoreProtocol, + *, + old_ref: str | None, + new_ref: str, + known_llm_profiles: Collection[str] | None = None, + profile_name: str = SEED_PROFILE_NAME, +) -> bool: + """Repoint the seed profile's ``llm_profile_ref``, but only while in sync. + + Activating an LLM profile (``POST /profiles/{name}/activate``) is supposed + to keep the seeded default profile's ref pointed at the active LLM profile + — but a naive unconditional write would clobber a ref the user has since + pinned to something else on purpose, silently discarding their intent + (#4338). So this is deliberately narrower than ``cascade_rename``: it + considers exactly one profile (``profile_name``, normally + :data:`SEED_PROFILE_NAME`) and only rewrites it when the current ref is + still *explainable* as "not yet repointed" rather than "chosen": + + * ``current == old_ref`` — the seed was tracking the previous activation, + so following the new one preserves that tracking behavior, or + * ``current == SEED_PROFILE_NAME`` and ``known_llm_profiles`` is given and + does not contain ``SEED_PROFILE_NAME`` — the dangling soft-ref a freshly + seeded profile is born with before any LLM profile named ``"default"`` + necessarily exists (#3933), not a real pin. + + Any other current ref is a deliberate pin and is left alone. Note that with + ``old_ref=None`` only the second branch can match — a stored ref is always + a string, never ``None``, so the first comparison can't accidentally + succeed. + + Known gap — state C is never repaired: the seeded profile is born in one + of three states. (A) an LLM profile was already active, so the ref names + it — repaired by the first branch above. (B) no active profile and no + real LLM config, so the ref is ``SEED_PROFILE_NAME`` and dangles — + repaired by the soft-ref branch above. (C) no active profile but a + *real* LLM config, so ``_seed_default_llm_profile`` mints an actual LLM + profile named ``SEED_PROFILE_NAME`` mirroring it and the ref resolves. + State C is indistinguishable from a user having deliberately pinned a + profile they authored and named ``"default"`` themselves — there is no + sound discriminator between the two. In particular ``revision`` does not + work: ``save_profile_preserving_identity`` bumps it only on overwrite, + never on create, and it defaults to ``0``, so a user's own first save of + a ``"default"`` profile is ``revision == 0``, identical to the seed. + Treating state C as eligible would risk exactly the clobber this + function exists to prevent, so it stays unrepaired; the resulting drift + is not invisible — ``llm_profile_ref_matches_active`` in + ``agent_profiles_router.py`` reports it as ``False``, which is what that + flag is for. + + Concurrency note: two overlapping activations can interleave their + read-check-write windows and settle with the seed ref naming one + activated profile while ``active_profile`` names another — e.g. the + later activation's sync runs first, finds itself ineligible, and + declines, then the earlier activation's sync runs after and writes. The + outcome is a stale ref, the same condition the staleness flag surfaces; + it is not corruption, since each write is still atomic under the store + lock. + + ``known_llm_profiles`` is a plain collection of names rather than an LLM + store/mutator handle: ``LLMProfileMutator`` exposes only + ``delete``/``rename`` and cannot answer an existence query, so accepting a + store here would force every backend (including cloud adapters) to grow a + lookup it may not otherwise need. Callers pass whatever listing they + already have to hand. + + Holds :meth:`~AgentProfileStoreProtocol.lock` for the whole + read-check-write, closing the same TOCTOU window as ``cascade_rename``. + Returns ``True`` iff a write happened; ``current == new_ref`` returns + ``False`` without writing, so an already-synced seed produces no revision + churn and no mtime change. + """ + _validate_name(new_ref) + with store.lock(): + summary = next( + (s for s in store.list_summaries() if s.get("name") == profile_name), + None, + ) + if summary is None: + return False + if summary.get("agent_kind", "openhands") != "openhands": + return False + + current = summary.get("llm_profile_ref") + eligible = current == old_ref or ( + current == SEED_PROFILE_NAME + and known_llm_profiles is not None + and SEED_PROFILE_NAME not in known_llm_profiles + ) + if not eligible or current == new_ref: + return False + + store.set_llm_profile_ref(profile_name, new_ref) + + logger.info( + f"[Profile FK] Synced seed profile `{profile_name}` llm_profile_ref " + f"`{current}` -> `{new_ref}`." + ) + return True diff --git a/tests/agent_server/test_agent_profiles_router.py b/tests/agent_server/test_agent_profiles_router.py index 735be21376..6dfb19c144 100644 --- a/tests/agent_server/test_agent_profiles_router.py +++ b/tests/agent_server/test_agent_profiles_router.py @@ -575,6 +575,109 @@ def test_get_corrupted_returns_400(client, temp_agent_profiles_dir): assert response.status_code == 400 +# ── Staleness flag (llm_profile_ref_matches_active, #4338) ───────────────── + + +def test_list_llm_profile_ref_matches_active_true_and_false( + client, store, default_llm_profile_store +): + """Two OpenHands profiles in one list response: current vs. drifted. + + Activating an LLM profile never updates AgentProfile.llm_profile_ref + (#4338), so a profile whose ref still names the old active LLM must read + False, while one whose ref already matches reads True. + """ + default_llm_profile_store.save("my-llm", LLM(model="gpt-4o-mini")) + store.save(OpenHandsAgentProfile(name="current", llm_profile_ref="my-llm")) + store.save(OpenHandsAgentProfile(name="stale", llm_profile_ref="old-llm")) + client.patch("/api/settings", json={"active_profile": "my-llm"}) + + body = client.get("/api/agent-profiles").json() + by_name = {p["name"]: p for p in body["profiles"]} + assert by_name["current"]["llm_profile_ref_matches_active"] is True + assert by_name["stale"]["llm_profile_ref_matches_active"] is False + + +def test_get_llm_profile_ref_matches_active_consistent_with_list( + client, store, default_llm_profile_store +): + """The detail endpoint agrees with that profile's list entry.""" + default_llm_profile_store.save("my-llm", LLM(model="gpt-4o-mini")) + store.save(OpenHandsAgentProfile(name="stale", llm_profile_ref="old-llm")) + client.patch("/api/settings", json={"active_profile": "my-llm"}) + + list_entry = next( + p + for p in client.get("/api/agent-profiles").json()["profiles"] + if p["name"] == "stale" + ) + detail = client.get("/api/agent-profiles/stale").json() + + assert ( + detail["llm_profile_ref_matches_active"] + == list_entry["llm_profile_ref_matches_active"] + ) + assert detail["llm_profile_ref_matches_active"] is False + + +def test_llm_profile_ref_matches_active_none_for_acp_profile( + client, store, default_llm_profile_store +): + """An ACP profile carries no llm_profile_ref, so the flag is null (not + False) in both the list entry and the detail response.""" + default_llm_profile_store.save("my-llm", LLM(model="gpt-4o-mini")) + store.save(ACPAgentProfile(name="acp-p", acp_server="codex")) + client.patch("/api/settings", json={"active_profile": "my-llm"}) + + list_entry = next( + p + for p in client.get("/api/agent-profiles").json()["profiles"] + if p["name"] == "acp-p" + ) + assert list_entry["llm_profile_ref_matches_active"] is None + + detail = client.get("/api/agent-profiles/acp-p").json() + assert detail["llm_profile_ref_matches_active"] is None + + +def test_llm_profile_ref_matches_active_none_when_no_active_llm_profile(client, store): + """No active LLM profile -> null, never False. + + Tri-state pin: null means "not applicable / unknown", not "stale". A + naive `llm_profile_ref == active_profile` comparison would read False + here (a real string never equals None), which would misreport an + unconfigured account as drifted. + """ + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="some-llm")) + + list_entry = next( + p + for p in client.get("/api/agent-profiles").json()["profiles"] + if p["name"] == "p" + ) + assert list_entry["llm_profile_ref_matches_active"] is None + + detail = client.get("/api/agent-profiles/p").json() + assert detail["llm_profile_ref_matches_active"] is None + + +def test_llm_profile_ref_matches_active_is_additive_only(client, store): + """The new field doesn't disturb any pre-existing field on either + endpoint (additive-only contract).""" + store.save(OpenHandsAgentProfile(name="p", llm_profile_ref="base")) + + list_entry = client.get("/api/agent-profiles").json()["profiles"][0] + assert list_entry["name"] == "p" + assert list_entry["agent_kind"] == "openhands" + assert list_entry["llm_profile_ref"] == "base" + assert list_entry["mcp_server_refs"] is None + + detail = client.get("/api/agent-profiles/p").json() + assert detail["name"] == "p" + assert detail["profile"]["llm_profile_ref"] == "base" + assert detail["profile"]["agent_kind"] == "openhands" + + def test_delete_removes_existing(client, store): store.save(OpenHandsAgentProfile(name="to-delete", llm_profile_ref="x")) diff --git a/tests/agent_server/test_profiles_router.py b/tests/agent_server/test_profiles_router.py index e7530e71a6..e400a56b7d 100644 --- a/tests/agent_server/test_profiles_router.py +++ b/tests/agent_server/test_profiles_router.py @@ -87,6 +87,52 @@ def agent_store(temp_agent_profiles_dir): return AgentProfileStore(base_dir=temp_agent_profiles_dir) +@pytest.fixture +def client_with_agent_profiles_router( + temp_profiles_dir, temp_agent_profiles_dir, temp_settings_dir, monkeypatch +): + """Client wiring *both* the LLM-profile router (this module) and the + agent-profile router to the same temp stores. + + The plain ``client`` fixture only patches ``profiles_router``'s store + getters, so an unpatched ``agent_profiles_router`` (used e.g. by + ``GET /api/agent-profiles/{name}``) would resolve the real singleton + store at the env-var-derived persistence dir instead -- a different + directory than ``agent_store``. The end-to-end seed-sync repro (#4338) + needs activation (this router) and the agent-profile read endpoints to + observe the same on-disk state, so both routers are patched here. + """ + 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) + + llm_store = LLMProfileStore(base_dir=temp_profiles_dir) + agent_store = AgentProfileStore(base_dir=temp_agent_profiles_dir) + + with ( + patch( + "openhands.agent_server.profiles_router.get_llm_profile_store", + lambda: llm_store, + ), + patch( + "openhands.agent_server.profiles_router.get_agent_profile_store", + lambda: agent_store, + ), + patch( + "openhands.agent_server.agent_profiles_router.get_llm_profile_store", + lambda: llm_store, + ), + patch( + "openhands.agent_server.agent_profiles_router.get_agent_profile_store", + lambda: agent_store, + ), + ): + yield TestClient(app) + + reset_stores() + + # ── FK Guard: deleting/renaming a referenced LLM profile ──────────────────── @@ -1248,3 +1294,119 @@ def test_list_profiles_no_auto_create_after_deleting_active_profile(client, stor body = response.json() assert body["profiles"] == [] assert body["active_profile"] is None + + +# ── Seed Sync on Activation (#4338) ───────────────────────────────────────── + + +def test_activate_profile_syncs_seed_default_llm_profile_ref( + client_with_agent_profiles_router, store +): + """End-to-end repro of the issue's steps: save profile-a/profile-b, + activate profile-a, let GET /api/agent-profiles lazily seed the 'default' + AgentProfile the way a real client triggers it, then activate profile-b + -- the seeded default's llm_profile_ref must follow the new activation + instead of silently pointing at profile-a.""" + llm = LLM(model="gpt-4o") + store.save("profile-a", llm) + store.save("profile-b", llm) + + client_with_agent_profiles_router.post("/api/profiles/profile-a/activate") + + # Trigger the real lazy seed (rather than hand-writing the 'default' + # AgentProfile via agent_store.save), so this test verifies the helper + # against a state the seeding logic itself produced. profile-a is active + # at seed time, so this lands in state A and the seed must already track + # it -- assert that precondition explicitly so the test fails loudly if + # seeding behavior ever changes, instead of silently testing nothing. + seeded = client_with_agent_profiles_router.get("/api/agent-profiles").json() + default_summary = next(p for p in seeded["profiles"] if p["name"] == "default") + assert default_summary["llm_profile_ref"] == "profile-a" + + response = client_with_agent_profiles_router.post( + "/api/profiles/profile-b/activate" + ) + + assert response.status_code == 200 + assert response.json()["llm_profile_ref_synced"] is True + + detail = client_with_agent_profiles_router.get("/api/agent-profiles/default").json() + assert detail["profile"]["llm_profile_ref"] == "profile-b" + + +def test_activate_profile_does_not_clobber_pinned_seed_ref( + client_with_agent_profiles_router, store, agent_store +): + """A deliberately pinned seed ref (llm_profile_ref='profile-c', not the + previously-active profile) survives activation of a different profile -- + sync_seed_llm_ref's eligibility check refuses to overwrite a real pin.""" + llm = LLM(model="gpt-4o") + store.save("profile-a", llm) + store.save("profile-b", llm) + store.save("profile-c", llm) + + client_with_agent_profiles_router.post("/api/profiles/profile-a/activate") + agent_store.save(OpenHandsAgentProfile(name="default", llm_profile_ref="profile-c")) + + response = client_with_agent_profiles_router.post( + "/api/profiles/profile-b/activate" + ) + + assert response.status_code == 200 + assert response.json()["llm_profile_ref_synced"] is False + + detail = client_with_agent_profiles_router.get("/api/agent-profiles/default").json() + assert detail["profile"]["llm_profile_ref"] == "profile-c" + + +def test_activate_profile_sync_timeout_is_best_effort( + client, store, agent_store, monkeypatch +): + """A TimeoutError raised while syncing the seed ref must not turn a + successful activation into an error response -- the sync is best-effort + and activation has already succeeded by the time it runs.""" + llm = LLM(model="gpt-4o") + store.save("profile-a", llm) + agent_store.save(OpenHandsAgentProfile(name="default", llm_profile_ref="profile-a")) + + def boom(self, timeout=None): + raise TimeoutError("locked") + + monkeypatch.setattr(AgentProfileStore, "lock", boom) + + response = client.post("/api/profiles/profile-a/activate") + + assert response.status_code == 200 + body = response.json() + assert body["llm_profile_ref_synced"] is False + + settings = client.get("/api/settings").json() + assert settings["agent_settings"]["llm"]["model"] == "gpt-4o" + + +def test_activate_profile_empty_agent_profile_store_no_sync(client, store): + """Activation against an empty, never-seeded agent-profile store is a + quiet no-op sync: 200, llm_profile_ref_synced=False, no exception.""" + llm = LLM(model="gpt-4o") + store.save("profile-a", llm) + + response = client.post("/api/profiles/profile-a/activate") + + assert response.status_code == 200 + assert response.json()["llm_profile_ref_synced"] is False + + +def test_delete_referenced_llm_profile_returns_409_default_referrer( + client, store, agent_store +): + """Regression pin for the FK guard: it already returns 409 naming + 'default' as a referrer when the seeded AgentProfile still cites the LLM + profile being deleted. Not a bug fix -- pins the existing guard so it + cannot silently regress once activation starts writing to 'default'.""" + store.save("profile-a", LLM(model="gpt-4o")) + agent_store.save(OpenHandsAgentProfile(name="default", llm_profile_ref="profile-a")) + + response = client.delete("/api/profiles/profile-a") + + assert response.status_code == 409 + assert "default" in response.json()["detail"] diff --git a/tests/sdk/profiles/test_profile_refs.py b/tests/sdk/profiles/test_profile_refs.py index 04bfe5061c..fab3844df3 100644 --- a/tests/sdk/profiles/test_profile_refs.py +++ b/tests/sdk/profiles/test_profile_refs.py @@ -9,6 +9,7 @@ from openhands.sdk.llm import LLM from openhands.sdk.llm.llm_profile_store import LLMProfileStore from openhands.sdk.profiles import ( + SEED_PROFILE_NAME, ACPAgentProfile, AgentProfileStore, OpenHandsAgentProfile, @@ -17,6 +18,7 @@ delete_llm_profile, find_referrers, rename_llm_profile, + sync_seed_llm_ref, ) @@ -220,3 +222,135 @@ def renamer() -> None: assert sorted(find_referrers(store, "renamed")) == sorted( f"p{i}" for i in range(num) ) + + +# ── sync_seed_llm_ref ─────────────────────────────────────────────────────── + + +def test_sync_seed_llm_ref_repoints_when_in_sync( + agent_store: AgentProfileStore, +) -> None: + agent_store.save(_oh(SEED_PROFILE_NAME, "profile-a")) + + result = sync_seed_llm_ref(agent_store, old_ref="profile-a", new_ref="profile-b") + + assert result is True + assert _ref(agent_store, SEED_PROFILE_NAME) == "profile-b" + + +def test_sync_seed_llm_ref_does_not_clobber_pinned_ref( + agent_store: AgentProfileStore, +) -> None: + """The anti-clobber guarantee: a seed ref that has drifted away from + ``old_ref`` was deliberately pinned by the user and must never be + overwritten by an activation-triggered sync (#4338).""" + agent_store.save(_oh(SEED_PROFILE_NAME, "profile-c")) + + result = sync_seed_llm_ref(agent_store, old_ref="profile-a", new_ref="profile-b") + + assert result is False + assert _ref(agent_store, SEED_PROFILE_NAME) == "profile-c" + + +def test_sync_seed_llm_ref_repoints_dangling_soft_ref( + agent_store: AgentProfileStore, +) -> None: + """A fresh instance seeds its default profile with + ``llm_profile_ref == SEED_PROFILE_NAME`` before any LLM profile named + ``"default"`` necessarily exists (#3933). When that name is not among the + known LLM profiles, the ref is a dangling soft-ref, not a pin.""" + agent_store.save(_oh(SEED_PROFILE_NAME, SEED_PROFILE_NAME)) + + result = sync_seed_llm_ref( + agent_store, + old_ref=None, + new_ref="profile-b", + known_llm_profiles=set(), + ) + + assert result is True + assert _ref(agent_store, SEED_PROFILE_NAME) == "profile-b" + + +def test_sync_seed_llm_ref_leaves_real_pin_matching_seed_name( + agent_store: AgentProfileStore, +) -> None: + """If ``SEED_PROFILE_NAME`` resolves to a real, known LLM profile, the ref + is a genuine pin (not the #3933 dangling default) and must be left alone.""" + agent_store.save(_oh(SEED_PROFILE_NAME, SEED_PROFILE_NAME)) + + result = sync_seed_llm_ref( + agent_store, + old_ref="profile-a", + new_ref="profile-b", + known_llm_profiles={SEED_PROFILE_NAME, "profile-a"}, + ) + + assert result is False + assert _ref(agent_store, SEED_PROFILE_NAME) == SEED_PROFILE_NAME + + +def test_sync_seed_llm_ref_acp_profile_returns_false( + agent_store: AgentProfileStore, +) -> None: + agent_store.save(ACPAgentProfile(name=SEED_PROFILE_NAME, acp_server="codex")) + raw_before = (agent_store.base_dir / f"{SEED_PROFILE_NAME}.json").read_text() + + result = sync_seed_llm_ref(agent_store, old_ref="profile-a", new_ref="profile-b") + + assert result is False + raw_after = (agent_store.base_dir / f"{SEED_PROFILE_NAME}.json").read_text() + assert raw_after == raw_before + + +def test_sync_seed_llm_ref_empty_store_returns_false( + agent_store: AgentProfileStore, +) -> None: + result = sync_seed_llm_ref(agent_store, old_ref="profile-a", new_ref="profile-b") + assert result is False + + +def test_sync_seed_llm_ref_noop_when_ref_already_matches_new( + agent_store: AgentProfileStore, +) -> None: + agent_store.save(_oh(SEED_PROFILE_NAME, "profile-a")) + path = agent_store.base_dir / f"{SEED_PROFILE_NAME}.json" + mtime_before = path.stat().st_mtime_ns + + result = sync_seed_llm_ref(agent_store, old_ref="profile-a", new_ref="profile-a") + + assert result is False + assert path.stat().st_mtime_ns == mtime_before + + +def test_sync_seed_llm_ref_only_touches_seed_profile( + agent_store: AgentProfileStore, +) -> None: + """Proves narrow targeting: a namesake with the same stale ref but a + different name is never touched, even though it would match a broader + ``cascade_rename``-style scan.""" + agent_store.save(_oh(SEED_PROFILE_NAME, "profile-a")) + agent_store.save(_oh("other", "profile-a")) + + result = sync_seed_llm_ref(agent_store, old_ref="profile-a", new_ref="profile-b") + + assert result is True + assert _ref(agent_store, SEED_PROFILE_NAME) == "profile-b" + assert _ref(agent_store, "other") == "profile-a" + + +def test_sync_seed_llm_ref_preserves_id_and_revision( + agent_store: AgentProfileStore, +) -> None: + before = OpenHandsAgentProfile( + name=SEED_PROFILE_NAME, llm_profile_ref="profile-a", revision=5 + ) + agent_store.save(before) + + result = sync_seed_llm_ref(agent_store, old_ref="profile-a", new_ref="profile-b") + + assert result is True + after = agent_store.load(SEED_PROFILE_NAME) + assert isinstance(after, OpenHandsAgentProfile) + assert after.id == before.id + assert after.revision == before.revision