From 3a6a04d224524d014b7b837140d50e0c39fac443 Mon Sep 17 00:00:00 2001
From: kevin9327 <kevin9327@users.noreply.github.com>
Date: Sun, 30 Aug 2026 15:00:38 +0900
Subject: [PATCH] fix(agent): surface HTTP errors from the unload_models tool

The unload_models tool discarded the response from POST /model/unload-all
and always returned the success string, so when the unload failed (any
4xx/5xx) the assistant and user were told VRAM had been freed when it had
not. Every other POST tool in execute_tool calls raise_for_status(), and
the MCP server's modly_unload_models does too; this brings the tool in
line so the shared HTTPStatusError handler reports the failure.

Adds api/tests/test_agent_router.py covering the error path (fails before
this change, passes after) and the success path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 api/routers/agent.py           |  3 +-
 api/tests/test_agent_router.py | 53 ++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 api/tests/test_agent_router.py

diff --git a/api/routers/agent.py b/api/routers/agent.py
index 3eeb2a73..40329414 100644
--- a/api/routers/agent.py
+++ b/api/routers/agent.py
@@ -267,7 +267,8 @@ async def execute_tool(name: str, arguments: dict, context: dict) -> tuple[str,
                 return f"Available models:\n{lines}", None
 
             elif name == "unload_models":
-                await client.post(f"{MODLY_API}/model/unload-all")
+                r = await client.post(f"{MODLY_API}/model/unload-all")
+                r.raise_for_status()
                 return "All 3D generation models have been unloaded from VRAM.", None
 
             elif name == "get_mesh_info":
diff --git a/api/tests/test_agent_router.py b/api/tests/test_agent_router.py
new file mode 100644
index 00000000..ecd2ed7c
--- /dev/null
+++ b/api/tests/test_agent_router.py
@@ -0,0 +1,53 @@
+import asyncio
+import unittest
+from unittest import mock
+
+import httpx
+
+import routers.agent as agent
+
+
+class _MockClientFactory:
+    """Builds real AsyncClients wired to a MockTransport, so execute_tool talks to
+    a fake Modly API instead of the network."""
+
+    def __init__(self, handler) -> None:
+        self._handler = handler
+        self._real = httpx.AsyncClient
+
+    def __call__(self, *args, **kwargs):
+        kwargs["transport"] = httpx.MockTransport(self._handler)
+        return self._real(*args, **kwargs)
+
+
+def _run_tool(name: str, handler) -> tuple[str, object]:
+    factory = _MockClientFactory(handler)
+    with mock.patch.object(agent.httpx, "AsyncClient", factory):
+        return asyncio.run(agent.execute_tool(name, {}, {}))
+
+
+class UnloadModelsErrorTests(unittest.TestCase):
+    """unload_models must report a failed unload, not claim success (like every
+    other POST tool and like the MCP server's modly_unload_models)."""
+
+    def test_http_error_is_surfaced(self) -> None:
+        def handler(request: httpx.Request) -> httpx.Response:
+            return httpx.Response(500, text="boom")
+
+        text, payload = _run_tool("unload_models", handler)
+        # Before the fix the response was discarded and the success string was
+        # returned even on a 500; now the shared HTTPStatusError handler runs.
+        self.assertTrue(text.startswith("API error 500"), text)
+        self.assertIsNone(payload)
+
+    def test_success_still_reports_unloaded(self) -> None:
+        def handler(request: httpx.Request) -> httpx.Response:
+            return httpx.Response(200, json={"ok": True})
+
+        text, payload = _run_tool("unload_models", handler)
+        self.assertIn("unloaded", text.lower())
+        self.assertIsNone(payload)
+
+
+if __name__ == "__main__":
+    unittest.main()