Repository navigation
feat(runtime): implement HandlerBootstrapSource for centralized handler wiring [OMN-1087] - #176
Conversation
…er wiring [OMN-1087] Add HandlerBootstrapSource class that implements ProtocolContractSource protocol to centralize all hardcoded handler registration. This replaces scattered handler wiring with a single source of truth for bootstrap handlers. Handlers registered: - bootstrap.consul: HashiCorp Consul service discovery - bootstrap.db: PostgreSQL database handler - bootstrap.http: HTTP REST protocol handler - bootstrap.vault: HashiCorp Vault secret management All handlers are registered as ModelHandlerDescriptor instances with: - handler_kind="effect" (all are effect-type handlers) - version="1.0.0" (stable bootstrap version) - contract_path=None (no YAML contract, bootstrap-defined) Includes 41 comprehensive unit tests covering: - Protocol compliance with ProtocolContractSource - Handler discovery and descriptor validation - Graceful mode API consistency - Idempotency and performance characteristics
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded a bootstrap handler source that provides four hardcoded bootstrap handler descriptors (consul, db, http, vault) with fully-qualified handler class paths; ModelHandlerDescriptor gained an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host as RuntimeHostProcess
participant Bootstrap as HandlerBootstrapSource
participant Importer as importlib
participant Registry as HandlerRegistry
Host->>Bootstrap: discover_handlers()
activate Bootstrap
Bootstrap->>Bootstrap: build descriptors for\nconsul, db, http, vault
Bootstrap-->>Host: ModelContractDiscoveryResult(descriptors)
deactivate Bootstrap
Host->>Host: _register_bootstrap_handlers()
loop per descriptor
Host->>Importer: import descriptor.handler_class
alt import succeeds
Importer-->>Host: HandlerClass
Host->>Registry: register HandlerClass by protocol
else import fails
Importer-->>Host: ImportError
Host->>Host: log warning, continue
end
end
Host->>Host: proceed with contract-based discovery/wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Comment |
PR Review: HandlerBootstrapSource Implementation [OMN-1087]Overall Assessment: ✅ APPROVEThis is a well-implemented, thoroughly tested PR that centralizes bootstrap handler registration. The code follows ONEX patterns correctly and includes excellent test coverage. Strengths1. Excellent Code Quality
2. Comprehensive Test Coverage (813 lines)The test suite is exemplary with 41 tests covering:
Test organization into logical classes makes the suite maintainable and easy to understand. 3. Smart Forward Reference HandlingThe _model_rebuild_state: list[bool] = [False] # Avoids global statement (PLW0603)This is a clean solution to a common Pydantic issue. Issues Found🔴 CRITICAL: Missing Handler Module PathsLocation: The Current definition: _BOOTSTRAP_HANDLER_DEFINITIONS: list[dict[str, str]] = [
{
"handler_id": "bootstrap.consul",
"name": "Consul Handler",
"description": "HashiCorp Consul service discovery handler",
"handler_kind": "effect",
"input_model": "omnibase_core.types.JsonDict",
"output_model": "omnibase_core.models.dispatch.ModelHandlerOutput",
# MISSING: handler_module and handler_class
},
# ... other handlers
]Expected fields (based on CLAUDE.md handler_contract.yaml pattern): "handler_module": "omnibase_infra.handlers.handler_consul",
"handler_class": "HandlerConsul",Impact: If Recommendation:
|
| Test Class | Test Count | Coverage |
|---|---|---|
| Protocol Compliance | 5 | ✅ Excellent |
| Handler Discovery | 10 | ✅ Excellent |
| Descriptor Validation | 11 | ✅ Excellent |
| Graceful Mode | 6 | ✅ Excellent |
| Idempotency | 4 | ✅ Excellent |
| Edge Cases | 4 | ✅ Excellent |
| Performance | 2 | ✅ Excellent |
Notable test strengths:
- Tests verify protocol compliance via
isinstance(source, ProtocolContractSource) - Validates all 4 handler descriptors individually
- Checks immutability (frozen models)
- Performance bounds testing (100 rapid calls)
No significant gaps identified.
CLAUDE.md Compliance
✅ Fully compliant with ONEX patterns:
| Requirement | Status | Evidence |
|---|---|---|
No Any types |
✅ | No Any usage detected |
| File naming | ✅ | Not a handler, handler_bootstrap_source.py is correct for handler infrastructure |
| Strong typing | ✅ | Proper Pydantic models, list[dict[str, str]] |
| Protocol implementation | ✅ | Extends ProtocolContractSource, runtime checkable |
| Naming exemption | ✅ | Properly documented in validation_exemptions.yaml |
| No backwards compatibility | ✅ | New code, no legacy support |
The inline comment # naming-ok (line 140) properly documents the exemption.
Recommendations Summary
Must Fix (Blocking)
- ❗ Verify
handler_module/handler_classfields - Check ifModelHandlerDescriptorrequires these and add if needed
Should Fix (Non-blocking)
⚠️ Document graceful_mode behavior - Add note that parameter is unused but kept for API consistency
Nice to Have
- ℹ️ Improve
type: ignorecomment clarity - ℹ️ Consider more specific validation exemption pattern
Conclusion
This is high-quality code that demonstrates strong engineering discipline:
- Clean architecture with proper protocol usage
- Exceptional test coverage (41 tests, 813 lines)
- Good performance characteristics
- Excellent documentation
The only blocking concern is verifying whether handler_module/handler_class fields are required in the handler descriptors. Once confirmed, this PR is ready to merge.
Recommendation: ✅ APPROVE (pending verification of handler descriptor fields)
Review conducted following CLAUDE.md guidelines for ONEX infrastructure.
…rce [OMN-1087] - Add handler_class field to ModelHandlerDescriptor for dynamic handler import - Add handler_class to all bootstrap handler definitions (Consul, DB, HTTP, Vault) - Extract handler_class in HandlerContractSource with TODO for omnibase_core update - Document graceful_mode as unused but kept for API consistency - Improve type: ignore comment clarity with safety rationale - Add pattern specificity documentation to validation exemption
Pull Request Review: HandlerBootstrapSource Implementation [OMN-1087]SummaryThis PR implements ✅ StrengthsArchitecture & Design
Code Quality
Testing
|
| Category | Test Count | Coverage |
|---|---|---|
| Protocol Compliance | 5 | ✅ Excellent |
| Handler Discovery | 8 | ✅ Excellent |
| Descriptor Validation | 11 | ✅ Excellent |
| Graceful Mode | 6 | ✅ Excellent |
| Idempotency | 4 | ✅ Excellent |
| Edge Cases | 5 | ✅ Excellent |
| Performance | 2 | ✅ Good |
Test Quality Observations
- Positive: Tests are well-organized with descriptive names
- Positive: Each test class focuses on a specific aspect
- Positive: Good coverage of both happy paths and edge cases
- Suggestion: Consider adding integration tests that verify the handlers can actually be imported dynamically using the
handler_classpaths
🔒 Security Considerations
✅ No Security Risks Identified
- Bootstrap handlers are hardcoded - no dynamic code execution
- No user input processing
- No filesystem access or network I/O
- Handler class paths are hardcoded constants, not user-provided
📝 Documentation Quality
✅ Excellent Documentation
- Module docstring clearly explains purpose and lists all registered handlers
- Class docstring includes protocol compliance notes, API consistency explanation, examples, and performance characteristics
- Method docstrings explain parameters, return values, and behavior
- TODO comments explain workarounds (though ticket number needs updating)
Suggestion
Add a brief mention in CLAUDE.md about the bootstrap handler pattern to help future developers understand when to use HandlerBootstrapSource vs HandlerContractSource.
🎯 Recommendations
Must Fix Before Merge
- Resolve TODO ticket number - Either create the ticket or document why it's deferred
- Consider type safety improvement - Remove
type: ignoreby using proper typing
Should Fix (Non-Blocking)
- Clarify
_HANDLER_TYPE_DATABASEvs `"db"" naming inconsistency - Add inline comment for unused
graceful_modestorage
Nice to Have
- Add integration test for dynamic handler class loading
- Update
CLAUDE.mdwith bootstrap handler pattern guidance
✅ Final Verdict
APPROVED with minor suggestions
This is a well-designed, thoroughly tested implementation that follows ONEX infrastructure patterns correctly. The identified issues are minor and mostly relate to documentation/type safety improvements rather than functional problems.
The PR successfully:
- Centralizes hardcoded handler registration per OMN-1087
- Implements the
ProtocolContractSourceprotocol properly - Provides comprehensive test coverage (41 tests)
- Follows ONEX naming conventions and validation patterns
- Maintains clean separation from filesystem-based contract discovery
Recommendation: Merge after addressing the TODO ticket number (item #1). Other suggestions can be addressed in follow-up PRs if preferred.
Great work on this implementation! The code quality, testing, and documentation are all excellent. 🎉
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/handler_bootstrap_source.py`:
- Around line 100-137: _BOOTSTRAP_HANDLER_DEFINITIONS contains four handler
dicts whose "input_model" wrongly points to "omnibase_core.types.JsonDict"
(which doesn't exist); update each handler dict's "input_model" to the correct
type path — either "omnibase_core.types.JsonType" (preferred canonical type) or
"omnibase_infra.models.types.JsonDict" if you want the local alias — so change
the "input_model" value for the entries with handler_class
"omnibase_infra.handlers.handler_consul.HandlerConsul",
"omnibase_infra.handlers.handler_db.HandlerDb",
"omnibase_infra.handlers.handler_http.HandlerHttpRest", and
"omnibase_infra.handlers.handler_vault.HandlerVault".
🧹 Nitpick comments (2)
tests/unit/runtime/test_handler_bootstrap_source.py (1)
687-700: Consider using a more specific exception type for frozen model test.The
pytest.raises(Exception)is broad. For Pydantic frozen models, the specific exception ispydantic_core._pydantic_core.ValidationError(or the wrapperpydantic.ValidationError). Using a more specific type would make the test more precise.💡 Suggested refinement
`@pytest.mark.asyncio` async def test_descriptors_are_frozen(self) -> None: """ModelHandlerDescriptor instances should be frozen (immutable). The ModelHandlerDescriptor model config has frozen=True. """ + from pydantic import ValidationError + source = HandlerBootstrapSource() result = await source.discover_handlers() for descriptor in result.descriptors: # Attempting to modify a frozen model should raise an error - with pytest.raises(Exception): # ValidationError for frozen models + with pytest.raises(ValidationError): descriptor.handler_id = "modified" # type: ignore[misc]src/omnibase_infra/runtime/handler_contract_source.py (1)
480-495: Placeholder ticket number should be replaced with actual ticket.The TODO comment references
OMN-XXXXwhich is a placeholder. This should be replaced with an actual ticket number to track the work of addinghandler_classtoModelHandlerContractin omnibase_core.📋 Suggested fix
Create a ticket in your issue tracker for updating
ModelHandlerContractto includehandler_class, then replace the placeholder:- # TODO [OMN-XXXX]: Extract handler_class from raw_data + # TODO [OMN-1234]: Extract handler_class from ModelHandlerContractWhere
OMN-1234is the actual ticket number.
- Fix incorrect input_model path: omnibase_core.types.JsonDict → omnibase_infra.models.types.JsonDict (CRITICAL) - Add BootstrapEffectDefinition TypedDict to eliminate type: ignore comment - Add design note explaining handler_class vs handler_module decision - Use pydantic.ValidationError instead of generic Exception in test - Replace placeholder OMN-XXXX with OMN-1087 in handler_contract_source - Export LiteralHandlerKind from models/handlers for TypedDict usage
Pull Request Review: HandlerBootstrapSource ImplementationI've reviewed PR #176 implementing HandlerBootstrapSource for centralized handler registration. Overall, this is a well-designed and well-tested implementation that follows ONEX conventions. Here's my detailed feedback: ✅ Strengths1. Excellent Code Quality
2. Outstanding Test Coverage
3. Documentation Excellence
4. Proper Error Handling
🔍 Code Quality ObservationsArchitectural Decisions ✅
Potential Improvements 🔧1. Thread Safety Consideration (Minor)Location: Issue: The current implementation has a potential race condition in multi-threaded contexts: if _model_rebuild_state[0]: # Thread A checks: False
return
# Thread B could execute here and also pass the check
ModelContractDiscoveryResult.model_rebuild() # Both threads call rebuild
_model_rebuild_state[0] = TrueImpact: Low - Suggestion: Use import threading
_model_rebuild_lock = threading.Lock()
_model_rebuild_state: list[bool] = [False]
def _ensure_model_rebuilt() -> None:
if _model_rebuild_state[0]:
return
with _model_rebuild_lock:
# Double-check pattern
if _model_rebuild_state[0]:
return
from omnibase_infra.models.errors import ModelHandlerValidationError
ModelContractDiscoveryResult.model_rebuild()
_ = ModelHandlerValidationError
_model_rebuild_state[0] = TruePriority: Optional - only relevant if bootstrap source is called concurrently during startup. 2. Handler Class Path Validation (Minor Enhancement)Location: Observation: The Current risk: Low - changes to handler locations would be caught at import time by the plugin loader. Suggestion (optional): Consider adding a validation test that attempts to import each handler class to catch stale paths early: @pytest.mark.asyncio
async def test_all_handler_classes_are_importable() -> None:
"""Verify all bootstrap handler classes can be imported."""
import importlib
source = HandlerBootstrapSource()
result = await source.discover_handlers()
for descriptor in result.descriptors:
if descriptor.handler_class:
module_path, class_name = descriptor.handler_class.rsplit('.', 1)
module = importlib.import_module(module_path)
handler_cls = getattr(module, class_name)
assert handler_cls is not NonePriority: Low - this is a nice-to-have that would catch refactoring issues earlier. 3. Consistency with HandlerContractSource (Documentation)Location: Observation: The TODO comment in Note: This PR correctly adds Suggestion: Track the TODO [OMN-1087] mentioned in the comment to update 🔒 Security ReviewNo Security Concerns ✅
📋 ONEX Convention Compliance
🧪 Testing RecommendationsAdditional Test Scenarios (Optional)
📊 Performance CharacteristicsThe implementation correctly documents and tests performance guarantees:
The performance tests (lines 1265-1313) are well-designed with appropriate tolerances for CI variability. ✨ Best Practices Highlights
🎯 Final RecommendationAPPROVE ✅ with optional minor improvements This PR is production-ready and demonstrates excellent software engineering practices. The suggested improvements are minor and optional - they don't block merging. Required Before Merge
Optional Follow-ups
📝 SummaryThis is a high-quality implementation that successfully centralizes hardcoded handler registration. The code is clean, well-tested, properly documented, and follows all ONEX conventions. The architectural design is sound and provides a clean foundation for the handler plugin system. Great work on this PR! 🚀 |
…strapSource [OMN-1087] - Add thread-safe initialization for _ensure_model_rebuilt() using double-checked locking pattern to prevent race conditions - Add test_all_handler_classes_are_importable() test to verify handler class paths are valid and can be dynamically imported
Pull Request Review: HandlerBootstrapSource ImplementationThis PR implements HandlerBootstrapSource to centralize hardcoded handler registration. The code is high-quality, well-tested, and follows all ONEX patterns. Strengths
Code Review FindingsCRITICAL: Potential Division by Zero (Low Impact)
MINOR: Unused graceful_mode Parameter
Test Coverage AnalysisExcellent coverage: protocol compliance, all 4 handlers discovered correctly, idempotency, descriptor immutability, dynamic import validation, performance. Potential gaps: No concurrent access test for _ensure_model_rebuilt() thread safety, no error injection tests. Security ReviewNo concerns - hardcoded definitions, no user input, handler class validation via regex, all classes verified importable. ONEX Architecture ComplianceFully compliant with CLAUDE.md: No Any types, PEP 604 unions, strong typing with LiteralHandlerKind, protocol-based design, proper exports, pattern validation exemption documented. Recommendations SummaryMust Fix: None - code is production-ready Should Consider (low priority): Division by zero edge case, optional concurrent access test Nice to Have: Add warning when graceful_mode=True, stricter regex validation Final Verdict: APPROVEDHigh-quality, well-tested, properly documented code following all ONEX patterns. Thread-safe, type-safe, comprehensive test coverage (41 tests). Minor issues are cosmetic and do not block merging. Reviewed against: CLAUDE.md (ONEX Infrastructure patterns) |
…Source [OMN-1087] - Add inline comment explaining graceful_mode parameter storage (stored for API consistency only - bootstrap handlers cannot fail) - Fix division by zero edge case in handlers_per_sec calculation (returns inf when duration_seconds == 0 and handlers discovered)
PR Review: HandlerBootstrapSource Implementation [OMN-1087]This PR implements a clean, well-designed solution for centralizing hardcoded handler registration. Strengths1. Excellent Code Quality
2. Strong ONEX Compliance
3. Comprehensive Test Coverage
4. Security
Code Review FindingsCritical IssuesNone - PR is ready to merge ✅ Design Considerations
Test Quality: 5/5All categories excellent:
Architecture: 5/5Correct ONEX patterns:
RecommendationsMust Have (Before Merge)None - ready to merge ✅ Should Have (Low Priority)
Nice to Have
Summary Score
Final VerdictAPPROVED ✅ This PR is exceptionally well-implemented with:
Recommendation: Merge after optionally addressing documentation note about performance claims. Great work! The TypedDict for type safety and comprehensive test suite set a high bar for future implementations. Review by Claude Code following ONEX standards in CLAUDE.md |
…urce [OMN-1087] - Clarify performance claims: docstring now distinguishes typical (<1ms local) from test threshold (100ms for CI variance) - Update TODO comment to reference OMN-1420 (follow-up for ModelHandlerContract handler_class field in omnibase_core) - Fix pyproject.toml: remove invalid TC006 rule (doesn't exist in ruff)
Code Review - PR #176: HandlerBootstrapSource ImplementationThis PR implements a centralized bootstrap handler source for hardcoded handler registration. The implementation is high quality with excellent test coverage and follows ONEX architectural patterns. Strengths1. Excellent Architecture and Design
2. Outstanding Test Coverage (856 lines!)
3. Clear Documentation
Potential IssuesCRITICAL: handler_class Field ValidationLocation: model_handler_descriptor.py:49-54 The regex pattern is too permissive and allows minimal paths like a.b.c or evil.module.DeleteDatabase. Recommendation: Ensure namespace allowlisting is ENABLED in production configurations per CLAUDE.md security patterns. Minor: TODO Comment TrackingLocation: handler_contract_source.py:478-494 TODO OMN-1420 references extracting handler_class from ModelHandlerContract. Verify this ticket exists and is tracked. PerformanceExcellent performance characteristics:
SecurityBuilt-in protections include YAML safe loading, protocol validation, and namespace allowlisting. Ensure deployment checklist items from CLAUDE.md are followed especially namespace allowlisting ENABLED in production. Test CoverageExceptional test quality with 41 tests across 7 test classes covering all aspects. Overall AssessmentVerdict: APPROVE with minor recommendations This is excellent work demonstrating strong architectural design, outstanding test coverage, clear documentation, and proper ONEX conventions. The implementation is production-ready. Action Items for follow-up:
Great job! |
… [OMN-1087] Ruff 0.14.x introduced TC006 lint rule requiring quoted type expressions in typing.cast() calls to prevent runtime type evaluation issues with TYPE_CHECKING-only imports. Changes: - Add quotes to 26 cast() type expressions across 13 files - Auto-fixed via `ruff check --fix` Affected modules: - handlers/mixins/mixin_consul_kv.py - handlers/service_discovery/handler_service_discovery_consul.py - mixins/mixin_node_introspection.py - mixins/mixin_retry_execution.py - nodes/node_registration_orchestrator/wiring.py - plugins/examples/plugin_json_normalizer.py - plugins/examples/plugin_json_normalizer_error_handling.py - runtime/service_message_dispatch_engine.py - utils/util_dsn_validation.py - validation/validator_chain_propagation.py - validation/validator_runtime_shape.py - tests/chaos/test_recovery_dlq.py - tests/integration/registration/e2e/performance_utils.py
PR Review: HandlerBootstrapSource ImplementationOverviewThis PR implements HandlerBootstrapSource to centralize hardcoded handler registration for core infrastructure handlers. Implements ProtocolContractSource protocol and provides 4 bootstrap handlers as ModelHandlerDescriptor instances. Strengths1. Excellent Architecture
2. Type Safety
3. Documentation
Potential IssuesCRITICAL: Input Model Path VerificationBootstrap handlers specify: input_model: omnibase_core.types.JsonDict Action Required: Verify this path is correct vs omnibase_infra.models.types.JsonDict Recommendation: Add test to verify input_model and output_model paths are importable Test Coverage GapsMissing:
Minor: Validation Exemption DocumentationNeeds clearer explanation in validation_exemptions.yaml Test Coverage: 41 Tests
Approval RecommendationAPPROVE with minor fixes Excellent implementation following ONEX patterns. Only blocking issue is verifying input_model import path. Risk: LOW Highlights
Review conducted per CLAUDE.md guidelines |
…087] Address PR #176 review recommendations: 1. Input/output model importability tests (TestHandlerBootstrapSourceModelImportability): - test_all_input_models_are_importable: Verifies input_model paths resolve - test_all_output_models_are_importable: Verifies output_model paths resolve - test_input_model_jsondict_is_correct_type: Confirms omnibase_infra path - test_output_model_handler_output_is_pydantic_model: Validates BaseModel 2. Concurrent discovery thread safety tests (TestHandlerBootstrapSourceThreadSafety): - test_concurrent_discovery_returns_consistent_results: Asyncio concurrency - test_concurrent_discovery_with_multiple_sources: Multiple source instances - test_model_rebuild_lock_is_thread_safe: Thread-based concurrency - test_rapid_concurrent_discovery_stress: High-concurrency stress test The input_model paths were already fixed in prior commit (4f12625) to use omnibase_infra.models.types.JsonDict instead of the incorrect omnibase_core.types.JsonDict path. These tests ensure the fix stays correct.
PR Review: HandlerBootstrapSource Implementation (OMN-1087)SummaryThis PR implements a well-designed bootstrap handler source that centralizes hardcoded handler registration. The implementation is solid with excellent test coverage (41 comprehensive unit tests) and follows ONEX patterns consistently. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Test Coverage ⭐Exceptionally thorough test suite covering:
4. Documentation
5. Adherence to ONEX Patterns
🔍 Code Review FindingsMinor Issues1. Type Checker Suppression Comment (Low Priority)File: # Suppress unused variable warning - the import is needed for model_rebuild()
_ = ModelHandlerValidationErrorIssue: The comment mentions "unused variable warning" but the actual suppression is for model_rebuild() side effects. Suggestion: Clarify comment to better reflect intent: # Import needed in scope for Pydantic model_rebuild() to resolve forward references
_ = ModelHandlerValidationError # Keep import in scope for rebuild2. Graceful Mode Documentation ClarityFile: Observation: The docstring notes that Suggestion: Add note about future extensibility: Note: This parameter is currently unused. Bootstrap handlers are hardcoded
definitions that cannot fail validation. If future bootstrap handlers involve
dynamic discovery or validation, this mode would enable error collection.3. Error Handling for Model RebuildFile: Observation: The Question: Should we wrap the rebuild in a try-except to handle potential circular import or validation errors more gracefully? This would prevent obscure errors during bootstrap. Potential Enhancement: try:
ModelContractDiscoveryResult.model_rebuild()
_model_rebuild_state[0] = True
except Exception as e:
logger.error("Failed to rebuild ModelContractDiscoveryResult: %s", e)
raise RuntimeError(
f"Model rebuild failed during bootstrap initialization: {e}"
) from e🎯 Verification Items✅ Completed
|
…y [OMN-1087] Address code review feedback: 1. Clarified comment explaining why ModelHandlerValidationError import must be in scope for Pydantic's forward reference resolution 2. Added try-except around model_rebuild() to provide clear error message instead of obscure Pydantic errors when circular imports or missing type definitions occur 3. Renamed comment from "suppress unused variable warning" to better reflect the intent: "keep import reference in scope for forward refs"
…ent-handlerbootstrapsource-descriptor-based
PR Review: HandlerBootstrapSource Implementation [OMN-1087]This PR implements a well-structured HandlerBootstrapSource class that centralizes hardcoded handler registration for core infrastructure handlers. The implementation follows ONEX patterns and includes comprehensive test coverage (41 unit tests, 1145 lines). Strengths1. Excellent Code Quality
2. Strong Test Coverage (1145 lines)
3. Proper ONEX Patterns
4. Security
Issues & RecommendationsCRITICAL: Unused graceful_mode ParameterLocation: handler_bootstrap_source.py:250-270 Issue: The graceful_mode parameter is accepted but never used. While the docstring explains this is for API consistency, this creates a misleading interface. Why this matters:
Recommendation (PREFERRED): Remove the parameter entirely
Alternative: Keep for protocol compatibility but add warning when used MINOR: Ruff TC006 RuleThe PR removes the TC006 ignore, requiring cast expressions to use string literals. This is a minor style consistency improvement. No action needed. Files affected: mixin_consul_kv.py, handler_service_discovery_consul.py, test files OBSERVATION: Model Rebuild PatternThe model rebuild pattern (lines 77-120) is identical to handler_contract_source.py. Consider extracting to shared utility if more sources are added (NOT for this PR). PerformanceExcellent performance characteristics validated by tests:
SecurityNo security concerns identified:
ONEX ComplianceChecked against CLAUDE.md:
VerdictAPPROVE with recommendation to remove graceful_mode parameter This is a high-quality implementation with exceptional test coverage. The only significant issue is the unused graceful_mode parameter. Action Items:
Merge Readiness:
Great work on the comprehensive test coverage and clean implementation! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/handler_bootstrap_source.py`:
- Around line 37-46: Replace uses of RuntimeError raised for rebuild failures
with ModelOnexError (or the project's OnexError subclass for model errors) and
include an appropriate error code and descriptive message; update imports to
bring ModelOnexError into scope and change every raise RuntimeError(...) in this
module (including the occurrences around the rebuild logic at the earlier and
later blocks referenced) to raise ModelOnexError(code="MODEL_REBUILD_FAILED",
message="...") or the project's canonical code/message pattern. Ensure the new
error preserves the original failure message and any chained exception info when
available (e.g., raise ModelOnexError(...) from err).
♻️ Duplicate comments (1)
src/omnibase_infra/runtime/handler_bootstrap_source.py (1)
157-193: Prefer canonical JsonType for bootstrap input_model.
Align the hardcoded input_model with the canonical JSON alias. As per coding guidelines, use JsonType from omnibase_core.types for JSON-compatible values.♻️ Suggested update
- "input_model": "omnibase_infra.models.types.JsonDict", + "input_model": "omnibase_core.types.JsonType", @@ - "input_model": "omnibase_infra.models.types.JsonDict", + "input_model": "omnibase_core.types.JsonType", @@ - "input_model": "omnibase_infra.models.types.JsonDict", + "input_model": "omnibase_core.types.JsonType", @@ - "input_model": "omnibase_infra.models.types.JsonDict", + "input_model": "omnibase_core.types.JsonType",
🧹 Nitpick comments (1)
tests/unit/runtime/test_handler_bootstrap_source.py (1)
1094-1132: Consider de‑flaking timing-based performance assertions.The
<100msand<10msthresholds can be noisy under CI contention. Consider marking these as@pytest.mark.slowor relaxing bounds / making them configurable to avoid intermittent failures.Also applies to: 1114-1132
| import logging | ||
| import threading | ||
| import time | ||
| from typing import TypedDict | ||
|
|
||
| from omnibase_infra.models.handlers import ( | ||
| LiteralHandlerKind, | ||
| ModelContractDiscoveryResult, | ||
| ModelHandlerDescriptor, | ||
| ) |
There was a problem hiding this comment.
Use ModelOnexError instead of RuntimeError for rebuild failures.
Project error-handling conventions require OnexError types with error codes. As per coding guidelines, use OnexError for error handling.
🔧 Proposed fix
-from omnibase_infra.models.handlers import (
+from omnibase_core.enums import EnumCoreErrorCode
+from omnibase_core.models.errors import ModelOnexError
+from omnibase_infra.models.handlers import (
@@
- except Exception as e:
- raise RuntimeError(
- f"Failed to rebuild ModelContractDiscoveryResult during bootstrap "
- f"initialization. This typically indicates a circular import or missing "
- f"type definition: {e}"
- ) from e
+ except Exception as e:
+ raise ModelOnexError(
+ message=(
+ "Failed to rebuild ModelContractDiscoveryResult during bootstrap "
+ "initialization. This typically indicates a circular import or missing "
+ f"type definition: {e}"
+ ),
+ error_code=EnumCoreErrorCode.INTERNAL_ERROR,
+ ) from eAlso applies to: 101-115
🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/handler_bootstrap_source.py` around lines 37 - 46,
Replace uses of RuntimeError raised for rebuild failures with ModelOnexError (or
the project's OnexError subclass for model errors) and include an appropriate
error code and descriptive message; update imports to bring ModelOnexError into
scope and change every raise RuntimeError(...) in this module (including the
occurrences around the rebuild logic at the earlier and later blocks referenced)
to raise ModelOnexError(code="MODEL_REBUILD_FAILED", message="...") or the
project's canonical code/message pattern. Ensure the new error preserves the
original failure message and any chained exception info when available (e.g.,
raise ModelOnexError(...) from err).
Address PR review recommendation to remove misleading graceful_mode parameter. **Why removed:** - Protocol (ProtocolContractSource) doesn't require constructor params - Bootstrap handlers are hardcoded and CANNOT fail validation - graceful_mode was semantically meaningless for this source - Per CLAUDE.md: "NO backwards compatibility is maintained" - Tests were verifying "both modes behave identically" (testing nothing) **Changes:** - Remove __init__ graceful_mode parameter from HandlerBootstrapSource - Remove self._graceful_mode from logging extra fields - Remove "API Consistency Note" from class docstring - Delete TestHandlerBootstrapSourceGracefulMode test class (6 tests) Test count: 50 → 44 (removed pointless tests)
PR Review: HandlerBootstrapSource Implementation (OMN-1087)This PR implements a centralized bootstrap handler source for core infrastructure handlers. Overall, this is high-quality code with excellent test coverage and clean architecture. Strengths
Critical Issue: Missing Runtime IntegrationThe HandlerBootstrapSource is implemented but there is no visible integration point in the runtime. Expected to see it used by HandlerPluginLoader or ContractHandlerDiscovery during bootstrap. Questions:
Recommendation: Either add integration in this PR OR document this as part 1 of multi-PR implementation. Minor Issues
Security Review
Final VerdictAPPROVE with clarification needed on runtime integration. This is high-quality code that follows ONEX patterns consistently. The only blocking concern is understanding how this integrates with the runtime. Once clarified, this is ready to merge. Great work on the comprehensive test coverage! 🎉 |
…N-1087] Integrate HandlerBootstrapSource into RuntimeHostProcess to ensure core infrastructure handlers (consul, db, http, vault) are always available during bootstrap, regardless of user contract discovery errors. Changes: - Add _register_bootstrap_handlers() to service_runtime_host_process.py that loads bootstrap handlers FIRST before contract discovery - Create ModelBootstrapHandlerDescriptor with required handler_class field - Add thread-safe model_rebuild pattern to handler_bootstrap_source.py - Update tests to verify graceful degradation behavior (handlers registered even when contracts fail) - Fix macOS symlink path resolution in test fixtures Test coverage: - 12 new integration tests for bootstrap source runtime integration - 24 new unit tests for ModelBootstrapHandlerDescriptor - Updated 9 handler discovery tests for graceful degradation - All 2339 tests passing
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Bootstrap handlers now provide core infrastructure (Consul, DB, HTTP, Vault), making the wire_handlers() fallback redundant. Changes: - Remove wire_handlers() fallback from _discover_or_wire_handlers() - Update docstring to reflect bootstrap-first architecture - Update test_start_wires_handlers → test_start_registers_bootstrap_handlers - Fix outdated comments referencing fallback behavior in tests
…ptor Remove unused _VersionField and LiteralHandlerKind imports that were inadvertently included during initial implementation. These type aliases are not used in the bootstrap descriptor module. [OMN-1087]
Replace manual field-by-field copy with Pydantic's model_dump() pattern. This is more concise and automatically captures any new fields added to the parent class in future versions.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/integration/runtime/test_bootstrap_source_integration.py`:
- Around line 229-263: The test mocks HandlerBootstrapSource to return empty
descriptors which can cause RuntimeHostProcess.start() to raise
ProtocolConfigurationError when the global registry has no handlers; modify the
test (test_bootstrap_source_called_during_start and the similar case at 358-391)
to ensure a non-empty registry before calling process.start() by seeding a stub
handler or creating/injecting a fresh RegistryProtocolBinding containing a dummy
handler (so that RuntimeHostProcess sees at least one registered handler) or by
passing a test-specific RegistryProtocolBinding into RuntimeHostProcess for
isolation; reference HandlerBootstrapSource, RuntimeHostProcess.start, and
RegistryProtocolBinding when making the change.
In `@tests/unit/runtime/test_runtime_host_process.py`:
- Around line 650-675: The test currently reads global state via
get_handler_registry(), which couples it to a singleton registry; instead
construct and inject a fresh RegistryProtocolBinding (or test double) into
RuntimeHostProcess before calling start(), call process.start(), then assert the
fresh registry instance has HANDLER_TYPE_CONSUL, HANDLER_TYPE_DATABASE,
HANDLER_TYPE_HTTP, and HANDLER_TYPE_VAULT registered (use the same constants)
and finally call process.stop(); update the test to avoid calling
get_handler_registry() and to pass the new registry into RuntimeHostProcess (or
its constructor/initializer) so assertions target that injected instance.
🧹 Nitpick comments (2)
src/omnibase_infra/runtime/service_runtime_host_process.py (1)
1134-1188: Skip already-registered bootstrap handlers to avoid noisy re-registration.When multiple RuntimeHostProcess instances share the singleton registry (or start/stop cycles happen), repeated registration can emit errors or overwrite existing entries. A quick guard keeps logs clean and avoids unnecessary imports.
♻️ Suggested guard
handler_id = descriptor.handler_id if handler_id.startswith("bootstrap."): protocol_type = handler_id[len("bootstrap.") :] else: # Fallback: use full handler_id as protocol type protocol_type = handler_id + + if handler_registry.is_registered(protocol_type): + logger.debug( + "Bootstrap handler already registered, skipping", + extra={ + "handler_id": handler_id, + "protocol_type": protocol_type, + "source_type": SOURCE_TYPE_BOOTSTRAP, + }, + ) + continuetests/integration/runtime/test_bootstrap_source_integration.py (1)
175-223: Ordering assertion doesn’t match the docstring intent.This test says it verifies bootstrap handlers are registered before contract-based handlers, but it only checks presence. Consider either asserting ordering (e.g., dummy contract registration index is after bootstrap) or renaming the test to reflect the actual assertion.
| async def test_bootstrap_source_called_during_start(self) -> None: | ||
| """HandlerBootstrapSource.discover_handlers() is called during start. | ||
|
|
||
| Verifies that RuntimeHostProcess actually calls the bootstrap source | ||
| discover_handlers() method during startup. | ||
| """ | ||
| event_bus = EventBusInmemory() | ||
|
|
||
| # Patch at the source module where it's imported from | ||
| with patch( | ||
| "omnibase_infra.runtime.handler_bootstrap_source.HandlerBootstrapSource" | ||
| ) as MockBootstrapSource: | ||
| # Create a mock that returns proper discovery result | ||
| mock_source = MagicMock() | ||
| mock_discovery_result = MagicMock() | ||
| mock_discovery_result.descriptors = [] # Empty for simplicity | ||
| mock_source.discover_handlers = AsyncMock( | ||
| return_value=mock_discovery_result | ||
| ) | ||
| MockBootstrapSource.return_value = mock_source | ||
|
|
||
| process = RuntimeHostProcess( | ||
| event_bus=event_bus, | ||
| input_topic="test.input", | ||
| ) | ||
|
|
||
| try: | ||
| await process.start() | ||
|
|
||
| # Verify HandlerBootstrapSource was instantiated and called | ||
| MockBootstrapSource.assert_called_once() | ||
| mock_source.discover_handlers.assert_called_once() | ||
|
|
||
| finally: | ||
| await process.stop() |
There was a problem hiding this comment.
Potential flakiness: start() can fail if registry is empty in these mocked cases.
When HandlerBootstrapSource is mocked to return empty descriptors, start() will raise ProtocolConfigurationError if no handlers are registered. These tests currently rely on singleton state from prior tests. Consider seeding a stub handler or injecting a fresh RegistryProtocolBinding with a dummy handler to keep them order-independent.
🔧 Example seeding approach
+ from omnibase_infra.runtime.handler_registry import RegistryProtocolBinding
+
+ class DummyHandler:
+ async def execute(self, envelope: dict[str, object]) -> dict[str, object]:
+ return {"status": "ok"}
+
+ async def shutdown(self) -> None:
+ return None
+
+ async def health_check(self) -> dict[str, object]:
+ return {"healthy": True}
+
+ registry = RegistryProtocolBinding()
+ registry.register("dummy", DummyHandler)
process = RuntimeHostProcess(
event_bus=event_bus,
input_topic="test.input",
+ handler_registry=registry,
)Also applies to: 358-391
🤖 Prompt for AI Agents
In `@tests/integration/runtime/test_bootstrap_source_integration.py` around lines
229 - 263, The test mocks HandlerBootstrapSource to return empty descriptors
which can cause RuntimeHostProcess.start() to raise ProtocolConfigurationError
when the global registry has no handlers; modify the test
(test_bootstrap_source_called_during_start and the similar case at 358-391) to
ensure a non-empty registry before calling process.start() by seeding a stub
handler or creating/injecting a fresh RegistryProtocolBinding containing a dummy
handler (so that RuntimeHostProcess sees at least one registered handler) or by
passing a test-specific RegistryProtocolBinding into RuntimeHostProcess for
isolation; reference HandlerBootstrapSource, RuntimeHostProcess.start, and
RegistryProtocolBinding when making the change.
| async def test_start_registers_bootstrap_handlers(self) -> None: | ||
| """Test that start() registers bootstrap handlers. | ||
|
|
||
| The RuntimeHostProcess should use the wiring module to register | ||
| all configured handlers when started. | ||
| The RuntimeHostProcess should register bootstrap handlers (consul, db, | ||
| http, vault) via HandlerBootstrapSource when started. | ||
| """ | ||
| from omnibase_infra.runtime.handler_registry import ( | ||
| HANDLER_TYPE_CONSUL, | ||
| HANDLER_TYPE_DATABASE, | ||
| HANDLER_TYPE_HTTP, | ||
| HANDLER_TYPE_VAULT, | ||
| get_handler_registry, | ||
| ) | ||
|
|
||
| process = RuntimeHostProcess() | ||
| await process.start() | ||
|
|
||
| with patch( | ||
| "omnibase_infra.runtime.service_runtime_host_process.wire_handlers" | ||
| ) as mock_wire: | ||
| mock_wire.return_value = {} | ||
| await process.start() | ||
|
|
||
| try: | ||
| mock_wire.assert_called_once() | ||
| finally: | ||
| await process.stop() | ||
| try: | ||
| # Verify bootstrap handlers are registered | ||
| registry = get_handler_registry() | ||
| assert registry.is_registered(HANDLER_TYPE_CONSUL) | ||
| assert registry.is_registered(HANDLER_TYPE_DATABASE) | ||
| assert registry.is_registered(HANDLER_TYPE_HTTP) | ||
| assert registry.is_registered(HANDLER_TYPE_VAULT) | ||
| finally: | ||
| await process.stop() |
There was a problem hiding this comment.
Avoid singleton registry coupling in this bootstrap test.
This test relies on get_handler_registry() (global state), which can leak across tests and make results order-dependent. Prefer injecting a fresh RegistryProtocolBinding into RuntimeHostProcess and asserting against that instance.
🔧 Example isolation tweak
from omnibase_infra.runtime.handler_registry import (
HANDLER_TYPE_CONSUL,
HANDLER_TYPE_DATABASE,
HANDLER_TYPE_HTTP,
HANDLER_TYPE_VAULT,
+ RegistryProtocolBinding,
get_handler_registry,
)
- process = RuntimeHostProcess()
+ registry = RegistryProtocolBinding()
+ process = RuntimeHostProcess(handler_registry=registry)
await process.start()
try:
# Verify bootstrap handlers are registered
- registry = get_handler_registry()
assert registry.is_registered(HANDLER_TYPE_CONSUL)
assert registry.is_registered(HANDLER_TYPE_DATABASE)
assert registry.is_registered(HANDLER_TYPE_HTTP)
assert registry.is_registered(HANDLER_TYPE_VAULT)🤖 Prompt for AI Agents
In `@tests/unit/runtime/test_runtime_host_process.py` around lines 650 - 675, The
test currently reads global state via get_handler_registry(), which couples it
to a singleton registry; instead construct and inject a fresh
RegistryProtocolBinding (or test double) into RuntimeHostProcess before calling
start(), call process.start(), then assert the fresh registry instance has
HANDLER_TYPE_CONSUL, HANDLER_TYPE_DATABASE, HANDLER_TYPE_HTTP, and
HANDLER_TYPE_VAULT registered (use the same constants) and finally call
process.stop(); update the test to avoid calling get_handler_registry() and to
pass the new registry into RuntimeHostProcess (or its constructor/initializer)
so assertions target that injected instance.
- Add @Final decorator to HandlerBootstrapSource to prevent subclassing - Replace float("inf") with 1_000_000.0 cap in handlers_per_sec calculation to avoid issues with downstream logging/monitoring systems - Add comprehensive docstring to to_base_descriptor() explaining why model_dump() without exclude_unset is safe (field parity with parent)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/handler_bootstrap_source.py`:
- Around line 158-165: Replace the RuntimeError raised in the
ModelContractDiscoveryResult.model_rebuild() exception handler with the
project's OnexError: catch Exception as e around
ModelContractDiscoveryResult.model_rebuild(), then raise OnexError(...) from e
using the same message text; also ensure OnexError is imported into the module
so the new exception class is available for use.
🧹 Nitpick comments (1)
src/omnibase_infra/models/handlers/model_bootstrap_handler_descriptor.py (1)
96-100: Consider removing redundantmodel_configredefinition.The parent
ModelHandlerDescriptoralready defines identicalmodel_configsettings. In Pydantic v2, child classes inherit the parent's model configuration automatically. This redefinition is not harmful but adds maintenance overhead if the parent config changes.♻️ Optional removal
class ModelBootstrapHandlerDescriptor(ModelHandlerDescriptor): # ... docstring ... - model_config = ConfigDict( - frozen=True, - extra="forbid", - strict=True, - ) - # Override handler_class to be required (no default, not optional)
| try: | ||
| ModelContractDiscoveryResult.model_rebuild() | ||
| except Exception as e: | ||
| raise RuntimeError( | ||
| f"Failed to rebuild ModelContractDiscoveryResult during bootstrap " | ||
| f"initialization. This typically indicates a circular import or missing " | ||
| f"type definition: {e}" | ||
| ) from e |
There was a problem hiding this comment.
Use OnexError instead of RuntimeError for error handling.
Per coding guidelines: "Use raise OnexError(...) from e for error handling - never raise other exception types." The current RuntimeError should be replaced with OnexError from the project's error handling framework.
🔧 Proposed fix
+from omnibase_core.errors import OnexError
+
# ... in _ensure_model_rebuilt() ...
try:
ModelContractDiscoveryResult.model_rebuild()
except Exception as e:
- raise RuntimeError(
- f"Failed to rebuild ModelContractDiscoveryResult during bootstrap "
- f"initialization. This typically indicates a circular import or missing "
- f"type definition: {e}"
+ raise OnexError(
+ message=(
+ "Failed to rebuild ModelContractDiscoveryResult during bootstrap "
+ "initialization. This typically indicates a circular import or missing "
+ f"type definition: {e}"
+ ),
) from e🤖 Prompt for AI Agents
In `@src/omnibase_infra/runtime/handler_bootstrap_source.py` around lines 158 -
165, Replace the RuntimeError raised in the
ModelContractDiscoveryResult.model_rebuild() exception handler with the
project's OnexError: catch Exception as e around
ModelContractDiscoveryResult.model_rebuild(), then raise OnexError(...) from e
using the same message text; also ensure OnexError is imported into the module
so the new exception class is available for use.
Add rationale explaining there are no external users yet, making this the optimal time for aggressive breaking changes. Strengthen language to make deletion of deprecated code mandatory, not optional.
- Replace nested ternary with if/elif/else in handlers_per_sec calculation - Use str.removeprefix() for cleaner protocol type extraction
1 participant
Summary
Implement
HandlerBootstrapSourceclass that centralizes all hardcoded handler registration per OMN-1087.Key Changes:
HandlerBootstrapSourceimplementingProtocolContractSourceprotocolModelHandlerDescriptorinstancesHandlerBootstrapSourceclass nameHandlers Registered
bootstrap.consulbootstrap.dbbootstrap.httpbootstrap.vaultTest Plan
pytest tests/unit/runtime/test_handler_bootstrap_source.py)Files Changed
src/omnibase_infra/runtime/handler_bootstrap_source.py- New implementationsrc/omnibase_infra/runtime/__init__.py- Added exportssrc/omnibase_infra/validation/validation_exemptions.yaml- Added exemptiontests/unit/runtime/test_handler_bootstrap_source.py- Comprehensive testsLinear Ticket
OMN-1087
Summary by CodeRabbit
New Features
Tests
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.