[Router] fix recipe-blind routing reads and pin every read scope - #2724
Closed
drivebyer wants to merge 10 commits into
Closed
[Router] fix recipe-blind routing reads and pin every read scope#2724drivebyer wants to merge 10 commits into
drivebyer wants to merge 10 commits into
Conversation
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
assigned
AayushSaini101,
drivebyer,
FAUST-BENCHOU,
ramkrishs,
shraderdm and
WUKUNTAI-0211
Contributor
👥 vLLM Semantic Team NotificationThe following members have been identified for the changed files in this PR and have been automatically assigned when their GitHub accounts are assignable in this repository: 📁
|
Contributor
✅ Supply Chain Security Report — All Clear
Scanned at |
Contributor
Performance Benchmark ResultsComponent benchmarks completed with no regressions beyond thresholds. Summary
DetailsSee attached benchmark artifacts for detailed results and profiles. Performance testing powered by vLLM Semantic Router |
Part of the #2331 recipe work: the flat IntelligentRouting decisions field holds only the default routing profile, but its name suggested the whole routing surface. Rename it so default-profile reads are visible at every call site and the field carries a distinct literal that read-scope tooling can audit. Pure rename; no behavior change. The yaml and json tags keep the serialized names, so the config file format and the /info/classifier response are byte-for-byte unchanged. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
…on and lookup sites DefaultDecisions carries only the default recipe, so every site that reasons about routing as a whole silently ignored named recipes. Migrate those sites to AllRoutingDecisions()/GetDecisionByName() and leave the request-path default-profile sites alone. Four reads were measurably wrong for a decision living in a named recipe: - the classifier could not resolve its models or system prompt - its reasoning_effort was dropped, falling back to the global default - its memory plugin never enabled the memory runtime - its Router Replay recorder was never registered The memory pair was the worst shape: validator_memory already validated a recipe's memory plugin, while isMemoryEnabled never enabled it, so the config passed validation and then did nothing. Five contract validators (domain, projection, complexity, modality, learning) also only walked the default profile, leaving recipe decisions' signal and projection references entirely unchecked. Named recipes are the unreleased #2612/#2613 surface, so nothing changes for any released configuration; single-profile configs are bit-for-bit unaffected because their whole surface IS the default profile. Algorithm virtual slugs deliberately stay default-profile-only. They select decisions without consulting the entrypoint table, so widening them would let a recipe's decisions be selected without going through that recipe's entrypoint. HasXDecision() stays default-scoped for the same reason: it gates whether /v1/models advertises the slug, and advertising one that cannot route would be a lie. The by-name lookups are allocation free: getReasoningEffort runs several times per request, so it resolves through GetDecisionByName rather than materializing a merged decision slice. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
The rename made default-profile reads visible, but nothing stops a new production site from picking that scope by accident: before the read-site fixes, four validators read the whole surface while five read the default profile, with no marker distinguishing them. Pin the complete set of production files that may reference DefaultDecisions, one comment per entry saying why the default-profile scope is correct there. A new site fails the test until it either switches to a whole-surface accessor or documents its scope here, which puts the choice in front of the reviewer instead of leaving it silent. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
…lt profile Three of the read-site widenings overshot. Narrow them back: - findDecisionScopedSelectionConfigs feeds first-match algorithm configs (elo, router_dc, rl_driven, gmtrouter, multi_factor) into the process-level selector singleton. Reading every recipe made that global config claimable by a named recipe and dependent on YAML declaration order: recipes-only layouts preserve the declared order, so swapping two recipe blocks changed routing behavior. A singleton can have one config source; that source is the default profile. - The entropy and MCP reasoning maps and the keyword-category lookup serve only the classify API, whose decision engine evaluates the default profile. Widening them let a named recipe's use_reasoning leak into metrics and logs for requests that can never route to that recipe's decisions. One API, one scope. - getRecommendedModel could recommend a model reachable only through a named recipe's entrypoint while the same response's decision_result came from the default profile; under a recipes-only layout every classify call took that path. The recommendation now reads the default profile directly, keeping the response internally coherent. The extproc request path is untouched: per-request lookups resolve the already-selected decision by its globally unique name and stay recipe-wide. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
… profile candidate_set: tier unions the model refs of every decision sharing the matched decision's tier. Reading the whole routing surface let a tier match pull in models declared only by another recipe's decisions, and a sampling win then rewrote the route to a model the request's entrypoint never authorized. The default-profile baseline had the mirrored problem (default models leaking into recipe-scoped requests) plus a worse failure mode: with the base model absent from the candidate set, the score-margin fallback rewrote every request unconditionally. Tier candidates now follow the request's decision scope, mirroring decisionCandidatesForRequest: the entrypoint recipe when one is scoped, the default profile otherwise. candidate_set: global keeps the whole surface — it is the deployed model inventory by contract. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
findDecisionBy's contract says the first match is the only match, but the load-time uniqueness check compared names exactly while GetDecisionByNameFold matches case-insensitively: two profiles could declare names differing only by case and silently alias each other in every fold lookup. Fold the cross-profile check; the within-profile exact rule is unchanged, so no accepted single-profile config is affected (all shipped decision names are lower-case snake_case). Signed-off-by: drivebyer <wuyangmuc@gmail.com>
isMemoryEnabled, shouldInitMemoryStore and memoryConfigured were three byte-identical copies of the same predicate in three packages — the read-site fix had to repair all three, and a fourth consumer would have copied whichever it found first. One IsMemoryEnabled accessor now lives in config next to IsHallucinationModelEnabled, the same shape for the same layer; MemoryPluginDecisionName keeps the extproc auto-enable log naming the triggering decision. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
…ath cleanups - CountRoutingDecisions joins HasRoutingDecisions as the allocation-free counting sibling; the six decision_count sites (parse/reload logs and the classification metrics endpoint) no longer materialize the merged slice just to take its length. - reasoningEffortForDecision takes the decision by pointer: the caller sits on the per-request path whose comment advertises the allocation-free lookup, and the value deref copied the 13-field Decision struct on every call. - GetModelsForCategory resolves through findDecision like the rest of the classifier, removing the second spelling of the same lookup. - Three comments still named the pre-rename Decisions field; the field name is the audit signal, so stale spellings send readers grepping for a field that no longer exists. - AllRoutingDecisions documents that element order is unspecified, keeping first-match consumers from re-growing an implicit contract. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
…ixtures - The guard now also matches DefaultRecipe(): rewriting a read site from cfg.DefaultDecisions to cfg.DefaultRecipe().Decisions is semantically identical and would have silenced the guard for free. Extending the spelling set costs zero new allowlist entries today. - Repo-root discovery reuses repoRootFromTestFile like every other repo-walking test in this package, and the sanity probe checks a source-tree marker instead of .git (.dockerignore strips .git/, and a source tarball carries none). The walk skips Rust target/ trees and matches on raw bytes instead of copying each file into a string. - The mirror invariant now asserts the two views share one backing array: DeepEqual alone is satisfied by an equal copy, so a defensive copy in the loader would have passed the very test meant to catch it. - The invariant tests reuse the canonical recipe fixtures instead of carrying near-identical private copies, and the RoutingProfileSignals scope assertion moves next to the existing registry-merge test. One fixture, one invariant, one place to update. - The ValidateEndpoints subtest no longer implies a load-time endpoint check exists; it pins the helper's recipe scope and says so. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
drivebyer
force-pushed
the
refactor/router-recipes-single-source
branch
from
August 1, 2026 16:16
f0f9bcc to
ea16957
Compare
drivebyer
changed the title
…d gap as TD045 The plan file still named the pre-rename Decisions field and its loop state stopped at #2613; the operating rule now points new default-profile reads at the allowlist. The dashboard's recipe blindness (topology panel, setup wizard activation gate, config projection) outlives this PR, so it gets a durable TD entry owned by pl-0038 T12 instead of living only in a test comment. Signed-off-by: drivebyer <wuyangmuc@gmail.com>
drivebyer
force-pushed
the
refactor/router-recipes-single-source
branch
from
August 2, 2026 07:00
ea16957 to
ce13c72
Compare
Collaborator
Author
|
Superseded by #2741, which completes the rest of #2331 from the issue author and redesigns the recipe model along the way (recipe-local decision names, per-recipe classifier/selector scopes). That branch also fixes the read-scope defects this PR targeted — memory-plugin enablement, replay recorder registration, the contract validators — on the new model, so the whole-surface accessors, the cross-recipe name validation, and the allowlist guard built here no longer apply. The defect catalogue in #2723 should now be tracked against #2741. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2723 (part of #2331). Follow-up to #2612 / #2613; the flat-field removal sketched alongside that issue is deferred, see Non-goals.
Purpose
#2612/#2613 added named recipes next to the flat
IntelligentRoutingdecisions field, but left ~15 whole-surface read sites reading only the default profile. The failures were silent: a memory plugin configured on a recipe decision passed validation and never enabled the runtime, five contract validators skipped recipe decisions entirely, and the replay recorder for a recipe decision was never registered. Named recipes are unreleased, so no released configuration is affected — but the reference config already promotes them.This PR is the scoped correction, three layers:
Decisions→DefaultDecisions(mechanical, 106 files). The yaml/json tags keep the config format and the/info/classifierwire key byte-identical; the new name makes every default-profile read visible at its call site.AllRoutingDecisions()/GetDecisionByName(). Sites whose scope really is the default profile stay that way, each with its rationale recorded.A review pass over the branch then tightened the scope decisions themselves:
elo/multi_factor/… configs) and the classify-API reasoning/recommendation paths deliberately stay on the default profile — widening them made the global config dependent on YAML recipe order and let recipes leak into an API that previews default-profile routing.candidate_set: tierlearning candidates now follow the request's recipe scope instead of unioning every profile (which could route past a recipe's entrypoint).config(was copied in three packages), allocation-freeCountRoutingDecisions(), and the mirror invariant asserts slice aliasing rather than tautologicalDeepEqual.No user-visible behavior change on any released surface: single-profile configs are bit-for-bit unaffected, and the only behavior changes sit on the unreleased named-recipe surface.
Non-goals (deferred, tracked)
Test Plan
go test(pkg/cacheexcluded: env-dependent backends), including the cgo-linkedextproc/classification/services/apiserversuites.make agent-lintover all changed files,make check-go-mod-tidy, markdownlint on the docs.recipe_read_sites_test.gopins every corrected read site;default_decisions_allowlist_test.gopins the deliberate default-profile reads (and also matches theDefaultRecipe()spelling);recipes_invariant_test.gopins the flat-field/default-recipe aliasing; a new loader test pins the case-insensitive cross-recipe name check.Test Result
All of the above pass locally; every commit builds and passes lint, so the sequence bisects.
Semantic Router PR Checklist
[Router],[CLI],[Dashboard],[Operator],[Fleet-Sim],[Bindings],[Training],[E2E],[Docs], or[CI/Build]git commit -s