Repository navigation
chore(release)(OMN-12245): backmerge omnimarket v0.4.3 version bump to dev - #1070
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
Warning Review limit reached
More reviews will be available in 1 minute and 5 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR updates CI and pre-commit enforcement, expands delegation and runtime contract handling, adds shared event and wire models, introduces multiple new node packages and CLI features, refreshes routing/model configuration, and records related work-tracking and evidence artifacts. ChangesRuntime contracts and node expansion
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/omnimarket/nodes/node_delegation_quality_gate_reducer/handlers/handler_quality_gate.py (1)
137-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNon-Python fenced blocks can cause false deterministic failures.
Lines 202–207 parse every fenced block as Python regardless of fence language. Mixed outputs containing
```jsonor```bashcan failcompiles_without_errorseven when Python artifact blocks are valid.♻️ Suggested parser narrowing
-_MARKDOWN_FENCE_RE = re.compile(r"```(?:\w+)?\n(.*?)```", re.DOTALL) +_MARKDOWN_FENCE_RE = re.compile(r"```(?P<lang>\w+)?\n(?P<body>.*?)```", re.DOTALL) -def _extract_fenced_code_blocks(content: str) -> list[str]: - """Return all fenced code block bodies from mixed content.""" - return _MARKDOWN_FENCE_RE.findall(content) +def _extract_fenced_code_blocks(content: str) -> list[tuple[str, str]]: + """Return (lang, body) tuples for fenced code blocks.""" + out: list[tuple[str, str]] = [] + for m in _MARKDOWN_FENCE_RE.finditer(content): + out.append(((m.group("lang") or "").lower(), m.group("body"))) + return out @@ - blocks = _extract_fenced_code_blocks(content) - candidates = blocks if blocks else [_strip_markdown_code_fence(content)] + blocks = _extract_fenced_code_blocks(content) + py_blocks = [body for lang, body in blocks if lang in ("", "py", "python")] + candidates = py_blocks if py_blocks else [_strip_markdown_code_fence(content)]Also applies to: 195-209
🤖 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_delegation_quality_gate_reducer/handlers/handler_quality_gate.py` around lines 137 - 154, The current regex and extractor treat all fenced blocks as Python which causes non-Python blocks (e.g., ```json```, ```bash```) to be tested; update _MARKDOWN_FENCE_RE to capture a named lang group and body (e.g., r"```(?P<lang>\w+)?\n(?P<body>.*?)```", re.DOTALL), change _extract_fenced_code_blocks to return list[tuple[str,str]] of (lang, body) using finditer and .group("lang")/.group("body"), and then when building candidates filter blocks to only Python-friendly fences (lang in ("", "py", "python")) producing py_blocks and falling back to _strip_markdown_code_fence(content) if none.src/omnimarket/adapters/claude_code/delegate.py (1)
136-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
max_tokensdefaults across all public entry points.Line 136 now uses
DELEGATION_DEFAULT_MAX_TOKENS, but Line 252 and Line 290 still hardcode2048. Programmatic callers ofcompile_request()/dispatch_sync()can now diverge from CLI and wire defaults.Suggested patch
@@ - max_tokens: int = 2048, + max_tokens: int = DELEGATION_DEFAULT_MAX_TOKENS, @@ - max_tokens: int = 2048, + max_tokens: int = DELEGATION_DEFAULT_MAX_TOKENS,🤖 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/adapters/claude_code/delegate.py` around lines 136 - 152, Defaults for max_tokens are inconsistent: replace the hardcoded 2048 values used in compile_request and dispatch_sync with the declared DELEGATION_DEFAULT_MAX_TOKENS constant so all public entry points share the same default; update the parameters/signatures in compile_request and dispatch_sync to use DELEGATION_DEFAULT_MAX_TOKENS (and remove any literal 2048 in their bodies), ensure the constant is imported/available in delegate.py, and run tests to verify behavior remains unchanged.src/omnimarket/nodes/node_delegation_orchestrator/plugin.py (1)
202-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the
service_registryhard gate from deferred dispatcher wiring.With dispatcher wiring now deferred, this method still returns
skippedwhenservice_registryis absent, which leaves_dispatcher_wiring_succeededfalse and preventsstart_consumers()from running in no-registry environments even though that path already has fallback logic.💡 Proposed fix
async def wire_dispatchers( self, config: ModelDomainPluginConfig, ) -> ModelDomainPluginResult: """Defer dispatcher route registration to contract auto-wiring.""" start_time = time.time() - if config.container.service_registry is None: - logger.warning( - "DEGRADED_MODE: ServiceRegistry not available, skipping " - "delegation dispatcher wiring (correlation_id=%s)", - config.correlation_id, - ) - return ModelDomainPluginResult.skipped( - plugin_id=self.plugin_id, - reason="ServiceRegistry not available", - ) - if config.dispatch_engine is None: logger.warning( "DEGRADED_MODE: dispatch_engine not available, skipping " "delegation dispatcher wiring (correlation_id=%s)", config.correlation_id,🤖 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_delegation_orchestrator/plugin.py` around lines 202 - 239, Remove the hard gate that returns skipped when config.container.service_registry is None so deferred dispatcher wiring does not block start_consumers(); specifically, delete or bypass the block that checks config.container.service_registry and returns ModelDomainPluginResult.skipped, but keep the existing dispatch_engine check; if service_registry is None, emit a warning log about degraded mode (as currently done) yet continue to set self._dispatcher_wiring_succeeded = True, compute duration, log the deferred-wiring info, and return a successful ModelDomainPluginResult (same shape as the existing success return) so start_consumers() can run in no-registry environments.src/omnimarket/nodes/node_contract_registry/handlers/handler_contract_registry.py (1)
90-97:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate top-level
handler.moduleagainst the allowlist before adapter derivation.The allowlist check currently inspects only
handler_routing.handlers[].handler.module, but adapter derivation prioritizes top-levelhandler.module. A contract can bypass allowlist enforcement and still publish an adapted handler class.🔧 Suggested fix
- handler_routing = parsed.get("handler_routing", {}) - handlers = handler_routing.get("handlers", []) - for entry in handlers: - handler = entry.get("handler", {}) if isinstance(entry, dict) else {} - module = handler.get("module", "") or entry.get("handler_module", "") - if module and not any( - module.startswith(p) for p in ALLOWED_HANDLER_PREFIXES - ): - return self._reject( - request, EnumMaterializationRejection.HANDLER_ALLOWLIST - ) + modules_to_check: list[str] = [] + top_handler = parsed.get("handler") + if isinstance(top_handler, dict): + top_module = top_handler.get("module") + if isinstance(top_module, str) and top_module: + modules_to_check.append(top_module) + + handler_routing = parsed.get("handler_routing", {}) + handlers = handler_routing.get("handlers", []) + for entry in handlers: + if not isinstance(entry, dict): + continue + handler = entry.get("handler", {}) + if isinstance(handler, dict): + module = handler.get("module", "") + if isinstance(module, str) and module: + modules_to_check.append(module) + module_fallback = entry.get("handler_module", "") + if isinstance(module_fallback, str) and module_fallback: + modules_to_check.append(module_fallback) + + for module in modules_to_check: + if not any(module.startswith(p) for p in ALLOWED_HANDLER_PREFIXES): + return self._reject( + request, EnumMaterializationRejection.HANDLER_ALLOWLIST + )Also applies to: 141-143
🤖 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_contract_registry/handlers/handler_contract_registry.py` around lines 90 - 97, The code currently only checks handler_routing.handlers[].handler.module against ALLOWED_HANDLER_PREFIXES but later derives/adapts using the top-level handler.module, allowing bypass; update the validation so that before any adapter derivation you normalize and check the top-level module value (the computed variable module derived from parsed -> handler_routing -> handlers -> entry -> handler) against ALLOWED_HANDLER_PREFIXES and reject/raise/log when it’s not allowed; apply the same fix to the other derivation path mentioned (the code around the second adapter derivation referenced in the comment) so both places use the normalized top-level module variable and the same allowlist check before proceeding with adapter/class import.src/omnimarket/nodes/node_delegate_skill_orchestrator/contract.yaml (1)
80-127:⚠️ Potential issue | 🟠 MajorFix duplicated top-level
inputsin contract YAML
src/omnimarket/nodes/node_delegate_skill_orchestrator/contract.yamldefinesinputstwice (at lines 22 and 80). Most YAML loaders keep only one value for duplicate keys (commonly the last), which can silently drop part of the contract schema. Remove one block and merge all fields into a singleinputsmapping.🤖 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_delegate_skill_orchestrator/contract.yaml` around lines 80 - 127, The contract YAML currently defines the top-level mapping "inputs" twice which causes one block to override the other; open the contract and merge the two "inputs" blocks into a single "inputs" mapping containing all fields (prompt, task_type, source, cwd, source_file_path, working_directory, session_id, recipient, codex_sandbox_mode, wait, max_tokens) and remove the duplicate "inputs" key so no fields are silently dropped by YAML loaders.
🟡 Minor comments (18)
src/omnimarket/nodes/node_delegation_orchestrator/dispatchers/topic_utils.py-16-18 (1)
16-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten canonical-topic validation before rewriting
event_type.Current parsing rewrites any
onex.*topic with 5+ segments, including non-canonical formats, which contradicts the function contract and can mislabel envelope event types.💡 Proposed fix
def derive_event_type_from_topic(topic: str) -> str: @@ parts = topic.split(".") - if len(parts) >= 5 and parts[0] == "onex": + if ( + len(parts) == 5 + and parts[0] == "onex" + and parts[1] in {"cmd", "evt", "dlq"} + and parts[4].startswith("v") + and parts[4][1:].isdigit() + and parts[2] + and parts[3] + ): return f"{parts[2]}.{parts[3]}" return topic🤖 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_delegation_orchestrator/dispatchers/topic_utils.py` around lines 16 - 18, The current rewrite rewrites any topic with parts[0]=="onex" and len(parts)>=5; tighten this by only rewriting when the topic exactly matches the expected canonical ONEX pattern: require len(parts) == 5, parts[0] == "onex", and validate that the other segments (parts[1], parts[2], parts[3], parts[4]) are non-empty and only contain allowed characters (e.g. alphanumeric, hyphen, underscore, dot) before returning f"{parts[2]}.{parts[3]}"; otherwise return the original topic unchanged (use the existing variables parts and topic to locate the logic).docs/work-tracking/contracts/OMN-12570.yaml-68-77 (1)
68-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the DoD check prove the
MERGING -> POST_MERGE_TAILedge.The description promises that exact transition, but the command only checks for
post_merge_tail,transitions_recorded: true, andPOST_MERGE_TAIL. A contract missing the actualMERGING -> POST_MERGE_TAILtransition would still pass this evidence step.🤖 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 `@docs/work-tracking/contracts/OMN-12570.yaml` around lines 68 - 77, The DoD check "dod-contract-phase-declaration" currently verifies only presence of post_merge_tail, transitions_recorded: true, and POST_MERGE_TAIL but does not assert the actual MERGING -> POST_MERGE_TAIL transition; update the check_value for that check (in the checks array under id "dod-contract-phase-declaration") to include a grep (or equivalent) that looks for the exact edge string representing the transition from MERGING to POST_MERGE_TAIL (e.g., a pattern matching "MERGING.*POST_MERGE_TAIL" or the exact "MERGING -> POST_MERGE_TAIL" text) so the contract must contain the explicit MERGING -> POST_MERGE_TAIL transition to pass.src/omnimarket/configs/bifrost_delegation.yaml-99-105 (1)
99-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the existing
documentationcapability name.
cloud-gemini-flashis the only backend here usingdocumentwhile the rest of the config and routing rules usedocumentation. Any capability matcher keyed ondocumentationwill never consider this backend.💡 Proposed fix
- - document + - documentation🤖 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/configs/bifrost_delegation.yaml` around lines 99 - 105, Replace the incorrect capability key "document" with the existing "documentation" in the capabilities list for the cloud-gemini-flash backend so it matches the rest of the routing and capability matchers; update the capabilities entry under the backend where you see capabilities: [..., document, ...] to use "documentation" instead so matchers keyed on documentation will include this backend.src/omnimarket/adapters/codex/local_runtime_dispatch.py-405-410 (1)
405-410:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
terminal_eventsshape before dict access.Line 405-Line 410 uses
cast(...)without runtime validation. Ifterminal_eventsis not a mapping,.get(...)on Line 426/Line 443 raises anAttributeErrorinstead of surfacing a clear contract-validation error.💡 Proposed fix
- terminals = cast( - dict[str, Any], - runtime_dispatch.get("terminal_events") - or contract.get("terminal_events") - or {}, - ) + terminals_raw = ( + runtime_dispatch.get("terminal_events") + or contract.get("terminal_events") + or {} + ) + if not isinstance(terminals_raw, dict): + raise ValueError("Contract terminal_events must be a mapping") + terminals = cast(dict[str, Any], terminals_raw)Also applies to: 426-427, 443-443
🤖 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/adapters/codex/local_runtime_dispatch.py` around lines 405 - 410, The code assigns terminals via cast(...) from runtime_dispatch.get("terminal_events") or contract.get("terminal_events") without runtime validation; add an explicit type/shape check after computing the raw value (e.g., terminals_raw = runtime_dispatch.get("terminal_events") or contract.get("terminal_events") or {}) and ensure it is a mapping/dict (has items()/get attribute or isinstance(..., dict)); if it is not, raise a clear ContractValidationError (or ValueError) mentioning "terminal_events" and include the offending value; then set terminals = cast(dict[str, Any], terminals_raw) and proceed, and apply the same validation pattern where terminal_events is accessed later (the sites referenced around get calls in local_runtime_dispatch.py that use terminals.get(...)).src/omnimarket/configs/routing_tiers.yaml-102-107 (1)
102-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate tier-order comments to reflect
cheap_frontier.This file now includes a
cheap_frontiertier, but the header docs still describe order aslocal → cheap_cloud → claude. Please align the comment to avoid operator confusion during tuning.🤖 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/configs/routing_tiers.yaml` around lines 102 - 107, The header documentation above the tier definitions still documents the tier order as "local → cheap_cloud → claude" but the new tier cheap_frontier has been added; update the header comment to include cheap_frontier in the correct ordering (e.g., "local → cheap_cloud → cheap_frontier → claude" or whatever intended precedence) so operators see the true routing precedence when tuning, and ensure any descriptive text that mentions the free-tier/OPENROUTER_API_KEY behavior references cheap_frontier by name.src/omnimarket/adapters/llm/adapter_llm_caller_delegation.py-97-100 (1)
97-100:⚠️ Potential issue | 🟡 MinorPropagate auth (api_key_ref/extra_headers) in
LlmCallerDelegation, or ensure it’s never used in the live runtime path
src/omnimarket/adapters/llm/adapter_llm_caller_delegation.pyconstructs the provider like this (auth is never forwarded—onlyprovider_typeflips):api_key=None, provider_name=f"delegation-{intent.model}", provider_type="local" if intent.api_key_ref is None else "external_trusted",In the live delegation runtime path,
node_llm_delegation_call_effect/handlers/handler_inference_intent.pydoes resolveintent.api_key_refinto anAuthorization: Bearer ...header and mergesintent.extra_headersbefore the outbound HTTP call. So the “unauthenticated calls” risk is only relevant ifLlmCallerDelegationis actually used in production wiring (e.g., viaDelegationIntentBridgedirect mode).Either wire
LlmCallerDelegationso it forwards the resolved auth/headers (or remove/adjust the misleadingexternal_trustedflip), or add a clear guarantee/documentation that this adapter is never used where auth must be forwarded.🤖 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/adapters/llm/adapter_llm_caller_delegation.py` around lines 97 - 100, LlmCallerDelegation currently constructs the provider with api_key=None and only flips provider_type, which drops intent.auth (api_key_ref) and intent.extra_headers; update LlmCallerDelegation to propagate resolved auth by setting the provider's api_key (or authorization header) and merging extra_headers when building the provider (the code that sets api_key=None, provider_name=f"delegation-{intent.model}", provider_type=...) so outbound calls retain Authorization: Bearer ... and any intent.extra_headers, or if you cannot forward auth, explicitly remove/rename the external_trusted behavior and add a clear runtime guard/documentation in DelegationIntentBridge and node_llm_delegation_call_effect/handlers/handler_inference_intent to ensure LlmCallerDelegation is never used in flows requiring auth.src/omnimarket/events/generation.py-39-39 (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate SHA-256 format, not only length.
Line 39 currently accepts any 64-character string. Add a hex pattern check so malformed digests are rejected at model-parse time.
💡 Proposed patch
- content_sha256: str = Field(min_length=64, max_length=64) + content_sha256: str = Field( + min_length=64, + max_length=64, + pattern=r"^[0-9a-fA-F]{64}$", + )🤖 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/events/generation.py` at line 39, The Field for content_sha256 currently only enforces length; update the Pydantic Field for content_sha256 to validate hex characters by adding a regex/pattern that only allows 64 hexadecimal characters (0-9, a-f, A-F) so malformed digests are rejected during model parsing; modify the declaration of content_sha256: str = Field(min_length=64, max_length=64) in the events/generation.py model to include the hex pattern (e.g., a pattern matching exactly 64 hex chars) and keep length constraints consistent with the pattern.src/omnimarket/events/design_to_plan.py-34-36 (1)
34-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject negative completion counters.
Line 34-Line 36 should enforce non-negative values; otherwise invalid terminal events can pass validation.
Proposed fix
-from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, Field @@ - created_count: int = 0 - skipped_count: int = 0 - failed_count: int = 0 + created_count: int = Field(default=0, ge=0) + skipped_count: int = Field(default=0, ge=0) + failed_count: int = Field(default=0, ge=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/events/design_to_plan.py` around lines 34 - 36, The three counter fields created_count, skipped_count, and failed_count must never be negative; add validation where the event object is constructed (e.g., the event class's __post_init__ or constructor in src/omnimarket/events/design_to_plan.py) to check each of created_count, skipped_count and failed_count and raise a ValueError (or otherwise reject the event) if any value < 0; alternatively implement property setters or a validation helper that enforces non-negative integers for these three symbols before the event is accepted.src/omnimarket/models/swarm/model_swarm_subtask_result.py-18-18 (1)
18-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConstrain
latency_msto non-negative values.Line 18 currently accepts negative latency, which is semantically invalid for execution telemetry.
Proposed fix
-from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, Field @@ - latency_ms: int = 0 + latency_ms: int = Field(default=0, ge=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/models/swarm/model_swarm_subtask_result.py` at line 18, The latency_ms field currently allows negative values; update the model to enforce latency_ms >= 0 by adding validation: if this is a dataclass implement a __post_init__ that raises ValueError when self.latency_ms < 0, or if the model inherits from pydantic.BaseModel use Field(ge=0) or a `@validator` for "latency_ms" to reject negatives; reference the latency_ms attribute and the model's constructor/validation hook (e.g., __post_init__ or pydantic validator) when making the change.src/omnimarket/events/demo_pipeline.py-58-62 (1)
58-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd validation for
cheapest_model_id/costsinvariants.Line 58-Line 62 allows inconsistent states that the description explicitly disallows, which can break downstream model selection logic.
Proposed fix
-from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator @@ class ModelDemoCostResult(BaseModel): @@ cheapest_model_id: str | None = Field( default=None, description="model_id with the lowest total_cost_usd; null when costs list is empty", ) + + `@model_validator`(mode="after") + def _validate_cheapest_model_id(self) -> "ModelDemoCostResult": + if not self.costs: + if self.cheapest_model_id is not None: + raise ValueError("cheapest_model_id must be null when costs is empty") + return self + valid_ids = {entry.model_id for entry in self.costs} + if self.cheapest_model_id is None or self.cheapest_model_id not in valid_ids: + raise ValueError("cheapest_model_id must reference one of the costs[].model_id values") + return self🤖 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/events/demo_pipeline.py` around lines 58 - 62, Add a Pydantic validation that enforces the invariant between costs: list[ModelDemoCostEntry] and cheapest_model_id: ensure that when costs is empty cheapest_model_id is None, and when costs is non-empty cheapest_model_id is non-None and equals the model_id of the ModelDemoCostEntry with the lowest total_cost_usd (and that the id exists in costs). Implement this as a validator/root_validator on the model that references the costs field and cheapest_model_id to compute the min total_cost_usd and either raise a ValidationError or set/normalize cheapest_model_id accordingly; use the existing field names costs, ModelDemoCostEntry, and cheapest_model_id to locate and modify the class.src/omnimarket/events/demo.py-48-52 (1)
48-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnforce
cheapest_model_idconsistency withcosts.Line 48-Line 52 documents an invariant but does not validate it, so invalid payloads can pass (e.g., unknown
cheapest_model_idor non-null value with empty costs).Proposed fix
-from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator @@ class ModelDemoCostResult(BaseModel): @@ cheapest_model_id: str | None = Field( default=None, description="model_id with the lowest total_cost_usd; null when costs list is empty", ) + + `@model_validator`(mode="after") + def _validate_cheapest_model_id(self) -> "ModelDemoCostResult": + if not self.costs: + if self.cheapest_model_id is not None: + raise ValueError("cheapest_model_id must be null when costs is empty") + return self + valid_ids = {entry.model_id for entry in self.costs} + if self.cheapest_model_id is None or self.cheapest_model_id not in valid_ids: + raise ValueError("cheapest_model_id must reference one of the costs[].model_id values") + return self🤖 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/events/demo.py` around lines 48 - 52, Add a Pydantic validator on the model that defines costs and cheapest_model_id to enforce the documented invariant: if costs is empty then cheapest_model_id must be None, and if costs is non-empty then cheapest_model_id must be non-null and equal to one of the model_id values in the costs list. Implement this as a `@root_validator` (or a validator for "cheapest_model_id" that accesses "costs") inside the same class containing the costs: list[ModelDemoCostEntry] and cheapest_model_id: str | None Field, and raise a ValueError with a clear message when the invariant is violated.src/omnimarket/nodes/node_decision_store_orchestrator/handlers/handler_decision_store_orchestrator.py-102-102 (1)
102-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve valid falsy cursors in query responses.
Line 102 uses a truthy check, so a valid cursor like
0is converted toNone, breaking pagination continuity.🔧 Proposed fix
- next_cursor=str(next_cursor) if next_cursor else None, + next_cursor=str(next_cursor) if next_cursor is not None else None,🤖 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_decision_store_orchestrator/handlers/handler_decision_store_orchestrator.py` at line 102, The response builder currently uses a truthy check on next_cursor which converts valid falsy cursors (e.g., 0) to None; in the handler_decision_store_orchestrator (handler_decision_store_orchestrator.py) locate the expression that sets next_cursor to "str(next_cursor) if next_cursor else None" and change it to use an explicit None check so that next_cursor is converted to a string when it is not None (preserving values like 0) and set to None only when it is actually None.src/omnimarket/nodes/node_decision_store_orchestrator/__init__.py-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale “STUB: not yet implemented” wording from the module docstring.
This now conflicts with the contract state (
node_not_implemented: false) and can cause operator confusion.🤖 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_decision_store_orchestrator/__init__.py` at line 1, Update the module docstring in node_decision_store_orchestrator's __init__.py to remove the stale phrase "STUB: not yet implemented" so it no longer contradicts the module's implemented state; keep the descriptive portion ("conflict check, LLM review, persist, notify for architectural decisions.") and adjust the docstring to present the current functionality without implying it's unimplemented.src/omnimarket/nodes/node_data_flow_sweep/__main__.py-137-139 (1)
137-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--collect --flows "[]"is currently rejected, but should be allowed.This check treats any non-empty
--flowsstring as conflicting, including"[]". That contradicts the non-empty-list exclusivity documented in this flow and contract behavior.Suggested fix
- if args.collect and args.flows: - _log.error("--collect and --flows are mutually exclusive") - sys.exit(1) + if args.collect and args.flows: + try: + maybe_flows = json.loads(args.flows) + except json.JSONDecodeError as exc: + _log.error("invalid --flows JSON: %s", exc) + sys.exit(1) + if maybe_flows: + _log.error("--collect and non-empty --flows are mutually exclusive") + sys.exit(1)🤖 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_data_flow_sweep/__main__.py` around lines 137 - 139, The mutual-exclusion check incorrectly rejects the string "[]" as a non-empty flows value; update the check in __main__.py to allow an empty list representation by treating args.flows == "[]" (or JSON-parsing it and checking len == 0) as not conflicting with args.collect. Concretely, modify the if condition that references args.collect and args.flows so it only errors when args.collect is true AND args.flows represents a non-empty list (e.g., args.flows not None/empty and not equal to "[]" or json.loads(args.flows) yields length > 0), keeping the error and sys.exit(1) behavior otherwise.src/omnimarket/nodes/node_create_followup_tickets_effect/models/model_create_followup_tickets_state.py-40-43 (1)
40-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
line_numberas positive when provided.
line_numbercurrently accepts0/negative values, which can generate invalid source locations in downstream ticket payloads.🛠️ Proposed fix
line_number: int | None = Field( default=None, + ge=1, description="Line number within the source file.", )🤖 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_create_followup_tickets_effect/models/model_create_followup_tickets_state.py` around lines 40 - 43, The line_number Field in the model (model_create_followup_tickets_state.py) must be validated as a positive integer when provided; update the Field definition for line_number to include a minimum constraint (e.g., Field(default=None, ge=1, description="Line number within the source file.")) or add a pydantic validator on the model class to raise an error when line_number is not None and is < 1 so downstream payloads never receive 0/negative values.src/omnimarket/nodes/node_dashboard_sweep/handlers/handler_dashboard_sweep.py-324-333 (1)
324-333:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize routes before building probe URLs.
If
extra_routescontains values like"metrics"(no leading slash), Line 331 builds malformed URLs (<base_url>metrics) and silently downgrades recon quality.🛠️ Proposed fix
all_routes: list[str] = [] seen: set[str] = set() - for route in list(discovered) + list(_KNOWN_ROUTES) + list(extra_routes): + def _normalize_route(route: str) -> str: + clean = (route or "").strip().split("?")[0].split("#")[0] + if not clean: + return "/" + return clean if clean.startswith("/") else f"/{clean}" + + for route in ( + _normalize_route(r) + for r in list(discovered) + list(_KNOWN_ROUTES) + list(extra_routes) + ): if route not in seen: seen.add(route) all_routes.append(route) @@ - url = base_url + route + url = urllib.parse.urljoin(base_url + "/", route.lstrip("/")) status_code, content_type, body_size, body_snippet = _fetch_page(url)🤖 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_dashboard_sweep/handlers/handler_dashboard_sweep.py` around lines 324 - 333, The loop builds probe URLs using route values from discovered, _KNOWN_ROUTES and extra_routes without normalizing leading slashes, so entries like "metrics" produce malformed URLs when concatenated with base_url; before adding to seen/all_routes (or immediately before forming url), normalize each route by trimming whitespace, skipping empty strings, and ensuring it starts with a single leading '/' (e.g., if not route.startswith('/'): route = '/' + route) and optionally normalize base_url trailing slash handling so you don't create double slashes; apply this normalization where routes are collected (the loop handling discovered/_KNOWN_ROUTES/extra_routes) so _fetch_page(base_url + route) always receives a valid path and _ERROR_INDICATORS checks remain accurate.src/omnimarket/nodes/node_delegation_ab_runner/handlers/handler_delegation_ab_runner.py-120-125 (1)
120-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix quality-pass default for failed delegated runs.
With
quality_threshold > 0, a call error leavesquality_passed=Trueby default, which misreports failed delegated runs as passing quality.Suggested patch
- quality_score = 0.0 - quality_passed = True + quality_score = 0.0 + quality_passed = quality_threshold <= 0.0 if not error and quality_threshold > 0.0: quality_score = _evaluate_quality(raw_output, quality_threshold) quality_passed = quality_score >= quality_threshold🤖 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_delegation_ab_runner/handlers/handler_delegation_ab_runner.py` around lines 120 - 125, The code sets quality_passed=True by default so when a delegated run returns an error and quality_threshold>0 it incorrectly reports as passing; change the logic so that when quality_threshold>0 and there is an error (the variable error) quality_passed is set to False, otherwise compute quality_score via _evaluate_quality(raw_output, quality_threshold) and set quality_passed = quality_score >= quality_threshold; update the block around quality_score, quality_passed, error, quality_threshold and _evaluate_quality to reflect this conditional.src/omnimarket/nodes/node_delegation_routing_feedback_reducer/models/model_delegation_feedback_event.py-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorDisallow negative
latency_msat schema parse time
ModelDelegationFeedbackEvent.latency_msis an unconstrainedint(= 0), so negative values can still pass Pydantic parsing.HandlerDelegationRoutingFeedbackcurrently only updatesavg_latency_mswhenevent.latency_ms > 0, so negatives don’t affect the reducer average today—but constraininglatency_ms >= 0keeps the event invariant and prevents future downstream misuse.Suggested patch
-from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, Field @@ - latency_ms: int = 0 + latency_ms: int = Field(default=0, ge=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_delegation_routing_feedback_reducer/models/model_delegation_feedback_event.py` at line 37, ModelDelegationFeedbackEvent currently allows negative latency_ms; change its field to enforce non-negative values at parse time by replacing the unconstrained int default with a Pydantic-constrained field (e.g., use Field(..., ge=0) or conint(ge=0)) on the latency_ms attribute in the ModelDelegationFeedbackEvent model so parsing rejects negatives; update imports if necessary (from pydantic import Field or conint) and keep the default/optional behavior consistent with the model (e.g., latency_ms: int = Field(0, ge=0) or latency_ms: conint(ge=0) = 0).
b560d9f to
ab086ca
Compare
✅ 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 — Qwen3-Coder-30B reviewer + DeepSeek-R1 judge (OMN-7963) |
|
| 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)
2 participants
Evidence-Ticket: OMN-12245
Evidence-Source: OCC#2231
Backmerge companion for main release-version PR #1069 so dev retains the v0.4.3 release metadata after main release publication.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration