Respect STRICT_PERSISTENCE_ERRORS for persistence DI errors #623
Conversation
WalkthroughAdds environment-variable override for strict persistence error handling by reading STRICT_PERSISTENCE_ERRORS in src/core/persistence.py and updating the decision logic. Includes corresponding unit test that sets the environment variable, triggers a failing provider, asserts ServiceResolutionError is raised, and cleans up the environment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test as Unit Test
participant Env as Environment
participant Persist as Persistence
participant Provider as DI Provider
Test->>Env: Set STRICT_PERSISTENCE_ERRORS=true
Test->>Persist: _apply_default_backend("openai")
Persist->>Env: Read STRICT_PERSISTENCE_ERRORS
alt strict enabled
Persist->>Persist: _should_raise_strict_errors = True
Persist->>Provider: Resolve backend provider
Provider-->>Persist: ServiceResolutionError
Persist-->>Test: Raise ServiceResolutionError
else strict disabled
Persist->>Persist: Use config-based logic
Persist-->>Test: Proceed per existing behavior
end
Test->>Env: Unset STRICT_PERSISTENCE_ERRORS
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/test_config_persistence.py (1)
234-260: Test correctly validates the strict error behavior.The test properly:
- Sets the
STRICT_PERSISTENCE_ERRORSenvironment variable.- Uses a failing provider that raises
ServiceResolutionError.- Verifies that the exception propagates when strict mode is enabled.
- Cleans up the environment variable.
Optional: Simplify
_StrictAppStateclass.The
_StrictAppStateclass only overridesset_backend_typewith the same implementation as its parent_DummyAppState. Consider using_DummyAppStatedirectly:- class _StrictAppState(_DummyAppState): - def set_backend_type(self, backend_type: str | None) -> None: # type: ignore[override] - self.backend_type = backend_type - class _FailingProvider: def get_required_service(self, _service_type): # type: ignore[no-untyped-def] raise ServiceResolutionError("boom") monkeypatch.setenv("STRICT_PERSISTENCE_ERRORS", "true") manager = ConfigManager( FastAPI(), path=str(tmp_path / "config.json"), service_provider=_FailingProvider(), - app_state=_StrictAppState(), + app_state=_DummyAppState(), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/persistence.py(2 hunks)tests/unit/test_config_persistence.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid using emojis in Python code comments or docstrings
Follow PEP 8 and use type hints for all functions
Import order must be: standard library, third-party, then local imports, separated by blank lines
Naming conventions: snake_case for variables and functions; PascalCase for classes
Error handling: use specific exceptions and include meaningful error messages
Prefer f-strings for string formatting
Files:
src/core/persistence.pytests/unit/test_config_persistence.py
src/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.py: Catch specific exceptions; avoid broad except Exception blocks
If a broad exception must be caught, log with exc_info=True and re-raise a specific custom exception
Use the most specific exception class from src.core.common.exceptions that accurately describes the error
Provide clear, helpful error messages and include relevant details in the details dictionary of custom exceptions
Files:
src/core/persistence.py
🧬 Code graph analysis (1)
tests/unit/test_config_persistence.py (3)
src/core/common/exceptions.py (2)
ConfigurationError(96-105)ServiceResolutionError(141-150)tests/unit/core/app/controllers/test_models_controller.py (2)
get_required_service(24-32)get_required_service(77-80)src/core/persistence.py (2)
ConfigManager(25-532)_apply_default_backend(94-175)
⏰ 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). (1)
- GitHub Check: test (3.10)
🔇 Additional comments (3)
src/core/persistence.py (2)
3-3: LGTM!The
osimport is correctly placed with other standard library imports, following PEP 8 import ordering conventions.
40-48: LGTM!The environment variable check is well-implemented:
- Gives environment variable precedence over config, which aligns with the PR objectives.
- Uses sensible truthy values (
"true","1","yes").- Maintains backward compatibility by falling back to config-based logic.
tests/unit/test_config_persistence.py (1)
15-19: LGTM!The import of
ServiceResolutionErroris correctly added and follows PEP 8 import ordering (standard library, third-party, then local imports).
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Fast-Track Processing Result: ❌ REJECTEDReason: Cherry-pick conflicts detected Eligibility Check ✅
Processing Attempt ❌
Next StepsThis PR requires manual review and merge due to conflicts with current dev branch. Please use standard PR review process. 🤖 Generated with Claude Code |
Summary
ConfigManagerrespects theSTRICT_PERSISTENCE_ERRORSenvironment flag when deciding whether to raise DI failuresTesting
https://chatgpt.com/codex/tasks/task_e_68ec25e4aee08333a1a09066d47a3acf
Summary by CodeRabbit