feat(routing): add min_turns/max_turns and metadata gte/lte comparators

[Bekhouche]

Phase 1 of trajectory-signal routing (issue #3086): min_turns/max_turns is a new native deterministic condition on conversation depth (RouteContext:: Params::turn_count, counted from role:"user" entries in messages/input, harness-agnostic). metadata gains gte/lte numeric comparators alongside equals/any/exists, so a calling agent harness can compute its own signals (e.g. a tool-error streak) and pass them through metadata for the router to threshold on, without lemonade ever parsing tool-call semantics itself.

Both additions are additive v1 (new leaf op, new comparator key) per the schema's own evolution rule - no major bump. Verified with a full native MSVC build (VS 2026, build-vs18) and the existing WSL/GCC build (build/): all 12 routing CTest suites pass on both toolchains, plus the Python schema and fixture tests.

[jeremyfowers]

[AI-assisted review] Automated pre-review from repo-manager — flags for the human reviewer, not a replacement for one.

Ready for review — the checks below passed.

Attention level: Routine — needs any 1 reviewer. Already on this PR: ramkrishna2910, slawomirnowaczyk.

Why this rung: a contained fix or extension. The diff adds new leaf conditions (min_turns/max_turns) and new comparators (gte/lte) to the existing routing policy engine, a contained extension to the code structure.

Author's description vs. the diff: accurate

The description accurately covers both additions (min_turns/max_turns and metadata gte/lte), their additive v1 nature, and testing across both toolchains.

Focus: focused

The diff solves one problem — enabling trajectory-signal routing — with changes confined to the routing policy engine, its schema, its documentation, and its tests.

Checked: All changed files are within the routing policy subsystem: routing_policy.h, routing_classifier_services.cpp, routing_policy.cpp, routing_policy_parser.cpp, route_policy.schema.json, schema-lock.json, docs/dev/router-policy.md, and the two corresponding test files.

Alignment issues: None found

Checked: PR body references issue #3086 as the design pre-agreement artifact; @ramkrishna2910 (maintainer table: smart router and orchestration) is requested as reviewer; diff is focused to routing policy conditions with no unfocused or AI-generated sprawl.

Documentation: adequate

Checked: Read docs/dev/router-policy.md for the new min_turns/max_turns table row and the new Trajectory-signal routing section with usage examples; read src/cpp/resources/schemas/README.md for the updated parameter table; read src/cpp/resources/schemas/route_policy.schema.json for the new JSON schema entries.

Testing: adequate

Checked: Read test/cpp/test_routing_classifier_services.cpp for four new turn_count tests covering multi-turn, role filtering, responses input, and single-shot inputs; read test/cpp/test_routing_policy_deterministic.cpp for test_turns() and test_metadata_gte_lte(); both existing test files are already registered in the C++ CI workflow (C++ unit tests check passed).

Breaking changes: None found

Checked: Confirmed the diff adds new leaf operations (min_turns, max_turns) and comparator keys (gte, lte) without removing or renaming existing ones; the JSON schema uses additive oneOf branches and new properties with no removed keys or changed required constraints.

Reviewed at head 46e7039 by repo-manager (tier: reviewers). Regenerate with repo-manager review-pr 3169.

[jeremyfowers]

bot is happy :)

[ramkrishna2910]

Thanks for this — the design reasoning is genuinely good, and I want to say what's right before what's blocking. The "Trajectory-signal routing" section is the clearest writing in router-policy.md: explaining why harness-specific signals stay caller-computed (no universal "error" field on a tool result, and baking harness assumptions in would make the router not-generic) is exactly the right call and exactly the right place to record it.

The metadata extension is also correctly additive, which I checked rather than assumed: the presence gate is byte-identical to main, the equals/any/exists arms are untouched, gte/lte sit inside that gate so absent-value handling is inherited rather than altered, and the comma-split token-set decode is not applied to the numeric path ("3,5" fails the full-consume check and returns no match, as it should). No existing l1_conditions_metadata case is invalidated. The lock is refreshed in the same diff. That part is clean.

Three things to fix before this can go in.

1. min_turns/max_turns are dead in POST /v1/routing/validate. handle_routing_validate hand-assembles its RouteContext and sets only input, params.chars, params.has_images, params.has_tools, and metadata. turn_count stays at its 0 default and this PR adds no turn input, so the policy from this PR's own new docs —

{"all": [{"min_turns": 4}, {"metadata": {"key": "tool_error_streak", "gte": 2}}]}

— reports min_turns false in the preview endpoint and falls through to default_model, while the identical request through /v1/chat/completions matches. The gte half works, since metadata is plumbed through, which makes this harder to diagnose rather than easier. This isn't scope creep: #3181 patches this same function for its own new field, and the endpoint is already parameterized per-op (test/server_endpoints.py asserts "/routing/validate honored the has_images flag"). A turns integer input defaulting to 1 would do it.

2. The schema pins an invariant the code doesn't hold. The min_turns description says "at least 1 for any request that reaches the engine". The floor is gated on !messages.empty(), so {"model": "my-router", "messages": []} yields 0, as does a body with none of messages/prompt/input, as does the validate endpoint above. So {"min_turns": 1} — the natural spelling of "any real request" — silently never fires in those cases. Either apply the floor unconditionally or drop the parenthetical from both the schema description and the frozen-semantics row. This one matters disproportionately because it's about to be frozen into a description and a lock hash, so correcting it later costs another reviewed lock refresh.

3. No conformance corpus cases, and the coverage matrix isn't updated. This adds four deterministic behaviors (min_turns lower bound, max_turns upper bound, metadata gte, metadata lte) plus two new trace-emitting leaf names, and test/conformance/routing/ gains nothing. The corpus README states the matrix as load-bearing rather than aspirational — "every deterministic behavior the engine defines for v1 has exactly one lock" — and there's a trace emits condition <op> row for every existing leaf. This gap is also invisible to CI by construction: the runner only replays cases that exist, so a green RoutingConformanceCorpusTest proves nothing here. The cases most worth locking are metadata gte against an absent key, against a non-numeric value, and against a comma-list value.

Non-blocking, but worth folding in while you're here:

• std::stod is locale-sensitive. Its decimal separator follows LC_NUMERIC, so under a comma-decimal locale "3.5" and "3,5" would swap meaning. Nothing in lemonade calls setlocale, so this is latent rather than live — but it sits awkwardly against a frozen table that goes out of its way to specify the keyword case-fold as locale-independent, and #3100 explicitly reaches for std::locale::classic() for the same reason on the formatting side.
• The accepted grammar is wider than documented. stod also takes 0x10 → 16, 1e9, unary +, bare .5, and trailing .. All of that becomes frozen v1 behavior under a description that says only "parse the value as a number". Switching to strtod under an explicit C locale fixes this and the locale point together, and drops the throw-per-non-numeric-value on the request path.
• Whitespace handling is asymmetric. Leading whitespace is skipped by strtod (including \v/\f); trailing is trimmed by is_ascii_ws, which covers only space/tab/CR/LF. The schema says "leading/trailing ASCII whitespace tolerated" as though symmetric.
• A bounded range can't be written in one leaf. {"key":"x","gte":1,"lte":5} is rejected by both the schema oneOf and the C++ comparators != 1 check — and the PR tests that rejection, so it's clearly deliberate. But this is the vocabulary's only non-band bound pair (min_score/max_score, min_chars/max_chars, and your own min_turns/max_turns all form bands), and the docs don't mention that a range needs two leaves under an all. One sentence and an example would cover it. Relaxing oneOf later is backward-compatible, so this isn't a one-way door.
• min_turns > max_turns parses and silently never matches. Consistent with min_chars/max_chars, but validate_metadata_match's neighbour does cross-check min_score > max_score and throws.
• Parser-path validation is untested. The new rejection tests exercise build_metadata/build_turns in routing_policy.cpp, but the edits to validate_metadata_match/validate_leaf in routing_policy_parser.cpp — the path taken at registration — have no coverage; test_routing_policy_parser.cpp is untouched.
• Scope convention is drifting. min_chars means the latest turn, min_turns means the whole conversation, and #3181's min_total_chars means the whole conversation with a total_ prefix. I don't think min_total_turns is worth it, but please state the scope explicitly in the docs row so the asymmetry reads as deliberate.

Merge-order note since you'll be rebasing anyway: #3181 rewrites the image-detection loops you carry as context — in both the messages branch and the Responses input-array branch — replacing the early break with a latched flag so a totalling pass can continue. On conflict, keep #3181's loop bodies and graft your turn-count block above them; taking your side wholesale would silently truncate its total_chars at the first image-bearing message. Once both land, the messages branch will have three separate walks of one array, so folding the turn count into the combined pass would be welcome.

[SlawomirNowaczyk]

Overall look good, here are four improvement suggestions:

1. Replace std::stod in parse_metadata_number with locale-independent parsing

parse_metadata_number (added in routing_policy.cpp) uses std::stod, which has two problems that matter for behavior that's about to be frozen into a schema lock:

• Locale-sensitive decimal separator — stod honors LC_NUMERIC, so under a comma-decimal locale "3.5" and "3,5" swap meaning. The rest of the routing vocabulary goes out of its way to be locale-independent (the keyword case-fold explicitly is), so this is an inconsistency waiting to bite.
• Grammar wider than the docs — stod also accepts 0x10, 1e9, unary +, bare .5, and trailing ., none of which the schema description ("parse the value as a number") implies.

Switching to strtod under an explicit std::locale::classic() / C locale fixes both at once, and lets you drop the try/catch on the request hot path:

std::optional<double> parse_metadata_number(const std::string& s) {
    if (is_blank(s)) return std::nullopt;
    const char* begin = s.c_str();
    char* parse_end = nullptr;
    errno = 0;
    // strtod is C-locale via classic(); guard the decimal separator explicitly.
    double value = std::strtod(begin, &parse_end);
    std::size_t consumed = static_cast<std::size_t>(parse_end - begin);
    std::size_t end = s.size();
    while (end > consumed && is_ascii_ws(s[end - 1])) --end;
    if (consumed == 0 || consumed != end || !std::isfinite(value)) return std::nullopt;
    return value;
}

(If you want to also reject hex/exponent forms to match the docs exactly, that needs an explicit pre-scan — otherwise loosen the schema wording instead.)

2. min_turns/max_turns are dead in POST /v1/routing/validate

server.cpp:3803-3809 hand-assembles its RouteContext and only sets input, params.chars, params.has_images, params.has_tools, and metadata. It never touches turn_count, so it stays at the 0 default:

RouteContext ctx;
ctx.input = prompt;
ctx.params.chars = prompt.size();
ctx.params.has_images = has_images;
ctx.params.has_tools = has_tools;
ctx.metadata = std::move(metadata);

That means a policy like {"all":[{"min_turns":4},{"metadata":{"key":"tool_error_streak","gte":2}}]} previews as not matching on this endpoint (the min_turns half is always false) while the identical request through /v1/chat/completions matches — the gte half working makes the discrepancy harder to diagnose, not easier. Add an optional turns integer input (defaulting to 1, consistent with "any real request is one turn") and set ctx.params.turn_count, mirroring how the endpoint already plumbs has_images/has_tools.

3. Collapse the repeated messages/input walks and cross-check the bounds

In routing_classifier_services.cpp:376-389, the PR adds a turn-count loop alongside the existing image-scan loop and the reverse last-user-text loop — three separate passes over the same messages array (and likewise for the Responses input array). Folding the role=="user" count into the existing forward image-scan pass keeps the counting and image detection from drifting apart and is a straightforward simplification:

for (const auto& msg : messages) {
    if (!msg.is_object()) continue;
    if (msg.value("role", std::string()) == "user") ++ctx.params.turn_count;
    if (!ctx.params.has_images && msg.contains("content") &&
        content_has_image(msg["content"])) {
        ctx.params.has_images = true;
    }
}

Note this depends on the merge order with #3181, which converts the image loop's early break into a latched flag for its own totalling pass — the combined loop above is compatible with that.

4. Improve parser

Separately, at parse time min_turns > max_turns parses cleanly and then silently never matches. That's consistent with min_chars/max_chars, but the neighbouring validate_metadata_match does cross-check min_score > max_score and throws. Adding the same guard for the turn bounds (and ideally the char bounds) would fail-fast on an obviously-unsatisfiable policy at registration instead of at request time.

[Bekhouche]

@ramkrishna2910 @SlawomirNowaczyk Pushed a fix covering both reviews: turns field added to /v1/routing/validate, the turn floor is now unconditional, min_turns > max_turns is rejected at parse time, parse_metadata_number is now a locale-independent hand-rolled grammar, the three-pass walk is folded into one, and conformance + parser-path tests are added. Ready for re-review.

[SlawomirNowaczyk]

I see one final issue: I think parse_metadata_number is not actually locale-independent, despite the new comment.

The rewrite hand-rolls the grammar but still does the final conversion with std::stod(s.substr(...)) and drops the old consumed == end guard. std::stod honors LC_NUMERIC, so under a comma-decimal locale std::stod("3.5") parses 3, stops at the ., and returns 3.0 — and with no consumed-length check that truncation now goes undetected (the old code would have returned nullopt). So the comment's claim:

stod … never sees anything a comma-decimal LC_NUMERIC could misread — locale-independent by construction

is inaccurate: the grammar acceptance is locale-independent, but the parsed value is not. It's arguably a slight regression (silent 3.5→3 instead of no-match). Fix: convert with something genuinely locale-independent — std::from_chars<double> (C++17, and you've already hand-validated the grammar so the fallback portability worry is small), or reinstate a full-consume check, or just soften the comment to "latent — nothing sets a non-C locale."

[Bekhouche]

@SlawomirNowaczyk the conversion still used stod after the grammar check, so it was still locale-sensitive. Fixed it by using an istringstream with std::locale::classic() instead. I also added a regression test with a synthetic comma-decimal locale, so it does not depend on the OS locale. Ready for re-review.

[SlawomirNowaczyk]

LGTM now