fix(api): propagate exception handlers to versioned apps

[olivermeyer]

Deprecating version_exception_handler_registrations such that exception_handler_registrations applies to the root API instance and all versioned API instances, to match Bridge behaviour.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines	Coverage Δ
src/aignostics_foundry_core/api/core.py	98.16% <100.00%> (+0.06%)	⬆️

[claude]

Claude finished @olivermeyer's task in 2m 35s —— View job

---

Code Review for PR #34

Overview

This PR unifies exception handler registration across root and versioned FastAPI apps. The implementation is sound and well-tested, but there are important considerations around the breaking change.

---

✅ Strengths

1. Correct architectural fix: Properly addresses FastAPI's behavior where mounted sub-apps handle exceptions independently
2. Excellent test coverage: 100% coverage with comprehensive unit tests
3. Proper test markers: All tests correctly marked with @pytest.mark.unit
4. Good documentation: Updated docstrings and AGENTS.md accurately reflect the new behavior
5. Code quality: No SonarQube issues, constants properly defined
6. Clean implementation: Eliminates the confusing dual-parameter API in favor of consistent handler registration

---

🚨 Blocking Issues

1. Breaking Change Classification - src/aignostics_foundry_core/api/core.py:388-393

The parameter version_exception_handler_registrations has been removed entirely, not deprecated. This is a breaking change.

Issue:

• PR title says "fix" but this is a breaking API change
• No deprecation warning or transition period
• Existing code using version_exception_handler_registrations will fail immediately with TypeError: init_api() got an unexpected keyword argument

Impact: Any downstream code calling init_api(version_exception_handler_registrations=[...]) will break.

Fix options:

1. Recommended: Add a deprecation warning for one version cycle:

   def init_api(
       # ... other params ...
       version_exception_handler_registrations: list[tuple[type[Exception], Any]] | None = None,
       **fastapi_kwargs: Any,
   ) -> FastAPI:
       if version_exception_handler_registrations is not None:
           warnings.warn(
               "version_exception_handler_registrations is deprecated. "
               "Use exception_handler_registrations instead, which now applies to all apps.",
               DeprecationWarning,
               stacklevel=2,
           )
           # Could merge with exception_handler_registrations if needed

2. Alternative: Accept this as breaking, but update PR title to feat(api)!: or fix(api)!: (conventional commits breaking change marker) and bump to 1.0.0 (MAJOR)

Note: The version bump is already 0.7.2 → 0.8.0 (MINOR), which is correct for semantic versioning pre-1.0 where MINOR can include breaking changes.

---

💡 Suggestions

1. Type hint improvements - src/aignostics_foundry_core/api/core.py:445-447

The code uses # type: ignore[arg-type] and # pyright: ignore[reportArgumentType]:

version_app.add_exception_handler(  # type: ignore[arg-type]
    exc_class_or_status_code=ApiException,
    handler=api_exception_handler,  # pyright: ignore[reportArgumentType]
)

Suggestion: This likely indicates a signature mismatch. Consider:

• Verifying the handler signatures match FastAPI's expected type
• If this is a known FastAPI typing limitation, add a comment explaining why the ignore is necessary
• Check if there's a more precise type annotation that would satisfy the checker

2. Test naming consistency - tests/aignostics_foundry_core/api/core_test.py:296

Test name is verbose but clear:

def test_init_api_propagates_custom_exception_handlers_to_versioned_apps

Consider shorter but equally clear:

def test_init_api_applies_custom_handlers_to_versioned_apps

Not blocking - current name is fine, just slightly verbose

3. Documentation enhancement

The docstring explains the "why" well, but could benefit from a migration example:

"""...existing docstring...

Migration note:
    Prior to v0.8.0, versioned apps required a separate
    ``version_exception_handler_registrations`` parameter.
    This is no longer needed; all handlers in
    ``exception_handler_registrations`` now apply to both
    root and versioned apps automatically.

    Before (v0.7.x)::

        init_api(
            exception_handler_registrations=[(MyError, handler)],
            version_exception_handler_registrations=[(MyError, handler)],
            versions=["v1"]
        )

    After (v0.8.0+)::

        init_api(
            exception_handler_registrations=[(MyError, handler)],
            versions=["v1"]
        )
"""

4. Consider adding a CHANGELOG entry

Document this breaking change:

### [0.8.0] - 2026-04-08
#### Changed
- **BREAKING**: Removed `version_exception_handler_registrations` parameter from `init_api()`. 
  All exception handlers (including custom ones) now propagate to versioned apps automatically.
  Use `exception_handler_registrations` for all apps. (#34)

---

🧪 Test Quality

All tests follow best practices:

• ✅ Proper markers (@pytest.mark.unit)
• ✅ Clear test names describing expected behavior
• ✅ Good use of mocks to isolate unit under test
• ✅ Assertions cover both custom and standard handlers
• ✅ Constants defined at module level (no string duplication)

---

📊 Metrics

• ✅ Codecov: 100% coverage on new code
• ✅ SonarQube: Quality gate passed, 0 new issues
• ✅ All tests passing

---

Recommendation

Near approval - The code quality and implementation are excellent. The main concern is the breaking change handling:

1. If this is intended for internal use only (within aignostics org): Accept as-is with the 0.8.0 version bump
2. If this has external users: Add the deprecation warning suggested above, or clearly document the breaking change in a CHANGELOG

The technical implementation is sound and ready to merge once the breaking change is properly communicated.

---

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud