feat(models): OMN-1049 - Create typed context models for ModelActionPayload

[jonahgabriel]

Summary

Create 4 new typed context models to replace dict[str, ModelSchemaValue] fields in ModelActionPayload:

• ModelActionExecutionContext: action/node execution context with UUID workflow_id, environment settings, timeout/retry policies
• ModelRoutingMetadata: service routing with load balancing strategy, UUID sticky_session_id, circuit breaker settings
• ModelServiceDiscoveryMetadata: service discovery with capabilities, dependencies, UUID service_instance_id, protocol validation
• ModelActionParameters: typed action parameters with ModelSemVer action_version, validation flags, @allow_dict_str_any extensions

Changes

File	Change
model_action_execution_context.py	New - execution context model
model_routing_metadata.py	New - routing metadata model
model_service_discovery_metadata.py	New - service discovery model
model_action_parameters.py	New - action parameters model
context/__init__.py	Updated - export new models
model_action_payload.py	Updated - use typed models

Type Improvements

• ID fields use UUID instead of str (workflow_id, service_instance_id, sticky_session_id)
• Version fields use ModelSemVer instead of str (action_version)
• Extensions dict uses @allow_dict_str_any decorator with justification

Before/After

# BEFORE (untyped)
parameters: dict[str, ModelSchemaValue]
execution_context: dict[str, ModelSchemaValue]
routing_metadata: dict[str, ModelSchemaValue]
service_metadata: dict[str, ModelSchemaValue]

# AFTER (typed)
parameters: ModelActionParameters
execution_context: ModelActionExecutionContext
routing_metadata: ModelRoutingMetadata
service_metadata: ModelServiceDiscoveryMetadata | None

Test plan

• All models import successfully
• All models instantiate with defaults and custom values
• Field validators correctly reject invalid values
• All models frozen (immutable, thread-safe)
• mypy strict mode passes
• All pre-commit hooks pass

Resolves: OMN-1049

Summary by CodeRabbit

• New Features

  • Added four immutable context models: action execution, action parameters, routing metadata, and service discovery.
  • Added payload fields: parent correlation id, execution chain, target service, trust level, and tool discovery tags.

• Refactor

  • Payload now uses strongly typed, immutable context models instead of generic dictionaries for clearer, safer metadata handling.

• Tests

  • Added comprehensive unit tests for validation, serialization, immutability, inheritance, and edge cases.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

Added four frozen Pydantic context models (action execution, action parameters, routing metadata, service discovery), exported them from the context package, replaced several dict-typed fields in ModelActionPayload with these typed models, and added comprehensive unit tests for the new models and payload behavior.

Changes

Cohort / File(s)	Summary
Action / Execution Models src/omnibase_core/models/context/model_action_execution_context.py, src/omnibase_core/models/context/model_action_parameters.py	Added frozen Pydantic models: ModelActionExecutionContext (node/workflow ids, environment, timeout_ms, retry_count/max_retries, dry_run/debug/trace, correlation_id) and ModelActionParameters (action_name/version, idempotency_key, timeout_override_ms, input/output paths, format, validate flags, extensions). Both forbid extras, support from_attributes, and are exported.
Routing & Service Discovery Models src/omnibase_core/models/context/model_routing_metadata.py, src/omnibase_core/models/context/model_service_discovery_metadata.py	Added frozen Pydantic models: ModelRoutingMetadata (target_region, preferred_instance, validated LoadBalanceStrategy, sticky_session_id, priority, weight, timeout_override_ms, circuit_breaker_enabled; also exports LoadBalanceStrategy and VALID_LOAD_BALANCE_STRATEGIES) and ModelServiceDiscoveryMetadata (service_name/version/instance_id, validated protocol, health_check_url validation, capabilities, dependencies, tags). Both forbid extras and are exported.
Context Module Exports src/omnibase_core/models/context/__init__.py	Imported and re-exported the four new models plus LoadBalanceStrategy and VALID_LOAD_BALANCE_STRATEGIES; updated __all__ and adjusted header grouping to Action/Routing context.
Core Payload Typing src/omnibase_core/models/core/model_action_payload.py	Replaced dict-typed metadata fields with typed models: parameters → ModelActionParameters, execution_context → ModelActionExecutionContext, routing_metadata → ModelRoutingMetadata, service_metadata → `ModelServiceDiscoveryMetadata
Unit Tests tests/unit/models/context/*.py, tests/unit/models/core/test_model_action_payload.py	Added extensive unit tests for each new model and the updated payload covering instantiation, validation, immutability, serialization/deserialization, equality/hashing, from_attributes/dataclass construction, model_copy/model_dump, edge cases, and integration/round-trip scenarios.

Sequence Diagram(s)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• refactor(models): replace dict[str, str] metadata with typed context models (#OMN-1048) #246 — Refactors untyped dict-based context metadata into typed, frozen Pydantic context models and updates context package exports (strong overlap at module API/export level).

Poem

🐇 I hopped from loose dict fields to rows that shine,
Typed models snug, each field in line.
Execution, params, routing, service too,
Immutable carrots in a tidy queue.
✨🥕

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 directly and clearly summarizes the main change: introducing typed context models (ModelActionExecutionContext, ModelRoutingMetadata, ModelServiceDiscoveryMetadata, ModelActionParameters) to replace untyped dict fields in ModelActionPayload for OMN-1049.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feature/omn-1049-typed-context-models

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 53c4e82 and 7c96a69.

📒 Files selected for processing (1)

• tests/unit/models/core/test_model_action_payload.py

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

• tests/unit/models/core/test_model_action_payload.py

⏰ 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). (20)

• GitHub Check: Tests (Split 13/20)
• GitHub Check: Tests (Split 12/20)
• GitHub Check: Tests (Split 17/20)
• GitHub Check: Tests (Split 19/20)
• GitHub Check: Tests (Split 8/20)
• GitHub Check: Tests (Split 7/20)
• GitHub Check: Tests (Split 2/20)
• GitHub Check: Tests (Split 20/20)
• GitHub Check: Tests (Split 11/20)
• GitHub Check: Tests (Split 10/20)
• GitHub Check: Tests (Split 15/20)
• GitHub Check: Tests (Split 9/20)
• GitHub Check: Tests (Split 3/20)
• GitHub Check: Tests (Split 14/20)
• GitHub Check: Tests (Split 1/20)
• GitHub Check: Tests (Split 18/20)
• GitHub Check: Tests (Split 16/20)
• GitHub Check: Tests (Split 5/20)
• GitHub Check: Tests (Split 6/20)
• GitHub Check: claude-review

---

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

[claude]

Code Review: Typed Context Models for ModelActionPayload

Thank you for this well-structured PR that introduces strong typing to replace dict[str, ModelSchemaValue] fields. The overall direction is excellent - this improves type safety, developer experience, and code maintainability.

✅ Strengths

1. Excellent Documentation: All four new models have comprehensive docstrings with clear examples, thread safety notes, and attribute descriptions
2. Strong Type Safety: Proper use of UUID, ModelSemVer, and Literal types instead of generic strings
3. Validation: Good use of field validators for environment, load_balance_strategy, protocol, and health_check_url
4. Immutability: All new context models use frozen=True and from_attributes=True following ONEX patterns
5. Sensible Defaults: All fields have appropriate defaults, avoiding required field proliferation
6. Pattern Compliance: Proper use of @allow_dict_str_any decorator with justification for extensions field

⚠️ Critical Issues

1. Thread Safety Violation in ModelActionPayload (src/omnibase_core/models/core/model_action_payload.py:84-86)

def add_to_execution_chain(self, action_name: str) -> None:
    """Add action to execution chain for composition tracking."""
    self.execution_chain.append(action_name)

Problem: This method mutates a list on an instance that is NOT frozen. According to CLAUDE.md, models should be immutable (frozen=True) for thread safety, especially when used in distributed/concurrent scenarios.

Evidence:

• The new context models (ModelActionExecutionContext, etc.) are all frozen=True
• CLAUDE.md states: "All models in this module are frozen (immutable) after creation."
• The parent class ModelOnexInputState does NOT have frozen=True in its ConfigDict

Impact: This creates a race condition if multiple threads/tasks call add_to_execution_chain() on the same payload instance.

Recommendation:

• Either make ModelActionPayload frozen and remove mutating methods
• OR add explicit thread safety warnings to the docstring
• OR use a different pattern (e.g., return new instance with updated chain)

2. Missing Test Coverage

Problem: I searched the test suite and found zero tests for the four new models:

grep -r "ModelActionExecutionContext|ModelActionParameters|ModelRoutingMetadata|ModelServiceDiscoveryMetadata" tests/
# Returns: No files found

Impact: The PR description claims tests pass, but there are no unit tests validating:

• Field validation (environment, load_balance_strategy, protocol, health_check_url validators)
• Default value behavior
• Immutability (frozen=True enforcement)
• Edge cases (empty strings, invalid UUIDs, boundary values)
• Integration with ModelActionPayload

Recommendation: Add comprehensive unit tests in tests/unit/models/context/:

• test_model_action_execution_context.py
• test_model_action_parameters.py
• test_model_routing_metadata.py
• test_model_service_discovery_metadata.py

Tests should cover:

• Valid instantiation with defaults
• Valid instantiation with custom values
• Validator edge cases (invalid environment, invalid protocol, malformed URLs)
• Frozen/immutability enforcement
• from_attributes=True compatibility (pytest-xdist scenario)

📋 Minor Issues

3. Inconsistent Timeout Units (model_action_execution_context.py vs model_routing_metadata.py)

# ModelActionExecutionContext
timeout_seconds: int = Field(default=30, ge=1, ...)  # ✅ seconds

# ModelRoutingMetadata  
timeout_override_ms: int | None = Field(default=None, ...)  # ⚠️ milliseconds

Suggestion: Consider using consistent units (seconds) across all timeout fields, or add a comment explaining why milliseconds are used for routing overrides.

4. Validator Type Checking Order (model_service_discovery_metadata.py:136-138)

if not isinstance(v, str):
    raise ValueError(f"Protocol must be a string, got {type(v).__name__}")
normalized = v.lower().strip()

Minor Issue: The validator performs type checking, but Pydantic already enforces the protocol: str type annotation. This is redundant but harmless (defensive programming is acceptable).

Suggestion: Consider removing isinstance checks in validators since Pydantic handles type coercion before validators run. However, keeping them adds clarity for non-string inputs.

5. Environment Validator Inconsistency (model_action_execution_context.py:145-146)

if isinstance(v, str):
    v = v.lower()

This only lowercases if already a string, but the field is typed as str so this will always be true. Consider simplifying to:

v = v.lower()

🔍 Questions/Clarifications

1. service_metadata vs service_discovery_metadata: In ModelActionPayload, the field is named service_metadata but typed as ModelServiceDiscoveryMetadata. Should the field name be service_discovery_metadata for clarity?

2. Why is service_metadata optional but routing_metadata required? What's the use case for routing without service discovery metadata?

3. ModelSemVer for action_version: Should service_version in ModelServiceDiscoveryMetadata also use ModelSemVer instead of str for consistency?

📊 Coverage Impact

The PR adds 641 lines but zero test lines. With the project's 60% coverage requirement and 12,000+ test suite, this could impact overall coverage metrics. Recommend adding ~400-500 lines of tests to maintain coverage ratio.

✅ Approval Recommendations

Do NOT merge until:

1. ✅ Thread safety issue in add_to_execution_chain() is resolved
2. ✅ Comprehensive unit tests are added for all four models
3. ✅ CI passes (mypy strict + pytest + coverage)

Optional improvements:

• Address timeout unit inconsistency
• Clarify service_metadata naming
• Consider ModelSemVer for service_version

---

Overall Assessment: This is excellent architectural work that significantly improves type safety. The code quality is high, documentation is thorough, and the patterns follow ONEX conventions. The two critical blockers (thread safety + tests) must be addressed before merge, but the foundation is solid.

Great work! 🚀

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (4)

src/omnibase_core/models/context/model_action_execution_context.py (1)

131-152: Validator return type mismatch with Literal field type.

The validate_environment method returns str, but the field is typed as Literal["development", "staging", "production"]. While Pydantic handles this at runtime, for strict mypy compliance the return type should match.

🔎 Suggested type annotation fix

     @field_validator("environment", mode="before")
     @classmethod
-    def validate_environment(cls, v: str) -> str:
+    def validate_environment(cls, v: str) -> Literal["development", "staging", "production"]:
         """Validate that environment is one of the allowed values.
 
         Args:
             v: The environment string to validate.
 
         Returns:
             The validated environment string (lowercase).
 
         Raises:
             ValueError: If the value is not a valid environment.
         """
         if isinstance(v, str):
             v = v.lower()
         allowed = {"development", "staging", "production"}
         if v not in allowed:
             raise ValueError(
                 f"Invalid environment '{v}': must be one of {sorted(allowed)}"
             )
-        return v
+        return v  # type: ignore[return-value]

Note: Alternatively, since Literal already enforces valid values, the validator's main purpose is case-insensitivity. The # type: ignore may be needed since mypy can't narrow the type after the validation check.

src/omnibase_core/models/context/model_routing_metadata.py (1)

84-90: Consider using Literal type for load_balance_strategy for improved type safety.

The ModelActionExecutionContext.environment field uses Literal["development", "staging", "production"] for similar constrained string values. For consistency across the new models, load_balance_strategy could also use Literal:

🔎 Optional: Use Literal type for consistency

+from typing import Literal
+
+LoadBalanceStrategy = Literal["round_robin", "least_connections", "random", "weighted"]
+
 class ModelRoutingMetadata(BaseModel):
     ...
-    load_balance_strategy: str = Field(
+    load_balance_strategy: LoadBalanceStrategy = Field(
         default="round_robin",
         description=(
             "Load balancing strategy. Valid values: round_robin, least_connections, "
             "random, weighted"
         ),
     )

The validator would still be useful for case-insensitivity if needed, but Literal provides better IDE autocomplete and static type checking.

src/omnibase_core/models/context/model_service_discovery_metadata.py (2)

93-96: Consider using ModelSemVer for service_version for consistency.

ModelActionParameters.action_version uses ModelSemVer | None, but service_version here uses str | None. For type consistency across the typed context models and to leverage semantic version validation:

🔎 Suggested change for consistency

+from omnibase_core.models.primitives.model_semver import ModelSemVer
+
 class ModelServiceDiscoveryMetadata(BaseModel):
     ...
-    service_version: str | None = Field(
+    service_version: ModelSemVer | None = Field(
         default=None,
-        description="Semantic version of the service (e.g., '1.2.3').",
+        description="Semantic version of the service using ModelSemVer type.",
     )

Note: If string versions must be accepted from external sources, ModelSemVer.parse() can be used in a field_validator with mode="before".

---

181-184: Simplify redundant exception handling.

The except ValueError: raise pattern is redundant since it just re-raises the same exception. The generic except Exception block already handles other cases.

🔎 Simplified exception handling

         try:
             parsed = urlparse(v)
             # Validate scheme
             if parsed.scheme not in ("http", "https"):
                 raise ValueError(
                     f"Invalid health check URL '{v}': scheme must be 'http' or 'https', "
                     f"got '{parsed.scheme or '(empty)'}'"
                 )
             # Validate netloc (host[:port])
             if not parsed.netloc:
                 raise ValueError(f"Invalid health check URL '{v}': missing host")
             return v
-        except ValueError:
-            raise
         except Exception as e:
+            if isinstance(e, ValueError):
+                raise
             raise ValueError(f"Invalid health check URL '{v}': {e}") from e

Or simply remove lines 181-182 since the generic handler will convert non-ValueError exceptions appropriately.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 997a5d8 and 0b3a405.

📒 Files selected for processing (6)

• src/omnibase_core/models/context/__init__.py
• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/model_action_parameters.py
• src/omnibase_core/models/context/model_routing_metadata.py
• src/omnibase_core/models/context/model_service_discovery_metadata.py
• src/omnibase_core/models/core/model_action_payload.py

🧰 Additional context used

📓 Path-based instructions (3)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always use Poetry for Python package management and task execution - use poetry run for all Python commands, never use pip or python directly
Use protocol names (not concrete class names) for service resolution via container.get_service()
Use structured ModelOnexError with error codes instead of generic Exception
Use PEP 604 union syntax (str | None) instead of Optional/Union types
Use duck typing with protocols for service interfaces - never use isinstance checks
Format code with Black and sort imports with isort before committing

Files:

• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/__init__.py
• src/omnibase_core/models/core/model_action_payload.py
• src/omnibase_core/models/context/model_service_discovery_metadata.py
• src/omnibase_core/models/context/model_routing_metadata.py
• src/omnibase_core/models/context/model_action_parameters.py

**/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/models/**/*.py: Add from_attributes=True to ConfigDict for Pydantic value objects to support pytest-xdist parallel execution
Pydantic models must pass both mypy strict mode and pyright type checking

Files:

• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/__init__.py
• src/omnibase_core/models/core/model_action_payload.py
• src/omnibase_core/models/context/model_service_discovery_metadata.py
• src/omnibase_core/models/context/model_routing_metadata.py
• src/omnibase_core/models/context/model_action_parameters.py

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Run mypy in strict mode with 0 errors across all source files

Files:

• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/__init__.py
• src/omnibase_core/models/core/model_action_payload.py
• src/omnibase_core/models/context/model_service_discovery_metadata.py
• src/omnibase_core/models/context/model_routing_metadata.py
• src/omnibase_core/models/context/model_action_parameters.py

🧬 Code graph analysis (3)

src/omnibase_core/models/context/model_service_discovery_metadata.py (3)

src/omnibase_core/models/primitives/model_semver.py (1)

• parse (448-480)

src/omnibase_core/models/metadata/model_metadata_field_info.py (1)

• uuid (157-176)

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

• capabilities (149-151)

src/omnibase_core/models/context/model_routing_metadata.py (2)

src/omnibase_core/models/metadata/model_metadata_field_info.py (1)

• uuid (157-176)

src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)

• priority (90-92)

src/omnibase_core/models/context/model_action_parameters.py (1)

src/omnibase_core/models/primitives/model_semver.py (1)

• ModelSemVer (101-480)

⏰ 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). (16)

• GitHub Check: Tests (Split 19/20)
• GitHub Check: Tests (Split 9/20)
• GitHub Check: Tests (Split 17/20)
• GitHub Check: Tests (Split 14/20)
• GitHub Check: Tests (Split 20/20)
• GitHub Check: Tests (Split 2/20)
• GitHub Check: Tests (Split 15/20)
• GitHub Check: Tests (Split 7/20)
• GitHub Check: Tests (Split 18/20)
• GitHub Check: Tests (Split 12/20)
• GitHub Check: Tests (Split 16/20)
• GitHub Check: Tests (Split 1/20)
• GitHub Check: Tests (Split 3/20)
• GitHub Check: Tests (Split 11/20)
• GitHub Check: Tests (Split 5/20)
• GitHub Check: claude-review

🔇 Additional comments (7)

src/omnibase_core/models/context/model_action_execution_context.py (1)

82-82: LGTM on model configuration.

The ConfigDict properly includes from_attributes=True as required by the coding guidelines for Pydantic value objects, and the frozen/extra="forbid" settings ensure immutability and strict validation.

src/omnibase_core/models/context/model_action_parameters.py (1)

30-34: LGTM on decorator usage and class definition.

The @allow_dict_str_any decorator is properly applied with a clear justification for why the extensions field needs flexibility. This follows the established pattern for allowing arbitrary dictionaries in a controlled manner.

src/omnibase_core/models/context/__init__.py (1)

59-92: LGTM on public API exports.

The new models are properly imported and exported via __all__. The imports use direct module paths which helps avoid circular import issues, and the __all__ list maintains alphabetical ordering for consistency.

src/omnibase_core/models/core/model_action_payload.py (4)

13-21: LGTM on import strategy.

Using direct module imports instead of importing from context/__init__.py correctly avoids potential circular import issues. The comment explains the rationale clearly.

---

39-46: LGTM on typed field definitions.

The default_factory pattern is correctly used for the Pydantic model types, ensuring each instance gets a fresh default model rather than sharing a mutable reference.

---

94-101: LGTM on create_child_payload implementation.

The comment on line 99 correctly explains that no copy is needed since ModelServiceDiscoveryMetadata is frozen/immutable. The execution_chain.copy() on line 97 correctly copies the mutable list.

---

75-78: No action required. Verification shows that downstream consumers do not directly access attributes of service_metadata from ModelActionPayload instances. The field is treated as an opaque immutable value that is passed through payload creation and included in serialization without accessing its internal attributes. The change from default_factory to default=None is compatible with actual usage patterns in the codebase.

[claude]

Pull Request Review - OMN-1049: Typed Context Models

Overview

This PR successfully replaces untyped dict[str, ModelSchemaValue] fields in ModelActionPayload with four strongly-typed context models. The change significantly improves type safety and API clarity while following ONEX architectural patterns.

---

✅ Strengths

1. Excellent Type Safety Improvements

• Replaces generic dict[str, ModelSchemaValue] with explicit Pydantic models
• Uses proper typed primitives: UUID for IDs, ModelSemVer for versions, Literal for enums
• Field validators ensure runtime correctness (environment values, protocols, URLs, load balancing strategies)

2. Strong Adherence to ONEX Conventions

• All models use frozen=True (immutable, thread-safe per CLAUDE.md threading requirements)
• Proper use of from_attributes=True for pytest-xdist compatibility
• Follows extra="forbid" convention to prevent field pollution
• Comprehensive docstrings with field explanations and examples
• PEP 604 union syntax (str | None instead of Optional[str])

3. Well-Designed API

• ModelActionExecutionContext: Execution metadata (timeout, retry, environment, debug flags)
• ModelActionParameters: Typed action parameters with validated extensions field
• ModelRoutingMetadata: Load balancing, circuit breaker, sticky sessions
• ModelServiceDiscoveryMetadata: Service registration with capabilities and dependencies
• Sensible defaults minimize boilerplate while allowing full customization

4. Good Documentation

• Clear module docstrings explaining purpose and distinction from similar models
• Thread safety guarantees explicitly documented
• Inline examples demonstrate usage patterns
• Proper See Also cross-references

5. Proper Import Strategy

• Direct module imports in model_action_payload.py avoid circular import via context/__init__.py
• Public API exports through __init__.py maintain clean interface

---

⚠️ Issues & Recommendations

1. CRITICAL: Missing Test Coverage

# No tests found for new models
find tests/ -name "*action_execution_context*"  # Empty
grep -r "ModelActionExecutionContext" tests/    # No matches

Impact: Per CLAUDE.md, minimum 60% coverage required. These models are untested.

Action Required: Add comprehensive tests to tests/unit/models/context/test_context_models.py:

• Basic instantiation with defaults and custom values
• Field validation (invalid environment, protocol, URL, load balance strategy)
• Frozen behavior (immutability)
• from_attributes=True behavior
• JSON serialization/deserialization
• Edge cases (empty strings, boundary values, None handling)

Reference: Follow existing patterns in test_context_models.py:1-100 (SessionContextAttrs, HttpRequestMetadataAttrs helpers)

2. Potential Issue: Timeout Unit Inconsistency

# model_action_execution_context.py:100
timeout_seconds: int = Field(default=30, ge=1)

# model_action_parameters.py:103
timeout_override_seconds: int | None = Field(default=None, gt=0)

# model_routing_metadata.py:105
timeout_override_ms: int | None = Field(default=None)  # milliseconds\!

Problem: Mixing seconds and milliseconds creates confusion and potential bugs. timeout_override_ms lacks validation.

Recommendations:

1. Add validator to timeout_override_ms requiring gt=0
2. Consider documenting conversion factor or standardizing on one unit
3. Add validation test ensuring millisecond values are reasonable (e.g., < 3600000 for 1 hour max)

3. Minor: Field Validation Inconsistency

ModelRoutingMetadata.load_balance_strategy validator:

# Line 128: No type check before validation
if v not in VALID_LOAD_BALANCE_STRATEGIES:
    raise ValueError(...)

ModelServiceDiscoveryMetadata.protocol validator:

# Line 136: Explicit type check
if not isinstance(v, str):
    raise ValueError(...)

Recommendation: Add isinstance(v, str) check to validate_load_balance_strategy for consistency and better error messages.

4. Documentation: @allow_dict_str_any Justification

The @allow_dict_str_any decorator on ModelActionParameters is well-justified:

# Line 30-33
@allow_dict_str_any(
    "Extensions field intentionally allows flexible dict for domain-specific "
    "parameters that cannot be pre-defined."
)

Good: Proper use of the decorator for legitimate extensibility needs.

Minor Enhancement: Consider documenting in the class docstring that common parameters should NOT go in extensions but as explicit fields (already mentioned in field description, could reinforce in class docs).

5. ModelServiceDiscoveryMetadata: Missing Validation

# Line 88-91: service_name is required
service_name: str = Field(..., min_length=1)

# Line 105-114: Lists have default_factory=list
capabilities: list[str] = Field(default_factory=list)
dependencies: list[str] = Field(default_factory=list)

Potential Issue: No validation preventing empty strings in capabilities, dependencies, or tags lists.

Recommendation: Add list item validation:

@field_validator('capabilities', 'dependencies', 'tags', mode='after')
@classmethod
def validate_non_empty_strings(cls, v: list[str]) -> list[str]:
    if any(not item.strip() for item in v):
        raise ValueError("List items cannot be empty or whitespace")
    return v

6. ModelActionPayload: Mutability Warning

# model_action_payload.py:84-86
def add_to_execution_chain(self, action_name: str) -> None:
    """Add action to execution chain for composition tracking."""
    self.execution_chain.append(action_name)

Problem: ModelActionPayload inherits from ModelOnexInputState which may be frozen. If frozen, this method will fail at runtime.

Action Required: Verify ModelOnexInputState configuration. If frozen, remove this method or document that it only works with unfrozen instances.

---

🔍 Performance & Security

Performance

• ✅ Immutable models enable safe caching and concurrent access
• ✅ Field validators run once at construction (no repeated overhead)
• ✅ default_factory prevents shared mutable default issues

Security

• ✅ URL validation prevents basic injection (urlparse with scheme/netloc checks)
• ✅ Bounded numeric fields (weight: 0.0-100.0, timeout_seconds: ≥1)
• ✅ Enum-like validation (environment, protocol, load_balance_strategy)
• ⚠️ No input sanitization: service_name, action_name, format accept arbitrary strings (consider regex validation if used in shell commands or file paths)

---

📊 Architectural Alignment

Per CLAUDE.md requirements:

Requirement	Status	Evidence
Type safety	✅	UUID, ModelSemVer, Literal types
Thread safety	✅	frozen=True, from_attributes=True
Immutability	✅	All models frozen
Error handling	✅	Field validators with clear messages
Documentation	✅	Comprehensive docstrings
Test coverage	❌	No tests found
PEP 604 syntax	✅	str | None throughout

---

🎯 Summary

Overall Assessment: Strong implementation with excellent type safety improvements, but CRITICAL missing test coverage.

Must-Fix Before Merge:

1. Add comprehensive unit tests (12+ test cases covering validators, defaults, edge cases)
2. Add validation to timeout_override_ms (gt=0)
3. Verify add_to_execution_chain() compatibility with frozen models

Should-Fix:

4. Add isinstance check to validate_load_balance_strategy
5. Add list item validation to prevent empty strings
6. Document timeout unit conversions

Nice-to-Have:

7. Add regex validation to string fields used in commands/paths
8. Add test demonstrating migration from old dict[str, ModelSchemaValue] pattern

---

📋 Checklist

• ✅ Code follows ONEX patterns (frozen models, type safety)
• ✅ Documentation is comprehensive
• ✅ No obvious bugs (pending verification of feat(tests): Complete TKT-002 ONEXContainer validation with comprehensive test suite #3, feat: Phase 2 Reducer Pattern Engine with Multi-Workflow Support and ONEX Compliance #6)
• ❌ Test coverage requirement NOT met
• ⚠️ Performance: Acceptable (no concerns)
• ⚠️ Security: Minor input validation gaps

Recommendation: Request tests before merge. Once tests are added and validation issues (#2, #3, #6) are addressed, this is a solid improvement to the codebase.

---

Files Reviewed:

• model_action_execution_context.py (152 lines)
• model_action_parameters.py (138 lines)
• model_routing_metadata.py (133 lines)
• model_service_discovery_metadata.py (184 lines)
• model_action_payload.py (18 additions, 10 deletions)
• context/__init__.py (23 additions, 1 deletion)

Review completed: 2025-12-26

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/omnibase_core/models/context/__init__.py (1)

87-116: Group all Action/Routing imports together for consistency.

The imports for ModelRoutingMetadata and ModelServiceDiscoveryMetadata (lines 113-116) are currently placed in the "Error/Retry context models" section, but they should be grouped with the other Action/Routing imports (lines 87-91). This creates an inconsistency with both the module docstring (lines 27-31) and the __all__ list (lines 138-142), which group all four Action/Routing models together.

🔎 Proposed refactor to group imports consistently

Move the ModelRoutingMetadata and ModelServiceDiscoveryMetadata imports to the Action/Routing section:

 # Action/Routing context models (OMN-1049)
 from omnibase_core.models.context.model_action_execution_context import (
     ModelActionExecutionContext,
 )
 from omnibase_core.models.context.model_action_parameters import ModelActionParameters
+from omnibase_core.models.context.model_routing_metadata import ModelRoutingMetadata
+from omnibase_core.models.context.model_service_discovery_metadata import (
+    ModelServiceDiscoveryMetadata,
+)
 
 # Metadata context models
 from omnibase_core.models.context.model_audit_metadata import ModelAuditMetadata

And remove them from the Error/Retry section:

 from omnibase_core.models.context.model_resource_context import ModelResourceContext
 from omnibase_core.models.context.model_retry_context import ModelRetryContext
-from omnibase_core.models.context.model_routing_metadata import ModelRoutingMetadata
-from omnibase_core.models.context.model_service_discovery_metadata import (
-    ModelServiceDiscoveryMetadata,
-)
 from omnibase_core.models.context.model_session_context import ModelSessionContext

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b3a405 and ee2dcaa.

📒 Files selected for processing (1)

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

🧰 Additional context used

📓 Path-based instructions (3)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always use Poetry for Python package management and task execution - use poetry run for all Python commands, never use pip or python directly
Use protocol names (not concrete class names) for service resolution via container.get_service()
Use structured ModelOnexError with error codes instead of generic Exception
Use PEP 604 union syntax (str | None) instead of Optional/Union types
Use duck typing with protocols for service interfaces - never use isinstance checks
Format code with Black and sort imports with isort before committing

Files:

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

**/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/models/**/*.py: Add from_attributes=True to ConfigDict for Pydantic value objects to support pytest-xdist parallel execution
Pydantic models must pass both mypy strict mode and pyright type checking

Files:

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

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Run mypy in strict mode with 0 errors across all source files

Files:

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

🧬 Code graph analysis (1)

src/omnibase_core/models/context/__init__.py (4)

src/omnibase_core/models/context/model_action_execution_context.py (1)

• ModelActionExecutionContext (33-152)

src/omnibase_core/models/context/model_action_parameters.py (1)

• ModelActionParameters (34-138)

src/omnibase_core/models/context/model_routing_metadata.py (1)

• ModelRoutingMetadata (32-133)

src/omnibase_core/models/context/model_service_discovery_metadata.py (1)

• ModelServiceDiscoveryMetadata (35-184)

⏰ 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). (21)

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

🔇 Additional comments (2)

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

27-31: Documentation update looks good.

The new "Action/Routing Context (OMN-1049)" section clearly documents the four new models and follows the existing documentation style.

---

138-142: all exports are correctly organized.

All four Action/Routing context models are properly exported and clearly grouped together with appropriate documentation.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/omnibase_core/models/context/model_action_parameters.py (1)

140-162: Mutable dict limitation is thoroughly documented.

The extensive inline comments, field description warnings, and module/class docstring notes about the mutable dict limitation address the concern raised in previous reviews.

🧹 Nitpick comments (1)

src/omnibase_core/models/context/model_routing_metadata.py (1)

109-137: Timeout validation is correct, but consider consistency.

The validator correctly enforces that timeout_override_ms must be positive when provided. However, ModelActionParameters uses gt=0 in the Field definition instead of a custom validator. Both approaches are valid, but using gt=0 would be more consistent.

🔎 Optional: Use gt=0 for consistency with ModelActionParameters

     timeout_override_ms: int | None = Field(
         default=None,
         description=(
             "Override the default service timeout in milliseconds. "
             "Must be a positive integer when provided. None uses the default timeout."
         ),
+        gt=0,
     )
-
-    @field_validator("timeout_override_ms")
-    @classmethod
-    def validate_timeout_positive(cls, v: int | None) -> int | None:
-        """Validate that timeout_override_ms is positive when provided.
-
-        Args:
-            v: The timeout value in milliseconds to validate.
-
-        Returns:
-            The validated timeout value.
-
-        Raises:
-            ValueError: If the value is not positive when provided.
-        """
-        if v is not None and v <= 0:
-            raise ValueError("timeout_override_ms must be positive")
-        return v

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ee2dcaa and 7345bb5.

📒 Files selected for processing (8)

• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/model_action_parameters.py
• src/omnibase_core/models/context/model_routing_metadata.py
• src/omnibase_core/models/context/model_service_discovery_metadata.py
• tests/unit/models/context/test_model_action_execution_context.py
• tests/unit/models/context/test_model_action_parameters.py
• tests/unit/models/context/test_model_routing_metadata.py
• tests/unit/models/context/test_model_service_discovery_metadata.py

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always use Poetry for Python package management and task execution - use poetry run for all Python commands, never use pip or python directly
Use protocol names (not concrete class names) for service resolution via container.get_service()
Use structured ModelOnexError with error codes instead of generic Exception
Use PEP 604 union syntax (str | None) instead of Optional/Union types
Use duck typing with protocols for service interfaces - never use isinstance checks
Format code with Black and sort imports with isort before committing

Files:

• src/omnibase_core/models/context/model_service_discovery_metadata.py
• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/model_action_parameters.py
• tests/unit/models/context/test_model_action_execution_context.py
• tests/unit/models/context/test_model_service_discovery_metadata.py
• tests/unit/models/context/test_model_routing_metadata.py
• src/omnibase_core/models/context/model_routing_metadata.py
• tests/unit/models/context/test_model_action_parameters.py

**/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/models/**/*.py: Add from_attributes=True to ConfigDict for Pydantic value objects to support pytest-xdist parallel execution
Pydantic models must pass both mypy strict mode and pyright type checking

Files:

• src/omnibase_core/models/context/model_service_discovery_metadata.py
• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/model_action_parameters.py
• tests/unit/models/context/test_model_action_execution_context.py
• tests/unit/models/context/test_model_service_discovery_metadata.py
• tests/unit/models/context/test_model_routing_metadata.py
• src/omnibase_core/models/context/model_routing_metadata.py
• tests/unit/models/context/test_model_action_parameters.py

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Run mypy in strict mode with 0 errors across all source files

Files:

• src/omnibase_core/models/context/model_service_discovery_metadata.py
• src/omnibase_core/models/context/model_action_execution_context.py
• src/omnibase_core/models/context/model_action_parameters.py
• src/omnibase_core/models/context/model_routing_metadata.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) for test categorization

Files:

• tests/unit/models/context/test_model_action_execution_context.py
• tests/unit/models/context/test_model_service_discovery_metadata.py
• tests/unit/models/context/test_model_routing_metadata.py
• tests/unit/models/context/test_model_action_parameters.py

🧠 Learnings (2)

📓 Common learnings

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:55:06.476Z
Learning: Applies to **/models/**/*.py : Add `from_attributes=True` to ConfigDict for Pydantic value objects to support pytest-xdist parallel execution

📚 Learning: 2025-12-26T16:55:06.476Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-26T16:55:06.476Z
Learning: Applies to **/models/**/*.py : Add `from_attributes=True` to ConfigDict for Pydantic value objects to support pytest-xdist parallel execution

Applied to files:

• tests/unit/models/context/test_model_action_parameters.py

🧬 Code graph analysis (8)

src/omnibase_core/models/context/model_service_discovery_metadata.py (1)

src/omnibase_core/models/primitives/model_semver.py (2)

• parse (448-480)
• ModelSemVer (101-480)

src/omnibase_core/models/context/model_action_execution_context.py (3)

src/omnibase_core/models/metadata/model_metadata_field_info.py (1)

• uuid (157-176)

src/omnibase_core/protocols/event_bus/protocol_event_bus.py (1)

• environment (107-114)

src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)

• retry_count (105-107)

src/omnibase_core/models/context/model_action_parameters.py (1)

src/omnibase_core/models/primitives/model_semver.py (1)

• ModelSemVer (101-480)

tests/unit/models/context/test_model_action_execution_context.py (1)

src/omnibase_core/models/context/model_action_execution_context.py (1)

• ModelActionExecutionContext (33-160)

tests/unit/models/context/test_model_service_discovery_metadata.py (1)

src/omnibase_core/models/context/model_service_discovery_metadata.py (1)

• ModelServiceDiscoveryMetadata (46-222)

tests/unit/models/context/test_model_routing_metadata.py (1)

src/omnibase_core/models/context/model_routing_metadata.py (1)

• ModelRoutingMetadata (36-159)

src/omnibase_core/models/context/model_routing_metadata.py (2)

src/omnibase_core/models/metadata/model_metadata_field_info.py (1)

• uuid (157-176)

src/omnibase_core/protocols/event_bus/protocol_event_bus_headers.py (1)

• priority (90-92)

tests/unit/models/context/test_model_action_parameters.py (2)

src/omnibase_core/models/context/model_action_parameters.py (1)

• ModelActionParameters (38-162)

src/omnibase_core/models/primitives/model_semver.py (1)

• ModelSemVer (101-480)

🪛 Gitleaks (8.30.0)

tests/unit/models/context/test_model_action_parameters.py

[high] 64-64: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

---

[high] 76-76: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

⏰ 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). (21)

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

🔇 Additional comments (21)

src/omnibase_core/models/context/model_action_execution_context.py (2)

83-83: LGTM! Model configuration follows guidelines.

The model_config correctly includes frozen=True, extra="forbid", and from_attributes=True as per the coding guidelines for Pydantic value objects to support pytest-xdist parallel execution. Based on learnings.

---

136-160: Environment validator correctly normalizes case.

The validator properly handles case-insensitive input by converting to lowercase before validation. The isinstance check is a good defensive measure.

src/omnibase_core/models/context/model_service_discovery_metadata.py (3)

107-107: LGTM! Model configuration follows guidelines.

The model_config correctly includes all required settings per coding guidelines.

---

126-158: Excellent documentation of mutable list limitation.

The thorough warnings about list mutability in frozen models at multiple levels (module docstring, class docstring, inline comments, and field descriptions) effectively communicate this Pydantic limitation to consumers.

---

164-222: LGTM! Validators are well-implemented.

Both validators handle edge cases correctly:

• Protocol validator normalizes case and strips whitespace
• URL validator properly handles None, empty strings, and whitespace while enforcing HTTP(S) scheme and host requirements

tests/unit/models/context/test_model_action_execution_context.py (3)

51-109: Comprehensive instantiation tests.

Tests cover all three scenarios: full fields, partial fields, and all defaults. Good coverage of the model's field combinations.

---

160-246: Thorough validation testing.

Good coverage of environment validation (case insensitivity, invalid values, empty string), timeout/retry constraints, and UUID field handling including string-to-UUID coercion.

---

447-455: Hashability test is correct.

Verifying that the frozen model is hashable confirms it can be used in sets and as dict keys, which is important for caching and deduplication scenarios.

tests/unit/models/context/test_model_routing_metadata.py (3)

151-181: Good coverage of load_balance_strategy validation.

Tests verify acceptance of all valid strategies and rejection of invalid values including empty strings.

---

231-246: Correct timeout validation tests.

Tests properly verify that timeout_override_ms accepts positive values and rejects zero and negative values, which aligns with the model's validator.

---

529-538: Integration test validates constant-model consistency.

Good practice to verify that all values in VALID_LOAD_BALANCE_STRATEGIES are accepted by the model, ensuring the constant stays synchronized with validation logic.

tests/unit/models/context/test_model_action_parameters.py (3)

64-64: Static analysis false positive - not an API key.

The value "idem-key-12345" is an idempotency key (a user-defined string for request deduplication), not an API key or secret. This is a known false positive from Gitleaks pattern matching.

---

417-431: Correct documentation of unhashable behavior.

Good test that explicitly documents the expected behavior: frozen models with mutable container fields (dict) are not hashable in Pydantic. The detailed docstring explains the reasoning clearly.

---

518-530: ModelSemVer integration test is valuable.

This test verifies the integration between ModelActionParameters and ModelSemVer, including string representation and helper methods like bump_minor().

tests/unit/models/context/test_model_service_discovery_metadata.py (3)

116-131: Required field validation tests are important.

Good coverage of the required service_name field including the minimum length constraint. This differs from other context models where all fields are optional.

---

270-284: Edge case tests for URL normalization.

These tests verify the smart behavior where empty strings and whitespace-only values are normalized to None rather than causing validation errors.

---

585-604: List equality testing confirms expected behavior.

Testing equality comparison with list fields verifies that Pydantic's equality implementation compares list contents by value, which is the expected behavior.

src/omnibase_core/models/context/model_action_parameters.py (3)

34-37: Good use of @allow_dict_str_any decorator.

The decorator provides clear justification for the flexible dict field, documenting that extensions are intentionally flexible for domain-specific parameters that cannot be pre-defined.

---

94-94: LGTM! Model configuration follows guidelines.

The model_config correctly includes all required settings per coding guidelines.

---

111-118: Correct timeout constraint.

Using gt=0 in the Field definition ensures timeout must be a positive integer when provided, which is the correct approach. This is cleaner than a separate validator.

src/omnibase_core/models/context/model_routing_metadata.py (1)

78-78: LGTM! Model configuration follows guidelines.

The model_config correctly includes all required settings per coding guidelines.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

tests/unit/models/core/test_model_action_payload.py (2)

78-93: Unused helper class ActionPayloadAttrs.

This dataclass is defined for testing from_attributes but is never used in any test. Either add a test that exercises ModelActionPayload.model_validate(ActionPayloadAttrs(...)) or remove this dead code.

---

660-675: Assertion logic for exclude_none test could be simplified.

The compound assertions on lines 670-674 are slightly confusing. Consider a clearer approach that directly checks the expected behavior:

🔎 Suggested simplification

-        # None fields should be excluded
-        assert (
-            "parent_correlation_id" not in data
-            or data.get("parent_correlation_id") is not None
-        )
-        assert "target_service" not in data or data.get("target_service") is not None
+        # None fields should be excluded when exclude_none=True
+        assert "parent_correlation_id" not in data
+        assert "target_service" not in data

src/omnibase_core/models/context/model_routing_metadata.py (1)

25-33: Consider exporting LoadBalanceStrategy and VALID_LOAD_BALANCE_STRATEGIES in __all__.

These are useful for external consumers who may want to type-hint or validate load balance strategies. Currently only ModelRoutingMetadata is exported.

🔎 Suggested change

-__all__ = ["ModelRoutingMetadata"]
+__all__ = ["LoadBalanceStrategy", "ModelRoutingMetadata", "VALID_LOAD_BALANCE_STRATEGIES"]

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7345bb5 and bdafcfd.

📒 Files selected for processing (4)

• src/omnibase_core/models/context/model_routing_metadata.py
• src/omnibase_core/models/core/model_action_payload.py
• tests/unit/models/context/test_model_routing_metadata.py
• tests/unit/models/core/test_model_action_payload.py

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

• tests/unit/models/context/test_model_routing_metadata.py

🧰 Additional context used

📓 Path-based instructions (4)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always use Poetry for Python package management and task execution - use poetry run for all Python commands, never use pip or python directly
Use protocol names (not concrete class names) for service resolution via container.get_service()
Use structured ModelOnexError with error codes instead of generic Exception
Use PEP 604 union syntax (str | None) instead of Optional/Union types
Use duck typing with protocols for service interfaces - never use isinstance checks
Format code with Black and sort imports with isort before committing

Files:

• src/omnibase_core/models/context/model_routing_metadata.py
• tests/unit/models/core/test_model_action_payload.py
• src/omnibase_core/models/core/model_action_payload.py

**/models/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/models/**/*.py: Add from_attributes=True to ConfigDict for Pydantic value objects to support pytest-xdist parallel execution
Pydantic models must pass both mypy strict mode and pyright type checking

Files:

• src/omnibase_core/models/context/model_routing_metadata.py
• tests/unit/models/core/test_model_action_payload.py
• src/omnibase_core/models/core/model_action_payload.py

src/omnibase_core/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Run mypy in strict mode with 0 errors across all source files

Files:

• src/omnibase_core/models/context/model_routing_metadata.py
• src/omnibase_core/models/core/model_action_payload.py

tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance) for test categorization

Files:

• tests/unit/models/core/test_model_action_payload.py

🧬 Code graph analysis (2)

tests/unit/models/core/test_model_action_payload.py (7)

src/omnibase_core/models/context/model_action_execution_context.py (1)

• ModelActionExecutionContext (33-160)

src/omnibase_core/models/context/model_action_parameters.py (1)

• ModelActionParameters (38-162)

src/omnibase_core/models/context/model_routing_metadata.py (1)

• ModelRoutingMetadata (36-147)

src/omnibase_core/models/core/model_action_payload.py (3)

• ModelActionPayload (65-313)
• add_to_execution_chain (204-232)
• create_child_payload (234-313)

src/omnibase_core/models/core/model_node_action.py (1)

• ModelNodeAction (22-221)

src/omnibase_core/models/core/model_node_action_type.py (1)

• ModelNodeActionType (17-191)

src/omnibase_core/models/primitives/model_semver.py (1)

• ModelSemVer (101-480)

src/omnibase_core/models/core/model_action_payload.py (5)

src/omnibase_core/models/context/model_action_execution_context.py (1)

• ModelActionExecutionContext (33-160)

src/omnibase_core/models/context/model_action_parameters.py (1)

• ModelActionParameters (38-162)

src/omnibase_core/models/context/model_routing_metadata.py (1)

• ModelRoutingMetadata (36-147)

src/omnibase_core/models/context/model_service_discovery_metadata.py (1)

• ModelServiceDiscoveryMetadata (46-222)

src/omnibase_core/models/core/model_node_action.py (1)

• ModelNodeAction (22-221)

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

• GitHub Check: Tests (Split 15/20)
• GitHub Check: Tests (Split 17/20)
• GitHub Check: claude-review

🔇 Additional comments (10)

tests/unit/models/core/test_model_action_payload.py (4)

100-173: LGTM!

The instantiation tests are well-structured, covering required-only, all-fields, and partial-fields scenarios. The assertions comprehensively verify each field assignment.

---

270-285: Test verifies correct default factory isolation.

This test properly validates that default_factory creates independent list instances, preventing the classic mutable default argument bug. The mutation test on line 283 correctly demonstrates list independence.

---

380-404: LGTM on version coercion tests.

The # type: ignore comments are appropriate here since you're testing Pydantic's automatic coercion from strings and dicts to ModelSemVer. These tests validate important user-facing behavior.

---

843-960: LGTM!

The integration tests effectively validate complex nested structures, workflow simulation with parent-child payloads, and serialization round-trips. The test_workflow_simulation test is particularly valuable for validating trust level inheritance behavior.

src/omnibase_core/models/context/model_routing_metadata.py (2)

78-120: LGTM!

The model configuration and field definitions follow best practices:

• frozen=True for immutability
• from_attributes=True per coding guidelines for pytest-xdist compatibility
• extra="forbid" for strict schema enforcement
• Appropriate constraints (ge, le, gt) on numeric fields
• Good default values

---

122-147: LGTM on the validator implementation.

The validator correctly:

• Checks for non-string input before calling .lower().strip()
• Normalizes the input consistently with other validators in the codebase
• Provides a helpful error message with valid options

The # type: ignore[return-value] is acceptable since the value is validated to be within the Literal type.

src/omnibase_core/models/core/model_action_payload.py (4)

1-44: LGTM on module docstring and documentation.

The comprehensive docstring clearly documents thread safety concerns, key features, and relationships to other modules. The warnings about mutable list fields are valuable.

---

51-59: Good use of direct module imports to avoid circular imports.

The comment explains the rationale, and this is a valid pattern when dealing with circular dependency issues in Python.

---

158-202: LGTM on field definitions.

The migration from dict[str, ModelSchemaValue] to strongly-typed models improves type safety significantly. Default factories are correctly used for mutable types.

---

204-232: LGTM on add_to_execution_chain method.

The method is straightforward, and the docstring appropriately warns about thread safety. The implementation matches the documented behavior.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Potential duplicate keyword argument if trust_level is passed in kwargs.

If a caller passes trust_level in kwargs, this will cause a TypeError: got multiple values for keyword argument 'trust_level' because it's explicitly set on line 309 and then unpacked again from **kwargs on line 312.

The same issue applies to service_metadata (line 310) and version (line 311).

🔎 Proposed fix

     def create_child_payload(
         self,
         child_action: ModelNodeAction,
         **kwargs: Any,
     ) -> "ModelActionPayload":
+        # Remove keys that are explicitly set to avoid duplicate argument errors
+        child_trust_level = kwargs.pop("trust_level", 1.0)
+        kwargs.pop("service_metadata", None)
+        kwargs.pop("version", None)
+        kwargs.pop("parent_correlation_id", None)
+        kwargs.pop("execution_chain", None)
+        kwargs.pop("action", None)
+
         return ModelActionPayload(
             action=child_action,
             parent_correlation_id=self.correlation_id,
             execution_chain=self.execution_chain.copy(),
-            trust_level=min(self.trust_level, kwargs.get("trust_level", 1.0)),
+            trust_level=min(self.trust_level, child_trust_level),
             service_metadata=self.service_metadata,  # Immutable, no copy needed
             version=self.version,  # Required field from ModelOnexInputState
             **kwargs,
         )

🤖 Prompt for AI Agents

In src/omnibase_core/models/core/model_action_payload.py around lines 305 to
312, the constructor call explicitly supplies trust_level, service_metadata, and
version while also unpacking **kwargs which can lead to TypeError if those keys
exist in kwargs; fix by removing these keys from kwargs before the call (e.g.,
pop trust_level, service_metadata, version from kwargs with sensible fallbacks)
or build an explicit payload dict merging defaults with kwargs so the explicit
values are not duplicated when unpacking.

[claude]

PR Review: OMN-1049 - Create typed context models for ModelActionPayload

Summary

This PR successfully replaces untyped dict fields with four well-designed typed context models. The implementation demonstrates excellent attention to ONEX conventions, type safety, and documentation. The 3,186 lines of comprehensive tests show exceptional thoroughness.

Strengths

1. Excellent Type Safety

• Proper use of Literal types for constrained values
• UUID types for identifiers instead of strings
• ModelSemVer for version fields
• Field constraints for positive-only values

2. ONEX Convention Adherence

• Timeout fields use _ms suffix
• All models use frozen=True, extra=forbid, from_attributes=True
• PEP 604 union syntax
• Proper use of @allow_dict_str_any decorator

3. Robust Validation

• Smart case normalization in validators
• Proper URL validation using urlparse
• Clear error messages with allowed values
• Validator return types match Literal field types

4. Outstanding Documentation

• Comprehensive module docstrings
• Detailed attribute descriptions with use cases
• Thread safety caveats clearly documented
• Excellent examples in docstrings

5. Comprehensive Testing

• 3,186 lines of test code
• Covers instantiation, validation, immutability, serialization, edge cases
• Test markers properly used
• Well-organized test classes

Security Considerations

1. Trust Level Propagation - create_child_payload correctly takes minimum trust level between parent and child, preventing privilege escalation

2. URL Validation - health_check_url properly restricts to HTTP(S) schemes only, preventing SSRF vectors

3. Service Discovery - service_name requires min_length=1. Consider adding format restrictions to prevent injection attacks

Thread Safety

The PR excellently documents thread safety limitations for mutable fields (extensions dict, list fields). Documentation provides clear guidance for multi-threaded scenarios.

Potential Issues

1. Validator Consistency (Minor) - ModelActionExecutionContext.validate_environment does not validate type before calling .lower(), while other validators do

2. Service Name Validation (Low Priority) - Consider format restrictions if service names are used in URLs or file paths

3. Weight Range (Question) - ModelRoutingMetadata.weight allows 0.0-100.0. Is weight=0.0 semantically valid?

Recommendations

Medium Priority:

1. Make all validators check isinstance before string operations
2. Add migration guide if needed for downstream code

Low Priority:

1. Consider service name format validation
2. Consider whether weight=0.0 should be allowed

Conclusion

Recommendation: APPROVE

This is exemplary work demonstrating deep understanding of ONEX conventions, excellent type safety practices, comprehensive testing, outstanding documentation, and security awareness.

Merge Status: Ready to merge after CI passes. No blocking issues identified.

[claude]

Code Review: OMN-1049 - Typed Context Models for ModelActionPayload

Summary

This PR successfully introduces 4 new typed context models to replace untyped dict[str, ModelSchemaValue] fields in ModelActionPayload. The implementation demonstrates excellent adherence to ONEX architecture patterns, strong type safety, and comprehensive test coverage.

---

✅ Strengths

1. Excellent Type Safety

• UUID fields: Properly typed as UUID instead of str (workflow_id, service_instance_id, sticky_session_id)
• ModelSemVer: Correctly used for version fields (action_version, service_version)
• Literal types: Well-implemented for enums (LoadBalanceStrategy, ServiceProtocol, environment)
• Field validators: Comprehensive validation with clear error messages (see validate_protocol, validate_load_balance_strategy, validate_environment)

2. Robust Field Validation

All four models include thorough validators:

• Environment validation: Case-insensitive normalization with clear error messages
• Protocol validation: Supports grpc, http, https, ws, wss with normalization
• URL validation: health_check_url validated using urlparse for http/https schemes
• Timeout bounds: timeout_ms and timeout_override_ms constrained to positive integers (gt=0)
• Weight bounds: weight field constrained to 0.0-100.0 range

3. Thread Safety Documentation

Excellent documentation of Pydantic's frozen=True limitations:

• Clear warnings that mutable containers (lists, dicts) can still be mutated
• Explicit guidance to "treat as immutable by convention"
• Appropriate use of frozen=True on context models (immutable) vs. no freeze on ModelActionPayload (mutable by design)

4. Architecture Compliance

• Protocol-driven patterns: Models use typed fields instead of isinstance checks
• PEP 604 unions: Correctly uses str | None instead of Optional[str]
• from_attributes=True: Properly set for value objects per CLAUDE.md guidance
• ConfigDict: Consistent use of frozen=True, extra="forbid", from_attributes=True

5. Exceptional Test Coverage

• 4,391 additions including comprehensive test suites
• Tests cover:
  • Basic instantiation (all fields, partial fields, defaults)
  • Field validation (positive and negative cases)
  • Immutability (frozen behavior)
  • Serialization round-trips
  • Edge cases (empty strings, boundary values)
  • from_attributes compatibility

---

🔍 Issues & Recommendations

1. @allow_dict_str_any Usage ⚠️ Minor

Location: model_action_parameters.py:34

Issue: The extensions field uses dict[str, Any] which is discouraged in the codebase (per ruff/mypy configuration).

Current Justification:

@allow_dict_str_any(
    "Extensions field intentionally allows flexible dict[str, Any] for domain-specific "
    "parameters that cannot be pre-defined..."
)

Assessment: ✅ ACCEPTABLE - The decorator and detailed justification document the architectural decision. The comment explains this is an "escape hatch" for truly dynamic needs. This is a reasonable trade-off for extensibility.

Recommendation: Consider adding a TODO or note about eventually migrating common extension keys to typed fields once usage patterns emerge.

2. Mutable Default Arguments ✅ Correct

Status: No issues found - All list/dict fields correctly use default_factory=list or default_factory=dict instead of mutable defaults. Well done!

3. Health Check URL Validation 💡 Enhancement Opportunity

Location: model_service_discovery_metadata.py:187-222

Current Implementation:

@field_validator("health_check_url", mode="before")
def validate_health_check_url(cls, v: str | None) -> str | None:
    if v is None:
        return None
    parsed = urlparse(v)
    if parsed.scheme not in {"http", "https"}:
        raise ValueError(...)

Observations:

• Validates scheme is http/https ✅
• Does NOT validate netloc (hostname) is present ⚠️
• Could accept invalid URLs like http:// or https:///path

Recommendation: Add netloc validation:

if not parsed.netloc:
    raise ValueError(f"Invalid health_check_url '{v}': missing hostname")

This would reject URLs like http://, https:///, etc.

4. ModelActionPayload.create_child_payload Trust Level ✅ Excellent

Location: model_action_payload.py:236-317

The trust level propagation logic is excellent:

trust_level=min(self.trust_level, child_trust_level)

This ensures trust can only decrease through action chains, never increase - critical for security boundaries. 👍

5. Execution Chain Mutation ⚠️ By Design

Location: model_action_payload.py:206-234

add_to_execution_chain() mutates the list in place - this is intentional per the docstring and ModelActionPayload is NOT frozen. The documentation clearly warns this is not thread-safe. ✅

However, consider if this could lead to unintended side effects when payloads are passed around. The create_child_payload method correctly copies the list (self.execution_chain.copy()) to prevent this.

---

🔒 Security Assessment

Potential Concerns Checked:

1. Command Injection: ❌ Not applicable - no shell commands
2. SQL Injection: ❌ Not applicable - no database queries
3. XSS: ❌ Not applicable - no HTML rendering
4. Path Traversal: ⚠️ input_path/output_path fields in ModelActionParameters accept arbitrary strings
   • Recommendation: If these paths are used for file I/O, ensure validation/sanitization at usage site
5. Trust Level Enforcement: ✅ Correctly propagates minimum trust through action chains

Overall: No critical security issues ✅

---

📊 Performance Considerations

1. Pydantic Validation Overhead: Minimal - validators use simple string operations
2. Memory: UUID fields more efficient than string UUIDs
3. Serialization: model_dump() and model_validate() tested and working
4. Thread Safety: Models are frozen where appropriate, clear documentation of limitations

Overall: No performance concerns ✅

---

📝 Code Quality

Metrics:

• Additions: 4,391 lines (including comprehensive tests)
• Deletions: 22 lines (removed untyped dict fields)
• Files Changed: 11 files
• Test Coverage: Extensive unit tests for all 4 new models + updated payload tests

Documentation:

• ✅ Excellent docstrings on all models and methods
• ✅ Clear field descriptions with examples
• ✅ Thread safety caveats well-documented
• ✅ Type hints complete and accurate

Conventions:

• ✅ Follows CLAUDE.md patterns (base classes, DI, error handling)
• ✅ PEP 604 union syntax throughout
• ✅ Conventional commits style PR title
• ✅ Proper use of Field() with descriptions

---

🎯 Final Verdict

Overall Assessment: EXCELLENT ✅

This PR represents high-quality work that significantly improves type safety and maintainability of the ModelActionPayload system. The implementation:

1. ✅ Eliminates untyped dictionaries in favor of explicit typed models
2. ✅ Provides comprehensive validation with clear error messages
3. ✅ Includes excellent test coverage (>2,400 test lines)
4. ✅ Documents thread safety implications clearly
5. ✅ Follows all ONEX architecture patterns and conventions

Recommendations Summary:

1. Optional Enhancement: Add netloc validation to health_check_url
2. Future Work: Consider path validation/sanitization for input_path/output_path at usage sites
3. Documentation: Consider adding migration guide for code using old dict[str, ModelSchemaValue] fields

Approval Status: APPROVED ✅

This PR is ready to merge. The minor recommendations above are enhancements, not blockers. Excellent work on maintaining code quality and comprehensive testing!

---

Reviewed by: Claude Code (Sonnet 4.5)
Review Date: 2025-12-26
PR: #248 - OMN-1049

[claude]

Code Review - PR #248: Typed Context Models for ModelActionPayload

Summary

This PR replaces untyped dict[str, ModelSchemaValue] fields in ModelActionPayload with four strongly-typed context models. The implementation is excellent and follows ONEX best practices throughout. I recommend approval with a few minor suggestions.

---

✅ Strengths

1. Exceptional Type Safety

• Replaces generic dicts with explicit typed models (ModelActionExecutionContext, ModelActionParameters, ModelRoutingMetadata, ModelServiceDiscoveryMetadata)
• Uses UUID for IDs (workflow_id, service_instance_id, sticky_session_id) instead of str
• Uses ModelSemVer for versions instead of strings
• Proper use of Literal types for enums (environment, load_balance_strategy, protocol)
• Follows PEP 604 union syntax (str | None instead of Optional[str])

2. Comprehensive Documentation

• Outstanding docstrings with clear attribute descriptions
• Excellent thread safety documentation with honest caveats about mutable containers
• Well-documented validators explaining normalization logic
• Helpful examples in docstrings

3. Validation Excellence

• Proper field validators with normalization (lowercase + strip)
• Comprehensive URL validation for health_check_url (scheme + netloc checks)
• Sensible constraints (ge, gt, le, min_length)
• Good error messages with context

4. Thread Safety Awareness

• Models correctly use frozen=True for immutability
• Honest documentation about mutable container limitations (lists/dicts can still be mutated)
• Clear guidance: "Treat as immutable by convention"
• Follows ONEX thread safety guidelines from CLAUDE.md

5. Test Coverage

• Comprehensive test suite (2,639 test additions across 5 test files)
• Tests validation, serialization, immutability, edge cases
• Good use of helper functions and fixtures

6. Architectural Consistency

• Follows ONEX model patterns (ConfigDict(frozen=True, extra="forbid", from_attributes=True))
• Proper use of @allow_dict_str_any decorator with detailed justification
• Avoids circular imports with direct module imports
• Clear distinction from similar models (noted in docstrings)

---

💡 Suggestions (Minor)

1. ModelActionPayload Thread Safety (Priority: Medium)

Current: ModelActionPayload is NOT frozen and contains mutable lists (execution_chain, tool_discovery_tags).

Observation: The docstring correctly documents this is NOT thread-safe, which is good. However, consider:

# Current (mutable)
model_config = ConfigDict(from_attributes=True)

# Suggested (frozen)
model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True)

Trade-off:

• ✅ Freeze it: Prevents accidental mutation, consistent with other models
• ❌ Keep mutable: add_to_execution_chain() mutates in place (convenience)

Recommendation: If add_to_execution_chain() is commonly used, keep current design but ensure it's well-documented (which it is). Otherwise, consider freezing and using a builder pattern or copy-on-write.

---

2. Field Validator Type Hints (Priority: Low)

Some validators accept str in signature but should handle Any for robustness:

# Current (model_action_execution_context.py:139)
def validate_environment(cls, v: str) -> Literal[...]:
    if not isinstance(v, str):  # This check suggests v could be non-str

# Suggested
def validate_environment(cls, v: Any) -> Literal[...]:
    if not isinstance(v, str):

Why: Pydantic can pass non-string values if user provides wrong type. Using Any makes the type hint match the runtime behavior.

Affected validators:

• model_action_execution_context.py:138 (validate_environment)
• model_routing_metadata.py:128 (validate_load_balance_strategy)
• model_service_discovery_metadata.py:166 (validate_protocol)

---

3. Timeout Field Naming Consistency (Priority: Low)

Multiple models have timeout fields with slight variations:

• ModelActionExecutionContext.timeout_ms: ge=1
• ModelActionParameters.timeout_override_ms: gt=0
• ModelRoutingMetadata.timeout_override_ms: gt=0

Question: Should all use gt=0 for consistency? Or is timeout_ms intentionally allowing exactly 1ms?

Impact: Minimal, but consistency helps maintainability.

---

4. Health Check URL Validator Return Type (Priority: Low)

# model_service_discovery_metadata.py:189
def validate_health_check_url(cls, v: str | None) -> str | None:
    if v is None:
        return None
    if not isinstance(v, str):  # Suggests v could be non-str

Suggestion: Use v: Any for the same reason as #2.

---

5. Extensions Field Deep Immutability (Nice-to-have)

ModelActionParameters.extensions: dict[str, Any] has excellent documentation about mutability. For true immutability, consider:

from types import MappingProxyType

@field_validator("extensions", mode="after")
@classmethod
def freeze_extensions(cls, v: dict[str, Any]) -> MappingProxyType:
    return MappingProxyType(v)  # Read-only dict proxy

Trade-off:

• ✅ Enforces immutability at runtime
• ❌ Breaks code expecting mutable dict
• ❌ Serialization may need adjustment

Recommendation: Only do this if there's evidence of accidental mutations in production. Current documentation is sufficient.

---

🔍 Security Review

✅ No security concerns identified:

• URL validation properly checks scheme and netloc
• No command injection vectors
• No unsafe deserialization
• Proper input validation throughout
• Trust level propagation correctly uses min() to prevent privilege escalation

---

🎯 Performance Review

✅ No performance concerns:

• Models are lightweight value objects
• Field validators are O(1) operations
• List/dict fields use default_factory (correct)
• No N+1 query patterns
• Proper use of frozen models for hashability (future caching)

---

📊 Test Coverage Analysis

Test Statistics:

• 4,449 additions across 11 files
• 5 new test files (590-614 lines each)
• 1,117 lines in test_model_action_payload.py

Coverage appears excellent:

• ✅ Validation tests (invalid values, edge cases)
• ✅ Serialization round-trip tests
• ✅ Immutability tests (frozen models)
• ✅ Field default tests
• ✅ Inheritance tests (from_attributes)
• ✅ Method tests (add_to_execution_chain, create_child_payload)

Suggestion: Verify tests cover the specific validators:

• Lowercase normalization for environment, load_balance_strategy, protocol
• URL validation edge cases (missing scheme, missing host, non-HTTP schemes)
• UUID vs string handling for ID fields

---

🏗️ Architecture Review

✅ Follows ONEX patterns:

• ConfigDict(frozen=True, extra="forbid", from_attributes=True) ✓
• Proper use of Field() with descriptions ✓
• __all__ exports ✓
• SPDX headers ✓
• Direct imports to avoid circular dependencies ✓
• Clear distinction from similar models (noted in docstrings) ✓

✅ Integration with existing code:

• ModelActionPayload properly imports new models
• Context __init__.py updated with exports
• No breaking changes (dict fields replaced with compatible typed models)

---

🚀 Migration Impact

Breaking Changes: None (fields retain same names, Pydantic handles dict→model coercion)

Upgrade Path: Seamless for existing code that passes dicts - Pydantic will auto-convert.

Example:

# Old code (still works)
ModelActionPayload(
    action=action,
    parameters={"action_name": "test", "format": "json"},
)

# New code (preferred)
ModelActionPayload(
    action=action,
    parameters=ModelActionParameters(action_name="test", format="json"),
)

---

✅ Final Recommendation: APPROVE

This PR is production-ready with exceptional quality:

• ✅ Dramatically improves type safety
• ✅ Comprehensive documentation and tests
• ✅ Follows all ONEX conventions
• ✅ No security or performance concerns
• ✅ Seamless migration path

Minor suggestions above are optional improvements - none are blocking.

Great work! 🎉

---

Reviewed against: CLAUDE.md v0.4.0, ONEX architectural patterns, thread safety guidelines

[claude]

Code Review: OMN-1049 - Typed Context Models for ModelActionPayload

Summary

This PR successfully replaces untyped dict[str, ModelSchemaValue] fields in ModelActionPayload with four strongly-typed context models. The implementation is well-structured, thoroughly tested, and aligns with ONEX architecture principles.

✅ Strengths

1. Excellent Type Safety

• UUID fields for identifiers (workflow_id, service_instance_id, sticky_session_id)
• ModelSemVer for version fields (action_version, service_version)
• Literal types for enums (environment, load_balance_strategy, protocol)
• Strong validation with clear error messages

2. Comprehensive Testing (4,455 additions!)

• All models have complete unit tests covering:
  • Instantiation (full/partial/defaults)
  • Field validation (boundaries, invalid values, type checking)
  • Immutability/frozen behavior
  • Serialization round-trips
  • Edge cases and integration scenarios
• Test coverage appears excellent - tests follow ONEX patterns with pytest markers
• from_attributes=True properly tested for pytest-xdist compatibility

3. Thread Safety Considerations

• All new context models use frozen=True for immutability
• CRITICAL CAVEAT DOCUMENTED: ModelActionParameters.extensions dict and list fields in ModelServiceDiscoveryMetadata CAN still be mutated despite frozen model
• Documentation explicitly warns about this limitation and recommends treating as immutable by convention
• This is consistent with CLAUDE.md thread safety guidance

4. Documentation Excellence

• Every model has comprehensive docstrings with:
  • Field descriptions with rationale
  • Thread safety warnings
  • Usage examples
  • Cross-references to related models
• @allow_dict_str_any decorator properly justified for ModelActionParameters.extensions
• Clear distinction between ModelActionExecutionContext (action execution) and ModelExecutionContext (CLI execution)
• Clear distinction between ModelServiceDiscoveryMetadata and ModelServiceMetadata (DI container)

5. Architecture Alignment

• ✅ Follows protocol-driven DI principles
• ✅ Uses Pydantic 2.11+ conventions (model_config = ConfigDict(...))
• ✅ PEP 604 union syntax (str | None not Optional[str])
• ✅ Proper use of from_attributes=True for value objects
• ✅ Consistent with ONEX error handling patterns
• ✅ ModelActionPayload properly NOT frozen (mutable) with clear thread safety warnings

6. Validation Robustness

• Environment validation: case-insensitive, strips whitespace, clear error messages
• Load balance strategy: normalized lowercase, comprehensive validation
• Protocol validation: enforces valid values with detailed error context
• Health check URL: proper URL validation with urlparse
• Numeric constraints: proper use of ge, gt, le for bounds
• Service name: min_length=1 prevents empty strings

📝 Minor Observations (Not Blocking)

1. Consistent Timeout Units ✅

All timeout fields correctly use milliseconds (timeout_ms, timeout_override_ms) with clear documentation stating "Uses milliseconds for consistency with ONEX timeout conventions." Well done!

2. ModelActionParameters.extensions - Justified Use of dict[str, Any]

The @allow_dict_str_any decorator provides proper justification:

"Extensions field intentionally allows flexible dict[str, Any] for domain-specific parameters that cannot be pre-defined. All common parameters should use explicit typed fields; extensions is for truly dynamic needs."

This is an acceptable escape hatch, properly documented. Consider promoting recurring extension parameters to typed fields in future iterations.

3. Field Validators - Excellent Error Messages

The custom validators (validate_environment, validate_load_balance_strategy, validate_protocol, validate_health_check_url) all:

• Check type explicitly with clear error messages
• Normalize values (lowercase, strip whitespace)
• Provide helpful error messages with allowed values
• Use proper return type annotations

Great work on validation quality!

4. ModelActionPayload.create_child_payload - Smart Trust Level Handling

The fix for the duplicate keyword argument bug (kwargs.pop("trust_level", 1.0)) is elegant and properly tested. The minimum trust level propagation is a good security pattern.

5. Test Organization - Excellent Structure

Tests are organized into logical classes:

• TestModelActionPayloadInstantiation
• TestModelActionPayloadDefaults
• TestModelActionPayloadValidation
• TestModelActionPayloadMethods
• TestModelActionPayloadSerialization
• TestModelActionPayloadEdgeCases
• TestModelActionPayloadIntegration

This follows ONEX testing conventions perfectly.

🔍 Questions/Suggestions

1. ModelServiceDiscoveryMetadata List Fields

The model has three mutable list fields:

• capabilities: list[str]
• dependencies: list[str]
• tags: list[str]

These are properly documented as mutable despite frozen=True, but consider if these should use tuple[str, ...] for true immutability in thread-safe contexts. Not blocking - current approach with convention is acceptable.

2. Missing Import in __init__.py?

The context/__init__.py file exports ModelActionParameters but the import statement is at the top without the corresponding import for other models in the same section. This is actually correct - I see all imports are properly organized. Good!

3. CI Status

What's the current CI status? All 20 parallel test splits passing? Mypy and pyright both green?

🎯 Recommendations

Before Merge:

1. ✅ Verify all CI checks pass (20 test splits, mypy strict, pyright, ruff, black, isort)
2. ✅ Confirm test coverage meets 60% minimum (should easily exceed with 4,455 test lines)
3. ✅ Ensure pre-commit hooks all pass

Future Enhancements (Not This PR):

1. Monitor ModelActionParameters.extensions usage - promote frequently used keys to typed fields
2. Consider tuple-based lists for truly immutable collections if thread safety becomes critical
3. Add integration tests showing these models in real REDUCER/ORCHESTRATOR workflows

🚀 Verdict

LGTM with Minor Verification ✅

This PR is production-ready with excellent code quality:

• ✅ Architecture: Strongly typed, immutable context models
• ✅ Testing: Comprehensive coverage with 4,455 additions
• ✅ Documentation: Clear, thorough, with proper warnings
• ✅ Thread Safety: Properly documented with caveats
• ✅ ONEX Compliance: Follows all project conventions

Recommended Actions:

1. Verify CI passes (all checks green)
2. Squash and merge when ready

Excellent work on this refactoring! The type safety improvements will prevent entire classes of bugs in action payload handling.

---

Generated with Claude Code 🤖

[claude]

PR Review - OMN-1049: Typed Context Models for ModelActionPayload

Summary

This is an excellent PR that significantly improves type safety and code quality in the action payload system. The migration from untyped dict[str, ModelSchemaValue] fields to strongly-typed models is well-executed with comprehensive testing and documentation.

---

✅ Strengths

1. Architecture & Design

• Clean separation of concerns: Four well-defined models each handle a specific domain (execution, parameters, routing, service discovery)
• Consistent type upgrade: UUID for IDs, ModelSemVer for versions, Literal types for enums
• Immutability by default: All context models use frozen=True with clear documentation about mutable field caveats
• Proper use of from_attributes=True: Follows CLAUDE.md convention for value objects used in parallel testing

2. Type Safety

• Strong Literal types: LoadBalanceStrategy, ServiceProtocol, environment all use Literal instead of plain strings
• Proper validators: Case normalization and type checking in validators (e.g., validate_load_balance_strategy)
• Field constraints: Appropriate use of ge, gt, le Field constraints for numeric validation
• Exported type aliases: LoadBalanceStrategy and VALID_LOAD_BALANCE_STRATEGIES properly exported for consumer convenience

3. Documentation

• Exceptional docstrings: Every model has comprehensive docstrings with:
  • Purpose and context
  • Thread safety caveats (CRITICAL for this codebase)
  • Detailed field descriptions with examples
  • Usage examples
• Thread safety warnings: Clear documentation about mutable dict/list limitations in frozen models
• Cross-references: Proper "See Also" sections linking related models

4. Testing (3,665 new test lines!)

• Comprehensive coverage: 172+ unit tests across all four context models + 49 for ModelActionPayload
• Edge case coverage: Invalid types, boundary values, normalization, validation errors
• Integration testing: Tests for create_child_payload(), inheritance, serialization
• from_attributes testing: Proper testing of Pydantic's attribute-based initialization

5. Error Handling

• Informative error messages: Validators provide clear, actionable error messages
• Type checking before operations: Validators check isinstance() before string operations
• Proper exception types: Uses ValueError with descriptive messages

---

🔍 Code Quality Observations

Excellent Patterns:

1. Fixed duplicate kwargs bug (commit bdafcfd):

   • Properly pops trust_level from kwargs before computing minimum in create_child_payload()
   • Shows attention to detail and responsiveness to review feedback

2. Timeout consistency (commit 7345bb5):

   • Renamed timeout_seconds → timeout_ms for ONEX convention alignment
   • Demonstrates understanding of project-wide patterns

3. ARCHITECTURE DECISION comments in model_action_parameters.py:

   • Explains rationale for dict[str, Any] in extensions field
   • Documents escape hatch philosophy

4. Validation normalization:

   • .lower().strip() applied consistently for case-insensitive validation
   • Prevents user input errors

Minor Observations (Not Issues):

1. ModelActionPayload is NOT frozen (by design):

   • Intentionally mutable for add_to_execution_chain()
   • Well-documented in comprehensive docstring
   • Thread safety caveats clearly explained

2. Type ignore comments justified:

   • # type: ignore[return-value] used appropriately in validators where Literal type is validated at runtime
   • Necessary workaround for mypy's static type checking limitations

---

🧪 Test Quality

Test Structure:

• Organized by category: Instantiation, validation, serialization, immutability, edge cases
• Descriptive test names: Clear intent (e.g., test_environment_validator_rejects_invalid_type)
• Proper markers: @pytest.mark.unit applied consistently
• Edge case coverage: Tests for weight=0.0, timeout boundaries, whitespace handling

Example of Excellent Testing (from test_model_action_execution_context.py):

def test_environment_validator_rejects_invalid_type(self) -> None:
    """Test that environment validator rejects non-string types."""
    with pytest.raises(ValidationError) as exc_info:
        ModelActionExecutionContext(environment=123)  # type: ignore
    
    errors = exc_info.value.errors()
    assert any("must be a string" in str(e) for e in errors)

---

🔒 Security & Safety

Thread Safety:

• ✅ Proper use of frozen=True for context models
• ✅ Clear documentation of mutable field caveats
• ✅ Warnings about list/dict mutation in frozen models

Input Validation:

• ✅ All fields have appropriate constraints
• ✅ URL validation for health_check_url (protocol checks)
• ✅ Trust level clamped to [0.0, 1.0]
• ✅ Timeout values must be positive

Type Safety:

• ✅ UUID types for correlation IDs
• ✅ ModelSemVer for version fields
• ✅ Literal types prevent invalid enum values

---

📊 Metrics

Metric	Value	Assessment
Lines Added	4,460	Substantial but justified by 4 new models + tests
Lines Deleted	22	Clean replacement of untyped dict fields
Test Files	5 new	Comprehensive coverage
Test Cases	236+	Excellent
Documentation	Exceptional	Clear docstrings, thread safety warnings, examples
Type Safety	Strong	Literal types, UUID, ModelSemVer, validators

---

🎯 Alignment with CLAUDE.md

✅ Perfect Adherence:

1. PEP 604 union syntax: Consistently uses str | None instead of Optional[str]
2. from_attributes=True: Applied to all value object models for pytest-xdist compatibility
3. Thread safety documentation: Explicit warnings about mutable fields in frozen models
4. Pydantic conventions: Proper use of ConfigDict, Field, validators
5. Error handling: Structured errors with clear messages (no generic exceptions)
6. Testing standards: Comprehensive unit tests with @pytest.mark.unit

📋 ONEX Patterns Followed:

1. ModelSemVer for versions: action_version, service_version
2. Timeout in milliseconds: timeout_ms, timeout_override_ms
3. Protocol-based design: Models work with DI container system
4. Immutability preference: frozen=True with documented exceptions

---

🚀 Impact Assessment

Benefits:

1. Type Safety: Eliminates entire class of runtime errors from dict key typos
2. IDE Support: Full autocomplete and type hints for all context fields
3. Refactoring Safety: Renames and changes caught by mypy
4. Documentation: Self-documenting code with clear field types
5. Validation: Automatic validation of all fields via Pydantic

Migration Path:

• Clean replacement of dict[str, ModelSchemaValue] fields
• No breaking changes to existing code (fields default to empty models)
• Gradual adoption possible

---

✨ Final Verdict

APPROVED with high confidence.

This PR demonstrates:

• ✅ Excellent software engineering practices
• ✅ Deep understanding of the ONEX architecture
• ✅ Comprehensive testing and documentation
• ✅ Strong type safety improvements
• ✅ Responsiveness to review feedback (7 iterations addressing feedback)
• ✅ Perfect alignment with project standards

Recommendations:

1. Merge when CI passes - This is production-ready code
2. Consider documenting this pattern - Could serve as a template for future typed context migrations
3. Monitor for performance - 4 new model instantiations per payload (likely negligible but worth monitoring)

Minor Future Enhancements (Not Blocking):

1. Consider adding frozen dataclass alternatives for zero-overhead immutability in hot paths
2. Consider adding __slots__ to models if memory usage becomes a concern
3. Could add helper constructors for common patterns (e.g., ModelActionExecutionContext.for_production())

---

Great work, @jonahgabriel! This is exactly the kind of incremental, well-tested improvement that builds a robust codebase. The attention to detail, comprehensive testing, and clear documentation set a high standard. 🎉

---

Reviewed by: Claude Sonnet 4.5 (Automated Code Review)
Review Date: 2025-12-27