Repository navigation
fix(OMN-10657): resolve delegation endpoints from deployed contract, not env vars - #1530
Merged
Merged
Conversation
…not env vars Routing handler reads endpoint URLs from the deployed bifrost contract at ~/.omninode/delegation/bifrost_delegation.yaml instead of os.environ.get(). The installer (install-delegation.sh --apply) deploys the contract and populates endpoint_url fields. - bifrost_delegation.yaml: base_url_env → endpoint_url (empty in source) - routing_tiers.yaml: env_var → backend_ref referencing bifrost backends - ModelTierModel: env_var field → backend_ref field - handler_delegation_routing: _load_bifrost_endpoints() reads deployed contract, _select_model_for_task() uses contract lookup, selected_model uses bifrost model_name (full vLLM model ID) for correct inference dispatch - All os.environ.get() calls removed from endpoint resolution path - Tests rewritten to use BIFROST_CONTRACT_PATH with temp YAML files
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR moves endpoint and model resolution from environment variables to an installer-populated bifrost contract YAML, updates configs and model types to reference backend IDs, implements a cached bifrost loader in the routing handler, and revises unit and integration tests to use temporary bifrost contracts. ChangesBifrost Contract-Driven Endpoint Configuration
Sequence Diagram(s)sequenceDiagram
participant Client as delta()
participant Loader as _load_bifrost_endpoints()
participant Selector as _select_model_for_task()
participant Decision as RoutingDecision
Client->>Loader: load bifrost contract (BIFROST_CONTRACT_PATH or default)
Loader-->>Client: backend_id → BifrostBackendRef
Client->>Selector: select model using bifrost_backends and tiers
Selector-->>Client: chosen model tier
Client->>Decision: resolve endpoint_url & model_name from bifrost
Decision-->>Client: RoutingDecision(selected_model, endpoint_url)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/nodes/node_delegation_routing_reducer/handlers/test_handler_delegation_routing.py (1)
60-82: 💤 Low valueConsider extracting to shared conftest.py.
The
_write_bifrosthelper is duplicated betweentest_handler_delegation_routing.pyandtest_handler_delegation_routing_contract.py. While this works, extracting it to a sharedconftest.pyfixture would improve maintainability.🤖 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 `@tests/unit/nodes/node_delegation_routing_reducer/handlers/test_handler_delegation_routing.py` around lines 60 - 82, The _write_bifrost helper is duplicated across test_handler_delegation_routing.py and test_handler_delegation_routing_contract.py; extract it into a shared conftest.py as a fixture (e.g., a tmp_bifrost_writer or write_bifrost fixture) that accepts tmp_path and backends dict and returns the file path, then update both tests to use the new fixture instead of the local _write_bifrost function (replace direct calls to _write_bifrost with the fixture call or pass the fixture into tests that need it).
🤖 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.
Nitpick comments:
In
`@tests/unit/nodes/node_delegation_routing_reducer/handlers/test_handler_delegation_routing.py`:
- Around line 60-82: The _write_bifrost helper is duplicated across
test_handler_delegation_routing.py and
test_handler_delegation_routing_contract.py; extract it into a shared
conftest.py as a fixture (e.g., a tmp_bifrost_writer or write_bifrost fixture)
that accepts tmp_path and backends dict and returns the file path, then update
both tests to use the new fixture instead of the local _write_bifrost function
(replace direct calls to _write_bifrost with the fixture call or pass the
fixture into tests that need it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4df8917f-1bcc-49f5-aef6-56ba7d31be0e
📒 Files selected for processing (7)
src/omnibase_infra/configs/bifrost_delegation.yamlsrc/omnibase_infra/configs/routing_tiers.yamlsrc/omnibase_infra/nodes/node_delegation_routing_reducer/handlers/handler_delegation_routing.pysrc/omnibase_infra/nodes/node_delegation_routing_reducer/models/model_delegation_config.pysrc/omnibase_infra/nodes/node_delegation_routing_reducer/models/model_tier_model.pytests/unit/nodes/node_delegation_routing_reducer/handlers/test_handler_delegation_routing.pytests/unit/nodes/node_delegation_routing_reducer/handlers/test_handler_delegation_routing_contract.py
…ntegration tests - Bump node contract version 0.2.0 → 0.3.0, update description to reflect bifrost-based endpoint resolution, remove LLM env var config_dependencies, add BIFROST_CONTRACT_PATH config dependency - Add contracts/OMN-10657.yaml with dod-deploy evidence for deploy gate - Add tests/integration/delegation/test_bifrost_contract_loading.py — 5 tests proving bifrost contract loading from on-disk source file - Fix test_routing_with_contract_loaded_picks_local_tier to use bifrost contract instead of deprecated LLM env vars (OMN-10657 removed env-var-based routing)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/delegation/test_bifrost_contract_loading.py (1)
25-25: ⚡ Quick winUse the repo’s non-resolving REPO_ROOT pattern in integration tests.
Line 25 should follow the established lexical parent traversal style (same depth) instead of
.resolve().♻️ Suggested change
-REPO_ROOT = Path(__file__).resolve().parents[3] +REPO_ROOT = Path(__file__).parent.parent.parent.parentBased on learnings: “In this repository’s integration tests, follow the established convention for computing
REPO_ROOTwithout calling.resolve()… use lexical parent traversal depth.”🤖 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 `@tests/integration/delegation/test_bifrost_contract_loading.py` at line 25, Replace the REPO_ROOT assignment to follow the repository's non-resolving lexical parent traversal style: remove the .resolve() call and compute REPO_ROOT using the Path(__file__) parents chain (the same depth used elsewhere) so the REPO_ROOT variable (named in this file) is assigned via Path(__file__).parents[3] instead of Path(__file__).resolve().parents[3].
🤖 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/omnibase_infra/nodes/node_delegation_routing_reducer/contract.yaml`:
- Line 27: The contract description for the "Deterministic routing reducer"
currently claims "Pure function, no I/O" while also describing reading on-disk
contracts and overrideable paths; remove the contradictory "Pure function, no
I/O" phrase and replace it with wording that clearly states the runtime performs
read-only I/O to load the bifrost contract and task-class contracts (respecting
BIFROST_CONTRACT_PATH and TASK_CLASS_CONTRACT_PATH), e.g., "Performs read-only
configuration I/O to load on-disk contracts (overrideable via
BIFROST_CONTRACT_PATH and TASK_CLASS_CONTRACT_PATH)." Ensure the updated
sentence appears alongside the existing descriptions of resolving endpoint URLs
and enforcing cloud_routing_policy, pricing_ceiling_per_1k_tokens, and
escalation_policy.tier_order.
In `@tests/integration/delegation/test_bifrost_contract_loading.py`:
- Around line 67-98: Update the two tests
(test_bifrost_endpoints_load_from_source_contract_when_deployed_absent and
test_bifrost_endpoints_load_from_env_override) to assert behavior rather than
just return type: call _handler_mod._load_bifrost_endpoints() and verify it
contains a known sentinel backend/endpoint key/value that exists in
SOURCE_BIFROST_PATH (e.g., a specific backend name or endpoint_url present in
that source file) to prove the source fallback; for the env-override test, set
BIFROST_CONTRACT_PATH to SOURCE_BIFROST_PATH and assert the same sentinel is
present (and that any deployed-file sentinel is absent) so the env path was
used; keep using SOURCE_BIFROST_PATH and the _load_bifrost_endpoints symbol to
locate the code under test.
---
Nitpick comments:
In `@tests/integration/delegation/test_bifrost_contract_loading.py`:
- Line 25: Replace the REPO_ROOT assignment to follow the repository's
non-resolving lexical parent traversal style: remove the .resolve() call and
compute REPO_ROOT using the Path(__file__) parents chain (the same depth used
elsewhere) so the REPO_ROOT variable (named in this file) is assigned via
Path(__file__).parents[3] instead of Path(__file__).resolve().parents[3].
🪄 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: c7c20ee8-b33c-4af3-becf-73ccbadf4746
📒 Files selected for processing (4)
contracts/OMN-10657.yamlsrc/omnibase_infra/nodes/node_delegation_routing_reducer/contract.yamltests/integration/delegation/test_bifrost_contract_loading.pytests/integration/delegation/test_routing_reducer_contract_loading.py
✅ Files skipped from review due to trivial changes (1)
- contracts/OMN-10657.yaml
…ndpoint resolution LLM_CODER_URL/LLM_CODER_FAST_URL/LLM_DEEPSEEK_R1_URL are no longer in config_dependencies after OMN-10657 removed env-var endpoint resolution. Update tests to assert BIFROST_CONTRACT_PATH instead.
Evidence-Source: OCC#807 Evidence-Ticket: OMN-10657
…ssertions - contract.yaml: replace "Pure function, no I/O" with accurate "Performs read-only configuration I/O" wording (CR thread #3209105277) - test_bifrost_contract_loading.py: strengthen fallback/override tests to assert specific sentinel backend keys rather than just dict type (CR thread #3209105282)
jonahgabriel
enabled auto-merge (squash)
May 8, 2026 14:26
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/model_bifrost_delegation_config.py (1)
118-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd validator to ensure endpoint_url is populated at config load time
The schema allows
endpoint_urlto default to""andbase_url_envto beNone, meaning backends can load with no URL source specified. The canonical YAML assumes the installer populatesendpoint_urlat deploy time, but there's no schema validator to enforce this. If deployment fails to write the URL, configs will validate successfully but fail at request-time when the handler tries to reach a backend with an empty endpoint.Add a Pydantic validator (e.g.,
@field_validator) toModelDelegationBackendConfigto ensure at leastendpoint_urlis non-empty after config load, so misconfigured backends fail during config validation rather than during request handling.🤖 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/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/model_bifrost_delegation_config.py` around lines 118 - 125, Add a Pydantic field validator to ModelDelegationBackendConfig that runs after model initialization and checks the endpoint_url field is non-empty (and/or resolves from base_url_env if you intend to support that), raising a ValueError if no usable URL was provided; use a `@field_validator`("endpoint_url", mode="after") (or the appropriate Pydantic validator API in this codebase) to perform the check and ensure config load fails when endpoint_url == "" (referencing the ModelDelegationBackendConfig class and the endpoint_url and base_url_env fields).
🧹 Nitpick comments (1)
tests/unit/delegation/test_delegation_chain_e2e.py (1)
91-99: ⚡ Quick winUse non-empty
model_namevalues in the test contract to validate model dispatch path.With blank
model_nameentries, this E2E can pass without exercising the contract-driven selected-model behavior introduced in this PR. Populate sentinel model IDs here and assert them in the routing/inference path.💡 Proposed update
- " model_name: \n" + ' model_name: "Qwen/Qwen2.5-Coder-32B-Instruct"\n' @@ - " model_name: \n" + ' model_name: "deepseek-ai/DeepSeek-R1-Distill-Qwen-14B"\n'# Example assertion to add in test_full_chain_completes_with_passing_gate assert decision.selected_model in { "Qwen/Qwen2.5-Coder-32B-Instruct", "deepseek-ai/DeepSeek-R1-Distill-Qwen-14B", }🤖 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 `@tests/unit/delegation/test_delegation_chain_e2e.py` around lines 91 - 99, The test uses empty model_name entries in the YAML contract so the contract-driven model-selection path isn't exercised; update the contract data in tests/unit/delegation/test_delegation_chain_e2e.py to use sentinel non-empty model IDs (e.g., "Qwen/Qwen2.5-Coder-32B-Instruct" and "deepseek-ai/DeepSeek-R1-Distill-Qwen-14B") for the backend_id entries, then enhance test_full_chain_completes_with_passing_gate to assert that decision.selected_model is one of those sentinel IDs to validate routing/inference selection.
🤖 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 `@tests/integration/nodes/test_delegation_pipeline_e2e.py`:
- Around line 84-90: The test contract in
tests/integration/nodes/test_delegation_pipeline_e2e.py leaves model_name blank
for backends (including the entry with backend_id local-deepseek-r1-14b), which
allows fallback behavior and hides regressions; populate the model_name fields
in the fixture with the actual model identifiers used by those backends and
update the assertion in test_happy_path_document_task_routes_to_deepseek to
expect the full deployed model ID (not an empty or partial name) so the test
validates contract-driven model dispatch end-to-end.
---
Outside diff comments:
In
`@src/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/model_bifrost_delegation_config.py`:
- Around line 118-125: Add a Pydantic field validator to
ModelDelegationBackendConfig that runs after model initialization and checks the
endpoint_url field is non-empty (and/or resolves from base_url_env if you intend
to support that), raising a ValueError if no usable URL was provided; use a
`@field_validator`("endpoint_url", mode="after") (or the appropriate Pydantic
validator API in this codebase) to perform the check and ensure config load
fails when endpoint_url == "" (referencing the ModelDelegationBackendConfig
class and the endpoint_url and base_url_env fields).
---
Nitpick comments:
In `@tests/unit/delegation/test_delegation_chain_e2e.py`:
- Around line 91-99: The test uses empty model_name entries in the YAML contract
so the contract-driven model-selection path isn't exercised; update the contract
data in tests/unit/delegation/test_delegation_chain_e2e.py to use sentinel
non-empty model IDs (e.g., "Qwen/Qwen2.5-Coder-32B-Instruct" and
"deepseek-ai/DeepSeek-R1-Distill-Qwen-14B") for the backend_id entries, then
enhance test_full_chain_completes_with_passing_gate to assert that
decision.selected_model is one of those sentinel IDs to validate
routing/inference selection.
🪄 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: 71156d60-e051-4d99-bce5-b4cf5c09e83f
📒 Files selected for processing (5)
src/omnibase_infra/nodes/node_delegation_routing_reducer/contract.yamlsrc/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/model_bifrost_delegation_config.pytests/integration/delegation/test_bifrost_contract_loading.pytests/integration/nodes/test_delegation_pipeline_e2e.pytests/unit/delegation/test_delegation_chain_e2e.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/delegation/test_bifrost_contract_loading.py
- src/omnibase_infra/nodes/node_delegation_routing_reducer/contract.yaml
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
Test plan
Evidence
Evidence-Source: OCC#807
Evidence-Ticket: OMN-10657
OMN-10657
Summary by CodeRabbit
New Features
Chores
Tests