Repository navigation
feat(OMN-12559): auto-discover omnimarket node migrations into infra forward set - #1820
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThis PR implements node-owned migration auto-discovery and vendoring for omnimarket node projects. It introduces a sync script, updates the forward migration runner to apply node migrations with namespaced tracking, excludes node migrations from flat sequence validation, adds delegation and savings projection tables and views, and provides comprehensive unit and integration tests. ChangesNode Migration Discovery & Vendoring
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Codex foreground tick: fixed the Receipt Gate metadata by replacing the placeholder |
|
Codex foreground tick: OCC#2007 now has a fresh pushed evidence commit |
|
Codex foreground tick: added missing |
|
Codex foreground tick: Receipt Gate run 26720541331 attempt 4 completed |
|
Codex foreground tick: OCC#2007 is now merged with all fresh checks green, so OMN-12559 central evidence is durable in onex_change_control. Cleaned up the merged OCC worktree under |
5d17e5c to
7117d0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/sync-node-migrations.sh (1)
168-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDetect/prune stale vendored migrations in drift checks.
Current logic only iterates allowlisted source files. Extra files already present under
docker/migrations/forward/nodes/**are never flagged (check mode) or removed (sync mode), so obsolete migrations can still be applied.Suggested fix
DRIFT=0 COPIED=0 +EXPECTED_REL="$(mktemp)" +trap 'rm -f "${EXPECTED_REL}"' EXIT # Discover every node migration file and mirror it (allowlisted nodes only). while IFS= read -r src_file; do @@ filename="$(basename "${src_file}")" if ! file_is_allowed "${node_name}" "${filename}"; then continue fi + printf '%s/%s\n' "${node_name}" "${filename}" >> "${EXPECTED_REL}" dest_dir="${DEST_ROOT}/${node_name}" dest_file="${dest_dir}/${filename}" @@ done < <(find "${NODES_DIR}" -type f -path "*/migrations/*.sql" | sort) + +# Detect vendored files that are no longer expected (removed upstream or de-allowlisted). +while IFS= read -r existing_dest; do + rel="${existing_dest#${DEST_ROOT}/}" + if ! grep -Fxq "${rel}" "${EXPECTED_REL}"; then + if [ "${CHECK_MODE}" -eq 1 ]; then + echo "[sync-node-migrations] DRIFT (stale vendored file): ${rel}" >&2 + DRIFT=1 + else + rm -f "${existing_dest}" + echo "[sync-node-migrations] removed stale ${rel}" + COPIED=$((COPIED + 1)) + fi + fi +done < <(find "${DEST_ROOT}" -type f -name "*.sql" | sort)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/sync-node-migrations.sh` around lines 168 - 205, The script only checks source migrations and never detects or removes extra files under DEST_ROOT, so stale vendored migrations remain; add a second pass that discovers every dest_file (find "${DEST_ROOT}" -type f -path "*/migrations/*.sql") and for each compute node_name and filename (same basename/dirname logic), skip if file_is_allowed(node_name,filename) is false, then if the corresponding src_file ("${NODES_DIR}/${node_name}/migrations/${filename}") does not exist treat it as drift: in CHECK_MODE set DRIFT=1 and echo a DRIFT message, else in sync mode delete the dest_file (and remove empty parent dir if desired), log removal and increment a counter (or separate REMOVED counter) so output reflects removals; ensure checks use the same file_is_allowed, CHECK_MODE, DRIFT, COPIED symbols and produce consistent messages as existing logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@docker/migrations/forward/nodes/node_projection_savings/076_create_delegation_savings_projection_view.sql`:
- Around line 45-46: The COALESCE fallback for the latency column currently uses
delegation_latency_ms before latency_ms and should prefer the canonical
latency_ms first; update the projection so latency_ms is chosen when present
(i.e., use COALESCE(latency_ms, delegation_latency_ms)::int AS latency_ms) to
avoid returning stale delegation_latency_ms values while leaving the created_at
line unchanged.
In `@tests/ci/test_validate_migration_sequence.py`:
- Around line 225-264: Add the pytest unit marker to the two new test functions
in tests/ci/test_validate_migration_sequence.py: annotate
test_node_subtree_file_does_not_collide_with_flat_sequence and
test_node_subtree_excluded_even_when_flat_migration_staged with
`@pytest.mark.unit`; ensure pytest is imported at the top of the file if not
already (add import pytest) so the decorator resolves.
In `@tests/integration/migrations/test_node_migration_discovery_applies.py`:
- Around line 61-103: The tests call PostgresConfig.from_env() and run
subprocesses in _require_db, _query, and the test runner without first sourcing
~/.omnibase/.env and they replace the whole environment when calling
subprocess.run; update _require_db, any places that call
PostgresConfig.from_env(), _query, and the runner invocation to first load and
merge the dotenv (e.g. parse ~/.omnibase/.env into a dict) into os.environ (or
into a new env = {**os.environ, **dotenv_vars}) before using
PostgresConfig.from_env() and pass that merged env to subprocess.run instead of
the current env={...} overwrite so inherited vars (like DB DSN/KV settings) are
preserved for subprocesses and config resolution.
---
Outside diff comments:
In `@scripts/sync-node-migrations.sh`:
- Around line 168-205: The script only checks source migrations and never
detects or removes extra files under DEST_ROOT, so stale vendored migrations
remain; add a second pass that discovers every dest_file (find "${DEST_ROOT}"
-type f -path "*/migrations/*.sql") and for each compute node_name and filename
(same basename/dirname logic), skip if file_is_allowed(node_name,filename) is
false, then if the corresponding src_file
("${NODES_DIR}/${node_name}/migrations/${filename}") does not exist treat it as
drift: in CHECK_MODE set DRIFT=1 and echo a DRIFT message, else in sync mode
delete the dest_file (and remove empty parent dir if desired), log removal and
increment a counter (or separate REMOVED counter) so output reflects removals;
ensure checks use the same file_is_allowed, CHECK_MODE, DRIFT, COPIED symbols
and produce consistent messages as existing logic.
🪄 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: c24bbc68-4004-43da-8873-4b8df8c7d7dd
📒 Files selected for processing (19)
.pre-commit-config.yamlcontracts/OMN-12559.yamldocker/Dockerfile.migratedocker/migrations/forward/nodes/.synced-nodesdocker/migrations/forward/nodes/node_projection_delegation/0007_delegation_events.sqldocker/migrations/forward/nodes/node_projection_delegation/0008_generation_events.sqldocker/migrations/forward/nodes/node_projection_delegation/0009_delegate_skill_projection_metrics.sqldocker/migrations/forward/nodes/node_projection_delegation/0010_create_delegation_dashboard_projection_views.sqldocker/migrations/forward/nodes/node_projection_savings/076_create_delegation_savings_projection_view.sqlpyproject.tomlscripts/run-forward-migrations.shscripts/sync-node-migrations.shscripts/validation/validate_migration_sequence.pytests/ci/test_validate_migration_sequence.pytests/integration/migrations/__init__.pytests/integration/migrations/test_node_migration_discovery_applies.pytests/unit/migrations/__init__.pytests/unit/migrations/test_node_migration_discovery.pytests/unit/runtime/auto_wiring/test_handler_wiring_handle_async_dispatch.py
Pull request was converted to draft
7117d0f to
f094490
Compare
…forward set Vendor omnimarket projection node migrations into a namespaced location (docker/migrations/forward/nodes/<node>/) and extend run-forward-migrations.sh to auto-discover and apply them under namespaced migration ids (node:<node>:<file>). Reproduces the OMN-12489 delegation dashboard + savings projection views on a clean clone with NO manual copy and NO manual renumber, eliminating the savings 076 -> 084 renumber-as-operational-pattern. - Vendored: node_projection_delegation 0007-0010 (base tables + views) and node_projection_savings 076 view, read verbatim from omnimarket origin/dev. - scripts/sync-node-migrations.sh: discovers + vendors node migrations from an installed/cloned omnimarket (allowlist-scoped), with --check drift guard. - run-forward-migrations.sh: namespaced node:<node>:<file> discovery pass. - validate_migration_sequence.py: exclude the namespaced nodes/ subtree so a node 076 never collides with the flat infra 076 (no renumber required). - Dockerfile.migrate COPY is recursive -> nodes/ ships in the migrate image. - Pre-commit hook wires the sync --check drift guard (enforcement, not detection). - Tests: unit (vendoring + wiring + drift), integration (real-DB discovery applies views + namespace no-collision, opt-in), and sequence-validator exclusion cases. Also fixes a pre-existing SPDX year violation in tests/unit/runtime/auto_wiring/test_handler_wiring_handle_async_dispatch.py.
…uard The contract-validation CI gate rejected evidence_requirements[1].kind: 'validation' (not in enum tests/docs/ci/benchmark/manual). The drift guard (sync-node-migrations.sh --check, wired as a pre-commit hook + CI) is a CI gate, so 'ci' is the correct kind. Evidence-Ticket: OMN-12559 Evidence-Source: OCC#2007
c0a72f5 to
5da58dc
Compare
…omnimarket source (OmniNode-ai#1837) The vendored copy of node_projection_savings 076_create_delegation_savings_projection_view.sql diverged from its canonical owner in omnimarket (OMN-12559 / PR OmniNode-ai#1820 vendored a non-verbatim copy): the COALESCE argument order on the latency_ms projection column was swapped. omnimarket source has COALESCE(delegation_latency_ms, latency_ms); the vendored infra copy had COALESCE(latency_ms, delegation_latency_ms). omnimarket owns its node migrations, so the vendored copy must mirror the source 1:1. Re-synced via scripts/sync-node-migrations.sh. The auto-discovery drift guard (tests/unit/migrations/test_node_migration_discovery.py::test_sync_check_reports_in_sync) now passes when the omnimarket source is resolvable; it FAILED before this fix. Adds contracts/OMN-12597.yaml for the deploy-gate. Plan: docs/plans/2026-06-02-system-stability-delegation-sea-recovery-plan.md Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com>
OMN-12559: auto-discover omnimarket node migrations into the infra forward set
Implements OMN-12559.
Addresses the integration gap surfaced by OMN-12489 (PR #1005): omnimarket projection nodes ship SQL under
src/omnimarket/nodes/*/migrations/*.sql, but the forward-migration runner only readdocker/migrations/forward/. Landing the delegation/savings views previously needed a manual host copy + a hand-renumber (savings076 -> 084) to dodge a numeric collision with the flat infra076. This eliminates that renumber-as-operational-pattern.What changed
docker/migrations/forward/nodes/<node>/, read verbatim from omnimarketorigin/dev(HEAD26a343de):node_projection_delegation0007–0010 (base tables + dashboard views) andnode_projection_savings076 (savings view).scripts/run-forward-migrations.sh: new discovery pass walksforward/nodes/<node>/*.sqland applies each under a namespacednode:<node>:<file>migration_id— a separate identity space from the flatdocker/<file>sequence, so a node076never collides with infra flat076. No renumber required.scripts/sync-node-migrations.sh: auto-discovers + vendors node migrations from an installed/cloned omnimarket (allowlist-scoped vianodes/.synced-nodes), with a--checkdrift guard wired as a pre-commit hook (enforcement, not detection).scripts/validation/validate_migration_sequence.py: excludes the namespacednodes/subtree from the flat sequence check.docker/Dockerfile.migrate: COPY is recursive, so thenodes/subtree ships in the migrate image automatically.dod_evidence (fresh-clone reproduction proof)
run-forward-migrations.shagainst a throwaway local Postgres pointed at a tempMIGRATIONS_DIR(savings base 074 + thenodes/subtree). The runner auto-discovered and applied all 5 node migrations under namespaced ids and createdprojection_delegation_summary,projection_delegation_model_routing,projection_delegation_quality_gate,projection_delegation_token_usage,projection_delegation_savings— with no manual copy. Re-run = all skipped (idempotent). Infra flat076and node076coexist as distinctschema_migrationsrows (no collision).tests/unit/migrations/+tests/integration/migrations/+tests/ci/test_validate_migration_sequence.pygreen. Full unit suite: 19981 passed, 15 skipped. CI validator suite: 133 passed. The integration test that mutates a DB is opt-in (RUN_NODE_MIGRATION_DISCOVERY_TEST=1) and skips by default.pre-commit run --all-files: clean (also fixed a pre-existing SPDX year violation intest_handler_wiring_handle_async_dispatch.py).Contract:
contracts/OMN-12559.yaml(in-repo, deploy-gate) and the central OCC contract + receipts (Evidence-Source below).Deploy-gate: no live deploy performed pre-merge; the next migrate image build carries the
nodes/subtree and the discovery runner.Evidence-Ticket: OMN-12559
Evidence-Source: OCC#2080
Summary by CodeRabbit
New Features
Infrastructure/Chores
Tests