Repository navigation
feat(errors): standardize error handling patterns (OMN-1299) - #374
Conversation
- Add 360+ exception documentation markers (fallback-ok, boundary-ok, etc.) - Increase @standard_error_handling decorator adoption from 7 to 50 usages - Increase exception_groups.py adoption from 17 to 51 files - Document ValueError usage in Pydantic validators with # error-ok comments - Replace individual exception tuples with centralized groups Closes OMN-1299
📝 WalkthroughWalkthroughCentralizes exception handling into grouped exception tuples and new decorators, enriches ModelOnexError with contextual metadata, narrows many broad except clauses to centralized groups (e.g., VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, ATTRIBUTE_ACCESS_ERRORS), ensures asyncio.CancelledError is re-raised, and adds documentation and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Decorator as standard_error_handling
participant Impl as _impl / target method
participant Logger
participant ModelOnexError
Caller->>Decorator: call decorated public method(...)
Decorator->>Impl: invoke underlying implementation
alt Impl returns normally
Impl-->>Decorator: return value
Decorator-->>Caller: return value
else Impl raises asyncio.CancelledError
Impl-->>Decorator: raises CancelledError
Decorator->>Caller: re-raise CancelledError
else Impl raises other Exception
Impl-->>Decorator: raises Exception
Decorator->>Logger: log exception with context
Decorator->>ModelOnexError: construct enriched error (original_error_type, operation, flags)
Decorator-->>Caller: raise ModelOnexError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/omnibase_core/models/**/model_*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/omnibase_core/**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (11)📓 Common learnings📚 Learning: 2026-01-13T22:34:48.697ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.697ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.698ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.698ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.697ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.697ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.698ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.697ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.697ZApplied to files:
📚 Learning: 2026-01-13T22:34:48.697ZApplied to files:
🪛 LanguageToolCLAUDE.md[grammar] ~516-~516: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (19)
Comment |
Code Review: Error Handling Standardization (PR #374)SummaryThis PR successfully establishes comprehensive error handling patterns across omnibase_core through documentation markers, decorator adoption, and centralized exception groups. The changes are well-structured, consistent, and align with the project's ONEX architecture principles. ✅ Strengths1. Excellent Documentation
2. Consistent Pattern Application
3. Smart Decorator Design PatternThe wrapper/implementation split in def get_default_contract_profile(...):
"""Public API with docstring"""
return _get_default_contract_profile_impl(...)
@standard_error_handling("Contract profile resolution")
def _get_default_contract_profile_impl(...):
"""Internal implementation"""
# Business logic hereThis preserves clean public APIs while eliminating boilerplate. Excellent pattern. 4. Centralized Exception ManagementThe
5. Marker ConsistencyException handling markers are consistently applied with clear rationale:
6. Type Safety Maintained
🔍 Observations & Minor Suggestions1. Decorator Overhead (Non-blocking)The wrapper/implementation pattern adds an extra function call per operation. For hot paths (called millions of times), consider benchmarking:
However: The performance impact is likely negligible (<1μs) compared to the value of consistent error handling. The pattern is solid. 2. Exception Group Coverage (Enhancement opportunity)Some files still use individual exception tuples that could benefit from centralized groups: # Current (individual tuples)
except (AttributeError, TypeError, ValueError) as e:
# Could use
except PYDANTIC_MODEL_ERRORS as e:Suggested files for follow-up sweep:
Not blocking: This can be done incrementally in future PRs. 3. ValueError Documentation Comments (Clarification)The 4. Async Exception Handling (Edge case)
# ✅ Correct pattern
try:
await async_operation()
except asyncio.CancelledError:
# Cleanup
raise # Always re-raise!
except ASYNC_ERRORS as e:
# Handle other errorsRecommendation: Audit async functions in 🧪 TestingRequired:
Recommended Follow-up:
🔒 Security ConsiderationsNo security concerns identified. The changes:
📊 Performance ImpactExpected: Negligible (< 0.1% overhead)
Recommendation: Monitor CI test suite runtime. If PR #374 CI runs exceed 3m30s baseline (per CI_MONITORING_GUIDE.md), investigate decorator hot paths. 🎯 Alignment with ONEX Principles✅ Protocol-Driven Design: Error handling respects ONEX exception contract ✅ RecommendationAPPROVE with minor follow-up suggestions. This PR represents a significant quality improvement to the codebase. The error handling patterns are well-designed, consistently applied, and thoroughly documented. The changes eliminate boilerplate, improve debuggability, and establish clear conventions for future development. Minor Follow-ups (Non-blocking):
Before Merge:
Excellent work on this comprehensive refactoring! 🎉 Review conducted by Claude Code (Sonnet 4.5) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (14)
src/omnibase_core/models/cli/model_cli_execution_config.py (1)
158-168: Unreachable exception handler - dead code.The
tryblock contains onlyreturn True, which cannot raise any exception. Theexcept PYDANTIC_MODEL_ERRORSclause is unreachable dead code. Either:
- Remove the try/except entirely if no validation logic is intended
- Add actual validation logic that could raise
PYDANTIC_MODEL_ERRORSOption 1: Remove dead exception handling
def validate_instance(self) -> bool: """Validate instance integrity (ProtocolValidatable protocol).""" - try: - # Basic validation - ensure required fields exist - # Override in specific models for custom validation - return True - except PYDANTIC_MODEL_ERRORS as e: - raise ModelOnexError( - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Operation failed: {e}", - ) from e + # Basic validation - ensure required fields exist + # Override in specific models for custom validation + return TrueIf option 1 is chosen, the unused import on line 16 should also be removed.
src/omnibase_core/contracts/diff_computer.py (1)
177-192: Alphabetize exception tuple and avoid logging full ValidationError details.
- Exception tuple at lines 180-186 is not alphabetized (current:
AttributeError, TypeError, ValidationError, ValueError, ModelOnexError; correct:AttributeError, ModelOnexError, TypeError, ValidationError, ValueError).str(e)forpydantic.ValidationErrorincludes the offending input by default, which can leak sensitive data. Usee.errors()instead to safely access structured error information.Proposed fix
- except ( - AttributeError, - TypeError, - ValidationError, - ValueError, - ModelOnexError, - ) as e: - # fallback-ok: fingerprint computation is optional for non-contract models - logger.debug( - "Fingerprint computation skipped for %s: %s", - type(before).__name__, - str(e), - ) + except ValidationError as e: + # fallback-ok: fingerprint computation is optional for non-contract models + logger.debug( + "Fingerprint computation skipped for %s due to ValidationError (%d errors)", + type(before).__name__, + len(e.errors()), + ) + except (AttributeError, ModelOnexError, TypeError, ValueError) as e: + # fallback-ok: fingerprint computation is optional for non-contract models + logger.debug( + "Fingerprint computation skipped for %s: %s", + type(before).__name__, + type(e).__name__, + )src/omnibase_core/container/container_service_registry.py (3)
217-239: Avoid partial registry state onregister_service()failure (rollback on exception).
Right now failures after writing_registrations/ maps (e.g.,implementation()/_store_instance) can leave an unusable registration behind.Proposed fix (atomic registration + safer error context)
@@ async def register_service( @@ """ @@ Raises: ModelOnexError: If registration fails """ + registration_id = uuid4() + interface_name: str | None = None + impl_name: str | None = None try: - registration_id = uuid4() - interface_name = ( + interface_name = ( interface.__name__ if hasattr(interface, "__name__") else str(interface) ) impl_name = ( implementation.__name__ if hasattr(implementation, "__name__") else str(implementation) ) @@ return registration_id @@ except Exception as e: # boundary-ok: wrap registration failures in structured ONEX error self._failed_registrations += 1 + # Best-effort rollback to keep registry consistent + self._instances.pop(registration_id, None) + self._registrations.pop(registration_id, None) + if interface_name and interface_name in self._interface_map: + self._interface_map[interface_name] = [ + rid for rid in self._interface_map[interface_name] if rid != registration_id + ] + if not self._interface_map[interface_name]: + self._interface_map.pop(interface_name, None) + if impl_name and self._name_map.get(impl_name) == registration_id: + self._name_map.pop(impl_name, None) msg = ( f"Failed to register service '{interface.__name__ if hasattr(interface, '__name__') else str(interface)}'. " - f"Error: {e}. " + f"Error type: {type(e).__name__}. " f"Check that the interface type is valid and metadata is properly formatted." ) - emit_log_event(EnumLogLevel.ERROR, msg, {"error": str(e)}) + emit_log_event( + EnumLogLevel.ERROR, + msg, + { + "error_type": type(e).__name__, + "interface": interface_name, + "implementation": impl_name, + # Avoid logging raw str(e) unless you're sure it can't contain secrets. + }, + ) raise ModelOnexError( message=msg, error_code=EnumCoreErrorCode.REGISTRY_RESOLUTION_FAILED, ) from e
313-347: Avoid partial registry state onregister_instance()failure (rollback on exception).
Same issue asregister_service(): maps are updated before_store_instance()and can be left inconsistent if anything throws.Proposed fix (rollback + safer error details)
@@ async def register_instance( @@ Raises: ModelOnexError: If registration fails """ + registration_id = uuid4() + interface_name: str | None = None + instance_type: str | None = None try: - registration_id = uuid4() - interface_name = ( + interface_name = ( interface.__name__ if hasattr(interface, "__name__") else str(interface) ) - instance_type = type(instance).__name__ + instance_type = type(instance).__name__ @@ return registration_id @@ except Exception as e: # boundary-ok: wrap instance registration failures in structured ONEX error self._failed_registrations += 1 - interface_name = ( - interface.__name__ if hasattr(interface, "__name__") else str(interface) - ) - instance_type = type(instance).__name__ + # Best-effort rollback + self._instances.pop(registration_id, None) + self._registrations.pop(registration_id, None) + if interface_name and interface_name in self._interface_map: + self._interface_map[interface_name] = [ + rid for rid in self._interface_map[interface_name] if rid != registration_id + ] + if not self._interface_map[interface_name]: + self._interface_map.pop(interface_name, None) + if instance_type and self._name_map.get(instance_type) == registration_id: + self._name_map.pop(instance_type, None) msg = ( f"Failed to register instance of type '{instance_type}' for interface '{interface_name}'. " - f"Error: {e}. " + f"Error type: {type(e).__name__}. " f"Verify that the instance implements the interface protocol correctly." ) emit_log_event( EnumLogLevel.ERROR, msg, { - "error": str(e), + "error_type": type(e).__name__, "interface": interface_name, "instance_type": instance_type, }, ) raise ModelOnexError( message=msg, error_code=EnumCoreErrorCode.REGISTRY_RESOLUTION_FAILED, ) from e
500-522: Don’t leak raw exception strings inresolve_service()errors; add safe structured context if supported.
Theboundary-okmarker is good, but includingError: {e}in user-facing messages can leak details; also consider logging error type + safe identifiers, and passing structured context intoModelOnexError(if the model supports it). Based on learnings, ...src/omnibase_core/models/context/model_detection_metadata.py (1)
137-165: WrapModelSemVer.parse()to convertModelOnexErrortoValueErrorand fix exception tuple alphabetization. Thestrpath at line 139 callsModelSemVer.parse()which raisesModelOnexError(per its implementation), but Pydantic field validators must emitValueError. Additionally, theexcepttuple at line 157 is out of alphabetical order and includes an unnecessaryKeyError(sincedict.get()never raises it).Proposed fix
@@ -from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator + +from omnibase_core.models.errors.model_onex_error import ModelOnexError @@ if isinstance(v, str): # Use ModelSemVer.parse() for string parsing - return ModelSemVer.parse(v) + try: + return ModelSemVer.parse(v) + except ModelOnexError as e: + # error-ok: Pydantic field_validator requires ValueError + raise ValueError(f"Invalid rule_version string: {v!r}") from e @@ - except (TypeError, KeyError, ValueError) as e: + except (TypeError, ValueError) as e: # error-ok: Pydantic field_validator requires ValueError raise ValueError( f"Invalid rule_version dict format: expected {{'major': int, " f"'minor': int, 'patch': int}}, got {v}" ) from esrc/omnibase_core/mixins/mixin_effect_execution.py (1)
159-1609: Potential regression:_extract_response_fields()may now leak non-Pydantic exceptions.With only
except PYDANTIC_MODEL_ERRORS, things likeTypeError(badpath_exprtype), jsonpath parsing errors, etc. can propagate as raw exceptions instead ofModelOnexError. This is a behavior change vs the previous broader catch tuple.Proposed fix (keep group, but preserve structured-error boundary)
except ModelOnexError: raise - except PYDANTIC_MODEL_ERRORS as e: + except PYDANTIC_MODEL_ERRORS as e: raise ModelOnexError( message=f"Field extraction failed for {output_name}: {e!s}", error_code=EnumCoreErrorCode.OPERATION_FAILED, ) from e + except Exception as e: # catch-all-ok: extraction utility must not leak raw exceptions + raise ModelOnexError( + message=f"Field extraction failed for {output_name}: {e!s}", + error_code=EnumCoreErrorCode.OPERATION_FAILED, + ) from eBased on learnings, broad catches are acceptable when documented as an error boundary and the method re-wraps into
ModelOnexError.src/omnibase_core/models/core/model_state_contract.py (1)
297-306: Remove dead exception handler—load_and_validate_yaml_modelalready raisesModelOnexError.
load_and_validate_yaml_model(fromutil_safe_yaml_loader.py) comprehensively handles all exceptions internally—file I/O errors, YAML parsing errors, and validation errors—and always raisesModelOnexError. The except block catchingPYDANTIC_MODEL_ERRORSwill never execute becauseModelOnexErroris not in that exception group. Delete the try-except wrapper since error handling is already complete in the utility function.src/omnibase_core/models/cli/model_cli_debug_info.py (1)
199-209: Unreachable exception handler —return Truecannot raisePYDANTIC_MODEL_ERRORS.The try block (lines 201-204) only contains
return True, which cannot raise any of the exceptions inPYDANTIC_MODEL_ERRORS. The except clause on line 205 is dead code.Either:
- Remove the try/except entirely since no validation actually occurs, or
- Implement actual validation logic that could raise these exceptions
♻️ Option 1: Remove dead try/except
def validate_instance(self) -> bool: """Validate instance integrity (ProtocolValidatable protocol).""" - try: - # Basic validation - ensure required fields exist - # Override in specific models for custom validation - return True - except PYDANTIC_MODEL_ERRORS as e: - raise ModelOnexError( - error_code=EnumCoreErrorCode.VALIDATION_ERROR, - message=f"Operation failed: {e}", - ) from e + # Basic validation - override in specific models for custom validation + return Truesrc/omnibase_core/utils/util_safe_yaml_loader.py (1)
523-538: PYDANTIC_MODEL_ERRORS does not include KeyError — dict access errors will not be caught.The exception group
PYDANTIC_MODEL_ERRORS(defined insrc/omnibase_core/errors/exception_groups.pylines 63–68) contains(AttributeError, TypeError, ValidationError, ValueError)and explicitly excludesKeyError. However, the comment mentions "dict access errors," which raiseKeyErrorwhen accessing missing dictionary keys. The previous implementation caughtKeyErrorexplicitly, but the refactored code usingPYDANTIC_MODEL_ERRORSloses this coverage.Use
ATTRIBUTE_ACCESS_ERRORSinstead, which includesKeyError(along withAttributeError,IndexError,TypeError), or explicitly catch(PYDANTIC_MODEL_ERRORS, KeyError).src/omnibase_core/models/container/model_workflow_coordinator.py (2)
112-137: Add an exception-marker for the group catch + confirm re-raise-only is desired.Same note as above: please add a marker explaining why this group catch is acceptable here, and verify that only logging + re-raising (no wrapping) is the intended contract for
_execute_workflow_type(). As per coding guidelines, ...
48-99: Add exception marker to document intent for broad exception group handler.The
except PYDANTIC_MODEL_ERRORS as e:on line 80 catches a broad group of exceptions (AttributeError, TypeError, ValidationError, ValueError) without a standardized comment marker explaining why it's acceptable. Per coding guidelines, broad exception handlers require an explicit marker.Suggested fix
- except PYDANTIC_MODEL_ERRORS as e: + except PYDANTIC_MODEL_ERRORS as e: # boundary-ok: normalize model/validation failures into ModelOnexError for workflow boundary # Log workflow failuresrc/omnibase_core/utils/util_workflow_executor.py (1)
849-866: Don’t swallow async cancellation inside workflow step error handling.
Ifasyncio.CancelledErroris caught byexcept Exceptionhere, cancellation becomes a “failed step” instead of aborting the task, which can hang shutdowns/timeouts. Suggest explicitly re-raising cancellation before the broad catch.Proposed fix
@@ import heapq +import asyncio import json import logging import time @@ - except Exception as e: + except asyncio.CancelledError: + raise + except Exception as e: # boundary-ok: workflow steps execute external code with unknown exception types # Production workflows require resilient error handling - all failures logged # with full traceback for debugging. Failed steps tracked per error_action config. failed_steps.append(str(step.step_id)) logging.exception( f"Workflow '{workflow_definition.workflow_metadata.workflow_name}' step '{step.step_name}' ({step.step_id}) failed with unexpected error: {e}" )src/omnibase_core/models/core/model_event_type_registry.py (1)
160-166: Preserve original exception context when wrapping, and align exception tuple ordering.
- Line 162: wrap should use
raise ... from eto retain traceback.- Line 232: guideline says exception tuples should be alphabetically ordered. As per coding guidelines, please align ordering.
Proposed fix
@@ - except (AttributeError, KeyError, OSError, ValueError) as e: + except (AttributeError, KeyError, OSError, ValueError) as e: msg = f"Failed to parse contract {contract_file}: {e}" raise ModelOnexError( error_code=EnumCoreErrorCode.INTERNAL_ERROR, message=msg, - ) + ) from e @@ - except (ModelOnexError, AttributeError, RuntimeError) as e: + except (AttributeError, ModelOnexError, RuntimeError) as e: raise ModelOnexError( message="DI container not initialized - cannot get event type registry. " "Initialize the container first.", error_code=EnumCoreErrorCode.CONFIGURATION_ERROR, ) from eAlso applies to: 232-237
🤖 Fix all issues with AI agents
In @src/omnibase_core/mixins/mixin_node_service.py:
- Around line 38-39: Replace the repeated custom exception tuples in
_subscribe_to_tool_invocations, _emit_tool_response, and _emit_shutdown_event
with the centralized PYDANTIC_MODEL_ERRORS (which covers AttributeError,
TypeError, ValidationError, ValueError) and remove RuntimeError from that tuple
unless you can justify and document why RuntimeError must be caught defensively;
then extract the repeated container-access logic into a single helper
_try_get_event_bus_from_container() that returns the event bus or None (used by
the three methods with the existing "# fallback-ok:" behavior) to eliminate
duplication and reduce drift.
In @src/omnibase_core/services/service_protocol_migrator.py:
- Around line 13-15: The standard_error_handling decorator is sync-only and
won't catch exceptions from async functions like
ModelOperationParameters.execute; update the decorator (standard_error_handling)
to handle both sync and async callables by detecting
asyncio.iscoroutinefunction(func) and returning an async wrapper that awaits
func(*args, **kwargs) inside the existing try/except, while keeping the current
sync wrapper for regular functions; ensure the async wrapper preserves the same
exception handling (using EnumCoreErrorCode where applicable) and that existing
sync usages such as create_migration_plan remain unchanged.
🧹 Nitpick comments (19)
src/omnibase_core/models/health/model_health_check_metadata.py (1)
120-128: Good: init-time catch rationale is now tagged withinit-errors-ok.
Consider logging at DEBUG inside thesepassblocks to aid diagnosis while keeping init resilient. As per coding guidelines / learnings.Proposed tweak (debug-only logging)
def _resolve_forward_references() -> None: """Resolve forward references for ModelHealthCheckMetadata.""" try: @@ except ImportError: # init-errors-ok: Import may fail during early module loading # The forward reference will be resolved when health/__init__.py completes + # Optional: keep silent at higher levels, but leave a breadcrumb. + # logger.debug("Deferred ModelCustomFields import during early module loading", exc_info=True) pass except Exception: # init-errors-ok: model_rebuild may fail during circular import resolution # This is safe to ignore as the forward reference will be resolved # when the full module graph is loaded + # logger.debug("Deferred model_rebuild during circular import resolution", exc_info=True) passsrc/omnibase_core/models/context/model_error_metadata.py (2)
199-202: Good: ValueError requirement is clearly documented for this Pydantic validator.
Optional: since this ismode="before", consider acceptingvalue: object(or coercing strings) to avoidTypeErroron unexpected inputs.
232-242: Potential type-robustness gap inmode="before"validator (non-str inputs).
Ifvalueis not astr(e.g., anintfrom upstream),ERROR_CODE_PATTERN.match(value)can raiseTypeError; consider explicitly checkingisinstance(value, str)and raisingValueErrorwith a clearer message.src/omnibase_core/validation/validator_utils.py (1)
42-45: Nice: switched to centralizedATTRIBUTE_ACCESS_ERRORSfor batch resilience.
To align with the standardized marker convention for broad-ish catches, consider adding a# fallback-ok:(or# tool-resilience-ok:) tag above this handler. As per coding guidelines / learnings.Proposed marker tweak
- except ATTRIBUTE_ACCESS_ERRORS as e: + # fallback-ok: Batch protocol scanning should skip malformed ASTs / unexpected extractor shapes + # rather than failing the entire run. + except ATTRIBUTE_ACCESS_ERRORS as e: # Handle AST processing errors: malformed tree structures, missing attributes, # or unexpected types from extractor operations logger.warning(f"Error processing AST in {file_path}: {e}. Skipping file.") return NoneAlso applies to: 661-665
src/omnibase_core/models/health/model_health_check_config.py (1)
235-245: Good:# init-errors-okis the right marker for swallowingmodel_rebuild()failures during import-time circular refs.
Optional: if feasible without triggering more imports, narrowexcept Exceptionto expected Pydantic-related exceptions (or a centralized exception group) to avoid masking unrelated bugs. Based on learnings, ...src/omnibase_core/models/detection/__init__.py (1)
12-20: Good:# init-errors-okclearly documents why the broad import-time catch is acceptable here.
Optional: consider catching the narrowest expected exception set to avoid hiding unrelated failures. Based on learnings, ...src/omnibase_core/models/context/model_routing_metadata.py (1)
146-153: Optional: replacetype: ignore[return-value]with an explicitcast()after validation.
Keeps the same runtime behavior but makes the return typing cleaner/safer.src/omnibase_core/models/cli/model_result_summary.py (1)
71-85: Add a standardized marker + includecontextwhen wrapping validation errors. This is a broad catch and should be explicitly justified, andModelOnexErrorshould carry context for debugging/observability. As per coding guidelines/learnings.Proposed diff
@@ try: # Basic validation - ensure required fields exist # Override in specific models for custom validation return True except PYDANTIC_MODEL_ERRORS as e: + # boundary-ok: translate model/validation exceptions into structured ONEX error for callers raise ModelOnexError( error_code=EnumCoreErrorCode.VALIDATION_ERROR, message=f"Instance validation failed: {e}", + context={"model": self.__class__.__name__}, ) from esrc/omnibase_core/models/examples/model_example_metadata.py (1)
83-93:validate_instance()try/except is currently redundant.Since the method just returns
True, theexcept PYDANTIC_MODEL_ERRORSblock will never run; consider removing the try/except or adding actual validation logic.src/omnibase_core/utils/util_payload_migration.py (1)
28-248: Looks good; tighten the comment to match what’s actually being caught.If
PYDANTIC_MODEL_ERRORSis strictly “Pydantic/model validation” exceptions, Line 234’s mention of “dict access errors” / generic “type conversion issues” is misleading—either adjust the comment or confirm the group intentionally includes those. Based on learnings, exception groups are the preferred approach here.src/omnibase_core/services/service_validation_suite.py (1)
119-151: Consider adding a documentation marker for the exception handling inrun_all_validations.The method intentionally catches exceptions to aggregate validation results rather than fail fast. While this is correct behavior for a batch operation, adding a
# fallback-ok:or similar marker would document this intent and align with the PR's goal of standardizing exception documentation.📝 Suggested documentation marker
for validation_type in self.validators: try: result = self.run_validation(validation_type, directory, **kwargs) results[validation_type] = result - except ModelOnexError as e: + except ModelOnexError as e: # fallback-ok: aggregate errors for batch validation # ONEX framework validation errors results[validation_type] = ModelValidationResult(src/omnibase_core/utils/util_fsm_executor.py (1)
19-21: Consider adding a brief fallback marker/comment for the broad validation-group catch in numeric operators.Behavior looks right (invalid numeric coercion →
False). Given the PR’s standardization on documenting broad catches, a short note like# fallback-ok: non-numeric values fail the conditionwould make intent obvious ifVALIDATION_ERRORSis broad.Also applies to: 553-584
src/omnibase_core/contracts/contract_hash_registry.py (1)
56-58: Nice centralization; consider enrichingModelOnexErrorcontext for fingerprinting failures.The switch to
VALIDATION_ERRORS+ rethrow asModelOnexErroris consistent with the PR direction. If this error is user-facing during migrations, adding a bit more context (e.g.,contract_type=type(contract).__name__, config flags) would make diagnostics faster. As per coding guidelines, boundary errors should carry useful context.Also applies to: 181-214
src/omnibase_core/pipeline/manifest_generator.py (1)
209-248: Consider if decorator overhead is warranted for simple accumulator methods.The
@standard_error_handlingdecorator is applied to methods that primarily perform simple list/dict append operations (e.g.,record_emissionjust appends to lists,record_phase_durationsets a dict key). While consistency is valuable, these trivial operations are unlikely to raise exceptions beyond memory exhaustion.The Pydantic model instantiation in methods like
record_capability_activationandadd_dependency_edgecould benefit from the decorator asValidationErrorcould occur, but pure accumulator methods add decorator call overhead without clear benefit.This is a tradeoff between consistency and performance — acceptable if consistency is prioritized.
Also applies to: 252-275, 277-300, 304-330, 332-404, 408-429, 449-479, 483-492
src/omnibase_core/models/context/model_service_discovery_metadata.py (1)
301-323: Tighten exception tuple + avoid unlikely KeyError catch in dict coercion.
coerce_service_version()usesdict.get(), soKeyErrorshouldn’t occur; also the tuple order should be alphabetical per guidelines. (Consider also not interpolating the full dict into the error message if it may be large/noisy.) As per coding guidelines, ...Proposed tweak
- except (TypeError, KeyError, ValueError) as e: + except (TypeError, ValueError) as e: # error-ok: Pydantic field_validator requires ValueError raise ValueError( f"Invalid service_version dict format: expected {{'major': int, " f"'minor': int, 'patch': int}}, got {v}" ) from esrc/omnibase_core/decorators/decorator_convert_to_schema.py (1)
73-76: Good move to exception groups; update “tested Pydantic range” and confirm group membership.Catching
PYDANTIC_MODEL_ERRORSis consistent with the PR direction, but please verify the group still matches the set of exceptions you want to normalize here (so unexpected programming errors don’t get converted intoCONVERSION_ERROR). Also, this module says “tested through 2.11+” but dependency context indicates Pydantic 2.12.5—worth updating the messaging. As per coding guidelines, ...Minimal doc/message tweak
- # 6. Tested with: Pydantic 2.6+ through 2.11+. + # 6. Tested with: Pydantic 2.6+ through 2.12+.Also applies to: 330-353, 391-415, 447-471
src/omnibase_core/utils/util_forward_reference_resolver.py (1)
144-145:VALIDATION_ERRORSconsolidation looks good; consider updating docs to match.
The behavior is now “validation group” rather than only “TypeError/ValueError”; updating the doc sections that enumerate error types would keep this self-consistent. Based on learnings, exception-group usage is preferred.Also applies to: 321-330, 478-487
src/omnibase_core/mixins/mixin_event_listener.py (1)
472-483: Broad catches are well-documented, but consider explicitly re-raising cancellations if any async path can reach these blocks.
Right now theseexcept Exceptionblocks will also catchasyncio.CancelledErroron some Python versions; if any of these handlers can run inside an event loop context, consider adding anexcept asyncio.CancelledError: raisebefore the catch-all. (If these are strictly thread-only, current behavior is fine.)Also applies to: 557-565, 642-658
src/omnibase_core/services/registry/service_registry_provider.py (1)
53-56: LGTM: consistent_impl+@standard_error_handlingadoption for provider registry.
One small readability tweak:_find_by_capability_implcould callp.has_capability(capability)to centralize capability semantics on the model (if that’s the intended “exact match” behavior).Also applies to: 172-190, 295-306
| from pydantic import ValidationError | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Check if exception_groups.py exists and show its definitions
set -euo pipefail
python3 << 'PY'
import pathlib
import re
p = pathlib.Path("src/omnibase_core/errors/exception_groups.py")
print("=== Exception Groups File Check ===")
print(f"exists: {p.exists()}")
if p.exists():
txt = p.read_text(encoding="utf-8")
# Extract all group assignments
groups = {}
for match in re.finditer(r'^(\w+)\s*=\s*\(([^)]+)\)', txt, re.MULTILINE):
group_name = match.group(1)
exceptions = match.group(2)
groups[group_name] = exceptions.strip()
print(f"\nFound {len(groups)} exception groups:")
for name in sorted(groups.keys()):
exc_list = groups[name]
print(f"\n{name}:")
print(f" {exc_list[:150]}{'...' if len(exc_list) > 150 else ''}")
else:
print("exception_groups.py not found!")
PYRepository: OmniNode-ai/omnibase_core
Length of output: 144
🏁 Script executed:
# Step 2: Inspect the target file to see the exception handling patterns at the mentioned lines
cat -n src/omnibase_core/mixins/mixin_node_service.py | head -100Repository: OmniNode-ai/omnibase_core
Length of output: 4323
🏁 Script executed:
# Step 3: Check lines 424-437, 637-650, 678-691 to see the repeated tuples
sed -n '38,45p; 424,440p; 637,655p; 678,695p' src/omnibase_core/mixins/mixin_node_service.py | cat -nRepository: OmniNode-ai/omnibase_core
Length of output: 2843
🏁 Script executed:
cat src/omnibase_core/errors/exception_groups.pyRepository: OmniNode-ai/omnibase_core
Length of output: 4378
🏁 Script executed:
# Also get the full context of the repeated exception tuples in the target file
sed -n '38,39p' src/omnibase_core/mixins/mixin_node_service.py
sed -n '424,437p' src/omnibase_core/mixins/mixin_node_service.py
sed -n '637,650p' src/omnibase_core/mixins/mixin_node_service.py
sed -n '678,691p' src/omnibase_core/mixins/mixin_node_service.pyRepository: OmniNode-ai/omnibase_core
Length of output: 1532
Use centralized PYDANTIC_MODEL_ERRORS exception group for most service-lookup cases; clarify RuntimeError handling.
The same exception tuple is repeated across _subscribe_to_tool_invocations, _emit_tool_response, and _emit_shutdown_event. Per coding guidelines, use PYDANTIC_MODEL_ERRORS (AttributeError, TypeError, ValidationError, ValueError) instead of custom tuples. The RuntimeError in your current tuple is unusual for this context—consider either removing it if it's not needed, or documenting why defensive handling of runtime failures is necessary here. Additionally, extract a single _try_get_event_bus_from_container() helper to eliminate repetition and reduce drift.
The # fallback-ok: marker comments are correctly applied and appropriately document intent.
🤖 Prompt for AI Agents
In @src/omnibase_core/mixins/mixin_node_service.py around lines 38 - 39, Replace
the repeated custom exception tuples in _subscribe_to_tool_invocations,
_emit_tool_response, and _emit_shutdown_event with the centralized
PYDANTIC_MODEL_ERRORS (which covers AttributeError, TypeError, ValidationError,
ValueError) and remove RuntimeError from that tuple unless you can justify and
document why RuntimeError must be caught defensively; then extract the repeated
container-access logic into a single helper _try_get_event_bus_from_container()
that returns the event bus or None (used by the three methods with the existing
"# fallback-ok:" behavior) to eliminate duplication and reduce drift.
- Add async support to error handling decorators (standard_error_handling, validation_error_handling, io_error_handling) with proper CancelledError propagation - Use centralized PYDANTIC_MODEL_ERRORS in mixin_node_service.py with module-level constant to satisfy mypy - Add context dict to ModelOnexError wrappings for better debugging - Improve type-robustness in mode="before" validators with isinstance checks - Replace type: ignore[return-value] with explicit cast() calls - Remove redundant validate_instance() try/except blocks - Add explicit asyncio.CancelledError handling in async code paths - Update ERROR_HANDLING_BEST_PRACTICES.md with exception groups docs - Add 23 new async tests for decorator behavior
PR Review: Standardize Error Handling Patterns (OMN-1299)SummaryThis PR establishes comprehensive and consistent error handling patterns across omnibase_core. The changes are well-structured, extensively documented, and follow ONEX architectural principles. Overall: APPROVED with minor recommendations. ✅ Strengths1. Excellent Documentation
2. Centralized Exception GroupsThe
3. Standardized Comment Markers (84 occurrences)The 6 markers (
Example from except (*PYDANTIC_MODEL_ERRORS, ModelOnexError) as e:
# fallback-ok: fingerprint computation is optional for non-contract models
logger.debug("Fingerprint computation skipped for %s: %s", ...)4. Decorator Adoption (7 → 50 usages)The
5. Comprehensive Test Coverage (362 new lines)
🔍 Code Quality AnalysisArchitecture Adherence ✅
Error Handling Patterns ✅
Security Considerations ✅
Performance Impact ✅
💡 Minor Recommendations1. Documentation Consistency (CLAUDE.md vs ERROR_HANDLING_BEST_PRACTICES.md)Some content is duplicated between:
Recommendation: Consider adding a cross-reference in CLAUDE.md instead of duplicating: For detailed exception group patterns and usage examples, see [Error Handling Best Practices](docs/conventions/ERROR_HANDLING_BEST_PRACTICES.md#centralized-exception-groups).2. Test Naming Convention
3. ValueError Documentation CommentsThe PR adds 26
4. Exception Group Usage VerificationWith 51 files now using exception groups (up from 17), consider:
🎯 Test Plan VerificationBased on PR description, please confirm:
Note: I cannot execute these directly, but based on code review:
📊 Impact Assessment
🚀 Recommendation: APPROVEThis PR represents a significant improvement to omnibase_core's error handling consistency and maintainability. The implementation:
Merge ChecklistBefore merging, please verify:
Great work on establishing these patterns! This will significantly improve error handling consistency across the ONEX framework. 🎉 Review generated by Claude Sonnet 4.5 via Claude Code CLI |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/omnibase_core/models/context/model_metrics_context.py (1)
238-262: Inconsistent exception type:_validate_semverraisesModelOnexError, notValueError.The docstring states this raises
ValueError, and the type check at lines 257-261 correctly raisesValueError. However,_validate_semver()raisesModelOnexError.Pydantic 2.x field validators must raise
ValueError,TypeError, orAssertionErrorfor proper validation error handling.ModelOnexErroris not automatically converted to aValidationError, leading to inconsistent behavior: type errors (caught by Pydantic) vs. format errors (potentially propagating asModelOnexError).All other field validators in this file (trace_id, span_id, sampling_rate) correctly raise
ValueErrordirectly. Align this validator with the established pattern.🔧 Option 1: Catch and re-raise as ValueError
if not isinstance(value, str): # error-ok: Pydantic field_validator requires ValueError raise ValueError( f"service_version must be a string, got {type(value).__name__}" ) - return _validate_semver(value) + try: + return _validate_semver(value) + except ModelOnexError as e: + # error-ok: Pydantic field_validator requires ValueError + raise ValueError(str(e.message)) from e🔧 Option 2: Inline the validation logic
if not isinstance(value, str): # error-ok: Pydantic field_validator requires ValueError raise ValueError( f"service_version must be a string, got {type(value).__name__}" ) - return _validate_semver(value) + if not value: + # error-ok: Pydantic field_validator requires ValueError + raise ValueError("Semantic version cannot be empty") + if not _SEMVER_PATTERN.match(value): + # error-ok: Pydantic field_validator requires ValueError + raise ValueError(f"Invalid semantic version format: '{value}'") + return valuesrc/omnibase_core/models/core/model_state_contract.py (1)
280-284:PYDANTIC_MODEL_ERRORSis missingKeyErrorfrom the original exception handling.The original code caught
(AttributeError, KeyError, TypeError, ValueError), butPYDANTIC_MODEL_ERRORSonly contains(AttributeError, TypeError, ValidationError, ValueError). This removesKeyErrorhandling, which could cause dict access errors duringmodel_validate()to propagate uncaught. Either use a different exception group that includes all required exceptions, or revert to the explicit tuple that matches the original intent.
🧹 Nitpick comments (7)
src/omnibase_core/logging/logging_emit.py (1)
226-291: Consider addingfunctools.wrapsfor complete metadata preservation.The
cast(F, wrapper)correctly satisfies static type checkers, but the comment about preserving the function signature "via duck typing" is slightly misleading—castis a no-op at runtime. Without@functools.wraps(func), the wrapper loses__name__,__doc__, and other metadata, which can affect debugging tools and introspection.♻️ Suggested improvement
+from functools import wraps + def trace_function_lifecycle[F: Callable[..., Any]](func: F) -> F: ... + @wraps(func) def wrapper(*args: Any, **kwargs: Any) -> Any: ... - # Wrapper preserves original function signature via duck typing + # cast() satisfies static type checkers; @wraps preserves runtime metadata return cast(F, wrapper)src/omnibase_core/models/examples/model_example_metadata.py (1)
1-6: Import organization could be improved.The
PYDANTIC_MODEL_ERRORSimport (line 5) is placed before the module docstring (lines 8-13) and other imports. Consider moving it after the docstring and grouping with otheromnibase_coreimports for consistency.Suggested reordering
from __future__ import annotations from pydantic import Field -from omnibase_core.errors.exception_groups import PYDANTIC_MODEL_ERRORS from omnibase_core.models.errors.model_onex_error import ModelOnexError """ Example metadata model for examples collection. ... """ from pydantic import BaseModel from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode +from omnibase_core.errors.exception_groups import PYDANTIC_MODEL_ERRORS from omnibase_core.enums.enum_difficulty_level import EnumDifficultyLevelsrc/omnibase_core/decorators/decorator_error_handling.py (3)
1-5: Move import before module docstring.The
ModelOnexErrorimport at line 4 appears before the module docstring (lines 6-18). While syntactically valid, PEP 257 recommends the module docstring be the first statement. Consider moving the import after the docstring.Suggested reordering
from collections.abc import Callable from typing import Any -from omnibase_core.models.errors.model_onex_error import ModelOnexError - """ Standard error handling decorators for ONEX framework. ... """ import asyncio import functools +from omnibase_core.models.errors.model_onex_error import ModelOnexError from omnibase_core.enums.enum_core_error_code import EnumCoreErrorCode
169-180: Duck typing heuristic may misclassify some exceptions.The check
"validation" in str(e).lower()at lines 172 and 209 could misclassify errors that incidentally contain "validation" in their message (e.g., "Database validation timeout", "Authentication validation service unavailable").This is a minor concern since the fallback is
OPERATION_FAILED, which is acceptable. Consider whether explicit type checking against known validation exceptions (e.g.,pydantic.ValidationError) would be more reliable for critical paths.
272-285: Consider handling additional file system errors.The decorator handles
FileNotFoundError,IsADirectoryError, andPermissionError. You might also considerNotADirectoryErrorfor operations that expect a directory but receive a file. However, the current coverage is reasonable for most use cases.tests/unit/decorators/test_error_handling_extended.py (1)
527-535: Moveasyncioimport to module level.The
import asynciostatement is repeated inside each async test method (e.g., lines 527, 540, 558, 577, 601, etc.). Sinceasynciois already used by the decorators being tested, consider importing it once at the module level alongside the other imports.Suggested change at module level
+import asyncio + import pytest from omnibase_core.decorators.decorator_error_handling import (Then remove the local imports inside each test method.
src/omnibase_core/models/context/model_metrics_context.py (1)
197-199: Error message hardcodes "span_id" but validator also handles "parent_span_id".When validation fails for
parent_span_id, the error message will incorrectly say "span_id must be a string". Consider using a generic message or addingValidationInfoparameter to access the actual field name.♻️ Suggested improvement
- def validate_span_id(cls, value: str | None) -> str | None: + def validate_span_id(cls, value: str | None, info: ValidationInfo) -> str | None: ... if not isinstance(value, str): # error-ok: Pydantic field_validator requires ValueError - raise ValueError(f"span_id must be a string, got {type(value).__name__}") + raise ValueError(f"{info.field_name} must be a string, got {type(value).__name__}")This requires importing
ValidationInfofrom pydantic:from pydantic import BaseModel, ConfigDict, Field, field_validator, ValidationInfo
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/contracts/contract_hash_registry.pysrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/logging/logging_emit.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/models/cli/model_cli_execution_config.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/context/model_action_execution_context.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/context/model_error_metadata.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/context/model_node_init_metadata.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/models/context/model_service_discovery_metadata.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/models/core/model_state_contract.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/models/trace/model_execution_trace_step.pysrc/omnibase_core/services/replay/service_corpus_replay_orchestrator.pysrc/omnibase_core/services/service_contract_validation_event_emitter.pytests/unit/decorators/test_error_handling_extended.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/omnibase_core/mixins/mixin_effect_execution.py
- src/omnibase_core/models/trace/model_execution_trace_step.py
- src/omnibase_core/models/cli/model_cli_execution_config.py
- src/omnibase_core/models/context/model_service_discovery_metadata.py
- src/omnibase_core/models/core/model_field_accessor.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 (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
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 to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
src/omnibase_core/logging/logging_emit.pysrc/omnibase_core/models/context/model_node_init_metadata.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/cli/model_result_summary.pytests/unit/decorators/test_error_handling_extended.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/services/service_contract_validation_event_emitter.pysrc/omnibase_core/models/core/model_state_contract.pysrc/omnibase_core/contracts/contract_hash_registry.pysrc/omnibase_core/models/context/model_action_execution_context.pysrc/omnibase_core/services/replay/service_corpus_replay_orchestrator.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_error_metadata.py
**/src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/omnibase_core/**/*.py: Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, runtime_, service_*, etc.
Ensure 100% mypy strict mode compliance (0 errors) and pyright basic mode compliance across all source files in src/omnibase_core
Files:
src/omnibase_core/logging/logging_emit.pysrc/omnibase_core/models/context/model_node_init_metadata.pysrc/omnibase_core/models/context/model_routing_metadata.pysrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/context/model_session_context.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/services/service_contract_validation_event_emitter.pysrc/omnibase_core/models/core/model_state_contract.pysrc/omnibase_core/contracts/contract_hash_registry.pysrc/omnibase_core/models/context/model_action_execution_context.pysrc/omnibase_core/services/replay/service_corpus_replay_orchestrator.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/context/model_authorization_context.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_error_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/decorators/test_error_handling_extended.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Applied to files:
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/core/model_state_contract.pysrc/omnibase_core/contracts/contract_hash_registry.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Applied to files:
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Applied to files:
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/core/model_state_contract.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_error_metadata.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Applied to files:
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/decorators/decorator_error_handling.pytests/unit/decorators/test_error_handling_extended.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Applied to files:
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/models/core/model_state_contract.pysrc/omnibase_core/contracts/contract_hash_registry.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/src/omnibase_core/**/*.py : Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_*, container_*, decorator_*, enum_*, error_*, factory_*, mixin_*, model_*, node_*, runtime_*, service_*, etc.
Applied to files:
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.mdsrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/models/model_*.py : Do not use from __future__ import annotations when runtime type introspection is needed (Pydantic models, FastAPI endpoints) - use only when forward references or circular import prevention is needed
Applied to files:
src/omnibase_core/services/service_contract_validation_event_emitter.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use ModelONEXContainer for dependency injection in node __init__ methods, never ModelContainer - they are different types with different purposes
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use mixin composition for cross-cutting concerns: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowExecution
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use protocol names for service resolution (container.get_service('ProtocolEventBus')) - never use concrete class names
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
🧬 Code graph analysis (6)
src/omnibase_core/decorators/decorator_error_handling.py (2)
src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-255)
tests/unit/decorators/test_error_handling_extended.py (4)
src/omnibase_core/decorators/decorator_error_handling.py (3)
standard_error_handling(26-123)validation_error_handling(126-229)io_error_handling(232-338)src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
value(33-35)src/omnibase_core/models/errors/model_onex_error.py (3)
ModelOnexError(49-354)error_code(253-255)message(248-250)src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/models/core/model_state_contract.py (1)
src/omnibase_core/models/primitives/model_semver.py (1)
parse_semver_from_string(523-583)
src/omnibase_core/models/context/model_metrics_context.py (2)
src/omnibase_core/protocols/event_bus/protocol_event_message.py (1)
value(33-35)src/omnibase_core/models/common/model_numeric_value.py (1)
float_value(112-114)
src/omnibase_core/models/examples/model_example_metadata.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/mixins/mixin_node_service.py (2)
src/omnibase_core/mixins/mixin_introspection_publisher.py (1)
_extract_node_name(187-214)tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py (1)
_extract_node_name(52-54)
🪛 LanguageTool
docs/conventions/ERROR_HANDLING_BEST_PRACTICES.md
[grammar] ~460-~460: Use a hyphen to join words.
Context: ...ups ONEX provides centralized exception type tuples for consistent error handlin...
(QB_NEW_EN_HYPHEN)
⏰ 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: claude-review
- GitHub Check: Code Quality
🔇 Additional comments (53)
src/omnibase_core/models/context/model_authorization_context.py (2)
126-128: Good defensive type check for "before" validator.The
isinstancecheck is appropriate for amode="before"validator since Pydantic may pass arbitrary types before coercion. The# error-ok:comment correctly documents whyValueErroris used, aligning with the standardized exception marker conventions.
110-135: LGTM — validator implementation is sound.The complete validator correctly handles:
Nonepassthrough (line 124-125)- Type checking for non-string inputs (lines 126-128)
- ISO 8601 format validation with proper exception chaining (lines 129-134)
The
# error-ok:comments appropriately document the intentional use ofValueErroras required by Pydantic field validators. Based on coding guidelines, this follows the standardized exception comment markers pattern.src/omnibase_core/models/context/model_routing_metadata.py (2)
20-20: LGTM — importingcastfor type-safe returns.Good addition to support the explicit cast on line 153.
140-153: Well-structured validator improvements.The changes improve type safety and documentation:
# error-ok:comments — Correctly document thatValueErroris intentional per Pydantic's validator contract. This aligns with the project's standardized exception comment markers.
cast(LoadBalanceStrategy, normalized)— Good replacement for# type: ignore[return-value]. The cast is safe because the membership check on line 146 guaranteesnormalizedis one of the valid literals, andVALID_LOAD_BALANCE_STRATEGIESexactly mirrors theLoadBalanceStrategyLiteral definition.One maintenance note: if
LoadBalanceStrategyorVALID_LOAD_BALANCE_STRATEGIESis updated in the future, both must stay in sync for the cast to remain valid. The current definitions on lines 32-37 are correctly aligned.src/omnibase_core/models/context/model_node_init_metadata.py (2)
102-106: LGTM! Good defensive type check for "before" validator.The
isinstancecheck properly handles the case where raw input (before Pydantic coercion) may not be a string, preventing potentialAttributeErroron the subsequent.replace()call. The# error-ok:marker correctly documents the intentionalValueErrorusage per project conventions.
110-114: Appropriate error marker added.The
# error-ok: Pydantic field_validator requires ValueErrorcomment correctly documents whyValueErroris used instead of a custom error type here, aligning with the project's error handling documentation standards.src/omnibase_core/models/context/model_session_context.py (2)
132-139: LGTM! Error documentation comments align with project conventions.The
# error-ok: Pydantic field_validator requires ValueErrorcomments correctly document the intentional use of ValueError, which is required by Pydantic's validation contract. Based on learnings, this follows the standardized exception comment markers pattern.
170-185: LGTM! Type guard improves robustness of the "before" validator.The explicit type check on lines 174-176 is appropriate since Pydantic "before" validators receive raw input before type coercion, meaning any type can arrive regardless of the type annotation. This provides a clearer error message than letting
ipaddress.ip_address()fail with a confusing error on non-string input.The
# error-ok:comments correctly document the intentional ValueError usage per project conventions.src/omnibase_core/services/replay/service_corpus_replay_orchestrator.py (3)
526-531: LGTM! Explicit CancelledError handling improves clarity.The explicit
asyncio.CancelledErrorhandling is a good practice. While in Python 3.8+CancelledErrorinherits fromBaseException(notException) and wouldn't be caught by line 531 anyway, the explicit block provides clear documentation of intent and protects against accidental mishandling if someone later modifies the exception handling. Theboundary-ok:marker on the broadexcept Exceptioncorrectly documents its purpose for retry tracking.
471-475: Verify external cancellation propagates correctly withreturn_exceptions=True.The
return_exceptions=Trueparameter captures exceptions (includingCancelledErrorfrom individual tasks) as results rather than raising them. This is appropriate for preventing one task's cancellation from disrupting sibling tasks. However, if external cancellation of thereplay()coroutine is needed (beyond the cooperativecancel()method), verify thatasyncio.gatheritself propagates cancellation when its parent task is cancelled.
600-605: LGTM! Proper resilience pattern for callback invocation.The broad exception catch with
tool-resilience-ok:marker correctly prevents callback failures from crashing the replay orchestration. Logging the warning provides observability without disrupting the primary workflow. This follows the coding guidelines for standardized exception comment markers.src/omnibase_core/models/context/model_action_execution_context.py (4)
25-25: LGTM!The
castimport is correctly added to support the type-safe return on line 166.
155-157: LGTM!The
# error-ok:comment appropriately documents the intentional use ofValueErroras required by Pydantic's field_validator contract. Based on coding guidelines, this follows the standardized exception comment marker pattern.
160-164: LGTM!Consistent use of the
# error-ok:marker with a clear, actionable error message that includes both the invalid value and the allowed options.
165-166: LGTM!The
cast()usage is type-safe and the correct approach here. After the set membership check confirmsnormalizedis one of the three allowed values,cast()explicitly tells mypy about the narrowed type. This is preferable to# type: ignorecomments and aligns with mypy strict mode compliance requirements.src/omnibase_core/models/context/model_error_metadata.py (2)
199-209: LGTM! Robust type validation with correct boolean exclusion.The logic correctly handles the Python quirk where
boolis a subclass ofint. The explicitisinstance(value, bool)check properly rejects boolean values that would otherwise pass the integer check. The# error-ok:markers follow the standardized exception comment markers convention.
239-251: LGTM! Clear validation hierarchy with actionable error messages.The validation order (type → empty → pattern) provides progressively specific error messages. The hint about
checker_workflow_linterfor lint-style codes (W001, E001) is helpful for developers encountering format mismatches.src/omnibase_core/logging/logging_emit.py (2)
11-11: LGTM!The
castimport supports the improved type-safe decorator pattern used intrace_function_lifecycleandlog_performance_metrics.
268-269: LGTM!The
# boundary-ok:marker correctly documents that this decorator catches exceptions solely to log them before re-raising, following the standardized exception comment markers. As per coding guidelines, this is the appropriate marker for boundary layer exception handling.src/omnibase_core/contracts/contract_hash_registry.py (4)
57-57: LGTM!Import of
VALIDATION_ERRORSfrom centralized exception groups aligns with coding guidelines.
282-288: LGTM!Good defensive truncation of
version_valueto 100 characters prevents potentially large values from bloating error logs. The additionalcontract_typecontext improves error diagnostics.
367-372: LGTM!Using
repr(contract_name)is a good choice—it distinguishes between'',None, and other falsy values in the error context, aiding debugging.
187-217: Enhanced error context and centralized exception group usage is correct.The enriched
ModelOnexErrorwithactual_type,contract_type, andoriginal_error_typecontext significantly improves debuggability. UsingVALIDATION_ERRORSaligns with coding guidelines for centralized exception groups and correctly includesTypeError,ValueError, andValidationError—all of which can be raised during normalization, type conversion, and JSON serialization. The preserved error context (original_error,original_error_type) ensures that underlying errors remain traceable.src/omnibase_core/models/examples/model_example_metadata.py (2)
66-77: LGTM!The
configuremethod correctly uses the centralizedPYDANTIC_MODEL_ERRORSexception group and wraps errors inModelOnexErrorwith the appropriate error code. This aligns with the project's error handling standards.
83-92: LGTM!The simplified
validate_instancebase implementation that returnsTrueunconditionally is appropriate. The docstring clearly communicates that subclasses should override with custom validation logic and handle specific exceptions. This pattern is consistent with other models in this PR.docs/conventions/ERROR_HANDLING_BEST_PRACTICES.md (4)
458-472: LGTM! Comprehensive exception groups documentation.The new "Centralized Exception Groups" section provides clear, actionable guidance with import examples and usage patterns. This will help developers consistently apply the standardized error handling across the codebase.
497-514: LGTM! Critical async cancellation guidance.The documentation clearly explains why
asyncio.CancelledErrormust always be re-raised and is intentionally excluded from exception groups. The code example demonstrates the correct pattern. This aligns perfectly with the async handling changes throughout the PR.
515-561: LGTM! Well-documented exception handler markers.The marker table and usage examples provide clear guidance for documenting intentional broad exception catches. This supports code review quality and static analysis tooling.
1812-1813: LGTM!The new cross-references to the centralized exception groups and error handling decorators implementation files are helpful for developers navigating the documentation.
src/omnibase_core/mixins/mixin_node_service.py (8)
61-66: LGTM! Well-designed module-level exception tuple.The
_SERVICE_LOOKUP_ERRORSconstant correctly combinesPYDANTIC_MODEL_ERRORSwithRuntimeErrorat module level. The comment explains why this is needed (mypy limitation with tuple unpacking in except clauses). This follows the centralized exception groups pattern while accommodating Python/mypy constraints.
164-172: LGTM! Proper async cancellation handling.The explicit
asyncio.CancelledErrorcatch with immediate re-raise follows the documented best practice. The comment correctly notes that cleanup is handled by the caller invokingstop_service_mode().
221-229: LGTM! Robust shutdown handling with proper cancellation semantics.The
asyncio.CancelledErrorhandling correctly sets_service_running = Falsebefore re-raising, ensuring consistent state even during cancellation. Thecleanup-resilience-okmarker documents the intentional broad catch for non-cancellation errors.
352-355: LGTM! Correct cancellation handling in tool invocation.The
asyncio.CancelledErrorcatch is correctly placed after specific exception handling but before the fallbackExceptioncatch. The comment correctly notes that thefinallyblock will still execute to clean up_active_invocations.
444-459: LGTM! Enhanced error context for service lookup failures.The use of
_SERVICE_LOOKUP_ERRORScentralizes exception handling, and theModelOnexErrornow includesnode_typeandsubscription_topicfor better debugging context when the event bus is unavailable.
615-624: LGTM! Improved error context for None result validation.The
ModelOnexErrorfor None results now includesnode_typeandnode_name, providing better debugging context when nodes return invalid results.
654-661: LGTM!Consistent use of
_SERVICE_LOOKUP_ERRORSfor service lookup exception handling.
689-696: LGTM!Consistent use of
_SERVICE_LOOKUP_ERRORSfor service lookup exception handling with appropriatefallback-okmarker.src/omnibase_core/services/service_contract_validation_event_emitter.py (4)
53-53: LGTM!The
asyncioimport is correctly added to support the newCancelledErrorhandling.
264-269: LGTM! Critical async cancellation fix in emit loop.The
asyncio.CancelledErroris correctly caught before the generalExceptioncatch and re-raised immediately. This prevents cancellation from being incorrectly collected in theerrorslist and ensures proper task cancellation propagation.
300-305: LGTM!Consistent
asyncio.CancelledErrorhandling pattern withemit().
331-340: LGTM! State-aware cancellation handling in close().The
close()method correctly sets_closed = Truebefore re-raisingCancelledError. This ensures the emitter is marked as closed even during cancellation, preventing subsequent operations on a partially-closed emitter.src/omnibase_core/models/cli/model_cli_debug_info.py (1)
198-207: LGTM!The simplified
validate_instancebase implementation is consistent with the pattern inmodel_example_metadata.py. The docstring clearly communicates that subclasses should override with custom validation logic and handle specific exceptions.src/omnibase_core/decorators/decorator_error_handling.py (1)
69-122: LGTM - Clean async/sync branching with proper exception propagation.The decorator correctly:
- Uses
asyncio.iscoroutinefunctionfor detection- Propagates cancellation/exit signals without catching
- Re-raises
ModelOnexErrorto preserve context- Wraps other exceptions with enriched metadata (
original_error_type,operation)- Uses
functools.wrapsto preserve function metadataThe
boundary-ok:comment markers align with the coding guidelines.tests/unit/decorators/test_error_handling_extended.py (3)
520-608: Excellent async test coverage forstandard_error_handling.Tests comprehensively cover:
- Successful async execution
- Exception wrapping with proper error codes
ModelOnexErrorpropagation without double-wrappingCancelledErrorpropagation (critical for task cleanup)- Function metadata preservation
- Coroutine function detection
844-879: Critical signal propagation tests ensure proper shutdown semantics.These tests verify that
KeyboardInterrupt,SystemExit, andGeneratorExitare never caught by the decorators, which is essential for proper process termination and asyncio task cleanup. This directly validates the contract documented in the decorator docstrings.
1-14: Comprehensive test module with proper documentation.The module docstring clearly outlines coverage targets (90%+ for decorator module) and test scope. Test organization into classes by decorator type and sync/async variants is clean and maintainable.
src/omnibase_core/models/cli/model_result_summary.py (1)
68-77: Simplification to base implementation is appropriate.The change to always return
Truein the base class and defer custom validation to subclasses is correct. The updated docstring clearly communicates subclass responsibilities. ModelResultSummary has no subclasses in the codebase, so there is no risk of breaking existing code.src/omnibase_core/models/context/model_metrics_context.py (2)
152-179: LGTM!The type guard addition is a solid defensive measure for "before" mode validators, providing clear error messages with the actual type received. The lowercase normalization before pattern matching ensures consistent handling of mixed-case inputs.
209-236: LGTM!Good defensive handling:
- The
isinstance(value, bool)check correctly prevents booleans from passing as integers (sinceboolis a subclass ofintin Python).- Explicit conversion to float with range validation ensures consistent return type.
- Clear error messages for both type and range violations.
src/omnibase_core/models/core/model_state_contract.py (4)
41-41: LGTM!Using the centralized
PYDANTIC_MODEL_ERRORSexception group aligns with the coding guidelines for standardized error handling.
176-185: Explicit type handling improves clarity.The changes properly handle each input type with clear control flow and provide a descriptive error message for unsupported types. The inline import avoids potential circular import issues, and the
# error-ok:marker correctly documents the intentionalValueErrorusage per Pydantic requirements.
197-206: Consistent pattern withvalidate_contract_version.The implementation follows the same clear structure, ensuring consistency across version validators.
305-314: Remove ineffective exception handler;load_and_validate_yaml_modelalready wraps all errors inModelOnexError.The current code attempts to catch
PYDANTIC_MODEL_ERRORS, butload_and_validate_yaml_modeldoesn't raise these exceptions directly—it wraps all errors (validation, file I/O, YAML parsing) inModelOnexErrorbefore propagating them. This makes the except block dead code that will never execute.Either remove the try/except entirely to let
ModelOnexErrorpropagate naturally, or if re-wrapping is necessary, catchModelOnexErrorspecifically.Likely an incorrect or invalid review comment.
- Fix CI test: update test_audit_with_permission_denied to handle ModelOnexError from @standard_error_handling decorator - Fix span_id error message: use ValidationInfo for dynamic field names in model_metrics_context.py (handles both span_id and parent_span_id) - Improve import organization: move imports after module docstrings - Move asyncio imports to module level in mixin_event_listener.py and mixin_hybrid_execution.py - Add @functools.wraps to decorator_allow_any_type.py for metadata preservation - Add fallback markers to VALIDATION_ERRORS catches in util_fsm_executor and util_uuid_service Note: The "sync decorator on async methods" issue was a false positive - service_protocol_migrator.py has no async methods and decorator already supports async via asyncio.iscoroutinefunction().
Code Review: PR #374 - Standardize Error Handling PatternsSummaryThis PR establishes comprehensive and consistent error handling patterns across omnibase_core. The changes are well-structured, thoroughly documented, and significantly improve code maintainability. ✅ Strengths1. Excellent Documentation (CLAUDE.md)
2. Decorator ImplementationThe three decorators eliminate boilerplate effectively:
All decorators properly:
3. Centralized Exception GroupsExcellent consolidation in
4. Comprehensive Test Coverage (test_error_handling_extended.py)
5. Consistent Application
6. Implementation Pattern (factory_contract_profile.py)Clean separation pattern: def get_default_contract_profile(...):
return _get_default_contract_profile_impl(...)
@standard_error_handling("Contract profile resolution")
def _get_default_contract_profile_impl(...):
# Implementation with automatic error handlingThis keeps public API clean while adding error handling internally. 🔍 Areas for Consideration1. Performance Impact (Minor)The decorator wrapping adds a small overhead to every decorated function call. For hot paths (called >1000x/sec), consider:
Recommendation: Accept this trade-off for maintainability unless profiling shows issues. 2. Error Message ConsistencySome operation names could be more consistent: @standard_error_handling("Contract profile resolution") # Good
@standard_error_handling("Handler registration") # Good
@standard_error_handling("Orchestrator profile resolution") # GoodRecommendation: Consider documenting operation name conventions (e.g., " " pattern). 3. Async Decorator Auto-DetectionThe decorators use
Recommendation: This is fine - standard pattern in Python ecosystem. 4. Missing Exception Groups DocumentationWhile CLAUDE.md documents exception groups well, the
Recommendation: Add module-level docstring to 🐛 Potential Issues1. No Issues FoundI reviewed:
All implementations follow best practices. 🔒 Security
⚡ Performance
🧪 Test CoverageExcellent test coverage:
Recommendation: Verify CI passes with new tests. 📋 RecommendationsPriority: High
Priority: Medium (Follow-up PRs)
Priority: Low (Nice-to-have)
🎯 Final AssessmentApprove with high confidence. This PR represents a significant improvement to error handling consistency and maintainability:
The decorator pattern eliminates boilerplate while maintaining full error context. The centralized exception groups prevent tuple drift. The documentation markers make catch-all handlers self-documenting. Impact:
Recommendation: ✅ LGTM - Approve and merge |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/omnibase_core/models/context/model_metrics_context.py:
- Around line 261-266: The field validator for service_version currently raises
ValueError for non-strings but calls _validate_semver which raises
ModelOnexError on format issues; change the validator to catch ModelOnexError
around the _validate_semver(value) call and re-raise a ValueError (including the
original error message) so Pydantic receives a ValueError/AssertionError; e.g.,
wrap the _validate_semver call in try/except ModelOnexError and raise
ValueError(str(err)) to preserve semantics without leaking ModelOnexError.
🧹 Nitpick comments (5)
src/omnibase_core/mixins/mixin_hybrid_execution.py (1)
224-227: Consider moving remaining inline imports to module level for consistency.The
uuid4,EnumWorkflowStatus, andcastimports (lines 224-227, 248) remain inline whileasyncioandtimewere moved to module level. For consistency, consider moving these to the top of the file in a follow-up.src/omnibase_core/decorators/decorator_allow_any_type.py (1)
29-36: Good addition offunctools.wrapsfor metadata preservation.The wrapper pattern correctly preserves function metadata and works transparently for sync, async, and generator functions. This aligns with the decorator patterns used elsewhere in the codebase (e.g.,
decorator_error_handling.py).Optional: The docstring on line 26 states the function is "unchanged (documentation only)", but now a wrapper layer is added. Consider updating to clarify that behavior is unchanged while metadata is preserved via wrapper:
📝 Suggested docstring update
Returns: - Decorated function unchanged (documentation only) + Decorated function with preserved metadata (behavior unchanged)src/omnibase_core/decorators/decorator_error_handling.py (3)
167-178: Duck-typing heuristic for validation errors is fragile.The check
hasattr(e, "errors") or "validation" in str(e).lower()may produce false positives:
- Any exception message containing "validation" (e.g.,
"Input validation required before processing") triggersVALIDATION_ERROReven if not a validation exception.- Non-Pydantic exceptions with an
errorsattribute would also match unexpectedly.Consider using explicit type checks against the centralized
VALIDATION_ERRORSorPYDANTIC_MODEL_ERRORSexception groups for more precise classification.♻️ Suggested refinement using exception groups
from omnibase_core.errors.exception_groups import VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS # Then in the except block: except Exception as e: if isinstance(e, (*VALIDATION_ERRORS, *PYDANTIC_MODEL_ERRORS)): # Handle as validation error ...
254-336: LGTM with optional enhancement consideration.The decorator correctly handles common file I/O exceptions with appropriate error codes. The async/sync branching follows the same well-structured pattern as the other decorators.
Consider using the centralized
FILE_IO_ERRORSexception group for broader coverage of I/O-related exceptions, which would also maintain consistency with the PR's objective of adoptingexception_groups.pyacross more files.
67-119: Optional: Consider extracting common exception handling logic.There's notable repetition between the async and sync wrappers across all three decorators (signal propagation,
ModelOnexErrorre-raise, metadata enrichment). A helper function could reduce duplication:def _wrap_exception(e: Exception, operation_name: str, code: EnumCoreErrorCode, **extra_context: Any) -> ModelOnexError: return ModelOnexError( f"{operation_name} failed: {e!s}", code, original_error_type=type(e).__name__, operation=operation_name, **extra_context, )This is purely optional—the current explicit approach is readable and maintainable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/omnibase_core/decorators/decorator_allow_any_type.pysrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/decorators/decorator_pattern_exclusions.pysrc/omnibase_core/mixins/mixin_event_listener.pysrc/omnibase_core/mixins/mixin_hybrid_execution.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/utils/util_fsm_executor.pysrc/omnibase_core/utils/util_uuid_service.pytests/unit/validation/test_protocol_auditor.py
✅ Files skipped from review due to trivial changes (1)
- src/omnibase_core/decorators/decorator_pattern_exclusions.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_core/mixins/mixin_event_listener.py
- src/omnibase_core/utils/util_fsm_executor.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 (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
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 to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
src/omnibase_core/mixins/mixin_hybrid_execution.pysrc/omnibase_core/decorators/decorator_allow_any_type.pytests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/utils/util_uuid_service.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/decorators/decorator_error_handling.py
**/src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/omnibase_core/**/*.py: Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, runtime_, service_*, etc.
Ensure 100% mypy strict mode compliance (0 errors) and pyright basic mode compliance across all source files in src/omnibase_core
Files:
src/omnibase_core/mixins/mixin_hybrid_execution.pysrc/omnibase_core/decorators/decorator_allow_any_type.pysrc/omnibase_core/utils/util_uuid_service.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/decorators/decorator_error_handling.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/validation/test_protocol_auditor.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Applied to files:
tests/unit/validation/test_protocol_auditor.pysrc/omnibase_core/decorators/decorator_error_handling.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Applied to files:
tests/unit/validation/test_protocol_auditor.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Applied to files:
src/omnibase_core/utils/util_uuid_service.pysrc/omnibase_core/decorators/decorator_error_handling.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Applied to files:
src/omnibase_core/decorators/decorator_error_handling.py
🧬 Code graph analysis (4)
src/omnibase_core/decorators/decorator_allow_any_type.py (1)
src/omnibase_core/decorators/decorator_error_handling.py (3)
wrapper(97-117)wrapper(192-223)wrapper(298-332)
tests/unit/validation/test_protocol_auditor.py (1)
src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
src/omnibase_core/utils/util_uuid_service.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/decorators/decorator_error_handling.py (2)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-255)src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
🔇 Additional comments (12)
src/omnibase_core/mixins/mixin_hybrid_execution.py (2)
8-10: LGTM!Moving
asyncioandtimeimports to module level follows PEP 8 best practices and eliminates repeated import overhead during method execution.
252-259: LGTM!The
fallback-ok:marker is semantically accurate for this use case—workflow failures gracefully degrade to direct execution with proper logging. The comment clearly documents why the broad exception catch is intentional. Based on learnings, this follows the standardized exception comment markers convention.tests/unit/validation/test_protocol_auditor.py (1)
293-296: LGTM!The exception handling correctly accounts for both the raw
PermissionErrorand the wrappedModelOnexErrorthat results from@standard_error_handlingdecorator application. The inline comments clearly document this behavior, and the assertion onstr(e)works for both types sinceModelOnexError.__str__preserves the original message.src/omnibase_core/decorators/decorator_allow_any_type.py (1)
8-10: LGTM!The import additions are appropriate —
functoolsis needed forwrapsto preserve function metadata, and the typing imports are correctly organized.src/omnibase_core/models/context/model_metrics_context.py (4)
22-22: LGTM!The addition of
ValidationInfoandConfigDictimports aligns with pydantic v2 best practices and supports the enhanced validation flow with dynamic field name awareness.
168-179: LGTM!The explicit string type check provides clear error messaging with the actual type received. The
# error-ok:markers correctly document the intentional use ofValueErrorin Pydantic validators per coding guidelines.
181-211: LGTM!Excellent use of
ValidationInfoto provide dynamic, field-aware error messages for bothspan_idandparent_span_id. This eliminates duplication while maintaining precise error context for each field.
229-240: LGTM!The explicit boolean rejection on line 229 is important since
boolis a subclass ofintin Python. Without this check,TrueandFalsewould be accepted as valid sampling rates. The conversion tofloatensures type consistency.src/omnibase_core/decorators/decorator_error_handling.py (2)
15-21: LGTM!Clean import organization using modern
collections.abc.Callableand appropriate internal module imports.
67-121: Well-structured async/sync branching with proper metadata preservation.The decorator correctly:
- Uses
asyncio.iscoroutinefunctionto select the appropriate wrapper- Applies
functools.wrapsto both wrappers for proper introspection- Propagates cancellation/exit signals without catching them
- Re-raises
ModelOnexErrorunchanged to preserve error context- Enriches wrapped exceptions with
original_error_typeandoperationmetadata- Uses
# boundary-ok:marker per coding guidelinessrc/omnibase_core/utils/util_uuid_service.py (2)
9-10: LGTM! Import aligns with centralized exception handling pattern.This follows the coding guidelines to use centralized exception groups from
omnibase_core.errors.exception_groups. Based on learnings, this is the preferred approach.
31-32: VALIDATION_ERRORS correctly includes both exceptions raised by UUID().The implementation correctly uses
VALIDATION_ERRORS(containingTypeError,ValidationError, andValueError) across all three methods (is_valid,parse, andensure_uuid). This properly handles both invalid UUID strings (ValueError) and potential non-string type mismatches (TypeError). The standardizedfallback-ok:markers are consistently applied and document the intended fallback behavior.
- Extract repeated event bus lookup logic into _try_get_event_bus_from_container() - Reduces code duplication across _subscribe_to_tool_invocations, _emit_tool_response, and _emit_shutdown_event - Net reduction of 17 lines while improving maintainability
Pull Request Review: feat(errors): standardize error handling patterns (OMN-1299)SummaryThis is an excellent, comprehensive PR that significantly improves error handling consistency across the omnibase_core codebase. The changes are well-architected, properly documented, and align with the project's ONEX framework standards. ✅ Strengths1. Exceptional Documentation Quality
2. Strong Architectural Patterns
3. Excellent Async Support
4. Robust Testing
5. Consistent Conventions
🔍 Areas for Consideration1. Minor: Exception Marker Consistency (Low Priority)Some exception markers could be more specific. For example: File: except Exception as e:
# catch-all-ok: retry loop captures failures for exponential backoff✅ Good marker, but could specify WHICH exceptions are expected (e.g., network errors, timeout errors). Suggestion: Consider documenting expected exception types in complex retry loops for future maintainers. 2. Performance: Decorator Overhead (Minor)The decorators add a try/except wrapper to EVERY decorated function call. While Python's exception handling is fast when exceptions DON'T occur, there is a small overhead. Current Impact: Negligible for most use cases (microseconds per call) 3. Documentation: Decision Matrix Coverage (Enhancement)The CLAUDE.md decision matrix (lines 420-430) is excellent but could include:
4. Type Safety: Exception Group Splat (Addressed)The PR originally had mypy issues with exception tuple splats ( 🔒 Security Review✅ No Security Concerns
🧪 Test Coverage Assessment✅ Excellent Coverage
Potential Gap (Low Priority)Consider adding integration tests that verify decorator behavior with actual ONEX pipeline components (e.g., NodeCompute with decorator, event bus error handling). Current tests are unit-level only. 📊 Code Quality Metrics
🐛 Potential Issues Found1. RESOLVED: CI Test Failure ✅Commit c468ca2 fixed 2. RESOLVED: Mypy Errors ✅Commit 786b0cd fixed exception splat patterns by using module-level constants. 3. RESOLVED: Span ID Error Messages ✅Commit c468ca2 improved span_id validation to use ValidationInfo for dynamic field names. 💡 RecommendationsMust Address Before Merge: ✅ NoneAll critical issues have been resolved in subsequent commits. Should Consider (Optional Enhancements):
Nice to Have (Future Work):
🎯 Alignment with CLAUDE.md Standards
📝 Final Verdict⭐⭐⭐⭐⭐ APPROVE WITH COMMENDATIONThis PR represents exemplary software engineering:
Impact Assessment
Merge Recommendation: ✅ APPROVEOnce CI passes, this is ready to merge. Outstanding work! 🚀 👏 KudosSpecial recognition for:
Reviewed by: Claude Sonnet 4.5 via claude-code |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_core/mixins/mixin_node_service.py (1)
451-458: Use protocol name"ProtocolEventBus"for service resolution to follow ONEX conventions.The service lookup uses
container.get_service("event_bus"), but node files in the codebase explicitly use protocol names for DI lookups. Seenode_compute.pyline 175 andnode_orchestrator.pyline 365, which callcontainer.get_service("ProtocolHandlerRegistry")with the comment "Protocol-based DI lookup per ONEX conventions". Change tocontainer.get_service("ProtocolEventBus")to align with this established pattern.
🤖 Fix all issues with AI agents
In @src/omnibase_core/mixins/mixin_node_service.py:
- Around line 61-66: The exception tuple _SERVICE_LOOKUP_ERRORS must be
alphabetically ordered; change its construction so the final tuple reads
AttributeError, RuntimeError, TypeError, ValidationError, ValueError. Locate
_SERVICE_LOOKUP_ERRORS and either explicitly list the five exception types in
that order or build it by inserting RuntimeError after the first element of
PYDANTIC_MODEL_ERRORS (e.g., place RuntimeError between AttributeError and the
remaining PYDANTIC_MODEL_ERRORS entries) so the resulting tuple is
alphabetically sorted.
🧹 Nitpick comments (1)
src/omnibase_core/models/context/model_metrics_context.py (1)
266-270: Minor: Redundantstr()wrapper.The
e.messageproperty already returns astr(as seen inModelOnexError.message), so thestr()wrapper is unnecessary.♻️ Suggested simplification
try: return _validate_semver(value) except ModelOnexError as e: # error-ok: Pydantic field_validator requires ValueError - raise ValueError(str(e.message)) from e + raise ValueError(e.message) from e
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_metrics_context.pytests/unit/models/nodes/node_services/test_model_service_effect_health.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 (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
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 to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
tests/unit/models/nodes/node_services/test_model_service_effect_health.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_metrics_context.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/nodes/node_services/test_model_service_effect_health.py
**/src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/omnibase_core/**/*.py: Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, runtime_, service_*, etc.
Ensure 100% mypy strict mode compliance (0 errors) and pyright basic mode compliance across all source files in src/omnibase_core
Files:
src/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_metrics_context.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use protocol names for service resolution (container.get_service('ProtocolEventBus')) - never use concrete class names
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_effect_health.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use ModelONEXContainer for dependency injection in node __init__ methods, never ModelContainer - they are different types with different purposes
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use mixin composition for cross-cutting concerns: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowExecution
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/src/omnibase_core/**/*.py : Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_*, container_*, decorator_*, enum_*, error_*, factory_*, mixin_*, model_*, node_*, runtime_*, service_*, etc.
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use NodeCompute, NodeReducer, NodeOrchestrator, and NodeEffect base classes - import from omnibase_core.nodes and always call super().__init__(container)
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Applied to files:
src/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_metrics_context.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Applied to files:
src/omnibase_core/models/context/model_metrics_context.py
🧬 Code graph analysis (2)
tests/unit/models/nodes/node_services/test_model_service_effect_health.py (1)
src/omnibase_core/mixins/mixin_node_service.py (1)
_try_get_event_bus_from_container(429-460)
src/omnibase_core/models/context/model_metrics_context.py (1)
src/omnibase_core/models/errors/model_onex_error.py (2)
ModelOnexError(49-354)message(248-250)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (12)
src/omnibase_core/models/context/model_metrics_context.py (4)
22-22: LGTM!The
ValidationInfoimport is correctly added for the dynamic field name access in validators.
152-179: LGTM!The explicit type check is appropriate for a
mode="before"validator where raw input may not be a string. The# error-ok:comments properly document the intentional use ofValueErroras required by Pydantic field validators. Based on learnings, this aligns with the standardized exception comment markers convention.
181-211: LGTM!Excellent use of
ValidationInfoto provide dynamic field names in error messages. This is the idiomatic Pydantic pattern when a single validator handles multiple fields (span_id,parent_span_id), resulting in clearer error messages that identify the specific field that failed validation.
213-240: LGTM!The boolean rejection at line 229 is essential since
boolis a subclass ofintin Python (isinstance(True, int)returnsTrue). The check order—boolean first, then numeric type—correctly prevents booleans from being accepted as valid sampling rates. The explicit conversion tofloatensures a consistent return type.src/omnibase_core/mixins/mixin_node_service.py (7)
164-172: LGTM: Correct CancelledError propagation.Explicitly catching and re-raising
asyncio.CancelledErrorbefore the broadexcept Exceptionhandler is the correct pattern for preserving asyncio cancellation semantics. The comment clearly documents that cleanup responsibility belongs to the caller viastop_service_mode().
221-229: LGTM: Proper cancellation handling with state cleanup.Setting
_service_running = Falsebefore re-raisingCancelledErrorensures consistent state even during cancellation. The# cleanup-resilience-ok:marker on line 227 correctly documents the intent of the broad exception handler.
352-356: LGTM: CancelledError handling preserves cleanup via finally block.Correctly re-raises
CancelledErrorwhile allowing thefinallyblock (lines 382-384) to clean up_active_invocations. This prevents the fallbackExceptionhandler from catching cancellation while ensuring proper resource cleanup.
462-478: LGTM: Clean refactoring with enhanced error context.Good use of the new
_try_get_event_bus_from_containerhelper, reducing duplication. TheModelOnexErrornow includes valuable debugging context (node_type,subscription_topic) as per coding guidelines for structured error handling.
632-638: LGTM: Enhanced error metadata for debugging.Adding
node_typeandnode_nameto theModelOnexErrorprovides valuable context for debugging validation failures. This is consistent with the enhanced error metadata pattern applied elsewhere in this file.
654-678: LGTM: Consistent use of centralized event bus lookup.Both
_emit_tool_responseand_emit_shutdown_eventnow use_try_get_event_bus_from_container, eliminating code duplication. The specific exception handling in_emit_shutdown_event(lines 677-678) appropriately ensures shutdown resilience.
207-212: LGTM: Appropriate use of fallback-ok marker.The
# fallback-ok: user callbacks must not crash shutdownmarker correctly documents why the broadexcept Exceptionis acceptable here. User-provided callbacks should never prevent graceful shutdown.tests/unit/models/nodes/node_services/test_model_service_effect_health.py (1)
95-97: LGTM: Consistent test fixture binding for new helper method.The lambda binding for
_try_get_event_bus_from_containerfollows the same pattern as other method bindings in this fixture (e.g.,get_service_health,add_shutdown_callback). This correctly routes calls through the mixin's implementation for testing.
- test_shutdown_during_active_invocation: Fix race condition by awaiting background task before assertions to ensure invocation removal completes - test_audit_with_permission_denied: Fix module caching issue by using decorator-based patching (@patch) instead of context manager, ensuring mock is applied before test code runs in pytest-xdist parallel workers
PR Review: Standardize Error Handling Patterns (OMN-1299)SummaryThis PR establishes consistent error handling patterns across omnibase_core through three key improvements:
The changes significantly improve code maintainability and error handling consistency. Overall this is excellent work with strong attention to detail. ✅ Strengths1. Comprehensive Async SupportThe decorators now properly handle both sync and async functions with correct
Example (decorator_error_handling.py:68-93): if asyncio.iscoroutinefunction(func):
@functools.wraps(func)
async def async_wrapper(*args: Any, **kwargs: Any) -> Any:
try:
return await func(*args, **kwargs)
except asyncio.CancelledError:
raise # Correct propagation2. Centralized Exception GroupsThe
3. Consistent Documentation MarkersThe standardized markers ( 4. Decorator Pattern with SeparationSmart pattern of public API + private implementation: def get_default_contract_profile(...):
return _get_default_contract_profile_impl(...)
@standard_error_handling("Contract profile resolution")
def _get_default_contract_profile_impl(...):
# ImplementationThis keeps error handling separate from function signature. 5. Thorough TestingThe new
🔍 Issues FoundCRITICAL: Missing
|
- Fix async mock issues in test_model_service_effect_invocation.py by binding _try_get_event_bus_from_container to fixtures - Fix async mock issues in test_model_service_orchestrator_health.py by adding event bus binding and fixing _get_event_bus mock - Rewrite _SERVICE_LOOKUP_ERRORS in mixin_node_service.py with explicit alphabetical ordering and RuntimeError documentation - Remove redundant str() wrapper in model_metrics_context.py Resolves 19 failing tests in CI Split 14/20.
PR Review: Error Handling Standardization (OMN-1299)OverviewThis PR successfully establishes comprehensive error handling patterns across omnibase_core. The changes are well-executed and significantly improve code maintainability and consistency. ✅ Strengths1. Excellent Documentation
2. Async Support in DecoratorsThe error handling decorators now properly support both sync and async functions:
3. Centralized Exception Groups
4. Decorator Pattern ImprovementThe separation of public API from decorated implementation is clean: def get_default_contract_profile(...) -> ModelContractBase:
"""Public API with docstring"""
return _get_default_contract_profile_impl(...)
@standard_error_handling("Contract profile resolution")
def _get_default_contract_profile_impl(...) -> ModelContractBase:
"""Internal implementation"""
# Business logicThis preserves docstrings while eliminating boilerplate. 5. Comprehensive Test Coverage
🔍 Areas for Consideration1. CancelledError Handling in Sync FunctionsThe decorators catch Current (lines 103-105 in decorator_error_handling.py): @functools.wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
try:
return func(*args, **kwargs)
except (GeneratorExit, KeyboardInterrupt, SystemExit):
raise
except asyncio.CancelledError: # Unlikely in pure sync code
raiseConsideration: This is defensive programming and doesn't hurt, but it's worth noting that sync functions shouldn't raise 2. Exception Group Tuple UnpackingIn except (*PYDANTIC_MODEL_ERRORS, ModelOnexError) as e:This pattern works but can be harder to read. Consider defining a new constant if this combination is used frequently: # In exception_groups.py
PYDANTIC_MODEL_ERRORS_WITH_ONEX = (*PYDANTIC_MODEL_ERRORS, ModelOnexError)However, the current approach is valid and mypy-compliant. 3. Context Dict ConsistencyThe decorators add
Suggestion: Document the standard context keys in 4. Marker Comment PlacementMost marker comments are well-placed, but a few are at the end of long comment blocks. For example: except Exception as e:
# fallback-ok: use defaults if config unavailable
config = DEFAULT_CONFIGvs. except Exception as e:
# This is a long explanation about why we catch all exceptions here
# and what the implications are for the system...
# fallback-ok: use defaults if config unavailableThe marker should ideally be on the first line after 5. ValueError in Pydantic ValidatorsThe 🐛 Potential Issues1. Test Flakiness AddressedThe commit history shows fixes for flaky tests in CI:
✅ These are well-addressed in commit 98ed549. 2. mypy Satisfaction PatternThe module-level 📊 Performance & Coverage
The ratio of documentation to code changes is appropriate for an error handling standardization effort. 🔐 Security Considerations✅ No security concerns identified. The error handling patterns:
✅ Recommendations
📝 Final VerdictAPPROVED ✅ This PR represents a significant improvement in code quality and maintainability:
The error handling infrastructure is now production-ready and provides a solid foundation for future development. Excellent work on this comprehensive refactoring! 🎉 Code Quality Score: 9.5/10
Review performed by Claude Code following omnibase_core CLAUDE.md conventions |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py (2)
426-430: Misplaced pytest marker on inner Pydantic model.The
@pytest.mark.unitdecorator on line 426 is applied toTestResult, which is a Pydantic model class used as test data, not a test class or function. This decorator has no effect here and should be removed.🔧 Suggested fix
- @pytest.mark.unit class TestResult(BaseModel): status: str data: str
624-767: Consider extracting duplicate mock node setup into a fixture.Both
test_empty_parametersandtest_large_result_serializationhave nearly identical mock node setup code (~45 lines each). Consider creating a fixture inTestModelServiceEffectEdgeCasessimilar to theservice_nodefixture in the sibling test class to reduce duplication and improve maintainability.♻️ Example fixture extraction
@pytest.fixture def edge_case_node(self): """Create mock node for edge case testing.""" mock_event_bus = AsyncMock() node_id_val = uuid4() node = Mock() node._node_id = node_id_val node.node_id = node_id_val node.event_bus = mock_event_bus node._get_event_bus = Mock(return_value=mock_event_bus) node._active_invocations = set() node._total_invocations = 0 node._successful_invocations = 0 node._failed_invocations = 0 node._extract_node_name = Mock(return_value="TestNode") node._serialize_result = MixinNodeService._serialize_result.__get__( node, MixinNodeService ) node._input_state_class = None node._infer_input_state_class = Mock(return_value=None) # Bind MixinNodeService methods node.handle_tool_invocation = MixinNodeService.handle_tool_invocation.__get__( node, type(node) ) node._is_target_node = MixinNodeService._is_target_node.__get__(node, type(node)) node._convert_event_to_input_state = ( MixinNodeService._convert_event_to_input_state.__get__(node, type(node)) ) node._execute_tool = MixinNodeService._execute_tool.__get__(node, type(node)) node._emit_tool_response = MixinNodeService._emit_tool_response.__get__( node, type(node) ) node._try_get_event_bus_from_container = ( MixinNodeService._try_get_event_bus_from_container.__get__(node, type(node)) ) return node, mock_event_busThen tests would just set the
runmethod behavior:async def test_empty_parameters(self, edge_case_node): node, mock_event_bus = edge_case_node async def run(input_state): return {"action": input_state.action} node.run = AsyncMock(side_effect=run) # ... rest of testsrc/omnibase_core/mixins/mixin_node_service.py (1)
639-648: Enhanced error context for None result validation.Adding
node_typeandnode_nameto the error context improves traceability when debugging validation failures from nodes returningNone.Note: This uses keyword arguments directly (
node_type=,node_name=) while_execute_tool(line 619-627) uses acontext={}dict. Both patterns appear to be supported byModelOnexError, but consider standardizing on one approach for consistency across the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_metrics_context.pytests/unit/models/nodes/node_services/test_model_service_effect_invocation.pytests/unit/models/nodes/node_services/test_model_service_orchestrator_health.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/nodes/node_services/test_model_service_orchestrator_health.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 (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
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 to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
src/omnibase_core/models/context/model_metrics_context.pytests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/mixins/mixin_node_service.py
**/src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/omnibase_core/**/*.py: Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, runtime_, service_*, etc.
Ensure 100% mypy strict mode compliance (0 errors) and pyright basic mode compliance across all source files in src/omnibase_core
Files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_service.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/nodes/node_services/test_model_service_effect_invocation.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Applied to files:
src/omnibase_core/models/context/model_metrics_context.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use ModelONEXContainer for dependency injection in node __init__ methods, never ModelContainer - they are different types with different purposes
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use protocol names for service resolution (container.get_service('ProtocolEventBus')) - never use concrete class names
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use mixin composition for cross-cutting concerns: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowExecution
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use NodeCompute, NodeReducer, NodeOrchestrator, and NodeEffect base classes - import from omnibase_core.nodes and always call super().__init__(container)
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Order exception tuples alphabetically by exception class name when catching multiple exceptions
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/src/omnibase_core/**/*.py : Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_*, container_*, decorator_*, enum_*, error_*, factory_*, mixin_*, model_*, node_*, runtime_*, service_*, etc.
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
🧬 Code graph analysis (3)
src/omnibase_core/models/context/model_metrics_context.py (1)
src/omnibase_core/models/errors/model_onex_error.py (2)
ModelOnexError(49-354)message(248-250)
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py (1)
src/omnibase_core/mixins/mixin_node_service.py (1)
_try_get_event_bus_from_container(439-470)
src/omnibase_core/mixins/mixin_node_service.py (7)
src/omnibase_core/mixins/mixin_event_listener.py (2)
event_bus(149-151)event_bus(154-156)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-255)tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py (1)
_extract_node_name(52-54)tests/unit/models/nodes/node_services/test_model_service_compute_health.py (1)
_extract_node_name(68-70)tests/unit/models/nodes/node_services/test_model_service_compute_integration.py (1)
_extract_node_name(69-71)tests/unit/models/nodes/node_services/test_model_service_reducer_health.py (1)
_extract_node_name(92-94)tests/unit/models/nodes/node_services/test_model_service_reducer_integration.py (1)
_extract_node_name(96-98)
⏰ 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 (14)
src/omnibase_core/models/context/model_metrics_context.py (4)
152-179: LGTM!The validator correctly adds runtime type checking before pattern validation. The
# error-ok:markers properly document the intentional use ofValueErroras required by Pydantic field validators, following the coding guidelines for standardized exception comment markers.
181-211: Well-designed use of ValidationInfo for dynamic field names.Using
info.field_nameallows a single validator to handle bothspan_idandparent_span_idwith accurate error messages. The type check and# error-ok:markers align with the project's error handling patterns.
213-240: Good handling of boolean edge case.The explicit
isinstance(value, bool)check before the numeric check is correct and important—sinceboolis a subclass ofintin Python,True/Falsewould otherwise pass validation as1/0. The explicit float conversion ensures consistent return type.
242-270: Correct exception conversion pattern.The validator properly converts
ModelOnexErrorfrom_validate_semvertoValueErroras required by Pydantic, while preserving the exception chain withfrom e. This maintains the project's structured error handling internally while conforming to Pydantic's validator requirements.tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py (3)
96-98: LGTM!The binding of
_try_get_event_bus_from_containerfollows the established pattern for otherMixinNodeServicemethod bindings in this fixture. This is necessary because_emit_tool_responsenow uses this helper method internally.
677-679: LGTM!Consistent with the
service_nodefixture. The binding is required sincehandle_tool_invocationis called, which internally uses_emit_tool_responsethat now depends on_try_get_event_bus_from_container.
749-751: LGTM!Same pattern applied consistently across test cases that call
handle_tool_invocation.src/omnibase_core/mixins/mixin_node_service.py (7)
174-177: Correct CancelledError propagation pattern.Properly re-raises
asyncio.CancelledErrorto honor task cancellation semantics. The comment accurately documents that cleanup is the caller's responsibility.
231-235: Defensive state update before re-raising CancelledError.Setting
_service_running = Falsebefore propagating the cancellation ensures consistent service state. This defensive pattern prevents stale state if the cancellation propagates up without returning to this method.
362-365: Correct CancelledError handling preserves finally cleanup.The
asyncio.CancelledErroris correctly isolated from both the specific exception handling and the fallback handler. Thefinallyblock at line 392 will still execute to clean up_active_invocations, ensuring no resource leak on cancellation.
439-470: Good consolidation of event bus lookup logic.This helper effectively eliminates duplicate event bus retrieval code across
_subscribe_to_tool_invocations,_emit_tool_response, and_emit_shutdown_event. The three-strategy fallback approach is well-structured.Minor consideration: The return type
Anyworks for duck typing, but ifProtocolEventBusListener(frommixin_event_listener.py) or a similar protocol covers bothsubscribeandpublishmethods, using it as the return type would provide better type safety for callers.
472-488: Improved error context for subscription failures.The enhanced
ModelOnexErrornow includesnode_typeandsubscription_topic, which will significantly aid debugging when event bus subscription fails. The refactoring to use_try_get_event_bus_from_container()reduces code duplication.
664-688: Consistent use of consolidated event bus helper.Both
_emit_tool_responseand_emit_shutdown_eventnow use_try_get_event_bus_from_container(), ensuring consistent event bus retrieval behavior across all event emission paths.
62-76: Well-documented module-level exception tuple with proper alphabetical ordering.The
_SERVICE_LOOKUP_ERRORStuple is correctly alphabetized (AttributeError, RuntimeError, TypeError, ValidationError, ValueError) and properly documented with clear rationale for both the module-level placement (mypy compatibility) and RuntimeError inclusion. The exception handler uses the standardized marker# fallback-okas required.The definition is appropriate for this mixin's specific service-lookup needs. While the centralized exception groups in
exception_groups.pycould eventually host a SERVICE_LOOKUP_ERRORS group if this pattern becomes widely reused, the current local definition is justified given that the pattern appears only in this file and requires service-specific context.
- Use centralized PYDANTIC_MODEL_ERRORS in mixin_node_service.py - Improve _is_validation_error() with robust 4-tier detection strategy - Fix inconsistent exception handling in model_metrics_context.py validator - Rename validate_span_id to validate_span_format for clarity - Remove 74 misplaced @pytest.mark.unit from inner test classes - Update stacking test to expect VALIDATION_ERROR for ValueError
Code Review - PR #374: Standardize Error Handling PatternsSummaryThis PR establishes comprehensive error handling patterns across omnibase_core with 360+ documentation markers, increased @standard_error_handling decorator adoption (7→50 usages), and centralized exception groups adoption (17→51 files). The changes significantly improve error handling consistency and developer experience. ✅ Strengths1. Excellent Documentation Updates
2. Robust Decorator ImplementationThe error handling decorators in
3. Comprehensive Test CoverageThe test suite (
4. Consistent Pattern Application
🔍 Areas for Consideration1. Potential Issue: asyncio.CancelledError in Sync FunctionsIn both sync and async wrappers, there's a catch for # Line 155-157 (sync wrapper)
except asyncio.CancelledError:
# Never suppress async cancellation - required for proper task cleanup
raiseQuestion: Can
2. decorator_allow_any_type.py ChangeThe addition of @functools.wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
return func(*args, **kwargs)
wrapper.__allow_any_reason__ = reason
return wrapper # type: ignore[return-value]Observations:
Recommendation: This change is fine, but verify the type annotation works correctly with mypy. 3. Error Context Enrichment OpportunitiesWhile the decorators add In raise ModelOnexError(
message="Contract must be a dictionary",
error_code=EnumCoreErrorCode.VALIDATION_ERROR,
actual_type=type(converted).__name__,
contract_type=type(contract).__name__,
)✅ Excellent - provides both actual and expected types for debugging. This pattern could be applied more broadly where type mismatches occur. 4. Documentation Markers: Consistency CheckThe PR adds 360+ markers. Spot-checking shows good consistency:
Recommendation: Consider a pre-commit hook or linter rule to ensure all broad exception handlers have markers (future work). 🚀 Performance ConsiderationsDecorator OverheadThe decorators add a try/except wrapper to every decorated function. For hot paths:
🔒 Security ConsiderationsError Message SafetyThe decorators include
Spot check: The decorators don't introduce new security risks - they preserve existing error messages. 📊 Test CoverageCurrent State
Recommendations
🎯 Code QualityMypy/Type Checking
Pre-commit HooksPer CLAUDE.md instructions, this PR should:
Critical: As documented in CLAUDE.md, this PR must NEVER use 📝 RecommendationsHigh Priority
Medium Priority
Low Priority
🎉 ConclusionThis is high-quality work that addresses a critical infrastructure concern (OMN-1299). The decorator implementation is robust, well-tested, and follows Python best practices. The documentation updates are comprehensive and will significantly help developers. Recommendation: ✅ APPROVE with minor clarification requested on sync CancelledError handling. Impact Assessment:
Great work! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_core/mixins/mixin_node_service.py (1)
169-177: CancelledError cleanup responsibility needs clarification and enforcement.The current design re-raises
CancelledErrorwithout cleanup, expecting the caller to invokestop_service_mode(). However, examination of test call sites reveals this responsibility is not being met—tests set_shutdown_requested = True, cancel the task, and catchCancelledError, but do not callstop_service_mode(). This creates a resource leak risk.The comment correctly documents the design intent, but the pattern is not being enforced or demonstrated in the test suite. Either:
- Tests should explicitly call
stop_service_mode()after catchingCancelledError, orstart_service_mode()should handle cleanup forCancelledError(similar to theExceptioncase on line 176), or- Callers should wrap calls in an async context manager that guarantees cleanup.
Without demonstrated cleanup at call sites, this introduces a maintenance risk for future callers who may not understand or follow this implicit contract.
🧹 Nitpick comments (4)
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py (1)
624-693: Consider extracting duplicated mock node setup into a fixture.Both
test_empty_parametersandtest_large_result_serializationcontain nearly identical mock node setup code (~45 lines each). This duplication could be reduced by extracting the common setup into a class-level fixture similar toservice_nodeinTestModelServiceEffectToolInvocation.♻️ Suggested refactor
@pytest.mark.timeout(60) @pytest.mark.unit class TestModelServiceEffectEdgeCases: """Test edge cases for tool invocation handling.""" @pytest.fixture def edge_case_node(self): """Create mock node for edge case tests.""" mock_event_bus = AsyncMock() node_id_val = uuid4() node = Mock() node._node_id = node_id_val node.node_id = node_id_val node.event_bus = mock_event_bus node._get_event_bus = Mock(return_value=mock_event_bus) node._active_invocations = set() node._total_invocations = 0 node._successful_invocations = 0 node._failed_invocations = 0 node._extract_node_name = Mock(return_value="TestNode") node._serialize_result = MixinNodeService._serialize_result.__get__( node, MixinNodeService ) node._input_state_class = None node._infer_input_state_class = Mock(return_value=None) # Bind MixinNodeService methods node.handle_tool_invocation = MixinNodeService.handle_tool_invocation.__get__( node, type(node) ) node._is_target_node = MixinNodeService._is_target_node.__get__(node, type(node)) node._convert_event_to_input_state = ( MixinNodeService._convert_event_to_input_state.__get__(node, type(node)) ) node._execute_tool = MixinNodeService._execute_tool.__get__(node, type(node)) node._emit_tool_response = MixinNodeService._emit_tool_response.__get__( node, type(node) ) node._try_get_event_bus_from_container = ( MixinNodeService._try_get_event_bus_from_container.__get__(node, type(node)) ) return node, mock_event_bus @pytest.mark.asyncio async def test_empty_parameters(self, edge_case_node): """Test invocation with no parameters.""" node, mock_event_bus = edge_case_node async def run(input_state): return {"action": input_state.action} node.run = AsyncMock(side_effect=run) # ... rest of testAlso applies to: 696-766
tests/unit/decorators/test_error_handling_extended.py (1)
528-528: Consider movingimport asyncioto top-level.The
import asynciostatement is repeated inside ~20 test methods. Moving it to the top of the file alongside other imports would reduce redundancy and follow Python conventions.♻️ Suggested improvement
Add at line 16 or 17:
import asyncioThen remove all the
import asynciostatements from inside the test methods.src/omnibase_core/mixins/mixin_node_service.py (2)
61-71: Consider alphabetical ordering of exception types.Per coding guidelines, exception tuples should be ordered alphabetically by class name. The current concatenation appends
RuntimeErrorat the end, but alphabetically it should appear betweenTypeErrorandValidationError.Since this is a module-level constant and the concatenation approach is necessary for mypy compatibility, the current structure is pragmatic. However, you could document the ordering deviation or restructure if strict alphabetical order is required.
434-465: Good consolidation of event bus lookup logic.The helper effectively centralizes the multi-strategy event bus resolution, reducing code duplication across subscription and emission methods. The use of
_SERVICE_LOOKUP_ERRORSaligns with the centralized exception handling pattern.Consider using a
Protocoltype (e.g.,ProtocolEventBusListener | None) for the return type instead ofAnyto preserve some type safety while still allowing duck-typed access. The relevant code snippet shows this type is already used inmixin_event_listener.py.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_metrics_context.pytests/unit/decorators/test_decorator_allow_any_type.pytests/unit/decorators/test_error_handling_extended.pytests/unit/enums/test_enum_artifact_type.pytests/unit/enums/test_enum_cli_status.pytests/unit/enums/test_enum_effect_handler_type.pytests/unit/enums/test_enum_filter_type.pytests/unit/enums/test_enum_node_kind.pytests/unit/enums/test_enum_registry_status.pytests/unit/enums/test_enum_scenario_status.pytests/unit/mixins/test_mixin_hash_computation.pytests/unit/mixins/test_mixin_lazy_evaluation.pytests/unit/mixins/test_mixin_redaction.pytests/unit/models/events/test_model_config.pytests/unit/models/events/test_model_event_envelope.pytests/unit/models/intents/test_core_intents.pytests/unit/models/nodes/node_services/test_model_service_compute_invocation.pytests/unit/models/nodes/node_services/test_model_service_effect_invocation.pytests/unit/primitives/test_model_semver.pytests/unit/utils/test_decorators.py
💤 Files with no reviewable changes (17)
- tests/unit/models/events/test_model_config.py
- tests/unit/models/nodes/node_services/test_model_service_compute_invocation.py
- tests/unit/mixins/test_mixin_redaction.py
- tests/unit/enums/test_enum_artifact_type.py
- tests/unit/models/events/test_model_event_envelope.py
- tests/unit/enums/test_enum_node_kind.py
- tests/unit/utils/test_decorators.py
- tests/unit/enums/test_enum_registry_status.py
- tests/unit/models/intents/test_core_intents.py
- tests/unit/enums/test_enum_filter_type.py
- tests/unit/enums/test_enum_scenario_status.py
- tests/unit/primitives/test_model_semver.py
- tests/unit/enums/test_enum_cli_status.py
- tests/unit/mixins/test_mixin_lazy_evaluation.py
- tests/unit/enums/test_enum_effect_handler_type.py
- tests/unit/decorators/test_decorator_allow_any_type.py
- tests/unit/mixins/test_mixin_hash_computation.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 (poetry run pytest, poetry add, poetry install) - never use pip or python directly
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Use @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
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 to document intent
Order exception tuples alphabetically by exception class name when catching multiple exceptions
Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Use PEP 604 union syntax (str | None) instead of legacy Union/Optional syntax - enforced by ruff UP007
Use FileRegistry from omnibase_core.runtime.runtime_file_registry for loading YAML contracts - handle error codes: FILE_NOT_FOUND, FILE_READ_ERROR, CONFIGURATION_PARSE_ERROR, CONTRACT_VALIDATION_ERROR, DUPLICATE_REGISTRATION, DIRECTORY_NOT_FOUND
Maintain minimum 60% test coverage requirement across the codebase measured with pytest-cov
Files:
tests/unit/decorators/test_error_handling_extended.pytests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_service.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/decorators/test_error_handling_extended.pytests/unit/models/nodes/node_services/test_model_service_effect_invocation.py
**/src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/src/omnibase_core/**/*.py: Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_, container_, decorator_, enum_, error_, factory_, mixin_, model_, node_, runtime_, service_*, etc.
Ensure 100% mypy strict mode compliance (0 errors) and pyright basic mode compliance across all source files in src/omnibase_core
Files:
src/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_service.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Applied to files:
tests/unit/decorators/test_error_handling_extended.pysrc/omnibase_core/decorators/decorator_error_handling.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never generic Exception - provide message, error_code, and context
Applied to files:
tests/unit/decorators/test_error_handling_extended.pysrc/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/context/model_metrics_context.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use ModelONEXContainer for dependency injection in node __init__ methods, never ModelContainer - they are different types with different purposes
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use protocol names for service resolution (container.get_service('ProtocolEventBus')) - never use concrete class names
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/nodes/node_*.py : Use mixin composition for cross-cutting concerns: MixinDiscoveryResponder, MixinEventHandler, MixinEventListener, MixinNodeExecutor, MixinNodeLifecycle, MixinRequestResponseIntrospection, MixinWorkflowExecution
Applied to files:
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups (VALIDATION_ERRORS, PYDANTIC_MODEL_ERRORS, FILE_IO_ERRORS, etc.) for exception handling
Applied to files:
src/omnibase_core/decorators/decorator_error_handling.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/src/omnibase_core/**/*.py : Follow directory-specific file naming conventions enforced by checker_naming_convention.py: cli_*, container_*, decorator_*, enum_*, error_*, factory_*, mixin_*, model_*, node_*, runtime_*, service_*, etc.
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : 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 to document intent
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-11T18:12:47.310Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T18:12:47.310Z
Learning: Applies to **/*.py : Order exception tuples alphabetically by exception class name when catching multiple exceptions
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
🧬 Code graph analysis (3)
tests/unit/decorators/test_error_handling_extended.py (3)
src/omnibase_core/models/errors/model_onex_error.py (3)
error_code(253-255)ModelOnexError(49-354)message(248-250)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-255)src/omnibase_core/decorators/decorator_error_handling.py (3)
standard_error_handling(76-173)validation_error_handling(176-281)io_error_handling(284-390)
src/omnibase_core/decorators/decorator_error_handling.py (2)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-255)src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
src/omnibase_core/mixins/mixin_node_service.py (3)
src/omnibase_core/mixins/mixin_event_listener.py (2)
event_bus(149-151)event_bus(154-156)src/omnibase_core/mixins/mixin_node_executor.py (1)
_subscribe_to_tool_invocations(268-275)src/omnibase_core/mixins/mixin_introspection_publisher.py (1)
_extract_node_name(187-214)
⏰ 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: Code Quality
- GitHub Check: Pyright Type Checking
- GitHub Check: claude-review
🔇 Additional comments (25)
tests/unit/models/nodes/node_services/test_model_service_effect_invocation.py (1)
96-98: LGTM!The method binding for
_try_get_event_bus_from_containercorrectly mirrors the pattern used for otherMixinNodeServicemethods in this fixture, ensuring the mock node has all required method bindings.src/omnibase_core/models/context/model_metrics_context.py (6)
22-22: LGTM!The
ValidationInfoimport is correctly added to support the dynamic field name access invalidate_span_format.
46-69: LGTM!The helper function correctly documents its purpose for Pydantic validators, uses standardized
# error-ok:markers per coding guidelines, and provides clear error messages for both empty and invalid format cases.
148-175: LGTM!The explicit type check correctly handles arbitrary input from
mode="before"validation, and the error message helpfully includes the actual type received.
177-214: LGTM!Good refactoring to consolidate
span_idandparent_span_idvalidation into a single validator usingValidationInfo.field_namefor field-specific error messages. The validation flow (None → type → normalize → pattern) is logically correct.
216-243: LGTM!The bool check on line 232 is a correct and important edge case handling since
boolis a subclass ofintin Python. The explicit conversion tofloaton line 237-243 ensures consistent return type.
245-269: LGTM!The pre-type check before delegating to
_validate_semveris good defensive coding, ensuring the helper receives the expected string type. This is consistent with the pattern applied to other validators in this PR.tests/unit/decorators/test_error_handling_extended.py (5)
378-380: Updated assertion reflects correct stacking behavior.The comment and assertion now correctly document that
ValueErroris detected as a validation error by the_is_validation_error()helper, causing the inner decorator to wrap it withVALIDATION_ERROR. The outer decorator then re-raises theModelOnexErroras-is, preserving this code.
521-609: Comprehensive async test coverage for standard_error_handling.Tests cover all critical paths: successful execution, exception wrapping with correct error codes,
ModelOnexErrorpassthrough,CancelledErrorpropagation, metadata preservation, and coroutine function verification.
611-675: Async validation error handling tests are thorough.Tests verify that the
_is_validation_error()detection works correctly in async context, including duck-typing for.errorsattribute and message-based detection.
677-789: Async I/O error handling tests properly verify file-specific error codes.Tests correctly verify that
FileNotFoundErrormaps toFILE_NOT_FOUND, whileIsADirectoryErrorandPermissionErrormap toFILE_OPERATION_ERROR. Generic exceptions fall back toOPERATION_FAILED.
845-880: Critical signal propagation tests ensure proper async shutdown semantics.These tests are essential for verifying that
KeyboardInterrupt,SystemExit, andGeneratorExitare never caught by the decorators, which is required for proper task cancellation and graceful shutdown.src/omnibase_core/decorators/decorator_error_handling.py (6)
17-22: Imports align with coding guidelines.Using
collections.abc.Callableinstead oftyping.Callablefollows modern Python conventions, and importingVALIDATION_ERRORSfromexception_groupsadheres to the centralized exception group pattern.
25-73: Well-structured multi-tier validation error detection.The 4-tier detection strategy is clearly documented and ordered from most to least reliable. The function correctly handles Pydantic v2's
.errors()method pattern and includes sensible fallbacks for validation-like exceptions.
119-172: Correct async/sync branching with consistent exception handling.The
asyncio.iscoroutinefunction()check properly routes to async or sync wrappers. Both paths correctly:
- Propagate exit signals without catching
- Re-raise
ModelOnexErroras-is- Wrap generic exceptions with enriched metadata
The code duplication between async and sync paths is acceptable given the clarity and the different execution semantics (
awaitvs direct call).
219-239: Validation error detection properly integrated.The decorator correctly uses
_is_validation_error()to determine the appropriate error code, and includes theis_validation_error=Trueflag in the context for downstream handling. Non-validation errors appropriately fall back toOPERATION_FAILED.
324-337: File-specific error handling with correct code mapping.The implementation correctly maps
FileNotFoundErrortoFILE_NOT_FOUNDand other file system errors toFILE_OPERATION_ERROR. Theis_file_error=Truecontext flag enables downstream code to identify file-related failures. Exception tuple ordering is alphabetical per coding guidelines.
126-128: Exception tuples correctly alphabetized.The exit signal tuple
(GeneratorExit, KeyboardInterrupt, SystemExit)follows the coding guideline requiring alphabetical ordering of exception class names.src/omnibase_core/mixins/mixin_node_service.py (7)
42-42: LGTM!Using the centralized
PYDANTIC_MODEL_ERRORSfromexception_groupsaligns with the coding guidelines for exception handling.
226-234: LGTM - Robust shutdown handling.The
CancelledErrorhandler correctly updates service state before re-raising, ensuring the service is marked as not running even during cancellation. The standardized exception markers (fallback-ok,cleanup-resilience-ok) properly document the intent.
357-360: LGTM - Proper cancellation with cleanup guarantee.Correctly re-raises
CancelledErrorwhile allowing thefinallyblock to clean up active invocations. This ensures proper resource cleanup even during task cancellation.
467-483: LGTM - Clean refactoring with enhanced error context.Using the centralized helper eliminates duplicated lookup logic. The additional
node_typeandsubscription_topickwargs inModelOnexErrorprovide valuable debugging context, aligning with the PR's goal of enriching error metadata.
634-643: LGTM - Enhanced error context for debugging.Adding
node_typeandnode_nameto the error payload provides useful context when diagnosing why a tool execution returnedNone.
659-667: LGTM!Consistent use of the centralized
_try_get_event_bus_from_containerhelper.
669-683: LGTM!Consistent use of the centralized helper for event bus lookup in the shutdown path.
Merged origin/main into feature branch to resolve PR merge conflicts and incorporate recent project-wide naming convention updates.
PR Review Fixes: - Alphabetize exception tuples in contract_diff_computer.py and model_event_type_registry.py - Add catch-all handler in mixin_effect_execution.py for non-Pydantic exceptions - Fix ModelSemVer.parse() to convert ModelOnexError to ValueError in model_detection_metadata.py - Remove unnecessary KeyError from exception tuple in model_detection_metadata.py - Add asyncio.CancelledError re-raise in util_workflow_executor.py - Preserve exception context with 'from e' in model_event_type_registry.py - Add boundary-ok markers to broad exception handlers in model_workflow_coordinator.py Merge: Incorporate latest changes from main (no conflicts)
Comprehensive Code Review - PR #374: Error Handling StandardizationExecutive Summary ✅This PR successfully implements comprehensive error handling standardization across omnibase_core. No critical issues found. The implementation demonstrates excellent attention to detail with proper async/sync handling, correct cancellation semantics, and security-conscious error messaging. Key Accomplishments✅ 373 documentation markers added across 141 files Detailed Analysis1. Async/Sync Handling ⭐ EXCELLENTThe decorator implementation correctly handles both patterns: Async Functions: if asyncio.iscoroutinefunction(func):
@functools.wraps(func)
async def async_wrapper(*args, **kwargs):
return await func(*args, **kwargs) # ✅ Properly awaits
return async_wrapperSync Functions: else:
@functools.wraps(func)
def wrapper(*args, **kwargs):
return func(*args, **kwargs) # ✅ No await
return wrapperEvidence: Test coverage validates with 2. CancelledError Propagation ⭐ EXCELLENTAll three decorators properly handle async cancellation: except (GeneratorExit, KeyboardInterrupt, SystemExit):
raise # Never catch cancellation/exit signals
except asyncio.CancelledError:
raise # Never suppress async cancellation - required for proper task cleanup
except ModelOnexError:
raise # Always re-raise as-is to preserve error context
except Exception as e:
# boundary-ok: convert generic exceptions to ModelOnexError
raise ModelOnexError(...) from eCritical: The order matters - exit signals are caught before generic exceptions, ensuring proper task cleanup during shutdown. Test Coverage: 23 async tests specifically validate CancelledError propagation, including:
3. Security Analysis ⭐ SECUREError Message Construction: msg = f"{operation_name} failed: {e!s}" # Safe string conversion
raise ModelOnexError(
msg,
EnumCoreErrorCode.OPERATION_FAILED,
original_error_type=type(e).__name__, # Type name, not exception object
operation=operation_name,
)Security Best Practices:
4. Documentation Markers ⭐ EXCELLENTDistribution:
These markers significantly improve code review by explicitly documenting why broad exception handlers are acceptable. 5. Exception Groups Adoption ⭐ GOODGroups Defined:
Adoption: 47 files now use centralized exception groups, eliminating ad-hoc exception tuples and ensuring consistency. Minor Opportunity: 6. Decorator Error Code Assignment ⭐ EXCELLENTSemantically Correct:
Validation Error Detection uses multi-tier strategy:
7. Test Coverage ⭐ EXCELLENTFile: Coverage:
Critical Tests:
RecommendationsNo Critical Issues Found ✅All error handling patterns are correctly implemented with proper async/sync handling, cancellation semantics, and security. Optional Improvements (Low Priority)
Performance Considerations✅ No performance concerns - decorators add negligible overhead Test Coverage Assessment✅ Comprehensive: 56 test methods cover all decorator variants ConclusionThis PR represents high-quality engineering with:
Recommendation: Approve and merge. This PR successfully achieves its goal of standardizing error handling patterns across omnibase_core while maintaining backward compatibility and adding comprehensive documentation. Review conducted by: Claude Sonnet 4.5 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/omnibase_core/models/context/model_detection_metadata.py (1)
145-167: MissingModelOnexErrorcatch in dict validation path.The string parsing path (lines 140-144) correctly catches
ModelOnexErrorfromModelSemVer.parse(), but the dict path does not. According to the relevant code snippet,ModelSemVer.validate_non_negativeraisesModelOnexErrorwhen values are negative. If a dict like{"major": -1, "minor": 0, "patch": 0}is passed, theisinstancechecks pass (it's anint), butModelSemVer()constructor will raiseModelOnexError, which won't be caught byexcept (TypeError, ValueError).🐛 Proposed fix to catch ModelOnexError
- except (TypeError, ValueError) as e: + except (ModelOnexError, TypeError, ValueError) as e:Note: The exception tuple is alphabetically ordered as required by coding guidelines.
src/omnibase_core/contracts/contract_diff_computer.py (1)
180-192: The exception tuple includesModelOnexErrorwhich is not covered by existing centralized groups; consider splitting the handler or create a dedicated group.The exception tuple is correctly alphabetically ordered and uses the proper
fallback-okmarker. However, coding guidelines require using centralized exception constants fromomnibase_core.errors.exception_groupsinstead of inline tuples.The challenge:
PYDANTIC_MODEL_ERRORScovers 4 of the 5 exceptions (AttributeError, TypeError, ValidationError, ValueError) but excludesModelOnexError. Options:
- Use
PYDANTIC_MODEL_ERRORSand catchModelOnexErrorseparately in an additional handler- Create a
FINGERPRINT_COMPUTATION_ERRORSgroup inexception_groups.pythat includes both the pydantic model errors andModelOnexError
🤖 Fix all issues with AI agents
In @src/omnibase_core/models/core/model_field_accessor.py:
- Around line 58-61: Add the required standardized comment markers to each broad
exception handler that catches ATTRIBUTE_ACCESS_ERRORS: in get_field (the except
block that returns default or an error) append "# init-errors-ok"; in set_field,
has_field, and remove_field (the except blocks that return False on access
errors) append either "# fallback-ok" or "# catch-all-ok" per guideline. Locate
the except ATTRIBUTE_ACCESS_ERRORS as e: lines in functions get_field,
set_field, has_field, and remove_field and add the specified comment text after
the except clause.
🧹 Nitpick comments (3)
src/omnibase_core/models/examples/model_fallback_metadata.py (1)
8-8: Consider removingfrom __future__ import annotations.As per coding guidelines, Pydantic models should avoid
from __future__ import annotationsunless forward references are needed. This file doesn't appear to use any forward references that would require it. In Pydantic v2, this import can cause issues with runtime type validation since annotations become strings rather than actual types.Suggested fix
-from __future__ import annotations -src/omnibase_core/infrastructure/execution/infra_phase_sequencer.py (1)
414-449: Consider adding decorator tocreate_empty_execution_planfor consistency.This public function creates a
ModelExecutionPlandirectly (line 444), which could raise Pydantic validation errors if, for example,source_profilecontains invalid characters orcreated_athas an unexpected type. Unlikecreate_default_execution_plan(which has indirect coverage viacreate_execution_plan), this function has no error handling coverage.♻️ Suggested change
+@standard_error_handling("Empty execution plan creation") def create_empty_execution_plan( source_profile: str | None = None, created_at: datetime | None = None, ) -> ModelExecutionPlan:src/omnibase_core/models/core/model_field_accessor.py (1)
116-117: LGTM!The outer exception handler correctly uses the centralized
ATTRIBUTE_ACCESS_ERRORSgroup.Note: The inner handlers at lines 82-83 and 108-109 still use inline
(AttributeError, TypeError). This appears intentional since they're try-alternative-approach fallbacks (e.g., try setattr, fall through to dict access) rather than error handlers. If you prefer full consistency, you could extract these to a narrower centralized constant, but the current approach is reasonable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CLAUDE.mdsrc/omnibase_core/contracts/contract_diff_computer.pysrc/omnibase_core/infrastructure/execution/infra_phase_sequencer.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/models/cli/model_cli_execution_config.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/connections/model_custom_connection_properties.pysrc/omnibase_core/models/container/model_workflow_coordinator.pysrc/omnibase_core/models/context/model_detection_metadata.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/models/examples/model_example_metadata.pysrc/omnibase_core/models/examples/model_fallback_metadata.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/omnibase_core/models/cli/model_cli_execution_config.py
- src/omnibase_core/models/examples/model_example_metadata.py
- src/omnibase_core/models/container/model_workflow_coordinator.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution - never use pip or python directly
Files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/infrastructure/execution/infra_phase_sequencer.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/contracts/contract_diff_computer.pysrc/omnibase_core/models/connections/model_custom_connection_properties.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/context/model_detection_metadata.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection resolution (e.g., 'ProtocolEventBus'), never concrete class names
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception
Use the @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Exception tuples must be alphabetically ordered by exception class name
Use standardized 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
Use centralized exception constants from omnibase_core.errors.exception_groups instead of inline exception tuples
All Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py
Use PEP 604 union syntax (str | None) instead of Optional/Union from typing module
Type checking must pass both mypy in strict mode (0 errors) and pyright in basic mode with 100% compliance
Use duck typing with protocols and never use isinstance checks for service type validation
Node instances are NOT thread-safe by default; use thread-local instances, separate instances per thread, or explicit locks
Files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/infrastructure/execution/infra_phase_sequencer.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/contracts/contract_diff_computer.pysrc/omnibase_core/models/connections/model_custom_connection_properties.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/context/model_detection_metadata.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Every Pydantic model must have an explicit ConfigDict with properly declared extra field policy and frozen status
All frozen Pydantic models must include from_attributes=True in ConfigDict for pytest-xdist compatibility
Use Field() with metadata when providing validation constraints or aliases, but not for simple defaults (use = None instead)
Always use default_factory for mutable default values (list, dict) in Pydantic models to prevent shared state bugs
Do not use 'from future import annotations' in Pydantic models or FastAPI endpoints; only use when forward references needed
Files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/models/connections/model_custom_connection_properties.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/context/model_detection_metadata.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use black for code formatting, isort for import sorting, and ruff for linting across all Python source and test files
Files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/infrastructure/execution/infra_phase_sequencer.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/contracts/contract_diff_computer.pysrc/omnibase_core/models/connections/model_custom_connection_properties.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/context/model_detection_metadata.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use standardized 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
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of inline exception tuples
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use the standard_error_handling decorator to eliminate try/catch boilerplate in async operations
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of inline exception tuples
Applied to files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/cli/model_result_summary.pyCLAUDE.mdsrc/omnibase_core/models/connections/model_custom_connection_properties.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/context/model_detection_metadata.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception
Applied to files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/mixins/mixin_effect_execution.pysrc/omnibase_core/models/cli/model_result_summary.pyCLAUDE.mdsrc/omnibase_core/models/cli/model_cli_debug_info.pysrc/omnibase_core/models/connections/model_custom_connection_properties.pysrc/omnibase_core/models/core/model_event_type_registry.pysrc/omnibase_core/models/context/model_detection_metadata.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Field() with metadata when providing validation constraints or aliases, but not for simple defaults (use = None instead)
Applied to files:
src/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/models/**/*.py : All frozen Pydantic models must include from_attributes=True in ConfigDict for pytest-xdist compatibility
Applied to files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/context/model_detection_metadata.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/models/**/*.py : Every Pydantic model must have an explicit ConfigDict with properly declared extra field policy and frozen status
Applied to files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/models/examples/model_fallback_metadata.pysrc/omnibase_core/models/cli/model_result_summary.pysrc/omnibase_core/models/context/model_detection_metadata.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use the standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Applied to files:
src/omnibase_core/infrastructure/execution/infra_phase_sequencer.pyCLAUDE.mdsrc/omnibase_core/models/core/model_event_type_registry.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use standardized 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
Applied to files:
src/omnibase_core/models/examples/model_fallback_metadata.pyCLAUDE.md
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Exception tuples must be alphabetically ordered by exception class name
Applied to files:
CLAUDE.md
🧬 Code graph analysis (8)
src/omnibase_core/models/core/model_field_accessor.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/infrastructure/execution/infra_phase_sequencer.py (1)
src/omnibase_core/decorators/decorator_error_handling.py (1)
standard_error_handling(76-173)
src/omnibase_core/models/examples/model_fallback_metadata.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/mixins/mixin_effect_execution.py (2)
src/omnibase_core/models/errors/model_onex_error.py (3)
ModelOnexError(49-354)message(248-250)error_code(253-255)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-255)
src/omnibase_core/contracts/contract_diff_computer.py (1)
src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
src/omnibase_core/models/connections/model_custom_connection_properties.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/models/core/model_event_type_registry.py (2)
src/omnibase_core/decorators/decorator_error_handling.py (1)
standard_error_handling(76-173)src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
src/omnibase_core/models/context/model_detection_metadata.py (1)
src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
🪛 LanguageTool
CLAUDE.md
[grammar] ~435-~435: Use a hyphen to join words.
Context: ...rators Three decorators eliminate error handling boilerplate. All decorators fol...
(QB_NEW_EN_HYPHEN)
⏰ 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 (33)
src/omnibase_core/models/examples/model_fallback_metadata.py (4)
13-13: LGTM!The import of
PYDANTIC_MODEL_ERRORSfrom centralized exception groups aligns with the project's error handling standards. Based on learnings, this replaces inline exception tuples with centralized groups.
55-59: LGTM!ConfigDict properly declares the extra field policy (
extra="ignore") and enables assignment validation. Since this model is not frozen,from_attributes=Trueis not required per the guidelines.
63-74: LGTM!The error handling correctly follows the standardized pattern:
- Uses centralized
PYDANTIC_MODEL_ERRORSinstead of inline exception tuple- Raises
ModelOnexErrorwith appropriateEnumCoreErrorCode.VALIDATION_ERROR- Preserves exception chain with
from eThis aligns with the project's error handling guidelines and the PR's objective to standardize exception handling across the codebase.
76-84: No changes detected in these methods.The
serialize()andvalidate_instance()methods are unchanged and follow standard Pydantic v2 patterns.src/omnibase_core/models/cli/model_cli_debug_info.py (1)
198-207: LGTM! Base implementation pattern is appropriate.The simplification to always return
Truewith delegation to subclasses follows a clean template method pattern. The docstring clearly communicates that subclasses should override for custom validation logic. This aligns with the PR's broader goal of centralizing and standardizing error handling.src/omnibase_core/models/cli/model_result_summary.py (1)
68-77: LGTM! Consistent with the standardized base validation pattern.The changes mirror the pattern established in
model_cli_debug_info.py, providing a simple base implementation that subclasses can override. The removal ofEnumCoreErrorCodeandModelOnexErrorimports (per AI summary) is appropriate since the base implementation no longer raises exceptions.src/omnibase_core/models/connections/model_custom_connection_properties.py (2)
20-20: LGTM!Import correctly added and properly placed in alphabetical order with other
omnibase_coreimports. This follows the centralized exception groups pattern. Based on learnings, this aligns with the guideline to use centralized exception constants fromomnibase_core.errors.exception_groups.
734-744: LGTM!The exception handling change properly adopts the centralized
PYDANTIC_MODEL_ERRORSgroup, replacing what was likely an inline exception tuple. The inline comment on line 735 transparently documents the included exceptions, and the structuredModelOnexErrorwithfailed_keyscontext provides good observability for debugging configuration failures.src/omnibase_core/models/context/model_detection_metadata.py (4)
22-22: LGTM!Import is correctly added to support catching
ModelOnexErrorfromModelSemVer.parse()in the validator below.
140-144: LGTM!Proper translation from
ModelOnexError(raised byModelSemVer.parse()) toValueErrorfor Pydantic field validator compatibility. Exception chain is correctly preserved withfrom e, and theerror-okmarker documents the intentional pattern per project conventions.
168-169: LGTM!Error marker correctly documents the intentional
ValueErrorfor Pydantic field validator compatibility. Based on learnings, this follows the project's standardized comment marker conventions.
66-66: LGTM!
ConfigDictcorrectly includesfrozen=True,extra="forbid", andfrom_attributes=Trueas required by the coding guidelines for frozen Pydantic models.src/omnibase_core/contracts/contract_diff_computer.py (1)
60-61: LGTM!Import of
ValidationErrorfrom pydantic is appropriate for handling validation failures during fingerprint computation.CLAUDE.md (2)
384-596: Excellent comprehensive error handling documentation.This section provides clear, actionable guidance for standardizing error handling across the codebase. The structure is well-organized:
- ValueError vs ModelOnexError Policy: Clear decision matrix helps developers choose the right exception type
- Error Handling Decorators: Good examples showing how to eliminate boilerplate
- Exception Groups: Comprehensive table with appropriate use cases
- Critical asyncio.CancelledError Note: Correctly emphasizes that cancellation signals must never be caught by exception groups and must be re-raised
The documentation aligns perfectly with the retrieved learnings and PR objectives to standardize error handling patterns across omnibase_core.
Based on learnings, these patterns (standardized markers, ModelOnexError usage, centralized exception groups, and decorators) should now be consistently applied across src/omnibase_core/**/*.py.
859-859: LGTM: Date updated appropriately.The last-updated date correctly reflects the documentation changes made on 2026-01-12.
src/omnibase_core/models/core/model_event_type_registry.py (5)
11-11: LGTM!Import aligns with the PR objective to expand
@standard_error_handlingdecorator usage across the codebase. Based on learnings, this decorator eliminates try/catch boilerplate in operations.
30-40: LGTM!The decorator standardizes error handling for event registration, consistent with the PR's goal of expanding decorator usage across the codebase.
160-165: LGTM!Good improvement adding
from efor proper exception chaining, which preserves the original error context in tracebacks. Exception tuple is correctly ordered alphabetically per coding guidelines.
169-196: LGTM!Decorator appropriately wraps the bootstrap operation. Since the decorator re-raises
ModelOnexErroras-is (per the decorator implementation), nested calls toregister_event_typewon't cause double-wrapping.
232-237: LGTM!Exception tuple is correctly ordered alphabetically (
AttributeError,ModelOnexError,RuntimeError). The pattern of catchingModelOnexErrorand rewrapping with a specific boundary message is intentional—this normalizes various container initialization failures into a consistent user-facing error while preserving the original context viafrom e.src/omnibase_core/mixins/mixin_effect_execution.py (3)
83-83: LGTM!The import of
PYDANTIC_MODEL_ERRORSfrom centralized exception groups aligns with the coding guidelines to use centralized exception constants instead of inline exception tuples. Thecastimport is appropriately added for the type narrowing in_execute_operation.Also applies to: 159-159
1609-1620: LGTM!The refactored exception handling correctly follows the project's error handling patterns:
- Catches
PYDANTIC_MODEL_ERRORSfrom centralized exception groups first (as per coding guidelines)- Falls back to general
Exceptionwith propercatch-all-okmarker- Both paths wrap errors in
ModelOnexErrorwith structured contextBased on learnings, this aligns with the guideline to use centralized exception constants from
omnibase_core.errors.exception_groupsinstead of inline exception tuples.
1438-1445: Result handling logic is correct and well-documented.The code properly validates result types:
isinstancecheck confirms membership inEffectResultType(str, int, float, bool, dict, list) before castingNoneis explicitly converted to{}per the type definition, which excludesNone- All other types fall back to string representation
The
castusage is appropriate since theisinstancecheck already validates the type membership. The comment "None not in EffectResultType" aligns with the type definition insrc/omnibase_core/types/type_effect_result.pywhich documents this constraint.src/omnibase_core/infrastructure/execution/infra_phase_sequencer.py (6)
44-44: LGTM!Import correctly added for the
standard_error_handlingdecorator used throughout the file. Placement follows the existing import organization.
81-126: LGTM!Decorator application is appropriate here. The function iterates over input and performs string operations—if unexpected types are passed, wrapping the resulting
TypeError/AttributeErrorintoModelOnexErroris the correct behavior rather than silently returningFalse. Based on learnings, this aligns with the standard error handling pattern.
129-184: LGTM!Consistent application of the decorator for the strict validation variant. When called from
create_execution_plan(line 317), anyModelOnexErrorraised here will propagate correctly since the decorator re-raisesModelOnexErroras-is.
187-218: LGTM!Simple grouping logic benefits from the standardized error wrapper for malformed inputs.
221-268: LGTM!Decorator correctly applied. Sorting could raise
TypeErrorif handlers contain non-comparable elements, which will be properly wrapped.
271-368: LGTM!The decorator application here is well-considered:
- Explicit
ModelOnexErrorat lines 319-323 passes through unchanged (decorator re-raisesModelOnexErroras-is)- Pydantic validation errors from
ModelPhaseStep/ModelExecutionPlanconstruction get wrapped with proper context- Calls to other decorated functions (
validate_phase_list_strict,group_handlers_by_phase,order_handlers_in_phase) propagate theirModelOnexErrorcorrectlysrc/omnibase_core/models/core/model_field_accessor.py (4)
12-12: LGTM!Import of centralized
ATTRIBUTE_ACCESS_ERRORSaligns with the coding guidelines and PR objectives to use centralized exception constants fromomnibase_core.errors.exception_groupsinstead of inline exception tuples. Based on learnings.
135-136: LGTM!The exception handling correctly uses
ATTRIBUTE_ACCESS_ERRORSand appropriately returnsFalsewhen an error occurs during the field existence check.
171-172: LGTM!The exception handling correctly uses
ATTRIBUTE_ACCESS_ERRORSand appropriately returnsFalsewhen an error occurs during field removal.
196-205: LGTM!The expanded docstring clearly documents the base implementation behavior and provides helpful guidance for subclasses that need custom validation logic.
| except ATTRIBUTE_ACCESS_ERRORS as e: | ||
| if default is not None: | ||
| return ModelResult.ok(default) | ||
| return ModelResult.err(f"Error accessing field '{path}': {e!s}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f 'exception_groups.py' src/omnibase_core --exec cat {}Repository: OmniNode-ai/omnibase_core
Length of output: 4378
🏁 Script executed:
cat -n src/omnibase_core/models/core/model_field_accessor.pyRepository: OmniNode-ai/omnibase_core
Length of output: 10129
Add standardized comment markers to broad exception handlers.
The ATTRIBUTE_ACCESS_ERRORS exception group is correctly defined and alphabetically ordered, but all usages (lines 58, 116, 135, 171) are missing required comment markers per coding guidelines. Since ATTRIBUTE_ACCESS_ERRORS is a broad handler catching multiple exception types, each except block must include a standardized marker:
- Line 58-61 (
get_field): Use# init-errors-ok(returns default on access failure) - Lines 116-117, 135-136, 171-172 (
set_field,has_field,remove_field): Use# fallback-okor# catch-all-ok(returns False on any access error)
Example:
except ATTRIBUTE_ACCESS_ERRORS as e: # init-errors-ok
if default is not None:
return ModelResult.ok(default)
return ModelResult.err(f"Error accessing field '{path}': {e!s}")🤖 Prompt for AI Agents
In @src/omnibase_core/models/core/model_field_accessor.py around lines 58 - 61,
Add the required standardized comment markers to each broad exception handler
that catches ATTRIBUTE_ACCESS_ERRORS: in get_field (the except block that
returns default or an error) append "# init-errors-ok"; in set_field, has_field,
and remove_field (the except blocks that return False on access errors) append
either "# fallback-ok" or "# catch-all-ok" per guideline. Locate the except
ATTRIBUTE_ACCESS_ERRORS as e: lines in functions get_field, set_field,
has_field, and remove_field and add the specified comment text after the except
clause.
- Replace type: ignore with explicit cast() in decorator files - Add ValidationError to mixin_node_service.py exception tuple - Fix mode="before" validator type hints to use object in metrics context - Add # fallback-ok: markers to exception handlers in field accessor - Add # fallback-ok: markers and alphabetize exceptions in validation suite - Update documentation comments to match exception handling patterns
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_core/decorators/decorator_allow_any_type.py (1)
25-26: Update docstring to reflect wrapping behavior.The docstring states "Decorated function unchanged (documentation only)" but the function is now wrapped with an additional call layer. Consider updating to reflect the actual behavior:
Returns: - Decorated function unchanged (documentation only) + Decorated function wrapped to preserve metadata and signature
🤖 Fix all issues with AI agents
In @src/omnibase_core/models/context/model_metrics_context.py:
- Around line 167-173: Update the comment marker in the trace_id validator to
match the standardized format used in _validate_semver: replace the existing "#
validation-error-ok: Pydantic field_validator requires ValueError" with "#
error-ok: Pydantic validator requires ValueError" in the block that raises
ValueError for non-matching _TRACE_ID_PATTERN (the trace_id
normalization/validation code referencing _TRACE_ID_PATTERN).
🧹 Nitpick comments (1)
src/omnibase_core/models/core/model_field_accessor.py (1)
83-85: Consider usingATTRIBUTE_ACCESS_ERRORSfor consistency.The
fallback-ok:marker is appropriate. However, this inner handler still uses an inline tuple(AttributeError, TypeError)while the outer handler at line 119 uses the centralizedATTRIBUTE_ACCESS_ERRORS. For consistency with this PR's standardization goal, consider using the centralized constant here as well.That said, if the intent is to exclude
KeyError(sincesetattrwon't raise it), the current approach is defensible.♻️ Optional: Use centralized constant
try: setattr(obj, part, {}) next_obj = getattr(obj, part) - except (AttributeError, TypeError): + except ATTRIBUTE_ACCESS_ERRORS: # fallback-ok: return False if initializing nested dict fails return False
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/omnibase_core/decorators/decorator_allow_any_type.pysrc/omnibase_core/decorators/decorator_enforce_execution_shape.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/services/service_validation_suite.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_core/services/service_validation_suite.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{py,toml}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use Poetry for Python package management and task execution - never use pip or python directly
Files:
src/omnibase_core/decorators/decorator_allow_any_type.pysrc/omnibase_core/decorators/decorator_enforce_execution_shape.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/mixins/mixin_node_service.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection resolution (e.g., 'ProtocolEventBus'), never concrete class names
Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception
Use the @standard_error_handling decorator to eliminate try/catch boilerplate in async operations
Exception tuples must be alphabetically ordered by exception class name
Use standardized 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
Use centralized exception constants from omnibase_core.errors.exception_groups instead of inline exception tuples
All Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py
Use PEP 604 union syntax (str | None) instead of Optional/Union from typing module
Type checking must pass both mypy in strict mode (0 errors) and pyright in basic mode with 100% compliance
Use duck typing with protocols and never use isinstance checks for service type validation
Node instances are NOT thread-safe by default; use thread-local instances, separate instances per thread, or explicit locks
Files:
src/omnibase_core/decorators/decorator_allow_any_type.pysrc/omnibase_core/decorators/decorator_enforce_execution_shape.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/mixins/mixin_node_service.py
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use black for code formatting, isort for import sorting, and ruff for linting across all Python source and test files
Files:
src/omnibase_core/decorators/decorator_allow_any_type.pysrc/omnibase_core/decorators/decorator_enforce_execution_shape.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/mixins/mixin_node_service.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Every Pydantic model must have an explicit ConfigDict with properly declared extra field policy and frozen status
All frozen Pydantic models must include from_attributes=True in ConfigDict for pytest-xdist compatibility
Use Field() with metadata when providing validation constraints or aliases, but not for simple defaults (use = None instead)
Always use default_factory for mutable default values (list, dict) in Pydantic models to prevent shared state bugs
Do not use 'from future import annotations' in Pydantic models or FastAPI endpoints; only use when forward references needed
Files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use standardized 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
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of inline exception tuples
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use the standard_error_handling decorator to eliminate try/catch boilerplate in async operations
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use structured error handling with ModelOnexError and EnumCoreErrorCode, never raise generic Exception
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of inline exception tuples
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/models/**/*.py : Every Pydantic model must have an explicit ConfigDict with properly declared extra field policy and frozen status
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/models/**/*.py : Use Field() with metadata when providing validation constraints or aliases, but not for simple defaults (use = None instead)
Applied to files:
src/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Use standardized 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
Applied to files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/**/*.py : Exception tuples must be alphabetically ordered by exception class name
Applied to files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/models/**/*.py : All frozen Pydantic models must include from_attributes=True in ConfigDict for pytest-xdist compatibility
Applied to files:
src/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Always use ModelONEXContainer for dependency injection in node constructors, never use generic ModelContainer
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-13T11:03:28.097Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T11:03:28.097Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use available mixins (MixinDiscoveryResponder, MixinEventHandler, etc.) to add reusable behavior to nodes
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
🧬 Code graph analysis (3)
src/omnibase_core/decorators/decorator_allow_any_type.py (1)
src/omnibase_core/decorators/decorator_error_handling.py (3)
wrapper(149-169)wrapper(245-277)wrapper(352-386)
src/omnibase_core/models/core/model_field_accessor.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/mixins/mixin_node_service.py (6)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(9-255)src/omnibase_core/mixins/mixin_event_listener.py (2)
event_bus(149-151)event_bus(154-156)src/omnibase_core/mixins/mixin_node_executor.py (1)
_subscribe_to_tool_invocations(268-275)src/omnibase_core/models/errors/model_onex_error.py (3)
ModelOnexError(49-354)message(248-250)error_code(253-255)src/omnibase_core/mixins/mixin_introspection_publisher.py (1)
_extract_node_name(187-214)tests/unit/models/nodes/node_services/test_model_service_compute_lifecycle.py (1)
_extract_node_name(52-54)
🔇 Additional comments (24)
src/omnibase_core/decorators/decorator_allow_any_type.py (2)
8-10: LGTM!The imports for
functoolsandcastare appropriate for the new wrapper pattern. This aligns with the decorator patterns used indecorator_error_handling.py.
29-37: Wrapper implementation aligns with PR's standardized decorator patterns.The wrapper correctly:
- Uses
@functools.wraps(func)to preserve metadata and signature- Delegates to the original function without modification
- Stores
__allow_any_reason__on the wrapper for introspection- Uses
cast(F, wrapper)to maintain type safetyThis is consistent with the wrapper patterns in
decorator_error_handling.pyshown in the relevant snippets.src/omnibase_core/decorators/decorator_enforce_execution_shape.py (3)
20-20: LGTM!Import of
castis appropriate for the type annotation fix in the async wrapper.
140-141: LGTM!The cast is appropriate here. Since
async deffunctions have typeCallable[P, Coroutine[...]]but the decorator returnsCallable[P, T], the cast correctly informs the type checker thatfunctools.wrapspreserves the original signature.
143-150: LGTM!The sync wrapper correctly returns without a cast since regular functions naturally match
Callable[P, T], unlike async functions which return coroutines.src/omnibase_core/models/core/model_field_accessor.py (6)
12-12: LGTM!Import of centralized
ATTRIBUTE_ACCESS_ERRORSaligns with coding guidelines for using exception constants fromexception_groupsinstead of inline tuples. Based on learnings.
58-62: LGTM!Good use of centralized
ATTRIBUTE_ACCESS_ERRORSwith properfallback-ok:marker documenting the rationale. The fallback behavior (return default or error result) is appropriate for field accessor semantics.
119-121: LGTM!Good standardization using
ATTRIBUTE_ACCESS_ERRORSwith clearfallback-ok:documentation. Boolean return for failure aligns with the method's contract.
139-141: LGTM!Consistent application of
ATTRIBUTE_ACCESS_ERRORSwith proper documentation. ReturningFalseon access failure is the correct semantic for an existence check.
176-178: LGTM!Consistent with other methods - centralized exception constant with proper
fallback-ok:marker.
202-211: LGTM!Good documentation improvement clarifying the base implementation's behavior and providing guidance for subclass exception handling. This supports the PR's goal of standardizing error handling patterns through clear documentation.
src/omnibase_core/models/context/model_metrics_context.py (6)
22-22: LGTM!The
ValidationInfoimport is correctly added to support field-specific error messaging in thevalidate_span_formatvalidator.
46-69: LGTM!The helper function correctly validates SemVer format and uses the standard
# error-ok:marker to document intentional ValueError usage for Pydantic field validators.
180-218: Good refactor using ValidationInfo for field-specific messages.The validator correctly handles both
span_idandparent_span_idwith dynamic error messages. Same marker inconsistency applies here—use# error-ok:instead of# validation-error-ok:at lines 207 and 214.
239-250: LGTM!Correctly excludes booleans from numeric acceptance (since
boolis a subclass ofintin Python), converts to float, and validates the 0.0-1.0 range. Same marker format note applies (lines 240, 246).
252-279: LGTM!Proper type guard before delegating to
_validate_semver. Same marker format note applies (line 275).
117-146: LGTM!The model configuration follows all coding guidelines: explicit
ConfigDictwithfrozen=True,extra="forbid", andfrom_attributes=True; PEP 604 union syntax throughout; proper use ofField()for metadata.src/omnibase_core/mixins/mixin_node_service.py (7)
63-73: Good use of centralized exception groups with a minor ordering note.The module-level constant with mypy justification is well-documented. Using
PYDANTIC_MODEL_ERRORSaligns with the coding guidelines for centralized exception constants.One consideration: the coding guidelines specify exception tuples should be alphabetically ordered. After concatenation, the order is
(AttributeError, TypeError, ValidationError, ValueError, RuntimeError), whereRuntimeErrorbreaks alphabetical sequence. If strict ordering is required, you could either:
- Define a new centralized constant in
exception_groups.pythat includesRuntimeErrorin proper order- Accept this as a pragmatic trade-off for centralization benefits
Given the centralization goal, the current approach is reasonable.
171-179: Correct asyncio cancellation handling pattern.Re-raising
CancelledErrorbefore the generic exception handler ensures task cancellation semantics are preserved. The comment clearly documents that cleanup responsibility lies with the caller viastop_service_mode().
228-236: Proper shutdown resilience with state consistency.Setting
_service_running = Falsebefore re-raisingCancelledErrorensures the service state remains consistent even when cancellation occurs mid-shutdown. The standardized markers (cleanup-resilience-ok,fallback-ok) correctly document the intentional broad exception handling per coding guidelines.
320-364: Well-structured exception handling with proper cancellation propagation.The exception tuple is alphabetically ordered and comprehensively covers expected failure modes. Adding
ValidationErroraligns with the PR goal of centralizing Pydantic error handling. The separateCancelledErrorblock (lines 361-364) correctly preserves asyncio cancellation semantics while still allowing thefinallyblock to clean up_active_invocations.
438-469: Good consolidation of event bus lookup logic.This helper effectively reduces code duplication across
_emit_tool_response,_emit_shutdown_event, and_subscribe_to_tool_invocations. The three-strategy approach (method, attribute, container) provides flexibility for different mixin compositions. TheAnyreturn type is appropriately documented as intentional for duck-typed access.
471-487: Enhanced error context improves debuggability.The addition of
node_typeandsubscription_topicto theModelOnexErrorcontext provides valuable diagnostic information when event bus lookup fails. This aligns with the PR's goal of enriching error payloads across the codebase.
638-681: Consistent use of helper and enriched error context.The
_serialize_resultmethod now includesnode_typeandnode_namein error payloads, improving traceability. Both_emit_tool_responseand_emit_shutdown_eventnow use the consolidated_try_get_event_bus_from_container()helper, eliminating code duplication.
Resolved conflicts: - decorator_allow_any_type.py: Accept main's simplified approach with NOTE comment - decorator_enforce_execution_shape.py: Accept main's type: ignore pattern - logging_emit.py: Accept main's type: ignore pattern (2 locations) - mixin_hybrid_execution.py: Accept main's deletion All resolutions follow the new code hygiene policy (OMN-1302) with proper NOTE comments for type ignores.
Pull Request Review - PR #374: Standardize Error Handling PatternsSummaryThis PR establishes comprehensive and consistent error handling patterns across omnibase_core through documentation markers, decorator adoption, and centralized exception groups. The implementation is well-executed and follows ONEX best practices. ✅ Strengths1. Excellent Documentation (CLAUDE.md + Best Practices Guide)
2. Strong Decorator Implementation
3. Consistent Exception Documentation
4. Centralized Exception Groups
5. Boilerplate Reduction
🔍 Areas for Improvement1. Decorator Error Code Precision (Minor)Location: Current: raise ModelOnexError(
msg,
EnumCoreErrorCode.OPERATION_FAILED, # Generic code
original_error_type=type(e).__name__,
operation=operation_name,
) from eSuggestion: Consider adding a parameter to allow callers to specify a custom error code: def standard_error_handling(
operation_name: str = "operation",
error_code: EnumCoreErrorCode = EnumCoreErrorCode.OPERATION_FAILED,
) -> Callable:
# ...
raise ModelOnexError(msg, error_code, ...) from eImpact: Low priority - the current generic code is acceptable, but custom codes would improve error categorization for specific operations. 2. Test Coverage for Edge Cases (Minor)Location: Current coverage: Excellent for core paths (90%+), but consider adding:
Example test: async def test_nested_decorators_preserve_context():
@standard_error_handling("Outer operation")
@validation_error_handling("Inner validation")
async def nested_function():
raise ValueError("Deep error")
with pytest.raises(ModelOnexError) as exc_info:
await nested_function()
# Verify inner error is wrapped once, not twice
assert exc_info.value.context["operation"] == "Inner validation"Impact: Low priority - existing tests are comprehensive, this is defensive depth. 3. Exception Group Adoption Audit (Observation)Stat: 51 files now use exception groups, but the codebase has 1865 source files. Recommendation: Consider a follow-up PR to audit remaining files for exception tuples that could be centralized. Low priority since critical paths are already covered. Example grep to find candidates: grep -r "except (.*Error,.*Error" src/ | grep -v "exception_groups" | wc -l4. Documentation Cross-Reference (Minor)Location: The Quick Reference section is excellent, but consider adding a direct link to the #### Quick Reference
For comprehensive patterns including circuit breakers, retry logic, and graceful degradation, see [Error Handling Best Practices](docs/conventions/ERROR_HANDLING_BEST_PRACTICES.md).
```python
# Standard structured error
...Impact: Trivial - improves discoverability. 🎯 Code Quality Assessment
🚀 RecommendationsImmediate Actions (Pre-Merge)
Future Enhancements (Post-Merge)
📊 Impact AnalysisPositive Impacts
Risk Assessment
✅ Final VerdictAPPROVE with minor suggestions for future work. This PR is a significant quality improvement to the omnibase_core error handling infrastructure. The implementation is thorough, well-tested, and properly documented. The standardized patterns will improve code quality and maintainability across the project. Key Achievements
Great work on this foundational improvement! 🎉 📋 Checklist for Merge
Once CI confirms all checks pass, this PR is ready to merge. Review completed on 2026-01-13 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/omnibase_core/utils/util_forward_reference_resolver.py:
- Around line 144-145: Docstrings and an inconsistent exception tuple need
updating because VALIDATION_ERRORS now includes ValidationError; update all
docstrings that list "TypeError/ValueError" (lines noted in the review) to
include ValidationError, and replace the manual tuple "(TypeError, ValueError,
AttributeError)" with the exception-group-based form from
omnibase_core.errors.exception_groups — either use a provided ATTRIBUTE_ERRORS
group if available or combine VALIDATION_ERRORS with AttributeError (e.g.,
VALIDATION_ERRORS + (AttributeError,)) so the code and docs consistently
reference the centralized exception groups.
🧹 Nitpick comments (2)
src/omnibase_core/models/core/model_metadata_tool_collection.py (1)
295-298: Consider refining the broad exception catch.While the
# fallback-ok:marker properly documents this catch, the outerexcept Exceptionswallows all errors including potentially unexpected ones. For a registration method, consider either:
- Catching a more specific group like
PYDANTIC_MODEL_ERRORScombined with other expected exceptions, or- Logging the exception before returning
Falseto aid debuggingThis is a minor suggestion since the pattern is documented and the method semantics (returning
Falseon failure) are clear.src/omnibase_core/utils/util_forward_reference_resolver.py (1)
672-675: Consider using centralized exception group for consistency.This function still uses a manual exception tuple
(TypeError, ValueError, AttributeError)whilerebuild_model_references(line 322) andhandle_subclass_forward_refs(line 479) now useVALIDATION_ERRORS.If
VALIDATION_ERRORSincludes these exception types, consider using it here for consistency. IfAttributeErrorrequires separate handling (since it's excluded fromVALIDATION_ERRORSand caught separately at line 332), then combiningVALIDATION_ERRORSwithAttributeErrormay be appropriate.#!/bin/bash # Description: Check if ATTRIBUTE_ACCESS_ERRORS exists and could be combined with VALIDATION_ERRORS rg -n 'ATTRIBUTE_ACCESS_ERRORS\s*=' --type py
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CLAUDE.mdsrc/omnibase_core/backends/metrics/backend_metrics_prometheus.pysrc/omnibase_core/decorators/decorator_allow_any_type.pysrc/omnibase_core/decorators/decorator_convert_to_schema.pysrc/omnibase_core/decorators/decorator_pattern_exclusions.pysrc/omnibase_core/infrastructure/node_core_base.pysrc/omnibase_core/logging/logging_emit.pysrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/models/pipeline/model_pipeline_hook.pysrc/omnibase_core/nodes/node_compute.pysrc/omnibase_core/pipeline/handlers/handler_capability_caching.pysrc/omnibase_core/services/invariant/service_invariant_evaluator.pysrc/omnibase_core/services/replay/service_corpus_replay_orchestrator.pysrc/omnibase_core/services/service_contract_validator.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/omnibase_core/models/pipeline/model_pipeline_hook.py
- src/omnibase_core/decorators/decorator_pattern_exclusions.py
- src/omnibase_core/infrastructure/node_core_base.py
- src/omnibase_core/decorators/decorator_allow_any_type.py
- src/omnibase_core/services/invariant/service_invariant_evaluator.py
- src/omnibase_core/services/replay/service_corpus_replay_orchestrator.py
- src/omnibase_core/nodes/node_compute.py
- src/omnibase_core/services/service_contract_validator.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
poetry runfor Python commands; never usepip installor directpythonexecution
Files:
src/omnibase_core/backends/metrics/backend_metrics_prometheus.pysrc/omnibase_core/pipeline/handlers/handler_capability_caching.pysrc/omnibase_core/logging/logging_emit.pysrc/omnibase_core/decorators/decorator_convert_to_schema.pysrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use protocol names for dependency injection (e.g.,container.get_service('ProtocolEventBus'), never concrete class names)
RaiseModelOnexErrorwith specificEnumCoreErrorCodevalues; never use genericException
Use the@standard_error_handlingdecorator to eliminate try/catch boilerplate in async operations
Document broad exception handlers with standardized markers:# fallback-ok:,# catch-all-ok:,# cleanup-resilience-ok:,# boundary-ok:,# init-errors-ok:, or# tool-resilience-ok:
Order exception tuples alphabetically by exception class name (e.g.,(AttributeError, TypeError, ValidationError))
Use centralized exception groups fromomnibase_core.errors.exception_groupsinstead of manually constructing exception tuples
Use PEP 604 union syntax (str | None) instead ofOptionalorUniontype hints
Use absolute imports across packages; only use relative imports within tight module clusters
Order imports in three groups: standard library, third-party, first-party (omnibase_core), then local/relative imports
All TODO comments must reference a Linear ticket in format# TODO(OMN-XXXX): description; use# TODO(OMN-TBD): [NEEDS TICKET]only during triage
Place[NEEDS TICKET]marker on the first line of multi-line TODOs for grep-ability
Alltype: ignorecomments must include specific mypy error codes (e.g.,# type: ignore[arg-type]) with explanation comment above
Python files insrc/omnibase_core/must follow directory-specific naming prefixes (e.g.,model_*in models/,enum_*in enums/,decorator_*in decorators/)
UseFileRegistryto load YAML contracts from files viaregistry.load(Path(...))orregistry.load_all(Path(...))
Never share node instances across threads; use thread-local instances or separate instances per thread for NodeCompute, NodeEffect, ModelComputeCache, and ModelCircuitBreaker
Both mypy (strict mode) and pyright must pass in CI with 0 errors across all source files
Use ...
Files:
src/omnibase_core/backends/metrics/backend_metrics_prometheus.pysrc/omnibase_core/pipeline/handlers/handler_capability_caching.pysrc/omnibase_core/logging/logging_emit.pysrc/omnibase_core/decorators/decorator_convert_to_schema.pysrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Every Pydantic model MUST have an explicitmodel_configwithConfigDict()that declares field behavior (frozen, extra, from_attributes)
Frozen Pydantic models MUST includefrom_attributes=Truein ConfigDict for pytest-xdist compatibility
UseField(default_factory=list)for mutable default values; never use bare[]or{}
Files:
src/omnibase_core/models/core/model_metadata_tool_collection.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Document broad exception handlers with standardized markers: `# fallback-ok:`, `# catch-all-ok:`, `# cleanup-resilience-ok:`, `# boundary-ok:`, `# init-errors-ok:`, or `# tool-resilience-ok:`
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use the `standard_error_handling` decorator to eliminate try/catch boilerplate in async operations
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception groups from `omnibase_core.errors.exception_groups` instead of manually constructing exception tuples
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Raise `ModelOnexError` with specific `EnumCoreErrorCode` values; never use generic `Exception`
📚 Learning: 2026-01-13T15:05:43.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Document broad exception handlers with standardized markers: `# fallback-ok:`, `# catch-all-ok:`, `# cleanup-resilience-ok:`, `# boundary-ok:`, `# init-errors-ok:`, or `# tool-resilience-ok:`
Applied to files:
src/omnibase_core/backends/metrics/backend_metrics_prometheus.pysrc/omnibase_core/pipeline/handlers/handler_capability_caching.pysrc/omnibase_core/logging/logging_emit.pysrc/omnibase_core/decorators/decorator_convert_to_schema.pyCLAUDE.mdsrc/omnibase_core/models/core/model_metadata_tool_collection.py
📚 Learning: 2026-01-13T15:05:43.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception groups from `omnibase_core.errors.exception_groups` instead of manually constructing exception tuples
Applied to files:
src/omnibase_core/pipeline/handlers/handler_capability_caching.pysrc/omnibase_core/decorators/decorator_convert_to_schema.pyCLAUDE.mdsrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T15:05:43.781Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.781Z
Learning: Applies to src/omnibase_core/models/**/*.py : Every Pydantic model MUST have an explicit `model_config` with `ConfigDict()` that declares field behavior (frozen, extra, from_attributes)
Applied to files:
src/omnibase_core/pipeline/handlers/handler_capability_caching.py
📚 Learning: 2026-01-13T15:05:43.781Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.781Z
Learning: Applies to src/omnibase_core/models/**/*.py : Frozen Pydantic models MUST include `from_attributes=True` in ConfigDict for pytest-xdist compatibility
Applied to files:
src/omnibase_core/pipeline/handlers/handler_capability_caching.py
📚 Learning: 2026-01-13T15:05:43.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Raise `ModelOnexError` with specific `EnumCoreErrorCode` values; never use generic `Exception`
Applied to files:
src/omnibase_core/decorators/decorator_convert_to_schema.pyCLAUDE.mdsrc/omnibase_core/models/core/model_metadata_tool_collection.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T15:05:43.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/nodes/**/*.py : Use `ModelONEXContainer` (not `ModelContainer`) for dependency injection in node constructors
Applied to files:
src/omnibase_core/decorators/decorator_convert_to_schema.py
📚 Learning: 2026-01-13T15:05:43.781Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.781Z
Learning: Applies to src/omnibase_core/**/*.py : Python files in `src/omnibase_core/` must follow directory-specific naming prefixes (e.g., `model_*` in models/, `enum_*` in enums/, `decorator_*` in decorators/)
Applied to files:
src/omnibase_core/decorators/decorator_convert_to_schema.py
📚 Learning: 2026-01-13T15:05:43.781Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.781Z
Learning: Applies to src/omnibase_core/**/*.py : All `type: ignore` comments must include specific mypy error codes (e.g., `# type: ignore[arg-type]`) with explanation comment above
Applied to files:
src/omnibase_core/decorators/decorator_convert_to_schema.py
📚 Learning: 2026-01-13T15:05:43.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Use the `standard_error_handling` decorator to eliminate try/catch boilerplate in async operations
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T15:05:43.780Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T15:05:43.780Z
Learning: Applies to src/omnibase_core/**/*.py : Order exception tuples alphabetically by exception class name (e.g., `(AttributeError, TypeError, ValidationError)`)
Applied to files:
CLAUDE.md
🧬 Code graph analysis (2)
src/omnibase_core/pipeline/handlers/handler_capability_caching.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
src/omnibase_core/utils/util_forward_reference_resolver.py (1)
src/omnibase_core/protocols/validation/protocol_constraint_validation_result.py (1)
errors(80-82)
🪛 LanguageTool
CLAUDE.md
[grammar] ~436-~436: Use a hyphen to join words.
Context: ...rators Three decorators eliminate error handling boilerplate. All decorators fol...
(QB_NEW_EN_HYPHEN)
⏰ 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: Pyright Type Checking
- GitHub Check: claude-review
🔇 Additional comments (18)
src/omnibase_core/backends/metrics/backend_metrics_prometheus.py (1)
373-387: LGTM!The
# catch-all-ok:marker correctly documents this intentional broad exception handler per the project's standardized error handling patterns. The rationale is appropriate—retry loops legitimately need to catch all exceptions to implement exponential backoff, and the handler properly logs failures, tracks consecutive failures for circuit breaker behavior, and sanitizes URLs in log output.src/omnibase_core/logging/logging_emit.py (1)
270-291: LGTM!The updated
# boundary-ok:marker correctly documents this exception handler's intent per the project's standardized error handling markers. The decorator appropriately logs exception details at the boundary before re-raising, which aligns with the marker's documented use case for system/API boundaries. Based on learnings, this follows the required pattern for documenting broad exception handlers.CLAUDE.md (5)
385-433: LGTM!The "ValueError vs ModelOnexError Policy" section provides clear, actionable guidance with practical examples. The decision matrix is particularly useful for quick reference. This aligns well with the coding guidelines requiring
ModelOnexErrorwith specificEnumCoreErrorCodevalues for ONEX-specific errors.
434-473: LGTM!The decorator documentation clearly explains the ONEX exception contract, particularly the critical point that cancellation signals always propagate. The three-decorator approach (
standard_error_handling,validation_error_handling,io_error_handling) provides appropriate specialization for common scenarios. This aligns with the retrieved learnings about using@standard_error_handlingto eliminate try/catch boilerplate.
543-567: LGTM!The explicit documentation that
asyncio.CancelledErroris NOT in any group is critical for async correctness. The code example correctly demonstrates the required pattern: catch and re-raiseCancelledErrorbefore handling other async errors. This prevents accidental swallowing of task cancellation, which would cause subtle bugs in async workflows.
569-597: LGTM!The Quick Reference section provides practical, copy-paste-ready examples that reinforce the patterns documented earlier. The example combining
FILE_IO_ERRORSwith the# fallback-ok:marker demonstrates proper usage of both exception groups and documentation markers together.
489-527: LGTM!The example usage section demonstrates practical applications of each marker type. The exception tuple ordering section clearly illustrates the alphabetical ordering requirement per the coding guidelines. The before/after format makes the convention immediately clear to developers.
src/omnibase_core/pipeline/handlers/handler_capability_caching.py (2)
82-102: LGTM on the generate_cache_key implementation structure.The fallback pattern using
str(data).encode()for non-serializable data is appropriate, and the# fallback-ok:marker correctly documents the rationale for the broad exception catch per coding guidelines.
31-31: The code is correct.VALIDATION_ERRORSfromomnibase_core.errors.exception_groupsincludes bothTypeErrorandValueError(in addition toValidationError), so the exception handling properly catches the exceptionsjson.dumps()can raise for non-serializable objects and circular references. The# fallback-ok:marker is correctly applied per coding guidelines.src/omnibase_core/models/core/model_metadata_tool_collection.py (3)
26-26: LGTM!The import correctly references the centralized exception group from
omnibase_core.errors.exception_groups, as per the coding guidelines.
107-114: LGTM!The exception handling correctly uses the centralized
PYDANTIC_MODEL_ERRORSgroup and includes the required# fallback-ok:documentation marker with clear rationale.
256-262: LGTM!The inner exception handling correctly uses the centralized
PYDANTIC_MODEL_ERRORSgroup with the required documentation marker.src/omnibase_core/utils/util_forward_reference_resolver.py (2)
322-331: Verify docstring consistency with expanded exception handling.The docstrings at lines 177-178 and 209-215 specifically document
TypeErrorandValueErrorhandling. IfVALIDATION_ERRORSincludes additional exception types (e.g.,ValidationError), the documented behavior no longer matches the implementation.If
VALIDATION_ERRORSis broader, update the docstring's error handling section (lines 172-221) to reflect all caught exception types.
479-487: Same docstring consistency concern applies here.The docstring section at lines 380-391 specifically documents
TypeError/ValueErrorbehavior with detailed explanations. Ensure consistency ifVALIDATION_ERRORScatches additional exception types.src/omnibase_core/decorators/decorator_convert_to_schema.py (4)
75-75: LGTM - centralized exception group importThe import correctly uses the centralized
PYDANTIC_MODEL_ERRORSexception group fromomnibase_core.errors.exception_groups, aligning with the coding guidelines for standardized error handling.
392-414: Consistent error handling for all-None dict path.This handler follows the same well-structured pattern as
_convert_list_value. Thesample_keys[:5]limit is a good practice to avoid excessive log output for large dictionaries.
448-470: LGTM - consistent error handling for general dict conversion.The error handling pattern is consistent across all three conversion functions. Each provides appropriate context for debugging conversion failures.
330-352: Exception handling pattern is correct and complete.The exception handler correctly:
- Catches the centralized
PYDANTIC_MODEL_ERRORSgroup, which includesValidationError(that wrapsModelOnexErrorfrom field validators inModelNumericValue.from_numeric())- Logs diagnostic info with relevant context before re-raising
- Wraps in
ModelOnexErrorwith specific error code and detailed context- Preserves the exception chain via
from eAll exceptions that
ModelSchemaValue.from_value()can raise—including those from nestedModelNumericValuefield validation—are covered byPYDANTIC_MODEL_ERRORS.
Resolved conflict in decorator_error_handling.py: - Kept async-aware implementation from our branch - Preserved functools.wraps on all wrappers
Improvements to error handling based on PR review: **Duck-typing heuristic improvements** (decorator_error_handling.py): - Added _is_pydantic_validation_error_structure() to validate errors() output - Added _has_pydantic_error_count_method() for more reliable Pydantic detection - Added _VALIDATION_NAME_FALSE_POSITIVES denylist to exclude timeout/cancellation errors that contain "validation" in the name - Updated _is_validation_error() with improved multi-tier detection strategy - Added comprehensive docstrings documenting limitations and strategy **Exception handling consistency** (mixin_node_service.py): - Switched to PYDANTIC_MODEL_ERRORS exception group for consistency - Fixed alphabetical ordering of exception tuple - Separated RuntimeError handling with explicit comment **Comment marker fixes**: - model_field_accessor.py: Reformatted fallback-ok marker to follow standards - model_metrics_context.py: Fixed error-ok markers for Pydantic validators - util_forward_reference_resolver.py: Updated docstrings for exception groups **Tests**: - Added 13 new tests covering duck-typing heuristic edge cases
PR Review: Standardize Error Handling Patterns (OMN-1299)SummaryThis PR successfully establishes comprehensive error handling patterns across omnibase_core with 360+ documentation markers, increased decorator adoption (7→50 usages), and centralized exception groups (17→51 files). The changes align well with the ONEX architecture and significantly improve code consistency. ✅ Strengths1. Excellent Documentation
2. Centralized Exception GroupsThe
# Excellent example of centralized pattern
from omnibase_core.errors.exception_groups import PYDANTIC_MODEL_ERRORS
try:
result = model.model_validate(data)
except PYDANTIC_MODEL_ERRORS as e:
# Handle validation errors consistently
pass3. Decorator AdoptionThe increased use of 4. Test Coverage
🔍 Issues & Recommendations1. CRITICAL: Runtime Coupling in
|
| Metric | Value | Status |
|---|---|---|
| Files Changed | 98 | ✅ Large but focused |
| Lines Added | 1,975 | ✅ Mostly docs/comments |
| Lines Removed | 472 | ✅ Good cleanup |
| Test Coverage | +622 test lines | ✅ Strong |
| Documentation | +154 CLAUDE.md lines | ✅ Excellent |
🎯 Recommendations Summary
Must Fix (Before Merge)
- Simplify
_is_validation_error()- Remove brittle duck typing (decorator_error_handling.py:151-195) - Add async cancellation test - Verify CancelledError propagation
Should Fix (High Priority)
- Standardize comment marker placement - Always on line after
except - Clarify ValueError policy - Add dedicated section in CLAUDE.md
Nice to Have (Low Priority)
- Pre-compute exception group unions - Minor perf optimization
🏆 Overall Assessment
Status: ✅ Approve with Recommendations
This PR represents significant improvement to error handling consistency. The centralized exception groups, decorator adoption, and documentation are all excellent patterns. The identified issues are minor and mostly related to edge case handling and optimization opportunities.
Key Wins:
- 360+ exception handlers now have clear documentation markers
- 43 additional files use centralized exception groups (17→51)
- 43 additional decorators eliminate error handling boilerplate (7→50)
- Comprehensive documentation guides future development
Merge Recommendation: Approve after addressing the critical duck-typing issue in _is_validation_error() and adding the async cancellation test. Other recommendations can be addressed in follow-up PRs.
Reviewed by: Claude Sonnet 4.5
Review Date: 2026-01-13
Review Methodology: Full diff analysis, architecture alignment check, CLAUDE.md compliance verification
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CLAUDE.md (1)
516-516: Minor grammar refinement: compound modifier hyphenation.According to English grammar conventions, compound modifiers before nouns should be hyphenated. Consider "error-handling boilerplate" instead of "error handling boilerplate" for grammatical precision.
src/omnibase_core/decorators/decorator_error_handling.py (2)
102-195: Well-documented multi-tier validation detection strategy.The three-tier approach is sound and well-ordered by reliability. One minor inconsistency to verify:
The comment at line 86 says "Pattern: exception class name suffix (case-insensitive matching)" but the implementation at line 190 checks
exc_class_name_lower not in _VALIDATION_NAME_FALSE_POSITIVES, which requires an exact match of the full lowercased class name, not a suffix match.This appears intentional (exact match is safer), but the comment is misleading. Consider updating the comment to reflect the actual behavior.
📝 Suggested comment fix
# Denylist of class name patterns that contain "validation" but are NOT # validation errors. These are typically timeout/cancellation/infrastructure # errors that happen to be related to a validation process. -# Pattern: exception class name suffix (case-insensitive matching) +# Pattern: exact exception class name (case-insensitive matching) _VALIDATION_NAME_FALSE_POSITIVES = frozenset(
446-459: Consider adding a comment explaining why the inline tuple is used instead ofFILE_IO_ERRORS.The coding guideline requires using centralized exception constants from
exception_groups, butFILE_IO_ERRORScannot be directly substituted here because it doesn't includeIsADirectoryError, which this code deliberately handles to distinguishFILE_NOT_FOUNDfromFILE_OPERATION_ERROR. Adding a brief comment explaining this rationale would clarify the intent.This applies to both the sync (lines 446–459) and async (lines 486–499) versions of the decorator.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CLAUDE.mdsrc/omnibase_core/decorators/decorator_error_handling.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution. Never use pip or python directly.
Use protocol names for dependency injection (e.g., 'ProtocolEventBus'), never concrete class names.
Use PEP 604 union syntax (X | Y) instead of Union[X, Y] and use Optional syntax is forbidden in favor of X | None.
Files:
src/omnibase_core/decorators/decorator_error_handling.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Always use ModelOnexError with error codes instead of generic Exception.
Use the @standard_error_handling decorator to eliminate try/catch boilerplate in async operations.
Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py.
Never use relative imports across package boundaries. Use absolute imports for stability and readability.
All TODO comments must reference a Linear ticket in the format # TODO(OMN-XXXX): description or use # TODO(OMN-TBD): description [NEEDS TICKET].
All type: ignore comments must include specific error codes and explanations above the line.
Docstrings must explain WHY, describe non-obvious behavior, or document edge cases. Do not write tautological docstrings that restate the method name.
Exception tuples in except clauses must be alphabetically ordered by exception class name.
Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Both mypy and pyright must pass in CI with 100% strict mode compliance (0 errors across all source files).
Files:
src/omnibase_core/decorators/decorator_error_handling.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use the standard_error_handling decorator to eliminate try/catch boilerplate in async operations.
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError with error codes instead of generic Exception.
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use the standard_error_handling decorator to eliminate try/catch boilerplate in async operations.
Applied to files:
src/omnibase_core/decorators/decorator_error_handling.pyCLAUDE.md
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError with error codes instead of generic Exception.
Applied to files:
src/omnibase_core/decorators/decorator_error_handling.pyCLAUDE.md
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Applied to files:
src/omnibase_core/decorators/decorator_error_handling.pyCLAUDE.md
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Applied to files:
src/omnibase_core/decorators/decorator_error_handling.pyCLAUDE.md
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : All type: ignore comments must include specific error codes and explanations above the line.
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Exception tuples in except clauses must be alphabetically ordered by exception class name.
Applied to files:
CLAUDE.md
🧬 Code graph analysis (1)
src/omnibase_core/decorators/decorator_error_handling.py (2)
src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(11-257)src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
🪛 LanguageTool
CLAUDE.md
[grammar] ~516-~516: Use a hyphen to join words.
Context: ...rators Three decorators eliminate error handling boilerplate. All decorators fol...
(QB_NEW_EN_HYPHEN)
⏰ 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 19/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 10/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: claude-review
🔇 Additional comments (8)
CLAUDE.md (1)
465-677: Excellent comprehensive error handling documentation!This section provides clear, actionable guidance that aligns perfectly with the project's error handling standards. The additions are particularly valuable:
- Decision Matrix (lines 504-512): Helps developers choose between
ValueErrorandModelOnexErrorbased on context- Decorator Documentation (lines 514-553): Shows how the three decorators eliminate boilerplate while maintaining consistent error contracts
- Exception Groups Table (lines 625-635): Well-structured reference for centralized exception constants
- asyncio.CancelledError Warning (lines 636-647): Correctly emphasizes that cancellation must be handled separately and always re-raised
The examples are clear, the guidance is actionable, and the content integrates seamlessly with the existing documentation structure.
Based on learnings, this documentation correctly reflects the project's standards for exception comment markers, the standard_error_handling decorator, centralized exception groups, ModelOnexError usage, and alphabetical exception tuple ordering.
src/omnibase_core/decorators/decorator_error_handling.py (7)
1-23: LGTM!The imports properly use centralized exception constants from
exception_groupsand the module docstring clearly documents the exception handling contract. Based on learnings, this follows the project's error handling patterns.
25-56: LGTM!The Pydantic structure validation is well-implemented with appropriate performance optimization (checking only the first error) and clear documentation of limitations.
59-80: LGTM!The helper correctly identifies Pydantic's
error_count()method with proper exception handling using thefallback-okmarker as per coding guidelines.
83-99: LGTM!Good use of
frozensetfor immutable constant lookup, and the denylist patterns appropriately exclude infrastructure errors that contain "validation" but are not actual validation errors.
241-294: LGTM - Proper exception handling contract implementation.The decorator correctly:
- Propagates cancellation/exit signals (never catching them)
- Re-raises
ModelOnexErrorto preserve context- Wraps generic exceptions with enriched metadata
The exception tuple is alphabetically ordered per coding guidelines. The code duplication between async/sync wrappers is acceptable here for clarity and maintainability—extracting shared logic would add complexity without significant benefit.
325-402: LGTM - Validation-specific error handling is correct.The decorator properly distinguishes between validation errors (using the multi-tier
_is_validation_error()detection) and other failures, applying the appropriate error codes. Theis_validation_error=Trueflag provides useful downstream context.
406-511: LGTM - I/O error handling with proper categorization.The decorator correctly categorizes file system errors with appropriate error codes (
FILE_NOT_FOUNDvsFILE_OPERATION_ERROR) and sets theis_file_errormetadata flag for downstream processing.
- Fix import ordering in service_protocol_migrator.py (ModelOnexError import moved after docstring) - Standardize error markers in model_metrics_context.py (change 7 instances of `# validation-error-ok` to `# error-ok` for consistency with codebase conventions) - Add Checker* classes to naming validation exception patterns to fix false positive on CheckerEnumMemberCasing class
PR Review: Error Handling Standardization (OMN-1299)SummaryThis is an excellent, comprehensive PR that significantly improves error handling consistency across omnibase_core. The changes are well-structured, thoroughly documented, and extensively tested. ✅ Strengths1. Documentation Excellence
2. Code Quality
3. Robust Implementation
4. Test Coverage
5. Consistency Improvements
🔍 Observations & Minor Suggestions1. Exception Detection ComplexityThe Consideration: Monitor for false positives/negatives in production. The Tier 3 class name heuristic may need denylist expansion as new libraries are added. File: 2. Potential Performance ImpactThe validation error detection involves:
Question: Has profiling been done on hot paths? The decorator is used 50+ times now. Suggestion: If performance becomes an issue, consider caching validation error class names or using 3. Documentation ConsistencyThe PR description mentions "Added 360+ documentation markers" but doesn't specify the breakdown. Consider adding: This helps reviewers understand the distribution patterns. 4. Validation Name False PositivesThe denylist in _VALIDATION_NAME_FALSE_POSITIVES = frozenset({
"validationtimeouterror",
"validationcancellederror",
# ... etc
})File: Suggestion: Document in comments which libraries these patterns come from (if known), or add a note that this list should be extended when new false positives are discovered. 5. Import OrganizationIn 🔒 Security Considerations✅ No Security Concerns Identified
🧪 Testing AssessmentCoverage Analysis
RecommendationsConsider adding tests for:
📋 Compliance with CLAUDE.md Standards
🎯 Final VerdictAPPROVE with Minor ObservationsThis PR is production-ready and represents a significant quality improvement. The observations above are for future consideration, not blocking issues. Impact Summary
RecommendationMerge when CI passes. This is exemplary work. 📚 Suggested Follow-ups (Post-Merge)
Reviewed by: Claude Sonnet 4.5 Great work, @jonahgabriel! This sets an excellent standard for error handling across the ONEX framework. 🎉 |
- Add explicit YAML document start markers (---) - Convert string versions to ModelSemVer format as required by ONEX standards
Resolve merge conflict in node_core_base.py by keeping both: - FILE_IO_ERRORS import from this branch - EnumNodeHealthStatus/EnumNodeLifecycleStatus imports from main
Address remaining PR review feedback for error handling standardization: - Add standardized comment markers to mixin_node_service.py exception handlers (boundary-ok, cleanup-resilience-ok, fallback-ok) - Fix error-ok markers in model_metrics_context.py to use proper format - Update util_forward_reference_resolver.py docstrings to accurately reflect VALIDATION_ERRORS includes ValidationError - Fix alphabetical ordering of exception tuples across 9 files (13 fixes): - decorator_convert_to_schema.py - mixin_event_bus.py (4 fixes) - mixin_fail_fast.py - mixin_node_executor.py - mixin_service_registry.py - model_event_bus_listener_handle.py - model_effect_transaction.py - util_field_converter.py - util_safe_yaml_loader.py (4 fixes)
Code Review: Standardize Error Handling Patterns (OMN-1299)SummaryThis PR establishes comprehensive error handling standards across omnibase_core with 360+ documentation markers, expanded decorator usage (7 → 50), and centralized exception groups (17 → 51 files). The implementation is well-architected and aligns with the project's quality standards. ✅ Strengths1. Excellent Documentation
2. Architectural Consistency
3. Test Quality
4. Code Hygiene
🔍 Observations & Minor Suggestions1. Exception Detection Complexity (src/omnibase_core/decorators/decorator_error_handling.py)The validation error detection logic in def _has_pydantic_error_count_method(exc: Exception) -> bool:
"""Check if exception has Pydantic's error_count() method."""
error_count_attr = getattr(exc, "error_count", None)
if not callable(error_count_attr):
return False
try:
result = error_count_attr()
return isinstance(result, int)
except Exception:
# fallback-ok: error_count() call failed, not Pydantic-like
return FalseObservation: The heuristics are well-tested (lines 884-1138 in test file), but this introduces fragility if Pydantic's internal API changes. Consider:
2. ValueError vs ModelOnexError Policy (CLAUDE.md)The decision matrix is clear, but the distinction between "Standard Python validation" and "ONEX-specific domain errors" can be subjective. Example from CLAUDE.md: | Invalid function argument | ValueError | Standard Python convention |
| Missing required config | ModelOnexError | Needs error code for logging |Question: What about "invalid node_id format" - is that a function argument (ValueError) or domain error (ModelOnexError)? Recommendation: Consider adding 2-3 ambiguous examples to the decision matrix to guide developers in gray areas. 3. Exception Group CoverageThe
Recommendation: Consider adding database-specific groups in a follow-up PR if database operations are common. 4. YAML Contract Changes (examples/contracts/handlers/*.yaml)The version format changed from strings to structured objects: # Before
version: 1.0.0
# After
version: {major: 1, minor: 0, patch: 0}Question: Is this intentional cleanup or related to error handling changes? The PR description doesn't mention it. Recommendation: If unrelated, consider separating this change or adding a note in the PR description. 🐛 Potential IssuesNone FoundAll code follows project conventions, tests are comprehensive, and the implementation is production-ready. 📊 Impact Assessment
🚀 RecommendationsBefore Merge
Follow-up PRs (Optional)
🎯 VerdictApproved ✅ This is excellent work that significantly improves error handling consistency across the codebase. The implementation is:
The minor observations above are suggestions for future improvements, not blockers. This PR delivers exactly what OMN-1299 promised and follows all project conventions from CLAUDE.md. Great job, @jonahgabriel! 🎉 Reviewed by: Claude Sonnet 4.5 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/omnibase_core/utils/util_field_converter.py (1)
78-94: Use centralized exception group and add intent marker.Replace the inline exception tuple with
PYDANTIC_MODEL_ERRORSfromomnibase_core.errors.exception_groups. This provides the required AttributeError, TypeError, and ValueError, plus ValidationError for Pydantic field operations. Add the# init-errors-ok:marker to document the intent of providing defaults on error:Suggested change at line 78-79:
except PYDANTIC_MODEL_ERRORS as e: # init-errors-ok: use default or raise ModelOnexError # Catch type conversion errors or attribute access issues # Use default if availablesrc/omnibase_core/utils/util_safe_yaml_loader.py (1)
523-538:PYDANTIC_MODEL_ERRORSis missingKeyErrorcompared to the original inline tuple.The centralized exception group
PYDANTIC_MODEL_ERRORSincludes(AttributeError, TypeError, ValidationError, ValueError), but the original inline tuple was(AttributeError, TypeError, KeyError, ValueError). This change removes coverage forKeyErrorexceptions. While the current code path does not raiseKeyErrordirectly (using.get()instead of[]access), this is a regression that should be addressed: either addKeyErrortoPYDANTIC_MODEL_ERRORSif it is a common pattern, or create a function-specific exception tuple that extendsPYDANTIC_MODEL_ERRORS + (KeyError,)to preserve the original exception handling semantics.
🧹 Nitpick comments (5)
src/omnibase_core/mixins/mixin_fail_fast.py (1)
275-280: Alphabetical ordering is correct.The exception tuple
(KeyError, RuntimeError, ValueError)is now properly alphabetized per coding guidelines.As an optional follow-up, consider whether this tuple should be extracted to a centralized constant in
exception_groups.py(e.g.,DEPENDENCY_CHECK_ERRORS) to align with the PR's broader goal of using centralized exception groups. Based on learnings, the project prefers centralized exception constants over inline tuples.src/omnibase_core/models/context/model_metrics_context.py (2)
148-176: Consider addingerror-ok:markers for consistency.The validator correctly uses
objecttype hint formode="before"and validates type before pattern matching. However, theValueErrorraises on lines 168 and 172 lack the# error-ok: Pydantic validator requires ValueErrormarkers that are present in_validate_semver(lines 62, 66).For consistency with the PR's goal of adding standardized documentation markers, consider adding the markers here as well.
🔧 Suggested markers
if not isinstance(value, str): + # error-ok: Pydantic validator requires ValueError raise ValueError(f"trace_id must be a string, got {type(value).__name__}") # Normalize to lowercase for comparison normalized = value.lower() if not _TRACE_ID_PATTERN.match(normalized): + # error-ok: Pydantic validator requires ValueError raise ValueError( f"Invalid trace_id '{value}': must be 32 lowercase hex characters " "(W3C Trace Context format)" )
178-214: Good refactoring to unify span validators.Using
ValidationInfo.field_nameto handle bothspan_idandparent_span_idwith field-specific error messages is a clean approach that reduces code duplication.Same optional suggestion applies: consider adding
# error-ok:markers to theValueErrorraises on lines 205-207 and 211-213 for consistency with the helper function pattern.src/omnibase_core/utils/util_safe_yaml_loader.py (1)
133-147: LGTM! Alphabetical ordering is correct.The exception tuple
(AttributeError, OSError, RuntimeError, TypeError)is properly ordered alphabetically.Consider using a centralized exception group.
For consistency with line 523's use of
PYDANTIC_MODEL_ERRORS, consider whether this tuple could be consolidated into a centralized exception group (e.g.,FILE_IO_ERRORSor a new group). This is optional and could be deferred. Based on learnings, the codebase prefers centralized exception constants fromomnibase_core.errors.exception_groups.src/omnibase_core/mixins/mixin_node_service.py (1)
61-90: Consider maintaining alphabetical ordering within concatenated tuples.The module-level exception tuples use centralized groups as recommended, but the concatenation breaks alphabetical ordering. For example,
_SERVICE_LOOKUP_ERRORSresults in(AttributeError, TypeError, ValidationError, ValueError, RuntimeError)whereRuntimeErrorshould come beforeTypeError.If strict alphabetical ordering is desired even within constants, you could define them with explicit ordering:
♻️ Optional: Maintain alphabetical order in concatenated tuples
-_SERVICE_LOOKUP_ERRORS: tuple[type[Exception], ...] = PYDANTIC_MODEL_ERRORS + ( - RuntimeError, # init-errors-ok: service not available or container issues -) +# Alphabetically ordered: AttributeError, RuntimeError, TypeError, ValidationError, ValueError +_SERVICE_LOOKUP_ERRORS: tuple[type[Exception], ...] = ( + AttributeError, + RuntimeError, # init-errors-ok: service not available or container issues + TypeError, + ValidationError, + ValueError, +) -_TOOL_EXECUTION_ERRORS: tuple[type[Exception], ...] = PYDANTIC_MODEL_ERRORS + ( - KeyError, - ModelOnexError, - OSError, - RuntimeError, - TimeoutError, -) +# Alphabetically ordered: AttributeError, KeyError, ModelOnexError, OSError, RuntimeError, +# TimeoutError, TypeError, ValidationError, ValueError +_TOOL_EXECUTION_ERRORS: tuple[type[Exception], ...] = ( + AttributeError, + KeyError, + ModelOnexError, + OSError, + RuntimeError, + TimeoutError, + TypeError, + ValidationError, + ValueError, +)However, the current approach with concatenation provides clearer semantic grouping (PYDANTIC_MODEL_ERRORS + domain-specific errors), so this is a stylistic preference. The benefit of using the centralized group outweighs strict alphabetical ordering here.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/omnibase_core/decorators/decorator_convert_to_schema.pysrc/omnibase_core/infrastructure/node_core_base.pysrc/omnibase_core/mixins/mixin_event_bus.pysrc/omnibase_core/mixins/mixin_fail_fast.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/mixins/mixin_service_registry.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/event_bus/model_event_bus_listener_handle.pysrc/omnibase_core/models/infrastructure/model_effect_transaction.pysrc/omnibase_core/utils/util_field_converter.pysrc/omnibase_core/utils/util_forward_reference_resolver.pysrc/omnibase_core/utils/util_safe_yaml_loader.py
✅ Files skipped from review due to trivial changes (1)
- src/omnibase_core/models/infrastructure/model_effect_transaction.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/omnibase_core/mixins/mixin_service_registry.py
- src/omnibase_core/decorators/decorator_convert_to_schema.py
- src/omnibase_core/infrastructure/node_core_base.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. Never use pip or python directly.
Use protocol names for dependency injection (e.g., 'ProtocolEventBus'), never concrete class names.
Use PEP 604 union syntax (X | Y) instead of Union[X, Y] and use Optional syntax is forbidden in favor of X | None.
Files:
src/omnibase_core/mixins/mixin_fail_fast.pysrc/omnibase_core/models/event_bus/model_event_bus_listener_handle.pysrc/omnibase_core/utils/util_field_converter.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_forward_reference_resolver.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/mixins/mixin_event_bus.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Always use ModelOnexError with error codes instead of generic Exception.
Use the @standard_error_handling decorator to eliminate try/catch boilerplate in async operations.
Python files in src/omnibase_core/ must follow directory-specific naming prefixes enforced by checker_naming_convention.py.
Never use relative imports across package boundaries. Use absolute imports for stability and readability.
All TODO comments must reference a Linear ticket in the format # TODO(OMN-XXXX): description or use # TODO(OMN-TBD): description [NEEDS TICKET].
All type: ignore comments must include specific error codes and explanations above the line.
Docstrings must explain WHY, describe non-obvious behavior, or document edge cases. Do not write tautological docstrings that restate the method name.
Exception tuples in except clauses must be alphabetically ordered by exception class name.
Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Both mypy and pyright must pass in CI with 100% strict mode compliance (0 errors across all source files).
Files:
src/omnibase_core/mixins/mixin_fail_fast.pysrc/omnibase_core/models/event_bus/model_event_bus_listener_handle.pysrc/omnibase_core/utils/util_field_converter.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_forward_reference_resolver.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/mixins/mixin_event_bus.py
src/omnibase_core/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/*.py: Every Pydantic model must have an explicit model_config using ConfigDict. Empty ConfigDict() is not allowed.
Frozen Pydantic models must include from_attributes=True in ConfigDict for pytest-xdist compatibility.
Use default_factory for mutable Pydantic field defaults (list, dict) to avoid shared state bugs.
Files:
src/omnibase_core/models/event_bus/model_event_bus_listener_handle.pysrc/omnibase_core/models/context/model_metrics_context.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use the standard_error_handling decorator to eliminate try/catch boilerplate in async operations.
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError with error codes instead of generic Exception.
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Exception tuples in except clauses must be alphabetically ordered by exception class name.
Applied to files:
src/omnibase_core/mixins/mixin_fail_fast.pysrc/omnibase_core/models/event_bus/model_event_bus_listener_handle.pysrc/omnibase_core/utils/util_field_converter.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_forward_reference_resolver.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/mixins/mixin_event_bus.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use centralized exception constants from omnibase_core.errors.exception_groups instead of writing exception tuples inline.
Applied to files:
src/omnibase_core/utils/util_field_converter.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_forward_reference_resolver.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/mixins/mixin_event_bus.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Always use ModelOnexError with error codes instead of generic Exception.
Applied to files:
src/omnibase_core/utils/util_field_converter.pysrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/mixins/mixin_node_executor.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_forward_reference_resolver.pysrc/omnibase_core/mixins/mixin_node_service.pysrc/omnibase_core/mixins/mixin_event_bus.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Use exception comment markers (fallback-ok:, catch-all-ok:, cleanup-resilience-ok:, boundary-ok:, init-errors-ok:, tool-resilience-ok:) to document intent for broad exception handlers.
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/utils/util_safe_yaml_loader.pysrc/omnibase_core/utils/util_forward_reference_resolver.pysrc/omnibase_core/mixins/mixin_node_service.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : All type: ignore comments must include specific error codes and explanations above the line.
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/models/**/*.py : Every Pydantic model must have an explicit model_config using ConfigDict. Empty ConfigDict() is not allowed.
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/utils/util_safe_yaml_loader.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/models/**/*.py : Frozen Pydantic models must include from_attributes=True in ConfigDict for pytest-xdist compatibility.
Applied to files:
src/omnibase_core/utils/util_safe_yaml_loader.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to src/omnibase_core/**/*.py : Never use relative imports across package boundaries. Use absolute imports for stability and readability.
Applied to files:
src/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T16:21:16.950Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T16:21:16.950Z
Learning: Applies to **/nodes/node_*.py : Use ModelONEXContainer in node constructors, never ModelContainer. ModelONEXContainer provides dependency injection with service resolution via get_service().
Applied to files:
src/omnibase_core/mixins/mixin_node_service.py
🧬 Code graph analysis (3)
src/omnibase_core/mixins/mixin_node_executor.py (1)
src/omnibase_core/models/errors/model_onex_error.py (1)
ModelOnexError(49-354)
src/omnibase_core/utils/util_forward_reference_resolver.py (1)
tests/unit/decorators/test_error_handling_extended.py (5)
errors(899-900)errors(1061-1070)errors(1087-1088)errors(1106-1107)errors(1125-1127)
src/omnibase_core/mixins/mixin_node_service.py (2)
src/omnibase_core/models/errors/model_onex_error.py (3)
ModelOnexError(49-354)message(248-250)error_code(253-255)src/omnibase_core/mixins/mixin_event_bus.py (1)
publish(81-83)
⏰ 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 10/20)
- GitHub Check: Tests (Split 9/20)
- GitHub Check: Tests (Split 12/20)
- GitHub Check: Tests (Split 15/20)
- GitHub Check: Tests (Split 17/20)
- GitHub Check: Tests (Split 13/20)
- GitHub Check: Tests (Split 19/20)
- GitHub Check: Tests (Split 16/20)
- GitHub Check: Tests (Split 2/20)
- GitHub Check: Tests (Split 20/20)
- GitHub Check: Tests (Split 7/20)
- GitHub Check: Tests (Split 14/20)
- GitHub Check: Tests (Split 6/20)
- GitHub Check: Tests (Split 4/20)
- GitHub Check: Tests (Split 18/20)
- GitHub Check: Tests (Split 1/20)
- GitHub Check: Tests (Split 3/20)
- GitHub Check: Tests (Split 8/20)
- GitHub Check: Tests (Split 11/20)
- GitHub Check: Tests (Split 5/20)
- GitHub Check: claude-review
🔇 Additional comments (32)
src/omnibase_core/models/event_bus/model_event_bus_listener_handle.py (1)
192-206: LGTM! Exception tuple correctly alphabetized.The reordering from
(TypeError, RuntimeError)to(RuntimeError, TypeError)now complies with the repository's guideline requiring alphabetically ordered exception tuples in except clauses. The inline comments appropriately document the rationale for catching each exception type.src/omnibase_core/mixins/mixin_node_executor.py (1)
423-424: LGTM! Exception tuple now correctly alphabetically ordered.The reordering from
(ValueError, RuntimeError, ModelOnexError)to(ModelOnexError, RuntimeError, ValueError)aligns with the coding guideline requiring exception tuples to be alphabetically sorted by class name. Based on learnings, this is the expected pattern for this codebase.src/omnibase_core/models/context/model_metrics_context.py (7)
20-24: LGTM!Imports are appropriate for the Pydantic model with field validators. The addition of
ValidationInfocorrectly supports the unifiedvalidate_span_formatvalidator.
26-43: LGTM!The regex patterns are correctly defined for W3C Trace Context compliance. The documentation explaining the SemVer pattern duplication to avoid circular imports is helpful.
46-69: LGTM!The
error-ok:markers correctly document the intentional use ofValueErroras required by Pydantic field validators. The docstring accurately explains this design choice. Based on learnings and coding guidelines, this follows the project's error handling documentation pattern.
117-146: LGTM!The
model_configcorrectly usesConfigDictwithfrozen=True,extra="forbid", andfrom_attributes=Trueas required by coding guidelines for frozen Pydantic models. Field definitions use proper PEP 604 union syntax (str | None,float | None).
216-244: LGTM!The validator correctly handles the edge case where Python booleans are subclasses of
int. Checkingisinstance(value, bool)first ensures thatTrue/Falseare rejected despite passingisinstance(value, int). The explicit conversion tofloathandles integer inputs properly.
246-272: LGTM!The validator correctly type-checks before delegating to
_validate_semver, which has theerror-ok:markers. This separation keeps the validation logic DRY while maintaining proper documentation.
274-293: LGTM!The utility methods
is_sampled()andhas_parent()have clear, correct logic with appropriate docstrings that explain the behavior without being tautological.src/omnibase_core/utils/util_forward_reference_resolver.py (4)
144-148: LGTM!Correctly imports centralized exception groups per the coding guidelines, replacing inline exception tuples with standardized constants. Based on learnings, this is the preferred pattern.
180-181: LGTM!The exception hierarchy is well-structured—specific Pydantic errors are caught first, then the broader
VALIDATION_ERRORSgroup. The dynamictype(e).__name__capture ensures accurate error_type reporting regardless of which exception from the group was raised.Minor note: The docstring at lines 180-181 explicitly lists the group contents. If
VALIDATION_ERRORSis updated inexception_groups.py, this docstring should be updated accordingly to stay accurate.Also applies to: 325-334
383-396: LGTM!The docstring accurately documents the error handling behavior—
VALIDATION_ERRORSare logged at WARNING level with a UserWarning, allowing subclass creation to proceed while alerting developers to potential configuration issues. The explicit mention ofValidationErrorat line 396 is helpful for developers debugging Pydantic validation failures during subclass creation.Also applies to: 484-492
525-526: LGTM!The use of
PYDANTIC_MODEL_ERRORSfor fail-fast behavior is appropriate here. Unlike the deferred handling inhandle_subclass_forward_refs, errors during automatic module-load rebuilds should crash the import to surface configuration problems early. The inline comment at lines 677-678 documenting the group contents improves readability for developers debugging import failures.Also applies to: 676-682
src/omnibase_core/utils/util_safe_yaml_loader.py (4)
46-46: LGTM!Import of
PYDANTIC_MODEL_ERRORSfrom centralized exception groups aligns with the coding guidelines to use centralized exception constants instead of inline tuples.
202-215: LGTM!The exception tuple
(AttributeError, RuntimeError, TypeError, ValueError)is correctly ordered alphabetically.
335-348: LGTM!The exception tuple
(AttributeError, RuntimeError, TypeError, yaml.YAMLError)is correctly ordered alphabetically (comparingYAMLErroras the class name).
383-392: LGTM!The exception tuple is correctly ordered alphabetically, consistent with the same pattern at line 335.
src/omnibase_core/mixins/mixin_event_bus.py (4)
1009-1016: LGTM: Exception tuple alphabetically ordered.The exception tuple
(RuntimeError, TypeError, ValueError)is now correctly ordered alphabetically per coding guidelines. The error handling preserves the log-and-continue pattern for non-critical publish failures.
1066-1073: LGTM: Consistent alphabetical ordering in sync publish.Same alphabetical fix applied consistently to the synchronous publish method, maintaining pattern coherence across the event publishing methods.
1132-1139: LGTM: Alphabetical ordering in async publish.Consistent application of alphabetical ordering across all three publish methods (
publish_event,publish_completion_event,apublish_completion_event).
1916-1926: LGTM: Alphabetical ordering and improved error context.Good changes:
- Exception tuple
(AttributeError, IndexError, TypeError)now alphabetically ordered- Error code changed from
VALIDATION_FAILEDtoTYPE_INTROSPECTION_ERRORwhich is semantically more precise for type parameter extraction failures- Enhanced context includes
node_name,class_name, anderror_typefor better debuggingsrc/omnibase_core/mixins/mixin_node_service.py (11)
35-42: LGTM: Imports for centralized error handling.Correctly imports
PYDANTIC_MODEL_ERRORSfrom the centralized exception groups module andAnyfor duck-typed return values. This aligns with the coding guideline to use centralized exception constants.
188-196: LGTM: Proper CancelledError propagation in start_service_mode.Critical fix:
asyncio.CancelledErroris now re-raised immediately to honor task cancellation semantics. This prevents the service from masking cancellation requests and ensures proper async shutdown behavior. The comment correctly notes that cleanup should be handled bystop_service_mode().
245-253: LGTM: CancelledError handling in stop_service_mode.Correctly re-raises
CancelledErrorwhile ensuring_service_runningis set toFalsebefore propagation. This maintains proper state even during cancellation, preventing subsequent operations from incorrectly assuming the service is still running.
337-365: LGTM: Centralized exception handling with CancelledError propagation.Good improvements:
- Uses
_TOOL_EXECUTION_ERRORSfor consistent exception handling per coding guidelines- Adds explicit
asyncio.CancelledErrorblock to honor task cancellation- The
boundary-okmarker documents the design intent that tool invocation boundaries return error responses rather than crash- The
finallyblock still executes to clean up_active_invocations
439-470: LGTM: Consolidated event bus lookup helper.Good refactoring that consolidates duplicated event bus lookup logic. The method:
- Tries multiple strategies (mixin method, direct attribute, container service)
- Uses
_SERVICE_LOOKUP_ERRORSfor consistent error handling- Returns
Anyto support duck-typed access (documented)- Has a clear fallback-ok marker on the exception handler
472-488: LGTM: Cleaner subscription method with enhanced error context.The refactored method correctly uses the new helper and raises
ModelOnexErrorwith enriched context (node_type,subscription_topic) for better debugging when the event bus is unavailable.
511-517: LGTM: Standardized comment markers for boundary error handling.The
boundary-okmarkers on the exception handlers in_service_event_loopand_health_monitor_loopproperly document the intent that these error handlers exist to log and propagate/exit gracefully rather than crash the service. The specific exception types are documented in the comments.Also applies to: 552-558
647-656: LGTM: Enhanced error context in _serialize_result.Adding
node_typeandnode_nameto the validation error context improves debuggability when tool execution returnsNone. This aligns with the PR's goal of enrichingModelOnexErrorwith contextual metadata.
674-680: LGTM: Consolidated event bus lookup in emit methods.Both
_emit_tool_responseand_emit_shutdown_eventnow use the new_try_get_event_bus_from_containerhelper, reducing code duplication. Thecleanup-resilience-okmarker on the shutdown event emission correctly documents that emission failures should not crash shutdown.Also applies to: 690-700
722-730: LGTM: Cleanup resilience markers with appropriate documentation.The
cleanup-resilience-okandfallback-okmarkers document why these broad exception handlers exist:
- Line 726-728: Task cancellation race conditions during cleanup
- Line 744: Cleanup must complete even on unexpected errors
The comments provide sufficient context for future maintainers.
Also applies to: 744-746
796-801: LGTM: Fallback markers for optional functionality.The
fallback-okmarkers correctly document that:
- Signal handler registration may fail in non-main threads (Lines 797-800)
- Introspection publishing is optional functionality (Lines 827-829)
These are appropriate uses of the exception marker pattern per coding guidelines.
Also applies to: 826-830
CI Fixes: - Fix handler contract YAML version format (dict → string SemVer) - compute_handler.yaml: "1.0.0" - effect_handler.yaml: "2.0.0" - reducer_handler.yaml: "1.2.0" - Exclude handler contracts from string-version hook (ModelHandlerContract uses str) Code Quality Fixes: - model_field_accessor.py: Replace bare Exception with PYDANTIC_MODEL_ERRORS - model_metrics_context.py: Add error-ok markers for Pydantic validators - util_forward_reference_resolver.py: Fix docstrings to match VALIDATION_ERRORS All tests pass (97/97), mypy and ruff checks pass.
Resolve conflict in mixin_effect_execution.py: - Keep cast import from typing (used in file) - Remove unused Callable import (ruff auto-fix)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_core/utils/util_forward_reference_resolver.py (1)
678-684: Add exception handling comment markers to broad exception handlers.The code uses broad exception handlers (
PYDANTIC_MODEL_ERRORSandRuntimeError) but is missing required exception handling comment markers per coding guidelines. Add appropriate markers (e.g.,# boundary-ok:or# init-errors-ok:) to document the exception handling strategy, following the pattern used elsewhere in the codebase.Example:
except PYDANTIC_MODEL_ERRORS: # boundary-ok: configuration errors must fail fast during module load raise except RuntimeError: # init-errors-ok: runtime errors during module initialization must fail fast raise
🧹 Nitpick comments (6)
.pre-commit-config.yaml (1)
212-219: LGTM — Consider adding inline rationale comment for the new exclusion.The exclusion for
examples/contracts/handlers/is appropriate since handler contract YAML files legitimately use string versions in their version fields (standard for YAML config/contract formats). This aligns with the PR objective of correcting handler contract YAML version formats.For consistency with the detailed exclusion rationale provided for
PROTOCOL EXCLUSIONat lines 207-211, consider adding a brief inline comment explaining why example handler contracts are excluded.src/omnibase_core/models/context/model_metrics_context.py (4)
167-168: Missing# error-ok:marker for Pydantic validator ValueError.For consistency with other validators in this file and across the codebase, add the
# error-ok:marker to document the intentionalValueErrorusage.Suggested fix
if not isinstance(value, str): + # error-ok: Pydantic validator requires ValueError raise ValueError(f"trace_id must be a string, got {type(value).__name__}")
204-213: Missing# error-ok:markers for Pydantic validator ValueErrors.Both
ValueErrorraises in this validator should have# error-ok:markers for consistency with the documented pattern in_validate_semver.Suggested fix
if not isinstance(value, str): + # error-ok: Pydantic validator requires ValueError raise ValueError( f"{field_name} must be a string, got {type(value).__name__}" ) # Normalize to lowercase for comparison normalized = value.lower() if not _SPAN_ID_PATTERN.match(normalized): + # error-ok: Pydantic validator requires ValueError raise ValueError( f"Invalid {field_name} '{value}': must be 16 lowercase hex characters" )
235-243: Missing# error-ok:markers for Pydantic validator ValueErrors.Both
ValueErrorraises in this validator should have# error-ok:markers for consistency.Suggested fix
if isinstance(value, bool) or not isinstance(value, int | float): + # error-ok: Pydantic validator requires ValueError raise ValueError( f"sampling_rate must be a number, got {type(value).__name__}" ) float_value = float(value) if not 0.0 <= float_value <= 1.0: + # error-ok: Pydantic validator requires ValueError raise ValueError( f"Invalid sampling_rate {value}: must be between 0.0 and 1.0 inclusive" )
268-271: Missing# error-ok:marker for Pydantic validator ValueError.The type check
ValueErrorshould have the marker for consistency.Suggested fix
if not isinstance(value, str): + # error-ok: Pydantic validator requires ValueError raise ValueError( f"service_version must be a string, got {type(value).__name__}" )src/omnibase_core/models/core/model_field_accessor.py (1)
86-88: Consider usingATTRIBUTE_ACCESS_ERRORSfor consistency.These inner exception handlers still use hardcoded
(AttributeError, TypeError)tuples while the outer handler at line 122 uses the centralizedATTRIBUTE_ACCESS_ERRORSgroup. For consistency with the PR's standardization goal, consider using the centralized group here as well.♻️ Suggested refactor
try: setattr(obj, part, {}) next_obj = getattr(obj, part) - except (AttributeError, TypeError): + except ATTRIBUTE_ACCESS_ERRORS: # fallback-ok: return False if initializing nested dict fails return Falsetry: setattr(obj, final_key, raw_value) return True - except (AttributeError, TypeError): + except ATTRIBUTE_ACCESS_ERRORS: # fallback-ok: try dict-style access if attribute setting fails passBased on learnings, the project prefers centralized exception groups over hardcoded tuples.
Also applies to: 113-115
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.pre-commit-config.yamlexamples/contracts/handlers/compute_handler.yamlexamples/contracts/handlers/effect_handler.yamlexamples/contracts/handlers/reducer_handler.yamlsrc/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/contracts/handlers/effect_handler.yaml
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution; never use pip or python directly
Use protocol names (not concrete class names) for dependency injection via container.get_service()
Always use ModelOnexError with specific error codes instead of generic Exception
Use @standard_error_handling decorator to eliminate boilerplate try/catch blocks
Use PEP 604 union syntax (str | None) instead of Optional[str] or Union types
All TODO comments must reference a Linear ticket number in format TODO(OMN-XXXX); use TODO(OMN-TBD) temporarily when ticket is not yet created
Place [NEEDS TICKET] marker on the first line of multi-line TODO(OMN-TBD) comments for grep-ability
All type: ignore comments must include specific error codes and explanatory NOTE comments
Order exception tuples alphabetically by exception class name
Use centralized exception groups from omnibase_core.errors.exception_groups instead of hardcoding exception tuples
Do not write tautological docstrings that simply restate the function name; only write docstrings that explain non-obvious behavior, WHY, edge cases, or important side effects
Do NOT share node instances across threads; use thread-local instances or separate instances per thread
Load contracts using FileRegistry from omnibase_core.runtime.runtime_file_registry for YAML contract loading
Format and sort imports using black and isort; enforce with poetry run commands
Run ruff linter to check code quality; use ruff check --fix for automatic fixes
Use exception handling comment markers (fallback-ok, catch-all-ok, cleanup-resilience-ok, boundary-ok, init-errors-ok, tool-resilience-ok) to document broad exception handlers
Files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
src/omnibase_core/models/**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/models/**/model_*.py: Every Pydantic model MUST have an explicit model_config with ConfigDict; empty ConfigDict() is forbidden
Frozen Pydantic models MUST include from_attributes=True for pytest-xdist compatibility
Use Field(default_factory=list) or Field(default_factory=dict) for mutable default values in Pydantic models
Files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use absolute imports across packages; restrict relative imports to tight module clusters within the same subpackage
Follow directory-specific file naming prefixes enforced by checker_naming_convention.py (e.g., decorator_* in decorators/, enum_* in enums/, model_* in models/)
Enforce strict mypy type checking with 0 errors across all source files
Both mypy and pyright must pass type checking in CI
Files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.698Z
Learning: Applies to **/*.py : Use exception handling comment markers (fallback-ok, catch-all-ok, cleanup-resilience-ok, boundary-ok, init-errors-ok, tool-resilience-ok) to document broad exception handlers
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups instead of hardcoding exception tuples
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Always use ModelOnexError with specific error codes instead of generic Exception
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate boilerplate try/catch blocks
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Always use ModelOnexError with specific error codes instead of generic Exception
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T22:34:48.698Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.698Z
Learning: Applies to **/*.py : Use exception handling comment markers (fallback-ok, catch-all-ok, cleanup-resilience-ok, boundary-ok, init-errors-ok, tool-resilience-ok) to document broad exception handlers
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to src/omnibase_core/models/**/model_*.py : Every Pydantic model MUST have an explicit model_config with ConfigDict; empty ConfigDict() is forbidden
Applied to files:
src/omnibase_core/models/context/model_metrics_context.pysrc/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups instead of hardcoding exception tuples
Applied to files:
src/omnibase_core/models/core/model_field_accessor.pysrc/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : All type: ignore comments must include specific error codes and explanatory NOTE comments
Applied to files:
src/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Place [NEEDS TICKET] marker on the first line of multi-line TODO(OMN-TBD) comments for grep-ability
Applied to files:
src/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to src/omnibase_core/models/**/model_*.py : Frozen Pydantic models MUST include from_attributes=True for pytest-xdist compatibility
Applied to files:
src/omnibase_core/models/core/model_field_accessor.py
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to src/omnibase_core/**/*.py : Use absolute imports across packages; restrict relative imports to tight module clusters within the same subpackage
Applied to files:
src/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T22:34:48.698Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.698Z
Learning: Restrict use of `from __future__ import annotations` to cases with forward references; do not use for Pydantic models
Applied to files:
src/omnibase_core/utils/util_forward_reference_resolver.py
📚 Learning: 2026-01-13T22:34:48.698Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.698Z
Learning: Applies to src/omnibase_core/**/*.py : Follow directory-specific file naming prefixes enforced by checker_naming_convention.py (e.g., decorator_* in decorators/, enum_* in enums/, model_* in models/)
Applied to files:
src/omnibase_core/utils/util_forward_reference_resolver.py
🔇 Additional comments (16)
src/omnibase_core/models/context/model_metrics_context.py (3)
22-22: LGTM!The
ValidationInfoimport is correctly added to support the unified span validator that handles multiple fields dynamically.
46-69: LGTM!The helper function properly documents its Pydantic validator context in the docstring and correctly uses
# error-ok:markers for the intentionalValueErrorraises, aligning with the repository's error handling guidelines.
117-146: LGTM!The model configuration correctly includes
frozen=Truewithfrom_attributes=Truefor pytest-xdist compatibility, uses explicit non-emptyConfigDict, and all field declarations use PEP 604 union syntax. As per coding guidelines, this is compliant.src/omnibase_core/models/core/model_field_accessor.py (7)
12-15: LGTM!Good adoption of centralized exception groups per project standards.
61-65: LGTM!Correct use of
ATTRIBUTE_ACCESS_ERRORSwith appropriatefallback-okmarker. The fallback logic gracefully handles access failures.
122-124: LGTM!Correct use of centralized exception group with proper documentation marker.
142-144: LGTM!Appropriate fallback behavior for existence checks.
179-181: LGTM!Consistent exception handling pattern with other field operations.
198-201: LGTM!Good choice of
PYDANTIC_MODEL_ERRORShere sincesetattron Pydantic models can raise validation-related exceptions. The comment provides helpful context.
207-216: LGTM!Clean base implementation with clear docstring guidance for subclasses. The removal of exception handling is appropriate since the base implementation has no logic that can fail.
examples/contracts/handlers/compute_handler.yaml (1)
1-1: LGTM!Adding the YAML document start marker (
---) and quoting the version string ensures consistent parsing across YAML parsers. Unquoted versions like1.0.0are generally safe, but quoting them explicitly guarantees string interpretation and aligns with SemVer conventions.Also applies to: 13-13
examples/contracts/handlers/reducer_handler.yaml (2)
1-1: LGTM!Document start marker and quoted version string are consistent with the other handler contract files.
Also applies to: 13-13
84-84: Thefsm_versionchange is valid for this example file — no code in the repository consumes it.The
metadatafield inModelHandlerContractis a flexible extensibility container (dict[str, Any]) with no schema constraints, allowing any structure. Since no code in the repository references or parsesfsm_version, the structural change from a scalar string to a map does not break internal consumers. The example file loads and deserializes without errors.If external systems depend on this field format, consider documenting the schema change. Otherwise, this change is safe.
src/omnibase_core/utils/util_forward_reference_resolver.py (3)
145-148: LGTM! Centralized exception groups imported correctly.This aligns with the coding guidelines to use centralized exception groups from
omnibase_core.errors.exception_groupsinstead of hardcoding exception tuples. Based on learnings, this is the preferred pattern.
327-336: LGTM! Exception handling properly expanded to include ValidationError.The switch to
VALIDATION_ERRORScorrectly broadens the catch to includeValidationErrorfrom Pydantic, which can be raised duringmodel_rebuild(). The exception ordering is correct—the more specificPydanticSchemaGenerationErrorandPydanticUserErrorare caught first, and the fallback assignments (lines 290-291) remain effective.
486-494: LGTM! Consistent exception handling in subclass forward ref handler.The change to
VALIDATION_ERRORSis consistent with the pattern inrebuild_model_references. The function correctly maintains its deferred error semantics—logging a warning and emitting aUserWarningrather than raising.
Code Review: Error Handling Standardization (OMN-1299)SummaryThis is an excellent, comprehensive PR that significantly improves error handling consistency across omnibase_core. The work is thorough, well-documented, and follows ONEX conventions meticulously. Strengths1. Comprehensive Documentation ✅
2. Decorator Implementation ✅
3. Centralized Exception Groups ✅
4. Robust Testing ✅
5. Type Safety ✅
Code Quality ObservationsDecorator Design (decorator_error_handling.py:25-150)The multi-tier validation strategy in
Suggestion: Consider adding a docstring example showing when each tier triggers. Service Mixin (mixin_node_service.py:62-90)The module-level exception tuples with detailed comments are exemplary: # Service lookup errors - explicit tuple concatenation for mypy
_SERVICE_LOOKUP_ERRORS: tuple[type[Exception], ...] = PYDANTIC_MODEL_ERRORS + (
RuntimeError, # init-errors-ok: service not available
)This pattern should be documented as a best practice for other contributors. YAML Contract Changes (examples/contracts/handlers/*.yaml)Converting version strings to proper format is good, though minor: --- # Document start marker (YAML best practice)
version: "1.0.0" # String format per ModelHandlerContractPotential Concerns1. Exception Group UnpackingIn some files (e.g., except (*PYDANTIC_MODEL_ERRORS, ModelOnexError) as e:Question: Does mypy handle the splat unpacking correctly in all contexts? The module-level constant approach in 2. ValueError in Pydantic Validators92 files use
3. CancelledError Propagation
except Exception:
# fallback-ok: error_count() call failed, not Pydantic-like
return FalseConcern: This catches 4. Test Coverage for Edge CasesThe
Performance ConsiderationsDecorator OverheadThe decorators add minimal overhead (~2 function calls per invocation), but
Recommendation: Profile decorator overhead on hot paths (e.g., event bus, pipeline execution). Consider caching exception type → validation status mappings. Security ConsiderationsException Information LeakageThe decorators preserve full exception context via raise ModelOnexError(..., context={"original_error": str(e)}) from eGood: This aids debugging Recommendation: Add a sanitization pass for exception messages in production mode. Documentation ImprovementsCLAUDE.md Error Handling SectionThe updated section (lines 381-595) is comprehensive, but could benefit from:
Exception Groups TableThe table in CLAUDE.md is excellent. Consider adding:
Testing Recommendations
Merge Readiness✅ Approved - This PR is ready to merge with minor observations noted above. Checklist:
Post-Merge Follow-ups
Excellent work on this comprehensive error handling standardization! The duck-typing approach for validation errors is particularly clever. 🎉 Reviewed by: Claude Sonnet 4.5 (automated review) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @CLAUDE.md:
- Around line 514-553: The docs show only sync usage for the error-handling
decorators; add parallel async examples for @standard_error_handling,
@validation_error_handling, and @io_error_handling (using async def functions
and awaiting calls) to demonstrate that the decorators support coroutines (as
implemented via asyncio.iscoroutinefunction); keep the same short descriptive
captions (e.g., "Contract processing", "Schema validation", "Config file
reading") and mirror the sync examples but using async/await so readers see both
sync and async usage.
🧹 Nitpick comments (2)
CLAUDE.md (1)
516-516: Consider hyphenating compound modifier.The phrase "error handling boilerplate" could be hyphenated as "error-handling boilerplate" when used as a compound adjective modifying a noun, per standard English style guides.
📝 Suggested style improvement
-Three decorators eliminate error handling boilerplate. All decorators follow the ONEX exception contract: +Three decorators eliminate error-handling boilerplate. All decorators follow the ONEX exception contract:src/omnibase_core/mixins/mixin_effect_execution.py (1)
1553-1564: PYDANTIC_MODEL_ERRORS usage is functionally correct but semantically misleading.The exception group contains (AttributeError, TypeError, ValidationError, ValueError) which overlap with exceptions that jsonpath-ng's
parse()andfind()might raise (ValueError for invalid expressions, AttributeError/TypeError for response structure issues). However, PYDANTIC_MODEL_ERRORS is documented for Pydantic model operations (model_validate, model_dump, field access), not JSONPath parsing.In practice, the broad
Exceptionhandler immediately following it (withcatch-all-okmarker) provides sufficient coverage regardless. The dotpath engine via_extract_field()returns None safely without raising exceptions, so PYDANTIC_MODEL_ERRORS isn't strictly necessary for that path.The code is correct—both handlers wrap in ModelOnexError with consistent error codes—but consider whether a more semantically aligned approach would be clearer: either use VALIDATION_ERRORS (for expression parsing failures) or remove the specific PYDANTIC_MODEL_ERRORS catch since the broad Exception handler covers all cases.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CLAUDE.mdscripts/validation/validate_naming.pysrc/omnibase_core/decorators/decorator_pattern_exclusions.pysrc/omnibase_core/mixins/mixin_effect_execution.py
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/validation/validate_naming.py
- src/omnibase_core/decorators/decorator_pattern_exclusions.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use Poetry for Python package management and task execution; never use pip or python directly
Use protocol names (not concrete class names) for dependency injection via container.get_service()
Always use ModelOnexError with specific error codes instead of generic Exception
Use @standard_error_handling decorator to eliminate boilerplate try/catch blocks
Use PEP 604 union syntax (str | None) instead of Optional[str] or Union types
All TODO comments must reference a Linear ticket number in format TODO(OMN-XXXX); use TODO(OMN-TBD) temporarily when ticket is not yet created
Place [NEEDS TICKET] marker on the first line of multi-line TODO(OMN-TBD) comments for grep-ability
All type: ignore comments must include specific error codes and explanatory NOTE comments
Order exception tuples alphabetically by exception class name
Use centralized exception groups from omnibase_core.errors.exception_groups instead of hardcoding exception tuples
Do not write tautological docstrings that simply restate the function name; only write docstrings that explain non-obvious behavior, WHY, edge cases, or important side effects
Do NOT share node instances across threads; use thread-local instances or separate instances per thread
Load contracts using FileRegistry from omnibase_core.runtime.runtime_file_registry for YAML contract loading
Format and sort imports using black and isort; enforce with poetry run commands
Run ruff linter to check code quality; use ruff check --fix for automatic fixes
Use exception handling comment markers (fallback-ok, catch-all-ok, cleanup-resilience-ok, boundary-ok, init-errors-ok, tool-resilience-ok) to document broad exception handlers
Files:
src/omnibase_core/mixins/mixin_effect_execution.py
src/omnibase_core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/omnibase_core/**/*.py: Use absolute imports across packages; restrict relative imports to tight module clusters within the same subpackage
Follow directory-specific file naming prefixes enforced by checker_naming_convention.py (e.g., decorator_* in decorators/, enum_* in enums/, model_* in models/)
Enforce strict mypy type checking with 0 errors across all source files
Both mypy and pyright must pass type checking in CI
Files:
src/omnibase_core/mixins/mixin_effect_execution.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.698Z
Learning: Applies to **/*.py : Use exception handling comment markers (fallback-ok, catch-all-ok, cleanup-resilience-ok, boundary-ok, init-errors-ok, tool-resilience-ok) to document broad exception handlers
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups instead of hardcoding exception tuples
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Always use ModelOnexError with specific error codes instead of generic Exception
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate boilerplate try/catch blocks
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Always use ModelOnexError with specific error codes instead of generic Exception
Applied to files:
CLAUDE.mdsrc/omnibase_core/mixins/mixin_effect_execution.py
📚 Learning: 2026-01-13T22:34:48.698Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.698Z
Learning: Applies to **/*.py : Use exception handling comment markers (fallback-ok, catch-all-ok, cleanup-resilience-ok, boundary-ok, init-errors-ok, tool-resilience-ok) to document broad exception handlers
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Use standard_error_handling decorator to eliminate boilerplate try/catch blocks
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : All type: ignore comments must include specific error codes and explanatory NOTE comments
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Order exception tuples alphabetically by exception class name
Applied to files:
CLAUDE.md
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to **/*.py : Use centralized exception groups from omnibase_core.errors.exception_groups instead of hardcoding exception tuples
Applied to files:
CLAUDE.mdsrc/omnibase_core/mixins/mixin_effect_execution.py
📚 Learning: 2026-01-13T22:34:48.697Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-13T22:34:48.697Z
Learning: Applies to src/omnibase_core/models/**/model_*.py : Every Pydantic model MUST have an explicit model_config with ConfigDict; empty ConfigDict() is forbidden
Applied to files:
src/omnibase_core/mixins/mixin_effect_execution.py
🧬 Code graph analysis (1)
src/omnibase_core/mixins/mixin_effect_execution.py (2)
src/omnibase_core/models/errors/model_onex_error.py (3)
ModelOnexError(49-354)message(248-250)error_code(253-255)src/omnibase_core/enums/enum_core_error_code.py (1)
EnumCoreErrorCode(11-257)
🪛 LanguageTool
CLAUDE.md
[grammar] ~516-~516: Use a hyphen to join words.
Context: ...rators Three decorators eliminate error handling boilerplate. All decorators fol...
(QB_NEW_EN_HYPHEN)
⏰ 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: Pyright Type Checking
- GitHub Check: claude-review
🔇 Additional comments (8)
CLAUDE.md (6)
465-466: LGTM! Clear section introduction.The introduction effectively summarizes the comprehensive error handling policy, setting clear expectations for readers.
467-513: LGTM! Excellent policy documentation with actionable guidance.The ValueError vs ModelOnexError policy provides clear criteria, practical examples, and a helpful decision matrix. This will significantly reduce ambiguity for developers choosing exception types.
556-595: LGTM! Clear marker documentation with practical examples.The standardized exception handling markers are well-documented with a comprehensive table and three practical examples demonstrating graceful degradation, API boundary handling, and cleanup resilience patterns.
636-647: LGTM! Critical asyncio.CancelledError handling is properly documented.The documentation correctly emphasizes that
asyncio.CancelledErrormust be caught separately and re-raised. The example demonstrates the proper pattern for cleanup before re-raising cancellation signals.
649-677: LGTM! Comprehensive quick reference ties all concepts together.The quick reference effectively demonstrates ModelOnexError usage, decorator application, and exception group handling with a concrete fallback pattern. This provides developers with immediately actionable examples.
623-635: No issues found. The exception groups table in CLAUDE.md (lines 623-635) accurately matches the implementation inexception_groups.py. All 8 exception groups and their listed exceptions are correctly documented.src/omnibase_core/mixins/mixin_effect_execution.py (2)
82-82: LGTM!The addition of
castfor type narrowing andPYDANTIC_MODEL_ERRORSfrom centralized exception groups aligns with the PR objectives and coding guidelines for standardized error handling.Also applies to: 103-103
1382-1389: TheNone-to-{}conversion is intentional and correct.
EffectResultTypeis explicitly designed to never includeNone(as documented: "This does NOT include None - effect results are always defined"). The code at lines 1382–1389 properly implements this contract by convertingNoneto an empty dict. Thecastusage for type narrowing after theisinstancecheck is appropriate. No downstream issues found—callers do not expectNonefromexecute_effectresults.
- Add async error handling examples to CLAUDE.md documentation - Add # error-ok: markers to Pydantic validators in 4 model files - Update util_forward_reference_resolver.py docstrings to match actual exception groups (VALIDATION_ERRORS vs PYDANTIC_MODEL_ERRORS) - Fix docstring exception tuple ordering in decorator_error_handling.py - Remove misplaced pytest markers from inner helper classes in tests
🔍 PR Review: Error Handling Standardization (OMN-1299)Overall Assessment: ✅ Excellent WorkThis is a comprehensive and well-executed PR that significantly improves error handling consistency across the codebase. The changes are well-documented, thoroughly tested, and follow project standards. 🎯 Strengths1. Comprehensive Scope ⭐⭐⭐⭐⭐
2. Excellent Async Support ⭐⭐⭐⭐⭐The error handling decorators now properly support async functions with:
Example from if asyncio.iscoroutinefunction(func):
@functools.wraps(func)
async def async_wrapper(*args: Any, **kwargs: Any) -> Any:
try:
return await func(*args, **kwargs)
except asyncio.CancelledError:
raise # ✅ CRITICAL: Always propagate cancellation3. Robust Validation Error Detection ⭐⭐⭐⭐⭐The
Smart denylist (
4. Centralized Exception Groups ⭐⭐⭐⭐⭐The
5. Documentation Excellence ⭐⭐⭐⭐⭐
6. Test Coverage ⭐⭐⭐⭐⭐
🔧 Technical ImplementationError Decorator DesignAll three decorators (
Context EnrichmentModelOnexError now includes:
Module-Level Exception TuplesIn _SERVICE_LOOKUP_ERRORS: tuple[type[Exception], ...] = PYDANTIC_MODEL_ERRORS + (RuntimeError,)
_TOOL_EXECUTION_ERRORS: tuple[type[Exception], ...] = PYDANTIC_MODEL_ERRORS + (KeyError, ModelOnexError, OSError, RuntimeError, TimeoutError,)🎨 Code QualityStrengths:
Minor Observations:
🧪 TestingCoverage:
Test Quality:
📊 Impact AnalysisPositive Impacts:
Risk Assessment: LOW
🏆 Best Practices FollowedONEX Standards:
Error Handling:
🎓 Learning OpportunitiesThis PR demonstrates excellent software engineering practices:
✅ Recommendation: APPROVERationale:
Post-Merge Suggestions:
🙏 AcknowledgmentsExcellent work on:
Thank you for this significant contribution to codebase quality! 🚀 Review generated by Claude Sonnet 4.5 |
1 participant
Summary
Establishes consistent error handling patterns across omnibase_core using ModelOnexError, documentation markers, and centralized exception groups.
Changes
Exception Documentation Markers
# fallback-ok:,# boundary-ok:,# catch-all-ok:,# cleanup-resilience-ok:,# init-errors-ok:,# tool-resilience-ok:@standard_error_handling Decorator Adoption
exception_groups.py Adoption
VALIDATION_ERRORSPYDANTIC_MODEL_ERRORSATTRIBUTE_ACCESS_ERRORSFILE_IO_ERRORSValueError Documentation
# error-ok: Pydantic validator requires ValueErrorcomments to 26 filesDocumentation
Test plan
Closes OMN-1299
Summary by CodeRabbit
Documentation
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.