Repository navigation
feat(OMN-1095): Add handler source mode feature flag (bootstrap | contract | hybrid) - #187
Conversation
…(bootstrap | contract | hybrid)"" This reverts commit a441b7c.
📝 WalkthroughWalkthroughAdds handler-source selection primitives and configuration, a HandlerSourceResolver for BOOTSTRAP/CONTRACT/HYBRID merging (with per-handler override rules), runtime wiring including a PluginLoaderContractSource and proto.* identity normalization, handler_version propagation, validation exemptions, many tests (new/updated/removed), and related model/type updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as HandlerSourceResolver
participant Bootstrap as BootstrapSource
participant Contract as ContractSource
participant Merge as MergeLogic
Client->>Resolver: resolve_handlers()
alt mode == BOOTSTRAP
Resolver->>Bootstrap: discover_handlers()
Bootstrap-->>Resolver: bootstrap_descriptors + errors
Resolver-->>Client: descriptors + validation_errors
else mode == CONTRACT
Resolver->>Contract: discover_handlers()
Contract-->>Resolver: contract_descriptors + errors
Resolver-->>Client: descriptors + validation_errors
else mode == HYBRID
Resolver->>Bootstrap: discover_handlers()
Bootstrap-->>Resolver: bootstrap_descriptors + errors
Resolver->>Contract: discover_handlers()
Contract-->>Resolver: contract_descriptors + errors
Resolver->>Merge: merge(contract_first, allow_bootstrap_override)
Merge-->>Resolver: merged_descriptors + stats
Resolver->>Resolver: aggregate_validation_errors()
Resolver->>Resolver: log_resolution_stats()
Resolver-->>Client: merged_descriptors + validation_errors
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/models/handlers/model_handler_source_config.py`:
- Around line 121-135: The is_bootstrap_expired property currently uses naive
datetime.now() which will raise TypeError when self.bootstrap_expires_at is
timezone-aware; update is_bootstrap_expired to obtain "now" that matches
bootstrap_expires_at.tzinfo: if self.bootstrap_expires_at is None return False,
otherwise if self.bootstrap_expires_at.tzinfo is not None get now via
datetime.now(tz=self.bootstrap_expires_at.tzinfo) (or use timezone.utc when
tzinfo indicates UTC), else use naive datetime.now(), then compare now >
self.bootstrap_expires_at. Ensure to reference the is_bootstrap_expired property
and bootstrap_expires_at attribute when making the change.
In `@src/omnibase_infra/runtime/handler_source_resolver.py`:
- Around line 84-103: The resolver currently ignores
ModelHandlerSourceConfig.allow_bootstrap_override; update the
HandlerSourceResolver so this flag is accepted and honored: add a parameter (or
the config object) to __init__ (the constructor shown) and store it as
self._allow_bootstrap_override, then modify the handler merging logic (the
method that merges/discovers bootstrap and contract handlers around lines
171-205 — e.g., resolve_handlers/merge_handlers/_merge logic) so when
allow_bootstrap_override is False contract-discovered handlers cannot overwrite
existing bootstrap handlers (i.e., prefer bootstrap entries), and when True
allow contract handlers to override bootstrap; also ensure callers that
construct HandlerSourceResolver pass the ModelHandlerSourceConfig or the boolean
through.
In `@tests/unit/runtime/test_handler_source_mode.py`:
- Around line 3-26: Update the module docstring in
tests/unit/runtime/test_handler_source_mode.py to reflect that
HandlerSourceResolver is implemented and tests are green: remove phrases like
"does NOT exist yet" and "should FAIL initially", change TDD/RED-phase wording
to indicate these are acceptance/unit tests that verify HYBRID resolution
behavior, and keep the listed responsibilities (Resolving handlers, per-handler
identity, contract precedence, bootstrap fallback) and test categories intact;
ensure references to HandlerSourceResolver and the test categories (Hybrid Mode
Resolution, Bootstrap Fallback, Bootstrap Only Mode, Contract Only Mode,
Structured Logging) remain accurate and phrased as current/green tests rather
than future/TDD expectations.
Wire the HandlerSourceResolver into RuntimeHostProcess to enable mode-driven
handler discovery (BOOTSTRAP, CONTRACT, or HYBRID).
Changes:
- Add timezone-aware validator for bootstrap_expires_at (rejects naive datetimes)
- Add _load_handler_source_config() to parse mode from runtime config
- Add _resolve_handler_descriptors() using HandlerSourceResolver
- Replace sequential discovery with unified resolver-based flow
- Remove obsolete test_handler_discovery.py (superseded by resolver tests)
- Add 16 integration tests for runtime-level handler source mode behavior
Configuration:
handler_source:
mode: "hybrid" # bootstrap|contract|hybrid
bootstrap_expires_at: "2026-02-01T00:00:00Z" # Optional, UTC required
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py`:
- Around line 982-1042: In _load_handler_source_config, parse the
allow_bootstrap_override value from the handler_source_config dict,
validate/coerce it to a boolean (accept Python bools and common truthy string
forms), and pass it into the ModelHandlerSourceConfig constructor (in addition
to handler_source_mode and bootstrap_expires_at) so the returned config reflects
the runtime setting instead of always using the default False; update any
logging to warn on invalid values and default to False when missing or
unparsable.
♻️ Duplicate comments (3)
src/omnibase_infra/models/handlers/model_handler_source_config.py (1)
105-111:allow_bootstrap_overridefield is defined but never honored.The
allow_bootstrap_overridefield is documented as controlling handler resolution in HYBRID mode, but theHandlerSourceResolverdoes not accept or use this flag. The field currently has no effect.Either:
- Wire this flag into
HandlerSourceResolver.__init__and apply it during hybrid resolution, or- Remove the field if the behavior isn't needed yet
src/omnibase_infra/runtime/handler_source_resolver.py (1)
84-102:allow_bootstrap_overrideflag is not wired into the resolver.The
ModelHandlerSourceConfig.allow_bootstrap_overridefield exists but this resolver does not accept it as a parameter. The HYBRID resolution logic (lines 171-205) always uses contract-first precedence regardless of the config setting.This was flagged in a previous review and remains unaddressed.
src/omnibase_infra/runtime/service_runtime_host_process.py (1)
1104-1108:allow_bootstrap_overridefrom config is not passed to the resolver.Even if
source_config.allow_bootstrap_overridewere parsed, it's not passed toHandlerSourceResolver. The resolver constructor doesn't accept this parameter (as flagged separately), but this is where the wiring would need to happen.
🧹 Nitpick comments (2)
tests/integration/runtime/test_runtime_handler_source_mode.py (1)
84-91: Consider using the isolated registry fixture consistently.The
isolated_handler_registryfixture creates a freshRegistryProtocolBindinginstance, but several tests (e.g., lines 209, 267, 441, 489) useget_handler_registry()singleton instead. This could cause test pollution if tests run in parallel or in a specific order.Consider passing the isolated registry to
RuntimeHostProcessvia thehandler_registryparameter for better test isolation.src/omnibase_infra/runtime/service_runtime_host_process.py (1)
1091-1108: Contract source fallback may cause unexpected HYBRID behavior.When
contract_pathsis empty,HandlerBootstrapSource()is used as thecontract_source. In HYBRID mode, this means both sources return the same bootstrap handlers, and the resolver will see them as "contract handlers" overriding "bootstrap handlers" with the same IDs—resulting inoverride_countequal to the handler count andfallback_count = 0.This is technically correct (bootstrap handlers are still loaded), but the logging and semantics may be confusing. Consider:
- Logging a warning when using bootstrap source as contract fallback
- Or documenting this edge case behavior
- Change default handler_source_mode from CONTRACT to HYBRID for consistency - Add warning log when CONTRACT mode used without contract_paths - Add isinstance type check before handler registration - Fix double HandlerBootstrapSource instantiation by reusing instance - Create PluginLoaderContractSource adapter for backwards compatibility with simpler contract schema (handler_name, handler_class, handler_type) - Fix protocol_type extraction to only strip bootstrap. prefix - Update test mock to return valid ModelHandlerDescriptor
- Restore differentiated exception handling (ImportError/AttributeError vs generic) - Tighten HYBRID mode test assertion from >= 1 to == 2 (matches comment) - Extract PluginLoaderContractSource class to module level for testability - Replace magic string "hybrid" with EnumHandlerSourceMode.HYBRID.value - Use removeprefix() for clearer prefix stripping logic
CONTRACT mode now raises ProtocolConfigurationError when no contract_paths are provided, rather than silently falling back to bootstrap handlers. This ensures user intent is honored - CONTRACT mode means "only contract handlers" not "contract handlers if available, bootstrap otherwise". Changes: - Raise ProtocolConfigurationError in _resolve_handler_descriptors() when CONTRACT mode is configured but no contract_paths provided - Update test_contract_mode_raises_error_without_contract_paths - Update test_expired_bootstrap_forces_contract_mode (expired bootstrap forces effective_mode to CONTRACT, which now requires paths)
…solution Changes: - Implement allow_bootstrap_override flag for configurable HYBRID precedence - Add debug logging for individual handler resolution decisions - Fix hardcoded handler_kind with proper EnumHandlerTypeCategory mapping - Consolidate model_rebuild documentation with cross-references The allow_bootstrap_override parameter (default False) controls which source takes precedence in HYBRID mode conflicts. Debug logs provide visibility into override/fallback decisions for each handler.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py`:
- Around line 186-240: The PluginLoaderContractSource currently instantiates
HandlerPluginLoader() without namespace restrictions; update
PluginLoaderContractSource.__init__ to accept an optional allowed_namespaces:
Iterable[str] | None parameter, store it on the instance (e.g.,
self._allowed_namespaces), and pass it into
HandlerPluginLoader(allowed_namespaces=self._allowed_namespaces) when creating
self._plugin_loader; ensure any callers constructing PluginLoaderContractSource
provide the allowed_namespaces where needed and preserve default behavior when
None.
♻️ Duplicate comments (1)
src/omnibase_infra/runtime/service_runtime_host_process.py (1)
1159-1189:allow_bootstrap_overrideis currently ignored.
The config field isn’t parsed in_load_handler_source_config, and the resolver is instantiated without it, so the flag never affects HYBRID precedence.🛠️ Proposed fix
@@ if isinstance(handler_source_config, dict): mode_str = handler_source_config.get( "mode", EnumHandlerSourceMode.HYBRID.value ) expires_at_str = handler_source_config.get("bootstrap_expires_at") + allow_override_raw = handler_source_config.get( + "allow_bootstrap_override", False + ) @@ - return ModelHandlerSourceConfig( - handler_source_mode=mode, - bootstrap_expires_at=expires_at, - ) + if isinstance(allow_override_raw, str): + allow_override = ( + allow_override_raw.strip().lower() + in {"1", "true", "yes", "on"} + ) + else: + allow_override = bool(allow_override_raw) + + return ModelHandlerSourceConfig( + handler_source_mode=mode, + bootstrap_expires_at=expires_at, + allow_bootstrap_override=allow_override, + ) @@ resolver = HandlerSourceResolver( bootstrap_source=bootstrap_source, contract_source=contract_source, mode=source_config.effective_mode, + allow_bootstrap_override=source_config.allow_bootstrap_override, )Also applies to: 1262-1267
| def __init__(self, contract_paths: list[Path]) -> None: | ||
| """Initialize the contract source with paths to scan. | ||
|
|
||
| Args: | ||
| contract_paths: List of filesystem paths containing handler contracts. | ||
| """ | ||
| self._contract_paths = contract_paths | ||
| self._plugin_loader = HandlerPluginLoader() | ||
|
|
||
| @property | ||
| def source_type(self) -> str: | ||
| """Return the source type identifier. | ||
|
|
||
| Returns: | ||
| str: Always "CONTRACT" for this filesystem-based source. | ||
| """ | ||
| return "CONTRACT" | ||
|
|
||
| async def discover_handlers(self) -> ModelContractDiscoveryResult: | ||
| """Discover handlers using HandlerPluginLoader. | ||
|
|
||
| Scans all configured contract paths and loads handler contracts using | ||
| the HandlerPluginLoader. Each discovered handler is converted to a | ||
| ModelHandlerDescriptor for use by the handler resolution framework. | ||
|
|
||
| Returns: | ||
| ModelContractDiscoveryResult: Container with discovered descriptors | ||
| and any validation errors encountered during discovery. | ||
|
|
||
| Note: | ||
| This method uses graceful degradation - if a single contract path | ||
| fails to load, discovery continues with remaining paths and the | ||
| error is logged but not raised. | ||
| """ | ||
| # NOTE: ModelContractDiscoveryResult.model_rebuild() is called at module-level | ||
| # in handler_source_resolver.py and handler_contract_source.py to resolve | ||
| # forward references. No need to call it here - see those modules for rationale. | ||
|
|
||
| descriptors: list[ModelHandlerDescriptor] = [] | ||
| validation_errors: list[ModelHandlerValidationError] = [] | ||
|
|
||
| for path in self._contract_paths: | ||
| path_obj = Path(path) if isinstance(path, str) else path | ||
| if not path_obj.exists(): | ||
| logger.warning( | ||
| "Contract path does not exist, skipping: %s", | ||
| path_obj, | ||
| ) | ||
| continue | ||
|
|
||
| try: | ||
| # Use plugin loader to discover handlers with simpler schema | ||
| loaded_handlers = self._plugin_loader.load_from_directory( | ||
| directory=path_obj, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "handler_plugin_loader.py" -o "handler*plugin*loader*.py"Repository: OmniNode-ai/omnibase_infra
Length of output: 239
🏁 Script executed:
fd -t f "handler_plugin_loader"Repository: OmniNode-ai/omnibase_infra
Length of output: 365
🏁 Script executed:
wc -l src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 126
🏁 Script executed:
rg -n "def load_from_directory|allowed_namespaces|permission|stat|chmod|owner" src/omnibase_infra/runtime/handler_plugin_loader.py | head -50Repository: OmniNode-ai/omnibase_infra
Length of output: 3599
🏁 Script executed:
sed -n '672,750p' src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 3733
🏁 Script executed:
sed -n '740,850p' src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 4473
🏁 Script executed:
rg -n "def load_from_contract" src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 101
🏁 Script executed:
sed -n '374,550p' src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 7944
🏁 Script executed:
sed -n '550,750p' src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 9132
🏁 Script executed:
rg -n "_validate_namespace\|_import_handler_class" src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
rg -n "namespace" src/omnibase_infra/runtime/handler_plugin_loader.py | head -30Repository: OmniNode-ai/omnibase_infra
Length of output: 2455
🏁 Script executed:
sed -n '1533,1620p' src/omnibase_infra/runtime/handler_plugin_loader.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 3833
🏁 Script executed:
rg -n "class PluginLoaderContractSource" src/omnibase_infra/runtime/service_runtime_host_process.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 131
🏁 Script executed:
sed -n '155,200p' src/omnibase_infra/runtime/service_runtime_host_process.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 1835
🏁 Script executed:
rg -n "allowed_namespaces\|HandlerPluginLoader" src/omnibase_infra/runtime/service_runtime_host_process.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
sed -n '155,240p' src/omnibase_infra/runtime/service_runtime_host_process.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 3555
HandlerPluginLoader enforces file permissions, but namespace allowlisting is not configured.
The loader validates contract file access via stat operations and permission checks, which is correct. However, PluginLoaderContractSource instantiates HandlerPluginLoader() without the optional allowed_namespaces parameter, leaving namespace allowlisting disabled. If namespace-based import restrictions are required, plumb a allowed_namespaces parameter through the adapter's __init__ and pass it to the loader.
🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py` around lines 186
- 240, The PluginLoaderContractSource currently instantiates
HandlerPluginLoader() without namespace restrictions; update
PluginLoaderContractSource.__init__ to accept an optional allowed_namespaces:
Iterable[str] | None parameter, store it on the instance (e.g.,
self._allowed_namespaces), and pass it into
HandlerPluginLoader(allowed_namespaces=self._allowed_namespaces) when creating
self._plugin_loader; ensure any callers constructing PluginLoaderContractSource
provide the allowed_namespaces where needed and preserve default behavior when
None.
…solution - Add clear error handling for naive datetime in _load_handler_source_config() with warning about UTC requirement and example ISO formats - Add _DEFAULT_HANDLER_KIND constant to replace hardcoded "effect" fallback - Document HYBRID mode behavior when no contract_paths provided (intentional double-discovery for consistent behavior) - Add TestHybridModeBootstrapOverride test class with 5 tests covering allow_bootstrap_override=True scenario
…r-source-mode-feature-flag-bootstrap-contract-hybrid
Address CodeRabbit PR review feedback: - Parse allow_bootstrap_override from handler_source config dict - Add boolean coercion for truthy string values (true/yes/1/on) - Pass allow_bootstrap_override to ModelHandlerSourceConfig constructors - Pass allow_bootstrap_override to HandlerSourceResolver - Update test docstring to reflect GREEN phase (implementation exists)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py`:
- Around line 257-266: The handler_id format mismatch causes duplicate entries
because bootstrap handlers use "bootstrap.{type}" while contract descriptors use
loaded.protocol_type; fix this by normalizing contract-created
ModelHandlerDescriptor.handler_id to include the bootstrap prefix (e.g., set
handler_id=f"bootstrap.{loaded.protocol_type}" where the descriptor is
constructed), so HandlerSourceResolver and handlers_by_id key lookups will
correctly detect and override bootstrap handlers.
♻️ Duplicate comments (2)
tests/unit/runtime/test_handler_source_mode.py (1)
257-258: Remove stale RED-phase note.The resolver exists and the tests are green, so this comment is now misleading.
🧹 Suggested update
- # Import will fail until implementation exists (TDD RED phase) from omnibase_infra.runtime.handler_source_resolver import HandlerSourceResolversrc/omnibase_infra/runtime/service_runtime_host_process.py (1)
190-198: Plumb namespace allowlisting intoHandlerPluginLoader.Contracts are effectively executable. Without an allowlist, any module can be imported. Thread an optional
allowed_namespacesthrough this adapter (and its callers) and pass it toHandlerPluginLoaderso deployments can lock this down.Based on learnings, enforce namespace allowlisting for handler contracts.🔐 Suggested wiring
-from collections.abc import Awaitable, Callable +from collections.abc import Awaitable, Callable, Iterable @@ - def __init__(self, contract_paths: list[Path]) -> None: + def __init__( + self, + contract_paths: list[Path], + allowed_namespaces: Iterable[str] | None = None, + ) -> None: @@ - self._plugin_loader = HandlerPluginLoader() + self._allowed_namespaces = allowed_namespaces + self._plugin_loader = HandlerPluginLoader( + allowed_namespaces=self._allowed_namespaces + )
- Add handler_class to exception log extras for better debugging - Document accepted truthy string values for allow_bootstrap_override - Add debug log for HYBRID mode bootstrap source fallback behavior - Add comment explaining deferred imports in _load_handler_source_config()
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_infra/runtime/service_runtime_host_process.py (1)
1409-1461: Verify HandlerSourceResolver identity matching does not normalize bootstrap handler_id format.The
removeprefix("bootstrap.")normalization at line 1415 correctly handles registration, but HandlerSourceResolver's conflict detection in HYBRID mode operates on rawhandler_idvalues:
- Bootstrap handlers have
handler_id="bootstrap.{type}"(e.g.,"bootstrap.consul")- Contract handlers have
handler_id=loaded.protocol_typefrom YAML (e.g.,"effect.mcp.handler")At lines 230-235 in
handler_source_resolver.py, the resolver builds lookup maps keyed by these rawhandler_idstrings. When it attempts conflict detection at line 277 (if descriptor.handler_id in handlers_by_id), bootstrap and contract handlers won't match due to different identifier formats. This results in both handlers being included instead of contract overriding bootstrap during per-identity resolution, defeating the HYBRID mode's precedence logic.
♻️ Duplicate comments (1)
src/omnibase_infra/runtime/service_runtime_host_process.py (1)
190-198: HandlerPluginLoader instantiated without namespace allowlisting.Per the retrieved learnings, handler contracts are treated as executable code and should optionally use
allowed_namespacesparameter for namespace allowlisting. ThePluginLoaderContractSourcecurrently createsHandlerPluginLoader()without this restriction.If namespace-based import restrictions are required for security, consider adding an
allowed_namespacesparameter toPluginLoaderContractSource.__init__and passing it through to the loader. Based on learnings.♻️ Suggested enhancement
- def __init__(self, contract_paths: list[Path]) -> None: + def __init__( + self, + contract_paths: list[Path], + allowed_namespaces: tuple[str, ...] | None = None, + ) -> None: """Initialize the contract source with paths to scan. Args: contract_paths: List of filesystem paths containing handler contracts. + allowed_namespaces: Optional tuple of allowed module namespaces for + handler class imports. If None, all namespaces are allowed. """ self._contract_paths = contract_paths - self._plugin_loader = HandlerPluginLoader() + self._plugin_loader = HandlerPluginLoader( + allowed_namespaces=allowed_namespaces, + )
🧹 Nitpick comments (2)
src/omnibase_infra/runtime/service_runtime_host_process.py (2)
257-269: Hardcoded version and model paths may not reflect actual handler metadata.The descriptor is created with:
version=ModelSemVer(major=1, minor=0, patch=0)- hardcoded regardless of actual handler versioninput_modelandoutput_model- hardcoded string pathsIf
ModelLoadedHandlercarries version or model information from the contract, consider using those values instead. Otherwise, this is acceptable for MVP but may need refinement when handlers declare their own versions.
1293-1295: Minor: Redundant Path conversion.
self._contract_pathsis alreadylist[Path](converted at lines 500-501 in__init__), so the list comprehension[Path(p) for p in self._contract_paths]unnecessarily re-wraps Path objects.♻️ Suggested simplification
contract_source: ProtocolContractSource = PluginLoaderContractSource( - contract_paths=[Path(p) for p in self._contract_paths], + contract_paths=self._contract_paths, )
…space
- Fix handler ID mismatch in HYBRID mode: contract handlers now use
bootstrap.{protocol_type} format to match bootstrap handler IDs,
enabling proper per-identity resolution and override behavior
- Add handler_version field to ModelHandlerContract with default 1.0.0
and propagate through ModelLoadedHandler and HandlerPluginLoader,
replacing hardcoded version in PluginLoaderContractSource
- Wire allowed_namespaces parameter through PluginLoaderContractSource
to HandlerPluginLoader for security namespace allowlisting
- Remove redundant Path conversion in _resolve_handler_descriptors()
since self._contract_paths is already list[Path]
- Remove stale TDD RED phase comment in unit tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py`:
- Around line 239-279: discover_handlers currently treats every contract_path as
a directory and skips files; update the loop that iterates self._contract_paths
to handle file paths as well as directories by detecting path_obj.is_file() and
calling the plugin loader's file-based API (e.g.,
self._plugin_loader.load_from_file or load_from_path) for files, while
continuing to call self._plugin_loader.load_from_directory for directories;
ensure loaded_handlers is collected from whichever loader was used and the
subsequent mapping to ModelHandlerDescriptor (handler_kind mapping, handler_id,
input_model/output_model, description, handler_class, contract_path) remains
unchanged so file-backed contract descriptors are produced the same way as
directory-backed ones.
| for path in self._contract_paths: | ||
| path_obj = Path(path) if isinstance(path, str) else path | ||
| if not path_obj.exists(): | ||
| logger.warning( | ||
| "Contract path does not exist, skipping: %s", | ||
| path_obj, | ||
| ) | ||
| continue | ||
|
|
||
| try: | ||
| # Use plugin loader to discover handlers with simpler schema | ||
| loaded_handlers = self._plugin_loader.load_from_directory( | ||
| directory=path_obj, | ||
| ) | ||
|
|
||
| # Convert ModelLoadedHandler to ModelHandlerDescriptor | ||
| for loaded in loaded_handlers: | ||
| # Map EnumHandlerTypeCategory to LiteralHandlerKind. | ||
| # handler_type is required on ModelLoadedHandler, so this always | ||
| # provides a valid value. The mapping handles COMPUTE, EFFECT, | ||
| # and NONDETERMINISTIC_COMPUTE. Falls back to "effect" for any | ||
| # unknown types as the safer option (stricter policy envelope). | ||
| handler_kind = _HANDLER_TYPE_TO_KIND.get( | ||
| loaded.handler_type, _DEFAULT_HANDLER_KIND | ||
| ) | ||
|
|
||
| descriptor = ModelHandlerDescriptor( | ||
| # Use "bootstrap." prefix to match bootstrap handler ID format. | ||
| # This enables contract handlers to override bootstrap handlers | ||
| # with the same identity in HYBRID mode, where the resolver | ||
| # compares handler_id values for per-identity resolution. | ||
| handler_id=f"bootstrap.{loaded.protocol_type}", | ||
| name=loaded.handler_name, | ||
| version=loaded.handler_version, | ||
| handler_kind=handler_kind, | ||
| input_model="omnibase_infra.models.types.JsonDict", | ||
| output_model="omnibase_core.models.dispatch.ModelHandlerOutput", | ||
| description=f"Handler: {loaded.handler_name}", | ||
| handler_class=loaded.handler_class, | ||
| contract_path=str(loaded.contract_path), | ||
| ) |
There was a problem hiding this comment.
Handle file paths in contract discovery.
contract_paths are documented as supporting files or directories, but discover_handlers() only calls load_from_directory(). File paths will be skipped and never loaded, which breaks file-based configs.
🛠️ Suggested fix
- try:
- # Use plugin loader to discover handlers with simpler schema
- loaded_handlers = self._plugin_loader.load_from_directory(
- directory=path_obj,
- )
+ try:
+ # Use plugin loader to discover handlers with simpler schema
+ if path_obj.is_file():
+ loaded_handlers = [
+ self._plugin_loader.load_from_contract(path_obj)
+ ]
+ elif path_obj.is_dir():
+ loaded_handlers = self._plugin_loader.load_from_directory(
+ directory=path_obj,
+ )
+ else:
+ logger.warning(
+ "Contract path is neither file nor directory, skipping: %s",
+ path_obj,
+ )
+ continue🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py` around lines 239
- 279, discover_handlers currently treats every contract_path as a directory and
skips files; update the loop that iterates self._contract_paths to handle file
paths as well as directories by detecting path_obj.is_file() and calling the
plugin loader's file-based API (e.g., self._plugin_loader.load_from_file or
load_from_path) for files, while continuing to call
self._plugin_loader.load_from_directory for directories; ensure loaded_handlers
is collected from whichever loader was used and the subsequent mapping to
ModelHandlerDescriptor (handler_kind mapping, handler_id,
input_model/output_model, description, handler_class, contract_path) remains
unchanged so file-backed contract descriptors are produced the same way as
directory-backed ones.
- Replace assert statement in handler_plugin_loader.py with proper ProtocolConfigurationError for production safety (assert can be stripped with Python -O flag) - Enhance handler_id prefix comment in PluginLoaderContractSource with NOTE/WARNING explaining the "bootstrap." namespace convention for HYBRID mode identity matching
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py`:
- Around line 1234-1255: In the except ValidationError as e block, instead of
re-raising the raw ValidationError for non-timezone issues, wrap and raise a
ProtocolConfigurationError created via
ModelInfraErrorContext.with_correlation(...).attach(...) (or the project's
pattern) so the new exception is an OnexError subclass and preserve chaining
with "raise ProtocolConfigurationError(...) from e"; keep the existing
timezone-aware branch behavior (return ModelHandlerSourceConfig(...)) and only
perform the wrapping/raising for other validation errors so correlation context
and cause are preserved.
♻️ Duplicate comments (1)
src/omnibase_infra/runtime/service_runtime_host_process.py (1)
239-253: Handle file contract paths, not just directories.
contract_pathscan be files, but discovery always callsload_from_directory, so file paths are silently skipped.🛠️ Suggested fix
- loaded_handlers = self._plugin_loader.load_from_directory( - directory=path_obj, - ) + if path_obj.is_file(): + loaded_handlers = [ + self._plugin_loader.load_from_contract(path_obj) + ] + elif path_obj.is_dir(): + loaded_handlers = self._plugin_loader.load_from_directory( + directory=path_obj, + ) + else: + logger.warning( + "Contract path is neither file nor directory, skipping: %s", + path_obj, + ) + continue
| except ValidationError as e: | ||
| # Check if error is due to naive datetime (no timezone info) | ||
| error_messages = [err.get("msg", "") for err in e.errors()] | ||
| if any("timezone-aware" in msg for msg in error_messages): | ||
| logger.warning( | ||
| "bootstrap_expires_at must be timezone-aware (UTC recommended). " | ||
| "Naive datetime provided - falling back to no expiry. " | ||
| "Use ISO format with timezone: '2026-02-01T00:00:00+00:00' " | ||
| "or '2026-02-01T00:00:00Z'", | ||
| extra={ | ||
| "invalid_value": expires_at_str, | ||
| "parsed_datetime": str(expires_at) if expires_at else None, | ||
| }, | ||
| ) | ||
| # Fall back to config without expiry | ||
| return ModelHandlerSourceConfig( | ||
| handler_source_mode=mode, | ||
| bootstrap_expires_at=None, | ||
| allow_bootstrap_override=allow_bootstrap_override, | ||
| ) | ||
| # Re-raise other validation errors | ||
| raise |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and view the specific lines mentioned
wc -l src/omnibase_infra/runtime/service_runtime_host_process.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 133
🏁 Script executed:
# Find files with ProtocolConfigurationError and ModelInfraErrorContext
fd . -name "*.py" -type f | head -20
rg "ProtocolConfigurationError|ModelInfraErrorContext|EnumInfraTransportType" --max-count=5 -lRepository: OmniNode-ai/omnibase_infra
Length of output: 15392
🏁 Script executed:
# Look for error class definitions
rg "class ProtocolConfigurationError|class.*InfraErrorContext|class.*EnumInfraTransportType" -A 2Repository: OmniNode-ai/omnibase_infra
Length of output: 2685
🏁 Script executed:
# View the specific lines mentioned in the review comment
sed -n '1234,1255p' src/omnibase_infra/runtime/service_runtime_host_process.py | cat -nRepository: OmniNode-ai/omnibase_infra
Length of output: 1447
🏁 Script executed:
# Get more context around those lines to understand the function
sed -n '1220,1260p' src/omnibase_infra/runtime/service_runtime_host_process.py | cat -nRepository: OmniNode-ai/omnibase_infra
Length of output: 2256
🏁 Script executed:
# Check the EnumInfraTransportType enum values
cat src/omnibase_infra/enums/enum_infra_transport_type.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 2840
🏁 Script executed:
# Check ModelInfraErrorContext and its with_correlation method
cat src/omnibase_infra/models/errors/model_infra_error_context.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 3750
🏁 Script executed:
# Check ProtocolConfigurationError class definition and usage
cat src/omnibase_infra/errors/error_infra.py | head -150Repository: OmniNode-ai/omnibase_infra
Length of output: 6114
🏁 Script executed:
# Look for error chaining patterns with ProtocolConfigurationError in the codebase
rg "raise ProtocolConfigurationError" -B 2 -A 2 | head -50Repository: OmniNode-ai/omnibase_infra
Length of output: 3942
🏁 Script executed:
# Verify imports in service_runtime_host_process.py to see what's already available
head -50 src/omnibase_infra/runtime/service_runtime_host_process.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 1848
🏁 Script executed:
# Find the function signature that contains the problematic code
rg "def.*load_handler_source_config|def.*bootstrap_expires_at" src/omnibase_infra/runtime/service_runtime_host_process.py -B 2 -A 15Repository: OmniNode-ai/omnibase_infra
Length of output: 859
🏁 Script executed:
# Get the full function definition
sed -n '1200,1280p' src/omnibase_infra/runtime/service_runtime_host_process.py | cat -nRepository: OmniNode-ai/omnibase_infra
Length of output: 4208
🏁 Script executed:
# Check the imports section more thoroughly
sed -n '1,100p' src/omnibase_infra/runtime/service_runtime_host_process.py | cat -nRepository: OmniNode-ai/omnibase_infra
Length of output: 4472
🏁 Script executed:
# Look for existing error handling patterns in this file
rg "raise.*Error|ProtocolConfigurationError|ModelInfraErrorContext" src/omnibase_infra/runtime/service_runtime_host_process.py -B 2 -A 2Repository: OmniNode-ai/omnibase_infra
Length of output: 3649
Wrap ValidationError in ProtocolConfigurationError with proper error chaining.
Re-raising ValidationError directly violates the mandatory OnexError-only rule and drops correlation context for non-timezone validation errors, breaking distributed tracing. All exceptions in this codebase must be OnexError subclasses with raise ... from e chaining and ModelInfraErrorContext.with_correlation() for infrastructure operations.
Fix
- # Re-raise other validation errors
- raise
+ context = ModelInfraErrorContext.with_correlation(
+ transport_type=EnumInfraTransportType.RUNTIME,
+ operation="load_handler_source_config",
+ )
+ raise ProtocolConfigurationError(
+ "Invalid handler_source configuration",
+ context=context,
+ ) from e🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/service_runtime_host_process.py` around lines 1234
- 1255, In the except ValidationError as e block, instead of re-raising the raw
ValidationError for non-timezone issues, wrap and raise a
ProtocolConfigurationError created via
ModelInfraErrorContext.with_correlation(...).attach(...) (or the project's
pattern) so the new exception is an OnexError subclass and preserve chaining
with "raise ProtocolConfigurationError(...) from e"; keep the existing
timezone-aware branch behavior (return ModelHandlerSourceConfig(...)) and only
perform the wrapping/raising for other validation errors so correlation context
and cause are preserved.
Address code review feedback with 3 improvements: 1. Handler ID semantics (neutral prefix + shared helper) - Create handler_identity.py with shared handler_identity() function - Change prefix from "bootstrap." to "proto." (identity namespace, not source) - Both bootstrap and contract sources now use shared function - Prevents confusion about handler source vs identity 2. Add namespace allowlist security tests (10 new tests) - test_rejects_handler_outside_allowed_namespace - test_accepts_handler_inside_allowed_namespace - test_rejects_before_import_prevents_side_effects - test_prevents_underscore_boundary_bypass - test_prevents_similar_name_typosquatting - Tests verify NAMESPACE_NOT_ALLOWED (HANDLER_LOADER_013) error 3. Document HYBRID double-discovery design decision - Add detailed docstring explaining WHY double-discovery is intentional - Add inline warning against "optimizing" to single call - Document observability symmetry and semantic correctness reasons
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/runtime/test_handler_bootstrap_source.py (2)
183-219: Docstring still references old "bootstrap." prefix pattern.Lines 187-192 describe handler IDs as
bootstrap.consul,bootstrap.db, etc., but the actual expected IDs are nowproto.*. The docstring should be updated for consistency.📝 Update docstring to reflect proto.* prefix
async def test_discovers_exactly_five_handlers(self) -> None: """discover_handlers() should return exactly 5 bootstrap handlers. The bootstrap handlers are: - - bootstrap.consul: HashiCorp Consul service discovery - - bootstrap.db: PostgreSQL database operations - - bootstrap.http: HTTP REST protocol - - bootstrap.mcp: Model Context Protocol for AI agent integration - - bootstrap.vault: HashiCorp Vault secret management + - proto.consul: HashiCorp Consul service discovery + - proto.db: PostgreSQL database operations + - proto.http: HTTP REST protocol + - proto.mcp: Model Context Protocol for AI agent integration + - proto.vault: HashiCorp Vault secret management """
203-219: Docstring references old "bootstrap." prefix pattern.Line 207-208 states
Handler IDs must follow the pattern "bootstrap.<service_name>"but the test now expectsproto.*IDs.📝 Update docstring pattern description
async def test_discovered_handler_ids_match_expected(self) -> None: """All discovered handlers should have expected handler_id values. - Handler IDs must follow the pattern "bootstrap.<service_name>" where + Handler IDs must follow the pattern "proto.<service_name>" where service_name is one of: consul, db, http, mcp, vault. """
♻️ Duplicate comments (2)
src/omnibase_infra/runtime/service_runtime_host_process.py (2)
243-256: Handle file-based contract paths.
contract_pathsare documented to accept files, but discovery always callsload_from_directory(), so file paths will be skipped.🛠️ Proposed fix
- loaded_handlers = self._plugin_loader.load_from_directory( - directory=path_obj, - ) + if path_obj.is_file(): + loaded_handlers = [ + self._plugin_loader.load_from_contract(path_obj) + ] + elif path_obj.is_dir(): + loaded_handlers = self._plugin_loader.load_from_directory( + directory=path_obj, + ) + else: + logger.warning( + "Contract path is neither file nor directory, skipping: %s", + path_obj, + ) + continue
1232-1260: WrapValidationErrorinProtocolConfigurationErrorwith chaining.
Re‑raisingValidationErrorbreaks the OnexError‑only rule and drops correlation context.As per coding guidelines, only OnexError subclasses should be raised.🛠️ Proposed fix
- # Re-raise other validation errors - raise + context = ModelInfraErrorContext.with_correlation( + transport_type=EnumInfraTransportType.RUNTIME, + operation="load_handler_source_config", + ) + raise ProtocolConfigurationError( + "Invalid handler_source configuration", + context=context, + ) from e
🧹 Nitpick comments (6)
src/omnibase_infra/runtime/handler_identity.py (1)
45-75: Consider adding input validation forprotocol_type.The function accepts any string, including empty strings or strings with invalid characters. This could produce malformed handler IDs like
"proto."or"proto.foo.bar".♻️ Optional: Add minimal validation
def handler_identity(protocol_type: str) -> str: + if not protocol_type or "." in protocol_type: + raise ValueError( + f"protocol_type must be a non-empty string without dots, got: {protocol_type!r}" + ) return f"{HANDLER_IDENTITY_PREFIX}.{protocol_type}"Alternatively, if callers are trusted internal code, document the expected format in the docstring Args section (e.g., "must be a simple identifier like 'consul', 'http'").
src/omnibase_infra/runtime/handler_bootstrap_source.py (1)
429-438: Verify exception handling doesn't suppress useful context.The bare
raisepreserves the originalProtocolConfigurationError, but the precedinglogger.exception()already logs the traceback. This is fine, but consider whether the log level should be ERROR instead of usingexception()which implies ERROR + traceback.The current approach is acceptable since fail-fast is the intended behavior for bootstrap handlers. No change needed.
tests/unit/runtime/handler_plugin_loader/test_handler_plugin_loader_security.py (3)
300-324: Good boundary bypass test, but accesses private method directly.The test calls
loader._validate_namespace()which is a private method (underscore prefix). While this provides precise testing of the validation logic, consider whether this creates coupling to implementation details.If the public API (
load_from_contract) provides sufficient coverage, testing through that interface is more robust against refactoring. However, for security boundary tests, direct testing of the validation function is acceptable to ensure the exact behavior.
351-380: Clarify expected behavior in docstring.The docstring states that
"omnibase"(without period) should match"omnibase.handlers.Auth"but NOT"omnibase_core.handlers.Handler". This is the correct behavior, but the test nametest_prevents_namespace_without_period_from_matching_extended_namesis slightly confusing since "extended names" could be misread.The test logic is correct. The current name works, but consider renaming to
test_namespace_without_period_enforces_dot_boundaryfor clarity.
146-195: Remove unnecessary@pytest.mark.asynciodecorator.The
load_from_contract()method is synchronous (defined asdefnotasync definhandler_plugin_loader.py:374), and this test contains no await statements or other async operations.♻️ Consider making this a sync test
- `@pytest.mark.asyncio` - async def test_accepts_handler_inside_allowed_namespace( + def test_accepts_handler_inside_allowed_namespace( self, tmp_path: Path, ) -> None:tests/integration/runtime/test_bootstrap_source_integration.py (1)
238-312: Well-documented HYBRID mode double-discovery test.The docstring clearly explains the intentional design decision:
discover_handlers()is called twice in HYBRID mode for observability symmetry and semantic correctness. The warning against "optimizing" this behavior is valuable for future maintainers.However, the inline imports at lines 259-260 could be moved to the module level for consistency.
♻️ Consider moving imports to module level
from omnibase_infra.runtime.service_runtime_host_process import RuntimeHostProcess +from omnibase_core.models.primitives import ModelSemVer +from omnibase_infra.models.handlers import ModelHandlerDescriptor # ... async def test_bootstrap_source_called_during_start(self) -> None: # ... - from omnibase_core.models.primitives import ModelSemVer - from omnibase_infra.models.handlers import ModelHandlerDescriptorInline imports in test methods are acceptable but can make import analysis harder. Since these are standard project models, module-level imports are cleaner.
… metric Address code review feedback with two improvements: 1. ModelHandlerSourceConfig: Add _coerce_allow_bootstrap_override field validator to handle string-to-bool coercion (true/yes/1/on) at the Pydantic layer rather than in runtime config loading 2. HandlerSourceResolver: Add validation_error_count metric to HYBRID mode logging for complete observability of error aggregation
…_config The manual bool coercion for allow_bootstrap_override was redundant since ModelHandlerSourceConfig._coerce_allow_bootstrap_override field validator (mode="before") already handles string-to-bool coercion at the Pydantic layer. This simplifies the code by passing the raw value directly to the model and letting the field validator handle coercion uniformly.
Code review follow-up improvements: - Add explicit int/float handling in _coerce_allow_bootstrap_override validator (0/0.0 → False, other numbers → True, unknown types → False) - Document forward reference resolution pattern for ModelContractDiscoveryResult in __init__.py and model docstrings - Ensure model_rebuild() called at module level in all runtime modules that create ModelContractDiscoveryResult instances (idempotent pattern due to circular import constraint) - Clean up redundant code in handler_bootstrap_source.py and handler_contract_source.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/models/handlers/model_handler_source_config.py`:
- Around line 152-180: The string-to-bool parsing should strip whitespace before
lowercasing (change the str branch to call value.strip().lower()) and the
_validate_expires_at_timezone validator must raise the project's OnexError (or
an appropriate OnexError subclass) instead of ValueError when value.tzinfo is
None; update the exception type and message accordingly and ensure OnexError is
imported where _validate_expires_at_timezone is defined (reference symbols: the
string-to-bool branch at the top of the snippet and the
_validate_expires_at_timezone validator for bootstrap_expires_at).
In `@tests/unit/runtime/test_handler_contract_source.py`:
- Around line 953-958: Update the comment that currently claims "model_rebuild()
is called centrally in models.handlers.__init__" to accurately state that
forward-reference resolution (model_rebuild) is performed by the runtime
modules, not in omnibase_infra.models.handlers.__init__ which explicitly avoids
it; adjust the two comment occurrences referencing ModelContractDiscoveryResult
and model_rebuild to say the forward reference is resolved centrally in the
runtime modules so future readers are not misled.
| if isinstance(value, str): | ||
| return value.lower() in ("true", "yes", "1", "on") | ||
| if isinstance(value, (int, float)): | ||
| # Explicit: 0/0.0 = False, any other number = True | ||
| return bool(value) | ||
| # Unknown types default to False for safety | ||
| return False | ||
|
|
||
| @field_validator("bootstrap_expires_at") | ||
| @classmethod | ||
| def _validate_expires_at_timezone(cls, value: datetime | None) -> datetime | None: | ||
| """Validate and normalize bootstrap_expires_at to UTC. | ||
|
|
||
| Args: | ||
| value: The datetime value to validate. | ||
|
|
||
| Returns: | ||
| None if value is None, otherwise the datetime normalized to UTC. | ||
|
|
||
| Raises: | ||
| ValueError: If the datetime is naive (no timezone info). | ||
| """ | ||
| if value is None: | ||
| return None | ||
| if value.tzinfo is None: | ||
| raise ValueError( | ||
| "bootstrap_expires_at must be timezone-aware (UTC recommended). " | ||
| "Use datetime.now(timezone.utc) or datetime(..., tzinfo=timezone.utc)." | ||
| ) |
There was a problem hiding this comment.
Use OnexError for expiry validation; trim boolean strings.
Line 176 raises ValueError, which conflicts with the repo rule to raise only OnexError subclasses. Also, Line 152 would be more robust if it stripped whitespace before lowercasing. As per coding guidelines.
🛠️ Proposed fix
-from datetime import UTC, datetime, timezone
+from datetime import UTC, datetime, timezone
from pydantic import BaseModel, ConfigDict, Field, field_validator
+from omnibase_core.models.errors.model_onex_error import ModelOnexError
from omnibase_infra.enums.enum_handler_source_mode import EnumHandlerSourceMode
@@
if isinstance(value, bool):
return value
if isinstance(value, str):
- return value.lower() in ("true", "yes", "1", "on")
+ return value.strip().lower() in ("true", "yes", "1", "on")
@@
if value.tzinfo is None:
- raise ValueError(
- "bootstrap_expires_at must be timezone-aware (UTC recommended). "
- "Use datetime.now(timezone.utc) or datetime(..., tzinfo=timezone.utc)."
- )
+ raise ModelOnexError(
+ "bootstrap_expires_at must be timezone-aware (UTC recommended). "
+ "Use datetime.now(timezone.utc) or datetime(..., tzinfo=timezone.utc).",
+ error_code="HANDLER_SOURCE_CONFIG_001",
+ )🤖 Prompt for AI Agents
In `@src/omnibase_infra/models/handlers/model_handler_source_config.py` around
lines 152 - 180, The string-to-bool parsing should strip whitespace before
lowercasing (change the str branch to call value.strip().lower()) and the
_validate_expires_at_timezone validator must raise the project's OnexError (or
an appropriate OnexError subclass) instead of ValueError when value.tzinfo is
None; update the exception type and message accordingly and ensure OnexError is
imported where _validate_expires_at_timezone is defined (reference symbols: the
string-to-bool branch at the top of the snippet and the
_validate_expires_at_timezone validator for bootstrap_expires_at).
| This test imports ModelContractDiscoveryResult - the forward reference is | ||
| resolved centrally in omnibase_infra.models.handlers.__init__. | ||
| """ | ||
| # Import through handler_contract_source which calls model_rebuild() | ||
| # Import models - model_rebuild() is called centrally in models.handlers.__init__ | ||
| from omnibase_infra.models.errors import ModelHandlerValidationError | ||
| from omnibase_infra.runtime.handler_contract_source import ( | ||
| ModelContractDiscoveryResult, | ||
| ) | ||
| from omnibase_infra.models.handlers import ModelContractDiscoveryResult |
There was a problem hiding this comment.
Align forward-reference comments with actual rebuild location.
The comments say model_rebuild is centralized in models.handlers.init, but that module explicitly avoids it; runtime modules handle resolution. Updating the wording prevents confusion.
📝 Suggested wording
- resolved centrally in omnibase_infra.models.handlers.__init__.
+ resolved in runtime modules that import ModelHandlerValidationError
+ (e.g., handler_contract_source.py, registry_contract_source.py).
@@
- # Import models - model_rebuild() is called centrally in models.handlers.__init__
+ # Import models - model_rebuild() is called in runtime modules
+ # (e.g., handler_contract_source.py)Also applies to: 1007-1008
🤖 Prompt for AI Agents
In `@tests/unit/runtime/test_handler_contract_source.py` around lines 953 - 958,
Update the comment that currently claims "model_rebuild() is called centrally in
models.handlers.__init__" to accurately state that forward-reference resolution
(model_rebuild) is performed by the runtime modules, not in
omnibase_infra.models.handlers.__init__ which explicitly avoids it; adjust the
two comment occurrences referencing ModelContractDiscoveryResult and
model_rebuild to say the forward reference is resolved centrally in the runtime
modules so future readers are not misled.
1 participant
Summary
EnumHandlerSourceModeenum with BOOTSTRAP, CONTRACT, HYBRID valuesModelHandlerSourceConfigwith bootstrap expiry supportHandlerSourceResolverwith per-handler identity resolutioncontract_handler_count,bootstrap_handler_count,fallback_handler_count,override_countKey Decision
Resolution is per-handler identity, not whole-source failover.
Test plan
Linear: https://linear.app/omninode/issue/OMN-1095
Summary by CodeRabbit
New Features
Configuration
Models
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.