Repository navigation
reapply: ProtocolHttpClient and health check refactor [OMN-566] - #203
Conversation
WalkthroughThis PR adds an HTTP client protocol abstraction, refactors the HTTP health-check to accept injected clients and validate URLs, restructures health-related TypedDicts and conversion logic, updates the transport-import allowlist handling, and adds comprehensive unit and integration tests for the new HTTP path and allowlist reporting. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant HealthCheck as check_http_service_health()
participant URLParser as urlparse()
participant HTTPClient as ProtocolHttpClient
participant HTTPResp as ProtocolHttpResponse
Caller->>HealthCheck: call(service_url, http_client, headers, expected_status)
alt no http_client
HealthCheck-->>Caller: return unhealthy (missing ProtocolHttpClient issue)
else http_client provided
HealthCheck->>URLParser: parse(service_url)
alt invalid URL / non-HTTP scheme
HealthCheck-->>Caller: return unhealthy (configuration issue)
else valid HTTP(S) URL
HealthCheck->>HTTPClient: get(url[, timeout, headers])
HTTPClient->>HTTPResp: provide response
HTTPResp-->>HealthCheck: status, text/json()
alt status == expected
HealthCheck-->>Caller: return healthy (score, duration if tracked)
else status != expected
HealthCheck-->>Caller: return degraded/unhealthy with issue (include URL and status)
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
PR Review: ProtocolHttpClient and Health Check Refactor [OMN-566]SummaryThis PR successfully reapplies the ProtocolHttpClient protocol implementation from PR #159 while removing the fabricated documentation claims that triggered the original revert. The technical implementation is solid and follows ONEX architectural patterns correctly. ✅ Code Quality & Best PracticesExcellent Protocol DesignThe
Particularly Strong: The extensive documentation in Clean Mixin RefactoringThe
Good catch: URL validation at Test CoverageThe PR adds 52 comprehensive tests (not 15,624 as PR #159 claimed):
Note: The mock classes 🔍 Potential Issues & Recommendations1.
|
![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
🧹 Nitpick comments (3)
src/omnibase_core/protocols/http/protocol_http_client.py (1)
1-162: Consider moving detailed migration guide to separate documentation.The 162-line module docstring is exceptionally thorough but makes the file harder to navigate. Consider:
- Keeping a concise summary in the module docstring
- Moving the detailed migration guide, lifecycle patterns, and examples to a separate documentation file (e.g.,
docs/protocols/http_protocol.md)- Adding a reference link in the docstring
This would improve maintainability while preserving the valuable documentation.
src/omnibase_core/protocols/http/__init__.py (1)
1-166: Consider moving extensive examples to separate documentation.Similar to
protocol_http_client.py, this 166-line docstring with complete DI container and testing examples would be better suited to separate documentation files. Consider:
- Keeping a brief module summary
- Moving detailed examples to
docs/protocols/http_integration.md- Adding a reference link
This pattern improves file navigability while preserving valuable documentation.
src/omnibase_core/mixins/mixin_health_check.py (1)
897-905: Consider trailing slash edge case in/healthdetection.The
endswith("/health")check doesn't account for URLs ending with/health/(trailing slash), which would incorrectly append another/health. This is a minor edge case but worth noting.health_url = ( - service_url if service_url.endswith("/health") else f"{service_url}/health" + service_url if service_url.rstrip("/").endswith("/health") else f"{service_url.rstrip('/')}/health" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
scripts/check_transport_imports.py(2 hunks)src/omnibase_core/mixins/mixin_health_check.py(4 hunks)src/omnibase_core/protocols/__init__.py(3 hunks)src/omnibase_core/protocols/http/__init__.py(1 hunks)src/omnibase_core/protocols/http/protocol_http_client.py(1 hunks)src/omnibase_core/types/__init__.py(2 hunks)src/omnibase_core/types/converter_health_to_typed_dict.py(1 hunks)src/omnibase_core/types/typed_dict_converted_health.py(1 hunks)src/omnibase_core/types/typed_dict_health_status.py(1 hunks)tests/integration/mixins/__init__.py(1 hunks)tests/integration/mixins/test_mixin_health_check_integration.py(1 hunks)tests/integration/scripts/test_check_transport_imports_integration.py(4 hunks)tests/unit/mixins/test_mixin_health_check.py(2 hunks)tests/unit/test_thread_safety.py(0 hunks)tests/unit/validation/test_fsm_analysis.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/unit/test_thread_safety.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly. Usepoetry runfor all commands (e.g.,poetry run pytest,poetry run mypy,poetry run black)
Always callsuper().__init__(container)in node class constructors where container is of typeModelONEXContainer- this is MANDATORY and handles all boilerplate initialization
UseModelONEXContainer(notModelContainer[T]) in node constructors for dependency injection.ModelContainer[T]is only for wrapping single values with metadata, never for DI in node__init__
Always raiseModelOnexErrorwith anEnumCoreErrorCode, never use genericException. Use the@standard_error_handlingdecorator to eliminate boilerplate try/catch blocks
Use protocol-based dependency injection withcontainer.get_service("ProtocolName")- never get services by concrete class names, and never use isinstance checks with protocols (use duck typing instead)
UseEnumNodeKindfor architectural classification in the ONEX workflow (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) andEnumNodeTypefor specific implementation type discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Do NOT poll background jobs repeatedly (AI Agent Anti-Pattern) - instead call operations once with longer timeout (e.g.,wait_up_to=300) or continue with other work. Pre-commit/pre-push can take 2-5 minutes on this codebase (1865+ files with mypy strict)
Files:
src/omnibase_core/types/typed_dict_converted_health.pytests/unit/mixins/test_mixin_health_check.pytests/integration/mixins/test_mixin_health_check_integration.pysrc/omnibase_core/protocols/http/protocol_http_client.pysrc/omnibase_core/types/__init__.pytests/integration/mixins/__init__.pysrc/omnibase_core/mixins/mixin_health_check.pysrc/omnibase_core/protocols/__init__.pytests/unit/validation/test_fsm_analysis.pyscripts/check_transport_imports.pysrc/omnibase_core/protocols/http/__init__.pysrc/omnibase_core/types/converter_health_to_typed_dict.pysrc/omnibase_core/types/typed_dict_health_status.pytests/integration/scripts/test_check_transport_imports_integration.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Achieve 100% mypy strict mode compliance - all functions must have type annotations, warn on functions returning Any, and pass pyright basic mode checks
Format code with Black and sort imports with isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before committing
Use Pydantic 2.11+ models with proper validation, field descriptions, and examples - ensure models are immutable (frozen=True) where appropriate
Never use generic exceptions - always useModelOnexErrorwith appropriateEnumCoreErrorCodevalues for structured error handling with Pydantic models and context
Use base node classes (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator) from omnibase_core.nodes - they provide unidirectional left-to-right data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR) with zero boilerplate initialization
Files:
src/omnibase_core/types/typed_dict_converted_health.pysrc/omnibase_core/protocols/http/protocol_http_client.pysrc/omnibase_core/types/__init__.pysrc/omnibase_core/mixins/mixin_health_check.pysrc/omnibase_core/protocols/__init__.pysrc/omnibase_core/protocols/http/__init__.pysrc/omnibase_core/types/converter_health_to_typed_dict.pysrc/omnibase_core/types/typed_dict_health_status.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Do not share node instances across threads without explicit synchronization - each thread must use its own instance or thread-local storage. Nodes are NOT thread-safe by default
Run tests withpoetry run pytest tests/using parallelization (-n4 by default) with 60s timeout per test - disable parallelism only for debugging with-n 0 -xvs
Maintain minimum 60% code coverage across the test suite (12,000+ tests collected) - run withpoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
Files:
tests/unit/mixins/test_mixin_health_check.pytests/integration/mixins/test_mixin_health_check_integration.pytests/integration/mixins/__init__.pytests/unit/validation/test_fsm_analysis.pytests/integration/scripts/test_check_transport_imports_integration.py
🧠 Learnings (1)
📚 Learning: 2025-12-15T04:56:21.046Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-15T04:56:21.046Z
Learning: Applies to src/omnibase_core/**/*.py : Never use generic exceptions - always use `ModelOnexError` with appropriate `EnumCoreErrorCode` values for structured error handling with Pydantic models and context
Applied to files:
tests/unit/validation/test_fsm_analysis.py
🧬 Code graph analysis (8)
tests/unit/mixins/test_mixin_health_check.py (2)
src/omnibase_core/protocols/http/protocol_http_client.py (4)
status(187-194)text(196-206)json(208-218)get(292-314)src/omnibase_core/mixins/mixin_health_check.py (1)
check_http_service_health(792-948)
tests/integration/mixins/test_mixin_health_check_integration.py (3)
src/omnibase_core/protocols/http/protocol_http_client.py (6)
status(187-194)text(196-206)json(208-218)get(292-314)ProtocolHttpResponse(170-218)ProtocolHttpClient(222-314)src/omnibase_core/mixins/mixin_health_check.py (1)
check_http_service_health(792-948)scripts/validation/timeout_utils.py (1)
TimeoutError(44-45)
src/omnibase_core/protocols/http/protocol_http_client.py (3)
tests/integration/mixins/test_mixin_health_check_integration.py (4)
status(60-62)text(64-66)json(68-70)get(111-135)tests/unit/mixins/test_mixin_health_check.py (4)
status(459-461)text(463-465)json(467-469)get(503-517)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
headers(38-40)
src/omnibase_core/types/__init__.py (1)
src/omnibase_core/types/typed_dict_converted_health.py (1)
TypedDictConvertedHealth(13-35)
src/omnibase_core/protocols/__init__.py (1)
src/omnibase_core/protocols/http/protocol_http_client.py (2)
ProtocolHttpClient(222-314)ProtocolHttpResponse(170-218)
tests/unit/validation/test_fsm_analysis.py (1)
src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
src/omnibase_core/protocols/http/__init__.py (1)
src/omnibase_core/protocols/http/protocol_http_client.py (2)
ProtocolHttpClient(222-314)ProtocolHttpResponse(170-218)
src/omnibase_core/types/converter_health_to_typed_dict.py (2)
src/omnibase_core/types/typed_dict_converted_health.py (1)
TypedDictConvertedHealth(13-35)src/omnibase_core/types/typed_dict_legacy_health.py (1)
TypedDictLegacyHealth(11-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: claude-review
🔇 Additional comments (28)
tests/integration/scripts/test_check_transport_imports_integration.py (2)
130-149: LGTM! Robust validation of allowlisted files count.The test correctly validates the
allowlisted_filesfield structure (presence, type, non-negative value) and aligns with the updated allowlist behavior where the allowlist is now empty.
189-217: LGTM! Comprehensive validation of allowlisted results structure.The test correctly validates that allowlisted files in results match the summary count and have the proper structure (skip_reason contains "Allowlisted", is_clean is True, violations suppressed). The test name with "_when_present" suffix accurately conveys the conditional validation.
scripts/check_transport_imports.py (2)
163-176: LGTM! Clean allowlist implementation with conditional expiration tracking.The empty allowlist with conditional expiration date is well-implemented. The
None if not TEMPORARY_ALLOWLIST else date(2026, 6, 10)logic correctly disables expiration tracking when the allowlist is empty, and the placeholder date provides a 6-month baseline for future additions.
577-583: LGTM! Correct early-return for empty allowlist.The early-return logic when
ALLOWLIST_EXPIRATION_DATE is Nonecorrectly skips expiration checking for empty allowlists, avoiding unnecessary date calculations.tests/integration/mixins/__init__.py (1)
1-1: LGTM!Clean module docstring following standard Python package conventions.
src/omnibase_core/protocols/http/protocol_http_client.py (2)
169-218: LGTM!Clean protocol definition with appropriate use of
@runtime_checkableand minimal interface. TheAnyreturn type forjson()is correct since JSON can represent various types.
221-317: LGTM!Well-designed protocol with optional parameters and clear return type. The lifecycle management guidance in the docstring is valuable for implementers.
tests/unit/validation/test_fsm_analysis.py (1)
29-29: LGTM!Moving the
ModelOnexErrorimport to module level eliminates redundant inner imports (previously at lines 833, 1052, 1720) and follows Python best practices.src/omnibase_core/protocols/__init__.py (2)
120-124: LGTM!Clean addition of HTTP protocol exports following the existing module pattern.
270-274: LGTM!HTTP protocols properly added to
__all__exports.src/omnibase_core/types/typed_dict_converted_health.py (1)
1-38: LGTM!Well-structured TypedDict with clear field types and comprehensive documentation. The separation from
TypedDictHealthStatusis a good design decision for type precision.src/omnibase_core/types/__init__.py (1)
132-132: LGTM!New TypedDict properly imported and exported, following the existing module pattern.
Also applies to: 448-448
src/omnibase_core/types/converter_health_to_typed_dict.py (1)
8-25: LGTM!Return type updated to
TypedDictConvertedHealthand all fields properly converted fromTypedDictLegacyHealth. The conversion logic correctly handles optional/None values with theor 0pattern.src/omnibase_core/protocols/http/__init__.py (1)
168-176: LGTM!Clean re-export pattern for HTTP protocols following standard Python package conventions.
tests/unit/mixins/test_mixin_health_check.py (4)
452-469: LGTM!The
MockHttpResponseclass correctly implements theProtocolHttpResponseprotocol with the requiredstatusproperty and asynctext()/json()methods.
472-517: LGTM!Well-designed mock HTTP client with:
- Factory methods for common test scenarios
- Call tracking for verification in assertions
- Proper exception propagation
525-723: Comprehensive test coverage forcheck_http_service_health.Tests properly cover:
- Missing HTTP client returns unhealthy
- Successful responses, unexpected status codes
- Timeout and connection error handling
- URL normalization (
/healthappending)- Parameter propagation (timeout, headers, expected_status)
730-806: Good URL validation test coverage.Tests properly verify that invalid URLs (missing scheme, missing host, non-HTTP schemes, empty strings) result in unhealthy status with appropriate error messages.
src/omnibase_core/types/typed_dict_health_status.py (1)
10-28: TypedDict restructured to align withModelHealthStatus.The new field structure (
node_id,is_healthy,status,health_score,issues) matches the health status model used throughout the codebase. The docstring clearly documents the attributes and their types.src/omnibase_core/mixins/mixin_health_check.py (5)
33-34: LGTM!Appropriate imports for URL validation (
urlparse), unique issue IDs (uuid4), and the new HTTP client protocol abstraction (ProtocolHttpClient).Also applies to: 40-40
796-829: Well-documented dependency injection pattern.The updated signature maintains backward compatibility via default parameters. The docstring clearly explains that
http_clientis required for actual HTTP calls and provides a proper example usingcontainer.get_service("ProtocolHttpClient")per the coding guidelines.
833-847: Good fail-fast behavior for missing HTTP client.Returns an unhealthy status with a clear, actionable message directing users to inject a
ProtocolHttpClientimplementation. This prevents runtime errors from attempting HTTP calls without a client.
849-895: Thorough URL validation before HTTP request.Validates:
- Scheme and host presence
- HTTP/HTTPS scheme requirement
- Graceful exception handling for malformed URLs
Each validation failure returns an unhealthy status with a specific, actionable error message.
940-948: Improved error message format.The updated error message includes the service URL, exception type name, and exception message, providing comprehensive context for debugging health check failures.
tests/integration/mixins/test_mixin_health_check_integration.py (4)
20-70: Well-documented test adapter with production example.The docstring includes a production implementation example, which is helpful for developers implementing actual adapters. The adapter correctly implements all
ProtocolHttpResponsemethods.
73-135: Flexible multi-URL client adapter for integration testing.Supports:
- URL-specific response configuration
- Configurable default exception behavior
- Clear error message for unconfigured URLs
The production example in the docstring provides a template for actual implementations.
146-167: Protocol compliance verification in tests.Using
isinstancechecks withruntime_checkableprotocols is appropriate here since these tests specifically verify that the adapter implementations conform to the protocol contracts. This is different from production code where duck typing should be preferred.
185-334: Comprehensive integration test coverage.Tests verify end-to-end behavior through the protocol abstraction including:
- Healthy and degraded responses
- Custom expected status codes
- Timeout and connection error handling
- URL
/healthsuffix handling- Multi-URL client configurations
| # ============================================================================= | ||
| # Compute Module Exports | ||
| # ============================================================================= |
There was a problem hiding this comment.
Remove duplicate section header.
The "Compute Module Exports" comment appears twice (lines 96-98 and earlier). This appears to be a copy-paste error that should be removed or corrected.
Apply this diff:
-# =============================================================================
-# Compute Module Exports
-# =============================================================================📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ============================================================================= | |
| # Compute Module Exports | |
| # ============================================================================= |
🤖 Prompt for AI Agents
In src/omnibase_core/protocols/__init__.py around lines 96-98, there is a
duplicated comment header "Compute Module Exports" that was likely copied twice;
remove the redundant duplicate header (the second occurrence) so only a single,
correctly placed "Compute Module Exports" section header remains and ensure
surrounding imports/exports stay intact and correctly grouped.
Implements three improvements from PR review: 1. **CRITICAL FIX**: Narrow exception handling in URL validation - Replace broad `except Exception` with specific `ValueError, AttributeError` - Only catches urlparse-specific errors, allows programming errors to propagate - Improves debuggability and code maintainability - File: src/omnibase_core/mixins/mixin_health_check.py:895-900 2. **ENHANCEMENT**: Add optional response timing to health checks - New parameter: `track_duration: bool = False` (backward compatible) - Captures response time in milliseconds when enabled - Stores duration in `ModelHealthStatus.check_duration_ms` field - Enables observability for service degradation detection - File: src/omnibase_core/mixins/mixin_health_check.py:798-948 3. **DOCUMENTATION**: Document HTTP protocol extensibility limitation - Added TODO comment explaining current GET-only design (YAGNI principle) - Lists future HTTP methods: POST, HEAD, PUT for advanced use cases - Guides future protocol extensions - File: src/omnibase_core/protocols/http/protocol_http_client.py:292-297 **Quality Gates**: ✅ mypy strict mode passes (0 errors in 2198 files) **Tests**: ✅ All 41 existing health check tests pass **Backward Compatibility**: ✅ All changes maintain existing behavior Co-authored-by: Claude Code (Polymorphic Agent)
PR Review - ProtocolHttpClient and Health Check RefactorOverviewThis PR successfully reapplies the ProtocolHttpClient protocol and health check refactoring (originally PR #159) while correctly removing the fabricated documentation claims that caused the revert. The technical implementation is solid and aligns well with the ONEX architecture patterns. ✅ Strengths1. Excellent Protocol Design
2. Robust Error Handling
3. Strong Test Coverage
4. Dependency Injection Done Right
5. Backward Compatibility
🔍 Areas for Improvement1. Missing HTTP Methods (Minor - Future Enhancement)The Recommendation: Either:
2. URL Validation Edge CaseIn if parsed.scheme not in ("http", "https"):
return ModelHealthStatus.create_unhealthy(...)Issue: This check happens AFTER the Minor optimization: if not parsed.scheme or parsed.scheme not in ("http", "https"):
# Single validation covering both cases3. Duration Tracking Logic (Code Quality)The start_time = datetime.now(UTC) if track_duration else None
# ... later ...
duration_ms = None
if track_duration and start_time is not None:
duration_ms = int((datetime.now(UTC) - start_time).total_seconds() * 1000)Issue: The Suggestion: Simplify to: if track_duration:
duration_ms = int((datetime.now(UTC) - start_time).total_seconds() * 1000)Since 4. Test File Organization (Minor)The integration test file at Recommendation: Consider extracting these adapters to a shared test fixtures module if they'll be reused across test files. This follows DRY principles and makes adapters easier to maintain. 5. TypedDict Changes Not ExplainedThe PR description mentions changes to Question: Are these TypedDict changes:
Adding a sentence in the PR description would clarify the intent. 🔒 Security Considerations✅ No security concerns identified
🎯 Performance Considerations✅ Good performance characteristics
Minor: The 📊 Test CoverageExcellent coverage:
Potential gap: No test verifies that async def test_check_health_duration_tracking():
response = AioHttpResponseAdapter(status=200)
client = AioHttpClientAdapter(responses={...})
result = await check_http_service_health(
"http://test.com",
http_client=client,
track_duration=True,
)
assert result.check_duration_ms is not None
assert result.check_duration_ms >= 0📝 Documentation QualityExcellent documentation:
This is a model for how protocols should be documented in the codebase. ✅ Acceptance Criteria VerificationAll acceptance criteria met:
🎯 Final VerdictLGTM with minor suggestions ✅ This is high-quality work that demonstrates:
The minor suggestions above are non-blocking and could be addressed in follow-up PRs if desired. Recommendation: Approve and merge Reviewed by: Claude Code (Sonnet 4.5) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/omnibase_core/mixins/mixin_health_check.py (2)
848-864: Config error for missing HTTP client is clearly surfaced.Treating a missing
http_clientas anUNHEALTHYconfiguration issue with a critical severity and UUID-backedModelHealthIssueis a good failure mode and aligns with the migration note that callers must injectProtocolHttpClient. You might consider factoring the repeateddatetime.now(UTC)into a local variable for tiny readability/perf gains, but this is non-blocking.
915-952: HTTP call + duration handling work, but health URL building could be more robust.The use of
http_client.get(..., timeout=timeout_seconds, headers=headers)matches theProtocolHttpClientinterface, and the conditionalcheck_duration_msupdate viamodel_copyis correct.One improvement to consider:
health_urlis built with a simpleendswith("/health")string check onservice_url. For full URLs that include a query string or non‑/healthpaths (e.g.https://svc/health?env=prodorhttps://svc/custom-health), this can produce slightly surprising URLs like...?env=prod/health. If you want this helper to be more forgiving for “full endpoint” URLs, consider deriving the decision fromurlparse(service_url).pathand/or usingurllib.parse.urljointo append/healthin a path-aware way.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/omnibase_core/mixins/mixin_health_check.py(4 hunks)src/omnibase_core/protocols/http/protocol_http_client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_core/protocols/http/protocol_http_client.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly. Usepoetry runfor all commands (e.g.,poetry run pytest,poetry run mypy,poetry run black)
Always callsuper().__init__(container)in node class constructors where container is of typeModelONEXContainer- this is MANDATORY and handles all boilerplate initialization
UseModelONEXContainer(notModelContainer[T]) in node constructors for dependency injection.ModelContainer[T]is only for wrapping single values with metadata, never for DI in node__init__
Always raiseModelOnexErrorwith anEnumCoreErrorCode, never use genericException. Use the@standard_error_handlingdecorator to eliminate boilerplate try/catch blocks
Use protocol-based dependency injection withcontainer.get_service("ProtocolName")- never get services by concrete class names, and never use isinstance checks with protocols (use duck typing instead)
UseEnumNodeKindfor architectural classification in the ONEX workflow (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) andEnumNodeTypefor specific implementation type discovery (COMPUTE_GENERIC, TRANSFORMER, AGGREGATOR, etc.)
Do NOT poll background jobs repeatedly (AI Agent Anti-Pattern) - instead call operations once with longer timeout (e.g.,wait_up_to=300) or continue with other work. Pre-commit/pre-push can take 2-5 minutes on this codebase (1865+ files with mypy strict)
Files:
src/omnibase_core/mixins/mixin_health_check.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Achieve 100% mypy strict mode compliance - all functions must have type annotations, warn on functions returning Any, and pass pyright basic mode checks
Format code with Black and sort imports with isort - runpoetry run black src/ tests/andpoetry run isort src/ tests/before committing
Use Pydantic 2.11+ models with proper validation, field descriptions, and examples - ensure models are immutable (frozen=True) where appropriate
Never use generic exceptions - always useModelOnexErrorwith appropriateEnumCoreErrorCodevalues for structured error handling with Pydantic models and context
Use base node classes (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator) from omnibase_core.nodes - they provide unidirectional left-to-right data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR) with zero boilerplate initialization
Files:
src/omnibase_core/mixins/mixin_health_check.py
🧬 Code graph analysis (1)
src/omnibase_core/mixins/mixin_health_check.py (2)
src/omnibase_core/models/health/model_health_status.py (2)
ModelHealthStatus(17-234)create_unhealthy(223-234)src/omnibase_core/protocols/http/protocol_http_client.py (3)
ProtocolHttpClient(222-320)get(298-320)status(187-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: claude-review
🔇 Additional comments (4)
src/omnibase_core/mixins/mixin_health_check.py (4)
29-41: Imports now correctly depend on the HTTP protocol abstraction.Using
urlparse,uuid4, andProtocolHttpClienthere matches the PR goal of removing directaiohttpusage and wiring HTTP via the protocol surface. No issues spotted with import order or types.
792-847: Signature and docs line up with DI + duration tracking design.The extended signature (
http_client,headers,track_duration) and docstring/example correctly reflect protocol-based DI (container.get_service("ProtocolHttpClient")) and optional latency tracking, consistent with the HTTP client protocol snippet and coding guidelines.
866-913: URL validation is strict and safe; behavior is reasonable.The
urlparsecheck for scheme/netloc plus explicit scheme allowlist (http/https) cleanly rejects malformed or non-HTTP URLs and converts them into configuration issues instead of raising. This matches the “never raise, always return status” contract and looks sound.
965-982: Enhanced error logging/message for unexpected exceptions looks good.Including both
service_urlandtype(e).__name__in the log metadata and in the finalModelHealthIssuemessage makes debugging HTTP health failures easier without changing the contract (still returns an unhealthy status instead of raising).
1 participant
Summary
Reapplies PR #159 (OMN-566) by reverting the revert commit 63799c6. This PR restores the ProtocolHttpClient protocol and health check refactoring while removing the fabricated documentation claims that triggered the original revert.
What Was Reverted (PR #164)
PR #159 was reverted 4 minutes after merge because it contained:
What This PR Does
✅ Technical Changes (KEPT)
ProtocolHttpClientandProtocolHttpResponseprotocolsmixin_health_check.pyto use dependency injectionaiohttpimport (transport ban compliance)mixin_health_check.pyfrom transport import allowlist❌ Documentation Claims (REMOVED)
Acceptance Criteria (OMN-566)
aiohttpimport inmixin_health_check.pyProtocolHttpClientprotocolpoetry run python scripts/check_transport_imports.pypasses (0 violations)Validation Results
References
Migration Notes
Consumers of
get_health_status()should continue to work as-is. The return type remains compatible (TypedDict structure unchanged).The new
check_http_service_health()function requires an HTTP client implementingProtocolHttpClient. See tests for usage examples.Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.