Repository navigation
feat(OMN-10847): populate routing_outcomes.quality_score from canary grading - #605
Closed
jonahgabriel wants to merge 2 commits into
Closed
jonahgabriel wants to merge 2 commits into
jonahgabriel wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthrough
ChangesMaterialize Result Refactoring
Sequence DiagramsequenceDiagram
participant State as ModelScoreReducerState
participant Handler as HandlerCanaryScoreReducer
participant Result as ModelMaterializeResult
State->>Handler: state.scores (list of rows)
Handler->>Handler: build capability_score_rows from scores
Handler->>Handler: compute best_key (highest composite_score)
Handler->>Handler: build routing_outcome_rows with selected flags
Handler->>Result: wrap both row lists
Result->>Handler: return ModelMaterializeResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
jonahgabriel
enabled auto-merge
May 11, 2026 20:29
✅ Architectural Review — PASSEDErrors: 0 What this checks
No architectural violations found. Powered by node_pr_review_bot |
✅ PR Review Bot — CLEANFindings: 0
Verdict semantics
Powered by node_pr_review_bot — FSM pipeline with multi-model review fan-out |
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical/error findings | No |
blocked |
CRITICAL or ERROR findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnimarket/nodes/node_canary_score_reducer/handlers/handler_canary_score_reducer.py (1)
79-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSelect winners per
task_typeinstead of a single global winnerLine 80/Line 107 use one
best_keyacross all rows, so only one row total can beselected=True. This breaks the “highest-scoring model per task type” behavior when multiple task types are present instate.scores.Proposed fix
- best_key: str | None = None - best_score: float = -1.0 + best_key_by_task: dict[str, str] = {} + best_score_by_task: dict[str, float] = {} for row in scored_rows: - if row.composite_score is not None and row.composite_score > best_score: - best_score = row.composite_score - best_key = f"{row.model_key}::{row.task_type}" + if row.composite_score is None: + continue + task_type = row.task_type + if row.composite_score > best_score_by_task.get(task_type, float("-inf")): + best_score_by_task[task_type] = row.composite_score + best_key_by_task[task_type] = f"{row.model_key}::{task_type}" @@ - "selected": (f"{row.model_key}::{row.task_type}" == best_key), + "selected": ( + f"{row.model_key}::{row.task_type}" + == best_key_by_task.get(row.task_type) + ),🤖 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 `@src/omnimarket/nodes/node_canary_score_reducer/handlers/handler_canary_score_reducer.py` around lines 79 - 107, The code currently computes a single global best_key so only one row is marked selected; instead compute the highest composite_score per task_type and mark selection per task_type. Iterate scored_rows to build a dict mapping task_type -> (best_score, best_model_key) using row.composite_score, then when populating capability_score_rows and routing_outcome_rows use that dict to set "selected" by comparing f"{row.model_key}::{row.task_type}" (or compare model_key and task_type) against the per-task best entry; update any references to best_key accordingly (look at scored_rows, best_key, routing_outcome_rows, capability_score_rows, row.model_key, row.task_type, row.composite_score).
🤖 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.
Outside diff comments:
In
`@src/omnimarket/nodes/node_canary_score_reducer/handlers/handler_canary_score_reducer.py`:
- Around line 79-107: The code currently computes a single global best_key so
only one row is marked selected; instead compute the highest composite_score per
task_type and mark selection per task_type. Iterate scored_rows to build a dict
mapping task_type -> (best_score, best_model_key) using row.composite_score,
then when populating capability_score_rows and routing_outcome_rows use that
dict to set "selected" by comparing f"{row.model_key}::{row.task_type}" (or
compare model_key and task_type) against the per-task best entry; update any
references to best_key accordingly (look at scored_rows, best_key,
routing_outcome_rows, capability_score_rows, row.model_key, row.task_type,
row.composite_score).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 552c215a-b325-49be-aafb-4c086f7fa833
📒 Files selected for processing (3)
src/omnimarket/nodes/node_canary_score_reducer/handlers/handler_canary_score_reducer.pysrc/omnimarket/nodes/node_canary_score_reducer/models/model_materialize_result.pysrc/omnimarket/nodes/node_canary_score_reducer/tests/test_handler_canary_score_reducer.py
3 tasks
…oop (#606) * feat(OMN-10846): bridge capability_scores to routing policy engine via score lookup Adds build_available_models_from_scores() — a caller-side bridge that converts capability_scores table rows into ModelAvailableModel instances accepted by the routing policy engine. HandlerRoutingPolicy is not modified. * fix(OMN-10846): resolve mypy strict arg-type error in score lookup * test(OMN-10848): proof of life — canary → scores → routing feedback loop End-to-end integration test proving the full chain works in-memory: ModelCanaryReport → HandlerCanaryScoreReducer → ModelMaterializeResult → build_available_models_from_scores → HandlerRoutingPolicy selects champion. Also re-exports EnumTaskType, ModelRoutingPolicyRequest, ModelAvailableModel, and build_available_models_from_scores from node_routing_policy_engine.__init__ to avoid cross-node .models.* reach-in violations (OMN-9263).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/omnimarket/nodes/node_routing_policy_engine/tests/test_handler_score_lookup.py (1)
39-44: ⚡ Quick winAdd a unit test for documented fallback/default behavior.
The tests currently validate normal and empty paths, but not the contract that missing/
Nonesuccess_rateand missingcost_mapentries default to0.0.Suggested test addition
`@pytest.mark.unit` def test_build_available_models_empty_scores_returns_empty() -> None: """Empty input rows and cost_map produce an empty list.""" result = build_available_models_from_scores([], {}) assert result == [] + + +@pytest.mark.unit +def test_build_available_models_defaults_for_missing_values() -> None: + rows: list[dict[str, object]] = [ + {"model_key": "qwen3-coder", "success_rate": None}, + {"model_key": "deepseek-r1"}, + ] + result = build_available_models_from_scores(rows, {"qwen3-coder": 0.001}) + + assert len(result) == 2 + assert result[0].score == pytest.approx(0.0) + assert result[0].cost_per_token == pytest.approx(0.001) + assert result[1].score == pytest.approx(0.0) + assert result[1].cost_per_token == pytest.approx(0.0)🤖 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 `@src/omnimarket/nodes/node_routing_policy_engine/tests/test_handler_score_lookup.py` around lines 39 - 44, Add a unit test that verifies build_available_models_from_scores applies documented defaults: when input score rows have missing or None success_rate and when cost_map lacks an entry for a model, the function should treat both success_rate and cost as 0.0; create test rows with a model missing the success_rate field and another with success_rate=None, pass a cost_map that omits those model keys, call build_available_models_from_scores and assert the returned entries for those models contain success_rate == 0.0 and cost == 0.0 (and otherwise match expected fields).
🤖 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
`@src/omnimarket/nodes/node_routing_policy_engine/handlers/handler_score_lookup.py`:
- Around line 33-36: The current parsing coerces invalid model_key and can crash
on malformed success_rate; update the parsing in handler_score_lookup.py so you
first validate model_key from row (ensure it's not None, not empty, and of
expected type/string) and skip or continue when invalid instead of doing
str(None); for success_rate (raw_score) parse it safely using a try/except
around numeric conversion (handle ValueError/TypeError), default to 0.0 on
failure, and clamp the resulting score into a safe range (e.g., 0.0–1.0) before
using it; keep the cost lookup via cost_map.get(model_key, 0.0) unchanged.
---
Nitpick comments:
In
`@src/omnimarket/nodes/node_routing_policy_engine/tests/test_handler_score_lookup.py`:
- Around line 39-44: Add a unit test that verifies
build_available_models_from_scores applies documented defaults: when input score
rows have missing or None success_rate and when cost_map lacks an entry for a
model, the function should treat both success_rate and cost as 0.0; create test
rows with a model missing the success_rate field and another with
success_rate=None, pass a cost_map that omits those model keys, call
build_available_models_from_scores and assert the returned entries for those
models contain success_rate == 0.0 and cost == 0.0 (and otherwise match expected
fields).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85a43a3e-c70d-465d-9fc9-3314577470f4
📒 Files selected for processing (4)
src/omnimarket/nodes/node_canary_score_reducer/tests/test_integration_feedback_loop.pysrc/omnimarket/nodes/node_routing_policy_engine/__init__.pysrc/omnimarket/nodes/node_routing_policy_engine/handlers/handler_score_lookup.pysrc/omnimarket/nodes/node_routing_policy_engine/tests/test_handler_score_lookup.py
Comment on lines
+33
to
+36
| model_key = str(row["model_key"]) | ||
| raw_score = row.get("success_rate") | ||
| score = float(str(raw_score)) if raw_score is not None else 0.0 | ||
| cost = cost_map.get(model_key, 0.0) |
There was a problem hiding this comment.
Harden row parsing to avoid bad data poisoning or runtime failures.
This bridge currently coerces invalid keys (None → "None") and can throw on malformed success_rate, which can fail routing on a single bad row. Add explicit key validation and safe numeric parsing with bounded score handling.
Proposed fix
+import math
+
def build_available_models_from_scores(
capability_rows: list[dict[str, object]],
cost_map: dict[str, float],
) -> list[ModelAvailableModel]:
@@
models: list[ModelAvailableModel] = []
for row in capability_rows:
- model_key = str(row["model_key"])
+ model_key_raw = row.get("model_key")
+ if not isinstance(model_key_raw, str) or not model_key_raw.strip():
+ continue
+ model_key = model_key_raw
raw_score = row.get("success_rate")
- score = float(str(raw_score)) if raw_score is not None else 0.0
+ try:
+ score = float(raw_score) if raw_score is not None else 0.0
+ except (TypeError, ValueError):
+ score = 0.0
+ if not math.isfinite(score) or not (0.0 <= score <= 1.0):
+ score = 0.0
cost = cost_map.get(model_key, 0.0)
models.append(🤖 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
`@src/omnimarket/nodes/node_routing_policy_engine/handlers/handler_score_lookup.py`
around lines 33 - 36, The current parsing coerces invalid model_key and can
crash on malformed success_rate; update the parsing in handler_score_lookup.py
so you first validate model_key from row (ensure it's not None, not empty, and
of expected type/string) and skip or continue when invalid instead of doing
str(None); for success_rate (raw_score) parse it safely using a try/except
around numeric conversion (handle ValueError/TypeError), default to 0.0 on
failure, and clamp the resulting score into a safe range (e.g., 0.0–1.0) before
using it; keep the cost lookup via cost_map.get(model_key, 0.0) unchanged.
Contributor
Author
|
Code delivered via PR #606 which cherry-picked this work. Closing as superseded. |
auto-merge was automatically disabled
May 11, 2026 21:23
Pull request was closed
jonahgabriel
deleted the
jonah/omn-10847-populate-routing_outcomesquality_score-from-canary-grading
branch
June 11, 2026 17:56
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
1 participant
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.
Summary
Extends
HandlerCanaryScoreReducer.materialize()to produce rows for bothcapability_scoresandrouting_outcomestables. AddsModelMaterializeResultto hold both output sets. Therouting_outcomes.quality_scorecolumn (migration 066) is now populated from canary grading composite scores.node_platform_readinessalready queriesSELECT COUNT(*) FROM routing_outcomes WHERE quality_score IS NOT NULL— this change makes that query return real data after canary runs.Part of the canary-to-routing feedback loop (OMN-10843).
Ticket: OMN-10847
dod_evidence
materialize()returnsModelMaterializeResultwith bothcapability_score_rowsandrouting_outcome_rowsrouting_outcomes.quality_scoreset to composite score from canary gradingselected=Truefor highest-scoring model per task typeTest plan
uv run pytest src/omnimarket/nodes/node_canary_score_reducer/tests/ -v— 6 tests passSummary by CodeRabbit
New Features
Tests