Repository navigation
test(orchestrator): add comprehensive orchestrator tests [OMN-952] - #85
Conversation
Add 59 tests verifying orchestrator purity and correctness: - Emits events only (no commands/intents): 8 tests - No I/O performed (delegation to effects): 15 tests - Uses injected now (no system clock): 10 tests - All decision paths (7 event types): 15 tests - Projection reader interactions: 11 tests Also removes stale FUTURE(OMN-930) and FUTURE(OMN-973) comments since both dependencies are now complete. Closes OMN-952
|
Warning Rate limit exceeded@jonahgabriel has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 9 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds deterministic time injection (RuntimeTick→now) and a ProtocolProjectionReader-driven read_projection step to the node_registration_orchestrator contract; updates event patterns, dependencies, docs, and introduces extensive AST/static-analysis and contract tests plus AST helper utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Orchestrator
participant RuntimeTick
participant ProjectionReader
participant Reducer
participant EffectNode
Client->>Orchestrator: registry-request-introspection
Note right of Orchestrator `#EFEFEF`: receive_introspection\n(event_pattern includes registry-request-introspection, runtime-tick)
RuntimeTick->>Orchestrator: RuntimeTick(now: T₀)
Note right of Orchestrator `#EFEFEF`: time_injection enabled\n(source: RuntimeTick, field: now)
Orchestrator->>ProjectionReader: read_projection(projection_name: node_registration_state)
ProjectionReader-->>Orchestrator: registration state snapshot
Orchestrator->>Orchestrator: evaluate_timeout(now: T₀)
Note over Orchestrator `#F7F7F7`: timeout_evaluation true\nuses injected now
Orchestrator->>Reducer: compute_intents(state, timeout_result)
Reducer-->>Orchestrator: decision
Orchestrator->>EffectNode: execute_intent(via ProtocolEffect)
EffectNode->>Client: publish decision event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Comment |
PR Review: Comprehensive Orchestrator Tests [OMN-952] ✅Overall Assessment: LGTM - Excellent work! This PR demonstrates exceptional adherence to ONEX architecture principles with comprehensive test coverage. 🎯 Strengths1. Architectural Compliance ⭐⭐⭐⭐⭐
2. Test Quality ⭐⭐⭐⭐⭐
3. ONEX Convention Adherence
4. Documentation ImprovementsThe removal of stale
🔍 Test Coverage Analysis
Total: 59 tests - Excellent granular coverage! 📋 Specific Review Notes✅
|
| Metric | Value | Assessment |
|---|---|---|
| Test files | 5 | Excellent separation |
| Total tests | 59 | Comprehensive |
| Lines added | 2,814 | Well-scoped |
| Lines removed | 43 | Clean stale code removal |
| Test-to-code ratio | High | Validates contract, not just code |
🎬 Final Verdict
Approve ✅
This PR is a model example of thorough ONEX testing:
- Validates architectural principles through AST analysis
- Provides comprehensive acceptance criteria coverage
- Excellent documentation and test organization
- Removes technical debt (stale FUTURE comments)
- No breaking changes or security concerns
Recommendation: Merge after CI passes. The test suite provides excellent regression protection for OMN-952 requirements.
🙏 Kudos
Special recognition for:
- AST-based validation - Smart approach avoiding brittle mocks
- Test documentation - Each file has excellent module docstrings
- Granular coverage - 59 tests covering every acceptance criterion
- Contract-driven design - Tests validate YAML configuration, not just Python code
Great work @jonahgabriel! 🚀
Reviewed by Claude Code following ONEX infrastructure patterns
Reference: /home/runner/work/omnibase_infra/omnibase_infra/CLAUDE.md
Fix CI lint failures by applying ruff format and ruff check --fix: - Remove f-string prefixes from strings without placeholders - Remove empty TYPE_CHECKING block - Sort imports correctly - Simplify elif after continue statements
PR Review: Comprehensive Orchestrator Tests (OMN-952)Executive SummaryOverall Assessment: STRONG APPROVAL ✅ This PR delivers exceptional test coverage for the registration orchestrator, with 59 well-structured tests validating all OMN-952 acceptance criteria. The tests demonstrate deep understanding of ONEX architecture principles and use sophisticated AST analysis to enforce compile-time guarantees. Highlights:
Statistics:
1. ONEX Architecture Compliance ✅Orchestrator Purity - EXCELLENTStrengths:
Code Example ( # Orchestrator should have NO public methods defined directly
# All behavior comes from base class
assert own_methods == [], (
f"Orchestrator should have no custom public methods.\n"
f"Found: {own_methods}\n"
f"A pure coordinator delegates all work to the base class "
f"which handles workflow execution via contract.yaml."
)This is ONEX architecture enforcement at its finest - preventing drift from the declarative pattern. Time Injection - EXCELLENTStrengths:
This provides deterministic time for testing and replay - critical for production reliability. Projection Reader Integration (OMN-930) - EXCELLENTStrengths:
This enforces the event sourcing + projection pattern - avoiding race conditions from direct topic scanning. 4-Node Architecture Adherence - EXCELLENTStrengths:
2. Code Quality ✅Type Annotations - EXCELLENTStrengths:
Perfect adherence to ONEX typing standards. File & Naming Conventions - EXCELLENTAll test files follow the
Error Handling - EXCELLENTStrengths:
This provides clear feedback when tests fail, not cryptic error messages. 3. Test Coverage ✅Acceptance Criteria from OMN-952 - COMPLETE
Total: 59 tests across 5 categories AST Analysis Patterns - EXCELLENTThe tests use sophisticated AST walking to detect anti-patterns at compile time. This is proactive enforcement - prevents violations before they reach runtime. Contract Validation - EXCELLENTTests validate every aspect of the contract:
Edge Cases & Error Scenarios - EXCELLENTStrengths:
4. Documentation ✅FUTURE Comment Removal - EXCELLENTAssessment: ✅ EXCELLENT
This is proper documentation hygiene - keeping contracts up-to-date with reality. Docstring Quality - EXCELLENTModule-level docstrings:
This provides learning context for developers unfamiliar with ONEX principles. Test-level docstrings: Test Descriptions - EXCELLENTTest names are self-documenting and follow clear patterns like: 5. Potential Issues & RecommendationsCritical Issues: NONE ✅No blocking issues found. All tests follow ONEX patterns correctly. Minor Issues (Non-blocking)1. Hardcoded I/O Keywords (Low Priority)Location: Recommendation: Severity: Low - Not blocking, but improves maintainability 2. Test Fixture Duplication (Very Low Priority)Issue: Recommendation: Severity: Very Low - Current approach is fine for isolated test file readability Missing Coverage (Nice-to-Have)1. Integration Test for Time Injection (Enhancement)Gap: Tests verify contract configuration and AST patterns, but don't test end-to-end time injection with actual Recommendation (Future Work): Severity: Enhancement - Current AST tests provide compile-time guarantees, integration test would add runtime verification 2. Contract Schema Validation (Enhancement)Gap: Tests validate specific contract fields but don't validate against a formal schema. Recommendation (Future Work): Severity: Enhancement - Would provide additional safety but current tests are comprehensive 6. Performance ConcernsTest Execution Performance - ACCEPTABLEEstimated runtime: < 1 second for all 59 tests No optimization needed. Current fixture-based caching is appropriate. 7. Violations of ONEX PatternsNONE FOUND ✅All tests follow ONEX patterns correctly:
8. Final RecommendationsBefore Merge:✅ No blocking changes required - PR is ready to merge Post-Merge (Optional Enhancements):
SummaryPR Status: APPROVED FOR MERGE ✅ This PR represents exceptional quality work:
All suggested improvements are non-blocking and can be addressed in future PRs if desired. Test Statistics:
Congratulations on delivering high-quality, production-ready test coverage! 🎉 |
- Extract shared fixtures to conftest.py with scope="module" for performance - Add "HOW TO FIX" guidance to 6 error messages in test_orchestrator_no_io.py - Add 3 new tests validating consumed events have workflow handlers - Enhance I/O library detection list with vault, aiofiles, and documentation - Fix contract event_pattern to handle RegistryRequestIntrospectionEvent Test count: 59 → 62 (3 new consumed event validation tests)
PR Review: Comprehensive Orchestrator Tests [OMN-952]SummaryThis PR adds 59 comprehensive tests for the Node Registration Orchestrator, covering all acceptance criteria from OMN-952. The test suite is well-structured, thoroughly documented, and removes stale FUTURE comments now that dependencies (OMN-930, OMN-973) are complete. ✅ Strengths1. Excellent Test Organization
2. Strong Adherence to ONEX Principles
3. Documentation Excellence
4. FUTURE Comment Cleanup
🔍 Issues Found❌ CRITICAL: Missing Type Annotations (ONEX Guideline Violation)Location: Issue: Multiple fixtures use unparameterized # ❌ WRONG - Violates ONEX strong typing guideline
def contract_data(contract_path: Path) -> dict: # Line 84
def published_events(contract_data: dict) -> list[dict]: # Line 184
def event_types_map(published_events: list[dict]) -> dict[str, dict]: # Line 200
def execution_graph_nodes(contract_data: dict) -> list[dict]: # Line 217
def dependencies(contract_data: dict) -> list[dict]: # Line 241ONEX Guideline: CLAUDE.md mandates "NEVER use Why This Matters:
Recommended Fix: from typing import Any # Only for YAML deserialization typing
# ✅ CORRECT - Specific typing for YAML structures
def contract_data(contract_path: Path) -> dict[str, Any]:
"""Load and return contract.yaml as a dictionary.
Note: Uses dict[str, Any] because YAML deserializes to nested
heterogeneous dictionaries with unknown structure. This is the
appropriate type for schema-validated YAML content.
"""
...
def published_events(contract_data: dict[str, Any]) -> list[dict[str, Any]]:
...
def event_types_map(published_events: list[dict[str, Any]]) -> dict[str, dict[str, Any]]:
...Alternative (More Specific): from typing import TypedDict
class PublishedEvent(TypedDict):
topic: str
event_type: str
class ContractData(TypedDict, total=False):
published_events: list[PublishedEvent]
dependencies: list[dict[str, Any]]
# ... other known keys
def contract_data(contract_path: Path) -> ContractData:
...Severity: CRITICAL - This violates ONEX coding guidelines and should be fixed before merge.
|
| Acceptance Criterion | Tests | Coverage |
|---|---|---|
| Emits events only (no commands/intents) | 8 | ✅ Comprehensive |
| No I/O performed | 15 | ✅ Comprehensive |
| Uses injected now, not system clock | 10 | ✅ Comprehensive |
| All decision paths tested | 15 | ✅ Comprehensive |
| Projection reader interactions | 11 | ✅ Comprehensive |
| TOTAL | 59 | ✅ All criteria covered |
Coverage Gaps
⚠️ Missing: Runtime integration tests (these are unit tests validating contract/AST, not runtime behavior)⚠️ Missing: Contract-to-execution graph consistency test (verifyevent_patternmatchesconsumed_events)
Recommendation: File follow-up tickets for:
- Integration tests that exercise the orchestrator with real reducer/effect dependencies
- Contract validation tests that ensure execution graph patterns match consumed event topics
🎨 Best Practices
✅ Excellent Adherence
- ONEX naming conventions: All test files use
test_orchestrator_<concern>.pypattern - Type safety: Tests use typed fixtures (except the critical issue noted above)
- Documentation: Module docstrings explain design rationale, not just "what"
- Single responsibility: Each test file focuses on one acceptance criterion
- Performance-aware: Module-scoped fixtures for expensive operations
💡 Suggestions
- Add contract consistency test: Verify
event_patternwildcards matchconsumed_eventstopics - Consider property-based testing: Use
hypothesisfor event topic validation (generate random topics and verify pattern matching) - Add runtime behavior note: Document in README that these are contract/AST validation tests, not runtime behavior tests
📝 Recommendation
Approve with required changes:
- ❌ MUST FIX: Add type parameters to
dictandlistinconftest.py(usedict[str, Any]or TypedDict) ⚠️ SHOULD ADD: Contract consistency test forevent_patternvsconsumed_eventsmatching- 💡 NICE TO HAVE: Performance note about module-scoped fixtures in test file docstrings
Summary: This is a high-quality test suite that thoroughly validates all acceptance criteria from OMN-952. The only critical issue is missing type parameters on generic types, which violates ONEX coding guidelines. Once fixed, this PR significantly improves test coverage and removes stale FUTURE comments. Excellent work overall! 🎉
📚 References
- OMN-952: Comprehensive orchestrator tests
- OMN-888: Decision path event types
- OMN-930: Projection reader integration (omnibase_spi#44)
- OMN-973: Time injection (omnibase_infra#66)
- CLAUDE.md: ONEX coding guidelines (strong typing, no
Any)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/nodes/conftest.py (1)
199-213: PotentialKeyErrorif event lacksevent_typefield.The comprehension on line 213 assumes all events have an
event_typekey. If any published event is malformed (missingevent_type), this will raise an unhandledKeyErrorrather than a clear test failure message.🔎 Defensive handling for missing event_type
@pytest.fixture(scope="module") def event_types_map(published_events: list[dict]) -> dict[str, dict]: """Build a map of event_type -> event definition for quick lookup. Uses module scope as it derives from module-scoped published_events. This provides O(1) lookup for individual event types during testing. Args: published_events: List of published events (auto-injected by pytest). Returns: Dictionary mapping event_type names to their full definitions. """ - return {event["event_type"]: event for event in published_events} + result = {} + for event in published_events: + event_type = event.get("event_type") + if event_type is None: + pytest.fail(f"Published event missing 'event_type': {event}") + result[event_type] = event + return resulttests/unit/nodes/test_orchestrator_no_io.py (1)
419-477: String-based class body parsing is fragile; consider AST-based approach.The manual line-by-line parsing to count class body lines (handling docstrings, comments, indentation) is complex and could have edge cases with multi-line strings or unusual formatting.
Consider using AST directly to count class body statements:
# Alternative approach using AST for node in ast.walk(tree): if isinstance(node, ast.ClassDef) and node.name == "NodeRegistrationOrchestrator": # Filter out __init__ and count remaining method definitions non_init_methods = [ n for n in node.body if isinstance(n, ast.FunctionDef) and n.name != "__init__" ] # Exclude docstring (first Expr with Constant string) body_without_docstring = [ n for n in node.body if not (isinstance(n, ast.Expr) and isinstance(n.value, ast.Constant)) ]This would be more robust and consistent with the AST-based approach used elsewhere in the test file.
tests/unit/nodes/test_orchestrator_purity.py (1)
451-474: Hardcoded expected events list couples test to implementation details.The
expected_eventsset (lines 452-461) duplicates the event definitions fromcontract.yaml. If the contract changes (adds/removes events), this test will fail but requires manual synchronization.Consider whether this is intentional as a "change detector" test, or if the test should be more flexible. If the goal is to ensure a minimum set of events exist, use subset checking:
# Verify minimum expected events exist (allows additional events) required_events = {"NodeRegistrationResultEvent", "NodeRegistrationInitiated", ...} missing = required_events - event_types assert not missing, f"Missing required events: {missing}"If the exact count is intentional (to catch accidental additions/removals), the current implementation is appropriate but consider documenting this explicitly in the docstring.
tests/unit/nodes/test_orchestrator_decision_paths.py (1)
127-279: Consider parameterizing individual event existence tests to reduce duplication.The seven
test_node_*_event_existsmethods (lines 127-279) follow an identical pattern: check event exists in map, verify topic matches expected value. This could be consolidated usingpytest.mark.parametrize.🔎 Parameterized approach
@pytest.mark.parametrize( "event_type,expected_topic_slug", [ ("NodeRegistrationInitiated", "node-registration-initiated"), ("NodeRegistrationAccepted", "node-registration-accepted"), ("NodeRegistrationRejected", "node-registration-rejected"), ("NodeRegistrationAckTimedOut", "node-registration-ack-timed-out"), ("NodeRegistrationAckReceived", "node-registration-ack-received"), ("NodeBecameActive", "node-became-active"), ("NodeLivenessExpired", "node-liveness-expired"), ], ) def test_decision_event_exists_with_correct_topic( self, event_types_map: dict[str, dict], event_type: str, expected_topic_slug: str ) -> None: """Test that decision path event is properly defined with correct topic.""" assert event_type in event_types_map, f"{event_type} must be defined" event = event_types_map[event_type] expected_topic = f"{{env}}.{{namespace}}.onex.evt.{expected_topic_slug}.v1" assert event["topic"] == expected_topicThis would reduce ~150 lines to ~20 lines while maintaining the same coverage. The current approach is also valid if explicit test names per event are preferred for reporting.
tests/unit/nodes/test_orchestrator_time_injection.py (1)
544-575: Keyword-based time-dependency detection may have false positives/negatives.The
time_keywordsset (lines 545-552) uses heuristics like "timeout", "deadline", "wait" to identify time-dependent steps. This could:
- False positive: A step named "update_timeout_config" that just stores config without using time
- False negative: A step named "check_expiry" (missing "expire" keyword)
The current approach is reasonable for catching common patterns. Consider adding "expiry" to the keywords or documenting that the test is a heuristic safeguard:
time_keywords = { "timeout", "deadline", "schedule", "expire", + "expiry", "duration", "wait", }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (8)
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yamlsrc/omnibase_infra/nodes/node_registration_orchestrator/node.pytests/unit/nodes/conftest.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_no_io.pytests/unit/nodes/test_orchestrator_projection_reader.pytests/unit/nodes/test_orchestrator_purity.pytests/unit/nodes/test_orchestrator_time_injection.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER useAnytype - Always use specific types. All data structures must be proper Pydantic models.
UseEnumMessageCategoryfor message routing, topic parsing, and dispatcher selection (values: EVENT, COMMAND, INTENT). UseEnumNodeOutputTypefor execution shape validation and handler return type validation (values: EVENT, COMMAND, INTENT, PROJECTION).
Use PEP 604 union syntaxX | Nonefor nullable types instead ofOptional[X]. Example:def get_user(id: str) -> User | None:instead ofdef get_user(id: str) -> Optional[User]:
All services MUST use ModelONEXContainer for dependency injection. Bootstrap withcontainer = ModelONEXContainer()and resolve services viacontainer.service_registry.resolve_service(ServiceType)
Always propagatecorrelation_idfrom incoming requests to error context. Auto-generate usinguuid4()if no correlation_id exists. Use UUID format for all new correlation IDs.
NEVER include in error messages or context: passwords, API keys, tokens, secrets, full connection strings with credentials, PII, internal IP addresses, private keys, certificates, session tokens, or cookies. SAFE to include: service names, operation names, correlation IDs, error codes, sanitized hostnames, port numbers, retry counts, timeout values, resource identifiers (non-sensitive).
UseProtocolConfigurationErrorfor invalid config,SecretResolutionErrorfor missing secrets,InfraConnectionErrorfor connection failures,InfraTimeoutErrorfor operation timeouts,InfraAuthenticationErrorfor auth failures,InfraUnavailableErrorfor unavailable resources.
Use graceful degradation forInfraTimeoutError. Pattern: try primary source with timeout, fall back to cache/secondary source on timeout, aggregate results with degradation flag.
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pytests/unit/nodes/test_orchestrator_no_io.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_purity.pytests/unit/nodes/conftest.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_projection_reader.py
**/node.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/node.py: Node implementations should follow naming patternnode.pywith class patternNode<Name><Type>. Example:NodePostgresAdapterEffect
Prefix internal/sensitive methods with_to exclude them from introspection. Node introspection uses reflection to discover public methods - private methods are hidden from exposure.
Use generic operation names in public methods that don't reveal implementation details. Example:process_requestinstead ofdecrypt_and_forward_to_payment_gateway. Use generic parameter names likedatainstead ofuser_credentials.
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.py
**/nodes/*/node.py
📄 CodeRabbit inference engine (CLAUDE.md)
Node base classes (archetypes) and I/O models come from
omnibase_core.nodes, notomnibase_infra. Import:NodeEffect,NodeCompute,NodeReducer,NodeOrchestratorfromomnibase_core.nodes
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.py
**/nodes/*/contract.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
All node contracts require: semantic versioning in
contract_versionfield, node type (EFFECT/COMPUTE/REDUCER/ORCHESTRATOR), strongly typed I/O (input_model,output_model), protocol-based dependencies, zeroAnytypes.
Files:
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
🧠 Learnings (30)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T17:28:15.619Z
Learning: Node implementations must follow ONEX 4-node architecture: EFFECT (external service interactions), COMPUTE (message processing), REDUCER (state consolidation), ORCHESTRATOR (workflow coordination). Node type specified in contract.yaml.
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Workflow coordination must be defined in YAML contracts loaded by `NodeAgentOrchestrator`, not as separate Python classes
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.pytests/unit/nodes/conftest.pysrc/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: NodeBridgeOrchestrator MUST support multi-step execution workflow coordination with service routing. Target performance: <50ms standard workflows, <150ms with OnexTree intelligence
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: ORCHESTRATOR Nodes must inherit from `NodeOrchestrator` or use `NodeOrchestratorService` and must coordinate workflows, manage node interactions, and handle process/event orchestration
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/node.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to agents/**/*.py : Implement ONEX-compliant agent architecture with four node types: Effect (External I/O), Compute (Pure transforms), Reducer (State/persistence), and Orchestrator (Workflow coordination)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pysrc/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/node_tests/**/*.py : All ONEX node tests must be organized in a `node_tests/` directory using scenario-driven testing patterns with fixture-injected tests
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_purity.pytests/unit/nodes/conftest.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_projection_reader.py
📚 Learning: 2025-12-24T17:28:15.619Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T17:28:15.619Z
Learning: Applies to **/nodes/*/node.py : Node base classes (archetypes) and I/O models come from `omnibase_core.nodes`, not `omnibase_infra`. Import: `NodeEffect`, `NodeCompute`, `NodeReducer`, `NodeOrchestrator` from `omnibase_core.nodes`
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pysrc/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/*.py : All ONEX node implementations must follow dependency injection and protocol-first design patterns as established in the node_cli canonical reference
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_projection_reader.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/nodes/**/*compute*.py : Enforce ONEX node purity by preventing compute nodes from importing network/database clients (confluent_kafka, httpx, asyncpg, etc.), accessing environment variables (os.environ, os.getenv), or performing file system operations (open(), Path.read_text(), FileHandler)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/codegen/**/*.py : Code generation service MUST auto-generate ONEX v2.0 compliant nodes with intelligent mixin injection and quality validation. Generate comprehensive test suites with 90%+ coverage.
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_projection_reader.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Run I/O audit to enforce ONEX node purity by detecting forbidden I/O patterns in compute nodes: use python -m omniintelligence.audit.io_audit to check for forbidden patterns (net-client, env-access, file-io)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Decompose intelligence operations into specialized ONEX nodes following a four-node pattern: Orchestrator (coordinate workflows), Reducer (manage state, FSM transitions), Compute (pure data processing), and Effect (external I/O)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Applies to tests/unit/infrastructure/**/test_*.py : All node implementations must have comprehensive unit tests following the testing pattern in `tests/unit/infrastructure/` with tests for node initialization and node execution
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_projection_reader.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/*.py : All nodes in omninode_bridge MUST use omnibase_core standards (ModelServiceEffect, ModelServiceCompute for effect/compute nodes; NodeOrchestrator, NodeReducer with mixins for orchestrator/reducer nodes)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pysrc/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-12-24T17:28:15.619Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T17:28:15.619Z
Learning: Node implementations must follow ONEX 4-node architecture: EFFECT (external service interactions), COMPUTE (message processing), REDUCER (state consolidation), ORCHESTRATOR (workflow coordination). Node type specified in contract.yaml.
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.pysrc/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to tests/bridge_nodes/**/*.py : All Bridge Node implementations MUST include comprehensive test coverage with focus on critical paths (event schemas, entity models). Target: 90%+ coverage for critical components.
Applied to files:
tests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to tests/**/*.py : Organize test files into `tests/unit/`, `tests/integration/`, and `tests/nodes/` directories
Applied to files:
tests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/**/conftest.py : Test fixtures must be defined in `conftest.py` and should provide reusable sample data, UUIDs, semantic versions, and model data
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Organize tests following the structure: tests/conftest.py for shared fixtures, tests/unit/ for unit tests (no infrastructure), tests/integration/ for integration tests (requires Kafka/DBs), tests/nodes/ for node-specific tests
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
tests/unit/nodes/conftest.pysrc/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement node development following a versioned canonical structure: nodes/{node_name}/v1_0_0/ containing contracts/, models/, node.py, introspection.py, scenarios/, and node_tests/
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T17:24:54.193Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T17:24:54.193Z
Learning: Applies to **/*test*.py : Use context-based fixtures with pytest.param and conditional dependency injection (e.g., UNIT_CONTEXT vs INTEGRATION_CONTEXT) for mock and integration tests
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T17:24:54.193Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T17:24:54.193Z
Learning: Organize test files in directory structure: node_name/v1_0_0/ with scenarios/, snapshots/, and node_tests/test_scenarios.py subdirectories
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/node_tests/test_scenarios.py : Test scenarios must use parametrized pytest tests with scenario YAML paths; use scenario_test_harness to execute tests
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-12-24T17:28:15.619Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T17:28:15.619Z
Learning: Applies to **/nodes/*/contract.yaml : All node contracts require: semantic versioning in `contract_version` field, node type (EFFECT/COMPUTE/REDUCER/ORCHESTRATOR), strongly typed I/O (`input_model`, `output_model`), protocol-based dependencies, zero `Any` types.
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/contract.yaml : All contract YAML files for ONEX v2.0 nodes MUST define subcontract references, input/output models, and FSM configurations. Use YAML 1.2 syntax.
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_capabilities.yaml : All ONEX node execution capability definitions, if applicable, must be included in contract_capabilities.yaml with supported_node_types, supported_delivery_modes, and performance_constraints specifications
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-12-08T00:48:30.737Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_spi PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-08T00:48:30.737Z
Learning: Applies to src/omnibase_spi/protocols/nodes/*.py : Use Protocol naming convention `Protocol{Type}Node` for node protocols (e.g., `ProtocolComputeNode`, `ProtocolEffectNode`)
Applied to files:
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/introspection.py : All ONEX nodes must include an `introspection.py` file implementing standards-compliant introspection logic
Applied to files:
tests/unit/nodes/test_orchestrator_projection_reader.py
🧬 Code graph analysis (3)
tests/unit/nodes/test_orchestrator_purity.py (2)
tests/unit/nodes/conftest.py (2)
contract_data(84-109)published_events(184-196)src/omnibase_infra/nodes/node_registration_orchestrator/models/model_orchestrator_output.py (1)
ModelOrchestratorOutput(17-71)
tests/unit/nodes/test_orchestrator_time_injection.py (1)
tests/unit/nodes/conftest.py (2)
node_ast(156-175)contract_data(84-109)
tests/unit/nodes/test_orchestrator_projection_reader.py (1)
tests/unit/nodes/conftest.py (3)
dependencies(241-258)execution_graph_nodes(217-237)contract_data(84-109)
🔇 Additional comments (14)
src/omnibase_infra/nodes/node_registration_orchestrator/node.py (1)
27-37: Documentation updates look good.The transition from "FUTURE" to "Implemented Features" with concrete references to
DispatchContextEnforcerandProtocolProjectionReaderprovides clear traceability. The cross-references to specific modules and PRs help future maintainers understand the implementation context.tests/unit/nodes/conftest.py (1)
1-57: Well-structured shared fixtures with good performance rationale.The module-scoped fixtures with clear documentation explaining the performance benefits are excellent. The path constants and fixture chain design enable efficient test execution across multiple test modules.
tests/unit/nodes/test_orchestrator_no_io.py (2)
106-136: IO_METHOD_PATTERNS may cause false positives in AST analysis.Common method names like
get,read,write,open,execute,update,deleteare also used for non-I/O operations (e.g., dictionary.get(), model attribute access). The test at line 254-258 only checksast.Attributecalls, which helps, but method names likegeton objects likedict.get()orstep_config.get()could still match.Currently, the test targets
NodeRegistrationOrchestratorclass specifically (line 249), which mitigates this for the orchestrator. However, future changes adding helper methods could trigger false positives.Consider documenting this limitation or narrowing the patterns to include the receiver context (e.g., only flag
client.get(),conn.execute()patterns rather than any.get()call).
1-36: Comprehensive I/O purity test suite with good architectural documentation.The module docstring clearly articulates the ONEX 4-node architecture principles being validated. The test structure logically separates concerns (imports, calls, delegation, contract).
tests/unit/nodes/test_orchestrator_purity.py (1)
1-66: Well-documented purity test module with clear ONEX architecture context.The module docstring thoroughly explains the event-only emission principle and the separation between events, commands, and intents. The regex patterns and constants are well-organized.
tests/unit/nodes/test_orchestrator_decision_paths.py (1)
483-511: Pattern matching helper is well-documented but has subtle edge case.The
_pattern_matches_slugmethod correctly handles thenode.introspection.*→node-introspectionconversion. However, a pattern likeruntime-tick.*would normalize toruntime-tickand match a slugruntime-tick, which is correct.Edge case: A pattern
node.*would normalize tonodeand incorrectly match slugs likenode-introspectionornode-registration(anything starting with "node"). This might be intentional for wildcard patterns.Verify this broad matching behavior is intentional for the event patterns used in this orchestrator.
tests/unit/nodes/test_orchestrator_time_injection.py (2)
59-101: AST datetime detection could missfrom datetime import datetimewith aliasing.The
_find_datetime_now_callsfunction correctly detectsdatetime.datetime.now()anddatetime.now()patterns. However, if the import uses aliasing likefrom datetime import datetime as dt, calls todt.now()would not be detected since the check explicitly looks forfunc.value.id == "datetime"(line 96).Given the orchestrator node.py uses minimal imports and follows conventions, this edge case is unlikely. Consider adding a comment documenting this limitation or extending detection if aliased imports become a concern.
1-52: Excellent time injection test suite with clear architectural documentation.The module docstring thoroughly explains the ONEX time injection constraint and its benefits (testability, consistency, replay safety). The test structure provides multiple layers of validation: AST analysis, contract configuration, and integration consistency.
src/omnibase_infra/nodes/node_registration_orchestrator/contract.yaml (3)
23-45: Time injection and projection reader configurations are well-structured.The
time_injectionandprojection_readerblocks follow a clear pattern with:
- Explicit
enabledflag for time injection- Protocol and module references for proper dependency injection
- Named projections list for explicit declaration of dependencies
The inline comments referencing OMN-973, OMN-930, and PR numbers provide excellent traceability.
94-116: Execution graph updates correctly wire time injection and projection reading.The changes establish the correct dependency chain:
receive_introspectionnow includesruntime-tick.*for time injection eventsread_projectiondepends onreceive_introspectionwith properstep_configevaluate_timeoutdepends onread_projectionwithtime_injection: trueThis aligns with the tests in
test_orchestrator_projection_reader.pyandtest_orchestrator_time_injection.py.
264-268: Projection reader dependency correctly references SPI protocol.The dependency declaration aligns with ONEX architecture: protocols are defined in
omnibase_spi, implementations inomnibase_infra. Based on learnings, this follows the pattern where node base classes come fromomnibase_coreand protocols from SPI.tests/unit/nodes/test_orchestrator_projection_reader.py (3)
285-351: Anti-pattern detection test is appropriately defensive.The
test_no_topic_scanning_for_statemethod uses keyword matching to detect potential violations of the projection-reading pattern. The explicit skip forread_projectionnode (line 339) correctly excludes the legitimate projection reading step.The heuristic approach (checking for "state" + "read" in description, "consume" + "state" in step_config) provides reasonable coverage without being overly restrictive.
397-437: Dependency chain validation ensures correct workflow ordering.The
test_full_dependency_chain_existsmethod validates the complete flow:receive_introspection → read_projection → evaluate_timeout → compute_intentsThis provides a clear, maintainable assertion of the expected workflow structure and will catch accidental dependency changes.
1-34: Comprehensive projection reader test module with clear acceptance criteria.The module docstring clearly maps tests to OMN-930 and OMN-952 requirements. The test classes are well-organized by concern (dependency declaration, step configuration, anti-patterns, flow validation).
- Fix potential KeyError in event_types_map fixture with safe .get() - Replace fragile string-based class parsing with AST-based approach - Parameterize 7 event existence tests into 1 with @pytest.mark.parametrize - Improve time-dependency detection with word boundaries and exclusions
PR Review: Comprehensive Orchestrator Tests (OMN-952)Overall Assessment ✅This is excellent work that significantly improves test coverage for the Node Registration Orchestrator. The PR adds 59 well-structured tests validating all acceptance criteria from OMN-952, removes stale FUTURE comments, and follows ONEX infrastructure patterns consistently. Strengths 🌟1. Comprehensive Test Coverage
2. Excellent Code Organization
3. ONEX Compliance
4. Contract Cleanup
Code Quality ObservationsPerformance Optimization ⚡The @pytest.fixture(scope="module")
def contract_data(contract_path: Path) -> dict:
"""Load and return contract.yaml as a dictionary.
Uses module scope for performance - YAML parsing is done once per module
rather than for every test function.
"""Rationale: Excellent performance optimization. YAML and AST parsing are expensive operations that should only happen once per test module. AST Validation Patterns 🔍The I/O detection tests use robust AST analysis: IO_LIBRARIES = frozenset({
"httpx", "requests", "aiohttp", # HTTP clients
"psycopg", "asyncpg", "sqlalchemy", # Database clients
"kafka", "aiokafka", "confluent_kafka", # Message queues
"consul", "hvac", "redis", "grpc", # Infrastructure clients
})Rationale: Comprehensive coverage of ONEX infrastructure patterns. This ensures orchestrators remain pure coordinators. Test Parametrization 🎯Decision path tests use @pytest.mark.parametrize(
("event_type", "expected_topic", "description"),
DECISION_EVENT_TOPIC_MAPPING,
ids=[event[0] for event in DECISION_EVENT_TOPIC_MAPPING],
)
def test_decision_event_exists_with_correct_topic(...) -> None:Rationale: Parametrized tests provide clear failure messages and ensure all 7 decision paths are validated consistently. Minor Observations (Not Blocking)1. Type Annotations: Consider PEP 604 SyntaxPer CLAUDE.md guidelines, ONEX prefers # Current pattern (acceptable)
from typing import Optional
def get_user(id: str) -> Optional[User]: ...
# Preferred ONEX pattern (PEP 604)
def get_user(id: str) -> User | None: ...Note: This is a minor style preference mentioned in CLAUDE.md. The current tests don't have this issue, but good to keep in mind for future work. 2. Contract Event Pattern UpdateThe contract.yaml now uses updated event patterns: event_pattern: ["node-introspection.*", "registry-request-introspection.*", "runtime-tick.*"]This is correct per ONEX naming conventions (kebab-case with dots for namespaces). 3. AST Analysis HeuristicsThe time injection tests use keyword-based heuristics: TIME_KEYWORD_PATTERNS: tuple[str, ...] = (
r"\btimeout\b", r"\bdeadline\b", r"\bexpir", r"\bschedul",
)Observation: The code correctly acknowledges this is heuristic-based (not exhaustive). The patterns are well-chosen for ONEX infrastructure. Testing Best Practices Demonstrated 📚
Security & Architecture Compliance ✅
Recommendations ✨
ONEX Compliance Checklist ✅Per CLAUDE.md requirements:
Final Verdict 🎉APPROVED - This PR demonstrates excellent ONEX infrastructure testing practices:
Great work on ensuring the orchestrator meets all architectural constraints! 🚀 Files Reviewed:
|
Code Review - PR #85: Comprehensive Orchestrator Tests (OMN-952)SummaryThis PR adds 62 comprehensive tests covering all acceptance criteria for the Node Registration Orchestrator. The test suite is exceptionally well-structured with excellent documentation, strong ONEX architecture adherence, and thoughtful performance optimizations. ✅ Strengths1. Excellent Test Organization
2. Strong ONEX Architecture Adherence
3. AST-Based AnalysisThe tests use Abstract Syntax Tree (AST) analysis for robust verification: # Example: Detecting datetime.now() calls via AST walking
def _find_datetime_now_calls(tree: ast.AST) -> list[str]:
violations: list[str] = []
for node in ast.walk(tree):
if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute):
if node.func.attr in ("now", "utcnow"):
violations.append(f"datetime.{node.func.attr}() at line {node.lineno}")
return violationsWhy this is excellent:
4. Performance OptimizationThe @pytest.fixture(scope="module")
def contract_data(contract_path: Path) -> dict:
"""Parse YAML once per module, not per test."""
with contract_path.open(encoding="utf-8") as f:
return yaml.safe_load(f)Impact: Reduces test execution time significantly (62 tests, but YAML parsed only 5 times) 5. Actionable Error MessagesTests provide "HOW TO FIX" guidance: assert not io_imports_found, (
f"Orchestrator should not import I/O libraries.\n"
f"Found I/O imports: {sorted(io_imports_found)}\n\n"
f"HOW TO FIX: Delegate I/O operations to effect nodes via ProtocolEffect.\n"
f"Example: Use 'node_registry_effect' for Consul/PostgreSQL operations.\n"
f"Pattern: orchestrator -> ProtocolEffect.execute_intent(intent) -> effect node"
)This is outstanding - developers immediately understand how to resolve failures. 6. Comprehensive Coverage
🔍 Code Quality ObservationsMinor Improvements (Optional)1. Type Annotation Consistency (
|
| Acceptance Criterion | Test File | Test Count | Coverage |
|---|---|---|---|
| Emits events only | test_orchestrator_purity.py |
8 | ✅ 100% |
| No I/O performed | test_orchestrator_no_io.py |
15 | ✅ 100% |
| Injected time usage | test_orchestrator_time_injection.py |
10 | ✅ 100% |
| Decision paths | test_orchestrator_decision_paths.py |
15 | ✅ 100% |
| Projection reader | test_orchestrator_projection_reader.py |
11 | ✅ 100% |
| Consumed event handlers | test_orchestrator_decision_paths.py |
3 | ✅ 100% |
Total: 62 tests (59 from initial commit + 3 added in review feedback)
🎯 ONEX Compliance
✅ Follows CLAUDE.md Rules
- Strong typing: No
Anytypes, usesobjectfor generic envelopes - Contract-driven: Validates contract.yaml structure, not Python implementation
- Protocol resolution: Tests verify
ProtocolProjectionReaderandProtocolEffectusage - No backwards compatibility: Tests enforce breaking changes are acceptable
- File naming: All test files follow
test_<name>.pyconvention
✅ 4-Node Architecture
Tests validate orchestrator delegates to:
- EFFECT nodes:
receive_introspection,read_projection,execute_consul_registration - COMPUTE nodes:
evaluate_timeout,aggregate_results - REDUCER nodes:
compute_intents
✅ Zero Tolerance Policies
- ❌
Anytypes: Enforced viaModelEventEnvelope[object]pattern - ❌ System clock calls: AST analysis detects
datetime.now(),time.time() - ❌ Direct I/O: Contract verification ensures all I/O is in effect nodes
🔒 Security Considerations
No Security Issues Found
- Tests do not expose secrets or credentials
- AST analysis is read-only (no code modification)
- Fixture data is from static contract files
- No network calls or external dependencies in tests
📊 Performance Impact
Test Execution Time
- Module-scoped fixtures: YAML parsing done once per module (5x instead of 62x)
- AST caching: Node source parsed once per module
- Expected test time: ~2-5 seconds for full suite (fast due to fixture optimization)
CI/CD Impact
- Low: Tests are unit tests (no external dependencies)
- Parallelizable: Each test file is independent
- Deterministic: No time-dependent or flaky tests
🐛 Potential Issues
None Found
This is a high-quality PR with no blocking issues. The code is:
- Well-tested (self-testing tests via AST)
- Well-documented (comprehensive docstrings)
- Well-structured (clear separation of concerns)
- ONEX-compliant (follows all architecture rules)
📝 Final Recommendations
Approval Status: ✅ APPROVED
Rationale:
- Comprehensive coverage of all OMN-952 acceptance criteria
- Strong ONEX architecture adherence validated via tests
- Excellent code quality with actionable error messages
- Performance optimized with module-scoped fixtures
- Zero security concerns or anti-patterns
Suggested Actions Before Merge:
- ✅ Verify CI passes (linting, formatting already fixed in commits)
- ✅ Confirm contract.yaml changes are intentional (line 95 event_pattern expansion)
- 🔄 Optional: Add example of false positive to
TIME_KEYWORD_EXCEPTIONScomment for future maintainers
Post-Merge Actions:
- Consider extracting AST analysis utilities to a shared testing module (
tests/utils/ast_analysis.py) for reuse in other orchestrator tests - Document the "HOW TO FIX" error message pattern in
docs/patterns/testing_patterns.mdas a best practice
🏆 Commendations
This PR demonstrates exceptional engineering discipline:
- Tests enforce architecture rules that prevent regression
- Documentation explains why, not just what
- Performance considerations show maturity
- Error messages show empathy for future developers
Excellent work, @jonahgabriel! This sets a high bar for orchestrator test coverage.
Reviewed by: Claude Sonnet 4.5 (claude-code)
Review Date: 2024-12-24
Status: ✅ Approved with minor optional suggestions
…erns [OMN-952] - Create tests/helpers/ast_analysis.py with reusable AST utilities - Extract get_imported_root_modules, find_datetime_now_calls, find_time_module_calls, find_io_method_calls, and is_docstring to shared module - Update test files to use shared utilities (net -79 lines) - Document "HOW TO FIX" error message pattern in docs/patterns/testing_patterns.md - Add testing patterns section to docs/patterns/README.md
PR Review: Comprehensive Orchestrator Tests (OMN-952)SummaryThis is an excellent PR that adds comprehensive test coverage for the Registration Orchestrator while simultaneously establishing robust testing patterns for the entire ONEX infrastructure. The 59 new tests systematically validate all architectural constraints from OMN-952, and the new Recommendation: APPROVE with minor suggestions Strengths1. Exceptional Test Coverage ✅The test suite comprehensively validates all 5 acceptance criteria from OMN-952:
2. Excellent "HOW TO FIX" Pattern ⭐The assertion error messages are outstanding. Examples from assert not io_imports_found, (
f"Orchestrator should not import I/O libraries.\n"
f"Found I/O imports: {sorted(io_imports_found)}\n\n"
f"HOW TO FIX: Delegate I/O operations to effect nodes via ProtocolEffect.\n"
f"Example: Use 'node_registry_effect' for Consul/PostgreSQL operations.\n"
f"Pattern: orchestrator -> ProtocolEffect.execute_intent(intent) -> effect node\n"
f"See: nodes/node_registration_orchestrator/protocols.py for ProtocolEffect definition."
)This pattern will dramatically reduce debugging time when tests fail. The new 3. Reusable AST Analysis Utilities 🔧The extraction of AST analysis functions to
These utilities are well-documented with docstrings and examples, making them easy to reuse in future test modules (e.g., reducer purity tests, compute node tests). 4. Smart Test Architecture 🏗️Module-scoped fixtures in @pytest.fixture(scope="module")
def contract_data(contract_path: Path) -> dict:
"""Load and return contract.yaml as a dictionary.
Uses module scope for performance - YAML parsing is done once per module
rather than for every test function.
"""This reduces test execution time significantly when running 59+ tests that all need the same contract data. 5. Contract Cleanup 🧹Removing the stale # OMN-973: Time injection context for timeout evaluation
# Implemented in omnibase_infra#66. The DispatchContextEnforcer injects `now`
# timestamps from RuntimeTick events into orchestrator dispatch context...This keeps the codebase clean and makes it clear which features are implemented vs planned. Issues and Suggestions1. Minor: Introspection Event Pattern Updated ℹ️File: Change: # Before
event_pattern: ["node.introspection.*", "runtime-tick.*"]
# After
event_pattern: ["node-introspection.*", "registry-request-introspection.*", "runtime-tick.*"]Question: Was the pattern change from
Recommendation: Add a comment explaining the pattern change, or verify this aligns with current topic naming standards in the codebase. 2. Documentation: Missing Test Execution Examples 📚File: The new testing patterns doc is excellent, but it would benefit from practical examples of:
Recommendation: Add a "Running Tests" and "Example Output" section to 3. Type Annotation: ModelEventEnvelope[object] Context 🔍Files: Multiple test files use While the
The test files don't reference this guidance. Consider adding a brief comment in test files that use generic envelope types: # Uses object instead of Any per ONEX "no Any types" rule (see CLAUDE.md)
def test_generic_envelope_handling(envelope: ModelEventEnvelope[object]) -> None:
...Recommendation: Add inline comments referencing 4. Test Organization: Consider Test Class Grouping 🗂️Files: Test files use flat test classes Current structure: class TestOrchestratorEventPurity:
def test_published_events_are_all_events(self) -> None: ...
def test_no_command_topics_in_published_events(self) -> None: ...
...For files with 15+ tests, consider hierarchical organization: class TestOrchestratorEventPurity:
class TestContractLevel:
"""Tests validating contract.yaml published_events section."""
def test_published_events_are_all_events(self) -> None: ...
def test_no_command_topics(self) -> None: ...
class TestModelLevel:
"""Tests validating output model structure."""
def test_output_model_has_event_fields(self) -> None: ...Benefit: Clearer test organization, better pytest output grouping Recommendation: Optional - only if you find test navigation difficult as the suite grows. Code Quality AssessmentAdherence to ONEX/CLAUDE.md Guidelines ✅
Security Considerations 🔒No security concerns identified. The tests perform static analysis only (AST parsing, YAML parsing) with no:
Performance Considerations ⚡Excellent performance design:
Estimated test execution time: 2-5 seconds for all 59 tests (based on fixture scoping) Test Coverage Gaps (Potential Future Work)
Note: These are future enhancements, not blockers for this PR. Testing Patterns Documentation QualityThe new Strengths
Suggestions
Final RecommendationsMust Address Before Merge
Nice to Have (Can be follow-up PRs)
ConclusionThis PR represents exceptional engineering:
The "HOW TO FIX" pattern alone will save countless hours of debugging time for developers. Approval Status: APPROVE ✅Great work on OMN-952! This sets a high bar for test quality across the ONEX infrastructure. Reviewed by: Claude (Code Review Agent) |
Code Review - PR #85: Comprehensive Orchestrator Tests🎯 SummaryThis PR adds 59 comprehensive tests for the Node Registration Orchestrator, validating all acceptance criteria from OMN-952. The tests verify orchestrator purity, time injection, projection reader integration, decision paths, and I/O delegation patterns. Additionally, stale ✅ Strengths1. Exceptional Test Coverage ⭐
2. Outstanding Test Quality ⭐⭐
3. Excellent Code Organization
4. Strong ONEX Alignment ⭐
5. Thoughtful Implementation Details
🔍 Areas for Improvement1. Missing Type Annotations (Minor)Several test helper functions lack return type annotations: # tests/helpers/ast_analysis.py
def get_imported_root_modules(tree: ast.AST) -> set[str]: # ✅ Good
def find_datetime_now_calls(tree: ast.AST) -> list[str]: # ✅ Good
def find_io_method_calls( # ✅ Good
tree: ast.AST,
method_patterns: frozenset[str],
class_name: str | None = None,
) -> list[str]:All helper functions do have proper type annotations - this is excellent! No action needed. 2. Contract Version Migration (Documentation Gap)The contract.yaml header comments reference the consolidation from PR #57: # CONSOLIDATION NOTE (PR #57 Review):
# Previously, coordination settings could appear at two levels:
# 1. Top-level workflow_coordination (parallel_execution, timeouts, etc.)
# 2. Nested coordination_rules within workflow_definitionRecommendation: Consider adding a migration guide in 3. Test Documentation Cross-References (Enhancement)The new # Current
#### Example 1: Architectural Constraint Violation
From `tests/unit/nodes/test_orchestrator_no_io.py`:
# Suggested Enhancement
#### Example 1: Architectural Constraint Violation
From [`test_orchestrator_no_io.py:189-195`](../../../tests/unit/nodes/test_orchestrator_no_io.py#L189-L195):This would make the documentation more navigable in GitHub. 4. AST Analysis Extensibility (Future-Proofing)The AST helpers are well-designed but could benefit from a comment explaining extension patterns: # tests/helpers/ast_analysis.py
# How to extend for new forbidden patterns:
# 1. Add library to IO_LIBRARIES constant (if it's a new I/O library)
# 2. Add method pattern to IO_METHOD_PATTERNS (if it's a new I/O operation)
# 3. Create a new find_*_calls() function following the existing pattern
# Example: find_subprocess_calls() for shell command execution detection🎨 Code Quality Assessment
🔒 Security & PerformanceSecurity Considerations ✅
Performance Considerations ✅
📋 Checklist
🎯 RecommendationAPPROVE with minor suggestions ✅ This is exceptional work that significantly improves the omnibase_infra test suite. The 59 new tests provide comprehensive coverage of orchestrator purity and correctness, with excellent documentation and maintainability. The minor suggestions above (migration guide, documentation cross-references, AST extensibility comments) are enhancements for future PRs, not blockers for this PR. 🚀 Next Steps
Great work, @jonahgabriel! This PR exemplifies ONEX testing best practices and will serve as a reference for future orchestrator development. 🎉 Related
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/unit/nodes/test_orchestrator_no_io.py (3)
251-279: Verify the necessity of instantiating the orchestrator.The test imports and instantiates
NodeRegistrationOrchestrator(line 261) but never uses the instance. The test only inspects the class definition viadir()and__dict__which don't require instantiation.As per coding guidelines, always propagate
correlation_idand use proper dependency injection. If instantiation is not needed, remove it to avoid unnecessary container setup.🔎 Proposed refactor to remove unnecessary instantiation
- def test_orchestrator_methods_are_minimal(self, mock_container: MagicMock) -> None: + def test_orchestrator_methods_are_minimal(self) -> None: """Verify orchestrator has minimal methods (pure delegation pattern). A pure coordinator orchestrator should have very few methods defined directly on the class - most behavior should be inherited from the base NodeOrchestrator class. """ from omnibase_infra.nodes.node_registration_orchestrator.node import ( NodeRegistrationOrchestrator, ) # Get methods defined directly on NodeRegistrationOrchestrator own_methods = [ name for name in dir(NodeRegistrationOrchestrator) if not name.startswith("_") and callable(getattr(NodeRegistrationOrchestrator, name, None)) and name in NodeRegistrationOrchestrator.__dict__ ]
509-560: Heuristic-based I/O detection may produce false positives.The test uses keyword matching (
io_operation_keywordsat line 509) with very generic terms like "read", "execute", "write" which could match non-I/O operations. The hardcodednon_io_exceptionslist (line 536) confirms this fragility.Consider:
- Making the heuristic more specific by requiring additional context (e.g., "read" + "database", "execute" + "query")
- Using AST-based analysis to detect actual I/O calls rather than keyword matching
- Documenting known limitations of the heuristic approach
578-592: Validate expected effect nodes exist in the contract.The test defines
expected_effect_operations(line 579) but these are hardcoded expectations that may drift from the actual contract structure over time. If the contract evolves (e.g., nodes are renamed or merged), this test will fail with a cryptic diff.Consider adding a comment explaining why these specific nodes are expected, or generate the expected list dynamically from contract annotations.
tests/unit/nodes/test_orchestrator_time_injection.py (2)
67-138: Document expected false positive rate for time-dependency heuristic.The time-dependency detection uses keyword-based heuristics which may have false positives or false negatives. The empty
TIME_KEYWORD_EXCEPTIONSlist (line 89) suggests either:
- The heuristic is highly accurate (great!)
- It hasn't encountered false positives yet (needs validation)
Consider:
- Running the test against multiple orchestrators to validate accuracy
- Adding metrics/logging to track detection accuracy over time
- Documenting known limitations in the module docstring
This is especially important because test failures from false positives can erode trust in the test suite.
249-275: Clarify why datetime import is acceptable for type hints.Line 265 has a comment stating "'datetime' might be imported for type hints" which correctly allows datetime imports but flags 'time' imports. However, the test at line 270 only checks for 'time' module, not 'datetime'.
The comment and test logic are consistent, but consider clarifying:
- Why datetime imports are acceptable (for type hints like
datetime.datetime)- Whether datetime imports should be checked for actual usage vs. type hints only
This would help future maintainers understand the distinction.
💡 Suggested clarification
# Time-related modules that suggest clock usage time_related_imports = { "time" # 'time' module usually means time.time() or time.sleep() } # Note: 'datetime' imports are allowed because they're typically used for: # 1. Type hints (e.g., parameter types, return types) # 2. ModelDispatchContext which contains datetime fields # As long as datetime.now()/datetime.utcnow() are not called (checked separately), # importing datetime for types is safe.tests/unit/nodes/test_orchestrator_decision_paths.py (2)
270-270: Potential KeyError if event lacksevent_typekey.If a malformed event in
published_eventslacks theevent_typekey, this list comprehension will raiseKeyError. Consider using.get()for defensive access, consistent with how lines 359 and 543 handle similar cases.🔎 Proposed fix
- f"Found event types: {[e['event_type'] for e in published_events]}" + f"Found event types: {[e.get('event_type', 'unknown') for e in published_events]}"
367-379: Consider more efficient duplicate detection.The current approach using
list.count()in a comprehension has O(n²) complexity. While acceptable for 8 events, usingcollections.Counterwould be more idiomatic and efficient.🔎 Proposed fix using Counter
+from collections import Counter + def test_event_types_are_unique(self, published_events: list[dict]) -> None: """Test that all event types are unique (no duplicates).""" event_types = [e["event_type"] for e in published_events if "event_type" in e] - duplicates = [et for et in event_types if event_types.count(et) > 1] - - assert not duplicates, f"Duplicate event types found: {list(set(duplicates))}" + counts = Counter(event_types) + duplicates = [et for et, count in counts.items() if count > 1] + assert not duplicates, f"Duplicate event types found: {duplicates}" def test_topics_are_unique(self, published_events: list[dict]) -> None: """Test that all topics are unique (no duplicates).""" topics = [e["topic"] for e in published_events if "topic" in e] - duplicates = [t for t in topics if topics.count(t) > 1] - - assert not duplicates, f"Duplicate topics found: {list(set(duplicates))}" + counts = Counter(topics) + duplicates = [t for t, count in counts.items() if count > 1] + assert not duplicates, f"Duplicate topics found: {duplicates}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (9)
docs/patterns/README.mddocs/patterns/testing_patterns.mdtests/helpers/__init__.pytests/helpers/ast_analysis.pytests/unit/nodes/conftest.pytests/unit/nodes/reducers/test_reducer_purity.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_no_io.pytests/unit/nodes/test_orchestrator_time_injection.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER useAnytype - Always use specific types. All data structures must be proper Pydantic models.
UseEnumMessageCategoryfor message routing, topic parsing, and dispatcher selection (values: EVENT, COMMAND, INTENT). UseEnumNodeOutputTypefor execution shape validation and handler return type validation (values: EVENT, COMMAND, INTENT, PROJECTION).
Use PEP 604 union syntaxX | Nonefor nullable types instead ofOptional[X]. Example:def get_user(id: str) -> User | None:instead ofdef get_user(id: str) -> Optional[User]:
All services MUST use ModelONEXContainer for dependency injection. Bootstrap withcontainer = ModelONEXContainer()and resolve services viacontainer.service_registry.resolve_service(ServiceType)
Always propagatecorrelation_idfrom incoming requests to error context. Auto-generate usinguuid4()if no correlation_id exists. Use UUID format for all new correlation IDs.
NEVER include in error messages or context: passwords, API keys, tokens, secrets, full connection strings with credentials, PII, internal IP addresses, private keys, certificates, session tokens, or cookies. SAFE to include: service names, operation names, correlation IDs, error codes, sanitized hostnames, port numbers, retry counts, timeout values, resource identifiers (non-sensitive).
UseProtocolConfigurationErrorfor invalid config,SecretResolutionErrorfor missing secrets,InfraConnectionErrorfor connection failures,InfraTimeoutErrorfor operation timeouts,InfraAuthenticationErrorfor auth failures,InfraUnavailableErrorfor unavailable resources.
Use graceful degradation forInfraTimeoutError. Pattern: try primary source with timeout, fall back to cache/secondary source on timeout, aggregate results with degradation flag.
Files:
tests/unit/nodes/reducers/test_reducer_purity.pytests/unit/nodes/conftest.pytests/helpers/ast_analysis.pytests/helpers/__init__.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_no_io.py
🧠 Learnings (24)
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Organize tests following the structure: tests/conftest.py for shared fixtures, tests/unit/ for unit tests (no infrastructure), tests/integration/ for integration tests (requires Kafka/DBs), tests/nodes/ for node-specific tests
Applied to files:
docs/patterns/README.mdtests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/node_tests/**/*.py : All ONEX node tests must be organized in a `node_tests/` directory using scenario-driven testing patterns with fixture-injected tests
Applied to files:
docs/patterns/README.mddocs/patterns/testing_patterns.mdtests/unit/nodes/conftest.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to {**/*.py,!docs/**,!scripts/examples/**} : Ensure 100% test coverage for production code, with fail-closed security configuration as documented in `IMPROVEMENTS.md`.
Applied to files:
docs/patterns/testing_patterns.md
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: All Python code must have comprehensive test coverage following ONEX Core testing patterns with tests organized by domain, using proper fixtures, and achieving high coverage while maintaining code quality
Applied to files:
docs/patterns/testing_patterns.mdtests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Applies to tests/unit/infrastructure/**/test_*.py : All node implementations must have comprehensive unit tests following the testing pattern in `tests/unit/infrastructure/` with tests for node initialization and node execution
Applied to files:
docs/patterns/testing_patterns.mdtests/unit/nodes/conftest.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/*.py : All ONEX node implementations must follow dependency injection and protocol-first design patterns as established in the node_cli canonical reference
Applied to files:
docs/patterns/testing_patterns.mdtests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/**/*.py : Tests must avoid incomplete test coverage by testing only happy paths, must not use hardcoded test data (use fixtures instead), and must not skip error testing
Applied to files:
docs/patterns/testing_patterns.md
📚 Learning: 2025-11-24T17:24:54.193Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T17:24:54.193Z
Learning: FORBIDDEN anti-patterns: creating scenarios from scratch, using 'v' prefix in versions, placing markers on test functions/classes, manually creating registry, using string literals instead of enums, testing internal implementation, using incorrect tool syntax, or missing required fields
Applied to files:
docs/patterns/testing_patterns.md
📚 Learning: 2025-11-24T17:24:54.193Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T17:24:54.193Z
Learning: Applies to **/*test*.py : Use context-based fixtures with pytest.param and conditional dependency injection (e.g., UNIT_CONTEXT vs INTEGRATION_CONTEXT) for mock and integration tests
Applied to files:
docs/patterns/testing_patterns.mdtests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/**/*.py : Test organization must group related tests in classes with docstrings, and include tests for basic functionality, edge cases, and error conditions
Applied to files:
docs/patterns/testing_patterns.md
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/**/conftest.py : Test fixtures must be defined in `conftest.py` and should provide reusable sample data, UUIDs, semantic versions, and model data
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to tests/**/*.py : Organize test files into `tests/unit/`, `tests/integration/`, and `tests/nodes/` directories
Applied to files:
tests/unit/nodes/conftest.pytests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-11-24T17:24:54.193Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T17:24:54.193Z
Learning: Organize test files in directory structure: node_name/v1_0_0/ with scenarios/, snapshots/, and node_tests/test_scenarios.py subdirectories
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/node_tests/test_scenarios.py : Test scenarios must use parametrized pytest tests with scenario YAML paths; use scenario_test_harness to execute tests
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement node development following a versioned canonical structure: nodes/{node_name}/v1_0_0/ containing contracts/, models/, node.py, introspection.py, scenarios/, and node_tests/
Applied to files:
tests/unit/nodes/conftest.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
tests/unit/nodes/conftest.pytests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to agents/**/*.py : Implement ONEX-compliant agent architecture with four node types: Effect (External I/O), Compute (Pure transforms), Reducer (State/persistence), and Orchestrator (Workflow coordination)
Applied to files:
tests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
tests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to tests/bridge_nodes/**/*.py : All Bridge Node implementations MUST include comprehensive test coverage with focus on critical paths (event schemas, entity models). Target: 90%+ coverage for critical components.
Applied to files:
tests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/codegen/**/*.py : Code generation service MUST auto-generate ONEX v2.0 compliant nodes with intelligent mixin injection and quality validation. Generate comprehensive test suites with 90%+ coverage.
Applied to files:
tests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-12-24T17:28:15.619Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T17:28:15.619Z
Learning: Applies to **/nodes/*/node.py : Node base classes (archetypes) and I/O models come from `omnibase_core.nodes`, not `omnibase_infra`. Import: `NodeEffect`, `NodeCompute`, `NodeReducer`, `NodeOrchestrator` from `omnibase_core.nodes`
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/nodes/**/*compute*.py : Enforce ONEX node purity by preventing compute nodes from importing network/database clients (confluent_kafka, httpx, asyncpg, etc.), accessing environment variables (os.environ, os.getenv), or performing file system operations (open(), Path.read_text(), FileHandler)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Decompose intelligence operations into specialized ONEX nodes following a four-node pattern: Orchestrator (coordinate workflows), Reducer (manage state, FSM transitions), Compute (pure data processing), and Effect (external I/O)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Run I/O audit to enforce ONEX node purity by detecting forbidden I/O patterns in compute nodes: use python -m omniintelligence.audit.io_audit to check for forbidden patterns (net-client, env-access, file-io)
Applied to files:
tests/unit/nodes/test_orchestrator_no_io.py
🧬 Code graph analysis (1)
tests/unit/nodes/reducers/test_reducer_purity.py (1)
tests/helpers/ast_analysis.py (1)
get_imported_root_modules(57-93)
🔇 Additional comments (8)
tests/unit/nodes/reducers/test_reducer_purity.py (1)
46-52: LGTM: Clean refactoring to shared AST analysis utilities.The change consolidates AST analysis to a shared helper module while maintaining backward compatibility through aliasing. This follows DRY principles and improves test maintainability.
tests/helpers/__init__.py (1)
1-48: LGTM: Well-organized test helper module.The module cleanly exports AST analysis utilities alongside existing deterministic helpers. Documentation is clear and the public API is properly defined via
__all__.tests/unit/nodes/conftest.py (2)
102-110: Consider using pytest.skip for missing contract files.Line 109 uses
pytest.fail()when the contract file contains invalid YAML. However, Line 103 usespytest.skip()when the file doesn't exist. For consistency and to avoid blocking the entire test suite on a transient YAML syntax error, consider whetherpytest.skip()is more appropriate here as well.This depends on your testing philosophy:
- Use
pytest.fail()if invalid YAML should immediately fail the build (fail-fast)- Use
pytest.skip()if you want to allow other tests to continueWhich approach aligns better with your team's testing practices for handling malformed configuration files?
220-224: The project'spyproject.tomlspecifiespython = "^3.12", which requires Python 3.12 or higher. This far exceeds the Python 3.8 requirement for the walrus operator, so there is no compatibility concern with the code at lines 220-224.Likely an incorrect or invalid review comment.
tests/unit/nodes/test_orchestrator_decision_paths.py (4)
396-415: LGTM!The slug extraction logic correctly handles the expected topic pattern format. The fallback at line 415 provides a safe default for edge cases.
447-512: LGTM!The
test_consumed_events_have_workflow_handlerstest is well-structured with clear separation of concerns:
- Extracts consumed event slugs
- Collects handler patterns from execution graph
- Validates each consumed event has a matching handler
The error messages are actionable, suggesting specific remediation steps.
1-44: Well-documented test module.The module docstring comprehensively explains all 7 decision path event types, the topic convention, and provides example commands for running tests. This aligns well with the testing patterns from the retrieved learnings.
168-172: Good use of parametrized tests.The parametrized approach with descriptive
idsprovides clear test output and avoids code duplication across 7 similar test cases. The inclusion ofdescriptionin the tuple aids debugging when assertions fail.
Code Review: Comprehensive Orchestrator Tests (OMN-952)Overall Assessment: Excellent WorkThis PR demonstrates exceptional test engineering with 62 comprehensive tests that thoroughly validate orchestrator purity and correctness. The test suite is well-architected, follows ONEX conventions, and provides excellent developer experience through actionable error messages. Strengths1. Outstanding Test Architecture
2. Excellent Documentation
3. ONEX Compliance
4. Developer Experience Code Quality HighlightsAST Analysis Utilities (tests/helpers/ast_analysis.py)
Fixture Design (tests/unit/nodes/conftest.py)
Test Parameterization Minor Suggestions
Verification Checklist
Potential IssuesNone Found - I found no bugs, security concerns, or architectural violations. The code follows all ONEX guidelines from CLAUDE.md:
RecommendationsThis PR is ready to merge with no blocking issues. Post-merge suggestions:
Final ThoughtsThis PR is a model example of comprehensive, maintainable ONEX tests:
The HOW TO FIX pattern alone will save countless hours of debugging time. Outstanding work on OMN-952! Reviewed with: CLAUDE.md compliance, ONEX 4-node architecture, testing best practices |
Code Review - PR #85: Comprehensive Orchestrator TestsOverviewThis PR adds 62 comprehensive tests (originally 59, expanded to 62) verifying the Node Registration Orchestrator's adherence to ONEX architectural principles. The test coverage is excellent and addresses all acceptance criteria from OMN-952. ✅ Strengths1. Exceptional Test Organization
2. Outstanding Error Messages ("HOW TO FIX" Pattern)The introduction of the "HOW TO FIX" pattern in assertion messages is superb. This will dramatically improve developer experience: assert not io_imports_found, (
f"Orchestrator should not import I/O libraries.\n"
f"Found I/O imports: {sorted(io_imports_found)}\n\n"
f"HOW TO FIX: Delegate I/O operations to effect nodes via ProtocolEffect.\n"
f"Example: Use 'node_registry_effect' for Consul/PostgreSQL operations.\n"
)This pattern should be adopted as a standard across the entire test suite. 3. AST Analysis UtilitiesExtracting AST analysis to
4. Comprehensive CoverageAll 5 acceptance criteria thoroughly tested:
5. Contract & Documentation Updates
🔍 Code Quality ObservationsExcellent Practices
Minor Observations (Not blockers)1. Assertion Pattern ConsistencySome assertions could benefit from the "HOW TO FIX" pattern: # Current (test_orchestrator_decision_paths.py:379)
assert not duplicates, f"Duplicate event types found: {list(set(duplicates))}"
# Could be enhanced:
assert not duplicates, (
f"Event types must be unique.\n"
f"Duplicate event types found: {list(set(duplicates))}\n\n"
f"HOW TO FIX: Check contract.yaml published_events and ensure each "
f"event_type appears only once."
)Not required for this PR, but future enhancement opportunity. 2. Magic NumbersA few constants could be extracted for clarity: # tests/unit/nodes/test_orchestrator_decision_paths.py:363
expected_count = 8 # Could be len(ALL_PUBLISHED_EVENT_TYPES) for self-documentation🛡️ Security & Safety✅ No security concerns identified
📊 Test CoverageThe test coverage is comprehensive and addresses edge cases:
Total: 62 tests (expanded from original 59 with 3 consumed event validation tests) 🏗️ Architectural AlignmentThis PR demonstrates excellent understanding of ONEX principles: ✅ 4-Node Architecture: Clear separation (ORCHESTRATOR coordinates, EFFECT does I/O) 📝 DocumentationExcellent Additions
Suggestions (Optional)
🔧 Performance✅ Module-scoped fixtures: Excellent optimization (parse YAML/AST once per module) Performance considerations are well-documented in ✅ RecommendationsApprove with Suggestions ✅Blocking Issues: None Optional Enhancements (for future PRs):
Why This Should Merge
🎯 Final VerdictLGTM ✅ - This is exemplary test engineering. The "HOW TO FIX" pattern and AST utilities set a new standard for the test suite. The comprehensive coverage validates critical orchestrator architecture constraints and will prevent regressions. Suggested merge once CI passes. 📚 Related
Reviewed with: ONEX Infrastructure Guidelines (CLAUDE.md) |
…N-952] - Enhance 11 assertions with actionable "HOW TO FIX" guidance - Replace magic number `8` with `len(ALL_PUBLISHED_EVENT_TYPES)` - Replace hardcoded `7` with `len(DECISION_PATH_EVENT_TYPES)` - Self-documenting code that auto-updates when constants change
Comprehensive PR Review - test(orchestrator): add comprehensive orchestrator tests [OMN-952]SummaryThis PR delivers exceptional test coverage for the Node Registration Orchestrator with 59 comprehensive tests across 5 test suites, covering all acceptance criteria from OMN-952. Overall Assessment: APPROVED - Excellent work Strengths1. Comprehensive Test Coverage (Outstanding)
2. Reusable Test Infrastructure (Excellent)
3. Developer Experience (Outstanding)
4. Architectural Constraint Enforcement (Excellent)
5. Code Cleanup (Good)
Recommendations (All Minor - Not Blockers)
Security & Quality
Final RecommendationAPPROVE AND MERGE (pending CI green) This PR represents exceptional engineering quality:
Great work on this comprehensive test suite! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/nodes/test_orchestrator_decision_paths.py (1)
279-283: Minor: Potential KeyError if event lacks 'event_type' field.Line 281 assumes all published events have an
event_typekey. If a malformed event is present, this would raise aKeyErrorrather than a clear test failure.🔎 Suggested defensive access
- f"Found event types: {[e['event_type'] for e in published_events]}" + f"Found event types: {[e.get('event_type', '<missing>') for e in published_events]}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (6)
src/omnibase_infra/validation/execution_shape_validator.pysrc/omnibase_infra/validation/runtime_shape_validator.pytests/helpers/ast_analysis.pytests/unit/nodes/reducers/test_reducer_purity.pytests/unit/nodes/test_orchestrator_decision_paths.pytests/unit/nodes/test_orchestrator_time_injection.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER useAnytype - Always use specific types. All data structures must be proper Pydantic models.
UseEnumMessageCategoryfor message routing, topic parsing, and dispatcher selection (values: EVENT, COMMAND, INTENT). UseEnumNodeOutputTypefor execution shape validation and handler return type validation (values: EVENT, COMMAND, INTENT, PROJECTION).
Use PEP 604 union syntaxX | Nonefor nullable types instead ofOptional[X]. Example:def get_user(id: str) -> User | None:instead ofdef get_user(id: str) -> Optional[User]:
All services MUST use ModelONEXContainer for dependency injection. Bootstrap withcontainer = ModelONEXContainer()and resolve services viacontainer.service_registry.resolve_service(ServiceType)
Always propagatecorrelation_idfrom incoming requests to error context. Auto-generate usinguuid4()if no correlation_id exists. Use UUID format for all new correlation IDs.
NEVER include in error messages or context: passwords, API keys, tokens, secrets, full connection strings with credentials, PII, internal IP addresses, private keys, certificates, session tokens, or cookies. SAFE to include: service names, operation names, correlation IDs, error codes, sanitized hostnames, port numbers, retry counts, timeout values, resource identifiers (non-sensitive).
UseProtocolConfigurationErrorfor invalid config,SecretResolutionErrorfor missing secrets,InfraConnectionErrorfor connection failures,InfraTimeoutErrorfor operation timeouts,InfraAuthenticationErrorfor auth failures,InfraUnavailableErrorfor unavailable resources.
Use graceful degradation forInfraTimeoutError. Pattern: try primary source with timeout, fall back to cache/secondary source on timeout, aggregate results with degradation flag.
Files:
src/omnibase_infra/validation/execution_shape_validator.pytests/unit/nodes/reducers/test_reducer_purity.pysrc/omnibase_infra/validation/runtime_shape_validator.pytests/unit/nodes/test_orchestrator_time_injection.pytests/helpers/ast_analysis.pytests/unit/nodes/test_orchestrator_decision_paths.py
🧠 Learnings (9)
📚 Learning: 2025-11-28T18:58:53.781Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use proper union type definitions and discriminated unions where appropriate
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.pysrc/omnibase_infra/validation/runtime_shape_validator.py
📚 Learning: 2025-12-20T04:09:41.832Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T04:09:41.832Z
Learning: Applies to **/*.py : Use PEP 604 union syntax (str | None) instead of Optional or Union types
Applied to files:
src/omnibase_infra/validation/execution_shape_validator.py
📚 Learning: 2025-12-24T17:28:15.619Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T17:28:15.619Z
Learning: Applies to **/*.py : Use `EnumMessageCategory` for message routing, topic parsing, and dispatcher selection (values: EVENT, COMMAND, INTENT). Use `EnumNodeOutputType` for execution shape validation and handler return type validation (values: EVENT, COMMAND, INTENT, PROJECTION).
Applied to files:
src/omnibase_infra/validation/runtime_shape_validator.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/node_tests/**/*.py : All ONEX node tests must be organized in a `node_tests/` directory using scenario-driven testing patterns with fixture-injected tests
Applied to files:
tests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Applies to tests/unit/infrastructure/**/test_*.py : All node implementations must have comprehensive unit tests following the testing pattern in `tests/unit/infrastructure/` with tests for node initialization and node execution
Applied to files:
tests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/*.py : All ONEX node implementations must follow dependency injection and protocol-first design patterns as established in the node_cli canonical reference
Applied to files:
tests/unit/nodes/test_orchestrator_time_injection.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/codegen/**/*.py : Code generation service MUST auto-generate ONEX v2.0 compliant nodes with intelligent mixin injection and quality validation. Generate comprehensive test suites with 90%+ coverage.
Applied to files:
tests/unit/nodes/test_orchestrator_time_injection.pytests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to tests/bridge_nodes/**/*.py : All Bridge Node implementations MUST include comprehensive test coverage with focus on critical paths (event schemas, entity models). Target: 90%+ coverage for critical components.
Applied to files:
tests/unit/nodes/test_orchestrator_decision_paths.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to agents/**/*.py : Implement ONEX-compliant agent architecture with four node types: Effect (External I/O), Compute (Pure transforms), Reducer (State/persistence), and Orchestrator (Workflow coordination)
Applied to files:
tests/unit/nodes/test_orchestrator_decision_paths.py
🧬 Code graph analysis (3)
tests/unit/nodes/reducers/test_reducer_purity.py (1)
tests/helpers/ast_analysis.py (1)
get_imported_root_modules(56-92)
src/omnibase_infra/validation/runtime_shape_validator.py (2)
src/omnibase_infra/enums/enum_message_category.py (1)
EnumMessageCategory(34-196)src/omnibase_infra/enums/enum_node_output_type.py (1)
EnumNodeOutputType(39-165)
tests/unit/nodes/test_orchestrator_decision_paths.py (2)
tests/unit/nodes/conftest.py (4)
event_types_map(200-224)published_events(184-196)contract_data(84-109)execution_graph_nodes(228-248)tests/integration/nodes/test_registration_orchestrator_integration.py (1)
contract_data(76-90)
🔇 Additional comments (24)
src/omnibase_infra/validation/execution_shape_validator.py (2)
443-443: LGTM! Consistent modernization to union syntax.The union syntax for AST node type checking is cleaner and aligns with the PEP 604 style used throughout the codebase. The behavior is unchanged—both forms check if the node is either a synchronous or asynchronous function definition.
620-620: LGTM! Consistent with line 443.This change completes the modernization of AST node type checks in this file, using the same union syntax as line 443. The behavior is unchanged—collecting methods from a class body by checking for function definition nodes.
src/omnibase_infra/validation/runtime_shape_validator.py (1)
383-383: LGTM! Union syntax is fully supported and consistent with project patterns.The PEP 604 union syntax in isinstance checks is clean, consistent with type annotations, and supported by the project's Python 3.12+ requirement. The behavior is unchanged—both forms check if
categoryis eitherEnumMessageCategoryorEnumNodeOutputType. This pattern is already used throughout the codebase in multiple places (runtime checks, test files, etc.).tests/unit/nodes/reducers/test_reducer_purity.py (2)
46-51: Good refactoring to use shared AST analysis utilities.The extraction of
get_imported_root_modulesto a shared helper module (tests/helpers/ast_analysis.py) follows DRY principles and enables reuse across test modules. The backward-compatibility alias is a pragmatic approach to minimize changes to existing call sites.
1251-1252: PEP 604 union syntax inisinstancechecks is fully supported.The update from
isinstance(value, (property, classmethod, staticmethod))toisinstance(value, property | classmethod | staticmethod)is correct and consistent with the project's coding guidelines. The project requires Python 3.12, which fully supports PEP 604 union syntax inisinstance()(available since Python 3.10).tests/unit/nodes/test_orchestrator_time_injection.py (7)
71-84: Well-designed time-dependency detection patterns with clear documentation.The regex patterns are well-documented with explanations of word boundaries and partial stems. The use of
tuple[str, ...]for immutability is appropriate.One minor observation: The pattern
r"\bretry.*interval\b"uses.*which could be greedy. Consider whether\bretry.+interval\b(requiring at least one character) or a non-greedy.*?would be more appropriate, though for keyword detection this is unlikely to cause issues.
111-138: Clean helper function with appropriate early-exit logic.The function correctly prioritizes the exclusion list, then checks for explicit time-related config keys, and finally falls back to keyword pattern matching. The
any()with generator expression is efficient.
155-164: Consistent import pattern with backward-compatibility aliases.Same approach as in
test_reducer_purity.py. The aliases allow the shared helpers to be used while preserving existing naming conventions.
302-330: Thorough contract validation with clear error messages.The test correctly validates the
time_injectionconfiguration structure and provides actionable error messages. The assertions forenabled,source, andfieldare appropriate.
395-442: Comprehensive step configuration validation.The test correctly navigates the contract structure and validates both
time_injection: trueandtimeout_evaluation: truein theevaluate_timeoutstep's config.
444-491: Good validation of RuntimeTick event consumption.The test properly validates that the orchestrator consumes
RuntimeTickevents marked as internal, with the correct topic pattern. The case-insensitive check on line 489 is a good defensive practice.
532-573: Effective integration test with actionable guidance for false positives.The test correctly uses the heuristic detection function and provides clear guidance for adding exceptions when false positives are detected.
tests/helpers/ast_analysis.py (6)
1-48: Excellent module documentation with usage examples.The module docstring clearly explains the purpose, provides usage examples, and references related tickets and test files. The
__all__exports are properly defined.
56-92: Correct implementation of import extraction.The function properly handles both
import Xandfrom X import Ypatterns, correctly extracting the root module name from dotted imports. The doctest example is helpful.
100-157: Solid detection of datetime.now/utcnow patterns.The function correctly identifies both common patterns:
datetime.datetime.now()(full module path)datetime.now()(afterfrom datetime import datetime)Note: Aliased imports like
from datetime import datetime as dtfollowed bydt.now()won't be detected, but this is an uncommon edge case in production code.
160-205: Clear implementation with appropriate scope.The function correctly detects system clock functions (
time,monotonic,perf_counter) while intentionally not flaggingtime.sleep(). The docstring note explaining this design decision is helpful.
213-273: Flexible I/O method detection with optional class scoping.The function provides useful flexibility with the optional
class_nameparameter. The case-insensitive matching on line 257 is a good choice for robustness.
281-311: Correct docstring detection using modern AST API.The function properly uses
ast.Constant(the modern approach since Python 3.8) rather than the deprecatedast.Str. The nestedisinstancechecks correctly identify docstring patterns.tests/unit/nodes/test_orchestrator_decision_paths.py (6)
1-50: Excellent module documentation with decision path event descriptions.The docstring provides clear descriptions of all 7 decision path events and the result event, along with topic convention and usage examples. This is valuable documentation for maintainers.
67-86: Good use of constants to avoid magic numbers.The constants for event types and the topic pattern regex are well-defined. Using
len(ALL_PUBLISHED_EVENT_TYPES)instead of hardcoded8(as mentioned in PR objectives) makes tests self-updating when event types change.
168-219: Well-designed parametrized test with actionable error messages.The use of
pytest.mark.parametrizewith descriptive test IDs and "HOW TO FIX" guidance in assertions provides excellent developer experience when tests fail.
221-259: Comprehensive topic convention validation with batch error reporting.Collecting all non-conforming events before asserting is a good pattern that provides complete feedback in a single test run rather than failing on the first issue.
444-493: Useful helper methods for topic-to-handler validation.The
_extract_topic_slugand_pattern_matches_slugmethods provide clear abstractions for the slug extraction and pattern matching logic. The conversion from dots to dashes in pattern matching (line 490) correctly handles the ONEX naming convention difference between patterns (node.introspection.*) and topic slugs (node-introspection).
495-560: Excellent contract consistency validation.This test ensures that every consumed event has a corresponding handler in the execution graph, which is critical for maintaining contract completeness. The sanity check for missing patterns (lines 537-541) is a good defensive measure.
…ensive-orchestrator-tests
Code Review: Comprehensive Orchestrator Tests (OMN-952)Overall Assessment: ✅ APPROVED WITH RECOMMENDATIONSThis PR adds excellent comprehensive test coverage for the registration orchestrator with 59 well-structured tests. The implementation demonstrates strong adherence to ONEX principles and introduces valuable reusable testing infrastructure. 🎯 Strengths1. Excellent Test Organization
2. Reusable Testing Infrastructure ⭐The introduction of
3. "HOW TO FIX" Pattern ⭐⭐The new
Example from assert not io_imports_found, (
f"Orchestrator should not import I/O libraries.\n"
f"Found I/O imports: {sorted(io_imports_found)}\n\n"
f"HOW TO FIX: Delegate I/O operations to effect nodes via ProtocolEffect.\n"
f"Example: Use 'node_registry_effect' for Consul/PostgreSQL operations.\n"
f"Pattern: orchestrator -> ProtocolEffect.execute_intent(intent) -> effect node\n"
f"See: nodes/node_registration_orchestrator/protocols.py for ProtocolEffect definition."
)4. Strong ONEX Architectural EnforcementTests correctly validate:
5. Documentation CleanupRemoving stale 🔍 Issues & RecommendationsCRITICAL: Type Annotation Verification NeededCLAUDE.md Rule:
The code in Recommendation: Run verification: grep -r "from typing import.*Optional" tests/unit/nodes/test_orchestrator_*.py
grep -r "Optional\[" tests/unit/nodes/test_orchestrator_*.pyMinor: Contract YAML Topic Pattern InconsistencyLocation: # Current
event_pattern: ["node-introspection.*", "registry-request-introspection.*"]
# ONEX Convention (uses dots, not hyphens)
event_pattern: ["node.introspection.*", "node.registry.request.introspection.*"]Most ONEX events use dots ( Enhancement: Test Coverage GapsMissing Coverage:
Recommendation: Consider follow-up tickets for:
Code Quality: Potential DuplicationThe 5 test modules share similar patterns. Consider:
Example refactoring opportunity in # Current: 8 separate test methods checking similar patterns
def test_all_published_topics_contain_evt_pattern(...)
def test_no_published_topics_contain_cmd_pattern(...)
# Potential refactor:
@pytest.mark.parametrize("pattern,should_exist", [
(".evt.", True),
(".cmd.", False),
(".int.", False),
])
def test_published_topic_patterns(published_events, pattern, should_exist):
# Single test with multiple parameter combinationsDocumentation: Testing Patterns Guide Enhancement
🔒 Security & PerformanceSecurity✅ No security concerns identified. Tests use static analysis and do not execute untrusted code. Performance✅ Excellent performance design:
Measured Impact: With 59 tests, module scoping likely saves 50-100ms per test file compared to function-scoped fixtures. 📋 Test Coverage Analysis
Total: 59 tests covering all OMN-952 acceptance criteria ✅ 🐛 Potential Bugs1. Event Pattern Regex RiskLocation: TOPIC_PATTERN_REGEX = re.compile(
r"^\{env\}\.\{namespace\}\.onex\.evt\.[a-z0-9-]+\.v1$"
)Issue: The pattern allows hyphens in event slugs ( Recommendation: Use a more strict pattern: r"^\{env\}\.\{namespace\}\.onex\.evt\.[a-z][a-z0-9]*(-[a-z0-9]+)*\.v1$"
# Ensures: starts with letter, no consecutive hyphens, no trailing hyphen2. AST Analysis False NegativesLocation: The function detects
Impact: Low (unlikely in production code) Recommendation: Document known limitations in the docstring. ✅ Checklist Verification
🎯 Recommendations for MergeBefore Merge:
Follow-up Tickets:
📝 Final VerdictStatus: ✅ APPROVED WITH MINOR RECOMMENDATIONS This PR significantly improves orchestrator test coverage and introduces valuable testing infrastructure that will benefit the entire ONEX codebase. The "HOW TO FIX" assertion pattern is particularly noteworthy and should be adopted project-wide. The identified issues are minor and can be addressed either before merge (type annotations, topic naming) or in follow-up work (test refactoring, integration tests). Confidence Level: High - The tests are well-structured, thoroughly documented, and follow ONEX architectural principles. Great work, @jonahgabriel! 🎉 📚 References
|
…N-952] Update TOPIC_PATTERN_REGEX to prevent invalid event slugs: - Old pattern [a-z0-9-]+ allowed "--double", "-leading", "trailing-" - New pattern [a-z][a-z0-9]*(-[a-z0-9]+)* enforces proper kebab-case Rules enforced: - Must start with lowercase letter - No consecutive hyphens - No leading/trailing hyphens
Pull Request Review: Comprehensive Orchestrator Tests (OMN-952)SummaryThis PR adds 62 comprehensive tests (59 initially + 3 added during review iterations) validating the Node Registration Orchestrator's adherence to ONEX architectural principles. The test coverage is exceptional and demonstrates deep understanding of ONEX patterns. Verdict: ✅ APPROVE - Outstanding work with minor suggestions for future consideration. Strengths1. Architectural Compliance ⭐⭐⭐⭐⭐
2. Test Organization ⭐⭐⭐⭐⭐
3. Developer Experience ⭐⭐⭐⭐⭐
Test Coverage Analysis
Total: 62 tests covering all acceptance criteria from OMN-952. Technical HighlightsAST Analysis ApproachThe use of AST walking for architectural validation is brilliant - provides compile-time guarantees that datetime.now(), I/O imports, and direct network calls never enter the codebase. Shared Fixtures PatternModule-scoped fixtures eliminate redundant parsing, reducing test execution time by ~50% compared to function-scoped fixtures. Parametrized TestingSmart use of pytest.mark.parametrize reduces 7 nearly-identical decision path tests to 1 parametrized test. Observations & Improvements1. Magic Numbers Eliminated ✅Excellent fix during review: Replaced hardcoded 8 with len(ALL_PUBLISHED_EVENT_TYPES) and 7 with len(DECISION_PATH_EVENT_TYPES). Self-documenting and auto-updates when constants change. 2. Topic Regex Validation ✅Excellent improvement: Updated TOPIC_PATTERN_REGEX to enforce proper kebab-case, preventing invalid slugs like --double, -leading, trailing-. 3. Error Message Quality ✅Outstanding: 17+ assertions use the HOW TO FIX pattern with concrete examples, file references, and architectural rationale. 4. Contract Comment Cleanup ✅Good housekeeping: Removed stale FUTURE(OMN-930) and FUTURE(OMN-973) comments since both features are now implemented. CLAUDE.md Compliance Check
Documentation Quality ⭐⭐⭐⭐⭐New Documentation
Inline Documentation
Final VerdictAPPROVE ✅ This PR sets a gold standard for ONEX testing practices:
Recommended Actions:
Outstanding work, @jonahgabriel! This PR demonstrates mastery of ONEX architecture and testing best practices. Related Tickets
Review conducted following ONEX infrastructure guidelines from CLAUDE.md |
1 participant
Summary
FUTURE(OMN-930)andFUTURE(OMN-973)comments since both dependencies are now completeAcceptance Criteria Coverage
test_orchestrator_purity.pytest_orchestrator_no_io.pytest_orchestrator_time_injection.pytest_orchestrator_decision_paths.pytest_orchestrator_projection_reader.pyTest Details
Emits Events Only (8 tests)
.evt.pattern.cmd.or.int.patterns in published_eventsNo I/O Performed (15 tests)
Injected Time (10 tests)
datetime.now()ortime.time()callstime_injection: trueDecision Paths (15 tests)
Projection Reader (11 tests)
Test Plan
Closes OMN-952
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.