feat(routing): port Pareto frontier from helios_router to Rust substrate (T35)#67
feat(routing): port Pareto frontier from helios_router to Rust substrate (T35)#67KooshaPari wants to merge 1 commit into
Conversation
…ate (T35) Migrate the generalized Pareto frontier algorithm from KooshaPari/helios-cli/src/helios_router_ui/pareto/engine.py into Tokn as the canonical Rust router substrate. The new module (src/routing/pareto_frontier.rs) provides: - ParetoOffer: generic (attribute_name, value) carrier with the canonical 'new(id, quality, cost_usd, speed_score)' constructor. - ParetoObjective: Minimize | Maximize enum. - pareto_front_mask(offers, objectives): O(N^2) non-dominated sort. Missing attributes are treated as worst-case so an offer with a missing field never dominates a peer that has it. - compute_pareto(offers, min_cost, min_speed, max_quality): defaults-matching wrapper that returns ParetoResult with frontier_indices, total_count, frontier_count, objectives. - compute_combos(offers, k): iterative k-combination generator producing ParetoCombo with mean quality / sum cost / min speed aggregates per combo. Replaces the simpler weighted-sum scoring in pareto_router.rs with a proper multi-objective frontier suitable for fleet-grade model selection. Test coverage (12 unit tests, all passing): - Empty input, no objectives, missing-attribute sentinels. - Dominated offers, three-offer classic, canonical 5-offer scenario. - compute_pareto default + quality-only modes. - compute_combos pairs / trios / under-sized input. - ParetoOffer attribute lookup. Migration gaps (documented in SPEC_ROUTER.md for follow-up): - nats_client.py: NATS JetStream event bus (belongs in pheno-events per ADR-035B, not in router core). - db/schema.py: SQLite persistence for offers/providers/roles (belongs in a downstream dashboard substrate). - ui/components.py: Streamlit dashboard widgets (UI layer, out of scope for the Rust router substrate). Per ADR-040 quality bar: spec (SPEC_ROUTER.md, 1 page) + unit tests + hexagonal port-adapter boundary (mod.rs re-exports ParetoOffer, ParetoObjective, ParetoResult, ParetoCombo, pareto_front_mask, compute_pareto, compute_combos). Refs: T35, ADR-001 (helios-router consolidation).
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41064d0b18
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let speed_score = if !speed_values.is_empty() { | ||
| speed_values.iter().copied().reduce(f64::min) |
There was a problem hiding this comment.
When a combo contains one offer with speed_score and another without it, this branch still returns Some(min_observed) instead of marking the aggregate unknown. That makes a partially unmeasured combo look like it has a valid bottleneck speed, so downstream filters/rankings can keep or prefer a combo whose slowest member is actually unknown; this should mirror the quality/cost aggregation and only return Some when every member has the attribute.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| let speed_score = if !speed_values.is_empty() { | ||
| speed_values.iter().copied().reduce(f64::min) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Suggestion: The combo speed aggregation is inverted for the stated latency semantics: compute_pareto treats lower speed_score as better (faster), so the combo "bottleneck" should be the worst/slowest member (the maximum score), not the minimum. Using min makes a multi-offer combo look artificially fast and can select dominated bundles. [logic error]
Severity Level: Critical 🚨
- ❌ Combo speed mis-modeled; dominated bundles can appear Pareto-optimal.
- ⚠️ Downstream routers may over-select slow multi-offer strategies.Steps of Reproduction ✅
1. Open `crates/tokenledger/src/routing/pareto_frontier.rs` and locate `compute_combos`
and `combo_from_indices` (lines 256–345) plus the doc comment above `compute_combos`
stating "`speed_score` — min across members (slowest member is the bottleneck)" and the
`compute_pareto` docs (lines 206–229) describing `minimize_speed` where lower
`speed_score` is better (latency proxy).
2. In the same file, inspect test `test_compute_combos_pairs` at lines 471–485, which
constructs three `ParetoOffer`s with `speed_score` values `100.0`, `80.0`, and `60.0` and
calls `compute_combos(&offers, 2)` to generate all 2-combinations.
3. Observe that inside `combo_from_indices` at lines 71–79, `speed_score` for each combo
is calculated via `speed_values.iter().copied().reduce(f64::min)` (line 76), i.e., the
minimum `speed_score` across members, so the combo `["a", "b"]` ends up with `speed_score
= Some(80.0)` (the best/fastest member).
4. Compare this behavior with the documented semantics where `speed_score` is a
latency-like metric minimized by `compute_pareto` (lower is faster) and the comment
describing the combo speed as a bottleneck/slowest member: for latency, the bottleneck
should be the maximum `speed_score`, so the current use of `min` makes combos look
artificially faster than any individual slow member, leading to logically incorrect
aggregate speed for any multi-offer combo produced by `compute_combos`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/tokenledger/src/routing/pareto_frontier.rs
**Line:** 314:318
**Comment:**
*Logic Error: The combo speed aggregation is inverted for the stated latency semantics: `compute_pareto` treats lower `speed_score` as better (faster), so the combo "bottleneck" should be the worst/slowest member (the maximum score), not the minimum. Using `min` makes a multi-offer combo look artificially fast and can select dominated bundles.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /// - `providers`, `models` — distinct values across members | ||
| /// | ||
| /// Returns an empty `Vec` if `offers.len() < size`. | ||
| pub fn compute_combos(offers: &[ParetoOffer], size: usize) -> Vec<ParetoCombo> { |
There was a problem hiding this comment.
SUGGESTION: Combo generation can explode without a documented guard
compute_combos returns all C(n, k) combinations and has no maximum size/cap. Large size values can produce huge allocations or long runtimes; document expected input bounds or add a guard/iterator API.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| fn combo_from_indices(offers: &[ParetoOffer], indices: &[usize]) -> ParetoCombo { | ||
| let members: Vec<&ParetoOffer> = indices.iter().map(|&i| &offers[i]).collect(); | ||
|
|
||
| let quality_values: Vec<f64> = members |
There was a problem hiding this comment.
WARNING: Combo aggregation does not match Pareto sentinel handling for non-finite values
pareto_front_mask treats NaN/inf as missing, but combo_from_indices includes any returned attribute in quality/cost/speed aggregation. A single non-finite attribute can poison sum, mean, or reduce and produce Some(NaN)/Some(inf) instead of None, which can mislead downstream ranking.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| None | ||
| }; | ||
|
|
||
| let mut providers: Vec<String> = members |
There was a problem hiding this comment.
SUGGESTION: Sorting after BTreeSet collection is redundant
providers and models are already sorted by BTreeSet, so providers.sort() and models.sort() add unnecessary work and noise. Collect directly from the set iterator instead.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| } | ||
|
|
||
| // ============================================================================= |
There was a problem hiding this comment.
CRITICAL: File exceeds the 500-line hard limit
crates/tokenledger/src/routing/pareto_frontier.rs is 539 lines. The repo constraint requires files to stay at or below 500 lines; move tests into a separate #[cfg(test)] module/file or split helper modules so the production file and tests remain within the limit.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| #[test] | ||
| fn test_pareto_front_mask_missing_attribute_treated_as_worst() { |
There was a problem hiding this comment.
SUGGESTION: Test uses a mutable borrow only to keep an unnecessary mut
let mut a is not mutated; let _ = &mut a; only obscures intent and can trip readability/clippy checks. Declare a as immutable.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| |----------|---------|--------| | ||
| | `pareto_front_mask(offers, objectives)` | Returns `Vec<bool>` parallel to `offers`; `true` = non-dominated. | `helios_router/pareto/engine.py::pareto_front_mask` | | ||
| | `compute_pareto(offers, min_cost, min_speed, max_quality)` | Defaults to `(cost_usd ↓, speed_score ↓, quality ↑)`; returns `ParetoResult` with frontier indices. | `helios_router/pareto/engine.py::compute_pareto` | | ||
| | `compute_combos(offers, k)` | Returns all k-sized subsets with aggregate (mean quality, sum cost, min speed). | `helios_router/pareto/engine.py::compute_combos` | |
There was a problem hiding this comment.
WARNING: Spec documents inconsistent combo speed semantics
The PR intent describes "slowest speed" and compute_pareto treats lower speed_score as faster, but this line says compute_combos returns "min speed." If speed is latency-like, the bottleneck should be the maximum speed_score; otherwise docs and contract encourage incorrect ranking behavior.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| ## Testing | ||
|
|
||
| Unit tests live in `pareto_frontier::tests` (11 cases covering empty input, no objectives, dominated offers, missing-attribute sentinels, default and quality-only objectives, pairs/trios/empty combos, attribute lookup, and a canonical 5-offer scenario). Run with `cargo test -p tokenledger routing::pareto_frontier`. |
There was a problem hiding this comment.
SUGGESTION: Test count in spec is stale
The spec says the module has 11 unit tests, but the current file contains 12 tests. Keep this count accurate or remove the exact count to avoid stale quality-bar claims.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 7 New Issues Found | 2 Existing Bot Comments Also Unresolved | Recommendation: Request Changes Overview
Existing inline comments already cover Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (3 files)
Validation performed: Fix these issues in Kilo Cloud Reviewed by nex-n2-pro:free · Input: 1M · Output: 17.4K · Cached: 0 |
1 participant
User description
Summary
T35 work: Consolidate helios-router source into Tokn as canonical routing substrate.
What's in this PR
41064d0Pareto frontier port from helios_router to Rust substrate165df6eIntent+boundary doc refresh for L7-00113871e8Intent+boundary snapshot docsNotes
pareto-rsmodule now owns the canonical router contractSee findings/2026-06-20-T35-tokn-consolidation.md for full migration log.
CodeAnt-AI Description
Port the Pareto frontier router to Rust and document its contract
What Changed
Impact
✅ Clearer model selection✅ Fewer bad rankings from incomplete offer data✅ Safer routing for cost, speed, and quality tradeoffs💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.