fix(cli): accept a copied conversation URL as a server, and stop the SPA mislabeling missing API routes by dhruv0811 · Pull Request #4374 · omnigent-ai/omnigent

dhruv0811 · 2026-08-07T19:31:02Z

Related issue

Summary

A bare omni crashed at session-create with OmnigentError: {'detail': 'Method Not Allowed'}, on a machine that was never pointed at a remote by hand. Three separate defects stack up:

A copied conversation URL was stored as the server. The config held https://<host>/c/9bed9ec6... — /c/<id> is the web UI's client-side chat route (web/src/App.tsx), i.e. what's in the address bar with a conversation open, so it's what a user pastes when asked for their omnigent URL. omnigent login stored it verbatim, and bare omni (which rewrites to run, which falls back to the config server key) then addressed every API call under that route, where no router matches.
Nothing caught the bad URL earlier, because the SPA answers any GET with its shell. The web UI is mounted at / and receives every unmatched request, so GET <base>/c/<id>/v1/me returns 200 text/html. The login probe reads that 200 as "header-auth mode, no login needed" and persists the URL; /health passes too. The first request needing a real route is the session create.
That failure then reported the wrong thing. StaticFiles serves only GET/HEAD and raises 405 otherwise. Its body is byte-identical to FastAPI's path-matched-wrong-method response, so POST /v1/sessions → 405 Method Not Allowed reads as "this endpoint exists, wrong verb" and points at server routing rather than at the URL. (POST /v1/sessions is registered unconditionally; a healthy server 400s a bogus bundle.)

Each fix sits at the single chokepoint for its defect:

strip_conversation_path (the inverse of the existing conversation_url) is applied in _resolve_server_url, which every entry point — including _ensure_backend — already normalizes through. An already-stored bad link is therefore repaired on the next run, with no hand-edited config.
The SPA catch-all answers 404, not 405, for anything that reaches it: nothing there exists, and a non-GET is never an SPA navigation.
A failed session create raises ClickException naming the URL — which the function's docstring already promised. The raw client error was reaching the crash handler as a traceback.

before:  POST <host>/c/<id>/v1/sessions  ->  405 {"detail":"Method Not Allowed"}   (traceback)
after:   POST <host>/v1/sessions         ->  route matches; the /c/<id> route is trimmed
         a genuinely wrong URL           ->  404 not_found  ->  "Could not start a session on <url>"

ELI5 + request flow

You copied the link to a chat out of your browser and gave it to the CLI as the address of the server. Those look the same but aren't: the chat link points at a page inside the app, not the app's front door. The server's web UI is friendly enough to answer "hello" at any address, so nothing noticed until the CLI tried to actually do something. Then the error message said "wrong knock" instead of "wrong door".

flowchart TD
    A["bare omni (TTY)"] --> B["rewritten to: run"]
    B --> C["no --server, so read config server key"]
    C --> D["https://host/c/9bed9ec6..."]
    D --> E{"POST base + /v1/sessions"}
    E -->|"path is /c/id/v1/sessions"| F["no router owns this prefix"]
    F --> G["falls through to SPA mount at /"]
    G -->|"before: non-GET"| H["405 Method Not Allowed<br/>(reads as 'endpoint exists')"]
    H --> I["escapes as traceback"]
    G -->|"after"| J["404 not_found"]
    D -.->|"after: trimmed in _resolve_server_url"| K["https://host"]
    K --> L["POST /v1/sessions matches"]

Test Plan

# the three new/changed test files
pytest tests/test_conversation_browser.py tests/cli/test_chat.py \
       tests/server/integration/test_app.py -q          # 150 passed

# full server integration suite, against the final commit
pytest tests/server/integration/ -q                     # 1109 passed, 3 xfailed

New coverage:

test_strip_conversation_path — 7 cases, including the exact URL from the crash report, and asserting real API bases (/api/2.0/omnigent, loopback) are left untouched.
test_strip_conversation_path_inverts_conversation_url — pins the two helpers as inverses, since the CLI prints a link with one and must accept it back through the other.
test_unmatched_api_path_404s_for_every_method — 404 across POST/GET/OPTIONS incl. the reported /omnigent/v1/sessions shape, while /c/<id> still serves the SPA shell.
test_prepare_chat_session_via_daemon_reports_create_failure_as_click_error — ClickException naming the URL, and no runner launched for a session that was never created.

Both server-side and client-side tests fail without the fix (405 vs 404; traceback vs ClickException).

Also verified manually: the stored URL from the crash report now resolves to https://<host>, so POST lands on /v1/sessions; and SPA routes, HEAD, real assets, and missing assets are all unchanged — only non-GET-on-unmatched flips 405→404.

CI's Pytest (server-integration) job is green on the final commit. Its first attempt
failed on test_on_runner_disconnect_spares_idle_sessions_and_labels_interrupted_ones,
which is unrelated to this diff and flaky under sharding: it passes locally both with
this change and with these four source files reverted to main, the same job fails on
main itself (414f1f5, with 20 failures including a different test in that same file),
and it passed on re-run here with no code change.

Two notes on this environment, both confirmed pre-existing by stashing the change and re-running:

tests/cli/ reports 96 failures from missing optional deps (json5, respx); the failure set is byte-identical to clean main (96/873 there vs 96/874 here, the +1 being the new test).
The pyrefly pre-commit hook reports 97 errors on a clean tree in this worktree (it resolves site-packages through another checkout). Run directly against the changed files with the right interpreter it reports 0 errors.

Demo

Not a UI change, so no screenshot. The user-visible surface is the CLI's behavior on a
stored bad URL, driven here through the real code paths (_resolve_server_url and the
real _SPAStaticFiles mounted at /), with the exact URL from the crash report:

~/.omnigent/config.yaml:38
  server: https://omnigents-3272836215725701.aws.databricksapps.com/c/9bed9ec6fd244725b60e159dc0052fea

  ^ this is the web UI's /c/<id> chat route, not a server base

==============================================================================
WHY NOTHING CAUGHT IT: the SPA at / answers any GET with its shell
==============================================================================
  GET   /c/9bed9ec6fd244725b60e159dc0052fea/v1/me  -> 200 text/html; charset=utf-8
  GET   /c/9bed9ec6fd244725b60e159dc0052fea        -> 200 text/html; charset=utf-8
  => omnigent login reads 200 as 'header-auth mode' and stores the URL

==============================================================================
BEFORE: first real API call, and what the user saw
==============================================================================
  POST /c/9bed9ec6fd244725b60e159dc0052fea/v1/sessions
  -> 405 {'detail': 'Method Not Allowed'}   (pre-fix behavior)

  $ omni
  Traceback (most recent call last):
    File ".../omnigent/cli.py", line 7740, in run
    File ".../omnigent/chat.py", line 1464, in _prepare_chat_session_via_daemon
    File ".../omnigent_client/_sessions.py", line 399, in create
  omnigent_client._errors.OmnigentError: {'detail': 'Method Not Allowed'}

  ^ 405 reads as "endpoint exists, wrong verb" -> points at the server,
    not at the URL. Nothing names the real problem.

==============================================================================
AFTER (1): the stored URL is repaired at the resolve chokepoint
==============================================================================
  stored   : https://omnigents-3272836215725701.aws.databricksapps.com/c/9bed9ec6fd244725b60e159dc0052fea
  resolved : https://omnigents-3272836215725701.aws.databricksapps.com
  POST to  : https://omnigents-3272836215725701.aws.databricksapps.com/v1/sessions        <- matches the API route

  $ omni
  (starts normally; no config edit needed)

==============================================================================
AFTER (2): a genuinely wrong URL now says so
==============================================================================
  POST /c/9bed9ec6fd244725b60e159dc0052fea/v1/sessions
  -> 404 {'error': {'code': 'not_found', 'message': 'Not found'}}

  $ omni
  Error: Could not start a session on https://<host>/c/9bed9ec6...: Not found

  ^ names the URL, exits clean, no traceback.

_{The $ omni blocks are the CLI-level effect of the HTTP results above them; the

status codes, the resolved URL, and the response bodies are all live output. Reproducing

the traceback verbatim needs the pre-fix build, so that block is quoted from the crash

report in #4303.}

Type of change

Test coverage

Coverage notes

Manual verification covered the parts the tests can't assert directly: that the URL from the crash report resolves to a working API base through _resolve_server_url (the path a real omni takes), and that the 405→404 change doesn't disturb the SPA's own behaviour (client-side routes, HEAD, real vs missing static assets). Automated tests cover the resolver cases, the round-trip invariant, the server response codes, and the client error type.

Not covered: an end-to-end run against a live Databricks Apps deployment. Reproducing the exact deployment shape needs a hosted app, so the SPA catch-all was exercised via its real _SPAStaticFiles class mounted on the real app factory instead.

Changelog

A conversation link copied from your browser now works wherever a server URL is expected, instead of failing later with an opaque "Method Not Allowed" crash

…SPA mislabeling missing API routes A conversation link copied from the browser (`<host>/c/<id>`) is what a user naturally pastes when asked for their omnigent URL, and `omnigent login` stored it verbatim as the default server. `/c/<id>` is a client-side SPA route, so every later API call was addressed under it and matched no router. A bare `omni` then crashed at session-create, on a machine the user never pointed at a remote by hand. Nothing caught the bad URL earlier because the web UI is mounted at `/` and answers any unmatched GET with its HTML shell: `GET <base>/c/<id>/v1/me` returns 200, so the login probe reads it as header-auth mode and persists it, and `/health` passes too. The first request that needs a real route is the session create. That failure then reported `405 Method Not Allowed`, because StaticFiles serves only GET/HEAD and raises 405 for anything else. The body is identical to FastAPI's path-matched-wrong-method response, so the error reads as "this endpoint exists, you used the wrong verb" and points at the server instead of the URL. - Trim the `/c/<id>` route in `_resolve_server_url`, the chokepoint every entry point already normalizes through, so an existing stored link is repaired on the next run rather than needing a hand-edited config. - Answer 404, not 405, for anything reaching the SPA catch-all: nothing that gets there exists, and a non-GET is never an SPA navigation. - Report a failed session create as a ClickException naming the URL, which the function's docstring already promised; the raw client error was reaching the crash handler as a traceback. Signed-off-by: Dhruv Gupta <dhruv.gupta@databricks.com>

omnigent-ci · 2026-08-07T19:33:36Z

Polly AI Review

Missing visual demonstration

This PR changes user-visible CLI behavior: a bare omni that previously crashed with a raw OmnigentError traceback now (a) auto-repairs a stored conversation URL and (b) fails a genuinely-wrong URL with a clean Could not start a session on <url> message. The before/after is described in prose, but a short terminal capture (asciinema/GIF or two screenshots) showing the old traceback vs. the new clean message would let reviewers confirm the fixed flow without reproducing the config state. Please attach one, or mark N/A if you consider the text tables sufficient.

Blocking issues

None. I verified each of the three fixes against the diff and surrounding code:

strip_conversation_path is correctly the inverse of conversation_url (regex /c/[^/]+/?$ anchored to the end; /c/…/extra is left untouched, and the inverse round-trip test pins the contract).
It is applied inside _resolve_server_url before any probing, so both fresh logins and already-stored bad URLs are repaired on the next run.
The server 405→404 change lives only in _SPAStaticFiles.get_response, which handles exceptions raised by StaticFiles itself. Real router-level 405s (method-not-allowed on an existing API route) originate in the router and never reach this catch-all, so no legitimate 405 is being masked.

Security vulnerabilities

None. No new network calls, deserialization, or auth-boundary changes. strip_conversation_path only trims a trailing client-side route from a URL the user already supplied; it does not broaden what host the CLI will talk to (netloc/scheme are preserved). The 405→404 change narrows information disclosure slightly (never a concern here) rather than widening it.

Non-blocking notes

_SPAStaticFiles.get_response now returns 404 for any non-GET/HEAD reaching the catch-all, including OPTIONS. If a CORS preflight ever falls through to this mount (it normally shouldn't — CORS middleware answers first), it would now get a 404 JSON body instead of a 405. Almost certainly harmless, but worth keeping in mind if preflight handling is ever restructured.
The try/except in _prepare_chat_session_via_daemon wraps the fork and resume branches too, so a fork/resume failure also surfaces as "Could not start a session on ". That's accurate enough, though the inline comment frames it purely as the "wrong base URL" case; a fork against a valid server that fails for another reason would inherit the same wording. Minor.
strip_conversation_path returns the original url unchanged (not rstripped) on the no-match path, while the matched path returns a rstripped/reconstructed URL. Callers here already rstrip beforehand, so it's inconsequential, but the asymmetry is a small latent inconsistency.

Summary

A well-scoped, well-reasoned fix: three defects (bad stored URL, SPA masking it as healthy, and 405 mislabeling the failure) each addressed at their single chokepoint, with focused tests including the exact URL from the crash report and an inverse round-trip test tying strip_conversation_path to conversation_url. I found no correctness or security blockers. The only ask is a before/after CLI demonstration for the user-visible crash-to-clean-error change.

_{Automated review by Polly · workflow run}

- Return the rstripped URL on the no-match path too, so both branches of strip_conversation_path normalize a trailing slash identically. - Reword the session-create guard's comment: it covers fork and resume rejections as well, not only a wrong base URL. - Pin the OPTIONS case in the catch-all test. No CORS middleware is installed, so a preflight reaching the SPA mount was already a 405 no browser could use; 404 is more accurate rather than a lost capability. Signed-off-by: Dhruv Gupta <dhruv.gupta@databricks.com>

dhruv0811 · 2026-08-07T20:17:36Z

/review

omnigent-ci · 2026-08-07T20:19:46Z

Polly AI Review

Missing visual demonstration

This PR fixes a user-visible CLI failure: bare omni went from crashing with an OmnigentError traceback to either working (URL trimmed) or surfacing a clean ClickException. The description explains the before/after in prose and a mermaid diagram, but the "Attached images/videos" scan found none. A short terminal capture (asciinema/screenshot) of omni before (traceback) vs. after (clean error naming the URL, or a successful session) would let reviewers confirm the user-facing behavior without reproducing the exact broken config. Please attach one.

Blocking issues

None. I verified the three fixes against the checked-out source:

chat.py — ClientOmnigentError is already imported (as alias, line 38) and the new try/except wraps only the create/fork calls that actually hit the network; the resume_conversation_id branch does no I/O, so nothing is over-caught. The raise ... from exc preserves the chain.
app.py — the 405 branch is inside StaticFiles.get_response, which is only reached for paths no router matched. A matched-path/wrong-method 405 is produced by the router upstream and never reaches this handler, so converting the catch-all 405 → 404 does not mask any legitimate "endpoint exists, wrong verb" response. The branch correctly short-circuits before the extensionless index.html fallback.
conversation_browser.py — _CONVERSATION_PATH_RE anchors on $ and only matches a trailing /c/<single-segment>[/], so real API bases (/api/2.0/omnigent), the workspace mount (/omnigent), and /c/<id>/extra are all left intact, matching the parametrized tests.

Security vulnerabilities

None introduced. The 404-vs-405 change only reshapes error responses for already-unmatched paths and does not widen any surface (WebSocket rejection and API-namespace guarding are unchanged). The ClickException passes the server's message through but adds no secrets. No lockfile or dependency/extras changes are present.

Non-blocking notes

The catch-all now answers 404 for every non-GET/HEAD unmatched request, including OPTIONS. The test correctly notes no CORS middleware is installed today, so no real preflight relied on the old 405 — but this is a latent coupling: if CORS is ever added at a layer below these routers, a preflight reaching this mount would get a 404 instead of a handled preflight. Worth a one-line comment or awareness, not a change now.
strip_conversation_path trims only a single trailing /c/<id> segment. A hypothetical legitimate API base literally ending in /c/<segment> would be mis-trimmed, but that's not a realistic base for this product; acceptable.

Summary

A well-scoped, well-tested three-part bugfix, each fix placed at the correct single chokepoint (URL normalization, SPA catch-all status, CLI error surfacing). I confirmed the claims hold against the current source and the new tests exercise the reported crash URL and method matrix. No blocking correctness or security concerns. The only gap is a missing before/after visual for a user-facing CLI crash fix — please attach one; otherwise this is ready.

_{Automated review by Polly · workflow run}

github-actions · 2026-08-07T20:21:10Z

🏷️ Doc impact: no-doc-update

Internal bugfixes—stripping the SPA /c/<id> route from copied server URLs, surfacing session-create failures as CLI errors, and returning 404 instead of 405 for unmatched non-GET paths—that improve error handling without changing any documented behavior, flag, or default.

_{Auto-classified on merge. Set the label manually before merging to override. · run}

github-actions Bot added P1-high Priority: major feature broken, no workaround size/L Pull request size: L labels Aug 7, 2026

dhruv0811 merged commit ba571a6 into main Aug 7, 2026
97 of 99 checks passed

dhruv0811 deleted the fix/4303-spa-catchall-405-conversation-url branch August 7, 2026 20:20

github-actions Bot added the no-doc-update Merged PR does not need a docs update label Aug 7, 2026

dhruv0811 mentioned this pull request Aug 7, 2026

fix(cli): select local mode for --server local and --server "" #4387

Merged

13 tasks

Navigation Menu

Uh oh!

fix(cli): accept a copied conversation URL as a server, and stop the SPA mislabeling missing API routes - #4374

fix(cli): accept a copied conversation URL as a server, and stop the SPA mislabeling missing API routes#4374
dhruv0811 merged 2 commits into
mainfrom
fix/4303-spa-catchall-405-conversation-url

dhruv0811 commented Aug 7, 2026 •

edited

Loading

Uh oh!

omnigent-ci Bot commented Aug 7, 2026

Uh oh!

dhruv0811 commented Aug 7, 2026

Uh oh!

omnigent-ci Bot commented Aug 7, 2026

Uh oh!

Uh oh!

github-actions Bot commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

dhruv0811 commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issue

Summary

Test Plan

Demo

Type of change

Test coverage

Coverage notes

Changelog

Uh oh!

omnigent-ci Bot commented Aug 7, 2026

Polly AI Review

Missing visual demonstration

Blocking issues

Security vulnerabilities

Non-blocking notes

Summary

Uh oh!

dhruv0811 commented Aug 7, 2026

Uh oh!

omnigent-ci Bot commented Aug 7, 2026

Polly AI Review

Missing visual demonstration

Blocking issues

Security vulnerabilities

Non-blocking notes

Summary

Uh oh!

Uh oh!

github-actions Bot commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

dhruv0811 commented Aug 7, 2026 •

edited

Loading