Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions omnigent/chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
8 changes: 6 additions & 2 deletions omnigent/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -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/<id>`` 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:
Expand Down
33 changes: 33 additions & 0 deletions omnigent/conversation_browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from __future__ import annotations

import re
import subprocess
import sys
import urllib.parse
Expand All @@ -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/<conversation_id>`` from a server URL.

The web UI's address bar shows ``<base>/c/<id>`` 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 <base>/c/<id>/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:
"""
Expand Down
10 changes: 9 additions & 1 deletion omnigent/server/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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={
Expand Down
38 changes: 38 additions & 0 deletions tests/cli/test_chat.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
62 changes: 62 additions & 0 deletions tests/server/integration/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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("<!doctype html><div id='root'></div>")
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 "<div id='root'>" in spa.text
56 changes: 56 additions & 0 deletions tests/test_conversation_browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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/<id>``, 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
Loading