Repository navigation
feat(OMN-1732): Runtime dependency injection for zero-code nodes - #238
jonahgabriel merged 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds protocol dependency resolution: new ProtocolDependencyResolutionError, an immutable ModelResolvedDependencies container, a ContractDependencyResolver that imports and resolves protocol instances from a container and raises on missing required dependencies, wiring to accept pre-resolved dependencies in node persistence effects, and tests for resolver and model. Changes
Sequence DiagramsequenceDiagram
participant Node as NodeContractPersistenceEffect
participant Resolver as ContractDependencyResolver
participant Container as ModelONEXContainer
participant Registry as ServiceRegistry
participant Model as ModelResolvedDependencies
Node->>Resolver: resolve(contract, allow_missing=false)
activate Resolver
Resolver->>Resolver: _extract_protocol_dependencies(contract)
loop each protocol dependency
Resolver->>Resolver: _import_protocol_class(class_name, module)
Resolver->>Container: access service_registry
Container->>Registry: lookup(protocol_class)
Registry-->>Resolver: instance or None
alt resolved
Resolver->>Resolver: add to protocols map
else missing
Resolver->>Resolver: record missing_protocols
end
end
alt missing_protocols empty or allow_missing=true
Resolver->>Model: construct ModelResolvedDependencies(protocols)
Model-->>Resolver: ModelResolvedDependencies
Resolver-->>Node: return ModelResolvedDependencies
else required missing and allow_missing=false
Resolver-->>Node: raise ProtocolDependencyResolutionError(with missing_protocols context)
end
deactivate Resolver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/node_contract_persistence_effect/registry/registry_infra_contract_persistence_effect.py`:
- Around line 98-103: The example uses resolver.resolve(contract) but
ContractDependencyResolver.resolve() is async; change the example to await the
call so resolved holds the actual dependencies (i.e., use resolved = await
resolver.resolve(contract)) before passing them into
RegistryInfraContractPersistenceEffect.create(..., dependencies=resolved) to
avoid assigning a coroutine.
🧹 Nitpick comments (4)
tests/unit/runtime/test_contract_dependency_resolver.py (4)
1-22: Missing@pytest.mark.unitmarkers on test classes.Per coding guidelines, test files should use pytest markers for classification. This unit test file uses
@pytest.mark.asyncioon async tests but lacks the required@pytest.mark.unitmarker to classify tests appropriately.♻️ Proposed fix to add unit marker
Add the marker to each test class or apply it at module level:
import pytest +pytestmark = pytest.mark.unit + from omnibase_infra.errors import ProtocolDependencyResolutionErrorAs per coding guidelines:
tests/**/*.py: Use pytest markers:@pytest.mark.unit,@pytest.mark.integration,@pytest.mark.slow,@pytest.mark.chaos,@pytest.mark.serialfor test classification.
114-157: Unnecessary@pytest.mark.asyncioon synchronous tests.Tests
test_resolve_extracts_protocol_dependencies_is_protocol_method,test_resolve_extracts_protocol_dependencies_type_field, andtest_resolve_extracts_protocol_dependencies_enum_typecall the synchronous method_extract_protocol_dependenciesbut are marked as async. While this works, it adds unnecessary overhead.♻️ Example fix for one test
- `@pytest.mark.asyncio` - async def test_resolve_extracts_protocol_dependencies_is_protocol_method( + def test_resolve_extracts_protocol_dependencies_is_protocol_method( self, mock_container: MagicMock, mock_contract_with_protocols: MagicMock, ) -> None:
325-397: Unnecessary@pytest.mark.asyncioon synchronous helper method tests.Tests for
_import_protocol_class,_is_protocol_dependency, and_extract_protocol_dependenciesare all testing synchronous methods but are marked with@pytest.mark.asyncio. Consider removing the async markers andasynckeyword for clarity.
533-538: Consider using a more specific exception type for immutability test.The test catches a broad
Exceptionwhen testing frozen model immutability. Pydantic raisesValidationErrorwhen attempting to mutate a frozen model.♻️ Proposed improvement
def test_model_is_immutable(self) -> None: """Test that the model is frozen (immutable).""" + from pydantic import ValidationError + resolved = ModelResolvedDependencies(protocols={"Test": MagicMock()}) - with pytest.raises(Exception): # ValidationError or AttributeError + with pytest.raises(ValidationError): resolved.protocols = {} # type: ignore[misc]
| >>> | ||
| >>> # With pre-resolved dependencies (OMN-1732) | ||
| >>> resolved = resolver.resolve(contract) | ||
| >>> effect = RegistryInfraContractPersistenceEffect.create( | ||
| ... container, dependencies=resolved | ||
| ... ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine lines 98-103
cat -n src/omnibase_infra/nodes/node_contract_persistence_effect/registry/registry_infra_contract_persistence_effect.py | sed -n '90,110p'Repository: OmniNode-ai/omnibase_infra
Length of output: 1106
🏁 Script executed:
# Search for ContractDependencyResolver definition to check the resolve method
rg -n "class ContractDependencyResolver" --type py -A 20Repository: OmniNode-ai/omnibase_infra
Length of output: 2299
🏁 Script executed:
# Also search for any async resolve methods
rg -n "async def resolve" --type py | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 756
🏁 Script executed:
# Check the create method signature and full docstring context
cat -n src/omnibase_infra/nodes/node_contract_persistence_effect/registry/registry_infra_contract_persistence_effect.py | sed -n '80,110p'Repository: OmniNode-ai/omnibase_infra
Length of output: 1749
🏁 Script executed:
# Also check the class definition and method signatures
ast-grep --pattern 'def create($_) {
$$$
}'Repository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
# Look for async create method
rg -n "def create|async def create" src/omnibase_infra/nodes/node_contract_persistence_effect/registry/registry_infra_contract_persistence_effect.py -A 5Repository: OmniNode-ai/omnibase_infra
Length of output: 333
Async example should await resolver.
ContractDependencyResolver.resolve() is async, so the example at line 100 must use await to correctly resolve the dependencies. Without it, the code would assign a coroutine object instead of the resolved dependencies.
✏️ Suggested doc fix
- >>> resolved = resolver.resolve(contract)
+ >>> resolved = await resolver.resolve(contract)📝 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.
| >>> | |
| >>> # With pre-resolved dependencies (OMN-1732) | |
| >>> resolved = resolver.resolve(contract) | |
| >>> effect = RegistryInfraContractPersistenceEffect.create( | |
| ... container, dependencies=resolved | |
| ... ) | |
| >>> | |
| >>> # With pre-resolved dependencies (OMN-1732) | |
| >>> resolved = await resolver.resolve(contract) | |
| >>> effect = RegistryInfraContractPersistenceEffect.create( | |
| ... container, dependencies=resolved | |
| ... ) |
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_contract_persistence_effect/registry/registry_infra_contract_persistence_effect.py`
around lines 98 - 103, The example uses resolver.resolve(contract) but
ContractDependencyResolver.resolve() is async; change the example to await the
call so resolved holds the actual dependencies (i.e., use resolved = await
resolver.resolve(contract)) before passing them into
RegistryInfraContractPersistenceEffect.create(..., dependencies=resolved) to
avoid assigning a coroutine.
Implement ContractDependencyResolver that reads protocol dependencies from node contract.yaml files and resolves them from container.service_registry. Architecture (DEC-003): - Container: service provider (owns protocol instances) - Runtime (resolver): wiring authority (resolves + validates) - Node: consumer (receives resolved deps via constructor) Components added: - ContractDependencyResolver: Extracts deps from contracts, resolves from container - ModelResolvedDependencies: Pydantic model for resolved protocol instances - ProtocolDependencyResolutionError: Fail-fast error for missing protocols Changes: - NodeContractPersistenceEffect accepts optional dependencies parameter - RegistryInfra*.get_required_protocols() deprecated (contract is source of truth) Includes 31 unit tests covering resolution, fail-fast behavior, and mock injection.
9c7dc3e to
45110ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/services/contract_publisher/test_service.py`:
- Line 136: Three tests use the outdated topic string missing the "platform."
segment; update the inputs passed to ServiceContractPublisher.resolve_topic in
the tests named test_resolve_topic_* so they use
"onex.evt.platform.contract-registered.v1" instead of
"onex.evt.contract-registered.v1" (apply this change to the three occurrences
referenced near lines 488–489, 505–506, and 524–525) so the tests align with
TOPIC_SUFFIX_CONTRACT_REGISTERED and the platform namespace.
🧹 Nitpick comments (2)
tests/unit/runtime/test_contract_dependency_resolver.py (2)
24-26: Consider adding@pytest.mark.unitclass decorator.Per coding guidelines, test files should use pytest markers for test classification. Consider adding
@pytest.mark.unitto both test classes.♻️ Suggested change
+@pytest.mark.unit class TestContractDependencyResolver: """Tests for ContractDependencyResolver."""And similarly for
TestModelResolvedDependenciesat line 421.
129-157: Remove unnecessary@pytest.mark.asynciofrom synchronous tests.Tests for
_extract_protocol_dependencies(lines 129-157) and_import_protocol_class(lines 325-365) don't contain async code but have@pytest.mark.asyncio. While not a bug, this adds unnecessary overhead.♻️ Suggested change for lines 129-141
- `@pytest.mark.asyncio` - async def test_resolve_extracts_protocol_dependencies_type_field( + def test_resolve_extracts_protocol_dependencies_type_field( self, mock_container: MagicMock, mock_contract_with_type_field: MagicMock, ) -> None:Similar changes apply to other synchronous test methods.
… platform segment Update test_resolve_topic_* tests to use "onex.evt.platform.contract-registered.v1" instead of "onex.evt.contract-registered.v1" to match TOPIC_SUFFIX_CONTRACT_REGISTERED constant and platform-wide namespace evolution. Addresses CodeRabbit review feedback.
1 participant
Summary
Implement
ContractDependencyResolverthat reads protocol dependencies from nodecontract.yamlfiles and resolves them fromcontainer.service_registry.Architecture (DEC-003):
Changes
Components Added
ContractDependencyResolver: Extracts deps from contracts, resolves from containerModelResolvedDependencies: Pydantic model for resolved protocol instancesProtocolDependencyResolutionError: Fail-fast error for missing protocolsNode Changes
NodeContractPersistenceEffectaccepts optionaldependenciesparameterRegistryInfra*.get_required_protocols()deprecated (contract is source of truth)Files Changed
runtime/contract_dependency_resolver.pymodels/runtime/model_resolved_dependencies.pyerrors/error_infra.pyProtocolDependencyResolutionErrornodes/node_contract_persistence_effect/node.pynodes/node_contract_persistence_effect/registry/...get_required_protocols()Test Plan
ContractDependencyResolverModelResolvedDependenciesmodel methodsLinear Ticket
OMN-1732
Summary by CodeRabbit
New Features
Deprecations
Tests
Bug Fixes