[jonahgabriel]

Reverts #159

Summary by CodeRabbit

• Refactor

  • Simplified health check API: method now returns a standardized dictionary and no longer requires HTTP client parameters.
  • Updated health status structure with new fields: status, uptime metrics, error counts, and check statistics.
  • Removed HTTP protocol abstractions from public API.

• Chores

  • Optimized CI test parallelization from 24 to 20 splits.
  • Updated test configuration and benchmark documentation.

• Tests

  • Removed obsolete integration and unit test suites.
  • Renamed test methods for clarity.

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

[linear]

Issue reopened: OMN-566 Refactor mixin_health_check.py to use ProtocolHttpClient instead of aiohttp

[coderabbitai]

Caution

Review failed

The pull request is closed.

Walkthrough

This PR reduces CI test parallelization from 24 to 20 splits, refactors the health-check mixin to inline aiohttp instead of using protocol abstractions, removes HTTP protocol exports from the public API, restructures health-status TypedDicts, and removes related integration and unit tests while updating others.

Changes

Cohort / File(s)	Summary
CI and Documentation .github/workflows/test.yml, CLAUDE.md	Updated test parallelization from 24 to 20 splits across workflow configuration and test matrix; revised documentation metrics (15,624 → 12,198 tests) and benchmark timings to reflect 20-split baseline.
Health Check Mixin Refactoring src/omnibase_core/mixins/mixin_health_check.py	Refactored health-check logic to inline aiohttp for HTTP requests, removing dependency on external HTTP client protocol abstractions. Changed get_health_status() return type from TypedDictHealthStatus to dict[str, Any] and removed http_client/headers parameters from check_http_service_health(). Simplified error messaging and updated health status computation.
HTTP Protocol Removal src/omnibase_core/protocols/__init__.py, src/omnibase_core/protocols/http/__init__.py, src/omnibase_core/protocols/http/protocol_http_client.py	Removed public exports and definitions of ProtocolHttpClient and ProtocolHttpResponse from public API surface.
Type Structure Refactoring src/omnibase_core/types/__init__.py, src/omnibase_core/types/typed_dict_converted_health.py, src/omnibase_core/types/typed_dict_health_status.py, src/omnibase_core/types/converter_health_to_typed_dict.py	Replaced TypedDictConvertedHealth with restructured TypedDictHealthStatus (old fields: node_id, is_healthy, health_score, issues → new fields: status, uptime_seconds, last_check, error_count, warning_count, checks_passed, checks_total). Updated converter function to return new type structure.
Script Updates scripts/check_transport_imports.py	Changed ALLOWLIST_EXPIRATION_DATE from optional (date | None) to required (date(2026, 6, 10)), removing conditional expiration logic.
Test Deletion and Updates tests/integration/mixins/test_mixin_health_check_integration.py (deleted), tests/integration/mixins/__init__.py, tests/unit/mixins/test_mixin_health_check.py, tests/integration/scripts/test_check_transport_imports_integration.py, tests/unit/test_thread_safety.py, tests/unit/validation/test_contract_linter.py, tests/unit/validation/test_fsm_analysis.py	Deleted health-check integration test suite; removed HTTP mock utilities and unit tests; updated allowlist test naming and assertions; added xfail marker to thread-safety test; removed docstring from test module; adjusted test data representation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Health check mixin logic: Substantial refactoring with inline aiohttp integration and modified return signatures requires careful validation of control flow and error handling.
• Public API changes: Protocol removal and method signature updates need verification that dependent code is properly updated or no longer exists.
• Type system restructuring: Multiple TypedDict changes across converter and type files require tracing to ensure all usages align with new structure.
• Test deletions: Removal of full integration test suite for health checks needs verification that coverage is maintained or intentionally reduced.
• Heterogeneous changes: Mix of CI config, logic refactoring, type changes, and test modifications across disparate files increases cognitive load.

Possibly related PRs

• PR #159: Directly modifies the same health-check and HTTP protocol code but in opposite direction (adds protocol abstractions where this PR removes them).
• PR #147: Updates identical files (scripts/check_transport_imports.py, mixin_health_check.py) for related allowlist and health-check logic.
• PR #71: Modifies .github/workflows/test.yml to configure 20-split parallelization (same as this PR's CI change).

Poem

🐰 From protocols we now break free,
HTTP calls inlined so clean,
Twenty splits make CI more lean,
Health checks bloom in simplicity! ✨

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch revert-159-jonah/omn-566-refactor-mixin_health_checkpy-to-use-protocolhttpclient

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2fd2a4c and 7b599ab.

📒 Files selected for processing (18)

• .github/workflows/test.yml (3 hunks)
• CLAUDE.md (6 hunks)
• scripts/check_transport_imports.py (2 hunks)
• src/omnibase_core/mixins/mixin_health_check.py (4 hunks)
• src/omnibase_core/protocols/__init__.py (0 hunks)
• src/omnibase_core/protocols/http/__init__.py (0 hunks)
• src/omnibase_core/protocols/http/protocol_http_client.py (0 hunks)
• src/omnibase_core/types/__init__.py (0 hunks)
• src/omnibase_core/types/converter_health_to_typed_dict.py (1 hunks)
• src/omnibase_core/types/typed_dict_converted_health.py (0 hunks)
• src/omnibase_core/types/typed_dict_health_status.py (1 hunks)
• tests/integration/mixins/__init__.py (0 hunks)
• tests/integration/mixins/test_mixin_health_check_integration.py (0 hunks)
• tests/integration/scripts/test_check_transport_imports_integration.py (4 hunks)
• tests/unit/mixins/test_mixin_health_check.py (0 hunks)
• tests/unit/test_thread_safety.py (1 hunks)
• tests/unit/validation/test_contract_linter.py (2 hunks)
• tests/unit/validation/test_fsm_analysis.py (1 hunks)

---

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

[claude]

Code Review - PR #164: Revert ProtocolHttpClient Feature

Summary

This PR reverts #159 which introduced ProtocolHttpClient protocol abstraction and refactored health checks to use dependency injection. The revert appears clean and restores the codebase to its pre-#159 state.

Positive Aspects

1. Clean Revert - All changes from feat(protocols): add ProtocolHttpClient and refactor health check [OMN-566] #159 are properly reverted with no partial reversions or merge conflicts
2. Documentation Consistency - CLAUDE.md updated to revert test count from 15,624 to 12,198 and CI split count reverted from 24 to 20
3. CI Configuration Alignment - .github/workflows/test.yml properly restored to 20 splits matching historical baseline
4. Allowlist Management - scripts/check_transport_imports.py properly restored with mixin_health_check.py re-added to transport allowlist

Areas of Concern

1. Missing Revert Rationale - The PR body only states 'Reverts #159' without explaining WHY. Was there a critical bug, CI failure, performance regression, or architectural decision reversal?

2. Transport Import Violation Re-Introduced - mixin_health_check.py is back on the allowlist for direct aiohttp imports. The original PR #159 was specifically created to remove this violation (OMN-566). Reverting undoes that compliance work.

3. Test Suite Size Discrepancy - Test count reverted from 15,624 to 12,198 (loss of ~3,400 tests). However, only 697 tests were in the removed test files. This math does not add up and should be verified.

4. Protocol Module Orphaned - The entire src/omnibase_core/protocols/http/ module is removed. Verify no other branches/PRs have started using this protocol.

Impact Assessment

• Test Coverage: 3,426 fewer tests collected (Medium severity)
• CI Performance: Restored to 20-split baseline (Neutral)
• Code Compliance: Re-introduced transport import violation (Medium severity)
• Breaking Changes: Removes public protocol API (High severity)

Approval Status

APPROVE with reservations

The revert itself is technically clean and correct, but the lack of rationale is concerning. Safe to merge as a tactical revert, but strategic questions remain unanswered.

Action Items

For PR Author:

• Add revert rationale to PR description
• Verify test count post-merge matches documentation
• Update OMN-566 Linear ticket status
• Document plan for transport import compliance

For Team:

• Investigate root cause that required this revert
• Decide if ProtocolHttpClient abstraction should be re-attempted
• Review allowlist expiration strategy

---

Reviewed by: Claude (AI Code Reviewer) | Date: 2025-12-11 | Risk: Medium