diff --git a/omnigent/chat.py b/omnigent/chat.py index 0a493f6d62..5d62554a1c 100644 --- a/omnigent/chat.py +++ b/omnigent/chat.py @@ -1448,16 +1448,24 @@ async def _prepare_chat_session_via_daemon( from omnigent.native_terminal import bind_session_runner async with OmnigentClient(base_url=base_url, headers=headers, auth=auth) as sdk: - if fork_session_id is not None: - fork_result = await sdk.sessions.fork(fork_session_id) - session_id = fork_result["id"] - elif resume_conversation_id is not None: - session_id = resume_conversation_id - else: - created = await sdk.sessions.create( - bundle, filename="agent.tar.gz", workspace=workspace - ) - session_id = created.id + try: + if fork_session_id is not None: + fork_result = await sdk.sessions.fork(fork_session_id) + session_id = fork_result["id"] + elif resume_conversation_id is not None: + session_id = resume_conversation_id + else: + created = await sdk.sessions.create( + bundle, filename="agent.tar.gz", workspace=workspace + ) + session_id = created.id + except ClientOmnigentError as exc: + # Any create/fork/resume rejection here is a server-side answer, not + # a client bug worth a traceback: a wrong base URL that answers + # /health but has no session API, a fork of a session that is gone, + # a permission refusal. Name the URL, since a wrong one is the case + # that looks least like itself, and pass the server's message through. + raise click.ClickException(f"Could not start a session on {base_url}: {exc}") from exc # A separate raw httpx client for the host-runner protocol (the daemon # launch helpers operate on httpx, not the SDK). diff --git a/omnigent/cli.py b/omnigent/cli.py index 5c297322f4..c60031ad20 100644 --- a/omnigent/cli.py +++ b/omnigent/cli.py @@ -10381,9 +10381,13 @@ def _resolve_server_url(server: str) -> str: :returns: The normalized API base URL without a trailing slash, e.g. ``"https://example.cloud.databricks.com/api/2.0/omnigent"``. """ - from omnigent.conversation_browser import display_server_url + from omnigent.conversation_browser import display_server_url, strip_conversation_path - normalized = _with_default_scheme(server.rstrip("/")) + # A URL copied from the browser while a conversation is open carries the + # SPA's ``/c/`` route. The SPA catch-all answers any GET under it with + # its HTML shell, so it probes as a healthy server and is accepted, then + # every API call 404s. Trim it back to the base before anything probes it. + normalized = _with_default_scheme(strip_conversation_path(server.rstrip("/"))) expanded = _workspace_api_server_url(normalized) candidate = _canonical_azure_databricks_url(normalized) if candidate is None: diff --git a/omnigent/conversation_browser.py b/omnigent/conversation_browser.py index 634b92ea4c..c504476dd6 100644 --- a/omnigent/conversation_browser.py +++ b/omnigent/conversation_browser.py @@ -2,6 +2,7 @@ from __future__ import annotations +import re import subprocess import sys import urllib.parse @@ -15,6 +16,38 @@ WORKSPACE_API_PATH = "/api/2.0/omnigent" WORKSPACE_UI_PATH = "/omnigent" +# Client-side SPA route for one conversation (see web/src/App.tsx's +# ``c/:conversationId``). ``conversation_url`` appends it; ``strip_conversation_path`` +# is the inverse, for a URL copied out of the browser's address bar. +_CONVERSATION_PATH_RE = re.compile(r"/c/[^/]+/?$") + + +def strip_conversation_path(url: str) -> str: + """ + Drop a trailing ``/c/`` from a server URL. + + The web UI's address bar shows ``/c/`` for an open + conversation, so that is what a user copies when asked for "the + omnigent URL". It is a client-side route, not a server mount: the SPA + catch-all answers ``GET /c//v1/me`` with a ``200`` HTML shell, + so such a URL passes an auth probe and is accepted as a server, then + every real API call 404s because no router owns that prefix. Trimming + the route recovers the base the API actually lives on. + + :param url: A server URL, possibly a copied conversation link, e.g. + ``"https://app.databricksapps.com/c/9bed9ec6"``. + :returns: The URL without the conversation route, e.g. + ``"https://app.databricksapps.com"``. + """ + stripped = url.rstrip("/") + parsed = urllib.parse.urlsplit(stripped) + trimmed = _CONVERSATION_PATH_RE.sub("", parsed.path) + if trimmed == parsed.path: + return stripped + return urllib.parse.urlunsplit( + (parsed.scheme, parsed.netloc, trimmed, parsed.query, parsed.fragment) + ) + def is_workspace_hosted_url(base_url: str) -> bool: """ diff --git a/omnigent/server/app.py b/omnigent/server/app.py index 9444f7b4cd..99ee8a200e 100644 --- a/omnigent/server/app.py +++ b/omnigent/server/app.py @@ -2541,7 +2541,15 @@ async def get_response(self, path: str, scope: Scope) -> Response: try: response = await super().get_response(path, scope) except StarletteHTTPException as exc: - if exc.status_code == 404 and _is_web_ui_api_fallback_path(path): + # StaticFiles only serves GET/HEAD, so it answers every other + # method with 405, which reads as "this endpoint exists, wrong + # method" and sends a client pointed at the wrong base URL + # hunting a server bug instead. Nothing reaching this catch-all + # exists, and a non-GET is never an SPA navigation, so answer + # 404 whatever the path looks like. + if exc.status_code == 405 or ( + exc.status_code == 404 and _is_web_ui_api_fallback_path(path) + ): return JSONResponse( status_code=404, content={ diff --git a/tests/cli/test_chat.py b/tests/cli/test_chat.py index 0c467cc993..c57db5408c 100644 --- a/tests/cli/test_chat.py +++ b/tests/cli/test_chat.py @@ -1444,6 +1444,44 @@ def test_prepare_chat_session_via_daemon_fork_wins_over_resume( assert launch["session_id"] == "conv_forked" +def test_prepare_chat_session_via_daemon_reports_create_failure_as_click_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A failed session create is a ``ClickException`` naming the server URL. + + A base URL that answers ``/health`` but exposes no session API (e.g. one + carrying the workspace web-UI path) fails here. Letting the SDK's + ``OmnigentError`` escape turns that wrong-URL case into a crash-handler + traceback, which hides the one detail that identifies it: the URL. + """ + captured: dict[str, object] = {} + _patch_daemon_launch(monkeypatch, captured) + + async def _boom(_self: object, _bundle: bytes, *, filename: str, workspace: str) -> object: + raise ClientOmnigentError({"detail": "Method Not Allowed"}, 405, "") + + monkeypatch.setattr(_FakeSessionsApi, "create", _boom) + + with pytest.raises(click.ClickException) as excinfo: + asyncio.run( + _prepare_chat_session_via_daemon( + base_url="https://example.databricks.com/omnigent", + headers={}, + auth=None, + host_id="host_x", + bundle=b"bundle-bytes", + resume_conversation_id=None, + fork_session_id=None, + workspace="/tmp/proj", + ) + ) + + # The URL is what tells the user their server target is wrong. + assert "https://example.databricks.com/omnigent" in str(excinfo.value) + # No runner is launched for a session that was never created. + assert "launch" not in captured + + # ── OMNIGENT_MODEL env-var fallback ─────────────────── # # These tests pin explicit-environment and discovered-default precedence on diff --git a/tests/server/integration/test_app.py b/tests/server/integration/test_app.py index 786d208d8e..725225fc58 100644 --- a/tests/server/integration/test_app.py +++ b/tests/server/integration/test_app.py @@ -436,3 +436,65 @@ async def test_web_ui_serves_pwa_service_worker_and_manifest( # is no-cache for the same reason as sw.js — a stale sentinel must not linger. assert version.status_code == 200 assert version.headers["cache-control"] == app_module._WEB_UI_HTML_CACHE_CONTROL + + +async def test_unmatched_api_path_404s_for_every_method( + runtime_init: None, + db_uri: str, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """ + An unmatched ``/v1`` path 404s regardless of HTTP method. + + The web UI is mounted at ``/`` and sees every unmatched request. + Starlette's ``StaticFiles`` answers any non-GET with 405, which reads as + "the endpoint exists, wrong method", so a client pointed at the wrong + base URL (e.g. one carrying the workspace web-UI path) sees + ``405 Method Not Allowed`` from ``POST /v1/sessions`` and blames the + server instead of its own URL. + + :param runtime_init: Fixture that initializes the runtime with a mock LLM. + :param db_uri: Test database URI. + :param tmp_path: Pytest temporary directory fixture. + :param monkeypatch: Pytest monkeypatch fixture. + :returns: None. + """ + web_ui_dist = tmp_path / "web-ui" + web_ui_dist.mkdir(parents=True) + (web_ui_dist / "index.html").write_text("
") + monkeypatch.setattr(app_module, "_WEB_UI_DIST", web_ui_dist) + artifact_store = LocalArtifactStore(str(tmp_path / "artifacts")) + app = app_module.create_app( + agent_store=SqlAlchemyAgentStore(db_uri), + file_store=SqlAlchemyFileStore(db_uri), + conversation_store=SqlAlchemyConversationStore(db_uri), + artifact_store=artifact_store, + agent_cache=AgentCache( + artifact_store=artifact_store, + cache_dir=tmp_path / "cache", + ), + ) + transport = httpx.ASGITransport(app=app) + async with httpx.AsyncClient(transport=transport, base_url="http://test") as client: + # The reported crash: a base URL carrying the web-UI path, so the + # bundled-create route never matches. + prefixed = await client.post( + "/omnigent/v1/sessions", + data={"metadata": "{}"}, + files={"bundle": ("agent.tar.gz", b"x", "application/gzip")}, + ) + unmatched_post = await client.post("/v1/nope", json={}) + unmatched_get = await client.get("/v1/nope") + # OPTIONS is covered too. No CORS middleware is installed, so a + # preflight reaching this mount was already a 405 that no browser + # could use; 404 is the more accurate answer, not a lost capability. + unmatched_options = await client.request("OPTIONS", "/v1/nope") + # An extensionless non-API path still gets the SPA shell. + spa = await client.get("/c/conv_abc123") + + for resp in (prefixed, unmatched_post, unmatched_get, unmatched_options): + assert resp.status_code == 404, resp.text + assert resp.json()["error"]["code"] == "not_found" + assert spa.status_code == 200 + assert "
" in spa.text diff --git a/tests/test_conversation_browser.py b/tests/test_conversation_browser.py index 1df64a0ea7..1520336436 100644 --- a/tests/test_conversation_browser.py +++ b/tests/test_conversation_browser.py @@ -290,3 +290,59 @@ def test_conversation_url_plain_server_unchanged(tmp_path, monkeypatch) -> None: conversation_url("http://127.0.0.1:6767", "conv_abc123") == "http://127.0.0.1:6767/c/conv_abc123" ) + + +@pytest.mark.parametrize( + ("url", "expected"), + [ + # The URL a user copies from the address bar with a conversation open. + ( + "https://app.databricksapps.com/c/9bed9ec6fd244725b60e159dc0052fea", + "https://app.databricksapps.com", + ), + ("https://app.databricksapps.com/c/conv_abc/", "https://app.databricksapps.com"), + ("http://127.0.0.1:6767/c/conv_abc", "http://127.0.0.1:6767"), + # Workspace web-UI mount keeps its prefix; only the route is trimmed. + ("https://ws.databricks.com/omnigent/c/conv_abc", "https://ws.databricks.com/omnigent"), + # Real server bases must survive untouched. + ( + "https://ws.databricks.com/api/2.0/omnigent", + "https://ws.databricks.com/api/2.0/omnigent", + ), + ("http://127.0.0.1:6767", "http://127.0.0.1:6767"), + # A path that merely contains /c/ elsewhere is not a conversation route. + ("https://host.example/c/conv_abc/extra", "https://host.example/c/conv_abc/extra"), + ], +) +def test_strip_conversation_path(url: str, expected: str) -> None: + """A copied conversation link resolves back to the server base. + + The SPA catch-all serves its HTML shell for any GET under ``/c/``, so a + pasted conversation URL answers an auth probe with ``200`` and is accepted + as a server; every later API call then 404s because no router owns that + prefix. Trimming the client-side route is what keeps that URL usable. + + :param url: Input URL. + :param expected: Expected server base. + :returns: None. + """ + assert browser.strip_conversation_path(url) == expected + + +def test_strip_conversation_path_inverts_conversation_url( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """``strip_conversation_path`` undoes what ``conversation_url`` builds. + + These two must stay inverses: the CLI prints a conversation link with one + and has to accept that same link back through the other. If either changes + shape independently, a pasted link silently becomes an unusable server URL. + + :param monkeypatch: Pytest monkeypatch fixture. + :returns: None. + """ + monkeypatch.setattr("omnigent.cli_auth.load_databricks_org_id", lambda _url: None) + base = "https://app.databricksapps.com" + link = browser.conversation_url(base, "conv_abc123") + assert link == f"{base}/c/conv_abc123" + assert browser.strip_conversation_path(link) == base