feat(proxy): annotate GET /v1/models behind ?debug

rcsheets commented

2026-07-28 07:08:42 +00:00

Owner

The plain catalog is ids and nothing else, which is right for a client and
useless when that client is being told its model does not exist. The mapping
exists in the startup log and in slp_upstream_model_info, neither of which is
at hand mid-incident.

?debug answers from the same route table the plain catalog is built from --
no backend is touched -- adding the upstream model name, where that name came
from (discovered / pending / pinned / passthrough), the last probe outcome,
and the route's timeout, retry, and aggregate settings.

The upstream URL is deliberately not among them. /v1/models is
unauthenticated, and naming backend hosts there would undo the reticence
clientErr already keeps on the error path, where a dial failure tells the log
the host, port, and resolved IP and tells the client "upstream request
failed". Probe failures are the same: last_probe says "error", the log says
which address refused.

Plain and debug entries are separate types over one envelope, so the
unparameterized response cannot drift. Route grows a lastProbe written by the
supervisor beside the existing metric; nothing on the request path reads it.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

The plain catalog is ids and nothing else, which is right for a client and useless when that client is being told its model does not exist. The mapping exists in the startup log and in slp_upstream_model_info, neither of which is at hand mid-incident. ?debug answers from the same route table the plain catalog is built from -- no backend is touched -- adding the upstream model name, where that name came from (discovered / pending / pinned / passthrough), the last probe outcome, and the route's timeout, retry, and aggregate settings. The upstream URL is deliberately not among them. /v1/models is unauthenticated, and naming backend hosts there would undo the reticence clientErr already keeps on the error path, where a dial failure tells the log the host, port, and resolved IP and tells the client "upstream request failed". Probe failures are the same: last_probe says "error", the log says which address refused. Plain and debug entries are separate types over one envelope, so the unparameterized response cannot drift. Route grows a lastProbe written by the supervisor beside the existing metric; nothing on the request path reads it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

rcsheets added 1 commit

2026-07-28 07:08:42 +00:00

feat(proxy): annotate GET /v1/models behind ?debug

ci / check (pull_request) Successful in 48s

Details

5f8ccceb8d

The plain catalog is ids and nothing else, which is right for a client and
useless when that client is being told its model does not exist. The mapping
exists in the startup log and in slp_upstream_model_info, neither of which is
at hand mid-incident.

?debug answers from the same route table the plain catalog is built from --
no backend is touched -- adding the upstream model name, where that name came
from (discovered / pending / pinned / passthrough), the last probe outcome,
and the route's timeout, retry, and aggregate settings.

The upstream URL is deliberately not among them. /v1/models is
unauthenticated, and naming backend hosts there would undo the reticence
clientErr already keeps on the error path, where a dial failure tells the log
the host, port, and resolved IP and tells the client "upstream request
failed". Probe failures are the same: last_probe says "error", the log says
which address refused.

Plain and debug entries are separate types over one envelope, so the
unparameterized response cannot drift. Route grows a lastProbe written by the
supervisor beside the existing metric; nothing on the request path reads it.

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

pr-reviewer-bot commented

2026-07-28 07:08:43 +00:00

Collaborator

Automated review by pr-reviewer v0.42.1 | Safety Check | Claude Sonnet 5 | tracking id r-68557b-f31621
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

⚠️ The primary review (Mistral Small) failed. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: LGTM — The PR adds a well-tested, additive debug view to /v1/models that explicitly avoids leaking upstream host/URL info, and nothing in the diff looks alarming.

Reviewed the diff for the ?debug annotation feature on GET /v1/models.

The change is additive and well-scoped: plain catalog path is untouched in shape (separate model/debugModel types over a generic catalog[T] envelope), so there's no risk of the unparameterized response drifting.
debugRequested correctly treats bare ?debug, ?debug=1, ?debug=yes as true and only explicit false/0 as false — matches the documented behavior and is tested.
Upstream URLs/hosts are deliberately excluded from the debug payload, consistent with the stated security rationale (unauthenticated endpoint), and there's a test (TestServeModelsDebugDoesNotLeakUpstreamURLs) verifying no host strings leak into the response. This is the one place I'd worry about a security regression, and it looks handled correctly.
recordProbe fans out to both the metrics recorder and the new route.NoteProbe, done atomically via atomic.Pointer[probeState], avoiding races since the route field is read concurrently by requests.
Route struct's new lastProbe field is documented as purely observational (not read on the request path), consistent with the PR's stated design goal, and I don't see any place where it influences routing decisions.
Tests cover plain-catalog shape, debug annotation fields, absence of last_probe_at for unprobed routes, and the debug flag parsing — reasonable coverage for the new logic.
No secrets, credentials, or large binaries in the diff; nothing looks like leftover debug/test code accidentally committed.

Nothing here looks like it would cause incorrect behavior or a security issue; the feature does what it claims and takes care not to leak upstream host info through the newly-added unauthenticated debug view.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.42.1 | Safety Check | Claude Sonnet 5 | tracking id `r-68557b-f31621`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: LGTM** — The PR adds a well-tested, additive debug view to /v1/models that explicitly avoids leaking upstream host/URL info, and nothing in the diff looks alarming. Reviewed the diff for the ?debug annotation feature on GET /v1/models. - The change is additive and well-scoped: plain catalog path is untouched in shape (separate `model`/`debugModel` types over a generic `catalog[T]` envelope), so there's no risk of the unparameterized response drifting. - `debugRequested` correctly treats bare `?debug`, `?debug=1`, `?debug=yes` as true and only explicit `false`/`0` as false — matches the documented behavior and is tested. - Upstream URLs/hosts are deliberately excluded from the debug payload, consistent with the stated security rationale (unauthenticated endpoint), and there's a test (`TestServeModelsDebugDoesNotLeakUpstreamURLs`) verifying no host strings leak into the response. This is the one place I'd worry about a security regression, and it looks handled correctly. - `recordProbe` fans out to both the metrics recorder and the new `route.NoteProbe`, done atomically via `atomic.Pointer[probeState]`, avoiding races since the route field is read concurrently by requests. - Route struct's new `lastProbe` field is documented as purely observational (not read on the request path), consistent with the PR's stated design goal, and I don't see any place where it influences routing decisions. - Tests cover plain-catalog shape, debug annotation fields, absence of `last_probe_at` for unprobed routes, and the debug flag parsing — reasonable coverage for the new logic. - No secrets, credentials, or large binaries in the diff; nothing looks like leftover debug/test code accidentally committed. Nothing here looks like it would cause incorrect behavior or a security issue; the feature does what it claims and takes care not to leak upstream host info through the newly-added unauthenticated debug view.

rcsheets merged commit 187b7b2fae into main

2026-07-28 07:12:32 +00:00

rcsheets deleted branch feat/models-debug

2026-07-28 07:12:32 +00:00

rcsheets referenced this pull request from a commit

2026-07-28 07:12:34 +00:00

Merge pull request 'feat(proxy): annotate GET /v1/models behind ?debug' (#14) from feat/models-debug into main

Rows
Columns

feat(proxy): annotate GET /v1/models behind ?debug #14