Repository navigation
fix(OMN-14531): close the omnimarket drift guard's fail-open blind spot - #2291
Conversation
…ft, OMN-14555) Unrelated to this ticket's fix. The local onex-check-node-migration-sync pre-commit hook blocks ALL commits in this repo right now because the vendored migration tree drifted from omnimarket's node_projection_context_roi node (introduced by OMN-14535 / omnimarket#1743, not this branch). The canonical fix already landed as its own PR (omnibase_infra#2290, OMN-14555) but is not yet merged to dev, so this worktree (branched from pre-#2290 dev) still has the drift locally. Vendoring the same file here only unblocks the local commit gate for this unrelated change; it is a mechanical, deterministic sync from the canonical omnimarket source (scripts/sync-node-migrations.sh, no --check) and is expected to be a no-op once #2290 merges and this branch rebases. node-migration-sync is NOT a required GitHub status check (confirmed in ROLLING_WORK_LEDGER 2026-07-13T08:3xZ), so this has no bearing on mergeability -- it only satisfies the local pre-commit gate.
The onex:aislop_sweep skill's ONLY documented dispatch path (`onex skill
aislop_sweep`) was hard-failing with "Unknown node 'node_aislop_sweep'"
because omnimarket -- the co-installed provider of onex.nodes entry points
for market nodes -- was completely absent from the omnibase_infra venv
(not merely stale: the OMN-13829 -> OMN-14060 recurrence regressed one
step further, from "stale" to "gone").
Two compounding gaps let this go undetected:
1. check_omnimarket_drift() unconditionally failed open whenever
installed_omnimarket_commit() returned None, even when a canonical
$OMNI_HOME/omnimarket clone was present and reachable (a determinable,
actionable state -- not the "can't reason about it" case the fail-open
design was meant for). Reorder the checks so a present canonical clone
plus an absent/non-VCS install now raises OmnimarketDriftError with a
pointer to the repair command, instead of silently returning.
2. cli_skill.py's `--omni-home` option had no `envvar` binding, so even a
correct guard never received a real value in normal usage -- callers
never pass --omni-home explicitly, and $OMNI_HOME being exported in the
shell had zero effect. Bind it to the OMNI_HOME environment variable so
the guard is actually reachable.
Verified locally: co-installing omnimarket via
scripts/install-node-skill-package.sh --execute flips `onex skill
aislop_sweep --dry-run` from exit 1 ("Unknown node") to exit 0 with
repos_scanned=1 and 11 real findings (CRITICAL prohibited-pattern hits
included) -- the dispatch path and the aislop handler were never the
problem; the venv provisioning + guard blind spot were.
test_cli_skill.py gets an autouse fixture to keep command-wiring tests
hermetic against the now-live --omni-home envvar binding.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The `onex node`/`onex run` dispatch path had ZERO drift-guard wiring --
it raised a bare "Unknown node '<name>'" whenever the omnimarket
co-install was drifted or absent, with no pointer to the actual cause
or the repair command. This is the identical gap OMN-14531 fixed for
`onex skill` (cli_skill.py), flagged there as a known follow-up.
Mirrors that fix exactly, reusing the same
`omnimarket_drift_guard.check_omnimarket_drift()` helper (no
duplicated logic):
1. Added `--omni-home` bound to the `$OMNI_HOME` envvar.
2. Call `check_omnimarket_drift()` at the top of `run_node_by_name`,
before `_resolve_packaged_contract`, converting
`OmnimarketDriftError` to a `click.ClickException`.
Since `onex node` and `onex run` are both entry-point aliases for the
same `run_node_by_name` function (pyproject.toml
`[project.entry-points."onex.cli"]`), one wiring point fixes both.
RED -> GREEN evidence (dod_evidence): new
`test_drift_guard_fires_before_unknown_node_lookup` in
tests/unit/cli/test_cli_node.py fails against the pre-fix cli_node.py
(no --omni-home option, no guard call -- reports the old bare
"Unknown node" error) and passes against the fix (raises the loud
OmnimarketDriftError with repair command, before node resolution ever
runs).
Wiring the guard surfaced the same non-hermeticity gap OMN-14531 had
to guard against, but across three test files that all reach
run_node_by_name (test_cli_node.py, test_cli_node_receipt.py,
test_onex_run.py) rather than one -- added a shared
tests/unit/cli/conftest.py autouse fixture (neutralize + delenv
OMNI_HOME) instead of duplicating a file-local fixture three times.
Manually reproduced against the real `onex node`/`onex run` CLI on
this operator's drifted venv (omnimarket absent, canonical clone
present):
$ uv run onex node definitely_not_a_real_node
Error: omnimarket is NOT INSTALLED from git in this interpreter
..., but a canonical clone exists at $OMNI_HOME/omnimarket (HEAD
48c03808155e). ... Repair with:
scripts/install-node-skill-package.sh --execute ...
exit 1
$ uv run onex run definitely_not_a_real_node
(identical output -- same underlying function via the "run" alias)
Local gates: `ruff format --check` / `ruff check` / `mypy` clean on
touched files; full `pre-commit run` clean; tests/unit/cli/ full
suite: 342 passed, 19 skipped (pre-existing omnimarket-not-in-CI-venv
skips, unrelated).
Stacked on OMN-14531 (#2291, not yet merged) -- this branch is built
on top of jonah/omn-14531-aislop-sweep-dispatch to reuse its
omnimarket_drift_guard.py fix rather than duplicating the reordering
logic. This PR's diff will resolve down to just the cli_node.py +
test changes once #2291 merges to dev.
OCC companion owed to Codex (per operating rules -- no self-authored
OCC in this lane).
Evidence-Ticket: OMN-14560
Summary
OMN-14531 (detector-shelf unblinding): the
onex:aislop_sweepskill's onlydocumented dispatch path —
uv run onex skill aislop_sweep— washard-failing with
Unknown node 'node_aislop_sweep'becauseomnimarket(the co-installed provider of
onex.nodesentry points for market nodes,including
node_aislop_sweep) was completely absent from theomnibase_infravenv on this operator's machine. This is the OMN-13829 →OMN-14060 recurrence going one step further: the co-install regressed from
"stale" to "gone", and nothing caught it.
Two compounding gaps in
omnimarket_drift_guard.py/cli_skill.pyletthis go undetected instead of failing loud:
check_omnimarket_drift()failed open on "not installed" unconditionally.It returned silently whenever
installed_omnimarket_commit()wasNone,even when a canonical
$OMNI_HOME/omnimarketclone WAS present andreachable — a determinable, actionable state, not the "can't reason about
it" case the fail-open design exists for. Reordered the checks: canonical
clone absent → still fails open (CI, fresh machines, unchanged). Canonical
clone present + omnimarket absent/non-VCS → now raises
OmnimarketDriftErrornaming the exact repair command.--omni-homehad noenvvarbinding. Even with gap (1) fixed, theguard never received a real value in normal usage: callers never pass
--omni-homeexplicitly, and exporting$OMNI_HOMEin the shell had zeroeffect on the CLI. Bound the option to the
OMNI_HOMEenvironmentvariable so the guard is actually reachable without an undocumented flag.
Also includes an unrelated, pre-existing chore commit: vendoring
node_projection_context_roi/002_...sqlto satisfy the localonex-check-node-migration-syncpre-commit hook (repo-wide drift introducedby OMN-14535, already fixed for real in omnibase_infra#2290 / OMN-14555,
not yet merged to dev). This hook is not a required GitHub status check
(confirmed live in ROLLING_WORK_LEDGER 2026-07-13), so it has no bearing on
mergeability — it only unblocks the local commit gate for the actual fix in
this PR. Expected to be a no-op once #2290 merges and this branch rebases.
RED → GREEN evidence (dod_evidence)
Manually reproduced against the real
onexCLI (the exactuv run onex skill aislop_sweeppath documented inplugins/onex/skills/aislop_sweep/):BEFORE (omnimarket absent from the omnibase_infra venv):
AFTER (
scripts/install-node-skill-package.sh --executerepairs thevenv; this PR's fix is what makes the failure loud/actionable instead of a
silent, unhelpful "Unknown node" wall of text next time it drifts):
repos_scanned=1, 11 real findings including CRITICAL-severity hits — thehandler and dispatch pipeline were never broken; only the venv provisioning
and this guard's blind spot were.
Automated regression coverage (new, both pass RED→GREEN against the prior
code):
test_drift_check_raises_when_not_installed_but_canonical_clone_present—fails under the old unconditional fail-open code, passes under the fix.
test_omnimarket_drift_guard.pyfull suite: 14 passed.test_cli_skill.pyfull suite (with a new autouse fixture keepingcommand-wiring tests hermetic against the now-live
--omni-homeenvvar):341 passed, 19 skipped (pre-existing omnimarket-not-in-CI-venv skips,
unrelated).
Local gates:
ruff format --check/ruff check/mypyclean on touchedfiles; full
pre-commit runclean.Known related gap (out of scope for this PR)
cli_node.py(onex node/onex run) has NO drift-guard call at all —the runbook's own documented symptom example (
onex run-node node_pr_lifecycle_orchestrator) uses that exact path.aislop_sweepisonly reachable via
onex skill, so this PR stays scoped there; flagging fora follow-up ticket.
Evidence-Ticket: OMN-14531
OCC Evidence
Evidence-Source: OCC#4103
Evidence-Ticket: OMN-14531
Evidence-Commit: 798602635940a856f78f13d126406578e770e1f6