Repository navigation
feat(nodes): add contract-driven zero-code node base classes [OMN-1731] - #460
Merged
jonahgabriel merged 5 commits intoJan 31, 2026
Conversation
Enable fully declarative ONEX nodes where `class NodeMyEffect(NodeEffect): pass` works entirely from contract.yaml - all behavior driven by contract and handlers. ## New Models - ModelProtocolDependency: Protocol dependency declaration with name, protocol path, required flag, bind_as alias, and lazy_import support - ModelProtocolsNamespace: Immutable namespace for `self.protocols.<name>` access ## New Resolvers - resolver_handler: Resolves handlers from "module:callable" import paths with caching - resolver_protocol_dependency: Resolves protocol deps from container with validation ## Schema Extensions - ModelHandlerRoutingEntry: Added `callable` and `lazy_import` fields - ModelContractBase: Added `protocol_dependencies` list with duplicate detection - ModelHandlerRoutingSubcontract: Added `validate_zero_code_requirements()` method ## Node Updates - NodeCoreBase: Added `protocols` namespace and `_resolve_protocol_dependencies()` - NodeEffect: Auto-loads effect_subcontract, dispatches via `default_handler` - NodeCompute: Dispatches via `default_handler` with generic support ## Error Handling - Added PROTOCOL_CONFIGURATION_ERROR (code 126) for DI failures ## Tests - 52 invariant tests covering contract completeness, protocol deps, namespace immutability, and handler resolution Architectural decisions: - Entry point: handler_routing.default_handler (single dispatch target) - Protocol deps: dedicated protocol_dependencies section with self.protocols namespace - Handlers: import path in contract (no method discovery) - Backward compatible: manual injection takes precedence over contract-driven DI
Contributor
📝 WalkthroughWalkthroughAdds zero-code contract support: new ModelProtocolDependency and ModelProtocolsNamespace, resolvers for protocol dependencies and handlers (eager/lazy, cache), node wiring and contract-driven dispatch, contract accessors, unit tests, a new error code, and a validation-exclusion entry. Changes
Sequence Diagram(s)sequenceDiagram
actor App as Application
participant Node as Node (Compute/Effect)
participant Container as ONEX Container
participant ResPD as Protocol Resolver
participant ResH as Handler Resolver
participant NS as ModelProtocolsNamespace
App->>Node: Initialize node
Node->>Node: _initialize_node_resources()
Node->>ResPD: resolve_protocol_dependencies(deps, container, node_id)
ResPD->>Container: lookup service/class per dependency
Container-->>ResPD: instance or None
ResPD-->>Node: dict(bind_name → instance|None)
Node->>NS: ModelProtocolsNamespace(resolved_dict)
NS-->>Node: frozen namespace
App->>Node: Dispatch work (process)
Node->>NS: lookup default_handler info
NS-->>Node: handler path or LazyLoader
Node->>ResH: resolve_handler(path, eager=...)
ResH-->>Node: HandlerCallable or LazyLoader
Node->>Node: invoke handler (sync/async)
Node-->>App: result (wrapped as output)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
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_core/infrastructure/node_core_base.py (1)
488-533:⚠️ Potential issue | 🟠 MajorNormalize dict-based protocol dependencies before resolution.
contract_dataexplicitly supports dicts, butresolve_protocol_dependencies()expectsModelProtocolDependency. Ifprotocol_dependenciesis a list of dicts, this path will raiseAttributeError(and can bypass required/optional handling). Consider normalizing entries before calling the resolver.🔧 Proposed fix (normalize dict entries + validate)
+from omnibase_core.models.contracts.subcontracts.model_protocol_dependency import ( + ModelProtocolDependency, +) @@ - # Resolve dependencies - resolved = resolve_protocol_dependencies( - protocol_deps, + # Normalize dict-based deps into ModelProtocolDependency + normalized: list[ModelProtocolDependency] = [] + if isinstance(protocol_deps, list): + for dep in protocol_deps: + if isinstance(dep, ModelProtocolDependency): + normalized.append(dep) + elif isinstance(dep, Mapping): + normalized.append(ModelProtocolDependency(**dep)) + else: + raise ModelOnexError( + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + message="Invalid protocol dependency entry", + context={ + "node_id": self.node_id, + "dependency_type": type(dep).__name__, + }, + ) + else: + raise ModelOnexError( + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + message="protocol_dependencies must be a list", + context={"node_id": self.node_id}, + ) + + # Resolve dependencies + resolved = resolve_protocol_dependencies( + normalized, self.container, node_id=str(self.node_id), )
🤖 Fix all issues with AI agents
In `@tests/unit/validation/test_zero_code_invariants.py`:
- Around line 1-9: Trim the module-level docstring to a concise 3–6 line
summary: replace the current long paragraph with a short description (3–6 lines)
that states this file contains tests for zero-code contract-driven node
invariants and briefly notes purpose (enforce architectural invariants for
zero-code nodes), removing extra explanatory sentences; update the top-of-file
module docstring accordingly so it's within the 3–6 line guideline.
- Around line 13-34: Add a pytest marker to categorize these tests as unit tests
by defining a module-level marker variable; since pytest is already imported,
add the line pytestmark = pytest.mark.unit near the top of the file (or
alternatively decorate individual test functions with `@pytest.mark.unit`) so that
the tests using ModelHandlerRoutingEntry, ModelHandlerRoutingSubcontract,
ModelProtocolDependency, ModelSemVer, resolve_handler, and clear_handler_cache
are properly marked.
- Around line 241-322: Add a short explanatory comment above every occurrence of
"# type: ignore[...]" in the tests (e.g., the ignores in
test_protocol_dependency_immutable and TestProtocolsNamespaceImmutability such
as the ignores on dep.name = "NewName", ns.new_attr = "value", ns.logger =
object(), del ns.logger, ns.logger is logger, and _ = ns.event_bus) that briefly
states why the ignore is required and includes a Linear ticket reference (e.g.,
"Reason: testing immutability; Linear: LINEAR-XXXX"); ensure each comment is
immediately above the line with the type ignore and is concise.
🧹 Nitpick comments (4)
src/omnibase_core/nodes/node_compute.py (1)
345-379: Consider tracking timing for contract-driven dispatch.The contract-driven dispatch path sets
processing_time_ms=0.0unconditionally (line 375). If timing service is available, you might want to track the actual execution time for consistency with the standard computation path.This is a minor observation - the current behavior is acceptable if the intent is to distinguish contract-driven dispatch from internal computations in metrics.
💡 Optional: Track timing for contract-driven dispatch
# Wrap raw result in ModelComputeOutput + # Calculate processing time if timing service available + handler_time: float = 0.0 + if self._timing_service is not None and start_time is not None: + handler_time = self._timing_service.stop_timer(start_time) + return ModelComputeOutput( result=result, operation_id=input_data.operation_id, computation_type=input_data.computation_type, - processing_time_ms=0.0, + processing_time_ms=handler_time, cache_hit=False, parallel_execution_used=False, metadata={"contract_driven_dispatch": True}, )src/omnibase_core/models/contracts/subcontracts/model_handler_routing_subcontract.py (1)
193-211: Consider using ModelOnexError for consistency.The
validate_no_duplicate_handler_keysvalidator raisesValueError(line 210), whilevalidate_routing_configurationabove usesModelOnexError. For framework consistency and structured error context, consider usingModelOnexErrorhere as well.Additionally, the duplicate detection uses
keys.count(k)which is O(n²). For handler lists this is typically fine, but a set-based approach would be O(n).♻️ Optional: Consistent error handling and O(n) detection
`@model_validator`(mode="after") def validate_no_duplicate_handler_keys(self) -> Self: """Ensure no duplicate handler_key values in handlers.""" if not self.handlers: return self - keys = [h.handler_key for h in self.handlers] - if len(keys) != len(set(keys)): - duplicates = [k for k in keys if keys.count(k) > 1] - raise ValueError(f"Duplicate handler_key values: {set(duplicates)}") + seen: set[str] = set() + duplicates: set[str] = set() + for h in self.handlers: + if h.handler_key in seen: + duplicates.add(h.handler_key) + seen.add(h.handler_key) + if duplicates: + raise ModelOnexError( + message=f"Duplicate handler_key values: {duplicates}", + error_code=EnumCoreErrorCode.VALIDATION_ERROR, + field="handlers.handler_key", + value=duplicates, + constraint="unique_handler_key", + ) return selfsrc/omnibase_core/nodes/node_effect.py (1)
334-341: Consider defensive default forlazy_importattribute access.
getattr(handler_entry, "lazy_import", False)at line 341 correctly provides a default, but line 335-336 usesgetattrwithout a default forhandler_keyandcallable. If these attributes are missing, the comparison will work (returnsNone) but could be more explicit.This is minor since the current logic short-circuits correctly when
callable_pathis falsy.src/omnibase_core/resolution/resolver_protocol_dependency.py (1)
228-246: Lazy import handling has a subtle redundancy.When
dep.lazy_import=True, the code skips validation at line 231-232, then immediately tries to validate and resolve at lines 241-243. This means lazy_import dependencies are still imported during resolution, just with a try/except fallback.This is likely intentional (deferring only to catch failures gracefully), but the docstring states "Validate protocol exists (import check) unless lazy_import is enabled" which might suggest the import is deferred further. Consider clarifying the lazy_import semantics in the docstring if the current behavior is intentional.
Comment on lines
+1
to
+9
| """ | ||
| Tests for zero-code contract-driven node invariants. | ||
|
|
||
| This module enforces the architectural invariants that make zero-code nodes work. | ||
| Zero-code nodes rely entirely on contract configuration for handler dispatch, | ||
| protocol dependency injection, and runtime behavior. | ||
|
|
||
| Part of OMN-1731: Contract-driven zero-code node base classes. | ||
| """ |
Contributor
There was a problem hiding this comment.
Trim the module docstring to 3–6 lines.
Current module docstring exceeds the concise length requirement for simple modules.
✂️ Suggested shorter docstring
-"""
-Tests for zero-code contract-driven node invariants.
-
-This module enforces the architectural invariants that make zero-code nodes work.
-Zero-code nodes rely entirely on contract configuration for handler dispatch,
-protocol dependency injection, and runtime behavior.
-
-Part of OMN-1731: Contract-driven zero-code node base classes.
-"""
+"""
+Zero-code contract invariant tests.
+
+Validates handler routing, protocol DI, and handler resolution
+for OMN-1731 contract-driven nodes.
+"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """ | |
| Tests for zero-code contract-driven node invariants. | |
| This module enforces the architectural invariants that make zero-code nodes work. | |
| Zero-code nodes rely entirely on contract configuration for handler dispatch, | |
| protocol dependency injection, and runtime behavior. | |
| Part of OMN-1731: Contract-driven zero-code node base classes. | |
| """ | |
| """ | |
| Zero-code contract invariant tests. | |
| Validates handler routing, protocol DI, and handler resolution | |
| for OMN-1731 contract-driven nodes. | |
| """ |
🤖 Prompt for AI Agents
In `@tests/unit/validation/test_zero_code_invariants.py` around lines 1 - 9, Trim
the module-level docstring to a concise 3–6 line summary: replace the current
long paragraph with a short description (3–6 lines) that states this file
contains tests for zero-code contract-driven node invariants and briefly notes
purpose (enforce architectural invariants for zero-code nodes), removing extra
explanatory sentences; update the top-of-file module docstring accordingly so
it's within the 3–6 line guideline.
Comment on lines
+241
to
+322
| def test_protocol_dependency_immutable(self) -> None: | ||
| """Protocol dependency model is frozen (immutable).""" | ||
| dep = ModelProtocolDependency( | ||
| name="ProtocolLogger", | ||
| protocol="omnibase_core.protocols.protocol_logger:ProtocolLogger", | ||
| ) | ||
| with pytest.raises(Exception): # Pydantic raises ValidationError for frozen | ||
| dep.name = "NewName" # type: ignore[misc] | ||
|
|
||
| def test_pascal_to_snake_conversion(self) -> None: | ||
| """Protocol name is correctly converted to snake_case.""" | ||
| dep = ModelProtocolDependency( | ||
| name="ProtocolEventBus", | ||
| protocol="m:C", # Minimal valid format | ||
| ) | ||
| assert dep.get_bind_name() == "event_bus" | ||
|
|
||
| def test_name_without_protocol_prefix(self) -> None: | ||
| """Name without Protocol prefix is still converted correctly.""" | ||
| dep = ModelProtocolDependency( | ||
| name="EventBus", | ||
| protocol="m:C", | ||
| ) | ||
| assert dep.get_bind_name() == "event_bus" | ||
|
|
||
| def test_invalid_name_format_fails(self) -> None: | ||
| """Invalid Python identifier as name should fail.""" | ||
| with pytest.raises(ModelOnexError): | ||
| ModelProtocolDependency( | ||
| name="123Invalid", # Starts with number | ||
| protocol="m:C", | ||
| ) | ||
|
|
||
| def test_invalid_bind_as_format_fails(self) -> None: | ||
| """Invalid Python identifier as bind_as should fail.""" | ||
| with pytest.raises(ModelOnexError): | ||
| ModelProtocolDependency( | ||
| name="ProtocolLogger", | ||
| protocol="m:C", | ||
| bind_as="123-invalid", # Not a valid identifier | ||
| ) | ||
|
|
||
|
|
||
| class TestProtocolsNamespaceImmutability: | ||
| """Tests that protocols namespace is truly immutable.""" | ||
|
|
||
| def test_cannot_set_attribute_after_init(self) -> None: | ||
| """Setting attribute after init must raise AttributeError.""" | ||
| ns = ModelProtocolsNamespace({"logger": object()}) | ||
| with pytest.raises(AttributeError, match="immutable"): | ||
| ns.new_attr = "value" # type: ignore[attr-defined] | ||
|
|
||
| def test_cannot_modify_existing_protocol(self) -> None: | ||
| """Cannot overwrite an existing protocol.""" | ||
| ns = ModelProtocolsNamespace({"logger": object()}) | ||
| with pytest.raises(AttributeError, match="immutable"): | ||
| ns.logger = object() # type: ignore[misc] | ||
|
|
||
| def test_cannot_delete_attribute(self) -> None: | ||
| """Cannot delete attributes from namespace.""" | ||
| ns = ModelProtocolsNamespace({"logger": object()}) | ||
| with pytest.raises(AttributeError, match="immutable"): | ||
| del ns.logger # type: ignore[attr-defined] | ||
|
|
||
| def test_attribute_access_works(self) -> None: | ||
| """Attribute access returns the protocol.""" | ||
| logger = object() | ||
| ns = ModelProtocolsNamespace({"logger": logger}) | ||
| assert ns.logger is logger # type: ignore[attr-defined] | ||
|
|
||
| def test_dict_access_works(self) -> None: | ||
| """Dict-style access returns the protocol.""" | ||
| logger = object() | ||
| ns = ModelProtocolsNamespace({"logger": logger}) | ||
| assert ns["logger"] is logger | ||
|
|
||
| def test_missing_protocol_raises_attribute_error(self) -> None: | ||
| """Accessing non-existent protocol raises AttributeError.""" | ||
| ns = ModelProtocolsNamespace({"logger": object()}) | ||
| with pytest.raises(AttributeError, match="not found"): | ||
| _ = ns.event_bus # type: ignore[attr-defined] | ||
|
|
Contributor
There was a problem hiding this comment.
Add ticketed explanations above type ignores.
Type ignores need a short explanation and a Linear ticket reference above each occurrence.
📝 Example pattern (apply to all ignores in this block)
- with pytest.raises(Exception): # Pydantic raises ValidationError for frozen
- dep.name = "NewName" # type: ignore[misc]
+ with pytest.raises(Exception): # Pydantic raises ValidationError for frozen
+ # NOTE(OMN-1731): Intentional mutation to validate frozen model behavior.
+ dep.name = "NewName" # type: ignore[misc]🤖 Prompt for AI Agents
In `@tests/unit/validation/test_zero_code_invariants.py` around lines 241 - 322,
Add a short explanatory comment above every occurrence of "# type: ignore[...]"
in the tests (e.g., the ignores in test_protocol_dependency_immutable and
TestProtocolsNamespaceImmutability such as the ignores on dep.name = "NewName",
ns.new_attr = "value", ns.logger = object(), del ns.logger, ns.logger is logger,
and _ = ns.event_bus) that briefly states why the ignore is required and
includes a Linear ticket reference (e.g., "Reason: testing immutability; Linear:
LINEAR-XXXX"); ensure each comment is immediately above the line with the type
ignore and is concise.
Resolve merge conflict in subcontracts/__init__.py (include both ModelProtocolDependency and new request-response models). Address CodeRabbit review comments: - [Major] Normalize dict-based protocol dependencies before resolution in NodeCoreBase._resolve_protocol_dependencies() to handle both Pydantic models and raw dicts from YAML parsing - [Consistency] Use ModelOnexError with O(n) set-based detection for duplicate handler_key validation in ModelHandlerRoutingSubcontract - [Docstring] Clarify lazy_import semantics in resolver_protocol_dependency Also update test to expect ModelOnexError instead of ValueError for duplicate handler_key validation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_core/infrastructure/node_core_base.py`:
- Around line 498-557: The current initialize() flow masks protocol-specific
ModelOnexError codes (like PROTOCOL_CONFIGURATION_ERROR) thrown by
_resolve_protocol_dependencies()/resolve_protocol_dependencies by rewrapping
them as a generic OPERATION_FAILED; change initialize() so that when it catches
ModelOnexError it either re-raises it unchanged or preserves its
error_code/context when wrapping (i.e., if isinstance(exc, ModelOnexError):
raise; or include exc.error_code and exc.context in the new ModelOnexError),
ensuring errors raised from _resolve_protocol_dependencies() keep their original
error_code rather than being replaced.
🧹 Nitpick comments (1)
src/omnibase_core/models/contracts/subcontracts/model_handler_routing_subcontract.py (1)
225-281: Avoid O(n²) duplicate detection in zero-code validation.
keys.count(k)is quadratic; a set-based pass keeps it linear and simpler for large handler lists.♻️ Suggested linear duplicate check
- if self.handlers: - keys = [h.handler_key for h in self.handlers] - duplicates = [k for k in keys if keys.count(k) > 1] - if duplicates: - errors.append(f"Duplicate handler_key values: {set(duplicates)}") + if self.handlers: + seen: set[str] = set() + duplicates: set[str] = set() + for key in (h.handler_key for h in self.handlers): + if key in seen: + duplicates.add(key) + seen.add(key) + if duplicates: + errors.append(f"Duplicate handler_key values: {duplicates}")
…espace [OMN-1731] - Extract shared _get_default_handler_callable() from NodeCompute and NodeEffect to NodeCoreBase - Add get_contract_attr() utility for cleaner dict/Pydantic model attribute access - Enhance ModelProtocolsNamespace.__repr__ to show protocol types for debugging - Net reduction of 69 lines through code deduplication
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/omnibase_core/nodes/node_compute.py`:
- Around line 245-289: The contract-driven dispatch path (involving
_get_default_handler_callable and resolved_handler) currently returns early
without stopping the _timing_service or updating metrics, leaving
processing_time_ms at 0 and counters underreported; fix by measuring and
recording timing and metrics before both return points: call the same timing
stop/record logic used elsewhere (e.g., stop the timer from _timing_service and
compute processing_time_ms), update the relevant metrics/counters on the node
(same counters updated in the non-contract path), and then if the handler
returned a raw value wrap it in ModelComputeOutput with the computed
processing_time_ms/metrics metadata, and if it returned ModelComputeOutput
update its processing_time_ms/metrics fields (or copy into a new
ModelComputeOutput) so all contract-driven returns go through the same
timing/metrics update code.
In `@src/omnibase_core/nodes/node_effect.py`:
- Around line 364-397: The default-handler dispatch in
_get_default_handler_callable resolution should verify the handler result is a
coroutine before awaiting to avoid TypeError; update the block that assigns
resolved_handler and calls await resolved_handler(self, input_data) to first
call the handler, check with asyncio.iscoroutine(...), and if not a coroutine
raise ModelOnexError with EnumCoreErrorCode.HANDLER_EXECUTION_ERROR and the same
context as the existing type-check branch (or alternatively support sync
handlers by handling non-coroutine results consistently as in NodeCompute);
ensure asyncio is imported and keep the runtime ModelEffectOutput validation
after obtaining the result.
In `@src/omnibase_core/utils/util_contract_accessor.py`:
- Around line 12-32: The function get_contract_attr currently calls hasattr(obj,
name) before checking dict-like objects, which wrongly treats dict methods
(e.g., "get", "items") as attributes and can mask actual dict keys; update
get_contract_attr to first detect mapping/dict objects (use isinstance(obj,
dict) or isinstance(obj, Mapping)) and return obj.get(name, default) when true,
then fall back to hasattr/getattr for objects (and finally default); apply the
same ordering fix to the other similar helper referenced in the file (the one
around lines 35-50) to ensure dict keys are always preferred over attribute
lookups.
Comment on lines
+245
to
+289
| # Check for contract-driven dispatch via default_handler | ||
| # If contract specifies a handler callable, dispatch to it directly | ||
| handler_info = self._get_default_handler_callable() | ||
| if handler_info is not None: | ||
| handler_or_loader, is_lazy = handler_info | ||
| # Resolve the handler - either directly or via lazy loader | ||
| resolved_handler: HandlerCallable | ||
| if is_lazy: | ||
| # NOTE(OMN-1731): Type narrowing for union with boolean flag. | ||
| # When is_lazy=True, handler_or_loader is LazyLoader (a callable | ||
| # that returns HandlerCallable). mypy cannot narrow automatically. | ||
| resolved_handler = handler_or_loader() # type: ignore[assignment] | ||
| else: | ||
| # NOTE(OMN-1731): When is_lazy=False, handler_or_loader is already | ||
| # HandlerCallable. mypy cannot narrow the union type automatically. | ||
| resolved_handler = handler_or_loader # type: ignore[assignment] | ||
|
|
||
| # Dispatch to contract-specified handler | ||
| # Handler signature: async def handler(node, input_data) -> output | ||
| # or sync: def handler(node, input_data) -> output | ||
| # NOTE(OMN-1731): mypy cannot infer the return type from | ||
| # the dynamically resolved handler. Safe because contract defines the | ||
| # handler signature requirement. | ||
| handler_result = resolved_handler(self, input_data) # type: ignore[misc] | ||
|
|
||
| # Handle both async and sync handlers | ||
| if asyncio.iscoroutine(handler_result): | ||
| result: Any = await handler_result | ||
| else: | ||
| result = handler_result | ||
|
|
||
| # If handler returns ModelComputeOutput, return as-is | ||
| # Otherwise, wrap in ModelComputeOutput for type safety | ||
| if isinstance(result, ModelComputeOutput): | ||
| return result | ||
| # Wrap raw result in ModelComputeOutput | ||
| return ModelComputeOutput( | ||
| result=result, | ||
| operation_id=input_data.operation_id, | ||
| computation_type=input_data.computation_type, | ||
| processing_time_ms=0.0, | ||
| cache_hit=False, | ||
| parallel_execution_used=False, | ||
| metadata={"contract_driven_dispatch": True}, | ||
| ) |
Contributor
There was a problem hiding this comment.
Populate timing + metrics in the contract-driven handler path.
Early returns skip _timing_service stop and metrics updates, so processing_time_ms stays 0 and counters underreport when handlers are used.
🛠️ Suggested fix
# Handle both async and sync handlers
if asyncio.iscoroutine(handler_result):
result: Any = await handler_result
else:
result = handler_result
+ processing_time: float = 0.0
+ if self._timing_service is not None and start_time is not None:
+ processing_time = self._timing_service.stop_timer(start_time)
+ self._update_specialized_metrics(
+ self.computation_metrics,
+ input_data.computation_type,
+ processing_time,
+ True,
+ )
+ await self._update_processing_metrics(processing_time, True)
+
# If handler returns ModelComputeOutput, return as-is
# Otherwise, wrap in ModelComputeOutput for type safety
if isinstance(result, ModelComputeOutput):
return result
# Wrap raw result in ModelComputeOutput
return ModelComputeOutput(
result=result,
operation_id=input_data.operation_id,
computation_type=input_data.computation_type,
- processing_time_ms=0.0,
+ processing_time_ms=processing_time,
cache_hit=False,
parallel_execution_used=False,
metadata={"contract_driven_dispatch": True},
)🤖 Prompt for AI Agents
In `@src/omnibase_core/nodes/node_compute.py` around lines 245 - 289, The
contract-driven dispatch path (involving _get_default_handler_callable and
resolved_handler) currently returns early without stopping the _timing_service
or updating metrics, leaving processing_time_ms at 0 and counters underreported;
fix by measuring and recording timing and metrics before both return points:
call the same timing stop/record logic used elsewhere (e.g., stop the timer from
_timing_service and compute processing_time_ms), update the relevant
metrics/counters on the node (same counters updated in the non-contract path),
and then if the handler returned a raw value wrap it in ModelComputeOutput with
the computed processing_time_ms/metrics metadata, and if it returned
ModelComputeOutput update its processing_time_ms/metrics fields (or copy into a
new ModelComputeOutput) so all contract-driven returns go through the same
timing/metrics update code.
…ses [OMN-1731] - Fix O(n²) duplicate detection in validate_zero_code_requirements() to use O(n) set-based detection matching the model validator pattern - Track actual processing time for contract-driven dispatch in NodeCompute instead of always returning 0.0ms - Add pytestmark = pytest.mark.unit to test file per project conventions - Trim test module docstring to 3-6 lines per project guidelines
Address code review feedback for zero-code node base classes: Major fixes: - Preserve PROTOCOL_CONFIGURATION_ERROR instead of masking with OPERATION_FAILED - Populate timing + metrics in contract-driven handler dispatch path - Use asyncio.iscoroutine() in NodeEffect for sync/async handler support - Remove `from __future__ import annotations` from node_effect.py to fix signature snapshot test failures (annotations must be class refs, not strings) Minor fixes: - Add NOTE(OMN-1731) comments above type ignores in test file - Check dict keys before attribute lookup in util_contract_accessor.py Nitpick fixes: - Streamline lazy import handling in resolver_protocol_dependency.py to eliminate redundant import calls
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/omnibase_core/nodes/node_effect.py`:
- Around line 264-279: Auto-load of effect_subcontract is currently in __init__
so it runs before contract_data is populated (set during
initialize/_load_contract); move the logic into _initialize_node_resources and
run it after contract_data is loaded there (or immediately after calling
_load_contract) so zero-code nodes get their effect_subcontract; keep the same
behavior: check hasattr(self, "contract_data") and not None, support both
ModelEffectSubcontract instances and dicts, and use object.__setattr__(self,
"effect_subcontract", ...) when setting; also update the duplicated block around
the other occurrence (lines ~641-647) to the same _initialize_node_resources
location to avoid redundancy.
In `@src/omnibase_core/resolution/resolver_protocol_dependency.py`:
- Around line 1-34: The module docstring in resolver_protocol_dependency.py is
too long; replace it with a concise 3–6 line summary that states the module
purpose (resolving ModelProtocolDependency entries from the ONEX container),
mention key behavior (validates/imports unless lazy_import, resolves by
dep.get_bind_name(), handles required vs optional), and remove extended
examples, See Also, and versionadded blocks; ensure
resolve_protocol_dependencies and ModelProtocolDependency remain referenced in
the short summary so callers can quickly identify functionality.
🧹 Nitpick comments (1)
src/omnibase_core/resolution/resolver_protocol_dependency.py (1)
136-145: Prevent unbounded growth of_last_warning_times.In long-lived processes with many node/dependency pairs, the map can grow without bound. Consider pruning stale entries when recording a new warning.
♻️ Suggested pruning
current_time_ms = time.time() * 1000 + + # Prune stale entries to keep the rate-limit map bounded + cutoff = current_time_ms - (_OPTIONAL_WARNING_INTERVAL_MS * 2) + for k, ts in list(_last_warning_times.items()): + if ts < cutoff: + _last_warning_times.pop(k, None) last_warning = _last_warning_times.get(key, 0)
Comment on lines
+264
to
+279
| # Auto-load effect_subcontract from contract if present | ||
| if hasattr(self, "contract_data") and self.contract_data is not None: | ||
| # Support both ModelContractBase instances and dict-based contracts | ||
| effect_sub = None | ||
| if hasattr(self.contract_data, "effect_subcontract"): | ||
| effect_sub = getattr(self.contract_data, "effect_subcontract", None) | ||
| elif isinstance(self.contract_data, dict): | ||
| effect_sub = self.contract_data.get("effect_subcontract") | ||
|
|
||
| if effect_sub is not None: | ||
| if isinstance(effect_sub, ModelEffectSubcontract): | ||
| object.__setattr__(self, "effect_subcontract", effect_sub) | ||
| elif isinstance(effect_sub, dict): | ||
| object.__setattr__( | ||
| self, "effect_subcontract", ModelEffectSubcontract(**effect_sub) | ||
| ) |
Contributor
There was a problem hiding this comment.
Auto-load effect_subcontract after contract_data is loaded.
contract_data is set during initialize() after _load_contract(), so doing this in __init__ means the block never runs for contract-loaded nodes. As a result, zero-code nodes can still hit “Effect subcontract not loaded.”
🛠️ Move auto-load into _initialize_node_resources
- # Auto-load effect_subcontract from contract if present
- if hasattr(self, "contract_data") and self.contract_data is not None:
- # Support both ModelContractBase instances and dict-based contracts
- effect_sub = None
- if hasattr(self.contract_data, "effect_subcontract"):
- effect_sub = getattr(self.contract_data, "effect_subcontract", None)
- elif isinstance(self.contract_data, dict):
- effect_sub = self.contract_data.get("effect_subcontract")
-
- if effect_sub is not None:
- if isinstance(effect_sub, ModelEffectSubcontract):
- object.__setattr__(self, "effect_subcontract", effect_sub)
- elif isinstance(effect_sub, dict):
- object.__setattr__(
- self, "effect_subcontract", ModelEffectSubcontract(**effect_sub)
- ) async def _initialize_node_resources(self) -> None: # stub-ok
"""Initialize effect-specific resources.
Circuit breakers are lazily initialized on first use via get_circuit_breaker().
No additional resources needed for contract-driven execution.
"""
+ if self.contract_data is None:
+ return
+ effect_sub = None
+ if hasattr(self.contract_data, "effect_subcontract"):
+ effect_sub = getattr(self.contract_data, "effect_subcontract", None)
+ elif isinstance(self.contract_data, dict):
+ effect_sub = self.contract_data.get("effect_subcontract")
+
+ if effect_sub is not None:
+ if isinstance(effect_sub, ModelEffectSubcontract):
+ object.__setattr__(self, "effect_subcontract", effect_sub)
+ elif isinstance(effect_sub, dict):
+ object.__setattr__(
+ self, "effect_subcontract", ModelEffectSubcontract(**effect_sub)
+ )Also applies to: 641-647
🤖 Prompt for AI Agents
In `@src/omnibase_core/nodes/node_effect.py` around lines 264 - 279, Auto-load of
effect_subcontract is currently in __init__ so it runs before contract_data is
populated (set during initialize/_load_contract); move the logic into
_initialize_node_resources and run it after contract_data is loaded there (or
immediately after calling _load_contract) so zero-code nodes get their
effect_subcontract; keep the same behavior: check hasattr(self, "contract_data")
and not None, support both ModelEffectSubcontract instances and dicts, and use
object.__setattr__(self, "effect_subcontract", ...) when setting; also update
the duplicated block around the other occurrence (lines ~641-647) to the same
_initialize_node_resources location to avoid redundancy.
Comment on lines
+1
to
+34
| """Protocol dependency resolver for contract-driven DI. | ||
|
|
||
| This module provides resolution of protocol dependencies declared in node contracts. | ||
| At node initialization, the resolver processes ModelProtocolDependency declarations | ||
| and resolves them from the ONEX container. | ||
|
|
||
| Resolution Process: | ||
| 1. Validate protocol exists (import check) unless lazy_import is enabled | ||
| 2. Resolve service from container using the dependency name | ||
| 3. Return dict keyed by bind name (from dep.get_bind_name()) | ||
| 4. Handle required vs optional (fail fast vs None) | ||
|
|
||
| Thread Safety: | ||
| This module is thread-safe. The rate-limiting state uses a module-level | ||
| dictionary with simple timestamp comparisons, which is safe for concurrent | ||
| reads with occasional writes. | ||
|
|
||
| Example: | ||
| >>> from omnibase_core.resolution import resolve_protocol_dependencies | ||
| >>> deps = [ | ||
| ... ModelProtocolDependency( | ||
| ... name="ProtocolLogger", | ||
| ... protocol="omnibase_core.protocols.protocol_logger:ProtocolLogger", | ||
| ... ) | ||
| ... ] | ||
| >>> resolved = resolve_protocol_dependencies(deps, container) | ||
| >>> print(resolved) # {"logger": <ProtocolLogger instance>} | ||
|
|
||
| See Also: | ||
| - OMN-1731: Contract-driven zero-code node base classes | ||
| - ModelProtocolDependency: The dependency declaration model | ||
|
|
||
| .. versionadded:: 0.4.1 | ||
| """ |
Contributor
There was a problem hiding this comment.
Shorten the module docstring to 3–6 lines.
This reads like full documentation and exceeds the concise-module guideline.
✂️ Suggested trim
-"""Protocol dependency resolver for contract-driven DI.
-
-This module provides resolution of protocol dependencies declared in node contracts.
-At node initialization, the resolver processes ModelProtocolDependency declarations
-and resolves them from the ONEX container.
-
-Resolution Process:
- 1. Validate protocol exists (import check) unless lazy_import is enabled
- 2. Resolve service from container using the dependency name
- 3. Return dict keyed by bind name (from dep.get_bind_name())
- 4. Handle required vs optional (fail fast vs None)
-
-Thread Safety:
- This module is thread-safe. The rate-limiting state uses a module-level
- dictionary with simple timestamp comparisons, which is safe for concurrent
- reads with occasional writes.
-
-Example:
- >>> from omnibase_core.resolution import resolve_protocol_dependencies
- >>> deps = [
- ... ModelProtocolDependency(
- ... name="ProtocolLogger",
- ... protocol="omnibase_core.protocols.protocol_logger:ProtocolLogger",
- ... )
- ... ]
- >>> resolved = resolve_protocol_dependencies(deps, container)
- >>> print(resolved) # {"logger": <ProtocolLogger instance>}
-
-See Also:
- - OMN-1731: Contract-driven zero-code node base classes
- - ModelProtocolDependency: The dependency declaration model
-
-.. versionadded:: 0.4.1
-"""
+"""Protocol dependency resolver for contract-driven DI.
+
+Resolves ModelProtocolDependency entries from the ONEX container,
+handling required/optional semantics and lazy imports.
+"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Protocol dependency resolver for contract-driven DI. | |
| This module provides resolution of protocol dependencies declared in node contracts. | |
| At node initialization, the resolver processes ModelProtocolDependency declarations | |
| and resolves them from the ONEX container. | |
| Resolution Process: | |
| 1. Validate protocol exists (import check) unless lazy_import is enabled | |
| 2. Resolve service from container using the dependency name | |
| 3. Return dict keyed by bind name (from dep.get_bind_name()) | |
| 4. Handle required vs optional (fail fast vs None) | |
| Thread Safety: | |
| This module is thread-safe. The rate-limiting state uses a module-level | |
| dictionary with simple timestamp comparisons, which is safe for concurrent | |
| reads with occasional writes. | |
| Example: | |
| >>> from omnibase_core.resolution import resolve_protocol_dependencies | |
| >>> deps = [ | |
| ... ModelProtocolDependency( | |
| ... name="ProtocolLogger", | |
| ... protocol="omnibase_core.protocols.protocol_logger:ProtocolLogger", | |
| ... ) | |
| ... ] | |
| >>> resolved = resolve_protocol_dependencies(deps, container) | |
| >>> print(resolved) # {"logger": <ProtocolLogger instance>} | |
| See Also: | |
| - OMN-1731: Contract-driven zero-code node base classes | |
| - ModelProtocolDependency: The dependency declaration model | |
| .. versionadded:: 0.4.1 | |
| """ | |
| """Protocol dependency resolver for contract-driven DI. | |
| Resolves ModelProtocolDependency entries from the ONEX container, | |
| handling required/optional semantics and lazy imports. | |
| """ |
🤖 Prompt for AI Agents
In `@src/omnibase_core/resolution/resolver_protocol_dependency.py` around lines 1
- 34, The module docstring in resolver_protocol_dependency.py is too long;
replace it with a concise 3–6 line summary that states the module purpose
(resolving ModelProtocolDependency entries from the ONEX container), mention key
behavior (validates/imports unless lazy_import, resolves by dep.get_bind_name(),
handles required vs optional), and remove extended examples, See Also, and
versionadded blocks; ensure resolve_protocol_dependencies and
ModelProtocolDependency remain referenced in the short summary so callers can
quickly identify functionality.
4 of 5 tasks
jonahgabriel
deleted the
jonah/omn-1731-feat-contract-driven-zero-code-node-base-classes
branch
April 4, 2026 02:04
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
Enable fully declarative ONEX nodes where
class NodeMyEffect(NodeEffect): passworks entirely fromcontract.yaml- all behavior driven by contract and handlers.Linear Ticket: OMN-1731
What Changed
New Models
name,protocolpath,requiredflag,bind_asalias, andlazy_importsupportself.protocols.<name>accessNew Resolvers
module:callableimport paths with cachingSchema Extensions
callableandlazy_importfieldsprotocol_dependencieslist with duplicate detectionvalidate_zero_code_requirements()methodNode Updates
protocolsnamespace and_resolve_protocol_dependencies()effect_subcontract, dispatches viadefault_handlerdefault_handlerwith generic supportError Handling
PROTOCOL_CONFIGURATION_ERROR(code 126) for DI failuresArchitectural Decisions
handler_routing.default_handlerself.protocolsnamespaceExample Usage
Contract (contract.yaml):
Node (node.py):
Test plan
Phase 2 (Future)
Summary by CodeRabbit
New Features
Validation
Tests
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.