Repository navigation
feat(OMN-13321): vendor node_pr_lifecycle_state_reducer ledger migration into infra forward-migration tree - #2112
Conversation
…ion into infra forward-migration tree The omnimarket node node_pr_lifecycle_state_reducer (merged in #1440) declares projection_api over pr_lifecycle_ledger_entries and UPSERTs one user-readable ledger row per PR EVERY sweep iteration. Its node-owned migration (0001_create_pr_lifecycle_ledger_entries.sql) creates the table in omnidash_analytics, but the vendored SQL was NEVER committed into omnibase_infra's tracked forward-migration tree (docker/migrations/forward/nodes/node_pr_lifecycle_state_reducer/). The forward-migration runner therefore had no SQL to apply, the table was missing on dev, and the OMN-13415 migration-sync guard correctly aborted the deploy. Fix: vendor the node migration via scripts/sync-node-migrations.sh (1:1 mirror, byte-identical to the omnimarket source @ dev/#1440). The runner applies it under the namespaced id node:node_pr_lifecycle_state_reducer:0001_create_pr_lifecycle_ledger_entries.sql, so a clean clone / redeploy materializes pr_lifecycle_ledger_entries. TDD: added test_pr_lifecycle_ledger_migration_vendored (failing before the vendor, passing after) pinning the vendored file + ticket-required ledger fields + UNIQUE(sweep_id,repo,pr_number,iteration) conflict key. This is the OMN-13636 migration-gap class made concrete. Redeploy is a separate orchestration step (note for the batched redeploy).
|
Warning Review limit reached
More reviews will be available in 56 minutes and 28 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 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 (2)
📝 WalkthroughWalkthroughAdds a forward migration that creates ChangesPR lifecycle ledger migration
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/migrations/test_node_migration_discovery.py (1)
331-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the conflict-key contract, not one DDL spelling.
This hardcodes the inline
UNIQUE (...)form, so it will fail if the migration is fixed to use a standaloneCREATE UNIQUE INDEX IF NOT EXISTSfor warm-table reconciliation. Assert either form, or pin a dedicated unique-index name instead.🤖 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 `@tests/unit/migrations/test_node_migration_discovery.py` around lines 331 - 333, The test for the migration conflict-key contract is too specific to the inline UNIQUE constraint spelling, so it will break if the migration uses a standalone unique index instead. Update the assertion in test_node_migration_discovery to validate the conflict-key behavior more generally by accepting either the UNIQUE (...) table constraint form or the CREATE UNIQUE INDEX IF NOT EXISTS form, or by asserting against the dedicated unique-index name rather than raw SQL text.
🤖 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_pr_lifecycle_state_reducer/0001_create_pr_lifecycle_ledger_entries.sql`:
- Around line 36-50: The `CREATE TABLE IF NOT EXISTS` in the
`pr_lifecycle_ledger_entries` migration only adds the `UNIQUE (sweep_id, repo,
pr_number, iteration)` constraint on fresh tables, so warm existing tables can
still miss the `ON CONFLICT` target. Update this migration to explicitly ensure
the conflict key exists for `public.pr_lifecycle_ledger_entries` even when the
table is preexisting, using the table name and unique key as the locating
symbols, so the reducer’s upsert remains idempotent.
---
Nitpick comments:
In `@tests/unit/migrations/test_node_migration_discovery.py`:
- Around line 331-333: The test for the migration conflict-key contract is too
specific to the inline UNIQUE constraint spelling, so it will break if the
migration uses a standalone unique index instead. Update the assertion in
test_node_migration_discovery to validate the conflict-key behavior more
generally by accepting either the UNIQUE (...) table constraint form or the
CREATE UNIQUE INDEX IF NOT EXISTS form, or by asserting against the dedicated
unique-index name rather than raw SQL text.
🪄 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: 4a1df009-a1ad-4ad4-81a8-eee7bb93c3b4
📒 Files selected for processing (2)
docker/migrations/forward/nodes/node_pr_lifecycle_state_reducer/0001_create_pr_lifecycle_ledger_entries.sqltests/unit/migrations/test_node_migration_discovery.py
|
CR thread gate refresh: unresolved review threads are resolved after re-vendoring the omnimarket hardening in 113e768. Please rerun the thread gate on the current PR head. |
OMN-13321 (REOPEN) — vendor the missing node migration that the batched redeploy surfaced
This reopens / completes OMN-13321. The omnimarket node code merged in omnimarket #1440, but its projection table could not persist on dev:
node_pr_lifecycle_state_reducerdeclaresprojection_apioverpr_lifecycle_ledger_entries, but the node-owned migration that creates the table inomnidash_analyticswas never committed into omnibase_infra's tracked forward-migration tree (docker/migrations/forward/nodes/node_pr_lifecycle_state_reducer/). The forward-migration runner therefore had no SQL to apply, the table was missing on dev, and the OMN-13415 migration-sync guard correctly aborted the deploy. This is the OMN-13636 migration-gap class made concrete.Fix
scripts/sync-node-migrations.sh(the canonical OMN-12559 vendoring path). The vendored file is byte-identical to the omnimarket source @dev/fix(OMN-10168): add orchestrator dispatcher coverage gate #1440 (cmpIDENTICAL).run-forward-migrations.shapplies it under the namespaced idnode:node_pr_lifecycle_state_reducer:0001_create_pr_lifecycle_ledger_entries.sql, so a clean clone / redeploy materializespr_lifecycle_ledger_entries.test_pr_lifecycle_ledger_migration_vendored(failed before the vendor, passes after) pinning the vendored file, the ticket-required ledger fields, and theUNIQUE(sweep_id,repo,pr_number,iteration)conflict key.dod_evidence
test_pr_lifecycle_ledger_migration_vendoredfailed (file absent) before vendoring, passes after.scripts/sync-node-migrations.sh --check→check: in sync(exit 0). Pre-commitONEX Node Migration Vendor Sync Check→ Passed.IF NOT EXISTS); table created with all 12 columns, the UNIQUE constraint, and 3 indexes (idx_pr_lifecycle_ledger_sweep,..._sweep_iter,..._found_at).select count(*) from pr_lifecycle_ledger_entries where sweep_id='20260626-dodproof'returns 8; distinct iterations{0,1}, 4 rows each; re-applying iteration 1's UPSERT keeps count at 8 (no overwrite, idempotent).tests/unit/migrations/ + tests/ci/test_validate_migration_sequence.py + tests/scripts/test_check_deployed_migration_tree_sync.py→ 79 passed;mypy --strictclean on changed test;ruff format/checkclean;pre-commit run --files <changed>all green (migration freeze / sequence-duplicate / vendor-sync / writer-migration-coupling all Passed).Redeploy is a separate orchestration step — a forward-migration run (batched redeploy) will apply the now-vendored SQL and create the table on dev. This PR does not mutate any runtime lane. No live deploy is performed here.
Evidence-Source: OCC#3165
Evidence-Ticket: OMN-13321
The paired OCC receipt PR #3165 carries the OMN-13321 contract update + the
dod-omnibase-infra-pr-2112receipt binding this PR;dod-occ-pr-selfself-binds to OCC #3165. Relates OMN-13636, OMN-13415.Closes OMN-13321 (vendoring gap).