Skip to content

Pass through non-fold endpoints (e.g. GET /v1/models) instead of 404#3

Open
Minsecrus wants to merge 1 commit into
neteroster:mainfrom
Minsecrus:fix/transparent-passthrough
Open

Pass through non-fold endpoints (e.g. GET /v1/models) instead of 404#3
Minsecrus wants to merge 1 commit into
neteroster:mainfrom
Minsecrus:fix/transparent-passthrough

Conversation

@Minsecrus

@Minsecrus Minsecrus commented Jul 7, 2026

Copy link
Copy Markdown

The proxy only registered POST on its listen paths, so anything else fell through to a 404/405. In particular GET /v1/models returned 404, so agents couldn't refresh their model list through the proxy (#1).

Before / after

request before after
GET /v1/models 404 transparent passthrough → <upstream>/models
POST /v1/models 404 passthrough
GET /v1/responses 405 passthrough
POST /v1/responses fold fold (unchanged)

What changed

  • A catch-all /{path:path} route forwards every non-fold request to the matching upstream path (GET /v1/models<upstream_base>/models), reusing the existing _passthrough + build_upstream_headers path and the same credential-leak guard. The fold routes stay POST-only and are listed first, so POST /v1/responses is untouched.
  • open_passthrough / _passthrough gained a method kwarg (default POST). The method was previously hardcoded to POST, which silently turned a GET /v1/models into a POST upstream; the caller's method is now forwarded verbatim.
  • Upstream base is derived by stripping the /responses suffix from the resolved upstream URL; the incoming path is mapped by stripping the matching listen-path root. Honours fixed / header / header_required and preserves the query string.

Tests

Two offline cases added (URL mapping across modes + method forwarding). Full suite: 113/113.

Verified live against an OpenAI-compatible relay through the proxy:

  • GET /v1/models200, returns the upstream model list (previously a proxy-level 404);
  • POST /v1/responses folding behaviour unchanged.

Fixes #1.

Only POST on the listen paths was routed, so GET /v1/models (and
anything else off the fold path) returned 404/405 and agents couldn't
refresh their model list.

Add a catch-all transparent passthrough: GET /v1/models now forwards
to <upstream>/models, etc. The fold routes stay POST-only and listed
first, so POST /v1/responses is unchanged. Reuses the existing
passthrough + credential guard; open_passthrough gains a method kwarg
(default POST) so the caller's method is forwarded verbatim instead of
hardcoded to POST.

Fixes neteroster#1

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@neteroster neteroster left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


[P1] The catch-all route only handles GET/HEAD — POST/PUT/PATCH/DELETE non-fold endpoints still 405.

Nice fix for the GET /v1/models case, and threading method through open_passthrough/_passthrough is the right call. But the catch-all route as written doesn't deliver the "pass through non-fold endpoints" promise for non-GET methods — see the inline comment. Reproduced empirically below on Starlette 1.3.1. Requesting changes: an explicit methods=[...] plus a route-level test.


AI usage disclosure

The final review verdict and the inline review were generated by Claude Code (w/ Claude Opus 4.8), operated by NeterOster. The issue surfaced by the original review was generated by Pi Coding Agent (w/ GPT-5.5).

Comment thread middleware/app.py
# Catch-all transparent passthrough for everything else (GET /v1/models, etc.).
# Fold routes are POST-only and listed first, so they win on full match; any other
# path/method falls through here and is forwarded to the upstream unchanged.
routes.append(Route("/{path:path}", handle_passthrough))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Route(..., methods=None) doesn't match all methods — Starlette defaults it to GET and auto-adds HEAD. So this catch-all only passes through GET/HEAD; every other method hits the path but not the method and returns 405, before handle_passthrough (and the new method=request.method forwarding) ever runs. The PR description's POST /v1/embeddings, DELETE /v1/files/..., etc. are not actually fixed.

Reproduced on Starlette 1.3.1 with this PR's routing shape:

GET      /v1/models       -> 200  PASS GET
POST     /v1/embeddings   -> 405  Method Not Allowed
DELETE   /v1/files/x      -> 405  Method Not Allowed
PUT      /v1/foo          -> 405  Method Not Allowed
PATCH    /v1/foo          -> 405  Method Not Allowed
OPTIONS  /v1/foo          -> 405  Method Not Allowed
GET      /v1/responses    -> 200  PASS GET     (catch-all)
POST     /v1/responses    -> 200  FOLD         (fold route, unchanged)

routes[1].methods prints {'HEAD', 'GET'}, confirming the cause.

Suggested fix — declare the methods explicitly:

routes.append(Route(
    "/{path:path}",
    handle_passthrough,
    methods=["GET", "HEAD", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"],
))

Also worth adding a route-level test (starlette.testclient.TestClient) asserting a non-GET passthrough like POST /v1/embeddings and that POST /v1/responses still folds. The two added tests only cover the pure functions (_resolve_passthrough_url / open_passthrough), which is why this slipped past 113/113 + the live check (the live check only exercised GET /v1/models).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

请求增加对 /v1/models 接口的兼容性支持

2 participants