Repository navigation
fix(OMN-1867): Address CodeRabbit review feedback for contract persistence - #233
Conversation
…tence Quick fixes from CodeRabbit review of PR #232: - Add limit validation to search_contracts() in ProjectionReaderContract to prevent unbounded database queries (matches other list methods) - Remove stale docstring note in handlers/__init__.py that incorrectly stated handlers were "to be implemented"
📝 WalkthroughWalkthroughThis PR introduces a centralized PostgreSQL error handling mechanism through a new Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Handler (e.g., ContractUpsert)
participant MixinPostgres as MixinPostgresErrorResponse
participant ErrorContext as PostgresErrorContext
participant LogSystem as Logging & Response
Handler->>Handler: Execute operation (SQL, etc.)
Handler->>Handler: Exception occurs
Handler->>ErrorContext: new PostgresErrorContext(exception, operation, ...)
ErrorContext-->>Handler: context initialized with error metadata
Handler->>MixinPostgres: _build_error_response(ctx)
MixinPostgres->>MixinPostgres: Map exception to EnumPostgresErrorCode
MixinPostgres->>MixinPostgres: Sanitize error message
MixinPostgres->>LogSystem: Log with appropriate level & context
MixinPostgres->>MixinPostgres: Calculate duration_ms
MixinPostgres-->>Handler: Return ModelBackendResult (success=False)
Handler-->>Handler: Return result to caller
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
🧪 Generate unit tests (beta)
Comment |
…stence
- Add X-Correlation-ID header to all HTTPException error responses in
contract/topic API endpoints (routes.py)
- Change topics_published/topics_subscribed from list[str] to tuple[str, ...]
in ModelContractView for true immutability with frozen=True
- Create MixinPostgresErrorResponse with PostgresErrorContext dataclass
for shared PostgreSQL exception handling pattern
- Add 19 edge case tests for normalize_topic_for_storage() covering:
- Empty string input
- Only-prefix inputs (dev., {env}., etc.)
- Double-prefix scenarios
- Prefix-in-middle (should not strip)
Refactored all PostgreSQL handlers in NodeContractPersistenceEffect to use the new MixinPostgresErrorResponse mixin, reducing ~473 lines of duplicated exception handling code: - HandlerPostgresContractUpsert: -110 lines - HandlerPostgresTopicUpdate: -108 lines - HandlerPostgresMarkStale: -77 lines - HandlerPostgresHeartbeat: -70 lines - HandlerPostgresDeactivate: -54 lines - HandlerPostgresCleanupTopics: -54 lines Each handler now uses PostgresErrorContext with operation-specific: - operation name for logging - log_context with relevant fields (contract_id, topic_suffix, etc.) - operation_error_code for precise error classification
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/node_contract_persistence_effect/handlers/handler_postgres_deactivate.py`:
- Around line 191-200: Replace the PostgresErrorContext usage in the PostgreSQL
handlers (e.g., deactivate_contract handler in handler_postgres_deactivate.py
and other handlers: topic_update, heartbeat, mark_stale, contract_upsert,
cleanup_topics) with ModelInfraErrorContext.with_correlation(), passing the
original exception, correlation_id, and include transport_type="postgres" plus
operation="deactivate_contract" (or the appropriate operation name for each
handler) and start_time/log_context/operation_error_code as before; then return
self._build_error_response(...) using the new ModelInfraErrorContext result so
all node_* handlers follow the ModelInfraErrorContext.with_correlation() pattern
required by the coding guidelines.
🧹 Nitpick comments (1)
src/omnibase_infra/nodes/node_contract_persistence_effect/handlers/handler_postgres_topic_update.py (1)
286-298: Avoid swallowing task cancellations in async contexts.
CatchingExceptioncan interceptasyncio.CancelledErroron Python versions where it subclassesException, preventing cooperative cancellation. Consider re‑raising it explicitly.♻️ Suggested guard for cancellation
+import asyncio ... - except Exception as e: + except asyncio.CancelledError: + raise + except Exception as e: ctx = PostgresErrorContext(
| except Exception as e: | ||
| ctx = PostgresErrorContext( | ||
| exception=e, | ||
| operation="deactivate_contract", | ||
| correlation_id=correlation_id, | ||
| start_time=start_time, | ||
| log_context={"contract_id": payload.contract_id}, | ||
| operation_error_code=EnumPostgresErrorCode.DEACTIVATE_ERROR, | ||
| ) | ||
| return self._build_error_response(ctx) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check PostgresErrorContext usage in this node's handlers
rg -n "PostgresErrorContext" --type py -C 2 src/omnibase_infra/nodes/node_contract_persistence_effect/handlers/Repository: OmniNode-ai/omnibase_infra
Length of output: 8328
🏁 Script executed:
# Check if ModelInfraErrorContext is used elsewhere in the nodes
rg -n "ModelInfraErrorContext" --type py src/omnibase_infra/nodes/Repository: OmniNode-ai/omnibase_infra
Length of output: 8646
🏁 Script executed:
# Check the definition of PostgresErrorContext to understand its structure
rg -n "class PostgresErrorContext" --type py -A 10Repository: OmniNode-ai/omnibase_infra
Length of output: 1228
🏁 Script executed:
# Check if _build_error_response method wraps or transforms the context
rg -n "_build_error_response" --type py -B 2 -A 8 src/omnibase_infra/nodes/node_contract_persistence_effect/handlers/Repository: OmniNode-ai/omnibase_infra
Length of output: 5017
Use ModelInfraErrorContext.with_correlation() for exception handling instead of PostgresErrorContext.
The coding guidelines mandate using ModelInfraErrorContext.with_correlation() for all exception raises in **/nodes/**/*.py files with transport_type and operation parameters. The current implementation uses PostgresErrorContext, which is a specialized mixin-level context that does not comply with this requirement. This pattern is inconsistent with other nodes in the codebase (e.g., node_registration_orchestrator, node_ledger_write_effect) which correctly use ModelInfraErrorContext.with_correlation().
Additionally, the transport_type parameter required by the guideline is missing from the error context. Replace PostgresErrorContext with ModelInfraErrorContext.with_correlation() and include the appropriate transport_type.
Note: This issue affects all PostgreSQL handlers in this node (at least 6 handlers: deactivate, topic_update, heartbeat, mark_stale, contract_upsert, cleanup_topics).
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_contract_persistence_effect/handlers/handler_postgres_deactivate.py`
around lines 191 - 200, Replace the PostgresErrorContext usage in the PostgreSQL
handlers (e.g., deactivate_contract handler in handler_postgres_deactivate.py
and other handlers: topic_update, heartbeat, mark_stale, contract_upsert,
cleanup_topics) with ModelInfraErrorContext.with_correlation(), passing the
original exception, correlation_id, and include transport_type="postgres" plus
operation="deactivate_contract" (or the appropriate operation name for each
handler) and start_time/log_context/operation_error_code as before; then return
self._build_error_response(...) using the new ModelInfraErrorContext result so
all node_* handlers follow the ModelInfraErrorContext.with_correlation() pattern
required by the coding guidelines.
- Add 46 integration tests covering all exception type mappings - Test error sanitization, duration calculation, correlation ID propagation - Test logging levels (WARNING for retriable, ERROR for non-retriable) - Test operation-specific error codes for all 6 handler operations - Add debug logging when search_contracts() limit is corrected
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/integration/mixins/test_mixin_postgres_error_handler.py`:
- Around line 33-34: This module under tests/integration lacks an integration
marker; add a module-level pytest marker by defining pytestmark =
pytest.mark.integration (after the existing import pytest) so all tests in this
file are marked as integration tests; ensure you use the pytestmark symbol
rather than decorating individual functions.
- Around line 58-82: The three test helper factories (make_infra_timeout_error,
make_infra_auth_error, make_infra_connection_error) build error contexts without
a correlation_id; update each to ensure the context includes a correlation_id by
generating one when absent (use uuid.uuid4() -> string) and attach it to the
ModelTimeoutErrorContext / ModelInfraErrorContext before constructing the
Infra*Error so tests propagate a correlation_id for observability.
| import pytest | ||
|
|
There was a problem hiding this comment.
Add an integration pytest marker for this module.
This file lives under tests/integration/ but has no integration marker, which prevents reliable test selection. A module-level marker keeps the intent explicit.
✅ Suggested change
import pytest
+pytestmark = pytest.mark.integrationAs per coding guidelines: Use pytest markers: @pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow, @pytest.mark.chaos, @pytest.mark.serial for test classification.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import pytest | |
| import pytest | |
| pytestmark = pytest.mark.integration |
🤖 Prompt for AI Agents
In `@tests/integration/mixins/test_mixin_postgres_error_handler.py` around lines
33 - 34, This module under tests/integration lacks an integration marker; add a
module-level pytest marker by defining pytestmark = pytest.mark.integration
(after the existing import pytest) so all tests in this file are marked as
integration tests; ensure you use the pytestmark symbol rather than decorating
individual functions.
| def make_infra_timeout_error(message: str) -> InfraTimeoutError: | ||
| """Create an InfraTimeoutError with required context.""" | ||
| ctx = ModelTimeoutErrorContext( | ||
| transport_type=EnumInfraTransportType.DATABASE, | ||
| operation="test_operation", | ||
| ) | ||
| return InfraTimeoutError(message, context=ctx) | ||
|
|
||
|
|
||
| def make_infra_auth_error(message: str) -> InfraAuthenticationError: | ||
| """Create an InfraAuthenticationError with optional context.""" | ||
| ctx = ModelInfraErrorContext( | ||
| transport_type=EnumInfraTransportType.DATABASE, | ||
| operation="test_operation", | ||
| ) | ||
| return InfraAuthenticationError(message, context=ctx) | ||
|
|
||
|
|
||
| def make_infra_connection_error(message: str) -> InfraConnectionError: | ||
| """Create an InfraConnectionError with optional context.""" | ||
| ctx = ModelInfraErrorContext( | ||
| transport_type=EnumInfraTransportType.DATABASE, | ||
| operation="test_operation", | ||
| ) | ||
| return InfraConnectionError(message, context=ctx) |
There was a problem hiding this comment.
Include correlation_id in Infra error contexts created by helpers.
The helper factories build infra error contexts without a correlation_id, which violates the correlation propagation requirement and weakens observability in tests.
🛠️ Suggested change
-def make_infra_timeout_error(message: str) -> InfraTimeoutError:
+def make_infra_timeout_error(
+ message: str,
+ correlation_id: UUID | None = None,
+) -> InfraTimeoutError:
"""Create an InfraTimeoutError with required context."""
ctx = ModelTimeoutErrorContext(
transport_type=EnumInfraTransportType.DATABASE,
operation="test_operation",
+ correlation_id=correlation_id or uuid4(),
)
return InfraTimeoutError(message, context=ctx)
-def make_infra_auth_error(message: str) -> InfraAuthenticationError:
+def make_infra_auth_error(
+ message: str,
+ correlation_id: UUID | None = None,
+) -> InfraAuthenticationError:
"""Create an InfraAuthenticationError with optional context."""
- ctx = ModelInfraErrorContext(
+ ctx = ModelInfraErrorContext.with_correlation(
transport_type=EnumInfraTransportType.DATABASE,
operation="test_operation",
+ correlation_id=correlation_id,
)
return InfraAuthenticationError(message, context=ctx)
-def make_infra_connection_error(message: str) -> InfraConnectionError:
+def make_infra_connection_error(
+ message: str,
+ correlation_id: UUID | None = None,
+) -> InfraConnectionError:
"""Create an InfraConnectionError with optional context."""
- ctx = ModelInfraErrorContext(
+ ctx = ModelInfraErrorContext.with_correlation(
transport_type=EnumInfraTransportType.DATABASE,
operation="test_operation",
+ correlation_id=correlation_id,
)
return InfraConnectionError(message, context=ctx)As per coding guidelines: Propagate correlation_id from incoming requests; auto-generate with uuid4() if missing; include in all error context.
🤖 Prompt for AI Agents
In `@tests/integration/mixins/test_mixin_postgres_error_handler.py` around lines
58 - 82, The three test helper factories (make_infra_timeout_error,
make_infra_auth_error, make_infra_connection_error) build error contexts without
a correlation_id; update each to ensure the context includes a correlation_id by
generating one when absent (use uuid.uuid4() -> string) and attach it to the
ModelTimeoutErrorContext / ModelInfraErrorContext before constructing the
Infra*Error so tests propagate a correlation_id for observability.
jonahgabriel
left a comment
jonahgabriel
left a comment
There was a problem hiding this comment.
Code Review
Issue Found
File: src/omnibase_infra/mixins/mixin_postgres_error_handler.py:106
CLAUDE.md naming violation: File is mixin_postgres_error_handler.py but class is MixinPostgresErrorResponse. Per CLAUDE.md, mixin naming requires exact correspondence: mixin_<name>.py → Mixin<Name>.
All 27 other mixins in the codebase follow this pattern exactly.
Fix: Rename file to mixin_postgres_error_response.py (preferred, since the mixin builds error responses via ModelBackendResult).
Fix: Rename src/omnibase_infra/mixins/mixin_postgres_error_handler.py to mixin_postgres_error_response.py to match class name MixinPostgresErrorResponse per CLAUDE.md naming convention
Checked for bugs and CLAUDE.md compliance. No other issues found.
Rename mixin_postgres_error_handler.py to mixin_postgres_error_response.py to comply with CLAUDE.md naming convention requiring exact correspondence between file name and class name (mixin_<name>.py → Mixin<Name>).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/mixins/mixin_postgres_error_response.py`:
- Around line 186-203: The code uses ctx.correlation_id directly when building
base_context which can be None; before computing duration_ms and constructing
base_context (in the error handling block where correlation_id =
ctx.correlation_id and duration_ms is computed) add a fallback to generate a new
UUID (uuid.uuid4()) when ctx.correlation_id is falsy and assign it back to
correlation_id so all logs/results use a non-null string; ensure you import uuid
at top of the module and convert the generated UUID to a string when placing it
into base_context (references: ctx.correlation_id, correlation_id, duration_ms,
base_context, ModelBackendResult/BackendResult).
| exception = ctx.exception | ||
| operation = ctx.operation | ||
| correlation_id = ctx.correlation_id | ||
| start_time = ctx.start_time | ||
| log_context = ctx.log_context | ||
| operation_error_code = ctx.operation_error_code | ||
| # Local import to avoid circular import at module load time | ||
| # (mixins/__init__.py loads before nodes/__init__.py in some paths) | ||
| from omnibase_infra.nodes.effects.models.model_backend_result import ( | ||
| ModelBackendResult as BackendResult, | ||
| ) | ||
|
|
||
| duration_ms = (time.perf_counter() - start_time) * 1000 | ||
|
|
||
| # Build base log context | ||
| base_context: dict[str, object] = { | ||
| "correlation_id": str(correlation_id), | ||
| "duration_ms": duration_ms, |
There was a problem hiding this comment.
Auto-generate correlation_id when missing to keep error context traceable.
If ctx.correlation_id is None, logs and results will carry "None", breaking traceability. Add a uuid4() fallback before building the base context.
🛠️ Suggested fix
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING
+from uuid import uuid4
@@
- correlation_id = ctx.correlation_id
+ correlation_id = ctx.correlation_id or uuid4()As per coding guidelines: Propagate correlation_id from incoming requests; auto-generate with uuid4() if missing; include in all error context.
🤖 Prompt for AI Agents
In `@src/omnibase_infra/mixins/mixin_postgres_error_response.py` around lines 186
- 203, The code uses ctx.correlation_id directly when building base_context
which can be None; before computing duration_ms and constructing base_context
(in the error handling block where correlation_id = ctx.correlation_id and
duration_ms is computed) add a fallback to generate a new UUID (uuid.uuid4())
when ctx.correlation_id is falsy and assign it back to correlation_id so all
logs/results use a non-null string; ensure you import uuid at top of the module
and convert the generated UUID to a string when placing it into base_context
(references: ctx.correlation_id, correlation_id, duration_ms, base_context,
ModelBackendResult/BackendResult).
1 participant
Summary
Quick fixes from CodeRabbit review of PR #232 (NodeContractPersistenceEffect).
Changes
search_contracts()inProjectionReaderContractto prevent unbounded database queries (matches other list methods withlimit <= 1000bounds)handlers/__init__.pythat incorrectly stated handlers were "to be implemented" when they are fully implementedContext
These are the "fix now" items from the CodeRabbit review analysis. The remaining items (X-Correlation-ID in error responses, tuple types in frozen models) are deferred to the main follow-up ticket OMN-1867.
Test plan
test_projection_reader_contract.py)Summary by CodeRabbit
Bug Fixes
Improvements