Repository navigation
fix(infra): purge hardcoded localhost/paths [OMN-7226] - #1266
Merged
jonahgabriel merged 2 commits intoApr 13, 2026
Merged
Conversation
- introspect.py: replace hardcoded http://localhost:8080 health endpoint
with os.environ.get("ONEX_RUNTIME_URL", "http://localhost:8080")
- snapshot_publisher_registration.py: annotate docstring localhost:19092
example with local-path-ok marker
📝 WalkthroughWalkthroughIntrospection payload now builds the health endpoint from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/omnibase_infra/cli/infra_test/introspect.py (1)
26-26:⚠️ Potential issue | 🟠 MajorReplace raw topic literal with canonical topic constant (CI blocker).
Line 26 introduces/retains a raw topic string and is currently failing CI (
check_topic_literals.py). Please switchDEFAULT_INTROSPECTION_TOPICto the repository’s canonical topics constant/loader entry instead of a literal.As per coding guidelines,
src/**/*.py: Hardcoded topic strings like 'onex.evt.foo.bar.v1' are forbidden - must use contract-declared topic names via contract loader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/cli/infra_test/introspect.py` at line 26, Replace the raw string assigned to DEFAULT_INTROSPECTION_TOPIC with the repository’s canonical topic constant/loader entry: import the contract topics constant or loader function used elsewhere in the codebase (the module that exposes the canonical topic names, e.g., TOPICS or get_topic) and set DEFAULT_INTROSPECTION_TOPIC to that canonical value (for example TOPICS.NODE_INTROSPECTION or get_topic("platform.node-introspection.v1")); ensure you remove the hardcoded "onex.evt.platform.node-introspection.v1" literal so the file passes check_topic_literals.py.
🧹 Nitpick comments (1)
src/omnibase_infra/cli/infra_test/introspect.py (1)
84-86: NormalizeONEX_RUNTIME_URLbefore concatenation.Line 85 can produce
//whenONEX_RUNTIME_URLends with/. Strip trailing slashes before composing the health URL.♻️ Proposed refactor
- return { + runtime_base_url = os.environ.get("ONEX_RUNTIME_URL", "http://localhost:8080").rstrip("/") + return { "node_id": nid, "node_type": node_type, "node_version": {"major": major, "minor": minor, "patch": patch}, "declared_capabilities": {}, "discovered_capabilities": {}, "contract_capabilities": None, "endpoints": { - "health": f"{os.environ.get('ONEX_RUNTIME_URL', 'http://localhost:8080')}/{nid}/health" + "health": f"{runtime_base_url}/{nid}/health" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnibase_infra/cli/infra_test/introspect.py` around lines 84 - 86, Normalize the ONEX_RUNTIME_URL before concatenation: retrieve the env var (os.environ.get('ONEX_RUNTIME_URL', 'http://localhost:8080')), call .rstrip('/') on it to remove trailing slashes, and then build the health endpoint using that normalized base (e.g., f"{base}/{nid}/health") so that the "endpoints" -> "health" entry in the dict never contains a double slash; update the code that constructs the endpoints/health URL in introspect.py (where ONEX_RUNTIME_URL and nid are used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/omnibase_infra/cli/infra_test/introspect.py`:
- Line 26: Replace the raw string assigned to DEFAULT_INTROSPECTION_TOPIC with
the repository’s canonical topic constant/loader entry: import the contract
topics constant or loader function used elsewhere in the codebase (the module
that exposes the canonical topic names, e.g., TOPICS or get_topic) and set
DEFAULT_INTROSPECTION_TOPIC to that canonical value (for example
TOPICS.NODE_INTROSPECTION or get_topic("platform.node-introspection.v1"));
ensure you remove the hardcoded "onex.evt.platform.node-introspection.v1"
literal so the file passes check_topic_literals.py.
---
Nitpick comments:
In `@src/omnibase_infra/cli/infra_test/introspect.py`:
- Around line 84-86: Normalize the ONEX_RUNTIME_URL before concatenation:
retrieve the env var (os.environ.get('ONEX_RUNTIME_URL',
'http://localhost:8080')), call .rstrip('/') on it to remove trailing slashes,
and then build the health endpoint using that normalized base (e.g.,
f"{base}/{nid}/health") so that the "endpoints" -> "health" entry in the dict
never contains a double slash; update the code that constructs the
endpoints/health URL in introspect.py (where ONEX_RUNTIME_URL and nid are used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4505844a-d702-46dc-b6b0-1f4629f4b2ea
📒 Files selected for processing (2)
src/omnibase_infra/cli/infra_test/introspect.pysrc/omnibase_infra/projectors/snapshot_publisher_registration.py
…st [OMN-7226] Use EnumPlatformTopic.EVT_NODE_INTROSPECTION_V1.value instead of the hardcoded literal in introspect.py. Add integration tests asserting the CLI invokes rpk with the canonical topic constant.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/infra_test/introspect.py`:
- Around line 84-86: Normalize the ONEX_RUNTIME_URL before composing
endpoints['health']: read os.environ.get('ONEX_RUNTIME_URL'), treat empty/blank
values as missing (fallback to 'http://localhost:8080'), strip any trailing
slashes (e.g., rstrip('/')) to avoid double slashes, then build the health URL
as base + '/' + nid + '/health'. Update the code that constructs the "endpoints"
dict (the "health" entry referencing nid and ONEX_RUNTIME_URL) to use this
normalized base.
In `@tests/integration/test_introspect_cli.py`:
- Around line 38-39: Replace the hardcoded broker CLI arg list ["introspect",
"--broker", "localhost:19092"] with a value read from the
KAFKA_BOOTSTRAP_SERVERS environment variable (or set it in the test with
monkeypatch.setenv) so tests use env-driven broker addresses; update all
occurrences (the CLI invocation lists at the three occurrences) to build args
like ["introspect", "--broker", os.environ["KAFKA_BOOTSTRAP_SERVERS"]] or use
monkeypatch.setenv("KAFKA_BOOTSTRAP_SERVERS", value) before calling the CLI
runner, ensuring you import os and/or use the pytest monkeypatch fixture and
keep catch_exceptions=False unchanged.
🪄 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: 73d6205b-8dca-4bf2-98c9-47d6b1967022
📒 Files selected for processing (2)
src/omnibase_infra/cli/infra_test/introspect.pytests/integration/test_introspect_cli.py
Comment on lines
+84
to
+86
| "endpoints": { | ||
| "health": f"{os.environ.get('ONEX_RUNTIME_URL', 'http://localhost:8080')}/{nid}/health" | ||
| }, |
There was a problem hiding this comment.
Normalize ONEX_RUNTIME_URL before building endpoints.health.
At Line 85, an empty ONEX_RUNTIME_URL produces /{nid}/health, and a trailing slash produces //{nid}/health. Normalize and fallback on empty values.
Suggested patch
- "endpoints": {
- "health": f"{os.environ.get('ONEX_RUNTIME_URL', 'http://localhost:8080')}/{nid}/health"
- },
+ "endpoints": {
+ "health": f"{(os.environ.get('ONEX_RUNTIME_URL') or 'http://localhost:8080').rstrip('/')}/{nid}/health"
+ },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/omnibase_infra/cli/infra_test/introspect.py` around lines 84 - 86,
Normalize the ONEX_RUNTIME_URL before composing endpoints['health']: read
os.environ.get('ONEX_RUNTIME_URL'), treat empty/blank values as missing
(fallback to 'http://localhost:8080'), strip any trailing slashes (e.g.,
rstrip('/')) to avoid double slashes, then build the health URL as base + '/' +
nid + '/health'. Update the code that constructs the "endpoints" dict (the
"health" entry referencing nid and ONEX_RUNTIME_URL) to use this normalized
base.
Comment on lines
+38
to
+39
| ["introspect", "--broker", "localhost:19092"], | ||
| catch_exceptions=False, |
There was a problem hiding this comment.
Replace hardcoded broker CLI args with env-driven values in tests.
Lines 38, 69, and 92 pass a fixed broker address (localhost:19092). Read broker from KAFKA_BOOTSTRAP_SERVERS (or set it via monkeypatch.setenv in each test) and pass that variable instead.
Suggested patch pattern
+import os
...
- ["introspect", "--broker", "localhost:19092"],
+ ["introspect", "--broker", os.environ["KAFKA_BOOTSTRAP_SERVERS"]],
...
- ["introspect", "--broker", "localhost:19092", "--topic", custom_topic],
+ ["introspect", "--broker", os.environ["KAFKA_BOOTSTRAP_SERVERS"], "--topic", custom_topic],
...
- ["introspect", "--broker", "localhost:19092"],
+ ["introspect", "--broker", os.environ["KAFKA_BOOTSTRAP_SERVERS"]],As per coding guidelines: "Never hardcode broker addresses -- use environment variables instead".
Also applies to: 69-70, 92-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_introspect_cli.py` around lines 38 - 39, Replace the
hardcoded broker CLI arg list ["introspect", "--broker", "localhost:19092"] with
a value read from the KAFKA_BOOTSTRAP_SERVERS environment variable (or set it in
the test with monkeypatch.setenv) so tests use env-driven broker addresses;
update all occurrences (the CLI invocation lists at the three occurrences) to
build args like ["introspect", "--broker",
os.environ["KAFKA_BOOTSTRAP_SERVERS"]] or use
monkeypatch.setenv("KAFKA_BOOTSTRAP_SERVERS", value) before calling the CLI
runner, ensuring you import os and/or use the pytest monkeypatch fixture and
keep catch_exceptions=False unchanged.
jonahgabriel
added a commit
that referenced
this pull request
Apr 13, 2026
Fallout from PRs #1264 (OMN-8605) and #1262 (OMN-8550) which landed on main without test verification, blocking PR #1266 from going green. Fix 1 — test_topic_suffix_exports.py: `from omnibase_infra.topics import __init__ as topics_init` resolved to a method-wrapper, not the module. Replace with a standard module import. Fix 2 — test_protocol_ownership.py: KNOWN_DUPLICATE_LOCATIONS["ProtocolPublisher"] was missing the node_baseline_capture entry added by OMN-7484. Also allowlists ProtocolDomainPlugin (still present in runtime/ as a backward-compat shim post OMN-8550 SPI migration). Fix 3 — test_topic_parity.py / check_contract_topic_parity.py: 8 SUFFIX_* constants added in #1264 had no spec group entries: - 6 omniclaude injection-effectiveness topics (OMN-1889, OMN-2942) added to _OMNICLAUDE_AGENT_OBSERVABILITY_TOPIC_SUFFIXES in platform_topic_suffixes.py, and to _LEGACY_ALLOWLIST (cross-repo, contract.yaml lives in omniclaude). - onex.evt.git.hook.v1 and onex.evt.linear.snapshot.v1 added to ALL_OMNIBASE_INFRA_TOPIC_SPECS (3 partitions, 7-day retention), and to _LEGACY_ALLOWLIST (CLI relay producers, no node contract). No new tickets filed per standing rule — OMN-8605 and OMN-8550 are the parent tickets for these regressions.
4 tasks done
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
cli/infra_test/introspect.py: replace hardcodedhttp://localhost:8080/{nid}/healthwithos.environ.get("ONEX_RUNTIME_URL", "http://localhost:8080")/{nid}/health— fixes finding feat: Complete Phase 2 infrastructure migration to ONEX nodes #5projectors/snapshot_publisher_registration.py: annotate docstring examplelocalhost:19092with# local-path-okmarker — finding feat: Complete infrastructure containers operational with Docker secrets #4Findings triaged
local-path-okONEX_RUNTIME_URLenv varos.environ["ONEX_RUNTIME_TARGET"]— cleanos.environ.get("INTELLIGENCE_URL", "")— cleanTest plan
introspect.pyrespectsONEX_RUNTIME_URLenv var when setSummary by CodeRabbit
Improvements
Documentation
Tests