Repository navigation
feat(runtime): complete Vault integration for SecretResolver [OMN-1374] - #167
jonahgabriel merged 13 commits into
Conversation
Implements production-ready Vault integration with observability: - Full Vault integration via HandlerVault (path parsing: mount/path#field) - Correlation ID propagation on get_secret() and get_secret_async() - Metrics tracking: success/failure counts, cache hits, latency - External metrics collector support via set_metrics_collector() - Graceful degradation when vault_handler is None Includes comprehensive unit tests for all features and integration tests for real Vault scenarios with proper test isolation and cleanup.
|
Warning Rate limit exceeded
⌛ 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)
📝 WalkthroughWalkthroughSecretResolver now implements Vault KV v2 resolution with mount/path#field parsing, per-key async locks and LRU eviction, cache TTL jitter, correlation_id propagation, metrics hooks (ProtocolSecretResolverMetrics), improved error/logging, and accompanying unit and integration tests for Vault flows and metrics. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant SR as SecretResolver
participant Vault as HandlerVault
participant Metrics as MetricsCollector
Client->>SR: get_secret(logical_name, correlation_id)
activate SR
rect rgba(200,150,255,0.5)
Note over SR: Check cache (hit/miss)
end
alt Cache miss
SR->>SR: Parse vault path (mount/path#field)
SR->>SR: Acquire per-key lock
SR->>Vault: read_secret_kv2(mount, path, correlation_id)
activate Vault
Vault-->>SR: secret payload / not_found / error
deactivate Vault
rect rgba(150,200,255,0.5)
Note over SR: Extract field, normalize to SecretStr, update cache (TTL + jitter)
end
SR->>Metrics: record latency, success/failure, cache_miss
activate Metrics
Metrics-->>SR: ack
deactivate Metrics
SR->>SR: Release per-key lock
else Cache hit
SR->>Metrics: record cache_hit
end
SR-->>Client: SecretStr or None
deactivate SR
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Comment |
Code Review: Vault Integration for SecretResolver [OMN-1374]SummaryThis PR implements complete Vault integration for the SecretResolver with correlation ID propagation and observability metrics. The implementation is solid overall with comprehensive test coverage. ✅ Strengths1. Excellent Test Coverage
2. Strong Security Practices
3. Robust Error Handling
4. Observability Features
5. Documentation Quality
🔍 Code Quality Observations1. Type Safety ✅
2. ONEX Pattern Compliance ✅
3. Thread Safety ✅
🚨 Potential Issues1. Sync Method in Async Context
|
- Switch _resolution_latencies from list to deque(maxlen=1000) for O(1) rotation - Add Warning section to get_secret() docstring about async context limitation - Consolidate Protocol/runtime_checkable imports into main typing import - Enhance ProtocolSecretResolverMetrics thread safety docs with reasoning - Document path parsing edge case for unusual "secret#field" format
PR Review: Vault Integration for SecretResolver [OMN-1374]SummaryThis PR implements full Vault integration for the SecretResolver, including correlation ID propagation, metrics/observability, and comprehensive test coverage. The implementation is well-structured and follows ONEX patterns closely. ✅ Strengths1. Strong Type Safety
2. Security Best Practices
3. Thread Safety Implementation
4. Observability & Metrics
5. Test Coverage
🔴 Critical Issues1. Missing Error Context Validation (src/omnibase_infra/runtime/secret_resolver.py:424-434)Severity: HIGH The error context creation doesn't validate the # Line 424-434
context = ModelInfraErrorContext.with_correlation(
correlation_id=effective_correlation_id,
transport_type=EnumInfraTransportType.RUNTIME,
operation="get_secret",
target_name="secret_resolver",
)Problem: If Wait - Re-analysis: Actually, this is correct on second look. The pattern 2. Potential Denial of Service via Unbounded Lock Dictionary (src/omnibase_infra/runtime/secret_resolver.py:352)Severity: MEDIUM # Line 352
self._async_key_locks: dict[str, asyncio.Lock] = {}Problem: The Recommendation:
3. Missing Vault Handler Initialization Validation (src/omnibase_infra/runtime/secret_resolver.py:326-341)Severity: MEDIUM The constructor accepts def __init__(
self,
config: ModelSecretResolverConfig,
vault_handler: HandlerVault | None = None,
metrics_collector: ProtocolSecretResolverMetrics | None = None,
) -> None:
self._vault_handler = vault_handler # No validation\!Problem: If a user passes an uninitialized Recommendation:
|
![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/runtime/secret_resolver.py`:
- Around line 777-783: The current _record_resolution_failure unconditionally
calls self._metrics_collector.record_cache_miss on any failure; change it to
only call record_cache_miss for actual cache-miss conditions. Modify
_record_resolution_failure (and callers if needed) to accept or inspect the
failure/exception (e.g., an error object or failure_type) and only call
self._metrics_collector.record_cache_miss() when the failure indicates a true
cache miss (for example SecretNotFound/CacheMissError or a specific enum value),
but continue to call record_resolution_failure(source_type) for all failures;
leave auth/timeouts/unavailable errors without the cache-miss call. Ensure you
reference self._metrics_collector.record_resolution_failure and
self._metrics_collector.record_cache_miss in the conditional logic.
🧹 Nitpick comments (6)
src/omnibase_infra/runtime/secret_resolver.py (2)
682-691:set_metrics_collectoris not thread-safe.This method directly assigns to
_metrics_collectorwithout holding_lock. If called while_record_resolution_successor_record_resolution_failureis executing, there could be a race condition where the collector reference changes mid-operation.♻️ Suggested fix for thread-safe assignment
def set_metrics_collector( self, collector: ProtocolSecretResolverMetrics | None ) -> None: """Set the external metrics collector. Args: collector: Metrics collector implementing ProtocolSecretResolverMetrics, or None to disable external metrics collection. """ - self._metrics_collector = collector + with self._lock: + self._metrics_collector = collector
1361-1427: Vault response handling converts all secret values to strings, regardless of stored type.Vault KV v2 can store non-string values (numbers, booleans, objects, arrays) via the HTTP API, but the code on line 1427 converts all values to strings using
str(value). This means:
- Non-string values will be stringified (e.g.,
{"key": "val"}becomes'{"key": "val"}')- Complex types lose their semantic meaning
Confirm whether string-only secret values are the intended design, or whether non-string values should be preserved or explicitly rejected with a clear error.
tests/unit/runtime/test_secret_resolver.py (2)
1294-1334: Unused import ofModelHandlerOutput.Line 1298 imports
ModelHandlerOutputfromomnibase_core.models.dispatch, but it's never used in the test. The mock returns aMagicMockwith aresultattribute instead.♻️ Remove unused import
def test_vault_resolves_secret_with_mocked_handler(self) -> None: """Vault secrets should resolve when handler returns valid response.""" from unittest.mock import AsyncMock, MagicMock - from omnibase_core.models.dispatch import ModelHandlerOutput - # Create a mock vault handler mock_vault_handler = MagicMock()
1729-1750: Redundant import ofuuid4inside test method.Line 1733 imports
uuid4insidetest_vault_auth_error_propagates, butuuid4is already imported at the module level (line 34). The import inside the function is unused and theuuid.uuid4()call isn't actually made in this test.♻️ Remove redundant import
`@pytest.mark.asyncio` async def test_vault_auth_error_propagates(self) -> None: """Should propagate InfraAuthenticationError from handler.""" from unittest.mock import AsyncMock, MagicMock - from uuid import uuid4 from omnibase_infra.enums import EnumInfraTransportType from omnibase_infra.errors import ( ModelInfraErrorContext, )tests/integration/runtime/test_secret_resolver_vault_integration.py (2)
79-96: Cleanup fixture should be more defensive about delete operation errors.The cleanup fixture catches all exceptions but only logs at debug level. If the delete operation fails for reasons other than "secret doesn't exist" (e.g., permission issues), this could leave test artifacts in Vault without clear indication.
♻️ Consider differentiating error types in cleanup
# Cleanup: delete the test secret (idempotent) try: envelope = { "operation": "vault.delete_secret", "payload": {"path": unique_secret_path, "mount_point": "secret"}, "correlation_id": str(uuid.uuid4()), } await vault_handler.execute(envelope) except Exception as e: - logger.debug("Cleanup: secret may not exist - %s", e) + # Log at warning level for unexpected errors (not just "not found") + logger.warning( + "Cleanup: failed to delete test secret %s - %s", + unique_secret_path, + e, + )
352-394: Graceful degradation tests ensure reliability without Vault.The
TestSecretResolverVaultGracefulDegradationclass verifies the system behaves correctly whenvault_handlerisNone:
- Returns
Noneinstead of raising- Tracks failure metrics
These tests don't require Vault connectivity since they test the handler-absent path, but they're still guarded by the module skip markers. Consider whether these should run unconditionally.
♻️ Consider removing skip markers for tests that don't need Vault
The graceful degradation tests don't actually interact with Vault since they test the
vault_handler=Nonepath. You could mark them to run unconditionally:class TestSecretResolverVaultGracefulDegradation: """Tests for graceful degradation scenarios.""" + # Note: These tests run even without Vault since they test handler-absent behavior + pytestmark = [] # Override module-level skip markers + def test_vault_handler_none_returns_none(self) -> None:Alternatively, consider moving these tests to the unit test file since they don't require Vault infrastructure.
…-1374] - Fix record_cache_miss called unconditionally in _record_resolution_failure - Add thread safety to set_metrics_collector with lock - Make cleanup fixture more defensive with specific exception handling - Fix inconsistent logging levels (security=ERROR, config=WARNING, expected=DEBUG) - Add comprehensive documentation for Vault string conversion behavior - Remove unused imports and organize imports properly
PR Review: Vault Integration for SecretResolver [OMN-1374]This PR implements comprehensive Vault integration for SecretResolver with correlation ID propagation and observability metrics. Strengths
Critical Issue: asyncio.Lock Event Loop BindingLocation: secret_resolver.py:1581 asyncio.Lock objects are bound to the event loop in which they are created. If _get_async_key_lock() is called from different event loops (sequential asyncio.run() calls), the lock may raise RuntimeError. Recommendation: Track event loop with each lock and recreate if loop changes, OR document that SecretResolver instances must not be shared across asyncio.run() boundaries. Impact: Medium - Will fail in CLI tools or tests that call asyncio.run() multiple times. Medium Issue: Missing Cache Miss MetricsLocation: secret_resolver.py:795-797 While self._misses is incremented, the external metrics collector record_cache_miss() is NEVER called. External monitoring systems won't see cache miss metrics. Recommendation: Add record_cache_miss() calls where self._misses += 1 occurs. Minor Issues
Test CoverageWell covered: Vault path parsing, error handling, correlation ID, integration tests RecommendationsMust Fix: asyncio.Lock event loop binding issue Final VerdictAPPROVE with required fixes High-quality code with excellent security and ONEX compliance. The asyncio.Lock issue must be addressed before merge. Risk: Medium (due to asyncio.Lock) to Low (once fixed) |
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_infra/runtime/secret_resolver.py (1)
361-361: Prevent self-deadlock when recording metrics under_lock.
get_secret/get_secret_asynccall_record_resolution_successwhile holding_lock, and_record_resolution_successre-acquires_lock, which will deadlock in sync paths.🐛 Proposed fix (use a re-entrant lock)
- self._lock = threading.Lock() + self._lock = threading.RLock()Also applies to: 421-427, 515-519, 726-731
🧹 Nitpick comments (2)
src/omnibase_infra/runtime/secret_resolver.py (2)
338-343: Consider container-based DI for the new constructor dependency.
With the newmetrics_collectorparameter, it’s a good moment to alignSecretResolverwith theModelONEXContainerpattern for service construction. As per coding guidelines, use the container in__init__for service dependencies.
1393-1401: UseJsonTypefor Vault envelopes to match typing conventions.
The envelope is JSON-compatible; usingJsonTypewill align with internal type standards and avoidobjectleakage in API boundaries. As per coding guidelines, preferJsonTypefor JSON payloads.
…r [OMN-1374] - Fix deadlock: Change threading.Lock to threading.RLock (reentrant) - get_secret() holds lock while calling _record_resolution_success() - _record_resolution_success() also needs the lock - Non-reentrant Lock caused deadlock; RLock allows same thread to re-acquire - Fix thread safety race in metrics collector access - Capture _metrics_collector reference while holding lock - Use captured reference outside lock to avoid race with set_metrics_collector() - Use JsonType for Vault envelope type annotation (ONEX convention) - Document _async_key_locks unbounded growth limitation and DoS mitigation - Update all docstrings: threading.Lock -> threading.RLock
Code Review - PR #167: Vault Integration for SecretResolverSummaryThis PR implements complete Vault integration for the SecretResolver with correlation ID propagation and metrics tracking. The implementation is comprehensive and well-tested. Overall quality is excellent with only minor suggestions for improvement. ✅ Strengths1. Excellent Documentation
2. Strong Type Safety
3. Comprehensive Test Coverage
4. Security Best Practices
5. Error Handling
🔍 Code Quality Issues1. Potential Race Condition in Async Resolution (Minor)Location: # THREAD SAFETY: Use lock for cache write to prevent race with sync callers
# Check-before-write pattern avoids unnecessary overwrites if sync caller
# already cached this secret between our cache check and now
with self._lock:
if logical_name not in self._cache:
self._cache_secret(logical_name, secret, source_type)Issue: The check-before-write pattern is good, but there's still a TOCTOU (time-of-check-time-of-use) window:
Recommendation: This is acceptable as documented ("last-write-wins"), but consider using with self._lock:
if logical_name not in self._cache:
self._cache_secret(logical_name, secret, source_type)
# OR: self._cache.setdefault(logical_name, cached_entry)Severity: Low - documented behavior, no correctness issue 2. Unbounded Dictionary Growth (Documented Limitation)Location: The # Known Limitation - Unbounded Dictionary Growth:
# The _async_key_locks dictionary grows monotonically...Risk: DoS if attacker controls logical names passed to Mitigation: Already documented - "validate logical names against configured mappings before calling resolution methods". Consider adding a size limit check: if len(self._async_key_locks) > 10000: # Sanity limit
logger.warning("Async key locks dictionary exceeds 10k entries - potential DoS")Severity: Low - well-documented, bounded in practice 3. Inconsistent Metrics Collector Error Handling (Minor)Location: Metrics collector errors are silently suppressed with debug logging. This is correct for metrics (never fail resolution due to metrics), but consider: except Exception as e:
# Never let metrics failures affect resolution
logger.debug(
"Metrics collector error (ignored): %s",
e,
# SUGGESTION: Add exception type for better diagnostics
extra={
"logical_name": logical_name,
"correlation_id": str(correlation_id),
"error_type": type(e).__name__, # ADD THIS
},
)Severity: Very Low - quality-of-life improvement 🚀 Performance Considerations1. Vault Path Parsing on Every CallLocation: mount_point, vault_path, field = self._parse_vault_path_components(path)Optimization: Path parsing happens on every Vault resolution, even for cached secrets. This is mitigated by cache hits, but consider pre-parsing paths during Impact: Negligible - string split operations are O(n) where n is path length (~10-50 chars) 2. Sequential Secret Resolution in Sync
|
| Policy | Status | Notes |
|---|---|---|
Zero Any types |
✅ PASS | No Any types detected |
| PEP 604 unions | ✅ PASS | Uses `X |
| Pydantic models | ✅ PASS | All data structures are proper models |
| Error handling | ✅ PASS | Uses ModelInfraErrorContext.with_correlation() |
| Correlation ID propagation | ✅ PASS | Implemented throughout |
| Security patterns | ✅ PASS | Path traversal protection, sanitized errors |
| No backwards compatibility | ✅ PASS | Breaking changes are acceptable per CLAUDE.md |
🎯 Recommendations
Priority: Low
- Add
error_typeto metrics collector exception logging (lines 779-788, 840-848) - Consider size limit warning for
_async_key_locksdictionary (line 607) - Document
mlock()alternative for high-security deployments (module docstring)
Priority: Very Low (Optional)
- Pre-parse Vault paths during
__init__for slight performance gain (negligible impact)
✅ Approval Status
APPROVED - This PR is production-ready with excellent code quality, comprehensive tests, and strong adherence to ONEX patterns.
Final Score: 9.5/10
- Code Quality: Excellent
- Test Coverage: Excellent
- Documentation: Excellent
- Security: Excellent
- Performance: Good (intentional trade-offs for clarity)
Suggested merge after:
- ✅ Pre-commit hooks pass
- ✅ CI validators pass (
any_types,mypy,ruff) - ✅ Integration tests pass (or skip gracefully without Vault)
Great work! 🎉
…est [OMN-1374] The test was accessing correlation_id via context dict attribute (exc_info.value.context.correlation_id) but ModelOnexError exposes correlation_id as a top-level property. Fixed to use the correct access pattern (exc_info.value.correlation_id).
…pede [OMN-1374] Add 0-10% random jitter to cache TTL values in SecretResolver to prevent cache stampede (thundering herd) scenarios where many cached entries expire simultaneously, causing burst resolution requests. Changes: - Add `import random` for jitter calculation - Modify `_cache_secret()` to apply jitter factor to base TTL - Update docstring to document jitter behavior This addresses the PR #167 review feedback requesting TTL jitter support.
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_infra/runtime/secret_resolver.py (2)
426-455: Cache misses aren’t recorded, so hit/miss metrics are misleading.
_missesis only incremented when resolution returnsNone, so successful cache misses are never counted, and external collectors never receiverecord_cache_miss()(it’s not called anywhere). That skews cache stats and the metrics payload. Consider recording the miss immediately after a cache lookup fails (once per request) and remove the increment on the finalNonepath to avoid double-counting.🐛 Proposed fix: record cache miss on lookup
@@ with self._lock: # Check cache first cached = self._get_from_cache(logical_name) if cached is not None: self._record_resolution_success( logical_name, "cache", effective_correlation_id ) return cached + # Cache miss accounting (before resolution) + self._misses += 1 + collector = self._metrics_collector + if collector is not None and hasattr(collector, "record_cache_miss"): + try: + collector.record_cache_miss() + except Exception as e: + logger.debug( + "Metrics collector error (ignored): %s", + e, + extra={ + "logical_name": logical_name, + "correlation_id": str(effective_correlation_id), + }, + ) # Resolve from source result = self._resolve_secret(logical_name, effective_correlation_id) if result is None: - self._misses += 1 if required: context = ModelInfraErrorContext.with_correlation( correlation_id=effective_correlation_id, transport_type=EnumInfraTransportType.RUNTIME, @@ - # Use threading lock for cache check (fast operation, prevents race with sync) - with self._lock: + collector = None + # Use threading lock for cache check (fast operation, prevents race with sync) + with self._lock: cached = self._get_from_cache(logical_name) if cached is not None: self._record_resolution_success( logical_name, "cache", effective_correlation_id ) return cached + self._misses += 1 + collector = self._metrics_collector + + if collector is not None and hasattr(collector, "record_cache_miss"): + try: + collector.record_cache_miss() + except Exception as e: + logger.debug( + "Metrics collector error (ignored): %s", + e, + extra={ + "logical_name": logical_name, + "correlation_id": str(effective_correlation_id), + }, + ) @@ if result is None: - with self._lock: - self._misses += 1 if required: context = ModelInfraErrorContext.with_correlation( correlation_id=effective_correlation_id, transport_type=EnumInfraTransportType.RUNTIME,Also applies to: 518-566
1036-1060: Record failure metrics when Vault operations raise exceptions.
_read_vault_secret_asyncraisesInfraAuthenticationError,InfraTimeoutError,InfraUnavailableError, andSecretResolutionError, but these exceptions propagate unhandled through_resolve_secretand_resolve_secret_async. When exceptions occur, failure metrics are never recorded because the_record_resolution_failurecall at line 1059 (sync) and line 1149 (async) is only reached whenvalue is None, not when exceptions are raised.Wrap both the sync call at line 1053 and async call at line 1143 with try-catch blocks that record
_record_resolution_failurebefore re-raising, matching the pattern already used for thehandler_not_configuredcase.
🧹 Nitpick comments (1)
src/omnibase_infra/runtime/secret_resolver.py (1)
695-719: Use JsonType (or a model) for metrics payload typing.
dict[str, object]is too loose for a JSON payload and doesn’t align with the JsonType guideline. Considerdict[str, JsonType](or a dedicatedModelSecretResolverMetricsPydantic model if you want stronger structure).♻️ Suggested typing tweak
- def get_resolution_metrics(self) -> dict[str, object]: + def get_resolution_metrics(self) -> dict[str, JsonType]:As per coding guidelines, use JsonType for JSON-compatible values.
|
@claude review this |
|
Claude finished @jonahgabriel's task in 2m 25s —— View job Code Review: Vault Integration for SecretResolver [OMN-1374]OverviewThis PR implements comprehensive Vault integration for SecretResolver with correlation ID propagation and observability metrics. The implementation is well-structured and production-ready with excellent test coverage and strong adherence to ONEX patterns. Verdict: APPROVE ✅ Strengths1. Excellent Type Safety ✅
2. Comprehensive Documentation 📚
3. Robust Error Handling 🛡️
4. Strong Security Practices 🔒
5. Excellent Test Coverage 🧪
6. Observability & Metrics 📊
Issues Addressed from CodeRabbit Reviews✅ Fixed Issues
Remaining Concerns1.
|
| Policy | Status | Notes |
|---|---|---|
Zero Any types |
✅ PASS | No Any types detected |
| PEP 604 unions | ✅ PASS | Uses `X |
| Pydantic models | ✅ PASS | All data structures are proper models |
| Error handling | ✅ PASS | Uses ModelInfraErrorContext.with_correlation() |
| Correlation ID propagation | ✅ PASS | Implemented throughout (lines 424, 516, 1012, 1098, 1357, 1429) |
| Security patterns | ✅ PASS | Path traversal protection, sanitized errors, bootstrap isolation |
| No backwards compatibility | ✅ PASS | Breaking changes acceptable per CLAUDE.md |
| Strong typing | ✅ PASS | JsonType for envelopes, SecretStr for values |
| Thread safety | ✅ PASS | RLock for metrics, per-key async locks, check-before-write pattern |
Performance Considerations
✅ Strengths:
- Efficient cache rotation:
deque(maxlen=1000)provides O(1) rotation (line 373) - Parallel async resolution:
get_secrets_asyncusesasyncio.gather(line 653) - TTL jitter: Prevents cache stampede with 0-10% jitter (lines 1631-1632)
⚠️ Minor Concerns:
- Sync Vault resolution creates new event loop:
asyncio.run()has overhead (line 1380)- Mitigation: Well-documented limitation (lines 404-409), explicit error when called from async context
Security Review
✅ EXCELLENT Security Posture
- Path traversal protection:
is_relative_to()check for relative paths (lines 1259-1267) - Secret sanitization: File paths never logged, Vault paths never logged (lines 52, 55)
- Bootstrap secret isolation: Prevents circular Vault dependency (lines 1015-1025)
- TOCTOU mitigation: Atomic file reads instead of stat-then-read (lines 1269-1283)
- File size limits: 1MB max to prevent memory exhaustion (lines 1276-1282)
- Error message sanitization: Logical names only, never paths or values (lines 1286-1321)
📋 Security Checklist:
- ✅ No secret leakage in logs
- ✅ Path traversal blocked
- ✅ TOCTOU race mitigated
- ✅ Memory exhaustion prevented (file size limits)
- ✅ Bootstrap secrets isolated
- ✅ Error messages sanitized
Test Coverage Analysis
Unit Tests (test_secret_resolver.py): EXCELLENT
- 15 test classes covering:
- Basic resolution (env, file, convention fallback)
- Required vs optional secrets
- Caching (hit/miss, TTL, expiration, refresh)
- Thread safety (concurrent reads, refresh races, sync/async coordination)
- Async API
- Vault integration (mocked)
- Correlation ID propagation
- Metrics tracking
- Bootstrap secrets
- Security (path traversal, file size limits, symlink loops)
Integration Tests (test_secret_resolver_vault_integration.py): EXCELLENT
- Real Vault integration with:
- Field-specific and first-value extraction
- Correlation ID propagation through Vault calls
- Error handling (not found, missing fields)
- Caching behavior with real Vault latency
- Metrics tracking
- Graceful skipping when Vault unavailable (CI-friendly)
Coverage Gaps: MINOR
- ✅ No significant gaps - edge cases well-covered
- ✅ Thread safety thoroughly tested
- ✅ Security scenarios (path traversal, symlink loops) verified
Recommendations
Must Fix: NONE
All critical issues from previous reviews have been addressed.
Should Fix (Post-Merge):
- Add size limit warning for
_async_key_locksdictionary (Issue feat: PostgreSQL Adapter with Comprehensive Tests and Structured Logging #1) - Validate or reject
"secret#field"Vault path format (Issue Add Claude Code GitHub Workflow #2)
Nice to Have:
- Extract
MAX_LATENCY_SAMPLES = 1000constant (Issue feat: RedPanda Event Bus Integration with Fail-Fast Infrastructure #3) - Elevate metrics collector errors to
warninglevel (Issue feat: Complete infrastructure containers operational with Docker secrets #4) - Document sync Vault resolution event loop cost more prominently in public API
Final Verdict
APPROVED ✅
This is high-quality, production-ready code that demonstrates:
- ✅ Strong adherence to ONEX patterns
- ✅ Excellent security practices
- ✅ Comprehensive test coverage
- ✅ Robust error handling
- ✅ Strong type safety
- ✅ Well-documented edge cases and design decisions
The identified issues are minor enhancements rather than correctness bugs. The implementation is safe to merge and deploy.
Risk Assessment: LOW
- No blocking issues
- Well-tested with real Vault integration
- Security best practices followed
- Thread-safe design verified
Great work on this implementation! 🎉
Branch: jonah/omn-1374-secretresolver-complete-vault-integration-and-add
…solver [OMN-1374] Address remaining review feedback items: 1. DoS mitigation: Add warning when _async_key_locks dictionary reaches 1000 entries to alert operators of potential memory exhaustion 2. Extract MAX_LATENCY_SAMPLES constant (was magic number 1000) for latency deque maxlen 3. Vault path edge case: Log warning when unusual path format detected (e.g., "secret#field" without path component) 4. Metrics collector errors: Change logging from debug to warning level so operators can detect broken metrics collectors
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/omnibase_infra/runtime/secret_resolver.py`:
- Around line 1622-1627: The warning currently logs a raw Vault path via
logger.warning and the extra={"vault_path": path}, which violates the "no Vault
paths in logs" rule; update the logger.warning call in secret_resolver.py (the
block using logger.warning and the path variable) to emit a generic message that
does not include the path or attach it in extra (e.g., mention the expected
'mount/path#field' format and potential KV v2 issue) and remove any usage of the
path variable from the log and extra fields.
PR 167 Review: Vault Integration for SecretResolverExecutive SummaryThis PR implements Vault integration with strong ONEX pattern adherence. Excellent security awareness and comprehensive testing, but THREE CRITICAL ISSUES must be fixed before merge. Critical Issues1. Security: Vault Path Logged (Line 1622-1627)The raw Vault path is logged in edge case, violating the security policy. This reveals secret structure to attackers. MUST mask the path and remove the extra vault_path parameter. 2. DoS Vector: Unbounded Dictionary Growth (Line 614-616)The _async_key_locks dictionary grows without bounds. Attackers controlling logical names can exhaust memory. MUST implement LRU cache with configurable max size. 3. Missing Type Conversion TestsDocstring documents int/bool/dict to string conversion, but NO tests verify this critical behavior. MUST add test cases. High Priority Issues
Strengths
ONEX Compliance: PASS (with security caveats)RecommendationRequest changes for 3 critical issues, then re-review. High-quality implementation overall. Great work on documentation and security considerations! |
…OMN-1374] - Security: Remove vault path from warning log to prevent info disclosure - Typing: Add ModelSecretResolverMetrics Pydantic model for get_resolution_metrics() - Docs: Add thread safety docs to set_metrics_collector() method - Docs: Add container-based DI note to constructor docstring - Tests: Update to use model attributes instead of dict key access All 70 unit tests pass. Type checking clean.
PR Review: SecretResolver Vault Integration [OMN-1374]SummaryThis PR implements comprehensive Vault integration for SecretResolver with observability features. The implementation demonstrates strong engineering practices with detailed documentation, thread safety considerations, and extensive test coverage. ✅ Strengths1. Excellent Documentation
2. Strong Security Practices
3. Thread Safety & Concurrency
4. Observability & Metrics
5. Test Coverage
🔍 Issues & ConcernsCRITICAL: Potential Memory Leak in _async_key_locksLocation: secret_resolver.py:586-637 Issue: The _async_key_locks dictionary grows unboundedly and is never cleaned up. While the docstring acknowledges this, the mitigation is insufficient. Why This is Problematic:
Recommended Fix - Option 1 (LRU eviction): Recommended Fix - Option 2 (Validation): MEDIUM: TTL Jitter TimingLocation: secret_resolver.py:1664-1679 Issue: Jitter is additive only (1.0 to 1.1x). Should be symmetric (±10%) for better cache stampede prevention. Recommendation: MEDIUM: Metrics Protocol Thread Safety DocsLocation: secret_resolver.py:169-227 Issue: Protocol mentions thread safety but doesn't specify which primitives are safe. Recommendation: Add concrete examples of thread-safe implementations (threading.Lock, prometheus_client) 📋 ONEX Pattern Compliance
🎯 RecommendationsMust Fix (Before Merge)
Should Fix (Before Merge)
Nice to Have (Follow-up PR)
🏆 Overall AssessmentGrade: A- (Excellent with minor issues) This is a production-ready implementation with exceptional attention to:
Primary concern: Unbounded _async_key_locks dictionary growth should be addressed before merge. Recommendation: Approve with changes - fix the memory leak, then merge. Great work on this implementation! 🎉 |
…r [OMN-1374] - Add LRU eviction to _async_key_locks to prevent unbounded memory growth - Evicts oldest 10% of entries when reaching MAX_ASYNC_KEY_LOCKS (1000) - Logs warning on eviction as DoS/misconfiguration indicator - Change TTL jitter from additive-only (0-10%) to symmetric (±10%) - Better cache stampede prevention with spread distribution - Add max(1, ...) guard to ensure TTL is never zero - Add thread-safety documentation to ProtocolSecretResolverMetrics - Document recommended primitives (Lock, RLock, Counter, prometheus_client) - Add minimal ThreadSafeSecretResolverMetrics example using threading.Lock - Extract CACHE_TTL_JITTER_PERCENT constant (0.1)
PR Review: Vault Integration for SecretResolver [OMN-1374]SummaryThis PR implements production-ready Vault integration with comprehensive observability features. The implementation is well-architected with strong attention to thread safety, security, and performance. Overall quality is excellent with only minor suggestions for consideration. StrengthsThread Safety - Exceptional
Security - Well Thought Out
Observability - Production Ready
Test Coverage - Thorough
Documentation - Exemplary
Code QualityType Safety ✓
Error Handling ✓
Performance ✓
Minor Suggestions (Non-Blocking)1. Metrics Collector Error HandlingFile: secret_resolver.py:892-902, 954-963 Consider adding circuit breaker for metrics collector if failures become common. Current implementation is acceptable for initial release. 2. Vault Handler Envelope FormatFile: secret_resolver.py:1550-1557 Ensure integration tests verify the envelope format contract with HandlerVault to catch interface changes. 3. String Conversion WarningFile: secret_resolver.py:1624-1638 Consider adding warning log when converting non-string Vault values to alert about Python repr vs JSON. Security AssessmentWell-Designed Security Controls:
No Critical Vulnerabilities Detected ✓ PerformanceEfficient design choices:
Potential bottleneck (low risk): RLock contention minimal due to brief lock hold times. Test Coverage ✓
Optional additional tests:
ONEX Compliance ✓
Final VerdictAPPROVED ✓ This is excellent work with production-ready quality. The implementation demonstrates:
The minor suggestions above are non-blocking and can be addressed in follow-up PRs if needed. Review Checklist
Great work, @jonahgabriel! |
…e contract [OMN-1374] Add comprehensive integration tests verifying the envelope format contract between SecretResolver and HandlerVault. This catches interface drift if either component changes their expected envelope structure. Tests cover: - Envelope structure (operation, payload, correlation_id) - Payload structure (path, mount_point parsing) - Response parsing and field extraction - Malformed response handling - Contract stability documentation
PR Review: Complete Vault Integration for SecretResolver [OMN-1374]Overall AssessmentThis is an excellent, production-ready implementation with exceptional attention to detail. The code demonstrates strong engineering practices with comprehensive error handling, thorough documentation, extensive test coverage, and careful consideration of security, thread safety, and observability. Recommendation: Approve with Minor Suggestions Strengths1. Exceptional Documentation
2. Security Best Practices
3. Thread Safety & Concurrency
4. Observability & Metrics
5. Resilience Patterns
6. Test Coverage
Issues & RecommendationsCRITICAL: Type AnnotationLine 1550 uses JsonType for envelope, but dict[str, object] is more appropriate per CLAUDE.md lines 32-35 (JsonDict pattern when dict key access is needed). HIGH: Missing Container-Based Dependency InjectionConstructor uses explicit parameters instead of ONEX container pattern (lines 398-418). Per CLAUDE.md, should use ModelONEXContainer for DI. Recommendation: Add factory method from_container() for ONEX integration while keeping current constructor for standalone use. MEDIUM: LRU Eviction Logging May Be NoisyLine 694 logs warning on every eviction. In high-throughput systems, this could generate excessive logs. Consider rate limiting (max 1 warning per minute). MEDIUM: Metrics Collector Errors Use WARNING LevelLines 895, 956 log metrics errors at WARNING but these are explicitly ignored. Should use DEBUG level since failures don't affect resolution. Code Quality ObservationsExcellent Patterns
Consistency with ONEX Patterns
Performance Considerations
Security AssessmentStrong security posture with proper SecretStr wrapping, path traversal prevention, sanitized errors, file size limits, bootstrap isolation, and TOCTOU prevention. Recommendations:
ConclusionThis PR demonstrates excellent engineering practices and is ready for production. Action ItemsBefore Merge (Optional but Recommended):
Future Improvements:
Approve: Yes, with minor suggestions Great work! 🎉 |
Address 4 PR review items: 1. CRITICAL - Type annotation: Change envelope type from JsonType to dict[str, object] per CLAUDE.md JsonDict pattern for dict key access 2. HIGH - Container DI: Add from_container() async factory method for ONEX container-based dependency injection while preserving standalone constructor for direct instantiation 3. MEDIUM - Log noise: Rate-limit LRU eviction warnings to max 1 per minute using time.monotonic() to prevent log flooding in high-throughput 4. MEDIUM - Log level: Change metrics collector error logs from WARNING to DEBUG since failures are explicitly ignored and non-fatal
PR Review: SecretResolver Vault Integration [OMN-1374]SummaryThis PR implements production-ready Vault integration for SecretResolver with comprehensive observability features. The implementation is thorough, well-documented, and includes extensive test coverage. However, there are several areas where the code deviates from ONEX infrastructure patterns documented in CLAUDE.md. 🔴 CRITICAL Issues1. Missing Container-Based Dependency Injection PatternLocation: Issue: While the PR adds ONEX Pattern (from CLAUDE.md): # CORRECT - Container injection
class MyOrchestrator(NodeOrchestrator):
def __init__(self, container: ModelONEXContainer) -> None:
super().__init__(container)Current Implementation: # INCORRECT - Direct dependency injection
def __init__(
self,
config: ModelSecretResolverConfig,
vault_handler: HandlerVault | None = None,
metrics_collector: ProtocolSecretResolverMetrics | None = None,
) -> None:Recommendation:
Why This Matters: CLAUDE.md states "All services MUST use ModelONEXContainer for dependency injection." The current pattern creates inconsistency across the codebase and bypasses centralized lifecycle management. 2. Vault Envelope Type Annotation Uses
|
…OMN-1374] - Use JsonType for Vault envelopes instead of dict[str, object] - Fix bootstrap secret resolution to respect explicit env mappings - Elevate metrics collector error log level from debug to warning - Add tests for bootstrap secret explicit mapping behavior
PR Review: Vault Integration for SecretResolver [OMN-1374]Overall AssessmentVerdict: APPROVE with minor recommendations This is a well-architected, production-ready implementation of Vault integration for SecretResolver. The code demonstrates excellent attention to security, observability, thread safety, and testing. Strengths1. Security-First Design ✅
2. Excellent Thread Safety ✅
3. Comprehensive Observability ✅
4. Production-Ready Error Handling ✅
5. Testing Excellence ✅
Architecture HighlightsPath Parsing ExcellenceThe Cache TTL Jitter# ±10% symmetric jitter prevents cache stampede
jitter_factor = random.uniform(-CACHE_TTL_JITTER_PERCENT, CACHE_TTL_JITTER_PERCENT)
ttl_seconds = max(1, int(base_ttl_seconds * (1 + jitter_factor)))Prevents thundering herd when many entries expire simultaneously. DoS Mitigation# Warning threshold + LRU eviction for async key locks
if lock_count >= MAX_ASYNC_KEY_LOCKS:
evict_count = max(1, MAX_ASYNC_KEY_LOCKS // 10) # Evict oldest 10%Rate-limited warnings (max 1/minute) with graceful degradation. Recommendations (All Minor)1. Type Conversion DocumentationLocation: The # Example validation pattern for structured data:
# value_str = resolver.get_secret("config.json")
# config = json.loads(value_str.get_secret_value())Priority: Low - Current documentation is clear. 2. Bootstrap Secret DocumentationLocation: The bootstrap secret isolation is critical but could be more discoverable. Consider adding a prominent note in the class docstring listing which secrets bypass normal resolution. Priority: Low - Already well-documented in method docstrings. 3. Async Context Detection EnhancementLocation: The sync Priority: Very Low - Current behavior is correct. Security Audit✅ Passed
Performance AnalysisStrengths
Potential Concern (Documented)
Test Coverage Analysis
Strengths:
ONEX Compliance Checklist
Final RecommendationAPPROVE ✅ This PR demonstrates:
The minor recommendations are enhancements, not blockers. This code is ready to merge. Code Metrics
Reviewed by: Claude Code |
1 participant
Summary
_resolve_vault_secret()via HandlerVault with path parsing (mount/path#field format)correlation_idparameter toget_secret()andget_secret_async()for distributed tracingChanges
src/omnibase_infra/runtime/secret_resolver.py_resolve_vault_secret()with HandlerVault integrationmount/path#fieldformatcorrelation_idparameter propagation through resolution chainget_resolution_metrics()method for observabilityset_metrics_collector()for external metrics integrationvault_handleris Nonetests/unit/runtime/test_secret_resolver.pyTestSecretResolverVaultIntegrationclass (path parsing, error handling)TestSecretResolverCorrelationIdclass (ID propagation verification)TestSecretResolverMetricsclass (metrics tracking, external collector)tests/integration/runtime/test_secret_resolver_vault_integration.py(new)Test Plan
pytest tests/unit/runtime/test_secret_resolver.py -vpytest tests/integration/runtime/test_secret_resolver_vault_integration.py -vSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.