Repository navigation
feat(replay): add ModelExecutionCorpus for replay test sets (OMN-1202) - #329
Conversation
Add execution corpus model representing a collection of recorded executions as a replayable test set. A corpus is a curated collection of 20-50+ real production requests used for systematic testing. Models added (each in separate file per ONEX conventions): - ModelExecutionCorpus: Main corpus with dual-mode support (materialized executions or reference IDs) - ModelCorpusStatistics: Computed stats (success rate, handler distribution, avg duration) - ModelCorpusTimeRange: Time bounds with duration property - ModelCorpusCaptureWindow: Capture window for corpus collection Features: - Frozen/immutable Pydantic models with from_attributes=True - with_execution() for immutable updates - get_statistics() for computed metrics - get_time_range() for execution time bounds - validate_for_replay() for corpus validation - get_executions_by_handler() for querying Tests: 61 unit tests covering creation, serialization, statistics, querying, validation, and edge cases.
📝 WalkthroughWalkthroughAdds four immutable Pydantic models to the replay module—ModelExecutionCorpus, ModelCorpusStatistics, ModelCorpusTimeRange, and ModelCorpusCaptureWindow—plus exports, documentation, and comprehensive unit tests covering corpus construction, validation, statistics, querying, mutation helpers, and mode conversions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
PR Review: Execution Corpus Model (OMN-1202)SummaryThis PR adds a well-designed execution corpus model for replay testing with excellent adherence to ONEX conventions. The implementation is clean, well-tested, and follows all project standards. Overall: ✅ Strong approval with minor suggestions. ✅ Strengths1. Excellent Architecture & Design
2. Outstanding Documentation
3. Code Quality
4. Comprehensive Testing
🔍 Issues Found1. Potential Division by Zero in get_statistics() (MINOR)Location: # Line 315-316
durations = [m.get_total_duration_ms() for m in self.executions]
avg_duration = sum(durations) / totalIssue: If Current Protection: if total == 0:
return ModelCorpusStatistics() # line 300Recommendation: No change needed - the early return is correct. Just noting for awareness. 2. Inconsistent Validation Logic (MEDIUM)Location: # Line 260 - Property only checks materialized
@property
def is_valid_for_replay(self) -> bool:
return len(self.executions) > 0
# Line 424 - Method checks both modes
if self.execution_count == 0: # Uses execution_count (materialized + refs)
msg = (...)Issue: The # This corpus would fail validation
corpus = ModelExecutionCorpus(
name="test", version="1.0.0", source="test",
execution_ids=(uuid4(),) # Reference mode
)
assert corpus.is_valid_for_replay == False # Property says invalid
corpus.validate_for_replay() # But method passes\!Recommendation: Update @property
def is_valid_for_replay(self) -> bool:
"""Check if corpus is valid for replay."""
return self.execution_count > 03. Missing Validation for Reference Mode (LOW)Location: The # Validates executions but not execution_ids
for i, manifest in enumerate(self.executions):
if not manifest.node_identity.node_id:
raise ModelOnexError(...)Issue: In reference mode, there's no validation that Recommendation: Consider adding a comment explaining that reference mode validation is intentionally minimal (since manifests are stored externally), or add a check that execution_ids contains valid UUIDs if needed. 4. Potential Data Integrity Issue (MEDIUM)Location: is_reference: bool = Field(
default=False,
description="Whether corpus is in reference mode",
)Issue: The # Possible inconsistent state
corpus = ModelExecutionCorpus(
name="test", version="1.0.0", source="test",
executions=(manifest1, manifest2),
execution_ids=(uuid4(), uuid4()),
is_reference=True # Says reference mode but has materialized data\!
)Recommendation: Add a model validator to enforce consistency: @model_validator(mode="after")
def _validate_mode_consistency(self) -> "ModelExecutionCorpus":
"""Validate that is_reference matches the data."""
has_executions = len(self.executions) > 0
has_refs = len(self.execution_ids) > 0
if self.is_reference and has_executions:
msg = "Reference mode corpus should not have materialized executions"
raise ModelOnexError(
message=msg,
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
)
if not self.is_reference and has_refs and not has_executions:
msg = "Materialized mode corpus should not have only execution_ids"
raise ModelOnexError(
message=msg,
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
)
return selfAlternative: Consider making 💡 Suggestions for Enhancement1. Statistics Only Compute From Materialized ModeLocation: The corpus = ModelExecutionCorpus(
name="test", version="1.0.0", source="test",
execution_ids=(uuid4(), uuid4(), uuid4()) # 3 refs
)
stats = corpus.get_statistics()
assert stats.total_executions == 0 # Surprise\! Expected 3Suggestion: Add a note in the docstring or raise an informative error if called in reference mode: def get_statistics(self) -> ModelCorpusStatistics:
if self.is_reference and not self.executions:
msg = "Statistics unavailable in reference-only mode"
raise ModelOnexError(
message=msg,
error_code=EnumCoreErrorCode.OPERATION_FAILED,
)
# ... rest of implementation2. Add Validation for success_count + failure_countLocation: Consider adding a validator to ensure @model_validator(mode="after")
def _validate_counts(self) -> "ModelCorpusStatistics":
if self.success_count + self.failure_count \!= self.total_executions:
msg = "success_count + failure_count must equal total_executions"
raise ModelOnexError(
message=msg,
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
)
return self3. Consider Adding Bulk OperationsFor large corpora, consider adding bulk operation methods: def with_executions(
self, manifests: Sequence[ModelExecutionManifest]
) -> "ModelExecutionCorpus":
"""Add multiple execution manifests at once."""
return self.model_copy(
update={"executions": (*self.executions, *manifests)}
)🔒 Security Considerations✅ No security issues identified
📊 Performance ConsiderationsGood:
Watch Out For:
📝 Test Coverage AssessmentExcellent Coverage:✅ 61 tests covering all major paths Potential Gaps:
✅ Checklist ReviewBased on CLAUDE.md requirements:
🎯 Final RecommendationStatus: ✅ APPROVE with suggestions This is high-quality code that follows all ONEX conventions. The identified issues are minor and mostly around edge case consistency. The PR can be merged as-is, with the noted improvements tracked for future refinement if needed. Priority Fixes (Before Merge):
Post-Merge Improvements:
Great work on this feature! The dual-mode design is particularly well thought out. 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/omnibase_core/models/replay/model_corpus_time_range.py (1)
66-86: Consider adding validation for min_time <= max_time.The model allows
min_time > max_time, which would result in a negativeduration. WhileModelExecutionCorpus.get_time_range()always creates valid instances from actual data, direct construction could produce semantically invalid ranges.🔎 Optional: Add model validator
from datetime import datetime, timedelta -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator class ModelCorpusTimeRange(BaseModel): # ... existing code ... model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True) min_time: datetime = Field( ..., description="Earliest execution timestamp", ) max_time: datetime = Field( ..., description="Latest execution timestamp", ) + @model_validator(mode="after") + def _validate_time_order(self) -> "ModelCorpusTimeRange": + """Validate that min_time <= max_time.""" + if self.min_time > self.max_time: + raise ValueError("min_time must be <= max_time") + return self + @property def duration(self) -> timedelta:tests/unit/models/replay/test_model_execution_corpus.py (1)
877-879: Import path inconsistency: prefer the canonical module path.Tests import
ModelCorpusStatisticsfrommodel_execution_corpusrather than from its own module or the package's__init__.py. This works ifmodel_execution_corpus.pyre-exports these models, but the canonical import path would beomnibase_core.models.replayor the specific model file.Consider updating imports to use the package-level exports for consistency:
from omnibase_core.models.replay import ModelCorpusStatisticsThis applies to similar imports of
ModelCorpusTimeRangeandModelCorpusCaptureWindowthroughout the test file (lines 896, 1022, 1039, 1282-1284, 1325, 1342).src/omnibase_core/models/replay/model_execution_corpus.py (1)
536-538: Re-export related models for test import convenience.Tests import
ModelCorpusStatistics,ModelCorpusTimeRange, andModelCorpusCaptureWindowfrom this module (e.g.,from omnibase_core.models.replay.model_execution_corpus import ModelCorpusStatistics). Consider adding these to__all__if intentional, or update tests to use the package-level imports.If re-exporting is intended:
__all__ = [ + "ModelCorpusCaptureWindow", + "ModelCorpusStatistics", + "ModelCorpusTimeRange", "ModelExecutionCorpus", ]Otherwise, tests should import from
omnibase_core.models.replaydirectly.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/omnibase_core/models/replay/__init__.pysrc/omnibase_core/models/replay/model_corpus_capture_window.pysrc/omnibase_core/models/replay/model_corpus_statistics.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_execution_corpus.pytests/unit/models/replay/test_model_execution_corpus.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 in omnibase_core project
Always callsuper().__init__(container)in node class constructors to initialize base class and eliminate boilerplate
Use Protocol-Driven Dependency Injection: retrieve services by protocol interface name usingcontainer.get_service('ProtocolName')instead of concrete class names
Always use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax in type annotations
ORCHESTRATOR nodes CANNOT return typed results - they can only emit events and intents, returningModelHandlerOutput[None]withresult=None
Use EnumNodeKind for architectural role (EFFECT, COMPUTE, REDUCER, ORCHESTRATOR, RUNTIME_HOST) and EnumNodeType for implementation type (COMPUTE_GENERIC, TRANSFORMER, etc.)
Files:
src/omnibase_core/models/replay/__init__.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_corpus_statistics.pytests/unit/models/replay/test_model_execution_corpus.pysrc/omnibase_core/models/replay/model_corpus_capture_window.pysrc/omnibase_core/models/replay/model_execution_corpus.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use structured error handling with ModelOnexError and EnumCoreErrorCode instead of generic Exception classes
Use @standard_error_handling decorator to eliminate 6+ lines of try/catch boilerplate in Python methods
Use standardized exception comment markers (fallback-ok, catch-all-ok, cleanup-resilience-ok, boundary-ok, init-errors-ok, tool-resilience-ok) when using catch-all or broad exception handlers
Order exception tuples alphabetically by exception class name in except clauses
Use centralized exception constants fromomnibase_core.errors.exception_groupsinstead of manually specifying exception tuples
Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py
Use duck typing with protocols instead of isinstance checks when working with protocol-based dependency injection
Never share node instances (NodeCompute, NodeEffect, etc.) across threads without explicit synchronization
Use ModelONEXContainer in node constructors instead of ModelContainer for dependency injection
Load contracts using FileRegistry withFileRegistry().load()for single files orload_all()for multiple files
Files:
src/omnibase_core/models/replay/__init__.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_corpus_statistics.pysrc/omnibase_core/models/replay/model_corpus_capture_window.pysrc/omnibase_core/models/replay/model_execution_corpus.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Addfrom_attributes=Trueto Pydantic model ConfigDict for immutable value objects nested in other models to support pytest-xdist parallel execution
Do not usefrom __future__ import annotationsfor Pydantic models and FastAPI endpoints that require runtime type introspection
Files:
src/omnibase_core/models/replay/__init__.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_corpus_statistics.pysrc/omnibase_core/models/replay/model_corpus_capture_window.pysrc/omnibase_core/models/replay/model_execution_corpus.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/replay/test_model_execution_corpus.py
🧬 Code graph analysis (3)
src/omnibase_core/models/replay/model_corpus_time_range.py (1)
src/omnibase_core/models/replay/model_corpus_capture_window.py (1)
duration(81-87)
tests/unit/models/replay/test_model_execution_corpus.py (8)
src/omnibase_core/models/replay/model_execution_corpus.py (9)
ModelExecutionCorpus(95-532)with_execution(348-375)validate_for_replay(404-449)is_valid_for_replay(253-260)get_statistics(274-325)get_executions_by_handler(451-479)get_time_range(327-346)is_materialized(243-250)execution_count(263-270)src/omnibase_core/models/manifest/model_contract_identity.py (1)
ModelContractIdentity(25-169)src/omnibase_core/models/manifest/model_execution_manifest.py (1)
ModelExecutionManifest(37-327)src/omnibase_core/models/manifest/model_node_identity.py (1)
ModelNodeIdentity(23-157)src/omnibase_core/models/primitives/model_semver.py (1)
ModelSemVer(101-499)src/omnibase_core/models/manifest/model_metrics_summary.py (1)
ModelMetricsSummary(19-171)src/omnibase_core/models/replay/model_corpus_statistics.py (1)
ModelCorpusStatistics(40-134)src/omnibase_core/models/replay/model_corpus_capture_window.py (2)
duration(81-87)ModelCorpusCaptureWindow(39-87)
src/omnibase_core/models/replay/model_corpus_capture_window.py (2)
src/omnibase_core/models/infrastructure/model_progress.py (1)
start_time(141-143)src/omnibase_core/models/replay/model_corpus_time_range.py (1)
duration(79-85)
⏰ 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: claude-review
- GitHub Check: Code Quality
- GitHub Check: Exports Validation
🔇 Additional comments (6)
src/omnibase_core/models/replay/model_corpus_capture_window.py (1)
39-88: LGTM!The model is well-structured with proper immutability, documentation, and the
durationproperty. Thestart_time <= end_timevalidation is appropriately handled byModelExecutionCorpus._validate_capture_window.src/omnibase_core/models/replay/__init__.py (1)
52-68: LGTM!The new model exports are properly organized, alphabetically sorted in
__all__, and the module docstring is updated with clear documentation and usage examples.src/omnibase_core/models/replay/model_corpus_statistics.py (1)
40-135: LGTM!The model is well-designed with appropriate constraints (
ge=0,le=1.0for success_rate), helpful__str__and__repr__implementations, and proper documentation. Thefrom_attributes=Trueconfig aligns with the coding guidelines for pytest-xdist compatibility.tests/unit/models/replay/test_model_execution_corpus.py (1)
1-1390: Comprehensive test coverage - well done!The test suite is thorough with 61 tests covering:
- Creation and metadata validation
- Immutability enforcement
- Adding executions with
with_execution()- Replay validation
- JSON/dict serialization round-trips
- Statistics calculation
- Handler querying and time range calculations
- Reference vs materialized modes
- Edge cases and string representations
Tests are properly marked with
@pytest.mark.unitas per coding guidelines.src/omnibase_core/models/replay/model_execution_corpus.py (2)
95-533: Well-designed immutable corpus model with comprehensive API.The implementation demonstrates good practices:
- Frozen model ensures thread safety
with_execution()pattern for immutable updates- Structured error handling with
ModelOnexErrorandEnumCoreErrorCodeper coding guidelines- Clear separation between materialized and reference modes
- Comprehensive docstrings with usage examples
Minor observations:
is_valid_for_replayproperty (line 253-260) only checksexecutions, notexecution_ids, which differs fromvalidate_for_replay()that checksexecution_count- This asymmetry is intentional per the docstrings (replay requires materialized manifests)
314-316: No action needed. This concern is not valid.The
get_total_duration_ms()method has an explicit return type annotation offloat(notfloat | None), and both code paths return float values: either frommetrics_summary.total_duration_ms(a float field with default 0.0) or fromsum(trace.duration_ms for trace in self.hook_traces). The sum() call cannot fail with a TypeError.Likely an incorrect or invalid review comment.
- Fix is_valid_for_replay to check both materialized and reference modes - Add nil UUID validation in validate_for_replay for execution_ids - Add min_time <= max_time validation in ModelCorpusTimeRange - Add bulk operations: with_executions() and with_execution_refs() - Re-export ModelExecutionManifest from replay module for convenience - Fix test imports to use canonical module paths - Add explicit comment for division by zero protection - Add 18 new tests for validation and bulk operations
Code Review - PR #329: Execution Corpus Model (OMN-1202)SummaryThis PR adds a well-designed execution corpus model for replay testing. The implementation follows ONEX conventions excellently with comprehensive documentation, proper error handling, and solid test coverage. Overall, this is high-quality work with only minor suggestions for improvement. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Error Handling
4. Testing
🔍 Issues & Suggestions1. Potential Redundant Validation (Minor)Location: The @model_validator(mode="after")
def _validate_capture_window(self) -> "ModelExecutionCorpus":
if self.capture_window is not None:
if self.capture_window.start_time > self.capture_window.end_time:
msg = "capture_window start_time must be <= end_time"
raise ModelOnexError(...)
return selfHowever, Recommendation:
This prevents duplicate validation logic and ensures consistency across similar models. 2. Data Integrity Consideration (Minor)Location: @property
def execution_count(self) -> int:
return len(self.executions) + len(self.execution_ids)Question: Is it valid to have both If not, consider adding a validator to ensure mutual exclusivity: @model_validator(mode="after")
def _validate_mode_exclusivity(self) -> "ModelExecutionCorpus":
if self.executions and self.execution_ids:
msg = "Cannot mix materialized executions and execution_ids"
raise ModelOnexError(
message=msg,
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
)
return selfIf mixing is intentional, document this behavior in the docstring. 3. Performance Consideration (Informational)Location:
Suggestion: Consider single-pass optimization for large corpora (50+ executions): def get_statistics(self) -> ModelCorpusStatistics:
total = len(self.executions)
if total == 0:
return ModelCorpusStatistics()
# Single pass
handler_counts: Counter[str] = Counter()
successful = 0
total_duration = 0.0
for manifest in self.executions:
# Handler distribution
if handler_id := manifest.node_identity.handler_descriptor_id:
handler_counts[handler_id] += 1
# Success counting
if manifest.is_successful():
successful += 1
# Duration
total_duration += manifest.get_total_duration_ms()
return ModelCorpusStatistics(
total_executions=total,
success_count=successful,
failure_count=total - successful,
handler_distribution=dict(handler_counts),
success_rate=successful / total,
avg_duration_ms=total_duration / total,
)This is a micro-optimization and not critical unless profiling shows it's a bottleneck. 4. API Consistency Question (Minor)Location: The def with_executions(
self,
manifests: tuple[ModelExecutionManifest, ...] | list[ModelExecutionManifest],
) -> "ModelExecutionCorpus":Question: Is there a reason to accept both Since the model stores
5. Test Coverage Note (Good Practice)The PR claims 61 unit tests with comprehensive coverage. Consider verifying coverage with: poetry run pytest tests/unit/models/replay/test_model_execution_corpus.py --cov=src/omnibase_core/models/replay --cov-report=term-missingEnsure edge cases are covered:
🔒 Security Considerations✅ No security concerns identified
📊 Test Coverage AssessmentCoverage appears comprehensive:
Recommendation: Verify coverage metrics meet the project's 60% minimum requirement. 🎯 Final VerdictStatus: ✅ Approve with minor suggestions This is excellent work that follows ONEX conventions closely. The suggestions above are non-blocking refinements that could be addressed in follow-up PRs if needed. Action Items (Optional):
Recommendation: ✅ Approve and merge - the current implementation is production-ready. Great work, @jonahgabriel! 🎉 |
…xecutionCorpus Address all PR #329 review feedback: - Add _validate_time_order to ModelCorpusCaptureWindow for start_time <= end_time - Add _validate_counts to ModelCorpusStatistics ensuring counts sum to total - Add _validate_mode_consistency to ModelExecutionCorpus for is_reference flag - Remove redundant capture_window validation from ModelExecutionCorpus - Improve get_statistics() docstring for reference-mode behavior - Add performance notes to with_execution() and with_execution_ref() docstrings - Add comprehensive tests for new validators (151 tests passing)
Code Review - PR #329: Execution Corpus ModelOverall Assessment: ✅ Excellent work! This PR demonstrates high-quality implementation following ONEX standards with comprehensive testing and documentation. ✅ StrengthsArchitecture & Design
Code Quality
Documentation
Testing
🔍 Observations & Minor Suggestions1. Performance Consideration -
|
| Requirement | Status |
|---|---|
File naming (model_* prefix) |
✅ |
| Poetry usage | ✅ |
| PEP 604 union syntax | ✅ |
from_attributes=True for value objects |
✅ |
| Thread safety documentation | ✅ |
Structured errors (ModelOnexError) |
✅ |
| Pydantic frozen models | ✅ |
| Type annotations | ✅ |
| Docstring completeness | ✅ |
| One class per file | ✅ |
Score: 10/10 🏆
🎯 Recommendations
-
Consider adding:
- A
to_reference()method that converts materialized → reference mode - A
merge()method to combine multiple corpora - Optional size limit validation (warn if corpus exceeds recommended 50 executions)
- A
-
Future enhancements (not blocking):
- Corpus diff/comparison utilities
- Export to standard formats (JUnit XML, TAP)
- Sampling strategies (random, stratified by handler)
-
Documentation: Consider adding a guide in
docs/guides/showing:- When to use materialized vs reference mode
- Best practices for corpus curation
- Integration with replay infrastructure
✅ Approval Status
Approved with enthusiasm! This PR is production-ready. The implementation is clean, well-tested, and follows all ONEX conventions. The dual-mode design is forward-thinking and will scale well.
Merge recommendation: ✅ APPROVE & MERGE
🙏 Acknowledgment
Excellent work addressing all previous review feedback:
- ✅ Added
_validate_time_orderto CaptureWindow - ✅ Added
_validate_countsto Statistics - ✅ Added
_validate_mode_consistencyto ExecutionCorpus - ✅ Improved docstrings with performance notes
- ✅ Comprehensive test additions (151 passing)
The iterative improvement shows great attention to detail.
Reviewed by: Claude Sonnet 4.5
Review Date: 2026-01-04
Confidence Level: High (comprehensive code review completed)
…ecutionCorpus Add three new methods to ModelExecutionCorpus for improved corpus management: - to_reference(): Convert materialized corpus to reference mode by extracting manifest IDs from executions tuple - merge(*others): Combine multiple corpora with deduplication of execution_ids and tags - validate_size(limit): Check if corpus exceeds recommended size (default 50) - warn_if_large(limit): Log UserWarning if oversized, returns self for chaining Also adds: - RECOMMENDED_MAX_EXECUTIONS class constant (50) - Comprehensive documentation guide in docs/guides/EXECUTION_CORPUS_GUIDE.md - 36 new tests covering all new functionality Part of OMN-1202
Code Review - PR #329: Execution Corpus ModelSummaryThis PR adds a well-architected execution corpus model for replay testing. The implementation is high quality with excellent documentation, comprehensive test coverage, and strong adherence to ONEX conventions. ✅ Strengths1. Architecture & Design
2. Code Quality
3. Test Coverage
4. ONEX Compliance
🔍 Code Quality ObservationsPerformance Considerations
Minor API Considerations
🧪 Test Coverage AnalysisExcellent coverage including:
Minor test suggestions (non-blocking):
📚 Documentation QualityThe 524-line EXECUTION_CORPUS_GUIDE.md is exceptional:
Minor documentation suggestions:
🔒 Security & Safety
🎯 Alignment with Project GoalsThis feature directly supports the beta demo (OMN-1202) by providing:
The dual-mode architecture shows excellent forward thinking for scaling from unit tests (materialized) to production corpora (reference). 📊 Final Assessment
✅ Recommendation: APPROVEThis is exemplary work that sets a high bar for model design in the ONEX framework. The implementation is production-ready with:
Suggested follow-ups (post-merge, non-blocking):
Great work! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/unit/models/replay/test_model_corpus_time_range.py (4)
27-28: Missing@pytest.mark.unitdecorator on test class.Per coding guidelines, test classes should be marked with
@pytest.mark.unitfor proper test categorization.🔎 Proposed fix
+@pytest.mark.unit class TestModelCorpusTimeRangeValidation: """Tests for ModelCorpusTimeRange time ordering validation."""
70-71: Missing@pytest.mark.unitdecorator on test class.🔎 Proposed fix
+@pytest.mark.unit class TestModelCorpusTimeRangeDuration: """Tests for ModelCorpusTimeRange duration property."""
97-98: Missing@pytest.mark.unitdecorator on test class.🔎 Proposed fix
+@pytest.mark.unit class TestModelCorpusTimeRangeImmutability: """Tests for ModelCorpusTimeRange immutability (frozen)."""
100-108: Consider using more specific exception type.The comment notes Pydantic raises
ValidationErrorfor frozen models. Usingpytest.raises(ValidationError)would be more precise and document the expected behavior.🔎 Proposed fix
+from pydantic import ValidationError + +... + def test_model_is_frozen(self) -> None: """Test that model is immutable after creation.""" time_range = ModelCorpusTimeRange( min_time=datetime(2024, 1, 1, 12, 0, 0, tzinfo=UTC), max_time=datetime(2024, 1, 1, 13, 0, 0, tzinfo=UTC), ) - with pytest.raises(Exception): # Pydantic raises ValidationError for frozen + with pytest.raises(ValidationError): time_range.min_time = datetime(2024, 1, 1, 11, 0, 0, tzinfo=UTC) # type: ignore[misc]src/omnibase_core/models/replay/model_execution_corpus.py (2)
238-241: Consider extracting the lambda to a module-level function.While the lambda works correctly, extracting it to a module-level function like
_get_utc_now() -> datetimewould improve consistency and testability.🔎 Proposed refactor
Add near the top of the file, after imports:
+def _get_utc_now() -> datetime: + """Return current UTC datetime for corpus timestamps.""" + return datetime.now(UTC) + + class ModelExecutionCorpus(BaseModel):Then update the field:
created_at: datetime = Field( - default_factory=lambda: datetime.now(UTC), + default_factory=_get_utc_now, description="When the corpus was created", )
878-890: Consider deduplicating execution_ids in to_reference().Unlike the
merge()method which deduplicates execution_ids (lines 609-620), theto_reference()method combines existingexecution_idswith newly extracted manifest IDs without deduplication. This could lead to duplicate UUIDs if a corpus has both materialized executions and their IDs already inexecution_ids.While this may be an edge case, adding deduplication would ensure consistency with
merge()behavior and prevent redundant reference lookups.🔎 Proposed refactor to add deduplication
def to_reference(self) -> "ModelExecutionCorpus": # Extract manifest IDs from materialized executions new_ids = tuple(m.manifest_id for m in self.executions) - # Combine existing execution_ids with newly extracted IDs - combined_ids = (*self.execution_ids, *new_ids) + # Combine execution_ids with deduplication (preserve order, remove dups) + seen_ids: set[UUID] = set() + combined_ids: list[UUID] = [] + for exec_id in (*self.execution_ids, *new_ids): + if exec_id not in seen_ids: + seen_ids.add(exec_id) + combined_ids.append(exec_id) return self.model_copy( update={ "executions": (), - "execution_ids": combined_ids, + "execution_ids": tuple(combined_ids), "is_reference": True, } )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/INDEX.mddocs/guides/EXECUTION_CORPUS_GUIDE.mdsrc/omnibase_core/models/replay/__init__.pysrc/omnibase_core/models/replay/model_corpus_capture_window.pysrc/omnibase_core/models/replay/model_corpus_statistics.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_execution_corpus.pytests/unit/models/replay/test_model_corpus_time_range.pytests/unit/models/replay/test_model_execution_corpus.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_core/models/replay/model_corpus_capture_window.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 (poetry run pytest, poetry run mypy, poetry add, poetry install). Never use pip directly or python -m for execution.
Implement ONEX Four-Node Architecture following the unidirectional workflow: EFFECT → COMPUTE → REDUCER → ORCHESTRATOR. Each node must inherit from appropriate base class (NodeEffect, NodeCompute, NodeReducer, NodeOrchestrator).
Always call super().init(container) in node constructors, where container is of type ModelONEXContainer. This eliminates boilerplate initialization code.
Use ModelONEXContainer for dependency injection in node constructors, never use ModelContainer[T]. Resolve services using protocol names only (e.g., container.get_service('ProtocolEventBus')), never by concrete class names.
Use ModelOnexError with EnumCoreErrorCode for all exception handling. Never raise generic Exception. Use the @standard_error_handling decorator to eliminate try/catch boilerplate.
Use PEP 604 union syntax (str | None) instead of Optional[str] and Union types. This is enforced by ruff UP007.
Prefix exception tuples with standardized markers (# fallback-ok:, # catch-all-ok:, # cleanup-resilience-ok:, # boundary-ok:, # init-errors-ok:, # tool-resilience-ok:) to document intent of broad exception handlers.
Order exception tuples alphabetically by exception class name (AttributeError, TypeError, ValidationError, ValueError).
Use centralized exception groups from omnibase_core.errors.exception_groups (PYDANTIC_MODEL_ERRORS, VALIDATION_ERRORS, FILE_IO_ERRORS, etc.) instead of manual exception tuple construction.
Never share node instances across threads. Use thread-local instances or separate instances per thread. Node instances (NodeCompute, NodeEffect) are not thread-safe by default.
Use duck typing with protocols - never use isinstance() checks. Rely on protocol interfaces for type checking and service resolution.
Load runtime contracts using FileRegistry from omnibase_core.runti...
Files:
src/omnibase_core/models/replay/model_corpus_statistics.pysrc/omnibase_core/models/replay/__init__.pytests/unit/models/replay/test_model_execution_corpus.pytests/unit/models/replay/test_model_corpus_time_range.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_execution_corpus.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: All Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py. Examples: cli_, container_, decorator_, enum_, error_, exception_, factory_, model_, node_, mixin_, protocol_, service_, util_, validator_, checker_, resolver_, etc.
Maintain 100% mypy strict mode compliance and ensure pyright passes in CI. Type annotations must be complete and correct.
Files:
src/omnibase_core/models/replay/model_corpus_statistics.pysrc/omnibase_core/models/replay/__init__.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_execution_corpus.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Add ConfigDict(from_attributes=True) to Pydantic models used as value objects for pytest-xdist parallel execution to prevent class identity issues across worker processes.
Canonical Pydantic base class for omnibase_core is BaseModel from pydantic. Use ConfigDict for configuration. Ensure model_config is properly set.
Files:
src/omnibase_core/models/replay/model_corpus_statistics.pysrc/omnibase_core/models/replay/__init__.pysrc/omnibase_core/models/replay/model_corpus_time_range.pysrc/omnibase_core/models/replay/model_execution_corpus.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Test with: poetry run pytest tests/ for all tests, poetry run pytest tests/unit/ for unit tests only, poetry run pytest tests/ -n 0 -xvs for debug mode. Use markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.smoke, @pytest.mark.performance.
Files:
tests/unit/models/replay/test_model_execution_corpus.pytests/unit/models/replay/test_model_corpus_time_range.py
🧬 Code graph analysis (3)
src/omnibase_core/models/replay/model_corpus_statistics.py (2)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-253)src/omnibase_core/models/errors/model_onex_error.py (3)
ModelOnexError(49-354)message(248-250)error_code(253-255)
tests/unit/models/replay/test_model_execution_corpus.py (7)
src/omnibase_core/models/replay/model_execution_corpus.py (15)
ModelExecutionCorpus(102-908)with_execution(404-437)with_executions(439-467)with_execution_refs(500-527)validate_for_replay(650-705)is_valid_for_replay(299-309)get_statistics(323-381)get_executions_by_handler(707-735)get_time_range(383-402)is_materialized(289-296)validate_size(755-797)execution_count(312-319)warn_if_large(799-824)to_reference(843-890)merge(529-648)src/omnibase_core/enums/enum_node_kind.py (1)
EnumNodeKind(13-118)src/omnibase_core/models/manifest/model_execution_manifest.py (1)
ModelExecutionManifest(37-327)src/omnibase_core/models/manifest/model_node_identity.py (1)
ModelNodeIdentity(23-157)src/omnibase_core/models/replay/model_corpus_statistics.py (1)
ModelCorpusStatistics(43-158)src/omnibase_core/models/replay/model_corpus_time_range.py (2)
ModelCorpusTimeRange(41-106)duration(100-106)src/omnibase_core/models/replay/model_corpus_capture_window.py (2)
duration(102-108)ModelCorpusCaptureWindow(42-108)
tests/unit/models/replay/test_model_corpus_time_range.py (2)
src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)src/omnibase_core/models/replay/model_corpus_time_range.py (2)
ModelCorpusTimeRange(41-106)duration(100-106)
🪛 LanguageTool
docs/guides/EXECUTION_CORPUS_GUIDE.md
[style] ~89-~89: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...Smaller serialized file size - Supports very large corpora - Manifests can be updated with...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
docs/guides/EXECUTION_CORPUS_GUIDE.md
37-37: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
49-49: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
53-53: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
97-97: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
107-107: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
124-124: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
139-139: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
143-143: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
234-234: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
244-244: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
309-309: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
344-344: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (2)
- GitHub Check: Code Quality
- GitHub Check: claude-review
🔇 Additional comments (23)
docs/guides/EXECUTION_CORPUS_GUIDE.md (1)
1-524: Comprehensive and well-structured documentation.The guide thoroughly covers the Execution Corpus concept including:
- Clear explanation of materialized vs reference modes with appropriate use cases
- Practical code examples for creation, querying, statistics, and replay integration
- Best practices for corpus curation (20-50 executions optimal range)
- Complete API reference tables
The static analysis hints about MD036 (emphasis used as headings) are stylistic choices that work well within the code example context sections.
docs/INDEX.md (2)
75-76: Documentation index correctly updated.The new Execution Corpus Guide is appropriately added to the "Other Guides" section with consistent formatting.
338-338: Task entry added correctly.The "Create replay test corpora" task links to the appropriate guide, maintaining consistency with the existing task table format.
src/omnibase_core/models/replay/__init__.py (1)
55-77: Clean module exports with proper organization.The new models are correctly imported and exported with:
- Alphabetically sorted
__all__entries- Clear separation between replay models and re-exported convenience models
- Updated module docstring documenting all exports
The re-export of
ModelExecutionManifestfrommodels.manifestis a sensible convenience for users working with replay infrastructure.src/omnibase_core/models/replay/model_corpus_statistics.py (1)
43-161: Well-designed immutable statistics model.The implementation correctly follows the coding guidelines:
ConfigDict(frozen=True, extra="forbid", from_attributes=True)per guidelines for value objectsModelOnexErrorwithEnumCoreErrorCode.VALIDATION_ERRORfor validation failures- Appropriate field constraints (
ge=0,le=1.0for rate)- Comprehensive docstrings with thread safety documentation
The
_validate_countsvalidator ensures data integrity by enforcingsuccess_count + failure_count == total_executions.src/omnibase_core/models/replay/model_corpus_time_range.py (1)
41-109: Correct implementation of time range model.The model properly enforces:
- Time ordering constraint (
min_time <= max_time) withModelOnexError- Immutability via
frozen=True- Read-only
durationproperty- Required fields (no defaults) ensuring valid time bounds
The
from_attributes=Trueconfig enables proper handling in pytest-xdist parallel execution as per coding guidelines.tests/unit/models/replay/test_model_execution_corpus.py (7)
1-44: Excellent TDD test structure with comprehensive coverage.The test file demonstrates thorough testing practices:
- Well-organized test classes by functionality domain
- Proper use of fixtures to create reusable test data
- TYPE_CHECKING import pattern for forward references
- Clear docstrings explaining test intent
51-196: Well-designed factory fixtures.The
create_test_manifestfixture is particularly well-crafted:
- Configurable parameters for flexibility (node_id, handler_name, success, created_at, duration_ms)
- Properly constructs related models (ModelNodeIdentity, ModelContractIdentity, ModelManifestFailure)
- Returns fully-typed ModelExecutionManifest instances
The
create_corpus_with_executionsandcreate_corpus_with_mixed_resultsfixtures enable testing various corpus configurations efficiently.
259-599: Thorough coverage of core corpus operations.Tests comprehensively verify:
- Required field validation (name, version, source)
- Auto-generated fields (corpus_id, created_at)
- Immutability enforcement
- Immutable update patterns (
with_executionreturns new instance, preserves metadata)
606-969: Comprehensive bulk operations and validation testing.Good coverage of:
with_executionsandwith_execution_refsbulk operations- Replay validation including nil UUID detection
- Mode consistency validation (reference vs materialized)
1086-1353: Statistics and time range tests verify computed properties.Tests correctly verify:
- Statistics calculation (total, success/failure counts, rates, handler distribution)
- Average duration computation
- Time range extraction with edge cases (empty, single execution)
1620-1828: Thorough size validation testing.Good coverage of
validate_size()andwarn_if_large():
- Tests at, under, and over the
RECOMMENDED_MAX_EXECUTIONSlimit- Custom limit parameters
- Reference mode counting
- Warning emission verification
- Method chaining support
2085-2730: Comprehensiveto_reference()andmerge()testing.Excellent coverage of conversion and merge operations:
to_reference()preserves metadata, extracts IDs, handles idempotencymerge()combines executions/refs, deduplicates tags and IDs, preserves primary corpus metadata- Edge cases (empty corpora, large corpora, mixed modes)
src/omnibase_core/models/replay/model_execution_corpus.py (10)
102-175: LGTM! Excellent Pydantic configuration.The model configuration follows best practices with
frozen=Truefor immutability/thread safety,from_attributes=Truefor pytest-xdist compatibility (as per coding guidelines), andextra="forbid"to prevent unexpected fields. The class constantRECOMMENDED_MAX_EXECUTIONSis properly typed withClassVar.
245-284: LGTM! Robust mode consistency validation.The validator correctly ensures that the
is_referenceflag matches the data state, with clear error messages and proper error handling usingModelOnexError.
288-319: LGTM! Clean property implementations.The properties are simple, efficient, and correctly implement the dual-mode counting logic.
323-381: LGTM! Well-designed statistics computation.The method correctly handles edge cases with the early return for empty executions (preventing division by zero), and properly handles None values in
handler_descriptor_idby checking before incrementing the counter.
383-402: LGTM! Clean time range calculation.The method correctly handles the empty case and efficiently computes min/max times.
404-527: LGTM! Excellent immutable mutation helpers.These methods correctly implement the frozen model pattern using
model_copy()with tuple unpacking. The performance note in the docstring is valuable guidance for users.
529-648: LGTM! Well-implemented merge with clear semantics.The merge logic correctly handles deduplication of UUIDs and tags while preserving order, and appropriately does not deduplicate manifest objects (as documented). The automatic mode determination based on content is elegant.
707-753: LGTM! Clean query method implementations.The filtering methods are simple, efficient, and correctly implemented using tuple comprehensions.
755-824: LGTM! Well-designed advisory size validation.The size validation provides helpful warnings without blocking functionality. The use of
stacklevel=2ensures warnings point to the caller's code, and the chainablewarn_if_large()method is convenient.
683-695: The validation is correct as written;node_idandcontract_idare string types, not UUIDs.Both
node_identity.node_idandcontract_identity.contract_idare defined asstrtypes (not UUIDs), making the truthiness checksif not manifest.node_identity.node_idandif not manifest.contract_identity.contract_idappropriate for detecting empty strings. The Pydantic Field definitions also enforcemin_length=1, providing additional validation at the model layer.
1 participant
Summary
Add execution corpus model representing a collection of recorded executions as a replayable test set. A corpus is a curated collection of 20-50+ real production requests used for systematic testing and comparison.
Models added (each in separate file per ONEX conventions):
ModelExecutionCorpus: Main corpus with dual-mode support (materialized executions or reference IDs)ModelCorpusStatistics: Computed stats (success rate, handler distribution, avg duration)ModelCorpusTimeRange: Time bounds with duration propertyModelCorpusCaptureWindow: Capture window for corpus collectionFeatures:
from_attributes=Truewith_execution()for immutable updatesget_statistics()for computed metricsget_time_range()for execution time boundsvalidate_for_replay()for corpus validationget_executions_by_handler()for queryingTests: 61 unit tests covering creation, serialization, statistics, querying, validation, and edge cases.
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.