docs(nodes): document mutable state as intentional MVP design [OMN-704]

[jonahgabriel]

Summary

• Document that mutable state in nodes (NodeCompute cache, NodeReducer FSM state, NodeOrchestrator workflow state) is an intentional design decision for MVP
• Expand Thread Safety sections in all node docstrings to explain MVP rationale
• Create comprehensive architecture document with production improvement roadmap

Changes

Node Documentation Updates

• NodeCompute (node_compute.py): Expanded Thread Safety section with MVP design decision, mutable state components (_cache, computation_registry, computation_metrics), and production path via protocol injection (OMN-700)
• NodeReducer (node_reducer.py): Expanded Thread Safety section with MVP design decision, mutable FSM state components, and future external state store plans
• NodeOrchestrator (node_orchestrator.py): Expanded Thread Safety section with MVP design decision, mutable workflow state components, and lease-based coordination roadmap

New Architecture Document

• docs/architecture/MUTABLE_STATE_STRATEGY.md (498 lines): Comprehensive documentation including:
  • MVP design decision rationale and trade-offs
  • Current mutable state breakdown by node type
  • Thread safety guidelines with quick reference tables
  • 4-phase production improvement roadmap:
    1. Protocol Injection (✅ Completed - OMN-700)
    2. External State Stores (Planned - v0.5.0+)
    3. Lease-Based Coordination (Planned - v0.6.0+)
    4. Event-Sourced State (Future - v1.0.0+)
  • Migration path guarantees (no breaking changes)

Acceptance Criteria (OMN-704)

• Add documentation explaining mutable state is intentional for MVP
• Document thread-safety limitations
• Create plan for production improvements
• Update architecture documentation

Test Plan

• poetry run mypy passes for all modified node files
• All pre-commit hooks pass
• All pre-push hooks pass (including mypy strict, pyright basic)
• CI tests pass

Related Issues

• Closes OMN-704
• References OMN-700 (Protocol Injection)

Summary by CodeRabbit

Release Notes

• Documentation
  • Introduced architecture documentation detailing state management approach and thread-safety considerations.
  • Updated core component documentation with current thread-safety guarantees, limitations, and mitigation guidance.
  • Documented production roadmap and migration path for state management evolution.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Documentation updates establish MVP design decisions for mutable state in ONEX nodes. A new architecture document outlines thread-safety implications, production roadmap with planned migration phases, and migration strategies. Three node classes' docstrings are updated to reflect intentional mutable state with current limitations and guidance.

Changes

Cohort / File(s)	Summary
New Architecture Documentation docs/architecture/MUTABLE_STATE_STRATEGY.md	Introduces comprehensive guide on MVP mutable state design, including thread-safety guidelines, current state inventory per node type, production roadmap phases (protocol injection → external state stores → lease coordination → event sourcing), and backward-compatible migration strategy with code examples.
Node Docstring Updates src/omnibase_core/nodes/node_compute.py, src/omnibase_core/nodes/node_orchestrator.py, src/omnibase_core/nodes/node_reducer.py	Thread Safety sections rewritten to document intentional MVP mutable state approach, enumerate mutable state components per class, state current limitations, reference production roadmap, and provide mitigation guidance (thread-local instances). All public signatures remain unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• New documentation file: Review for accuracy of design rationale, roadmap phases, and code examples
• Docstring updates: Verify consistency across three node classes in describing mutable state components, limitations, and migration paths; ensure alignment with new architecture document

Poem

🐰 Mutable state, once a worry and a fright,
Now documented clear from MVP height,
With phases planned and migrations smooth,
We'll thread-safe eventually—just wait, you'll see the proof! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: documenting mutable state as an intentional MVP design decision across the node classes, with reference to the tracking issue OMN-704.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch jonah/omn-704-document-mutable-state-in-nodes

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code Review - PR #191: Document Mutable State as Intentional MVP Design

Summary

This PR successfully documents the intentional use of mutable state in ONEX nodes as an explicit MVP design decision. The documentation is comprehensive, well-structured, and provides clear guidance for both current limitations and future improvements.

✅ Strengths

1. Excellent Documentation Quality

• Comprehensive Coverage: The new MUTABLE_STATE_STRATEGY.md document (498 lines) is exceptionally thorough, covering rationale, current state, thread safety, and roadmap
• Clear Structure: Well-organized with logical flow from "Why?" → "Current State" → "Mitigation" → "Future"
• Practical Examples: Includes concrete code examples for thread-local patterns and protocol injection
• Cross-Referenced: Properly links to existing documentation (THREADING.md, node architecture docs)

2. Node Docstring Updates

• Consistent Pattern: All three nodes (NodeCompute, NodeReducer, NodeOrchestrator) updated with identical structure
• MVP Context: Clearly states this is an intentional MVP decision, not an oversight
• Specific Components: Lists exact mutable state components (e.g., _cache, computation_registry, FSM state)
• Production Path: References OMN-700 protocol injection and future roadmap

3. Production-Ready Roadmap

• 4-Phase Plan: Clear progression from Protocol Injection → External State → Leases → Event Sourcing
• Realistic Timeline: Phases mapped to version targets (v0.4.0, v0.5.0+, v0.6.0+, v1.0.0+)
• Backwards Compatibility: Strong commitment to no breaking changes and opt-in features
• Protocol Interfaces: Includes proposed protocol definitions for future implementations

4. Thread Safety Guidance

• Quick Reference Tables: Easy-to-scan tables for thread-safe vs. not-thread-safe components
• Risk Levels: Components tagged with High/Critical risk levels
• Decision Guide: Practical "if this, then that" guidance for developers
• Debug Mode: Documents ONEX_DEBUG_THREAD_SAFETY=1 for runtime validation

📝 Suggestions for Improvement

Minor Issues

1. Documentation Index Missing Entry

The new MUTABLE_STATE_STRATEGY.md document should be added to docs/INDEX.md for discoverability.

Suggested addition to docs/INDEX.md in the Architecture section:

| [**Mutable State Strategy**](architecture/MUTABLE_STATE_STRATEGY.md) | MVP design decision and production roadmap ⭐ **Important** | ✅ Complete |

Place it near the Thread Safety documentation reference.

2. CLAUDE.md Should Reference New Doc

The project's CLAUDE.md has a "Thread Safety" section that should cross-reference the new comprehensive document.

Suggested addition to CLAUDE.md Thread Safety section (around line 815):

**See**: 
- [docs/guides/THREADING.md](docs/guides/THREADING.md) for thread-local instance patterns
- [docs/architecture/MUTABLE_STATE_STRATEGY.md](docs/architecture/MUTABLE_STATE_STRATEGY.md) for MVP design rationale and production roadmap

3. Minor Typo in Migration Path

In MUTABLE_STATE_STRATEGY.md line 465, the migration guide paths use backticks in the table. For consistency with other docs, consider using plain text or links.

Current:

| Phase 2 | `docs/guides/MIGRATING_TO_EXTERNAL_STATE.md` |

Suggested (for consistency):

| Phase 2 | docs/guides/MIGRATING_TO_EXTERNAL_STATE.md (planned) |

Code Quality Notes

✅ No Code Logic Changes

This is a documentation-only PR, which minimizes risk. The only code changes are docstring updates.

✅ Docstring Format Correct

All updated docstrings follow the project's reStructuredText conventions with proper indentation and formatting.

✅ Thread Safety Sections Expanded

The expanded Thread Safety sections in node docstrings are significantly more informative than the previous terse warnings.

Before (NodeReducer):

Thread Safety:
    NodeReducer instances are NOT thread-safe due to mutable FSM state:
    - Current state tracking
    - State transition history
    - Context accumulation
    
    Each thread should have its own NodeReducer instance, or implement
    explicit synchronization. See docs/guides/THREADING.md for patterns.

After:

Thread Safety:
    **MVP Design Decision**: NodeReducer uses mutable FSM state intentionally for
    the MVP phase to enable stateful workflow processing with minimal complexity.
    This is a documented trade-off.
    
    **Mutable State Components**:
    - fsm_contract: Loaded FSM subcontract reference
    - FSM execution state (via MixinFSMExecution):
      - Current state tracking
      - State transition history
      - Context accumulation
    
    **Current Limitations**:
    NodeReducer instances are NOT thread-safe. Concurrent access will corrupt
    FSM state.
    
    **Production Path**: Future versions will support stateless FSM execution
    with external state stores. See docs/architecture/MUTABLE_STATE_STRATEGY.md
    for the production improvement roadmap.
    
    **Mitigation**: Each thread should have its own NodeReducer instance.
    See docs/guides/THREADING.md for thread-local instance patterns.

The new version is much more informative and educational.

🔒 Security Considerations

✅ No security concerns - This is documentation-only.

🧪 Test Coverage

✅ Appropriate for documentation PR - No test changes needed since this is pure documentation.

The PR description indicates all pre-commit hooks passed, which validates:

• Docstring format (mypy strict mode)
• Markdown syntax
• Type checking

🚀 Performance Considerations

✅ No performance impact - Documentation and docstring changes only.

📋 Acceptance Criteria Check

All acceptance criteria from OMN-704 met:

• ✅ Add documentation explaining mutable state is intentional for MVP
• ✅ Document thread-safety limitations
• ✅ Create plan for production improvements
• ✅ Update architecture documentation

🎯 Final Recommendation

APPROVE ✅ with minor documentation index updates suggested above.

Why Approve?

1. Addresses Critical Knowledge Gap: Explicitly documents what was previously implicit
2. High-Quality Documentation: Comprehensive, well-structured, with concrete examples
3. Clear Production Path: Provides realistic roadmap without overpromising
4. Minimal Risk: Documentation-only changes with no code logic modifications
5. Consistent Updates: All affected nodes updated uniformly

Post-Merge Actions

After merging, consider:

1. Update docs/INDEX.md to include the new document (can be separate quick PR)
2. Update CLAUDE.md Thread Safety section to reference new doc (can be same PR as Complete SPI Migration and Core Stabilization: LogLevel Standardization + ONEX Architecture #1)
3. Share with team - This is valuable context for all ONEX developers

---

Great work documenting this design decision! This will significantly help developers understand the threading model and plan for production deployments.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

docs/architecture/MUTABLE_STATE_STRATEGY.md (3)

124-131: Thread-local pattern example could cause stale state issues.

The get_reducer function caches the NodeReducer instance but doesn't account for scenarios where the fsm_contract might change. If called with a different fsm_contract after initial creation, the cached instance with the old contract is returned.

Consider documenting this limitation or adjusting the pattern:

 def get_reducer(container, fsm_contract):
     """Get or create thread-local NodeReducer instance."""
-    if not hasattr(thread_local, 'reducer'):
+    if not hasattr(thread_local, 'reducer') or thread_local.reducer.fsm_contract != fsm_contract:
         node = NodeReducer(container)
         node.fsm_contract = fsm_contract
         thread_local.reducer = node
     return thread_local.reducer

Alternatively, add a note that this pattern assumes a single FSM contract per thread.

---

161-168: Same stale state concern applies to orchestrator pattern.

The get_orchestrator function has the same potential issue as get_reducer - if called with a different workflow_definition, the cached instance is returned with the old definition.

For consistency, consider the same fix or add documentation noting this limitation.

---

229-247: Add blank lines before tables for markdown compliance.

Per markdownlint MD058, tables should be surrounded by blank lines for consistent rendering across markdown parsers.

 **Thread-Safe Components**:
+
 | Component | Notes |
 |-----------|-------|
 ...
 
 **NOT Thread-Safe Components**:
+
 | Component | Risk Level | Recommended Mitigation |
 |-----------|------------|------------------------|

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 403ed21 and dc1f5ff.

📒 Files selected for processing (4)

• docs/architecture/MUTABLE_STATE_STRATEGY.md (1 hunks)
• src/omnibase_core/nodes/node_compute.py (1 hunks)
• src/omnibase_core/nodes/node_orchestrator.py (1 hunks)
• src/omnibase_core/nodes/node_reducer.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always use Poetry for Python package management and task execution - use poetry run for all Python commands (pytest, mypy, black, isort, etc.), never use pip or direct python execution
Call super().__init__(container) in all node class constructors - this is MANDATORY and eliminates 80+ lines of boilerplate initialization code
Always raise structured errors using ModelOnexError with appropriate EnumCoreErrorCode, never use generic Exception
Use container.get_service("ProtocolName") for dependency injection - always use protocol names, never concrete class names or isinstance checks
Use @standard_error_handling decorator on methods to eliminate 6+ lines of try/catch boilerplate
Do NOT share node instances across threads - use thread-local instances or separate instances per thread for NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator
Use FileRegistry for loading RuntimeHostContract YAML files - handles fail-fast error reporting with specific error codes (FILE_NOT_FOUND, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, etc.)
Use the correct container type: ModelONEXContainer for dependency injection in node constructors, ModelContainer[T] only for wrapping single values with metadata
Use EnumNodeKind for architectural classification (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR) and EnumNodeType for specific implementation type (TRANSFORMER, AGGREGATOR, GATEWAY, etc.)
Format code using poetry run black and sort imports using poetry run isort before committing
Lint code using poetry run ruff check to detect issues and apply auto-fixes with poetry run ruff check --fix

Files:

• src/omnibase_core/nodes/node_orchestrator.py
• src/omnibase_core/nodes/node_compute.py
• src/omnibase_core/nodes/node_reducer.py

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/omnibase_core/**/*.py: Achieve and maintain 100% mypy strict mode compliance with 0 errors across all source files
Achieve and maintain pyright basic mode compliance with 0 errors (targets Python 3.12)
For immutable value objects that are nested in other Pydantic models or used in parallel test execution, add from_attributes=True to ConfigDict
Use protocol-based dependency injection with container.get_service() - no isinstance checks with protocols (rely on duck typing instead)

Files:

• src/omnibase_core/nodes/node_orchestrator.py
• src/omnibase_core/nodes/node_compute.py
• src/omnibase_core/nodes/node_reducer.py

src/omnibase_core/nodes/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Import nodes from omnibase_core.nodes using the unified import path - use NodeCompute, NodeReducer, NodeOrchestrator, NodeEffect (NOT with 'Declarative' suffix)

Files:

• src/omnibase_core/nodes/node_orchestrator.py
• src/omnibase_core/nodes/node_compute.py
• src/omnibase_core/nodes/node_reducer.py

🧠 Learnings (3)

📚 Learning: 2025-12-13T20:59:19.537Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T20:59:19.537Z
Learning: Use the 4-Node ONEX Architecture pattern (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR) with unidirectional data flow and no backwards dependencies

Applied to files:

• docs/architecture/MUTABLE_STATE_STRATEGY.md

📚 Learning: 2025-12-13T20:59:19.537Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T20:59:19.537Z
Learning: Applies to **/*.py : Do NOT share node instances across threads - use thread-local instances or separate instances per thread for NodeCompute, NodeEffect, NodeReducer, NodeOrchestrator

Applied to files:

• src/omnibase_core/nodes/node_orchestrator.py
• src/omnibase_core/nodes/node_compute.py

📚 Learning: 2025-12-13T20:59:19.537Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-13T20:59:19.537Z
Learning: ModelIntent pattern should be used in REDUCER nodes for pure FSM transitions without direct side effects

Applied to files:

• src/omnibase_core/nodes/node_reducer.py

🪛 LanguageTool

docs/architecture/MUTABLE_STATE_STRATEGY.md

[style] ~387-~387: This phrase is redundant. Consider writing “point” or “time”.
Context: ...e changes - State reconstruction at any point in time - Debugging complex distributed scenari...

(MOMENT_IN_TIME)

🪛 markdownlint-cli2 (0.18.1)

docs/architecture/MUTABLE_STATE_STRATEGY.md

230-230: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)

---

239-239: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)

• GitHub Check: Tests (Split 18/20)
• GitHub Check: Tests (Split 9/20)
• GitHub Check: Tests (Split 13/20)
• GitHub Check: Tests (Split 19/20)
• GitHub Check: Tests (Split 17/20)
• GitHub Check: Tests (Split 20/20)
• GitHub Check: Tests (Split 14/20)
• GitHub Check: Tests (Split 10/20)
• GitHub Check: Tests (Split 12/20)
• GitHub Check: Tests (Split 5/20)
• GitHub Check: Tests (Split 4/20)
• GitHub Check: Tests (Split 15/20)
• GitHub Check: Tests (Split 3/20)
• GitHub Check: Tests (Split 2/20)
• GitHub Check: claude-review

🔇 Additional comments (5)

docs/architecture/MUTABLE_STATE_STRATEGY.md (2)

84-94: Protocol injection example is well-structured.

The example correctly demonstrates registering a thread-safe cache via container and using it with NodeCompute. This aligns with the OMN-700 pattern and coding guidelines for using container.register_service().

---

471-493: All referenced documentation files exist in the repository. No broken links found.

src/omnibase_core/nodes/node_reducer.py (1)

48-69: Thread Safety documentation is comprehensive and accurate.

The updated docstring clearly documents:

• MVP design rationale
• Specific mutable state components (fsm_contract, FSM execution state)
• Current limitations (not thread-safe)
• Production path (external state stores)
• Mitigation strategy (thread-local instances)

This aligns with the coding guidelines and the architecture document. Based on learnings, the thread-local instance recommendation correctly addresses the "Do NOT share node instances across threads" guideline.

src/omnibase_core/nodes/node_compute.py (1)

82-99: Thread Safety documentation correctly captures dual-mode behavior.

The updated docstring accurately describes:

• MVP design rationale for mutable state
• Specific mutable components (_cache, computation_registry, computation_metrics)
• Thread-safety guarantee in pure mode (no mutable state accessed)
• Caveat that injected implementations determine thread safety
• Production path via protocol injection (OMN-700)

The distinction between pure mode (thread-safe) and injected mode (depends on implementation) is valuable for users choosing their deployment model. Based on learnings, this aligns with the coding guidelines.

src/omnibase_core/nodes/node_orchestrator.py (1)

36-57: Thread Safety documentation is thorough and forward-looking.

The updated docstring comprehensively documents:

• MVP design rationale for mutable workflow state
• Specific mutable components (workflow_definition, active executions, step completion, context)
• Current limitations (not thread-safe)
• Production path mentioning lease-based coordination (aligns with Phase 3 in roadmap)
• Mitigation strategy (thread-local instances)

The reference to "lease-based coordination" correctly anticipates Phase 3 of the production roadmap. Based on learnings, the thread-local recommendation aligns with the coding guidelines.