feat(runtime): implement ContractHandlerDiscovery for contract-based handler registration [OMN-1133]

[jonahgabriel]

Summary

Implements ContractHandlerDiscovery that discovers handlers from contracts and auto-registers them with the runtime. This eliminates the need for manual handler wiring and integrates with RuntimeHostProcess.start().

Linear Ticket: OMN-1133

Changes

New Components

• ContractHandlerDiscovery (src/omnibase_infra/runtime/contract_handler_discovery.py)

  • Bridges HandlerPluginLoader and ProtocolBindingRegistry
  • Discovers handlers from contract files and auto-registers them
  • Graceful degradation: individual handler failures don't block discovery

• ProtocolHandlerDiscovery (src/omnibase_infra/runtime/protocol_handler_discovery.py)

  • Runtime-checkable protocol for handler discovery services
  • Defines discover_and_register(contract_paths) interface

• Discovery Models (src/omnibase_infra/models/runtime/)

  • ModelDiscoveryResult: Tracks handlers discovered/registered with errors/warnings
  • ModelDiscoveryError: Structured error tracking with error codes
  • ModelDiscoveryWarning: Non-fatal warning tracking

RuntimeHostProcess Integration

• Added contract_paths: list[str] | None parameter to __init__
• Auto-discovers handlers on start() if paths provided
• Falls back to wire_default_handlers() when no paths given
• Discovery errors are logged but don't block startup (graceful degradation)

Test Coverage

• 15 unit tests for ContractHandlerDiscovery

  • Protocol compliance, basic functionality, error handling
  • Correlation ID handling, mixed paths, registry integration

• 14 integration tests for RuntimeHostProcess discovery

  • Contract paths usage, fallback behavior, graceful degradation
  • Full lifecycle, restart, idempotent start/stop

Test plan

• All 15 unit tests pass for ContractHandlerDiscovery
• All 14 integration tests pass for RuntimeHostProcess discovery
• Pre-commit hooks pass (ruff, mypy, architecture validation)
• Pattern validation passes with exemptions
• CI pipeline passes

Dependencies

• ✅ OMN-1132 (Handler Plugin Loader) - Merged
• ✅ OMN-1134 (Registry Projection Extensions) - Merged

Summary by CodeRabbit

• New Features

  • Contract-based handler discovery at startup with a discovery protocol and service; discovery is non-fatal and reports aggregated results, errors, warnings, counts, timestamps, and success indicators.
  • Runtime host accepts contract paths to discover/register handlers or fall back to default wiring; health and logging surface contract path info and discovery outcomes.

• Tests

  • Extensive unit and integration tests covering discovery, error/warning handling, lifecycle, observability, and fallback behavior.

• Chores

  • Added validation exemptions for discovery-related files.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[linear]

OMN-1133

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds contract-based handler discovery: new discovery models and protocol, a ContractHandlerDiscovery implementation, runtime startup integration using contract paths (optional), validation exemptions, and extensive unit and integration tests for discovery and host behavior.

Changes

Cohort / File(s)	Summary
Discovery models src/omnibase_infra/models/runtime/model_discovery_error.py, src/omnibase_infra/models/runtime/model_discovery_warning.py, src/omnibase_infra/models/runtime/model_discovery_result.py, src/omnibase_infra/models/runtime/__init__.py	New immutable Pydantic models: ModelDiscoveryError, ModelDiscoveryWarning, ModelDiscoveryResult. Export symbols added; result model records counts, timestamps, errors/warnings and provides convenience properties and boolean semantics.
Discovery protocol & implementation src/omnibase_infra/runtime/protocol_handler_discovery.py, src/omnibase_infra/runtime/contract_handler_discovery.py	Added ProtocolHandlerDiscovery protocol and ContractHandlerDiscovery implementation. Discovery loads contracts (files/dirs) via a plugin loader, imports handler classes, registers them in a ProtocolBindingRegistry, aggregates results into ModelDiscoveryResult, and caches last result. Error/warning aggregation prevents whole-run failure.
Runtime integration src/omnibase_infra/runtime/runtime_host_process.py, src/omnibase_infra/runtime/__init__.py	RuntimeHostProcess accepts optional contract_paths and conditionally runs contract-based discovery (using ContractHandlerDiscovery) at startup or falls back to default wiring. Exports discovery types and surfaces observability (has_contract_paths, contract_path_count).
Validation exemptions src/omnibase_infra/validation/validation_exemptions.yaml	Added exemptions documenting new discovery modules and service files (OMN-1133 references).
Unit tests (discovery service) tests/unit/runtime/contract_handler_discovery/...	New fixtures, MockValidHandler, and extensive unit tests covering protocol compliance, single/multi-file discovery, mixed valid/invalid inputs, correlation ID handling, registry integration, observability, and logging.
Integration tests (runtime host) tests/integration/runtime/test_runtime_host_handler_discovery.py	Large integration suite validating end-to-end contract-based discovery during host startup: overriding default wiring, mixed-path handling, graceful degradation for invalid/non-existent paths, lifecycle (start/stop/restart), health checks, and logging.

Sequence Diagram(s)

sequenceDiagram
    participant RTH as RuntimeHostProcess
    participant CHD as ContractHandlerDiscovery
    participant PHL as ProtocolHandlerPluginLoader
    participant HReg as ProtocolBindingRegistry
    participant MDR as ModelDiscoveryResult

    RTH->>CHD: discover_and_register(contract_paths, correlation_id?)
    activate CHD
    loop per contract path
        CHD->>PHL: load_from_directory / load_from_contract(path)
        activate PHL
        PHL-->>CHD: handler metadata (class paths, names) / warnings
        deactivate PHL
        CHD->>CHD: _import_handler_class(class_path, correlation_id)
        CHD->>HReg: register(handler_name, handler_class)
        activate HReg
        HReg-->>CHD: registration result / errors
        deactivate HReg
        CHD->>CHD: collect counts, errors, warnings
    end
    CHD-->>RTH: ModelDiscoveryResult (handlers_discovered, handlers_registered, errors, warnings, discovered_at)
    deactivate CHD
    RTH->>RTH: log results, cache last result, continue startup

Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I hopped through contracts, sniffed each trace,

Found hidden handlers in filesystem space,
I noted each stumble and boxed every err,
Tallied the wins with a twitch of my fur,
Startup hums steady — hop, hop, hooray! 🎉

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review: Contract-Based Handler Discovery (OMN-1133)

This PR successfully implements contract-based handler discovery for ONEX infrastructure. The implementation is well-architected, thoroughly tested, and follows ONEX patterns consistently.

Strengths

• Clean separation of concerns between ContractHandlerDiscovery, HandlerPluginLoader, and ProtocolBindingRegistry
• Protocol-based design enables dependency injection and test isolation
• Graceful degradation: individual handler failures don't crash entire discovery
• Comprehensive documentation with examples and thread safety notes
• Strong type safety: no Any types, proper Pydantic models throughout
• Excellent error categorization with structured error codes
• 29 total tests (15 unit + 14 integration) with thorough coverage

Issues Found

CRITICAL: Exception Leak in Error Handler (line 416)

The catch-all exception handler at contract_handler_discovery.py:416 calls path.is_file() inside the except block, which could raise another OSError during error handling. This should be wrapped in a nested try/except or use cached path type.

Severity: Medium - could cause discovery to fail if filesystem errors occur during exception handling.

MINOR: Unused Import

InfraConnectionError appears to be imported but unused. Handler import errors are caught as ImportError, not InfraConnectionError. Verify if the InfraConnectionError handler at lines 393-408 is actually needed.

Severity: Low - doesn't affect functionality but dead code should be cleaned up.

Test Coverage

Excellent coverage of happy paths, error conditions, and edge cases. Tests properly isolate handlers requiring external services by using HttpRestHandler for most scenarios.

Security & Performance

Security model is sound - inherits controls from HandlerPluginLoader (YAML safe loading, file size limits, protocol validation). Performance characteristics are appropriate for startup-time operations.

ONEX Pattern Alignment

All patterns PASS: Strong Typing, Container DI, Protocol Resolution, OnexError Only, Correlation IDs, Graceful Degradation.

Final Verdict

APPROVE with minor fix required

This is an excellent implementation. The critical issue (exception leak) is straightforward to fix and doesn't affect core architecture. Once addressed, this PR is ready to merge.

Overall Quality: 5/5

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (6)

tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py (1)

125-144: Consider adding assertion for error presence in mixed valid/invalid test.

The test comment mentions "There may be errors from the invalid contracts" but doesn't assert on result.has_errors. Consider adding explicit verification:

# If invalid contracts produce errors, verify they're captured
# assert result.has_errors or result.handlers_discovered == 1

This would make the test behavior more explicit regarding whether invalid contracts are expected to produce errors in the result.

src/omnibase_infra/runtime/runtime_host_process.py (1)

957-1001: Consider caching discovery result for observability.

The discovery_result from discover_and_register() contains valuable information (discovered/registered counts, errors, warnings) that could be useful for:

• Health check enrichment
• Debugging startup issues
• Metrics/monitoring

Consider storing the result for later access:

💡 Suggested enhancement

+        # Store discovery result for observability (could be accessed via health_check)
+        self._last_discovery_result: ModelDiscoveryResult | None = None
+
         # Discover and register handlers from contract paths
         discovery_result = await self._handler_discovery.discover_and_register(
             contract_paths=self._contract_paths,
         )
+        self._last_discovery_result = discovery_result

tests/integration/runtime/test_runtime_host_handler_discovery.py (1)

356-386: Potential test pollution from singleton registry usage.

Lines 376-378 use get_handler_registry() (singleton) to verify default handlers. Since RuntimeHostProcess may register to the singleton when no handler_registry is provided, this could cause test pollution if tests run in a specific order.

Consider using the isolated_handler_registry fixture and passing it to RuntimeHostProcess:

💡 Suggested fix to avoid test pollution

     @pytest.mark.asyncio
-    async def test_fallback_to_default_handlers(self) -> None:
+    async def test_fallback_to_default_handlers(
+        self,
+        isolated_handler_registry: ProtocolBindingRegistry,
+    ) -> None:
         """RuntimeHostProcess uses wire_default_handlers when no contract_paths."""
         event_bus = InMemoryEventBus()
         process = RuntimeHostProcess(
             event_bus=event_bus,
             input_topic="test.input",
+            handler_registry=isolated_handler_registry,
             # No contract_paths - should use wire_default_handlers
         )

         try:
             await process.start()

             # Verify default handlers are available
-            registry = get_handler_registry()
-            assert registry.is_registered(HANDLER_TYPE_HTTP)
-            assert registry.is_registered(HANDLER_TYPE_DATABASE)
+            assert isolated_handler_registry.is_registered(HANDLER_TYPE_HTTP)
+            assert isolated_handler_registry.is_registered(HANDLER_TYPE_DATABASE)

src/omnibase_infra/runtime/contract_handler_discovery.py (3)

1-85: Docstring/examples look slightly stale (names don’t match actual protocol types).

The example imports HandlerPluginLoader / get_handler_registry, but this module type-hints ProtocolHandlerPluginLoader / ProtocolBindingRegistry. Consider aligning the example to the real public API to avoid misleading copy/paste.

---

137-217: async def wraps fully synchronous I/O/import work (likely blocks the event loop).

discover_and_register() performs filesystem checks, YAML loading (via loader), imports, and registry mutations synchronously. If this runs on an asyncio event loop (e.g., RuntimeHostProcess startup), it can stall other tasks.

Consider either:

• making the API synchronous, or
• offloading the heavy sync work (load_from_*, maybe imports) via asyncio.to_thread.

---

289-363: Import failures aren’t consistently classified as “import” errors (AttributeError/TypeError leak into generic bucket).

_import_handler_class() can raise AttributeError, ValueError, TypeError, but the caller only treats ImportError specially; the rest become REGISTRATION_UNEXPECTED_ERROR, which is noisy and hides the common “class not found / not a class” cases.

Consider normalizing _import_handler_class() to raise ImportError for “module/class resolution” problems so the caller’s except ImportError path reliably captures them.

Proposed fix

 def _import_handler_class(
@@
-        if "." not in class_path:
-            raise ValueError(
-                f"Invalid class path '{class_path}': must be fully qualified "
-                "(e.g., 'myapp.handlers.AuthHandler')"
-            )
+        if "." not in class_path:
+            raise ImportError(
+                f"Invalid class path {class_path!r}: must be fully qualified "
+                "(e.g., 'myapp.handlers.AuthHandler')"
+            )

         module_path, class_name = class_path.rsplit(".", 1)
-        module = importlib.import_module(module_path)
-        handler_class = getattr(module, class_name)
+        try:
+            module = importlib.import_module(module_path)
+        except ModuleNotFoundError as e:
+            raise ImportError(f"Module not found: {module_path!r}") from e
+
+        try:
+            handler_class = getattr(module, class_name)
+        except AttributeError as e:
+            raise ImportError(
+                f"Class not found: {class_name!r} in module {module_path!r}"
+            ) from e

         # Verify it's actually a class (also serves as type narrowing for mypy)
         if not isinstance(handler_class, type):
-            raise TypeError(
-                f"'{class_path}' is not a class (got {type(handler_class).__name__})"
-            )
+            raise ImportError(
+                f"{class_path!r} is not a class (got {type(handler_class).__name__})"
+            )

Also applies to: 448-514

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between 6151ca0 and 2b22971.

📒 Files selected for processing (13)

• src/omnibase_infra/models/runtime/__init__.py
• src/omnibase_infra/models/runtime/model_discovery_error.py
• src/omnibase_infra/models/runtime/model_discovery_result.py
• src/omnibase_infra/models/runtime/model_discovery_warning.py
• src/omnibase_infra/runtime/__init__.py
• src/omnibase_infra/runtime/contract_handler_discovery.py
• src/omnibase_infra/runtime/protocol_handler_discovery.py
• src/omnibase_infra/runtime/runtime_host_process.py
• src/omnibase_infra/validation/validation_exemptions.yaml
• tests/integration/runtime/test_runtime_host_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/__init__.py
• tests/unit/runtime/contract_handler_discovery/conftest.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Never use Any type - use object for generic payloads in function parameters and return types
Use X | None (PEP 604) instead of Optional[X] for nullable types
All services must use ModelONEXContainer for dependency injection via __init__(self, container: ModelONEXContainer)
Use @allow_any decorator with documented reason as exemption mechanism for Any type violations
Use JsonType from omnibase_core.types as the canonical type alias for JSON-compatible values
Use ModelEventEnvelope[object] for generic dispatcher interfaces and object for generic payloads
Infrastructure error handling must use OnexError base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, InfraUnavailableError for transport failures with proper ModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated with uuid4() if missing, and included in all error contexts
External service adapters must implement MixinAsyncCircuitBreaker with appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never use isinstance checks

Files:

• tests/unit/runtime/contract_handler_discovery/__init__.py
• src/omnibase_infra/models/runtime/model_discovery_warning.py
• src/omnibase_infra/models/runtime/model_discovery_error.py
• src/omnibase_infra/runtime/contract_handler_discovery.py
• src/omnibase_infra/runtime/__init__.py
• src/omnibase_infra/models/runtime/model_discovery_result.py
• src/omnibase_infra/runtime/runtime_host_process.py
• tests/unit/runtime/contract_handler_discovery/conftest.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py
• src/omnibase_infra/runtime/protocol_handler_discovery.py
• src/omnibase_infra/models/runtime/__init__.py
• tests/integration/runtime/test_runtime_host_handler_discovery.py

**/model_*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/model_*.py: File naming convention: Models must use model_<name>.py with class name Model<Name>
Each model file must contain exactly one Model* class
Pydantic workaround for Any type must include # NOTE: comment documenting the reason when technically required
Use SerializeAsAny type wrapper for Pydantic fields containing complex nested models to preserve subclass fields during serialization

Files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py
• src/omnibase_infra/models/runtime/model_discovery_error.py
• src/omnibase_infra/models/runtime/model_discovery_result.py

**/model_*result*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Result models may override __bool__ to enable idiomatic conditional checks, with required Warning section in docstring explaining non-standard behavior

Files:

• src/omnibase_infra/models/runtime/model_discovery_result.py

**/protocol_*.py

📄 CodeRabbit inference engine (CLAUDE.md)

File naming convention: Protocols must use protocol_<name>.py or protocols.py with class name Protocol<Name>

Files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

🧠 Learnings (42)

📓 Common learnings

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

📚 Learning: 2026-01-06T17:57:00.677Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.677Z
Learning: Applies to src/omnibase_spi/**/*.py : SPI MUST NOT define Pydantic models; all `BaseModel` classes must be defined in omnibase_core

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py

📚 Learning: 2025-11-24T17:22:32.195Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/models/*.py : All Pydantic models in models/ directory must be auto-generated from contract.yaml; never hand-write models

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py

📚 Learning: 2025-12-06T22:21:32.649Z

Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to shared/models/model_*.py : Use `model_*` prefix for Pydantic models in `shared/models/` directory

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py

📚 Learning: 2025-11-24T16:33:32.747Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/models/model_contract_*.py : Models must be auto-generated from contract.yaml files, never hand-written

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py

📚 Learning: 2025-11-24T17:23:49.777Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py
• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T17:22:32.195Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/models/*.py : All Pydantic models must use Field() with description for all properties; never use inline type hints without Field()

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py

📚 Learning: 2025-11-28T18:58:53.781Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Organize models under `src/omnibase_core/models/` by domain including: base, cli, common, config, core, contracts, discovery, health, infrastructure, logging, metadata, nodes, operations, results, security, service, tools, validation, and workflows

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_warning.py
• src/omnibase_infra/models/runtime/__init__.py

📚 Learning: 2026-01-11T18:12:47.296Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_error.py

📚 Learning: 2025-11-24T17:23:49.777Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/error_codes.py : All ONEX node error handling must use auto-generated error codes defined in `models/error_codes.py` from contract definitions

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_error.py

📚 Learning: 2025-11-28T18:58:53.781Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use `EnumCoreErrorCode` with `ModelOnexError` for proper error code usage

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_error.py

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py
• src/omnibase_infra/runtime/__init__.py
• src/omnibase_infra/runtime/runtime_host_process.py
• tests/unit/runtime/contract_handler_discovery/conftest.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py
• src/omnibase_infra/validation/validation_exemptions.yaml
• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2026-01-11T18:12:47.296Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py
• src/omnibase_infra/runtime/runtime_host_process.py
• tests/unit/runtime/contract_handler_discovery/conftest.py
• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py
• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T16:33:32.747Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/*.py : Import protocols from `omnibase.protocol.protocol_*` paths

Applied to files:

• src/omnibase_infra/runtime/__init__.py
• src/omnibase_infra/runtime/runtime_host_process.py
• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/*.py : Import protocols from `omnibase.protocol.protocol_<name>` module paths

Applied to files:

• src/omnibase_infra/runtime/__init__.py
• src/omnibase_infra/runtime/runtime_host_process.py
• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2026-01-06T17:57:00.677Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.677Z
Learning: Applies to src/omnibase_spi/protocols/handlers/**/*.py : Protocol naming convention: Handler protocols must follow `Protocol{Type}Handler` pattern

Applied to files:

• src/omnibase_infra/runtime/__init__.py
• src/omnibase_infra/validation/validation_exemptions.yaml
• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol for tool interfaces and plugin APIs based on method shape (structural typing), not Pydantic models with inheritance

Applied to files:

• src/omnibase_infra/runtime/__init__.py
• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/model_*result*.py : Result models may override `__bool__` to enable idiomatic conditional checks, with required `Warning` section in docstring explaining non-standard behavior

Applied to files:

• src/omnibase_infra/models/runtime/model_discovery_result.py

📚 Learning: 2025-11-24T16:32:55.606Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Node contracts must be validated using `ModelCounter` from `omnibase_core.validation.architecture`

Applied to files:

• src/omnibase_infra/runtime/runtime_host_process.py

📚 Learning: 2025-12-06T22:21:32.649Z

Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern

Applied to files:

• src/omnibase_infra/runtime/runtime_host_process.py

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_*.py : Handlers must NOT have direct event bus access - only orchestrators may have bus parameters and publish events

Applied to files:

• src/omnibase_infra/runtime/runtime_host_process.py

📚 Learning: 2025-11-24T16:33:51.604Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/**/conftest.py : Test fixtures must be defined in `conftest.py` and should provide reusable sample data, UUIDs, semantic versions, and model data

Applied to files:

• tests/unit/runtime/contract_handler_discovery/conftest.py

📚 Learning: 2025-11-28T18:58:53.781Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to tests/**/*.py : Write comprehensive test coverage following the test structure under `tests/unit/` organized by subsystem (enums, models, mixins, utils)

Applied to files:

• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

📚 Learning: 2025-11-24T17:23:49.777Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T17:23:49.777Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_capabilities.yaml : All ONEX node execution capability definitions, if applicable, must be included in contract_capabilities.yaml with supported_node_types, supported_delivery_modes, and performance_constraints specifications

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T17:23:49.777Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must support optional documents pattern with optional flag and required_capability field for future extensibility

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/service_*.py : File naming convention: Services must use `service_<name>.py` with class name `Service<Name>`

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2026-01-11T18:12:47.296Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/src/omnibase_core/**/*.py : Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_*, container_*, decorator_*, enum_*, error_*, factory_*, mixin_*, model_*, node_*, runtime_*, service_*, etc.

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-12-07T17:50:13.678Z

Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use file prefix naming conventions in Python files: model_* for Pydantic models, enum_* for enumerations, protocol_* for protocol interfaces, service_* for service implementations, node_* for ONEX nodes

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/protocol_*.py : File naming convention: Protocols must use `protocol_<name>.py` or `protocols.py` with class name `Protocol<Name>`

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Protocol class names must follow the pattern `Protocol<Name>` (e.g., `ProtocolFileGenerator`)

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/models/model_contract_{actions,models,validation,cli,capabilities}.py : Generated models from subcontracts must follow the naming pattern: `model_contract_actions.py`, `model_contract_models.py`, `model_contract_validation.py`, etc.

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/models/model_contract_*.py : Contract-backed model files must follow the naming pattern `model_contract_<domain>.py` and be located in `*/models/` directories

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T16:33:32.747Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/models/model_contract_*.py : Contract model files must follow the naming pattern `model_contract_<domain>.py` and be located in `*/models/` directories

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/!(tool_)+(handler_|utils_|core_)*.py : Do not use prefix patterns `handler_`, `utils_`, or `core_` for file names; use `tool_` prefix instead for business logic files

Applied to files:

• src/omnibase_infra/validation/validation_exemptions.yaml

📚 Learning: 2026-01-06T17:57:00.677Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.677Z
Learning: Applies to src/omnibase_spi/protocols/**/*.py : Every protocol must inherit from `typing.Protocol` and have the `runtime_checkable` decorator

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:22:32.195Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol from typing module for all interface definitions; never use ABC (Abstract Base Classes) for service interfaces

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T16:33:32.747Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Use Protocol for interface definitions when implementations may live outside core codebase; use Pydantic models only for base classes with shared logic

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Protocol files must follow the naming pattern `protocol_<name>.py` and be located in `*/protocols/` directories

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2026-01-06T17:57:00.677Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.677Z
Learning: Applies to src/omnibase_spi/protocols/contracts/**/*.py : Protocol naming convention: Compiler protocols must follow `Protocol{Type}ContractCompiler` pattern

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T16:33:32.747Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/*.py : Import models from shared core paths using `omnibase.model.core.model_*` pattern

Applied to files:

• src/omnibase_infra/models/runtime/__init__.py

🧬 Code graph analysis (6)

src/omnibase_infra/runtime/contract_handler_discovery.py (6)

src/omnibase_infra/errors/error_infra.py (2)

• InfraConnectionError (232-339)
• ProtocolConfigurationError (154-189)

src/omnibase_infra/models/runtime/model_discovery_error.py (1)

• ModelDiscoveryError (23-78)

src/omnibase_infra/models/runtime/model_discovery_result.py (2)

• ModelDiscoveryResult (26-159)
• has_errors (86-98)

src/omnibase_infra/models/runtime/model_discovery_warning.py (1)

• ModelDiscoveryWarning (23-71)

src/omnibase_infra/runtime/registry/registry_protocol_binding.py (2)

• RegistryError (82-123)
• ProtocolBindingRegistry (131-439)

src/omnibase_infra/runtime/protocol_handler_plugin_loader.py (1)

• ProtocolHandlerPluginLoader (80-322)

src/omnibase_infra/runtime/__init__.py (2)

src/omnibase_infra/runtime/protocol_handler_discovery.py (1)

• ProtocolHandlerDiscovery (64-218)

src/omnibase_infra/runtime/contract_handler_discovery.py (1)

• ContractHandlerDiscovery (87-514)

src/omnibase_infra/models/runtime/model_discovery_result.py (2)

src/omnibase_infra/models/runtime/model_discovery_error.py (1)

• ModelDiscoveryError (23-78)

src/omnibase_infra/models/runtime/model_discovery_warning.py (1)

• ModelDiscoveryWarning (23-71)

tests/unit/runtime/contract_handler_discovery/conftest.py (3)

src/omnibase_infra/runtime/contract_handler_discovery.py (1)

• ContractHandlerDiscovery (87-514)

src/omnibase_infra/runtime/handler_plugin_loader.py (1)

• HandlerPluginLoader (237-1984)

src/omnibase_infra/runtime/registry/registry_protocol_binding.py (1)

• ProtocolBindingRegistry (131-439)

tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py (6)

src/omnibase_infra/models/runtime/model_discovery_result.py (3)

• ModelDiscoveryResult (26-159)
• has_errors (86-98)
• has_warnings (101-113)

src/omnibase_infra/runtime/contract_handler_discovery.py (2)

• ContractHandlerDiscovery (87-514)
• discover_and_register (137-446)

src/omnibase_infra/runtime/handler_plugin_loader.py (1)

• HandlerPluginLoader (237-1984)

src/omnibase_infra/runtime/registry/registry_protocol_binding.py (1)

• ProtocolBindingRegistry (131-439)

src/omnibase_infra/runtime/protocol_handler_discovery.py (2)

• ProtocolHandlerDiscovery (64-218)
• discover_and_register (104-218)

tests/unit/runtime/contract_handler_discovery/conftest.py (6)

• discovery_service (116-128)
• handler_registry (96-102)
• valid_contract_path (132-150)
• valid_contract_directory (154-186)
• empty_directory (225-233)
• mixed_valid_invalid_directory (190-221)

src/omnibase_infra/models/runtime/__init__.py (3)

src/omnibase_infra/models/runtime/model_discovery_error.py (1)

• ModelDiscoveryError (23-78)

src/omnibase_infra/models/runtime/model_discovery_result.py (1)

• ModelDiscoveryResult (26-159)

src/omnibase_infra/models/runtime/model_discovery_warning.py (1)

• ModelDiscoveryWarning (23-71)

🔇 Additional comments (28)

src/omnibase_infra/models/runtime/__init__.py (1)

12-14: LGTM!

The new discovery model imports and exports are properly structured, follow the model_* naming convention, and maintain alphabetical ordering in __all__.

Also applies to: 28-30

tests/unit/runtime/contract_handler_discovery/__init__.py (1)

1-6: LGTM!

Standard test package initializer with proper license header and documentation.

src/omnibase_infra/validation/validation_exemptions.yaml (2)

656-673: LGTM!

Service naming exemptions properly documented with clear rationale and references to CLAUDE.md conventions.

---

1184-1208: LGTM!

Contract handler discovery exemptions correctly follow the established exemption pattern and align with the PR's OMN-1133 objectives.

src/omnibase_infra/models/runtime/model_discovery_error.py (1)

1-81: LGTM!

The ModelDiscoveryError model is well-implemented with proper typing (object instead of Any for the details field), follows naming conventions, and includes comprehensive documentation. The frozen, strict configuration ensures data integrity.

src/omnibase_infra/models/runtime/model_discovery_result.py (3)

26-83: LGTM!

The ModelDiscoveryResult model is well-structured with proper field typing, default factories for mutable fields, and appropriate validation constraints. The model is intentionally not frozen (unlike ModelDiscoveryError) to support potential result mutation during aggregation.

---

85-113: LGTM!

The has_errors and has_warnings properties provide a clean, expressive API for checking discovery status.

---

115-159: LGTM!

The __bool__ override correctly follows coding guidelines by including a comprehensive Warning section documenting the non-standard behavior (lines 118-147). This enables idiomatic conditional checks like if result: while clearly documenting the semantic difference from standard Pydantic models.

Based on coding guidelines for **/model_*result*.py files.

src/omnibase_infra/models/runtime/model_discovery_warning.py (1)

1-74: LGTM! Well-structured discovery warning model.

The model follows all conventions:

• File naming (model_discovery_warning.py → ModelDiscoveryWarning) is correct
• Uses PEP 604 union syntax (Path | None, str | None)
• Immutable (frozen=True) with strict validation
• All fields use Field() with descriptions
• Proper __all__ export

src/omnibase_infra/runtime/__init__.py (1)

157-163: LGTM! Clean public API exposure for discovery components.

The new exports follow the established module patterns:

• Section comment links to ticket OMN-1133
• Imports are organized after the plugin loader section
• __all__ entries are properly placed with matching comment

Also applies to: 286-288

src/omnibase_infra/runtime/protocol_handler_discovery.py (2)

57-60: Verify runtime type annotation availability.

The ModelDiscoveryResult is imported under TYPE_CHECKING, which means it's only available during static type checking. Since this is a Protocol with runtime_checkable, the return type annotation should work correctly at runtime (Python handles forward references in annotations). However, if any runtime introspection of the return type is needed, it would fail.

This pattern is acceptable for protocol definitions where runtime type checking of return values is not performed.

---

63-218: Well-designed protocol with comprehensive documentation.

The protocol follows all ONEX conventions:

• Uses @runtime_checkable decorator
• Inherits from typing.Protocol (not ABC)
• Uses duck typing pattern with guidance on verification via hasattr
• PEP 604 syntax for optional types (UUID | None)
• Comprehensive docstrings with examples, thread safety notes, and error handling documentation

The ... (Ellipsis) body is the correct convention for Protocol methods per PEP 544.

tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py (3)

27-44: LGTM! Protocol compliance tests.

Good coverage of protocol compliance:

• isinstance check works because ProtocolHandlerDiscovery is @runtime_checkable
• Method existence and callability verification

---

255-275: LGTM! Registry integration tests with good coverage.

The tests verify:

• Handlers can be retrieved after discovery
• Multiple discoveries accumulate (or overwrite) registrations

The comment on line 268 clarifies the expected behavior (overwrite on re-discovery), which is a good documentation practice.

---

109-123: The error code "PATH_NOT_FOUND" used in the test matches the implementation in ContractHandlerDiscovery.py (line 254), where it is raised when a path does not exist. No changes needed.

src/omnibase_infra/runtime/runtime_host_process.py (4)

284-324: Comprehensive documentation for contract_paths parameter.

The docstring clearly explains:

• Purpose and behavior
• Path types (directories vs files)
• Error handling semantics (graceful degradation)
• Example usage

---

336-343: LGTM! Proper initialization of contract discovery state.

Good implementation choices:

• Converts list[str] to list[Path] upfront for consistent filesystem operations
• Lazy initialization of _handler_discovery (only created if contract_paths provided)

---

892-919: Clean separation of discovery vs wiring logic.

The _discover_or_wire_handlers() method provides a clear branch point:

• If contract_paths is truthy: use contract-based discovery
• Otherwise: fall back to wire_handlers() (which aliases wire_default_handlers)

The docstring accurately describes both modes.

---

961-964: HandlerPluginLoader is created but never stored.

A new HandlerPluginLoader() is created each time _discover_handlers_from_contracts() is called. While the current implementation only calls this once during start(), consider whether this should be:

1. Stored as an instance variable for potential reuse
2. Passed via constructor for dependency injection (testability)

For the current use case (single call during startup), this is acceptable.

tests/unit/runtime/contract_handler_discovery/conftest.py (3)

50-88: LGTM! MockValidHandler correctly implements ProtocolHandler interface.

The mock handler implements all 5 required protocol methods:

• handler_type (property)
• initialize(config: dict[str, object])
• shutdown(timeout_seconds: float)
• execute(request: object, operation_config: object)
• describe() (classmethod)

Type hints use object instead of Any, following coding guidelines.

---

189-221: Good coverage of error scenarios in mixed_valid_invalid_directory.

The fixture creates three distinct scenarios:

1. Valid contract with importable handler
2. Invalid YAML syntax (unclosed bracket)
3. Valid YAML but missing required handler_class field

This provides comprehensive coverage for graceful degradation testing.

---

131-150: Handler class path resolution is correct and will resolve properly at test time.

The fixture correctly uses f"{__name__}.MockValidHandler", which resolves to tests.unit.runtime.contract_handler_discovery.conftest.MockValidHandler at runtime. The MockValidHandler class exists in the conftest module, and ContractHandlerDiscovery._import_handler_class() properly handles this path using importlib.import_module() to dynamically import the module and getattr() to retrieve the class. The test module hierarchy includes the necessary __init__.py file, ensuring the path is importable by pytest.

tests/integration/runtime/test_runtime_host_handler_discovery.py (4)

86-93: LGTM! Isolated handler registry fixture.

Creating a fresh ProtocolBindingRegistry() instance for tests provides isolation from the singleton registry, preventing test pollution.

---

559-617: Comprehensive lifecycle tests with proper cleanup.

The lifecycle tests cover:

• Full start/stop cycle with discovered handlers
• Restart after stop (with fresh event bus)
• Idempotent start/stop behavior

Good practice: The comment on lines 606-607 acknowledges that singleton registry state from other tests may affect healthy status, focusing the test on is_running instead.

---

729-778: LGTM! Logging verification test.

The test uses caplog to verify that discovery/registration generates appropriate log messages. The flexible assertion (has_discovery_log or has_registered_log) accommodates implementation variations while still ensuring observability.

---

781-787: Test classes exported via all.

Good practice for test module organization, making it clear which test classes are the public API of this test module.

src/omnibase_infra/runtime/contract_handler_discovery.py (2)

121-136: This class correctly uses constructor-based dependency injection with concrete protocol types rather than ModelONEXContainer. As a stateless utility coordinator that bridges the plugin loader and registry, concrete dependencies are the appropriate pattern here. Note that HandlerPluginLoader is never registered in the container and is instantiated inline throughout the codebase, including in RuntimeHostProcess which instantiates ContractHandlerDiscovery with concrete deps. The container-based DI guideline applies to infrastructure services (PolicyRegistry, ProtocolBindingRegistry) registered in container_wiring.py, not to utility coordinators following the duck-typing protocol pattern.

Likely an incorrect or invalid review comment.

---

289-307: Registry key must be protocol type, not handler name: loaded.handler_name is incorrect.

ProtocolBindingRegistry.register(protocol_type, handler_cls) expects protocol type identifiers like "http", "db", "kafka" as the first argument. The code registers with loaded.handler_name (e.g., "auth.validate_token"), which is a friendly identifier, not a protocol type. At runtime, handler_registry.get(handler_type) will fail because the registry was keyed by handler_name instead of the protocol type. Additionally, ModelLoadedHandler lacks a protocol_type field—the contract model must extract or provide the protocol type identifier from the handler contract YAML.

⛔ Skipped due to learnings

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.677Z
Learning: Applies to src/omnibase_spi/protocols/handlers/**/*.py : Protocol naming convention: Handler protocols must follow `Protocol{Type}Handler` pattern

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/protocols/protocol_*.py : All Protocol definitions must use model-only signatures: methods accept only validated Pydantic models, never dict, primitives, or argument models

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Protocol method signatures must use Pydantic models only, never primitives or dicts as parameters or return types

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/protocols/protocol_*.py : All protocol method signatures must use Pydantic models exclusively, never primitives or dicts

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/protocols/protocol_*.py : Protocol class names must follow the pattern `Protocol<Name>` using PascalCase

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Protocol class names must follow the pattern `Protocol<Name>` (e.g., `ProtocolFileGenerator`)

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol for tool interfaces and plugin APIs based on method shape (structural typing), not Pydantic models with inheritance

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Avoid using Any, dict, or primitive types in protocol signatures; use the strongest typing possible with Pydantic models

[claude]

Pull Request Review: Contract-Based Handler Discovery (OMN-1133)

Overview

This PR implements ContractHandlerDiscovery to enable automatic handler discovery from YAML contracts, eliminating manual handler wiring. The implementation follows ONEX architectural patterns with graceful degradation and comprehensive test coverage.

---

✅ Code Quality & Best Practices

Excellent Adherence to ONEX Patterns

Strong Points:

1. Declarative Architecture - Discovery logic properly separated from registration
2. Protocol-Driven Design - ProtocolHandlerDiscovery enables dependency injection and testability
3. PEP 604 Type Annotations - Consistently uses X | None throughout
4. Custom __bool__ Pattern - ModelDiscoveryResult.__bool__() correctly follows ADR with proper warning documentation
5. Error Sanitization - All error messages avoid leaking sensitive data
6. Strong Typing - No Any types detected

Documentation Quality

• Module docstrings: Comprehensive with examples, thread safety notes, and version tags
• Method docstrings: Include Args, Returns, Raises, Examples, and Notes sections
• Inline comments: Well-placed to explain non-obvious design decisions (e.g., line 454 explains why stored is_file variable is used)

---

🔒 Security Considerations

Positive Security Patterns

1. Error Context Sanitization - Discovery errors expose only safe metadata (paths, handler names, error codes)
2. Import Safety - Dynamic imports use importlib.import_module() (safer than exec/eval)
3. Path Validation - Handles OSError exceptions from filesystem operations

Security Recommendation

CRITICAL: Per CLAUDE.md, YAML contracts are treated as executable code. Consider adding namespace allowlisting:

loader = HandlerPluginLoader(
    allowed_namespaces=["omnibase_infra.", "omnibase_core."]
)

Recommendation: Add allowed_namespaces parameter to RuntimeHostProcess.__init__() for production security.

---

🐛 Potential Bugs & Issues

Critical Issues

None identified - The second commit (f61a8fd) addressed the critical exception leak bug.

Minor Issues

1. Protocol Docstring Discrepancy (protocol_handler_discovery.py:145)

   • Protocol docstring mentions discovered_count, registered_count, failed_count fields
   • ModelDiscoveryResult uses handlers_discovered, handlers_registered (no failed_count)
   • Impact: Documentation mismatch could confuse implementers
   • Fix: Update protocol docstring to match actual model fields

2. Async Method with Sync I/O

   • discover_and_register() is async but performs synchronous file I/O
   • Verdict: Acceptable per docstring justification (startup simplicity)

---

⚡ Performance Considerations

Positive Patterns:

• Module import caching leverages Python's sys.modules
• Early validation prevents duplicate work
• Graceful degradation (individual failures don't block others)

Notes:

• Blocking I/O acceptable for startup (runs once)
• Future enhancement: Parallelize with asyncio.gather() for large codebases

---

🔬 Test Coverage Assessment

Coverage (29 tests total)

✅ Unit Tests (15): Protocol compliance, discovery, error handling, correlation IDs
✅ Integration Tests (14): RuntimeHostProcess integration, fallback, lifecycle

Coverage Gaps

1. Missing: Concurrent Discovery Test - Docstring claims thread safety but no concurrent test
2. Missing: Large Codebase Test - No test with 50+ contracts for performance validation
3. Missing: Namespace Allowlisting Test - Security-critical feature (CLAUDE.md) needs coverage

---

🏗️ Architecture & Design

Excellent Architectural Decisions

1. Two-Layer Design: Clean separation (HandlerPluginLoader vs ContractHandlerDiscovery)
2. Result Model Pattern: Aggregates results without throwing exceptions
3. RuntimeHostProcess Integration: Clean fallback logic preserves existing behavior
4. Lazy Initialization: Discovery service created only when needed

---

🎯 Recommendations Summary

Must Fix Before Merge

1. Update Protocol Docstring - Fix field name mismatch (discovered_count → handlers_discovered)

Should Fix (High Priority)

2. Add Namespace Allowlist Parameter - Enhance production security
3. Add Concurrent Discovery Test - Verify thread safety claims

Consider (Medium Priority)

4. Document Validation Exemptions - Ensure 43 new exemptions have rationale/ticket refs
5. Add Performance Benchmark Test - Test with 50+ contracts

---

✅ Final Verdict

APPROVED with recommendations

Strengths:

• ✅ Protocol-driven design
• ✅ Graceful error handling
• ✅ Comprehensive documentation
• ✅ 29 tests (15 unit + 14 integration)
• ✅ Follows ONEX conventions (PEP 604, no Any types, strong typing)

Action Items:

1. Fix protocol docstring field name mismatch (critical for API clarity)
2. Add namespace allowlisting for production security (high priority)
3. Add concurrent discovery test (validates thread safety claims)

No blocking issues - Ready to merge once critical docstring fix is applied.

---

Reviewed by: Claude Sonnet 4.5
Review Date: 2026-01-11

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py (2)

192-204: Consider strengthening correlation ID preservation test.

The test verifies the method completes successfully with a provided correlation_id, but doesn't actually assert that the ID was used in the result or logs. Consider adding verification, such as checking caplog for the correlation ID in log output.

♻️ Optional enhancement

     @pytest.mark.asyncio
     async def test_preserves_provided_correlation_id(
         self,
         discovery_service: ContractHandlerDiscovery,
         empty_directory: Path,
+        caplog: pytest.LogCaptureFixture,
     ) -> None:
         """Test that provided correlation ID is used."""
+        import logging
         correlation_id = uuid4()
-        result = await discovery_service.discover_and_register(
-            [empty_directory],
-            correlation_id=correlation_id,
-        )
-        assert isinstance(result, ModelDiscoveryResult)
+        with caplog.at_level(logging.DEBUG):
+            result = await discovery_service.discover_and_register(
+                [empty_directory],
+                correlation_id=correlation_id,
+            )
+        assert isinstance(result, ModelDiscoveryResult)
+        # Verify correlation ID appears in logs
+        assert any(str(correlation_id) in record.message or 
+                   str(correlation_id) in str(getattr(record, 'correlation_id', ''))
+                   for record in caplog.records)

---

286-306: Minor: Comment-test alignment.

The comment on line 299 states "should overwrite" but the test only verifies handlers remain registered, not that overwriting actually occurred. Consider either updating the comment to reflect what's actually being tested (idempotency) or adding verification that demonstrates overwriting behavior.

src/omnibase_infra/runtime/contract_handler_discovery.py (2)

391-404: Add exception comment marker for catch-all handler.

Per coding guidelines, catch-all exception handlers should use standardized comment markers to document intent. This helps distinguish intentional catch-alls from accidental ones.

♻️ Suggested fix

-                    except Exception as e:
-                        # Catch-all for unexpected registration errors
+                    except Exception as e:  # catch-all-ok: capture unexpected registration errors without blocking other handlers
                         errors.append(

---

417-463: LGTM! Correct use of stored is_file variable.

The exception handlers correctly use the stored is_file boolean (defined at line 277) rather than calling path.is_file() which could raise a secondary exception. This pattern ensures the error context is populated safely.

However, I note the catch-all handler (lines 452-463) should also have the exception comment marker per coding guidelines.

♻️ Add exception marker

-            except Exception as e:
-                # Unexpected errors during path processing
-                # NOTE: Use stored is_file boolean, NOT path.is_file() call
-                # which could raise OSError while already handling an exception
+            except Exception as e:  # catch-all-ok: unexpected path processing errors; ensures graceful degradation
+                # NOTE: Use stored is_file boolean, NOT path.is_file() call
+                # which could raise OSError while already handling an exception
                 errors.append(

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between 2b22971 and f61a8fd.

📒 Files selected for processing (2)

• src/omnibase_infra/runtime/contract_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Never use Any type - use object for generic payloads in function parameters and return types
Use X | None (PEP 604) instead of Optional[X] for nullable types
All services must use ModelONEXContainer for dependency injection via __init__(self, container: ModelONEXContainer)
Use @allow_any decorator with documented reason as exemption mechanism for Any type violations
Use JsonType from omnibase_core.types as the canonical type alias for JSON-compatible values
Use ModelEventEnvelope[object] for generic dispatcher interfaces and object for generic payloads
Infrastructure error handling must use OnexError base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, InfraUnavailableError for transport failures with proper ModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated with uuid4() if missing, and included in all error contexts
External service adapters must implement MixinAsyncCircuitBreaker with appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never use isinstance checks

Files:

• src/omnibase_infra/runtime/contract_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

🧠 Learnings (8)

📓 Common learnings

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

📚 Learning: 2026-01-11T18:12:47.296Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T18:12:47.296Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers to document intent

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/*.py : Infrastructure error handling must use `OnexError` base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2025-11-28T18:58:53.781Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Do not use bare `except:` without re-raise

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2025-11-28T18:58:53.781Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use `ModelOnexError` instead of standard Python exceptions for error handling

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

🧬 Code graph analysis (1)

src/omnibase_infra/runtime/contract_handler_discovery.py (8)

src/omnibase_infra/errors/error_infra.py (2)

• InfraConnectionError (232-339)
• ProtocolConfigurationError (154-189)

src/omnibase_infra/models/runtime/model_discovery_error.py (1)

• ModelDiscoveryError (23-78)

src/omnibase_infra/models/runtime/model_discovery_result.py (2)

• ModelDiscoveryResult (26-159)
• has_errors (86-98)

src/omnibase_infra/models/runtime/model_discovery_warning.py (1)

• ModelDiscoveryWarning (23-71)

src/omnibase_infra/runtime/registry/registry_protocol_binding.py (2)

• RegistryError (82-123)
• ProtocolBindingRegistry (131-439)

tests/unit/runtime/contract_handler_discovery/conftest.py (2)

• handler_registry (96-102)
• plugin_loader (106-112)

src/omnibase_infra/runtime/protocol_handler_plugin_loader.py (1)

• ProtocolHandlerPluginLoader (80-322)

tests/helpers/replay_utils.py (1)

• append (364-385)

🔇 Additional comments (14)

tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py (7)

1-25: LGTM! Well-structured test file with clear organization.

The imports are appropriate, and the test file follows good practices with clear class organization by feature area (protocol compliance, basic functionality, error handling, etc.). The test module docstring provides helpful context about what's being tested.

---

27-43: LGTM! Protocol compliance tests are appropriate.

Using isinstance check against the protocol in tests is acceptable for verifying protocol compliance. The production code follows duck typing principles while these tests validate the implementation correctly satisfies the ProtocolHandlerDiscovery interface.

---

46-103: LGTM! Comprehensive basic functionality tests.

The tests cover the essential scenarios: single file discovery, directory scanning with multiple contracts, and empty directory handling. The assertions appropriately verify both the ModelDiscoveryResult state and the actual registry registrations.

---

106-175: LGTM! Well-documented error handling tests.

The docstring on lines 133-138 clearly explains the intentional design decision where HandlerPluginLoader filters out failed contracts during loading. The log verification (lines 159-172) ensures observability requirements are met. This is a good example of documenting non-obvious behavior in tests.

---

207-228: LGTM!

The loose assertions (>= 1) appropriately decouple the test from specific fixture implementation details while still verifying the core behavior of processing both files and directories.

---

231-267: LGTM!

Good coverage of ModelDiscoveryResult properties including has_errors, has_warnings, and discovered_at timestamp verification.

---

309-373: LGTM! Excellent observability tests.

The tests comprehensively cover the last_discovery_result caching behavior including initial state (None), caching after discovery, updates on subsequent discoveries, and querying for observability purposes. The use of is for object identity checks (lines 330, 345, 351-352) is appropriate for verifying caching behavior.

src/omnibase_infra/runtime/contract_handler_discovery.py (7)

1-85: LGTM! Excellent module documentation.

The module docstring is comprehensive with clear sections covering purpose, thread safety, error handling strategy, usage examples, and cross-references. The imports are well-organized with TYPE_CHECKING guard for type-only imports.

---

125-140: LGTM!

The constructor follows the dependency injection pattern correctly. The absence of explicit protocol validation aligns with the coding guideline to use duck typing via protocols.

---

142-166: LGTM!

Clean implementation of the observability property with clear documentation.

---

270-323: LGTM! Robust path type detection.

The nested try-except for path.is_dir() and path.is_file() (lines 275-288) correctly handles OSError from filesystem access issues. The edge case handling for paths that exist but are neither file nor directory (lines 314-322) is a good defensive measure.

---

465-493: LGTM! Good logging practices.

The conditional log level (WARNING for errors, INFO otherwise) and structured logging with correlation ID in the extra dict follow observability best practices.

---

495-561: LGTM! Clean import helper implementation.

The method properly validates the class path format, uses rsplit correctly to separate module from class name, and verifies the imported object is actually a class. The isinstance(handler_class, type) check on line 545 is appropriate here since it's validating the nature of the object, not protocol compliance.

---

564-566: LGTM!

Appropriate minimal export surface.

[claude]

Code Review: ContractHandlerDiscovery Implementation [OMN-1133]

Summary

This PR implements contract-based handler discovery that auto-registers handlers from YAML contracts during runtime startup. The implementation is high quality with excellent error handling, comprehensive documentation, and strong test coverage. Recommend approval with minor suggestions.

---

✅ Strengths

1. Excellent Error Handling & Graceful Degradation

• Individual handler failures don't crash discovery (contract_handler_discovery.py:399-416)
• Per-path error isolation with try-catch blocks (contract_handler_discovery.py:464-481)
• Structured error tracking via ModelDiscoveryError with error codes
• Discovery errors are logged but don't block startup (enables partial success)

Aligns with ONEX resilience patterns - individual component failures don't cascade

2. Strong Type Safety & Protocol Compliance

• Zero Any types - uses object for generic payloads
• Proper Pydantic models (ModelDiscoveryResult, ModelDiscoveryError, ModelDiscoveryWarning)
• Runtime-checkable protocol (ProtocolHandlerDiscovery)
• Custom __bool__ with documented non-standard behavior (model_discovery_result.py:115-159)

Follows ONEX typing conventions - strong types throughout

3. Container-Based Dependency Injection

• RuntimeHostProcess accepts contract_paths parameter
• Lazy creation of ContractHandlerDiscovery (runtime_host_process.py:343)
• Fallback to wire_default_handlers() when no paths provided
• Registry resolved from container or singleton

Matches ONEX DI pattern from CLAUDE.md

4. Comprehensive Test Coverage

• 15 unit tests for ContractHandlerDiscovery
• 14 integration tests for RuntimeHostProcess discovery
• Protocol compliance, error handling, correlation IDs, mixed paths, lifecycle
• Excellent test organization with fixtures (conftest.py)

5. Documentation Excellence

• Detailed docstrings with examples, thread safety notes, performance considerations
• Protocol interface fully documented with usage patterns
• Custom __bool__ behavior clearly explained with warnings
• Validation exemptions properly documented (validation_exemptions.yaml:1185-1208)

---

🔍 Code Quality Observations

Protocol Interface Design (protocol_handler_discovery.py)

Good:

• Clear separation of protocol definition from implementation
• Runtime-checkable for isinstance checks
• Comprehensive docstring with examples

Suggestion: Protocol docstring mentions fields like discovered_count, registered_count, failed_count (protocol_handler_discovery.py:144-146) but ModelDiscoveryResult uses handlers_discovered and handlers_registered (model_discovery_result.py:62-70). Consider updating protocol docstring for consistency:

# Protocol docstring should reference actual field names:
- handlers_discovered: Number of handlers found
- handlers_registered: Number successfully registered  
- errors: List of error details (not failed_count)

Discovery Implementation (contract_handler_discovery.py)

Good:

• Clear separation of concerns (path validation, handler loading, registration)
• Correlation ID auto-generation (contract_handler_discovery.py:250)
• Structured logging with context (contract_handler_discovery.py:257-264)
• Variable pre-initialization to prevent NameError (contract_handler_discovery.py:280-281)

Minor Concern: Lines 429-462 extract error codes from exception context:

if hasattr(e, "model") and hasattr(e.model, "context"):
    context_dict = e.model.context
    if isinstance(context_dict, dict):
        error_code = context_dict.get("loader_error", error_code)

This assumes specific exception structure. If ProtocolConfigurationError or InfraConnectionError change their context structure, this could silently fail. Consider:

1. Document the expected exception structure in error_handling_patterns.md
2. Add a helper method to standardize error code extraction

RuntimeHostProcess Integration (runtime_host_process.py)

Good:

• Clean parameter addition (contract_paths: list[str] | None)
• Path conversion to Path objects (runtime_host_process.py:338-340)
• Lazy discovery service creation
• Clear fallback logic to wire_default_handlers()

Question: The contract_paths parameter accepts strings but converts to Path internally. Consider accepting list[str | Path] | None for flexibility, or document why strings are preferred.

Custom __bool__ Implementation

Good:

• Clearly documented with prominent warnings
• Aligns with ONEX custom __bool__ pattern from CLAUDE.md
• Enables idiomatic if result: checks

Note: This is non-standard Pydantic behavior. Ensure all users are aware:

• if result: checks for errors (custom behavior)
• if result is not None: checks for existence (standard behavior)

---

🔐 Security Considerations

Validation Exemptions

The PR adds appropriate exemptions for "Handler" naming pattern (validation_exemptions.yaml:1185-1208). These are well-justified - "Handler" refers to ONEX handler contracts being discovered, not anti-pattern manager classes.

Import Security

_import_handler_class (contract_handler_discovery.py:513-579) uses importlib.import_module which executes module-level code. This is acceptable because:

1. HandlerPluginLoader already validated classes during loading
2. Contracts should come from trusted sources (version-controlled)
3. Python's sys.modules caching makes re-imports efficient

Recommendation: Document in handler_plugin_loader.md security section that discovery phase assumes pre-validated contracts.

---

📊 Performance Considerations

Synchronous I/O in Async Method

discover_and_register is declared async but performs synchronous I/O (path.is_dir(), path.is_file(), importlib.import_module()). The docstring acknowledges this (contract_handler_discovery.py:239-245):

"This method performs synchronous file I/O... For high-concurrency scenarios... consider wrapping with asyncio.to_thread()"

Good: Pragmatic choice for startup operations where blocking is acceptable
Future Enhancement: If used outside startup, wrap blocking operations with asyncio.to_thread()

Observability

The last_discovery_result property (contract_handler_discovery.py:142-166) enables monitoring without re-running discovery. Excellent for observability tooling.

---

🧪 Test Coverage Assessment

Unit Tests (test_contract_handler_discovery.py)

Coverage: Protocol compliance, basic discovery, error handling, correlation IDs, mixed paths

Strengths:

• Tests both single file and directory discovery
• Tests empty directories (returns empty result)
• Tests error propagation without failing discovery
• Tests correlation ID propagation

Integration Tests (test_runtime_host_handler_discovery.py)

Coverage: Contract paths usage, fallback, graceful degradation, full lifecycle

Strengths:

• Tests integration with RuntimeHostProcess.start()
• Tests fallback to wire_default_handlers()
• Tests idempotent start/stop
• Tests restart behavior

Missing Tests? (Optional Enhancements)

Consider adding tests for:

1. Large-scale discovery - 100+ contracts to validate performance
2. Concurrent discovery - multiple threads calling discover_and_register
3. Path traversal edge cases - symlinks, circular references
4. Ambiguous contract configuration - both handler_contract.yaml and contract.yaml in same directory (should trigger HANDLER_LOADER_040 per CLAUDE.md)

---

🎯 Architecture Alignment

✅ Follows ONEX Patterns

• Container-based DI - injects plugin_loader and handler_registry
• Protocol resolution - ProtocolHandlerDiscovery protocol
• Error handling - raises OnexError subclasses (ProtocolConfigurationError, InfraConnectionError)
• Strong typing - zero Any types
• Graceful degradation - individual failures don't block operation

✅ No Backwards Compatibility Violations

Per CLAUDE.md: "NO backwards compatibility is maintained - breaking changes are always acceptable"

This PR adds new functionality without breaking existing behavior (fallback to wire_default_handlers()). No issues.

---

📝 Minor Suggestions

1. Protocol Docstring Field Names

Update protocol_handler_discovery.py:144-146 to match actual model fields:

- discovered_count: Number of contract files found
- registered_count: Number of handlers successfully registered
- failed_count: Number of handlers that failed to load/register
+ handlers_discovered: Number of handlers found during discovery
+ handlers_registered: Number of handlers successfully registered
+ errors: List of ModelDiscoveryError objects (not failed_count)

2. Error Code Extraction Helper

Extract error code parsing into a helper method:

def _extract_error_code(exception: Exception, default: str) -> str:
    """Extract error code from exception context if available."""
    if hasattr(exception, "model") and hasattr(exception.model, "context"):
        context_dict = exception.model.context
        if isinstance(context_dict, dict):
            return context_dict.get("loader_error", default)
    return default

3. Contract Path Type Flexibility

Consider accepting list[str | Path] | None instead of just list[str] | None for contract_paths parameter, or document why strings are preferred.

4. Ambiguous Contract Test

Add a test case for the AMBIGUOUS_CONTRACT_CONFIGURATION error (HANDLER_LOADER_040) documented in CLAUDE.md:

def test_ambiguous_contract_configuration_raises_error():
    """Test that both handler_contract.yaml and contract.yaml in same dir raises error."""
    # Setup directory with both contract types
    # Should raise ProtocolConfigurationError with HANDLER_LOADER_040

---

🏁 Verdict

✅ APPROVED with minor suggestions

Rationale:

• High code quality - error handling, typing, documentation are excellent
• Strong test coverage - 29 tests covering unit and integration scenarios
• Architecture alignment - follows all ONEX patterns from CLAUDE.md
• Security conscious - validation exemptions justified, import security addressed
• Graceful degradation - individual failures don't cascade
• Observability - last_discovery_result enables monitoring

Minor suggestions are non-blocking - they improve consistency and maintainability but don't affect correctness.

---

🎉 Great Work!

This is a well-engineered addition to the ONEX runtime. The contract-driven discovery pattern eliminates manual handler wiring and aligns perfectly with the declarative node philosophy from CLAUDE.md. The error handling ensures robustness in production environments.

Recommendation: Merge after addressing the protocol docstring field names (suggestion #1) for consistency. Other suggestions are optional enhancements.

---

Related Documentation

• CLAUDE.md - Handler Plugin Loader Patterns
• docs/patterns/handler_plugin_loader.md - Plugin-based handler loading
• docs/patterns/error_handling_patterns.md - Error hierarchy
• docs/decisions/adr-custom-bool-result-models.md - Custom __bool__ pattern

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py (1)

334-354: Consider clarifying test intent for multiple discoveries.

The test name test_multiple_discoveries_accumulate_registrations and the comment "should overwrite" (line 347) describe different behaviors. The current assertion only verifies handlers are registered after two calls, but doesn't distinguish between accumulation and overwrite semantics.

If the intent is to verify idempotent overwrite behavior, consider adding an assertion like:

# Verify same handler class is registered (not accumulated as duplicate)
handler1 = handler_registry.get("handler.one")
handler2 = handler_registry.get("handler.one")  # After second discovery
assert handler1 is handler2  # Same class reference

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between f61a8fd and 9434a13.

📒 Files selected for processing (2)

• src/omnibase_infra/runtime/contract_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Never use Any type - use object for generic payloads in function parameters and return types
Use X | None (PEP 604) instead of Optional[X] for nullable types
All services must use ModelONEXContainer for dependency injection via __init__(self, container: ModelONEXContainer)
Use @allow_any decorator with documented reason as exemption mechanism for Any type violations
Use JsonType from omnibase_core.types as the canonical type alias for JSON-compatible values
Use ModelEventEnvelope[object] for generic dispatcher interfaces and object for generic payloads
Infrastructure error handling must use OnexError base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, InfraUnavailableError for transport failures with proper ModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated with uuid4() if missing, and included in all error contexts
External service adapters must implement MixinAsyncCircuitBreaker with appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never use isinstance checks

Files:

• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py
• src/omnibase_infra/runtime/contract_handler_discovery.py

🧠 Learnings (6)

📓 Common learnings

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Applied to files:

• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py
• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T18:12:47.296Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T18:12:47.296Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.296Z
Learning: Applies to **/*.py : Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers to document intent

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T17:31:33.533Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.533Z
Learning: Applies to **/*.py : Infrastructure error handling must use `OnexError` base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2025-11-28T18:58:53.781Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Do not use bare `except:` without re-raise

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

🔇 Additional comments (14)

src/omnibase_infra/runtime/contract_handler_discovery.py (7)

1-84: LGTM - Clean module structure and imports.

The module docstring is comprehensive with clear documentation of thread safety guarantees, error handling strategy, and example usage. TYPE_CHECKING is correctly used for protocol imports to avoid circular dependencies.

---

87-140: LGTM - Well-structured class with proper DI.

The class follows dependency injection pattern with protocol types for the plugin loader and registry. Type hints correctly use PEP 604 syntax (ModelDiscoveryResult | None).

---

142-166: LGTM - Observability-friendly caching.

The read-only property provides clean access to the last discovery result for monitoring and debugging without requiring re-execution of discovery.

---

399-416: Catch-all handlers correctly documented with WHY comments.

The catch-all exception handlers follow the graceful degradation pattern - individual handler failures don't crash the entire discovery. The WHY comments explain the rationale clearly. Per the learnings, you could alternatively use the standardized marker # catch-all-ok: graceful degradation but the current approach is equally informative.

---

234-245: Thread safety note is accurate for typical usage.

The docstring correctly notes this is intended for startup scenarios where blocking is acceptable. The _last_discovery_result assignment is a simple reference swap, which is effectively atomic in CPython due to the GIL. For production observability, the minor race on this cache is acceptable. The suggestion to use asyncio.to_thread() for high-concurrency scenarios is appropriate.

---

513-566: Solid import helper with proper validation.

The method correctly validates the class path format, uses standard importlib.import_module, and verifies the result is actually a class type. The isinstance check here (line 563) is appropriate as it validates a type constraint rather than resolving a protocol.

One minor note: ValueError raised at line 553 for malformed class paths would be caught by the catch-all handler (line 399) resulting in REGISTRATION_UNEXPECTED_ERROR rather than IMPORT_ERROR. This is arguably acceptable since malformed paths are configuration errors rather than import failures.

---

582-584: LGTM - Correct public API export.

tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py (7)

1-25: LGTM - Clean test module setup.

Appropriate imports for testing the discovery service including the protocol interface for compliance testing.

---

27-44: LGTM - Protocol compliance tests.

The isinstance check in line 35 is appropriate for test assertions verifying protocol compliance with runtime_checkable protocols. This differs from the coding guideline against isinstance checks, which applies to production code protocol resolution via duck typing.

---

46-104: LGTM - Comprehensive basic functionality coverage.

Good test coverage of core scenarios: single contract file, directory with multiple contracts, and empty directory. Assertions verify both the result model properties and the actual registry state.

---

125-191: Excellent documentation of partial success behavior.

The test_mixed_valid_invalid_contracts_partial_success test has a thorough docstring explaining the intentional design where the HandlerPluginLoader filters out failed contracts and logs warnings, while ContractHandlerDiscovery only sees the successful handlers. This makes the assert not result.has_errors assertion clear and justified.

The verification that error indicators appear in warning logs (lines 181-188) confirms the loader is properly reporting issues despite the result having no errors from the discovery perspective.

---

194-252: LGTM - Thorough correlation ID verification.

The tests correctly verify both auto-generation and preservation of correlation IDs. The preservation test (lines 237-248) checks multiple locations where the correlation ID might appear in logs, which is robust against different logging configurations.

---

255-277: LGTM - Mixed path type handling verified.

The test confirms both file and directory paths are processed in a single discovery call. The >= 1 assertions are appropriate here since the exact count depends on fixture contents and the primary goal is verifying path type handling.

---

357-421: LGTM - Comprehensive observability testing.

Excellent coverage of the last_discovery_result caching feature including initial state, object identity verification (cached is result), updates on subsequent discoveries, and ability to query all result properties without re-running discovery.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @src/omnibase_infra/runtime/protocol_handler_discovery.py:
- Around line 150-160: The protocol docstring for ProtocolHandlerDiscovery
currently claims that implementations will raise ProtocolConfigurationError with
codes DISCOVERY_001/002/003, but the ContractHandlerDiscovery implementation
captures errors in its result.errors instead; update the
ProtocolHandlerDiscovery docstring to accurately reflect reality by stating that
implementations MAY raise ProtocolConfigurationError with those codes but are
not required to, or remove the explicit error codes and state that
implementations should either raise those errors or report issues via the
result.errors list (mentioning ProtocolHandlerDiscovery and
ContractHandlerDiscovery by name so readers can find the relevant
implementations).

🧹 Nitpick comments (2)

src/omnibase_infra/runtime/contract_handler_discovery.py (2)

249-251: Unused warnings list is initialized but never populated.

The warnings list is initialized on line 250 but never appended to in this method. While this doesn't cause issues and may be intentional for future extension, consider either:

1. Adding a TODO comment indicating future use, or
2. Removing if warnings are not planned for this operation

---

435-442: Context loss when directory paths fail unexpectedly.

When ProtocolConfigurationError or unexpected exceptions occur for a directory path, contract_path is set to None (via path if is_file else None on lines 439 and 457). This loses the directory path context in the error.

Consider preserving the path regardless of type:

♻️ Suggested improvement

                 errors.append(
                     ModelDiscoveryError(
                         error_code=error_code,
                         message=str(e),
-                        contract_path=path if is_file else None,
+                        contract_path=path,  # Preserve path for context
                         details={"exception_type": type(e).__name__},
                     )
                 )

Apply similar change to lines 453-460.

Also applies to: 453-460

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between 9434a13 and 5dd6328.

📒 Files selected for processing (4)

• src/omnibase_infra/runtime/contract_handler_discovery.py
• src/omnibase_infra/runtime/protocol_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/conftest.py
• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/unit/runtime/contract_handler_discovery/test_contract_handler_discovery.py

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Never use Any type - use object for generic payloads in function parameters and return types
Use X | None (PEP 604) instead of Optional[X] for nullable types
All services must use ModelONEXContainer for dependency injection via __init__(self, container: ModelONEXContainer)
Use @allow_any decorator with documented reason as exemption mechanism for Any type violations
Use JsonType from omnibase_core.types as the canonical type alias for JSON-compatible values
Use ModelEventEnvelope[object] for generic dispatcher interfaces and object for generic payloads
Infrastructure error handling must use OnexError base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, InfraUnavailableError for transport failures with proper ModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated with uuid4() if missing, and included in all error contexts
External service adapters must implement MixinAsyncCircuitBreaker with appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never use isinstance checks

Files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py
• src/omnibase_infra/runtime/contract_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/conftest.py

**/protocol_*.py

📄 CodeRabbit inference engine (CLAUDE.md)

File naming convention: Protocols must use protocol_<name>.py or protocols.py with class name Protocol<Name>

Files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

🧠 Learnings (18)

📓 Common learnings

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND

📚 Learning: 2026-01-06T17:57:00.689Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.689Z
Learning: Applies to src/omnibase_spi/protocols/**/*.py : Every protocol must inherit from `typing.Protocol` and have the `runtime_checkable` decorator

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2026-01-06T17:57:00.689Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.689Z
Learning: Applies to src/omnibase_spi/protocols/handlers/**/*.py : Protocol naming convention: Handler protocols must follow `Protocol{Type}Handler` pattern

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:22:32.195Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol from typing module for all interface definitions; never use ABC (Abstract Base Classes) for service interfaces

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol for tool interfaces and plugin APIs based on method shape (structural typing), not Pydantic models with inheritance

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T16:33:32.747Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Use Protocol for interface definitions when implementations may live outside core codebase; use Pydantic models only for base classes with shared logic

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T16:33:32.747Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/*.py : Import protocols from `omnibase.protocol.protocol_*` paths

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/*.py : Import protocols from `omnibase.protocol.protocol_<name>` module paths

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2026-01-06T17:57:00.689Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T17:57:00.689Z
Learning: Applies to src/omnibase_spi/protocols/contracts/**/*.py : Protocol naming convention: Compiler protocols must follow `Protocol{Type}ContractCompiler` pattern

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:24:41.687Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Avoid using Any, dict, or primitive types in protocol signatures; use the strongest typing possible with Pydantic models

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2025-11-24T17:22:32.195Z

Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/protocols/protocol_*.py : All Protocol definitions must use model-only signatures: methods accept only validated Pydantic models, never dict, primitives, or argument models

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py

📚 Learning: 2026-01-11T17:31:33.550Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/handler_contract.yaml : Handlers must be discovered and loaded dynamically via YAML contracts using plugin pattern, not hardcoded registries

Applied to files:

• src/omnibase_infra/runtime/protocol_handler_discovery.py
• src/omnibase_infra/runtime/contract_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/conftest.py

📚 Learning: 2026-01-11T18:12:47.310Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py
• tests/unit/runtime/contract_handler_discovery/conftest.py

📚 Learning: 2026-01-11T17:31:33.550Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/handler_contract.yaml : Handler contract files must declare handler routing with `routing_strategy`, event models, handler classes, and handler modules

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T18:12:47.310Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standardized exception comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using catch-all or broad exception handlers to document intent

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2026-01-11T17:31:33.550Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T17:31:33.550Z
Learning: Applies to **/*.py : Infrastructure error handling must use `OnexError` base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2025-11-28T18:58:53.781Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Do not use bare `except:` without re-raise

Applied to files:

• src/omnibase_infra/runtime/contract_handler_discovery.py

📚 Learning: 2025-11-24T16:33:51.604Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/**/conftest.py : Test fixtures must be defined in `conftest.py` and should provide reusable sample data, UUIDs, semantic versions, and model data

Applied to files:

• tests/unit/runtime/contract_handler_discovery/conftest.py

🧬 Code graph analysis (3)

src/omnibase_infra/runtime/protocol_handler_discovery.py (2)

src/omnibase_infra/models/runtime/model_discovery_result.py (1)

• ModelDiscoveryResult (26-159)

src/omnibase_infra/runtime/contract_handler_discovery.py (1)

• discover_and_register (165-490)

src/omnibase_infra/runtime/contract_handler_discovery.py (5)

src/omnibase_infra/errors/error_infra.py (1)

• ProtocolConfigurationError (154-189)

src/omnibase_infra/models/runtime/model_discovery_error.py (1)

• ModelDiscoveryError (23-78)

src/omnibase_infra/models/runtime/model_discovery_result.py (2)

• ModelDiscoveryResult (26-159)
• has_errors (86-98)

src/omnibase_infra/models/runtime/model_discovery_warning.py (1)

• ModelDiscoveryWarning (23-71)

src/omnibase_infra/runtime/registry/registry_protocol_binding.py (2)

• RegistryError (82-123)
• ProtocolBindingRegistry (131-439)

tests/unit/runtime/contract_handler_discovery/conftest.py (3)

src/omnibase_infra/runtime/contract_handler_discovery.py (1)

• ContractHandlerDiscovery (84-558)

src/omnibase_infra/runtime/handler_plugin_loader.py (1)

• HandlerPluginLoader (237-1984)

src/omnibase_infra/runtime/registry/registry_protocol_binding.py (1)

• ProtocolBindingRegistry (131-439)

🔇 Additional comments (15)

src/omnibase_infra/runtime/protocol_handler_discovery.py (2)

1-61: LGTM! Well-structured protocol definition.

The protocol follows all required conventions:

• Uses @runtime_checkable decorator as required
• Follows Protocol<Name> naming convention
• Uses TYPE_CHECKING to avoid circular imports
• Uses X | None pattern (PEP 604) for nullable types
• Uses ... (Ellipsis) for method body per PEP 544

---

219-221: LGTM!

The __all__ export correctly exposes only the protocol class.

src/omnibase_infra/runtime/contract_handler_discovery.py (8)

1-82: LGTM! Clean imports and comprehensive module documentation.

The module follows all coding guidelines:

• No Any type usage
• Proper TYPE_CHECKING usage for type-only imports
• Well-documented with usage examples and thread safety notes

---

139-163: LGTM!

The read-only property correctly uses X | None pattern and provides observability access to the last discovery result.

---

246-262: LGTM! Correlation ID handling follows ONEX guidelines.

Auto-generating correlation_id when not provided ensures all operations are traceable. The structured logging with extra dict includes all relevant context.

---

267-302: Good defensive initialization of path type flags.

Initializing is_directory and is_file to False before the try block (lines 277-278) correctly prevents NameError in the outer exception handlers. The OSError handling for filesystem access issues is appropriate.

---

378-413: Exception grouping for import errors is well-structured.

The reclassification of AttributeError, TypeError, and ValueError alongside ImportError under the unified IMPORT_ERROR code (per PR objectives) provides clearer error categorization. The catch-all handler with the explanatory comment follows the established pattern for graceful degradation.

---

492-558: LGTM! Clean import helper with proper validation.

The method correctly:

• Validates fully-qualified path format
• Uses standard importlib.import_module for dynamic import
• Verifies the result is a class (not protocol resolution, so isinstance(..., type) is appropriate)
• Provides detailed logging for debugging

---

561-563: LGTM!

The __all__ correctly exports only the public class.

---

122-138: Use ModelONEXContainer for dependency injection.

This class violates the coding guidelines which require all services to use ModelONEXContainer for dependency injection. Update the constructor to accept container: ModelONEXContainer and resolve ProtocolHandlerPluginLoader and ProtocolBindingRegistry from the container instead of using direct injection.

tests/unit/runtime/contract_handler_discovery/conftest.py (5)

20-43: LGTM! Well-defined test contract templates.

The templates provide good coverage:

• Valid contract with placeholders for flexibility
• Invalid YAML syntax for parser error testing
• Missing required field for validation error testing

---

50-88: LGTM! Mock handler correctly implements protocol interface.

The MockValidHandler:

• Implements all 5 required protocol methods
• Uses object for generic parameters (per coding guidelines, avoiding Any)
• Uses dict[str, object] for typed dictionaries
• Properly documents the protocol contract in the docstring

---

95-143: LGTM! Excellent fixture design with proper isolation.

The fixtures demonstrate good practices:

• Function scope (default) ensures fresh instances per test
• Comprehensive docstrings explain isolation guarantees
• discovery_service correctly composes other fixtures for full isolation
• No shared mutable state between tests

---

145-201: LGTM! Path fixtures provide comprehensive test scenarios.

Good use of:

• tmp_path fixture for automatic cleanup and isolation
• {__name__}.MockValidHandler for correct module path reference
• Realistic directory structures matching expected contract layouts

---

203-247: LGTM! Edge case fixtures are well-structured.

The mixed_valid_invalid_directory and empty_directory fixtures provide essential test scenarios for:

• Graceful degradation with partial failures
• Handling of empty/no-contracts directories

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Docstring describes exceptions that the implementation doesn't raise.

The protocol docstring specifies that ProtocolConfigurationError should be raised with error codes DISCOVERY_001, DISCOVERY_002, DISCOVERY_003 for empty paths, all invalid paths, and configuration issues. However, the ContractHandlerDiscovery implementation in contract_handler_discovery.py does not raise these exceptions—instead, it captures all errors in the result's errors list for graceful degradation.

Consider either:

1. Updating the protocol docstring to reflect that implementations MAY raise these exceptions but are not required to, or
2. Removing the specific error codes if the intent is for implementations to always use graceful degradation

🤖 Prompt for AI Agents

In @src/omnibase_infra/runtime/protocol_handler_discovery.py around lines 150 -
160, The protocol docstring for ProtocolHandlerDiscovery currently claims that
implementations will raise ProtocolConfigurationError with codes
DISCOVERY_001/002/003, but the ContractHandlerDiscovery implementation captures
errors in its result.errors instead; update the ProtocolHandlerDiscovery
docstring to accurately reflect reality by stating that implementations MAY
raise ProtocolConfigurationError with those codes but are not required to, or
remove the explicit error codes and state that implementations should either
raise those errors or report issues via the result.errors list (mentioning
ProtocolHandlerDiscovery and ContractHandlerDiscovery by name so readers can
find the relevant implementations).

[claude]

Code Review: ContractHandlerDiscovery Implementation (OMN-1133)

Summary

This PR implements contract-based handler discovery for the ONEX runtime. The implementation is well-architected, thoroughly tested, and production-ready.

---

✅ Strengths

1. Excellent Architecture

• Clean separation of concerns: ContractHandlerDiscovery bridges HandlerPluginLoader and ProtocolBindingRegistry
• Protocol-based design for dependency injection and testability
• Graceful degradation: Individual handler failures don't crash entire discovery
• Proper error containment with structured error models
• Container-based DI per ONEX conventions

2. Strong Type Safety

• Zero Any types - full ONEX compliance
• Pydantic strict validation on all models
• Proper X | None (PEP 604) usage throughout
• Custom bool properly documented

3. Comprehensive Test Coverage

• 29 total tests (15 unit + 14 integration)
• Coverage of directories, files, mixed paths, error scenarios
• Lifecycle testing, observability, graceful degradation

4. Excellent Documentation

• Detailed docstrings with examples
• Complex logic well-explained with inline comments
• Error code mapping to HandlerPluginLoader

5. Production-Ready Error Handling

• Proper error chaining with raise...from e
• Correlation ID propagation throughout
• Structured logging with comprehensive extra fields

---

🔍 Code Quality Observations

1. Async Method with Sync I/O (contract_handler_discovery.py:237)

• discover_and_register() is async but performs sync file I/O
• Recommendation: Accept as-is for MVP - docstring documents this limitation
• Startup context makes blocking acceptable

2. Path Type Checking Logic (contract_handler_discovery.py:268-327)

• Defensive pattern: pre-initializes is_directory/is_file before try block
• Recommendation: Keep current implementation - prevents subtle NameError bugs

3. Container Resolution Caching (runtime_host_process.py:1112-1172)

• _get_handler_registry() caches resolved registry
• Recommendation: Accept - appropriate for startup initialization

---

🔒 Security Considerations

Well-Handled:

• YAML safe_load(), 10MB file size limits, protocol validation
• Path traversal protection, optional namespace allowlisting

Deployment Checklist:

• Contract directories readable only by runtime user
• Mount contract directories as read-only
• Enable allowed_namespaces in production
• Enable INFO-level logging for audit trail

---

📊 Performance

Good patterns: Fail-fast validation, path caching, registry caching

Potential bottlenecks: Synchronous file I/O, sequential path processing

• Recommendation: Monitor discovered_at timestamps in production
• Accept as-is for now (startup performance acceptable)

---

🎯 CLAUDE.md Compliance

Fully Compliant:

• Zero Any types, proper X | None usage
• File naming follows model_*.py pattern
• Uses ModelInfraErrorContext for errors
• Container DI with ModelONEXContainer
• Comprehensive documentation

---

🎉 Final Verdict

APPROVE - Ready to Merge

This PR demonstrates excellent engineering practices:

• Well-designed architecture
• 29 comprehensive tests
• Production-ready error handling
• Full ONEX compliance
• Graceful degradation

Minor follow-ups (non-blocking):

1. Monitor discovery performance in production
2. Review validation exemptions in 1-2 sprints
3. Consider parallelizing path processing if needed
4. Ensure deployment follows security checklist

Great work! The code quality and test coverage are exemplary. 🎉