Repository navigation
feat(introspection): Add node introspection with configurable topics [BREAKING] [OMN-881] - #54
Conversation
Migrate from hardcoded EnumKafkaTopic enum to contract-driven topic configuration, allowing nodes to declare their pub/sub topics in contract.yaml files. Changes: - Add EVENT_STREAMING_TOPICS.md spec (12 topics, LOCKED for MVP) - Update MixinNodeIntrospection with optional topic parameters: - introspection_topic, heartbeat_topic, request_introspection_topic - Topics stored as instance variables with module-level defaults - Fully backwards compatible - Add event_channels section to NodeRegistryEffect contract.yaml: - subscribes_to: introspection.published, heartbeat.published - publishes_to: registered, registration_failed, deregistered - Update test assertions to use topic constants Architecture principle: Topics are contract-defined, not code-hardcoded. Kafka carries events, Postgres is source of truth.
|
Warning Rate limit exceeded@jonahgabriel has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 33 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 (7)
📝 WalkthroughWalkthroughAdds ONEX Kafka event-streaming topic specification and integrates per-instance introspection topic configuration, performance-metrics capture, and extensive topic-validation and contract-driven tests; no production runtime behavioral changes to core APIs beyond mixin/topic configuration and event payload enrichment. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Comment |
PR Review: Contract-Driven Topic Configuration [OMN-881]SummaryThis PR successfully migrates from hardcoded EnumKafkaTopic enum to contract-driven topic configuration. The implementation is well-designed, thoroughly documented, and aligns with ONEX principles. Overall, this is excellent work with only minor suggestions for improvement. Strengths1. Architecture and Design
2. Documentation Quality
3. Implementation Quality
4. Testing
Observations and Minor Suggestions1. Missing Test Coverage for New ParametersIssue: No tests verify the new introspection_topic, heartbeat_topic, and request_introspection_topic parameters work correctly. Suggestion: Add test cases to verify custom topics override defaults and are used during publish operations. Rationale: These tests ensure the contract-driven feature actually works and prevent future regressions. 2. Contract.yaml Integration Not DemonstratedIssue: The contract.yaml adds event_channels section but does not show how nodes load these topics programmatically. Suggestion: Add example code in contract.yaml comment or NodeRegistryEffect implementation showing how to parse event_channels and pass to initialize_introspection. Rationale: Shows developers how to use the new contract-driven approach end-to-end. 3. Documentation: Security Section Could Reference CLAUDE.mdIssue: EVENT_STREAMING_TOPICS.md has good security guidelines but could reference the error sanitization section in CLAUDE.md for consistency. Suggestion: Add cross-reference in section 6.6 (Infrastructure Signals) to CLAUDE.md error sanitization guidelines. 4. Topic Naming Convention - Consider EnumObservation: Topic names are now strings scattered across codebase. Consider creating an EnumONEXTopic enum post-MVP. Rationale:
Note: This can be a follow-up ticket post-MVP. Current string-based approach is acceptable for MVP. Code Style and ConventionsFollows ONEX conventions:
Naming conventions:
Security ConsiderationsProperly addressed:
Performance ConsiderationsNo performance impact:
RecommendationsRequired (Pre-Merge):
Recommended (Can be follow-up):
Final VerdictAPPROVED - This is high-quality work that significantly improves the infrastructure architecture. The migration from hardcoded topics to contract-driven configuration is exactly the right direction for ONEX. The documentation is exceptional, the implementation is clean, and the backwards compatibility strategy is sound. The minor suggestions above would make this even better, but they are not blockers. Great job on OMN-881! Reviewed according to: ONEX CLAUDE.md guidelines, Contract-Driven Development principles, and Infrastructure Error Patterns |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/omnibase_infra/mixins/mixin_node_introspection.py (1)
553-558: Consider explicit empty string handling.The
orpattern correctly handlesNonebut will also treat empty strings""as falsy, falling back to defaults. This is likely the desired behavior (empty string = use default), but worth confirming this edge case is intentional.If explicit
None-only handling is preferred:🔎 Alternative using ternary for explicit None check
- self._introspection_topic = introspection_topic or INTROSPECTION_TOPIC - self._heartbeat_topic = heartbeat_topic or HEARTBEAT_TOPIC - self._request_introspection_topic = ( - request_introspection_topic or REQUEST_INTROSPECTION_TOPIC - ) + self._introspection_topic = ( + INTROSPECTION_TOPIC if introspection_topic is None else introspection_topic + ) + self._heartbeat_topic = ( + HEARTBEAT_TOPIC if heartbeat_topic is None else heartbeat_topic + ) + self._request_introspection_topic = ( + REQUEST_INTROSPECTION_TOPIC + if request_introspection_topic is None + else request_introspection_topic + )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (5)
docs/architecture/EVENT_STREAMING_TOPICS.md(1 hunks)docs/patterns/README.md(1 hunks)src/omnibase_infra/mixins/mixin_node_introspection.py(14 hunks)src/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yaml(1 hunks)tests/unit/mixins/test_mixin_node_introspection.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use Any type annotation. Always use specific types
Files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All data structures must be proper Pydantic models
Use X | None (PEP 604) over Optional[X] for nullable type annotations
Error classes must raise OnexError (raise OnexError(...) from e) as the base infrastructure error pattern
Protocol resolution must use duck typing through protocols, never isinstance checks
Files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.py
**/nodes/*/v*/contract.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Node implementations must include contract.yaml with semantic versioning, node type (EFFECT/COMPUTE/REDUCER/ORCHESTRATOR), strongly typed I/O (input_model, output_model), and zero Any types
Files:
src/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yaml
**/mixin_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin files must follow naming convention: mixin_.py with class pattern Mixin
Files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧠 Learnings (24)
📓 Common learnings
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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
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 : All Kafka topics must use the prefix `dev.archon-intelligence` for development/staging environments.
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: All ONEX nodes must conform to the 4-Node Architecture pattern with clear separation of concerns and unidirectional data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR)
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
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: All ONEX node deviations from canonical patterns must be documented and justified in the node's root-level README.md and subject to maintainer review
📚 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 Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yamlsrc/omnibase_infra/mixins/mixin_node_introspection.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: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/nodes/node_registry_effect/v1_0_0/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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yamlsrc/omnibase_infra/mixins/mixin_node_introspection.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 **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
Applied to files:
docs/patterns/README.mdsrc/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yamlsrc/omnibase_infra/mixins/mixin_node_introspection.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 agent observability using three-layer traceability with correlation_id tracking through agent_routing_decisions, agent_manifest_injections, and agent_execution_logs tables
Applied to files:
docs/patterns/README.md
📚 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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yamlsrc/omnibase_infra/mixins/mixin_node_introspection.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: All ONEX nodes must conform to the 4-Node Architecture pattern with clear separation of concerns and unidirectional data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Deviations from omnibase_core standards are only acceptable for: (1) Orchestrator/Reducer nodes (ModelService* disabled), (2) Experimental features being prototyped for upstream, (3) Performance-critical optimizations with benchmark proof, (4) Bridge-specific unique patterns. All deviations require explicit documentation and justification.
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Implement ONEX 4-node architecture pattern for infrastructure tools: EFFECT (external service interactions), COMPUTE (message processing/transformation), REDUCER (state consolidation/decision making), ORCHESTRATOR (workflow coordination)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.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]*/introspection.py : All ONEX nodes must include an `introspection.py` file implementing standards-compliant introspection logic
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.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 nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yamlsrc/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Applies to **/nodes/*/v*/contract.yaml : Node implementations must include contract.yaml with semantic versioning, node type (EFFECT/COMPUTE/REDUCER/ORCHESTRATOR), strongly typed I/O (input_model, output_model), and zero Any types
Applied to files:
src/omnibase_infra/nodes/node_registry_effect/v1_0_0/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_*.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:
src/omnibase_infra/nodes/node_registry_effect/v1_0_0/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_registry_effect/v1_0_0/contract.yaml
📚 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 : All Kafka topics must use the prefix `dev.archon-intelligence` for development/staging environments.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Applies to **/nodes/**/node.py : Private methods prefixed with _ are excluded from capability discovery. Avoid exposing sensitive business logic in public method names. Use generic operation names.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 deployment/docker-compose*.yml : Docker Compose files MUST use dual listener architecture for Kafka (9092 internal, 29092 external). Update /etc/hosts with: 192.168.86.200 omninode-bridge-redpanda omninode-bridge-consul omninode-bridge-postgres
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧬 Code graph analysis (1)
src/omnibase_infra/mixins/mixin_node_introspection.py (1)
tests/unit/mixins/test_mixin_node_introspection.py (3)
publish_envelope(78-94)publish_envelope(823-826)publish(96-121)
🔇 Additional comments (15)
src/omnibase_infra/mixins/mixin_node_introspection.py (7)
140-147: LGTM! Topic naming migration to ONEX convention is correct.The migration from legacy topic names (e.g.,
node.introspection) to ONEX-standardized names (e.g.,onex.node.introspection.published.v1) follows the naming convention documented in EVENT_STREAMING_TOPICS.md. The comment block provides good traceability for the migration. Based on learnings, Kafka event publishing should use ONEX-standardized topics.
364-368: LGTM! Instance-level topic configuration attributes are well-documented.The new attributes enable contract-driven topic customization per node while maintaining sensible defaults to module constants.
458-460: LGTM! Optional topic parameters with clear documentation.The new optional parameters follow proper Python idioms with
str | None = Nonetype hints. The docstrings clearly explain the purpose and fallback behavior.Also applies to: 480-488
600-602: LGTM! Topic configuration visibility in debug logs.Including the configured topics in the debug log output aids troubleshooting and confirms per-node topic customization is applied correctly.
1239-1244: LGTM! publish_introspection correctly uses instance-configured topic.The migration from module constant to
self._introspection_topicis correctly applied in both thepublish_envelopeand fallbackpublishpaths.Also applies to: 1249-1255
1344-1358: LGTM! _publish_heartbeat uses instance-configured topic.Consistent with the introspection topic migration, heartbeat publishing now uses
self._heartbeat_topicin both publish paths.
1655-1671: LGTM! Registry listener uses instance-configured request topic.The subscription and logging correctly reference
self._request_introspection_topic, maintaining consistency with the contract-driven configuration approach.tests/unit/mixins/test_mixin_node_introspection.py (2)
45-52: LGTM! Import of topic constant ensures test consistency.Importing
INTROSPECTION_TOPICfrom the module ensures tests stay synchronized with topic naming changes, avoiding hardcoded string duplication.
668-668: LGTM! Test assertion uses the exported constant.Using
INTROSPECTION_TOPICinstead of a hardcoded string"onex.node.introspection.published.v1"ensures the test validates against the actual configured topic name.Consider adding test coverage for custom topic configuration. The mixin supports configurable
introspection_topic,heartbeat_topic, andrequest_introspection_topicparameters via the constructor, but no tests currently verify that custom values override the defaults.docs/patterns/README.md (1)
14-14: LGTM! Documentation link correctly added.The Event Streaming Topics link is appropriately placed in the Observability section and the relative path correctly points to the new architecture document.
docs/architecture/EVENT_STREAMING_TOPICS.md (4)
1-6: LGTM! Well-structured specification document.The "LOCKED (MVP-SAFE)" status clearly communicates the stability guarantee. The explicit non-goals section appropriately sets expectations about Kafka's role in the system.
28-48: Excellent clarification on event semantics.The explicit distinction that Kafka events are "state transition events, not delivery acknowledgements" prevents common misuse patterns. The guidance that "Nodes must rely on registry state queries for authoritative answers" aligns with the invariant that Postgres is the source of truth.
463-478: Comprehensive topic summary table.The summary table provides a clear reference for all 12 topics with their direction, keying, and retention. This will be valuable for developers implementing new consumers or producers.
482-491: Strong final invariants reinforce architectural principles.The invariants correctly emphasize that "Kafka carries events, not truth" and "Postgres is the source of record." The final statement — "If an event is required for correctness, it belongs in the database, not Kafka" — is excellent guidance for avoiding common distributed systems pitfalls.
src/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yaml (1)
38-55: LGTM! event_channels section correctly implements ONEX registry effect node responsibilities.The topic names follow the
onex.<domain>.<entity>.<event>.v<version>convention and align with EVENT_STREAMING_TOPICS.md. Thekey_field: "node_id"is correct for node lifecycle and registry node state events per the keying rules (section 5). The event_channels properly declare subscriptions to node introspection and heartbeat announcements, and publish registry state transitions (registered, registration_failed, deregistered).Note:
onex.registry.introspection.requested.v1is a registry control topic published by orchestrators to request node re-introspection (section 6.2), not a responsibility of the registry effect node. The registry effect node correctly focuses on state transition events, following the architectural pattern where effects perform I/O and orchestrators coordinate workflows.
…n format Address ONEX validator issues in files touched by this PR: 1. Initialize Introspection Parameter Reduction: - Create ModelIntrospectionConfig Pydantic model - Refactor initialize_introspection() to accept config model - Maintain backwards compatibility with legacy parameters - Add 9 new tests for config model usage 2. Contract Version Format: - Change contract_version from string "1.0.0" to semver object - Change node_version from string to semver object Changes: - src/omnibase_infra/mixins/mixin_node_introspection.py - src/omnibase_infra/mixins/__init__.py (export config model) - src/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yaml - tests/unit/mixins/test_mixin_node_introspection.py (9 new tests) Related: OMN-881, OMN-918
PR Review: Contract-Driven Topic Configuration [OMN-881]✅ Overall AssessmentThis is an excellent refactoring that successfully migrates from hardcoded topic enums to contract-driven configuration. The implementation demonstrates strong architectural thinking and follows ONEX principles rigorously. Recommendation: ✅ APPROVE with minor observations noted below. 🎯 Strengths1. Architectural Excellence
2. Implementation Quality
3. Testing Coverage
4. Documentation
📋 Code Quality Analysis✅ ONEX Compliance
✅ Type Annotation Best PracticesThe PR correctly uses PEP 604 union syntax ( # ✅ CORRECT - Modern syntax
introspection_topic: str | None = Field(default=None, ...)
event_bus: object | None = Field(default=None, ...)
# ❌ Would have been deprecated
introspection_topic: Optional[str] = Field(default=None, ...)This aligns perfectly with ONEX CLAUDE.md guidelines. ✅ Semantic Versioning in ContractThe contract properly uses semver objects instead of strings: # ✅ CORRECT
contract_version:
major: 1
minor: 0
patch: 0This addresses the ONEX validator requirement mentioned in the commit message. 🔍 Detailed Findings1. Topic Migration ✅Before: INTROSPECTION_TOPIC = "node.introspection" # LegacyAfter: INTROSPECTION_TOPIC = "onex.node.introspection.published.v1" # ONEX standardAnalysis: Perfect. The new naming follows the documented convention and includes version suffix. 2. Config Model Design ✅class ModelIntrospectionConfig(BaseModel):
node_id: str = Field(..., min_length=1) # Required, validated
node_type: str = Field(..., min_length=1) # Required, validated
cache_ttl: float = Field(default=300.0, ge=0.0) # Non-negative constraint
introspection_topic: str | None = Field(default=None, ...) # Optional overrideAnalysis:
3. Backwards Compatibility Implementation ✅def initialize_introspection(
self,
config: ModelIntrospectionConfig | None = None,
*,
node_id: str | None = None, # Legacy
node_type: str | None = None, # Legacy
...
) -> None:
if config is not None:
# Extract from config
node_id = config.node_id
...
elif node_id is None or node_type is None:
raise ValueError("Either config or both node_id and node_type must be provided")Analysis:
4. Contract Event Channels ✅event_channels:
subscribes_to:
- topic: "onex.node.introspection.published.v1"
description: "Node capability announcements"
key_field: "node_id"
publishes_to:
- topic: "onex.registry.node.registered.v1"
description: "Emitted when registration completes"
key_field: "node_id"Analysis:
🔒 Security Review✅ No New Security Issues
⚡ Performance Considerations✅ No Regression Risk
Analysis: The refactoring is purely structural with zero runtime performance impact. 📊 Test Coverage Analysis✅ Comprehensive Coverage
Specific coverage:
🐛 Potential Issues
|
| Convention | Required | Actual | Status |
|---|---|---|---|
| Model naming | Model* |
ModelIntrospectionConfig |
✅ |
| File naming | model_*.py |
Part of mixin_node_introspection.py |
✅ OK (mixin-scoped) |
| Type hints | PEP 604 (X | None) |
str | None, object | None |
✅ |
| Pydantic config | ConfigDict |
model_config = ConfigDict(...) |
✅ |
| Contract versioning | Semver object | major/minor/patch structure |
✅ |
No Any types |
Required | No Any used |
✅ |
🚀 Deployment Safety
✅ Safe to Merge
- Zero Breaking Changes: Legacy API fully preserved
- Incremental Adoption: New config model is opt-in
- Test Coverage: 100% of new functionality tested
- Documentation: Complete specification and examples
- Type Safety: Pydantic validation prevents invalid configs
Migration Strategy:
- Existing nodes continue using legacy params
- New nodes adopt
ModelIntrospectionConfig - Gradual migration as nodes are updated
- No flag day required
✅ Final Checklist
- ✅ Code Quality: Excellent type safety, validation, and structure
- ✅ ONEX Compliance: Follows all conventions and principles
- ✅ Testing: Comprehensive coverage with no regressions
- ✅ Documentation: Thorough specification and usage examples
- ✅ Security: No new vulnerabilities, existing protections maintained
- ✅ Performance: Zero runtime impact
- ✅ Backward Compatibility: Fully preserved
- ✅ Contract-Driven: Proper
event_channelsin contract.yaml
🎯 Verdict
APPROVED ✅
This PR represents high-quality infrastructure work that:
- Eliminates hardcoded topic enums in favor of contract-driven configuration
- Improves parameter ergonomics via Pydantic config model
- Maintains 100% backward compatibility
- Provides comprehensive documentation and test coverage
- Follows ONEX architectural principles rigorously
The implementation is production-ready and sets a strong pattern for future contract-driven migrations.
Great work on OMN-881! 🚀
Reviewed against ONEX CLAUDE.md standards and ONEX 4-node architecture principles.
- Add test coverage for topic name validation (18 new tests) - Add Section 12: Contract Integration with contract.yaml examples - Add Future Enhancements section for EnumONEXTopic consideration - Verify topic count consistency (12 topics matches summary) Tests: All 74 tests pass, linting passes
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/architecture/EVENT_STREAMING_TOPICS.md (1)
463-478: Reconcile documented topic count with OnexEnvelopeV1 specification (DUPLICATE CONCERN).This specification documents 12 Kafka topics, but the learning from omninode_bridge PR states: "Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming." The discrepancy remains unresolved from the previous review cycle. Verify whether:
- A 13th topic (possibly a Dead Letter Queue or infrastructure topic) should be included
- The learning is outdated or specific to a different scope (omninode_bridge vs. omnibase_infra)
- The count of 12 is intentional for this MVP scope
🧹 Nitpick comments (1)
tests/unit/event_bus/test_kafka_event_bus.py (1)
15-15: Consider importinguuid4at module level for consistency.Since the
correlation_idfixture (line 1448) importsuuid4locally, it would be cleaner to import bothUUIDanduuid4at the module level:-from uuid import UUID +from uuid import UUID, uuid4Then simplify the fixture at line 1448:
@pytest.fixture def correlation_id(self) -> UUID: """Create a correlation ID for tests.""" return uuid4()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
docs/architecture/EVENT_STREAMING_TOPICS.md(1 hunks)tests/unit/event_bus/test_kafka_event_bus.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use Any type annotation. Always use specific types
Files:
tests/unit/event_bus/test_kafka_event_bus.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All data structures must be proper Pydantic models
Use X | None (PEP 604) over Optional[X] for nullable type annotations
Error classes must raise OnexError (raise OnexError(...) from e) as the base infrastructure error pattern
Protocol resolution must use duck typing through protocols, never isinstance checks
Files:
tests/unit/event_bus/test_kafka_event_bus.py
🧠 Learnings (18)
📓 Common learnings
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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
📚 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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.mdtests/unit/event_bus/test_kafka_event_bus.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 Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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]*/SCHEMA_DECISIONS.md : Each versioned ONEX node implementation directory must include a `SCHEMA_DECISIONS.md` file documenting schema-specific design decisions, implementation notes, and validation strategies
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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_*/ARCHITECTURE_DECISIONS.md : All ONEX nodes must include an `ARCHITECTURE_DECISIONS.md` file at the node root directory level documenting key architectural choices with rationale, decision status, context, options, and consequences
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX node deviations from canonical patterns must be documented and justified in the node's root-level README.md and subject to maintainer review
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX nodes must conform to the 4-Node Architecture pattern with clear separation of concerns and unidirectional data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Deviations from omnibase_core standards are only acceptable for: (1) Orchestrator/Reducer nodes (ModelService* disabled), (2) Experimental features being prototyped for upstream, (3) Performance-critical optimizations with benchmark proof, (4) Bridge-specific unique patterns. All deviations require explicit documentation and justification.
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Implement ONEX 4-node architecture pattern for infrastructure tools: EFFECT (external service interactions), COMPUTE (message processing/transformation), REDUCER (state consolidation/decision making), ORCHESTRATOR (workflow coordination)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
tests/unit/event_bus/test_kafka_event_bus.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 scripts/tests/**/*.sh : Implement comprehensive test suites in scripts/tests/ with separate test files for Kafka, PostgreSQL, Intelligence, and Routing functionality
Applied to files:
tests/unit/event_bus/test_kafka_event_bus.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 **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
Applied to files:
tests/unit/event_bus/test_kafka_event_bus.py
🧬 Code graph analysis (1)
tests/unit/event_bus/test_kafka_event_bus.py (2)
src/omnibase_infra/event_bus/kafka_event_bus.py (2)
environment(461-467)_validate_topic_name(1461-1520)src/omnibase_infra/errors/infra_errors.py (1)
ProtocolConfigurationError(103-138)
🔇 Additional comments (4)
docs/architecture/EVENT_STREAMING_TOPICS.md (2)
605-607: Verify referenced documentation files are created or adjust links.Lines 605–607 reference documentation paths that may not exist yet:
../patterns/correlation_id_tracking.md../patterns/circuit_breaker_implementation.md../patterns/error_handling_patterns.mdConfirm these files are being created in this PR or adjust the references (e.g., mark as "TBD" or move to "Related Tickets" if they're tracked separately).
1-12: Overall structure and clarity are excellent.The document is well-organized, clearly differentiates Kafka's role (events, not RPC), and provides concrete examples with contract integration. The decision to lock 12 topics for MVP and defer EnumONEXTopic to post-MVP is sound. The canonical envelope format, keying rules, and retention policies provide solid operational guidance.
tests/unit/event_bus/test_kafka_event_bus.py (2)
1437-1450: LGTM - Fixtures are appropriate for validation testing.The
event_busfixture correctly omits lifecycle management since these tests only exercise the_validate_topic_namemethod, which doesn't require the bus to be started. Thecorrelation_idfixture provides consistent UUID values across tests.
1452-1610: Excellent comprehensive test coverage for topic validation.This test suite thoroughly validates Kafka topic naming rules with:
- 8 valid topic name scenarios covering all allowed character combinations and edge cases (max length)
- 12 invalid topic name scenarios covering all validation rules (empty, too long, reserved, special characters, unicode, whitespace)
The test structure is clear, well-documented, and follows pytest best practices. The pattern at lines 1566-1583 efficiently tests multiple invalid special characters in a loop.
PR Review: Contract-Driven Topic Configuration [OMN-881]Overall Assessment: STRONG APPROVEThis is an excellent implementation that successfully migrates from hardcoded topic enums to contract-driven configuration while maintaining backwards compatibility. The PR demonstrates strong adherence to ONEX principles and includes comprehensive documentation. Strengths1. Exceptional Documentation
2. Backwards Compatibility 3. Strong Type Safety
4. Contract-Driven Architecture 5. Test Coverage
Issues & RecommendationsCRITICAL: Thread Safety Documentation GapLocation: src/omnibase_infra/mixins/mixin_node_introspection.py Issue: The mixin uses instance-level state (_introspection_cache, _last_introspection_cache_time) but lacks thread safety documentation or async safety considerations. Risk: If get_introspection_data() or get_capabilities() are called concurrently from multiple async tasks, race conditions could occur on cache invalidation. Recommendation: Add thread safety documentation to class docstring and consider adding asyncio.Lock for cache operations. MEDIUM: Topic Constant ValidationLocation: src/omnibase_infra/mixins/mixin_node_introspection.py:158-160 Issue: Topic constants are strings with no runtime validation against EVENT_STREAMING_TOPICS.md. Recommendation: Add topic validation utility (post-MVP) or document as known limitation. MEDIUM: Error Handling in Registry ListenerGood: Rate-limiting error logs prevents log spam. Potential Gap: What happens if REQUEST_INTROSPECTION_TOPIC messages are malformed? Recommendation:
Security Review
Performance ConsiderationsCache uses TTL-based caching (300s TTL, 1ms threshold). Consider: For high-frequency heartbeat scenarios (30s interval), 5min TTL may be aggressive. Monitor cache hit rates in production. Checklist ValidationAll checks pass: tests, linting, type checking, YAML validation, backwards compatibility, naming conventions. Final RecommendationsMerge Decision: APPROVE Ready to merge with minor follow-up work: Pre-Merge (Optional but Recommended):
Post-Merge Follow-Up (Create tickets):
Exemplary Patterns to Replicate
SummaryGrade: A- (Excellent work with minor thread safety documentation gap) Great work on this migration! The attention to detail in the specification document and the thoughtful backwards compatibility approach are exactly what ONEX needs. Reviewed against ONEX principles in CLAUDE.md |
- Add asyncio.Lock for thread-safe cache operations in MixinNodeIntrospection - Protect cache reads/writes with async lock to prevent race conditions - Change invalidate_introspection_cache() from sync to async (breaking change) - Improve event_bus field comment to explain duck typing rationale - Add thread safety test for concurrent cache operations Post-merge follow-ups created: - OMN-922: Add topic name validation utility - OMN-923: Add malformed message handling metrics - OMN-924: Monitor introspection cache hit rates in production Tests: All 95 tests pass
Pull Request Review: Contract-Driven Topic Configuration [OMN-881]SummaryThis PR successfully migrates from hardcoded Kafka topics to a contract-driven configuration approach. The implementation is architecturally sound, well-documented, and follows ONEX principles. However, there are several areas that need attention before merging. 🟢 Strengths1. Excellent Documentation
2. Backwards Compatibility
3. Type Safety & Validation
4. Performance Considerations
5. Test Coverage
🟡 Issues Requiring Attention1. Critical: Contract Schema Validation Missing
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
src/omnibase_infra/mixins/mixin_node_introspection.py(22 hunks)tests/unit/mixins/test_mixin_node_introspection.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use Any type annotation. Always use specific types
Files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All data structures must be proper Pydantic models
Use X | None (PEP 604) over Optional[X] for nullable type annotations
Error classes must raise OnexError (raise OnexError(...) from e) as the base infrastructure error pattern
Protocol resolution must use duck typing through protocols, never isinstance checks
Files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.py
**/mixin_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin files must follow naming convention: mixin_.py with class pattern Mixin
Files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧠 Learnings (23)
📓 Common learnings
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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
📚 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 : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.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 nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.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]*/introspection.py : All ONEX nodes must include an `introspection.py` file implementing standards-compliant introspection logic
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.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/unit/mixins/test_mixin_*.py : Mixin tests must be organized in test classes and test mixin initialization, inheritance, and core mixin functionality
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.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/unit/models/**/test_model_*.py : Model tests must achieve 100% coverage and test instantiation, inheritance, serialization, deserialization, JSON serialization, roundtrip serialization, equality, hashing, string representation, repr, attributes, validation, metadata, data creation, copying, and immutability
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.183Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.183Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors, not ModelContainer[T]
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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: Node implementations must use mixin-based composition from `omnibase_core.mixins` (e.g., `MixinHealthCheck`, `MixinNodeExecutor`) to add capabilities
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 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 : Implement Node classes by inheriting from `NodeBase` with proper UUID and `ModelSemVer` fields
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeType for specific node implementation discovery and capability matching
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 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 Pydantic model inheritance patterns extending from BaseModel
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Use Protocol for interface definitions when implementations may live outside core codebase; use Pydantic models only for base classes with shared logic
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelEventEnvelope for inter-service event-driven communication
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add from_attributes=True to ConfigDict for immutable value objects nested inside other Pydantic models
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/*.py : Use type-safe configuration via Pydantic Settings from config/settings.py with 90+ type-safe variables organized into External Service Discovery, Shared Infrastructure, AI Provider API Keys, Local Services, and Feature Flags
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Applies to **/nodes/**/node.py : Private methods prefixed with _ are excluded from capability discovery. Avoid exposing sensitive business logic in public method names. Use generic operation names.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 deployment/docker-compose*.yml : Docker Compose files MUST use dual listener architecture for Kafka (9092 internal, 29092 external). Update /etc/hosts with: 192.168.86.200 omninode-bridge-redpanda omninode-bridge-consul omninode-bridge-postgres
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧬 Code graph analysis (2)
tests/unit/mixins/test_mixin_node_introspection.py (1)
src/omnibase_infra/mixins/mixin_node_introspection.py (5)
invalidate_introspection_cache(2053-2074)initialize_introspection(585-792)get_introspection_data(1214-1372)ModelIntrospectionConfig(175-289)publish_introspection(1374-1470)
src/omnibase_infra/mixins/mixin_node_introspection.py (3)
src/omnibase_infra/nodes/node_registry_effect/v1_0_0/protocols.py (1)
ProtocolEventBus(70-110)src/omnibase_infra/models/discovery/model_node_introspection_event.py (1)
ModelNodeIntrospectionEvent(11-138)tests/unit/mixins/test_mixin_node_introspection.py (3)
publish_envelope(78-94)publish_envelope(823-826)publish(96-121)
🔇 Additional comments (12)
tests/unit/mixins/test_mixin_node_introspection.py (4)
46-46: LGTM! Using topic constant improves maintainability.Importing the topic constant from the module ensures test assertions stay in sync with the implementation.
668-668: LGTM! Topic assertion uses standardized constant.Using the
INTROSPECTION_TOPICconstant ensures the test validates against the actual topic name used by the implementation.
1314-1355: Excellent test coverage for async lock thread safety.This test validates that the new
_introspection_cache_lockcorrectly protects cache state under concurrent access, covering:
- Lock initialization and type validation
- Mixed read/write/invalidate operations
- No deadlocks with 50 concurrent tasks
- Final state consistency
2303-2470: Comprehensive test suite for ModelIntrospectionConfig.The test class provides excellent coverage of the new configuration model API:
- ✅ Initialization patterns (config model vs legacy params)
- ✅ Event bus integration
- ✅ Custom keywords and topics
- ✅ Backwards compatibility
- ✅ Config precedence rules
- ✅ Pydantic validation
- ✅ Public API exports
src/omnibase_infra/mixins/mixin_node_introspection.py (8)
154-161: LGTM! Topic constants follow ONEX naming convention.The updated topic names follow the standardized ONEX format with proper namespacing, semantic action names, and versioning. The migration comment provides helpful context.
Based on learnings, Kafka event streaming should use proper ONEX topics with versioning.
495-500: Good design: Instance-level topic configuration enables contract-driven architecture.The per-instance topic fields allow nodes to declare custom topics in their contracts while maintaining sensible module-level defaults. This aligns with the PR's goal of contract-driven topic configuration.
585-793: Well-implemented backwards-compatible API with proper validation.The updated
initialize_introspection()signature correctly:
- ✅ Supports both config model (recommended) and legacy params
- ✅ Uses
*to enforce keyword-only legacy params- ✅ Validates required fields regardless of usage pattern
- ✅ Implements proper config precedence (config overrides legacy params)
- ✅ Initializes cache lock for thread safety
- ✅ Provides sensible defaults for optional topic configuration
The implementation preserves backwards compatibility while encouraging the new config model approach.
1243-1321: Excellent lock usage pattern minimizes contention.The cache lock implementation correctly:
- ✅ Uses
async withfor automatic lock release- ✅ Holds lock only during cache validity check and update
- ✅ Releases lock before expensive operations (get_capabilities, get_endpoints)
- ✅ Atomically updates both cache and timestamp together
- ✅ Returns cached data with minimal lock hold time
This design prevents race conditions while maximizing concurrency.
1429-1548: Instance-level topic configuration correctly applied.Both publishing methods (
publish_introspectionand_publish_heartbeat) now use the per-instance topic configuration (self._introspection_topicandself._heartbeat_topic), enabling contract-driven topic customization per node.
1845-1861: Registry listener correctly uses instance-level request topic.The subscription uses
self._request_introspection_topic, completing the contract-driven topic configuration for all three event types (introspection, heartbeat, and request).
2053-2074: Async cache invalidation ensures thread safety.The method correctly uses the async lock to atomically invalidate both cache and timestamp, preventing race conditions with concurrent cache reads/writes. The breaking API change (sync to async) was already flagged in the test file review.
2113-2126: Public API exports correctly updated.The
ModelIntrospectionConfigis properly added to__all__, making it part of the module's public API. The tests verify it's accessible from both module and package level imports.
Breaking Changes: - invalidate_introspection_cache() is now async for thread-safe operations Type Safety: - Replace event_bus: object | None with ProtocolIntrospectionEventBus protocol - Minimal protocol requiring only publish_envelope, publish, subscribe methods Documentation: - Add thread safety documentation to MixinNodeIntrospection class docstring - Document async lock usage pattern for cache operations - Add thread safety section to CLAUDE.md - Reconcile topic count (12 topics) in EVENT_STREAMING_TOPICS.md - Add contract.yaml to code integration example Testing: - Add 10 new tests for custom topic parameters (TestMixinNodeIntrospectionCustomTopics) - Verify custom topics used in publishing, heartbeat, registry listener Nitpicks: - Make empty string validation explicit (node_id is None or node_id == "") - Verify uuid4 already imported at module level Dependencies: - Update omnibase-core to ^0.5.1
PR Review: Contract-Driven Topic Configuration [OMN-881]OverviewThis PR successfully migrates from hardcoded ✅ StrengthsArchitecture & Design
Documentation
Testing
Code Quality
🔍 Areas for Improvement1. Topic Name Validation (Post-Merge)The PR creates follow-up ticket OMN-922 for topic validation utilities. This is the right approach, but consider: Recommendation: Add compile-time validation that contract.yaml topics exist in EVENT_STREAMING_TOPICS.md canonical list. This could be a pre-commit hook or CI check. Example: ci/validate_topics.pyCANONICAL_TOPICS = { def validate_contract_topics(contract_path): 2. Empty String Validation ConsistencyIn ```python Issue: When using Recommendation: Add comment explaining this is for legacy path only: Validate required fields - explicit handling of None and empty stringNote: When using ModelIntrospectionConfig, Pydantic's min_length=1 validatoralready prevents empty strings. This validation handles the legacy parameter path.``` 3. Topic Constants vs EnumONEXTopic (Future Enhancement)EVENT_STREAMING_TOPICS.md mentions Current: Future Consideration: This is correctly deferred to post-MVP, but document the trade-offs in a design decision record when implementing. 4. Contract Version Format ChangeThe contract version changed from string Recommendation: Add a validator migration guide for other nodes: Before (INCORRECT)contract_version: "1.0.0" After (CORRECT)contract_version: 5. Performance Metrics Exposure
Security Note: In multi-tenant environments, performance metrics could reveal infrastructure capacity. Ensure introspection topics have proper ACLs if performance data is sensitive. 6. Cache Invalidation Breaking Change
Recommendation: Add a deprecation guide for common migration patterns: Before (synchronous)node.invalidate_introspection_cache() After (asynchronous)await node.invalidate_introspection_cache() If called from sync context (NOT RECOMMENDED):asyncio.create_task(node.invalidate_introspection_cache()) 🐛 Potential IssuesNone FoundThe code is well-tested and follows ONEX patterns correctly. All potential issues identified have follow-up tickets (OMN-922, OMN-923, OMN-924). 🔒 Security Assessment✅ Strong Points
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/architecture/EVENT_STREAMING_TOPICS.md (1)
465-480: Verify topic count against system requirements.The specification documents 12 topics, but a past review noted a learning reference mentioning 13 topics for event streaming. Confirm whether:
- The 13th topic is specific to a different context (e.g., omninode_bridge workflows)
- A topic is missing from this specification
- The learning is outdated and 12 is correct
#!/bin/bash # Search for OnexEnvelopeV1 and topic definitions across the repository echo "=== Searching for topic definitions ===" rg "onex\.(node|registry|infra)\." --type=py -C2 | head -100 echo "" echo "=== Checking for references to 13 topics ===" rg "13.*topic|topic.*13" --type=md
🧹 Nitpick comments (6)
tests/unit/mixins/test_mixin_node_introspection.py (2)
41-42: Review Any type usage for protocol compliance.Lines 41-42 and 129 use
Anyin type hints for the subscribe callback parameter. Per coding guidelines: "NEVER useAnytypes - Always use specific types."The actual
ProtocolIntrospectionEventBus(from relevant snippets) shows:on_message: Callable[[ModelEventMessage], Awaitable[None]]Consider updating the mock to use the specific
ModelEventMessagetype instead ofAny:-from typing import Any +from omnibase_infra.event_bus.models import ModelEventMessage async def subscribe( self, topic: str, group_id: str, - on_message: Callable[[Any], Awaitable[None]], + on_message: Callable[[ModelEventMessage], Awaitable[None]], ) -> Callable[[], Awaitable[None]]:Also applies to: 129-129
1338-1378: Enhance thread-safety test for race condition coverage.The cache lock thread-safety test (lines 1338-1378) validates concurrent operations complete without deadlock, but could be strengthened to verify actual race condition prevention.
Consider adding assertions to verify cache consistency:
🔎 Suggested enhancement
async def test_cache_lock_thread_safety(self) -> None: """Test that cache operations are thread-safe with async lock. This test verifies that concurrent cache reads, writes, and invalidations do not cause race conditions when using the async lock. """ node = MockNode() node.initialize_introspection( node_id="lock-test-node", node_type="EFFECT", event_bus=None, cache_ttl=0.001, # Very short TTL to force frequent cache misses ) # Verify the lock is initialized assert hasattr(node, "_introspection_cache_lock") assert isinstance(node._introspection_cache_lock, asyncio.Lock) + # Track cache state observations + cache_states: list[tuple[object | None, float | None]] = [] + # Create mixed operations: reads, writes (via get_introspection_data), # and invalidations async def mixed_operations(idx: int) -> str: """Perform a mix of cache operations.""" if idx % 3 == 0: await node.invalidate_introspection_cache() + # Verify cache is cleared atomically + async with node._introspection_cache_lock: + cache_states.append((node._introspection_cache, node._introspection_cached_at)) return "invalidate" else: await node.get_introspection_data() + # Verify cache state is consistent + async with node._introspection_cache_lock: + cache = node._introspection_cache + cached_at = node._introspection_cached_at + # Both should be None or both should be set + assert (cache is None) == (cached_at is None), \ + "Cache state inconsistent: cache and cached_at should both be None or both be set" + cache_states.append((cache, cached_at)) return "read" # Run 50 concurrent mixed operations tasks = [mixed_operations(i) for i in range(50)] results = await asyncio.gather(*tasks) # All operations should complete without deadlock or error assert len(results) == 50 assert "invalidate" in results assert "read" in results + + # Verify no inconsistent states observed + for cache, cached_at in cache_states: + assert (cache is None) == (cached_at is None), \ + f"Observed inconsistent cache state: cache={'None' if cache is None else 'set'}, " \ + f"cached_at={'None' if cached_at is None else 'set'}" # Final state should be consistent data = await node.get_introspection_data() assert data.node_id == "lock-test-node"This enhancement verifies that
_introspection_cacheand_introspection_cached_atremain atomically consistent throughout concurrent operations, catching the race condition described in the mixin's docstring.src/omnibase_infra/mixins/mixin_node_introspection.py (4)
152-230: Protocol definition looks good, with one suggestion for improved type safety.The
ProtocolIntrospectionEventBussuccessfully addresses the previous review concern about usingobject | Nonefor the event_bus field. The protocol provides proper duck-typed compatibility while maintaining type checking.However, there's a minor type safety improvement opportunity:
envelope parameter (line 182): Using
objectfor the envelope parameter is pragmatic, but consider using a bounded TypeVar or a minimal Protocol to be more explicit:from typing import Protocol, TypeVar class HasModelDump(Protocol): """Protocol for objects with model_dump method.""" def model_dump(self, *, mode: str = "python") -> dict[str, object]: ... EnvelopeT = TypeVar('EnvelopeT', bound=HasModelDump)Then use
EnvelopeTinstead ofobjectin the protocol definition. This provides stronger type safety while maintaining flexibility.subscribe return type (line 218): The return type
Callable[[], Awaitable[None]]only allows async unsubscribe functions, but the code at line 617 suggests sync callables are also supported:Callable[[], None] | Callable[[], Awaitable[None]]. Consider updating the protocol to match:async def subscribe( self, topic: str, group_id: str, on_message: Callable[[ModelEventMessage], Awaitable[None]], ) -> Callable[[], None] | Callable[[], Awaitable[None]]:🔎 Proposed improvements
+class HasModelDump(Protocol): + """Protocol for objects with model_dump method.""" + def model_dump(self, *, mode: str = "python") -> dict[str, object]: + ... + @runtime_checkable class ProtocolIntrospectionEventBus(Protocol): """Minimal protocol for event bus used by MixinNodeIntrospection. ... """ async def publish_envelope( self, - envelope: object, + envelope: HasModelDump, topic: str, ) -> None: """Publish a typed envelope to a topic. Args: - envelope: Event model (e.g., ModelNodeIntrospectionEvent) with model_dump() + envelope: Event model with model_dump() method topic: Target topic name - - Note: - The envelope parameter uses ``object`` type to accept any Pydantic model - with a ``model_dump()`` method. This matches the actual implementations - in KafkaEventBus and InMemoryEventBus. """ ... async def subscribe( self, topic: str, group_id: str, on_message: Callable[[ModelEventMessage], Awaitable[None]], - ) -> Callable[[], Awaitable[None]]: + ) -> Callable[[], None] | Callable[[], Awaitable[None]]: """Subscribe to a topic with a message callback. Args: topic: Topic to subscribe to group_id: Consumer group ID for offset management on_message: Async callback invoked for each message Returns: - Async unsubscribe function to cancel the subscription + Unsubscribe function (sync or async) to cancel the subscription """ ...
254-369: Excellent config model implementation with one clarification needed.The
ModelIntrospectionConfigis well-designed with proper validation, sensible defaults, and comprehensive documentation. The use of Pydantic ensures type safety and prevents invalid configurations.Question about frozen=False (line 367): The model is configured with
frozen=False, which allows mutation after creation. Is this intentional?Configuration objects are typically immutable to prevent accidental modifications after initialization. If mutation isn't needed, consider
frozen=Truefor immutability:model_config = ConfigDict( arbitrary_types_allowed=True, frozen=True, # Prevent accidental mutation )If mutation is required for specific use cases, the current setting is fine, but it would be helpful to document why in a comment.
720-929: Excellent backward-compatible implementation with clear migration path.The updated
initialize_introspectionmethod successfully provides both:
- New config-based initialization (recommended)
- Legacy parameter-based initialization (backward compatible)
The implementation correctly prioritizes the config model when provided and validates all required fields. The topic configuration with fallback to module constants is clean and flexible.
One small improvement suggestion (lines 842-845):
The error message could be more helpful by indicating the new config model option:
elif node_id is None or node_type is None: raise ValueError( - "Either config or both node_id and node_type must be provided" + "Either config (ModelIntrospectionConfig) or both node_id and node_type must be provided. " + "Config model is recommended for new code." )This guides users toward the preferred approach.
🔎 Improved error message
elif node_id is None or node_type is None: raise ValueError( - "Either config or both node_id and node_type must be provided" + "Either config (ModelIntrospectionConfig) or both node_id and node_type must be provided. " + "Config model is recommended for new code." )
1792-1799: Registry listener correctly uses per-instance topic, but documentation could be clearer.The implementation correctly uses
self._request_introspection_topic(line 1995), enabling per-node topic configuration. However, the security documentation on lines 1792-1799 hardcodes the default topic name:"""Security Note: This method subscribes to the ``onex.registry.introspection.requested.v1`` Kafka topic and responds with full introspection data to any request.Consider updating the documentation to reflect that the topic is configurable:
"""Security Note: This method subscribes to the configured registry introspection topic (default: ``onex.registry.introspection.requested.v1``) and responds with full introspection data to any request.This clarifies that nodes can use custom topics per their contract configuration.
🔎 Clarified security documentation
"""Background loop listening for REQUEST_INTROSPECTION events. ... Security Note: - This method subscribes to the ``onex.registry.introspection.requested.v1`` - Kafka topic and responds with full introspection data to any request. + This method subscribes to the configured registry introspection topic + (default: ``onex.registry.introspection.requested.v1``) and responds + with full introspection data to any request. This creates a network-accessible endpoint for capability discovery. **Network Exposure**: - Any consumer on the Kafka cluster can request introspection data - - Responses are published to ``onex.node.introspection.published.v1`` topic + - Responses are published to the configured introspection topic - No authentication is performed on incoming requestsAlso applies to: 1992-2008
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.md(2 hunks)CLAUDE.md(1 hunks)docs/architecture/EVENT_STREAMING_TOPICS.md(1 hunks)pyproject.toml(1 hunks)src/omnibase_infra/mixins/__init__.py(2 hunks)src/omnibase_infra/mixins/mixin_node_introspection.py(23 hunks)tests/unit/mixins/test_mixin_node_introspection.py(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use Any type annotation. Always use specific types
Files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.pysrc/omnibase_infra/mixins/__init__.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All data structures must be proper Pydantic models
Use X | None (PEP 604) over Optional[X] for nullable type annotations
Error classes must raise OnexError (raise OnexError(...) from e) as the base infrastructure error pattern
Protocol resolution must use duck typing through protocols, never isinstance checks
Files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.pysrc/omnibase_infra/mixins/__init__.py
**/mixin_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin files must follow naming convention: mixin_.py with class pattern Mixin
Files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧠 Learnings (43)
📓 Common learnings
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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
📚 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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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 Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX node deviations from canonical patterns must be documented and justified in the node's root-level README.md and subject to maintainer review
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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]*/SCHEMA_DECISIONS.md : Each versioned ONEX node implementation directory must include a `SCHEMA_DECISIONS.md` file documenting schema-specific design decisions, implementation notes, and validation strategies
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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: All ONEX nodes must conform to the 4-Node Architecture pattern with clear separation of concerns and unidirectional data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Deviations from omnibase_core standards are only acceptable for: (1) Orchestrator/Reducer nodes (ModelService* disabled), (2) Experimental features being prototyped for upstream, (3) Performance-critical optimizations with benchmark proof, (4) Bridge-specific unique patterns. All deviations require explicit documentation and justification.
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Implement ONEX 4-node architecture pattern for infrastructure tools: EFFECT (external service interactions), COMPUTE (message processing/transformation), REDUCER (state consolidation/decision making), ORCHESTRATOR (workflow coordination)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.pyCHANGELOG.mdCLAUDE.mdsrc/omnibase_infra/mixins/__init__.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 nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.pysrc/omnibase_infra/mixins/__init__.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]*/introspection.py : All ONEX nodes must include an `introspection.py` file implementing standards-compliant introspection logic
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.183Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.183Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors, not ModelContainer[T]
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.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: Node implementations must use mixin-based composition from `omnibase_core.mixins` (e.g., `MixinHealthCheck`, `MixinNodeExecutor`) to add capabilities
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.pyCHANGELOG.mdCLAUDE.md
📚 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:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Avoid using Any, dict, or primitive types in protocol signatures; use the strongest typing possible with Pydantic models
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.183Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.183Z
Learning: Applies to src/omnibase_core/**/*.py : Use PEP 604 union syntax (str | None) instead of typing.Union and typing.Optional for type annotations
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/{models,protocols}/{model_*,protocol_*}.py : Avoid using Any, dict, or primitive types in model and protocol definitions; use strongest typing possible
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/**/*.py : Use duck typing with protocols instead of isinstance checks for protocol validation
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/protocols/protocol_*.py : Use Protocol from typing module for all interface definitions; never use ABC (Abstract Base Classes) for service interfaces
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelEventEnvelope for inter-service event-driven communication
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.pysrc/omnibase_infra/mixins/__init__.py
📚 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/**/*.py : Protocols must inherit from `typing.Protocol` and use `...` (ellipsis) for method bodies
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 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 : Implement Node classes by inheriting from `NodeBase` with proper UUID and `ModelSemVer` fields
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Use Protocol for interface definitions when implementations may live outside core codebase; use Pydantic models only for base classes with shared logic
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol for tool interfaces and plugin APIs based on method shape (structural typing), not Pydantic models with inheritance
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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: Node communication must use event-driven patterns through `ModelEventEnvelope` from `omnibase_core.models.events.model_event_envelope`
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/protocols/protocol_*.py : All Protocol definitions must use model-only signatures: methods accept only validated Pydantic models, never dict, primitives, or argument models
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/**/*.py : Use EnumNodeType for specific node implementation discovery and capability matching
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Applies to **/nodes/**/node.py : Private methods prefixed with _ are excluded from capability discovery. Avoid exposing sensitive business logic in public method names. Use generic operation names.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 deployment/docker-compose*.yml : Docker Compose files MUST use dual listener architecture for Kafka (9092 internal, 29092 external). Update /etc/hosts with: 192.168.86.200 omninode-bridge-redpanda omninode-bridge-consul omninode-bridge-postgres
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
CHANGELOG.md
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Applies to **/adapters/**/*.py : All infrastructure adapters and services should use MixinAsyncCircuitBreaker for fault tolerance with methods _init_circuit_breaker, _check_circuit_breaker, _record_circuit_failure, and _reset_circuit_breaker called under _circuit_breaker_lock
Applied to files:
CLAUDE.md
📚 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/**/*.py : SPI modules may import from `omnibase_core` for type hints and model runtime usage (allowed and required)
Applied to files:
src/omnibase_infra/mixins/__init__.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/*.py : Import protocols from `omnibase.protocol.protocol_*` paths
Applied to files:
src/omnibase_infra/mixins/__init__.py
🧬 Code graph analysis (3)
tests/unit/mixins/test_mixin_node_introspection.py (1)
src/omnibase_infra/mixins/mixin_node_introspection.py (7)
subscribe(213-229)invalidate_introspection_cache(2200-2234)initialize_introspection(720-928)get_introspection_data(1350-1519)ModelIntrospectionConfig(254-368)publish_introspection(1521-1617)publish(198-211)
src/omnibase_infra/mixins/mixin_node_introspection.py (3)
src/omnibase_infra/models/discovery/model_node_introspection_event.py (1)
ModelNodeIntrospectionEvent(11-138)src/omnibase_infra/event_bus/models/model_event_message.py (1)
ModelEventMessage(14-60)tests/unit/mixins/test_mixin_node_introspection.py (5)
publish_envelope(80-96)publish_envelope(847-850)publish(98-123)publish(2700-2712)publish(2750-2762)
src/omnibase_infra/mixins/__init__.py (1)
src/omnibase_infra/mixins/mixin_node_introspection.py (2)
ModelIntrospectionConfig(254-368)ProtocolIntrospectionEventBus(153-229)
🔇 Additional comments (13)
src/omnibase_infra/mixins/__init__.py (1)
20-21: LGTM! Clean public API surface extension.The new exports for
ModelIntrospectionConfigandProtocolIntrospectionEventBusproperly extend the mixins package API to support contract-driven topic configuration. The pattern follows standard Python conventions for package-level re-exports.Also applies to: 30-31
CLAUDE.md (1)
835-850: LGTM! Clear thread-safety documentation.The documentation accurately describes the async lock pattern for introspection cache operations. The distinction between internal lock management (introspection) and caller-held locking (circuit breaker) is helpful for developers working with both mixins.
tests/unit/mixins/test_mixin_node_introspection.py (2)
2327-2495: LGTM! Comprehensive config model testing.The
TestMixinNodeIntrospectionConfigModeltest class provides thorough coverage of:
- Config model initialization and parameter passing
- Precedence rules (config overrides legacy params)
- Error handling for missing required parameters
- Pydantic validation (empty strings, negative values)
- Public API exports verification
Well-structured test suite validating the new configuration pattern.
2499-2820: LGTM! Thorough custom topic configuration testing.The
TestMixinNodeIntrospectionCustomTopicstest class comprehensively validates:
- Custom topics used in publishing (introspection, heartbeat)
- Default topic fallbacks when not specified
- Partial customization with defaults for unspecified topics
- Fallback publish method with custom topics (no publish_envelope)
- Empty/None topic values correctly falling back to defaults
- Topic information exposed for debugging
Excellent coverage of the contract-driven topic configuration feature.
CHANGELOG.md (1)
12-17: LGTM! Breaking change properly documented.The async
invalidate_introspection_cache()breaking change is clearly documented with:
- Old vs new signature comparison
- Migration instructions (add
await)- Clear rationale (thread-safe operations require async)
This addresses the past review comment requesting CHANGELOG documentation for this breaking change.
src/omnibase_infra/mixins/mixin_node_introspection.py (8)
232-240: LGTM! Topic naming follows ONEX conventions.The standardized topic names align well with the contract-driven architecture described in the PR objectives. The migration comment provides good context for the naming changes.
465-519: Outstanding thread safety documentation!The thread safety section is exemplary. It clearly explains:
- Why
asyncio.Lockwas chosen over alternatives- What state is protected
- How the lock is used internally
- Performance implications
- Cross-references to similar patterns
This level of documentation makes it easy for future maintainers to understand the concurrency model.
622-622: LGTM! Instance attributes are properly typed and scoped.The new instance attributes support the contract-driven topic configuration and thread-safe caching:
_introspection_event_busnow usesProtocolIntrospectionEventBus | Nonefor proper type safety (addressing the previous review concern)- Per-instance topic attributes (
_introspection_topic,_heartbeat_topic,_request_introspection_topic) enable node-specific topic customization_introspection_cache_lockprovides thread-safe cache accessAll attributes are correctly scoped at the instance level.
Also applies to: 630-634, 647-650
1390-1412: Thread-safe cache implementation is correct and well-documented.The cache operations properly use
asyncio.Lockto protect against race conditions:Cache read (lines 1391-1412): The lock ensures atomic checking of cache validity and returning cached data. This prevents a race where another coroutine invalidates the cache between the TTL check and data return.
Cache write (lines 1464-1468): The lock ensures both
_introspection_cacheand_introspection_cached_atare updated atomically, preventing inconsistent cache state.The
cast()on line 1465 is safe becauseModelNodeIntrospectionEvent.model_dump(mode="json")output structure matchesIntrospectionCacheDictby design.Also applies to: 1464-1468
1576-1592: LGTM! Per-instance topic configuration working correctly.The publish_introspection method correctly uses
self._introspection_topicfor both:
- Primary path with
publish_envelope()(line 1580)- Fallback path with raw
publish()(line 1587)This enables contract-driven, per-node topic customization while maintaining backward compatibility with the module-level default constants.
1681-1695: LGTM! Heartbeat publishing uses per-instance topic correctly.The
_publish_heartbeatmethod follows the same pattern aspublish_introspection, usingself._heartbeat_topicfor both primary and fallback publishing paths. This consistency makes the codebase maintainable and predictable.
2273-2287: LGTM! Public API exports are correct.The
__all__list correctly includes the two new public API elements:
ModelIntrospectionConfig- configuration model for initialize_introspectionProtocolIntrospectionEventBus- protocol for type checking event bus implementationsThese additions align with the documented usage patterns and enable proper type checking for consumers of this module.
2200-2234: Thread-safe cache invalidation is correct and all callers have been updated to useawait.The method has been properly converted to async to support
asyncio.Lock. Verification confirms all actual calls toinvalidate_introspection_cache()useawait:
- Test calls (lines 628, 1361): both properly awaited
- Docstring example (line 2225): correctly shows await usage
The breaking change has been fully addressed—no unawaited call sites remain in the codebase.
Documentation fixes: - Remove unsupported shutdown_topic from code example - Add contract.yaml example with topic-to-config mapping table - Clarify only 3 topics are configurable via ModelIntrospectionConfig - Fix CHANGELOG ModelIntrospectionConfig field names to match implementation Test coverage improvements: - Add 8 tests for custom topic parameter handling (legacy + config model) - Add 12 comprehensive thread-safety race condition tests - Verify concurrent cache access patterns under high contention - Total tests: 125 (all passing)
PR Review: Contract-Driven Topic Configuration [OMN-881]SummaryThis PR implements contract-driven Kafka topic configuration, migrating from hardcoded EnumKafkaTopic to declarative contract.yaml event channels. The implementation is architecturally sound and follows ONEX principles, with comprehensive testing and documentation. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Testing
4. Documentation
🔍 Issues & RecommendationsCritical Issues: None ✅High Priority1. Missing Validation in ModelIntrospectionConfigLocation: src/omnibase_infra/mixins/mixin_node_introspection.py:254-369 The config model accepts optional topic names but doesn't validate their format. Recommend adding Pydantic field validators to ensure topic names follow ONEX naming convention (onex.*). 2. Inconsistent Error Handling in Registry ListenerLocation: src/omnibase_infra/mixins/mixin_node_introspection.py:1887-1967 The on_request callback resets failure counter on early exit (no message value) but this may hide systematic issues. Recommend only resetting counter after successful introspection publishing. Medium Priority3. Topic Name Constants Should Be Configurable Class VariablesLocation: src/omnibase_infra/mixins/mixin_node_introspection.py:238-240 Module-level constants limit testability and multi-tenant scenarios. Already partially addressed via config model. 4. Incomplete Active Operations TrackingLocation: src/omnibase_infra/mixins/mixin_node_introspection.py:1666-1678 Active operations count hardcoded to 0. Recommend creating ticket (OMN-XXX) and tracking as technical debt. 5. Missing Contract Validation ExamplesLocation: docs/architecture/EVENT_STREAMING_TOPICS.md:594-602 Validation rules documented but no code examples for enforcement. Recommend adding reference to validation tooling. Low Priority6. Test Coverage for Contract IntegrationTests validate config model but not contract.yaml → config → mixin integration. Recommend integration test in follow-up PR. 7. Performance Metrics Not Exposed via Introspection Eventget_performance_metrics() returns local metrics but doesn't include them in published events. Consider for post-MVP observability. 📋 Checklist Verification
🎯 Recommendations SummaryBefore Merge
Follow-up Tickets
📊 Overall AssessmentVerdict: ✅ Approve with Minor Changes This PR represents high-quality ONEX infrastructure work:
The recommended changes are minor and can be addressed quickly. The core implementation is production-ready. Estimated Risk: Low 🔗 Related Work
Great work on this foundational infrastructure piece! 🚀 |
…ation tests [OMN-881] High Priority: - Add Pydantic field validators for topic name format (must start with "onex.") - Fix inconsistent error handling in registry listener (only reset failure counter on success) Medium Priority: - Convert module-level topic constants to class-level defaults (no backwards compat) - Add contract validation examples to EVENT_STREAMING_TOPICS.md docs - Create Linear tickets: OMN-925 (active ops tracking), OMN-926 (perf metrics in events) Low Priority: - Add 20 contract integration tests for contract.yaml → config → mixin workflow - Add topic validation test coverage (9 new tests) - Add class-level defaults test coverage (6 new tests) BREAKING CHANGE: Removed deprecated module-level topic constants (INTROSPECTION_TOPIC, HEARTBEAT_TOPIC, REQUEST_INTROSPECTION_TOPIC). Use MixinNodeIntrospection.DEFAULT_*_TOPIC class variables instead.
PR Review: Contract-Driven Topic Configuration [OMN-881]Overall Assessment: APPROVED ✅Exceptionally well-executed PR migrating from hardcoded Kafka topics to contract-driven configuration. Key Strengths:
Code Quality: 9/10Type Safety & Validation ⭐⭐⭐⭐⭐
Thread Safety ⭐⭐⭐⭐⭐
Configuration Model ⭐⭐⭐⭐⭐
Security: 10/10✅ Network security documented Test Coverage: 10/10
Documentation: 10/10EVENT_STREAMING_TOPICS.md (796 lines)
RecommendationsMedium Priority:
Low Priority:
Final Verdict✅ APPROVED - Merge Immediately This PR represents exemplary engineering. It solves the right problem with production-ready code, comprehensive testing, and excellent documentation. Post-Merge Follow-ups:
Reviewed by: Claude Sonnet 4.5 |
…MN-881] Add validation to ensure topic names end with version suffix (.v1, .v2, etc.) following ONEX naming conventions. This enforces consistent versioning across all introspection, heartbeat, and request topics. - Add _VERSION_SUFFIX_PATTERN regex in ModelIntrospectionConfig - Add version suffix check in validate_topic_name() validator - Update existing tests to use valid topic names with version suffixes - Add 4 new tests for version suffix validation scenarios - Update EVENT_STREAMING_TOPICS.md documentation with new validation rule
Code Review: Contract-Driven Kafka Topic Configuration [OMN-881]SummaryThis PR successfully migrates from hardcoded ✅ StrengthsArchitecture & Design
Code Quality
Security
🔍 Issues Found1. CRITICAL: Breaking Change Not Mentioned in Title/SummaryThe PR title mentions "implement contract-driven topic configuration" but doesn't flag the breaking change to Impact: Developers may miss this when reviewing/merging 2. Contract YAML InconsistencyIn
But in EVENT_STREAMING_TOPICS.md (lines 432-467), the documented contract format uses:
Impact: Confusion between documentation and actual implementation 3. Missing Test for Invalid Topic NamesThe
Impact: Validators may not be fully exercised 4. Topic Name Validation Only on Config FieldsThe validators only apply to Impact: Contract-defined topics in 5. Potential Race Condition in Cache InvalidationIn Code location: mixin_node_introspection.py (cache invalidation method) 💡 Suggestions (Non-Blocking)Performance
Documentation
Code Structure
🔒 Security Review✅ No security issues found
🧪 Test CoverageUnit Tests: Excellent (1,940+ lines)
Integration Tests: Good (947 lines)
Missing Tests (see Issue #3 above):
📋 Checklist Review
🎯 Recommendations SummaryMust Fix Before Merge:
Should Fix Before Merge:
Nice to Have (Post-Merge):
Final VerdictApprove with Minor Changes ✅ This is high-quality work that significantly improves the infrastructure's flexibility. The contract-driven approach is exactly what ONEX needs for multi-tenant deployments. The breaking change is well-documented but should be more prominent. Address the critical and should-fix items, and this is ready to merge. Excellent work on the comprehensive documentation and test coverage! 🚀 Review completed by: Claude Code (Sonnet 4.5) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/integration/mixins/test_mixin_node_introspection_contract_integration.py (3)
47-115: Consider using specific types in MockEventBus for better test type safety.The
MockEventBusclass usesAnytype annotations in several places (lines 59, 97), which technically violates the coding guideline "NEVER use Any type annotation." While test utilities have more flexibility, using specific types would improve test reliability and catch type mismatches earlier.🔎 Suggested type improvements
class MockEventBus: """Mock event bus for testing introspection publishing without Kafka.""" def __init__(self) -> None: """Initialize mock event bus.""" - self.published_envelopes: list[tuple[Any, str]] = [] - self.published_events: list[dict[str, Any]] = [] + self.published_envelopes: list[tuple[object, str]] = [] + self.published_events: list[dict[str, object]] = [] self.subscribed_topics: list[str] = [] self.subscribed_groups: list[str] = [] async def publish_envelope( self, - envelope: Any, + envelope: object, topic: str, ) -> None:
451-494: Timing-based heartbeat test may be flaky in CI environments.The test uses a 50ms heartbeat interval with a 150ms sleep, expecting at least one heartbeat. While this should work in most cases, CI environments under load may experience scheduling delays that could cause intermittent failures.
Consider increasing the margins or using a more deterministic approach.
🔎 Suggested improvement for reliability
# Start heartbeat tasks with very short interval await node.start_introspection_tasks( enable_heartbeat=True, - heartbeat_interval_seconds=0.05, # 50ms for fast test + heartbeat_interval_seconds=0.1, # 100ms for test reliability enable_registry_listener=False, ) try: # Wait for at least one heartbeat - await asyncio.sleep(0.15) + await asyncio.sleep(0.25) # 2.5x interval for CI reliability
833-880: Missing test for version suffix validation.The
validate_topic_namevalidator requires topics to end with a version suffix (e.g.,.v1,.v2), but there's no test case verifying that topics without version suffixes are rejected.🔎 Suggested additional test case
async def test_invalid_topic_without_version_suffix_rejected(self) -> None: """Verify topics without version suffix are rejected.""" with pytest.raises(ValueError, match="must end with version suffix"): ModelIntrospectionConfig( node_id="validation-test", node_type="EFFECT", introspection_topic="onex.topic.without.version", )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (6)
CHANGELOG.md(2 hunks)docs/architecture/EVENT_STREAMING_TOPICS.md(1 hunks)src/omnibase_infra/mixins/mixin_node_introspection.py(28 hunks)tests/integration/mixins/__init__.py(1 hunks)tests/integration/mixins/test_mixin_node_introspection_contract_integration.py(1 hunks)tests/unit/mixins/test_mixin_node_introspection.py(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/integration/mixins/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/architecture/EVENT_STREAMING_TOPICS.md
- tests/unit/mixins/test_mixin_node_introspection.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
NEVER use Any type annotation. Always use specific types
Files:
src/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: All data structures must be proper Pydantic models
Use X | None (PEP 604) over Optional[X] for nullable type annotations
Error classes must raise OnexError (raise OnexError(...) from e) as the base infrastructure error pattern
Protocol resolution must use duck typing through protocols, never isinstance checks
Files:
src/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.py
**/mixin_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin files must follow naming convention: mixin_.py with class pattern Mixin
Files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧠 Learnings (36)
📓 Common learnings
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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/pr.mdc:0-0
Timestamp: 2025-11-24T16:33:09.011Z
Learning: Applies to docs_private/dev_logs/jonah/pr/pr_description_*.md : Each PR description must include the following required sections: PR Title, Branch, PR ID or Link, Summary of Changes, Key Achievements, Prompts & Actions (Chronological with timestamps in ISO 8601 format and agent attribution), Major Milestones, Blockers / Next Steps, Metrics (Lines Changed in "+X / -Y" format, Files Modified count, Time Spent if tracked), and must include optional sections where relevant: Related Issues/Tickets, Breaking Changes, Migration/Upgrade Notes, Documentation Impact, Test Coverage, Security/Compliance Notes, Reviewer(s), and Release Notes Snippet
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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
📚 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:
CHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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: Node implementations must use mixin-based composition from `omnibase_core.mixins` (e.g., `MixinHealthCheck`, `MixinNodeExecutor`) to add capabilities
Applied to files:
CHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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 : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
CHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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 nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
CHANGELOG.md
📚 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:
CHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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:
CHANGELOG.md
📚 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 nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
CHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.183Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.183Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use ModelONEXContainer for dependency injection in node constructors, not ModelContainer[T]
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelEventEnvelope for inter-service event-driven communication
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Avoid using Any, dict, or primitive types in protocol signatures; use the strongest typing possible with Pydantic models
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.184Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.184Z
Learning: Applies to src/omnibase_core/**/*.py : Use duck typing with protocols instead of isinstance checks for protocol validation
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T22:04:24.183Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T22:04:24.183Z
Learning: Applies to src/omnibase_core/**/*.py : Use PEP 604 union syntax (str | None) instead of typing.Union and typing.Optional for type annotations
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/{models,protocols}/{model_*,protocol_*}.py : Avoid using Any, dict, or primitive types in model and protocol definitions; use strongest typing possible
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/protocols/protocol_*.py : Use Protocol from typing module for all interface definitions; never use ABC (Abstract Base Classes) for service interfaces
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 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/**/*.py : Protocols must inherit from `typing.Protocol` and use `...` (ellipsis) for method bodies
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/protocols/protocol_*.py : Use TYPE_CHECKING guards and forward references for circular import prevention in protocol files
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 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 : Implement Node classes by inheriting from `NodeBase` with proper UUID and `ModelSemVer` fields
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Use Protocol for interface definitions when implementations may live outside core codebase; use Pydantic models only for base classes with shared logic
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Use Protocol for tool interfaces and plugin APIs based on method shape (structural typing), not Pydantic models with inheritance
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/protocols/protocol_*.py : All Protocol definitions must use model-only signatures: methods accept only validated Pydantic models, never dict, primitives, or argument models
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-12-18T17:14:26.130Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-18T17:14:26.130Z
Learning: Applies to **/nodes/**/node.py : Private methods prefixed with _ are excluded from capability discovery. Avoid exposing sensitive business logic in public method names. Use generic operation names.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 deployment/docker-compose*.yml : Docker Compose files MUST use dual listener architecture for Kafka (9092 internal, 29092 external). Update /etc/hosts with: 192.168.86.200 omninode-bridge-redpanda omninode-bridge-consul omninode-bridge-postgres
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/unit/mixins/test_mixin_*.py : Mixin tests must be organized in test classes and test mixin initialization, inheritance, and core mixin functionality
Applied to files:
tests/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.py
🔇 Additional comments (12)
CHANGELOG.md (1)
12-42: LGTM!The changelog accurately documents the breaking change for
invalidate_introspection_cache()becoming async with clear migration guidance. TheModelIntrospectionConfigfield documentation now correctly matches the implementation, including all topic configuration fields (introspection_topic,heartbeat_topic,request_introspection_topic).src/omnibase_infra/mixins/mixin_node_introspection.py (7)
245-443: LGTM!The
ModelIntrospectionConfigPydantic model is well-structured with:
- Proper field validation (
min_length=1for required strings,ge=0.0forcache_ttl)- Comprehensive topic name validation using
field_validatorenforcing ONEX naming conventions (prefix, valid characters, version suffix)- Type-safe
event_busfield using the newProtocolIntrospectionEventBusprotocol- Clear documentation with examples
978-986: Topic resolution uses falsy check - empty strings from config would use defaults.The topic resolution uses
oroperator:self._introspection_topic = ( introspection_topic or self.DEFAULT_INTROSPECTION_TOPIC )This means an empty string
""would be treated as falsy and fall back to the default. However,ModelIntrospectionConfigalready validates topic names, so empty strings would be rejected by the validator before reaching here. This is actually correct behavior since the validator requires theonex.prefix.
1497-1575: Potential optimization: Lock is released between cache check and population.The current implementation releases the lock after the cache validity check (line 1519), then performs expensive capability discovery without holding the lock, and finally reacquires the lock to update the cache (line 1571).
This is actually the correct pattern for performance - holding the lock during reflection would serialize all concurrent introspection calls. The trade-off is that concurrent cache misses may redundantly compute introspection data, but this is acceptable since:
- The operations are idempotent
- Cache population is atomic under the lock
- Subsequent calls will hit the cache
LGTM! The lock usage correctly balances thread safety with performance.
1683-1698: LGTM!The
publish_introspectionmethod correctly uses the instance-configuredself._introspection_topicfor publishing, enabling contract-driven topic configuration. The same pattern is consistently applied in_publish_heartbeatand_registry_listener_loop.
2317-2351: LGTM!The
invalidate_introspection_cachemethod is correctly made async to support the async lock. The implementation atomically clears both cache variables under the lock, preventing the race condition described in the docstring. The breaking change is properly documented in the CHANGELOG with migration guidance.
2401-2412: LGTM!The
__all__exports correctly include the new public entities (ModelIntrospectionConfig,ProtocolIntrospectionEventBus) alongside existing exports.
214-229: No changes needed. Thesubscribemethod's return typeCallable[[], Awaitable[None]]correctly matches all protocol implementations (KafkaEventBus and InMemoryEventBus both return async unsubscribe functions). The broader typing of_registry_unsubscribeto accept both sync and async is defensive handling at the attribute level, not a protocol issue.tests/integration/mixins/test_mixin_node_introspection_contract_integration.py (4)
118-178: LGTM!The
ContractDrivenEffectNodehelper class accurately simulates how a real ONEX node would extract configuration fromcontract.yamland initialize introspection. The topic extraction logic using dict comprehensions is clean and correctly mapsevent_typetotopic.
228-345: LGTM!Comprehensive test coverage for contract-to-config integration:
- Full contract with all event channels
- Domain-specific topic configuration
- Partial configuration with proper fallback to
DEFAULT_*constantsThe tests correctly verify both topic configuration and node metadata flow through from contract data.
888-947: LGTM!The subclass override tests effectively verify the topic configuration precedence:
- Subclass can override
DEFAULT_*class variables- Config values take precedence over subclass defaults
This validates the multi-tenant and domain-specific deployment patterns described in the PR objectives.
1-40: LGTM!Comprehensive integration test suite covering:
- Contract-driven topic configuration flow
- End-to-end introspection workflows
- Multi-domain and multi-channel scenarios
- Edge cases (empty/missing event_channels)
- Performance metrics validation
- Topic name validation
- Subclass override behavior
The module-level
pytestmarkcorrectly appliesintegrationandasynciomarkers to all tests.
…w [OMN-881] Add 12 new tests addressing PR #54 review feedback: Custom Topic Configuration Tests (6 tests): - test_initialize_introspection_custom_introspection_topic - test_initialize_introspection_custom_heartbeat_topic - test_initialize_introspection_custom_request_topic - test_initialize_introspection_default_topics - test_initialize_introspection_all_custom_topics - test_initialize_introspection_partial_custom_topics Enhanced Thread Safety Tests (6 tests): - test_concurrent_cache_invalidation_and_access_with_timing - test_high_contention_burst_invalidation - test_cache_consistency_verification_under_load - test_lock_contention_with_slow_operations - test_interleaved_invalidation_sequences - test_stress_test_sustained_concurrent_access All 156 tests pass with full linting compliance.
Pull Request Review: Contract-Driven Topic Configuration (OMN-881)🎯 OverviewThis PR successfully implements contract-driven Kafka topic configuration, migrating from hardcoded enums to flexible, per-node topic customization. The architectural changes are well-documented and align with ONEX principles. ✅ Strengths1. Excellent Documentation
2. Robust Configuration Model# ModelIntrospectionConfig provides clean API with validation
class ModelIntrospectionConfig(BaseModel):
node_id: str = Field(..., min_length=1)
node_type: str = Field(..., min_length=1)
introspection_topic: str | None = Field(default=None)
heartbeat_topic: str | None = Field(default=None)
request_introspection_topic: str | None = Field(default=None)
@field_validator('introspection_topic', 'heartbeat_topic', ...)
def validate_topic_name(cls, v: str | None) -> str | None:
# Enforces onex.* prefix, valid characters, version suffixValidation rules enforced:
3. Thread Safety
4. Backwards Compatibility
5. Test Coverage
🔴 Issues Requiring AttentionCRITICAL: Performance Concern - Reflection OverheadThe # Line 1288-1308 in mixin_node_introspection.py
cached_signatures = self._get_class_method_signatures()
discover_elapsed_ms = (time.perf_counter() - discover_start) * 1000
if elapsed_ms > PERF_THRESHOLD_GET_CAPABILITIES_MS: # 50ms threshold
logger.warning("Capability discovery exceeded 50ms target")Observations:
Recommendation: # Example optimization pattern
class CapabilityMeta(type):
def __new__(mcs, name, bases, attrs):
cls = super().__new__(mcs, name, bases, attrs)
cls._capability_cache = mcs._build_capabilities(cls)
return clsThis would eliminate reflection overhead entirely from the hot path. MEDIUM: Error Handling - Swallowed ExceptionsIn # Line 2043-2083
except Exception as e:
self._registry_callback_consecutive_failures += 1
# ... rate-limited logging ...
# Continue processing - graceful degradationIssue: This can hide persistent failures that should trigger alerts. While graceful degradation is good, consider:
MEDIUM: Security - Sensitive Data Exposure Risk
# Line 1114-1117
sig = inspect.signature(attr)
signatures[name] = str(sig) # Exposes: (user_id: str, payment_token: str) -> boolSecurity Note (Line 1074-1086):
Example sanitization: def _sanitize_signature(sig: str) -> str:
# Replace parameter names: (user_id: str, token: str) -> (arg1: str, arg2: str)
import re
return re.sub(r'\b[a-z_][a-z0-9_]*(?=:)', lambda m: f'arg{m.start()}', sig)LOW: Topic Name Validation Missing Semantic Checks
# Line 432-435 - validates version suffix exists
if not cls._VERSION_SUFFIX_PATTERN.search(v):
raise ValueError(f"Topic name must end with version suffix (e.g., .v1, .v2). Got: '{v}'")Missing validation:
Recommendation: _CANONICAL_TOPICS: ClassVar[set[str]] = {
"onex.node.introspection.published.v1",
"onex.node.heartbeat.published.v1",
# ... rest of canonical topics
}
@field_validator('introspection_topic')
def validate_canonical_topic(cls, v: str | None) -> str | None:
if v and v not in cls._CANONICAL_TOPICS:
logger.warning(f"Non-canonical topic: {v}")
return vThis is 📊 Performance AnalysisPerformance thresholds (with CI multiplier):
Test coverage:
Metrics tracking:
Recommendation: Monitor production metrics and consider OMN-926 (add metrics to published events) for distributed observability. 🔒 Security ReviewStrengths:
Recommendations:
🧪 Test Coverage AssessmentQuantitative:
Qualitative:
Gap: Integration tests for contract-driven topic wiring (though 📝 Code QualityCompliance with ONEX Standards:
Complexity Metrics:
🎯 RecommendationsBefore Merge:
Follow-up Tickets:
✅ Approval RecommendationStatus: ✅ APPROVE WITH MINOR FOLLOW-UPS This PR represents high-quality work that:
The identified issues are non-blocking and can be addressed in follow-up work. The critical performance concern is mitigated by class-level caching and should be monitored in production. Merge Checklist:
📚 ReferencesFiles Reviewed:
ONEX Compliance:
Performance:
Security:
Reviewed by: Claude Code (Sonnet 4.5) |
CRITICAL fixes: - Add thread safety documentation to CLAUDE.md - Document breaking change (async invalidate_introspection_cache) prominently MAJOR fixes: - Add explicit empty string validation for node_id/node_type fields - Add empty string handling for topic fields (converts to None) - Add 19 new tests for version suffix and topic name validation MINOR fixes: - Fix node_version -> version in EVENT_STREAMING_TOPICS.md example - Verify topic count documentation (12 is correct) NITPICK fixes: - Replace Any type with ModelEventMessage in MockEventBus - Use TYPE_CHECKING import pattern for test type safety Test coverage: 175 tests passing (19 new tests added)
Code Review: Contract-Driven Topic Configuration (OMN-881)SummaryThis PR implements contract-driven Kafka topic configuration for ONEX infrastructure, migrating from hardcoded ✅ Strengths1. Architecture & Design
2. Documentation Quality
3. Testing Coverage
4. Type Safety
🔍 Code Quality ObservationsBreaking Change Handling ✅The
Topic Validation Logic ✅The # Line 409-471: Comprehensive validation with clear error messages
@field_validator("introspection_topic", "heartbeat_topic", "request_introspection_topic")
@classmethod
def validate_topic_name(cls, v: str | None) -> str | None:
if v is None or v == "": return None # Graceful handling
if not v.startswith(cls._TOPIC_PREFIX): ... # onex. prefix check
if not cls._TOPIC_NAME_PATTERN.match(v): ... # Valid characters
if not cls._VERSION_SUFFIX_PATTERN.search(v): ... # .vN suffix checkPerformance Metrics ✅The
🚨 Issues Found1. Missing Validation: Empty String After Prefix (Minor)Location: # Current code checks non-empty suffix
suffix = v[len(cls._TOPIC_PREFIX):]
if not suffix:
raise ValueError(...)Issue: The version suffix check at line 466 makes the empty suffix check redundant. A topic like Recommendation: This is actually correct defensive programming - fail fast with a clear error message. The check provides better error messages for invalid topics. No change needed. 2. Inconsistent Error Logging Pattern (Minor)Location: Multiple locations use Examples:
# Current pattern
logger.error( # noqa: G201
f"Failed to publish...",
extra={...},
exc_info=True,
)Rationale in comments: "Use error() with exc_info=True instead of exception() to include structured error_type and error_message fields for log aggregation" Assessment: This is intentional and correct. The structured 3. Potential Race Condition in Cache Invalidation (Low Risk)Location: async def invalidate_introspection_cache(self) -> None:
async with self._introspection_cache_lock:
self._introspection_cache = None
self._introspection_cached_at = NoneIssue: If Analysis: Actually NOT a race condition - the lock is held during the entire cache validity check in Verdict: ✅ Thread-safe as implemented. 4. ModelIntrospectionConfig Field Ordering (Cosmetic)Location: Observation: Required fields ( Suggestion: Consider grouping related optional fields:
Verdict: Current ordering is fine, grouping would be a minor improvement for readability. 🎯 Security Review✅ Input Validation
✅ Information Disclosure Controls
|
Resolved conflicts: - CHANGELOG.md: kept both breaking change entries - pyproject.toml: accepted main's git tag for omnibase-core v0.5.6 - poetry.lock: accepted main's version - mixins/__init__.py: use ProtocolEventBusLike from separate file - mixin_node_introspection.py: accepted main's cleaner architecture - test_mixin_node_introspection.py: accepted main's version - node_registry_effect/v1_0_0/contract.yaml: accepted deletion from main
Code Review: Contract-Driven Topic Configuration (PR #54)SummaryThis PR successfully implements contract-driven Kafka topic configuration, migrating from hardcoded enums to a flexible, contract-based approach. The implementation is well-architected, thoroughly tested, and properly documented. ✅ Strengths1. Excellent Architecture & Design
2. Comprehensive Testing
3. Outstanding Documentation
4. ONEX Compliance
🔍 Areas for ImprovementCRITICALNone - No blocking issues found. MAJOR1. Topic Count Documentation Inconsistency (Low Priority)The PR summary states "12 topics, LOCKED for MVP" and the specification lists exactly 12 topics in Section 10. However, the documentation should explicitly verify this count matches the implementation: Recommendation: # In tests or validation script
EXPECTED_TOPIC_COUNT = 12
canonical_topics = [
"onex.node.introspection.published.v1",
"onex.node.heartbeat.published.v1",
# ... (all 12 topics)
]
assert len(canonical_topics) == EXPECTED_TOPIC_COUNT2. Missing Validation for Topic-Event Type ConsistencyThe contract defines both Example Risk: event_channels:
publishes:
- event_type: "introspection"
topic: "onex.node.introspection.published.v1"
# Node could still call publish_introspection() and publish to a different topicRecommendation: Add contract enforcement in async def publish_introspection(self, reason: str) -> bool:
# Validate topic matches contract declaration
if hasattr(self, '_contract_topics'):
assert self._introspection_topic in self._contract_topics['publishes']
# ... existing publish logicMINOR3. Type Annotation Enhancement OpportunityThe self.published_envelopes: list[tuple[Any, str]] = []Per ONEX guidelines ("NEVER use Any"), consider: from typing import Protocol
class SupportsDictProtocol(Protocol):
def model_dump(self) -> dict[str, Any]: ...
self.published_envelopes: list[tuple[SupportsDictProtocol, str]] = []4. Performance Metrics EnhancementThe PR adds # In ModelNodeIntrospectionEvent
class ModelNodeIntrospectionEvent(BaseModel):
# ... existing fields
performance_metrics: ModelIntrospectionPerformanceMetrics | None = NoneRelated ticket: OMN-926 (mentioned in commits) NITPICK5. Docstring ConsistencySome test docstrings use passive voice: """Test that publish fails if bus not started.""" # Current
"""Verify publish fails when bus not started.""" # More activeMinor style preference - current style is acceptable. 6. Magic Number in Testsawait asyncio.sleep(0.15) # Tests use various sleep durationsConsider constants: HEARTBEAT_TEST_INTERVAL = 0.05
CONSUMER_STARTUP_DELAY = 0.1
CIRCUIT_BREAKER_TIMEOUT = 0.15🔒 Security Review✅ Excellent Security Posture
📊 Test Coverage Analysis
Test Quality: ⭐⭐⭐⭐⭐
🎯 Performance Considerations✅ Good Performance Patterns
💡 Optimization Opportunities
🚀 Migration PathBreaking Changes Properly DocumentedThe
Migration Effort: Low (simple 📝 Recommendations Summary
✅ Final VerdictAPPROVE ✅ This PR represents high-quality work that:
The recommendations above are refinements, not blockers. This PR is ready to merge and sets a strong example for contract-driven infrastructure configuration. Reviewed by: Claude Sonnet 4.5 (ONEX Infrastructure Review Agent) |
MAJOR fixes: - Add topic count validation test (12 canonical topics) - Add contract enforcement in publish_introspection/heartbeat methods MINOR improvements: - Replace Any types with concrete types in MockEventBus - Add ModelIntrospectionPerformanceMetrics for observability - Replace magic numbers with named constants in tests
- Add 7 new validation tests for topic name edge cases - Enhance test thread-safety with asyncio.Barrier for true concurrency - Replace timing-based waits with polling loops for CI stability - Add cache-hit verification using get_performance_metrics() - Fix Vault concurrency test StopIteration bug (Python 3.12 compat) - Add documentation consistency comment in EVENT_STREAMING_TOPICS.md
Code Review: PR #54 - Node Introspection with Configurable TopicsOverall AssessmentThis is a well-architected PR that successfully migrates from hardcoded Kafka topics to contract-driven configuration. The code quality is high, follows ONEX conventions, and includes comprehensive documentation. I have some recommendations for improvement below. ✅ Strengths1. Excellent Documentation
2. Strong Type Safety
3. Performance Instrumentation
4. Robust Error Handling
5. Test Coverage
🔍 Issues & RecommendationsCritical Issues1. Security: Topic Validation is Not EnforcedLocation: The # Current behavior - only warns, does not block
if topic not in declared_publishes:
logger.warning(...) # ⚠️ Should raise insteadRecommendation: if topic not in declared_publishes:
raise ProtocolConfigurationError(
f"Topic '{topic}' not declared in contract publishes",
context=ModelInfraErrorContext(...)
)Rationale: Contract violations should fail fast, not silently continue. This prevents accidental publishing to wrong topics in production. 2. Legacy v1_0_0 Directory ViolationLocation: PR description mentions According to CLAUDE.md:
Recommendation:
High Priority Issues3. Topic Naming InconsistencyLocation: Default topics use legacy naming: DEFAULT_INTROSPECTION_TOPIC = "node.introspection" # Legacy
DEFAULT_HEARTBEAT_TOPIC = "node.heartbeat" # LegacyBut
Recommendation: 4. Type Annotation Violation: Use
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_infra/mixins/mixin_node_introspection.py (1)
1470-1554:publish_introspectiontopic selection and contract validation look correct; propagate correlation_id into error logsThe method now:
- validates
self._introspection_topicagainst_contract_topics,- uses the per-instance topic for both
publish_envelopeand rawpublish,- and allows callers to inject a
correlation_id(withuuid4()fallback), which is then written into the event and success log.One improvement: when the publish fails (the
except Exception as eblock), the structured log currently omits thecorrelation_id. Given the coding guideline to always propagate correlation IDs into error context, it would be better to include the effectivefinal_correlation_idin theextradict so downstream log aggregation/tracing can correlate failures to requests.
🧹 Nitpick comments (7)
tests/unit/mixins/test_mixin_node_introspection.py (2)
2847-3299: Topic-format tests are thorough but tightly coupled to error textThe
TestModelIntrospectionConfigTopicValidationsuite gives excellent coverage of the topic validator (whitespace, consecutive dots, invalid chars, case, start/end chars, version suffixes, empty/min-length, and single-character edge cases). One minor concern is the reliance on specific substrings fromValidationError.__str__in several tests – future tweaks to error wording invalidate_topic_formatcould cause brittle failures even if behavior is still correct. Consider, over time, asserting on structuredexc_info.value.errors()contents (field, type, message substring) rather than free‑formstr(exc)to decouple tests from exact phrasing.
3382-3609: Concurrent cache-access tests are well-designed; consider also asserting cache-hit/miss behaviorThe new
TestMixinNodeIntrospectionConcurrentCacheAccessclass does a nice job stress‑testing:
- concurrent cache hits under a long TTL,
- concurrent invalidation using
asyncio.Barrier,- concurrent expiration after TTL,
- concurrent init/access, and
- multi-instance isolation.
All of the
asyncio.gather(..., return_exceptions=True)checks and cardinality assertions look sound. As a small enhancement, you might also assert onget_performance_metrics().cache_hitin the concurrency/expiration scenarios (where deterministic) to link these tests more explicitly to the performance-instrumentation path you added elsewhere.src/omnibase_infra/mixins/mixin_node_introspection.py (3)
656-660: Per-instance topic fields are initialized correctly; consider adding class-level type hintsInitializing
_introspection_topic,_heartbeat_topic, and_request_introspection_topicfrom the config model here is the right place, and it keeps the legacyinitialize_introspection()path automatically in sync via delegation. For mypy readability, you might also add these attributes to the “Type annotations for instance attributes” section (e.g.,self._introspection_topic: str) so their existence is explicit to static analysis.
1415-1468: Contract-topic validation is safe but assumes_contract_topicsis mapping-like
_validate_contract_topicgives you a soft contract check (warning-only) against_contract_topics["publishes"], which is a good middle ground for backwards compatibility. One minor robustness concern is the implicit assumption that_contract_topicsis a dict-like object with.get; if some nodes accidentally set it to a list or other shape, this method will raise and potentially break publishing. A small defensive guard likeif not isinstance(contract_topics, dict): log + returnwould make this helper more resilient to bad contract wiring.
1586-1660: Heartbeat publishing now respects per-instance topics and contract validationSwitching
_publish_heartbeatto validateself._heartbeat_topicand to consistently use it for both envelope and raw publish paths lines this up with the new topic configuration model and mirrorspublish_introspectioncorrectly. Same as above, you might later consider including the heartbeat’s correlation_id in the error log payload for stronger traceability, but the functional behavior here is sound.tests/integration/mixins/test_mixin_node_introspection_contract_integration.py (2)
100-100: Consider moving the import to the module level.The
import jsonstatement is inside thepublishmethod. While this works and doesn't cause issues in test code, it's generally better practice to place imports at the module level for consistency and clarity.🔎 Proposed refactor
Move the import to the top of the file with other imports:
At line 25, add:
import asyncio +import json from collections.abc import Awaitable, CallableThen remove it from line 100.
144-144: Use specific types instead ofAnyfor dictionary values.Several locations use
dict[str, Any]which violates the coding guideline "NEVER useAnytype - Always use specific types." While test code may handle dynamic structures, consider using TypedDict or more specific type annotations for contract data and payloads.As per coding guidelines, all data structures should be proper Pydantic models or TypedDict definitions.
Example: Define TypedDict for contract structure
from typing import TypedDict class ContractMetadata(TypedDict, total=False): name: str version: str node_type: str class EventChannel(TypedDict): event_type: str topic: str class EventChannels(TypedDict, total=False): publishes: list[EventChannel] subscribes: list[EventChannel] class ContractData(TypedDict, total=False): metadata: ContractMetadata event_channels: EventChannelsThen use
contract_data: ContractDatainstead ofcontract_data: dict[str, Any].Also applies to: 173-173, 196-196, 242-242, 964-964
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (7)
CHANGELOG.mddocs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.pytests/unit/handlers/test_handler_vault_concurrency.pytests/unit/mixins/test_mixin_node_introspection.pytests/unit/validation/test_topic_count_validation.py
✅ Files skipped from review due to trivial changes (1)
- docs/architecture/EVENT_STREAMING_TOPICS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/handlers/test_handler_vault_concurrency.pytests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.pytests/unit/validation/test_topic_count_validation.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.py
**/*vault*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use credential refresh for
InfraAuthenticationError. Pattern: check if credential near expiration, refresh before use, automatically re-authenticate on auth failure with retry.
Files:
tests/unit/handlers/test_handler_vault_concurrency.py
**/mixin_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
mixin_<name>.pyfile naming pattern withMixin<Name>class pattern. Example:mixin_health_check.py→MixinHealthCheck
Files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧠 Learnings (20)
📓 Common learnings
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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
📚 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 : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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 nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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]*/introspection.py : All ONEX nodes must include an `introspection.py` file implementing standards-compliant introspection logic
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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 **/node.py : Prefix internal/sensitive methods with `_` to exclude them from introspection. Node introspection uses reflection to discover public methods - private methods are hidden from exposure.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Avoid using Any, dict, or primitive types in protocol signatures; use the strongest typing possible with Pydantic models
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/{models,protocols}/{model_*,protocol_*}.py : Avoid using Any, dict, or primitive types in model and protocol definitions; use strongest typing possible
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/protocols/protocol_*.py : Use Protocol from typing module for all interface definitions; never use ABC (Abstract Base Classes) for service interfaces
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/*dispatcher*.py : Use `ModelEventEnvelope[object]` instead of `Any` for generic dispatchers that accept any payload type. Use specific types like `ModelEventEnvelope[UserCreatedEvent]` when the payload type is known.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 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/**/*.py : Protocols must inherit from `typing.Protocol` and use `...` (ellipsis) for method bodies
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/protocols/protocol_*.py : Use TYPE_CHECKING guards and forward references for circular import prevention in protocol files
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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: Node implementations must use mixin-based composition from `omnibase_core.mixins` (e.g., `MixinHealthCheck`, `MixinNodeExecutor`) to add capabilities
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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: 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:
tests/unit/validation/test_topic_count_validation.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/validation/test_topic_count_validation.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.py
📚 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 tests/**/*.py : Write comprehensive test coverage following the test structure under `tests/unit/` organized by subsystem (enums, models, mixins, utils)
Applied to files:
tests/unit/validation/test_topic_count_validation.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/unit/mixins/test_mixin_*.py : Mixin tests must be organized in test classes and test mixin initialization, inheritance, and core mixin functionality
Applied to files:
tests/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.py
🧬 Code graph analysis (3)
tests/unit/handlers/test_handler_vault_concurrency.py (1)
tests/unit/handlers/test_handler_vault.py (1)
mock_hvac_client(58-72)
tests/unit/validation/test_topic_count_validation.py (1)
src/omnibase_infra/projectors/snapshot_publisher_registration.py (1)
topic(224-226)
tests/integration/mixins/test_mixin_node_introspection_contract_integration.py (2)
src/omnibase_infra/mixins/model_introspection_config.py (1)
ModelIntrospectionConfig(70-253)src/omnibase_infra/models/discovery/model_node_introspection_event.py (1)
ModelNodeIntrospectionEvent(61-266)
🔇 Additional comments (10)
tests/unit/handlers/test_handler_vault_concurrency.py (1)
13-13: LGTM! Appropriate import for cycling pattern.The
cycleimport is correctly added to support infinite iteration over mock responses, preventingStopIterationin concurrent scenarios.tests/unit/mixins/test_mixin_node_introspection.py (5)
59-72: Centralized timing constants for cache/heartbeat waits look solidUsing
_TIMING_MULTIPLIERderived fromPERF_MULTIPLIERto defineCACHE_TTL_WAIT,HEARTBEAT_WAIT,MULTIPLE_HEARTBEAT_WAIT, andCACHE_EXPIRE_WAITgives you a single place to tune CI vs local timing and reduces flakiness. The base values also line up sensibly with the documented TTL/intervals.
573-607: Cache/metrics assertions correctly exerciseIntrospectionPerformanceMetricsThe extended tests now validate both behavioral outcomes (timestamps/TTL behavior) and instrumentation (
cache_hitflips appropriately before/after cache use, expiry, and explicit invalidation). This is a good way to pin the semantics of_introspection_last_metricswithout over‑specifying absolute timings.Also applies to: 596-623, 650-681
799-812: Polling-based heartbeat tests are a clear improvement over fixed sleepsReplacing hard-coded
asyncio.sleepcalls with bounded polling loops usingHEARTBEAT_WAITandMULTIPLE_HEARTBEAT_WAITmakes the heartbeat tests more robust in slower CI environments while still having explicit upper bounds and informative assertion messages. The logic and time budgets look reasonable.Also applies to: 871-889, 956-982
1990-2002: UsingCACHE_EXPIRE_WAITkeeps metrics freshness test aligned with cache semanticsSwitching to
CACHE_EXPIRE_WAITfor the sleep before the second introspection call keeps this test coupled to the shared timing constants instead of a magic literal. Given you only assert positivity (not relative durations), this should remain stable even if thresholds are later tuned.
3303-3377: Custom topic wiring tests correctly exercise mixin–config integration
TestMixinNodeIntrospectionCustomTopicsvalidates that:
ModelIntrospectionConfig.introspection_topicandheartbeat_topicactually drive the topics used bypublish_introspectionand_publish_heartbeat, andrequest_introspection_topicis persisted on the mixin instance.This lines up with the new per-instance topic fields in the mixin and gives good guardrails around future refactors.
src/omnibase_infra/mixins/mixin_node_introspection.py (3)
172-195: Topic constants correctly delegate to config defaults while preserving exportsImporting
DEFAULT_INTROSPECTION_TOPIC/DEFAULT_HEARTBEAT_TOPIC/DEFAULT_REQUEST_INTROSPECTION_TOPICand wiring the publicINTROSPECTION_TOPIC,HEARTBEAT_TOPIC, andREQUEST_INTROSPECTION_TOPICto them keeps the existing module‑level constants stable while centralizing the actual string definitions inmodel_introspection_config. That’s a clean way to avoid divergence between config and mixin.
372-427: Thread-safety documentation is clear and appropriately scopedThe added sections spelling out single-threaded asyncio assumptions for the instance cache, class-level cache, and
invalidate_introspection_cache()are detailed and align with the actual implementation (no internal locking). Explicit examples of how to wrap withthreading.Lock/asyncio.Lockare helpful for anyone who wants to push this mixin into multi-threaded usage.Also applies to: 2171-2189
1958-1974: Registry listener subscription correctly uses the configured request-introspection topicUsing
self._request_introspection_topicin thesubscribecall and in the success log ensures the listener is bound to the same configurable topic surface as publishing, rather than a hard-coded global. This matches the ModelIntrospectionConfig defaults and the new custom-topic tests.tests/unit/validation/test_topic_count_validation.py (1)
1-351: Canonical topic governance tests are well-structured and consistent with the patternDefining a locked
CANONICAL_ONEX_TOPICSlist plusEXPECTED_TOPIC_COUNT, and then validating:
- count and uniqueness,
- naming pattern via
ONEX_TOPIC_PATTERN,- domain coverage (node/registry/infra),
- workflow/registration grouping,
- version suffix semantics, and
- a wide range of invalid examples,
gives a clear contract between docs and implementation. The regex matches all 12 canonical topics while correctly rejecting the negative cases you’ve enumerated. This should catch accidental drift in topic naming early.
- Add thread lock for cycle iterator in vault concurrency test to prevent race condition when VaultAdapter runs hvac calls in ThreadPoolExecutor - Clarify cache hit performance assertion comment to explain 2x variance allowance for CI stability - Standardize node_id type to UUID in contract integration tests for consistency with ModelIntrospectionConfig's NodeIdType coercion
Code Review: Node Introspection with Configurable Topics [OMN-881]Executive SummaryThis PR successfully implements contract-driven topic configuration for node introspection, migrating from hardcoded topic names to a flexible, typed configuration model. The implementation is well-architected and follows ONEX principles with comprehensive test coverage (982 integration tests, 889 unit tests added). Overall, this is high-quality work that significantly improves the infrastructure's flexibility and observability. Recommendation: ✅ Approve with minor suggestions Strengths1. Excellent Architecture Alignment
2. Comprehensive Documentation
3. Test Coverage
4. Performance Metrics
5. Topic Validation
Issues & Recommendations🔴 CRITICAL: Legacy v1_0_0 Directory Pattern ViolationLocation: Issue: The PR modifies a file in a
CLAUDE.md Reference: Recommendation:
Impact: Medium - This doesn't break functionality but creates technical debt and violates architectural standards. 🟡 MEDIUM: Cache Thread Safety Documentation GapLocation: Issue: While CLAUDE.md mentions "Cache operations are currently synchronous. For concurrent access patterns in high-contention async environments, external coordination may be needed," the mixin implementation doesn't clearly document the thread safety model. Current Cache Methods:
Concerns:
Recommendation:
Impact: Low-Medium - Current usage patterns (periodic heartbeats, startup introspection) have low contention, but high-traffic scenarios could hit race conditions. 🟡 MEDIUM: Topic Migration Strategy IncompleteLocation: Issue: The spec defines canonical ONEX topic names ( Current State: # model_introspection_config.py
DEFAULT_INTROSPECTION_TOPIC = "node.introspection" # Legacy
DEFAULT_HEARTBEAT_TOPIC = "node.heartbeat" # Legacy
DEFAULT_REQUEST_INTROSPECTION_TOPIC = "node.request_introspection" # LegacySpec Says: Recommendation:
Impact: Low - Feature works correctly with current topics, but creates inconsistency with spec. 🟢 MINOR: Performance Threshold Documentation InconsistencyLocation: Multiple files Issue: Performance thresholds are documented in multiple places with slight variations:
Recommendation: # Performance threshold constants (milliseconds)
PERF_THRESHOLD_GET_CAPABILITIES_MS = 50.0
PERF_THRESHOLD_DISCOVER_CAPABILITIES_MS = 30.0
PERF_THRESHOLD_TOTAL_INTROSPECTION_MS = 50.0
PERF_THRESHOLD_CACHE_HIT_MS = 1.0Then import these constants in both mixin and tests. This eliminates duplication and ensures consistency. Impact: Very Low - Documentation clarity improvement. 🟢 MINOR: Enum Opportunity for Node TypesLocation: Current: VALID_NODE_TYPES = frozenset({
"EFFECT", "COMPUTE", "REDUCER", "ORCHESTRATOR",
"effect", "compute", "reducer", "orchestrator",
})Recommendation: from enum import Enum
class EnumNodeType(str, Enum):
EFFECT = "EFFECT"
COMPUTE = "COMPUTE"
REDUCER = "REDUCER"
ORCHESTRATOR = "ORCHESTRATOR"Benefits:
Counter-argument: Current approach is simpler and works. Enums add complexity for marginal benefit. Acceptable as-is. Impact: Very Low - Optional enhancement. Security Review✅ Excellent Security DocumentationThe mixin includes comprehensive security documentation covering:
✅ No Security Vulnerabilities Detected
🟡 Minor Consideration: Registry Listener AuthenticationFrom docstring (line 100-101):
Recommendation: Consider adding a note in EVENT_STREAMING_TOPICS.md about authentication: ### Security: Request Introspection Topic
The `onex.registry.introspection.requested.v1` topic triggers nodes to re-broadcast introspection data. This is a **broadcast command** without built-in authentication.
**Mitigation**:
- Configure Kafka ACLs to restrict write access to registry services only
- Consider adding a `requestor_id` field to request payloads for audit trails
- Monitor request frequency to detect abuseImpact: Low - Current design is acceptable for MVP, but production should have ACLs. Test Coverage Assessment✅ Excellent CoverageUnit Tests (
Integration Tests (
Coverage Gaps: None identified. The test suite is comprehensive. Performance Review✅ Performance Targets MetThresholds (from
CI Considerations:
Recommendation: Current performance targets are reasonable for MVP. Consider adding percentile tracking (p50, p95, p99) in production metrics for better observability. Code Quality✅ High QualityStrengths:
Minor Improvements:
Compliance with ONEX Standards✅ Compliant
|
- Fix race condition in vault concurrency test mock response cycling - Add 37 new tests for topic validation (version suffix, invalid names) - Add class-level type hints for topic configuration fields - Add TypedDict for MockEventBus type safety - Add defensive Mapping type check for contract topics validation - Fix cache-hit performance assertion (was inverted) - Move uuid4 import to module level for consistency - Add thread safety documentation to CLAUDE.md - Fix contract.yaml examples (metadata → meta) - Update CHANGELOG with missing topic config fields
PR Review - Contract-Driven Kafka Topics [OMN-881]Overall AssessmentAPPROVE with observations. This is a well-architected, thoroughly tested PR that successfully migrates from hardcoded Kafka topic enums to contract-driven topic configuration. The implementation follows ONEX principles and demonstrates excellent engineering discipline. Key Strengths:
Code QualityArchitecture - ExcellentThe migration follows ONEX architectural principles:
Topic Naming Convention (from EVENT_STREAMING_TOPICS.md): Examples:
Design Highlight: The ModelIntrospectionConfig pattern is exemplary - consolidates 9 parameters into a typed config object, reduces initialize_introspection() parameter count below ONEX threshold, provides field validation (topic format, node type, cache TTL bounds), and enables contract-to-config-to-mixin workflow. SecurityInput Validation - ExcellentStrong validation in ModelIntrospectionConfig:
The introspection mixin has good security documentation in the module docstring (lines 23-113). Network security considerations for Kafka topics are well-documented. Test CoverageUnit Tests - Exceptional156 total tests with comprehensive coverage:
Test Organization: Well-structured test classes including TestMixinNodeIntrospectionCustomTopics, TestMixinNodeIntrospectionConcurrentCacheAccess, TestTopicVersionSuffixValidation, and more. Performance Testing: Includes CI-aware thresholds with PERF_MULTIPLIER (3.0 in CI vs 2.0 local). DocumentationEVENT_STREAMING_TOPICS.md - Outstanding771 lines of comprehensive Kafka topic specification:
Standout Section: Event Model Clarification prevents common distributed systems misconceptions by clearly stating that topics like onex.registry.node.registered.v1 are state transition events, not delivery acknowledgements. CHANGELOG.md - ExcellentBreaking changes clearly documented with migration paths and rationale. Breaking Changes1. invalidate_introspection_cache() Signature ChangeOld: await node.invalidate_introspection_cache() Impact: Low - Most nodes don't manually invalidate cache 2. New Configuration Model (ModelIntrospectionConfig)Impact: Low - Legacy method still supported Potential Issues1. Default Topics Still Use Legacy NamingFile: src/omnibase_infra/mixins/model_introspection_config.py:39-41 The defaults are still "node.introspection", "node.heartbeat", "node.request_introspection" while documentation promotes the new "onex." naming convention. Impact: Nodes not explicitly setting topics in contracts will publish to legacy topics. Recommendation: Consider updating defaults to match documented naming convention OR add a migration timeline comment explaining why legacy defaults are retained temporarily. 2. Topic Validation Missing Version Suffix CheckThe validate_topic_format() validator checks whitespace, consecutive dots, and special characters, but does not enforce version suffix (.v1, .v2) required by the naming convention. Example: "node.introspection" would pass validation but violates the onex....v convention. Note: This may be intentional if supporting both legacy and new naming during transition. If so, document this explicitly. 3. Performance Metrics Not Yet in Event PayloadModelIntrospectionPerformanceMetrics is defined but not yet included in ModelNodeIntrospectionEvent payload (planned in OMN-926). Recommendation: Consider adding a comment in ModelNodeIntrospectionEvent linking to OMN-926. Performance ConsiderationsCache Performance - ExcellentThe mixin includes TTL-based caching (default 300s), performance metrics tracking cache hit rates, thread safety for concurrent access, and sub-50ms target thresholds with CI-aware testing. Test Evidence: Cache hit performance verified in tests with assertions on cache_hit flag and timing thresholds. Kafka Publishing PerformanceOptimization Opportunity: For high-frequency heartbeats (every 30s per node), consider pre-serializing static parts of payload, using Kafka message batching if supported, and monitoring actual throughput in production (OMN-924). Code SmellsNone DetectedThe code follows ONEX conventions rigorously: No Any types, proper error handling with OnexError, duck typing via protocols, no backwards compatibility hacks, Pydantic models for all data structures, one model per file pattern followed. Metrics
RecommendationsMust-Fix Before MergeNone - PR is ready to merge. Should Consider
Nice-to-Have
HighlightsWhat This PR Does Exceptionally Well
Engineering Excellence ExamplesValidation Composition: Multiple validators on same field for whitespace, consecutive dots, special chars, and pattern matching. UUID Coercion: NodeIdType = Annotated[UUID, BeforeValidator(_coerce_to_uuid)] allows strings or UUIDs in config while ensuring UUID storage. CI-Aware Performance Testing: Uses environment detection to apply 3.0x multiplier in CI vs 2.0x local, preventing flaky tests. SummaryShip it. This is a high-quality PR that successfully achieves its goal of migrating to contract-driven topic configuration. The implementation is well-tested, thoroughly documented, and follows ONEX architectural principles throughout. The minor observations (default topic naming, version suffix validation) are not blockers and can be addressed in follow-up PRs if desired. Reviewed by: Claude Sonnet 4.5 |
…s [OMN-881] - Add version suffix validation for ONEX topics (.v1, .v2, etc.) - ONEX topics (onex.*) require version suffix or raise ValueError - Legacy topics warn but continue working for backward compatibility - Include performance metrics in introspection event payload - Add _to_pydantic_metrics() conversion method - Populate performance_metrics field in ModelNodeIntrospectionEvent - Add PerformanceMetricsCacheDict for typed cache operations - Add 6 new tests for version suffix validation - Merge with origin/main (resolve vault test conflict)
Replace non-existent ProtocolEventBusLike import with correct ProtocolEventBus from omnibase_core.protocols.event_bus. Move protocol import into TYPE_CHECKING block per linting rules.
Code Review: Node Introspection with Configurable Topics [OMN-881]SummaryThis PR introduces contract-driven topic configuration for node introspection, enabling nodes to declare their Kafka pub/sub topics in 🔴 Critical Issues1. Versioned Directory Pattern ViolationThe PR still references the legacy Location: ONEX Policy (from CLAUDE.md):
Required Action:
🟡 High Priority Issues2. Breaking Change Documentation IncompleteThe CHANGELOG correctly documents the Recommendations:
3. Topic Migration Path UnclearThe EVENT_STREAMING_TOPICS.md spec defines canonical topic names ( From the spec (Section 9):
Concerns:
Recommendations:
4. Security - Topic ACL Configuration MissingThe security documentation correctly identifies multi-tenant risks but lacks actionable deployment guidance. From mixin_node_introspection.py (lines 93-101):
Missing:
Recommendations: ## Security Configuration Example (Kafka ACLs)
# Introspection topics - internal cluster only
kafka-acls --add --allow-principal User:registry-service \
--operation Read --topic onex.node.introspection.published.v1
# Request topic - strict write access
kafka-acls --add --allow-principal User:registry-orchestrator \
--operation Write --topic onex.registry.introspection.requested.v1🟢 Code Quality Issues5. Type Annotation InconsistencyThe codebase mixes ONEX Standard (CLAUDE.md):
Found in mixin_node_introspection.py:
Action: Verify no 6. Performance Metrics Threshold DocumentationThe performance thresholds are well-defined as constants but lack rationale. Constants (lines 205-209): PERF_THRESHOLD_GET_CAPABILITIES_MS = 50.0
PERF_THRESHOLD_CACHE_HIT_MS = 1.0Question: How were these thresholds determined?
Recommendation: Add docstring comment explaining threshold selection: # Thresholds based on empirical testing with typical ONEX nodes (10-50 methods):
# - get_capabilities: 50ms covers reflection on ~100 methods at P95
# - cache_hit: 1ms for in-memory dict lookup at P99
PERF_THRESHOLD_GET_CAPABILITIES_MS = 50.07. ModelIntrospectionConfig Validation Edge CasesThe config model validates Current validation (model_introspection_config.py): node_type: str = Field(..., min_length=1)Consideration: Should this use an enum for type safety? from enum import Enum
class EnumNodeType(str, Enum):
EFFECT = "EFFECT"
COMPUTE = "COMPUTE"
REDUCER = "REDUCER"
ORCHESTRATOR = "ORCHESTRATOR"
node_type: EnumNodeType = Field(...)Trade-off: Enum provides compile-time safety but reduces flexibility. If ONEX plans to add node types dynamically, keep as string. Otherwise, consider enum. ✅ StrengthsExcellent Documentation
Strong Type Safety
Robust Error Handling
Comprehensive Test Coverage
📋 Minor Issues8. TODO Comment Without TicketLine 1358-1366: # TODO(ACTIVE-OP-TRACKING): Implement active operation tracking
# Ticket: Create Linear ticket for active operation tracking implementationIssue: TODO references creating a ticket but doesn't have a ticket number. Action: Create Linear ticket and update TODO with ticket ID (e.g., 9. Cache Invalidation Method NamingThe method Consideration: Would Verdict: Current name is acceptable since docstring clearly states it's synchronous. No change required, but worth considering for future API design. 10. Event Topic Constants LocationTopic constants are defined at module level (lines 196-198): INTROSPECTION_TOPIC = "node.introspection"
HEARTBEAT_TOPIC = "node.heartbeat"
REQUEST_INTROSPECTION_TOPIC = "node.request_introspection"Question: Should these move to a dedicated Current approach (module-level constants) is acceptable for MVP. Consider enum in post-MVP cleanup (see EVENT_STREAMING_TOPICS.md "Future Enhancements" section). 🎯 Test Coverage AssessmentIntegration Tests ✅
Unit Tests ✅
Coverage Gaps
🚀 Performance ConsiderationsCache Strategy
Potential Bottlenecks
Monitoring Recommendation: Track 📝 Documentation QualityCLAUDE.md Updates ✅
EVENT_STREAMING_TOPICS.md ✅
Missing Documentation
🔒 Security ReviewThreat Model ✅Excellent threat model documentation covering:
Mitigations ✅
Gaps
|
…[OMN-881] BREAKING CHANGE: invalidate_introspection_cache() is now synchronous ## Type Safety Fixes - Change event_bus type from object|None to ProtocolEventBus|None - Add performance_metrics field to IntrospectionCacheDict - Improve MockEventBus type safety (envelope: BaseModel) - Remove unused Any imports ## Test Fixes - Fix broken import path for topic constants - Update error message expectation for topic ending with dot - Move UUID imports to module level (4 test files) - Fix race condition in vault concurrency test mock ## Documentation Updates - Add comprehensive Topic Migration section (3-phase strategy) - Add Security and Access Control section (ACL policies) - Add Performance Metrics and Thresholds section - Fix ModelIntrospectionConfig path reference ## CHANGELOG Updates - Add Breaking Changes section with migration examples - Document cache invalidation API change - Add grep command to find affected code ## New Test Coverage (30 tests) - TestTopicVersionSuffixValidation (7 tests) - TestTopicInvalidNamesValidation (6 tests) - TestCustomTopicParameters (7 tests) - TestIntrospectionCacheThreadSafety (3 tests) - TestCacheHitPerformanceRobust (4 tests) - TestHeartbeatEventCounting (3 tests) ## New Features - Add topic validation with version suffix pattern (.v\d+) - Add configurable topic fields to ModelIntrospectionConfig - Create backward-compatible re-export module
PR Review: Node Introspection with Configurable Topics [OMN-881]Overall AssessmentThis PR successfully migrates from hardcoded Kafka topic enums to contract-driven topic configuration, enabling nodes to declare their pub/sub topics in ✅ Strengths
Code Quality Observations✅ ONEX Compliance
|
…ss [OMN-881] Addresses all PR #54 review feedback: CI Fix: - Fix ruff formatting in test_model_topic_parser.py Test Improvements: - Add 5 version suffix validation tests (.v0, .va, .v1a, missing dot, leading zero) - Make topic-format error assertions robust (check type, not exact message) - Make heartbeat tests CI-robust with polling instead of fixed sleep - Remove unused timing constants and Any types from integration tests - Replace dict[str, Any] with dict[str, object] for ONEX compliance Documentation: - Add comprehensive legacy topic migration guide (341 lines) - Document 4-phase migration process with rollback procedures - Add environment-based staged rollout configuration - Update CHANGELOG with accurate ModelIntrospectionConfig fields Code Quality: - Fix event_bus type annotation in ModelIntrospectionConfig
- Resolved TYPE_CHECKING comment conflict in timeout_emitter.py - Updated INFRA_MAX_UNIONS from 586 to 588 for OMN-811 RegistryCompute merge
PR Review: Contract-Driven Topic Configuration (OMN-881)SummaryThis PR implements a major architectural shift from hardcoded Kafka topics to contract-driven configuration. The implementation is excellent overall with strong adherence to ONEX principles. The PR demonstrates thorough testing (156 tests), comprehensive documentation, and careful attention to backwards compatibility. ✅ Strengths1. Architecture & Design Excellence
2. Documentation Quality
3. Testing Excellence
4. Type Safety
5. Breaking Changes Management
🔍 Code Quality ObservationsExcellent Patterns
|
- Replace `object | None` with `ProtocolEventBus | None` in model_introspection_config.py - Remove unnecessary deprecation warnings for legacy topics (no users yet) - Simplify EVENT_STREAMING_TOPICS.md documentation - Update tests to remove deprecation warning assertions
Code Review: Node Introspection with Configurable TopicsSummaryThis PR successfully migrates from hardcoded ✅ Strengths1. Excellent Documentation
2. Strong Type Safety
3. Comprehensive Testing
4. Breaking Changes Well-Documented
5. ONEX Compliance
🔍 Issues & RecommendationsCRITICAL: Legacy v1_0_0 Directory ViolationIssue: The PR modifies a file in a prohibited versioned directory: CLAUDE.md Policy:
Required Action:
Reference: See MODERATE: Thread Safety DocumentationIssue: The CLAUDE.md addition for thread safety is excellent, but the Current: # In mixin_node_introspection.py docstring
Related:
- Implementation: `src/omnibase_infra/mixins/mixin_node_introspection.py`
- Thread Safety Pattern: `docs/architecture/CIRCUIT_BREAKER_THREAD_SAFETY.md` (similar pattern)
- Ticket: OMN-893Recommendation: **Thread Safety**:
WARNING: This mixin is designed for single-threaded asyncio usage.
For multi-threaded environments, external synchronization is required.
See CLAUDE.md "Node Introspection Security Considerations" for details.Rationale: The thread safety implications are significant enough to warrant a warning-level callout, not just a "See Also" reference. MODERATE: Performance Metrics Threshold ValidationIssue: Performance thresholds are documented but not validated in tests. Current: # Constants defined
PERF_THRESHOLD_GET_CAPABILITIES_MS = 50.0
PERF_THRESHOLD_DISCOVER_CAPABILITIES_MS = 30.0
PERF_THRESHOLD_GET_INTROSPECTION_DATA_MS = 50.0
PERF_THRESHOLD_CACHE_HIT_MS = 1.0
# Logged when exceeded
if metrics.threshold_exceeded:
logger.warning("Introspection exceeded performance threshold", ...)Recommendation: async def test_performance_threshold_detection():
"""Verify performance metrics correctly identify threshold violations."""
# Mock slow operation
with patch('time.perf_counter', side_effect=[0, 0.060]): # 60ms > 50ms threshold
metrics = await node.get_introspection_data()
perf = node.get_performance_metrics()
assert perf.threshold_exceeded is True
assert 'total_introspection' in perf.slow_operationsRationale: Ensures threshold detection logic works correctly as thresholds evolve. MINOR: Topic Validation Error MessagesIssue: Topic validation error messages could be more actionable. Current: if not VERSION_SUFFIX_PATTERN.search(v):
raise ValueError(
f"ONEX topic must have version suffix (.v1, .v2, etc.): {v}"
)Recommendation: if not VERSION_SUFFIX_PATTERN.search(v):
suggestion = f"{v}.v1" if not v.endswith('.') else f"{v}v1"
raise ValueError(
f"ONEX topic must have version suffix (.v1, .v2, etc.): {v}\n"
f"Suggested: {suggestion}"
)Rationale: Helps developers fix validation errors faster. MINOR: Missing Nil UUID DocumentationIssue: The nil UUID fallback logic is used but not explained in user-facing docs. Current (in code): if node_id_uuid is None:
logger.warning(
"Node ID not initialized, using nil UUID - "
"ensure initialize_introspection() was called correctly",
extra={"operation": "get_introspection_data"},
)
# Use nil UUID (all zeros) as sentinel for uninitialized node
node_id_uuid = UUID("00000000-0000-0000-0000-000000000000")Recommendation: Note:
If `initialize_introspection()` is not called before introspection
operations, a nil UUID (all zeros) will be used as a sentinel value
and a warning will be logged. Always call `initialize_introspection()`
during node initialization.Rationale: Explains the nil UUID to users who might encounter it in logs. 🔒 Security ReviewPASS: Excellent Security Considerations
PASS: No Credential Leakage
PASS: Graceful Degradation
📊 Test Coverage AssessmentCoverage Summary:
Test Quality:
🚀 Performance ConsiderationsPASS: Performance Design
Recommendation:Consider adding a performance regression test that validates introspection completes within the 50ms threshold for a typical node: async def test_introspection_performance_baseline():
"""Ensure introspection completes within 50ms threshold."""
# Create typical node with 10 methods
node = create_test_node_with_methods(method_count=10)
start = time.perf_counter()
await node.get_introspection_data()
elapsed_ms = (time.perf_counter() - start) * 1000
assert elapsed_ms < 50.0, f"Introspection took {elapsed_ms:.2f}ms (threshold: 50ms)"📝 Documentation QualityEXCELLENT:
Recommendation:Add a migration example to the CHANGELOG showing before/after usage: **Migration Example**:
```python
# BEFORE (synchronous cache invalidation - no await)
node.invalidate_introspection_cache()
# AFTER (same - no await needed)
node.invalidate_introspection_cache() # Still synchronous! |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
docs/architecture/EVENT_STREAMING_TOPICS.md (3)
500-523: Unify the documented import path forModelIntrospectionConfig.This doc shows
ModelIntrospectionConfigimported from bothomnibase_infra.models.discoveryandomnibase_infra.mixins. If there is a single canonical public import (e.g., discovery models with mixins as a thin re-export), it would help to standardize on that in all examples or explicitly call out which is preferred vs. backward-compatible.Right now the mixed imports are slightly confusing for readers trying to follow the recommended API surface.
Also applies to: 747-763
827-837: Clarify the migration note referencing “Section 9”.The note says migration to canonical ONEX topics “is tracked in Section 9”, but Section 9 now describes topic formats rather than an explicit migration plan. Either update the cross-reference to the correct section (if a migration plan lives elsewhere) or reword this sentence to avoid implying a dedicated migration section.
This will keep the locked spec self-consistent for future readers.
70-83: Optionally call out the concrete envelope type used in code.The “Canonical Event Envelope” section is clear, but it describes the shape generically. If the actual runtime consistently uses a named envelope type (e.g.,
OnexEnvelopeV1in other ONEX services), consider briefly naming that here so teams can connect the spec to concrete models and reuse tooling across repos.Not required for correctness, but it would tighten the link between docs and implementation across the ONEX ecosystem.
Based on learnings, this would align with existing Kafka envelope guidance in related repositories.
tests/unit/models/dispatch/test_model_topic_parser.py (2)
718-729: Align uppercase-V1test name/docstring with actual behavior.
test_version_suffix_uppercase_v_invalidand its docstring describe.V1as “invalid”, but the implementation only checks that, if parsed as Environment-Aware, the version is normalized to"v1". That’s not actually asserting invalidity.Either:
- rename the test and adjust the docstring to describe normalization behavior, or
- tighten the assertions to explicitly require
ENVIRONMENT_AWAREstandard and clarify whether.V1should be accepted or rejected.Right now the intent is ambiguous.
1446-1453: Be aware that tests pin internaldomain == ""semantics for malformed topics.For inputs like
"onex..events"and"dev..events.v1", these tests assertresult.domain == "". That’s fine if the parser is intentionally exposing an empty-string domain for structural debugging, but it does couple the tests to a specific internal representation of “missing domain”.If you later refactor the parser to use
None(or omit domain entirely) for such cases, these tests will need updating. Consider adding a brief comment in the parser code or here to document that""is the chosen sentinel for “no domain” in malformed topics.Also applies to: 1593-1605
tests/unit/mixins/test_mixin_node_introspection.py (2)
3170-3362: Cache concurrency tests validate safety under async loadThe new
TestIntrospectionCacheThreadSafetyandTestCacheHitPerformanceRobustsuites exercise:
- Many concurrent readers on a warm cache.
- Interleaved cache invalidation with reads.
- TTL‑driven refresh under load and consistency of
node_id.- Cache‑hit behavior via timestamps and metrics rather than wall‑clock timing.
They don’t enforce strict single‑writer semantics (e.g., invalidation vs. in‑flight refresh winning), but they do confirm there are no race‑induced errors and that observable state remains consistent, which is appropriate for this cache design.
3364-3462: Heartbeat counting tests give deterministic coverage for event topicsThese heartbeat tests now assert:
- At least one heartbeat is published under a fast interval.
- No further events after stopping tasks.
- Consistent
node_idacross all heartbeat envelopes.They rely on literal
"node.heartbeat"rather than the exported default heartbeat constant; consider switching toDEFAULT_HEARTBEAT_TOPIC(once that’s the canonical value everywhere) to avoid any drift between tests and config defaults in future migrations, but current behavior is correct.src/omnibase_infra/mixins/mixin_node_introspection.py (2)
176-182: Performance metrics embedding and cache typing are coherentImporting
ModelIntrospectionPerformanceMetrics, introducingPerformanceMetricsCacheDict, extendingIntrospectionCacheDictwith an optionalperformance_metricsfield, and wiring_to_pydantic_metricsintoget_introspection_datagive you:
- Strongly‑typed metrics on the event payload.
- A JSON‑shape cache structure that matches
model_dump(mode="json")for both the introspection event and its metrics.The cache‑hit path correctly recomputes and stores
_introspection_last_metricswhile reusing the cached event. If you ever need method counts (or other metrics) to be meaningful on cache hits as well, you could additionally derivemethod_countfrom the cached capabilities there, but it’s not required by current tests.Also applies to: 276-305, 331-333, 1971-1996, 2044-2044
618-622: Per‑instance topic configuration is wired correctly through publish pathsStoring
config.introspection_topic,config.heartbeat_topic, andconfig.request_introspection_topicinto_introspection_topic,_heartbeat_topic, and_request_introspection_topic, and then using those in:
publish_introspection(for the introspection event),_publish_heartbeat(for heartbeats), and_registry_listener_loop(for subscriptions),properly decouples the mixin from hard‑coded topic constants and aligns with the new topic‑validation/configuration tests. Consider adding explicit
strattribute annotations for the three_..._topicfields alongside the other configuration attributes to make their presence and types clearer to static type checkers.Also applies to: 663-666, 669-686, 1330-1336, 1430-1432, 1743-1761
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (23)
CHANGELOG.mdCLAUDE.mddocs/architecture/EVENT_STREAMING_TOPICS.mddocs/patterns/README.mdsrc/omnibase_infra/mixins/__init__.pysrc/omnibase_infra/mixins/mixin_node_introspection.pysrc/omnibase_infra/mixins/model_introspection_config.pysrc/omnibase_infra/models/discovery/__init__.pysrc/omnibase_infra/models/discovery/model_introspection_config.pysrc/omnibase_infra/services/timeout_emitter.pysrc/omnibase_infra/validation/infra_validators.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.pytests/integration/nodes/test_registration_orchestrator_integration.pytests/integration/timeouts/conftest.pytests/unit/event_bus/test_kafka_event_bus.pytests/unit/handlers/test_handler_vault_concurrency.pytests/unit/mixins/test_mixin_node_introspection.pytests/unit/models/dispatch/test_model_topic_parser.pytests/unit/models/projection/test_model_snapshot_topic_config.pytests/unit/nodes/reducers/test_reducer_purity.pytests/unit/nodes/test_node_registration_orchestrator.pytests/unit/runtime/test_validation.pytests/unit/validation/test_topic_category_validator.py
✅ Files skipped from review due to trivial changes (1)
- src/omnibase_infra/services/timeout_emitter.py
🚧 Files skipped from review as they are similar to previous changes (4)
- CLAUDE.md
- tests/unit/handlers/test_handler_vault_concurrency.py
- tests/unit/event_bus/test_kafka_event_bus.py
- src/omnibase_infra/models/discovery/init.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:
tests/unit/nodes/test_node_registration_orchestrator.pytests/unit/validation/test_topic_category_validator.pytests/unit/runtime/test_validation.pysrc/omnibase_infra/models/discovery/model_introspection_config.pysrc/omnibase_infra/validation/infra_validators.pytests/unit/nodes/reducers/test_reducer_purity.pytests/unit/models/dispatch/test_model_topic_parser.pytests/unit/mixins/test_mixin_node_introspection.pytests/integration/nodes/test_registration_orchestrator_integration.pysrc/omnibase_infra/mixins/__init__.pysrc/omnibase_infra/mixins/model_introspection_config.pytests/unit/models/projection/test_model_snapshot_topic_config.pysrc/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.pytests/integration/timeouts/conftest.py
**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/model_*.py: One model per file - Each file contains exactly oneModel*class
Usemodel_<name>.pyfile naming pattern withModel<Name>class pattern for Pydantic models. Example:model_kafka_message.py→ModelKafkaMessage
Files:
src/omnibase_infra/models/discovery/model_introspection_config.pysrc/omnibase_infra/mixins/model_introspection_config.py
**/*infra*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*infra*.py: IncludeModelInfraErrorContextwith every infrastructure error. Context should include:transport_type(EnumInfraTransportType),operation,target_name, andcorrelation_id. Example:raise InfraConnectionError('Failed to connect', context=context)
UseEnumInfraTransportTypefor transport identification in error context. Values include: HTTP, DATABASE, KAFKA, CONSUL, VAULT, VALKEY, GRPC
Files:
src/omnibase_infra/validation/infra_validators.py
**/mixin_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
mixin_<name>.pyfile naming pattern withMixin<Name>class pattern. Example:mixin_health_check.py→MixinHealthCheck
Files:
src/omnibase_infra/mixins/mixin_node_introspection.py
🧠 Learnings (61)
📓 Common learnings
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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/pr.mdc:0-0
Timestamp: 2025-11-24T16:33:09.011Z
Learning: Applies to docs_private/dev_logs/jonah/pr/pr_description_*.md : Each PR description must include the following required sections: PR Title, Branch, PR ID or Link, Summary of Changes, Key Achievements, Prompts & Actions (Chronological with timestamps in ISO 8601 format and agent attribution), Major Milestones, Blockers / Next Steps, Metrics (Lines Changed in "+X / -Y" format, Files Modified count, Time Spent if tracked), and must include optional sections where relevant: Related Issues/Tickets, Breaking Changes, Migration/Upgrade Notes, Documentation Impact, Test Coverage, Security/Compliance Notes, Reviewer(s), and Release Notes Snippet
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Implement Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-24T17:28:15.619Z
Learning: KafkaEventBus intentionally violates pattern validator thresholds: 14 methods (threshold: 10) for lifecycle/pub-sub/circuit breaker requirements and 10 __init__ parameters (threshold: 5) for backwards compatibility during config migration. This complexity is acceptable and documented.
📚 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/test_node_registration_orchestrator.pytests/unit/mixins/test_mixin_node_introspection.pytests/integration/nodes/test_registration_orchestrator_integration.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 Kafka event-driven architecture with proper topic naming using prefix dev.archon-intelligence. and proper event flow pattern with Effect nodes consuming events, processing, and publishing results with Dead Letter Queue routing
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Use event-driven architecture with Kafka topics for asynchronous processing: enrichment, code analysis, manifest processing, and entity embedding pipelines.
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Follow canonical patterns from reference implementations: use node_cli/v1_0_0/ as primary reference and node_kafka_event_bus/v1_0_0/ for complex backend patterns
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.md
📚 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/events/**/*.py : Kafka event publishing MUST use OnexEnvelopeV1 format with 13 topics for event streaming at all workflow lifecycle stages
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 **/*.py : Publish intelligence requests to Kafka event bus using topics: dev.archon-intelligence.intelligence.code-analysis-{requested,completed,failed}.v1 for consistency and event-driven architecture
Applied to files:
docs/patterns/README.mddocs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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 agent observability using three-layer traceability with correlation_id tracking through agent_routing_decisions, agent_manifest_injections, and agent_execution_logs tables
Applied to files:
docs/patterns/README.md
📚 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:
src/omnibase_infra/models/discovery/model_introspection_config.pyCHANGELOG.mdtests/integration/mixins/test_mixin_node_introspection_contract_integration.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: 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:
src/omnibase_infra/models/discovery/model_introspection_config.pydocs/architecture/EVENT_STREAMING_TOPICS.mdCHANGELOG.md
📚 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 : Implement Node classes by inheriting from `NodeBase` with proper UUID and `ModelSemVer` fields
Applied to files:
tests/unit/nodes/reducers/test_reducer_purity.pytests/unit/mixins/test_mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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: All ONEX node deviations from canonical patterns must be documented and justified in the node's root-level README.md and subject to maintainer review
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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]*/SCHEMA_DECISIONS.md : Each versioned ONEX node implementation directory must include a `SCHEMA_DECISIONS.md` file documenting schema-specific design decisions, implementation notes, and validation strategies
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 schemas must use the canonical base state inheritance pattern with input_state containing only node-specific fields (inheriting from OnexInputState) and output_state containing only node-specific fields (inheriting from OnexOutputState)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 **/*contract*.yaml : All ONEX nodes must have validated YAML contracts following the contract-driven development pattern with input_state and output_state schema definitions
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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]/contract.yaml : All contract.yaml files must include linked document architecture with associated_documents section referencing node_config.yaml and deployment_config.yaml
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/contract.yaml : Main contract files must be named `contract.yaml` and serve as the interface definition (source of truth) for the node
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 reference shared schemas using project root paths (e.g., 'schemas/...' or 'omnibase/schemas/...') rather than relative paths
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 support optional documents pattern with optional flag and required_capability field for future extensibility
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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/architecture/EVENT_STREAMING_TOPICS.mdCHANGELOG.mdtests/integration/mixins/test_mixin_node_introspection_contract_integration.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 : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.mdtests/unit/mixins/test_mixin_node_introspection.pysrc/omnibase_infra/mixins/__init__.pysrc/omnibase_infra/mixins/model_introspection_config.pyCHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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: All ONEX nodes must use the node_kafka_event_bus as a secondary reference only for complex backend and event bus logic and advanced configuration patterns
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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: Deviations from omnibase_core standards are only acceptable for: (1) Orchestrator/Reducer nodes (ModelService* disabled), (2) Experimental features being prototyped for upstream, (3) Performance-critical optimizations with benchmark proof, (4) Bridge-specific unique patterns. All deviations require explicit documentation and justification.
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.mdCHANGELOG.md
📚 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: All ONEX nodes must conform to the 4-Node Architecture pattern with clear separation of concerns and unidirectional data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR)
Applied to files:
docs/architecture/EVENT_STREAMING_TOPICS.md
📚 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 tests/**/*.py : Write comprehensive test coverage following the test structure under `tests/unit/` organized by subsystem (enums, models, mixins, utils)
Applied to files:
tests/unit/models/dispatch/test_model_topic_parser.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 scripts/tests/**/*.sh : Implement comprehensive test suites in scripts/tests/ with separate test files for Kafka, PostgreSQL, Intelligence, and Routing functionality
Applied to files:
tests/unit/models/dispatch/test_model_topic_parser.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/unit/models/**/test_model_*.py : Model tests must achieve 100% coverage and test instantiation, inheritance, serialization, deserialization, JSON serialization, roundtrip serialization, equality, hashing, string representation, repr, attributes, validation, metadata, data creation, copying, and immutability
Applied to files:
tests/unit/models/dispatch/test_model_topic_parser.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 nodes/**/*.py : Use event bus mixins from `omnibase_core` for Kafka publishing instead of direct Kafka clients
Applied to files:
tests/unit/mixins/test_mixin_node_introspection.pyCHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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 : Use correlation_id UUID for end-to-end traceability across all agent routing, manifest injection, and execution events
Applied to files:
tests/integration/nodes/test_registration_orchestrator_integration.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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 : Always propagate `correlation_id` from incoming requests to error context. Auto-generate using `uuid4()` if no correlation_id exists. Use UUID format for all new correlation IDs.
Applied to files:
tests/integration/nodes/test_registration_orchestrator_integration.py
📚 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/**/*.py : SPI modules may import from `omnibase_core` for type hints and model runtime usage (allowed and required)
Applied to files:
src/omnibase_infra/mixins/model_introspection_config.pysrc/omnibase_infra/mixins/mixin_node_introspection.py
📚 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: Import `omnibase_core` models and types only for type hints and runtime usage - follow the SPI → Core dependency direction
Applied to files:
src/omnibase_infra/mixins/model_introspection_config.pysrc/omnibase_infra/mixins/mixin_node_introspection.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: Node implementations must use mixin-based composition from `omnibase_core.mixins` (e.g., `MixinHealthCheck`, `MixinNodeExecutor`) to add capabilities
Applied to files:
CHANGELOG.md
📚 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:
CHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.pytests/integration/mixins/test_mixin_node_introspection_contract_integration.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:
CHANGELOG.mdsrc/omnibase_infra/mixins/mixin_node_introspection.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:
CHANGELOG.mdtests/integration/mixins/test_mixin_node_introspection_contract_integration.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 nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
CHANGELOG.md
📚 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:
CHANGELOG.md
📚 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 **/node.py : Prefix internal/sensitive methods with `_` to exclude them from introspection. Node introspection uses reflection to discover public methods - private methods are hidden from exposure.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T17:24:41.687Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T17:24:41.687Z
Learning: Applies to **/protocols/protocol_*.py : Avoid using Any, dict, or primitive types in protocol signatures; use the strongest typing possible with Pydantic models
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/{models,protocols}/{model_*,protocol_*}.py : Avoid using Any, dict, or primitive types in model and protocol definitions; use strongest typing possible
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/protocols/protocol_*.py : Use Protocol from typing module for all interface definitions; never use ABC (Abstract Base Classes) for service interfaces
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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 **/*dispatcher*.py : Use `ModelEventEnvelope[object]` instead of `Any` for generic dispatchers that accept any payload type. Use specific types like `ModelEventEnvelope[UserCreatedEvent]` when the payload type is known.
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 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/**/*.py : Protocols must inherit from `typing.Protocol` and use `...` (ellipsis) for method bodies
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.py
📚 Learning: 2025-11-24T16:33:32.747Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/standards.mdc:0-0
Timestamp: 2025-11-24T16:33:32.747Z
Learning: Applies to **/protocols/protocol_*.py : Use TYPE_CHECKING guards and forward references for circular import prevention in protocol files
Applied to files:
src/omnibase_infra/mixins/mixin_node_introspection.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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/unit/mixins/test_mixin_*.py : Mixin tests must be organized in test classes and test mixin initialization, inheritance, and core mixin functionality
Applied to files:
tests/integration/mixins/test_mixin_node_introspection_contract_integration.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/integration/mixins/test_mixin_node_introspection_contract_integration.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 files must follow the naming convention `test_[module_name].py` (examples: `test_enum_acknowledgment_type.py`, `test_model_node_status.py`, `test_mixin_hash_computation.py`)
Applied to files:
tests/integration/mixins/test_mixin_node_introspection_contract_integration.py
📚 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 `UUID` instead of `str` for ID fields in models
Applied to files:
tests/integration/mixins/test_mixin_node_introspection_contract_integration.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 **/nodes/*/v[0-9]_[0-9]_[0-9]/node.py : Node classes must follow canonical reducer pattern with dependency injection: accept logger_tool and registry in constructor, validate they are not None
Applied to files:
tests/integration/mixins/test_mixin_node_introspection_contract_integration.py
🧬 Code graph analysis (8)
tests/unit/validation/test_topic_category_validator.py (1)
src/omnibase_infra/errors/error_chain_propagation.py (1)
violations(114-120)
tests/unit/models/dispatch/test_model_topic_parser.py (5)
src/omnibase_infra/models/dispatch/model_topic_parser.py (3)
ModelTopicParser(331-733)parse(439-483)validate_topic(594-645)src/omnibase_infra/enums/enum_topic_standard.py (1)
EnumTopicStandard(13-42)src/omnibase_infra/runtime/dispatcher_registry.py (1)
category(210-229)tests/unit/runtime/test_dispatcher_registry.py (1)
category(58-59)src/omnibase_infra/enums/enum_message_category.py (1)
EnumMessageCategory(34-196)
tests/unit/mixins/test_mixin_node_introspection.py (2)
src/omnibase_infra/models/discovery/model_introspection_config.py (1)
ModelIntrospectionConfig(49-255)tests/unit/runtime/test_runtime_host_process.py (1)
MockEventBus(151-238)
src/omnibase_infra/mixins/__init__.py (1)
src/omnibase_infra/mixins/mixin_node_introspection.py (1)
PerformanceMetricsCacheDict(276-304)
src/omnibase_infra/mixins/model_introspection_config.py (1)
src/omnibase_infra/models/discovery/model_introspection_config.py (1)
ModelIntrospectionConfig(49-255)
tests/unit/models/projection/test_model_snapshot_topic_config.py (1)
src/omnibase_infra/errors/infra_errors.py (1)
ProtocolConfigurationError(103-138)
tests/integration/mixins/test_mixin_node_introspection_contract_integration.py (3)
src/omnibase_infra/mixins/mixin_node_introspection.py (1)
MixinNodeIntrospection(335-2032)src/omnibase_infra/models/discovery/model_introspection_config.py (1)
ModelIntrospectionConfig(49-255)src/omnibase_infra/models/discovery/model_node_introspection_event.py (1)
ModelNodeIntrospectionEvent(61-266)
tests/integration/timeouts/conftest.py (3)
tests/unit/nodes/conftest.py (1)
published_events(184-196)tests/unit/mixins/test_mixin_node_introspection.py (2)
publish_envelope(94-113)publish_envelope(898-899)tests/integration/mixins/test_mixin_node_introspection_contract_integration.py (1)
publish_envelope(84-97)
🔇 Additional comments (25)
tests/unit/nodes/test_node_registration_orchestrator.py (1)
15-18: Module-leveluuid4import is correct and simplifies tests.Centralizing
uuid4import at the top avoids repeated local imports and has no behavioral impact.tests/unit/validation/test_topic_category_validator.py (1)
294-307: Looser warning-message assertion is appropriate here.Checking for key terms instead of an exact phrase keeps the test resilient to minor wording changes while still validating that a naming-convention warning is emitted.
tests/unit/models/projection/test_model_snapshot_topic_config.py (1)
78-101: Good alignment withProtocolConfigurationErrorand flexible message checks.Asserting the concrete error type plus presence of “compact”/“cleanup” keeps these tests coupled to semantics (cleanup policy must be compact-only) without overfitting to exact phrasing.
tests/unit/runtime/test_validation.py (1)
199-202: Relaxed error-string checks are well-scoped and still verify semantics.These assertions now key off field names and constraint phrases instead of full messages, which reduces brittleness while still guaranteeing:
- the correct field is being validated, and
- the intended constraint (non-negative / positive / within min–max / dict-like object) is enforced.
No issues from a validation-contract perspective.
Also applies to: 215-217, 238-240, 278-284, 293-299, 313-315
docs/patterns/README.md (1)
14-17: New Observability link looks good.Linking to
EVENT_STREAMING_TOPICS.mdfrom the Observability section is accurate and keeps the patterns index in sync with the new architecture doc.tests/integration/nodes/test_registration_orchestrator_integration.py (1)
35-35: LGTM! Import consolidation improves code organization.Moving
uuid4to module-level imports eliminates redundant imports within fixtures and follows Python best practices for import organization.src/omnibase_infra/validation/infra_validators.py (1)
371-375: LGTM! Threshold update is properly documented.The union count threshold increase from 586 to 588 is justified with a clear ticket reference (OMN-811) and explanation (+2 unions from RegistryCompute merge). The threshold history provides good audit trail for future maintainers.
tests/unit/nodes/reducers/test_reducer_purity.py (1)
21-21: LGTM! Module-level import follows best practices.Consolidating
uuid4at module level eliminates redundant imports within test functions and improves code organization.src/omnibase_infra/mixins/__init__.py (1)
22-22: LGTM! Public API exposure is appropriate.Adding
PerformanceMetricsCacheDictto the module exports provides proper access to the typed performance metrics cache structure for external consumers.Also applies to: 35-35
src/omnibase_infra/models/discovery/model_introspection_config.py (4)
34-46: LGTM! Topic validation constants are well-defined.The topic validation constants provide clear rules for ONEX and legacy topic naming:
- Default topics use legacy "node." prefix for backward compatibility
TOPIC_PATTERNcorrectly enforces lowercase-start and valid character constraintsVERSION_SUFFIX_PATTERNproperly validates.v[0-9]+format for ONEX topicsINVALID_TOPIC_CHARSset provides clear character exclusions
175-221: LGTM! Topic validation logic is comprehensive and defensive.The
validate_topic_namefield validator implements thorough validation with excellent error messages:
- Empty check (lines 191-192): Guards against empty strings
- Invalid characters (lines 195-197): Clear set-based detection
- Pattern validation (lines 200-211): Specific error messages for uppercase start, trailing dots, and invalid characters
- ONEX suffix enforcement (lines 214-218): Strict version suffix requirement for ONEX topics
- Legacy allowance (line 219): Explicit documentation of legacy topic support
The validation strikes a good balance between strictness for new ONEX topics and flexibility for legacy topics.
124-132: Duck-typed event_bus is acceptable for protocol compliance.The
event_busfield usesobject | Noneto support arbitrary types while maintaining protocol compliance:
TYPE_CHECKINGimport ofProtocolEventBusprovides type hints for static analysis- Runtime duck typing is performed by
MixinNodeIntrospectionat initializationarbitrary_types_allowed=Trueinmodel_configexplicitly permits this patternThis approach is appropriate for protocol-based duck typing despite the general guideline to avoid
Any. Theobjecttype is more specific thanAnyand the runtime validation ensures protocol compliance.
258-266: LGTM! Public API exports are complete.The
__all__list properly exposes all new topic-related constants and patterns alongsideModelIntrospectionConfig, making them available for external import and validation testing.src/omnibase_infra/mixins/model_introspection_config.py (1)
1-52: LGTM! Backward-compatible re-export module follows best practices.This re-export module properly maintains backward compatibility while guiding users toward the canonical import location:
- Clear documentation (lines 3-32): Explains purpose, provides examples of both old and new import patterns
- Complete re-exports (lines 34-42): All related constants and the main model class
- Proper
__all__(lines 44-52): Explicit public API definitionThis approach allows gradual migration while preventing breaking changes for existing code importing from
omnibase_infra.mixins.model_introspection_config.tests/unit/mixins/test_mixin_node_introspection.py (4)
44-45: MockEventBus now correctly models envelopes as Pydantic modelsNarrowing
publish_envelope’senvelopeparameter toBaseModelin both the import and signature matches how events are actually modeled and keeps the mock consistent with other event‑bus tests; no issues spotted.Also applies to: 94-104
827-831: Heartbeat periodicity test made more CI‑robustSwitching from a fixed sleep to a bounded polling loop with a lower minimum‑event threshold is a good trade‑off against CI flakiness while still asserting that periodic heartbeats are happening.
Also applies to: 843-865
918-922: Graceful‑degradation heartbeat test is resilient to timing varianceThe polling‑based approach for verifying the heartbeat task stays running under publish failures avoids brittle timing assumptions and still exercises the failure‑tolerance behavior effectively.
Also applies to: 934-959
2885-3067: Topic validation tests align with ModelIntrospectionConfig semanticsThese tests comprehensively cover:
- ONEX topics requiring
.v\d+suffix (valid/invalid patterns, multi‑digit versions).- Legacy topics (non‑
onex.) being allowed with or without version suffix.- Character‑class constraints (invalid symbols, whitespace), casing, and trailing‑dot rejection.
The expected error substrings match the validator’s messages from
validate_topic_name, so this suite should give solid regression coverage for future topic‑spec changes.tests/integration/timeouts/conftest.py (1)
41-50: Event‑bus test fixture now cleanly separates structured and raw eventsThe TYPE_CHECKING‑guarded
BaseModelimport,RawEventDictTypedDict, and the expansion ofMockEventBusto track:
- structured
(topic, BaseModel)envelopes, and- raw
(topic, RawEventDict)byte events,together with
get_events_for_topic,get_raw_events_for_topic,count_events,count_all_events, andclear, form a coherent, type‑safe fixture API for timeout tests. The design cleanly supports both envelope and raw‑Kafka paths without introducing behavioral regressions.Also applies to: 59-69, 84-92, 94-125, 126-188
tests/integration/mixins/test_mixin_node_introspection_contract_integration.py (6)
60-144: LGTM: Mock fixtures are well-designed.The mock event bus and type definitions are well-structured:
PublishedEventRecordTypedDict provides clear typing for test assertionsMockEventBusproperly implements the protocol methods needed for testing- Type annotations use specific types (
BaseModel, proper unions) withoutAny- Async subscribe/unsubscribe pattern correctly mimics the real event bus
271-390: LGTM: Contract configuration tests are thorough.The tests properly validate:
- Topic extraction from contract event_channels
- Domain-specific topic configuration
- Fallback to defaults for missing topics
- UUID generation for node_id
Test coverage is comprehensive and assertions are appropriate.
397-603: LGTM: End-to-end workflow tests are robust.The tests demonstrate good practices:
- Polling with retry instead of fixed sleep for heartbeat verification (lines 531-545) improves CI reliability
- Proper async task lifecycle with try/finally cleanup (lines 530-559, 590-602)
- Verification of UUID-based node_id throughout
- Comprehensive assertions on published envelopes and subscription behavior
610-833: LGTM: Multi-domain and edge case tests are comprehensive.The tests properly cover:
- Domain isolation with separate event buses
- Contracts with multiple publish/subscribe channels
- Edge cases (empty channels, missing keys)
- Cache invalidation workflow (synchronous method - correct per PR summary)
- Correlation ID propagation through workflow
Test assertions are thorough and appropriate.
840-904: LGTM: Performance test uses robust timing assertions.The test properly handles CI timing variability:
- Allows cache hits to be faster OR both operations to be very fast (< 1ms)
- Comprehensive comment explains the rationale (lines 881-891)
- Descriptive assertion message helps debugging if the test fails
- Avoids flakiness while catching real regressions
911-963: LGTM: Topic validation tests are correct.The tests properly validate:
- Acceptance of valid ONEX topic formats (including hyphens, underscores, numeric segments)
- Rejection of uppercase starting characters
- Rejection of invalid characters (@ symbol triggers "invalid characters" error)
- Rejection of trailing dots (matches updated error message from past review)
Error message expectations align with validation logic.
| #### Node Introspection (OMN-881, PR #54) | ||
| - **ModelIntrospectionConfig**: Configuration model for `MixinNodeIntrospection` that provides typed configuration | ||
| - `node_id` (required): Unique identifier for this node instance (UUID) | ||
| - `node_type` (required): Type of node (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR). Cannot be empty (min_length=1). | ||
| - `event_bus`: Optional event bus for publishing introspection and heartbeat events. Uses duck typing (`object | None`) to accept any object implementing `ProtocolEventBus` protocol. | ||
| - `version`: Node version string (default: `"1.0.0"`) | ||
| - `cache_ttl`: Cache time-to-live in seconds (default: `300.0`, minimum: `0.0`) | ||
| - `operation_keywords`: Optional set of keywords to identify operation methods. If None, uses `MixinNodeIntrospection.DEFAULT_OPERATION_KEYWORDS`. | ||
| - `exclude_prefixes`: Optional set of prefixes to exclude from capability discovery. If None, uses `MixinNodeIntrospection.DEFAULT_EXCLUDE_PREFIXES`. | ||
| - `introspection_topic`: Topic for publishing introspection events (default: `"node.introspection"`). ONEX topics (starting with `onex.`) require version suffix (e.g., `.v1`). | ||
| - `heartbeat_topic`: Topic for publishing heartbeat events (default: `"node.heartbeat"`). ONEX topics require version suffix. | ||
| - `request_introspection_topic`: Topic for receiving introspection requests (default: `"node.request_introspection"`). ONEX topics require version suffix. | ||
| - Model is frozen and forbids extra fields for immutability and strict validation. |
There was a problem hiding this comment.
Update event_bus field description to match the actual type.
The changelog describes event_bus as using duck typing with object | None, but other PR context indicates it is now explicitly typed as a ProtocolEventBus (breaking change for callers).
To avoid confusion for upgraders, please update this bullet to reflect the real signature (e.g., ProtocolEventBus | None) and drop the object | None wording so the documented API matches the implementation.
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 94 to 106, the entry for ModelIntrospectionConfig
incorrectly describes the event_bus field as duck-typed "object | None"; update
that bullet to state the actual type signature used by the codebase (e.g.,
"ProtocolEventBus | None") and remove the "object | None" wording so the
changelog reflects the breaking-change typing, keeping the rest of the sentence
about accepting an object implementing the protocol only if you want to clarify
compatibility.
| ```python | ||
| # node.py - Initializing MixinNodeIntrospection with configuration | ||
| from uuid import UUID | ||
|
|
||
| from omnibase_infra.mixins import MixinNodeIntrospection, ModelIntrospectionConfig | ||
| from omnibase_infra.event_bus import KafkaEventBus | ||
|
|
||
| class RegistryEffectNode(MixinNodeIntrospection): | ||
| """Effect node that uses MixinNodeIntrospection for capability discovery.""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| contract: NodeContract, | ||
| event_bus: KafkaEventBus, | ||
| ) -> None: | ||
| # Configure introspection with typed configuration model | ||
| # Topics can be configured via contract.yaml event_channels (implemented in OMN-881). | ||
| # Default topic names in mixin_node_introspection.py are used if not overridden: | ||
| # - INTROSPECTION_TOPIC = "node.introspection" | ||
| # - HEARTBEAT_TOPIC = "node.heartbeat" | ||
| # - REQUEST_INTROSPECTION_TOPIC = "node.request_introspection" | ||
| config = ModelIntrospectionConfig( | ||
| node_id=UUID(contract.metadata.name) if isinstance(contract.metadata.name, str) else contract.metadata.name, | ||
| node_type=contract.metadata.node_type, | ||
| version=contract.metadata.version, | ||
| event_bus=event_bus, | ||
| ) | ||
| self.initialize_introspection_from_config(config) |
There was a problem hiding this comment.
Avoid constructing node_id from contract.metadata.name in example.
Parsing UUID(contract.metadata.name) is likely wrong in real deployments (name is not guaranteed to be a UUID and this would raise at runtime). For the example, prefer a clearly valid/stable identifier source (e.g., a dedicated metadata.node_id field or an externally configured UUID) instead of overloading name.
Consider changing this line to use a proper node identifier and keep the example aligned with how nodes are actually identified in contracts.
🤖 Prompt for AI Agents
In docs/architecture/EVENT_STREAMING_TOPICS.md around lines 842 to 869, the
example constructs node_id by calling UUID(contract.metadata.name) which is
unsafe because name may not be a UUID; change the example to obtain a proper
identifier (e.g., use contract.metadata.node_id if present or accept an explicit
node_id parameter passed into the constructor), and only parse into a UUID when
the source is a dedicated UUID string (with validation/try/except). Update the
ModelIntrospectionConfig instantiation to use that proper node_id source and add
a short comment noting where node ids should come from in real deployments.
…nditions, style [OMN-881] ## Critical Fixes - Fix vault concurrency race condition: Use string markers instead of shared Exception objects to ensure thread-safe response cycling - Update integration tests to use canonical `initialize_introspection()` method instead of backwards-compatibility alias ## Type Safety Improvements - Add conditional `_EventBusType` type alias in ModelIntrospectionConfig: - Static analysis sees `ProtocolEventBus | None` for full type safety - Runtime uses `object | None` for duck typing compatibility - Update INFRA_MAX_UNIONS threshold: 588 → 589 (+1 for new type alias) ## Documentation Fixes - Fix EVENT_STREAMING_TOPICS.md node_id construction example to use `uuid4()` instead of attempting to parse contract.metadata.name as UUID ## Code Style (Nitpicks) - Remove unused CACHE_TTL_WAIT constant from integration tests - Remove redundant inline `from uuid import UUID` imports (already at module level) - Consolidate ModelIntrospectionConfig import from omnibase_infra.models.discovery
PR Review: Contract-Driven Topic Configuration (OMN-881)Overall AssessmentAPPROVE - This is an excellent implementation that successfully migrates from hardcoded enums to contract-driven topic configuration. The PR demonstrates exceptional attention to ONEX principles, comprehensive testing, and thorough documentation. Strengths1. Architecture Alignment
2. Type Safety ExcellenceThe _EventBusType pattern is particularly clever - satisfies ONEX no Any types rule while maintaining Pydantic compatibility. 3. Validation and Error Handling
4. Testing ExcellenceWith 175+ tests, this PR demonstrates outstanding test coverage including unit tests, integration tests, thread safety tests with asyncio barriers, performance tests with CI-aware thresholds, and edge case coverage. 5. Documentation Quality
6. Performance MetricsAdding ModelIntrospectionPerformanceMetrics for observability is excellent. Critical IssuesNONE FOUND - This PR is production-ready. ONEX Compliance ChecklistAll requirements met: No Any types, Pydantic models for all data structures, strong typing throughout, contract-driven configuration, proper error hierarchy usage, file naming conventions followed, no versioned directories, comprehensive documentation, breaking changes documented with migration path, and security considerations addressed. RecommendationMERGE IMMEDIATELY - This PR represents exemplary ONEX development: architecturally sound, comprehensively tested, thoroughly documented, and production-ready. Great work on this PR! 🎉 Reviewed by: Claude (ONEX Infrastructure Code Reviewer) |
1 participant
Summary
Migrate from hardcoded EnumKafkaTopic enum to contract-driven topic configuration, allowing nodes to declare their pub/sub topics in contract.yaml files.
Key Changes:
Architecture Principle
Files Changed
docs/architecture/EVENT_STREAMING_TOPICS.mdsrc/omnibase_infra/mixins/mixin_node_introspection.pysrc/omnibase_infra/nodes/node_registry_effect/v1_0_0/contract.yamltests/unit/mixins/test_mixin_node_introspection.pyTest Plan
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.