Repository navigation
fix(OMN-13930): typed-skill startup self-check — named fail-closed drift override + wire the guard into onex delegate - #2503
Conversation
…ift override + wire the guard into `onex delegate` The omnimarket drift guard already compared the installed co-install SHA against the canonical clone HEAD and refused with a repair pointer (OMN-14060/14531/14560). Two gaps remained in that self-check: 1. **No escape hatch, and none named.** The refusal was unconditional with no supported override, so the only way past it was unsetting `$OMNI_HOME` — which disables the guard globally, silently, on every surface. Adds `ONEX_ALLOW_OMNIMARKET_DRIFT`, named in every refusal message so the hatch is discoverable from the failure alone. Refusal stays default-ON: the guard takes a keyword-only `allow_drift=False`, so a call site added later that forgets it fails CLOSED. An override that actually suppresses a refusal logs a WARNING every dispatch — a silent bypass would recreate the invisible-drift failure the guard exists to end. 2. **`onex delegate` had zero guard wiring.** `DELEGATE_NODE_NAME` (`node_delegate_skill_orchestrator`) is omnimarket-provided, so it carried the same stale/absent co-install exposure as `onex skill` and `onex node` — but a drifted venv surfaced there as a bare contract-resolution failure with no pointer to the repair command. Now guarded, before any bus probe or payload write, so a drifted venv never produces a receipt that could be mistaken for evidence. The env var is read at the CLI boundary via click `envvar=` (the mechanism `--omni-home` already uses), never with a raw `os.environ` read in `src/` — the first cut did the latter and was correctly rejected by the `check-env-reads` pre-commit hook; the guard is now a pure function of its arguments. Click BOOL conversion is what makes the override fail closed: `0`/`false` parse False and an unparseable value is a hard usage error, so neither silently disables the guard. RED-first. Behavioral RED captured before the fix (14 failed on override semantics; 54 errors from the absent delegate wiring), not just a missing symbol. The env→argument binding is the load-bearing seam — an unbound option is exactly the OMN-14531 silent-no-op trap — so it is proven through the real command with the real env var, and mutation-checked: deleting `envvar=` turns `test_drift_override_env_is_actually_bound_to_the_flag` RED. Post-sync smoke already exists and is now documented: `--repair` re-runs `install-node-skill-package.sh`, whose step 3 asserts the mapped skill nodes actually resolve from `onex.nodes` entry points afterward. Gates (.200, rule 11a; patch-transferred with per-file sha256 verified equal on both hosts): ruff + format + mypy (2617 files) clean, pre-commit clean on all 8 changed files, unit suite 22146 passed / 0 failed. Full suite failures are live-service integration/performance only (Kafka/Postgres/LLM/containers) plus a pre-existing xdist env-contamination flake class — clean `dev` baselined 2 unit failures in the same run mode where this branch had 0. Refs: OMN-13930, OMN-13829, OMN-14060, OMN-14064, OMN-14531, OMN-14560
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 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 (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…ibase_infra#2503 (#5155) * evidence(OMN-13930): author OCC companion for OmniNode-ai/omnibase_infra#2503 OCC companion by node_pr_lifecycle_fix_effect (OMN-13317 F1 / OMN-13990 / OMN-14285). Product PR head 304891d121cf2551a95fae2c834b23fa716065e5. * evidence(OMN-13930): self-bind OCC#5155 + rebind contract_sha256 --------- Co-authored-by: omnimarket-bot <bot@omninode.ai>
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical findings | No |
blocked |
CRITICAL findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524)
1 participant
Ticket
OMN-13930 — [Friction]
merge_sweep:tooling/cli-node-resolution— market skills CLI env-drift (OMN-13829 class).Friction #7 from the 2026-07-27 friction sweep: typed-skill startup self-check — compare the installed omnimarket/onex skill environment SHA against canonical HEAD, refuse to run a stale typed skill with a one-command sync remedy, tiny post-sync smoke, default-ON fail-closed refusal with an explicit override env named in the error.
What was already there (scoping correction)
Most of that self-check already shipped under OMN-14060 / OMN-14531 / OMN-14560 and I did not rebuild it:
omnimarket_drift_guard.py, localgit rev-parse, no network)install-node-skill-package.shstep 3 asserts mapped skill nodes resolve fromonex.nodesonex skill/onex node/onex runonex delegateThis PR closes the last two rows only.
Changes
1.
ONEX_ALLOW_OMNIMARKET_DRIFT— the named, fail-closed escape hatch.The refusal was unconditional with no supported override, so the only way past it was unsetting
$OMNI_HOME— which disables the guard globally and silently on every surface, strictly worse than the drift it hides. The variable is named in every refusal message, so the hatch is discoverable from the failure alone.Refusal stays default-ON:
check_omnimarket_drifttakes a keyword-onlyallow_drift: bool = False, so a call site added later that forgets it fails closed. An override that actually suppresses a refusal logs a WARNING on every dispatch — a silent bypass would recreate the invisible-drift failure the guard exists to end. No warning fires when there is no drift, so the warning keeps its signal.2.
onex delegateis now guarded.DELEGATE_NODE_NAME(node_delegate_skill_orchestrator) is omnimarket-provided, so this surface always carried the same stale/absent co-install exposure asonex skillandonex node— it was simply the one of the three never wired. A drifted venv surfaced there as a bare contract-resolution failure with no pointer to the repair command. The check runs before any bus probe or payload write, so a drifted venv never produces a receipt that could be mistaken for evidence.Seams
envvar=(the same mechanism--omni-homealready uses), never with a rawos.environread insrc/. My first cut did the latter and was correctly rejected by thecheck-env-readspre-commit hook; rather than allowlist the file (forbidden by CLAUDE.md §7a), the guard became a pure function of its arguments. This is the better design regardless of the hook.BOOLconversion:0/falseparse False, an unparseable value is a hard usage error. Neither silently disables the guard — which matters because a variable left exported from an earlier session must not become standing consent.--allow-omnimarket-drift/--omni-homeare bound on all three commands (skill,node/run,delegate). The envvar binding is load-bearing: an unbound option is exactly the OMN-14531 silent-no-op trap where the guard receivedomni_home=Noneand never fired.Verification
RED-first, behaviorally. RED was captured before the fix as 14 failed (override semantics) + 54 errors (absent delegate wiring), not merely a missing-symbol ImportError — the constant was added first so the failures were about behavior.
The binding is mutation-checked. Deleting
envvar=DRIFT_OVERRIDE_ENVturnstest_drift_override_env_is_actually_bound_to_the_flagRED; restoring it turns it green. The test asserts execution reached a different downstream error (Unknown skill), proving it got past the guard rather than just exiting non-zero for a new reason.Coverage added: refusal names the env on both refusal paths; refusal is default-ON; override downgrades to a loud warning; no warning without drift; env unset refuses; env
1clears; env0/falsestill refuse; guard fires before delegate dispatch (withrun_receipt_modestubbed to raise if reached).Gates — run on
.200per rule 11a, patch-transferred with per-filesha256verified equal on both hosts before the run (no vacuous green from the edit-locality trap):ruff check+ruff format --check(4535 files) — cleanmypy src/omnibase_infra/(2617 files) — cleanpre-commit run --filesover all 8 changed files — cleanpytest tests/unit/ -n auto— 22146 passed, 0 failedFull-suite failures are live-service integration/performance only (Kafka
localhost:19092, Postgres, LLM endpoints, container health) and are not attributable to this diff. Rather than call them transient: cleandevbaselined 2 unit failures in the same-n automode where this branch had 0, and the 4 unit failures seen in the first full run pass in isolation — a pre-existing xdist env-contamination flake class in this repo, not introduced here.Notes
Evidence-Sourceline.Refs: OMN-13930, OMN-13829, OMN-14060, OMN-14064, OMN-14531, OMN-14560
Evidence-Ticket: OMN-13930
Evidence-Source: 0ff771355e8d56700a2434efe5a16ff2baa9d14d