Repository navigation
fix: purge localhost fallbacks from omnibase_infra [OMN-7227] - #1127
Merged
jonahgabriel merged 21 commits intoApr 3, 2026
Merged
Conversation
…[OMN-7227] Remove all localhost:19092 and localhost:5432 fallback defaults from Kafka, Postgres, Qdrant, and graph handler configs. All connection strings now require explicit env vars (KAFKA_BOOTSTRAP_SERVERS, DATABASE_URL, etc.) — silent fallback to localhost masked missing config in non-local environments. Adds validate_no_env_fallbacks.py CI script and updates test conftest to set required env vars. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces many hardcoded localhost/service fallbacks with environment-variable-driven defaults or required Pydantic fields across configs and models, and adds a script that scans source files for localhost-related fallback patterns and fails when found. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
jonahgabriel
enabled auto-merge
April 2, 2026 18:47
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/omnibase_infra/probes/capability_probe.py (1)
121-144:⚠️ Potential issue | 🟠 MajorDefault argument for
intelligence_urlcaptures stale environment state at module import time.The
os.environ.get("INTELLIGENCE_URL", "")default expression evaluates once when the module loads. IfINTELLIGENCE_URLis set later (by tests, runtime bootstrap, or environment sourcing),probe_platform_tier()will use the stale empty string and cannot detectfull_onex. This misclassifies the platform tier.Suggested fix
def probe_platform_tier( kafka_servers: str = "", - intelligence_url: str = os.environ.get("INTELLIGENCE_URL", ""), + intelligence_url: str | None = None, kafka_timeout: float = 1.0, intel_timeout: float = 1.0, ) -> TierName: """Probe available services and return the detected tier. Args: kafka_servers: Comma-separated Kafka bootstrap servers. intelligence_url: Base URL of the intelligence service. kafka_timeout: Timeout per Kafka host probe in seconds. intel_timeout: Timeout for intelligence /health check in seconds. Returns: One of "standalone", "event_bus", or "full_onex". """ + if intelligence_url is None: + intelligence_url = os.environ.get("INTELLIGENCE_URL", "") + kafka_ok = kafka_reachable(kafka_servers, timeout=kafka_timeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/probes/capability_probe.py` around lines 121 - 144, The default for intelligence_url is being evaluated at import time which can capture a stale value; change probe_platform_tier to accept intelligence_url: Optional[str] = None (or a sentinel) and inside the function read the env when intelligence_url is None or empty by doing intelligence_url = intelligence_url or os.environ.get("INTELLIGENCE_URL", ""), then build intel_health and call http_health_check as before; update the function signature and the internal assignment so tests or runtime sets to INTELLIGENCE_URL after import are respected.src/omnibase_infra/event_bus/models/config/model_kafka_event_bus_config.py (1)
958-976:⚠️ Potential issue | 🟡 MinorInconsistency:
default()still hardcodes localhost:19092.The
default()classmethod passesbootstrap_servers="localhost:19092"explicitly, bypassing the new required env var pattern. This creates an escape hatch that contradicts the PR's objective of removing all localhost fallbacks.Consider whether
default()should:
- Also require
KAFKA_BOOTSTRAP_SERVERS(remove the hardcoded value)- Be removed entirely if it's only used for local development
- Be explicitly documented as a "local-only" factory
Additionally, the docstring at lines 23-25 states
Default: "localhost:19092"which is now stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/event_bus/models/config/model_kafka_event_bus_config.py` around lines 958 - 976, The classmethod default() currently hardcodes bootstrap_servers="localhost:19092", which bypasses the new required KAFKA_BOOTSTRAP_SERVERS env pattern; change default() so it does not use a localhost fallback: read KAFKA_BOOTSTRAP_SERVERS from the environment (via the same codepath used by apply_environment_overrides() or os.getenv) and raise/throw a clear error if the env var is missing, or document default() explicitly as a local-only factory and remove the hardcoded host; also update the class/module docstring lines that still claim Default: "localhost:19092" to reflect the new behavior.
🧹 Nitpick comments (4)
src/omnibase_infra/services/session_registry/model_config_graph_projector.py (2)
27-27: Missingfrom_attributes=TrueinConfigDict.Per coding guidelines, Pydantic models should use
ConfigDict(frozen=True, extra='forbid', from_attributes=True). Thefrom_attributes=Trueis missing here.♻️ Add from_attributes=True
- model_config = ConfigDict(frozen=True, extra="forbid") + model_config = ConfigDict(frozen=True, extra="forbid", from_attributes=True)As per coding guidelines: "All Pydantic models must use ConfigDict with frozen=True, extra='forbid', and from_attributes=True for thread safety and strict validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/services/session_registry/model_config_graph_projector.py` at line 27, The ConfigDict assigned to model_config is missing from_attributes=True; update the ConfigDict call (the model_config variable) to include from_attributes=True alongside frozen=True and extra="forbid" so it reads ConfigDict(frozen=True, extra="forbid", from_attributes=True) to conform to the Pydantic model guidelines for thread safety and strict validation.
45-48: PotentialValueErrorifOMNIMEMORY_MEMGRAPH_PORTis non-numeric.The
default_factoryperformsint(os.environ["OMNIMEMORY_MEMGRAPH_PORT"])which will raise aValueErrorwith an unclear message if the env var contains non-numeric content. Consider wrapping this in a helper that provides a clearer error message, or rely on Pydantic's validation by keeping the factory aslambda: os.environ["OMNIMEMORY_MEMGRAPH_PORT"]and letting Pydantic coerce/validate the int.♻️ Let Pydantic handle int coercion
memgraph_port: int = Field( - default_factory=lambda: int(os.environ["OMNIMEMORY_MEMGRAPH_PORT"]), + default_factory=lambda: os.environ["OMNIMEMORY_MEMGRAPH_PORT"], description="Memgraph bolt port.", )Pydantic will automatically coerce the string to int and provide a clear validation error if it fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/services/session_registry/model_config_graph_projector.py` around lines 45 - 48, The Field for memgraph_port currently uses default_factory=lambda: int(os.environ["OMNIMEMORY_MEMGRAPH_PORT"]) which can raise an unclear ValueError; change the factory to return the raw environment string (e.g., default_factory=lambda: os.environ["OMNIMEMORY_MEMGRAPH_PORT"]) so Pydantic can coerce/validate to int and produce a clearer validation error, or replace the factory with a small helper function (e.g., parse_memgraph_port) that catches non-numeric values and raises a descriptive ValueError; update the memgraph_port Field to use that helper or the raw string factory accordingly.scripts/validate_no_env_fallbacks.py (1)
24-27: Consider enhancing regex patterns for completeness.The current patterns are good but could be slightly more precise:
- The patterns use unescaped
.which matches any character (though unlikely to cause false positives in practice)- Consider adding patterns for additional schemes mentioned in
tests/conftest.pylikehttp://localhostport variants🔧 Optional enhancement
VIOLATIONS_PATTERNS = [ - re.compile(r'os\.environ\.get\([^)]*,\s*"(localhost|http://localhost|bolt://localhost|redis://localhost|postgresql://localhost)'), - re.compile(r'default="(localhost|http://localhost|bolt://localhost|redis://localhost|postgresql://localhost)'), + re.compile(r'os\.environ\.get\([^)]*,\s*["\'](?:localhost|https?://localhost|bolt://localhost|redis://localhost|postgresql://localhost)'), + re.compile(r'default\s*=\s*["\'](?:localhost|https?://localhost|bolt://localhost|redis://localhost|postgresql://localhost)'), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate_no_env_fallbacks.py` around lines 24 - 27, The regexes in VIOLATIONS_PATTERNS should be tightened: escape literal dots in "os.environ.get" and the hostname patterns, and allow optional port numbers and optional path/ports for schemes referenced in tests (e.g., accept http://localhost(:<port>) and variants like redis://localhost:<port>); update the two compiled patterns (the one matching os.environ.get and the one matching default="...") to use escaped dots and an optional port group (e.g., :\d+)? and optional trailing slash or path segment so they catch host+port forms and prevent accidental overmatching.src/omnibase_infra/cli/git_hook_relay.py (1)
258-263: Consider improving error message for missingKAFKA_BOOTSTRAP_SERVERS.The lambda
default=lambda: os.environ["KAFKA_BOOTSTRAP_SERVERS"]will raise a rawKeyErrorif the env var is missing. For a user-facing CLI, this produces an unhelpful error message.Consider wrapping with a clearer error:
🔧 Suggested improvement
`@click.option`( "--bootstrap-servers", - default=lambda: os.environ["KAFKA_BOOTSTRAP_SERVERS"], + default=lambda: os.environ.get("KAFKA_BOOTSTRAP_SERVERS") or _require_kafka_env(), show_default=True, help="Kafka bootstrap servers (overrides KAFKA_BOOTSTRAP_SERVERS env var).", )Where
_require_kafka_env()raises aclick.ClickExceptionwith a clear message like "KAFKA_BOOTSTRAP_SERVERS environment variable is required".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/cli/git_hook_relay.py` around lines 258 - 263, The current click.option default uses lambda: os.environ["KAFKA_BOOTSTRAP_SERVERS"] which raises a raw KeyError when the env var is missing; replace this with a helper function (e.g., _require_kafka_env) that checks os.environ for "KAFKA_BOOTSTRAP_SERVERS" and raises click.ClickException("KAFKA_BOOTSTRAP_SERVERS environment variable is required") when absent, then pass default=_require_kafka_env to the click.option; ensure the helper is imported/defined in the module and referenced by name in the option instead of the lambda so the CLI shows a clear user-facing error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/omnibase_infra/cli/linear_relay.py`:
- Around line 153-157: The click option for "--bootstrap-servers" currently uses
a lambda default that raises KeyError if KAFKA_BOOTSTRAP_SERVERS is unset;
change the click.option to use envvar="KAFKA_BOOTSTRAP_SERVERS" and
required=True (remove the lambda default and show_default if desired) so Click
handles missing env var with a clean usage error; update the decorator where
"--bootstrap-servers" is declared in linear_relay.py (the click.option for
bootstrap-servers) to use envvar and required instead of default=lambda:
os.environ["KAFKA_BOOTSTRAP_SERVERS"].
In
`@src/omnibase_infra/handlers/registration_storage/handler_registration_storage_postgres.py`:
- Around line 179-180: The init currently reads os.environ["POSTGRES_HOST"]
whenever host is omitted, which breaks DSN-only usage; change the logic so that
POSTGRES_HOST is only read when no dsn was provided and host is still None.
Specifically, update the code that sets host (using the dsn and host parameters
in handler_registration_storage_postgres) to: if dsn is None and host is None
then set host = os.environ.get("POSTGRES_HOST") (or similar) instead of
unconditionally accessing os.environ["POSTGRES_HOST"].
In `@src/omnibase_infra/runtime/models/model_kafka_producer_config.py`:
- Around line 25-28: The bootstrap_servers Field default_factory in
ModelKafkaProducerConfig currently uses os.environ[...] (raising KeyError) while
the classmethod from_env() uses os.getenv(..., "localhost:19092"), causing
inconsistent behavior; fix by making both paths consistent: either (A) require
the env var in both places — change from_env() to read
os.environ["KAFKA_BOOTSTRAP_SERVERS"] and let instantiation keep default_factory
as-is, or (B) provide the same fallback in both — change the Field
default_factory to use os.getenv("KAFKA_BOOTSTRAP_SERVERS", "localhost:19092")
so ModelKafkaProducerConfig() and ModelKafkaProducerConfig.from_env() behave
identically. Update only the bootstrap_servers default_factory or the from_env()
lookup accordingly to keep semantics consistent.
In `@src/omnibase_infra/runtime/models/model_postgres_pool_config.py`:
- Line 29: The from_dsn()/from_env() path currently rewrites a missing hostname
to "localhost" (the host Field default and the code that returns host=hostname
or "localhost"), which silently masks misconfigured DSNs; change the logic in
from_dsn (the factory that parses DSNs) so it does not substitute "localhost"
when the parsed hostname is empty—either (a) validate and raise a ValueError (or
pydantic ValidationError) when hostname is missing in a TCP-style DSN, or (b) if
the DSN intends a Unix socket (host starts with "/" or socket-style authority),
preserve that host verbatim and do not rewrite to "localhost"; remove the
unconditional fallback to "localhost" and make from_env() propagate the
validation instead of hiding the error.
In `@src/omnibase_infra/runtime/models/model_runtime_scheduler_config.py`:
- Around line 224-225: The default() constructor is reintroducing a localhost
fallback for valkey_host and bypassing the Field default_factory that should
require VALKEY_HOST; locate the ModelRuntimeSchedulerConfig default() (or
wherever ModelRuntimeSchedulerConfig(...) is called with
valkey_host="localhost") and remove the hardcoded valkey_host="localhost"
argument so the Field default_factory (os.environ["VALKEY_HOST"]) is used and
will error if the env var is missing; apply this change to both occurrences
around the ModelRuntimeSchedulerConfig(...) calls noted (lines ~595-622 and
~605-621) to ensure no caller silently targets localhost.
In `@src/omnibase_infra/tui/consumers/consumer_status.py`:
- Around line 109-111: The code reads os.environ["KAFKA_BOOTSTRAP_SERVERS"] into
bootstrap_servers outside the try in consumer_status (so a missing env var
raises an uncaught KeyError); move the env var access (bootstrap_servers =
os.environ["KAFKA_BOOTSTRAP_SERVERS"]) inside the existing try block that
initializes consumer: AIOKafkaConsumer and handles exceptions so the KeyError is
caught by the function's except handler and logs/returns gracefully.
In
`@src/omnibase_infra/verification/models/model_contract_verification_report.py`:
- Around line 72-75: The runtime_target Field on ModelContractVerificationReport
currently uses default_factory=lambda: os.environ["ONEX_RUNTIME_TARGET"] which
will raise KeyError on every instantiation if the env var is missing; replace
this with a safe default (either use
Field(default=os.environ.get("ONEX_RUNTIME_TARGET")) to keep the env value when
present, or change the annotation to Optional[str] and use Field(default=None,
description=...) so missing env yields None) and update any downstream logic
that assumes runtime_target is non-None; refer to
ModelContractVerificationReport and the runtime_target Field to locate and fix
the code.
---
Outside diff comments:
In `@src/omnibase_infra/event_bus/models/config/model_kafka_event_bus_config.py`:
- Around line 958-976: The classmethod default() currently hardcodes
bootstrap_servers="localhost:19092", which bypasses the new required
KAFKA_BOOTSTRAP_SERVERS env pattern; change default() so it does not use a
localhost fallback: read KAFKA_BOOTSTRAP_SERVERS from the environment (via the
same codepath used by apply_environment_overrides() or os.getenv) and
raise/throw a clear error if the env var is missing, or document default()
explicitly as a local-only factory and remove the hardcoded host; also update
the class/module docstring lines that still claim Default: "localhost:19092" to
reflect the new behavior.
In `@src/omnibase_infra/probes/capability_probe.py`:
- Around line 121-144: The default for intelligence_url is being evaluated at
import time which can capture a stale value; change probe_platform_tier to
accept intelligence_url: Optional[str] = None (or a sentinel) and inside the
function read the env when intelligence_url is None or empty by doing
intelligence_url = intelligence_url or os.environ.get("INTELLIGENCE_URL", ""),
then build intel_health and call http_health_check as before; update the
function signature and the internal assignment so tests or runtime sets to
INTELLIGENCE_URL after import are respected.
---
Nitpick comments:
In `@scripts/validate_no_env_fallbacks.py`:
- Around line 24-27: The regexes in VIOLATIONS_PATTERNS should be tightened:
escape literal dots in "os.environ.get" and the hostname patterns, and allow
optional port numbers and optional path/ports for schemes referenced in tests
(e.g., accept http://localhost(:<port>) and variants like
redis://localhost:<port>); update the two compiled patterns (the one matching
os.environ.get and the one matching default="...") to use escaped dots and an
optional port group (e.g., :\d+)? and optional trailing slash or path segment so
they catch host+port forms and prevent accidental overmatching.
In `@src/omnibase_infra/cli/git_hook_relay.py`:
- Around line 258-263: The current click.option default uses lambda:
os.environ["KAFKA_BOOTSTRAP_SERVERS"] which raises a raw KeyError when the env
var is missing; replace this with a helper function (e.g., _require_kafka_env)
that checks os.environ for "KAFKA_BOOTSTRAP_SERVERS" and raises
click.ClickException("KAFKA_BOOTSTRAP_SERVERS environment variable is required")
when absent, then pass default=_require_kafka_env to the click.option; ensure
the helper is imported/defined in the module and referenced by name in the
option instead of the lambda so the CLI shows a clear user-facing error.
In
`@src/omnibase_infra/services/session_registry/model_config_graph_projector.py`:
- Line 27: The ConfigDict assigned to model_config is missing
from_attributes=True; update the ConfigDict call (the model_config variable) to
include from_attributes=True alongside frozen=True and extra="forbid" so it
reads ConfigDict(frozen=True, extra="forbid", from_attributes=True) to conform
to the Pydantic model guidelines for thread safety and strict validation.
- Around line 45-48: The Field for memgraph_port currently uses
default_factory=lambda: int(os.environ["OMNIMEMORY_MEMGRAPH_PORT"]) which can
raise an unclear ValueError; change the factory to return the raw environment
string (e.g., default_factory=lambda: os.environ["OMNIMEMORY_MEMGRAPH_PORT"]) so
Pydantic can coerce/validate to int and produce a clearer validation error, or
replace the factory with a small helper function (e.g., parse_memgraph_port)
that catches non-numeric values and raises a descriptive ValueError; update the
memgraph_port Field to use that helper or the raw string factory accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2580107d-19f6-4325-8b4f-28ed943b0c59
📒 Files selected for processing (32)
scripts/validate_no_env_fallbacks.pysrc/omnibase_infra/cli/git_hook_relay.pysrc/omnibase_infra/cli/linear_relay.pysrc/omnibase_infra/event_bus/models/config/model_kafka_event_bus_config.pysrc/omnibase_infra/handlers/models/graph/model_graph_handler_config.pysrc/omnibase_infra/handlers/models/qdrant/model_qdrant_handler_config.pysrc/omnibase_infra/handlers/registration_storage/handler_registration_storage_postgres.pysrc/omnibase_infra/nodes/node_registry_api_effect/handlers/handler_registry_api_get_health.pysrc/omnibase_infra/probes/capability_probe.pysrc/omnibase_infra/runtime/models/model_kafka_producer_config.pysrc/omnibase_infra/runtime/models/model_postgres_pool_config.pysrc/omnibase_infra/runtime/models/model_runtime_scheduler_config.pysrc/omnibase_infra/services/observability/agent_actions/config.pysrc/omnibase_infra/services/observability/consumer_health/config.pysrc/omnibase_infra/services/observability/context_audit/config.pysrc/omnibase_infra/services/observability/injection_effectiveness/config.pysrc/omnibase_infra/services/observability/llm_cost_aggregation/config.pysrc/omnibase_infra/services/observability/savings_estimation/config.pysrc/omnibase_infra/services/observability/skill_lifecycle/config.pysrc/omnibase_infra/services/post_merge/config.pysrc/omnibase_infra/services/session/config_consumer.pysrc/omnibase_infra/services/session/config_store.pysrc/omnibase_infra/services/session_registry/model_config_graph_projector.pysrc/omnibase_infra/services/waitlist_signup_notifier/config.pysrc/omnibase_infra/tui/consumers/consumer_status.pysrc/omnibase_infra/verification/batch.pysrc/omnibase_infra/verification/models/model_contract_verification_report.pysrc/omnibase_infra/verification/orchestrator.pytests/conftest.pytests/unit/cli/test_git_hook_relay.pytests/unit/services/session/test_config_store.pytests/unit/services/session_registry/conftest.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/omnibase_infra/verification/batch.py (1)
61-65: Consider providing a clearer error message when the environment variable is missing.The current implementation correctly fails when
ONEX_RUNTIME_TARGETis unset, butKeyError: 'ONEX_RUNTIME_TARGET'may be unclear to operators. A more descriptive exception improves debuggability.💡 Suggested improvement for clearer error handling
self.runtime_target = ( runtime_target if runtime_target is not None - else os.environ["ONEX_RUNTIME_TARGET"] + else os.environ.get("ONEX_RUNTIME_TARGET") + or (_ for _ in ()).throw( + ValueError("ONEX_RUNTIME_TARGET environment variable must be set") + ) )Or more readably:
if runtime_target is not None: self.runtime_target = runtime_target else: env_target = os.environ.get("ONEX_RUNTIME_TARGET") if not env_target: raise ValueError( "ONEX_RUNTIME_TARGET environment variable must be set " "when runtime_target is not provided" ) self.runtime_target = env_target🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/verification/batch.py` around lines 61 - 65, The code currently lets os.environ["ONEX_RUNTIME_TARGET"] raise a KeyError when runtime_target is None; change the logic in the class where self.runtime_target is set (the runtime_target assignment block) to explicitly check os.environ.get("ONEX_RUNTIME_TARGET") and raise a clearer ValueError if the env var is missing, e.g., when runtime_target is None read env_target = os.environ.get("ONEX_RUNTIME_TARGET") and if not env_target raise ValueError("ONEX_RUNTIME_TARGET environment variable must be set when runtime_target is not provided"), otherwise assign self.runtime_target = env_target.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/omnibase_infra/verification/batch.py`:
- Around line 61-65: The code currently lets os.environ["ONEX_RUNTIME_TARGET"]
raise a KeyError when runtime_target is None; change the logic in the class
where self.runtime_target is set (the runtime_target assignment block) to
explicitly check os.environ.get("ONEX_RUNTIME_TARGET") and raise a clearer
ValueError if the env var is missing, e.g., when runtime_target is None read
env_target = os.environ.get("ONEX_RUNTIME_TARGET") and if not env_target raise
ValueError("ONEX_RUNTIME_TARGET environment variable must be set when
runtime_target is not provided"), otherwise assign self.runtime_target =
env_target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfb12770-f29e-454b-a5ae-d2cc98ac0ce1
📒 Files selected for processing (3)
src/omnibase_infra/runtime/models/model_postgres_pool_config.pysrc/omnibase_infra/verification/batch.pysrc/omnibase_infra/verification/orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/omnibase_infra/verification/orchestrator.py
- src/omnibase_infra/runtime/models/model_postgres_pool_config.py
- test_store.py: assert postgres_host has no default (PydanticUndefined) now that OMN-7227 removed the localhost fallback from ConfigSessionStorage - test_handler_architecture_invariants.py: raise infra handler env-access max_allowed from 6 to 7 to account for the new violation introduced by this PR - test_kafka_event_bus_integration.py + test_node_reward_binder_effect.py: gate Kafka integration tests on KAFKA_INTEGRATION_TESTS=1 so they skip in CI where tests/conftest.py now unconditionally sets KAFKA_BOOTSTRAP_SERVERS=localhost:19092 - test_node_reward_binder_effect.py: fix EventBusKafka constructor call to use config=ModelKafkaEventBusConfig(...) instead of removed bootstrap_servers kwarg Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/event_bus/test_kafka_event_bus_integration.py (1)
48-57:⚠️ Potential issue | 🟡 MinorHarden Kafka availability gating and align skip reason with actual condition.
KAFKA_BOOTSTRAP_SERVERS is not Nonetreats""as available. Also, the skip reason now omits theKAFKA_INTEGRATION_TESTS=1requirement, which makes CI skips harder to interpret.Suggested patch
KAFKA_BOOTSTRAP_SERVERS = os.getenv("KAFKA_BOOTSTRAP_SERVERS") KAFKA_AVAILABLE = ( - KAFKA_BOOTSTRAP_SERVERS is not None and os.getenv("KAFKA_INTEGRATION_TESTS") == "1" + bool((KAFKA_BOOTSTRAP_SERVERS or "").strip()) + and os.getenv("KAFKA_INTEGRATION_TESTS") == "1" ) @@ pytestmark = [ pytest.mark.skipif( not KAFKA_AVAILABLE, - reason="Kafka not available (KAFKA_BOOTSTRAP_SERVERS not set)", + reason=( + "Kafka integration tests disabled " + "(requires non-empty KAFKA_BOOTSTRAP_SERVERS and " + "KAFKA_INTEGRATION_TESTS=1)" + ), ), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/event_bus/test_kafka_event_bus_integration.py` around lines 48 - 57, KAFKA availability gating currently treats an empty KAFKA_BOOTSTRAP_SERVERS as present and the pytest skip reason omits the KAFKA_INTEGRATION_TESTS requirement; update the KAFKA_AVAILABLE definition to require a non-empty value (e.g., use bool(KAFKA_BOOTSTRAP_SERVERS) or KAFKA_BOOTSTRAP_SERVERS not in (None, "") and keep the os.getenv("KAFKA_INTEGRATION_TESTS") == "1" check), and update the pytestmark skip reason (used for module-level markers) to mention both missing KAFKA_BOOTSTRAP_SERVERS and the KAFKA_INTEGRATION_TESTS env var so the skip message accurately reflects the gating conditions.tests/integration/nodes/test_node_reward_binder_effect.py (1)
56-65:⚠️ Potential issue | 🟡 MinorUse a non-empty bootstrap check and update skip reason text.
Current condition accepts empty
KAFKA_BOOTSTRAP_SERVERSvalues as “available,” and the skip reason no longer reflects theKAFKA_INTEGRATION_TESTS=1opt-in gate.Suggested patch
KAFKA_BOOTSTRAP_SERVERS = os.getenv("KAFKA_BOOTSTRAP_SERVERS") KAFKA_AVAILABLE = ( - KAFKA_BOOTSTRAP_SERVERS is not None and os.getenv("KAFKA_INTEGRATION_TESTS") == "1" + bool((KAFKA_BOOTSTRAP_SERVERS or "").strip()) + and os.getenv("KAFKA_INTEGRATION_TESTS") == "1" ) @@ pytest.mark.skipif( not KAFKA_AVAILABLE, - reason="Kafka not available (KAFKA_BOOTSTRAP_SERVERS not set)", + reason=( + "Kafka integration tests disabled " + "(requires non-empty KAFKA_BOOTSTRAP_SERVERS and " + "KAFKA_INTEGRATION_TESTS=1)" + ), ), ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/nodes/test_node_reward_binder_effect.py` around lines 56 - 65, KAFKA_AVAILABLE currently treats empty KAFKA_BOOTSTRAP_SERVERS as present and the skip reason omits the opt-in gate; change the availability check to ensure KAFKA_BOOTSTRAP_SERVERS is set and non-empty (e.g., truthy/strip() non-empty) and still require os.getenv("KAFKA_INTEGRATION_TESTS") == "1", and update the pytest.mark.skipif reason on the pytestmark to mention both that KAFKA_BOOTSTRAP_SERVERS is not set/non-empty and that KAFKA_INTEGRATION_TESTS != "1" so the message accurately reflects the two-gate condition (referencing KAFKA_AVAILABLE, KAFKA_BOOTSTRAP_SERVERS, and the pytest.mark.skipif entry).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ci/test_handler_architecture_invariants.py`:
- Around line 545-547: The CI test relaxes INV-3 by raising max_allowed to 7;
instead revert max_allowed to its original strict value and handle the OMN-7227
exception at source by annotating the offending handlers with an explicit
ONEX_EXCLUDE entry and include the OMN-7227 ticket reference; update the
relevant handler modules (e.g., handler_gmail_api, handler_slack_webhook or the
new offending class/function) to add the exclusion marker so tests in
tests/ci/test_handler_architecture_invariants.py continue to enforce INV-3
without increasing the allowed-violations budget.
---
Outside diff comments:
In `@tests/integration/event_bus/test_kafka_event_bus_integration.py`:
- Around line 48-57: KAFKA availability gating currently treats an empty
KAFKA_BOOTSTRAP_SERVERS as present and the pytest skip reason omits the
KAFKA_INTEGRATION_TESTS requirement; update the KAFKA_AVAILABLE definition to
require a non-empty value (e.g., use bool(KAFKA_BOOTSTRAP_SERVERS) or
KAFKA_BOOTSTRAP_SERVERS not in (None, "") and keep the
os.getenv("KAFKA_INTEGRATION_TESTS") == "1" check), and update the pytestmark
skip reason (used for module-level markers) to mention both missing
KAFKA_BOOTSTRAP_SERVERS and the KAFKA_INTEGRATION_TESTS env var so the skip
message accurately reflects the gating conditions.
In `@tests/integration/nodes/test_node_reward_binder_effect.py`:
- Around line 56-65: KAFKA_AVAILABLE currently treats empty
KAFKA_BOOTSTRAP_SERVERS as present and the skip reason omits the opt-in gate;
change the availability check to ensure KAFKA_BOOTSTRAP_SERVERS is set and
non-empty (e.g., truthy/strip() non-empty) and still require
os.getenv("KAFKA_INTEGRATION_TESTS") == "1", and update the pytest.mark.skipif
reason on the pytestmark to mention both that KAFKA_BOOTSTRAP_SERVERS is not
set/non-empty and that KAFKA_INTEGRATION_TESTS != "1" so the message accurately
reflects the two-gate condition (referencing KAFKA_AVAILABLE,
KAFKA_BOOTSTRAP_SERVERS, and the pytest.mark.skipif entry).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 927cf424-37a5-452a-a6ce-6353d5fd7115
📒 Files selected for processing (4)
tests/ci/test_handler_architecture_invariants.pytests/integration/event_bus/test_kafka_event_bus_integration.pytests/integration/nodes/test_node_reward_binder_effect.pytests/services/session/test_store.py
… skip logic [OMN-7227]
Kafka integration tests use KAFKA_AVAILABLE = os.getenv("KAFKA_BOOTSTRAP_SERVERS") is not None
evaluated at module import time. Setting KAFKA_BOOTSTRAP_SERVERS in conftest.py defaults
caused @requires_kafka skip markers to never fire, causing Kafka connection failures in CI
where ONEX_EVENT_BUS_TYPE=inmemory (Kafka not running).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…3 at 6 [OMN-7227] Per CodeRabbit feedback: use ONEX_EXCLUDE marker on the constructor injection fallback rather than bumping max_allowed. Reverts INV-3 budget from 7 to 6 (original baseline). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ci/test_handler_architecture_invariants.py`:
- Around line 545-547: Test's allowed INV-3 budget is too high because
_scan_file_for_env_access ignores ONEX_EXCLUDE-tagged lines (so
handler_registration_storage_postgres is excluded), therefore reduce the
max_allowed value in tests/ci/test_handler_architecture_invariants.py (the
variable named max_allowed) from 6 to 5 to match the actual counted baseline
which only includes handler_gmail_api (3) and handler_slack_webhook (2).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2591cb2a-61a3-493e-970b-0e0fd3d044ad
📒 Files selected for processing (2)
src/omnibase_infra/handlers/registration_storage/handler_registration_storage_postgres.pytests/ci/test_handler_architecture_invariants.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_infra/handlers/registration_storage/handler_registration_storage_postgres.py
…se-infra-purge-fallbacks
…postgres [OMN-7227] The ONEX_EXCLUDE marker correctly excludes handler_registration_storage_postgres, but handler_graph.py contributes 1 violation. max_allowed=6 is correct. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s ruff format [OMN-7227] The ternary expression for os.environ fallback is too long for an ONEX_EXCLUDE comment without ruff splitting the string key onto a separate line. Using # fmt: skip keeps the expression on one line and the ONEX_EXCLUDE guard intact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Unit tests that instantiate Kafka config models (ConfigSessionConsumer, ConfigContextAuditConsumer, test_kafka_config_acks) require KAFKA_BOOTSTRAP_SERVERS to be set to construct Pydantic models — they don't need a live broker. Removing the default broke config model unit tests. The @requires_kafka markers independently gate tests that need actual Kafka connectivity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Setting KAFKA_BOOTSTRAP_SERVERS at conftest import time caused integration tests that gate on `KAFKA_AVAILABLE = os.getenv(...) is not None` to skip incorrectly in CI — the env var was present but no live broker existed, causing DLQ and other Kafka integration tests to fail. Unit tests that need the variable for config model instantiation use monkeypatch.setenv or explicit env dicts; Pydantic default_factory lambdas are lazy and only evaluated at instantiation, not import time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…TEGRATION_TESTS guard to DLQ tests Removing KAFKA_BOOTSTRAP_SERVERS from _TEST_ENV_DEFAULTS broke tests that instantiate BaseSettings models with required kafka_bootstrap_servers fields (e.g. ConfigSavingsEstimation). Instead, restore the env var for model instantiation and add a KAFKA_INTEGRATION_TESTS=1 explicit opt-in to test_dlq_integration.py, consistent with test_kafka_event_bus_integration.py. This prevents DLQ tests from attempting real Kafka connections in CI without a live broker. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion test fixtures ConfigSavingsEstimation uses env_prefix="OMNIBASE_INFRA_SAVINGS_" so pydantic-settings resolves OMNIBASE_INFRA_SAVINGS_KAFKA_BOOTSTRAP_SERVERS, not plain KAFKA_BOOTSTRAP_SERVERS. Both test fixtures were relying on the env var which never matched the prefixed field lookup, causing ValidationError on every CI run since the PR was opened. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fixtures Five more config classes (ConfigContextAuditConsumer, ConfigInjectionEffectivenessConsumer, ConfigLlmCostAggregation, ConfigSkillLifecycleConsumer) use env_prefix in BaseSettings, so pydantic-settings looks for OMNIBASE_INFRA_*_KAFKA_BOOTSTRAP_SERVERS — not the plain KAFKA_BOOTSTRAP_SERVERS set in conftest. Pass it explicitly in every test fixture. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three TestConfigEnvironment tests constructed configs via env vars without setting OMNIBASE_INFRA_*_KAFKA_BOOTSTRAP_SERVERS, causing ValidationError since kafka_bootstrap_servers is now required. Add explicit setenv calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ther tests ConfigPostMergeConsumer, ConfigWaitlistSignupNotifier, and ConfigSessionConsumer all have required kafka/bootstrap_servers fields (Field(...)). Test methods that called the constructors with no args now pass the field explicitly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Setting KAFKA_BOOTSTRAP_SERVERS globally caused the E2E conftest to evaluate KAFKA_AVAILABLE=True, which made ALL_INFRA_AVAILABLE=True, which caused E2E integration tests (test_two_way_registration_e2e.py) to run and error with KafkaConnectionError — no real broker is present in the unit CI runner. Unit tests that instantiate pydantic-settings config models (e.g. ConfigSavingsEstimation, ConfigSkillLifecycleConsumer) now pass kafka_bootstrap_servers= explicitly in constructor calls, so they no longer need the global env var. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… globally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…n KAFKA_INTEGRATION_TESTS Unit tests in cli, runtime, tui, and session_registry use os.environ['KAFKA_BOOTSTRAP_SERVERS'] directly (hard key access), so they need the var set globally. Restore it in _TEST_ENV_DEFAULTS. To prevent E2E registration tests from running when only the address is set (no live broker), the E2E conftest now also checks KAFKA_INTEGRATION_TESTS=1 — same guard as the DLQ integration tests. This ensures E2E tests remain skipped in unit CI while still allowing CLI tests to access the env var. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QDRANT_URL is set globally in _TEST_ENV_DEFAULTS for model config instantiation, causing the Qdrant integration tests to run (instead of skip) in unit CI. Apply the same guard pattern as the Kafka E2E tests: require QDRANT_INTEGRATION_TESTS=1 to distinguish between 'URL known for config' and 'server actually available'. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jonahgabriel
added this pull request to the merge queue
Apr 3, 2026
jonahgabriel
added a commit
that referenced
this pull request
Jul 30, 2026
…ink meta-guard baseline to zero (#2555) * fix(OMN-15410): collect the 4 remaining uncollected pytest roots, shrink meta-guard baseline to zero OMN-15378 (#2542) wired scripts/deploy-agent/tests into CI and added a meta-guard that inventoried the test roots CI never collects. It recorded four more roots of the identical class as KNOWN_UNCOLLECTED_DEBT. This collects all four and closes the two seams that let the class exist. Roots now collected (366 tests that had never run in CI): scripts/ci/tests, scripts/tests, scripts/runtime_build/tests, src/omnibase_infra/services/observability/agent_actions/tests Rot found by running them for the first time: 4 tests in the agent_actions root have been RED since 2026-04-02. OMN-7227 (#1127) made ConfigAgentActionsConsumer.kafka_bootstrap_servers required; these four constructions never supplied it and raised ValidationError for ~17 weeks with zero signal. Fixed test-side, supplying the same literal the already-passing _make_health_consumer helper in the same module uses, not by relaxing the field, since fail-fast on missing Kafka wiring is what OMN-7227 bought. Mechanism, not just a baseline edit: * pyproject.toml testpaths is now the single source of truth for what CI collects, and ci.yml full-suite step passes NO positional path so it inherits that list verbatim. It previously hardcoded "pytest tests/", which made the workflow, not pyproject, the real definition of the suite: a root added to testpaths would still never have run. * scripts/ci/detect_test_paths.py gains COLLOCATED_TEST_ROOTS so a NARROWED smart-selection run can reach the collocated roots from a diff touching their code, rather than only via an unrelated full-suite escalation. * ModelTestSelection TestPath widened to admit a collocated tests/ dir; the final path component must still be "tests", so the selector still cannot emit a source directory. * The guard now derives collected roots from testpaths, rejects a testpaths entry with no directory behind it (pytest exit 5), fails closed on an empty testpaths, asserts ci.yml passes no positional path, and asserts testpaths vs COLLOCATED_TEST_ROOTS parity in both directions. * KNOWN_UNCOLLECTED_DEBT is now EMPTY: the guard has converged to zero known uncollected roots, its stated OMN-15378 goal. docker/runners/runner-image.lock.json regenerated: runner_image_identity binds pyproject.toml bytes, so the testpaths edit re-binds the identity (digest-only change, image_version unchanged at v6). Caught by tests/ci/test_runner_image_identity.py, which is exactly the gate working. Refs: OMN-15410, OMN-15378, OMN-14880 * chore(OMN-15410): retrigger CI after autobind added Evidence-Source: OCC#5556 --------- Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
localhost:19092andlocalhost:5432fallback defaults from Kafka, Postgres, Qdrant, and graph handler configs across 30 filesKAFKA_BOOTSTRAP_SERVERS,DATABASE_URL, etc.) — silent fallback to localhost masked missing config in non-local environmentsvalidate_no_env_fallbacks.pyCI script to prevent regressionThis is the final repo in the OMN-7227 sweep — all other repos already have merged PRs.
Test plan
validate_no_env_fallbacks.pycatches any re-introduced fallbacks🤖 Generated with Claude Code
Summary by CodeRabbit
Configuration Changes
Tooling
Tests