Repository navigation
feat: LLM dispatch wiring, eval models, bifrost config loader [OMN-6785] - #1063
Merged
Merged
Conversation
… [OMN-6785] Wire LLM inference and embedding handlers into the runtime container via wire_llm_handlers() in util_container_wiring.py. Remove deprecated Ollama handler references from both LLM contracts and add GLM/Gemini provider support. Rewrite bifrost config_loader to build routing rules from env vars including external providers (GLM, Gemini) and eval task routing. Add eval task/result/finding/budget-cap models with typed enums for status, task type, severity, and category. Register new Kafka topics for inference requests, embedding requests, and eval completion events.
📝 WalkthroughWalkthroughThis PR introduces evaluation task infrastructure including four new enums (finding categories, severity, task status, task type), four new Pydantic models (budget cap, finding, result, task), extends LLM handler configuration to support GLM and Gemini backends with cost-tier routing, updates runtime container wiring with refined error handling and return types, adds an evaluation completion event topic, and adjusts validation constants. Changes
Poem
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 3
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/contract.yaml (1)
10-27:⚠️ Potential issue | 🟡 MinorAlign contract description with declared handler routing.
The updated description says GLM/Gemini handlers are delegated here, but
handler_routing.handlerscurrently declares onlyinference.openai_compatible. Please either add explicit GLM/Gemini handler declarations or reword this text to describe them as routed backends behind the existing handler.As per coding guidelines:
src/**/nodes/*/contract.yaml: YAML contract files are the source of truth for node behavior - define handlers, routing, workflows, and all behavior in contract.yaml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/nodes/node_llm_inference_effect/contract.yaml` around lines 10 - 27, The contract description claims delegation to GLM/Gemini but the handler routing only declares inference.openai_compatible; update contract.yaml to make behavior authoritative by either (A) adding explicit handler entries for GLM and Gemini under handler_routing.handlers (e.g., identifiers for inference.glm and inference.gemini) and any corresponding routing rules, or (B) change the description text in the node_name/description block of node_llm_inference_effect to state that only an OpenAI-compatible handler (inference.openai_compatible) is declared and that other providers (GLM/Gemini) are supported only via upstream/backing handlers, ensuring the phrasing matches the declared handlers exactly; reference the contract fields contract_name/node_name "node_llm_inference_effect" and handler_routing.handlers to locate and update the YAML.
🧹 Nitpick comments (3)
tests/unit/models/eval/test_model_eval_task.py (1)
66-72: Consider using specific exception type for frozen model tests.Catching generic
Exceptionis broad; Pydantic v2 frozen models raiseValidationErroron mutation attempts. Using the specific type improves test precision and failure clarity.♻️ Suggested improvement
+from pydantic import ValidationError + # Then in each frozen test: def test_frozen(self) -> None: task = ModelEvalTask( task_type=EnumEvalTaskType.CODE_REVIEW, target_repo="test", ) - with pytest.raises(Exception): + with pytest.raises(ValidationError): task.status = EnumEvalTaskStatus.RUNNING # type: ignore[misc]Also applies to: 88-95, 143-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/models/eval/test_model_eval_task.py` around lines 66 - 72, The tests (e.g., test_frozen for ModelEvalTask) currently catch a broad Exception; update them to assert the specific pydantic.ValidationError raised on mutation by replacing pytest.raises(Exception) with pytest.raises(ValidationError) and add the required import from pydantic (ValidationError); apply the same change to the other frozen-model tests referenced (around the blocks at 88-95 and 143-146) so each test precisely expects ValidationError from attempted attribute mutation.src/omnibase_infra/runtime/util_container_wiring.py (1)
391-394: Consider exportingwire_llm_handlersin__all__for API parity.This new wiring entrypoint is public-facing in this module but is not currently listed in
__all__, unlike other wiring helpers.📦 Suggested export update (outside this hunk)
__all__: list[str] = [ + "wire_llm_handlers", # Container wiring functions "get_compute_registry_from_container",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/runtime/util_container_wiring.py` around lines 391 - 394, The module exposes a public wiring function wire_llm_handlers but it isn’t included in the module's exported symbol list (__all__), causing API inconsistency; update the module-level __all__ to include "wire_llm_handlers" (add the exact string to the existing list/tuple) so it is exported alongside the other wiring helpers—locate the __all__ declaration near the top of the file and append "wire_llm_handlers" to it.tests/unit/nodes/node_llm_inference_effect/handlers/bifrost/test_config_loader_bifrost.py (1)
59-60: Strengthen this routing-rule assertion to match the test intent.
len(config.routing_rules) >= 4is too loose for the stated expectation. Assert the required operation types directly so regressions are caught.✅ Suggested assertion hardening
- # Should have routing rules for premium, standard, cheap, embedding, reasoning, eval - assert len(config.routing_rules) >= 4 + operation_types = { + op + for rule in config.routing_rules + for op in rule.match_operation_types + } + assert {"chat_completion", "completion", "embedding", "reasoning", "eval"} <= operation_types🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/nodes/node_llm_inference_effect/handlers/bifrost/test_config_loader_bifrost.py` around lines 59 - 60, Replace the loose length check on config.routing_rules with explicit assertions that the expected operation types exist: inspect config.routing_rules (e.g., iterate over each rule in config.routing_rules and collect rule.operation_type) and assert that the set contains "premium", "standard", "cheap", "embedding", "reasoning", and "eval" (or assert equality/expected superset as appropriate) so the test fails if any required routing rule is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/omnibase_infra/enums/enum_eval_finding_category.py`:
- Around line 16-28: The package-level export is missing the new
EnumEvalFindingCategory so consumers cannot do from omnibase_infra.enums import
EnumEvalFindingCategory; update the enums package aggregator by importing
EnumEvalFindingCategory (the class name) into the package __init__ and add
"EnumEvalFindingCategory" to the module __all__ list alongside the other enum
names so it is exported at package level; ensure the import is added at top and
the __all__ list includes the exact string "EnumEvalFindingCategory".
In
`@src/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/config_loader_bifrost.py`:
- Around line 125-130: The debug log currently prints the full backend URL
(which can contain secrets) after creating a ModelBifrostBackendConfig; change
the logger.debug call that references backend_id, env_var, and url so it does
NOT include the URL value—log only the backend_id and env_var (or a masked
placeholder) when adding the backend (reference ModelBifrostBackendConfig,
backend_id, env_var and the logger.debug call) to avoid leaking credentials in
environment values.
In `@src/omnibase_infra/runtime/util_container_wiring.py`:
- Around line 455-459: Replace the raised ServiceRegistrationError message that
interpolates the raw exception (f"Failed to wire LLM handlers: {e}") with a
stable, sanitized message like "Failed to wire LLM handlers" and avoid embedding
str(e) in user-facing fields; set original_error to a non-sensitive identifier
such as type(e).__name__ or a masked placeholder and keep
error_type=type(e).__name__ as-is, and emit the full exception to an
internal/debug log (e.g., logger.exception or logger.debug with the exception
object) before raising so diagnostics remain available without exposing secrets
in ServiceRegistrationError (apply this change where ServiceRegistrationError is
raised in util_container_wiring.py around the LLM wiring block and ensure
context is preserved).
---
Outside diff comments:
In `@src/omnibase_infra/nodes/node_llm_inference_effect/contract.yaml`:
- Around line 10-27: The contract description claims delegation to GLM/Gemini
but the handler routing only declares inference.openai_compatible; update
contract.yaml to make behavior authoritative by either (A) adding explicit
handler entries for GLM and Gemini under handler_routing.handlers (e.g.,
identifiers for inference.glm and inference.gemini) and any corresponding
routing rules, or (B) change the description text in the node_name/description
block of node_llm_inference_effect to state that only an OpenAI-compatible
handler (inference.openai_compatible) is declared and that other providers
(GLM/Gemini) are supported only via upstream/backing handlers, ensuring the
phrasing matches the declared handlers exactly; reference the contract fields
contract_name/node_name "node_llm_inference_effect" and handler_routing.handlers
to locate and update the YAML.
---
Nitpick comments:
In `@src/omnibase_infra/runtime/util_container_wiring.py`:
- Around line 391-394: The module exposes a public wiring function
wire_llm_handlers but it isn’t included in the module's exported symbol list
(__all__), causing API inconsistency; update the module-level __all__ to include
"wire_llm_handlers" (add the exact string to the existing list/tuple) so it is
exported alongside the other wiring helpers—locate the __all__ declaration near
the top of the file and append "wire_llm_handlers" to it.
In `@tests/unit/models/eval/test_model_eval_task.py`:
- Around line 66-72: The tests (e.g., test_frozen for ModelEvalTask) currently
catch a broad Exception; update them to assert the specific
pydantic.ValidationError raised on mutation by replacing
pytest.raises(Exception) with pytest.raises(ValidationError) and add the
required import from pydantic (ValidationError); apply the same change to the
other frozen-model tests referenced (around the blocks at 88-95 and 143-146) so
each test precisely expects ValidationError from attempted attribute mutation.
In
`@tests/unit/nodes/node_llm_inference_effect/handlers/bifrost/test_config_loader_bifrost.py`:
- Around line 59-60: Replace the loose length check on config.routing_rules with
explicit assertions that the expected operation types exist: inspect
config.routing_rules (e.g., iterate over each rule in config.routing_rules and
collect rule.operation_type) and assert that the set contains "premium",
"standard", "cheap", "embedding", "reasoning", and "eval" (or assert
equality/expected superset as appropriate) so the test fails if any required
routing rule is missing.
🪄 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: 8091b9f8-41fa-4c51-a1a6-36558388b6fa
📒 Files selected for processing (19)
src/omnibase_infra/enums/enum_eval_finding_category.pysrc/omnibase_infra/enums/enum_eval_finding_severity.pysrc/omnibase_infra/enums/enum_eval_task_status.pysrc/omnibase_infra/enums/enum_eval_task_type.pysrc/omnibase_infra/models/eval/__init__.pysrc/omnibase_infra/models/eval/model_eval_budget_cap.pysrc/omnibase_infra/models/eval/model_eval_finding.pysrc/omnibase_infra/models/eval/model_eval_result.pysrc/omnibase_infra/models/eval/model_eval_task.pysrc/omnibase_infra/nodes/node_llm_embedding_effect/contract.yamlsrc/omnibase_infra/nodes/node_llm_inference_effect/contract.yamlsrc/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/config_loader_bifrost.pysrc/omnibase_infra/runtime/util_container_wiring.pysrc/omnibase_infra/topics/service_topic_registry.pysrc/omnibase_infra/topics/topic_keys.pysrc/omnibase_infra/validation/infra_validators.pytests/unit/models/eval/__init__.pytests/unit/models/eval/test_model_eval_task.pytests/unit/nodes/node_llm_inference_effect/handlers/bifrost/test_config_loader_bifrost.py
…icts Keep HEAD's extended config loader (GLM/Gemini/eval routing) and tests. Incorporate main's event bus topic sections in LLM contracts, and keep both EVAL_COMPLETED and ERROR_TRIAGED in topic_keys __all__. Remove duplicate wire_llm_handlers from main (HEAD version is superset).
jonahgabriel
enabled auto-merge
March 30, 2026 17:30
- Fix test_wire_with_publisher assertion to match actual service name - Update topic key count assertion from 24 to 25 - Update INFRA_MAX_UNIONS assertion from 132 to 133 - Export eval enums from enums package __init__ - Remove raw URL from bifrost config loader debug log - Sanitize ServiceRegistrationError message
Combine both ModelEval* and ModelAutoEval* exports in the eval models package __init__.py.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/omnibase_infra/enums/__init__.py`:
- Around line 156-159: Add the four new eval enums to the module docstring
"Exports:" list so documentation matches the actual exports: include brief
descriptions for EnumEvalFindingCategory, EnumEvalFindingSeverity,
EnumEvalTaskStatus, and EnumEvalTaskType in the "Exports:" section (insert
alphabetically near the other Enum entries) following the same one-line
description style used for existing enums.
In
`@src/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/config_loader_bifrost.py`:
- Around line 87-93: The error raised when no LLM backends are configured (the
backends check in config_loader_bifrost.py) lists only four env var names;
update the ValueError message to enumerate all valid backend environment
variables so users know their options. Specifically, include LLM_SMALL_URL plus
the external-provider vars GLM_BASE_URL and GEMINI_API_KEY alongside the
existing LLM_CODER_URL, LLM_CODER_FAST_URL, LLM_EMBEDDING_URL, and
LLM_DEEPSEEK_R1_URL in the message emitted where backends is empty.
- Around line 194-220: The routing rules for _RULE_UUID_REASONING and
_RULE_UUID_EVAL use string operation types ("reasoning", "eval") that will never
match the request.operation_type because EnumLlmOperationType only defines
CHAT_COMPLETION, COMPLETION, and EMBEDDING; update the code by either (A) adding
REASONING and EVAL to EnumLlmOperationType so request.operation_type can be
those values, or (B) convert the ModelBifrostRoutingRule entries (the rules
appended for _RULE_UUID_REASONING and _RULE_UUID_EVAL) to use existing enum
members (e.g., EnumLlmOperationType.CHAT_COMPLETION / COMPLETION) or another
matching mechanism (match_model_patterns or a custom field) and remove the
unsupported string literals—make the change consistently where
match_operation_types is consumed (the handler that checks
request.operation_type) so rules can actually match.
🪄 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: 7168b598-439d-405d-9160-2c54a3a36a79
📒 Files selected for processing (6)
src/omnibase_infra/enums/__init__.pysrc/omnibase_infra/nodes/node_llm_inference_effect/handlers/bifrost/config_loader_bifrost.pysrc/omnibase_infra/runtime/util_container_wiring.pytests/unit/nodes/node_llm_inference_effect/test_wire_llm_handlers.pytests/unit/topics/test_service_topic_registry.pytests/unit/validation/test_validator_defaults.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/topics/test_service_topic_registry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_infra/runtime/util_container_wiring.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/omnibase_infra/enums/__init__.py (1)
13-60:⚠️ Potential issue | 🟡 MinorSync the module docstring “Exports” list with the new eval enums.
The docstring export inventory is now stale relative to the new public exports on Line 89-Line 92 and Line 158-Line 161.
📝 Suggested docstring patch
EnumEnvironment: Deployment environment classification (DEVELOPMENT, STAGING, PRODUCTION, CI) + EnumEvalFindingCategory: Evaluation finding category classification + EnumEvalFindingSeverity: Evaluation finding severity levels + EnumEvalTaskStatus: Evaluation task status states + EnumEvalTaskType: Evaluation task type classification EnumExecutionShapeViolation: Specific execution shape violation types🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/enums/__init__.py` around lines 13 - 60, The module docstring "Exports" list is out of sync: add the newly exported eval enums to that docstring (the public Enum names that include "Eval" in their identifiers) so the docstring matches the actual public exports; locate the new Enum declarations by searching for enum identifiers containing "Eval" in this file and append concise descriptions for each to the Exports section, keeping naming and capitalization identical to the Enum class names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/omnibase_infra/enums/__init__.py`:
- Around line 13-60: The module docstring "Exports" list is out of sync: add the
newly exported eval enums to that docstring (the public Enum names that include
"Eval" in their identifiers) so the docstring matches the actual public exports;
locate the new Enum declarations by searching for enum identifiers containing
"Eval" in this file and append concise descriptions for each to the Exports
section, keeping naming and capitalization identical to the Enum class names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab45e478-303a-4a55-b1ac-44448c4bca19
📒 Files selected for processing (2)
src/omnibase_infra/enums/__init__.pysrc/omnibase_infra/models/eval/__init__.py
jonahgabriel
added this pull request to the merge queue
Mar 30, 2026
This was referenced Apr 2, 2026
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
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
wire_llm_handlers()inutil_container_wiring.py, registering inference and embedding handlers with optional metrics publisher supportconfig_loader_bifrost.pyto support external providers (GLM viaGLM_BASE_URL, Gemini viaGEMINI_API_KEY/GEMINI_BASE_URL), eval task routing rules, and cost-tier-based routing (premium/standard/cheap)ModelEvalTask,ModelEvalResult,ModelEvalFinding,ModelEvalBudgetCapwith typed enums for status (EnumEvalTaskStatus), task type (EnumEvalTaskType), finding severity (EnumEvalFindingSeverity), and category (EnumEvalFindingCategory)LLM_INFERENCE_REQUEST,LLM_EMBEDDING_REQUEST,EVAL_COMPLETEDin topic keys and service topic registrywire_llm_handlersreturn type)Overlap Note
OMN-6795 (in a separate worktree) also introduces eval models under
models/eval/withenum_auto_eval_task_type.py. The two branches share themodels/eval/directory but use different enum/model naming. Whichever merges second will need a reconciliation pass.Test plan
Summary by CodeRabbit
Release Notes
New Features