Skip to content

Commit 1f22f61

Browse files
Simon Rosenbergclaude
andcommitted
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) <noreply@anthropic.com>
1 parent d8978d2 commit 1f22f61

2 files changed

Lines changed: 39 additions & 24 deletions

File tree

openhands-agent-server/openhands/agent_server/agent_profiles_router.py

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
AgentProfileStore,
3838
OpenHandsAgentProfile,
3939
ProfileLimitExceeded,
40-
ProfileReferenced,
4140
ProfileVerificationSettings,
4241
validate_agent_profile,
4342
)
@@ -107,16 +106,14 @@ class RenameAgentProfileRequest(BaseModel):
107106

108107
@contextmanager
109108
def _store_errors() -> Iterator[None]:
110-
"""Map ``AgentProfileStore`` / FK errors to HTTP responses."""
109+
"""Map ``AgentProfileStore`` errors to HTTP responses.
110+
111+
Mirrors ``profiles_router._store_errors``: ``TimeoutError`` and
112+
``ValueError`` only. ``FileNotFoundError`` / ``FileExistsError`` are handled
113+
inline per-endpoint so each gets a clean, resource-specific message.
114+
"""
111115
try:
112116
yield
113-
except ProfileReferenced as e:
114-
# Names the referrers so the caller knows what to detach first.
115-
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e))
116-
except FileExistsError as e:
117-
raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e))
118-
except FileNotFoundError as e:
119-
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=str(e))
120117
except TimeoutError:
121118
raise HTTPException(
122119
status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
@@ -348,8 +345,14 @@ async def get_agent_profile(
348345
cipher = get_cipher(request)
349346

350347
store = AgentProfileStore()
351-
with _store_errors():
352-
profile = store.load(name, cipher=cipher)
348+
try:
349+
with _store_errors():
350+
profile = store.load(name, cipher=cipher)
351+
except FileNotFoundError:
352+
raise HTTPException(
353+
status_code=status.HTTP_404_NOT_FOUND,
354+
detail=f"Agent profile '{name}' not found",
355+
)
353356

354357
# The store leaves skills[].mcp_tools encrypted on load; decrypt to plaintext
355358
# so the expose serializer can correctly redact / re-encrypt / reveal them.
@@ -381,17 +384,12 @@ async def save_agent_profile(
381384
try:
382385
profile = validate_agent_profile({**body, "name": name})
383386
except ValidationError as e:
384-
# Surface field locations + error types so the client can fix the body,
385-
# but omit ``input``/``msg`` — a nested mcp_tools MCPConfig error embeds
386-
# the input (which may carry secrets) in its message.
387+
# Match FastAPI's request-validation shape (``detail`` is a list of error
388+
# objects), but surface only ``loc``/``type`` — a nested mcp_tools
389+
# MCPConfig error embeds the input (which may carry secrets) in ``msg``.
387390
raise HTTPException(
388391
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
389-
detail={
390-
"message": "Invalid agent profile",
391-
"errors": [
392-
{"loc": err["loc"], "type": err["type"]} for err in e.errors()
393-
],
394-
},
392+
detail=[{"loc": err["loc"], "type": err["type"]} for err in e.errors()],
395393
)
396394
except Exception:
397395
# Any other validation failure (e.g. SkillValidationError from a
@@ -485,8 +483,19 @@ async def rename_agent_profile(
485483
``new_name`` is taken.
486484
"""
487485
store = AgentProfileStore()
488-
with _store_errors():
489-
store.rename(name, body.new_name)
486+
try:
487+
with _store_errors():
488+
store.rename(name, body.new_name)
489+
except FileNotFoundError:
490+
raise HTTPException(
491+
status_code=status.HTTP_404_NOT_FOUND,
492+
detail=f"Agent profile '{name}' not found",
493+
)
494+
except FileExistsError:
495+
raise HTTPException(
496+
status_code=status.HTTP_409_CONFLICT,
497+
detail=f"Agent profile '{body.new_name}' already exists",
498+
)
490499

491500
if name == body.new_name:
492501
message = f"Agent profile '{name}' unchanged (same name)"

tests/agent_server/test_agent_profiles_router.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -251,12 +251,16 @@ def test_save_acp_profile(client, store):
251251

252252

253253
def test_save_missing_required_ref_returns_422(client):
254-
"""A missing required field is rejected and the field location is surfaced."""
254+
"""A missing required field is rejected and the field location is surfaced.
255+
256+
``detail`` mirrors FastAPI's request-validation shape: a list of error
257+
objects (here trimmed to loc/type to avoid leaking secret-bearing input).
258+
"""
255259
response = client.post("/api/agent-profiles/bad", json={})
256260
assert response.status_code == 422
257261
detail = response.json()["detail"]
258262
# The discriminated union tags the location with the variant ("openhands").
259-
assert any("llm_profile_ref" in err["loc"] for err in detail["errors"])
263+
assert any("llm_profile_ref" in err["loc"] for err in detail)
260264

261265

262266
def test_save_invalid_body_does_not_leak_mcp_secret(client):
@@ -312,6 +316,7 @@ def test_get_returns_profile(client, store):
312316
def test_get_not_found(client):
313317
response = client.get("/api/agent-profiles/nonexistent")
314318
assert response.status_code == 404
319+
assert "not found" in response.json()["detail"].lower()
315320

316321

317322
def test_get_corrupted_returns_400(client, temp_agent_profiles_dir):
@@ -379,6 +384,7 @@ def test_rename_conflict(client, store):
379384
json={"new_name": "target"},
380385
)
381386
assert response.status_code == 409
387+
assert "already exists" in response.json()["detail"].lower()
382388

383389

384390
def test_rename_invalid_new_name_returns_422(client, store):

0 commit comments

Comments
 (0)