Repository navigation
feat(handlers): adopt ModelHandlerOutput in infra handlers [OMN-975] - #62
Conversation
Update all omnibase_infra handlers to return ModelHandlerOutput[T] instead of raw dictionaries, aligning with the unified handler output model from omnibase_core.models.dispatch. Changes: - handler_consul.py: All operations return ModelHandlerOutput.for_compute() - handler_vault.py: All operations return ModelHandlerOutput.for_compute() - handler_http.py: All operations return ModelHandlerOutput.for_compute() - handler_db.py: All operations return ModelHandlerOutput.for_compute() Each handler now: - Imports ModelHandlerOutput from omnibase_core.models.dispatch - Extracts input_envelope_id and correlation_id as UUIDs - Wraps response data in ModelHandlerOutput.for_compute() - Uses consistent handler_id for traceability This enables node-kind constraint enforcement at runtime and ensures compatibility with the ONEX dispatch engine's causality-correct publishing.
|
Warning Rate limit exceeded@jonahgabriel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughAll four handlers (Consul, DB, HTTP, Vault) were changed to extract and propagate an input_envelope_id and to return ModelHandlerOutput[...] wrappers (via ModelHandlerOutput.for_compute) that include input_envelope_id, correlation_id, handler_id, and the operation result. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Comment |
|
PR Review: feat(handlers): adopt ModelHandlerOutput in infra handlers [OMN-975] Summary: This PR successfully migrates all infrastructure handlers to use ModelHandlerOutput[T] instead of raw dictionaries, aligning with the unified handler output model from omnibase_core.models.dispatch (PR #223, OMN-941). CRITICAL ISSUES:
MODERATE ISSUES:
MINOR SUGGESTIONS:
SECURITY: No concerns identified RECOMMENDATION: APPROVE WITH CHANGES REQUIRED Critical issues must be addressed before merge:
Reviewer: Claude Sonnet 4.5 (via Claude Code) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/omnibase_infra/handlers/handler_vault.py (1)
846-857: Consider defining handler_id as a constant for consistency with ConsulHandler.The Vault handler uses inline string
"vault-handler"whileConsulHandlerdefinesHANDLER_ID_CONSUL = "consul-handler"as a constant. For consistency and maintainability, consider extracting this to a constant.🔎 Suggested refactor
At the top of the file (after line 66):
HANDLER_ID_VAULT: str = "vault-handler"Then replace all occurrences of
handler_id="vault-handler"withhandler_id=HANDLER_ID_VAULT.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (4)
src/omnibase_infra/handlers/handler_consul.py(16 hunks)src/omnibase_infra/handlers/handler_db.py(9 hunks)src/omnibase_infra/handlers/handler_http.py(11 hunks)src/omnibase_infra/handlers/handler_vault.py(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER useAnytype - always use specific types and Pydantic models
UseX | None(PEP 604 union syntax) instead ofOptional[X]for nullable types in Python
RaiseOnexErrorinstead of other error types - always useraise OnexError(...) from epattern
NEVER include passwords, API keys, tokens, secrets, full connection strings, PII, internal IPs, private keys, or session tokens in error messages or context
Always propagatecorrelation_idfrom incoming requests to error context, or auto-generate usinguuid4()if not present
Protocol resolution should use duck typing through protocols, never useisinstancechecks
Files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_vault.pysrc/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_http.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use handler envelopes from `omnibase_infra` for all I/O operations (HTTP, database, Kafka) instead of custom clients
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use handler envelopes from `omnibase_infra` for all I/O operations (HTTP, database, Kafka) instead of custom clients
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_vault.pysrc/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-19T19:03:52.430Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T19:03:52.430Z
Learning: Applies to **/*.py : Always propagate `correlation_id` from incoming requests to error context, or auto-generate using `uuid4()` if not present
Applied to files:
src/omnibase_infra/handlers/handler_vault.pysrc/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to agents/**/*.py : Use correlation_id UUID for end-to-end traceability across all agent routing, manifest injection, and execution events
Applied to files:
src/omnibase_infra/handlers/handler_vault.pysrc/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use `HttpRestAdapter` envelope pattern for HTTP calls instead of direct httpx.AsyncClient
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-28T18:58:53.781Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use `EnumCoreErrorCode` with `ModelOnexError` for proper error code usage
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-20T04:09:41.822Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T04:09:41.822Z
Learning: Applies to **/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling instead of generic Exception
Applied to files:
src/omnibase_infra/handlers/handler_http.py
🧬 Code graph analysis (3)
src/omnibase_infra/handlers/handler_db.py (3)
src/omnibase_infra/handlers/handler_http.py (3)
execute(188-283)_extract_correlation_id(285-295)_extract_envelope_id(297-307)src/omnibase_infra/handlers/models/model_db_query_response.py (1)
ModelDbQueryResponse(19-56)src/omnibase_infra/handlers/models/model_db_query_payload.py (1)
ModelDbQueryPayload(22-52)
src/omnibase_infra/handlers/handler_consul.py (3)
src/omnibase_infra/handlers/handler_db.py (3)
execute(179-272)_extract_envelope_id(286-296)health_check(510-531)src/omnibase_infra/handlers/handler_http.py (3)
execute(188-283)_extract_envelope_id(297-307)health_check(724-733)src/omnibase_infra/handlers/handler_vault.py (4)
execute(401-496)_extract_envelope_id(510-520)_health_check_operation(1316-1340)health_check(1232-1314)
src/omnibase_infra/handlers/handler_http.py (3)
src/omnibase_infra/handlers/handler_consul.py (2)
_extract_correlation_id(499-509)_extract_envelope_id(511-521)src/omnibase_infra/handlers/handler_db.py (2)
_extract_correlation_id(274-284)_extract_envelope_id(286-296)src/omnibase_infra/handlers/handler_vault.py (2)
_extract_correlation_id(498-508)_extract_envelope_id(510-520)
🔇 Additional comments (34)
src/omnibase_infra/handlers/handler_http.py (6)
17-17: LGTM - Import added for ModelHandlerOutput.The import aligns with the PR objective to standardize handler outputs using
ModelHandlerOutputfromomnibase_core.models.dispatch.
188-208: LGTM - execute() signature and documentation updated correctly.The method now returns
ModelHandlerOutput[dict[str, object]]and the docstring accurately describes the new return structure includinginput_envelope_id,correlation_id, andhandler_id.
297-307: LGTM - _extract_envelope_id implementation is consistent with other handlers.The implementation correctly handles UUID objects, string UUIDs, invalid strings (fallback to uuid4()), and missing values. This matches the pattern used in handler_consul.py, handler_db.py, and handler_vault.py.
271-283: LGTM - input_envelope_id correctly propagated to _execute_request.Both GET and POST paths now pass
input_envelope_idto the downstream request execution, enabling causality tracking through the entire request lifecycle.
534-561: LGTM - _execute_request signature expanded for envelope tracking.The method now accepts
input_envelope_id: UUIDparameter and returnsModelHandlerOutput[dict[str, object]]. The docstring is updated to reflect these changes.
709-722: Remove the suggestion to consider removing nested correlation_id.The nested
correlation_idfield in the result dict is explicitly documented in the execute() method's docstring as part of the API contract ("result: dict with status, payload (status_code, headers, body), and correlation_id"). It is actively tested in 6 test assertions and is unique to this handler among all infra handlers. There is no evidence it exists for backward compatibility, and removing it would break the contract.Likely an incorrect or invalid review comment.
src/omnibase_infra/handlers/handler_db.py (6)
18-18: LGTM - Import added for ModelHandlerOutput.Correctly imports the new output wrapper type.
179-204: LGTM - execute() signature and envelope_id extraction added.The method correctly returns
ModelHandlerOutput[ModelDbQueryResponse](using the typed Pydantic model rather than raw dict), and properly extracts bothcorrelation_idandinput_envelope_idfrom the envelope.
286-296: LGTM - _extract_envelope_id implementation consistent with other handlers.Identical implementation to handler_http.py, ensuring consistent behavior across all handlers.
347-381: LGTM - _execute_query updated with envelope tracking.The method signature correctly includes
input_envelope_id: UUIDand passes it through to_build_response().
403-436: LGTM - _execute_statement updated with envelope tracking.Consistent with
_execute_query- properly propagatesinput_envelope_idto the response builder.
490-508: LGTM - _build_response correctly wraps typed result in ModelHandlerOutput.Good implementation that:
- Creates a typed
ModelDbQueryResponsePydantic model- Wraps it in
ModelHandlerOutput.for_compute()with proper handler metadata- Uses
handler_id="db-handler"for consistent traceabilitysrc/omnibase_infra/handlers/handler_consul.py (12)
45-45: LGTM - Import added for ModelHandlerOutput.Import correctly placed with other omnibase_core imports.
56-57: Good practice: Handler ID defined as a constant.Using
HANDLER_ID_CONSUL = "consul-handler"as a constant improves maintainability and prevents typos across multiple usage sites.
412-434: LGTM - execute() signature and envelope_id extraction updated.The method now returns
ModelHandlerOutput[dict[str, object]]and properly extractsinput_envelope_idfor causality tracking.
488-497: LGTM - All operation handlers receive input_envelope_id.Each operation routing path now correctly passes
input_envelope_idto the corresponding handler method.
511-521: LGTM - _extract_envelope_id implementation consistent with other handlers.Identical implementation ensures consistent envelope ID extraction across all infrastructure handlers.
772-787: LGTM - KV get "not found" path returns ModelHandlerOutput.Correctly wraps the "key not found" response in
ModelHandlerOutput.for_compute()with proper metadata.
806-821: LGTM - KV get "recurse" path returns ModelHandlerOutput.Recurse mode results are properly wrapped with handler metadata.
826-843: LGTM - KV get "single key" path returns ModelHandlerOutput.Single key results wrapped consistently.
912-925: LGTM - _kv_put returns ModelHandlerOutput.Put operation result wrapped with proper handler metadata.
1003-1017: LGTM - _register_service returns ModelHandlerOutput.Service registration result wrapped correctly.
1064-1077: LGTM - _deregister_service returns ModelHandlerOutput.Service deregistration result wrapped correctly.
1152-1178: LGTM - _health_check_operation returns ModelHandlerOutput.Health check operation properly wrapped with handler metadata.
src/omnibase_infra/handlers/handler_vault.py (10)
15-20: LGTM - Module docstring updated to document return type convention.Good documentation practice to document that all operations return
ModelHandlerOutput[dict[str, object]]per OMN-975 and explain whyfor_compute()is used.
32-33: LGTM - Import added for ModelHandlerOutput.Import correctly placed before other imports.
401-424: LGTM - execute() signature and docstring updated.The method now returns
ModelHandlerOutput[dict[str, object]]and properly documents the new return structure including envelope_id for causality tracking.
486-496: LGTM - All operation routing paths pass input_envelope_id.Each operation correctly receives
input_envelope_idfor propagation.
510-520: LGTM - _extract_envelope_id implementation consistent with other handlers.The implementation matches other handlers, though the docstring is slightly different ("Extract envelope ID from envelope, or generate one if not present" vs "Extract or generate envelope ID for causality tracking"). This minor variation doesn't affect functionality.
935-946: LGTM - _write_secret returns ModelHandlerOutput.Write operation correctly wrapped with handler metadata.
1002-1010: LGTM - _delete_secret returns ModelHandlerOutput.Delete operation correctly wrapped with handler metadata.
1071-1079: LGTM - _list_secrets returns ModelHandlerOutput.List operation correctly wrapped with handler metadata.
1219-1230: LGTM - _renew_token_operation returns ModelHandlerOutput.Token renewal operation correctly wrapped with handler metadata.
1332-1340: LGTM - _health_check_operation returns ModelHandlerOutput.Health check operation correctly wrapped with handler metadata.
…MN-975] Extract handler_id string literals to module-level constants in HTTP, DB, and Vault handlers to match the pattern established by ConsulHandler. This improves consistency and makes handler IDs easier to maintain.
PR Review: ModelHandlerOutput Adoption in Infrastructure HandlersOverviewThis PR successfully migrates all infrastructure handlers to return ✅ Strengths1. Consistent Implementation Across All HandlersAll four handlers (
2. Proper Type AnnotationsReturn types are correctly updated throughout: # BEFORE
async def execute(self, envelope: dict[str, object]) -> dict[str, object]:
# AFTER
async def execute(self, envelope: dict[str, object]) -> ModelHandlerOutput[dict[str, object]]:Generic type parameter properly reflects the wrapped result type (e.g., 3. Correlation ID ConversionProperly converts correlation IDs to strings in the result dict while keeping UUID format for result = {
"status": "success",
"payload": {...},
"correlation_id": str(correlation_id), # String in result
}
return ModelHandlerOutput.for_compute(
correlation_id=correlation_id, # UUID for wrapper
...
)4. Envelope ID Extraction HelperNew 5. Updated DocstringsComprehensive documentation updates explain the new return type and parameters (
|
| Priority | Issue | Recommendation |
|---|---|---|
| 🔴 CRITICAL | Tests will fail with new return type | Update all handler tests to access response.result[...] |
| 🟡 HIGH | Duplicate correlation_id in nested dict | Remove correlation_id from result dict or add TODO comment |
| 🟡 MEDIUM | Inconsistent result dict declaration in handler_http.py | Extract inline dict to typed variable |
| 🟢 LOW | Missing docstrings for input_envelope_id in private methods |
Add parameter documentation |
| 🟢 LOW | Integration test coverage | Add tests for envelope ID propagation |
✅ Approval Status
Approve with Required Changes
The implementation is solid and follows ONEX patterns correctly. However, the test updates are critical and should be included in this PR to avoid breaking CI/CD.
Next Steps:
- Update all handler tests to access
response.resultinstead of direct field access - Consider removing duplicate
correlation_idfrom nested result dicts - Add docstrings for
input_envelope_idparameters - Verify integration tests pass with updated return types
Great work on the consistent implementation across all handlers! The pattern is clean and maintainable. 🚀
Reviewed by: Claude Sonnet 4.5 (ONEX Infrastructure Code Review Agent)
ONEX Compliance: ✅ Passes (with test update requirement)
Security: ✅ No issues
Performance: ✅ Negligible impact
Address PR #62 review feedback: - VaultAdapter: Align _extract_envelope_id docstring with other handlers - ConsulHandler: Standardize envelope_id documentation in execute() docstring Both now use consistent wording: "for causality tracking"
PR Review: Adopt ModelHandlerOutput in Infrastructure Handlers [OMN-975]SummaryThis PR successfully migrates all infrastructure handlers to return ✅ Strengths1. Consistent Pattern ApplicationAll handlers follow the same refactoring pattern:
2. Comprehensive CoverageAll operations across all handlers updated:
3. Good DocumentationUpdated docstrings clearly describe:
4. Type SafetyStrong typing maintained throughout:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/omnibase_infra/handlers/handler_vault.py (1)
849-860: Minor inconsistency:correlation_idnot included in result dict.The Consul handler includes
"correlation_id": str(correlation_id)in the result dict (e.g., line 780, 814, 836), but Vault handler omits it. Both handlers passcorrelation_idtoModelHandlerOutput.for_compute(), so traceability is maintained at the envelope level.If the result dict's
correlation_idfield is intended for backward compatibility or downstream consumers, consider aligning the handlers. Otherwise, this is acceptable sinceModelHandlerOutputalready carries the correlation ID.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
src/omnibase_infra/handlers/handler_consul.py(16 hunks)src/omnibase_infra/handlers/handler_vault.py(14 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER useAnytype - always use specific types and Pydantic models
UseX | None(PEP 604 union syntax) instead ofOptional[X]for nullable types in Python
RaiseOnexErrorinstead of other error types - always useraise OnexError(...) from epattern
NEVER include passwords, API keys, tokens, secrets, full connection strings, PII, internal IPs, private keys, or session tokens in error messages or context
Always propagatecorrelation_idfrom incoming requests to error context, or auto-generate usinguuid4()if not present
Protocol resolution should use duck typing through protocols, never useisinstancechecks
Files:
src/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_vault.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use handler envelopes from `omnibase_infra` for all I/O operations (HTTP, database, Kafka) instead of custom clients
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use handler envelopes from `omnibase_infra` for all I/O operations (HTTP, database, Kafka) instead of custom clients
Applied to files:
src/omnibase_infra/handlers/handler_consul.py
📚 Learning: 2025-12-19T19:03:52.430Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T19:03:52.430Z
Learning: Applies to **/*.py : Always propagate `correlation_id` from incoming requests to error context, or auto-generate using `uuid4()` if not present
Applied to files:
src/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_vault.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to agents/**/*.py : Use correlation_id UUID for end-to-end traceability across all agent routing, manifest injection, and execution events
Applied to files:
src/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_vault.py
📚 Learning: 2025-09-23T22:28:00.333Z
Learnt from: jonahgabriel
Repo: OmniNode-ai/omnibase_core PR: 32
File: examples/practical_migration_example.py:125-134
Timestamp: 2025-09-23T22:28:00.333Z
Learning: The omnibase_core codebase extensively uses constrained TypeVars for primitive types with the pattern `T = TypeVar("T", str, int, float, bool)` across multiple models including ModelTypedMetrics, ModelGenericMetadata, and ModelExecutionResult. This is the preferred approach over hardcoded union types like `str | int | bool | float | None`.
Applied to files:
src/omnibase_infra/handlers/handler_vault.py
🧬 Code graph analysis (2)
src/omnibase_infra/handlers/handler_consul.py (3)
src/omnibase_infra/handlers/handler_vault.py (3)
execute(404-499)_extract_envelope_id(513-523)health_check(1235-1317)src/omnibase_infra/handlers/handler_db.py (3)
execute(182-275)_extract_envelope_id(289-299)health_check(513-534)src/omnibase_infra/handlers/handler_http.py (3)
execute(191-286)_extract_envelope_id(300-310)health_check(727-736)
src/omnibase_infra/handlers/handler_vault.py (2)
src/omnibase_infra/handlers/handler_consul.py (4)
execute(412-497)_extract_envelope_id(511-521)_health_check_operation(1152-1178)health_check(1079-1150)src/omnibase_infra/handlers/handler_http.py (3)
execute(191-286)_extract_envelope_id(300-310)health_check(727-736)
🔇 Additional comments (12)
src/omnibase_infra/handlers/handler_consul.py (6)
45-58: LGTM! Import and constant additions are consistent with other handlers.The
ModelHandlerOutputimport andHANDLER_ID_CONSULconstant follow the same pattern established inhandler_http.py,handler_db.py, andhandler_vault.py.
511-521: LGTM! Consistent envelope ID extraction pattern.The implementation matches the identical helper in
handler_http.pyandhandler_db.py, ensuring consistent causality tracking across all handlers.
412-497: LGTM! Execute method properly updated for ModelHandlerOutput.The return type, envelope ID extraction, and propagation to all operation handlers are correctly implemented.
719-843: LGTM! KV get operation properly wrapped with ModelHandlerOutput.All three return paths (key not found, recurse mode, single key) correctly use
ModelHandlerOutput.for_compute()with consistent metadata.
845-1077: LGTM! KV put, register, and deregister operations follow consistent pattern.All operations properly wrap results with
ModelHandlerOutput.for_compute()and propagateinput_envelope_idandcorrelation_id.
1152-1178: LGTM! Health check operation follows the standard pattern.The implementation matches the pattern used in
handler_vault.pyand other handlers.src/omnibase_infra/handlers/handler_vault.py (6)
15-20: LGTM! Helpful documentation about return type standardization.The docstring update clearly explains the OMN-975 return type changes and the rationale for using
ModelHandlerOutput.for_compute().
32-58: LGTM! Import and constant additions align with the PR objectives.The
ModelHandlerOutputimport andHANDLER_ID_VAULTconstant follow the established pattern across handlers.
404-499: LGTM! Execute method follows consistent pattern with other handlers.The return type update, envelope ID extraction, and propagation to all operation handlers are correctly implemented.
513-523: LGTM! Envelope ID extraction is consistent across handlers.Identical implementation to
handler_consul.py,handler_http.py, andhandler_db.py.
862-1082: LGTM! Write, delete, and list operations properly wrapped.All secret operations consistently use
ModelHandlerOutput.for_compute()with appropriate metadata.
1202-1343: LGTM! Token renewal and health check operations follow the standard pattern.Both operations properly extract relevant data and wrap results in
ModelHandlerOutput.for_compute().
…-975] - Fix correlation_id type inconsistency in HTTP handler (UUID → string) - Add correlation_id to all Vault handler result dicts for consistency - Update all handler unit tests to work with ModelHandlerOutput return type Changes: - handler_http.py: Convert correlation_id to string in result dict - handler_vault.py: Add correlation_id to 6 result dicts - test_handler_*.py: Update 66+ tests to access result via output.result
Code Review: ModelHandlerOutput Adoption in Infrastructure HandlersThis PR successfully migrates all infrastructure handlers to return ✅ Strengths1. Consistent Implementation PatternAll four handlers (Consul, Vault, HTTP, DB) follow an identical pattern:
This consistency makes the codebase easier to understand and maintain. 2. Proper UUID HandlingThe
3. Comprehensive Test CoverageAll unit tests were updated to work with the new return type:
4. Type Safety ImprovementsThe
5. Documentation Excellence
🔍 Observations & Minor Concerns1. Correlation ID Type Inconsistency ✅ (Already Fixed)The commits show this was identified and corrected:
2. Pattern DuplicationThe
3. Error Handling ConsistencyAll handlers properly maintain error context with 🚀 Architecture AlignmentThis PR aligns perfectly with ONEX principles: ✅ Strong Typing
✅ Contract-Driven Design
✅ Infrastructure Error Patterns
✅ ONEX 4-Node Architecture
🧪 Testing ConsiderationsTest Coverage Analysis
Integration Testing NoteThe PR description mentions:
Recommendation: Verify integration tests are updated/passing before merge, as they may expect raw dict returns. 🔒 Security Review✅ No Security Concerns
📊 Code Quality Metrics
📝 RecommendationsRequired Before Merge
Optional Future Improvements
🎯 Final Verdict✅ APPROVED - Excellent WorkThis PR demonstrates:
The migration to Merge RecommendationReady to merge once integration tests are confirmed passing. 🏆 HighlightsSpecial recognition for:
Great work on this refactoring! 🎉 |
… [OMN-975] - Create MixinEnvelopeExtraction with shared _extract_correlation_id and _extract_envelope_id methods to DRY up duplicated code across handlers - Update all 4 handlers to inherit from the new mixin - Add comprehensive docstrings documenting envelope_id vs correlation_id semantics and causality tracking patterns - Document envelope_id usage in health_check operations
Code Review - PR #62: Adopt ModelHandlerOutput in infra handlersSummaryThis PR successfully migrates all infrastructure handlers to return ✅ Strengths
🔍 Issues & Concerns1. Type Safety Issue:
|
Code Review: Handler Output Standardization (OMN-975)SummaryThis PR successfully adopts ✅ Strengths1. Excellent DRY RefactoringThe introduction of
Location: 2. Consistent Handler ID PatternHandler IDs extracted to module-level constants (commit 7cf88ca): HANDLER_ID_CONSUL = "consul-handler"
HANDLER_ID_DB = "db-handler"
HANDLER_ID_HTTP = "http-handler"
HANDLER_ID_VAULT = "vault-handler"This improves maintainability and follows the pattern established in ConsulHandler. 3. Strong Type SafetyAll handlers now return
4. Comprehensive Test CoverageAll 66+ handler tests updated to work with new return type:
Files: 5. Documentation Excellence
🔍 Observations & Minor Considerations1. Correlation ID String ConversionAll handlers now convert "correlation_id": str(correlation_id) # UUID → stringQuestion: Is this conversion intentional for JSON serialization, or should the result dict preserve UUID types? Impact: Low - string representation is JSON-safe and common practice. If downstream consumers expect UUID objects, this could be a breaking change. 2. Health Check Dual PathConsulHandler has two health check methods:
Location: This dual approach is well-documented and appropriate for different invocation contexts. Consider adopting this pattern in other handlers if they don't already have it. 3. Vault Handler Correlation ID ConsistencyCommit 53b69c8 added 4. Test Pattern ConsistencyAll tests follow the pattern: output = await handler.execute(envelope)
result = output.result
assert result["status"] == "success"This is clean and consistent. Consider adding assertions on 🚀 Performance ConsiderationsMinimal OverheadThe
Envelope Extraction PerformanceThe mixin's UUID validation uses try/except for string parsing: try:
return UUID(raw)
except ValueError:
passThis is fine for the happy path (valid UUID strings), but generates exceptions for invalid inputs. Given that UUIDs should be valid in production, this is acceptable. 🔒 Security Assessment✅ No Security Concerns
The changes are purely structural and don't introduce security risks. 📊 Code Quality Metrics
🎯 ONEX Compliance✅ Follows ONEX Patterns
✅ Naming Conventions
📋 RecommendationsRequired Before MergeNone - the PR is ready to merge as-is. Optional Enhancements (Future Work)
🏆 Final AssessmentLGTM ✅ This PR demonstrates:
Commit QualityThe 5 commits show a thoughtful progression:
Recommendation: Approve and merge. This is production-ready work that improves observability and enables runtime dispatch engine features per OMN-941. 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 (1)
tests/unit/handlers/test_handler_vault_concurrency.py (1)
496-577: State-transition test no longer verifies circuit closes after successful HALF_OPEN requestsIn
test_circuit_breaker_state_transition_race_open_to_half_open,resultsnow containModelHandlerOutput[...]instances (orNone), butsuccessful_resultsis filtered withisinstance(r, dict). That list will always be empty, so the assertion that the circuit closes after a successful request is never executed, reducing coverage for this critical path.Consider treating any non-
Noneresult as a success (or checking for the wrapper type) instead:Proposed fix to make the test actually assert recovery
- successful_results = [ - r for r in results if isinstance(r, dict) and r is not None - ] - if successful_results: - assert handler._circuit_breaker_open is False, ( - "Circuit should be CLOSED after successful test request in HALF_OPEN state" - ) + successful_results = [r for r in results if r is not None] + if successful_results: + assert handler._circuit_breaker_open is False, ( + "Circuit should be CLOSED after successful test request in HALF_OPEN state" + )
🧹 Nitpick comments (9)
src/omnibase_infra/mixins/mixin_envelope_extraction.py (1)
37-71: Envelope extraction logic is correct and matches tracing guidelines
_extract_correlation_id/_extract_envelope_idcorrectly accept UUID instances, parse UUID strings, and fall back touuid4()when missing/invalid, which aligns with the correlation/envelope ID propagation rules.- Mixin naming and single-class-per-file pattern are consistent with the mixin guidelines.
If you want to trim duplication, you could introduce a private helper like
_extract_uuid_field(envelope: dict[str, object], key: str) -> UUIDand call it from both methods; optional only.Also applies to: 73-105
tests/unit/handlers/test_handler_vault.py (1)
196-239: Vault handler tests correctly adapted toModelHandlerOutput
- Using
output = await handler.execute(envelope)followed byresult = output.resultis consistent with the newModelHandlerOutput[...]API.- Asserting
result["correlation_id"] == str(correlation_id)when the envelope contained a UUID matches the mixin behavior of normalizing to UUID then serializing to string in the result.- All the read/write/delete/list/renew/edge-case tests that were updated now validate status and payload fields through
resultas expected.If you want to extend coverage later, you could also assert wrapper-level metadata (e.g.,
output.correlation_id,output.input_envelope_id,output.handler_id) to fully lock in OMN‑975’s contract, but that’s optional.Also applies to: 242-280, 282-347, 376-443, 445-512, 513-562, 564-612, 618-657, 1090-1112, 1114-1172
tests/unit/handlers/test_handler_http.py (1)
151-184: HTTP handler tests align well with theModelHandlerOutputAPI
- Every updated execution path now treats
execute()as returning a wrapper (output) and reads the HTTP semantics fromoutput.result, which matches the handler implementation.- Correlation ID tests correctly cover UUID input, string input, missing, and invalid values, all expecting a string correlation ID in
resultand validating viaUUID(...).- Size limit, Content-Length, streaming, and logging behavior tests still exercise the same semantics but through the wrapped result, preserving coverage.
Optionally, you could add a couple of quick assertions on wrapper metadata (e.g.,
output.correlation_idvsresult["correlation_id"], oroutput.input_envelope_id) to pin down the envelope tracing contract, but the current coverage on behavior is already solid.Also applies to: 196-234, 237-303, 313-363, 366-401, 404-436, 439-493, 495-533, 750-787, 1040-1072, 1078-1107, 1110-1172, 1185-1212, 1215-1240, 1242-1269, 1271-1301, 1317-1353, 1355-1379, 1381-1403, 1406-1430, 1432-1463, 1465-1491, 1493-1527, 1529-1563, 1583-1596, 1640-1672, 1674-1710, 1712-1746, 1749-1790, 1792-1833, 1851-1897, 1952-1995, 2010-2051, 2054-2095, 2097-2167
tests/unit/handlers/test_handler_consul.py (1)
315-351: Consul handler tests correctly reflectModelHandlerOutputusage
- KV, service, and health-check tests now consistently access
output.resultrather than assuming a bare dict, which is aligned with the updated handler contract.- Correlation ID tests confirm that UUID and string inputs are both normalized to string correlation IDs in
result, and that a new UUID is generated when the envelope omits it.As with the other handlers, optionally asserting on the top-level wrapper fields (
output.correlation_id,output.input_envelope_id,output.handler_id) would more fully lock in OMN‑975 semantics, but the current assertions are functionally correct.Also applies to: 353-387, 388-438, 439-471, 558-595, 596-626, 654-683, 715-743, 795-819, 917-995
tests/unit/handlers/test_handler_vault_concurrency.py (1)
28-30: AlignHandlerResponsealias and annotations withModelHandlerOutputHelper functions in this file are annotated with
HandlerResponse/HandlerResponse | None, but they now return theModelHandlerOutput[...]wrapper fromVaultAdapter.execute(). Combined with theHandlerResponsealias being a plaindict[...], this is misleading and diverges from the actual type.You can clarify things and better match the handler’s contract by updating the alias and imports, for example:
Proposed refactor to use the wrapper type in tests
-from uuid import UUID, uuid4 +from uuid import UUID, uuid4 + +from omnibase_core.models.dispatch import ModelHandlerOutput @@ -# Type alias for handler response (status, payload, correlation_id) -HandlerResponse = dict[str, str | UUID | dict[str, str | int | bool | None]] +# Type alias for handler response wrapper (status, payload, correlation_id) +HandlerResponse = ModelHandlerOutput[dict[str, object]] @@ - async def execute_request(index: int) -> HandlerResponse | None: + async def execute_request(index: int) -> HandlerResponse | None: @@ - async def execute_request(index: int) -> HandlerResponse: + async def execute_request(index: int) -> HandlerResponse: @@ - async def execute_write(index: int) -> HandlerResponse: + async def execute_write(index: int) -> HandlerResponse: @@ - async def execute_request(index: int) -> HandlerResponse | None: + async def execute_request(index: int) -> HandlerResponse | None: @@ - async def execute_request(index: int) -> HandlerResponse | None: + async def execute_request(index: int) -> HandlerResponse | None: @@ - async def execute_during_transition(index: int) -> HandlerResponse | None: + async def execute_during_transition(index: int) -> HandlerResponse | None: @@ - async def execute_during_recovery(index: int) -> HandlerResponse | None: + async def execute_during_recovery(index: int) -> HandlerResponse | None:This keeps the tests honest about what they’re dealing with and makes future refactors around the wrapper type less error-prone.
Also applies to: 110-127, 180-187, 220-230, 257-267, 338-358, 422-429
tests/unit/handlers/test_handler_db.py (1)
1134-1138: Consider adding coverage forhandler_idandinput_envelope_idonModelHandlerOutputThese tests now nicely verify that
result.correlation_idandoutput.correlation_idare UUIDs and remain consistent across UUID, string, and generated cases. To fully exercise the new OMN‑975 surface, consider extending one of these tests to also assert:
- The wrapper’s
handler_idmatches the DB handler constant (e.g.,"db-handler").input_envelope_idis preserved when anenvelope_idis provided on the envelope, and auto‑generated otherwise.That would close the loop on the new tracing fields introduced by
ModelHandlerOutput.Also applies to: 1165-1171, 1195-1203
src/omnibase_infra/handlers/handler_http.py (1)
17-18: HTTP handler correctly wraps responses in ModelHandlerOutput and preserves envelope IDsThe HTTP adapter now:
- Extracts
correlation_idandinput_envelope_idviaMixinEnvelopeExtractioninexecute.- Threads
input_envelope_idthrough to_execute_requestand_build_response_from_bytes.- Uses
ModelHandlerOutput.for_computewithhandler_id="http-handler"and a result dict that preserves the existing{status, payload{status_code, headers, body}, correlation_id}shape, withcorrelation_idstringified inside the payload and kept as a UUID on the wrapper.This aligns well with the new unified handler output model and keeps external behavior stable for callers that only care about the inner dict. As a follow‑up (not blocking this PR), it may be worth considering a small Pydantic model for the HTTP result payload to bring this handler in line with the DB handler’s typed response structure.
Also applies to: 28-29, 39-41, 70-70, 192-210, 211-213, 275-287, 515-523, 540-542, 628-635, 689-702
src/omnibase_infra/handlers/handler_consul.py (1)
693-699: Consider centralizing ModelHandlerOutput construction and improving health‑check correlation_id propagationTwo small, non‑blocking improvements worth considering:
DRY for wrapper construction
The pattern:result = {..., "correlation_id": str(correlation_id)} return ModelHandlerOutput.for_compute( input_envelope_id=input_envelope_id, correlation_id=correlation_id, handler_id=HANDLER_ID_CONSUL, result=result, )is repeated across
_kv_get,_kv_put,_register_service,_deregister_service, and_health_check_operation. A tiny helper like_wrap_result(result, correlation_id, input_envelope_id)would reduce duplication and keep future changes to the wrapper shape in one place.Correlation ID propagation into underlying
health_check()
_health_check_operationreceives the envelope’scorrelation_idbut delegates tohealth_check(), which internally generates a new UUID for its own correlation ID. This means logs andModelInfraErrorContextinstances created during the health‑check RPCs will not use the same correlation ID that the caller sees on theModelHandlerOutput. If you want strict end‑to‑end tracing (per the correlation_id guidelines), consider updatinghealth_check()to accept an optionalcorrelation_id: UUID | Noneparameter so_health_check_operationcan pass the envelope’s ID through, falling back touuid4()only when called directly.Also applies to: 780-817, 977-992, 1181-1191
src/omnibase_infra/handlers/handler_vault.py (1)
1079-1107: Propagate envelope correlation_id into renew_token and health_check for consistent tracingRight now:
renew_token()andhealth_check()each generate their owncorrelation_id = uuid4()internally._renew_token_operationand_health_check_operationaccept acorrelation_idfrom the envelope and use it in theModelHandlerOutput, but they callrenew_token()/health_check()without passing that ID through.This means logs and
ModelInfraErrorContextinstances created inside those methods (and their_execute_with_retrycalls) will use a different correlation ID than the one exposed on the returnedModelHandlerOutput, which weakens end‑to‑end tracing.A small refactor could align everything:
- Let
renew_token()andhealth_check()accept an optional correlation ID and only fall back touuid4()when none is provided.- Pass the envelope’s correlation ID from
_renew_token_operationand_health_check_operationdown into those methods.For example:
- async def renew_token(self) -> dict[str, object]: + async def renew_token(self, correlation_id: UUID | None = None) -> dict[str, object]: @@ - correlation_id = uuid4() + correlation_id = correlation_id or uuid4() @@ - result = await self.renew_token() + result = await self.renew_token(correlation_id=correlation_id)And similarly add an optional
correlation_idparameter tohealth_check()and have_health_check_operationpass itscorrelation_idthrough. This keeps direct calls working as before while making envelope‑driven flows fully correlation‑aware.Also applies to: 1182-1214, 1216-1250, 1316-1364
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (11)
src/omnibase_infra/handlers/handler_consul.py(16 hunks)src/omnibase_infra/handlers/handler_db.py(10 hunks)src/omnibase_infra/handlers/handler_http.py(13 hunks)src/omnibase_infra/handlers/handler_vault.py(14 hunks)src/omnibase_infra/mixins/__init__.py(2 hunks)src/omnibase_infra/mixins/mixin_envelope_extraction.py(1 hunks)tests/unit/handlers/test_handler_consul.py(14 hunks)tests/unit/handlers/test_handler_db.py(11 hunks)tests/unit/handlers/test_handler_http.py(31 hunks)tests/unit/handlers/test_handler_vault.py(12 hunks)tests/unit/handlers/test_handler_vault_concurrency.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER useAnytypes - always use specific types. All data structures must be proper Pydantic models.
Use PEP 604 union syntaxX | Nonefor nullable types instead ofOptional[X]in type annotations.
Always propagatecorrelation_idfrom incoming requests to error context and auto-generate usinguuid4()if not present. Use UUID format for all new correlation IDs.
NEVER include passwords, API keys, tokens, secrets, full connection strings with credentials, PII, private IPs, private keys, or session tokens in error messages or context. Only include sanitized service names, operation names, correlation IDs, error codes, sanitized hostnames, port numbers, retry counts, and resource identifiers.
ForProtocolConfigurationError, use error codeINVALID_CONFIGURATIONand HTTP 400 Bad Request.
ForSecretResolutionError, use error codeRESOURCE_NOT_FOUNDand HTTP 404 Not Found.
ForInfraConnectionError, use transport-aware error code selection: DATABASE→DATABASE_CONNECTION_ERROR, HTTP/GRPC→NETWORK_ERROR, KAFKA/CONSUL/VAULT/VALKEY→SERVICE_UNAVAILABLE. HTTP equivalent is 503 Service Unavailable.
ForInfraTimeoutError, use error codeTIMEOUT_ERRORand HTTP 504 Gateway Timeout.
ForInfraAuthenticationError, use error codeAUTHENTICATION_ERRORand HTTP 401 Unauthorized.
ForInfraUnavailableError, use error codeSERVICE_UNAVAILABLEand HTTP 503 Service Unavailable.
Always createModelInfraErrorContextwhen raising infrastructure errors, includingtransport_type(HTTP, DATABASE, KAFKA, CONSUL, VAULT, VALKEY, GRPC),operationname,target_name(service identifier), andcorrelation_id.
All error classes MUST inherit fromOnexErrorvia the infrastructure error hierarchy. Raise errors asraise OnexError(...) from eto preserve exception chains.
Implement retry with exponential backoff for transientInfraConnectionErrorfailures. Use backoff pattern like 1s, 2s, 4s with configurable max retries.
Implement circuit breaker patter...
Files:
src/omnibase_infra/mixins/__init__.pysrc/omnibase_infra/mixins/mixin_envelope_extraction.pysrc/omnibase_infra/handlers/handler_db.pytests/unit/handlers/test_handler_http.pytests/unit/handlers/test_handler_vault.pytests/unit/handlers/test_handler_db.pysrc/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_http.pytests/unit/handlers/test_handler_vault_concurrency.pysrc/omnibase_infra/handlers/handler_vault.pytests/unit/handlers/test_handler_consul.py
**/mixin_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Mixin files follow naming convention
mixin_<name>.py→Mixin<Name>with exactly one mixin class per file.
Files:
src/omnibase_infra/mixins/mixin_envelope_extraction.py
🧠 Learnings (24)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use handler envelopes from `omnibase_infra` for all I/O operations (HTTP, database, Kafka) instead of custom clients
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/*.py : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented
Applied to files:
src/omnibase_infra/mixins/__init__.pysrc/omnibase_infra/mixins/mixin_envelope_extraction.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/{adapter,service}*.py : Infrastructure adapters and services SHOULD use `MixinAsyncCircuitBreaker` for fault tolerance. Initialize with `_init_circuit_breaker(threshold, reset_timeout, service_name, transport_type)` and always hold `self._circuit_breaker_lock` when calling circuit breaker methods.
Applied to files:
src/omnibase_infra/mixins/__init__.pysrc/omnibase_infra/handlers/handler_vault.py
📚 Learning: 2025-11-24T16:32:55.606Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Node implementations must use mixin-based composition from `omnibase_core.mixins` (e.g., `MixinHealthCheck`, `MixinNodeExecutor`) to add capabilities
Applied to files:
src/omnibase_infra/mixins/__init__.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/nodes/*/node.py : Node introspection via `MixinNodeIntrospection` automatically discovers node capabilities using reflection. Prefix internal/sensitive methods with `_` to exclude them from introspection. Use generic operation and parameter names that don't reveal implementation details.
Applied to files:
src/omnibase_infra/mixins/__init__.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/*.py : Always propagate `correlation_id` from incoming requests to error context and auto-generate using `uuid4()` if not present. Use UUID format for all new correlation IDs.
Applied to files:
src/omnibase_infra/mixins/mixin_envelope_extraction.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to agents/**/*.py : Use correlation_id UUID for end-to-end traceability across all agent routing, manifest injection, and execution events
Applied to files:
src/omnibase_infra/mixins/mixin_envelope_extraction.pytests/unit/handlers/test_handler_http.pytests/unit/handlers/test_handler_consul.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use `DbAdapter` envelope pattern for database operations instead of custom PostgreSQL clients
Applied to files:
src/omnibase_infra/handlers/handler_db.pytests/unit/handlers/test_handler_db.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use handler envelopes from `omnibase_infra` for all I/O operations (HTTP, database, Kafka) instead of custom clients
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_consul.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/*.py : Always create `ModelInfraErrorContext` when raising infrastructure errors, including `transport_type` (HTTP, DATABASE, KAFKA, CONSUL, VAULT, VALKEY, GRPC), `operation` name, `target_name` (service identifier), and `correlation_id`.
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/{postgres,database,connection}*.py : Database connections MUST be managed through dedicated connection pool managers. Use PostgreSQL adapter for database operations.
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : Database layer MUST use connection pooling (10-50 connections), prepared statements, and circuit breaker pattern for resilience. Monitor pool exhaustion at >90% utilization.
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-30T21:55:10.298Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : All input validation MUST prevent SQL injection using prepared statements and parameterized queries. Use asyncpg for PostgreSQL operations.
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-12-03T16:55:49.755Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Applies to **/*.py : Access PostgreSQL connection strings via settings.get_postgres_dsn() or settings.get_postgres_dsn(async_driver=True) rather than constructing connection strings manually
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/{consul,discovery,adapter}*.py : Use Consul adapter for service discovery and dynamic service resolution.
Applied to files:
src/omnibase_infra/handlers/handler_consul.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/*.py : Implement circuit breaker pattern to prevent cascading failures for `InfraUnavailableError`. Use states: CLOSED (normal), OPEN (blocked), HALF_OPEN (testing recovery).
Applied to files:
src/omnibase_infra/handlers/handler_consul.py
📚 Learning: 2025-12-06T22:21:32.649Z
Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/effect/**/*.py : Use `HttpRestAdapter` envelope pattern for HTTP calls instead of direct httpx.AsyncClient
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-28T18:58:53.781Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-28T18:58:53.781Z
Learning: Applies to **/*.py : Use `EnumCoreErrorCode` with `ModelOnexError` for proper error code usage
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/*.py : Transport types in error context MUST be from `EnumInfraTransportType`: HTTP, DATABASE, KAFKA, CONSUL, VAULT, VALKEY, GRPC.
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-20T04:09:41.822Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T04:09:41.822Z
Learning: Applies to **/*.py : Use ModelOnexError with EnumCoreErrorCode for all error handling instead of generic Exception
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/{vault,secret,credential}*.py : Use Vault adapter for secure credential and secret management.
Applied to files:
src/omnibase_infra/handlers/handler_vault.py
📚 Learning: 2025-09-23T22:28:00.333Z
Learnt from: jonahgabriel
Repo: OmniNode-ai/omnibase_core PR: 32
File: examples/practical_migration_example.py:125-134
Timestamp: 2025-09-23T22:28:00.333Z
Learning: The omnibase_core codebase extensively uses constrained TypeVars for primitive types with the pattern `T = TypeVar("T", str, int, float, bool)` across multiple models including ModelTypedMetrics, ModelGenericMetadata, and ModelExecutionResult. This is the preferred approach over hardcoded union types like `str | int | bool | float | None`.
Applied to files:
src/omnibase_infra/handlers/handler_vault.py
📚 Learning: 2025-12-20T16:31:18.964Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-20T16:31:18.964Z
Learning: Applies to **/*.py : For `SecretResolutionError`, use error code `RESOURCE_NOT_FOUND` and HTTP 404 Not Found.
Applied to files:
src/omnibase_infra/handlers/handler_vault.py
🧬 Code graph analysis (6)
src/omnibase_infra/mixins/__init__.py (1)
src/omnibase_infra/mixins/mixin_envelope_extraction.py (1)
MixinEnvelopeExtraction(37-105)
src/omnibase_infra/handlers/handler_db.py (3)
src/omnibase_infra/mixins/mixin_envelope_extraction.py (3)
MixinEnvelopeExtraction(37-105)_extract_correlation_id(47-71)_extract_envelope_id(73-105)src/omnibase_infra/handlers/models/model_db_query_response.py (1)
ModelDbQueryResponse(19-56)src/omnibase_infra/handlers/models/model_db_query_payload.py (1)
ModelDbQueryPayload(22-52)
tests/unit/handlers/test_handler_http.py (4)
src/omnibase_infra/handlers/handler_db.py (1)
execute(183-276)src/omnibase_infra/handlers/handler_http.py (1)
execute(192-287)src/omnibase_infra/handlers/handler_vault.py (1)
execute(404-499)tests/unit/runtime/test_runtime_host_process.py (4)
execute(95-118)execute(145-148)execute(839-841)execute(1492-1493)
tests/unit/handlers/test_handler_vault.py (2)
src/omnibase_infra/handlers/handler_vault.py (1)
execute(404-499)tests/unit/runtime/test_runtime_host_process.py (4)
execute(95-118)execute(145-148)execute(839-841)execute(1492-1493)
src/omnibase_infra/handlers/handler_vault.py (1)
src/omnibase_infra/mixins/mixin_envelope_extraction.py (1)
_extract_envelope_id(73-105)
tests/unit/handlers/test_handler_consul.py (1)
src/omnibase_infra/handlers/handler_consul.py (1)
execute(410-495)
🔇 Additional comments (6)
src/omnibase_infra/mixins/__init__.py (1)
16-16: Public export ofMixinEnvelopeExtractionlooks goodImport and
__all__wiring are consistent with the existing mixin exports; no issues.Also applies to: 26-36
tests/unit/handlers/test_handler_vault_concurrency.py (1)
157-197: Concurrency success-path assertions correctly useoutput.resultIn
test_concurrent_successful_operations,test_concurrent_mixed_write_operations, andtest_thread_pool_handles_concurrent_load, assertingall(output.result["status"] == "success" for output in results)is the right way to validate success now thatexecute()returns aModelHandlerOutput[...]. This keeps the concurrency semantics tests aligned with the new handler API.No changes needed here beyond the alias cleanup mentioned separately.
Also applies to: 239-277, 399-438
tests/unit/handlers/test_handler_db.py (1)
202-203: DB tests correctly adapted to ModelHandlerOutput wrapperThe pattern of capturing
output = await adapter.execute(envelope)and then usingresult = output.resultto assert onModelDbQueryResponsefields is consistent and keeps the original test intent intact. The added assertion onoutput.correlation_idwhere present ensures the wrapper preserves the correlation ID from the envelope. No functional issues here.Also applies to: 211-212, 242-243, 274-275, 320-321, 357-358, 385-386, 416-417, 1417-1418
src/omnibase_infra/handlers/handler_db.py (1)
18-19: DbAdapter’s ModelHandlerOutput integration and envelope tracking look solid
DbAdapternow cleanly wraps alldb.query/db.executeresults inModelHandlerOutput[ModelDbQueryResponse], with:
correlation_idderived viaMixinEnvelopeExtraction._extract_correlation_id.input_envelope_idderived via_extract_envelope_idand threaded throughexecute → _execute_query/_execute_statement → _build_response.- A stable
handler_idviaHANDLER_ID_DB.
_build_responsecentralizes the wrapping logic and still returns a strongly typedModelDbQueryResponse, so downstream code and tests remain type‑safe. Error paths continue to useModelInfraErrorContextwith the extractedcorrelation_id, which keeps tracing consistent with the new wrapper. No issues found in these changes.Also applies to: 28-29, 43-45, 49-49, 183-201, 207-209, 269-276, 327-334, 346-361, 383-390, 409-417, 470-488
src/omnibase_infra/handlers/handler_consul.py (1)
43-45: Consul handler’s ModelHandlerOutput integration and envelope tracking are consistentThe Consul handler now:
- Uses
MixinEnvelopeExtractionto extractcorrelation_idandinput_envelope_idinexecute.- Returns
ModelHandlerOutput[dict[str, object]]from all operations (kv_get,kv_put,register,deregister,health_check) with a stablehandler_id="consul-handler".- Preserves the prior result shapes, just wrapped in the new container, and stringifies
correlation_idinside the result while keeping the UUID on the wrapper.- Continues to route correlation IDs through
_execute_with_retry, so infrastructure errors still carry the tracing ID required byModelInfraErrorContext.The implementation looks correct and aligns with the new OMN‑975 output contract.
Also applies to: 54-56, 68-68, 410-424, 431-433, 485-496, 693-699, 744-762, 780-817, 819-838, 886-899, 901-922, 938-946, 977-992, 104-1192
src/omnibase_infra/handlers/handler_vault.py (1)
16-20: Vault adapter’s ModelHandlerOutput wrapping and envelope tracking are well‑implementedThe Vault adapter now:
- Uses
MixinEnvelopeExtractionto extract bothcorrelation_idandinput_envelope_idinexecute.- Returns
ModelHandlerOutput[dict[str, object]]from all envelope‑driven operations, withhandler_id="vault-handler".- Keeps the existing result payloads (secret data, write metadata, delete/list confirmations, token renew info, health status) unchanged, but wrapped and annotated with correlation and envelope IDs.
- Continues to use correctly populated
ModelInfraErrorContextinstances withEnumInfraTransportType.VAULTand the extractedcorrelation_idfor error cases.Overall this aligns cleanly with the OMN‑975 contract and preserves prior behavior for callers that only inspect the inner dict.
Also applies to: 32-33, 52-53, 56-58, 72-72, 404-419, 426-428, 488-500, 765-782, 825-837, 839-857, 915-927, 929-946, 953-962, 983-992, 1000-1011, 1182-1214, 1316-1364
…istency [OMN-975] - Add correlation_id to HTTP handler health_check() return dict - Add optional correlation_id parameter to Vault renew_token() and health_check() - Add optional correlation_id parameter to Consul health_check() - Propagate envelope correlation_id through _health_check_operation() callers - Update docstrings with envelope_id and correlation_id documentation All changes maintain backwards compatibility - direct calls auto-generate correlation_id while envelope-based dispatch preserves request tracing.
PR Review: feat(handlers): adopt ModelHandlerOutput in infra handlers [OMN-975]🎯 SummaryThis PR successfully migrates all infrastructure handlers to return ✅ Strengths1. Consistent Architecture
2. Improved Tracing & Observability
3. Type Safety
4. Excellent Documentation
🔍 Issues & Concerns1. Breaking Change in correlation_id Serialization
|
1 participant
Summary
ModelHandlerOutput[T]instead of raw dictionariesomnibase_core.models.dispatch(PR feat(OMN-1752): Extract ContractPublisher service to omnibase_infra #223, OMN-941)Changes
handler_consul.pyModelHandlerOutput.for_compute()handler_vault.pyModelHandlerOutput.for_compute()handler_http.pyModelHandlerOutput.for_compute()handler_db.pyModelHandlerOutput.for_compute()Each handler now:
ModelHandlerOutputfromomnibase_core.models.dispatchinput_envelope_idandcorrelation_idas UUIDsModelHandlerOutput.for_compute()handler_idfor traceabilityTicket
Closes OMN-975
Test plan
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.