Repository navigation
refactor(config): externalize hardcoded configuration values [OMN-1058] - #106
Conversation
Move ~20 hardcoded configuration values to environment variables for improved deployment flexibility. All defaults preserved for backward compatibility. Changes: - Handler defaults: ONEX_HTTP_TIMEOUT, ONEX_HTTP_MAX_REQUEST_SIZE, ONEX_HTTP_MAX_RESPONSE_SIZE, ONEX_DB_POOL_SIZE, ONEX_DB_TIMEOUT - Runtime timeouts: ONEX_HEALTH_CHECK_TIMEOUT, ONEX_DRAIN_TIMEOUT, ONEX_HTTP_PORT - Circuit breaker: Add from_env() to ModelCircuitBreakerConfig with ONEX_CB_THRESHOLD and ONEX_CB_RESET_TIMEOUT support - Idempotency store: ONEX_IDEMPOTENCY_TTL_SECONDS, ONEX_IDEMPOTENCY_CLEANUP_INTERVAL, ONEX_IDEMPOTENCY_BATCH_SIZE - Update .env.example with all new environment variables Updated consumers to use ModelCircuitBreakerConfig.from_env(): - projector_registration.py - projection_reader_registration.py - snapshot_publisher_registration.py - service_dlq_tracking.py
📝 WalkthroughWalkthroughEnvironment-driven configuration was added: new parse_env_int/parse_env_float utilities, ModelCircuitBreakerConfig.from_env, many ONEX_* environment variables (with legacy fallbacks), and multiple components (handlers, idempotency, DLQ, projectors, runtime, registry, tests) now derive defaults from environment. Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment (ONEX_*)
participant Parser as parse_env_int/parse_env_float
participant CBConf as ModelCircuitBreakerConfig.from_env
participant Component as Component (Handler/Projector/DLQ/Runtime/Registry)
participant CB as CircuitBreaker
Note over Env,Parser: ONEX_* numeric settings are read & soft-validated (warn + default)
Env->>Parser: GET ONEX_* (timeout/size/cache/...)
Parser-->>Component: return parsed value or default (warn if out-of-range)
Env->>CBConf: GET ONEX_CB_* (threshold/reset_timeout)
CBConf-->>Component: returns ModelCircuitBreakerConfig
Component->>CB: _init_circuit_breaker_from_config(config)
CB-->>Component: circuit-breaker initialized
Note right of Component: Components may call Parser directly for other ONEX_* settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Lite 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/util_*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2025-11-29T22:07:25.230ZApplied to files:
📚 Learning: 2025-12-03T16:55:49.755ZApplied to files:
📚 Learning: 2025-11-29T22:07:25.230ZApplied to files:
📚 Learning: 2025-12-19T02:58:44.081ZApplied to files:
📚 Learning: 2025-11-29T22:07:25.230ZApplied to files:
📚 Learning: 2025-11-29T22:07:25.230ZApplied to files:
🧬 Code graph analysis (1)src/omnibase_infra/utils/util_env_parsing.py (3)
🔇 Additional comments (9)
Comment |
PR Review: Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes ~20 hardcoded configuration values to environment variables, improving deployment flexibility. The implementation is well-documented and maintains backward compatibility. However, there are several critical issues that must be addressed before merging. 🚨 Critical Issues1. Missing Input Validation for Environment VariablesSeverity: HIGH | Security Risk: MEDIUM The code loads environment variables at module import time without validation against documented bounds: # runtime_host_process.py:89-98
DEFAULT_HEALTH_CHECK_TIMEOUT: float = float(
os.environ.get("ONEX_HEALTH_CHECK_TIMEOUT", "5.0")
) # No validation that 1.0 <= value <= 60.0!Impact:
Files affected:
Recommendation: def _validate_float_env(
key: str, default: float, min_val: float, max_val: float
) -> float:
"""Validate float environment variable against bounds."""
value = float(os.environ.get(key, str(default)))
if not min_val <= value <= max_val:
raise ValueError(
f"{key}={value} out of range [{min_val}, {max_val}]"
)
return value
DEFAULT_HEALTH_CHECK_TIMEOUT = _validate_float_env(
"ONEX_HEALTH_CHECK_TIMEOUT", 5.0, 1.0, 60.0
)This ensures fail-fast behavior at startup rather than silent misconfiguration. 2. No Exception Handling for Type ConversionSeverity: HIGH | Reliability Risk: HIGH
# If user sets ONEX_DB_POOL_SIZE="large"
_DEFAULT_POOL_SIZE: int = int(os.environ.get("ONEX_DB_POOL_SIZE", "5"))
# Raises: ValueError: invalid literal for int() with base 10: 'large'Impact:
Recommendation: try:
_DEFAULT_POOL_SIZE = int(os.environ.get("ONEX_DB_POOL_SIZE", "5"))
except ValueError as e:
raise ValueError(
f"Invalid ONEX_DB_POOL_SIZE: must be integer, got {os.environ.get('ONEX_DB_POOL_SIZE')}"
) from eOr use the validation function pattern from Issue #1. 3. Missing Test Coverage for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (11)
.env.examplesrc/omnibase_infra/dlq/service_dlq_tracking.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/projectors/projection_reader_registration.pysrc/omnibase_infra/projectors/projector_registration.pysrc/omnibase_infra/projectors/snapshot_publisher_registration.pysrc/omnibase_infra/runtime/health_server.pysrc/omnibase_infra/runtime/runtime_host_process.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/projectors/projection_reader_registration.pysrc/omnibase_infra/projectors/snapshot_publisher_registration.pysrc/omnibase_infra/runtime/health_server.pysrc/omnibase_infra/projectors/projector_registration.pysrc/omnibase_infra/runtime/runtime_host_process.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/dlq/service_dlq_tracking.py
**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/model_*.py: All data structures must be proper Pydantic models - one model per file with naming pattern model_.py and class pattern Model
Result models may override bool to enable idiomatic conditional checks, with Warning section in docstring explaining non-standard behavior
Files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.py
**/service_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Service files must follow naming pattern service_.py with class pattern Service
Files:
src/omnibase_infra/dlq/service_dlq_tracking.py
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
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 : Use type-safe configuration via Pydantic Settings from config/settings.py with 90+ type-safe variables organized into External Service Discovery, Shared Infrastructure, AI Provider API Keys, Local Services, and Feature Flags
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/runtime/runtime_host_process.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For all backend service HTTP calls, use HTTP/2 connection pooling with max connections (100 total, 20 keepalive), timeouts (5s connect, 10s read, 5s write), and retry logic with exponential backoff (3 attempts max, 1s→2s→4s).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/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/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
src/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 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_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : When extracting code entities, patterns, or performing semantic analysis, use the configuration from `config/timeout_config.py` for any I/O operations. Set appropriate timeouts for ML feature extraction (typically 10-30 seconds).
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/runtime/runtime_host_process.pysrc/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/**/{config,settings}/**/*.py : Environment variables MUST include: POSTGRES_HOST, POSTGRES_PORT, POSTGRES_DATABASE, POSTGRES_USER, POSTGRES_PASSWORD, KAFKA_BOOTSTRAP_SERVERS, CONSUL_HOST, CONSUL_PORT, LOG_LEVEL. Use secrets manager for production passwords.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Applied to files:
src/omnibase_infra/projectors/projection_reader_registration.pysrc/omnibase_infra/projectors/projector_registration.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/dlq/service_dlq_tracking.py
📚 Learning: 2025-12-27T15:46:10.813Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_core PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:46:10.813Z
Learning: Applies to src/omnibase_core/infrastructure/**/*.py : Use mixins (MixinDiscoveryResponder, MixinEventHandler, etc.) to add reusable capabilities to nodes
Applied to files:
src/omnibase_infra/projectors/projection_reader_registration.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/omninode_bridge/nodes/**/*.py : All nodes in omninode_bridge MUST use omnibase_core standards (ModelServiceEffect, ModelServiceCompute for effect/compute nodes; NodeOrchestrator, NodeReducer with mixins for orchestrator/reducer nodes)
Applied to files:
src/omnibase_infra/projectors/projection_reader_registration.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Applied to files:
src/omnibase_infra/projectors/projector_registration.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/dlq/service_dlq_tracking.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-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Applied to files:
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*dispatcher*.py : Use dispatcher-owned resilience pattern - MessageDispatchEngine does NOT wrap dispatchers with circuit breakers, each dispatcher must implement its own MixinAsyncCircuitBreaker
Applied to files:
src/omnibase_infra/dlq/service_dlq_tracking.py
🧬 Code graph analysis (5)
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)
src/omnibase_infra/projectors/projection_reader_registration.py (3)
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (2)
ModelCircuitBreakerConfig(42-205)from_env(138-205)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/mixins/mixin_async_circuit_breaker.py (1)
_init_circuit_breaker_from_config(282-324)
src/omnibase_infra/projectors/projector_registration.py (3)
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (2)
ModelCircuitBreakerConfig(42-205)from_env(138-205)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/mixins/mixin_async_circuit_breaker.py (1)
_init_circuit_breaker_from_config(282-324)
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (1)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
src/omnibase_infra/dlq/service_dlq_tracking.py (3)
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (2)
ModelCircuitBreakerConfig(42-205)from_env(138-205)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/mixins/mixin_async_circuit_breaker.py (1)
_init_circuit_breaker_from_config(282-324)
🔇 Additional comments (7)
.env.example (1)
248-302: LGTM! Comprehensive documentation for new environment variables.The new configuration sections are well-organized and include:
- Clear descriptions of each variable's purpose
- Sensible default values that match the code
- Valid ranges where applicable
- Context about how the settings affect system behavior
This documentation will help operators tune the system without needing to read the source code.
src/omnibase_infra/projectors/projection_reader_registration.py (1)
105-109: LGTM! Circuit breaker configuration properly externalized.The configuration approach is consistent with other projector components and uses appropriate service identification for error context.
src/omnibase_infra/projectors/projector_registration.py (1)
96-100: LGTM! Consistent circuit breaker externalization.The change aligns with the broader PR pattern and maintains proper service identification for the projector component.
src/omnibase_infra/projectors/snapshot_publisher_registration.py (1)
219-223: LGTM! Circuit breaker configuration uses appropriate transport type.The configuration correctly uses
EnumInfraTransportType.KAFKAfor the Kafka producer, and the service name dynamically includes the topic for better observability.src/omnibase_infra/runtime/health_server.py (1)
36-36: LGTM! Environment-driven port configuration implemented correctly.The addition of
osimport and conversion ofDEFAULT_HTTP_PORTto read fromONEX_HTTP_PORTenvironment variable aligns with the PR objectives to externalize configuration. The implementation:
- Preserves backward compatibility with default "8085"
- Uses explicit type annotation (
int)- Safely casts the string value with
int()which will raiseValueErroron invalid inputAlso applies to: 51-51
src/omnibase_infra/runtime/runtime_host_process.py (1)
41-41: LGTM! Timeout configuration externalized correctly.The conversion of
DEFAULT_HEALTH_CHECK_TIMEOUTandDEFAULT_DRAIN_TIMEOUT_SECONDSto environment-driven values is well-implemented:
- Preserves backward compatibility with defaults "5.0" and "30.0"
- Uses explicit type annotations (
float)- Safely casts string values with
float()which will raiseValueErroron invalid input- Bounds validation and clamping already handled in
__init__(lines 238-301)This follows the established pattern for this repository where configuration is injected via environment variables from Kubernetes ConfigMaps/Secrets.
Also applies to: 89-91, 97-99
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (1)
35-35: LGTM! Well-designed environment-driven factory method.The
from_env()classmethod is an excellent addition that:
- Enables runtime configuration via
ONEX_CB_THRESHOLDandONEX_CB_RESET_TIMEOUT- Preserves backward compatibility with sensible defaults (5 and 60.0)
- Correctly leaves
service_nameandtransport_typeas method parameters (context-specific, not env-driven)- Documents error behavior (
ValueErroron parse failure)- Includes comprehensive examples and usage guidance
- Leverages Pydantic validation on the returned instance (Field constraints automatically enforced)
This follows the deployment pattern for this repository where configuration is injected via environment variables from Kubernetes ConfigMaps/Secrets. Based on learnings, this is the preferred approach over Pydantic Settings for this codebase.
Also applies to: 137-205
… [OMN-1058] Address PR #106 review feedback: - Add error handling for invalid environment variable values in: - handler_db.py: _parse_env_int/_parse_env_float helpers - handler_http.py: _parse_env_int/_parse_env_float helpers - model_postgres_idempotency_store_config.py: _parse_env_int helper - model_circuit_breaker_config.py: from_env() method - All env var parsing now raises ProtocolConfigurationError with proper ModelInfraErrorContext instead of raw ValueError - Fix inconsistent environment variable naming (12 vars renamed): - RUNTIME_SCHEDULER_* -> ONEX_RUNTIME_SCHEDULER_* - COMPUTE_REGISTRY_CACHE_SIZE -> ONEX_COMPUTE_REGISTRY_CACHE_SIZE - CONTRACTS_DIR -> ONEX_CONTRACTS_DIR - Add comprehensive test coverage for ModelCircuitBreakerConfig.from_env() (42 new tests with 100% coverage) - Add tests for HTTP handler env var parsing (9 new tests)
PR Review: Externalize Hardcoded Configuration ValuesOverall Assessment: ✅ Strong implementation with excellent test coverage and adherence to ONEX patterns. The PR successfully externalizes ~20 hardcoded configuration values while maintaining backward compatibility. Strengths1. Excellent Security Practices 🔒
2. Comprehensive Test Coverage 🧪
3. Backward Compatibility ✅
4. ONEX Pattern Adherence 📋
5. Code Quality ✨
Issues Found🔴 Critical: Missing Range Validation in HTTP HandlerLocation: The HTTP handler's Problem: # handler_http.py - NO range validation
def _parse_env_float(env_var: str, default: float) -> float:
raw_value = os.environ.get(env_var)
if raw_value is None:
return default
try:
return float(raw_value)
except ValueError:
raise ProtocolConfigurationError(...)Impact:
Recommendation: Add range validation with warning logs like def _parse_env_int(
env_var: str,
default: int,
*,
min_value: int | None = None,
max_value: int | None = None,
) -> int:
# ... parse logic ...
if min_value is not None and parsed < min_value:
logger.warning("...")
return default
# ... etcThen use it: _DEFAULT_TIMEOUT_SECONDS: float = _parse_env_float(
"ONEX_HTTP_TIMEOUT", 30.0, min_value=0.1, max_value=3600.0
)
_DEFAULT_MAX_REQUEST_SIZE: int = _parse_env_int(
"ONEX_HTTP_MAX_REQUEST_SIZE", 10 * 1024 * 1024, min_value=1024, max_value=1073741824
)🟡 Moderate: Inconsistent Parameter NamingLocation: The # model_circuit_breaker_config.py - Uses 'parameter' and 'value' kwargs
raise ProtocolConfigurationError(
"...",
context=context,
parameter=threshold_var, # ← named param
value="[REDACTED]", # ← named param
)
# handler_db.py - Only uses 'parameter' and 'value' in context dict
raise ProtocolConfigurationError(
"...",
context=context,
parameter=env_var, # ← named param
value="[REDACTED]", # ← named param
)Question: Are Recommendation: Verify the error model signature and ensure consistent usage across all handlers. 🟡 Moderate: Missing Test Coverage for HTTP Handler Environment ParsingLocation: While there are 577 lines of tests for Missing test cases:
Recommendation: Add a test class 🟢 Minor: Code Duplication OpportunityLocation: Both handlers implement their own Recommendation: Consider extracting these to a shared module (e.g., 🟢 Minor: Documentation EnhancementLocation: The documentation is clear, but could benefit from examples of valid values: # SUGGESTION - Add example values
# HTTP handler timeout in seconds (default: 30.0, range: 0.1-3600.0)
# Examples: 30.0 (default), 60.0 (1 minute), 300.0 (5 minutes)
# ONEX_HTTP_TIMEOUT=30.0
# Request body size limit in bytes (default: 10485760 = 10MB)
# Examples: 1048576 (1MB), 10485760 (10MB), 52428800 (50MB)
# ONEX_HTTP_MAX_REQUEST_SIZE=10485760Security Analysis✅ No Security Vulnerabilities Detected
✅ Security Best Practices Followed
Performance Considerations✅ Minimal Performance Impact
Code Quality Metrics
Recommendations SummaryRequired Before Merge
Suggested Improvements (Non-Blocking)
Final VerdictStatus: ✅ APPROVE with Minor Changes Required This PR demonstrates excellent engineering practices with strong security, comprehensive testing, and proper ONEX pattern adherence. The only blocking issue is the missing range validation in the HTTP handler, which should be added to prevent invalid configuration values. The implementation successfully achieves the goal of externalizing hardcoded values while maintaining backward compatibility. Once the range validation is added to the HTTP handler and corresponding tests are written, this PR will be ready to merge. Estimated Effort for Fixes: ~30 minutes
Great work! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/omnibase_infra/handlers/handler_http.py (1)
104-110: Environment-driven defaults with backward compatibility.The defaults (30s timeout, 10MB request limit, 50MB response limit) are preserved while enabling runtime configuration. This follows the repository pattern of using
os.getenv()with sensible defaults per the learnings.
🧹 Nitpick comments (6)
src/omnibase_infra/runtime/kernel.py (1)
21-31: Align docs and optionally support legacyCONTRACTS_DIR.Code now reads
ONEX_CONTRACTS_DIR(Line 330) but thebootstrap()docstring’s Environment Variables section still documentsCONTRACTS_DIRand notONEX_CONTRACTS_DIR. That can confuse operators and obscure why a custom contracts path stopped working.Consider:
- Updating the
bootstrap()docstring to documentONEX_CONTRACTS_DIRconsistently with the top-of-file usage block, and- (Optional but safer) treating
CONTRACTS_DIRas a deprecated fallback, e.g.os.getenv("ONEX_CONTRACTS_DIR") or os.getenv("CONTRACTS_DIR") or DEFAULT_CONTRACTS_DIR, to smooth the migration for existing deployments.Also applies to: 295-303, 328-335
tests/unit/runtime/test_runtime_scheduler.py (1)
329-388: Tests correctly track ONEX_ env var rename; optional extra coverage.*The env override tests now use
ONEX_RUNTIME_SCHEDULER_*and still validate the same fields, so they guard the new mapping well. If you want to tighten coverage later, you could add small cases forONEX_RUNTIME_SCHEDULER_SEQUENCE_KEYandONEX_RUNTIME_SCHEDULER_METRICS_PREFIX, but not required for this PR.src/omnibase_infra/runtime/registry_compute.py (1)
151-156: Update docs to ONEX_COMPUTE_REGISTRY_CACHE_SIZE and consider legacy fallback.Code now reads cache size from
ONEX_COMPUTE_REGISTRY_CACHE_SIZEviaENV_COMPUTE_REGISTRY_CACHE_SIZE, but the class docstring and “Environment Variables” section still documentCOMPUTE_REGISTRY_CACHE_SIZE. That’s inconsistent and can mislead operators.Suggestions:
- Update the docstrings and attribute docs to reference
ONEX_COMPUTE_REGISTRY_CACHE_SIZE.- (Optional) For smoother rollout, read the legacy name as a fallback, e.g.
os.environ.get("ONEX_COMPUTE_REGISTRY_CACHE_SIZE") or os.environ.get("COMPUTE_REGISTRY_CACHE_SIZE").Also applies to: 239-255, 274-278
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py (1)
28-79: Env-driven idempotency defaults look correct; consider deduplicating helper and imports.The new
_parse_env_int+_DEFAULT_*constants correctly move TTL/cleanup defaults toONEX_IDEMPOTENCY_TTL_SECONDS,ONEX_IDEMPOTENCY_CLEANUP_INTERVAL, andONEX_IDEMPOTENCY_BATCH_SIZE, and they now raiseProtocolConfigurationErrorwith DATABASE context on invalid values, which aligns with the infra error guidelines. Based on learnings, this is a solid improvement over bareint()at import.Two small polish points you may want to address later:
- The helper is very similar to the
_parse_env_intused in HTTP/DB handlers but with slightly different semantics (string default, hard fail vs. warning+fallback). If you want more consistency, consider centralizing these into a small shared utility with explicit “strict vs. lenient” modes.- Inside
_parse_env_intyou re-importEnumInfraTransportType,ProtocolConfigurationError, andModelInfraErrorContextunder aliases, even though they’re already imported at the module top. Unless there’s a known circular-import issue, you could drop the deferred imports and use the existing symbols to simplify the code.Also applies to: 225-238, 255-258
src/omnibase_infra/handlers/handler_db.py (1)
59-76: DB env parsing helpers and defaults look solid; update docs to reflect configurability.The new
_parse_env_int/_parse_env_floathelpers correctly moveONEX_DB_POOL_SIZEandONEX_DB_TIMEOUTinto environment-driven defaults with:
- Proper
ProtocolConfigurationErroron non-numeric values, using DATABASEModelInfraErrorContextand redacting the invalid value.- Range checks that log a warning and fall back to safe defaults when out of bounds.
This aligns well with the “use ProtocolConfigurationError for invalid configuration scenarios” and infra-context guidelines. Based on learnings.
The remaining gap is documentation:
- The module/class docs and comments still describe a “fixed pool size (5)” and “configurable pool size deferred to Beta”, but
_DEFAULT_POOL_SIZEis now configurable viaONEX_DB_POOL_SIZE.ONEX_DB_TIMEOUTis not mentioned anywhere in this file’s docs.It would be good to:
- Update the top-level docstring/comments to describe the env-driven pool size and timeout, and
- Optionally add a small “Environment Variables” note (e.g.,
ONEX_DB_POOL_SIZE,ONEX_DB_TIMEOUT) for discoverability.Also applies to: 91-160, 162-244
tests/unit/models/resilience/test_model_circuit_breaker_config.py (1)
63-83: Consider using specific Pydantic exception type.The tests correctly verify that validation fails, but using
pytest.raises(Exception)is less precise than usingpydantic.ValidationError. This would make the tests more explicit about expected behavior.🔎 Proposed improvement
+from pydantic import ValidationError + def test_model_is_frozen(self) -> None: """Test model is immutable (frozen).""" config = ModelCircuitBreakerConfig() - with pytest.raises(Exception): # Pydantic ValidationError for frozen + with pytest.raises(ValidationError): config.threshold = 10 # type: ignore[misc] def test_threshold_minimum_validation(self) -> None: """Test threshold must be >= 1.""" - with pytest.raises(Exception): # Pydantic ValidationError + with pytest.raises(ValidationError): ModelCircuitBreakerConfig(threshold=0)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (11)
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/runtime/models/model_runtime_scheduler_config.pysrc/omnibase_infra/runtime/registry_compute.pytests/unit/handlers/test_handler_http.pytests/unit/models/resilience/__init__.pytests/unit/models/resilience/test_model_circuit_breaker_config.pytests/unit/runtime/test_runtime_scheduler.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/models/resilience/init.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
src/omnibase_infra/runtime/registry_compute.pytests/unit/models/resilience/test_model_circuit_breaker_config.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pytests/unit/handlers/test_handler_http.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/handlers/handler_db.pytests/unit/runtime/test_runtime_scheduler.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/runtime/models/model_runtime_scheduler_config.py
**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/model_*.py: All data structures must be proper Pydantic models - one model per file with naming pattern model_.py and class pattern Model
Result models may override bool to enable idiomatic conditional checks, with Warning section in docstring explaining non-standard behavior
Files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/runtime/models/model_runtime_scheduler_config.py
🧠 Learnings (28)
📓 Common learnings
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Applied to files:
tests/unit/models/resilience/test_model_circuit_breaker_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.py
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/unit/models/**/test_model_*.py : Model tests must achieve 100% coverage and test instantiation, inheritance, serialization, deserialization, JSON serialization, roundtrip serialization, equality, hashing, string representation, repr, attributes, validation, metadata, data creation, copying, and immutability
Applied to files:
tests/unit/models/resilience/test_model_circuit_breaker_config.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 tests/**/*.py : Write comprehensive test coverage following the test structure under `tests/unit/` organized by subsystem (enums, models, mixins, utils)
Applied to files:
tests/unit/models/resilience/test_model_circuit_breaker_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/runtime/models/model_runtime_scheduler_config.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/**/{config,settings}/**/*.py : Environment variables MUST include: POSTGRES_HOST, POSTGRES_PORT, POSTGRES_DATABASE, POSTGRES_USER, POSTGRES_PASSWORD, KAFKA_BOOTSTRAP_SERVERS, CONSUL_HOST, CONSUL_PORT, LOG_LEVEL. Use secrets manager for production passwords.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use ProtocolConfigurationError for invalid configuration scenarios
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-12-19T02:58:44.081Z
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_cli.yaml : All ONEX node CLI interface definitions, if applicable, must be included in contract_cli.yaml with entrypoint and commands specifications
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract*.yaml : All ONEX node contract definitions must reference shared schemas using project root paths (e.g., 'schemas/...' or 'omnibase/schemas/...') rather than relative paths
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For all backend service HTTP calls, use HTTP/2 connection pooling with max connections (100 total, 20 keepalive), timeouts (5s connect, 10s read, 5s write), and retry logic with exponential backoff (3 attempts max, 1s→2s→4s).
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.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-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/node_tests/**/*.py : All ONEX node tests must be organized in a `node_tests/` directory using scenario-driven testing patterns with fixture-injected tests
Applied to files:
tests/unit/runtime/test_runtime_scheduler.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use ONEX field naming conventions: {entity}_id for identifiers, {entity}_type for type discriminators, *_at for timestamps, *_ms for durations in milliseconds, *_count for counts, *_score for scores (0.0-1.0), *_enabled for boolean feature flags, is_* for boolean state checks, has_* for boolean presence checks
Applied to files:
tests/unit/runtime/test_runtime_scheduler.pysrc/omnibase_infra/runtime/models/model_runtime_scheduler_config.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Applied to files:
src/omnibase_infra/models/resilience/model_circuit_breaker_config.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_http.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to {services/**/*.py,scripts/bulk_ingest_repository.py} : Implement fail-closed configuration for security hardening. All external requests must validate URLs, implement DLQ routing, and handle failures gracefully.
Applied to files:
src/omnibase_infra/handlers/handler_http.py
🧬 Code graph analysis (5)
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)
src/omnibase_infra/handlers/handler_db.py (4)
src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)src/omnibase_infra/handlers/handler_http.py (2)
_parse_env_int(71-101)_parse_env_float(38-68)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
tests/unit/runtime/test_runtime_scheduler.py (1)
src/omnibase_infra/runtime/models/model_runtime_scheduler_config.py (2)
ModelRuntimeSchedulerConfig(94-553)default(532-553)
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)
src/omnibase_infra/handlers/handler_http.py (3)
src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)
🔇 Additional comments (13)
src/omnibase_infra/runtime/models/model_runtime_scheduler_config.py (1)
20-68: ONEX-prefixed scheduler env vars and mappings look consistent.The docs and
env_mappingsnow consistently useONEX_RUNTIME_SCHEDULER_*, and override behavior (int/float parsing, boolean handling, defaults) is preserved. This is a clean, backwards-compatible rename at the model layer.Also applies to: 420-451
tests/unit/handlers/test_handler_http.py (1)
2092-2257: Env-var parsing tests are thorough and correctly target handler_http helpers.The new
TestHttpRestHandlerEnvVarParsingsuite exercises both success and failure paths for_parse_env_floatand_parse_env_int, including error messages andModelInfraErrorContextcontents (operation="parse_env_config",target_name="http_handler"). This gives good confidence that misconfigured HTTP env vars surface asProtocolConfigurationErrorinstead of rawValueError, and the__all__update keeps discovery consistent.src/omnibase_infra/handlers/handler_http.py (3)
38-68: Well-structured environment parsing with proper error handling.The helper function correctly:
- Returns the default when the env var is not set
- Raises
ProtocolConfigurationErrorwith completeModelInfraErrorContext(transport_type, operation, target_name, correlation_id) on parse failureThis addresses the previous review feedback about adding error handling for invalid environment variable values.
71-101: LGTM!The integer parsing helper follows the same robust pattern as the float parser, with clear error messaging.
118-143: Good security practice for sanitized logging.The size categorization prevents exact payload sizes from being exposed in error messages and logs, which helps prevent attackers from probing size limits. The thresholds are well-chosen and documented.
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (3)
52-65: Lazy import pattern to avoid circular dependencies.The
_get_error_classes()helper defers imports until runtime, preventing circular import issues while maintaining type safety viaTYPE_CHECKING. This is a clean approach when model files need to reference error classes that themselves may depend on models.
229-261: Solid error handling with proper context and value redaction.The error handling correctly:
- Uses
ProtocolConfigurationErrorper coding guidelines- Includes complete
ModelInfraErrorContextwith transport_type, operation, target_name, and auto-generated correlation_id- Redacts the actual value (
"[REDACTED]") to prevent sensitive data leakage- Chains the original
ValueErrorfor debuggingOne observation: if a parsed value is valid syntactically but fails Pydantic validation (e.g.,
THRESHOLD=0), users will see a PydanticValidationErrorrather thanProtocolConfigurationError. This is acceptable since the tests document this behavior, but worth noting for consistency.
263-268: LGTM!The return statement correctly constructs the config with environment-derived values while allowing Pydantic to enforce field constraints.
tests/unit/models/resilience/test_model_circuit_breaker_config.py (5)
1-34: Well-structured comprehensive test suite.The test organization with clear class groupings (Basics, FromEnv, FromEnvErrors, FromEnvErrorContext, EdgeCases) and detailed docstrings makes the test suite easy to navigate and understand. The coverage goals are clearly documented.
86-175: Thorough environment variable testing with proper isolation.The tests use
patch.dict(os.environ, {...}, clear=True)which ensures complete isolation between tests. Coverage includes default prefix (ONEX_CB), custom prefixes, and verification that custom prefixes ignore default variables.
218-337: Comprehensive error case coverage.The error handling tests cover important scenarios: invalid values, empty strings, whitespace, type mismatches (float for int), special characters, and custom prefix error messages. This ensures users get clear feedback when configuration is invalid.
339-484: Thorough error context validation.These tests verify that error context includes all required fields (transport_type, operation, target_name, correlation_id) per coding guidelines. The security-focused test at line 404-416 confirms that invalid values are redacted as
"[REDACTED]"rather than exposed.
486-577: Good edge case coverage including all transport types.The edge case tests cover important scenarios like very large values, scientific notation, zero timeout, and all eight transport types. The parametric approach for transport types (lines 543-562) ensures complete coverage.
Note: Lines 500 and 510 use generic
Exceptionsimilar to earlier tests; same optional improvement applies.
…OMN-1058] Address PR #106 review feedback: - Add min/max range validation to HTTP handler env parsers - Extract shared env parsing utilities to util_env_parsing.py - Add legacy env var fallback for CONTRACTS_DIR and COMPUTE_REGISTRY_CACHE_SIZE - Update .env.example with range documentation - Add 14 new tests for range validation
Merge main into feature branch, resolving conflicts in: - kernel.py: Keep new ENV_CONTRACTS_DIR constants in __all__ - utils/__init__.py: Merge __all__ exports, keeping parse_env_* functions
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/omnibase_infra/handlers/handler_http.py (1)
38-171: Code duplication with centralized utilities.The
_parse_env_floatand_parse_env_inthelper functions duplicate the logic insrc/omnibase_infra/utils/util_env_parsing.py. Consider using the centralized utilities instead:from omnibase_infra.utils import parse_env_float, parse_env_int _DEFAULT_TIMEOUT_SECONDS: float = parse_env_float( "ONEX_HTTP_TIMEOUT", 30.0, min_value=0.1, max_value=3600.0, transport_type=EnumInfraTransportType.HTTP, service_name="http_handler" )The centralized utilities provide the same functionality with additional
transport_typeandservice_nameparameters for richer error context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (7)
.env.examplesrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/runtime/registry_compute.pysrc/omnibase_infra/utils/__init__.pysrc/omnibase_infra/utils/util_env_parsing.pytests/unit/handlers/test_handler_http.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/handlers/test_handler_http.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
src/omnibase_infra/utils/__init__.pysrc/omnibase_infra/utils/util_env_parsing.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/runtime/registry_compute.pysrc/omnibase_infra/runtime/kernel.py
**/util_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Utility files must follow naming pattern util_.py containing utility functions
Files:
src/omnibase_infra/utils/util_env_parsing.py
🧠 Learnings (26)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Applied to files:
src/omnibase_infra/utils/util_env_parsing.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must support optional documents pattern with optional flag and required_capability field for future extensibility
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_capabilities.yaml : All ONEX node execution capability definitions, if applicable, must be included in contract_capabilities.yaml with supported_node_types, supported_delivery_modes, and performance_constraints specifications
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX node deviations from canonical patterns must be documented and justified in the node's root-level README.md and subject to maintainer review
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.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: Applies to **/*contract*.yaml : All ONEX nodes must have validated YAML contracts following the contract-driven development pattern with input_state and output_state schema definitions
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_cli.yaml : All ONEX node CLI interface definitions, if applicable, must be included in contract_cli.yaml with entrypoint and commands specifications
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : All Kafka topics must use the prefix `dev.archon-intelligence` for development/staging environments.
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For all backend service HTTP calls, use HTTP/2 connection pooling with max connections (100 total, 20 keepalive), timeouts (5s connect, 10s read, 5s write), and retry logic with exponential backoff (3 attempts max, 1s→2s→4s).
Applied to files:
src/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_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use ProtocolConfigurationError for invalid configuration scenarios
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to {services/**/*.py,scripts/bulk_ingest_repository.py} : Implement fail-closed configuration for security hardening. All external requests must validate URLs, implement DLQ routing, and handle failures gracefully.
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/registry/registry_*.py : Registry classes must inherit from BaseOnexRegistry and define CANONICAL_TOOLS dictionary with default tool implementations
Applied to files:
src/omnibase_infra/runtime/registry_compute.py
📚 Learning: 2025-12-19T02:58:44.081Z
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
src/omnibase_infra/runtime/kernel.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: All ONEX nodes must conform to the 4-Node Architecture pattern with clear separation of concerns and unidirectional data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR)
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/{tools,registry}/**.py : Constructor-based dependency injection must be used for all tool and node classes; validate that required dependencies are not None, raising OnexError with specific error code if missing
Applied to files:
src/omnibase_infra/runtime/kernel.py
🧬 Code graph analysis (3)
src/omnibase_infra/utils/__init__.py (1)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)
src/omnibase_infra/utils/util_env_parsing.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)
src/omnibase_infra/handlers/handler_http.py (3)
src/omnibase_infra/models/errors/model_infra_error_context.py (1)
ModelInfraErrorContext(17-96)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)
🔇 Additional comments (13)
.env.example (2)
250-304: Well-structured environment variable documentation.The new configuration sections follow a consistent pattern with clear headers, range documentation, and sensible defaults. The inline comments explaining the purpose of each variable (e.g., circuit breaker behavior, idempotency store cleanup) are helpful for operators configuring production deployments.
195-196: Good backward compatibility documentation.The legacy variable notes appropriately guide operators toward the new
ONEX_prefixed variables while maintaining backward compatibility. This aligns with the PR's goal of preserving defaults for existing deployments.Also applies to: 233-233
src/omnibase_infra/runtime/kernel.py (2)
101-128: Clean implementation of legacy fallback with logging.The
_get_contracts_dir()function correctly implements the priority order:ONEX_CONTRACTS_DIR→CONTRACTS_DIR(legacy) → default. The info-level log when the legacy variable is used provides visibility for migration tracking without being overly noisy.
88-91: Constants properly exported for external reference.Exporting
ENV_CONTRACTS_DIRandENV_CONTRACTS_DIR_LEGACYin__all__allows other modules to reference the canonical environment variable names, avoiding magic strings scattered across the codebase.Also applies to: 829-836
src/omnibase_infra/utils/__init__.py (1)
19-22: Proper re-export of new utilities.The new
parse_env_intandparse_env_floatutilities are correctly imported and added to__all__, making them available through theomnibase_infra.utilsnamespace consistent with other utilities likegenerate_correlation_id.Also applies to: 39-40
src/omnibase_infra/handlers/handler_http.py (2)
174-182: Import-time environment parsing with fail-fast behavior.The module-level defaults are parsed at import time with proper error handling. Invalid environment values will raise
ProtocolConfigurationErrorearly, which is intentional fail-fast behavior appropriate for configuration errors. This addresses the previous review feedback about adding error handling.
196-215: Good security practice with size categorization.The
_categorize_sizehelper prevents exact payload sizes from being exposed in error messages and logs, which could help attackers probe size limits. This follows ONEX security guidelines for sanitized error context.src/omnibase_infra/utils/util_env_parsing.py (3)
117-131: Appropriate use offrom Nonefor exception suppression.Using
raise ... from Nonehere is intentional to suppress theValueErrorexception chain, producing cleaner error messages for operators. The redacted value and context provide sufficient debugging information without exposing the invalid input.
133-152: Clear distinction between parse errors and range violations.The design correctly distinguishes between parse failures (which raise
ProtocolConfigurationError) and range violations (which log a warning and fall back to the default). This provides graceful degradation for misconfigured-but-parseable values while failing fast for malformed input.Also applies to: 233-252
1-42: Well-documented utility module following conventions.The module docstring provides comprehensive usage examples and explicitly documents the security note about value redaction. The file correctly follows the
util_<name>.pynaming pattern required by the coding guidelines.src/omnibase_infra/runtime/registry_compute.py (3)
152-155: LGTM! Clear naming for primary and legacy environment variables.The environment variable constants follow the ONEX_ prefix convention and the comments clearly document the fallback behavior for backward compatibility.
271-279: Documentation updates look good.The comments and docstring properly document the new
ONEX_COMPUTE_REGISTRY_CACHE_SIZEenvironment variable, fallback behavior, and default value. The class-level assignment at Line 309 correctly uses the new_get_compute_registry_cache_size()function.Note: The class variable is initialized at module import time, so any configuration errors in
_get_compute_registry_cache_size()will surface during import (see previous comment about error handling).Also applies to: 286-286, 306-309
1073-1073: LGTM! Module exports updated correctly.The legacy environment variable constant is properly exported alongside the primary one, maintaining a clean public API.
Code Review: Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes 12+ hardcoded configuration values to environment variables, improving deployment flexibility. The implementation follows ONEX patterns with comprehensive test coverage and proper error handling. ✅ Strengths1. Excellent Centralized UtilitiesThe new
# Good: Centralized, reusable, well-documented
def parse_env_int(
env_var: str,
default: int,
*,
min_value: int | None = None,
max_value: int | None = None,
transport_type: EnumInfraTransportType | None = None,
service_name: str = "unknown",
) -> int:
"""Parse an integer environment variable with validation..."""2. Comprehensive Test Coverage
3. Security Best Practices
4. Backward Compatibility
|
…ies [OMN-1058] Address PR review feedback by eliminating duplicate env parsing code: - Remove local _parse_env_int/_parse_env_float from handler_db.py (~140 lines) - Remove local _parse_env_int/_parse_env_float from handler_http.py (~135 lines) - Remove local _parse_env_int from model_postgres_idempotency_store_config.py - All modules now use centralized parse_env_int/parse_env_float from util_env_parsing - Add error handling and range validation (1-10000) to registry_compute.py - Update .env.example with legacy env var documentation and ONEX_ preferred naming - Update tests to use centralized utilities
Include both DSN validation utilities (from main) and env parsing utilities (from this branch) in utils/__init__.py exports.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/unit/handlers/test_handler_http.py (1)
2301-2644: Range‑validation coverage for parse_env_ is thorough.*Below/above/within‑range, None‑bound, and boundary tests plus warning assertions give good confidence in min/max behavior for both int and float env values. Only nit: if you rely on
pytest -m unit, consider also markingTestHttpRestHandlerEnvVarParsingfor consistency.src/omnibase_infra/runtime/registry_compute.py (1)
151-156: Cache size env handling is now robust and backward‑compatible.
_get_compute_registry_cache_size()correctly prefersONEX_COMPUTE_REGISTRY_CACHE_SIZE, falls back to legacyCOMPUTE_REGISTRY_CACHE_SIZE, and usesparse_env_intwith a 1–10000 range and clear logging, avoiding import‑timeValueErrors and preserving old behavior. Consider usingEnumInfraTransportType.RUNTIMEinstead ofHTTPhere so configuration errors are tagged with the more precise transport type.Also applies to: 161-211, 294-303, 329-333, 1091-1095
src/omnibase_infra/handlers/handler_http.py (1)
37-61: Configuration externalization looks solid.The environment-driven configuration is well-implemented:
- Proper use of centralized
parse_env_floatandparse_env_intutilities with validation- Reasonable defaults (30s timeout, 10MB request, 50MB response) and bounds (0.1s-3600s timeout, 1B-1GB sizes)
- Correct transport type and error context wiring
- Fail-fast behavior for invalid configuration at module load time
💡 Optional: Consider consistent identifier naming
There's a minor naming inconsistency between
service_name="http_handler"(used in lines 44, 52, 60) andHANDLER_ID_HTTP = "http-handler"(line 67). For improved observability and debugging correlation, consider using a consistent identifier format across error contexts and handler metadata.+_SERVICE_NAME = "http-handler" # Align with HANDLER_ID_HTTP for consistency + _DEFAULT_TIMEOUT_SECONDS: float = parse_env_float( "ONEX_HTTP_TIMEOUT", 30.0, min_value=0.1, max_value=3600.0, transport_type=EnumInfraTransportType.HTTP, - service_name="http_handler", + service_name=_SERVICE_NAME, ) _DEFAULT_MAX_REQUEST_SIZE: int = parse_env_int( "ONEX_HTTP_MAX_REQUEST_SIZE", 10 * 1024 * 1024, min_value=1, max_value=1073741824, transport_type=EnumInfraTransportType.HTTP, - service_name="http_handler", + service_name=_SERVICE_NAME, ) _DEFAULT_MAX_RESPONSE_SIZE: int = parse_env_int( "ONEX_HTTP_MAX_RESPONSE_SIZE", 50 * 1024 * 1024, min_value=1, max_value=1073741824, transport_type=EnumInfraTransportType.HTTP, - service_name="http_handler", + service_name=_SERVICE_NAME, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (10)
.env.examplesrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/runtime/health_server.pysrc/omnibase_infra/runtime/kernel.pysrc/omnibase_infra/runtime/registry_compute.pysrc/omnibase_infra/utils/__init__.pytests/unit/handlers/test_handler_consul.pytests/unit/handlers/test_handler_http.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
tests/unit/handlers/test_handler_http.pysrc/omnibase_infra/runtime/health_server.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/runtime/registry_compute.pysrc/omnibase_infra/handlers/handler_db.pytests/unit/handlers/test_handler_consul.pysrc/omnibase_infra/utils/__init__.pysrc/omnibase_infra/runtime/kernel.py
🧠 Learnings (30)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.{ts,tsx,js,jsx} : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use environment variables or .env files. Use process.env with defaults or environment variable managers for all configuration values (API endpoints, service URLs, feature flags).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For all backend service HTTP calls, use HTTP/2 connection pooling with max connections (100 total, 20 keepalive), timeouts (5s connect, 10s read, 5s write), and retry logic with exponential backoff (3 attempts max, 1s→2s→4s).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to {services/**/*.py,scripts/**/*.py} : Use HTTP/2 connection pooling with 100 connections and 20 keepalive settings for service-to-service communication.
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/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/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern
Applied to files:
src/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 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_http.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use ProtocolConfigurationError for invalid configuration scenarios
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to {services/**/*.py,scripts/bulk_ingest_repository.py} : Implement fail-closed configuration for security hardening. All external requests must validate URLs, implement DLQ routing, and handle failures gracefully.
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/registry/registry_*.py : Registry classes must inherit from BaseOnexRegistry and define CANONICAL_TOOLS dictionary with default tool implementations
Applied to files:
src/omnibase_infra/runtime/registry_compute.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-12-19T02:58:44.081Z
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Applied to files:
src/omnibase_infra/handlers/handler_db.py.env.examplesrc/omnibase_infra/runtime/kernel.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-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to services/intelligence/**/*.py : Intelligence-consumer configuration: Max poll 10, 5 workers, 45s session timeout, 10min max poll interval, synchronous processing.
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must support optional documents pattern with optional flag and required_capability field for future extensibility
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_cli.yaml : All ONEX node CLI interface definitions, if applicable, must be included in contract_cli.yaml with entrypoint and commands specifications
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_capabilities.yaml : All ONEX node execution capability definitions, if applicable, must be included in contract_capabilities.yaml with supported_node_types, supported_delivery_modes, and performance_constraints specifications
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.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: Applies to **/*contract*.yaml : All ONEX nodes must have validated YAML contracts following the contract-driven development pattern with input_state and output_state schema definitions
Applied to files:
.env.examplesrc/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract*.yaml : All ONEX node contract definitions must reference shared schemas using project root paths (e.g., 'schemas/...' or 'omnibase/schemas/...') rather than relative paths
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
src/omnibase_infra/runtime/kernel.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: All ONEX nodes must conform to the 4-Node Architecture pattern with clear separation of concerns and unidirectional data flow (EFFECT → COMPUTE → REDUCER → ORCHESTRATOR)
Applied to files:
src/omnibase_infra/runtime/kernel.py
📚 Learning: 2025-11-24T17:22:32.195Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/canonical_patterns.mdc:0-0
Timestamp: 2025-11-24T17:22:32.195Z
Learning: Applies to **/v[0-9]_[0-9]_[0-9]/{tools,registry}/**.py : Constructor-based dependency injection must be used for all tool and node classes; validate that required dependencies are not None, raising OnexError with specific error code if missing
Applied to files:
src/omnibase_infra/runtime/kernel.py
🧬 Code graph analysis (4)
tests/unit/handlers/test_handler_http.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(111-146)
src/omnibase_infra/handlers/handler_http.py (2)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
src/omnibase_infra/runtime/registry_compute.py (2)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/utils/util_env_parsing.py (1)
parse_env_int(57-154)
src/omnibase_infra/handlers/handler_db.py (2)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
🔇 Additional comments (8)
tests/unit/handlers/test_handler_consul.py (1)
31-35: LGTM!The expanded docstring with the
Returnssection improves documentation clarity and follows Python docstring conventions..env.example (2)
193-199: ONEX_CONTRACTS_DIR and legacy fallback docs look consistent.Preferred ONEX_CONTRACTS_DIR plus explicit CONTRACTS_DIR deprecation note matches the new kernel
_get_contracts_dir()behavior and keeps backward compatibility.
214-220: New ONEX_ config documentation matches implementation ranges.*The Registration Orchestrator, compute registry cache size, handler HTTP/DB limits, runtime timeouts, circuit breaker, and idempotency env var docs (defaults and ranges) align with the new parsing logic and constraints in the runtime and handlers.
Also applies to: 241-244, 261-315
tests/unit/handlers/test_handler_http.py (1)
2092-2299: Env‑parsing tests correctly validate error handling and context.These tests cover defaulting, valid/invalid values, and ProtocolConfigurationError context for
parse_env_float/parse_env_int, matching the util implementations and error‑context expectations.src/omnibase_infra/utils/__init__.py (1)
7-10: Re‑exporting env parsers from utils is a good public API choice.Including
parse_env_float/parse_env_int(and documenting util_env_parsing) makes the configuration helpers easy to discover and keeps call sites consistent.Also applies to: 19-22, 33-45
src/omnibase_infra/handlers/handler_db.py (1)
82-83: DB handler defaults now use centralized, validated env parsing.Using
parse_env_int/parse_env_floatforONEX_DB_POOL_SIZEandONEX_DB_TIMEOUTwith DATABASE transport context and sensible ranges (1–100 pool, 0.1–3600s timeout) aligns with the new env‑parsing utilities and avoids brittle import‑timeValueErrors.Also applies to: 92-100, 103-110
src/omnibase_infra/runtime/kernel.py (1)
21-33: Contracts directory env handling is clean and backward‑compatible.
_get_contracts_dir()correctly prefersONEX_CONTRACTS_DIR, logs when falling back to legacyCONTRACTS_DIR, and defaults to./contracts, with bootstrap and module docs updated accordingly and the env constants exported via__all__. This centralizes path resolution nicely.Also applies to: 84-92, 101-127, 300-340, 365-373, 829-835
src/omnibase_infra/handlers/handler_http.py (1)
30-30: LGTM - Centralized environment parsing utilities imported.The import of
parse_env_floatandparse_env_intfrom the centralized utilities module aligns with the PR objective to standardize environment variable parsing across the codebase.
PR Review: Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes 12+ hardcoded configuration values to environment variables, improving deployment flexibility. The implementation follows ONEX patterns with strong typing, comprehensive error handling, and excellent test coverage (2460 tests passing). ✅ Strengths1. Excellent Code Organization
2. Robust Error Handling
3. Outstanding Test Coverage
4. Backward Compatibility
5. Documentation Excellence
🔍 Issues FoundCRITICAL: Inconsistent Error Handling PatternLocation: Issue: Uses # model_circuit_breaker_config.py (lines 229-244)
raise ProtocolConfigurationError(
f"Invalid value for {threshold_var} environment variable: "
"expected integer",
context=context,
parameter=threshold_var,
value="[REDACTED]",
) from e # ← Chains original ValueError
# util_env_parsing.py (lines 126-131)
raise ProtocolConfigurationError(
f"Invalid value for {env_var} environment variable: expected integer",
context=context,
parameter=env_var,
value="[REDACTED]",
) from None # ← Suppresses original ValueErrorWhy this matters:
Recommendation: Test verification: Test at line 429 explicitly checks MEDIUM: Code DuplicationIssue: Current state:
Recommendation: @classmethod
def from_env(
cls,
service_name: str = "unknown",
transport_type: EnumInfraTransportType = EnumInfraTransportType.HTTP,
prefix: str = "ONEX_CB",
) -> ModelCircuitBreakerConfig:
from omnibase_infra.utils.util_env_parsing import parse_env_int, parse_env_float
threshold = parse_env_int(
f"{prefix}_THRESHOLD",
default=5,
min_value=1, # Pydantic validation will catch this, but good practice
transport_type=transport_type,
service_name=service_name,
)
reset_timeout = parse_env_float(
f"{prefix}_RESET_TIMEOUT",
default=60.0,
min_value=0.0,
transport_type=transport_type,
service_name=service_name,
)
return cls(
threshold=threshold,
reset_timeout_seconds=reset_timeout,
service_name=service_name,
transport_type=transport_type,
)Benefits:
Note: This would require updating tests that check for specific error messages, since MINOR: Missing Range ValidationIssue: Current behavior:
Better behavior (what
Impact: Edge case handling (tests at lines 490-514 validate this behavior). Recommendation: Use MINOR: Import OrganizationLocation: Issue: Lazy import happens inside the method, but the function Current: def _get_error_classes(): # Module level
from omnibase_infra.errors.error_infra import ProtocolConfigurationError
...
@classmethod
def from_env(cls, ...):
ProtocolConfigurationError, ModelInfraErrorContext = _get_error_classes() # Called hereRecommendation: If 🔒 Security Review✅ Excellent Security Practices
|
| Old Variable | New Variable | Migration |
|---|---|---|
CONTRACTS_DIR |
ONEX_CONTRACTS_DIR |
Legacy supported |
COMPUTE_REGISTRY_CACHE_SIZE |
ONEX_COMPUTE_REGISTRY_CACHE_SIZE |
Legacy supported |
2. Clarify "Backward Compatible"
PR says "All default values preserved for backward compatibility" - but CLAUDE.md says "No backwards compatibility". This PR actually maintains compatibility through:
- Environment variable fallbacks (old names still work)
- Same default values
This is fine, just clarify this is deployment backward compatibility (not code API changes).
🎯 ONEX Compliance
✅ Full Compliance
- ✅ No
Anytypes - ✅ Pydantic models for all configs
- ✅ PEP 604 unions (
X | None) - ✅ Container injection patterns maintained
- ✅ ONEX error hierarchy used
- ✅ Transport-aware error context
- ✅ Strong typing throughout
- ✅ One model per file (where applicable)
CLAUDE.md Alignment:
| Rule | Status | Notes |
|---|---|---|
No Any types |
✅ Pass | All types explicit |
| Strong typing | ✅ Pass | Excellent type annotations |
| Error hierarchy | ✅ Pass | Uses ProtocolConfigurationError |
| Security patterns | ✅ Pass | Value redaction, correlation IDs |
| No backwards compat | See note above |
🐛 Potential Bugs
MINOR: Silent Failures on Out-of-Range Values
Scenario: User sets ONEX_DB_POOL_SIZE=200 (max is 100)
Current behavior:
_DEFAULT_POOL_SIZE: int = parse_env_int(
"ONEX_DB_POOL_SIZE", 5, min_value=1, max_value=100, ...
)
# Logs warning, returns 5Result: Pool size silently becomes 5 instead of 200. User's intent (large pool) is ignored.
Is this a bug? Debatable. Current behavior:
- ✅ Prevents crashes
- ✅ Provides observability (warning log)
- ❌ Violates principle of least surprise
Alternative: Raise ProtocolConfigurationError for out-of-range values (fail fast).
Recommendation: Current behavior is acceptable for MVP. Consider making this configurable later (strict mode vs. permissive mode).
🚀 Final Verdict
Approval Status: ✅ APPROVE WITH MINOR CHANGES
This is high-quality work that significantly improves deployment flexibility. The code is well-structured, thoroughly tested, and follows ONEX patterns.
Required Changes (Before Merge):
- Fix inconsistent error chaining: Change
from e→from NoneinModelCircuitBreakerConfig.from_env()for security consistency
Recommended Changes (Can be follow-up PRs):
- Refactor to use utility functions: Eliminate duplication in
from_env()by usingparse_env_int/float - Strengthen test assertions: Replace
pytest.raises(Exception)with specific exception types - Add migration guide: Document old → new variable mappings
Nice-to-Have:
- Consider strict mode: Add option to fail fast on out-of-range values instead of falling back to defaults
📈 Metrics
- Files Changed: 21
- Additions: 1,883 lines
- Deletions: 86 lines
- Net Change: +1,797 lines (mostly tests and docs)
- Test Coverage: 2,460 tests passing
- New Tests: 577 for circuit breaker config alone
- Environment Variables Added: 12+
🙏 Appreciation
Excellent work on:
- Comprehensive test suite covering edge cases
- Security-conscious error handling
- Clear documentation with examples
- Thoughtful backward compatibility
- ONEX pattern adherence
The team put significant effort into making this production-ready. Just address the error chaining inconsistency and this is good to merge.
Reviewed by: Claude Sonnet 4.5
Review Date: 2025-12-27
Standards: ONEX Infrastructure Guidelines (CLAUDE.md)
- Fix critical health_server.py import crash by using parse_env_int with port range validation (1-65535) instead of raw int() parsing - Add min/max range validation to idempotency store config env parsing to match Pydantic field constraints - Refactor circuit breaker config to use centralized parse_env_int/float utilities, removing 46 lines of duplicate error handling code - Update docker/.env.example with all ONEX env variables and valid ranges - Add 36 new tests for HTTP handler env variable parsing - Add 6 edge case tests for circuit breaker config (51 tests total)
|
APPROVED - Excellent refactoring with comprehensive test coverage, security-conscious implementation, and proper ONEX pattern adherence. See detailed review below. |
|
Code Quality: Environment parsing utilities are excellent with type-safe parsing, range validation with graceful fallback, value redaction for security, and proper ONEX error patterns. Circuit breaker config from_env() method verified working correctly with MixinAsyncCircuitBreaker._init_circuit_breaker_from_config() at line 282. |
|
Security Analysis: No concerns identified. Implementation follows OWASP secure configuration practices with: (1) Value redaction preventing sensitive config leaks in error logs, (2) Range validation preventing resource exhaustion (max 1GB request size, max 100 DB connections, port 1-65535, TTL 60s-30d), (3) Transport-aware error context for debugging without exposing secrets, (4) Proper input validation with try/except for all numeric parsing. |
|
ONEX Patterns Compliance: Fully compliant - Strong typing (no Any types), ProtocolConfigurationError with proper context, security features (value redaction, input validation, range limits), Container DI integration, comprehensive .env.example documentation, PEP 604 union syntax (X | None), proper error chaining (raise...from e). Code style is excellent with clear naming, comprehensive docstrings, type annotations throughout, and proper module-level constants. |
|
Test Coverage Summary: 640 lines circuit breaker tests with 51 cases, 724 lines HTTP handler tests. Covers all edge cases, error contexts, value redaction. All 2460 unit tests passing with over 90 percent coverage for new utilities. |
|
Recommendations (non-blocking): (1) Document range validation fallback behavior in deployment guide - out-of-range values use defaults with warnings, (2) Add import-time note in handler docstrings - env vars parsed at module load and require restart, (3) Consider config validation command for pre-deployment checks, (4) Create migration guide for operators. |
|
FINAL VERDICT: APPROVE and merge. This PR demonstrates excellent engineering with comprehensive testing (2693 additions, 23 files), security-conscious implementation (value redaction, range validation), clear documentation (68 lines root .env.example, 55 docker), and proper ONEX adherence. Successfully achieves deployment flexibility (OMN-1058) while maintaining backward compatibility. Great work! |
Merged Valkey configuration from main while keeping ONEX_ prefix for environment variables for consistency with the rest of the codebase.
PR Review: Externalize Hardcoded Configuration Values [OMN-1058]SummaryThis PR successfully externalizes ~20 hardcoded configuration values to environment variables, improving deployment flexibility. The implementation is well-architected with comprehensive test coverage (2460 tests passing, 640+ new tests added). Overall, this is high-quality work that follows ONEX patterns. ✅ Strengths1. Excellent Utility DesignThe new
# Good example from util_env_parsing.py:133-152
# Range validation with warning + default fallback (not hard error)
if min_value is not None and parsed < min_value:
logger.warning("...using default %d", default)
return default2. Comprehensive Test Coverage
3. Backward Compatibility
4. ModelCircuitBreakerConfig.from_env()Clean factory method with proper separation of concerns:
🔴 Critical Issues1. Missing
|
| Category | Rating | Notes |
|---|---|---|
| Architecture | ⭐⭐⭐⭐⭐ | Excellent separation of concerns, reusable utilities |
| Error Handling | ⭐⭐⭐⭐ | Transport-aware errors, value redaction, chaining ✅ |
| Testing | ⭐⭐⭐⭐⭐ | 640+ tests, comprehensive coverage, edge cases ✅ |
| Security | ⭐⭐⭐⭐⭐ | Value redaction, no credential exposure ✅ |
| Documentation | ⭐⭐⭐⭐ | Good .env.example, could add more examples |
| ONEX Compliance | ⭐⭐⭐⭐⭐ | Follows all ONEX patterns, no Any types ✅ |
| Backward Compat | ⭐⭐⭐⭐⭐ | Legacy names supported, defaults preserved ✅ |
Overall: ⭐⭐⭐⭐½ (4.5/5)
🔧 Action Items
Must Fix (Blocking):
- ✅ Add
max_valueconstraints toModelCircuitBreakerConfig.from_env()(lines 202-214) - ✅ Fix error message in
parse_env_floatto say"expected float"instead of"expected numeric value"(line 230) - ✅ Verify test expectations match actual error messages (test_model_circuit_breaker_config.py:253)
Should Fix (High Priority):
⚠️ Document range validation behavior in.env.example(warning + default fallback)⚠️ Add test verification that circuit breaker from_env respects max_value once added
Nice to Have:
- 💡 Add examples of invalid values to
.env.example - 💡 Consider if range violation should log at INFO vs WARNING level
🎯 Security Review
✅ No security concerns identified:
- Value redaction prevents credential exposure in errors ✅
- DSN sanitization not affected ✅
- No sensitive defaults exposed ✅
- Error context properly structured ✅
✅ Final Recommendation
APPROVE with minor fixes required
This PR demonstrates excellent engineering practices with comprehensive testing, proper error handling, and security-conscious design. The critical issue (#1 - missing max_value) should be addressed before merge, but it's a simple addition. Once fixed, this is production-ready.
The fallback-to-default behavior for out-of-range values is a reasonable design choice for infrastructure configuration, prioritizing availability over strict validation. Consider documenting this explicitly for operators.
Great work overall! 🚀
Reviewed following: ONEX CLAUDE.md patterns, ONEX error handling patterns, circuit breaker implementation guidelines
- Fix 6 failing CI tests in test_model_circuit_breaker_config.py:
- Update error message assertions ("expected numeric value" not "expected float")
- Convert raise-expecting tests to warning+fallback verification
- Fix error chaining test to match intentional `raise from None` pattern
- HTTP handler improvements:
- Update range validation: timeout (1-300s), sizes (1KB-100MB)
- Idempotency store config:
- Add env var support for pool_min_size, pool_max_size, command_timeout,
clock_skew_tolerance_seconds, cleanup_max_iterations
- Use centralized parse_env_int/parse_env_float utilities
- Health server safety:
- Move env parsing from module-level to lazy init
- Prevent import crashes on invalid env var values
- Add graceful fallback with warning logging
- Documentation:
- Comprehensive .env.example updates with all externalized config values
- Document defaults, ranges, and usage for all new env vars
Pull Request Review: Configuration Externalization [OMN-1058]OverviewThis PR successfully externalizes 12+ hardcoded configuration values to environment variables, improving deployment flexibility. The implementation is solid with excellent test coverage (724 new test lines for HTTP handler alone, 682 for circuit breaker config). ✅ Strengths1. Architecture & Design
2. Code Quality
3. Testing
4. Backwards Compatibility
|
| Priority | Issue | Effort | Impact |
|---|---|---|---|
| 🔴 P0 | HTTP timeout range mismatch (1.0-300 vs 0.1-3600) | Low | High |
| 🔴 P0 | HTTP size range mismatch (100MB vs 1GB) | Low | High |
| 🟡 P1 | Circuit breaker max constraints missing | Low | Medium |
| 🟢 P2 | Error message consistency ("float" vs "numeric value") | Low | Low |
| 🟢 P3 | Add Final type hints for constants |
Low | Low |
✅ Approval Recommendation
Status:
This is high-quality work with excellent testing and documentation. The core implementation is solid. However, the range constraint mismatches between code and documentation must be fixed before merge to prevent silent failures in production.
Required Changes Before Merge:
- ✅ Align HTTP timeout range (code vs docs)
- ✅ Align HTTP size ranges (code vs docs)
- ✅ Add circuit breaker max constraints OR update docs
Recommended Changes (Non-blocking):
- Improve error message consistency
- Add migration guide to docs
🎉 Excellent Work
The centralized util_env_parsing.py approach is a great pattern that should be reused across the codebase. The comprehensive test coverage and documentation set a high bar for future PRs.
Test Coverage: 1406+ new lines of tests
Documentation Quality: Excellent inline comments and examples
Security: Best practices followed throughout
Once the range constraint issues are resolved, this will be production-ready! 🚀
Reviewed by: Claude Sonnet 4.5
Review Date: 2025-12-27
…MN-1058] Fix test/handler range mismatch where tests used incorrect range assumptions (1B-1GB) that didn't match the actual handler configuration (1KB-100MB). Changes: - Update _HTTP_SIZE_MIN from 1 to 1024 (1 KB) - Update _HTTP_SIZE_MAX from 1073741824 to 104857600 (100 MB) - Update all docstrings referencing boundaries - Update boundary test assertions to match new values This addresses PR #106 review feedback about request/response size range consistency.
PR Review: Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes ~20 hardcoded configuration values to environment variables, significantly improving deployment flexibility. The implementation demonstrates excellent ONEX compliance, comprehensive testing, and careful attention to security and backwards compatibility. ✅ Strengths1. Architecture & Design
2. ONEX Compliance
3. Testing Excellence
4. Documentation
5. Backwards Compatibility
🎯 Potential Improvements (Optional)1. Validator Consistency 2. Range Documentation in Code 3. Pool Size Cross-Validation 📊 Test Coverage
🔒 Security AssessmentSecure Practices:
Deployment Checklist:
📝 Final VerdictStatus: ✅ APPROVED - Ready to Merge This PR represents exemplary infrastructure engineering:
Metrics:
Follow-up recommendations:
Reviewed by: Claude Sonnet 4.5 (ONEX Infrastructure Specialist) |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (14)
.env.exampledocker/.env.examplesrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/projectors/snapshot_publisher_registration.pysrc/omnibase_infra/runtime/health_server.pysrc/omnibase_infra/runtime/models/model_runtime_scheduler_config.pysrc/omnibase_infra/utils/__init__.pytests/unit/handlers/test_handler_http.pytests/unit/handlers/test_handler_http_env.pytests/unit/models/resilience/test_model_circuit_breaker_config.pytests/unit/runtime/test_runtime_scheduler.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/omnibase_infra/projectors/snapshot_publisher_registration.py
- src/omnibase_infra/handlers/handler_db.py
- tests/unit/runtime/test_runtime_scheduler.py
- src/omnibase_infra/runtime/models/model_runtime_scheduler_config.py
- tests/unit/handlers/test_handler_http.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
tests/unit/models/resilience/test_model_circuit_breaker_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/runtime/health_server.pysrc/omnibase_infra/utils/__init__.pysrc/omnibase_infra/handlers/handler_http.pytests/unit/handlers/test_handler_http_env.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/model_*.py: All data structures must be proper Pydantic models - one model per file with naming pattern model_.py and class pattern Model
Result models may override bool to enable idiomatic conditional checks, with Warning section in docstring explaining non-standard behavior
Files:
src/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
🧠 Learnings (33)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.{ts,tsx,js,jsx} : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use environment variables or .env files. Use process.env with defaults or environment variable managers for all configuration values (API endpoints, service URLs, feature flags).
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 : Use type-safe configuration via Pydantic Settings from config/settings.py with 90+ type-safe variables organized into External Service Discovery, Shared Infrastructure, AI Provider API Keys, Local Services, and Feature Flags
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Applied to files:
tests/unit/models/resilience/test_model_circuit_breaker_config.pysrc/omnibase_infra/models/resilience/model_circuit_breaker_config.py
📚 Learning: 2025-11-24T16:33:51.604Z
Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-11-24T16:33:51.604Z
Learning: Applies to tests/unit/models/**/test_model_*.py : Model tests must achieve 100% coverage and test instantiation, inheritance, serialization, deserialization, JSON serialization, roundtrip serialization, equality, hashing, string representation, repr, attributes, validation, metadata, data creation, copying, and immutability
Applied to files:
tests/unit/models/resilience/test_model_circuit_breaker_config.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 tests/**/*.py : Write comprehensive test coverage following the test structure under `tests/unit/` organized by subsystem (enums, models, mixins, utils)
Applied to files:
tests/unit/models/resilience/test_model_circuit_breaker_config.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must support optional documents pattern with optional flag and required_capability field for future extensibility
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX node deviations from canonical patterns must be documented and justified in the node's root-level README.md and subject to maintainer review
Applied to files:
.env.example
📚 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 config/README.md : Document all configuration variables in config/README.md with type information, validation rules, and usage examples
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
.env.example
📚 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 .env.example : Use .env.example as the canonical template for all environment variables with documentation for each variable
Applied to files:
.env.exampledocker/.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/docker-compose.yml : For Docker services connecting to Kafka/Redpanda, use `KAFKA_BOOTSTRAP_SERVERS=omninode-bridge-redpanda:9092` (internal Docker network port 9092). For host scripts, use `192.168.86.200:29092` (external published port). Do not hardcode ports - use environment variables with proper defaults.
Applied to files:
.env.examplesrc/omnibase_infra/runtime/health_server.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For Redpanda/Kafka connection patterns: Docker services use `omninode-bridge-redpanda:9092` (DNS resolves via /etc/hosts to 192.168.86.200:9092), host scripts use `192.168.86.200:29092` (direct IP with external port), remote server access uses `localhost:29092`. Never mix these contexts.
Applied to files:
.env.example
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
Applied to files:
src/omnibase_infra/models/resilience/model_circuit_breaker_config.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Applied to files:
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/tests/**/*.py : All integration tests must verify correct Kafka port usage for context (9092 for Docker, 29092 for host). Test both local (qdrant, memgraph) and remote (PostgreSQL, Redpanda) database connectivity. Never assume test environment configuration.
Applied to files:
src/omnibase_infra/runtime/health_server.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For host scripts (bulk_ingest_repository.py, test scripts running outside Docker), use `KAFKA_BOOTSTRAP_SERVERS = os.getenv('KAFKA_BOOTSTRAP_SERVERS', '192.168.86.200:29092')` to connect to remote Redpanda on external port 29092.
Applied to files:
src/omnibase_infra/runtime/health_server.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For all backend service HTTP calls, use HTTP/2 connection pooling with max connections (100 total, 20 keepalive), timeouts (5s connect, 10s read, 5s write), and retry logic with exponential backoff (3 attempts max, 1s→2s→4s).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.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_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use ProtocolConfigurationError for invalid configuration scenarios
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to {services/**/*.py,scripts/bulk_ingest_repository.py} : Implement fail-closed configuration for security hardening. All external requests must validate URLs, implement DLQ routing, and handle failures gracefully.
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Applied to files:
src/omnibase_infra/handlers/handler_http.pydocker/.env.examplesrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
src/omnibase_infra/handlers/handler_http.pysrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.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_http.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Applied to files:
tests/unit/handlers/test_handler_http_env.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/.env* : Never commit `.env` files to version control. Use `.env.example` as template. All configuration must be manageable via environment variables with sensible defaults. Document all required variables with their purposes.
Applied to files:
docker/.env.example
📚 Learning: 2025-12-19T02:58:44.081Z
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Applied to files:
docker/.env.examplesrc/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.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/**/{config,settings}/**/*.py : Environment variables MUST include: POSTGRES_HOST, POSTGRES_PORT, POSTGRES_DATABASE, POSTGRES_USER, POSTGRES_PASSWORD, KAFKA_BOOTSTRAP_SERVERS, CONSUL_HOST, CONSUL_PORT, LOG_LEVEL. Use secrets manager for production passwords.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Applied to files:
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.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/idempotency/models/model_postgres_idempotency_store_config.py
🧬 Code graph analysis (6)
tests/unit/models/resilience/test_model_circuit_breaker_config.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (2)
ModelCircuitBreakerConfig(43-222)from_env(139-222)
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (2)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)
src/omnibase_infra/runtime/health_server.py (3)
src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/utils/util_env_parsing.py (1)
parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
src/omnibase_infra/handlers/handler_http.py (2)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
tests/unit/handlers/test_handler_http_env.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py (2)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
🔇 Additional comments (23)
src/omnibase_infra/idempotency/models/model_postgres_idempotency_store_config.py (3)
36-99: Environment-driven defaults with proper validation.The module-level environment parsing correctly uses
parse_env_int/parse_env_floatwith range validation, addressing the previous review concern about bareint()conversions. The min/max bounds align with the Pydantic Field constraints below.
201-284: Field defaults and descriptions properly updated.The Pydantic Field definitions now reference environment-driven defaults and include env var names in descriptions. The
ge/leconstraints match themin_value/max_valuebounds used during parsing, ensuring consistent validation.
286-317: Pool size cross-field validation is well-implemented.The
validate_pool_sizesvalidator correctly ensurespool_max_size >= pool_min_sizeand raisesProtocolConfigurationErrorwith proper context including transport type, operation, target name, and correlation_id.docker/.env.example (1)
162-226: Comprehensive environment variable documentation.The
.env.examplefile now serves as a canonical template for all ONEX configuration variables with clear range documentation, defaults, and usage notes. The legacy deprecation notice forCOMPUTE_REGISTRY_CACHE_SIZEaids migration.src/omnibase_infra/utils/__init__.py (1)
24-27: Public API exports for environment parsing utilities.The
parse_env_floatandparse_env_intutilities are correctly exposed in the package's public API, enabling consistent usage across the codebase.Also applies to: 46-47
src/omnibase_infra/handlers/handler_http.py (2)
39-62: Environment-driven HTTP handler configuration with proper validation.The handler now uses
parse_env_float/parse_env_intwith appropriate range validation, addressing the previous review concern about barefloat()/int()conversions. The ranges (1.0-300.0s for timeout, 1KB-100MB for sizes) are reasonable for HTTP operations.
68-96: Handler ID and size categorization utilities.The
HANDLER_ID_HTTPconstant and size categorization utilities (_categorize_size) support sanitized error logging by preventing exact payload sizes from being exposed—a good security practice.tests/unit/handlers/test_handler_http_env.py (2)
51-188: Comprehensive timeout parsing tests.The test class thoroughly covers default values, valid custom values, invalid inputs, range validation with warnings, and boundary conditions. The use of
caplogfor warning verification is appropriate.
497-615: Error context validation tests are thorough.The tests properly verify all required error context fields (transport_type, operation, target_name, correlation_id, parameter) and importantly verify value redaction for security. This aligns with the coding guidelines requiring correlation_id and transport_type in error context.
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (1)
138-222: Well-designedfrom_env()factory method.The implementation correctly:
- Uses centralized
parse_env_int/parse_env_floatutilities with appropriate min_value validation- Requires caller to provide
service_nameandtransport_typefor context specificity (documented in docstring)- Supports custom prefixes for service-specific configuration
- Includes comprehensive documentation with usage examples
This aligns with the coding guideline to use Pydantic Settings patterns for environment configuration.
src/omnibase_infra/runtime/health_server.py (2)
49-87: Safe lazy port parsing prevents import-time crashes.The refactored implementation correctly:
- Keeps
DEFAULT_HTTP_PORTas a fixed constant to avoid import-time failures- Moves environment parsing to
_get_port_from_env()called at initialization time- Catches
ProtocolConfigurationErrorand falls back gracefully with a warning- Validates the port range (1-65535) appropriately
This addresses the previous review concern about env-driven
DEFAULT_HTTP_PORTbreaking validation and causing import crashes.
109-128: Port resolution order is clear and well-documented.The
__init__signature change toport: int | None = Nonewith the resolution logic (explicit port > env var > default) is intuitive and well-documented in the docstring.tests/unit/models/resilience/test_model_circuit_breaker_config.py (3)
1-40: Well-organized test module with clear coverage goals.The module docstring clearly outlines the test organization (5 classes, ~51 tests) and coverage goals including all from_env paths, error scenarios, and security validation. This follows the learning to achieve 100% model test coverage.
424-442: Important security test for exception chain suppression.The test correctly verifies that
error.__cause__ is None, confirming the implementation usesraise ... from Noneto prevent exposing raw invalid values in the originalValueError. This is a critical security pattern for value redaction.
580-600: Transport type coverage ensures consistent behavior.Testing all 8 transport types (
HTTP,DATABASE,KAFKA,CONSUL,VAULT,VALKEY,GRPC,RUNTIME) ensures thefrom_env()method correctly preserves the transport type in the returned config, which is essential for proper error context propagation..env.example (8)
191-198: Excellent documentation for HTTP port and contracts configuration.The ONEX_HTTP_PORT range (1-65535) and default are clearly documented. The ONEX_CONTRACTS_DIR section properly marks the legacy CONTRACTS_DIR as deprecated while indicating the preferred naming convention, which is valuable for backward-compatibility migration.
215-271: Runtime Scheduler Configuration is comprehensive and well-structured.The section includes clear documentation for tick intervals, restart-safety (sequence persistence), performance settings (jitter), circuit breaker configuration, metrics, and Valkey settings. Ranges are specified appropriately (e.g., tick interval 10-60000ms, CB threshold 1-100, Valkey timeout 0.1-60.0s). Legacy names are noted at the end, which helps with deprecation tracking.
273-295: Compute Registry Configuration documentation is helpful for operators.The cache sizing guidelines (128 for small, 256 for medium, 512 for large deployments) and memory footprint estimates (~100 bytes per entry) are practical and aid capacity planning. The range (1-10000) is reasonable.
297-324: Performance threshold documentation appropriately acknowledges no strict bounds.The "No strict range, any positive value" clarification for ONEX_PERF_THRESHOLD_* variables is correct since these are observability thresholds, not limits. The production vs. development/CI example configurations (lines 318-324) provide clear guidance for different environments.
326-366: Handler configuration section is thorough and production-ready.HTTP handler documentation (timeout, max request/response sizes) and database handler documentation (pool size, query timeout) are clear with operational guidance. The production example configuration (lines 361-366) demonstrates realistic settings. However, verify that the documented ranges are enforced by validation code:
- HTTP timeout (0.1-3600.0s) and sizes (1B-1GB) should be validated in
handler_http.py- DB pool size (1-100) and timeout (0.1-3600.0s) should be validated in
handler_db.py
368-377: Runtime timeouts are documented with reasonable ranges.Both ONEX_HEALTH_CHECK_TIMEOUT (1.0-60.0s, default 5.0) and ONEX_DRAIN_TIMEOUT (1.0-300.0s, default 30.0) have sensible defaults and ranges for health probe and graceful shutdown scenarios.
379-402: Circuit breaker configuration includes excellent operational context.The state machine descriptions (CLOSED, OPEN, HALF_OPEN) and configuration guidelines for high-reliability vs. best-effort services are valuable for operators tuning per-service overrides. The ranges (threshold 1-100, reset timeout 0.0-3600.0s) are sensible.
404-449: Idempotency store configuration is comprehensive with production guidance.All critical settings are documented: TTL (60-2592000 seconds), cleanup interval (60-86400 seconds), batch size (100-100000), pool configuration, clock skew tolerance (0-3600 seconds), and max iterations (1-1000). The high-volume system example (lines 445-449) provides actionable tuning guidance. The explanations of trade-offs (e.g., smaller batches reduce lock contention) are helpful.
Verify that these ranges are enforced in
model_postgres_idempotency_store_config.py:
- TTL and cleanup interval bounds
- Batch size limits (prevent excessively small or large values)
- Pool size constraints
- Clock skew tolerance validation
| # ONEX_CONTRACTS_DIR=./contracts # Preferred name | ||
| # | ||
| # Legacy name (deprecated, will be removed in future version): | ||
| # CONTRACTS_DIR=./contracts |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CONTRACTS_DIR vs ONEX_CONTRACTS_DIR usage consistency
echo "=== Checking CONTRACTS_DIR usage ==="
rg -n 'CONTRACTS_DIR' --type python -g '!**/test/**' -g '!.env*'
echo ""
echo "=== Checking ONEX_CONTRACTS_DIR usage ==="
rg -n 'ONEX_CONTRACTS_DIR' --type python -g '!**/test/**'Repository: OmniNode-ai/omnibase_infra
Length of output: 156
🏁 Script executed:
# Check for migration/upgrade documentation
echo "=== Looking for migration/upgrade notes ==="
find . -type f \( -name "*.md" -o -name "*.txt" \) | head -20 | xargs grep -l "CONTRACTS_DIR\|ONEX_CONTRACTS_DIR\|migration\|upgrade" 2>/dev/null | head -10Repository: OmniNode-ai/omnibase_infra
Length of output: 343
🏁 Script executed:
# Check .env files and related configuration for variable references
echo "=== Checking .env files ==="
fd -e env -e example -e template | head -20 | xargs cat 2>/dev/null | grep -A2 -B2 "CONTRACTS_DIR"Repository: OmniNode-ai/omnibase_infra
Length of output: 356
🏁 Script executed:
# Search for environment variable loading/parsing code
echo "=== Looking for environment variable loading code ==="
rg -n 'os\.environ|getenv|load_env|dotenv' --type python -A 3 | head -100Repository: OmniNode-ai/omnibase_infra
Length of output: 156
🏁 Script executed:
# Check CHANGELOG.md for migration information about CONTRACTS_DIR deprecation
head -200 CHANGELOG.md | grep -A 5 -B 5 "CONTRACTS_DIR\|ONEX_CONTRACTS_DIR" || echo "Not in first 200 lines"
tail -500 CHANGELOG.md | grep -A 5 -B 5 "CONTRACTS_DIR\|ONEX_CONTRACTS_DIR" || echo "Not in recent changes"Repository: OmniNode-ai/omnibase_infra
Length of output: 114
🏁 Script executed:
# Find Python files that likely load configuration/environment variables
fd -t f -e py . | grep -E '(config|env|settings|loader)' | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 1426
🏁 Script executed:
# Search in likely config locations for environment variable handling
rg 'getenv.*CONTRACTS' --type python --max-count 20Repository: OmniNode-ai/omnibase_infra
Length of output: 100
🏁 Script executed:
# Specific search in src/ directory if it exists
fd -t f -e py ./src 2>/dev/null | head -5Repository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
# Search for both CONTRACTS_DIR variables in all Python files
echo "=== CONTRACTS_DIR references ==="
rg 'CONTRACTS_DIR' -g '*.py' --max-count 30
echo ""
echo "=== ONEX_CONTRACTS_DIR references ==="
rg 'ONEX_CONTRACTS_DIR' -g '*.py' --max-count 30Repository: OmniNode-ai/omnibase_infra
Length of output: 3253
🏁 Script executed:
# Check if there's a main config loader or environment setup
fd -g '*.py' . | xargs grep -l 'os.environ\|getenv' 2>/dev/null | head -10Repository: OmniNode-ai/omnibase_infra
Length of output: 651
🏁 Script executed:
# Look specifically in config files for environment variable handling
cat src/omnibase_infra/runtime/models/model_logging_config.py 2>/dev/null | head -50Repository: OmniNode-ai/omnibase_infra
Length of output: 1132
🏁 Script executed:
# Check .env.example file more completely
cat .env.example | grep -A 10 -B 5 "CONTRACTS"Repository: OmniNode-ai/omnibase_infra
Length of output: 814
Update tests and enhance deprecation documentation for CONTRACTS_DIR migration.
The variable naming is consistent and properly implemented in src/omnibase_infra/runtime/kernel.py with correct precedence: ONEX_CONTRACTS_DIR is checked first, falling back to legacy CONTRACTS_DIR if not set. However:
- Tests in
tests/unit/runtime/test_kernel.pystill setCONTRACTS_DIRdirectly viamonkeypatch.setenv()— update these to useONEX_CONTRACTS_DIRinstead - Add a migration note to
CHANGELOG.mddocumenting theCONTRACTS_DIR→ONEX_CONTRACTS_DIRrename with clear guidance on the deprecation timeline
The implementation in kernel.py correctly emits a deprecation warning when the fallback is triggered, and the docstrings document the fallback behavior.
🤖 Prompt for AI Agents
In .env.example around lines 195-198, tests still use the deprecated
CONTRACTS_DIR and the repo lacks a changelog migration note; update unit tests
in tests/unit/runtime/test_kernel.py to set ONEX_CONTRACTS_DIR via
monkeypatch.setenv() instead of CONTRACTS_DIR (preserve any values and
assertions), and add a new entry to CHANGELOG.md under an "Unreleased" or next
version heading documenting the CONTRACTS_DIR → ONEX_CONTRACTS_DIR rename, the
deprecation timeline, and the fact that kernel.py will fall back to
CONTRACTS_DIR with a deprecation warning.
…MN-1058] - Update _HTTP_TIMEOUT_MIN from 0.1 to 1.0 to match handler_http.py - Update _HTTP_TIMEOUT_MAX from 3600.0 to 300.0 to match handler_http.py - Update .env.example HTTP timeout range documentation (1.0-300.0) - Update test docstrings and test values for boundary tests Addresses PR #106 review feedback about test constant mismatch.
PR Review: Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes ~20 hardcoded configuration values to environment variables with excellent test coverage (3012 additions) and strong ONEX pattern adherence. Strengths
Critical Issues (Must Fix Before Merge)1. Module-Level Env Parsing in HandlersLocation: handler_http.py:39-62, handler_db.py:91-108 Problem: Environment variables are parsed at module import time. Invalid env var values will crash on import, preventing runtime from starting. This violates ONEX Container DI pattern. Fix: Move to lazy initialization in init or create ModelHttpHandlerConfig following ModelCircuitBreakerConfig pattern. 2. Missing Pool Size ValidationLocation: model_postgres_idempotency_store_config.py:199-218 Problem: pool_min_size and pool_max_size can be set where min > max. Fix: Add Pydantic model_validator ensuring pool_min_size <= pool_max_size. Should Address3. Inconsistent Error HandlingRange violations use warn+fallback while parse errors raise exceptions. Document this design decision or make consistent. 4. Range Justification MissingAdd comments explaining why HTTP timeout is capped at 300s and sizes at 100MB. Code Quality Metrics
Final VerdictStatus: APPROVE WITH CHANGES Excellent work! Address the 2 critical issues (module-level parsing and pool size validation) before merge. The centralized util_env_parsing.py is a reusable pattern other teams can follow. |
…ncy [OMN-1058] - Replace raw float(os.environ.get()) with parse_env_float() in runtime_host_process.py - ONEX_HEALTH_CHECK_TIMEOUT: now has range validation (1.0-60.0s) - ONEX_DRAIN_TIMEOUT: now has range validation (1.0-300.0s) - Both include proper transport_type and service_name for error context - Update .env.example documentation: - Add Range/Default/Example format for all ONEX_ variables - Add deprecation notice for CONTRACTS_DIR → ONEX_CONTRACTS_DIR - Add deprecation notice for COMPUTE_REGISTRY_CACHE_SIZE → ONEX_COMPUTE_REGISTRY_CACHE_SIZE - Document HTTP handler settings (timeout, request/response size limits) - Document DB handler settings (pool size, timeout) - Document circuit breaker settings (threshold, reset timeout)
Code Review: Configuration Externalization [OMN-1058]This PR successfully externalizes ~20 hardcoded configuration values to environment variables while maintaining backward compatibility. The implementation follows ONEX patterns well with strong type safety and comprehensive test coverage. ✅ Strengths1. Excellent Type Safety & Error Handling
2. Comprehensive Test Coverage
3. Documentation Excellence
4. Backward Compatibility
5. Circuit Breaker Enhancement
🔍 Code Quality Observations1. Module-Level Parsing Pattern # handler_http.py:41-43
_DEFAULT_TIMEOUT_SECONDS: float = parse_env_float(
"ONEX_HTTP_TIMEOUT", 30.0, ...
)Impact: Env var parsing happens when module is imported, not when object is instantiated.
Recommendation: Current pattern is acceptable for MVP since:
For production hardening, consider documenting this behavior in CLAUDE.md. 2. Health Server Import-Time Safety ✅ def _get_port_from_env(default: int) -> int:
try:
return parse_env_int(...)
except ProtocolConfigurationError as e:
logger.warning("Invalid ONEX_HTTP_PORT, using default %d: %s", default, e)
return defaultThis prevents crashes if ONEX_HTTP_PORT is malformed. Consider applying this pattern to other module-level defaults. 3. Consistent Error Context ✅ parse_env_int(
"ONEX_DB_POOL_SIZE", 5,
min_value=1, max_value=100,
transport_type=EnumInfraTransportType.DATABASE, # ✅
service_name="db_handler", # ✅
)🐛 Potential IssuesNone Critical - The implementation is solid. Minor observations:
🔒 SecurityExcellent - No security concerns:
🚀 PerformanceNo Issues:
📋 Test Coverage VerificationThe PR description states "2460 unit tests pass" - excellent. Key test files added:
Recommendation: Run ✨ ONEX Pattern Compliance100% Compliant:
📝 Recommendations
✅ Approval RecommendationAPPROVE - This PR is production-ready with:
The minor recommendations above are nice-to-haves, not blockers. Great work on making ONEX infrastructure more deployment-flexible! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.env.example (2)
197-208: Deprecation notices are well-structured; consider adding removal timeline.The notices correctly explain the precedence order (new ONEX_* first, then fallback to legacy) and guide users toward migration. However, they lack a specific timeline for when the legacy variables will be removed (e.g., "Removal scheduled for v2.0.0 or [date]").
Adding a removal timeline helps users prioritize migration. Example format:
# DEPRECATION NOTICE: # ------------------- # CONTRACTS_DIR is DEPRECATED and will be removed in v2.1.0 (scheduled: Q2 2026). # Migration: Rename CONTRACTS_DIR to ONEX_CONTRACTS_DIR in your .env file. # The system currently checks ONEX_CONTRACTS_DIR first, then falls back to CONTRACTS_DIR.Also applies to: 307-314
233-281: Runtime Scheduler configuration is comprehensive and well-organized by functional category.The documentation separates settings into logical groups (Core, Restart-Safety, Performance, Circuit Breaker, Metrics, Valkey), clearly specifies ranges with units, and explains the purpose of each setting. This structure makes it easy for operators to understand and tune the scheduler.
However, Line 280–281 mentions legacy RUNTIME_SCHEDULER_* names but doesn't provide them explicitly or include a deprecation notice in the same format as CONTRACTS_DIR. Consider whether this warrants the same structured deprecation notice for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
.env.examplesrc/omnibase_infra/runtime/runtime_host_process.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
src/omnibase_infra/runtime/runtime_host_process.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.{ts,tsx,js,jsx} : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use environment variables or .env files. Use process.env with defaults or environment variable managers for all configuration values (API endpoints, service URLs, feature flags).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/runtime/runtime_host_process.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must support optional documents pattern with optional flag and required_capability field for future extensibility
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/docker-compose.yml : For Docker services connecting to Kafka/Redpanda, use `KAFKA_BOOTSTRAP_SERVERS=omninode-bridge-redpanda:9092` (internal Docker network port 9092). For host scripts, use `192.168.86.200:29092` (external published port). Do not hardcode ports - use environment variables with proper defaults.
Applied to files:
.env.example
🧬 Code graph analysis (1)
src/omnibase_infra/runtime/runtime_host_process.py (2)
src/omnibase_infra/utils/util_env_parsing.py (1)
parse_env_float(157-254)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
🔇 Additional comments (5)
.env.example (4)
345-400: Handler configuration is well-documented with clear production guidance.The HTTP and Database handler sections provide comprehensive documentation including ranges, defaults, DoS/resource protection rationale, and a helpful production example. The pool sizing guidance ("10-20 recommended for production") is particularly useful.
444-489: Idempotency store configuration is thorough with excellent trade-off documentation.The documentation clearly explains the performance/resource trade-offs (cleanup interval frequency vs. CPU usage vs. table size), provides clock skew tolerance context, and includes a high-volume system example. This level of detail supports informed operational decisions.
413-442: Circuit breaker configuration includes excellent operational guidance.The documentation explains the circuit breaker states, provides service-type-specific tuning recommendations (high-reliability vs. best-effort with concrete threshold/timeout pairs), and helps operators align configuration with service SLAs. This is a model for production configuration documentation.
1-539: Excellent documentation of externalized configuration with clear deprecation path.The .env.example file successfully documents ~20+ newly externalized configuration values with comprehensive ranges, defaults, and production examples. The deprecation notices for CONTRACTS_DIR and COMPUTE_REGISTRY_CACHE_SIZE clearly explain the fallback behavior and migration path. The addition of sections for Handler, Runtime Timeout, Circuit Breaker, and Idempotency configurations follows a consistent documentation pattern.
Coordination note: Per the past review comment, ensure that:
- Test files (
tests/unit/runtime/test_kernel.py, etc.) are updated to use the new ONEX_* variables instead of legacy names- CHANGELOG.md is updated with a migration note documenting the deprecation timeline for CONTRACTS_DIR and COMPUTE_REGISTRY_CACHE_SIZE
These tasks are outside the scope of this file but are critical for a complete migration story.
src/omnibase_infra/runtime/runtime_host_process.py (1)
63-63: Excellent externalization of configuration with proper validation.The implementation successfully moves hardcoded timeout defaults to environment variables using the centralized
parse_env_floatutility with validation and error handling. Default values are preserved for backward compatibility, and range validation ensures values stay within documented bounds.Based on learnings, this follows the established pattern of supporting environment variable overrides for timeout configuration.
Also applies to: 89-109
…deprecation timelines [OMN-1058] - Change transport_type from HTTP to RUNTIME for health check and drain timeout env parsing - Add v2.0.0 removal version and action required messages to deprecation notices
PR Review: Externalize Hardcoded Configuration Values [OMN-1058]Overall AssessmentAPPROVAL RECOMMENDED ✅ This is an excellent refactoring that significantly improves deployment flexibility by moving hardcoded values to environment variables. The implementation demonstrates strong adherence to ONEX principles with comprehensive testing, proper error handling, and excellent documentation. Strengths1. Architecture & Design ⭐⭐⭐⭐⭐
2. Error Handling ⭐⭐⭐⭐⭐
# Example from util_env_parsing.py:126-131
context = ModelInfraErrorContext(
transport_type=transport_type,
operation="parse_env_config",
target_name=service_name,
correlation_id=uuid4(),
)
raise ProtocolConfigurationError(..., context=context, value="[REDACTED]")3. Testing ⭐⭐⭐⭐⭐
4. Documentation ⭐⭐⭐⭐⭐
Issues & Recommendations🔴 CRITICAL: Missing Circuit Breaker MethodLocation: Issue: The code calls Evidence: # dlq/service_dlq_tracking.py:152-159
cb_config = ModelCircuitBreakerConfig.from_env(
service_name="dlq_tracking_service",
transport_type=EnumInfraTransportType.DATABASE,
)
self._init_circuit_breaker_from_config(cb_config) # ← Does this method exist?Action Required: Confirm 🟡 MINOR: Inconsistent Range Validation BehaviorLocation: Issue: Out-of-range values log warnings and return defaults, but this behavior may be surprising. Consider if some ranges should be hard failures. Example: If someone sets Current Behavior: if max_value is not None and parsed > max_value:
logger.warning("...using default %d", default)
return default # Silent fallbackRecommendation: Document this "soft validation" behavior prominently in the .env.example and consider adding a note that values outside ranges will use defaults (already done well in docs). 🟡 MINOR: Module-Level Parsing Side EffectsLocation: Issue: Environment variables are parsed at module import time, making it harder to test and potentially surprising. Example: # Parsed when module is imported, not when handler is instantiated
_DEFAULT_TIMEOUT_SECONDS: float = parse_env_float(
"ONEX_HTTP_TIMEOUT", 30.0, ...
)Impact:
Recommendation: This is acceptable for MVP, but consider lazy evaluation or config injection in future refactoring. Document this behavior. 🟢 ENHANCEMENT: Consider Type Aliases for Common PatternsLocation: Suggestion: The parsing functions have many repeated parameters. Consider a config class: @dataclass(frozen=True)
class EnvParsingContext:
transport_type: EnumInfraTransportType
service_name: str
# Then:
def parse_env_int(
env_var: str,
default: int,
*,
context: EnvParsingContext,
min_value: int | None = None,
max_value: int | None = None,
) -> int:
...Benefit: Reduces parameter count and groups related context together. Not critical for this PR. 🟢 ENHANCEMENT: Range Validation Edge CaseLocation: Edge Case: What if Suggestion: Add defensive validation: if min_value is not None and max_value is not None and min_value > max_value:
raise ValueError(f"min_value ({min_value}) > max_value ({max_value})")ONEX Compliance Review✅ Strong Typing: All models use proper Pydantic types, no Security Review✅ Value Redaction: Invalid config values never logged Performance Considerations✅ Module-Level Caching: Parsing happens once at import time (not per-request) Test CoverageFiles Added/Modified:
Coverage Areas:
Migration ImpactBackward Compatibility: ✅ EXCELLENT
Deployment Impact: ✅ LOW RISK
RecommendationsBefore Merge:
Post-Merge:
VerdictAPPROVED ✅ This PR represents high-quality infrastructure work that:
The code is production-ready pending verification of the circuit breaker method implementation. Estimated Risk: LOW Great work! 🎉 Review completed following ONEX guidelines from CLAUDE.md |
Update: Critical Issue Resolved ✅I've verified that the "critical issue" I flagged regarding The method exists in def _init_circuit_breaker_from_config(
self,
config: ModelCircuitBreakerConfig,
) -> None:
"""Initialize circuit breaker from a configuration model."""
self._init_circuit_breaker(
threshold=config.threshold,
reset_timeout=config.reset_timeout_seconds,
service_name=config.service_name,
transport_type=config.transport_type,
)All usages in the PR are correct:
Updated Verdict: NO BLOCKING ISSUES 🎉 This PR is ready to merge. |
…ior [OMN-1058] - Add "Range Validation Behavior" section to .env.example explaining soft validation - Document import-time env parsing in handler_http.py and handler_db.py docstrings - Include testing guidance for mocking environment variables
Pull Request Review - OMN-1058: Environment Variable ExternalizationOverall Assessment: ✅ APPROVED with Minor Suggestions This is a well-structured PR that follows ONEX conventions and significantly improves deployment flexibility. The implementation is production-ready with excellent test coverage and documentation. Strengths 🎯1. Excellent Code Organization
2. Comprehensive Test Coverage
3. Production-Ready Documentation
4. Security Considerations
Code Quality Issues 🔍Critical: Missing Test File
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/omnibase_infra/handlers/handler_http.py (1)
5-6: Update outdated timeout documentation.The docstring states "30-second fixed timeout" but the timeout is now configurable via the
ONEX_HTTP_TIMEOUTenvironment variable (with a 30-second default). Update the documentation to reflect this.🔎 Suggested documentation fix
-Supports GET and POST operations with 30-second fixed timeout. +Supports GET and POST operations with configurable timeout (default 30 seconds). PUT, DELETE, PATCH deferred to Beta. Retry logic and rate limiting deferred to Beta.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (3)
.env.examplesrc/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.{ts,tsx,js,jsx} : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use environment variables or .env files. Use process.env with defaults or environment variable managers for all configuration values (API endpoints, service URLs, feature flags).
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/.env* : Never commit `.env` files to version control. Use `.env.example` as template. All configuration must be manageable via environment variables with sensible defaults. Document all required variables with their purposes.
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Applied to files:
src/omnibase_infra/handlers/handler_db.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.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-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For all backend service HTTP calls, use HTTP/2 connection pooling with max connections (100 total, 20 keepalive), timeouts (5s connect, 10s read, 5s write), and retry logic with exponential backoff (3 attempts max, 1s→2s→4s).
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Applied to files:
src/omnibase_infra/handlers/handler_db.pysrc/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use ProtocolConfigurationError for invalid configuration scenarios
Applied to files:
src/omnibase_infra/handlers/handler_http.py
📚 Learning: 2025-11-29T17:13:38.776Z
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to {services/**/*.py,scripts/bulk_ingest_repository.py} : Implement fail-closed configuration for security hardening. All external requests must validate URLs, implement DLQ routing, and handle failures gracefully.
Applied to files:
src/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_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 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_http.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must support optional documents pattern with optional flag and required_capability field for future extensibility
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/docker-compose.yml : For Docker services connecting to Kafka/Redpanda, use `KAFKA_BOOTSTRAP_SERVERS=omninode-bridge-redpanda:9092` (internal Docker network port 9092). For host scripts, use `192.168.86.200:29092` (external published port). Do not hardcode ports - use environment variables with proper defaults.
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For Redpanda/Kafka connection patterns: Docker services use `omninode-bridge-redpanda:9092` (DNS resolves via /etc/hosts to 192.168.86.200:9092), host scripts use `192.168.86.200:29092` (direct IP with external port), remote server access uses `localhost:29092`. Never mix these contexts.
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For host scripts (bulk_ingest_repository.py, test scripts running outside Docker), use `KAFKA_BOOTSTRAP_SERVERS = os.getenv('KAFKA_BOOTSTRAP_SERVERS', '192.168.86.200:29092')` to connect to remote Redpanda on external port 29092.
Applied to files:
.env.example
🧬 Code graph analysis (2)
src/omnibase_infra/handlers/handler_db.py (2)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
src/omnibase_infra/handlers/handler_http.py (2)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
🔇 Additional comments (10)
.env.example (5)
191-209: Deprecation guidance is clear, but verify CHANGELOG.md migration notes were added.The ONEX_CONTRACTS_DIR and ONEX_HTTP_PORT documentation is well-structured with clear examples and range guidance. The deprecation notice for CONTRACTS_DIR appropriately documents the v2.0.0 timeline and fallback behavior. However, a past review flagged that CHANGELOG.md should document this migration with clear guidance for users. Please verify that migration notes have been added to CHANGELOG.md, as
.env.examplealone may not be discoverable by users reviewing release notes.
226-316: Scheduler and registry documentation is comprehensive; verify range specifications match implementation.The Runtime Scheduler Configuration (lines 233-283) and Compute Registry Configuration sections are well-documented with clear examples, memory footprint guidance, and sizing recommendations. The deprecation notices for legacy variable names follow a consistent pattern with v2.0.0 timelines.
Recommendation: Verify that the documented range constraints (e.g., tick interval 10-60000ms, cache sizes 1-10000) match the validation logic in the actual code implementation to prevent future inconsistencies.
353-418: Soft validation explanation is clear; verify that warning logs match documented behavior.The "Soft Validation" behavior explanation (lines 353-366) is a valuable addition that sets user expectations about range-constrained values. The design choice to log warnings rather than fail startup is documented with concrete examples and guidance on where to check for warnings.
Recommendation: Verify that the actual code implementation (specifically the
parse_env_intandparse_env_floatutilities mentioned in the PR summary) emits the exact log messages documented in lines 365-366. This ensures users can reliably locate validation issues in their logs.
419-507: Timeout, circuit breaker, and idempotency configurations are well-documented; verify clock skew tolerance implementation.These sections provide comprehensive configuration guidance with appropriate ranges, defaults, and production examples. The idempotency store documentation is particularly thorough, including often-overlooked details like clock skew tolerance (lines 492-495) and runaway cleanup prevention (lines 497-500).
Minor verification: Confirm that the clock skew tolerance and cleanup max iterations features are actually implemented in the idempotency store code, as these are important safety mechanisms documented here but should be validated in the actual implementation.
1-560: Overall documentation is comprehensive and well-organized; verify coverage of all ONEX_ variables.*The
.env.examplefile has been substantially enhanced with clear, production-ready documentation for the externalized configuration. The structure is logical, the ONEX_ prefix naming convention is consistently applied, and deprecation guidance for legacy variables (CONTRACTS_DIR, COMPUTE_REGISTRY_CACHE_SIZE) is clear.Final verification: Per the PR summary, approximately 12+ ONEX_* environment variables were documented. Please verify that all environment variables referenced in the code changes across the 11 modified files have corresponding entries or comments in this file to ensure completeness. Specifically, cross-check against:
- Handler configurations (HTTP timeout, request/response size limits)
- Database pool and timeout settings
- Runtime timeouts (health check, drain timeout)
- Circuit breaker settings
- Idempotency store settings
src/omnibase_infra/handlers/handler_http.py (2)
7-16: LGTM: Clear documentation on import-time behavior.The documentation clearly explains the import-time parsing behavior and provides practical testing guidance. The fail-fast approach for configuration validation at startup is a sound design choice.
49-72: LGTM: Proper environment variable parsing with validation.The implementation correctly uses
parse_env_floatandparse_env_intutilities with appropriate range validation, error handling, and context. This properly addresses the previous review concern about error handling for invalid environment variables.The ranges are reasonable:
- Timeout: 1-300 seconds
- Request/response sizes: 1KB-100MB
src/omnibase_infra/handlers/handler_db.py (3)
55-64: Excellent documentation of import-time configuration behavior.The note clearly explains the timing of environment variable parsing and provides practical testing guidance. This is especially helpful given the module-level constant pattern.
100-107: Proper validation with reasonable constraints.The use of
parse_env_intwith min/max validation addresses the previous review concern about unhandled import-time exceptions. The range 1-100 is appropriate, with the default of 5 documented as MVP-level in the comments above.
111-118: Timeout validation with appropriate flexibility.The
parse_env_floatimplementation properly handles validation with a sensible range. The max of 3600 seconds allows for long-running analytical queries while the default of 30 seconds is appropriate for typical operations.
…MN-1058] Document the design choice for environment variable range validation: - Out-of-range values log WARNING and use default (soft validation) - Invalid types still raise ProtocolConfigurationError (strict validation) - Prioritizes application availability over strict validation Includes validation matrix, alternatives considered, and monitoring guidance.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
docs/decisions/adr-soft-validation-env-parsing.md
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.{ts,tsx,js,jsx} : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use environment variables or .env files. Use process.env with defaults or environment variable managers for all configuration values (API endpoints, service URLs, feature flags).
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
docs/decisions/adr-soft-validation-env-parsing.md
📚 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 config/README.md : Document all configuration variables in config/README.md with type information, validation rules, and usage examples
Applied to files:
docs/decisions/adr-soft-validation-env-parsing.md
🔇 Additional comments (2)
docs/decisions/adr-soft-validation-env-parsing.md (2)
1-183: ADR structure and rationale are sound overall.This ADR effectively documents the soft validation decision with good context, clear trade-offs, and concrete implementation guidance. The approach of preserving application availability while logging configuration warnings is reasonable for production Kubernetes-based deployments. Ensure the pending verification items (function parameter documentation, security redaction implementation, test file paths) are resolved before merging.
45-67: Documenttransport_typeandservice_nameparameters in the implementation section.The function signatures show these parameters but provide no explanation. The implementation docstrings clarify they enable error context (
transport_typespecifies the infrastructure transport forModelInfraErrorContext, andservice_nameidentifies the service in error messages), but the ADR's implementation section does not document their purpose. Add a brief line describing their role, or reference the implementation docstrings.
| - **Clear operator feedback**: Warning logs provide actionable information | ||
| - **Safe defaults**: The system degrades gracefully to known-good values | ||
| - **Type safety maintained**: Invalid types still fail fast with exceptions | ||
| - **Security**: Invalid values are redacted in error messages to prevent credential exposure |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | grep -E "(env|parse)" | head -20Repository: OmniNode-ai/omnibase_infra
Length of output: 196
🏁 Script executed:
fd -type f -name "*env*" -o -name "*parse*" | grep -E "\.py$"Repository: OmniNode-ai/omnibase_infra
Length of output: 240
🏁 Script executed:
rg "parse_env_int|parse_env_float" -t py --files-with-matchesRepository: OmniNode-ai/omnibase_infra
Length of output: 681
🏁 Script executed:
cat -n src/omnibase_infra/utils/util_env_parsing.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 11028
🏁 Script executed:
rg "def parse_env_int|def parse_env_float" -A 20 src/omnibase_infra/utils/util_env_parsing.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 1878
🏁 Script executed:
rg "error|exception|logger|warning" -B 3 -A 3 src/omnibase_infra/utils/util_env_parsing.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 6061
Security claim about redacting invalid values is incomplete.
Line 134 claims "Invalid values are redacted in error messages," but the implementation only redacts values in parse errors (using [REDACTED]). Range validation scenarios expose actual numeric values in logger.warning calls (lines 135-150 in parse_env_int and 235-250 in parse_env_float). Either redact values in all validation scenarios or clarify the claim to specify only parse errors are redacted.
🤖 Prompt for AI Agents
In docs/decisions/adr-soft-validation-env-parsing.md around line 134, the
statement "Invalid values are redacted in error messages" is inaccurate because
only parse errors are redacted while range validation warnings log actual values
(see parse_env_int lines ~135-150 and parse_env_float lines ~235-250); update
wording to specify that only parse errors are redacted, or change the code paths
to redact values in range validation logs as well so the claim is true—pick one:
(a) edit the sentence to "Parse errors redact invalid values; range validation
may log numeric values" or (b) modify the validation logging calls to replace
exposed values with a redaction token like [REDACTED] and ensure examples and
notes reflect that behavior.
PR Review: Environment Variable Configuration ExternalizationSummaryThis PR successfully externalizes 12+ hardcoded configuration values to environment variables, improving deployment flexibility. The implementation is excellent overall with strong adherence to ONEX patterns. Strengths1. Outstanding Documentation
2. Excellent Error Handling & Security
3. Comprehensive Test Coverage
4. ONEX Pattern Compliance
Code Quality Issues1. CRITICAL: Backwards Compatibility ViolationCLAUDE.md explicitly forbids backwards compatibility: ## CRITICAL POLICIES
### No Backwards Compatibility
- Breaking changes are always acceptable
- Remove old patterns immediatelyIssues:
Recommended Action: Create a follow-up PR to remove all backwards compatibility code and documentation. 2. Missing max_value ValidationCircuit Breaker Config: The Recommendation: Add Security AssessmentExcellent Security Practices:
Security Concerns: None identified Performance ConsiderationsGood Practices:
RecommendationsMust Fix (Before Merge)
Should Fix (Follow-up PR)
Consider (Beta)
Final VerdictAPPROVE with required changes This PR demonstrates excellent engineering:
Blocking issue: Remove backwards compatibility code/docs per CLAUDE.md policy before merge. Once backwards compatibility is removed, this PR is production-ready. Test Plan Verified: All 2460 tests passing, pre-commit hooks passing Files Changed: 24 files (+3281 lines / -123 lines) Great work! |
… test coverage [OMN-1058] - Fix ADR security claim to distinguish type errors (redacted) from range warnings (visible) - Update ADR code snippet with complete parameters and correct float format - Add centralized env parsing to DLQ tracking config with ProtocolConfigurationError - Add dedicated test suite for util_env_parsing.py (66 tests) - Add tests for MixinAsyncCircuitBreaker._init_circuit_breaker_from_config() (5 tests) - Document DLQ environment variables in .env.example
Pull Request Review: Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes ~20 hardcoded configuration values to environment variables, improving deployment flexibility and operational control. The implementation is well-designed, thoroughly tested, and production-ready. The soft validation pattern is a pragmatic choice that balances availability with safety. Overall Assessment: ✅ APPROVED with minor suggestions 🎯 Strengths1. Excellent Architecture PatternThe introduction of
2. Well-Documented "Soft Validation" PatternThe ADR (
This pattern prioritizes availability over strict validation, which is appropriate for containerized deployments where misconfiguration shouldn't cause outages. 3. Comprehensive Test CoverageTest suite demonstrates production-grade quality:
4. Outstanding Documentation in .env.exampleThe
5. Security Considerations
6. Backward Compatibility
🔍 Code Quality Analysis
|
| Rule | Status | Notes |
|---|---|---|
Strong typing (no Any) |
✅ | Uses object for generic payloads |
PEP 604 unions (X | None) |
✅ | Consistently used throughout |
| One model per file | ✅ | N/A - utility functions, not models |
| Container injection | ✅ | Handlers use ModelONEXContainer |
| Error hierarchy | ✅ | Proper use of ProtocolConfigurationError, InfraConnectionError, etc. |
| Error sanitization | ✅ | Credentials redacted, safe values logged |
| Correlation ID propagation | ✅ | Included in all error contexts |
| Circuit breaker pattern | ✅ | ModelCircuitBreakerConfig.from_env() |
Compliance: 100% ✅
📊 Test Coverage Assessment
Based on PR diff:
- New test files: 3 comprehensive test suites
- Total test additions: 2,377 lines of test code
- Test-to-code ratio: Excellent (~1.7:1 for new utilities)
Coverage areas:
- ✅ Unit tests for
util_env_parsing.py - ✅ Integration tests for handler configuration
- ✅ Error context validation
- ✅ Edge cases (whitespace, scientific notation, boundaries)
- ✅ Security validation (value redaction)
Missing coverage (acceptable for MVP):
⚠️ No integration tests for environment variable changes requiring app restart⚠️ No load tests for idempotency store cleanup at high volume
These gaps are acceptable for an MVP and can be addressed in Beta.
🔒 Security Review
Strengths:
- ✅ Value redaction: Type errors redact invalid values
- ✅ DoS protection: Request/response size limits prevent memory exhaustion
- ✅ No credential exposure: DSN never logged or included in errors
- ✅ Size categorization: Prevents attackers from probing exact limits
Potential Concerns:
1. Range Validation Logs Actual Values (Low Risk)
The soft validation pattern logs actual numeric values when out of range:
logger.warning(
"Environment variable %s value %f is below minimum %f, using default %f",
env_var, parsed, min_value, default,
)Risk Assessment: LOW - These are numeric configuration values (timeouts, pool sizes), not secrets. However, in paranoid security contexts, exact values could provide reconnaissance data.
Recommendation: Document this behavior in the ADR (already done ✅).
2. HTTP Response Size Categorization
The _categorize_size() function prevents exact size disclosure but still reveals categories. This is a reasonable security/usability tradeoff.
🎯 Performance Considerations
1. Module-Level Environment Parsing ✅
The choice to parse environment variables at module import time (not handler instantiation) is correct:
Benefits:
- Fail-fast validation at startup
- No parsing overhead per request
- Clear separation of configuration vs runtime
Tradeoff: Requires app restart for configuration changes (documented in code comments ✅)
2. Idempotency Cleanup Batching ✅
The default ONEX_IDEMPOTENCY_BATCH_SIZE=10000 is well-chosen:
- Balances transaction size vs lock contention
- Configurable for tuning based on load
3. Double-Serialization Avoidance ✅
The HTTP handler caches serialized bytes during request size validation (handler_http.py:388-408). This is an excellent optimization that prioritizes CPU efficiency over peak memory usage.
📝 Documentation Quality
Excellent:
- ✅ ADR: Clear rationale, alternatives, consequences
- ✅
.env.example: Comprehensive with ranges, examples, security notes - ✅ Inline comments: Design tradeoffs explained (e.g., double-serialization)
- ✅ Docstrings: Complete with examples and security notes
Suggestions:
- Add environment variable reference to main
README.md(if not already present) - Consider a "Configuration Best Practices" document for operators
🚀 Deployment Recommendations
Pre-Deployment Checklist:
-
✅ Log Monitoring: Configure alerts for range validation warnings
- alert: ONEXConfigRangeWarning expr: count_over_time({app="onex"} |= "is below minimum" or "is above maximum" [5m]) > 0
-
✅ Documentation: Update operator runbooks with new environment variables
-
✅ Smoke Testing: Test with intentionally out-of-range values to verify soft validation
-
⚠️ Migration Path: Document process for migrating from hardcoded to env-based config
Production Tuning Guidance:
Add to operator documentation:
# Production recommendations:
ONEX_HTTP_TIMEOUT=60.0 # Higher for slow APIs
ONEX_DB_POOL_SIZE=15 # 10-20 for production
ONEX_IDEMPOTENCY_TTL_SECONDS=43200 # 12 hours for high-volume
ONEX_CB_THRESHOLD=3 # Strict for critical services✅ Final Verdict
This PR is production-ready and demonstrates excellent engineering practices.
Strengths Summary:
- ✅ Well-architected centralized parsing utilities
- ✅ Comprehensive test coverage (>2300 lines of tests)
- ✅ Excellent documentation (ADR + inline + .env.example)
- ✅ Security-conscious implementation
- ✅ Backward compatible with clear migration path
- ✅ 100% ONEX compliance
Minor Improvements:
- Consider DEBUG logging when using defaults
- Add version numbers to deprecation notices
- Document high-volume tuning for idempotency cleanup
- Optional: Add soft max validation for circuit breaker threshold
Recommendation:
MERGE - The minor suggestions above can be addressed in follow-up PRs if needed. This implementation is solid, well-tested, and ready for production deployment.
Great work on this refactor! The soft validation pattern is a pragmatic solution to a real operational problem, and the implementation quality is excellent. 🎉
Review conducted following ONEX infrastructure guidelines and CLAUDE.md conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/utils/test_util_env_parsing.py (1)
917-969: Consider adding GRPC and RUNTIME to transport type tests.The parametrized tests cover DATABASE, KAFKA, HTTP, CONSUL, VAULT, and VALKEY, but omit GRPC and RUNTIME from
EnumInfraTransportType. While these may be less common for environment parsing scenarios, including them would ensure complete coverage.🔎 Suggested addition to parametrized tests
@pytest.mark.parametrize( "transport_type", [ EnumInfraTransportType.DATABASE, EnumInfraTransportType.KAFKA, EnumInfraTransportType.HTTP, EnumInfraTransportType.CONSUL, EnumInfraTransportType.VAULT, EnumInfraTransportType.VALKEY, + EnumInfraTransportType.GRPC, + EnumInfraTransportType.RUNTIME, ], )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (5)
.env.exampledocs/decisions/adr-soft-validation-env-parsing.mdsrc/omnibase_infra/dlq/models/model_dlq_tracking_config.pytests/unit/mixins/test_mixin_async_circuit_breaker.pytests/unit/utils/test_util_env_parsing.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/decisions/adr-soft-validation-env-parsing.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use container injection pattern - def init(self, container: ModelONEXContainer) for all services and nodes
NEVER use Any type - use object for generic payloads instead
Use PEP 604 union syntax (X | None) instead of Optional[X] for nullable types
Use ModelEventEnvelope[object] for generic dispatchers to accept any event type
Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Circuit breaker service_name must be set and transport_type must be specified from EnumInfraTransportType
Always propagate correlation IDs from incoming requests, auto-generate with uuid4() if missing, and include in all error context
NEVER include passwords, API keys, PII, or connection strings with credentials in error messages - only include service names, operation names, correlation IDs, and ports
Prefix internal or sensitive methods with underscore (_) to exclude them from Node Introspection exposure
Use generic parameter names in method signatures (e.g., data not user_credentials) to avoid exposing sensitive data through introspection
Use ProtocolConfigurationError for invalid configuration scenarios
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, and InfraUnavailableError for corresponding infrastructure failure scenarios
Use type alias pattern with underscore prefix (_IntentUnion) for Pydantic validation unions, separate from protocol definitions used in function signatures
Use duck typing through protocols rather than isinstance checks for protocol resolution
Files:
tests/unit/utils/test_util_env_parsing.pytests/unit/mixins/test_mixin_async_circuit_breaker.pysrc/omnibase_infra/dlq/models/model_dlq_tracking_config.py
**/model_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/model_*.py: All data structures must be proper Pydantic models - one model per file with naming pattern model_.py and class pattern Model
Result models may override bool to enable idiomatic conditional checks, with Warning section in docstring explaining non-standard behavior
Files:
src/omnibase_infra/dlq/models/model_dlq_tracking_config.py
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : NO environment variables shall EVER be hardcoded in code files. ALL configuration MUST use `.env`. Use `os.getenv()` with defaults or Pydantic Settings (BaseSettings with Field and env parameter) for all configuration values (API endpoints, model names, dimensions, database credentials, timeouts).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use Pydantic Settings (BaseSettings with Field annotation and env parameter) for configuration management instead of direct os.getenv() calls when building configuration classes.
Learnt from: sudharsanv177
Repo: OmniNode-ai/omninode_infra PR: 4
File: docker/onex-api/main.py:0-0
Timestamp: 2025-12-19T02:58:44.081Z
Learning: In the omninode_infra repository, production configuration is managed via Kubernetes Secrets and ConfigMaps injected as environment variables, not committed .env files or Pydantic Settings. The deployment model uses os.getenv() with sensible defaults for local development, and explicit resolution patterns (e.g., checking POSTGRES_DSN first, then deriving from component variables) are preferred over mutating os.environ.
Learnt from: CR
Repo: OmniNode-ai/omniarchon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T17:13:38.776Z
Learning: Applies to **/*.py : NO environment variables hardcoded in code. ALL configuration must be provided via `.env` file. Use Pydantic Settings with `BaseSettings` and `Field` with `env` parameter for configuration management.
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-03T16:55:49.755Z
Learning: Use environment variables for all sensitive configuration values (API keys, database passwords) rather than hardcoding them
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All Docker services must receive environment variables via `docker-compose.yml`. Scripts reading configuration must support `.env` files via python-dotenv or similar. Never assume environment variables are set without defaults.
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
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/**/{config,settings}/**/*.py : Environment variables MUST include: POSTGRES_HOST, POSTGRES_PORT, POSTGRES_DATABASE, POSTGRES_USER, POSTGRES_PASSWORD, KAFKA_BOOTSTRAP_SERVERS, CONSUL_HOST, CONSUL_PORT, LOG_LEVEL. Use secrets manager for production passwords.
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node contract definitions must use the new subcontract architecture pattern, breaking down complex contracts into separate contract_actions.yaml, contract_models.yaml, contract_validation.yaml, and optional contract_cli.yaml and contract_capabilities.yaml files for separation of concerns, maintainability, reusability, modularity, and future tool-as-a-service readiness
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX node deviations from canonical patterns must be documented and justified in the node's root-level README.md and subject to maintainer review
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: All ONEX nodes must conform to the canonical structure, code generation, and interface patterns established in the `node_cli` node, using it as the primary source of truth for directory structure, contract schema patterns, linked document architecture, base state patterns, shared schema references, extensibility patterns, CLI interface declarations, code generation, dependency injection, error handling, testing, and documentation
Applied to files:
.env.example
📚 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: Remove all backward compatibility patterns and legacy support code; use proper ONEX patterns from day one
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/node.onex.yaml : All ONEX nodes must include a `node.onex.yaml` file containing schema-valid node metadata
Applied to files:
.env.example
📚 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: Manual deployment scripts (scripts/rebuild-service.sh, scripts/migrate-to-remote.sh) take precedence for production deployments until automated ONEX workflows complete validation phase
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must follow the linked document architecture pattern with contract.yaml linking to node_config.yaml and deployment_config.yaml as associated documents
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contracts/contract_*.yaml : All ONEX node subcontracts must be organized in a `contracts/` subdirectory within the versioned implementation directory with separate files for contract_actions.yaml, contract_models.yaml, contract_validation.yaml, contract_cli.yaml (optional), and contract_capabilities.yaml (optional)
Applied to files:
.env.example
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/models/model_contract_*.py : All ONEX node auto-generated Pydantic models must be organized in a `models/` directory with files for state.py, model_contract_actions.py, model_contract_models.py, model_contract_validation.py, model_contract_cli.py (optional), model_contract_capabilities.py (optional), and error_codes.py, generated from the corresponding contract definitions
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : All configuration classes using Pydantic must validate that no hardcoded secrets or sensitive defaults exist. Use Field(..., description=...) for all parameters. Generate comprehensive .env.example templates documenting all variables with descriptions.
Applied to files:
.env.examplesrc/omnibase_infra/dlq/models/model_dlq_tracking_config.py
📚 Learning: 2025-11-24T17:23:49.777Z
Learnt from: CR
Repo: OmniNode-ai/omniclaude PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T17:23:49.777Z
Learning: Applies to **/node_*/v[0-9]*_[0-9]*_[0-9]*/contract.yaml : All ONEX node contract definitions must support optional documents pattern with optional flag and required_capability field for future extensibility
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/docker-compose.yml : For Docker services connecting to Kafka/Redpanda, use `KAFKA_BOOTSTRAP_SERVERS=omninode-bridge-redpanda:9092` (internal Docker network port 9092). For host scripts, use `192.168.86.200:29092` (external published port). Do not hardcode ports - use environment variables with proper defaults.
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For Redpanda/Kafka connection patterns: Docker services use `omninode-bridge-redpanda:9092` (DNS resolves via /etc/hosts to 192.168.86.200:9092), host scripts use `192.168.86.200:29092` (direct IP with external port), remote server access uses `localhost:29092`. Never mix these contexts.
Applied to files:
.env.example
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For host scripts (bulk_ingest_repository.py, test scripts running outside Docker), use `KAFKA_BOOTSTRAP_SERVERS = os.getenv('KAFKA_BOOTSTRAP_SERVERS', '192.168.86.200:29092')` to connect to remote Redpanda on external port 29092.
Applied to files:
.env.example
📚 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 tests/**/*.py : Write comprehensive test coverage following the test structure under `tests/unit/` organized by subsystem (enums, models, mixins, utils)
Applied to files:
tests/unit/utils/test_util_env_parsing.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Infrastructure error context must include transport_type from EnumInfraTransportType, operation name, and correlation_id
Applied to files:
tests/unit/utils/test_util_env_parsing.py
📚 Learning: 2025-12-27T15:57:54.635Z
Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-27T15:57:54.635Z
Learning: Applies to **/*.py : Use MixinAsyncCircuitBreaker for external service integrations with proper threshold and reset_timeout configuration
Applied to files:
tests/unit/mixins/test_mixin_async_circuit_breaker.py
📚 Learning: 2025-12-07T17:50:13.678Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T17:50:13.678Z
Learning: Applies to **/*.py : Use Pydantic Settings for configuration with environment variables (e.g., ModelIntelligenceConfig.from_environment_variable() for INTELLIGENCE_SERVICE_URL, INTELLIGENCE_TIMEOUT, etc.)
Applied to files:
src/omnibase_infra/dlq/models/model_dlq_tracking_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : Use centralized timeout configuration from `config/timeout_config.py` via functions like `get_http_timeout()`, `get_db_timeout()`, `get_cache_timeout()`, `get_async_timeout()`. Never hardcode timeout values. Support environment variable overrides (e.g., HTTP_TIMEOUT_INTELLIGENCE).
Applied to files:
src/omnibase_infra/dlq/models/model_dlq_tracking_config.py
📚 Learning: 2025-11-29T22:07:25.230Z
Learnt from: CR
Repo: OmniNode-ai/omniintelligence PR: 0
File: migration_sources/omniarchon/CLAUDE.md:0-0
Timestamp: 2025-11-29T22:07:25.230Z
Learning: Applies to migration_sources/omniarchon/**/*.py : For all backend service HTTP calls, use HTTP/2 connection pooling with max connections (100 total, 20 keepalive), timeouts (5s connect, 10s read, 5s write), and retry logic with exponential backoff (3 attempts max, 1s→2s→4s).
Applied to files:
src/omnibase_infra/dlq/models/model_dlq_tracking_config.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/dlq/models/model_dlq_tracking_config.py
🧬 Code graph analysis (3)
tests/unit/utils/test_util_env_parsing.py (3)
src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)src/omnibase_infra/errors/error_infra.py (1)
ProtocolConfigurationError(154-189)src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)
tests/unit/mixins/test_mixin_async_circuit_breaker.py (3)
src/omnibase_infra/models/resilience/model_circuit_breaker_config.py (2)
ModelCircuitBreakerConfig(43-222)from_env(139-222)src/omnibase_infra/mixins/mixin_async_circuit_breaker.py (4)
MixinAsyncCircuitBreaker(145-592)_init_circuit_breaker_from_config(282-324)_check_circuit_breaker(326-423)_record_circuit_failure(425-515)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
src/omnibase_infra/dlq/models/model_dlq_tracking_config.py (2)
src/omnibase_infra/utils/util_env_parsing.py (2)
parse_env_float(157-254)parse_env_int(57-154)src/omnibase_infra/enums/enum_infra_transport_type.py (1)
EnumInfraTransportType(28-52)
🔇 Additional comments (21)
src/omnibase_infra/dlq/models/model_dlq_tracking_config.py (4)
11-16: Clear documentation of environment variables.The docstring properly documents all three environment variables with their defaults and valid ranges, aligning with the project's documentation standards.
34-57: Module-level env parsing correctly implemented.The environment parsing follows the established ONEX pattern with proper transport type context, service name, and range validation. The comment accurately describes the behavior: invalid types raise
ProtocolConfigurationError, out-of-range values log warnings and use defaults.One consideration: if an operator sets
ONEX_DLQ_POOL_MIN_SIZE=10andONEX_DLQ_POOL_MAX_SIZE=5, both pass individual range validation but the model'svalidate_pool_sizesvalidator will catch the inconsistency at instantiation time, which is the correct layered validation approach.
146-169: Field definitions properly reference environment-driven defaults.The field constraints (
ge/le) match the range validation used during environment parsing, providing defense-in-depth validation. The updated descriptions improve discoverability by documenting the corresponding environment variable names.
171-202: Pool size relationship validator correctly enforces consistency.The after-validation hook properly ensures
pool_max_size >= pool_min_size, which is essential when environment variables can set these values independently. The error context follows ONEX conventions with transport type, operation, target name, and correlation ID.tests/unit/utils/test_util_env_parsing.py (5)
1-44: Comprehensive test suite documentation.The module docstring provides excellent organization with clear test class descriptions, coverage goals, and cross-references to related test files. This follows best practices for test maintainability.
46-159: Comprehensive basic functionality tests forparse_env_int.Tests cover all essential scenarios: default value, valid integers (positive/negative/zero), and various error cases (invalid strings, float strings, empty strings, whitespace). The use of
patch.dict(os.environ, ..., clear=True)ensures proper test isolation.
161-281: Thorough range validation tests with boundary coverage.The tests properly verify the soft validation pattern: out-of-range values log warnings and return defaults, while boundary values are accepted. The use of
caplogfixture validates warning messages are correctly logged.
283-386: Security-focused error context validation.The test at lines 367-386 is particularly important—it verifies that sensitive values are redacted and never appear in error messages or context. This aligns with the coding guidelines: "NEVER include passwords, API keys, PII, or connection strings with credentials in error messages."
622-727: Excellent edge case coverage for float parsing.The special formats tests cover scientific notation (positive/negative exponents, uppercase E, explicit plus sign), very small/large values, and unconventional formats like leading (
.5) and trailing (5.) decimal points. Usingpytest.approxfor floating-point comparisons is the correct approach.tests/unit/mixins/test_mixin_async_circuit_breaker.py (3)
34-34: Correct use of TYPE_CHECKING for forward reference.The
TYPE_CHECKINGguard prevents circular imports at runtime while allowing type hints forModelCircuitBreakerConfig. The string annotation"ModelCircuitBreakerConfig"in the stub's__init__is consistent with this pattern.Also applies to: 46-48
602-635: Well-designed stub for config-based initialization tests.The
CircuitBreakerConfigServiceStubappropriately focuses on config-based initialization via_init_circuit_breaker_from_config. The thread-safe wrappers correctly acquire_circuit_breaker_lockbefore accessing circuit breaker state.
637-757: Comprehensive test coverage for config-based circuit breaker initialization.The test class covers:
- Default values validation
- Custom values propagation
- Functional correctness (circuit opens after failures, error context is correct)
- Environment-based configuration via
from_env()- All 8 transport types including GRPC and RUNTIME
The
test_init_from_config_circuit_functions_correctlytest is particularly valuable as it validates the complete flow from config to error context..env.example (9)
191-210: Well-documented ONEX_HTTP_PORT and CONTRACTS_DIR migration guidance. Lines 191-210 provide clear context for the HTTP port with ranges and examples, plus a clear deprecation notice for the CONTRACTS_DIR → ONEX_CONTRACTS_DIR migration (with v2.0.0 timeline).
226-282: Excellent Runtime Scheduler configuration documentation. Lines 226-282 comprehensively document tick interval, scheduler ID, restart-safety settings (sequence persistence), performance (jitter), circuit breaker settings, metrics, and Valkey settings. Each parameter includes range constraints, practical examples, and a deprecation note for legacy RUNTIME_SCHEDULER_* names. This aligns well with the PR's goal of externalizing configuration.
284-316: Compute Registry cache sizing guidance is thorough. Lines 284-316 provide sizing guidelines for small/medium/large deployments with memory footprint calculations, plus a clear deprecation path from COMPUTE_REGISTRY_CACHE_SIZE to ONEX_COMPUTE_REGISTRY_CACHE_SIZE (v2.0.0 timeline).
347-417: Exceptional Handler Configuration documentation with soft-validation semantics. Lines 347-417 clearly explain the "soft validation" behavior (lines 353–367: out-of-range values log warnings and use defaults rather than failing startup), detail HTTP handler timeouts and size limits with ranges, database pool and query timeout settings, and provide a production example configuration. This aligns with the PR's goal of externalizing handler defaults while preserving backward compatibility.
419-428: Runtime timeout settings are well-documented. Lines 419–428 provide ONEX_HEALTH_CHECK_TIMEOUT and ONEX_DRAIN_TIMEOUT with clear ranges and purposes, supporting the graceful shutdown and health probe requirements.
430-459: Circuit breaker configuration includes pattern explanation and tuning guidance. Lines 430–459 document ONEX_CB_THRESHOLD and ONEX_CB_RESET_TIMEOUT with a clear state machine explanation (CLOSED→OPEN→HALF_OPEN) and practical tuning guidelines for high-reliability vs. best-effort services. Well-structured for operational clarity.
461-506: Idempotency store configuration is comprehensive and production-ready. Lines 461–506 document TTL, cleanup interval, batch size, pool settings, clock-skew tolerance, and max iterations with ranges and a high-volume example configuration. The explanation of batched deletion reducing lock contention and the clock-skew buffer are valuable operational details.
508-523: DLQ Tracking configuration rounds out the infrastructure settings. Lines 508–523 document connection pool and timeout settings for PostgreSQL-based DLQ replay tracking with an example for high-volume operations.
353-367: Soft-validation behavior is correctly and consistently implemented. The documented semantics (out-of-range values log warnings and use defaults) are fully implemented in all parse_env utilities and configuration classes:
parse_env_intandparse_env_float(util_env_parsing.py) log warnings and return defaults for out-of-range valueshandler_http.pyandhandler_db.pyuse these functions with documented range constraintsModelCircuitBreakerConfig.from_env()applies soft validation for threshold and reset_timeoutModelPostgresIdempotencyStoreConfigapplies soft validation for all 8 environment variables (TTL, cleanup interval, pool sizes, timeouts, etc.)All implementations produce log messages matching the documented patterns (e.g., "Environment variable X value Y is above/below maximum/minimum Z, using default W").
…ts, and docs [OMN-1058] - Add max_value validation to circuit breaker from_env() (threshold<=1000, timeout<=3600s) - Clarify ADR security rationale for logging numeric values after successful parsing - Add warning message format documentation for both int (%d) and float (%f) types - Add tests for circuit breaker from_env() max_value validation - Add GRPC and RUNTIME transport type tests in util_env_parsing - Enhance .env.example with deprecation timelines and migration paths
PR Review: Environment Variable Externalization [OMN-1058]Executive SummaryThis is an excellent, production-ready PR that successfully externalizes 20+ hardcoded configuration values to environment variables. The implementation demonstrates exceptional software engineering discipline with comprehensive testing (1117 test cases in util_env_parsing alone), thorough documentation, and careful security considerations. Recommendation: ✅ APPROVE - Ready to merge Strengths1. Exemplary Architecture & Code Quality
2. Comprehensive Test Coverage
Test quality is exceptional with coverage of:
3. Excellent Documentation
4. Security Considerations✅ Value redaction: Type errors redact invalid values to prevent credential exposure 5. Backwards Compatibility✅ Legacy fallbacks: Minor Observations (Non-Blocking)1. Lazy Import PatternLocation: The use of lazy imports to avoid circular dependencies is a pragmatic solution: def parse_env_int(...):
# Lazy imports to avoid circular dependency
from omnibase_infra.enums import EnumInfraTransportType
from omnibase_infra.errors import ModelInfraErrorContext, ProtocolConfigurationErrorAnalysis: This is acceptable given the module-level parsing requirements, but consider if restructuring imports could eliminate the need for lazy loading in future refactoring. 2. Module-Level ParsingLocation: Environment variables are parsed at module import time: _DEFAULT_TIMEOUT_SECONDS: float = parse_env_float(
"ONEX_HTTP_TIMEOUT",
30.0,
min_value=1.0,
max_value=300.0,
transport_type=EnumInfraTransportType.HTTP,
service_name="http_handler",
)Analysis: This is intentional per the docstrings ("startup-time validation") and is appropriate for configuration that shouldn't change during runtime. The soft validation pattern ensures imports never crash, which is the right tradeoff. 3. Error Message PrecisionLocation: Float parsing error says "expected numeric value" while int parsing says "expected integer". Consider consistency:
Current approach is fine, but if you prefer strict precision, line 227 could say "expected float" instead. 4. Transport Type DefaultingLocation: Both functions default to ONEX Compliance Verification✅ Strong typing: No Testing VerificationVerified test execution: Specific File Highlights
|
…cation timelines [OMN-1058] - Update ADR code snippet to include all required parameters and constants - Add type hints, clear=True, and caplog.at_level() to match actual tests - Add explicit v2.0.0 removal timeline to docker/.env.example deprecation notice - Consistent deprecation format with migration instructions
Pull Request Review - Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes ~20 hardcoded configuration values to environment variables, significantly improving deployment flexibility. The implementation follows ONEX patterns consistently and includes comprehensive testing. Overall Assessment: ✅ APPROVED with minor observations Strengths1. Architecture & Design Excellence
2. Security Best Practices✅ Value Redaction: Invalid values are consistently redacted in error messages ( Example from raise ProtocolConfigurationError(
f"Invalid value for {env_var} environment variable: expected integer",
context=context,
parameter=env_var,
value="[REDACTED]", # ✅ Security-conscious
) from None3. Test Coverage Excellence1,117 test lines added for environment parsing utilities alone:
Coverage areas:
4. Code Quality
5. Backward CompatibilityMigration path is clear and well-documented:
Observations & Recommendations1. Module-Level Parsing Trade-offLocation: Environment variables are parsed at module import time: _DEFAULT_TIMEOUT_SECONDS: float = parse_env_float(
"ONEX_HTTP_TIMEOUT",
30.0,
min_value=1.0,
max_value=300.0,
transport_type=EnumInfraTransportType.HTTP,
service_name="http_handler",
)Observation: Rationale: Startup-time validation catches configuration errors early, preventing runtime failures. Recommendation: ✅ This is acceptable for the MVP. The documentation is clear and the trade-off is reasonable. 2. Default Transport Type FallbackLocation: if transport_type is None:
transport_type = EnumInfraTransportType.HTTPObservation: Impact: Low - error messages will still be actionable, just less specific. Recommendation: Consider requiring 3. Pre-Serialization Memory Trade-offLocation: The HTTP handler pre-serializes dict request bodies during size validation to avoid double serialization: # Serialize dict bodies once here during validation and cache the bytes
serialized_bytes = json.dumps(body).encode("utf-8")
size = len(serialized_bytes)Observation: Assessment: ✅ Well-documented trade-off with clear rationale in comments. The approach prioritizes CPU efficiency over peak memory, which is appropriate given the enforced size limits. 4. Circuit Breaker Configuration PatternLocation: The config = ModelCircuitBreakerConfig.from_env(
service_name="kafka.production",
transport_type=EnumInfraTransportType.KAFKA,
)
self._init_circuit_breaker_from_config(config)Assessment: ✅ Excellent pattern. Service name and transport type are correctly not read from environment (context-specific, should be provided by code). Performance ConsiderationsRange Validation PerformanceSoft validation approach has minimal performance impact:
Assessment: ✅ No performance concerns. Connection Pool SizingNew environment variables for pool configuration:
Recommendation: Document recommended production values in deployment guides. Default of 5 may be conservative for high-traffic scenarios. Documentation QualityADR: Soft Validation PatternLocation: ✅ Excellent ADR covering:
.env.example Enhancements✅ Comprehensive documentation:
Example: # ONEX_HTTP_TIMEOUT=30.0 # Range: 1.0-300.0 seconds
# Example: 30.0 (standard), 60.0 (slow APIs), 10.0 (fast local services)ONEX Compliance✅ Follows CLAUDE.md Guidelines
✅ No Backwards Compatibility HacksPer CLAUDE.md: "No backwards compatibility" policy is appropriately balanced with:
Test Plan VerificationFrom PR description:
Additional verification recommended:
Security Assessment✅ No Security Concerns
Security Highlights
Final RecommendationsMust Address (None)No blocking issues identified. Should Consider (Future Improvements)
Nice to Have
ConclusionThis PR represents high-quality work that significantly improves ONEX infrastructure's operational flexibility: ✅ Clean Architecture: Centralized parsing utilities with consistent patterns The soft validation approach is well-reasoned and appropriate for production deployments. Approval Status: ✅ APPROVEDGreat work on this refactor! The codebase is more maintainable and deployment-friendly with these changes. Reviewed by: Claude (ONEX Infrastructure Code Review) |
…s [OMN-1058] - Remove HTTP fallback for transport_type - was bad practice that could lead to misleading error context in non-HTTP services - Make transport_type a required parameter (no default value) - Update docstrings to reflect required parameter - Remove TestDefaultTransportType tests (tested removed behavior) - All callers already provide transport_type explicitly
PR Review: Environment Configuration Externalization [OMN-1058]SummaryThis PR successfully externalizes ~20 hardcoded configuration values to environment variables. The implementation demonstrates excellent adherence to ONEX Infrastructure guidelines with comprehensive test coverage. Overall Assessment: ✅ APPROVED Strengths1. Architecture & Design Excellence
2. Error Handling & Observability
3. Testing CoverageThe test suite is exemplary:
4. Documentation Quality
Code Quality✅ ONEX Compliance
✅ Security Best Practices
Minor Observations (Non-Blocking)
Assessment✅ Performance: No Concerns
✅ Security: No Concerns
✅ Test Coverage: Excellent
RecommendationsFor This PR: ✅ Ready to MergeNo blocking issues. Future Enhancements (Optional)
Final Verdict✅ APPROVED This PR represents high-quality infrastructure work:
Recommendation: Merge when CI passes. Checklist
Great work! 🎉 |
1 participant
Summary
Move ~20 hardcoded configuration values to environment variables for improved deployment flexibility, addressing OMN-1058.
from_env()class method toModelCircuitBreakerConfigAll default values preserved for backward compatibility.
Environment Variables Added (12 total)
ONEX_HTTP_TIMEOUTONEX_HTTP_MAX_REQUEST_SIZEONEX_HTTP_MAX_RESPONSE_SIZEONEX_DB_POOL_SIZEONEX_DB_TIMEOUTONEX_HEALTH_CHECK_TIMEOUTONEX_DRAIN_TIMEOUTONEX_HTTP_PORTONEX_CB_THRESHOLDONEX_CB_RESET_TIMEOUTONEX_IDEMPOTENCY_TTL_SECONDSONEX_IDEMPOTENCY_CLEANUP_INTERVALONEX_IDEMPOTENCY_BATCH_SIZEFiles Changed (11)
.env.example- Added new environment variable documentationhandlers/handler_http.py- Externalized timeout and size limitshandlers/handler_db.py- Externalized pool size and timeoutruntime/runtime_host_process.py- Externalized health check and drain timeoutsruntime/health_server.py- Externalized HTTP portmodels/resilience/model_circuit_breaker_config.py- Addedfrom_env()methodprojectors/projector_registration.py- UseModelCircuitBreakerConfig.from_env()projectors/projection_reader_registration.py- UseModelCircuitBreakerConfig.from_env()projectors/snapshot_publisher_registration.py- UseModelCircuitBreakerConfig.from_env()dlq/service_dlq_tracking.py- UseModelCircuitBreakerConfig.from_env()idempotency/models/model_postgres_idempotency_store_config.py- Externalized defaultsTest plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.