Repository navigation
feat(OMN-1726): Refactor 3 imperative nodes to declarative pattern - #217
jonahgabriel merged 5 commits into
Conversation
Resolves 18 violations by converting imperative nodes to declarative shells with contract-driven handler routing. ## NodeLedgerProjectionCompute (5 violations) - Created HandlerLedgerProjection with all compute logic - Added handlers/contract.yaml for handler routing - Node simplified to `pass` (declarative shell) ## NodeRegistryEffect (8 violations) - Removed setter methods, properties, and instance variables - Clients now resolved via container DI - Node simplified to pure declarative shell ## NodeArchitectureValidatorCompute (5 violations) - Created HandlerArchitectureValidation with all validation logic - Added handlers/contract.yaml for handler routing - Extracted SUPPORTED_RULE_IDS to constants.py - Node simplified to `pass` (declarative shell) All handlers follow ProtocolHandler pattern with contract.yaml defining input_model, output_model, and operation bindings. Tests: 417 passing (53 + 256 + 108)
📝 WalkthroughWalkthroughNodes turned into declarative shells; validation and ledger-projection behavior moved to new handler classes. Node registry registration now invokes per-backend handlers and a NodeRegistrationOrchestrator that executes handlers in parallel and reports per-backend results. Tests updated to exercise handlers and orchestrator. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Orchestrator as NodeRegistrationOrchestrator
participant ConsulH as HandlerConsulRegister
participant PostgresH as HandlerPostgresUpsert
participant ConsulC as ConsulClient
participant PostgresA as PostgresAdapter
Client->>Orchestrator: execute_registration(request)
par Parallel Handler Execution
Orchestrator->>ConsulH: handle(request, correlation_id)
ConsulH->>ConsulC: register node
ConsulC-->>ConsulH: consul_result
ConsulH-->>Orchestrator: consul_result
and
Orchestrator->>PostgresH: handle(request, correlation_id)
PostgresH->>PostgresA: upsert registry record
PostgresA-->>PostgresH: postgres_result
PostgresH-->>Orchestrator: postgres_result
end
Orchestrator->>Orchestrator: aggregate results / identify failed_backends
Orchestrator-->>Client: response (status + failed_backends)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@docs/patterns/operation_routing.md`:
- Around line 439-441: The example uses ProtocolConsulClient and
ProtocolPostgresAdapter but does not import them; update the snippet to add the
missing imports for those protocols at the top of the example so the
container.resolve calls work. Locate the example block that calls
container.resolve(ProtocolConsulClient) and
container.resolve(ProtocolPostgresAdapter) and add import statements for
ProtocolConsulClient and ProtocolPostgresAdapter from their defining module(s)
so the symbols are defined in the scope of the snippet.
In `@src/omnibase_infra/nodes/architecture_validator/constants.py`:
- Line 2: Update the copyright header line that currently reads "# Copyright (c)
2025 OmniNode Team" in constants.py to use 2026 so it matches the other new
files in this PR; locate the exact header string and replace the year literal
2025 with 2026.
In `@src/omnibase_infra/nodes/architecture_validator/handlers/__init__.py`:
- Line 2: Update the copyright year in the file header of handlers/__init__.py
from 2025 to 2026 to match the rest of the codebase (same change as made in
constants.py); locate the top-of-file copyright comment and replace the year
only.
In
`@src/omnibase_infra/nodes/architecture_validator/handlers/handler_architecture_validation.py`:
- Around line 120-146: The __init__ signature of HandlerArchitectureValidation
must accept a ModelONEXContainer for DI; change the constructor to def
__init__(self, container: ModelONEXContainer, rules:
tuple[ProtocolArchitectureRule, ...] = (),) -> None:, assign self._container =
container, keep calling self._validate_rules_against_contract(rules) and
self._rules = rules, and update the docstring Args to document the new container
parameter; ensure you import ModelONEXContainer if not already imported and
preserve existing validation behavior in _validate_rules_against_contract.
In `@src/omnibase_infra/nodes/node_ledger_projection_compute/contract.yaml`:
- Around line 72-81: The HandlerLedgerProjection handler is missing the required
describe() method from the ProtocolHandler contract; add a describe(self) method
to the HandlerLedgerProjection class that returns the handler's metadata (name,
handler_type, supported operations and a short description) consistent with
other handlers' describe implementations, ensuring the signature and return
shape match the ProtocolHandler's expected contract so
initialization/registration code that calls describe() works correctly.
In
`@src/omnibase_infra/nodes/node_ledger_projection_compute/handlers/handler_ledger_projection.py`:
- Around line 189-191: Replace the RuntimeError raised for a missing/invalid
envelope payload with an OnexError (or a more specific OnexError subclass) in
the payload validation block inside handler_ledger_projection.py where
payload_raw = envelope.get("payload"); ensure you import the OnexError symbol
(or the chosen subclass) and raise it with the same descriptive message
("Missing or invalid 'payload' in envelope") so the code follows the guideline
of only raising OnexError-derived exceptions.
🧹 Nitpick comments (3)
src/omnibase_infra/nodes/architecture_validator/handlers/contract.yaml (1)
24-29: Addnode_versionfor consistency with other handler contracts.All handler contracts in the codebase include
node_version, but this one is missing it. The ledger projection handler contract and all other handlers define bothcontract_versionandnode_versionfor semantic versioning of the handler itself.Suggested addition
contract_version: major: 1 minor: 0 patch: 0 +node_version: + major: 0 + minor: 9 + patch: 0 # Node type context - this handler is used by COMPUTE_GENERIC nodestests/unit/nodes/architecture_validator/test_node_architecture_validator.py (1)
1238-1241: Consider using a fresh handler instance instead of resetting private state.Directly resetting
_check_countaccesses private implementation details. Consider creating a new handler instance instead.♻️ Alternative approach
result_none = handler.validate_architecture(request_none) - - # Reset check count - passing_rule._check_count = 0 + + # Create fresh rule instance for second test + passing_rule_2 = MockRule( + rule_id="NO_HANDLER_PUBLISHING", + name="No Handler Publishing Rule", + severity=EnumValidationSeverity.ERROR, + should_pass=True, + ) + handler_2 = HandlerArchitectureValidation(rules=(passing_rule_2,)) # Empty tuple = check no rules request_empty = ModelArchitectureValidationRequest( nodes=(sample_node,), handlers=(), rule_ids=(), ) - result_empty = handler.validate_architecture(request_empty) + result_empty = handler_2.validate_architecture(request_empty)tests/unit/nodes/test_node_ledger_projection_compute.py (1)
696-725: Simplify declarative pattern verification.The hasattr checks with callable guards are overly complex. A simpler approach would be more readable.
♻️ Simplified approach
def test_node_has_no_compute_method( self, node: NodeLedgerProjectionCompute ) -> None: """Node should not have a custom compute method.""" # The node should just be a pass-through shell # It should not have custom methods like _b64, _normalize_headers, etc. - assert ( - not hasattr(node, "_b64") or callable(getattr(node, "_b64", None)) is False - ) - assert ( - not hasattr(node, "_normalize_headers") - or callable(getattr(node, "_normalize_headers", None)) is False - ) - assert ( - not hasattr(node, "_parse_offset") - or callable(getattr(node, "_parse_offset", None)) is False - ) - assert ( - not hasattr(node, "_extract_ledger_metadata") - or callable(getattr(node, "_extract_ledger_metadata", None)) is False - ) + # Verify handler-specific methods are NOT on the node + handler_methods = ["_b64", "_normalize_headers", "_parse_offset", "_extract_ledger_metadata"] + for method in handler_methods: + assert not hasattr(node, method), f"Node should not have {method} method"
| # Resolve backend adapters from container | ||
| consul_client = container.resolve(ProtocolConsulClient) | ||
| postgres_adapter = container.resolve(ProtocolPostgresAdapter) |
There was a problem hiding this comment.
Add missing protocol imports in the example.
ProtocolConsulClient and ProtocolPostgresAdapter are used but not imported, so the snippet won’t run as-is.
Proposed doc fix
+from omnibase_infra.protocols import ProtocolConsulClient, ProtocolPostgresAdapter # adjust module path if different📝 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.
| # Resolve backend adapters from container | |
| consul_client = container.resolve(ProtocolConsulClient) | |
| postgres_adapter = container.resolve(ProtocolPostgresAdapter) | |
| from omnibase_infra.protocols import ProtocolConsulClient, ProtocolPostgresAdapter | |
| # Resolve backend adapters from container | |
| consul_client = container.resolve(ProtocolConsulClient) | |
| postgres_adapter = container.resolve(ProtocolPostgresAdapter) |
🤖 Prompt for AI Agents
In `@docs/patterns/operation_routing.md` around lines 439 - 441, The example uses
ProtocolConsulClient and ProtocolPostgresAdapter but does not import them;
update the snippet to add the missing imports for those protocols at the top of
the example so the container.resolve calls work. Locate the example block that
calls container.resolve(ProtocolConsulClient) and
container.resolve(ProtocolPostgresAdapter) and add import statements for
ProtocolConsulClient and ProtocolPostgresAdapter from their defining module(s)
so the symbols are defined in the scope of the snippet.
| @@ -0,0 +1,36 @@ | |||
| # SPDX-License-Identifier: MIT | |||
| # Copyright (c) 2025 OmniNode Team | |||
There was a problem hiding this comment.
Minor: Copyright year inconsistency.
This file uses 2025 while other new files in the PR use 2026. Consider aligning for consistency.
🤖 Prompt for AI Agents
In `@src/omnibase_infra/nodes/architecture_validator/constants.py` at line 2,
Update the copyright header line that currently reads "# Copyright (c) 2025
OmniNode Team" in constants.py to use 2026 so it matches the other new files in
this PR; locate the exact header string and replace the year literal 2025 with
2026.
| @@ -0,0 +1,28 @@ | |||
| # SPDX-License-Identifier: MIT | |||
| # Copyright (c) 2025 OmniNode Team | |||
There was a problem hiding this comment.
Minor: Copyright year inconsistency.
Same as constants.py - uses 2025 while other new files use 2026.
🤖 Prompt for AI Agents
In `@src/omnibase_infra/nodes/architecture_validator/handlers/__init__.py` at line
2, Update the copyright year in the file header of handlers/__init__.py from
2025 to 2026 to match the rest of the codebase (same change as made in
constants.py); locate the top-of-file copyright comment and replace the year
only.
| def __init__( | ||
| self, | ||
| rules: tuple[ProtocolArchitectureRule, ...] = (), | ||
| ) -> None: | ||
| """Initialize handler with architecture rules. | ||
|
|
||
| Args: | ||
| rules: Architecture rules to enforce. These should implement | ||
| ProtocolArchitectureRule. Rules from OMN-1099 validators can be | ||
| passed directly. | ||
|
|
||
| Raises: | ||
| ProtocolConfigurationError: If any rule has a rule_id not in | ||
| SUPPORTED_RULE_IDS. The error message includes the unsupported | ||
| rule_id and the list of supported rules for debugging. | ||
|
|
||
| Example: | ||
| >>> from my_rules import NoHandlerPublishingRule, NoWorkflowInReducerRule | ||
| >>> | ||
| >>> handler = HandlerArchitectureValidation( | ||
| ... rules=(NoHandlerPublishingRule(), NoWorkflowInReducerRule()), | ||
| ... ) | ||
|
|
||
| .. versionadded:: 0.9.0 | ||
| """ | ||
| self._validate_rules_against_contract(rules) | ||
| self._rules = rules |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all handler implementations and their __init__ signatures
fd -e py "handler_.*\.py" --type f | while read file; do
echo "=== $file ==="
grep -A 8 "def __init__" "$file" | head -15
done | head -100Repository: OmniNode-ai/omnibase_infra
Length of output: 4762
🏁 Script executed:
#!/bin/bash
# Check for ModelONEXContainer usage in handlers
rg -l "class Handler" --type py -g "handler_*.py" | head -10Repository: OmniNode-ai/omnibase_infra
Length of output: 810
🏁 Script executed:
#!/bin/bash
# Examine specific handlers to understand the pattern
rg "def __init__" --type py -g "handler_*.py" -B 2 -A 5 | head -80Repository: OmniNode-ai/omnibase_infra
Length of output: 8337
🏁 Script executed:
#!/bin/bash
# Get the full handler_architecture_validation.py to understand its design
wc -l src/omnibase_infra/nodes/architecture_validator/handlers/handler_architecture_validation.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Show the complete handler class
cat -n src/omnibase_infra/nodes/architecture_validator/handlers/handler_architecture_validation.py | head -200Repository: OmniNode-ai/omnibase_infra
Length of output: 8855
🏁 Script executed:
#!/bin/bash
# Check if there are any similar handlers that take rules/domain objects instead of container
rg "def __init__" --type py -g "handler_*.py" -B 2 -A 3 | grep -E "(rules|protocol|domain)" -A 2 -B 2 | head -40Repository: OmniNode-ai/omnibase_infra
Length of output: 545
Add ModelONEXContainer parameter to align with mandatory handler DI pattern.
The handler violates the coding guideline requiring all handlers to use ModelONEXContainer for dependency injection. Per the guidelines for **/{service,adapter,dispatcher,handler}_*.py, the __init__ signature should be:
def __init__(
self,
container: ModelONEXContainer,
rules: tuple[ProtocolArchitectureRule, ...] = (),
) -> None:This follows the established pattern in HandlerLedgerAppend and HandlerLedgerQuery, which accept both the container and domain-specific parameters. Storing the container (self._container = container) ensures full ONEX integration for future service discovery, logging, and metrics needs, even if not immediately used.
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/architecture_validator/handlers/handler_architecture_validation.py`
around lines 120 - 146, The __init__ signature of HandlerArchitectureValidation
must accept a ModelONEXContainer for DI; change the constructor to def
__init__(self, container: ModelONEXContainer, rules:
tuple[ProtocolArchitectureRule, ...] = (),) -> None:, assign self._container =
container, keep calling self._validate_rules_against_contract(rules) and
self._rules = rules, and update the docstring Args to document the new container
parameter; ensure you import ModelONEXContainer if not already imported and
preserve existing validation behavior in _validate_rules_against_contract.
| # Handler routing - delegates compute logic to handler | ||
| handler_routing: | ||
| routing_strategy: "operation_match" | ||
| handlers: | ||
| - handler_type: "ledger_projection" | ||
| handler: | ||
| name: "HandlerLedgerProjection" | ||
| module: "omnibase_infra.nodes.node_ledger_projection_compute.handlers.handler_ledger_projection" | ||
| supported_operations: | ||
| - "ledger.project" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the handler file and examine its structure
fd -a 'handler_ledger_projection\.py' -x cat -n {}Repository: OmniNode-ai/omnibase_infra
Length of output: 15805
🏁 Script executed:
# Find the ProtocolHandler interface definition
fd -a '\.py' | xargs rg -l 'class ProtocolHandler|def ProtocolHandler' | head -5Repository: OmniNode-ai/omnibase_infra
Length of output: 225
🏁 Script executed:
# Search for ProtocolHandler interface/base class definition
rg -A 30 'class ProtocolHandler' --type pyRepository: OmniNode-ai/omnibase_infra
Length of output: 6298
🏁 Script executed:
# Search for ProtocolHandler interface (not the loader/discovery protocols)
rg 'class ProtocolHandler[^P]|class ProtocolHandler:' -A 50 --type pyRepository: OmniNode-ai/omnibase_infra
Length of output: 4954
🏁 Script executed:
# Also search for any file that might define handler protocol/interface
fd -a '\.py' | xargs rg -l 'ProtocolHandler' | grep -v loader | grep -v discovery | head -10Repository: OmniNode-ai/omnibase_infra
Length of output: 846
🏁 Script executed:
# Search for ProtocolHandler in protocol files specifically
fd -a '\.py' -path '*/protocol*' | xargs rg 'ProtocolHandler' | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 2541
🏁 Script executed:
# Search for the 5 required handler methods mentioned in guidelines
rg 'async def execute|async def initialize|async def shutdown|def handler_type|def handler_category' --type py src/omnibase_infra/ -cRepository: OmniNode-ai/omnibase_infra
Length of output: 2907
🏁 Script executed:
# Check if there's a base handler or abstract handler class
rg 'class.*Handler.*Protocol|ABC|abstractmethod' --type py | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 2360
🏁 Script executed:
# Find test files that reference ProtocolHandler and might show expected methods
rg -B5 -A20 'class.*ProtocolHandler|def.*ProtocolHandler' tests/ --glob '*.py' | head -100Repository: OmniNode-ai/omnibase_infra
Length of output: 9428
🏁 Script executed:
# Look at what HandlerLedgerProjection implements specifically
cat -n src/omnibase_infra/nodes/node_ledger_projection_compute/handlers/handler_ledger_projection.py | grep -E '^\s+(async\s+)?def |@property' | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 111
🏁 Script executed:
# Check if there are imports or references to ProtocolHandler in the handler file or nearby
rg 'ProtocolHandler' src/omnibase_infra/nodes/node_ledger_projection_compute/ -B2 -A2Repository: OmniNode-ai/omnibase_infra
Length of output: 779
🏁 Script executed:
# Check if HandlerLedgerProjection has a describe() method
rg -n 'def describe' src/omnibase_infra/nodes/node_ledger_projection_compute/handlers/handler_ledger_projection.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
# Get a complete list of all methods in HandlerLedgerProjection for verification
rg -n 'def |@property' src/omnibase_infra/nodes/node_ledger_projection_compute/handlers/handler_ledger_projection.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 645
HandlerLedgerProjection is missing the required describe() method.
The handler implements 4 of 5 required ProtocolHandler methods: handler_type, initialize(), shutdown(), and execute(). However, the describe() method for introspection is missing and must be implemented to satisfy the ProtocolHandler protocol contract.
🤖 Prompt for AI Agents
In `@src/omnibase_infra/nodes/node_ledger_projection_compute/contract.yaml` around
lines 72 - 81, The HandlerLedgerProjection handler is missing the required
describe() method from the ProtocolHandler contract; add a describe(self) method
to the HandlerLedgerProjection class that returns the handler's metadata (name,
handler_type, supported operations and a short description) consistent with
other handlers' describe implementations, ensuring the signature and return
shape match the ProtocolHandler's expected contract so
initialization/registration code that calls describe() works correctly.
| payload_raw = envelope.get("payload") | ||
| if not isinstance(payload_raw, dict): | ||
| raise RuntimeError("Missing or invalid 'payload' in envelope") |
There was a problem hiding this comment.
Use OnexError instead of RuntimeError.
Per coding guidelines, only OnexError or its subclasses should be raised. Replace RuntimeError with an appropriate error type.
🔧 Proposed fix
payload_raw = envelope.get("payload")
if not isinstance(payload_raw, dict):
- raise RuntimeError("Missing or invalid 'payload' in envelope")
+ raise OnexError("Missing or invalid 'payload' in envelope")As per coding guidelines: **/*.py: Only raise OnexError or its subclasses.
📝 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.
| payload_raw = envelope.get("payload") | |
| if not isinstance(payload_raw, dict): | |
| raise RuntimeError("Missing or invalid 'payload' in envelope") | |
| payload_raw = envelope.get("payload") | |
| if not isinstance(payload_raw, dict): | |
| raise OnexError("Missing or invalid 'payload' in envelope") |
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_ledger_projection_compute/handlers/handler_ledger_projection.py`
around lines 189 - 191, Replace the RuntimeError raised for a missing/invalid
envelope payload with an OnexError (or a more specific OnexError subclass) in
the payload validation block inside handler_ledger_projection.py where
payload_raw = envelope.get("payload"); ensure you import the OnexError symbol
(or the chosen subclass) and raise it with the same descriptive message
("Missing or invalid 'payload' in envelope") so the code follows the guideline
of only raising OnexError-derived exceptions.
- Extract handler ID to module-level constant HANDLER_ID_ARCHITECTURE_VALIDATION - Export constant in __all__ for consistent handler ID pattern - Aligns with HandlerLedgerProjection pattern for consistency
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/architecture_validator/handlers/handler_architecture_validation.py`:
- Line 42: Update the docstring examples that call a non-existent handle() to
call the actual public method validate_architecture() instead; specifically edit
the module-level example (currently showing "handler.handle(request)"), the
class docstring, and the method docstrings that reference handle() so they use
"handler.validate_architecture(request)" (or the correct call signature for
validate_architecture) to avoid AttributeError and keep examples consistent with
the public API.
| ... nodes=(my_orchestrator, my_reducer), | ||
| ... handlers=(my_handler,), | ||
| ... ) | ||
| >>> result = handler.handle(request) |
There was a problem hiding this comment.
Docstring examples reference non-existent handle() method.
The module docstring example uses handler.handle(request), but the actual public method is validate_architecture(). This inconsistency appears in multiple places (lines 42, 100, 240, 249) and will cause AttributeError for users copying the examples.
📝 Proposed fix for module docstring example
- >>> result = handler.handle(request)
+ >>> result = handler.validate_architecture(request)Apply similar fixes to class docstring (line 100) and method docstring (lines 240, 249).
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/architecture_validator/handlers/handler_architecture_validation.py`
at line 42, Update the docstring examples that call a non-existent handle() to
call the actual public method validate_architecture() instead; specifically edit
the module-level example (currently showing "handler.handle(request)"), the
class docstring, and the method docstrings that reference handle() so they use
"handler.validate_architecture(request)" (or the correct call signature for
validate_architecture) to avoid AttributeError and keep examples consistent with
the public API.
The declarative refactoring moved validation logic from the node to the handler. Update the runtime to use the new handler-based API instead of the removed node methods.
Update TestContainerHandling tests to match the new API where validation uses HandlerArchitectureValidation instead of NodeArchitectureValidatorCompute. - Renamed tests to reflect handler instantiation vs container injection - Updated patch targets to HandlerArchitectureValidation - Changed mock method from compute() to validate_architecture() - Updated assertions to verify rules kwarg instead of container
Standardize handler_id to "handler-architecture-validation" in contract.yaml to match HANDLER_ID_ARCHITECTURE_VALIDATION constant in Python module.
Summary
Resolves 18 violations by converting 3 imperative nodes to declarative shells with contract-driven handler routing.
HandlerLedgerProjectionwith all compute logicHandlerArchitectureValidationwith all validation logicAll handlers follow the
ProtocolHandlerpattern withcontract.yamldefining input/output models and operation bindings.Files Created
handlers/handler_ledger_projection.pyhandlers/handler_architecture_validation.pyhandlers/contract.yaml(x2)constants.pySUPPORTED_RULE_IDSDefinition of Done
NodeLedgerProjectionCompute- Extract to handlerNodeRegistryEffect- Use container DINodeArchitectureValidator- Extract to handlerpoetry run python scripts/validate.py declarative_nodespasses (validator in OMN-1725)Test plan
Linear: OMN-1726
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.