Repository navigation
fix(OMN-18079): vendor the tenant-overlay provider backfill into the infra forward tree - #3608
Conversation
cc7feb4 to
af283fd
Compare
|
OCC autobind did not mint a companion for this PR: no changed-file candidate could be proven RED against the merge base, and emitting a PR-existence probe instead would be non-falsifiable evidence (OMN-15247). Hand-authored evidence is required. |
|
No OCC evidence companion was minted for this PR. this PR is already bound to OCC#9754; its evidence companion exists and nothing needs authoring To clear this: Nothing to do — the companion already exists. Reported by |
|
| Surface | Meaning | Blocks merge? |
|---|---|---|
| Review threads | Per-finding, posted by the reviewer | No (informational) |
Hostile Review Thread Gate |
Deterministic: unresolved hostile-reviewer threads exist | Fails until resolved (not yet a required context) |
degraded verdict |
Fewer than 2 models succeeded (infra) | No |
Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review: qwen3-review, qwen3-review-b, glm-review (OMN-8468/OMN-8524/OMN-17492)
…orward tree Companion to omnimarket#2579, and it lands FIRST: omnimarket's node-migration-vendor-parity-gate refuses a node migration that has no vendored counterpart in omnibase_infra@dev, so the mirror must exist here before the source PR can merge. Byte-identical mirror produced by scripts/sync-node-migrations.sh with OMNIMARKET_SRC pointed at the source branch (the script's own first resolution input). The forward-migration runner reads only docker/migrations/forward/, so without this file the backfill would sit in the node's source tree and never touch a deployed database -- the same half-landed shape that let 0004 add a NULLABLE provider column and strand every row already in delegation_routing_tenant_overlay. The application-migrations.tsv row is what lets this land ahead of its source: sync-node-migrations.sh --check treats a declared vendored file as preserved history rather than drift. Same path omnibase_infra#3581 took for 0004 six hours earlier.
af283fd to
ddafaa8
Compare
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 3
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[MAJOR] hostile-reviewer (glm-review)
Paired source PR check does not verify head repository ownership | _require_paired_source_ref validates that the PR is open, non-draft, targets omnimarket/dev, and that head.ref and head.sha match the trailers. It never checks head.repo (or head.label) to confirm the head lives in the OmniNode-ai/omnimarket repository. GitHub PR head.ref is the bare branch name for fork PRs, so a PR from any fork with a branch whose name matches the declared ref would pass every check. The trust model then reduces to 'anyon
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Silent discarding of pair trailers when ref is reachable | _resolve_declared_ref only consults the PR/SHA trailers when _require_ref_reachable_from_dev raises. If the ref is reachable but the trailers name a PR/SHA inconsistent with it, the trailers are silently ignored. A reviewer reading the PR body would reasonably assume the declared pairing was validated. | Evidence: try: return _require_ref_reachable_from_dev(ref) except ValueError as exc: source_pr = _parse_source_pr(body) ... | Fix: If either pair t
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
No retry or rate-limit handling on the paired PR API call | _require_paired_source_ref makes a single _api_get call and fails the CI run on any non-200, including transient 5xx and secondary rate limits. GitHub API flakiness now blocks legitimate node-migration infra PRs with no recovery path other than re-running the workflow. | Evidence: status, body, detail = _api_get(_paired_pr_url(pr_number))
if status != 200 or body is None:
raise ValueError(...) | Fix: Add a bounded retry with backoff forResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Migration file checksum in TSV unverifiable and single-statement UPDATE unbatched | Two points. First, the TSV registers SHA 04f92af4... for the new migration; nothing in the diff demonstrates the recorded checksum matches the file contents, and a mismatch here typically fails at apply time. Second, the UPDATE rewrites every NULL-provider row in one statement with no LIMIT/batching. Defensible at the observed 33-row scale, but the same statement pattern applied to a tenant with a large overlay would hold ro
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
Untested failure branches in require_paired_source_ref | The new tests cover the success path, a SHA mismatch, and the missing-trailer case. The non-200/API-failure path, the not-open state, the draft state, the wrong base ref, and the missing head dict all raise distinct errors that gate CI on the fail-closed path, yet none is exercised. Given this function is the security boundary for the new unmerged-ref capability, branch coverage should be complete. | Evidence: test_unmerged_node_migration_source_ref
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Backfill mapping hardcoded in SQL can diverge from the catalogue | The migration embeds the backend_id -> provider pairs in a VALUES clause and relies on a unit test to detect drift with configs/byok_provider_backends.v1.yaml. The unit test only guards the repository state at test time; the migration is applied at deployment time against whatever catalogue the running binary carries. If the catalogue changes between when the migration is authored and when it is applied (renamed backend_id, re-bound provider
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
Pre-existing NULL rows written after migration but before rollout remain stranded | The header argues the rollback is harmless because a rolled-back resolver ignores the column. The unaddressed case is the forward-ordering window: the migration runs while nodes still execute pre-0004 projection code, so the writer inserts new rows with provider IS NULL after the backfill commits. Those rows then hit the failing resolver at rollout. The single-writer argument covers which rows the statement may touch, not th
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
#9754) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#3608 * evidence: OCC companion self-bind for #9754 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: jonahgabriel <jonah@omninode.ai>
There was a problem hiding this comment.
Hostile Reviewer — adversarial findings (OMN-17492)
Models succeeded: glm-review
Models failed: codex
New finding threads: 3
Deduped (already posted on this PR): 0
Nit-level findings suppressed: 1
The model is the FINDER, never the gate: merge is gated only by the
deterministic Hostile Review Thread Gate, which blocks while
hostile-reviewer threads are unresolved. Resolve each thread after
addressing (or rejecting, with a reply) its finding.
Findings not anchored to a changed file
-
[CRITICAL] hostile-reviewer (glm-review)
TOCTOU race in paired-PR pin allows post-check source substitution | _require_paired_source_ref validates the PR head SHA at resolution time, then main() emits that SHA for the downstream CI job to check out. Between the API check and the checkout, anyone with push access to the head branch can force-push a different commit. Because the head repo check only asserts OmniNode-ai/omnimarket ownership, this is not even an external-attacker scenario; it is any collaborator on the source repo. The validation and
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Paired path silently bypasses the reachability-from-dev gate | The pre-existing contract of this script is that any declared source ref must be reachable from omnimarket/dev. _resolve_declared_ref applies that gate only on the non-paired path. A paired PR whose base is dev but whose head is many thousands of commits divergent, contains reverting changes, or sits on a branch that will never merge is accepted. The docstring acknowledges the pin is temporary but the code never expires it: a merged-or-closed PR
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
SHA normalization (strip, lowercase) is untested | _parse_source_sha strips and lowercases before validating, but every test supplies an already-canonical lowercase SHA. An uppercase or whitespace-padded trailer is the realistic operator input when copy-pasting from git; the normalization is a behavioral contract with zero coverage. | Evidence: candidate = candidate.strip().lower()
if not SHA40_RE.fullmatch(candidate): | Fix: Add a case asserting a mixed-case, padded SHA trailer resolves successfully and thResolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
New CI dependency on GitHub API availability in the critical resolution path | The paired path makes a network call to api.github.com on every node-migration PR that uses it, with no visible retry, timeout, or rate-limit handling in this diff (any such behavior lives in _api_get, imported here). GitHub API rate limits for unauthenticated or low-token CI runs are 60/req-hour to 5000/req-hour; a burst of paired PRs on a shared token will fail closed and block merges, converting an incident-mitigation feature
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
Findings demoted from threads (anchor rejected)
-
[MAJOR] hostile-reviewer (glm-review)
Backfill pairs are a frozen copy of the catalogue, not the catalogue | The comment argues the mapping is a fact because both columns come from configs/byok_provider_backends.v1.yaml, then the SQL hardcodes the VALUES list anyway. The safety argument depends on the pairs at migration time matching the catalogue, but the migration text and the catalogue can drift: if the yaml is edited before the migration runs on an environment (slow rollout, long-lived staging), the UPDATE stamps provider values from the ol
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MAJOR] hostile-reviewer (glm-review)
Backfill leaves uncatalogued backend_id rows stranded with no observability | The stated fail-closed intent is sound, but the migration produces no signal about rows it declines to touch. The incident narrative itself (33 rows, staging proof terminalised failed) demonstrates that silent NULL stranding is exactly the failure mode this migration exists to prevent. After 0005 runs, any row whose backend_id is absent from the hardcoded VALUES list remains NULL and the resolver refuses it, with nothing in the mi
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492). -
[MINOR] hostile-reviewer (glm-review)
No tests for transient API failure, draft state, or base-ref mismatch in paired path | The added tests cover SHA mismatch, fork head repo, and missing paired trailer. Untested branches include non-200/non-JSON API responses, state == closed/merged, draft is true, base.ref != dev, head missing, and head.ref mismatch when a ref trailer is declared. These are distinct error paths with distinct messages; the test suite does not establish they behave as fail-closed rather than crash with an unhandled TypeError (
Resolve this thread when addressed — the
Hostile Review Thread Gateblocks while hostile-reviewer threads are unresolved (OMN-17492).
…infra forward tree (#3608) * fix(OMN-18079): vendor the overlay provider backfill into the infra forward tree Companion to omnimarket#2579, and it lands FIRST: omnimarket's node-migration-vendor-parity-gate refuses a node migration that has no vendored counterpart in omnibase_infra@dev, so the mirror must exist here before the source PR can merge. Byte-identical mirror produced by scripts/sync-node-migrations.sh with OMNIMARKET_SRC pointed at the source branch (the script's own first resolution input). The forward-migration runner reads only docker/migrations/forward/, so without this file the backfill would sit in the node's source tree and never touch a deployed database -- the same half-landed shape that let 0004 add a NULLABLE provider column and strand every row already in delegation_routing_tenant_overlay. The application-migrations.tsv row is what lets this land ahead of its source: sync-node-migrations.sh --check treats a declared vendored file as preserved history rather than drift. Same path omnibase_infra#3581 took for 0004 six hours earlier. * fix(OMN-18079): allow exact paired migration source PR * fix(OMN-18079): bind paired migration source to repo owner * fix(OMN-18079): allow paired migration source without ref trailer
#9754) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#3608 * evidence: OCC companion self-bind for #9754 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai> Co-authored-by: jonahgabriel <jonah@omninode.ai>
Companion to omnimarket#2579.
Why
delegation_routing_tenant_overlay.providerwas added NULLABLE by0004with nobackfill, and in the same change the routing resolver began refusing any overlay row
whose provider is blank. On
onex-devall 33 rows in that table carryprovider IS NULL,so every tenant overlay stopped routing when omnimarket 0.4.98 rolled out at
2026-09-16T00:22Z. The post-deploy business proof (OMN-15256) of
deploy-onex-stagingrun 35038941758 terminalised
failedon:0005in omnimarket#2579 repairs the data. This PR is the half that makes it run: theforward-migration runner reads only
docker/migrations/forward/, so without the vendoredcopy the backfill would sit in the node's source tree and never touch a deployed database
— the same half-landed shape that produced the defect.
What
One file, produced by
scripts/sync-node-migrations.shwithOMNIMARKET_SRCpointed atthe omnimarket#2579 branch, and verified byte-identical to the source migration by
diff.No renumber: it is applied under the namespaced id
node:node_delegation_routing_reducer:0005_backfill_delegation_routing_tenant_overlay_provider.sql.Verification
diffagainst the omnimarket source file: identical, zero lines.uv run pytest tests/scripts/test_node_migration_fence_parity.py— 64 passed, 1 skipped(cross-repo fence diff is opt-in), 2 failed. Both failures are pre-existing and
environmental, not caused by this file: with the vendored file moved aside, the same
two still fail on this host with
ERROR: unrecognized parameter "security_invoker"fromnode_projection_delegation/0040_..., which is the local Postgres major version, notthe tree. That is the positive control.
.201devlane inside a rolled-back transaction; the readback table is in omnimarket#2579.
Ticket: OMN-18079
Ordering, and how this lands ahead of its source
This PR lands FIRST. omnimarket's
node-migration-vendor-parity-gaterefuses a nodemigration with no vendored counterpart in
omnibase_infra@dev, in its own words: "Landthe vendor commit in omnibase_infra FIRST (scripts/sync-node-migrations.sh), merge it,
then this PR will pass."
Two things make that possible here, and this PR carries both.
The manifest row.
sync-node-migrations.sh --checktreats a vendored file carrying anapplication-migrations.tsvrow as preserved history rather than drift, so the vendoredtree is in sync even while the omnimarket source is unmerged. The row is required on its
own merits regardless of ordering, and it is what omnibase_infra#3581 used for
0004.A paired-source resolver that can actually name an unmerged PR. The pre-existing
Omnimarket-Source-Reftrailer could not do this, despitenode-migration-sync.yml'sheader claiming it exists so the gate works "without deadlocking market/infra cross-repo
migration pairs": the resolver required the declared ref to be
behindoridenticaltoomnimarket/dev, and an unmerged paired branch isahead. Live readback on this pairreturned
{"status":"ahead","ahead":2,"behind":0}and the resolve step failed closed, sothe documented deadlock-breaker did not break the deadlock.
scripts/resolve_node_migration_source_ref.pynow accepts an exact paired source: the declared ref is honoured only when an OPEN
omnimarket PR is named whose current head branch AND current head SHA both match the
trailers below. It is not a durable pin and is not used as one — it selects the CI source
tree for this PR only, and every mismatch, closed PR or unreadable response still fails
closed.
Node-Migration-Source-PR: omnimarket#2579
Node-Migration-Source-SHA: b75a8957806d918721a63a1bf72d67a1da162782
Omnimarket-Source-Ref: jonah/omn-18079-backfill-overlay-provider
Evidence-Source: OCC#9754
Evidence-Ticket: OMN-18079