Repository navigation
fix(OMN-14555): vendor node_projection_context_roi 002 migration; declare node-migration-sync parity gap - #2290
Merged
Conversation
…clare node-migration-sync parity gap (OMN-14556) omnimarket PR #1743 (OMN-14535) added src/omnimarket/nodes/node_projection_context_roi/migrations/002_add_factor_subset_hash_and_routing_source.sql on dev but the vendored copy under docker/migrations/forward/nodes/ was never created — the local pre-commit hook only fires on infra commits and cannot see an omnimarket-only PR. This is the exact OMN-13124 pattern_learning drift class the node-migration-sync gate exists to catch. Reproduced: `OMNIMARKET_SRC=<omnimarket@dev> scripts/sync-node-migrations.sh --check` exits 1 (DRIFT) before this commit, 0 (in sync) after. Also declares node-migration-sync as a `coverage: direct` load_bearing_gate in scripts/enforcement_parity_manifest.yaml (OMN-14556). node-migration-sync.yml is its own workflow file with its own run_id, separate from ci.yml — the CI Summary poller (ci_summary_gate.py) only inspects actions/runs/${RUN_ID}/jobs for its OWN run, so it structurally cannot see node-migration-sync's conclusion. Live proof: PR #2288 shows node-migration-sync=FAIL and CI Summary=PASS in two different run_ids. The check is also absent from dev's required_status_checks, so a red node-migration-sync does not block merge today. This manifest entry makes the report-only OMN-14288 parity ratchet flag the gap (`audit_required_context_parity_cli.py report` -> [MISSING] node-migration-sync) until the required_status_checks PUT lands — that mutation itself is left for operator/main sign-off per the branch-protection change-control note in CLAUDE.md rather than executed unilaterally in an unattended session.
📝 WalkthroughWalkthroughThe PR adds ChangesContext ROI schema
Enforcement parity
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
jonahgabriel
added a commit
that referenced
this pull request
Jul 13, 2026
…ot (#2291) * chore: vendor node_projection_context_roi migration (pre-existing drift, 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. * fix(OMN-14531): close the omnimarket drift guard's fail-open blind spot 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.
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
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
node-migration-sync(workflownode-migration-sync.yml) has been RED dev-wide on omnibase_infra since omnimarket PR #1743 (OMN-14535, merged to omnimarketdev) addedsrc/omnimarket/nodes/node_projection_context_roi/migrations/002_add_factor_subset_hash_and_routing_source.sqlwithout a corresponding vendoring PR here. Confirmed independently red on unrelated PRs #2288 and #2289, neither of which touches any migration file.Closes OMN-14555. Addresses the vendoring half of OMN-14556; see the parity decision below for the remainder.
Root cause (OMN-14555)
scripts/sync-node-migrations.shmirrors everysrc/omnimarket/nodes/<node>/migrations/*.sqlintodocker/migrations/forward/nodes/<node>/so the forward-migration runner can materialize the projection table on a clean redeploy. The local pre-commit hook only fires on infra commits, so an omnimarket-only PR (#1743) that adds a node migration cannot trigger it — nobody ran the sync script in omnibase_infra afterward. This is the same drift class OMN-13124 (node_projection_pattern_learning) already burned once; the gate exists specifically to catch it, and did — it just had nowhere to go (see OMN-14556).RED → GREEN, reproduced locally:
Fix is the single vendored file — no other node migrations are out of sync.
Parity gap (OMN-14556) — investigated and DECIDED
Finding:
node-migration-syncis structurally invisible toCI Summary, not merely "missing from its aggregation."node-migration-sync.ymlis its own workflow file, triggered by the samepull_requestevent but running under its ownrun_id.ci_summary_gate.py(theCI Summarypoller) only callsgh api repos/.../actions/runs/${RUN_ID}/jobsfor its own run'sRUN_ID— it can never see a job from a sibling workflow run.node-migration-syncis not inSTRICT_GATE_JOBS,SKIPPABLE_GATE_JOBS, orSOFT_ALLOWLISTeither. This corrects the OMN-14556 ticket's working hypothesis ("CI Summary's internal aggregation... may already treat it as load-bearing") — it does not, and structurally cannot.Live proof (PR #2288, unrelated to this fix):
node-migration-syncfail29234681423CI Summarypass29234681594Different run IDs, same PR, same commit — CI Summary's pass is blind to node-migration-sync's fail. It is also absent from
required_status_checks(gh api repos/OmniNode-ai/omnibase_infra/branches/dev/protection/required_status_checks --jq '.contexts') and was, until this PR, absent fromscripts/enforcement_parity_manifest.yaml— invisible to all three enforcement layers at once.Decision: it should be required — this PR does not remove or weaken it. It's a real, meaningful, deterministic drift detector (prevents exactly the class of incident that shipped OMN-13124's
pattern_learning_artifactstable missing on a clean redeploy). The honest fix is to make it properly required, not to fold it into CI Summary's non-cross-workflow-visible aggregation.What this PR does toward that: adds
node-migration-synctoscripts/enforcement_parity_manifest.yamlunderomnibase_infra:devascoverage: direct(the only correct mode for a gate in its own workflow file, per the manifest's own doc comment). This makes the OMN-14288 report-only parity ratchet (scripts/audit_required_context_parity_cli.py report) surface it as[MISSING]going forward — durable, mechanized evidence instead of a one-off finding:What this PR deliberately does NOT do: execute the
gh api --method PUT .../branches/dev/protectionmutation to actually addnode-migration-synctorequired_status_checks.contexts. Per this repo's own branch-protection change-control note (CLAUDE.md → "Branch protection"), that mutation should run behind the dry-run audit (scripts/audit-branch-protection.sh --repo omnibase_infra --dry-run) with an operator/main sign-off, not be executed unilaterally by the same unattended session that just made the check pass. The dry-run audit was run for context and separately surfaced 4 pre-existing Check-B mismatches unrelated to this change (push-vs-pull_request context visibility ondev's last 5 commits) — flagged, not fixed, out of scope here.Recommended follow-up (owned by main/operator):
(existing 14 contexts + the one addition — re-read
required_status_checks.contextsbefore running to confirm no drift since this was captured.)Verification
sync-node-migrations.sh --check: RED before, GREEN after (shown above).tests/scripts/test_check_deployed_migration_tree_sync.py: 9 passed (sibling regression gate untouched).tests/ci/test_required_context_parity.py: 32 passed (manifest schema/logic untouched by the new entry).scripts/audit_required_context_parity_cli.py report: confirms[MISSING] node-migration-syncforomnibase_infra:devwith the new manifest entry.pre-commit run --files <both changed files>: clean, including "ONEX Node Migration Vendor Sync Check" (the local hook this class of drift always evaded).OCC companion
Owed, not authored in this PR — per the no-self-authored-evidence rule the implementer should not hand-author its own OCC receipt. Flagged to the merge controller for a companion PR in
onex_change_controldev citing this PR's merge commit.Evidence-Ticket: OMN-14555
Summary by CodeRabbit
Bug Fixes
Chores
Evidence-Source: OCC#4099
Evidence-Commit: 4e0184c9e3e5b322d57c7f16b17d1dccf9efc767