[jonahgabriel]

Summary

Implement execution order resolution as a pure function over contract policy and profiles for OMN-1106.

• Pure function: No side effects, no global state, no I/O
• Deterministic: Same inputs always produce same outputs
• Fail-fast: Conflicts detected at validation, not runtime

New Models

Model	Purpose
ModelExecutionPlan	Output of resolution with ordered handlers per phase
ModelPhaseEntry	Handler entry in an execution phase
ModelResolutionMetadata	Tie-breaker decisions and resolution stats
ModelTieBreakerDecision	Records tie-breaker decisions
ModelConstraintSatisfaction	Records constraint evaluation
ModelExecutionConflict	Describes conflicts (cycles, unsatisfiable)

New Protocol

• ProtocolExecutionResolver: Interface for execution order resolution

Implementation

• Topological sort using Kahn's algorithm
• Cycle detection with detailed cycle path reporting
• Deterministic tie-breaking (priority, then alphabetical)
• Constraint types: handler:X, capability:Y, tag:Z

Test plan

• 34 comprehensive tests covering:
  • Basic ordering (simple, chains, diamond dependencies)
  • Cycle detection (direct, indirect, self-reference)
  • Tie-breaking (priority, alphabetical, combined)
  • Phase assignment
  • Constraint resolution (capability, handler, tag)
  • Determinism verification
  • Protocol compliance
• All tests pass: poetry run pytest tests/unit/resolution/ -v
• mypy strict mode passes
• All pre-commit hooks pass

Related

• Blocked by: OMN-1125 (Profile Factory) ✅, OMN-1117 (Handler Contract) ✅
• Linear: https://linear.app/omninode/issue/OMN-1106

Summary by CodeRabbit

• New Features

  • Deterministic execution-order resolver with cycle detection, tie‑breaking, phased ordering, and a public resolver interface.
  • Plans now include resolution metadata, tie‑breaker decisions, constraint evaluations, conflict/warning tracking, and an is-executable check.
  • Policy option to treat missing references as errors (strict mode).

• New Models

  • Public models for conflicts, phase entries, constraint results, tie‑breaker decisions, and resolution metadata.

• Tests

  • Extensive unit and performance test suites validating ordering, cycles, tie‑breaking, metadata, determinism, and scalability.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a deterministic execution-order resolution subsystem: new frozen resolution models, a Resolution protocol, a pure ExecutionResolver implementation (graph build, cycle detection, deterministic tie‑breaking, phase assignment), supporting internals, and extensive unit and performance tests.

Changes

Cohort / File(s)	Summary
Execution resolution models src/omnibase_core/models/execution/model_constraint_satisfaction.py, .../model_execution_conflict.py, .../model_phase_entry.py, .../model_resolution_metadata.py, .../model_tie_breaker_decision.py, src/omnibase_core/models/execution/__init__.py	Add five new frozen Pydantic models (constraint satisfaction, execution conflict, phase entry, resolution metadata, tie‑breaker decision) and re‑export them from the execution models package.
Execution plan updates src/omnibase_core/models/execution/model_execution_plan.py	Add resolution_metadata, conflicts, is_valid fields; add query helpers (has_conflicts, has_blocking_conflicts, get_blocking_conflicts, get_warnings, get_cycle_conflicts, had_tie_breakers, can_execute); update str/repr to include validity/resolution info.
Resolution protocol & exports src/omnibase_core/protocols/resolution/protocol_execution_resolver.py, src/omnibase_core/protocols/resolution/__init__.py, src/omnibase_core/protocols/__init__.py	New runtime-checkable ProtocolExecutionResolver.resolve(...) -> ModelExecutionPlan; added to resolution subpackage and top-level protocols exports.
Execution resolver implementation src/omnibase_core/resolution/execution_resolver.py, src/omnibase_core/resolution/__init__.py	New pure, deterministic ExecutionResolver implementing: handler indexing, dependency graph construction, cycle detection, Kahn topological sort with deterministic tie‑breaking, phase assignment, plan creation (valid/invalid) and populated resolution metadata; re‑exported via resolution package.
Resolver internals src/omnibase_core/resolution/_dependency_graph.py, src/omnibase_core/resolution/_handler_info.py	New internal dataclasses _DependencyGraph and _HandlerInfo for graph edges, reverse edges, nodes, and per‑handler metadata.
Ordering policy update src/omnibase_core/models/contracts/model_execution_ordering_policy.py	Add strict_mode: bool to ordering policy (defaults False) to escalate missing references from warnings to errors when enabled.
Tests tests/unit/resolution/__init__.py, tests/unit/resolution/test_execution_resolver.py, tests/unit/resolution/test_execution_resolver_performance.py	Add comprehensive unit and performance test suites covering ordering, cycles, tie‑breaking, constraints, strict mode, phase assignment, determinism, metadata, protocol conformance, and scalability.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor Client
    participant Resolver as ExecutionResolver
    participant Index as HandlerIndex
    participant Graph as DependencyGraph
    participant Sort as TopologicalSort
    participant Plan as PlanBuilder

    Client->>Resolver: resolve(profile, contracts)
    activate Resolver

    Resolver->>Index: build handler & capability/tag indices
    Index-->>Resolver: indices

    Resolver->>Graph: build_graph(indices, constraints)
    Graph-->>Resolver: edges, nodes

    rect rgb(240,220,200)
      Note over Resolver: Cycle detection (DFS)
      Resolver->>Resolver: detect_cycles()
    end

    alt cycles found
        Resolver->>Plan: create_invalid_plan(with conflicts + metadata)
    else no cycles
        Resolver->>Sort: kahn_topological_sort(edges, tie_breaker_policy)
        Sort-->>Resolver: ordered_groups
        rect rgb(220,240,220)
          Note over Resolver: apply tie-breakers (priority / alphabetical)
        end
        Resolver->>Plan: create_valid_plan(ordered_groups + metadata)
    end

    Plan-->>Client: ModelExecutionPlan
    deactivate Resolver

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I hopped through graphs and ties tonight,

handlers ordered left and right,
cycles called out, decisions penned,
metadata stitched from start to end,
now plans march forward, tidy and bright.

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 accurately summarizes the main change: adding ExecutionResolver for execution order resolution. It is concise, specific, and clearly indicates the primary purpose of the PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

---

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

[claude]

Code Review: ExecutionResolver Implementation (OMN-1106)

Summary

This is an excellent, production-ready implementation of pure execution order resolution. The code demonstrates strong software engineering practices with comprehensive testing, clear documentation, and adherence to project conventions.

✅ Strengths

Architecture & Design

• Pure function implementation: Truly stateless with no side effects - well done adhering to functional principles
• Deterministic ordering: Uses stable sorting (priority + alphabetical tie-breakers) ensuring reproducibility
• Fail-fast validation: Conflicts detected at resolution time, not runtime - excellent defensive programming
• Clean separation of concerns: Protocol definition separate from implementation
• Well-structured models: Immutable Pydantic models with frozen=True for thread safety

Code Quality

• Comprehensive test coverage: 34 tests across 942 lines covering all major scenarios (basic ordering, cycles, tie-breaking, constraints, edge cases)
• Clear documentation: Excellent docstrings with examples and version annotations
• Type safety: Proper use of from __future__ import annotations for forward references, Protocol conformance
• Error handling: Structured conflict reporting with detailed messages and suggestions
• Naming conventions: Consistent use of _ prefix for internal types (_DependencyGraph, _HandlerInfo)

Algorithm Implementation

• Kahn's algorithm: Correct topological sort implementation with cycle detection
• Cycle detection: Proper DFS-based cycle detection with full path reporting
• Constraint resolution: Smart indexing (capability, tag, handler) for O(1) lookups
• Deterministic tie-breaking: Stable ordering using priority then alphabetical fallback

🔍 Code Quality Observations

Good Practices Followed

✅ All base class conventions from CLAUDE.md
✅ Proper use of ModelONEXContainer vs ModelContainer distinction
✅ Protocol-driven design (ProtocolExecutionResolver)
✅ Immutable models with frozen=True and from_attributes=True
✅ Proper exception tuple ordering (alphabetical)
✅ No use of generic Exception - structured ModelExecutionConflict instead
✅ Clean separation of pure resolution logic from I/O

Implementation Details

ExecutionResolver.py (770 lines)

• Clean method organization with section comments
• Proper use of internal helper methods with _ prefix
• Good separation of index building, graph construction, cycle detection, sorting, and plan creation
• Efficient use of defaultdict for graph construction
• Correct handling of empty contracts edge case

Cycle Detection (lines 399-462)

• Proper DFS with recursion stack tracking
• Correct cycle path extraction from traversal path
• Good handling of multiple cycles (reset and continue)
• Clear conflict message generation

Topological Sort (lines 468-587)

• Correct in-degree calculation
• Stable tie-breaking with metadata recording
• Deterministic candidate ordering (sorted())
• Proper reverse edge traversal for dependent updates

Constraint Resolution (lines 321-393)

• Support for handler:, capability:, and tag: references
• Warning-level conflicts for missing dependencies (not errors - good UX)
• Clear suggested resolutions in conflict messages
• No self-edge creation (line 295, 314)

🎯 Potential Issues & Suggestions

1. In-degree Calculation Bug (Lines 486-494) ⚠️

in_degree: dict[str, int] = dict.fromkeys(graph.nodes, 0)
for node in graph.nodes:
    for dep in graph.edges.get(node, set()):
        if dep in in_degree:  # Only count edges to known nodes
            pass  # <-- THIS DOES NOTHING!
    # Count incoming edges
    in_degree[node] = len(
        [d for d in graph.edges.get(node, set()) if d in graph.nodes]
    )

Issue: The loop at lines 488-490 doesn't modify in_degree. The in-degree is recalculated at line 492-494, but it's counting outgoing edges (dependencies), not incoming edges.

Correct Implementation:

in_degree: dict[str, int] = dict.fromkeys(graph.nodes, 0)
for node in graph.nodes:
    for dep in graph.edges.get(node, set()):
        if dep in in_degree:
            in_degree[dep] += 1  # Increment in-degree of dependency

Or using reverse_edges:

in_degree: dict[str, int] = {
    node: len(graph.edges.get(node, set())) 
    for node in graph.nodes
}

Why tests pass: If the graph semantics are inverted (edges represent "depends on" rather than "must come before"), this would work. But the comment at line 22 says edges represent dependencies (must run AFTER), which contradicts the implementation.

Recommendation: Clarify graph edge semantics and fix in-degree calculation to match.

2. Missing Protocol Export

The ProtocolExecutionResolver is defined in src/omnibase_core/protocols/resolution/protocol_execution_resolver.py but the PR doesn't show it being exported from src/omnibase_core/protocols/__init__.py.

Check: Verify the protocol is properly exported:

from omnibase_core.protocols import ProtocolExecutionResolver  # Should work

3. Thread Safety Claims

The PR and docstrings claim thread safety because the class is "stateless." This is correct for the ExecutionResolver class itself, but:

• Pydantic models are thread-safe (immutable with frozen=True) ✅
• Internal data structures (_DependencyGraph) use defaultdict which is NOT thread-safe
• However, these are created locally in resolve() so no issue ✅

Recommendation: Add a note that while the resolver is stateless, the internal graph structures are NOT shared across calls, so thread safety is preserved.

4. Performance Consideration

The tie-breaker logic (lines 526-587) sorts candidates on every iteration of Kahn's algorithm. For large graphs with many ties, this could be O(n² log n).

Current: sorted() called repeatedly in while loop
Optimization: Use a priority queue (heapq) for O(n log n) total

Note: Premature optimization; current approach is clear and correct.

5. Constraint Reference Validation

The _resolve_constraint_ref method (lines 321-393) handles invalid prefixes by returning empty list and None (line 393). This silently ignores invalid constraint syntax.

Recommendation: Consider adding a warning-level conflict for unknown constraint prefixes to aid debugging.

6. Phase Assignment Hardcoded

All handlers currently assigned to EXECUTE phase (lines 624-646). The comment notes this is temporary, but the models and protocol support per-handler phase hints.

Question: Is there a follow-up ticket to implement phase hints from handler contracts?

🔐 Security Considerations

✅ No SQL injection risks (pure computation)
✅ No command injection (no shell calls)
✅ No path traversal (no file I/O)
✅ No unvalidated user input (Pydantic validation)
✅ Cycle detection prevents infinite loops
✅ No DoS concerns (deterministic, bounded algorithm)

📊 Test Coverage Analysis

Coverage: 34 tests across 942 lines
Test categories:

• Basic ordering: 6 tests ✅
• Cycle detection: 3 tests ✅
• Tie-breaking: Multiple scenarios ✅
• Constraint resolution: capability, handler, tag ✅
• Edge cases: empty, single, all independent ✅
• Determinism verification ✅
• Protocol compliance ✅

Missing test scenarios:

1. Very large graphs (performance testing)
2. Concurrent resolve() calls (thread safety verification)
3. Malformed constraint references (e.g., "invalid:prefix")
4. Extremely deep dependency chains (stack overflow check)

📝 Documentation Quality

✅ Comprehensive module docstrings
✅ Example usage in docstrings
✅ Version annotations (.. versionadded:: 0.4.1)
✅ See Also references to related models
✅ Type hints on all public methods
✅ Clear inline comments for complex logic

🎨 Style & Conventions

✅ Follows CLAUDE.md conventions
✅ PEP 604 union syntax (str | None)
✅ Proper use of from __future__ import annotations
✅ Alphabetical exception tuple ordering
✅ Consistent 80-char line wrapping
✅ SPDX license headers

🚀 Performance Characteristics

• Time complexity: O(V + E + V log V) for topological sort with tie-breaking
• Space complexity: O(V + E) for graph storage
• Determinism: Guaranteed via stable sorts
• Scalability: Should handle hundreds of handlers efficiently

✨ Recommendations

High Priority

1. Fix in-degree calculation (lines 486-494) - verify graph semantics and correct the bug
2. Add test for in-degree calculation to prevent regression

Medium Priority

3. Verify protocol export in protocols/__init__.py
4. Add performance test for large graphs (100+ handlers)
5. Add thread safety test (concurrent resolve calls)

Low Priority

6. Consider heapq optimization for tie-breaking if needed
7. Add warning for unknown constraint prefixes
8. Document follow-up work for phase hint support

🎯 Final Verdict

APPROVE with minor fixes recommended

This is high-quality, well-tested code that demonstrates excellent software engineering practices. The pure function design, comprehensive testing, and clear documentation make this a strong addition to the codebase.

The in-degree calculation issue (item #1) should be verified and fixed before merge, but the comprehensive test coverage gives confidence that the overall algorithm is working correctly (even if the implementation details need clarification).

Excellent work on this feature! 🎉

---

Reviewed by: Claude Code
Review date: 2026-01-02
PR: #307 (OMN-1106)

[coderabbitai]

Actionable comments posted: 7

🧹 Nitpick comments (5)

src/omnibase_core/resolution/execution_resolver.py (3)

454-460: Cycle detection may miss additional cycles after resetting.

After finding a cycle, the code resets visited, rec_stack, and path (lines 455-457), then marks only the cycle nodes as visited (lines 459-460). However, this approach may not correctly find all independent cycles if the graph has multiple disconnected cycle components, since the outer loop continues from where it left off in sorted(graph.nodes).

The logic should either:

1. Return after finding the first cycle (fail-fast), or
2. Properly handle finding multiple independent cycles

Currently, marking only cycle nodes as visited after reset may cause the algorithm to revisit non-cycle nodes incorrectly.

🔎 Consider simplifying to fail-fast after first cycle

         for node in sorted(graph.nodes):
             if node not in visited:
                 cycle = dfs(node)
                 if cycle:
                     # Build a readable message
                     cycle_str = " -> ".join(cycle)
                     conflicts.append(
                         ModelExecutionConflict(
                             conflict_type="cycle",
                             severity="error",
                             message=f"Circular dependency detected: {cycle_str}",
                             handler_ids=cycle[:-1],  # Unique handlers in cycle
                             cycle_path=cycle,
                             suggested_resolution=(
                                 "Remove one of the dependencies to break the cycle"
                             ),
                         )
                     )
-                    # Reset for finding other cycles
-                    visited.clear()
-                    rec_stack.clear()
-                    path.clear()
-                    # Mark the cycle nodes as visited to skip them
-                    for n in cycle[:-1]:
-                        visited.add(n)
+                    # Fail-fast: return immediately after first cycle
+                    return conflicts
 
         return conflicts

---

614-654: All handlers assigned to EXECUTE phase regardless of constraints.

Lines 624-628 and the loop at 632-653 assign all handlers to the EXECUTE phase. While the docstring acknowledges this is a current limitation ("For now, all handlers are assigned to the EXECUTE phase"), this means the profile_phases parameter and phase-specific handler assignment are not functional.

Consider adding a TODO comment or tracking issue reference for future implementation.

---

673-678: Empty plan sets source_profile=None despite having access to profile.

In _create_empty_plan, _create_invalid_plan, and _create_valid_plan, the source_profile field is always set to None (lines 673, 708, 747). The profile object is available and could provide useful debugging context.

🔎 Consider populating source_profile from the profile

         return ModelExecutionPlan(
             phases=[],
-            source_profile=None,
+            source_profile=getattr(profile, 'name', None),
             ordering_policy=profile.ordering_policy.strategy,

tests/unit/resolution/test_execution_resolver.py (2)

779-801: Order independence test assertion may be too weak.

The test_order_independence test only asserts that the same set of handlers appears in both plans (line 801: set(plan1...) == set(plan2...)), but doesn't verify they have the same order. For a determinism test, both the content AND order should be identical.

🔎 Strengthen the determinism assertion

         plan1 = resolver.resolve(default_profile, contracts1)
         plan2 = resolver.resolve(default_profile, contracts2)
 
         assert plan1.is_valid and plan2.is_valid
-        # Same handlers, possibly same order (deterministic)
-        assert set(plan1.get_all_handler_ids()) == set(plan2.get_all_handler_ids())
+        # Same handlers in same order (deterministic)
+        assert plan1.get_all_handler_ids() == plan2.get_all_handler_ids()

---

420-437: Tie-breaker decision recording test is incomplete.

The test test_tie_breaker_decision_recorded creates two handlers and checks that resolution_metadata is not None, but doesn't actually verify that tie-breaker decisions were recorded. The comment at lines 435-436 acknowledges this uncertainty.

🔎 Add assertion for tie-breaker recording

         plan = resolver.resolve(default_profile, contracts)
 
         assert plan.is_valid
         assert plan.resolution_metadata is not None
-        # Tie-breaking should have occurred
-        # (though the specific decision may vary based on implementation)
+        # Verify tie-breaking occurred and was recorded
+        assert plan.had_tie_breakers() or len(plan.resolution_metadata.tie_breaker_decisions) >= 0
+        # At minimum, verify the tie-breaker order is set
+        assert plan.resolution_metadata.tie_breaker_order == ["priority", "alphabetical"]

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 74ce9bd and 72d9ae5.

📒 Files selected for processing (16)

• src/omnibase_core/models/execution/__init__.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/models/execution/model_phase_entry.py
• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py
• src/omnibase_core/protocols/__init__.py
• src/omnibase_core/protocols/resolution/__init__.py
• src/omnibase_core/protocols/resolution/protocol_execution_resolver.py
• src/omnibase_core/resolution/__init__.py
• src/omnibase_core/resolution/_dependency_graph.py
• src/omnibase_core/resolution/_handler_info.py
• src/omnibase_core/resolution/execution_resolver.py
• tests/unit/resolution/__init__.py
• tests/unit/resolution/test_execution_resolver.py

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Use PEP 604 union syntax for type annotations (e.g., str | None instead of Optional[str])
Resolve services using protocol names (e.g., container.get_service('ProtocolEventBus')) never using concrete class names
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError))
Format code using poetry run black and sort imports using poetry run isort
Lint code with poetry run ruff check and fix automatically with poetry run ruff check --fix

Files:

• src/omnibase_core/protocols/resolution/__init__.py
• tests/unit/resolution/__init__.py
• src/omnibase_core/protocols/__init__.py
• src/omnibase_core/protocols/resolution/protocol_execution_resolver.py
• src/omnibase_core/resolution/execution_resolver.py
• src/omnibase_core/resolution/_dependency_graph.py
• src/omnibase_core/models/execution/__init__.py
• src/omnibase_core/resolution/__init__.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_resolution_metadata.py
• tests/unit/resolution/test_execution_resolver.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py
• src/omnibase_core/models/execution/model_phase_entry.py
• src/omnibase_core/resolution/_handler_info.py

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/omnibase_core/**/*.py: Use ModelOnexError with error codes from EnumCoreErrorCode for exception handling, never generic Exception
Use @standard_error_handling decorator from omnibase_core.decorators to eliminate try/catch boilerplate
Add standardized exception handling comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using broad exception handlers
Use centralized exception group constants from omnibase_core.errors.exception_groups instead of individual exception types
Ensure all source code passes mypy strict mode with zero errors and pyright basic mode validation
Use FileRegistry for loading YAML contracts and configuration files instead of direct file reading
Use duck typing with protocols for service resolution - never use isinstance checks for protocol-based dependencies

Files:

• src/omnibase_core/protocols/resolution/__init__.py
• src/omnibase_core/protocols/__init__.py
• src/omnibase_core/protocols/resolution/protocol_execution_resolver.py
• src/omnibase_core/resolution/execution_resolver.py
• src/omnibase_core/resolution/_dependency_graph.py
• src/omnibase_core/models/execution/__init__.py
• src/omnibase_core/resolution/__init__.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py
• src/omnibase_core/models/execution/model_phase_entry.py
• src/omnibase_core/resolution/_handler_info.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) to classify test types
All tests must complete within 60-second timeout protection to ensure CI performance

Files:

• tests/unit/resolution/__init__.py
• tests/unit/resolution/test_execution_resolver.py

src/omnibase_core/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/omnibase_core/models/**/*.py: Add from_attributes=True to ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution
Do NOT use from __future__ import annotations in Pydantic model files (use only for forward references and circular import prevention)

Files:

• src/omnibase_core/models/execution/__init__.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py
• src/omnibase_core/models/execution/model_phase_entry.py

🧠 Learnings (3)

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to src/omnibase_core/**/*.py : Use duck typing with protocols for service resolution - never use isinstance checks for protocol-based dependencies

Applied to files:

• src/omnibase_core/protocols/resolution/__init__.py
• src/omnibase_core/protocols/__init__.py
• src/omnibase_core/protocols/resolution/protocol_execution_resolver.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to src/omnibase_core/**/*.py : Use ModelOnexError with error codes from EnumCoreErrorCode for exception handling, never generic Exception

Applied to files:

• src/omnibase_core/models/execution/__init__.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution

Applied to files:

• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_execution_plan.py

🧬 Code graph analysis (11)

src/omnibase_core/protocols/resolution/__init__.py (1)

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (1)

• ProtocolExecutionResolver (75-249)

src/omnibase_core/protocols/__init__.py (2)

src/omnibase_core/protocols/resolution/protocol_dependency_resolver.py (1)

• ProtocolDependencyResolver (50-184)

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (1)

• ProtocolExecutionResolver (75-249)

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (4)

src/omnibase_core/models/contracts/model_execution_profile.py (1)

• ModelExecutionProfile (25-53)

src/omnibase_core/models/contracts/model_handler_contract.py (1)

• ModelHandlerContract (61-416)

src/omnibase_core/models/execution/model_execution_plan.py (1)

• ModelExecutionPlan (43-370)

src/omnibase_core/resolution/execution_resolver.py (1)

• resolve (97-185)

src/omnibase_core/resolution/execution_resolver.py (7)

src/omnibase_core/enums/enum_handler_execution_phase.py (2)

• EnumHandlerExecutionPhase (18-140)
• is_before (118-128)

src/omnibase_core/models/execution/model_execution_conflict.py (2)

• ModelExecutionConflict (28-214)
• is_blocking (157-164)

src/omnibase_core/models/execution/model_execution_plan.py (2)

• ModelExecutionPlan (43-370)
• total_handlers (211-222)

src/omnibase_core/models/execution/model_phase_step.py (1)

• ModelPhaseStep (24-148)

src/omnibase_core/models/execution/model_resolution_metadata.py (1)

• ModelResolutionMetadata (33-210)

src/omnibase_core/resolution/_dependency_graph.py (1)

• _DependencyGraph (19-27)

src/omnibase_core/resolution/_handler_info.py (1)

• _HandlerInfo (22-30)

src/omnibase_core/models/execution/__init__.py (7)

src/omnibase_core/models/execution/model_constraint_satisfaction.py (1)

• ModelConstraintSatisfaction (25-84)

src/omnibase_core/models/execution/model_execution_conflict.py (1)

• ModelExecutionConflict (28-214)

src/omnibase_core/models/execution/model_execution_plan.py (1)

• ModelExecutionPlan (43-370)

src/omnibase_core/models/execution/model_phase_entry.py (1)

• ModelPhaseEntry (30-161)

src/omnibase_core/models/execution/model_phase_step.py (1)

• ModelPhaseStep (24-148)

src/omnibase_core/models/execution/model_resolution_metadata.py (1)

• ModelResolutionMetadata (33-210)

src/omnibase_core/models/execution/model_tie_breaker_decision.py (1)

• ModelTieBreakerDecision (25-86)

src/omnibase_core/resolution/__init__.py (1)

src/omnibase_core/resolution/execution_resolver.py (1)

• ExecutionResolver (61-765)

src/omnibase_core/models/execution/model_execution_conflict.py (1)

src/omnibase_core/models/errors/model_onex_error.py (1)

• message (248-250)

src/omnibase_core/models/execution/model_resolution_metadata.py (1)

src/omnibase_core/models/execution/model_tie_breaker_decision.py (1)

• ModelTieBreakerDecision (25-86)

src/omnibase_core/models/execution/model_execution_plan.py (3)

src/omnibase_core/models/execution/model_execution_conflict.py (3)

• ModelExecutionConflict (28-214)
• is_blocking (157-164)
• is_cycle (166-173)

src/omnibase_core/models/execution/model_phase_step.py (3)

• ModelPhaseStep (24-148)
• is_empty (91-98)
• handler_count (100-107)

src/omnibase_core/models/execution/model_resolution_metadata.py (2)

• ModelResolutionMetadata (33-210)
• had_ties (166-173)

src/omnibase_core/models/execution/model_phase_entry.py (1)

src/omnibase_core/models/execution/model_constraint_satisfaction.py (1)

• ModelConstraintSatisfaction (25-84)

src/omnibase_core/resolution/_handler_info.py (1)

src/omnibase_core/models/contracts/model_execution_constraints.py (1)

• ModelExecutionConstraints (39-206)

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

• GitHub Check: claude-review

🔇 Additional comments (36)

src/omnibase_core/protocols/resolution/__init__.py (3)

14-15: LGTM! Clean protocol documentation.

The docstring addition accurately describes the new protocol's purpose and follows the established format for documenting resolution protocols.

---

71-73: LGTM! Import follows package conventions.

The import statement is properly formatted and consistent with the existing import patterns in the module.

---

78-78: LGTM! Export properly added.

The new protocol is correctly added to __all__ and maintains alphabetical ordering.

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (4)

1-54: LGTM! Excellent module documentation.

The module docstring is comprehensive and clearly articulates the design principles (pure, deterministic, fail-fast) and resolution strategy. The usage example effectively demonstrates the protocol contract.

---

56-72: LGTM! Proper use of TYPE_CHECKING.

The imports correctly use TYPE_CHECKING to avoid circular dependencies while providing complete type hints for static analysis. This follows best practices for protocol definitions.

---

74-147: LGTM! Well-designed protocol with excellent documentation.

The protocol class is properly decorated with @runtime_checkable for duck typing support, and the comprehensive docstring provides clear implementation guidance including resolution strategy, key properties, thread safety requirements, and a complete example.

---

149-249: LGTM! Complete method specification with excellent documentation.

The resolve() method is thoroughly documented with a comprehensive docstring covering the resolution process, parameter semantics, return value structure, and conflict handling strategy. The usage example effectively demonstrates how constraints translate to execution order. The method signature uses proper PEP 604 type hints and the ellipsis body is correct for protocol methods.

src/omnibase_core/protocols/__init__.py (2)

187-192: LGTM! Clean integration of new protocol.

The section comment is properly updated to reference both OMN-1123 and OMN-1106, and the multi-line import format is consistent with the file's style. Both resolution protocols are now properly imported.

---

385-388: LGTM! Export properly integrated.

The __all__ list is correctly updated to include ProtocolExecutionResolver in the Resolution Module section with proper alphabetical ordering and matching section comment.

tests/unit/resolution/__init__.py (1)

1-3: LGTM! Standard test package initializer.

The test package is properly initialized with SPDX headers and a descriptive docstring.

src/omnibase_core/resolution/__init__.py (1)

1-21: LGTM! Clean module organization.

The resolution module is well-structured with clear documentation and a focused export surface. The module properly exposes ExecutionResolver as the concrete implementation of execution order resolution.

src/omnibase_core/models/execution/model_tie_breaker_decision.py (1)

55-86: Model configuration and field definitions are well-structured.

The immutable configuration with frozen=True, proper validation settings, and from_attributes=True (per learnings for pytest-xdist support) are correctly applied. The min_length=2 constraint on handler_ids appropriately enforces the tie-breaking semantic.

src/omnibase_core/resolution/_handler_info.py (1)

21-31: Internal dataclass is correctly structured.

The _HandlerInfo dataclass appropriately encapsulates handler metadata for resolution. Fields are properly typed with PEP 604 syntax, and mutable types (list[str]) don't have default values, avoiding the mutable default argument pitfall.

src/omnibase_core/models/execution/model_resolution_metadata.py (1)

81-210: Model implementation is well-designed with useful helper methods.

The configuration correctly includes from_attributes=True for nested model support. The get_resolution_duration() method properly handles both explicit duration and computed duration from timestamps. The __str__ and __repr__ implementations provide good observability.

src/omnibase_core/models/execution/model_constraint_satisfaction.py (1)

53-84: Model configuration and field definitions are correct.

The immutable configuration with from_attributes=True properly supports nesting in other Pydantic models per coding guidelines.

src/omnibase_core/resolution/_dependency_graph.py (1)

18-28: Internal dependency graph dataclass is well-structured.

Using defaultdict(set) for edges allows convenient edge addition without key checks. The bidirectional edge storage (edges and reverse_edges) supports efficient traversal in both directions for Kahn's algorithm.

src/omnibase_core/models/execution/model_execution_conflict.py (2)

140-155: Model validator correctly enforces cycle_path requirement.

The mode="after" validator appropriately validates the cross-field constraint that cycle_path must be provided when conflict_type is "cycle". Raising ValueError is the correct approach for Pydantic validation failures.

---

191-201: Cycle length calculation correctly handles path representation.

The get_cycle_length() method correctly subtracts 1 from the path length to account for the cycle path convention where the starting node is repeated at the end (e.g., ["A", "B", "A"] represents a cycle of length 2).

src/omnibase_core/models/execution/model_phase_entry.py (2)

86-128: Model configuration and field definitions are well-designed.

The from_attributes=True configuration correctly supports the nested ModelConstraintSatisfaction instances in constraints_evaluated, which aligns with the coding guidelines for pytest-xdist parallel execution support. Based on learnings, this is the correct configuration for immutable value objects nested in other Pydantic models.

---

130-146: Helper methods for constraint checking are correct.

The has_unsatisfied_constraints() and get_unsatisfied_constraints() methods correctly filter based on the satisfied attribute and provide useful inspection capabilities for debugging resolution outcomes.

src/omnibase_core/resolution/execution_resolver.py (5)

32-32: Coding guideline violation: from __future__ import annotations in potential Pydantic model context.

Per coding guidelines, from __future__ import annotations should NOT be used in Pydantic model files. While this file is not a model file itself, it does construct Pydantic models. This import is acceptable here since the file only constructs models rather than defining them. No action needed.

---

547-573: Priority tie-breaker always breaks early, preventing alphabetical fallback.

The break statement at line 564 exits the loop after processing the "priority" tie-breaker, regardless of whether it actually resolved ties. The comment says "it provides a complete ordering," but priority sorting with secondary alphabetical (line 555) already handles this. However, if the first tie-breaker in the list is "alphabetical", and a second one is "priority", the second would never be evaluated.

The current implementation works correctly for the default ["priority", "alphabetical"] order because:

1. Priority sorts with secondary alphabetical for stability
2. Breaks immediately

But if a profile specifies ["alphabetical", "priority"], the priority tie-breaker would never run. This seems intentional based on the design but may surprise users.

---

97-185: Well-structured resolve() method with clear algorithmic flow.

The main resolve() method follows the documented algorithm steps clearly:

1. Empty contracts handling
2. Build handler info and indices
3. Build dependency graph
4. Detect cycles
5. Topological sort with tie-breaking
6. Assign to phases
7. Create valid plan

The fail-fast pattern (returning invalid plan on blocking conflicts) and pure-function semantics are correctly implemented.

---

321-393: Constraint resolution correctly handles all reference types with appropriate warnings.

The _resolve_constraint_ref method properly handles:

• handler:X - direct handler references
• capability:X - capability-based lookups
• tag:X - tag-based lookups

Missing references generate warnings (not errors), allowing resolution to continue. This aligns with the fail-fast design that surfaces conflicts at validation time.

---

486-494: Incorrect in-degree calculation in topological sort.

The in-degree calculation logic is flawed. Lines 487-494 iterate over nodes and their edges but the logic doesn't correctly compute in-degrees. The graph.edges[node] represents handlers that node depends on (must run AFTER), so the in-degree should count how many dependencies each node has, not incoming edges from dependents.

Looking at the graph structure from _DependencyGraph:

• edges[handler] = handlers it depends on (must run AFTER these)
• reverse_edges[handler] = handlers that depend on it

The current code at line 492-494 counts len([d for d in graph.edges.get(node, set()) if d in graph.nodes]), which is the out-degree (number of dependencies), not in-degree. This appears intentional based on the algorithm's perspective but the variable name in_degree is misleading given the graph direction semantics.

#!/bin/bash
# Verify the dependency graph edge direction semantics
rg -n -A10 "class _DependencyGraph" src/omnibase_core/resolution/

src/omnibase_core/models/execution/__init__.py (2)

50-64: Imports are properly organized and complete.

The new resolution model imports are correctly added alongside existing core model imports. The import grouping follows a logical pattern (constraint satisfaction, conflict, plan, phase entry, phase step, resolution metadata, tie-breaker decision).

---

66-76: __all__ exports are complete and well-categorized.

The exports are properly organized with comments distinguishing Core models (OMN-1108) from Resolution models (OMN-1106). All imported models are exported, maintaining a clean public API surface.

tests/unit/resolution/test_execution_resolver.py (4)

87-122: Well-designed helper function for test contract creation.

The _create_contract helper provides a clean, readable way to construct test fixtures with sensible defaults. The use of keyword-only arguments (*) after handler_id ensures explicit parameter naming, improving test readability.

---

869-889: Protocol compliance test uses isinstance with protocol.

The test at line 878 uses isinstance(resolver, ProtocolExecutionResolver) to verify protocol compliance. Per coding guidelines, duck typing with protocols should be preferred over isinstance checks. However, for test verification purposes, this usage is acceptable as it's explicitly testing protocol conformance rather than runtime type checking in production code.

---

130-251: Comprehensive basic ordering tests with good coverage.

The TestBasicOrdering class thoroughly covers:

• Simple A → B dependency
• Chain dependencies (A → B → C)
• Multiple dependencies (C depends on A and B)
• requires_after constraint
• Diamond dependency pattern

Each test properly asserts both validity and correct ordering relationships.

---

259-347: Cycle detection tests verify error details appropriately.

The cycle detection tests check:

• Direct cycles (A ↔ B)
• Indirect cycles (A → B → C → A)
• Self-reference filtering (not treated as cycle)

Tests properly verify is_valid=False, has_blocking_conflicts(), cycle path contents, and conflict types. Good coverage of edge cases.

src/omnibase_core/models/execution/model_execution_plan.py (5)

119-125: ConfigDict correctly includes from_attributes=True.

As per the retrieved learnings, from_attributes=True is required in ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution. This is correctly configured.

---

156-169: New resolution fields are well-defined with appropriate defaults.

The new fields follow good practices:

• resolution_metadata: ModelResolutionMetadata | None - Optional, defaults to None
• conflicts: list[ModelExecutionConflict] - Uses default_factory=list for mutable default
• is_valid: bool - Defaults to True (optimistic)

PEP 604 union syntax is correctly used.

---

275-347: Resolution methods provide comprehensive query API.

The new methods provide a clean API for querying resolution results:

• has_conflicts() / has_blocking_conflicts() - Boolean checks
• get_blocking_conflicts() / get_warnings() / get_cycle_conflicts() - Filtered lists
• had_tie_breakers() - Delegates to resolution_metadata
• can_execute() - Composite check for executability

All methods are pure (no side effects) and consistent with the immutable model design.

---

333-347: can_execute() logic is clear and well-documented.

The method correctly combines three conditions:

1. is_valid is True
2. No blocking conflicts
3. Plan is not empty

The docstring clearly documents these requirements.

---

349-370: String representations updated to reflect new fields.

__str__ now includes validity status, and __repr__ includes is_valid, conflict count, and resolution_metadata. This improves debugging experience.

[claude]

Code Review - PR #307: Execution Order Resolution (OMN-1106)

Overview

This PR implements a pure function-based execution order resolver for the ONEX framework. The implementation is well-architected, thoroughly tested, and aligns excellently with the project's architectural principles.

---

✅ Strengths

1. Excellent Architecture & Design

• Pure function implementation: Zero side effects, no global state, no I/O
• Deterministic resolution: Stable topological sort with tie-breaking guarantees same inputs → same outputs
• Fail-fast approach: Conflicts detected at validation time (not runtime) with detailed conflict models
• Protocol-driven: Clean separation between interface (ProtocolExecutionResolver) and implementation (ExecutionResolver)

2. Outstanding Test Coverage

• 34 comprehensive tests covering all major scenarios
• 949 lines of tests vs 880 lines of implementation (1.08:1 ratio) - excellent coverage
• Test categories:
  • Basic ordering (simple, chains, diamond dependencies)
  • Cycle detection (direct, indirect, self-reference)
  • Tie-breaking (priority, alphabetical, combined)
  • Phase assignment
  • Constraint resolution (capability, handler, tag)
  • Determinism verification
  • Protocol compliance

3. Robust Error Handling

• Rich conflict models with detailed metadata:
  • ModelExecutionConflict with 6 conflict types
  • Cycle path tracking for debugging
  • Severity levels (error/warning)
  • Suggested resolutions
• Returns invalid plans with conflicts rather than throwing exceptions (graceful degradation)

4. Strong Type Safety

• Immutable models (frozen=True)
• Comprehensive validation with Pydantic
• Thread-safe by design (stateless resolver)
• PEP 604 union syntax throughout

5. Excellent Documentation

• Comprehensive docstrings with examples
• Clear protocol documentation
• Version annotations (versionadded, versionchanged)
• Related issue references (OMN-1106, OMN-1108, etc.)

---

🔍 Code Quality Observations

Algorithm Implementation

The topological sort using Kahn's algorithm is correctly implemented with:

• Proper in-degree calculation
• Deterministic tie-breaking (priority → alphabetical)
• Cycle detection via DFS with recursion stack
• Early return on blocking conflicts

Data Structures

• _DependencyGraph: Clean separation of edges/reverse_edges/nodes
• _HandlerInfo: Efficient extraction of handler metadata
• Both use appropriate collection types (defaultdict, set)

Constraint Resolution

The constraint reference resolution (src/omnibase_core/resolution/execution_resolver.py:400-449) correctly handles:

• handler:X - Direct handler references
• capability:Y - Capability-based matching
• tag:Z - Tag-based matching
• Missing dependencies produce warnings (not errors) - good UX

---

💡 Minor Suggestions

1. Phase Assignment Logic

Location: execution_resolver.py (phase assignment section, not shown in snippet)

The phase assignment logic should be validated to ensure:

• Handlers are assigned to appropriate phases based on their handler_kind
• Cross-phase dependencies are detected (e.g., EXECUTE handler depending on AFTER handler would be invalid)

Suggestion: Add integration tests verifying phase assignment respects the ONEX Four-Node Architecture constraints.

2. Tie-Breaker Decision Tracking

The ModelTieBreakerDecision model tracks which tie-breaker was applied. Consider adding:

• Statistics on how often each tie-breaker was used (e.g., "priority used 5 times, alphabetical used 12 times")
• This would be valuable for debugging and optimizing ordering policies

3. Performance Consideration: Constraint Reference Resolution

Location: execution_resolver.py:400-449 (_resolve_constraint_ref)

Currently, each constraint reference lookup is O(1) via dictionaries (good!). However, if a single handler has many constraints, this could become a bottleneck.

Suggestion: Add a performance test for large-scale resolution (e.g., 100+ handlers, 500+ constraints) to validate algorithmic complexity remains acceptable.

4. Cycle Detection - Multiple Cycles

Location: execution_resolver.py:455-522 (_detect_cycles)

The implementation returns immediately after finding the first cycle:

# Line 518-520
# One blocking cycle is sufficient to invalidate the plan.
# Return immediately to avoid complex state management.
return conflicts

Rationale: This is actually correct for fail-fast behavior. However, consider documenting this limitation in the conflict model.

Alternative: If detecting ALL cycles is desired (for better debugging), consider using Tarjan's SCC algorithm to find all strongly connected components in one pass.

5. Missing Dependency Handling

Location: execution_resolver.py:411-446

Missing dependencies produce warnings (severity='warning'), which is great for UX. However:

Question: Should there be a policy flag to treat missing dependencies as errors? Some users might want strict validation.

Suggestion: Add an optional strict_mode parameter to the resolver (or profile policy) that upgrades warnings to errors.

---

🔒 Security Considerations

✅ No Security Concerns Identified

• Pure function with no I/O
• No user input directly executed
• No SQL injection, XSS, or command injection vectors
• Constraint references are validated before resolution

---

🧪 Test Coverage Analysis

Covered Scenarios ✅

• Simple dependencies (A → B)
• Dependency chains (A → B → C)
• Diamond dependencies (A → B, A → C, B/C → D)
• Direct cycles (A → B → A)
• Indirect cycles (A → B → C → A)
• Self-reference cycles (A → A)
• Priority tie-breaking
• Alphabetical tie-breaking
• Combined tie-breaking
• Empty contracts
• Single handler
• Independent handlers (no dependencies)
• Capability constraints
• Handler constraints
• Tag constraints
• Determinism verification
• Protocol compliance

Potential Gaps 🤔

1. Phase conflict detection: Are there tests for handlers with constraints that violate phase ordering? (e.g., EXECUTE handler depending on AFTER handler)
2. Large-scale resolution: No performance tests for 100+ handlers
3. Conflicting tie-breakers: What happens if priority tie-breaker fails AND alphabetical fails? (Unlikely but edge case)
4. Parallel eligibility: The parallel_eligible field exists in ModelPhaseEntry but I don't see tests validating this flag

Recommendation: Add integration tests for #1 (phase conflicts). Others are nice-to-have.

---

📊 Metrics

Metric	Value	Status
Test Coverage	34 tests, 949 lines	✅ Excellent
Test:Code Ratio	1.08:1	✅ Above 1.0
Type Safety	Frozen models, strict validation	✅ Strong
Documentation	Comprehensive docstrings	✅ Excellent
Thread Safety	Stateless, immutable models	✅ Safe
Algorithm Complexity	O(V + E) topological sort	✅ Optimal

---

🎯 Alignment with CLAUDE.md

✅ Follows Project Standards

• Poetry: All imports use Poetry-managed dependencies
• Type Checking: Uses PEP 604 union syntax (str | None)
• Error Handling: Structured errors via ModelExecutionConflict (not generic Exception)
• Pydantic Models: frozen=True, extra="forbid", from_attributes=True for value objects
• Thread Safety: Documented and implemented correctly
• Documentation: Follows versionadded/versionchanged conventions
• Architecture: Aligns with ONEX Four-Node Architecture

✅ Avoids Common Pitfalls

• ✅ Uses protocol-driven DI (not concrete classes)
• ✅ No generic Exception usage
• ✅ Immutable models (thread-safe)
• ✅ No backwards compatibility hacks
• ✅ No over-engineering (focused implementation)

---

🚀 Recommendation

APPROVE WITH MINOR SUGGESTIONS

This PR is production-ready with the following conditions:

1. ✅ Merge as-is - Core implementation is solid
2. 📝 Follow-up tasks (can be addressed in future PRs):
   • Add integration tests for phase conflict detection
   • Consider adding performance tests for large-scale resolution
   • Evaluate strict mode for missing dependencies

Why Approve?

• Pure function design is architecturally sound
• Test coverage is exceptional (34 tests, 1.08:1 ratio)
• Error handling is robust and user-friendly
• Aligns perfectly with ONEX principles
• No security concerns
• No breaking changes
• Documentation is comprehensive

---

📝 Final Notes

This is exemplary work that sets a high bar for future ONEX components. The combination of:

• Pure functional design
• Comprehensive testing
• Detailed conflict reporting
• Protocol-driven architecture

...makes this a model implementation for the framework. The minor suggestions above are optimizations and nice-to-haves, not blockers.

Great job on OMN-1106! 🎉

---

Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2026-01-02
Files Reviewed: 16 files, 3146 additions, 11 deletions

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/omnibase_core/resolution/execution_resolver.py (1)

400-402: Consider logging malformed constraint references.

The comment states "Should not happen due to validation," but this silently returns an empty list. Consider adding a warning log or conflict for observability in case upstream validation is bypassed.

🔎 Suggested enhancement

         if len(parts) != 2:
-            return [], None  # Should not happen due to validation
+            # Should not happen due to validation, but handle gracefully
+            return [], ModelExecutionConflict(
+                conflict_type="constraint_violation",
+                severity="warning",
+                message=f"Malformed constraint reference: '{ref}'",
+                handler_ids=[source_handler],
+                constraint_refs=[ref],
+                suggested_resolution="Use format 'prefix:value' where prefix is 'handler', 'capability', or 'tag'",
+            )

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72d9ae5 and 1408389.

📒 Files selected for processing (8)

• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/models/execution/model_phase_entry.py
• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py
• src/omnibase_core/resolution/execution_resolver.py
• tests/unit/resolution/test_execution_resolver.py

🚧 Files skipped from review as they are similar to previous changes (1)

• src/omnibase_core/models/execution/model_phase_entry.py

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Use PEP 604 union syntax for type annotations (e.g., str | None instead of Optional[str])
Resolve services using protocol names (e.g., container.get_service('ProtocolEventBus')) never using concrete class names
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError))
Format code using poetry run black and sort imports using poetry run isort
Lint code with poetry run ruff check and fix automatically with poetry run ruff check --fix

Files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_plan.py
• tests/unit/resolution/test_execution_resolver.py
• src/omnibase_core/resolution/execution_resolver.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/omnibase_core/**/*.py: Use ModelOnexError with error codes from EnumCoreErrorCode for exception handling, never generic Exception
Use @standard_error_handling decorator from omnibase_core.decorators to eliminate try/catch boilerplate
Add standardized exception handling comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using broad exception handlers
Use centralized exception group constants from omnibase_core.errors.exception_groups instead of individual exception types
Ensure all source code passes mypy strict mode with zero errors and pyright basic mode validation
Use FileRegistry for loading YAML contracts and configuration files instead of direct file reading
Use duck typing with protocols for service resolution - never use isinstance checks for protocol-based dependencies

Files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/resolution/execution_resolver.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py

src/omnibase_core/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/omnibase_core/models/**/*.py: Add from_attributes=True to ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution
Do NOT use from __future__ import annotations in Pydantic model files (use only for forward references and circular import prevention)

Files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) to classify test types
All tests must complete within 60-second timeout protection to ensure CI performance

Files:

• tests/unit/resolution/test_execution_resolver.py

🧠 Learnings (4)

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to src/omnibase_core/models/**/*.py : Do NOT use `from __future__ import annotations` in Pydantic model files (use only for forward references and circular import prevention)

Applied to files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_execution_plan.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution

Applied to files:

• src/omnibase_core/models/execution/model_execution_conflict.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to **/*.py : Use PEP 604 union syntax for type annotations (e.g., `str | None` instead of `Optional[str]`)

Applied to files:

• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_constraint_satisfaction.py
• src/omnibase_core/models/execution/model_tie_breaker_decision.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to tests/**/*.py : Use pytest markers (pytest.mark.unit, pytest.mark.integration, pytest.mark.slow, pytest.mark.smoke, pytest.mark.performance) to classify test types

Applied to files:

• tests/unit/resolution/test_execution_resolver.py

🧬 Code graph analysis (5)

src/omnibase_core/models/execution/model_resolution_metadata.py (1)

src/omnibase_core/models/execution/model_tie_breaker_decision.py (1)

• ModelTieBreakerDecision (23-84)

src/omnibase_core/models/execution/model_execution_conflict.py (1)

src/omnibase_core/models/errors/model_onex_error.py (1)

• message (248-250)

src/omnibase_core/models/execution/model_constraint_satisfaction.py (1)

src/omnibase_core/models/node_metadata/model_function_node_metadata_class.py (1)

• notes (92-94)

src/omnibase_core/models/execution/model_execution_plan.py (4)

src/omnibase_core/enums/enum_handler_execution_phase.py (1)

• EnumHandlerExecutionPhase (18-140)

src/omnibase_core/models/execution/model_execution_conflict.py (3)

• ModelExecutionConflict (26-212)
• is_blocking (155-162)
• is_cycle (164-171)

src/omnibase_core/models/execution/model_phase_step.py (3)

• ModelPhaseStep (24-148)
• is_empty (91-98)
• handler_count (100-107)

src/omnibase_core/models/execution/model_resolution_metadata.py (2)

• ModelResolutionMetadata (31-208)
• had_ties (164-171)

tests/unit/resolution/test_execution_resolver.py (3)

src/omnibase_core/models/contracts/model_handler_contract.py (1)

• ModelHandlerContract (61-416)

src/omnibase_core/resolution/execution_resolver.py (2)

• ExecutionResolver (61-875)
• resolve (97-185)

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (1)

• resolve (149-249)

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

• GitHub Check: claude-review

🔇 Additional comments (16)

src/omnibase_core/models/execution/model_constraint_satisfaction.py (1)

1-87: LGTM!

This model is well-structured with proper immutability (frozen=True), validation configuration, and PEP 604 union syntax. The from_attributes=True setting correctly supports pytest-xdist parallel execution as per coding guidelines.

src/omnibase_core/resolution/execution_resolver.py (4)

604-644: LGTM - Tie-breaker logic is correct.

The implementation correctly:

1. Applies tie-breakers in configured order
2. Uses secondary sort by handler_id within priority for determinism
3. Records decisions only when the leader changes (avoiding noise in metadata)
4. Breaks after priority since it provides complete ordering

---

478-520: LGTM - Cycle detection is well-implemented.

The DFS-based cycle detection correctly extracts the cycle path and returns early after finding the first blocking cycle, which aligns with the fail-fast design principle documented in the module docstring.

---

650-714: LGTM - Phase assignment correctly documented.

The current implementation assigns all handlers to the EXECUTE phase with a clear docstring explaining the limitation and future enhancement path. The scaffolding for other phases is in place for when handler contract hints are supported.

---

549-551: The in-degree calculation at lines 549-551 is semantically correct.

The calculation counts elements in graph.edges[node], which contains the dependencies of each node (nodes that must run before it). This correctly computes the in-degree for Kahn's topological sort algorithm. The algorithm properly uses graph.reverse_edges to update dependents when a node is processed, ensuring dependencies are honored.

src/omnibase_core/models/execution/model_tie_breaker_decision.py (1)

1-89: LGTM!

The model correctly enforces that tie-breaker decisions require at least 2 handlers (min_length=2), uses PEP 604 union syntax, and has proper immutability configuration.

src/omnibase_core/models/execution/model_resolution_metadata.py (1)

1-213: LGTM!

The model is well-structured with:

• Proper Literal types constraining valid values for strategy and tie_breaker_order
• Helpful convenience methods (tie_breaker_count(), had_ties(), get_resolution_duration())
• Fallback duration calculation from timestamps when explicit duration is not set
• PEP 604 union syntax throughout

src/omnibase_core/models/execution/model_execution_conflict.py (2)

138-153: LGTM - Model validator correctly uses string annotation.

The validate_cycle_path_for_cycle_type validator correctly uses a string annotation ("ModelExecutionConflict") for the return type, which is the appropriate pattern for self-referential types in Pydantic models when not using from __future__ import annotations.

---

155-212: LGTM - Helper methods are well-implemented.

The convenience methods correctly handle:

• is_blocking(): severity-based blocking check
• involves_handler(): checks both handler_ids and cycle_path
• get_cycle_length(): correctly accounts for the repeated start node in cycle paths

tests/unit/resolution/test_execution_resolver.py (4)

23-24: LGTM - Module-level pytest marker correctly applied.

Using pytestmark = pytest.mark.unit at module level properly applies the unit test marker to all tests in the file, addressing the coding guidelines requirement.

---

133-255: LGTM - Basic ordering tests are comprehensive.

The tests cover essential ordering patterns: simple dependencies, chains, multiple dependencies, requires_after constraints, and diamond dependency patterns. The assertions correctly verify relative ordering without being overly brittle about exact positions.

---

262-350: LGTM - Cycle detection tests are thorough.

The tests correctly cover:

• Direct cycles (A → B → A)
• Indirect cycles (A → B → C → A)
• Self-references (correctly filtered as non-cycles)

The assertions verify conflict types, severity, and cycle path contents.

---

786-808: Good test for order independence.

This test verifies that the resolver produces deterministic output regardless of input order, which is critical for the pure function guarantee.

src/omnibase_core/models/execution/model_execution_plan.py (3)

150-167: LGTM - Resolution fields are well-designed.

The new fields for OMN-1106 are properly typed with sensible defaults:

• resolution_metadata: Optional metadata for plans that haven't gone through the resolver
• conflicts: Empty list by default for valid plans
• is_valid: Defaults to True for backward compatibility

---

331-345: LGTM - can_execute() correctly combines all checks.

The method properly enforces that a plan can only execute when:

1. is_valid is True
2. No blocking conflicts exist
3. Plan is not empty

This provides a single, clear API for execution readiness checks.

---

269-329: LGTM - Resolution helper methods are consistent.

The methods mirror the corresponding methods on ModelExecutionConflict:

• has_conflicts() / has_blocking_conflicts() for quick checks
• get_blocking_conflicts() / get_warnings() / get_cycle_conflicts() for filtered access
• had_tie_breakers() delegates to resolution_metadata

[claude]

PR Review: ExecutionResolver for Pure Execution Order Resolution (OMN-1106)

Executive Summary

This is an excellent, well-architected implementation of a pure function execution order resolver. The code demonstrates strong software engineering principles and aligns well with the ONEX framework patterns. I recommend approval with minor suggestions for consideration.

Overall Assessment: ✅ APPROVE

---

Strengths 🎉

1. Architectural Excellence

• Pure function design: Stateless, deterministic, no side effects - exactly as specified
• Fail-fast approach: Returns invalid plans with conflicts instead of throwing exceptions
• Clear separation of concerns: Protocol definition, models, implementation cleanly separated
• Thread-safe: Stateless resolver design ensures safe concurrent use

2. Code Quality

• Type safety: Full mypy strict compliance with comprehensive type annotations
• Immutable models: All data models use frozen=True for thread safety
• Documentation: Excellent docstrings with examples, version tags, and cross-references
• Error handling: Uses centralized exception groups and structured error models
• Naming conventions: Follows ONEX patterns (Model*, Protocol*, Enum*)

3. Algorithm Implementation

• Kahn's algorithm: Proper topological sort with O(V+E) complexity
• Cycle detection: DFS-based with clear cycle path reporting
• Deterministic tie-breaking: Priority + alphabetical with metadata tracking
• Index optimization: O(1) lookups via capability/tag indices

4. Testing Coverage

• 34 comprehensive tests covering all major scenarios
• Performance tests with clear thresholds and benchmarks
• Edge cases: Empty plans, cycles, missing dependencies, tie-breaking
• Determinism verification: Tests ensure same inputs → same outputs

5. ONEX Framework Alignment

• Uses protocol-driven patterns (ProtocolExecutionResolver)
• Proper model organization under models/execution/
• Follows module export conventions in init.py
• Version tracking (_RESOLVER_VERSION = "0.4.1")

---

Code Quality Observations

Best Practices ✅

1. Proper use of from_attributes=True in value objects for pytest-xdist compatibility
2. Centralized constants: _RESOLVER_VERSION for tracking
3. Private implementation details: Internal helpers prefixed with underscore
4. Comprehensive validation: model_validator for cycle_path validation
5. Helper methods: Rich API on models (is_blocking, involves_handler, get_cycle_length)

Potential Improvements 🔍

1. Type Safety - Dictionary Default Factory

Location: src/omnibase_core/resolution/_dependency_graph.py:23-25

Current implementation uses defaultdict but annotates as dict. This can cause type confusion.

Suggestion: Use typing.DefaultDict for accurate type annotations

Impact: Low - works correctly but reduces type confusion

---

2. Cycle Detection - Fail-Fast Documentation

Location: src/omnibase_core/resolution/execution_resolver.py:480-514

✅ Well documented! The fail-fast behavior is clearly explained in both the method docstring and ModelExecutionConflict docstring.

Observation: This is actually a strength - excellent documentation of design decisions. The note about Tarjan's SCC algorithm for future enhancement shows good architectural thinking.

---

3. Performance Consideration - Sorted Iteration

Location: src/omnibase_core/resolution/execution_resolver.py:526, 542, 617

Observation: Sorting is used for determinism, which is correct. However, for large graphs (1000+ nodes), this adds O(n log n) overhead at multiple points.

Suggestion for future optimization (not blocking):

• Consider pre-sorting graph.nodes once after graph construction
• Store edges as sorted lists instead of sets if determinism is always required
• Current approach is fine for typical use cases (< 100 handlers)

Impact: Very low - current performance is acceptable per test suite

---

4. Strict Mode Implementation

Location: src/omnibase_core/models/contracts/model_execution_ordering_policy.py:41-47

✅ Excellent design choice! The strict_mode field allows users to control whether missing dependencies are errors or warnings. The three-level override (parameter → policy → default) is well thought out.

Best practice - clear precedence hierarchy with sensible defaults.

---

Security Considerations 🔒

✅ No Security Concerns Found

1. No external I/O: Pure function, no file/network access
2. No code execution: Only data structure manipulation
3. Input validation: Pydantic models validate all inputs
4. DoS protection: Performance tests ensure O(V+E) complexity
5. No injection risks: String operations are safe (constraint parsing)

---

Performance Analysis ⚡

Algorithmic Complexity

• Graph construction: O(V + E) where V=handlers, E=constraints
• Cycle detection: O(V + E) with early termination
• Topological sort: O(V + E) using Kahn's algorithm
• Tie-breaking: O(k log k) where k=tied handlers (typically small)

Overall: O(V + E) - optimal for this problem ✅

Performance Tests

Location: tests/unit/resolution/test_execution_resolver_performance.py

• Tests scale from 10 to 1000+ handlers
• Timeout protection at 60 seconds
• Marked appropriately for CI filtering
• Validates linear scaling (not exponential)

Excellent coverage of performance characteristics! 🌟

---

Test Coverage Assessment 📊

Test Files

1. test_execution_resolver.py: 1801 lines, 34 tests

   • Basic ordering (chains, diamonds, multiple dependencies)
   • Cycle detection (direct, indirect, self-reference)
   • Tie-breaking (priority, alphabetical, combined)
   • Constraint resolution (capability, handler, tag)
   • Missing dependencies (strict/non-strict modes)
   • Determinism verification

2. test_execution_resolver_performance.py: 694 lines

   • Scale testing (10, 50, 100, 500, 1000 handlers)
   • Dense constraint graphs
   • Worst-case scenarios

Coverage Strengths ✅

• All major code paths covered
• Edge cases handled (empty, single handler, all independent)
• Both success and failure paths tested
• Protocol conformance verified

Potential Additions (optional)

• Fuzz testing with randomized constraint graphs
• Property-based testing using hypothesis library
• Integration tests with actual ModelHandlerContract instances from real use cases

Current coverage is excellent for a pure function implementation! 🎯

---

Documentation Quality 📚

Strengths

1. Module docstrings: Clear, with version tags and cross-references
2. Function docstrings: Args, Returns, Examples, See Also sections
3. Inline comments: Explain complex logic (cycle detection, tie-breaking)
4. Design notes: Fail-fast behavior, future enhancements documented
5. Protocol documentation: Excellent usage examples in protocol definition

Cross-References

• Links to OMN-1106, OMN-1108, OMN-1117, OMN-1125 ✅
• Links to related models and protocols ✅
• Version tags (versionadded 0.4.1) ✅

Documentation is comprehensive and well-organized! 📖

---

ONEX Framework Compliance ✅

Follows CLAUDE.md Requirements

Requirement	Status	Evidence
Use Poetry for dependencies	✅	All imports from managed packages
Protocol-driven DI	✅	ProtocolExecutionResolver defined
Pydantic models	✅	All models use BaseModel
frozen=True for immutability	✅	All models configured correctly
from_attributes=True	✅	Used in value objects
Type safety (mypy strict)	✅	Full type annotations
Exception handling	✅	Structured error patterns
Testing (pytest)	✅	Comprehensive test suite
Version tracking	✅	_RESOLVER_VERSION constant
Module exports	✅	Proper all declarations

100% ONEX compliance! 🎖️

---

Recommendations

For Merging (Required)

1. ✅ Approve and merge - code is production-ready

For Future PRs (Optional Enhancements)

1. Type annotation refinement: Use DefaultDict type for _DependencyGraph (low priority)
2. Performance optimization: Pre-sort graph nodes if scale increases beyond 1000 handlers (future)
3. Property-based testing: Add hypothesis tests for randomized constraint graphs (nice-to-have)
4. Tarjan's SCC algorithm: For comprehensive cycle detection when needed (future enhancement)

---

Final Verdict

APPROVED ✅

This is a production-ready, high-quality implementation that:

• Solves the problem correctly and efficiently
• Follows ONEX framework patterns meticulously
• Has excellent test coverage and documentation
• Demonstrates strong software engineering principles
• Is well-positioned for future enhancements

Excellent work on OMN-1106! The pure function design, comprehensive testing, and attention to detail are exemplary. This sets a high standard for future ONEX implementations.

---

Test Plan Verification

• 34 comprehensive tests covering all scenarios
• Performance thresholds met (< 1 second for typical cases)
• All major code paths covered
• Edge cases and error conditions tested
• Determinism verified through repeated runs

---

Reviewed by: Claude Code Assistant
Review Date: 2026-01-03
PR: #307 (OMN-1106)
Files Changed: 18 (+4817/-11)
Test Coverage: Excellent (34 tests + performance suite)

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

src/omnibase_core/resolution/execution_resolver.py (2)

692-756: Consider documenting phase assignment extension point.

The current implementation assigns all handlers to the EXECUTE phase. The comment on lines 702-705 mentions future support for phase hints, but there's no TODO or tracking reference. Consider adding a comment with the related ticket number for future phase-based assignment.

Suggested documentation improvement

     def _assign_to_phases(
         self,
         sorted_handlers: list[str],
         profile_phases: list[str],
         handler_info_map: dict[str, _HandlerInfo],
     ) -> list[ModelPhaseStep]:
         """
         Assign handlers to execution phases.

         Currently, all handlers are assigned to the EXECUTE phase since phase
         hints are not yet supported in ModelHandlerContract.execution_constraints.
         Future versions may support explicit phase assignment via handler contract
         hints, allowing handlers to declare their target phase (e.g., PREFLIGHT
         for validation, FINALIZE for cleanup).

+        TODO(OMN-XXXX): Implement phase hint support in execution_constraints
+        to allow handlers to declare target phases.
+
         Args:

---

586-623: In-degree naming is non-standard but code is correct.

The in-degree calculation at lines 591-593 correctly counts dependencies from graph.edges[node], where edges[A] stores nodes that A depends on. This implements Kahn's algorithm correctly: nodes with zero dependencies (in-degree = 0) process first, and the algorithm properly updates dependents' in-degrees via reverse_edges.

However, the variable name "in-degree" is semantically misleading. In standard graph terminology, in-degree counts incoming edges (predecessors); here it counts outgoing dependency edges. Consider renaming to dependency_count or blocked_by_count for clarity.

tests/unit/resolution/test_execution_resolver.py (1)

90-125: Consider: Extract shared helper to avoid duplication.

The _create_contract() helper function is duplicated between this file and test_execution_resolver_performance.py (lines 86-135). Consider extracting it to a shared test utilities module (e.g., tests/conftest.py or tests/unit/resolution/conftest.py) to reduce duplication and ensure consistency.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1408389 and 0fe3617.

📒 Files selected for processing (7)

• src/omnibase_core/models/contracts/model_execution_ordering_policy.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/protocols/resolution/protocol_execution_resolver.py
• src/omnibase_core/resolution/execution_resolver.py
• tests/unit/resolution/test_execution_resolver.py
• tests/unit/resolution/test_execution_resolver_performance.py

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Use PEP 604 union syntax for type annotations (e.g., str | None instead of Optional[str])
Resolve services using protocol names (e.g., container.get_service('ProtocolEventBus')) never using concrete class names
Order exception tuples alphabetically by exception class name (e.g., except (AttributeError, TypeError, ValidationError) not except (ValueError, TypeError, AttributeError))
Format code using poetry run black and sort imports using poetry run isort
Lint code with poetry run ruff check and fix automatically with poetry run ruff check --fix

Files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/protocols/resolution/protocol_execution_resolver.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/contracts/model_execution_ordering_policy.py
• tests/unit/resolution/test_execution_resolver_performance.py
• src/omnibase_core/resolution/execution_resolver.py
• tests/unit/resolution/test_execution_resolver.py

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/omnibase_core/**/*.py: Use ModelOnexError with error codes from EnumCoreErrorCode for exception handling, never generic Exception
Use @standard_error_handling decorator from omnibase_core.decorators to eliminate try/catch boilerplate
Add standardized exception handling comment markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) when using broad exception handlers
Use centralized exception group constants from omnibase_core.errors.exception_groups instead of individual exception types
Ensure all source code passes mypy strict mode with zero errors and pyright basic mode validation
Use FileRegistry for loading YAML contracts and configuration files instead of direct file reading
Use duck typing with protocols for service resolution - never use isinstance checks for protocol-based dependencies

Files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/protocols/resolution/protocol_execution_resolver.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/contracts/model_execution_ordering_policy.py
• src/omnibase_core/resolution/execution_resolver.py

src/omnibase_core/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

src/omnibase_core/models/**/*.py: Add from_attributes=True to ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution
Do NOT use from __future__ import annotations in Pydantic model files (use only for forward references and circular import prevention)

Files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_execution_conflict.py
• src/omnibase_core/models/contracts/model_execution_ordering_policy.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) to classify test types
All tests must complete within 60-second timeout protection to ensure CI performance

Files:

• tests/unit/resolution/test_execution_resolver_performance.py
• tests/unit/resolution/test_execution_resolver.py

🧠 Learnings (4)

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to src/omnibase_core/models/**/*.py : Do NOT use `from __future__ import annotations` in Pydantic model files (use only for forward references and circular import prevention)

Applied to files:

• src/omnibase_core/models/execution/model_resolution_metadata.py
• src/omnibase_core/models/execution/model_execution_conflict.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to src/omnibase_core/models/**/*.py : Add `from_attributes=True` to ConfigDict for immutable value objects nested in other Pydantic models to support pytest-xdist parallel execution

Applied to files:

• src/omnibase_core/models/execution/model_execution_conflict.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to **/*.py : Use PEP 604 union syntax for type annotations (e.g., `str | None` instead of `Optional[str]`)

Applied to files:

• src/omnibase_core/models/execution/model_execution_conflict.py

📚 Learning: 2026-01-02T20:42:17.758Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-02T20:42:17.758Z
Learning: Applies to tests/**/*.py : Use pytest markers (pytest.mark.unit, pytest.mark.integration, pytest.mark.slow, pytest.mark.smoke, pytest.mark.performance) to classify test types

Applied to files:

• tests/unit/resolution/test_execution_resolver.py

🧬 Code graph analysis (5)

src/omnibase_core/models/execution/model_resolution_metadata.py (1)

src/omnibase_core/models/execution/model_tie_breaker_decision.py (1)

• ModelTieBreakerDecision (23-84)

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (4)

src/omnibase_core/models/contracts/model_execution_profile.py (1)

• ModelExecutionProfile (25-53)

src/omnibase_core/models/contracts/model_handler_contract.py (1)

• ModelHandlerContract (61-416)

src/omnibase_core/models/execution/model_execution_plan.py (1)

• ModelExecutionPlan (41-368)

src/omnibase_core/resolution/execution_resolver.py (1)

• resolve (98-198)

src/omnibase_core/models/execution/model_execution_conflict.py (1)

src/omnibase_core/models/errors/model_onex_error.py (1)

• message (248-250)

tests/unit/resolution/test_execution_resolver_performance.py (6)

src/omnibase_core/models/contracts/model_execution_constraints.py (1)

• ModelExecutionConstraints (39-206)

src/omnibase_core/models/contracts/model_execution_ordering_policy.py (1)

• ModelExecutionOrderingPolicy (13-54)

src/omnibase_core/models/contracts/model_execution_profile.py (1)

• ModelExecutionProfile (25-53)

src/omnibase_core/models/contracts/model_handler_contract.py (1)

• ModelHandlerContract (61-416)

src/omnibase_core/resolution/execution_resolver.py (2)

• ExecutionResolver (62-954)
• resolve (98-198)

src/omnibase_core/models/execution/model_execution_plan.py (2)

• total_handlers (209-220)
• get_all_handler_ids (190-207)

tests/unit/resolution/test_execution_resolver.py (5)

src/omnibase_core/models/contracts/model_execution_ordering_policy.py (1)

• ModelExecutionOrderingPolicy (13-54)

src/omnibase_core/models/contracts/model_execution_profile.py (1)

• ModelExecutionProfile (25-53)

src/omnibase_core/resolution/execution_resolver.py (2)

• ExecutionResolver (62-954)
• resolve (98-198)

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (2)

• resolve (149-262)
• ProtocolExecutionResolver (75-262)

src/omnibase_core/models/execution/model_execution_plan.py (3)

• get_all_handler_ids (190-207)
• is_empty (222-237)
• total_handlers (209-220)

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

• GitHub Check: claude-review

🔇 Additional comments (22)

src/omnibase_core/models/execution/model_resolution_metadata.py (3)

79-84: LGTM! Model configuration follows best practices.

The ConfigDict correctly includes from_attributes=True for pytest-xdist parallel execution support, frozen=True for immutability, and appropriate validation settings. This aligns with coding guidelines for Pydantic models.

---

163-179: LGTM! Helper methods are clean and correct.

The tie_breaker_count() and had_ties() methods provide convenient access to tie-breaker decision data with straightforward implementations.

---

181-193: LGTM! Duration computation handles edge cases correctly.

The get_resolution_duration() method correctly prioritizes the explicit resolution_duration_ms field, then falls back to computing from timestamps, and returns None if neither is available. Good defensive design.

src/omnibase_core/models/contracts/model_execution_ordering_policy.py (1)

41-47: LGTM! New strict_mode field is well-designed.

The field maintains backward compatibility with default=False, has a clear description explaining the fail-fast behavior for missing dependencies, and integrates well with the existing policy model.

src/omnibase_core/models/execution/model_execution_conflict.py (3)

100-105: LGTM! Model configuration follows best practices.

The ConfigDict correctly includes from_attributes=True as required by coding guidelines, with appropriate immutability and validation settings.

---

155-170: LGTM! Validator correctly enforces cycle_path requirement.

The model_validator ensures that cycle_path is provided when conflict_type is "cycle", which is essential for proper cycle conflict representation.

---

206-216: LGTM! Cycle length calculation is correct.

The get_cycle_length() method correctly accounts for the cycle path format where the starting node appears at both the beginning and end, subtracting 1 to get the unique handler count.

src/omnibase_core/protocols/resolution/protocol_execution_resolver.py (3)

56-72: LGTM! Appropriate use of from __future__ import annotations and TYPE_CHECKING.

This is a Protocol file (not a Pydantic model file), so the from __future__ import annotations import is acceptable. The TYPE_CHECKING block correctly defers expensive imports to type-checking time only, which is the recommended pattern for Protocol definitions.

---

74-75: LGTM! Protocol is correctly marked as @runtime_checkable.

This enables isinstance() checks against the protocol, which is used in the test suite to verify protocol compliance of the ExecutionResolver implementation.

---

149-154: LGTM! Protocol method signature is well-defined.

The resolve method signature correctly uses PEP 604 union syntax (bool | None) and provides a clear contract for implementations.

tests/unit/resolution/test_execution_resolver_performance.py (4)

143-147: LGTM! Test class correctly uses required pytest markers.

The test class properly applies @pytest.mark.performance, @pytest.mark.slow, @pytest.mark.timeout(60), and @pytest.mark.unit as required by coding guidelines. The 60-second timeout ensures CI performance per guidelines.

---

254-274: Verify: Circular dependencies are intentionally created in this stress test.

The wrapping logic dep_idx = (i - offset) % num_handlers intentionally creates circular dependencies to test cycle detection performance. The test correctly anticipates this with the conditional check on line 284-288 and validates that even with conflicts, resolution completes within the threshold.

---

612-614: LGTM! Determinism test class uses appropriate markers.

The class correctly uses @pytest.mark.performance and @pytest.mark.unit markers to classify the test type per coding guidelines.

---

676-685: Consider: Variance threshold may be flaky in CI environments.

The 50% standard deviation threshold (std_dev < mean_duration * 0.5) could be sensitive to CI environment variability. While this is reasonable for performance consistency validation, consider that cold starts, garbage collection, or resource contention might occasionally cause false failures.

The current implementation is acceptable given the purpose of detecting algorithmic complexity issues rather than precise timing.

src/omnibase_core/resolution/execution_resolver.py (3)

62-96: LGTM! Class design follows pure function principles.

The ExecutionResolver class is correctly designed as stateless and thread-safe with no instance variables. Each call to resolve() operates independently on inputs, adhering to the documented pure function design principles.

---

480-564: LGTM! Cycle detection with fail-fast behavior.

The DFS-based cycle detection is correctly implemented with:

• Visited set for global traversal tracking
• Recursion stack for detecting back edges within the current path
• Path tracking for extracting the cycle when found
• Deterministic traversal order via sorted() for reproducibility
• Fail-fast return after first cycle detection as documented

---

625-686: LGTM! Tie-breaking logic is deterministic and well-structured.

The tie-breaking implementation correctly:

• Applies tie-breakers in priority order from the policy
• Uses secondary sort by handler_id for stability within priority sorting
• Records decisions only when the leader changes
• Always returns sorted candidates for determinism

tests/unit/resolution/test_execution_resolver.py (5)

23-24: LGTM! Module-level pytest marker correctly applied.

Using pytestmark = pytest.mark.unit at module level efficiently applies the @pytest.mark.unit marker to all tests in the file, addressing the previous review comment.

---

351-416: LGTM! Fail-fast cycle detection test is well-documented.

The test test_multiple_cycles_only_first_reported thoroughly documents the fail-fast behavior with clear comments explaining:

• Why only one cycle is reported
• The alphabetical traversal order for determinism
• The expected user workflow (fix and re-run)

This aligns with the documented behavior in ModelExecutionConflict and _detect_cycles.

---

729-930: LGTM! Comprehensive strict mode test coverage.

The TestStrictMode class provides thorough coverage of the strict_mode parameter including:

• Missing handler/capability/tag references as errors
• Profile-level strict_mode setting
• Explicit parameter overriding profile setting
• Multiple missing dependencies
• Valid dependencies in strict mode

This validates the full matrix of strict mode behavior.

---

1147-1171: LGTM! Protocol compliance tests verify structural conformance.

The tests correctly verify that ExecutionResolver implements ProtocolExecutionResolver using isinstance() check (enabled by @runtime_checkable) and that the method signature matches the protocol.

---

1753-1773: LGTM! Statistics validation ensures consistency.

The test test_statistics_matches_decision_count correctly verifies that the sum of tie-breaker statistics equals the number of recorded decisions, ensuring internal consistency of the metadata.