Repository navigation
fix(OMN-15717): declare node_pr_review_bot migration stream/domain + pre-merge declaration gate - #2678
Merged
jonahgabriel merged 8 commits intoAug 10, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe change adds legacy node migration declaration support across validation, synchronization, CI, and ledger adoption. It also routes event-forward backend resolution through contract metadata, uses external Keycloak issuer URLs, and updates endpoint examples and documentation. ChangesLegacy migration declarations
Event-forward endpoint resolution
External endpoint configuration
Endpoint documentation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HandlerEventForward
participant ContractDescriptor
participant Environment
participant Backend
HandlerEventForward->>ContractDescriptor: Resolve backend URL
ContractDescriptor->>Environment: Expand EVENT_FORWARD_BACKEND_URL
Environment-->>ContractDescriptor: Return resolved endpoint
HandlerEventForward->>Backend: Forward event request
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
jonahgabriel
added a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 6, 2026
#6153) * evidence(OMN-15717): add OCC companion for infra PR 2678 * evidence(OMN-15717): self-bind OCC PR 6153 * fix: add active OMN-15717 OCC admissibility evidence
Collaborator
Author
|
Follow-up repair commit It adds deterministic legacy-ledger declarations for the two historically applied Verification: global |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/run-forward-migrations.sh`:
- Around line 555-562: Update the read loop containing legacy_stream,
legacy_owner, and legacy_version to use a literal tab as its IFS separator,
matching the existing loops, so legacy_version is parsed correctly and the
active-artifact guard evaluates the intended migration path.
In `@src/omnibase_infra/models/registration/model_node_registration.py`:
- Around line 65-71: Update the ModelNodeRegistration example’s
resolved_health_endpoint value to a syntactically valid reserved URL such as
https://health.example.invalid/health, while keeping the same value used for
both endpoints["health"] and health_endpoint.
In `@src/omnibase_infra/services/contract_resolver/main.py`:
- Line 14: Configure explicit CORS origins in both application entry points:
update create_app in src/omnibase_infra/services/contract_resolver/main.py lines
14-14 and src/omnibase_infra/services/registry_api/main.py lines 14-19 to pass
cors_origins=["https://dashboard.example.invalid"].
🪄 Autofix
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: cecaf593-1556-479d-8255-85929730aa1d
⛔ Files ignored due to path filters (2)
docker/migrations/forward/_ledger/application-migrations.tsvis excluded by!**/*.tsvdocker/migrations/forward/_ledger/legacy-node-migrations.tsvis excluded by!**/*.tsv
📒 Files selected for processing (33)
.github/workflows/ci.yml.pre-commit-config.yamldocker/migrations/forward/_ledger/bootstrap.sqldocker/migrations/forward/nodes/node_pr_review_bot/001_create_review_bot_bypass_log.sqlscripts/ci/ci_summary_gate.pyscripts/provision-infisical.pyscripts/provision-keycloak.pyscripts/run-forward-migrations.shscripts/seed-infisical.pyscripts/sync-node-migrations.shscripts/validation/validate_application_migration_manifest.pysrc/omnibase_infra/adapters/llm/adapter_llm_provider_openai.pysrc/omnibase_infra/handlers/registration_storage/models/model_update_registration_request.pysrc/omnibase_infra/models/registration/model_node_registration.pysrc/omnibase_infra/models/registration/model_node_registration_record.pysrc/omnibase_infra/nodes/node_event_forward_effect/contract.yamlsrc/omnibase_infra/nodes/node_event_forward_effect/contract_descriptor.pysrc/omnibase_infra/nodes/node_event_forward_effect/handlers/handler_event_forward.pysrc/omnibase_infra/nodes/node_registration_reducer/registration_reducer.pysrc/omnibase_infra/nodes/node_registration_storage_effect/models/model_registration_record.pysrc/omnibase_infra/nodes/node_registry_effect/models/model_registry_request.pysrc/omnibase_infra/nodes/node_setup_validate_effect/handlers/handler_service_validate.pysrc/omnibase_infra/nodes/node_vector_store_effect/contract_descriptor.pysrc/omnibase_infra/services/contract_resolver/main.pysrc/omnibase_infra/services/registry_api/main.pysrc/omnibase_infra/utils/util_pydantic_validators.pytests/fixtures/omn15717/001_create_review_bot_bypass_log.sql.capturedtests/incident_replays/registry.yamltests/integration/migrations/test_application_migration_ledger_omn15413.pytests/unit/migrations/test_node_migration_discovery.pytests/unit/nodes/node_event_forward_effect/test_contract_descriptor.pytests/unit/nodes/node_event_forward_effect/test_handler.pytests/unit/scripts/validation/test_application_migration_manifest.py
| from omnibase_core.container import ModelONEXContainer | ||
| container = ModelONEXContainer() | ||
| app = create_app(container=container, cors_origins=["http://localhost:3000"]) | ||
| app = create_app(container=container) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Provide CORS configuration in both application examples.
Both examples call create_app() without cors_origins, while the implementations raise when CORS_ORIGINS is unset. The examples fail in a clean environment.
src/omnibase_infra/services/contract_resolver/main.py#L14-L14: passcors_origins=["https://dashboard.example.invalid"]tocreate_app().src/omnibase_infra/services/registry_api/main.py#L14-L19: passcors_origins=["https://dashboard.example.invalid"]tocreate_app().
Proposed fix
- >>> app = create_app(container=container)
+ >>> app = create_app(
+ ... container=container,
+ ... cors_origins=["https://dashboard.example.invalid"],
+ ... )📍 Affects 2 files
src/omnibase_infra/services/contract_resolver/main.py#L14-L14(this comment)src/omnibase_infra/services/registry_api/main.py#L14-L19
🤖 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 `@src/omnibase_infra/services/contract_resolver/main.py` at line 14, Configure
explicit CORS origins in both application entry points: update create_app in
src/omnibase_infra/services/contract_resolver/main.py lines 14-14 and
src/omnibase_infra/services/registry_api/main.py lines 14-19 to pass
cors_origins=["https://dashboard.example.invalid"].
This was referenced Aug 7, 2026
jonahgabriel
added a commit
that referenced
this pull request
Aug 8, 2026
- scripts/run-forward-migrations.sh: fix IFS='\t' (literal backslash-t, splits fields on the two characters t and \) to a real tab, matching the sibling loops immediately below it. legacy_version was receiving a wrong fragment. - model_node_registration.py: docstring example used a non-URL placeholder for resolved_health_endpoint, which fails HttpUrl validation in a clean doctest run; replaced with a syntactically valid https URL (annotated url-authority-ok since it is a docstring example value, not a runtime literal). - contract_resolver/main.py: module Usage example called create_app() without cors_origins, which raises since CORS_ORIGINS is unset in a clean environment; added the same cors_origins example already used elsewhere in the file.
jonahgabriel
enabled auto-merge (squash)
August 8, 2026 10:00
Collaborator
Author
|
Temporarily closed to relieve CI runner-fleet congestion (merge-sweep 2026-08-08, operator-approved — see ROLLING_WORK_LEDGER [mergesweep-0808-close]). Nothing is lost: branch, commits, and review state remain; this PR will be reopened in batches once the queue drains (reopen order recorded in the ledger). |
auto-merge was automatically disabled
August 8, 2026 10:12
Pull request was closed
…add pre-merge declaration gate node_pr_review_bot:001_create_review_bot_bypass_log.sql was vendored into docker/migrations/forward/nodes/ (applied to at least one live database) without ever gaining a row in application-migrations.tsv. No pre-merge gate caught the omission, so it shipped silently; the gap surfaced only when a workspace-mode refresh_stability_lane.sh run hit bootstrap.sql's fail-closed "unknown migration stream/domain" guard against a database carrying the legacy adopted-history row for this exact migration id. - Restore the vendored migration file (unmodified historical bytes, sha256 verified against the omnimarket commit that introduced it) and declare it in application-migrations.tsv as node:node_pr_review_bot / omninode_internal (R-q: a bypass-audit log is orchestration bookkeeping, not tenant workload data -- confirmed against the classification of sibling governance nodes node_pr_lifecycle_state_reducer / node_pr_merged_projection / node_merge_state_projection). - Fix scripts/sync-node-migrations.sh: a vendored file with a checked-in manifest declaration is preserved applied history (OMN-15695 ruling), not drift, even after its omnimarket source is deleted -- otherwise the node-migration-sync gate would strip the file we just declared right back out on the next sync. - New scripts/check_node_migration_declarations.py: static, DB-free pre-merge gate asserting every vendored node migration file has exactly one declaration or explicit block. Wired as a CI job (registered in ci_summary_gate.STRICT_GATE_JOBS so it's fail-closed under dev's sole required "CI Summary" context) and a pre-commit hook. - Incident-replay case (OMN-15547 registry): captured the real historical file bytes from omnimarket@cedd2431 and drives the real guard against them with zero declarations, reproducing the exact pre-fix tree shape. RED verified against the live production error before the fix (bootstrap.sql adoption test against a real ephemeral Postgres, and the new declaration gate against the live tree); GREEN after. Evidence-Source: lab-runtimes lane 2026-08-05/06, forensic log .201:/tmp/refresh_stability_lane_20260805.log
…d of a new script scripts/validation/validate_application_migration_manifest.py already implemented the exact "declared set == vendored tree" check the previous commit reinvented as scripts/check_node_migration_declarations.py -- plus checksum, duplicate, and shape validation the new script didn't have. It was simply never wired to anything: no CI job, no pre-commit hook, only its own test file called it directly. - Delete the duplicate scripts/check_node_migration_declarations.py and its bespoke test file. - Point the CI job and pre-commit hook added in the prior commit at the existing validator instead. - Move the OMN-15717 incident-replay case onto the existing validator's test file (tests/unit/scripts/validation/test_application_migration_manifest.py), reusing the same captured fixture. - Fix the stale hardcoded declaration-count assertion (94 -> 95) that this validator's own pre-existing test carried, which a background pre-push run of the full suite caught (tests/unit/) before push. Discovered while chasing a pre-push governed-selector full-suite escalation on the prior commit; net effect is less new surface than originally shipped.
- scripts/run-forward-migrations.sh: fix IFS='\t' (literal backslash-t, splits fields on the two characters t and \) to a real tab, matching the sibling loops immediately below it. legacy_version was receiving a wrong fragment. - model_node_registration.py: docstring example used a non-URL placeholder for resolved_health_endpoint, which fails HttpUrl validation in a clean doctest run; replaced with a syntactically valid https URL (annotated url-authority-ok since it is a docstring example value, not a runtime literal). - contract_resolver/main.py: module Usage example called create_app() without cors_origins, which raises since CORS_ORIGINS is unset in a clean environment; added the same cors_origins example already used elsewhere in the file.
jonahgabriel
force-pushed
the
jonah/omn-15717-migration-stream-domain-declaration
branch
from
August 10, 2026 06:57
5d4061c to
3eaf3c0
Compare
Contributor
✅ Hostile Reviewer — PASSEDBlocking findings (critical): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524) |
…alification gate + contract-sync touch Two CI failures surfaced on the reopened/rebased PR #2678 that predate this lane's own diff surface but are exposed by it: 1. Application Database Domain Enforcement (OMN-15361): the restored node_pr_review_bot/001_create_review_bot_bypass_log.sql targets an unqualified `review_bot_bypass_log` table. It is a legacy-declared, vendor-synced migration (node-migration-sync, OMN-13332) restored byte-identical to the original omnimarket commit -- sha256-verified and bound to the live omnidash_analytics.platform_catalog.schema_migrations row this ticket exists to declare. Qualifying the SQL now would break both the byte-identity proof and the checksum this migration's declaration row binds to. Adds it to _LEGACY_DEFAULT_SCHEMA_SQL_EXACT_PATHS, the same node-migration-sync-forced-vendoring exemption already used for the OMN-15732 node_canary_score_reducer / node_projection_registration entries (exempting, not editing the SQL, is the canonical fix for this class per that precedent). Verified locally: _is_legacy_default_schema_sql_path() now returns True for this path; tests/ci/test_application_database_sql_gate.py + tests/ci/test_application_database_sql_authority_isolation.py + tests/unit/validation/test_application_database_sql_enforcement_regressions.py -- 98 passed. 2. Contract Sync Gate (Wave C, OMN-8915): a prior commit on this branch annotated node_setup_validate_effect's health-check probe URL with a url-authority-ok marker (no behavior change) without touching the node's contract.yaml. Bumps contract_version/node_version to 1.0.2 with an honest changelog entry describing the annotation. Verified locally by re-running scripts/validate-pr-contract-sync.sh against the actual changed-file set including this touch: "OK: contract-sync gate passed." Evidence-Ticket: OMN-15717
…un-level CI infra flakiness Multiple unrelated failure signatures across separate workflow runs on the same push: Arch Invariants (self-hosted runner lost communication with the server), reason-graph (Checkout code step exceeded 5m0s), PostgreSQL 16 Fresh + Legacy Fixture (application migration declaration missing — different failure signature than the PRIOR push's 'must be owner of table llm_cost_aggregates', i.e. non-reproducible across two different heads of the same PR, not a stable defect). Fleet health checked live before retriggering: 65 omninode-runner containers healthy on .201, all 4 redpanda brokers healthy, disk 273G free (93% used, not critical), Set-up-job steps succeeded on the affected runs (failures were mid-job, not at Set-up). No source change.
…fixture ledger Root-caused the "PostgreSQL 16 Fresh + Legacy Fixture" (OMN-15422) CI failure, reproduced across three different pushes with overlapping symptoms (ERROR: must be owner of table llm_cost_aggregates; FATAL: application migration declaration missing: legacy-node-migrations.tsv). This branch's earlier "adopt historical stability ledger rows" commit taught scripts/run-forward-migrations.sh's validate_application_migration_manifest() to require a fourth ledger file, LEGACY_NODE_MIGRATION_DECLARATIONS (legacy-node-migrations.tsv), unconditionally -- it added the real docker/migrations/forward/_ledger/legacy-node-migrations.tsv (populated, 2 rows) but never touched the legacy-RDS fixture's OWN synthetic ledger corpus at docker/legacy-rds-fixture/ledger-control/forward/_ledger/, which the fixture's Dockerfile COPYs wholesale (`COPY docker/legacy-rds-fixture/ledger-control/ /opt/omn15422/ledger-control/`) and runs the SAME script against. The fixture corpus had no legacy-node-migrations.tsv at all, so `[ -f "$manifest_file" ]` failed FATAL on every invocation of the fixture inside the container -- 100% reproducible, not flaky; the differing error text across pushes was two DIFFERENT fixture cases within the same job hitting the same missing-file FATAL at different points in their own sequences. Fix: add an empty legacy-node-migrations.tsv to the fixture's ledger corpus, matching the existing empty application-migration-blocks.tsv sibling in the same directory (the fixture legitimately declares zero historical/legacy node migrations). Verified locally: the file-existence check passes; the awk shape/duplicate validators exit 0 against an empty file (same as the pre-existing empty sibling); the historical-declaration read loop completes with zero iterations, matching the pre-existing pattern. Evidence-Ticket: OMN-15717
… red
tests/ci/test_node_migration_shape_reconciliation.py failed on the PR's
restored node_pr_review_bot/001 vendored SQL: its CREATE TABLE IF NOT EXISTS
declares 7 columns with no ALTER TABLE ADD COLUMN IF NOT EXISTS
reconciliation.
Editing the SQL to add reconciliation statements was considered and
rejected: it would change the file's content sha256
(63e2646a7f8767fad9ec969b224982e00b83aacb665107ab5b103964d5616e00), which is
bound both by application-migrations.tsv and by dev's already-applied
omnidash_analytics.platform_catalog.schema_migrations row — a changed
checksum makes scripts/run-forward-migrations.sh's migration_is_applied()
FATAL ("conflicting migration checksum") on that lane.
Fencing via docker/migrations/forward/fenced-node-migrations.yaml is safe,
not merely convenient: read-only .201 probes (2026-08-10) confirm this
migration is already applied on all four live lanes (dev via the
content_sha256 row above; stability-test/judge/prod via legacy
'applied-by-runner' rows dated 2026-06-10/06-11), and review_bot_bypass_log's
live columns already match the 7 declared columns exactly everywhere, so
there is no drift to reconcile on any lane that exists today. Residual,
accepted risk (same shape as the existing delegation entries): a genuinely
new future lane would skip creating this table until an operator adds a
lane release.
tests/scripts/test_node_migration_fence_parity.py pins the manifest content
exactly and in order, and separately checks the opt-in cross-repo k8s
effective-fence relationship — both updated to account for the 8th id.
OMN-15717
jonahgabriel
deleted the
jonah/omn-15717-migration-stream-domain-declaration
branch
August 10, 2026 11:14
jonahgabriel
added a commit
that referenced
this pull request
Aug 10, 2026
… (post-#2678 merge conflict) Rebasing onto origin/dev (now including infra#2678/OMN-15717, which merged concurrently) rewrote the guard-commit hash a second time (7a957a0 -> 90cd78a, same author/date/message/diff, confirmed via git show --stat). Also closes a second cross-PR seam gap #2678 introduced: validate_application_migration_manifest() in run-forward-migrations.sh now unconditionally requires a fourth manifest file, _ledger/legacy-node-migrations.tsv, that this test file's _write_application_ledger_contract() fixture helper didn't know to write. An empty file is valid (mirrors application-migration-blocks.tsv / cloud-migration-aliases.tsv: the awk per-record validators never fire on zero input lines). Verified: all 40 tests in this file pass.
jonahgabriel
added a commit
that referenced
this pull request
Aug 10, 2026
…rations (#2666) * fix(OMN-15336): refuse unclassified FORCE ROW LEVEL SECURITY node migrations Item-4 mechanism, not another manual sweep. The proof stage found no runner (compose or k8s) and no CI check inspects a node migration's SQL text before applying it -- the fence is a closed id-allowlist, so a FORCE ROW LEVEL SECURITY migration nobody remembers to add to it applies silently. That is exactly how node_projection_registration/0002 (since fenced by OMN-15343/OMN-15379/OMN-15349), node_projection_delegation_inference_response/0003, and node_projection_savings/081 all shipped ungated and applied unattended on the .201 dev lane. - scripts/run-forward-migrations.sh: new migration_declares_unclassified_force_rls() guard, called after the already-applied ledger probe (never before -- a guard placed earlier would retroactively FATAL every future run of a lane where an unclassified id already applied, e.g. .201 dev's 0003/081) and only for ids absent from the fence manifest entirely (an already-fenced id, released or not, already went through operator review). Comment-blind (`--` stripped before matching) and excludes `NO FORCE ROW LEVEL SECURITY` so a future FORCE-strip migration is never blocked by the guard it exists to route around. Single-sourced against the same docker/migrations/forward/fenced-node-migrations.yaml both runners already read (OMN-15349) -- no second fence list. - fenced-node-migrations.yaml: adds node_projection_delegation_inference_response/0003. Contract-declared TENANT domain (db_io.schema=tenant, confirmed live against omnimarket's contract.yaml), so unlike node_service_registry this is not a domain misclassification -- it is held for the same reason as the OMN-14974/OMN-15313 delegation quartet: OMN-15301 found the projection writer never sets app.tenant_id per connection, so an un-gated FORCE apply reproduces the identical false-clean write-lockout hazard on a live, actively-written table. Single-sourcing means this also extends the k8s Job's effective fence with zero k8s-side edit. - tests/scripts/test_node_migration_fence_parity.py: RED control (test_unclassified_force_rls_migration_is_refused) plus its own RED control (test_guard_free_runner_applies_the_unclassified_migration, proving the refusal isn't vacuous), four static structural assertions, and EXPECTED_FENCE/effective-k8s-fence updates for the new 8th id. All 30 tests in the file pass locally (7 integration against an ephemeral Postgres, 23 static). node_projection_savings/081 (savings_estimates) and the node_service_registry FORCE-strip disposition are intentionally NOT in this PR -- separate tickets/PRs, see OMN-15336 comment. OMN-15336 item 4 (required-fix #4, restated in operator ruling a3a1fd18 2026-07-29): "0003/081/0002 were never in the fence list on any runner... That gap is untouched by this ruling." Evidence: uv run pytest tests/scripts/test_node_migration_fence_parity.py -q -> 30 passed, 1 skipped (opt-in cross-repo check) in 32.67s pre-commit run --files <3 changed files> -> all hooks Passed * fix(OMN-15336): repair the FORCE-RLS guard's trigger condition (D1) The unclassified-FORCE-RLS guard (bbac520) refuses ANY FORCE-enabling node migration absent from the operator fence, with no notion of "already part of the tree." The vendored tree carries 13 FORCE-enabling node migrations; the fence classifies only 4. The other 9 were ordinary, already-shipped migrations that had been applying on every warm lane since before the guard existed -- but the guard could not distinguish them from a brand-new, unreviewed one. Reproduced live (Opus verdict D1): shipped runner against a virgin PG16 -> exit 1, FATAL at node:node_canary_score_reducer:0002 (first of the 9 in sort order), 1 node migration applied, 87 withheld. A cold lane bring-up (CI, a fresh compose volume, a new .201 lane) could never converge. The PR's own CI was red consistently with this. Fix: option (a), BASELINE SNAPSHOT. New docker/migrations/forward/grandfathered-force-rls-migrations.yaml is a frozen, committed snapshot (NOT a rolling allowlist) of exactly the 9 pre-existing FORCE-enabling ids, each verified via `git show bbac520~1:<path>` to have existed in the tree before the guard could ever have fired for it. The guard's call site now requires BOTH `! is_fenced_node_migration` AND `! is_grandfathered_force_rls_migration` before FATALing -- a genuinely new unfenced FORCE-RLS migration still refuses (RED control unchanged), and the 9 established ones apply normally. Why (a) over (b)/(c): (b) CI-time-only would leave the runtime guard FATALing on every cold lane bring-up until a human notices and reverts it -- worse than the defect it was meant to fix, and the acceptance bar (a virgin PG16 run reaching sentinel HEALTHY) can only be met at the runner itself. scripts/ci/prove_application_database_domain_enforcement.py, floated as a smaller CI-side fix, turns out not to fit: it behaviorally proves RLS enforcement against its own fixed synthetic fixture schema (tenant.events/tenant.tenants/...), not migration-file-level fence classification -- wiring it would not have caught this defect and is out of scope here. The ratchet clause of (a) ("CI check that the grandfather list cannot grow without review") is satisfied by test_grandfather_manifest_pins_the_snapshot_baseline, a real pytest assertion in the same file CI already runs unconditionally in the `not slow/chaos/kafka/performance` pytest step -- no new CI YAML wiring needed, consistent with "detection tools not wired as gates are advisory": this one already is one. Tests added (tests/scripts/test_node_migration_fence_parity.py): - 9 static/structural: manifest content pin (the ratchet), shell/YAML parse parity, every id names a real vendored file, every id genuinely declares FORCE ROW LEVEL SECURITY (guards against padding the list), every id verified to predate the guard commit via git, fence/grandfather disjoint, guard call site wired correctly (both conditions negated and ANDed). - 2 live integration proofs against the REAL committed docker/migrations/forward tree (not a synthetic stand-in), on a genuinely virgin database (new `virgin_pg_target`/`virgin_node_db` fixtures -- the shared OMN-15291 `pg_target` pre-seeds a minimal db_metadata specifically for its own lock-race tests, which collides with the real 029_create_db_metadata.sql and would have made this proof fail for an unrelated fixture-mismatch reason): - test_virgin_database_applies_the_full_real_vendored_tree: exit 0, no FATAL, sentinel HEALTHY, and capability_scores actually carries relforcerowsecurity=true (proves real DDL ran, not a silent skip). - test_virgin_database_still_refuses_a_new_unfenced_force_rls_migration: a genuinely new, unclassified FORCE-RLS migration layered onto a copy of the real tree is still refused, exit != 0, FATAL naming it, nothing applied. tests/scripts/test_forward_migration_advisory_lock.py: two fixtures (`migrations_dir`, `two_migrations_dir`) updated to also write an empty grandfathered-force-rls-migrations.yaml, matching the runner's new unconditional requirement for that file (same discipline OMN-15349 already established for the fence manifest). Evidence: - Manual reproduction of D1 against postgres:16-alpine (pre-fix): exit 1, FATAL at node_canary_score_reducer/0002, 1 node applied / 87 withheld. - Same tree, post-fix: exit 0, "87 node applied, 8 node skipped", "Sentinel set. Migration gate will report HEALTHY.", capability_scores.relforcerowsecurity=t. - RED control (new synthetic unfenced FORCE-RLS migration layered onto the real tree): exit 1, FATAL naming the new migration, nothing applied. - uv run pytest tests/scripts/test_node_migration_fence_parity.py -> 40 passed, 1 skipped (opt-in cross-repo check, pre-existing) against postgres:16-alpine via MIGRATION_LOCK_TEST_HOST. - uv run pytest tests/scripts/test_forward_migration_advisory_lock.py -> 13 passed against the same target. - pre-commit run --files <4 changed files> -> all hooks Passed. - mypy --strict on both touched test files -> Success, no issues. OMN-15336. Supersedes the broken-guard description on PR #2666 (item 4). * fix(OMN-15336): make the FORCE-RLS grandfather ratchet CI-selector-reachable The unclassified-FORCE-RLS guard's grandfather-laundering ratchet (tests/scripts/test_node_migration_fence_parity.py) lives outside src/, scripts/, and tests/, so a changed grandfather manifest, a new FORCE-RLS .sql, or its _ledger row produced NO selection under the change-aware selector (ENABLE_SMART_TESTS=true) and fell through to the conservative tests/unit/ fallback, which the ratchet is not under. Proven before this fix: `detect_test_paths` over the realistic breach set (grandfather YAML + new .sql + ledger row) returned selected_paths=["tests/unit/"]. Fix: map docker/migrations/forward/ -> tests/scripts/ in scripts/ci/detect_test_paths.py's `_resolve()`, deliberately as a plain prefix branch rather than a COLLOCATED_TEST_ROOTS entry -- tests/scripts/ is already collected via the plain "tests" testpaths entry, so adding it to COLLOCATED_TEST_ROOTS would trip check_collocated_selector_coverage's parity assertion (scripts/validation/validate_test_root_collection.py), which is scoped to roots requiring their own testpaths entry. Also deliberately scoped to tests/scripts/ only, not the full SCRIPTS_TEST_PREFIXES pair (tests/scripts/ + tests/unit/scripts/) -- the ratchet lives in tests/scripts/ alone, and this keeps the added footprint to one directory (58 files) rather than two (~125 files). Over-selection: 44/60 (73%) of recent commits touching docker/migrations/forward/ do not also touch scripts/, so those PRs will now additionally run tests/scripts/'s 58 test files, most of which are migration/deploy-adjacent (test_forward_migration_advisory_lock, test_check_deployed_migration_tree_sync, test_run_migrations, test_check_migration_required, validation/test_application_migration_manifest) but some of which are unrelated (keycloak seeding, dockerfile pin checks). Accepted per the selector's own documented risk posture ("over-selection here is safe; under-selection is the OMN-15378 false-green class"). Proof: - Before: `detect_test_paths` over breach set -> selected_paths=["tests/unit/"] - After: same input -> selected_paths=["tests/scripts/"] - test_grandfather_manifest_pins_the_snapshot_baseline: RED on a seeded 10th grandfather id, GREEN on the unmodified manifest - validate_test_root_collection.py: OK (parity guard unaffected) - New unit test: test_migration_tree_change_selects_the_fence_parity_ratchet OMN-15336 * fix(OMN-15336): make the migration-tree selector branch additive, not a swap Opus adversarial review of fbcf008 confirmed the ratchet-reachability fix was itself an under-selection defect. compute_selection()'s conservative fallback (`if not selected: selected = ["tests/unit/"]`) only fires when `_resolve()` returns nothing at all. Giving docker/migrations/forward/ changes their own non-empty selection (tests/scripts/, added in fbcf008) silently SUPPRESSED that fallback for the whole diff -- an ordinary migration change (new .sql + ledger row, no grandfather YAML) went from selecting the entire tests/unit/ tree (23209 tests) to selecting tests/scripts/ alone (577 tests), dropping tests/unit/migrations/, tests/unit/topology/, test_schema_fingerprint.py, test_db_ownership.py, and test_adversarial_fingerprint_drift.py -- all of which genuinely exercise migration/ledger changes, unlike the scripts/ mapping this branch was modeled on (where tests/unit/ never covered scripts/ code, so that swap was a real narrowing-to-equivalent, not a regression). Fix: docker/migrations/forward/ changes now select BOTH tests/scripts/ (the fence-parity ratchet) AND tests/unit/ (the pre-existing coverage), restoring the coverage this class of change always had via the fallback while keeping the ratchet reachable. Flagged the fallback-suppression pattern itself as structural in the MIGRATION_TREE_PREFIX comment: any future prefix branch added to `_resolve()` must check whether the blanket tests/unit/ fallback carried real coverage for that path class before assuming a narrower, targeted selection is safe to swap in. Proof (breach set / ordinary migration / control, before -> after): - Breach (grandfather YAML + new .sql + ledger row): before: ["tests/scripts/"] (577 tests) after: ["tests/scripts/", "tests/unit/"] (577 + 23209 tests) - Ordinary migration (new .sql + ledger row, no YAML): before: ["tests/scripts/"] (577 tests) after: ["tests/scripts/", "tests/unit/"] (577 + 23209 tests) - Control (src/omnibase_infra/utils/ change, non-migration): before/after: identical 15-path tests/unit/<module>/ selection -- unaffected - Ratchet re-verified non-vacuous: seeding a 10th grandfather id without updating EXPECTED_GRANDFATHER -> test_grandfather_manifest_pins_the_snapshot_baseline FAILS; manifest restored byte-clean (sha256 63a6c594c2... unchanged), full fence-parity suite re-run clean after restore: 40 passed, 1 skipped. - validate_test_root_collection.py: OK (parity guard unaffected -- tests/unit/ addition is a plain selected-path, not a COLLOCATED_TEST_ROOTS entry) - tests/unit/scripts/ci/test_detect_test_paths.py: 55 passed (existing breach test updated to assert tests/unit/ IS selected; new test_ordinary_migration_change_selects_both_ratchet_and_unit_fallback locks in the no-YAML case separately) OMN-15336 * fix(OMN-15336): supply the FORCE-RLS grandfather manifest in 2 stale runner test fixtures Discovered while pushing the migration-tree selector fix: docker/migrations/ forward/ now selects the full tests/unit/ tree, which reached tests/unit/migrations/ for the first time locally and surfaced a real, previously-undetected regression from this same PR's earlier commits (bbac520, 87b2a2b -- OMN-15336 item 4 repair, D1). Those commits made scripts/run-forward-migrations.sh unconditionally require grandfathered-force-rls-migrations.yaml under MIGRATIONS_DIR, same as the existing fence-manifest requirement, but only updated the fixtures in tests/scripts/test_node_migration_fence_parity.py (which already has a _write_grandfather_manifest helper). Two sibling fixtures in tests/unit/migrations/ that build their own minimal MIGRATIONS_DIR trees were never updated, so the runner FATALed with "FORCE-RLS grandfather manifest not found" before ever reaching the behavior each test targets (the postgres-wait retry limit, and malformed create-database-directive rejection) -- both tests were asserting on empty stdout/stderr non-matches rather than actually exercising their target code path. This escaped detection because the change-aware selector, before today's MIGRATION_TREE_PREFIX fix, never mapped a scripts/-tree change to tests/unit/migrations/ -- direct, live evidence of the under-selection failure mode the parent fix in this PR addresses. Fix: write an empty grandfathered-force-rls-migrations.yaml alongside the existing fenced-node-migrations.yaml in both fixtures, matching the established minimal-empty-list convention. Proof: tests/unit/migrations/test_migration_gate_vacuity_fix.py + tests/unit/migrations/test_node_migration_discovery.py: 44 passed (was 2 failed before this commit). tests/scripts/test_node_migration_fence_parity.py: 40 passed, 1 skipped (unaffected). OMN-15336 * fix(OMN-15336): copy force-rls manifest into fixture proof * fix(OMN-15336): repoint GUARD_INTRODUCTION_COMMIT at post-rebase guard-commit hash Rebasing this branch onto origin/dev rewrote every commit's hash, including the guard-introduction commit that GUARD_INTRODUCTION_COMMIT and the grandfather manifest's header comments pin by literal SHA (bbac520 -> 7a957a0, identical author/date/message/diff, only the parent-derived hash changed). The old hash is unreachable from any pushed ref after the force-push, so test_grandfathered_ids_predate_the_guard_commit's `git show <GUARD_INTRODUCTION_COMMIT>~1:<path>` failed closed on a fresh CI checkout (observed: infra CI run 31379592234, job 93428167987, FAILED on the first grandfathered id in iteration order). Repoints both the test constant and the two matching yaml header comments at the new hash; verified locally that `git show 7a957a0~1:<path>` resolves for the previously-failing id and all 8 grandfather-manifest tests pass. * fix(OMN-15336): repoint GUARD_INTRODUCTION_COMMIT after second rebase (post-#2678 merge conflict) Rebasing onto origin/dev (now including infra#2678/OMN-15717, which merged concurrently) rewrote the guard-commit hash a second time (7a957a0 -> 90cd78a, same author/date/message/diff, confirmed via git show --stat). Also closes a second cross-PR seam gap #2678 introduced: validate_application_migration_manifest() in run-forward-migrations.sh now unconditionally requires a fourth manifest file, _ledger/legacy-node-migrations.tsv, that this test file's _write_application_ledger_contract() fixture helper didn't know to write. An empty file is valid (mirrors application-migration-blocks.tsv / cloud-migration-aliases.tsv: the awk per-record validators never fire on zero input lines). Verified: all 40 tests in this file pass.
jonahgabriel
added a commit
that referenced
this pull request
Aug 10, 2026
…ions.tsv
Unrelated pre-existing breakage on dev tip, hit by this PR's pre-push
impacted-test selector: run-forward-migrations.sh's
validate_application_migration_manifest() (OMN-15717,
scripts/run-forward-migrations.sh:100/474) FATALs
("application migration declaration missing") if
_ledger/legacy-node-migrations.tsv does not exist at all -- empty is fine,
absent is not. _write_application_ledger_contract() in
tests/scripts/test_node_migration_fence_parity.py builds a synthetic
_ledger/ dir for 5 live-Postgres integration proofs that shell out to the
real runner, and was never updated when OMN-15717 added that requirement
(#2678, merged just ahead of this PR). All 5 tests were failing closed on a
missing file unrelated to the fence behavior each one actually tests.
Confirmed pre-existing: reproduces identically at OMN-15717's own merge
commit (5b4db96), before this PR's changes.
One-line fix, mirroring the already-correct sibling fixture in
tests/unit/scripts/validation/test_application_migration_manifest.py's
_minimal_fixture(). 24/24 pass (1 opt-in skip, unrelated).
OMN-15819
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
1 participant
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_pr_review_bot:001_create_review_bot_bypass_log.sqlwas vendored intodocker/migrations/forward/nodes/(applied historically to a live database, later removed as "stale" byscripts/sync-node-migrations.shonce omnimarket deleted the shell node in the OMN-13212 canonical rebuild) without ever gaining a row inapplication-migrations.tsv. This blocked every workspace-moderefresh_stability_lane.shrun at bootstrap.sql:673 (ERROR: unknown migration stream/domain: adopted node version node:node_pr_review_bot:001_create_review_bot_bypass_log.sql has no checked-in declaration).node:node_pr_review_bot/ omninode_internal per ruling R-q (a review-bot bypass audit log is orchestration bookkeeping, not tenant workload data — consistent with sibling governance nodesnode_pr_lifecycle_state_reducer/node_pr_merged_projection/node_merge_state_projection, allomninode_internal).scripts/sync-node-migrations.sh: a vendored file with a checked-in manifest declaration is preserved applied history (OMN-15695 ruling), not drift, even after its omnimarket source is deleted — otherwise the node-migration-sync gate would strip the file we just declared right back out on the next sync.scripts/validation/validate_application_migration_manifest.pyalready implements the exact "declared set matches the vendored tree" check (plus checksum/duplicate/shape validation) but was wired to nothing — no CI job, no pre-commit hook, only its own test file called it directly. Wired it as a new CI job (node-migration-declaration-check, registered inci_summary_gate.STRICT_GATE_JOBSso it's fail-closed under dev's sole requiredCI Summarycontext) and a pre-commit hook, rather than writing a new script.omnimarket@cedd2431and drives the real validator against them with zero declarations, reproducing the exact pre-fix tree shape (required —Incident-replay coveragepre-commit hook DEFAULT-DENYs a newly-wired guard with no case).RED/GREEN proof
RED reproduced verbatim against a real ephemeral Postgres (not a paraphrase — this is the actual production error):
(
tests/integration/migrations/test_application_migration_ledger_omn15413.py::test_pr_review_bot_bypass_log_adopts_cleanly_omn15717, verified RED against the tree with the manifest row reverted, GREEN with it present.)Declaration gate RED→GREEN:
scripts/validation/validate_application_migration_manifest.pyraisedManifestError: migration declaration set differs from the vendored node tree: missing=[...]before the TSV row was added, passes after.Evidence
Evidence-Source: OCC#6305
Evidence-Ticket: OMN-15717
part of an OMN-15717 fleet-blocker reland lane. Rebased onto a then-current origin/dev (afbba99) and
force-pushed (head bb7762c/5d4061c9c -> 3eaf3c0); governed pre-push selector escalated to the full
tests/unit/suite (23223 passed / 0 failed / 40 skipped) and the deploy-scope-dod pre-push mirror(OMN-14681) both re-verified green against the new head. OCC evidence chain re-bound twice across this
reland: OCC#6188 (merged) added the deploy-scope DoD probe the OMN-14681 gate required (originally
absent — the omnibase_infra#2319 gap) and a loose self-bind; OCC#6305 (merged) re-bound active evidence
to this PR's rebased head after the force-push, since #6188 had already merged pinned to the pre-rebase
head (append-only successor, not an edit to the merged companion).
uv run pytest tests/unit/scripts/validation/test_application_migration_manifest.py tests/unit/migrations/ tests/integration/migrations/test_application_migration_ledger_omn15413.py tests/ci/test_ci_summary_gate.py— 144 passedtests/unit/suite (shared-module blast radius: ci.yml, pre-commit config, sync script): 23171 passed / 0 failed / 40 skippedpre-commit run --all-fileson the changed set: all hooks green, includingIncident-replay coverage (OMN-15547)andONEX Node Migration Vendor Sync CheckTest plan
scripts/validation/validate_application_migration_manifest.pyCLI green against the live treescripts/sync-node-migrations.sh --checkgreen against live omnimarket (no false DRIFT on the legacy-declared file, and a synthetic undeclared/deleted-upstream file is still correctly flagged stale)scripts/ci/check_incident_replay_coverage.pygreen (new guard has a real replay case)pre-commit run --all-fileson changed files greentests/unit/escalation) greenNo merge performed (never merges PRs). CI started, watch via
gh pr checks.Summary by CodeRabbit
New Features
Bug Fixes
Documentation