feat: add DEGRADED health state

[olivermeyer]

• Porting the DEGRADED health state from Bridge; updating platform service to reflect this new state; updating specs
• Updating application CLI tests to use a common submitted_run context manager and always cancel run after submission

[claude]

Claude finished @olivermeyer's task in 4m 24s —— View job

---

🎯 PR Review Complete: feat/health-degraded

Reviewer: Claude Code
Review Date: 2025-03-13
PR: #470
Branch: feat/health-degraded → main
Files Changed: 9 files (+656/-504)

---

✅ APPROVAL: EXCELLENT IMPLEMENTATION

This PR successfully adds the DEGRADED health state to the SDK with high-quality implementation, comprehensive test coverage, and proper semantic behavior. All critical checks pass.

---

📊 Review Summary

Category	Status	Details
Test Markers	✅ PASS	All tests properly marked (unit, integration, e2e) - 0 unmarked tests
Test Coverage	✅ PASS	95.74% (above 85% minimum)
Code Quality	✅ PASS	Linting passed (ruff ✅, mypy ✅, pyright ✅)
Architecture	✅ PASS	Follows modulith principles, proper separation of concerns
Documentation	✅ PASS	Specifications updated, CLAUDE.md updated
Conventional Commits	✅ PASS	Both commits follow convention
Breaking Changes	✅ NONE	Backward compatible enhancement
Security	✅ PASS	No security concerns

---

🌟 Highlights

1. Correct Semantic Implementation ⭐⭐⭐

The PR correctly distinguishes between three health states:

• UP: Service fully operational (truthy in boolean context)
• DEGRADED: Service operational with reduced functionality (truthy - warns but allows operations)
• DOWN: Service not operational (falsy - blocks operations unless --force)

Key implementation (_health.py:132-138):

def __bool__(self) -> bool:
    """Convert health status to a boolean value.
    
    Returns:
        bool: True if the status is UP or DEGRADED, False otherwise.
    """
    return self.status in {HealthStatus.UP, HealthStatus.DEGRADED}

This allows _abort_if_system_unhealthy() to work correctly:

if not health:  # Only False for DOWN, True for UP/DEGRADED
    # Block operation

2. Comprehensive Test Coverage ⭐⭐⭐

New tests added (all properly marked with @pytest.mark.unit or @pytest.mark.e2e):

Utils module (tests/aignostics/utils/health_test.py):

• test_health_degraded_requires_reason() - Validation (line 190)
• test_str_representation_degraded() - String formatting (line 202)
• test_compute_health_from_components_single_degraded() - Single degraded component (line 210)
• test_compute_health_from_components_multiple_degraded() - Multiple degraded components (line 228)
• test_compute_health_from_components_down_trumps_degraded() - Precedence rules (line 248)

Platform module (tests/aignostics/platform/service_test.py):

• test_determine_api_public_health_degraded_response() - Basic DEGRADED response (line 143)
• test_determine_api_public_health_degraded_response_with_reason() - Custom reason extraction (line 160)

Application module (tests/aignostics/application/cli_test.py):

• test_cli_run_submit_succeeds_when_system_degraded_and_no_force() - E2E validation (line 314)

3. Health Propagation Logic ⭐⭐

Hierarchical health computation correctly implements precedence:

1. DOWN components → parent DOWN (highest priority)
2. DEGRADED components (no DOWN) → parent DEGRADED (medium priority)
3. All UP components → parent UP (lowest priority)

Implemented in compute_health_from_components() (_health.py:42-91).

4. Specification Updates ⭐

• SPEC-UTILS-SERVICE.md: Added DEGRADED to health status enum (line 154)
• Clarified that reason is "Required reason for DOWN or DEGRADED status"
• SPEC_SYSTEM_SERVICE.md: Removed duplicate health documentation (proper DRY principle)

5. Test Refactoring ⭐

Excellent improvement: Extracted submitted_run() context manager (line 87-119) to ensure proper cleanup:

@contextmanager
def submitted_run(runner: CliRunner, tmp_path: Path, csv_content: str) -> Iterator[str]:
    """Submit a run and cancel it during cleanup."""
    # ... submission logic ...
    try:
        yield run_id
    finally:
        # Always cancel run in cleanup
        cancel_result = runner.invoke(cli, ["application", "run", "cancel", run_id])

This prevents test pollution and ensures runs are always canceled.

---

🔍 Detailed Code Review

✅ src/aignostics/utils/_health.py

Lines 9-20: ✅ EXCELLENT - Clean enum definition

• Uses StrEnum for automatic string conversion
• Three states clearly defined with docstrings
• Aligns with SPEC-UTILS-SERVICE.md

Lines 27-29: ✅ EXCELLENT - Updated docstring

• Correctly documents that both DOWN and DEGRADED require reasons
• Matches implementation in validator (lines 115-118)

Lines 82-89: ✅ EXCELLENT - DEGRADED component propagation

• Correctly handles single vs multiple DEGRADED components
• Generates informative reason messages
• Mirrors DOWN propagation pattern

Lines 115-118: ✅ EXCELLENT - Validation ensures consistency

• Both DOWN and DEGRADED must have reason
• UP must NOT have reason
• Enforced via Pydantic model validator

Lines 128-130: ✅ EXCELLENT - String representation includes DEGRADED

• Shows reason for both DOWN and DEGRADED states
• Clean formatting

Lines 132-138: ✅ EXCELLENT - Boolean semantics (CRITICAL)

• Both UP and DEGRADED are truthy → operational services
• DOWN is falsy → non-operational service
• This enables correct CLI behavior

✅ src/aignostics/platform/_service.py

Lines 195-229: ✅ EXCELLENT - API response parsing

• Handles three states: UP, DEGRADED, DOWN (via unknown status)
• Extracts custom reason from API if provided
• Falls back to default reason
• Logs warnings for observability
• Note: "DOWN" in response body treated as unknown (falls through to line 226-229), which returns DOWN with appropriate message. This is correct - a 200 response with "status": "DOWN" is unexpected, and treating it as unknown status is appropriate.

Lines 231-253, 254-279: ✅ EXCELLENT - Dual health checks

• Public health check (unauthenticated)
• Authenticated health check (with token)
• Both use shared _health_from_response() helper
• Proper exception handling and logging

✅ tests/aignostics/utils/health_test.py

Lines 190-198: ✅ EXCELLENT - DEGRADED validation test

• Verifies reason is required
• Tests both success and failure cases
• Uses pytest.raises correctly
• Properly marked @pytest.mark.unit

Lines 210-245: ✅ EXCELLENT - DEGRADED propagation tests

• Single DEGRADED component (line 210)
• Multiple DEGRADED components (line 228)
• DOWN trumps DEGRADED (line 248) - priority order test
• Comprehensive coverage of edge cases
• All properly marked @pytest.mark.unit

✅ tests/aignostics/platform/service_test.py

Lines 143-173: ✅ EXCELLENT - DEGRADED API response tests

• Tests basic DEGRADED response (line 143)
• Tests DEGRADED with custom reason (line 160)
• Tests unknown status returns DOWN (line 177)
• Uses mocked HTTP responses properly
• All properly marked @pytest.mark.unit

✅ tests/aignostics/application/cli_test.py

Lines 87-119: ✅ EXCELLENT - Context manager refactoring

• Extracts common pattern for run submission and cleanup
• Ensures runs are always canceled in finally block
• Logs warnings if cancel fails (acceptable - run may already be canceled)
• Reduces code duplication across tests

Lines 314-323: ✅ EXCELLENT - E2E DEGRADED behavior test

• Properly marked @pytest.mark.e2e
• Timeout set to 10 seconds
• Mocks SystemService.health_static() to return DEGRADED
• Verifies submission succeeds without --force flag
• Uses submitted_run() context manager for cleanup

Line 326-354: ✅ GOOD - Contrasting DOWN behavior test

• Shows that DOWN requires --force or blocks
• Properly marked @pytest.mark.e2e

✅ src/aignostics/system/CLAUDE.md

Lines 276-277: ⚠️ MINOR ISSUE - Code example has potential bug

platform_health = health.components.get("platform")
if not platform_health:  # False only when DOWN (DEGRADED and UP are both truthy)
    print(f"Platform issue: {platform_health.reason}")

Issue: If platform_health is None (missing key), the if not platform_health: evaluates to True, but then platform_health.reason would raise AttributeError.

Should be:

platform_health = health.components.get("platform")
if platform_health is not None and not platform_health:  # Explicitly check for DOWN
    print(f"Platform issue: {platform_health.reason}")

Or better:

platform_health = health.components.get("platform")
if platform_health and platform_health.status == Health.Code.DOWN:
    print(f"Platform is DOWN: {platform_health.reason}")
elif platform_health and platform_health.status == Health.Code.DEGRADED:
    print(f"Platform is DEGRADED: {platform_health.reason}")

✅ specifications/SPEC-UTILS-SERVICE.md

Line 154: ✅ EXCELLENT - Updated enum

enum: [UP, DEGRADED, DOWN]

Line 159: ✅ EXCELLENT - Clarified reason requirement

description: Required reason for DOWN or DEGRADED status; must be null for UP

✅ specifications/SPEC_SYSTEM_SERVICE.md

Lines removed: ✅ EXCELLENT - DRY principle

• Removed duplicate Health schema documentation
• Now references SPEC-UTILS-SERVICE.md as single source of truth

---

🔴 Sentry Bot Comments - Addressed

Comment 1: _abort_if_system_unhealthy incorrectly treats DEGRADED as failure

Sentry's Claim: "The __bool__ method now only returns True for an UP status"

Actual Code (_health.py:138):

return self.status in {HealthStatus.UP, HealthStatus.DEGRADED}

Verdict: ❌ INVALID CONCERN - Sentry bot is incorrect. The __bool__ method returns True for BOTH UP and DEGRADED, which is the correct semantic behavior. The CLI correctly allows operations to proceed when system is DEGRADED.

Comment 2: _health_from_response() incorrectly handles DOWN status

Sentry's Claim: "The function only handles UP and DEGRADED explicitly; DOWN falls through to unknown status"

Actual Behavior:

• HTTP 503 (or non-200) → DOWN (line 208-212)
• HTTP 200 + unparseable body → DOWN (line 217)
• HTTP 200 + {"status": "DOWN"} → Falls through to "unknown status" → DOWN (line 226-229)

Verdict: ⚠️ ACCEPTABLE DESIGN CHOICE - A 200 response with "status": "DOWN" is semantically strange (200 means OK, but status says DOWN). The current implementation treats this as "unknown status" and returns DOWN with a descriptive message. This is reasonable defensive programming. However, adding an explicit if api_status == "DOWN": branch would make the code more explicit, though it's not critical.

---

🎓 Architectural Compliance

✅ Modulith Principles

• Changes confined to utils (foundation) and platform (API layer)
• No circular dependencies introduced
• Clear separation of concerns maintained

✅ Service Discovery

• Health system integrates with BaseService.health() interface
• System module aggregates health from all services
• No changes to dependency injection patterns

✅ Presentation Layer Pattern

• CLI layer (_cli.py) depends only on Service layer
• No GUI changes (platform module has no GUI)
• Proper separation maintained

---

📝 Conventional Commits

✅ Commit 1: feat(utils, platform): add DEGRADED state to Health model

• Type: feat ✅
• Scope: utils, platform ✅
• Description: Clear and concise ✅

✅ Commit 2: refactor(tests): extract submitted_run context manager

• Type: refactor ✅
• Scope: tests ✅
• Description: Clear ✅

---

🚀 Recommendations

🟢 Ready to Merge

This PR is approved for merge with one minor documentation fix suggestion.

Optional Improvements (Not Blocking)

1. Documentation Fix (MINOR - can be fixed in follow-up):

   • Fix code example in system/CLAUDE.md lines 275-277 to handle None case
   • See detailed suggestion above

2. Future Enhancement (NOT BLOCKING):

   • Consider adding explicit if api_status == "DOWN": branch in _health_from_response() for clarity, though current behavior is acceptable

3. Metrics/Observability (Future):

   • Track DEGRADED state frequency and duration in production
   • Add metrics to understand when/why services are degraded

---

🎯 Summary

What This PR Does Well

✅ Semantic correctness: DEGRADED means "operational but with reduced functionality"
✅ Comprehensive testing: 8 new tests covering all scenarios
✅ Proper test markers: All tests categorized correctly (unit/integration/e2e)
✅ Health propagation: Hierarchical computation with correct precedence
✅ Documentation: Specifications and CLAUDE.md updated
✅ Code quality: Passes all linting (ruff, mypy, pyright)
✅ Test refactoring: Extracted submitted_run() context manager
✅ No breaking changes: Backward compatible enhancement

Medical Device Software Context

This PR maintains the high standards required for medical device software:

• ✅ No security concerns
• ✅ No HIPAA compliance issues
• ✅ Proper error handling and logging
• ✅ Clear operational semantics (degraded vs down)
• ✅ Comprehensive test coverage (95.74%)

---

🏆 Excellent Work!

This PR demonstrates:

• ✅ Strong understanding of the SDK architecture
• ✅ Comprehensive testing approach
• ✅ Clean, maintainable implementation
• ✅ Proper documentation updates
• ✅ Attention to detail
• ✅ Good test hygiene (context managers for cleanup)

Recommendation: ✅ APPROVE AND MERGE

---

Review conducted according to: CLAUDE.md standards for medical device software SDK

[Copilot]

Pull request overview

Adds support for a DEGRADED health state across the SDK so services can report partial availability (between UP and DOWN), including platform health parsing and updated specs/tests.

Changes:

• Extend Health to include DEGRADED, with aggregation logic and validation rules.
• Update platform service health checks to parse /health JSON and return UP/DEGRADED/DOWN.
• Update specs and expand unit tests to cover DEGRADED behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/aignostics/utils/_health.py	Adds DEGRADED status, component aggregation priority, and validation/__str__ updates.
src/aignostics/platform/_service.py	Introduces response-to-Health mapping and uses it for public/authenticated health checks.
tests/aignostics/utils/health_test.py	Adds unit tests for DEGRADED validation, string formatting, and aggregation.
tests/aignostics/platform/service_test.py	Adds/adjusts tests to validate JSON-body-driven health mapping including DEGRADED.
specifications/SPEC-UTILS-SERVICE.md	Updates schema to include DEGRADED and clarifies reason requirements.
specifications/SPEC_SYSTEM_SERVICE.md	Removes duplicated Health schema documentation.

[codecov]

Codecov Report

❌ Patch coverage is 95.83333% with 2 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/aignostics/platform/_service.py	89.47%	2 Missing ⚠️

Files with missing lines	Coverage Δ
src/aignostics/utils/__init__.py	80.00% <100.00%> (ø)
src/aignostics/utils/_health.py	100.00% <100.00%> (ø)
src/aignostics/platform/_service.py	84.55% <89.47%> (-0.27%)	⬇️

... and 6 files with indirect coverage changes

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

[claude]

🎯 PR Review: feat/health-degraded

Reviewer: Claude Code
Review Date: 2025-03-11
PR Author: @olivermeyer
Files Changed: 8 files (+301/-85)

---

✅ APPROVAL: EXCELLENT IMPLEMENTATION

This PR successfully ports the DEGRADED health state from Bridge to the Python SDK with high quality implementation and comprehensive test coverage. All critical checks pass.

---

📊 Review Summary

Category	Status	Details
Test Markers	✅ PASS	All tests properly marked (unit, integration, e2e)
Test Coverage	✅ PASS	Comprehensive tests added for all three health states
Code Quality	✅ PASS	Linting passed (ruff, mypy, pyright)
Architecture	✅ PASS	Follows modulith principles, proper separation of concerns
Documentation	✅ PASS	Specifications updated correctly
Conventional Commits	✅ PASS	feat(utils, platform): add DEGRADED state to Health model
Breaking Changes	✅ NONE	Backward compatible enhancement
Security	✅ PASS	No security concerns

---

🌟 Highlights

1. Semantic Correctness ⭐⭐⭐

The implementation correctly distinguishes between three health states:

• UP: Service fully operational
• DEGRADED: Service operational with reduced functionality (⚠️ warns but allows)
• DOWN: Service not operational (🚫 blocks unless --force)

This is implemented in:

• Health.__bool__() → Returns True for both UP and DEGRADED
• CLI tests → DEGRADED doesn't require --force, DOWN does
• Platform service → Properly parses DEGRADED from API responses

2. Comprehensive Test Coverage ⭐⭐⭐

New tests added:

• test_health_degraded_requires_reason() - Validation
• test_str_representation_degraded() - String formatting
• test_compute_health_from_components_single_degraded() - Single component
• test_compute_health_from_components_multiple_degraded() - Multiple components
• test_compute_health_from_components_down_trumps_degraded() - Precedence rules
• test_determine_api_public_health_degraded_response() - Platform service integration
• test_determine_api_public_health_degraded_response_with_reason() - Reason extraction
• test_cli_run_submit_succeeds_when_system_degraded_and_no_force() - E2E validation

All tests properly marked with @pytest.mark.unit / @pytest.mark.e2e.

3. Health Propagation Logic ⭐⭐

The hierarchical health computation correctly implements:

1. DOWN components → parent DOWN (highest priority)
2. DEGRADED components (no DOWN) → parent DEGRADED (medium priority)
3. All UP components → parent UP (lowest priority)

This is recursively applied through compute_health_from_components().

4. Specification Updates ⭐

• SPEC-UTILS-SERVICE.md: Added DEGRADED to health status enum (line 154)
• SPEC_SYSTEM_SERVICE.md: Removed duplicate health documentation (proper DRY principle)

---

🔍 Detailed Findings

✅ NO BLOCKING ISSUES FOUND

All critical checks passed:

1. Test Markers: ✅ No unmarked tests found

   # Verified with:
   uv run pytest -m "not unit and not integration and not e2e" --collect-only
   # Result: collected 747 items / 747 deselected / 0 selected

2. Linting: ✅ All checks passed

   make lint
   # ruff check: All checks passed!
   # ruff format: 165 files already formatted
   # pyright: 0 errors, 0 warnings, 0 informations
   # mypy: Success: no issues found in 81 source files

3. Conventional Commits: ✅ Properly formatted

   feat(utils, platform): add DEGRADED state to Health model
   

4. Type Safety: ✅ Dual type checking passed (MyPy strict + PyRight basic)

5. Architecture Compliance: ✅ No circular dependencies, proper module boundaries

---

📝 Code Review Notes

src/aignostics/utils/_health.py

Lines 9-20: ✅ EXCELLENT - Clean enum definition

• Uses StrEnum for automatic string conversion
• Three states clearly defined with docstring
• Aligns with SPEC-UTILS-SERVICE.md

Lines 81-88: ✅ EXCELLENT - Degraded component propagation

• Correctly handles single vs multiple DEGRADED components
• Generates informative reason messages
• Mirrors DOWN propagation pattern

Lines 114-117: ✅ EXCELLENT - Validation ensures consistency

• DEGRADED must have reason (like DOWN)
• UP must NOT have reason
• Enforced via Pydantic model validator

Line 137: ✅ EXCELLENT - Boolean semantics (see inline comment)

• Both UP and DEGRADED are truthy → operational services
• DOWN is falsy → non-operational service

src/aignostics/platform/_service.py

Lines 195-229: ✅ EXCELLENT - API response parsing

• Handles three states: UP, DEGRADED, DOWN
• Extracts custom reason from API if provided
• Falls back to default reason
• Logs warnings for observability
• Consistent with Bridge implementation

Lines 231-253, 254-279: ✅ GOOD - Dual health checks

• Public health check (unauthenticated)
• Authenticated health check (with token)
• Both use shared _health_from_response() helper
• Proper exception handling and logging

tests/aignostics/utils/health_test.py

Lines 190-199: ✅ EXCELLENT - DEGRADED validation test

• Verifies reason is required
• Tests both success and failure cases
• Uses pytest.raises correctly

Lines 210-245: ✅ EXCELLENT - DEGRADED propagation tests

• Single DEGRADED component
• Multiple DEGRADED components
• DOWN trumps DEGRADED (priority order)
• Comprehensive coverage of edge cases

tests/aignostics/platform/service_test.py

Lines 142-157: ✅ EXCELLENT - DEGRADED API response tests

• Tests basic DEGRADED response
• Tests DEGRADED with custom reason
• Uses mocked HTTP responses properly

tests/aignostics/application/cli_test.py

Lines 217-252: ✅ EXCELLENT - E2E behavior validation

• DEGRADED allows submission without --force
• Properly submits run and cleans up
• Tests real workflow impact

---

🎓 Lessons & Best Practices Demonstrated

1. Semantic Health States: Distinguishes "operational but degraded" from "down" - important for graceful degradation strategies

2. Test Categorization: All tests properly marked (no unmarked tests found)

3. Hierarchical Propagation: Recursive health computation with clear precedence rules (DOWN > DEGRADED > UP)

4. DRY Documentation: Removed duplicate health documentation from SPEC_SYSTEM_SERVICE.md, referencing SPEC-UTILS-SERVICE.md instead

5. Backward Compatibility: Enhancement adds new state without breaking existing behavior

---

🚀 Recommendations for Merge

✅ Ready to Merge

This PR is approved for merge with no blocking issues. All quality gates passed.

Optional Future Enhancements (Not Blocking)

1. Metrics/Observability: Consider adding metrics to track DEGRADED state frequency and duration
2. User Documentation: Update user-facing documentation to explain DEGRADED state behavior
3. Platform Consistency: Ensure all Aignostics platform services (Bridge, PAPI) use consistent DEGRADED semantics

---

🏆 Excellent Work!

This PR demonstrates:

• ✅ Strong understanding of the SDK architecture
• ✅ Comprehensive testing approach
• ✅ Clean, maintainable implementation
• ✅ Proper documentation updates
• ✅ Attention to detail

No changes required. Ready to merge! 🎉

---

Review conducted according to: CLAUDE.md standards for medical device software SDK

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
97.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[arne-aignx]

LGTM