Router/2385 collection.router recipe dispatch

[SlawomirNowaczyk]

(this builds on #2519 and #2548 so only the final two commits 99b5f30 and 9707009 are meaningful)

Wire up request-time dispatch for collection.router models. Addressing a router collection by name on /chat/completions, /completions, or /responses now runs its routing engine, selects a candidate, and dispatches the request to it — the recipe itself is the trigger (no "auto" model, no separate /v1/route endpoint). Mirrors the existing collection.omni dispatch wiring.

Dispatch

• Server::route_collection_request() runs the collection's routing engine and returns the selected candidate; apply_router_collection_dispatch() rewrites the request's "model" field in place (no-op for non-router models).
• handle_chat_completions runs it inline; handle_completions and handle_responses call apply_router_collection_dispatch() before the usual load/forward path. handle_responses now re-serializes the (possibly rewritten) body so the selection reaches the backend on the streaming path.
• All paths fail open: a missing/unparseable policy or any dispatch error leaves the request untouched and logs a warning.

RouteContext extraction

• New build_route_context() translates a chat/completions, completions, or responses body into a backend-agnostic RouteContext: last user message text (string or typed-parts content), has_tools, has_images, char count, and string metadata.
• Factored shared helpers out of the classifier services: make_chat_request(), collect_text_from_content(), content_has_image().

Policy parsed once

• Router policies are parsed a single time when the models cache is built and stored on ModelInfo as shared_ptr<const RoutePolicy>, so dispatch just reads it instead of reconstructing and re-parsing per request. Null for non-router recipes; parse failures log and fail open.

Tests

• test_021z_router_collection_chat_dispatch: pulls a collection.router, addresses it by name, and asserts the response is a real completion from the engine-selected candidate (not the router alias).

Notes

• The RoutingPolicyEngine is still assembled per request because its ClassifierServices bind the live Router, which only exists on the Server side; engine construction is cheap next to the classifier work route() triggers.

[fl0rianr]

Thanks for pushing this forward — the overall direction looks good, especially parsing the router policy once and wiring the dispatch through the existing request paths.

I think this needs a few changes before it is merge-ready:

1. CI is currently failing in the new test_021z_router_collection_chat_dispatch test. The test registers a collection.router without version, but the parser requires collection.version and only accepts "1". This should be resolved either by making "version": "1" mandatory everywhere, including tests/examples/UI exports, or by defaulting missing versions to "1" server-side. (should not be the current self-hosted-runner issue)

2. Even if the test adds version, the policy seems likely to break after persistence/cache rebuild. register_user_model() only preserves keys from USER_DEFINED_MODEL_PROPS, and version is not currently included there. build_cache() also reconstructs the parser document only from recipe, components, and routing, so the parsed route_policy will still fail because version is missing. We should persist/restore version, or explicitly inject the supported default version before parsing.

3. The current “fail open” path does not actually leave the request untouched. route_collection_request() returns the collection name when no parsed policy exists, but the callers still assign that return value back to request_json["model"]. I think this should return an optional/result type so the model field is only rewritten when routing actually succeeds.

4. /load currently treats collection.router differently from collection.omni: only omni collections are excluded from the normal backend-load path. Since router collections are virtual as well, /load should either load the relevant components, be a no-op with success, or return a clear error — but it should not fall through to router_->load_model() as a normal backend recipe.

5. The PR claims coverage for /chat/completions, /completions, and /responses, but the integration coverage currently only exercises /chat/completions. I’d like to see endpoint coverage for /completions, /responses including streaming, persistence/cache rebuild after /pull, broken/missing routing policy behavior, and /load on a router collection.

Once those are addressed, this should be in much better shape. The architecture is promising, but the current version/persistence/load-path issues make it risky to merge as-is.

[SlawomirNowaczyk]

Thanks for the review — addressed all five:

1. Missing version in the test — the collection.router /pull payload now declares "version": "1"; I kept version explicitly required so the schema stays honest.
2. Version not persisted across cache rebuild — added version to the persisted user-model props and to the doc reconstruction in build_cache(), so the routing policy re-parses correctly after the cache is invalidated.
3. Fail-open still rewrote the model field — route_collection_request now returns std::optional<std::string> and returns std::nullopt when routing doesn't engage, so callers leave the request untouched instead of rewriting the model on fail-open.
4. /load mishandled router collections — /load now treats router collections as virtual: it skips the HF download and acknowledges success without bringing up a backend, since each request lazy-loads a single candidate at dispatch time.
5. Test coverage only exercised /chat/completions — added integration tests for /completions, /responses (streaming and non-streaming), cache-rebuild persistence, registration-time rejection of invalid/missing-version policies, and the /load no-op.

It's worth noting for the future that we should probably rethink how model load limits work with the router: we can expect multiple small models that should remain loaded all the time, which the current LRU eviction doesn't really account for — but that's a question for later, once we see some real usage examples.

[ramkrishna2910]

Merged latest main into this branch and resolved the conflicts (27bc1ca2) — this clears the DIRTY state. The conflicts all came from #2519 (parser/store) and #2548 (classifier services) landing on main after this branch forked from a future_main that carried their pre-merge versions.

How each conflict was resolved (8 files)

File	Resolution
routing_classifier_services.h	kept this branch — it's a superset that adds the build_route_context declaration
routing_classifier_services.cpp	union — took main's reviewed parse/validation body (validate_score_range, try_extract_chat_text, direct-score payloads) and re-added this branch's build_route_context + its collect_text_from_content / content_has_image helpers, which dispatch needs
routing_policy_store.{h,cpp}, test_routing_policy_store.cpp, test_routing_classifier_services.cpp	took main — strict supersets (C++20 atomic<shared_ptr> TODO, case-insensitive .json extension, added out-of-range-score / uppercase-.JSON tests)
model_manager.cpp (USER_DEFINED_MODEL_PROPS)	kept "version" on top of main's "routing" — router policies carry a required root version that must survive registration
server.cpp (download-skip)	is_model_collection_recipe (omni or router) — a collection.router has no checkpoint of its own, so it must skip the generic HF download path; main's is_omni_collection_recipe there would wrongly send router collections through HF

Heads-up: main also adopted the is_collection_recipe → is_omni_collection_recipe rename since you forked, so the merged model_types.h now carries is_omni_collection_recipe / is_router_collection_recipe / is_model_collection_recipe and the old bare name is gone.

Validation

• lemond builds and links (all three conflicted .cpp compile, build_route_context resolves).
• Routing unit tests pass locally: parser, engine, deterministic, classifier-services (9/9), collection-validation.
• The two RoutingPolicyStore watcher-triggered tests fail on my Windows box, but those files are byte-identical to main and the low-level DirectoryWatcherTest passes — pre-existing local fs-watch timing, not from this merge (they're green in CI).

One design note (not blocking, your call)

build_route_context currently lives in the classifier-services module. #2548's reviewers deliberately pulled that request→RouteContext translation out of that layer on main — arguably it belongs in the dispatch layer this PR owns. I kept it where you had it to preserve your design and keep the build green; worth considering relocating it into the dispatch code as part of addressing the existing review.

[fl0rianr]

Thanks for the follow-up and for merging latest main into this branch. I re-reviewed the current head, also in the context of #2519 and #2548.

And I still think this needs one more pass before merge:

1. Router collection JSON round-trip still seems incomplete.
   The parser intentionally requires root "version": "1", and this PR now persists/rebuilds that field internally. However, model_info_to_json() surfaces routing for router collections but does not surface version. Since #2519 introduced the import/export round-trip plumbing for router policies, exporting a router collection from /models/{id} or /models?show_all=true can lose the required schema version and then fail on re-import. Please emit version for collection.router models, probably from info.extras["version"], next to routing, and add a regression test that the exported router collection JSON includes "version": "1" and can be submitted back through /pull.

2. The /responses RouteContext extraction is still under-covered for typed content/images.
   The PR description says typed-parts content and has_images are handled, but the current helpers only read content parts with type == "text", only detect images with type == "image_url", and the input branch does not set has_images at all. The new /responses tests use plain string input, so they do not exercise typed parts or image-bearing input. Please either support the typed input parts Lemonade expects to accept on /responses and add coverage, or narrow the PR claim/scope.

Non-blocking design note: build_route_context() currently lives in routing_classifier_services, but from the #2548 layering discussion this feels more like server dispatch/request-shape code than classifier-service wiring. I would prefer relocating it eventually, but I do not think that needs to block this PR if the two behavior/coverage points above are addressed.

After those are fixed, I’d be comfortable approving this.

[SlawomirNowaczyk]

Comment 1 — version lost on export round-trip: server.cpp now emits version (from info.extras["version"]) alongside routing for collection.router models, so an exported collection re-imports through /pull. Added test_021zg_router_collection_export_roundtrip which asserts the exported JSON carries "version": "1" and can be pulled back verbatim.

Comment 2 — typed content/images under-covered on /responses: Extended the extraction helpers in routing_classifier_services.cpp — collect_text_from_content now accepts input_text (Responses API) in addition to text, content_has_image now accepts input_image in addition to image_url, and the input branch of build_route_context now sets has_images (handling both message items with typed content arrays and bare content parts). Added:

• Four C++ unit tests in test_routing_classifier_services.cpp covering typed text parts + images for both chat and Responses shapes (all pass).
• Integration test test_021zh_router_collection_responses_typed_input_dispatch exercising typed input parts end-to-end.

[fl0rianr]

Thanks for the quick follow-up. I re-reviewed the current head.

But there seems to be one remaining correctness issue before I would approve:

build_route_context() still does not quite match the PR’s stated “last user message text” behavior for /responses. The chat path scans backward and selects the last role == "user" message, but the Responses input array path currently iterates over every item and appends text from any object with content, regardless of role. That means prior assistant/developer/system content can influence routing if it contains a keyword or other matched text, even when the latest user input should route differently.

Could we adjust the Responses path to mirror the chat behavior where possible? For example: scan the input array from the end for the last object with role == "user" and content, use only that content for ctx.input, while still detecting images as appropriate. If there are bare content parts without roles, the current fallback behavior seems fine.

A small unit regression would be enough here, e.g. a Responses input array where an earlier assistant message contains "code" but the last user message does not, and ctx.input should only reflect the last user message..

[SlawomirNowaczyk]

But there seems to be one remaining correctness issue before I would approve:

Good catch — fixed. The /responses input-array path now scans from the end for the last role == "user" item and uses only its content for ctx.input, matching the chat path, so earlier assistant/system turns can't influence routing. Images are still detected across the whole input, and role-less bare content parts keep the concatenation fallback. Added a unit regression (test_build_route_context_responses_uses_last_user_message) with an earlier assistant "code" message and a later user message, asserting ctx.input reflects only the last user message.

[fl0rianr]

fine by me now.