feat(routetrace): add POST /v1/route/trace dry-run route explanation#247
feat(routetrace): add POST /v1/route/trace dry-run route explanation#247gr3enarr0w wants to merge 6 commits into
Conversation
…erro-labs#238) Adds an authenticated, client-facing dry-run endpoint that explains how the active routing strategy would SELECT a target for a request WITHOUT calling any provider (zero token spend, no circuit perturbation). Unblocks Pi's Ferro router UX and any client that needs a safe "why would this route here?" probe. Endpoint POST /v1/route/trace Body: chat/completions-shaped { model, messages?, stream?, metadata? } Auth: same authed /v1/* group as /v1/models + /v1/chat/completions (Bearer MASTER_KEY / key-store key; inherits auth, audit, OTel, X-Request-ID). Always returns dry_run: true. Response contract (TraceResponse): - strategy, requested_model, resolved_model (after alias resolution) - selected_target_key / selected_model (empty when no target is selectable, mirroring the gateway's "no provider supports model" error path) - candidate_targets[] ordered by the strategy's preference, each with: matched, reason, healthy, circuit_open, supports_model, weight, estimated_cost_usd, p50_latency_ms, has_latency_samples - catalog: { model_found, priced, context_window, mode, status, provider } Selection mirrors the per-strategy Execute() in internal/strategies/: single (first target), fallback (first healthy+supporting), loadbalance (eligible deterministic pick), conditional (rule-matched target, else Targets[0]), cost-optimized (lowest eligible cost via models.Calculate zero-usage probe), least-latency (lowest P50-with-samples; unsampled eligible last). If a strategy's selection rules change, update the matching branch in Trace(). No Pi-specific logic lands here; the tracer only consumes Ferro's existing routing primitives (targets/conditions/aliases, provider SupportsModel, circuit-breaker State, latency tracker P50, models.Calculate). SynapseRouter is not reintroduced as a dependency. Gateway exposure: - CircuitState(name) — MU-safe accessor (never perturbs counters). - LatencyTracker() — returns the latency tracker (nil when least-latency routing is inactive). Tests: - internal/routetrace/trace_test.go: 9 unit tests covering all 5 supported strategy modes + alias resolution + no-selection path + catalog-missing path. - internal/handler/route_trace_test.go: 4 handler tests (fallback selection, unknown model, missing-model 400, malformed-JSON 400) asserting Complete is never called. - test/integration/http/route_trace_test.go: 3 end-to-end tests through the production router asserting auth (401), X-Request-Id header, zero provider calls, and the structured response. Docs: README gets a "Dry-run route trace" example; AGENTS.md endpoint table lists the new route. Closes ferro-labs#238.
|
Preparing review... |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Really nice work on this, and thanks for the thorough tests and docs. Pulled the branch, it builds clean and all three suites pass locally. The package split and the mu-safe accessors are exactly the right shape, and I like that the no-provider-call guarantee is asserted in a test. Two things on selection faithfulness I would like to sort out before we merge, since the endpoint's whole value is matching what the gateway would really do:
Small stuff: estimated_cost_usd comes back as 0.0 rather than null for unpriced models, and the branch is carrying some unrelated local tooling (.superset/, CODEX.md, and the matching .gitignore entries) that should come out so the PR stays focused on the route trace change. None of this touches real routing or spend, it is just about the explain output being trustworthy. Happy to merge once the cost/latency selection lines up. Thanks again for unblocking the Pi router surface. |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an authenticated ChangesDry-run route trace endpoint
Local development ignore rules
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant RouteTrace
participant Gateway
participant Trace
Client->>Router: POST /v1/route/trace
Router->>RouteTrace: authenticate and dispatch
RouteTrace->>Gateway: snapshot routing state
Gateway-->>RouteTrace: routing inputs and lookup hooks
RouteTrace->>Trace: Trace(cfg, requestedModel)
Trace-->>RouteTrace: TraceResponse with candidates
RouteTrace-->>Client: dry_run JSON response
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.superset/scripts/context-gen.sh (1)
1-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing workspace
cd, inconsistent with sibling scripts.
run.sh,setup.sh, andteardown.shallcd "${SUPERSET_WORKSPACE_PATH:-$PWD}"first; this script doesn't, so a standalone invocation from a different CWD writes context files and reads git state for the wrong directory.🔧 Proposed fix
#!/usr/bin/env bash set -euo pipefail +cd "${SUPERSET_WORKSPACE_PATH:-$PWD}" + mkdir -p .superset/context🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.superset/scripts/context-gen.sh around lines 1 - 17, The workspace context script is reading and writing relative to the caller’s current directory instead of the configured workspace path. Update context-gen.sh to cd into "${SUPERSET_WORKSPACE_PATH:-$PWD}" at the start, matching run.sh, setup.sh, and teardown.sh, so the git status and generated current-status.txt are always based on the intended workspace. Refer to the script’s top-level setup before the mkdir/git status block when applying the fix.
♻️ Duplicate comments (1)
.superset/ports.json (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnrelated tooling scope already flagged.
This entire
.superset/addition (and matching.gitignoreentries) was already called out in review as unrelated local tooling that should be removed to keep the PR focused on the route-trace endpoint.Separately: this static port map isn't read by any script here —
run.sh/teardown.shsource.superset/runtime/ports.env, andsetup.shgenerates that file dynamically from a workspace-name hash rather than this file. If kept, this file appears dead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.superset/ports.json around lines 1 - 6, Remove the unrelated .superset port map from the change set and keep the PR focused on the route-trace endpoint. The .superset/ports.json file appears unused because run.sh, teardown.sh, and setup.sh rely on .superset/runtime/ports.env generated at runtime, so delete this static mapping (and any matching .gitignore entries) unless you also update the runtime scripts to consume it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.superset/teardown.sh:
- Around line 12-16: The teardown logic in the PID handling block uses an
unquoted $pids expansion in the kill path, which is fragile even though multiple
PIDs must be passed separately. Update the teardown.sh logic around the
lsof/kill sequence to handle multiple PIDs explicitly and robustly, preferably
by piping the PID list through xargs before invoking kill, while preserving the
existing TERM signal behavior and the conditional check around pids.
In `@internal/handler/route_trace.go`:
- Line 44: The request body in the trace route handler is decoded without any
size limit, so cap it before calling the JSON decoder. Update the handler that
uses json.NewDecoder(r.Body).Decode(&req) to read through a bounded body wrapper
(for example, the same pattern used by ChatCompletions or other request handlers
if present) and keep the limit consistent across handlers. Ensure the fix is
applied in the route handling path that parses the request into req, not just in
the decoder call itself.
In `@internal/routetrace/trace_test.go`:
- Line 256: The LatencyHasSamps stub in the route trace test triggers the
unused-parameter lint because the callback does not reference its name argument.
Update the anonymous function in the test setup to avoid an unused parameter,
either by removing the parameter entirely if the signature allows it or by
naming it with the blank identifier, and keep the change localized around the
LatencyHasSamps field in trace_test.go.
In `@internal/routetrace/trace.go`:
- Around line 29-36: Add a doc comment for the exported StrategyMode constants
block in trace.go so static analysis no longer flags ModeSingle, ModeFallback,
ModeLoadBalance, ModeConditional, ModeLatency, and ModeCostOptimized as
undocumented. Place a single comment above the const block describing that these
constants define the supported routing strategy modes, and keep the existing
names unchanged.
- Around line 332-359: The bare-model fallback in catalogMatch is
nondeterministic because it returns the first matching map entry from iteration,
so Provider/Priced/Status can vary across runs. Update the shared lookup path in
models.Catalog.Get or adjust catalogMatch to avoid ranged map tie-breaking by
requiring provider/model keys for ambiguous ids, or by using a deterministic
selection rule when multiple entries share the same ModelID. Use catalogMatch
and models.Catalog.Get as the main symbols to locate the lookup logic.
- Around line 264-278: The CandidateTarget reordering in the mode switch for
RouteTrace is incorrect for ModeSingle because runtime routing always uses the
first declared target, even when later targets are healthy. Update the logic in
the function handling mode-specific candidate ordering so that ModeSingle
preserves the original declared order and only ModeConditional applies the
matched-target-to-front behavior based on conditionalMatchKey, keeping the trace
aligned with actual routing.
---
Outside diff comments:
In @.superset/scripts/context-gen.sh:
- Around line 1-17: The workspace context script is reading and writing relative
to the caller’s current directory instead of the configured workspace path.
Update context-gen.sh to cd into "${SUPERSET_WORKSPACE_PATH:-$PWD}" at the
start, matching run.sh, setup.sh, and teardown.sh, so the git status and
generated current-status.txt are always based on the intended workspace. Refer
to the script’s top-level setup before the mkdir/git status block when applying
the fix.
---
Duplicate comments:
In @.superset/ports.json:
- Around line 1-6: Remove the unrelated .superset port map from the change set
and keep the PR focused on the route-trace endpoint. The .superset/ports.json
file appears unused because run.sh, teardown.sh, and setup.sh rely on
.superset/runtime/ports.env generated at runtime, so delete this static mapping
(and any matching .gitignore entries) unless you also update the runtime scripts
to consume it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 645ba94a-39ab-435b-bb22-5a9ef4b7b896
📒 Files selected for processing (16)
.gitignore.superset/ports.json.superset/run.sh.superset/scripts/context-gen.sh.superset/setup.sh.superset/teardown.shAGENTS.mdCODEX.mdREADME.mdgateway.gointernal/handler/route_trace.gointernal/handler/route_trace_test.gointernal/httpserver/router.gointernal/routetrace/trace.gointernal/routetrace/trace_test.gotest/integration/http/route_trace_test.go
| pids="$(lsof -tiTCP:"$port" -sTCP:LISTEN 2>/dev/null || true)" | ||
| if [ -n "$pids" ]; then | ||
| kill -TERM $pids 2>/dev/null || true | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Shellcheck SC2086: unquoted $pids expansion.
Intentional here (multiple PIDs need to split into separate args), but relying on unquoted word-splitting is fragile. Consider xargs for clarity and robustness.
🔧 Proposed fix
- pids="$(lsof -tiTCP:"$port" -sTCP:LISTEN 2>/dev/null || true)"
- if [ -n "$pids" ]; then
- kill -TERM $pids 2>/dev/null || true
- fi
+ pids="$(lsof -tiTCP:"$port" -sTCP:LISTEN 2>/dev/null || true)"
+ if [ -n "$pids" ]; then
+ printf '%s\n' "$pids" | xargs -r kill -TERM 2>/dev/null || true
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pids="$(lsof -tiTCP:"$port" -sTCP:LISTEN 2>/dev/null || true)" | |
| if [ -n "$pids" ]; then | |
| kill -TERM $pids 2>/dev/null || true | |
| fi | |
| fi | |
| pids="$(lsof -tiTCP:"$port" -sTCP:LISTEN 2>/dev/null || true)" | |
| if [ -n "$pids" ]; then | |
| printf '%s\n' "$pids" | xargs -r kill -TERM 2>/dev/null || true | |
| fi | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 14-14: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.superset/teardown.sh around lines 12 - 16, The teardown logic in the PID
handling block uses an unquoted $pids expansion in the kill path, which is
fragile even though multiple PIDs must be passed separately. Update the
teardown.sh logic around the lsof/kill sequence to handle multiple PIDs
explicitly and robustly, preferably by piping the PID list through xargs before
invoking kill, while preserving the existing TERM signal behavior and the
conditional check around pids.
Source: Linters/SAST tools
| func RouteTrace(gw *aigateway.Gateway) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| var req routeTraceRequest | ||
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Consider capping request body size.
json.NewDecoder(r.Body).Decode(...) has no size limit, letting a client send an arbitrarily large body to this authenticated endpoint before the JSON parser rejects it.
🛡️ Proposed fix
+ r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1MiB cap
var req routeTraceRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {Please confirm whether other handlers in this codebase (e.g. ChatCompletions) already apply a similar cap, so this stays consistent.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { | |
| r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1MiB cap | |
| if err := json.NewDecoder(r.Body).Decode(&req); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/route_trace.go` at line 44, The request body in the trace
route handler is decoded without any size limit, so cap it before calling the
JSON decoder. Update the handler that uses json.NewDecoder(r.Body).Decode(&req)
to read through a bounded body wrapper (for example, the same pattern used by
ChatCompletions or other request handlers if present) and keep the limit
consistent across handlers. Ensure the fix is applied in the route handling path
that parses the request into req, not just in the decoder call itself.
|
@gr3enarr0w let me know when you able to fix those issue so i can plan for release. |
…lookup, lint - orderCandidates no longer reorders ModeSingle candidates: runtime single-mode routing always uses Targets[0] regardless of a later target's health, so reordering made the trace disagree with actual routing. - catalogMatch's bare-model-id fallback now picks deterministically (lowest catalog key) instead of relying on Go's randomized map iteration order. - Add doc comment on the exported StrategyMode const block and rename an unused test closure parameter to satisfy revive. - Drop .superset/ and CODEX.md local tooling scaffold from the diff (picked up from an unrelated local dev-tooling daemon, not part of this feature).
|
Pushed fixes for lint and the CodeRabbit findings:
Full test suite (including race) passes. Let me know if you'd rather see the catalog nondeterminism fixed at the |
…ace handler Use httptest.NewRequestWithContext instead of httptest.NewRequest in route_trace_test.go, and preallocate the conditions slice with its known final capacity in route_trace.go. Pure lint fixes, no behavior change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Line 73: Update the CODEX.md entry in the root ignore configuration to
/CODEX.md so only the repository-root local artifact is ignored, while nested
CODEX.md documentation remains trackable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff12e2db-57ff-43a1-b04d-53c20e874708
📒 Files selected for processing (5)
.gitignoreinternal/handler/route_trace.gointernal/handler/route_trace_test.gointernal/routetrace/trace.gointernal/routetrace/trace_test.go
Unanchored CODEX.md in .gitignore would ignore any file with that name anywhere in the tree, not just the repo-root dev-tooling scaffold file it's meant for. Root-anchor it to /CODEX.md so a legitimate nested CODEX.md could still be tracked.
2 participants
Summary
Implements #238. Adds an authenticated, client-facing dry-run endpoint that explains how the active routing strategy would SELECT a target for a request WITHOUT calling any provider — zero token spend, no circuit perturbation. Unblocks Pi's Ferro router UX and any client that needs a safe "why would this route here?" probe.
Response contract
{ "strategy": "conditional", "requested_model": "qwen3.5:397b-cloud", "resolved_model": "qwen3.5:397b-cloud", "selected_target_key": "ollama-cloud", "selected_model": "qwen3.5:397b-cloud", "candidate_targets": [ { "target_key": "ollama-cloud", "matched": true, "reason": "model condition matched", "healthy": true, "circuit_open": false, "supports_model": true, "weight": 1.0, "estimated_cost_usd": 0.0, "p50_latency_ms": 0, "has_latency_samples": false } ], "catalog": { "model_found": true, "priced": false, "context_window": 128000, "mode": "chat", "status": "ga" }, "trace_id": "...", "dry_run": true }selected_target_key/selected_modelare empty when no target is selectable (mirrors the gateway's "no provider supports model" error path).candidate_targets[]is ordered by the strategy's preference; each carriesmatched,reason,healthy,circuit_open,supports_model,weight,estimated_cost_usd,p50_latency_ms,has_latency_samples.Selection faithfulness
The tracer reproduces each strategy's
Execute()selection rule frominternal/strategies/:Targets[0]is a candidate)Targets[0]fallbackmodels.Calculatezero-usage probe — same primitive as the live strategy)If a strategy's selection rules change, the matching branch in
routetrace.Trace()should be updated to keep the dry-run faithful (called out in the package doc).Non-goals enforced (per #238)
SupportsModel, circuit-breakerState, latency trackerP50,models.Calculate.Completeis never invoked; circuit counters are never incremented (verified by integration test asserting the stub provider'sCompletecall count stays at 0).Gateway exposure
Two small MU-safe accessors added to
*Gateway:CircuitState(name) circuitbreaker.State— returnsStateClosedwhen no breaker is configured; never perturbs counters.LatencyTracker() *latency.Tracker— returns the tracker (nil when least-latency routing is inactive).Tests
internal/routetrace/trace_test.go(9 unit)internal/handler/route_trace_test.go(4 handler)Completeis never calledtest/integration/http/route_trace_test.go(3 e2e)X-Request-Idheader emitted, zero provider calls, structured response shapego vet ./...clean;golangci-lintnot run locally (not installed) — CI will gate.Docs
curl+ response description.POST /v1/route/trace.Closes #238.
Summary by CodeRabbit
New Features
POST /v1/route/tracedry-run endpoint that previews which model/target would be selected—without contacting providers or consuming quota.Documentation
Tests