Repository navigation
feat(context): add ApplicationContext for thread-safe container management - #92
Conversation
…ement Introduces contextvars-based container management to replace the legacy singleton holder pattern. This provides proper isolation between async tasks and threads during parallel test execution with pytest-xdist. Key changes: - Add ApplicationContext class with sync/async context managers - Add isolation fixtures for parallel-safe test execution - Migrate registries and caches to use lru_cache patterns - Remove util_container_holder.py (replaced by contextvars) - Update conftest.py with improved cleanup and isolation
WalkthroughIntroduces context-based container management replacing global singleton access, adds logger caching with TTL-based protocol service lookups, reorganizes service wrapper import paths from node_services to service module, removes ~80 legacy migration and analysis scripts, updates pytest to use loadgroup distribution with test retry logic, and cleans up events module exports. Documentation and configuration updated accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review: ApplicationContext for Thread-Safe Container ManagementI've completed a thorough review of this PR following the project's architecture patterns and conventions outlined in CLAUDE.md. Overall, this is excellent architectural work that addresses real test isolation issues with a clean, modern solution. 🎯 SummaryThis PR successfully migrates from a legacy singleton holder pattern to contextvars-based container management, providing proper isolation for parallel test execution with pytest-xdist. The implementation is well-architected, thoroughly tested, and aligns with Python best practices. Recommendation: ✅ Approve with minor suggestions ✅ Strengths1. Excellent Architecture ⭐
2. Comprehensive Testing ⭐
3. Documentation ⭐
4. Type Safety ⭐
5. Backward Compatibility
🔍 Issues & Recommendations🔴 CRITICAL: Missing Test Pre-ConditionsLocation: "--reruns=2", # Auto-retry failed tests twice (handles flaky tests)
"--reruns-delay=1", # 1 second delay between rerunsIssue: Adding automatic test retries masks real race conditions and flakiness instead of fixing them. If tests are flaky, the root cause should be addressed, not hidden. Recommendation:
References:
🟡 MODERATE: Error Handling Inconsistencies1. Silent Fallback in get_model_onex_container_sync()Location: container = get_current_container()
if container is not None:
return container
# No container exists - create one
container = asyncio.run(create_model_onex_container())Issue: Creating a container silently when none exists can hide initialization bugs. This differs from the async version's explicit "get or create" semantics. Recommendation: Add logging or make the creation explicit: from omnibase_core.logging import core_logging
container = get_current_container()
if container is not None:
return container
# No container exists - create and log for debugging
core_logging.debug(
"No container in current context - creating new instance",
correlation_id=core_logging._get_correlation_id()
)
container = asyncio.run(create_model_onex_container())
set_current_container(container)
return container2. Registry Access Error HandlingLocation: Good: Registries now raise Issue: The error message says "Initialize the container first" but doesn't provide guidance on how to initialize it. Recommendation: Improve error messages with actionable guidance: raise ModelOnexError(
message="DI container not initialized - cannot get action registry. "
"Initialize with: container = await create_model_onex_container() or "
"use ApplicationContext(container).",
error_code=EnumCoreErrorCode.CONFIGURATION_ERROR,
) from e🟡 MODERATE: Test Configuration ConcernsLocation: "--dist=loadgroup", # Group tests by @pytest.mark.xdist_group for better controlIssue: Changing from Questions:
Recommendation:
🟢 MINOR: Code Quality Improvements1. Cache Clear Functions Could Return StatusLocation: def clear_logger_cache() -> None:
"""Clear the logger cache (for testing)."""
_get_cached_logger.cache_clear()Suggestion: Return cache info for debugging: def clear_logger_cache() -> dict[str, int]:
"""Clear the logger cache (for testing).
Returns:
Cache statistics before clearing (hits, misses, size).
"""
info = _get_cached_logger.cache_info()
_get_cached_logger.cache_clear()
return {
"hits": info.hits,
"misses": info.misses,
"size": info.currsize,
}This helps diagnose cache effectiveness in tests. 2. Magic Number in TTLLocation: _PROTOCOL_CACHE_TTL = 300.0Suggestion: Document the rationale for 5 minutes: # TTL for protocol services cache
# 300s (5 minutes) balances:
# - Enough time to avoid repeated container lookups during normal operation
# - Short enough to pick up container reinitialization in long-running processes
_PROTOCOL_CACHE_TTL = 300.03. Unused ImportsLocation: The diff shows imports were reordered but poetry run ruff check --select F401 src/omnibase_core/models/core/model_event_type_registry.py🟢 MINOR: Documentation Gaps1. CLAUDE.md Should Mention ApplicationContextThe project's CLAUDE.md extensively documents architecture but doesn't mention the new Recommendation: Add a section to CLAUDE.md: ### Application Context (v0.2.0+)
**contextvars-based container management** for thread-safe and async-safe isolation:
```python
from omnibase_core.context import ApplicationContext, get_current_container
# Set container for current context
async with ApplicationContext(container):
current = get_current_container()
# Use container...
# Or sync version
with ApplicationContext(container):
current = get_current_container()Benefits:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
320-328: Stale comment references outdated distribution strategy.The comment at Line 323 mentions
--dist=loadscopebut the actual configuration now uses--dist=loadgroup. Consider updating the comment to avoid confusion.# Memory optimization notes: # - Limited to 4 workers to prevent OOM on machines with 16GB RAM # - Use --timeout to prevent hanging tests accumulating memory -# - Use --dist=loadscope to group tests by module (reduces import overhead) +# - Use --dist=loadgroup to group tests by @pytest.mark.xdist_group (enables finer control) # Test execution strategies:
🧹 Nitpick comments (7)
tests/unit/mixins/test_mixin_hybrid_execution.py (1)
117-135: Improved log verification logic looks good.The more robust verification approach that scans all log calls for the MIXIN_INIT message is better than the previous simplistic assertion.
Minor nit: The unpacked
kwargsat Line 126 is unused. Consider using_to indicate it's intentionally ignored.for call in mock_log.call_args_list: - args, kwargs = call + args, _ = call # Message is the second positional argument if len(args) >= 2 and "MIXIN_INIT" in str(args[1]): mixin_init_found = True breaksrc/omnibase_core/models/core/model_action_registry.py (1)
280-286: Consider logging the exception for debugging test failures.While the silent no-op is intentional for the case where the container isn't initialized, catching all exceptions silently can hide unexpected errors during test debugging.
try: container = get_model_onex_container_sync() registry = cast(ModelActionRegistry, container.action_registry()) registry.clear() - except Exception: + except Exception: # noqa: BLE001 - intentional no-op for testing # If container is not initialized, nothing to reset passAlternatively, add debug logging:
except Exception as e: # If container is not initialized, nothing to reset import logging logging.debug(f"reset_action_registry no-op: {e}")tests/unit/models/core/test_model_cli_command_registry.py (1)
654-670: Consider using public reset API instead of accessing internal_current_container.The fixture directly accesses
_current_container, which is an internal implementation detail. The PR introduces public reset utilities intests/fixtures/isolation.py(e.g.,reset_container_singleton) that would be more appropriate here.@pytest.fixture(autouse=True) def reset_global_registry(self): """Reset global registry state before and after each test. - - The new pattern uses ApplicationContext (contextvars), so we reset - the context variable to ensure a fresh container is created for each test. """ - from omnibase_core.context.application_context import _current_container - - # Reset context variable to None (clear any existing container) - # This ensures a fresh container will be created - context_token = _current_container.set(None) - - yield - - # Restore original context variable state - _current_container.reset(context_token) + from tests.fixtures.isolation import SingletonResetContext + + with SingletonResetContext(holder_keys=["container"], clear_caches=True): + yieldAlternatively, since the
reset_container_singletonfixture is now globally available via conftest.py, you could simply use it:@pytest.fixture(autouse=True) def reset_global_registry(self, reset_container_singleton): """Reset global registry state before and after each test.""" yieldsrc/omnibase_core/context/__init__.py (1)
50-57: Consider sorting__all__alphabetically for consistency.Static analysis suggests sorting the exports. This is a minor style improvement.
__all__ = [ "ApplicationContext", "get_current_container", - "set_current_container", "reset_container", "run_with_container", "run_with_container_sync", + "set_current_container", ]tests/fixtures/isolation.py (1)
72-77: Accessing private_current_containerdirectly.The function imports
_current_container(a private module variable with underscore prefix) directly fromapplication_context. While this works, consider whether the publicset_current_container(None)API should be used instead, or if a dedicatedclear_current_container()function should be exposed from the module for test utilities.def reset_application_context() -> contextvars.Token[Any] | None: try: - from omnibase_core.context.application_context import _current_container + from omnibase_core.context.application_context import ( + _current_container, + ) return _current_container.set(None)Alternatively, expose a public API in
application_context.py:def clear_current_container() -> contextvars.Token[ModelONEXContainer | None]: """Clear the current container, returning a token for restore.""" return _current_container.set(None)tests/unit/enums/test_enum_scenario_status.py (2)
19-19: Add @pytest.mark.unit decorator for test categorization.Per coding guidelines, test classes should use pytest.mark decorators to categorize tests appropriately. Since this is in tests/unit/, add the unit marker.
Based on coding guidelines: "Use @pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests appropriately"
+@pytest.mark.unit class TestEnumScenarioStatus:
245-245: Add @pytest.mark.unit decorator for test categorization.Same as the previous test class - add the unit marker for proper test categorization.
+@pytest.mark.unit class TestEnumScenarioStatusEdgeCases:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
pyproject.toml(1 hunks)src/omnibase_core/context/__init__.py(1 hunks)src/omnibase_core/context/application_context.py(1 hunks)src/omnibase_core/logging/core_logging.py(2 hunks)src/omnibase_core/logging/emit.py(2 hunks)src/omnibase_core/models/container/model_onex_container.py(2 hunks)src/omnibase_core/models/core/model_action_registry.py(2 hunks)src/omnibase_core/models/core/model_cli_command_registry.py(3 hunks)src/omnibase_core/models/core/model_event_type_registry.py(2 hunks)src/omnibase_core/models/security/model_secret_manager.py(1 hunks)src/omnibase_core/utils/util_container_holder.py(0 hunks)src/omnibase_core/utils/util_singleton_holders.py(1 hunks)tests/conftest.py(2 hunks)tests/fixtures/isolation.py(1 hunks)tests/unit/context/__init__.py(1 hunks)tests/unit/context/test_application_context.py(1 hunks)tests/unit/enums/test_enum_scenario_status.py(1 hunks)tests/unit/fixtures/__init__.py(1 hunks)tests/unit/fixtures/test_isolation_fixtures.py(1 hunks)tests/unit/logging/test_core_logging.py(1 hunks)tests/unit/mixins/test_mixin_hybrid_execution.py(1 hunks)tests/unit/models/core/test_model_cli_command_registry.py(2 hunks)
💤 Files with no reviewable changes (1)
- src/omnibase_core/utils/util_container_holder.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly
Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Use @standard_error_handling decorator on async functions to eliminate boilerplate try/catch logic
Use duck typing with protocols instead of isinstance checks - protocols define contracts through interfaces, not runtime type checking
Always use 100% strict mypy type checking - all functions must have type annotations and no untyped definitions are allowed
Run code through black formatter, isort for import sorting, and ruff linter before committing
Use async/await pattern with asyncio for asynchronous operations - never use threading for I/O operations
Use ModelEventEnvelope for inter-service event-driven communication and process event payloads through envelope pattern
Files:
tests/unit/fixtures/__init__.pytests/unit/enums/test_enum_scenario_status.pytests/unit/context/test_application_context.pytests/unit/fixtures/test_isolation_fixtures.pytests/unit/logging/test_core_logging.pytests/conftest.pytests/unit/mixins/test_mixin_hybrid_execution.pysrc/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/core/model_action_registry.pysrc/omnibase_core/logging/core_logging.pysrc/omnibase_core/models/core/model_cli_command_registry.pysrc/omnibase_core/models/container/model_onex_container.pytests/unit/context/__init__.pysrc/omnibase_core/context/application_context.pysrc/omnibase_core/context/__init__.pysrc/omnibase_core/utils/util_singleton_holders.pytests/unit/models/core/test_model_cli_command_registry.pytests/fixtures/isolation.pysrc/omnibase_core/logging/emit.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage with pytest tests - unit tests go in tests/unit/, integration tests in tests/integration/
Run pytest tests with parallelization disabled (-n 0 -xvs) for debugging and use standard pytest run for CI
Use @pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests appropriately
Files:
tests/unit/fixtures/__init__.pytests/unit/enums/test_enum_scenario_status.pytests/unit/context/test_application_context.pytests/unit/fixtures/test_isolation_fixtures.pytests/unit/logging/test_core_logging.pytests/conftest.pytests/unit/mixins/test_mixin_hybrid_execution.pytests/unit/context/__init__.pytests/unit/models/core/test_model_cli_command_registry.pytests/fixtures/isolation.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use container.get_service('ProtocolName') for dependency resolution by protocol name, never by concrete class name
Files:
src/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/core/model_action_registry.pysrc/omnibase_core/logging/core_logging.pysrc/omnibase_core/models/core/model_cli_command_registry.pysrc/omnibase_core/models/container/model_onex_container.pysrc/omnibase_core/context/application_context.pysrc/omnibase_core/context/__init__.pysrc/omnibase_core/utils/util_singleton_holders.pysrc/omnibase_core/logging/emit.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for data validation and serialization - leverage Pydantic 2.11+ features for type safety
Files:
src/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/core/model_action_registry.pysrc/omnibase_core/models/core/model_cli_command_registry.pysrc/omnibase_core/models/container/model_onex_container.py
🧠 Learnings (6)
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to tests/**/*.py : Run pytest tests with parallelization disabled (-n 0 -xvs) for debugging and use standard pytest run for CI
Applied to files:
pyproject.toml
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to tests/**/*.py : Use pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests appropriately
Applied to files:
tests/conftest.py
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors, never use ModelContainer[T]
Applied to files:
src/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/core/model_action_registry.pysrc/omnibase_core/models/core/model_cli_command_registry.pysrc/omnibase_core/models/container/model_onex_container.pysrc/omnibase_core/context/application_context.py
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to **/*.py : Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Applied to files:
src/omnibase_core/models/security/model_secret_manager.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/core/model_cli_command_registry.py
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic models for data validation and serialization - leverage Pydantic 2.11+ features for type safety
Applied to files:
src/omnibase_core/models/core/model_event_type_registry.py
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/**/*.py : Use container.get_service('ProtocolName') for dependency resolution by protocol name, never by concrete class name
Applied to files:
src/omnibase_core/logging/emit.py
🧬 Code graph analysis (12)
tests/unit/context/test_application_context.py (1)
src/omnibase_core/context/application_context.py (8)
ApplicationContext(39-143)get_current_container(146-161)reset_container(191-208)run_with_container(212-235)run_with_container_sync(239-262)set_current_container(164-188)sync(131-143)container(74-76)
tests/unit/logging/test_core_logging.py (1)
tests/fixtures/isolation.py (1)
isolated_correlation_context(342-374)
tests/conftest.py (1)
tests/fixtures/isolation.py (5)
clear_caches_fixture(327-338)isolated_correlation_context(342-374)reset_all_singletons(265-284)reset_container_singleton(288-298)reset_registries(302-323)
src/omnibase_core/models/security/model_secret_manager.py (4)
src/omnibase_core/context/application_context.py (1)
container(74-76)src/omnibase_core/infrastructure/node_base.py (1)
container(752-760)src/omnibase_core/models/container/model_onex_container.py (3)
get_model_onex_container_sync(761-787)secret_manager(219-221)config(184-186)src/omnibase_core/container/service_registry.py (1)
config(104-106)
src/omnibase_core/models/core/model_action_registry.py (3)
src/omnibase_core/context/application_context.py (1)
container(74-76)src/omnibase_core/infrastructure/node_base.py (1)
container(752-760)src/omnibase_core/models/container/model_onex_container.py (2)
get_model_onex_container_sync(761-787)action_registry(204-206)
src/omnibase_core/logging/core_logging.py (4)
src/omnibase_core/context/application_context.py (1)
container(74-76)src/omnibase_core/models/container/model_onex_container.py (2)
get_model_onex_container_sync(761-787)get_service(478-484)src/omnibase_core/utils/util_bootstrap.py (1)
get_service(68-96)src/omnibase_core/utils/util_simple_fallback_logger.py (1)
_SimpleFallbackLogger(12-27)
src/omnibase_core/models/container/model_onex_container.py (2)
src/omnibase_core/context/application_context.py (3)
get_current_container(146-161)set_current_container(164-188)container(74-76)src/omnibase_core/infrastructure/node_base.py (2)
container(752-760)run(588-602)
src/omnibase_core/context/application_context.py (1)
src/omnibase_core/models/container/model_onex_container.py (1)
ModelONEXContainer(81-651)
src/omnibase_core/context/__init__.py (1)
src/omnibase_core/context/application_context.py (6)
ApplicationContext(39-143)get_current_container(146-161)reset_container(191-208)run_with_container(212-235)run_with_container_sync(239-262)set_current_container(164-188)
tests/unit/models/core/test_model_cli_command_registry.py (2)
src/omnibase_core/context/application_context.py (1)
get_current_container(146-161)src/omnibase_core/models/core/model_cli_command_registry.py (2)
get_global_command_registry(270-289)ModelCliCommandRegistry(27-267)
tests/fixtures/isolation.py (3)
src/omnibase_core/context/application_context.py (2)
container(74-76)get_current_container(146-161)src/omnibase_core/logging/core_logging.py (2)
clear_logger_cache(119-121)emit(74-83)src/omnibase_core/logging/emit.py (1)
clear_protocol_cache(484-487)
src/omnibase_core/logging/emit.py (5)
src/omnibase_core/context/application_context.py (1)
container(74-76)src/omnibase_core/infrastructure/node_base.py (1)
container(752-760)src/omnibase_core/models/container/model_onex_container.py (2)
ModelONEXContainer(81-651)get_service(478-484)src/omnibase_core/container/container_service_resolver.py (1)
get_service(60-150)src/omnibase_core/utils/util_bootstrap.py (1)
get_service(68-96)
🪛 Ruff (0.14.6)
tests/unit/enums/test_enum_scenario_status.py
278-278: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
tests/unit/context/test_application_context.py
154-154: Avoid specifying long messages outside the exception class
(TRY003)
221-221: Avoid specifying long messages outside the exception class
(TRY003)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
tests/unit/fixtures/test_isolation_fixtures.py
109-109: Unused method argument: reset_container_singleton
(ARG002)
113-113: Unused method argument: reset_all_singletons
(ARG002)
120-120: Unused method argument: isolated_correlation_context
(ARG002)
127-127: Unused method argument: reset_registries
(ARG002)
300-300: Unused method argument: reset_container_singleton
(ARG002)
307-307: Unused method argument: reset_all_singletons
(ARG002)
tests/unit/logging/test_core_logging.py
102-102: Unused method argument: isolated_correlation_context
(ARG002)
tests/conftest.py
234-234: Unused function argument: config
(ARG001)
tests/unit/mixins/test_mixin_hybrid_execution.py
126-126: Unpacked variable kwargs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
src/omnibase_core/models/core/model_event_type_registry.py
223-223: Consider moving this statement to an else block
(TRY300)
246-248: try-except-pass detected, consider logging the exception
(S110)
246-246: Do not catch blind exception: Exception
(BLE001)
src/omnibase_core/models/core/model_action_registry.py
261-261: Consider moving this statement to an else block
(TRY300)
284-286: try-except-pass detected, consider logging the exception
(S110)
284-284: Do not catch blind exception: Exception
(BLE001)
src/omnibase_core/logging/core_logging.py
113-113: Do not catch blind exception: Exception
(BLE001)
115-115: Do not catch blind exception: Exception
(BLE001)
src/omnibase_core/context/__init__.py
50-57: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
tests/fixtures/isolation.py
421-423: try-except-pass detected, consider logging the exception
(S110)
421-421: Do not catch blind exception: Exception
(BLE001)
429-430: try-except-pass detected, consider logging the exception
(S110)
429-429: Do not catch blind exception: Exception
(BLE001)
src/omnibase_core/logging/emit.py
472-472: Consider moving this statement to an else block
(TRY300)
473-473: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (3)
- GitHub Check: claude-review
- GitHub Check: Code Quality
- GitHub Check: Smoke Tests
🔇 Additional comments (44)
tests/unit/context/__init__.py (1)
1-1: LGTM!Standard package initializer for the new context test module.
pyproject.toml (1)
306-309: Approve the test distribution and rerun configuration changes.The switch to
--dist=loadgroupfor pytest-xdist and the addition of--reruns=2with delay aligns well with the PR's goal of improving test isolation during parallel execution.src/omnibase_core/utils/util_singleton_holders.py (1)
1-49: Well-documented deprecation with clear migration guidance.The comprehensive docstring provides excellent migration examples from legacy singleton holders to the new ApplicationContext-based patterns. Exporting nothing via
__all__ = []ensures consumers cannot accidentally use deprecated patterns while the module remains discoverable for documentation purposes.src/omnibase_core/models/core/model_event_type_registry.py (2)
205-229: Container-based registry access with proper error handling.The migration from singleton fallback to explicit DI container access with
ModelOnexErrorfollows coding guidelines. The auto-bootstrap behavior when the registry is empty maintains backward compatibility while enforcing proper initialization.
232-248: Silent no-op behavior in reset is appropriate for testing.The try-except-pass pattern here is intentional per the docstring ("Does nothing if container is not initialized"). This is a reasonable design for a reset function primarily used in test cleanup—failing silently when there's nothing to reset avoids test setup complexity.
tests/unit/logging/test_core_logging.py (1)
101-109: Correct use of fixture for test isolation.The
isolated_correlation_contextfixture parameter is intentionally "unused" in the function body—this is a standard pytest pattern where including the fixture as a parameter triggers its setup/teardown side effects. The Ruff ARG002 warning is a false positive in this context. The fixture ensures proper isolation by clearing any correlation ID from previous tests.src/omnibase_core/models/core/model_cli_command_registry.py (1)
270-289: Container-based registry access with structured error handling.The migration from singleton fallback to strict DI container access follows the coding guidelines—using
ModelOnexErrorwithEnumCoreErrorCode.CONFIGURATION_ERRORinstead of generic exceptions. The exception chaining (from e) preserves debugging context.This strict behavior (raise on failure rather than fallback) is appropriate for a registry access function where the consumer needs the registry to function correctly.
tests/unit/fixtures/__init__.py (1)
1-1: Minor typo in docstring.The docstring has a double period at the end: "Unit tests for fixtures.." → should be "Unit tests for fixtures."
-"""Unit tests for fixtures.""" +"""Unit tests for fixtures."""Wait, looking more closely, the issue is the extra period. Let me correct:
-"""Unit tests for fixtures..""" +"""Unit tests for fixtures."""Likely an incorrect or invalid review comment.
src/omnibase_core/logging/core_logging.py (3)
119-121: LGTM!The
clear_logger_cache()function is a clean addition for test isolation, allowing tests to reset the cached logger state.
124-126: LGTM!Clean simplification - the delegation to
_get_cached_logger()removes the previous double-checked locking complexity.
97-117: Based on my comprehensive investigation, I can now provide the verified review comment. The original review comment correctly identifies a real design flaw in how the logger caching interacts with the contextvars-based container isolation system.Do not use
lru_cachewith context-variable container—it defeats per-context isolation.The
@lru_cache(maxsize=1)decorator caches the logger instance globally (process-wide), butget_model_onex_container_sync()explicitly usescontextvarsto provide per-context containers. Different execution contexts (viarun_with_container()orApplicationContext) can have different containers, but the cached logger will always return the one from the first context, breaking isolation.The docstring claims "without global state," but
lru_cacheis exactly that—global state that ignores context boundaries.Recommended fixes:
- Remove the cache and accept minor performance cost—contexts need independent loggers
- Use a context-aware cache mechanism (e.g., dict keyed by container ID)
- If logger must be truly global (intentional singleton), remove the misleading docstring and add a clear comment explaining why context isolation is disabled
The existing
clear_logger_cache()function hints that this limitation is known; document it explicitly if global caching is intentional.src/omnibase_core/models/core/model_action_registry.py (1)
241-267: LGTM - Clean migration to DI container pattern.The change from fallback-to-singleton to raising
ModelOnexErrorwhen the container isn't initialized is a good design decision. It makes failures explicit rather than silently returning a potentially inconsistent registry.Minor note: The static analyzer suggests moving line 261 (
return registry) to anelseblock, but the current structure is clear and readable.tests/unit/context/test_application_context.py (7)
1-25: LGTM - Comprehensive test suite for ApplicationContext.Well-organized test module covering all aspects of the contextvars-based container management:
- Basic get/set/reset operations
- Sync and async context managers
- Exception handling and cleanup
- Nested contexts
- Thread isolation
- Async task inheritance and isolation
The imports are clean and the test structure follows good practices.
27-86: LGTM!Clean test implementation with proper context isolation using
contextvars.copy_context(). The reset pattern with token ensures no test pollution.
88-188: LGTM!Excellent coverage of sync context manager behavior including:
- Reset to previous value verification
- Normal exit cleanup
- Exception cleanup (critical for proper resource management)
- Nested context manager behavior
- The
sync()class method test
190-242: LGTM!Async context manager tests mirror the sync tests appropriately, with proper
@pytest.mark.asynciodecorators. Good coverage of async-specific behavior.
244-309: LGTM!Good tests for the
run_with_containerandrun_with_container_synchelper functions, verifying both the yielded value and cleanup behavior.
311-382: LGTM - Critical isolation tests.These tests verify the core PR objective: thread-safe and async-safe container isolation. The thread isolation test using
threading.Barrierfor synchronization is well-designed, and the async task tests correctly verify that child tasks inherit context but modifications don't propagate back to parents.
384-391: LGTM!Simple but necessary test for the
containerproperty accessor.tests/unit/models/core/test_model_cli_command_registry.py (3)
672-687: LGTM - Good verification of container-based registry access.The test correctly verifies that:
- Container is
Noneafter fixture reset- Getting the global registry creates a container
- Container is set in context after registry access
This validates the migration from singleton to DI container pattern.
688-694: LGTM!Simple but important test verifying that repeated calls return the same registry instance (singleton behavior within a context).
696-742: LGTM!Good integration tests verifying that module-level functions (
discover_commands_from_contracts,get_command_definition) properly use the container-based global registry.tests/conftest.py (3)
210-230: LGTM - Well-documented pytest markers for test isolation.The new markers provide clear semantics for test categorization:
isolated: fresh module stateno_shared_state: sensitive to global pollutionsingleton_sensitive: touches specific singletonsfresh_event_loop: needs isolated asyncio loopserial: runs after parallel tests completeGood documentation in the marker descriptions.
233-272: LGTM - Critical for parallel test execution.The separation of serial-marked tests to run last is essential for pytest-xdist compatibility. This ensures:
- Parallel tests run first without global state interference
- Serial tests run after parallel tests complete
- Consistent, deterministic test ordering within each group
The docstring clearly explains the rationale.
504-517: LGTM - Global fixture imports for test isolation.Importing these fixtures at the conftest level makes them available to all tests without explicit imports. This is the standard pytest pattern and aligns with the PR's goal of providing comprehensive test isolation support.
The imported fixtures cover:
- Cache clearing (
clear_caches_fixture)- Event loop isolation (
fresh_asyncio_loop)- Logging context isolation (
isolated_correlation_context)- Singleton/container reset (
reset_all_singletons,reset_container_singleton,reset_registries)src/omnibase_core/models/container/model_onex_container.py (3)
41-46: LGTM - Import of context management utilities.The imports correctly bring in the context-based container management functions that replace the legacy singleton holder pattern.
727-758: LGTM - Context-based async container access.The implementation correctly checks for an existing container in the current context before creating a new one. The comprehensive docstring explains the context isolation benefits across asyncio tasks, threads, and nested contexts.
761-787: LGTM - Well-documented sync container access with context awareness.The implementation correctly handles the nuance that
asyncio.run()creates a fresh context (line 780-781 comment). The solution of capturing the container and setting it in the caller's context afterasyncio.run()returns is the right approach to ensure context propagation works correctly.src/omnibase_core/logging/emit.py (2)
477-487: LGTM - TTL-based cache refresh mechanism.The TTL refresh logic is correct: when TTL is exceeded, the cache is cleared before calling the cached function, which then sets a fresh timestamp. The
clear_protocol_cache()function properly resets both the timestamp and the lru_cache for testing purposes.
765-766: LGTM - Clean integration of cached protocol services.The change simplifies the routing logic by delegating caching concerns to
get_protocol_services().src/omnibase_core/context/__init__.py (1)
1-39: LGTM - Well-documented module docstring with usage examples.The docstring provides clear examples for common use cases including token-based reset, async context managers, and sync context managers. This will help developers adopt the new context-based approach.
tests/unit/fixtures/test_isolation_fixtures.py (5)
5-9: Note: Test imports private_current_containerfor cleanup.Importing
_current_container(a private contextvar) is acceptable for test cleanup purposes, allowing tests to properly reset state via_current_container.reset(token)in finally blocks. This is a reasonable testing pattern.
26-88: LGTM - Comprehensive tests for SingletonResetContext.Tests cover the key scenarios: container reset to None inside context, legacy key filtering, proper restoration on exit, and the clear_caches parameter. The consistent use of try/finally with token-based cleanup is a good pattern.
106-135: LGTM - Fixture tests use standard pytest pattern.The unused method argument warnings (ARG002) are false positives. The fixture parameters (
reset_container_singleton,reset_all_singletons, etc.) trigger pytest to execute those fixtures before the test runs. The tests then verify the expected state after fixture execution.
137-217: LGTM - Good coverage of utility functions and legacy key handling.Tests properly verify:
get_all_holder_classes()returns empty dict (confirming migration)reset_singleton_by_key()works for container and returns True for legacy keysis_legacy_holder_key()correctly identifies all legacy keysLEGACY_HOLDER_KEYScontains all expected keys
220-311: LGTM - Thorough application context tests.The TestApplicationContext class covers:
- Basic reset functionality
- Idempotent reset behavior
- SingletonResetContext integration
- Proper restoration on context exit
- clear_all_caches effect on context
- Fixture integration tests
As per coding guidelines, these tests properly use pytest fixtures and markers for unit tests.
src/omnibase_core/context/application_context.py (3)
1-36: Well-structured module with proper contextvars usage.The module docstring clearly documents the thread-safety guarantees and benefits of the contextvars approach. The TYPE_CHECKING guard for the ModelONEXContainer import avoids circular dependencies while maintaining type safety.
39-143: Solid dual-protocol context manager implementation.The class correctly implements both sync and async context manager protocols with proper token-based cleanup. The
sync()classmethod is semantically redundant (since the instance already supports__enter__/__exit__), but it provides clear API intent for users and aligns with the documented usage pattern.
211-262: Context manager helpers provide convenient functional API.The
run_with_containerandrun_with_container_syncfunctions correctly use try/finally to ensure cleanup. They provide a functional alternative to the class-basedApplicationContext, which is useful for simpler use cases.tests/fixtures/isolation.py (4)
97-126: Cache clearing implementation looks correct.The function appropriately uses try/except to handle cases where modules may not be available. The discarded token from
reset_application_context()is intentional since this function is meant for one-way clearing, not save/restore semantics.
341-374: Correlation context isolation uses internal API.The fixture correctly saves and restores the correlation ID state. Accessing
core_logging._contextdirectly is necessary for test isolation but couples this fixture to internal implementation details.Consider adding a comment noting this dependency, so maintainers know to update this fixture if the logging internals change.
264-339: Well-designed pytest fixtures with clear documentation.The fixtures are appropriately scoped and documented with usage examples. The
reset_registriesfixture correctly notes the migration from individual holder resets to container-based access.
465-536: Utility functions provide clean backward compatibility.The
get_all_holder_classes()returning an empty dict andis_legacy_holder_key()checking against the frozen set provide backward compatibility while documenting the migration away from singleton holders.tests/unit/enums/test_enum_scenario_status.py (1)
268-282: The importlib workaround appears unnecessary for the actual test behavior.Standard Python enums are singletons within a process, and
pickle.dumps()/pickle.loads()automatically preserves enum identity without any special handling. Theimportlib.import_module()call returns the cached module fromsys.modules—the same object reference as the top-levelEnumScenarioStatusimport—so it provides no functional difference.Additionally, the test has no isolation markers or fixtures applied to it (confirmed in conftest.py), so no module reloading occurs that would require this workaround. The comment about fixing "pytest-xdist worker process module caching" misrepresents how the code works:
importlib.import_module()within one worker's process cannot affect another worker's separatesys.modules.Recommendation:
- Remove the importlib workaround and verify the test still passes with direct iteration over
EnumScenarioStatus- Update the comment if the workaround is retained for defensive purposes, clarifying that it doesn't actually address cross-worker pytest-xdist issues
- The identity assertion on line 281 is correct and should be kept—it validates that pickle preserves enum singleton semantics
Note on static analysis: The Ruff warning about
pickle.loads()on line 278 is acceptable in unit tests with controlled input data.
| @lru_cache(maxsize=1) | ||
| def _get_protocol_services_cached() -> tuple[Any, Any]: | ||
| """Get formatter and output handler with caching.""" | ||
| _protocol_cache_state[0] = time.time() | ||
|
|
||
| try: | ||
| from omnibase_core.models.container.model_onex_container import ( | ||
| ModelONEXContainer, | ||
| ) | ||
| from omnibase_core.nodes.node_logger.protocols.protocol_context_aware_output_handler import ( | ||
| ProtocolContextAwareOutputHandler, | ||
| ) | ||
| from omnibase_core.nodes.node_logger.protocols.protocol_smart_log_formatter import ( | ||
| ProtocolSmartLogFormatter, | ||
| ) | ||
|
|
||
| container = ModelONEXContainer() | ||
| formatter = container.get_service(ProtocolSmartLogFormatter) | ||
| output_handler = container.get_service(ProtocolContextAwareOutputHandler) | ||
| return (formatter, output_handler) | ||
| except Exception: # fallback-ok: protocol services init must never fail | ||
| return (None, None) |
There was a problem hiding this comment.
Use context-based container accessor instead of creating new container.
Line 469 creates a new ModelONEXContainer() directly, bypassing the context-based container management that this PR introduces. This could lead to inconsistent container instances across the codebase.
Apply this diff to use the context-based accessor:
@lru_cache(maxsize=1)
def _get_protocol_services_cached() -> tuple[Any, Any]:
"""Get formatter and output handler with caching."""
_protocol_cache_state[0] = time.time()
try:
from omnibase_core.models.container.model_onex_container import (
- ModelONEXContainer,
+ get_model_onex_container_sync,
)
from omnibase_core.nodes.node_logger.protocols.protocol_context_aware_output_handler import (
ProtocolContextAwareOutputHandler,
)
from omnibase_core.nodes.node_logger.protocols.protocol_smart_log_formatter import (
ProtocolSmartLogFormatter,
)
- container = ModelONEXContainer()
+ container = get_model_onex_container_sync()
formatter = container.get_service(ProtocolSmartLogFormatter)
output_handler = container.get_service(ProtocolContextAwareOutputHandler)
return (formatter, output_handler)
except Exception: # fallback-ok: protocol services init must never fail
return (None, None)🧰 Tools
🪛 Ruff (0.14.6)
472-472: Consider moving this statement to an else block
(TRY300)
473-473: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In src/omnibase_core/logging/emit.py around lines 453 to 474, the code currently
instantiates ModelONEXContainer() directly which bypasses the PR-introduced
context-based container management; replace the direct instantiation with the
context-based container accessor introduced in this PR (import the accessor
function that returns the current ModelONEXContainer from the appropriate
module) and use that returned container to call get_service for
ProtocolSmartLogFormatter and ProtocolContextAwareOutputHandler, keeping the
existing try/except fallback behavior if the accessor returns None or raises.
| try: | ||
| container = get_model_onex_container_sync() | ||
| return cast(ModelSecretManager, container.secret_manager()) | ||
| except ( | ||
| Exception | ||
| ): # fallback-ok: DI container unavailable during bootstrap or circular dependency scenarios | ||
| # Fallback to singleton holder | ||
| manager = _SecretManagerHolder.get() | ||
| if manager is None: | ||
| manager = ModelSecretManager() | ||
| _SecretManagerHolder.set(manager) | ||
| return manager | ||
| # Type narrowing: manager is now guaranteed to be ModelSecretManager | ||
| return cast(ModelSecretManager, manager) | ||
| except Exception as e: | ||
| raise ModelOnexError( | ||
| message="DI container not initialized - cannot get secret manager. " | ||
| "Call init_secret_manager() first.", | ||
| error_code=EnumCoreErrorCode.CONFIGURATION_ERROR, | ||
| ) from e |
There was a problem hiding this comment.
Verify secret_manager access pattern - property vs method call.
Line 397 calls container.secret_manager() as a method, but the relevant code snippet at src/omnibase_core/models/container/model_onex_container.py:218-220 shows secret_manager is a property, not a method.
Apply this diff to fix the property access:
try:
container = get_model_onex_container_sync()
- return cast(ModelSecretManager, container.secret_manager())
+ return cast(ModelSecretManager, container.secret_manager)
except Exception as e:🤖 Prompt for AI Agents
In src/omnibase_core/models/security/model_secret_manager.py around lines 395 to
403, the code calls container.secret_manager() but secret_manager is defined as
a property in model_onex_container.py; change the call to access the property
(container.secret_manager) instead of invoking it, keep the cast to
ModelSecretManager and preserve the existing exception handling and error
message.
| def init_secret_manager(config: ModelSecretConfig) -> ModelSecretManager: | ||
| """Initialize global secret manager with custom config (now via DI container).""" | ||
| """Initialize global secret manager with custom config via DI container. | ||
|
|
||
| Args: | ||
| config: The secret management configuration | ||
|
|
||
| Returns: | ||
| The initialized ModelSecretManager instance | ||
|
|
||
| Raises: | ||
| ModelOnexError: If DI container is not initialized | ||
| """ | ||
| from omnibase_core.models.container.model_onex_container import ( | ||
| get_model_onex_container_sync, | ||
| ) | ||
|
|
||
| manager = ModelSecretManager(config=config) | ||
| # Create the manager instance (for validation and potential future use) | ||
| _ = ModelSecretManager(config=config) | ||
|
|
||
| try: | ||
| # Try to set in container | ||
| container = get_model_onex_container_sync() | ||
| # Container manages its own instance, but we update holder as fallback | ||
| _SecretManagerHolder.set(manager) | ||
| result: ModelSecretManager = container.secret_manager() | ||
| return result | ||
| except ( | ||
| Exception | ||
| ): # fallback-ok: DI container unavailable during bootstrap or circular dependency scenarios | ||
| # Fallback to singleton holder | ||
| _SecretManagerHolder.set(manager) | ||
| return manager | ||
| return cast(ModelSecretManager, container.secret_manager()) | ||
| except Exception as e: | ||
| raise ModelOnexError( | ||
| message="DI container not initialized - cannot initialize secret manager.", | ||
| error_code=EnumCoreErrorCode.CONFIGURATION_ERROR, | ||
| ) from e |
There was a problem hiding this comment.
Provided config is validated but never used - API behavior is misleading.
The function creates a ModelSecretManager with the provided config (line 423) for validation purposes only, then discards it and returns the container's own secret manager instance. The caller's config is never actually applied to the container's secret manager.
This makes the function signature misleading - callers expect their config to be used. Consider either:
- Documenting that the config is only validated (not applied), or
- Actually using the config to configure the container's secret manager
🤖 Prompt for AI Agents
In src/omnibase_core/models/security/model_secret_manager.py around lines 406 to
432, the function currently constructs a ModelSecretManager with the provided
config only for validation and then returns the container's existing
secret_manager, so the caller's config is never applied; instead instantiate the
ModelSecretManager with the provided config and register/override that instance
in the DI container before returning it (or, if the container only supports
providers, update the container binding/provider to return a ModelSecretManager
constructed with the given config), preserving the existing try/except behavior
and raising ModelOnexError on DI initialization failures.
| def init_secret_manager_from_manager(manager: ModelSecretManager) -> ModelSecretManager: | ||
| """Initialize global secret manager from existing manager instance (now via DI container).""" | ||
| """Initialize global secret manager from existing manager instance via DI container. | ||
|
|
||
| Note: The provided manager is validated but the DI container manages its own singleton. | ||
| The container's secret_manager instance is returned. | ||
|
|
||
| Args: | ||
| manager: An existing ModelSecretManager instance (used for validation) | ||
|
|
||
| Returns: | ||
| The DI container's ModelSecretManager instance | ||
|
|
||
| Raises: | ||
| ModelOnexError: If DI container is not initialized | ||
| """ | ||
| from omnibase_core.models.container.model_onex_container import ( | ||
| get_model_onex_container_sync, | ||
| ) | ||
|
|
||
| # Validate the manager by accessing it (ensures it's a valid instance) | ||
| _ = manager.config | ||
|
|
||
| try: | ||
| # Try to set in container | ||
| container = get_model_onex_container_sync() | ||
| # Container manages its own instance, but we update holder as fallback | ||
| _SecretManagerHolder.set(manager) | ||
| result: ModelSecretManager = container.secret_manager() | ||
| return result | ||
| except ( | ||
| Exception | ||
| ): # fallback-ok: DI container unavailable during bootstrap or circular dependency scenarios | ||
| # Fallback to singleton holder | ||
| _SecretManagerHolder.set(manager) | ||
| return manager | ||
| return cast(ModelSecretManager, container.secret_manager()) | ||
| except Exception as e: | ||
| raise ModelOnexError( | ||
| message="DI container not initialized - cannot initialize secret manager.", | ||
| error_code=EnumCoreErrorCode.CONFIGURATION_ERROR, | ||
| ) from e |
There was a problem hiding this comment.
Same issues: property access and misleading API.
This function has the same two issues as init_secret_manager:
- Line 459 calls
container.secret_manager()as a method instead of property access - The provided
manageris only validated but never used - the container's own instance is returned
try:
container = get_model_onex_container_sync()
- return cast(ModelSecretManager, container.secret_manager())
+ return cast(ModelSecretManager, container.secret_manager)
except Exception as e:📝 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.
| def init_secret_manager_from_manager(manager: ModelSecretManager) -> ModelSecretManager: | |
| """Initialize global secret manager from existing manager instance (now via DI container).""" | |
| """Initialize global secret manager from existing manager instance via DI container. | |
| Note: The provided manager is validated but the DI container manages its own singleton. | |
| The container's secret_manager instance is returned. | |
| Args: | |
| manager: An existing ModelSecretManager instance (used for validation) | |
| Returns: | |
| The DI container's ModelSecretManager instance | |
| Raises: | |
| ModelOnexError: If DI container is not initialized | |
| """ | |
| from omnibase_core.models.container.model_onex_container import ( | |
| get_model_onex_container_sync, | |
| ) | |
| # Validate the manager by accessing it (ensures it's a valid instance) | |
| _ = manager.config | |
| try: | |
| # Try to set in container | |
| container = get_model_onex_container_sync() | |
| # Container manages its own instance, but we update holder as fallback | |
| _SecretManagerHolder.set(manager) | |
| result: ModelSecretManager = container.secret_manager() | |
| return result | |
| except ( | |
| Exception | |
| ): # fallback-ok: DI container unavailable during bootstrap or circular dependency scenarios | |
| # Fallback to singleton holder | |
| _SecretManagerHolder.set(manager) | |
| return manager | |
| return cast(ModelSecretManager, container.secret_manager()) | |
| except Exception as e: | |
| raise ModelOnexError( | |
| message="DI container not initialized - cannot initialize secret manager.", | |
| error_code=EnumCoreErrorCode.CONFIGURATION_ERROR, | |
| ) from e | |
| def init_secret_manager_from_manager(manager: ModelSecretManager) -> ModelSecretManager: | |
| """Initialize global secret manager from existing manager instance via DI container. | |
| Note: The provided manager is validated but the DI container manages its own singleton. | |
| The container's secret_manager instance is returned. | |
| Args: | |
| manager: An existing ModelSecretManager instance (used for validation) | |
| Returns: | |
| The DI container's ModelSecretManager instance | |
| Raises: | |
| ModelOnexError: If DI container is not initialized | |
| """ | |
| from omnibase_core.models.container.model_onex_container import ( | |
| get_model_onex_container_sync, | |
| ) | |
| # Validate the manager by accessing it (ensures it's a valid instance) | |
| _ = manager.config | |
| try: | |
| container = get_model_onex_container_sync() | |
| return cast(ModelSecretManager, container.secret_manager) | |
| except Exception as e: | |
| raise ModelOnexError( | |
| message="DI container not initialized - cannot initialize secret manager.", | |
| error_code=EnumCoreErrorCode.CONFIGURATION_ERROR, | |
| ) from e |
🤖 Prompt for AI Agents
In src/omnibase_core/models/security/model_secret_manager.py around lines
435-464, fix two issues: (1) stop calling container.secret_manager() as a method
and access it as a property (container.secret_manager) and (2) make the
function’s behavior explicit about the provided manager — either register the
passed-in manager into the DI container before returning (e.g., use the
container's setter/registration API or assign container.secret_manager = manager
if supported) or, if the intended behavior is only validation, change the
API/name to reflect that and return the validated manager; implement the chosen
approach consistently and preserve the existing exception handling.
| def __enter__(self) -> SingletonResetContext: | ||
| """Enter context: save state, reset context, and clear caches.""" | ||
| # Save and reset container context | ||
| if self._should_reset_container: | ||
| self._save_and_reset_context() | ||
|
|
||
| # Clear lru_cache caches if requested | ||
| if self._clear_caches: | ||
| clear_all_caches() | ||
|
|
||
| return self |
There was a problem hiding this comment.
Potential double reset of application context.
When _should_reset_container=True and clear_caches=True (the defaults), _save_and_reset_context() resets the container, then clear_all_caches() also calls reset_application_context(). This results in two resets, discarding the first token.
This doesn't cause incorrect behavior, but the first token (saved in _saved_state) becomes invalid for restoration since clear_all_caches() creates a new token that's discarded.
Consider reordering or conditionally skipping the reset in clear_all_caches:
def __enter__(self) -> SingletonResetContext:
"""Enter context: save state, reset context, and clear caches."""
+ # Clear caches first (without resetting container context)
+ if self._clear_caches:
+ # Only clear logger and protocol caches, not container
+ try:
+ from omnibase_core.logging.core_logging import clear_logger_cache
+ clear_logger_cache()
+ except ImportError:
+ pass
+ try:
+ from omnibase_core.logging.emit import clear_protocol_cache
+ clear_protocol_cache()
+ except ImportError:
+ pass
+
# Save and reset container context
if self._should_reset_container:
self._save_and_reset_context()
-
- # Clear lru_cache caches if requested
- if self._clear_caches:
- clear_all_caches()Or modify clear_all_caches to accept a skip_context_reset parameter.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/fixtures/isolation.py around lines 236–246: currently __enter__ calls
_save_and_reset_context() and then clear_all_caches(), which causes
reset_application_context() to run twice and discards the first saved token;
change the logic to avoid the double reset by either (A) calling
clear_all_caches(skip_context_reset=True) when _should_reset_container is true
(add the skip_context_reset parameter to clear_all_caches and have it bypass
reset_application_context), or (B) reorder operations to call clear_all_caches()
before _save_and_reset_context(), ensuring only a single reset occurs; implement
one of these two fixes and update the clear_all_caches signature/usages if you
choose option A.
| # Save the current event loop if any | ||
| try: | ||
| old_loop = asyncio.get_event_loop() | ||
| old_loop_running = old_loop.is_running() | ||
| except RuntimeError: | ||
| old_loop = None | ||
| old_loop_running = False | ||
|
|
||
| # Create fresh loop | ||
| new_loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(new_loop) | ||
|
|
||
| try: | ||
| yield new_loop | ||
| finally: | ||
| # Clean up the new loop | ||
| try: | ||
| # Cancel all pending tasks | ||
| pending = asyncio.all_tasks(new_loop) | ||
| for task in pending: | ||
| task.cancel() | ||
|
|
||
| # Give tasks a chance to handle cancellation | ||
| if pending: | ||
| new_loop.run_until_complete( | ||
| asyncio.gather(*pending, return_exceptions=True) | ||
| ) | ||
|
|
||
| new_loop.close() | ||
| except Exception: | ||
| # Best effort cleanup | ||
| pass | ||
|
|
||
| # Restore old loop if it wasn't running | ||
| if old_loop is not None and not old_loop_running: | ||
| try: | ||
| asyncio.set_event_loop(old_loop) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Address deprecated asyncio API and bare exception handling.
-
asyncio.get_event_loop()without a running loop is deprecated since Python 3.10. Consider usingasyncio.get_running_loop()with appropriate fallback. -
The bare
except Exceptionblocks (lines 421, 429) flagged by static analysis should at minimum log the exception for debugging parallel test issues.
# Save the current event loop if any
try:
- old_loop = asyncio.get_event_loop()
- old_loop_running = old_loop.is_running()
+ old_loop = asyncio.get_running_loop()
+ old_loop_running = True
except RuntimeError:
- old_loop = None
- old_loop_running = False
+ try:
+ old_loop = asyncio.get_event_loop_policy().get_event_loop()
+ old_loop_running = old_loop.is_running()
+ except RuntimeError:
+ old_loop = None
+ old_loop_running = FalseFor the cleanup blocks:
- except Exception:
- # Best effort cleanup
- pass
+ except Exception as e:
+ # Best effort cleanup - log for debugging
+ import logging
+ logging.getLogger(__name__).debug(f"Event loop cleanup error: {e}")🧰 Tools
🪛 Ruff (0.14.6)
421-423: try-except-pass detected, consider logging the exception
(S110)
421-421: Do not catch blind exception: Exception
(BLE001)
429-430: try-except-pass detected, consider logging the exception
(S110)
429-429: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In tests/fixtures/isolation.py around lines 392 to 430, replace the deprecated
use of asyncio.get_event_loop() with a call to asyncio.get_running_loop() inside
a try/except to detect a running loop and fall back to setting old_loop to None
if no running loop exists; also stop using bare "except Exception" in the two
cleanup blocks — catch Exception as e and record the error using the module
logger (or a test logger) so failures are visible (ensure you import or obtain a
logger); keep the existing best-effort cleanup behavior but log the exception
details before continuing.
- Remove primitives/ re-export module (use models.primitives directly) - Remove events/ re-export module (use models.events directly) - Remove models/nodes/ re-export module (use models.core directly) - Move service models from node_services/ to service/ - Remove ephemeral docs from project root - Update all imports to use direct paths - Add generics support to ModelMetricValue
Pull Request Review: Context-Based Container ManagementOverall Assessment: Strong architectural improvement with excellent test coverage and documentation. A few items need addressing before merge. SummaryThis PR migrates from a singleton holder pattern to contextvars-based container management, providing proper isolation for parallel test execution with pytest-xdist. This is a solid architectural improvement that addresses real test isolation issues. Changes: 67 files (+2,148 / -951 lines) Strengths1. Excellent Architecture
2. Comprehensive Test Coverage
3. Proper Error Handling
4. Test Infrastructure Improvements
Critical Issues1. Incomplete Event Loop CleanupFile: Silent exception swallowing in cleanup can mask serious issues. Recommend at minimum logging the exception rather than silent 2. Import Cycle RiskFile: The Recommendation: Add explicit documentation about initialization order. 3. Missing CI Status VerificationBefore merging, verify all CI checks pass, especially:
Code Quality Concerns1. Deletion of Verification ReportsFiles Deleted:
Question: Were these temporary artifacts or important audit trails? If audit records, consider moving to 2. Large Fixture FileFile: While well-organized, consider splitting into:
3. Documentation Path ChangesMultiple doc files updated import paths. Is this a breaking change for external users? Verify backward compatibility or update CHANGELOG.md Security & PerformanceSecurity: No Issues Detected
Performance: Improvements Expected
Thread Safety: Properly Addressed
Testing Recommendations1. Verify CI PerformancePer CLAUDE.md benchmarks:
2. Run Full Test Suite Locallypoetry run pytest tests/ --cov=src/omnibase_core --cov-report=term-missing
poetry run pytest tests/ -n 4
poetry run pytest tests/ -n 4 -p no:rerunfailures3. Verify mypy Compliancepoetry run mypy src/omnibase_core/4. Test Isolation Verificationpoetry run pytest tests/unit/context/ --random-order
poetry run pytest tests/unit/fixtures/ --random-orderDocumentation Needs1. Update CLAUDE.mdAdd section on ApplicationContext usage patterns 2. Migration GuideCreate
3. CHANGELOG.md EntryDocument breaking changes, additions, and fixes Best Practices FollowedONEX Standards Compliance:
Python Best Practices:
Testing Best Practices:
Metrics
RecommendationStatus: APPROVE WITH CONDITIONS Before Merge:
After Merge:
Future Enhancements (Optional)
Great work on this refactor! The architecture is solid and addresses real problems. Just need to verify CI status and address the critical items above. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/omnibase_core/models/discovery/model_custommetrics.py (1)
28-41: PotentialUnboundLocalErrorif value type is unexpected.If
valuesomehow doesn't match any of theisinstancechecks (e.g.,Noneat runtime despite type hints),metric_typewill be unbound when constructingModelMetricValue. Consider adding a fallback or raising a clear error.elif isinstance(value, float): metric_type = "float" + else: + raise TypeError(f"Unsupported metric value type: {type(value).__name__}") metrics.append(docs/architecture/architecture-research/RESEARCH_REPORT_4_NODE_ARCHITECTURE.md (1)
176-188: Update the import statement to use the full module pathThe example import is incorrect. Throughout the codebase,
ModelServiceEffectis imported using the full module path:from omnibase_core.models.service.model_service_effect import ModelServiceEffectThe package-level import shown in the example does not work since
ModelServiceEffectis not re-exported inomnibase_core.models.service.__init__.py. Update line 184 to match the correct import pattern used consistently across all tests and source files.
🧹 Nitpick comments (2)
src/omnibase_core/models/discovery/model_custom_metrics.py (1)
18-18: Sort__all__for consistency.Static analysis (ruff RUF022) flags that
__all__is not sorted. Apply isort-style sorting for consistency with project conventions.-__all__ = ["ModelCustomMetrics", "ModelMetricValue", "AnyMetricValue", "MetricValueT"] +__all__ = ["AnyMetricValue", "MetricValueT", "ModelCustomMetrics", "ModelMetricValue"]docs/reference/api/nodes.md (1)
149-197: Add language identifiers to service wrapper code fences to satisfy markdownlintmarkdownlint is flagging these fenced blocks for missing languages (MD040):
- The four service wrapper examples under “ModelServiceCompute/Effect/Reducer/Orchestrator”.
Recommend marking them as Python:
-``` +```python from omnibase_core.models.service.model_service_compute import ModelServiceCompute ... -``` +```python from omnibase_core.models.service.model_service_effect import ModelServiceEffect ... -``` +```python from omnibase_core.models.service.model_service_reducer import ModelServiceReducer ... -``` +```python from omnibase_core.models.service.model_service_orchestrator import ModelServiceOrchestrator ... -```This will clear the MD040 warnings and improves syntax highlighting without changing content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
PR88_GROUP_1A_VERIFICATION_REPORT.md(0 hunks)VERIFICATION_REPORT.md(0 hunks)docs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.md(1 hunks)docs/architecture/architecture-research/RESEARCH_REPORT_4_NODE_ARCHITECTURE.md(1 hunks)docs/architecture/overview.md(1 hunks)docs/guides/VERSION_FIELD_AUTOMATION_STRATEGY.md(3 hunks)docs/guides/node-building/01_WHAT_IS_A_NODE.md(1 hunks)docs/guides/node-building/02_NODE_TYPES.md(1 hunks)docs/reference/SERVICE_WRAPPERS.md(7 hunks)docs/reference/api/nodes.md(5 hunks)scripts/fix_string_id_violations.py(1 hunks)scripts/fix_version_field_pattern1.sh(2 hunks)src/omnibase_core/events/__init__.py(0 hunks)src/omnibase_core/infrastructure/infrastructure_bases.py(1 hunks)src/omnibase_core/models/core/__init__.py(1 hunks)src/omnibase_core/models/core/model_monitoring_metrics.py(2 hunks)src/omnibase_core/models/discovery/model_custom_metrics.py(1 hunks)src/omnibase_core/models/discovery/model_custommetrics.py(2 hunks)src/omnibase_core/models/discovery/model_metric_value.py(2 hunks)src/omnibase_core/models/nodes/__init__.py(0 hunks)src/omnibase_core/models/nodes/node_services/__init__.py(0 hunks)src/omnibase_core/models/registry/model_registry_health_report.py(1 hunks)src/omnibase_core/models/service/model_service_compute.py(1 hunks)src/omnibase_core/models/service/model_service_effect.py(1 hunks)src/omnibase_core/models/service/model_service_orchestrator.py(1 hunks)src/omnibase_core/models/service/model_service_reducer.py(1 hunks)src/omnibase_core/primitives/__init__.py(0 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_health.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_integration.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_invocation.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_effect_health.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_effect_integration.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_effect_lifecycle.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_orchestrator_integration.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_orchestrator_invocation.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_reducer_health.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_reducer_integration.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_reducer_invocation.py(1 hunks)tests/unit/models/nodes/node_services/test_model_service_reducer_lifecycle.py(1 hunks)tests/unit/nodes/test_retry_count_tracking.py(1 hunks)tests/unit/primitives/test_model_semver.py(1 hunks)
💤 Files with no reviewable changes (6)
- src/omnibase_core/events/init.py
- VERIFICATION_REPORT.md
- src/omnibase_core/models/nodes/init.py
- src/omnibase_core/primitives/init.py
- src/omnibase_core/models/nodes/node_services/init.py
- PR88_GROUP_1A_VERIFICATION_REPORT.md
✅ Files skipped from review due to trivial changes (6)
- tests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py
- scripts/fix_string_id_violations.py
- tests/unit/models/nodes/node_services/test_model_service_reducer_integration.py
- tests/unit/models/nodes/node_services/test_model_service_effect_health.py
- tests/unit/models/nodes/node_services/test_model_service_effect_lifecycle.py
- docs/guides/node-building/02_NODE_TYPES.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution - never use pip or python directly
Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Use @standard_error_handling decorator on async functions to eliminate boilerplate try/catch logic
Use duck typing with protocols instead of isinstance checks - protocols define contracts through interfaces, not runtime type checking
Always use 100% strict mypy type checking - all functions must have type annotations and no untyped definitions are allowed
Run code through black formatter, isort for import sorting, and ruff linter before committing
Use async/await pattern with asyncio for asynchronous operations - never use threading for I/O operations
Use ModelEventEnvelope for inter-service event-driven communication and process event payloads through envelope pattern
Files:
tests/unit/models/nodes/node_services/test_model_service_orchestrator_integration.pytests/unit/models/nodes/node_services/test_model_service_effect_integration.pytests/unit/models/nodes/node_services/test_model_service_compute_health.pysrc/omnibase_core/models/core/__init__.pytests/unit/models/nodes/node_services/test_model_service_compute_integration.pysrc/omnibase_core/models/core/model_monitoring_metrics.pytests/unit/models/nodes/node_services/test_model_service_reducer_health.pysrc/omnibase_core/models/service/model_service_compute.pysrc/omnibase_core/models/service/model_service_orchestrator.pysrc/omnibase_core/infrastructure/infrastructure_bases.pysrc/omnibase_core/models/registry/model_registry_health_report.pytests/unit/models/nodes/node_services/test_model_service_reducer_invocation.pytests/unit/nodes/test_retry_count_tracking.pytests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.pytests/unit/primitives/test_model_semver.pytests/unit/models/nodes/node_services/test_model_service_effect_invocation.pytests/unit/models/nodes/node_services/test_model_service_reducer_lifecycle.pysrc/omnibase_core/models/discovery/model_custom_metrics.pysrc/omnibase_core/models/service/model_service_reducer.pysrc/omnibase_core/models/discovery/model_custommetrics.pysrc/omnibase_core/models/service/model_service_effect.pytests/unit/models/nodes/node_services/test_model_service_compute_invocation.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_invocation.pysrc/omnibase_core/models/discovery/model_metric_value.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Maintain minimum 60% code coverage with pytest tests - unit tests go in tests/unit/, integration tests in tests/integration/
Run pytest tests with parallelization disabled (-n 0 -xvs) for debugging and use standard pytest run for CI
Use @pytest.mark decorators (unit, integration, slow, smoke, performance) to categorize tests appropriately
Files:
tests/unit/models/nodes/node_services/test_model_service_orchestrator_integration.pytests/unit/models/nodes/node_services/test_model_service_effect_integration.pytests/unit/models/nodes/node_services/test_model_service_compute_health.pytests/unit/models/nodes/node_services/test_model_service_compute_integration.pytests/unit/models/nodes/node_services/test_model_service_reducer_health.pytests/unit/models/nodes/node_services/test_model_service_reducer_invocation.pytests/unit/nodes/test_retry_count_tracking.pytests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.pytests/unit/primitives/test_model_semver.pytests/unit/models/nodes/node_services/test_model_service_effect_invocation.pytests/unit/models/nodes/node_services/test_model_service_reducer_lifecycle.pytests/unit/models/nodes/node_services/test_model_service_compute_invocation.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_invocation.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use container.get_service('ProtocolName') for dependency resolution by protocol name, never by concrete class name
Files:
src/omnibase_core/models/core/__init__.pysrc/omnibase_core/models/core/model_monitoring_metrics.pysrc/omnibase_core/models/service/model_service_compute.pysrc/omnibase_core/models/service/model_service_orchestrator.pysrc/omnibase_core/infrastructure/infrastructure_bases.pysrc/omnibase_core/models/registry/model_registry_health_report.pysrc/omnibase_core/models/discovery/model_custom_metrics.pysrc/omnibase_core/models/service/model_service_reducer.pysrc/omnibase_core/models/discovery/model_custommetrics.pysrc/omnibase_core/models/service/model_service_effect.pysrc/omnibase_core/models/discovery/model_metric_value.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use Pydantic models for data validation and serialization - leverage Pydantic 2.11+ features for type safety
Files:
src/omnibase_core/models/core/__init__.pysrc/omnibase_core/models/core/model_monitoring_metrics.pysrc/omnibase_core/models/service/model_service_compute.pysrc/omnibase_core/models/service/model_service_orchestrator.pysrc/omnibase_core/models/registry/model_registry_health_report.pysrc/omnibase_core/models/discovery/model_custom_metrics.pysrc/omnibase_core/models/service/model_service_reducer.pysrc/omnibase_core/models/discovery/model_custommetrics.pysrc/omnibase_core/models/service/model_service_effect.pysrc/omnibase_core/models/discovery/model_metric_value.py
🧠 Learnings (7)
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors, never use ModelContainer[T]
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_orchestrator_integration.pydocs/guides/node-building/01_WHAT_IS_A_NODE.mddocs/architecture/overview.mddocs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.mdtests/unit/models/nodes/node_services/test_model_service_compute_health.pytests/unit/models/nodes/node_services/test_model_service_compute_integration.pytests/unit/models/nodes/node_services/test_model_service_reducer_health.pysrc/omnibase_core/models/service/model_service_compute.pysrc/omnibase_core/models/service/model_service_orchestrator.pysrc/omnibase_core/infrastructure/infrastructure_bases.pytests/unit/nodes/test_retry_count_tracking.pytests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.pydocs/reference/api/nodes.mdtests/unit/models/nodes/node_services/test_model_service_reducer_lifecycle.pysrc/omnibase_core/models/service/model_service_reducer.pysrc/omnibase_core/models/service/model_service_effect.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_invocation.pydocs/reference/SERVICE_WRAPPERS.md
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use mixin classes (MixinDiscoveryResponder, MixinEventHandler, etc.) to add reusable behavior to nodes
Applied to files:
docs/architecture/overview.mdtests/unit/models/nodes/node_services/test_model_service_reducer_health.pysrc/omnibase_core/infrastructure/infrastructure_bases.pydocs/reference/api/nodes.mdtests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_invocation.py
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Implement EFFECT, COMPUTE, REDUCER, or ORCHESTRATOR node types following the four-node architecture pattern - data flows unidirectionally left to right
Applied to files:
docs/architecture/overview.mdtests/unit/models/nodes/node_services/test_model_service_effect_integration.pydocs/architecture/architecture-research/RESEARCH_REPORT_4_NODE_ARCHITECTURE.mdsrc/omnibase_core/infrastructure/infrastructure_bases.pydocs/reference/api/nodes.mdsrc/omnibase_core/models/service/model_service_effect.pydocs/reference/SERVICE_WRAPPERS.md
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Pydantic models for data validation and serialization - leverage Pydantic 2.11+ features for type safety
Applied to files:
src/omnibase_core/models/core/__init__.pysrc/omnibase_core/models/core/model_monitoring_metrics.pysrc/omnibase_core/models/discovery/model_custommetrics.py
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/**/*.py : Use container.get_service('ProtocolName') for dependency resolution by protocol name, never by concrete class name
Applied to files:
src/omnibase_core/models/service/model_service_compute.pysrc/omnibase_core/models/service/model_service_orchestrator.py
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to **/*.py : Always use ModelOnexError with EnumCoreErrorCode for structured error handling, never raise generic Exception
Applied to files:
tests/unit/primitives/test_model_semver.py
📚 Learning: 2025-09-23T22:28:00.333Z
Learnt from: jonahgabriel
Repo: OmniNode-ai/omnibase_core PR: 32
File: examples/practical_migration_example.py:125-134
Timestamp: 2025-09-23T22:28:00.333Z
Learning: The omnibase_core codebase extensively uses constrained TypeVars for primitive types with the pattern `T = TypeVar("T", str, int, float, bool)` across multiple models including ModelTypedMetrics, ModelGenericMetadata, and ModelExecutionResult. This is the preferred approach over hardcoded union types like `str | int | bool | float | None`.
Applied to files:
src/omnibase_core/models/discovery/model_custom_metrics.pysrc/omnibase_core/models/discovery/model_custommetrics.pysrc/omnibase_core/models/discovery/model_metric_value.py
🧬 Code graph analysis (15)
tests/unit/models/nodes/node_services/test_model_service_effect_integration.py (1)
src/omnibase_core/models/service/model_service_effect.py (1)
ModelServiceEffect(74-133)
tests/unit/models/nodes/node_services/test_model_service_compute_health.py (1)
src/omnibase_core/models/service/model_service_compute.py (1)
ModelServiceCompute(61-118)
tests/unit/models/nodes/node_services/test_model_service_compute_integration.py (1)
src/omnibase_core/models/service/model_service_compute.py (1)
ModelServiceCompute(61-118)
src/omnibase_core/models/core/model_monitoring_metrics.py (1)
src/omnibase_core/models/discovery/model_metric_value.py (1)
ModelMetricValue(34-75)
tests/unit/models/nodes/node_services/test_model_service_reducer_health.py (1)
src/omnibase_core/models/service/model_service_reducer.py (1)
ModelServiceReducer(61-116)
src/omnibase_core/infrastructure/infrastructure_bases.py (2)
src/omnibase_core/models/service/model_service_compute.py (1)
ModelServiceCompute(61-118)src/omnibase_core/models/service/model_service_effect.py (1)
ModelServiceEffect(74-133)
src/omnibase_core/models/registry/model_registry_health_report.py (1)
src/omnibase_core/models/discovery/model_metric_value.py (1)
ModelMetricValue(34-75)
tests/unit/models/nodes/node_services/test_model_service_reducer_invocation.py (1)
src/omnibase_core/models/service/model_service_reducer.py (1)
ModelServiceReducer(61-116)
tests/unit/nodes/test_retry_count_tracking.py (1)
src/omnibase_core/models/service/model_service_effect.py (1)
ModelServiceEffect(74-133)
tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py (1)
src/omnibase_core/models/service/model_service_compute.py (1)
ModelServiceCompute(61-118)
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py (1)
src/omnibase_core/models/service/model_service_effect.py (1)
ModelServiceEffect(74-133)
tests/unit/models/nodes/node_services/test_model_service_reducer_lifecycle.py (1)
src/omnibase_core/models/service/model_service_reducer.py (1)
ModelServiceReducer(61-116)
src/omnibase_core/models/discovery/model_custom_metrics.py (2)
src/omnibase_core/models/discovery/model_custommetrics.py (1)
ModelCustomMetrics(9-43)src/omnibase_core/models/discovery/model_metric_value.py (1)
ModelMetricValue(34-75)
src/omnibase_core/models/discovery/model_custommetrics.py (1)
src/omnibase_core/models/discovery/model_metric_value.py (1)
ModelMetricValue(34-75)
tests/unit/models/nodes/node_services/test_model_service_compute_invocation.py (1)
src/omnibase_core/models/service/model_service_compute.py (1)
ModelServiceCompute(61-118)
🪛 markdownlint-cli2 (0.18.1)
docs/reference/api/nodes.md
149-149: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
177-177: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
191-191: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.6)
src/omnibase_core/models/discovery/model_custom_metrics.py
18-18: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
⏰ 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). (2)
- GitHub Check: claude-review
- GitHub Check: Smoke Tests
🔇 Additional comments (34)
src/omnibase_core/models/discovery/model_metric_value.py (3)
29-31: Good rationale for usingbound=over constraints.The comment correctly explains why
bound=Union[...]is used instead of the typical constrained TypeVar pattern (TypeVar("T", str, int, float, bool)). This allowsAnyMetricValueto be typed asModelMetricValue[Union[str, int, float, bool]], which wouldn't work with discrete constraints.
34-52: LGTM!The generic
ModelMetricValue[MetricValueT]implementation is well-documented and correctly uses Pydantic 2.x generic model patterns. Thevaluefield properly uses the generic type parameter.
78-79: LGTM!The
AnyMetricValuetype alias provides a clean way to handle collections with mixed metric value types, which is the practical use case for most metric containers.src/omnibase_core/models/discovery/model_custommetrics.py (1)
3-6: LGTM!The import updates correctly bring in
AnyMetricValuealongsideModelMetricValue, aligning with the generic typing changes in the source module.src/omnibase_core/models/service/model_service_compute.py (1)
14-18: LGTM!The usage example import path correctly reflects the new module location (
omnibase_core.models.service.model_service_compute), ensuring documentation stays consistent with the actual module structure after the migration.tests/unit/models/nodes/node_services/test_model_service_reducer_health.py (1)
39-39: LGTM!The import path update aligns with the broader module migration from
omnibase_core.models.nodes.node_servicestoomnibase_core.models.service. The test file correctly importsModelServiceReducerfrom its new location.tests/unit/models/nodes/node_services/test_model_service_compute_health.py (1)
43-43: Import path updated to new service module is correctUsing
omnibase_core.models.service.model_service_computekeeps this test aligned with the refactored service wrapper location; no further changes needed here.tests/unit/models/nodes/node_services/test_model_service_orchestrator_lifecycle.py (1)
32-34: Orchestrator import refactor is consistent with new module layoutImporting
ModelServiceOrchestratorfromomnibase_core.models.service.model_service_orchestratormatches the service-wrapper relocation and keeps these lifecycle tests targeting the correct class.docs/reference/SERVICE_WRAPPERS.md (1)
35-35: Documentation imports now match the consolidatedmodels.serviceAPIAll service-wrapper examples (Effect, Compute, Orchestrator, Reducer, and testing snippets) now import from
omnibase_core.models.service, which reflects the refactor and avoids stale paths in user-facing docs.Also applies to: 80-80, 131-131, 185-185, 375-375, 399-399, 427-427
src/omnibase_core/models/service/model_service_reducer.py (1)
16-16: Reducer usage example import updated correctlyThe docstring example now imports
ModelServiceReducerfromomnibase_core.models.service.model_service_reducer, matching the actual implementation location.src/omnibase_core/models/service/model_service_effect.py (1)
16-16: Effect service usage example correctly referencesmodels.serviceThe import in the usage example now matches the real module path for
ModelServiceEffect, preventing confusion for consumers.tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py (1)
30-30: Compute service lifecycle tests now target the relocated classSwitching to
omnibase_core.models.service.model_service_computekeeps these lifecycle tests wired to the canonicalModelServiceComputedefinition after the namespace move.tests/unit/models/nodes/node_services/test_model_service_orchestrator_integration.py (1)
45-46: Integration tests correctly referenceModelServiceOrchestratorfrommodels.serviceUpdating the import to
omnibase_core.models.service.model_service_orchestratoraligns these MRO and integration tests with the new orchestrator module location without affecting behavior.src/omnibase_core/models/service/model_service_orchestrator.py (1)
16-16: Orchestrator usage example import now matches public module pathThe example correctly imports
ModelServiceOrchestratorfromomnibase_core.models.service.model_service_orchestrator, consistent with the refactor and other docs/tests.tests/unit/nodes/test_retry_count_tracking.py (1)
10-10: LGTM! Import path updated to reflect module reorganization.The import path correctly reflects the migration of service wrappers from
omnibase_core.models.nodes.node_servicestoomnibase_core.models.service. No behavioral changes.docs/guides/node-building/01_WHAT_IS_A_NODE.md (1)
379-379: LGTM! Documentation updated to reflect new module structure.The import path in the Quick Reference example correctly reflects the migration of service wrappers to the
omnibase_core.models.servicenamespace.docs/architecture/overview.md (1)
267-267: LGTM! Documentation synchronized with module restructuring.The example import path correctly reflects the service wrapper relocation to
omnibase_core.models.service.src/omnibase_core/models/registry/model_registry_health_report.py (1)
430-440: LGTM! Explicit typing improves mypy strict checking compliance.The addition of explicit type annotation
dict[str, AnyMetricValue]forcustom_metricscorrectly captures the heterogeneous metric value types (str, bool, int, float) used throughout the dictionary. This aligns with the coding guideline requiring 100% strict mypy type checking.docs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.md (1)
267-267: LGTM! Documentation reflects updated module organization.The import path update aligns with the service wrapper migration to
omnibase_core.models.service.tests/unit/models/nodes/node_services/test_model_service_compute_integration.py (1)
32-32: LGTM! Test import updated to new service module location.The import path correctly reflects the ModelServiceCompute relocation. Test coverage and assertions remain unchanged.
tests/unit/models/nodes/node_services/test_model_service_reducer_lifecycle.py (1)
22-22: LGTM! Test import path updated for service wrapper relocation.The import correctly references ModelServiceReducer at its new location in
omnibase_core.models.service. Test functionality unchanged.tests/unit/models/nodes/node_services/test_model_service_orchestrator_invocation.py (1)
31-33: LGTM! Test import reflects service wrapper module migration.The import path correctly references ModelServiceOrchestrator at its new location in
omnibase_core.models.service. Test coverage for tool invocation and workflow coordination remains unchanged.tests/unit/models/nodes/node_services/test_model_service_compute_invocation.py (1)
25-35: ModelServiceCompute import path update looks correctImporting
ModelServiceComputefromomnibase_core.models.service.model_service_computealigns with the new service wrapper location and matches the class definition module. No further changes needed.tests/unit/models/nodes/node_services/test_model_service_reducer_invocation.py (1)
33-52: ModelServiceReducer import path matches new service wrapper locationThe updated import from
omnibase_core.models.service.model_service_reducermatches the service module and keeps the test logic intact.tests/unit/models/nodes/node_services/test_model_service_effect_integration.py (1)
37-47: ModelServiceEffect import path aligned with service moduleImporting
ModelServiceEffectfromomnibase_core.models.service.model_service_effectis consistent with the new service namespace and other references.src/omnibase_core/models/core/model_monitoring_metrics.py (1)
12-15: custom_metrics now correctly uses AnyMetricValue for generic metric typingBringing in
AnyMetricValueand updatingcustom_metricstodict[str, AnyMetricValue] | Nonematches the new generic metric value model and keeps the default behavior ({}viadefault_factory) while still acceptingNonefor compatibility. No issues spotted.Also applies to: 91-94
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py (1)
16-27: ModelServiceEffect import path updated consistentlyThe import now points to
omnibase_core.models.service.model_service_effect, which matches the service wrapper module and keeps the test behavior unchanged.src/omnibase_core/infrastructure/infrastructure_bases.py (1)
29-40: Infrastructure service wrapper imports now aligned with service modulesUsing:
from omnibase_core.models.service.model_service_compute import ModelServiceCompute from omnibase_core.models.service.model_service_effect import ModelServiceEffect # ... # from omnibase_core.models.service.model_service_orchestrator import ModelServiceOrchestrator # from omnibase_core.models.service.model_service_reducer import ModelServiceReducerkeeps this infra module in sync with the new service wrapper locations and the usage examples in the docstring.
__all__remains accurate.docs/reference/api/nodes.md (2)
143-197: Service wrapper API docs now point to the correct modulesThe “Location” strings and example imports for:
ModelServiceComputeModelServiceEffectModelServiceReducerModelServiceOrchestratornow consistently use the
omnibase_core.models.service.model_service_*paths, which matches the service wrapper modules and the rest of the repo. Examples remain accurate with respect to constructor signatures and mixin composition.
331-337: Thread-safety section import updated consistentlyThe thread-safety example now imports
ModelServiceComputefromomnibase_core.models.service.model_service_compute, which matches the rest of the documentation and the infrastructure base module.src/omnibase_core/models/core/__init__.py (1)
10-11: LGTM! Documentation clearly guides developers to the new location.The updated note provides clear guidance on where to import ModelSemVer from after the module restructuring.
docs/guides/VERSION_FIELD_AUTOMATION_STRATEGY.md (1)
161-161: LGTM! Command examples consistently updated throughout the guide.All bash command examples have been updated to use the new import path
omnibase_core.models.primitives.model_semver. The changes are consistent across both the Phase 1 section and the Execution Order section.Also applies to: 177-180, 647-649
scripts/fix_version_field_pattern1.sh (1)
194-194: Import path verification complete — no issues found.The import path
omnibase_core.models.primitives.model_semveris correct and valid. ModelSemVer is properly defined atsrc/omnibase_core/models/primitives/model_semver.pyand is widely used throughout the codebase with this exact import path. No lingering references to the old incorrect path remain. The script will work as intended.tests/unit/primitives/test_model_semver.py (1)
18-23: Import path and symbol exports are correct — all four symbols properly defined and module-level.Verification confirms the import statement is valid:
ModelSemVer: class defined at line 15SemVerField: type alias at line 117 (SemVerField = ModelSemVer)parse_semver_from_string: function at line 136parse_input_state_version: function at line 181All symbols are defined at module level in
./src/omnibase_core/models/primitives/model_semver.pyand will be properly exported. The import path is consistent with the module restructuring.Note: The test execution verification cannot be completed in the sandbox environment (Poetry is unavailable), but static analysis confirms no import errors will occur when the tests are run locally.
Scripts cleanup: - Remove ~70 one-time migration/fix scripts - Move validation scripts to scripts/validation/ - Remove empty directories (intelligence/, performance/, migration/, spi-copy/, templates/) - Update validate-doc-links.py references in CI and pre-commit Code cleanup: - Remove 1,658 unused imports across src/ and tests/ - Update ECOSYSTEM_DIRECTORY_STRUCTURE.md for removed modules - Fix stale script references in 5 documentation files - Remove stale TODO and update version refs in model_onex_container.py Impact: 1,071 files changed, -27,472 lines of dead code
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (86)
.github/workflows/test.yml(1 hunks).pre-commit-config.yaml(1 hunks)docs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.md(4 hunks)docs/ci/CI_MONITORING_GUIDE.md(1 hunks)docs/conventions/VERSION_SEMANTICS.md(1 hunks)docs/guides/INTENT_PUBLISHER_TESTING_DOCUMENTATION.md(1 hunks)docs/guides/VERSION_FIELD_AUTOMATION_STRATEGY.md(6 hunks)docs/testing/CI_TEST_STRATEGY.md(1 hunks)scripts/PROTOCOL_IMPLEMENTATION_REPORT.md(0 hunks)scripts/README_PYDANTIC_FIXERS.md(0 hunks)scripts/TYPE_SAFETY_README.md(0 hunks)scripts/add_missing_imports.py(0 hunks)scripts/add_modelonexerror_import.py(0 hunks)scripts/analyze-to-dict-methods.py(0 hunks)scripts/analyze_exception_handling.py(0 hunks)scripts/analyze_multi_enum_files.py(0 hunks)scripts/analyze_mypy_errors.py(0 hunks)scripts/analyze_name_errors.py(0 hunks)scripts/analyze_pydantic_errors.py(0 hunks)scripts/apply_fixer_to_models.sh(0 hunks)scripts/audit_naming_conventions.py(0 hunks)scripts/auto_fix_type_annotations.py(0 hunks)scripts/batch_fix_all_models.py(0 hunks)scripts/batch_fix_name_errors.py(0 hunks)scripts/batch_fix_string_versions.sh(0 hunks)scripts/comprehensive_arg_type_fix.py(0 hunks)scripts/execute_duplicate_resolution.sh(0 hunks)scripts/fix-import-patterns.py(0 hunks)scripts/fix-imports.py(0 hunks)scripts/fix-semi-redundant-to-dict.py(0 hunks)scripts/fix_all_precommit_errors.py(0 hunks)scripts/fix_all_string_id_version_violations.py(0 hunks)scripts/fix_all_syntax_errors.sh(0 hunks)scripts/fix_arg_type_errors.py(0 hunks)scripts/fix_attr_defined_errors.py(0 hunks)scripts/fix_computation_type_enum.py(0 hunks)scripts/fix_default_factory.py(0 hunks)scripts/fix_documentation_links.py(0 hunks)scripts/fix_enum_naming.py(0 hunks)scripts/fix_error_raising_violations.py(0 hunks)scripts/fix_field_positional_defaults.py(0 hunks)scripts/fix_final_precommit_errors.py(0 hunks)scripts/fix_id_fallbacks.py(0 hunks)scripts/fix_import_syntax_errors.py(0 hunks)scripts/fix_incomplete_imports.py(0 hunks)scripts/fix_misplaced_imports.py(0 hunks)scripts/fix_missing_imports.py(0 hunks)scripts/fix_missing_self_comprehensive.py(0 hunks)scripts/fix_mypy_errors_batch.py(0 hunks)scripts/fix_mypy_type_annotations.py(0 hunks)scripts/fix_name_defined_errors.py(0 hunks)scripts/fix_no_any_return_errors.py(0 hunks)scripts/fix_no_redef_errors.py(0 hunks)scripts/fix_pydantic_field_syntax.py(0 hunks)scripts/fix_pydantic_missing_fields.py(0 hunks)scripts/fix_remaining_id_version_violations.py(0 hunks)scripts/fix_self_multiline.py(0 hunks)scripts/fix_simple_enum_renames.py(0 hunks)scripts/fix_string_id_violations.py(0 hunks)scripts/fix_string_version_violations.py(0 hunks)scripts/fix_string_version_violations_v2.py(0 hunks)scripts/fix_string_versions_ast.py(0 hunks)scripts/fix_string_versions_regex.py(0 hunks)scripts/fix_syntax_errors.py(0 hunks)scripts/fix_syntax_errors_v2.py(0 hunks)scripts/fix_type_annotations.py(0 hunks)scripts/fix_type_annotations_batch2.py(0 hunks)scripts/fix_uuid_errors.py(0 hunks)scripts/fix_valueerror_to_onexerror.py(0 hunks)scripts/fix_version_field_pattern1.sh(0 hunks)scripts/fix_version_field_pattern2.py(0 hunks)scripts/generate_all_exports.py(0 hunks)scripts/implement_protocols.py(0 hunks)scripts/intelligence/comprehensive_any_type_detector.py(0 hunks)scripts/intelligence/enhanced_type_system_validator.py(0 hunks)scripts/intelligence/intelligence_hook.py(0 hunks)scripts/migrate-pydantic-dict-calls.py(0 hunks)scripts/migrate_modelschemavalue.py(0 hunks)scripts/migrate_onex_error.py(0 hunks)scripts/migration/fix_version_defaults.py(0 hunks)scripts/migration/fix_version_defaults_v2.py(0 hunks)scripts/migration/migrate_protocols_safe.py(0 hunks)scripts/migration/remove_version_default_factory.py(0 hunks)scripts/performance/analyze_imports.py(0 hunks)scripts/performance/benchmark_serialization.py(0 hunks)scripts/performance/memory_usage_analysis.py(0 hunks)
💤 Files with no reviewable changes (78)
- scripts/fix_valueerror_to_onexerror.py
- scripts/PROTOCOL_IMPLEMENTATION_REPORT.md
- scripts/batch_fix_string_versions.sh
- scripts/execute_duplicate_resolution.sh
- scripts/add_missing_imports.py
- scripts/fix_uuid_errors.py
- scripts/README_PYDANTIC_FIXERS.md
- scripts/fix_all_syntax_errors.sh
- scripts/apply_fixer_to_models.sh
- scripts/fix_import_syntax_errors.py
- scripts/fix_mypy_errors_batch.py
- scripts/analyze_exception_handling.py
- scripts/fix_syntax_errors.py
- scripts/fix_pydantic_field_syntax.py
- scripts/fix_id_fallbacks.py
- scripts/fix-import-patterns.py
- scripts/analyze_name_errors.py
- scripts/audit_naming_conventions.py
- scripts/fix_mypy_type_annotations.py
- scripts/fix_version_field_pattern2.py
- scripts/batch_fix_all_models.py
- scripts/analyze_multi_enum_files.py
- scripts/fix_type_annotations_batch2.py
- scripts/auto_fix_type_annotations.py
- scripts/add_modelonexerror_import.py
- scripts/migration/fix_version_defaults_v2.py
- scripts/fix_attr_defined_errors.py
- scripts/fix_version_field_pattern1.sh
- scripts/fix_string_version_violations_v2.py
- scripts/fix_incomplete_imports.py
- scripts/fix-semi-redundant-to-dict.py
- scripts/fix_arg_type_errors.py
- scripts/generate_all_exports.py
- scripts/TYPE_SAFETY_README.md
- scripts/fix_remaining_id_version_violations.py
- scripts/batch_fix_name_errors.py
- scripts/intelligence/intelligence_hook.py
- scripts/fix_pydantic_missing_fields.py
- scripts/fix_missing_imports.py
- scripts/intelligence/comprehensive_any_type_detector.py
- scripts/fix_self_multiline.py
- scripts/migrate_modelschemavalue.py
- scripts/performance/memory_usage_analysis.py
- scripts/fix_final_precommit_errors.py
- scripts/analyze-to-dict-methods.py
- scripts/analyze_pydantic_errors.py
- scripts/fix_name_defined_errors.py
- scripts/fix_no_any_return_errors.py
- scripts/fix_enum_naming.py
- scripts/performance/analyze_imports.py
- scripts/migrate_onex_error.py
- scripts/fix-imports.py
- scripts/fix_all_precommit_errors.py
- scripts/fix_string_versions_ast.py
- scripts/fix_simple_enum_renames.py
- scripts/fix_error_raising_violations.py
- scripts/fix_field_positional_defaults.py
- scripts/fix_string_versions_regex.py
- scripts/migration/fix_version_defaults.py
- scripts/fix_computation_type_enum.py
- scripts/fix_type_annotations.py
- scripts/migration/migrate_protocols_safe.py
- scripts/comprehensive_arg_type_fix.py
- scripts/fix_no_redef_errors.py
- scripts/fix_syntax_errors_v2.py
- scripts/fix_misplaced_imports.py
- scripts/analyze_mypy_errors.py
- scripts/fix_missing_self_comprehensive.py
- scripts/fix_documentation_links.py
- scripts/intelligence/enhanced_type_system_validator.py
- scripts/fix_all_string_id_version_violations.py
- scripts/migration/remove_version_default_factory.py
- scripts/migrate-pydantic-dict-calls.py
- scripts/implement_protocols.py
- scripts/fix_default_factory.py
- scripts/performance/benchmark_serialization.py
- scripts/fix_string_version_violations.py
- scripts/fix_string_id_violations.py
✅ Files skipped from review due to trivial changes (2)
- docs/guides/INTENT_PUBLISHER_TESTING_DOCUMENTATION.md
- docs/ci/CI_MONITORING_GUIDE.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to **/*.py : Always use Poetry for Python package management and task execution - never use pip or python directly
Applied to files:
docs/guides/VERSION_FIELD_AUTOMATION_STRATEGY.md
📚 Learning: 2025-11-25T21:48:22.841Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T21:48:22.841Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors, never use ModelContainer[T]
Applied to files:
docs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.md
🪛 LanguageTool
docs/guides/VERSION_FIELD_AUTOMATION_STRATEGY.md
[style] ~331-~331: Consider replacing this word to strengthen your wording.
Context: ...is script was a one-time migration tool and has been removed after the migration wa...
(AND_THAT)
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.md
265-265: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (3)
- GitHub Check: Code Quality
- GitHub Check: Smoke Tests
- GitHub Check: claude-review
🔇 Additional comments (7)
docs/conventions/VERSION_SEMANTICS.md (1)
659-673: Documentation clarification is well-executed.The updated pre-commit validation section appropriately distinguishes between proposed enhancements and current enforcement mechanisms. Commenting out the example validation commands prevents users from attempting to run scripts that don't exist, and the explanatory note clearly states that compliance is currently enforced through code review and type checking (mypy strict mode).
docs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.md (1)
41-46: Documentation correctly reflects service wrapper module reorganization.The updates to paths (
models/nodes/node_services/→models/service/), import statements (from omnibase_core.models.service import), and service wrapper examples align well with the PR's core objective of reorganizing service wrappers. The documentation is internally consistent and properly guides developers to the new module locations.Also applies to: 263-263, 266-266
.github/workflows/test.yml (1)
232-235: Consistent path migration across configs and workflows.Both
.pre-commit-config.yaml(line 327) and.github/workflows/test.yml(line 234) have been updated consistently to reflect the script relocation. The changes align with the PR's validation script reorganization objective.Use the verification script provided in the
.pre-commit-config.yamlreview to confirm the script path migration is complete and all references have been updated..pre-commit-config.yaml (1)
325-333: Script relocation is complete and consistent.The verification confirms:
- ✓ New path
scripts/validation/validate-doc-links.pyexists- ✓ Old path has been cleaned up (no orphaned script)
- ✓ No dangling references to the old path pattern
The path update in
.pre-commit-config.yaml(line 327) correctly reflects the reorganization, and the file has been properly moved rather than copied.docs/guides/VERSION_FIELD_AUTOMATION_STRATEGY.md (3)
230-288: Document appropriately marks removed migration scripts as historical reference.The annotations marking
scripts/fix_version_field_pattern1.sh(line 238) andscripts/fix_version_field_pattern2.py(line 329) as "REMOVED" with supporting notes (lines 232, 331) are clear and appropriately contextualize the preserved code examples as historical reference. This maintains the document's value as a record of how the migration was accomplished.
661-689: Verify phase completion markers align with actual PR scope.The document marks Phase 1–3 migration steps as "COMPLETED" (lines 661, 676) and transitions command examples to "Historical Reference" (lines 563, 663, 678). Confirm these phases were indeed completed as part of the current PR or in prior work, and that users should not attempt to run these removed scripts. If the migration was completed in an earlier PR, consider adding a note explaining when/where it was done.
160-183: Import path migration verified and accurately documented.All verification points confirm the documentation is correct:
omnibase_core.models.primitives.model_semverexists and is the canonical location forModelSemVerModelSemVerclass is properly defined and accessible at line 15 of the module- Old import path (
omnibase_core.primitives.model_semver) no longer exists in the codebase- Active code consistently uses the new import path across 15+ test files and throughout the codebase
The documentation accurately reflects the completed migration.
| **Location**: `omnibase_core/src/omnibase_core/models/nodes/node_services/` | ||
| **Location**: `omnibase_core/src/omnibase_core/models/service/` | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add language specifications to fenced code blocks.
Markdown linting requires fenced code blocks to specify a language for syntax highlighting. Three code blocks are missing language specifications.
Apply these diffs to add python language specifications:
**Location**: `omnibase_core/src/omnibase_core/models/service/`
-```
+```python
from omnibase_core.models.service import ModelServiceCompute **Location**: `omninode_bridge/src/omninode_bridge/nodes/`
-```
+```python
from omnibase_core.nodes.node_effect import NodeEffect **Location**: `omnibase_infra/src/omnibase_infra/nodes/`
-```
+```python
from omnibase_core.infrastructure.infrastructure_bases import ModelServiceEffectAlso applies to: 284-285, 301-302
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
265-265: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/architecture/ECOSYSTEM_DIRECTORY_STRUCTURE.md around lines 265 (and
similarly for 284-285 and 301-302), several fenced code blocks are missing
language identifiers; update each opening triple-backtick for the listed blocks
to ```python so the blocks read as Python code fences (i.e., replace ``` with
```python at the start of each of the three code blocks referenced).
| **Configuration**: | ||
| ``` | ||
| python3 scripts/validate-doc-links.py --fix-case | ||
| python3 scripts/validation/validate-doc-links.py --fix-case |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify script path update and search for orphaned references
echo "=== Checking new script path ==="
if [ -f "scripts/validation/validate-doc-links.py" ]; then
echo "✓ New script path found: scripts/validation/validate-doc-links.py"
else
echo "✗ New script path NOT found"
fi
echo ""
echo "=== Checking old script path ==="
if [ -f "scripts/validate-doc-links.py" ]; then
echo "⚠ Old script path still exists: scripts/validate-doc-links.py"
else
echo "✓ Old script path removed"
fi
echo ""
echo "=== Searching for references to old path ==="
rg -n "scripts/validate-doc-links\.py" --type=md --type=py --type=yaml --type=toml 2>/dev/null | head -20
if [ $? -eq 1 ]; then
echo "✓ No references to old path found in docs/code"
fi
echo ""
echo "=== Checking if scripts/validation directory exists ==="
if [ -d "scripts/validation" ]; then
echo "✓ Directory scripts/validation exists"
echo "Contents:"
ls -la scripts/validation/ 2>/dev/null | head -10
else
echo "✗ Directory scripts/validation does NOT exist"
fiRepository: OmniNode-ai/omnibase_core
Length of output: 1203
Update inline documentation within the script to reflect the new path location.
The path update in the documentation is correct—scripts/validation/validate-doc-links.py exists and the old path has been removed. However, the script's own inline documentation still references the old path. Lines 12–13 in scripts/validation/validate-doc-links.py contain stale references:
poetry run python scripts/validate-doc-links.py
poetry run python scripts/validate-doc-links.py --fix-case
These should be updated to:
poetry run python scripts/validation/validate-doc-links.py
poetry run python scripts/validation/validate-doc-links.py --fix-case
🤖 Prompt for AI Agents
In scripts/validation/validate-doc-links.py around lines 12–13, the script's
inline documentation/docstring still references the old path
(scripts/validate-doc-links.py); update those two lines to use the new path
scripts/validation/validate-doc-links.py (i.e., replace both occurrences of
"poetry run python scripts/validate-doc-links.py" and the "--fix-case" variant
with the new path) and ensure any other examples or usage strings in the file
reflect the new location.
PR #92 Code Review: ApplicationContext for Thread-Safe Container ManagementExecutive SummaryThis PR introduces a contextvars-based container management system to replace the legacy singleton holder pattern. The implementation is well-designed but has two critical issues that must be fixed before merging. 🚨 Critical Issues (Must Fix)1. Container Creation Bypasses ApplicationContext - BLOCKINGLocation:
container = ModelONEXContainer() # Creates NEW container, ignoring context\!Fix: Use 2. Race Condition in TTL Cache Clearing - BLOCKINGLocation: The TTL cache clearing is NOT thread-safe. Multiple pytest-xdist workers can simultaneously check TTL and call cache_clear(). Fix: Add
|
Pull Request Review - PR #92: ApplicationContext MigrationOverviewThis is an excellent architectural improvement that migrates from singleton holders to Python's Summary✅ Approved - This PR represents a significant quality improvement with proper migration strategy, comprehensive testing, and solid architectural foundation. Strengths1. Solid Architecture ⭐⭐⭐⭐⭐The use of
Example of clean design ( def get_current_container() -> ModelONEXContainer | None:
"""Get the container from the current context."""
return _current_container.get()Simple, clear, and thread-safe. 2. Comprehensive Test Isolation ⭐⭐⭐⭐⭐The new
Particularly good: 3. Backward Compatibility ⭐⭐⭐⭐⭐Migration is handled exceptionally well:
Example ( 4. Thorough TestingThe
5. Clean Implementation
Issues & Recommendations🟡 Minor Issue #1: Potential Performance Cost in
|
Comprehensive Code Review - PR #92🎯 Overall Assessment: APPROVE with Minor RecommendationsThis is an excellent architectural improvement that eliminates a fundamental source of test flakiness in parallel execution. The migration from global singleton holders to ✅ Strengths1. Solid Architecture & Design
2. Code Quality
3. Test Isolation Strategy
4. Migration Excellence
🔍 Issues & RecommendationsCritical Issues: NONE ✅High Priority1. Update Stale Comment in pyproject.toml (line 323)Issue: Comment references Impact: Documentation inconsistency could confuse developers Fix: -# - Use --dist=loadscope to group tests by module (reduces import overhead)
+# - Use --dist=loadgroup to group tests by @pytest.mark.xdist_group (enables finer control)Location: Medium Priority2. Direct Access to Private
|
Pull Request Review: Context-Based Container ManagementOverviewThis PR introduces a significant architectural improvement by migrating from singleton holder patterns to contextvars-based container management. This is a well-designed solution to parallel test execution issues with pytest-xdist. Strengths1. Excellent Architecture Choice
2. Comprehensive Test CoverageThe new ApplicationContext has excellent test coverage in tests/unit/context/test_application_context.py (391 lines) thoroughly testing thread isolation, async task isolation, nested context managers, exception handling, and token-based reset. 3. Well-Documented
4. Test Configuration Improvements
Issues and Concerns1. CRITICAL: Stale Documentation CommentLocation: pyproject.toml:323 The comment references --dist=loadscope but line 306 uses --dist=loadgroup. Update comment to match actual configuration. 2. Direct Access to Private ContextVarLocation: tests/fixtures/isolation.py:73 Test utilities directly access private _current_container variable. Consider exposing a public clear_current_container() API to maintain encapsulation. 3. Inconsistent Error HandlingSome registries use silent no-op on error while others raise structured errors. Document when silent failures are acceptable (reset functions in tests) vs when errors should propagate (getter functions). 4. PR Size ConcernStats: 1,114 files changed. While massive deletions are positive (removing old migration scripts), large scope makes thorough review challenging. Code QualityPositive
Minor Issues
Testing RecommendationsBefore Merge
Post-Merge Monitoring
Final RecommendationAPPROVE with minor fixes required before merge Must Fix Before Merge
Why Approve
Risk Assessment
Great work on this architectural improvement! |
1 participant
Summary
ApplicationContext) to replace the legacy singleton holder patternreset_all_singletons,reset_container_singleton) for test isolationKey Changes
src/omnibase_core/context/module: Thread-safe container management using Python'scontextvarsutil_container_holder.pywithApplicationContextmodel_action_registry.py,model_cli_command_registry.py,model_event_type_registry.pyuselru_cachepatternstests/fixtures/isolation.pywith comprehensive test isolation supportWhy This Matters
The previous singleton holder pattern caused test isolation issues in parallel execution (pytest-xdist). Each parallel worker could interfere with others through shared global state. The new contextvars-based approach ensures:
Test Plan
poetry run pytest tests/unit/)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
models.servicestructureChores
✏️ Tip: You can customize this high-level summary in your review settings.