Repository navigation
fix(OMN-15395): RF policy must provision, not refuse — managed floor + fail-closed re-raise at every call site - #2546
Conversation
…fusal Remediation of five adversarial-review findings against the first revision of this PR. See the PR body for the finding-by-finding mapping and the live readbacks behind each claim. HIGH (correctness/false-claim) — the refuse-on-undeclared policy made topic provisioning a 100% no-op on MSK. Live readback against the real contracts_root: 0 of 168 production topics resolved, 168 refused; 75 of those have no producing declaration anywhere in this repo (subscribe-only, produced by omniclaude / omnimarket / CLI relays), so no in-repo contract could ever have fixed them. The managed profile now resolves an undeclared RF to MANAGED_MINIMUM_REPLICATION_FACTOR (2) — the MSK broker's own default, and the value the managed-staging namespace catalog already declares — instead of refusing. A declared RF below the floor still aborts the entire pass before any CreateTopics is issued. The eleven contract declarations the first revision deleted are restored at RF2, and the ratchet now asserts they still exist, so "delete every declaration" can no longer satisfy it. HIGH (correctness) — undisclosed hard boot regression on kernel_glue._provision_dlq_topics, the one caller with no try/except. Derived DLQ topics are absent from the contract-derived spec registry, so the no-default policy raised out of build_and_start_core_runtime and refused to start the S6 dispatch loop. Covered by a test that drives the real boot helper. MEDIUM (claim-vs-code) — the fail-closed re-raise the error class docstring promised was never implemented. Added at all four external call sites (handler_wiring interleave, service_kernel warm + auto-create, runtime_host_process), plus a static guard that reads the shipped source so a new best-effort call site cannot silently reintroduce the swallow. LOW (consistency) — ensure_topic_exists' snapshot-config branch now passes the resolver's OUTPUT to NewTopic instead of discarding it. LOW (dead-surface) — refresh_existing_topics (zero callers, zero coverage) deleted; the snapshot lifecycle is documented where the cache is declared. Self-hosted profiles gain an explicit capacity ceiling of 1, which is what makes a contract-declared RF2 landable at all: a single-node broker rejects RF>1 with INVALID_REPLICATION_FACTOR. The ceiling only ever reduces, and a validator forbids a ceiling that would undercut a durability floor.
…F2 declarations tests/ci/test_topic_config_contract_parity.py asserts field-by-field parity between each migrated topic's contract topic_config and its transitional ModelTopicSpec entry in platform_topic_suffixes.py. Restoring the eleven contract replication_factor declarations at RF2 moved one side of that seam, so the nine MIGRATED_TOPICS registry entries now declare the same value, bound to MANAGED_MINIMUM_REPLICATION_FACTOR rather than a second literal 2 — one constant, both sides, so the seam cannot drift by editing only one. Caught by the full unit suite on .200, not by the focused run.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
Comment |
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical findings | No |
blocked |
CRITICAL findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8468/OMN-8524)
#5511) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_infra#2546 * evidence: OCC companion self-bind for #5511 * fix(OMN-15395): repair OCC 5511 evidence bindings --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
… PR author (#2574) OMN-15496 (#2569, merged 2026-07-30T18:14:35Z) made CI Summary assert 17 cross-workflow contexts fail-closed. One of them, "gate / CodeRabbit Thread Check", is structurally absent on Dependabot PRs: cr-thread-gate-caller.yml skips the caller job when github.actor == dependabot[bot], and because the context is the caller-job/reusable-job form, the inner job never materialises so NO check-run is created. Absent burns the 90 min poll deadline and then fails closed against the SOLE required context on infra dev. The OMN-15496 seed measured this context 16/16 present over #2546-#2567 -- a window containing no Dependabot PR, which is how a 16/16 context can still be absent in production. Fixed consumer-side via a declared ACTOR_CONDITIONAL_CONTEXTS registry, not by removing the producer skip: OMN-10276 took the producer route on omnimemory, but omnimemory calls a LOCAL reusable and passes no secrets, whereas this caller invokes omniclaude reusable with secrets: CROSS_REPO_PAT. Dependabot pull_request runs do not receive regular repo secrets, so dropping the skip here risks trading an absent-wedge for a red-wedge. This is an applicability rule, not a bypass: the context stays fail-closed for every author not named in the registry, and a missing --pr-author drops nothing. RED-before/GREEN-after on the real #2522 payload (98 unedited check-run rows): pre-change gate -> PENDING (deadline FAILURE) naming the CR context; post-change with dependabot[bot] -> SUCCESS; post-change with jonahgabriel -> still PENDING. Replaying the whole Dependabot batch #2515-#2522 at merge time: 5/8 blocked before, 2/8 after, and both survivors are genuine URL Authority Gate reds that are correctly preserved -- zero residual blocks attributable to the CR context. Bound in ci.yml in the same change (--pr-author), pinned by a test. Co-authored-by: jonahgabriel <jonahgabriel@users.noreply.github.com>
…TERNAL_CONTEXTS OMN-13873 shipped `Dep Provenance Gate` (dep-provenance-gate.yml) but its own DoD item "required on infra dev/main branch protection" was never fulfilled live: the context reports on every PR (no job-level `if:`) but blocks nothing on dev or main today. Measured per ci_summary_gate.py's own admission rule: 16/16 present, 16/16 green over the last 16 merged omnibase_infra dev PRs (#2646-#2669, 2026-08-04T16:56Z -> 2026-08-07T01:08Z, started_at <= mergedAt). Re-verified against the SAME #2546-#2567 golden-fixture window already pinned by test_ci_summary_gate.py: also 16/16 present, 16/16 green. Both windows agree, so the context is folded into EXPECTED_EXTERNAL_CONTEXTS and the existing fixture rows, closing it fail-closed on dev via the sole required `CI Summary` umbrella (code path per CLAUDE.md rule 10 — no branch-protection API mutation in this PR). Also bundles a pre-existing, unrelated node-migration vendor-sync drift fix (scripts/sync-node-migrations.sh output) that was already red on dev HEAD before this change and blocked the pre-commit gate for this PR (no-pre-existing-excuse policy).
…ERNAL_CONTEXTS (#2730) The "Integration Test Removal Gate" job (OMN-8732) hard-blocks a PR that deletes a tests/integration/*.py file without a replacement and its own workflow header says "No override mechanism" -- but before this change it was absent from all three omnibase_infra dev enforcement surfaces: branch protection (required_status_checks = exactly ["CI Summary"]), EXPECTED_EXTERNAL_CONTEXTS, and MEASURED_NOT_ENFORCED_CONTEXTS. PR #2720 (head ce4e88f) merged 2026-08-11T04:47:58Z with this context reporting `failure`. Measured 16/16 present, 15/16 green over two independent 16-PR windows: the original OMN-15496 seed window 2026-07-29T23:04Z -> 2026-07-30T14:54Z (#2546...#2567, backfilled into omn15496_merge_time_external_check_runs.json -- 16/16 green there) and the current window 2026-08-09T23:11Z -> 2026-08-11T04:47:58Z (#2705...#2720, tests/ci/fixtures/omn15979_merge_time_external_check_runs.json -- 15/16 green). The one red, #2720, is root-caused: its job (id 93668844708) recorded ZERO steps -- a self-hosted-runner dispatch failure -- and the PR's diff touched only .github/workflows/build-and-push-runtime.yml (no tests/integration files), so the gate's own substantive check logic was never at risk of a legitimate red. TDD: TestIntegrationTestRemovalGateExternalContext added RED (verified by stashing the tuple entry and re-running -- 5 failures for the right reasons) before the fix went GREEN. Includes a synthetic-red regression (test_synthetic_red_flips_a_clean_pr_to_failure) proving a red check-run on this context flips CI Summary's verdict, and a live-replay of PR #2720's real payload proving it would now be blocked end-to-end. Ticket: OMN-15979
…TERNAL_CONTEXTS (#2684) * fix(OMN-15737): admit Dep Provenance Gate into CI Summary EXPECTED_EXTERNAL_CONTEXTS OMN-13873 shipped `Dep Provenance Gate` (dep-provenance-gate.yml) but its own DoD item "required on infra dev/main branch protection" was never fulfilled live: the context reports on every PR (no job-level `if:`) but blocks nothing on dev or main today. Measured per ci_summary_gate.py's own admission rule: 16/16 present, 16/16 green over the last 16 merged omnibase_infra dev PRs (#2646-#2669, 2026-08-04T16:56Z -> 2026-08-07T01:08Z, started_at <= mergedAt). Re-verified against the SAME #2546-#2567 golden-fixture window already pinned by test_ci_summary_gate.py: also 16/16 present, 16/16 green. Both windows agree, so the context is folded into EXPECTED_EXTERNAL_CONTEXTS and the existing fixture rows, closing it fail-closed on dev via the sole required `CI Summary` umbrella (code path per CLAUDE.md rule 10 — no branch-protection API mutation in this PR). Also bundles a pre-existing, unrelated node-migration vendor-sync drift fix (scripts/sync-node-migrations.sh output) that was already red on dev HEAD before this change and blocked the pre-commit gate for this PR (no-pre-existing-excuse policy). * fix(OMN-15737): preserve original 1-space indent in fixture json The previous commit's json.dump reformatted the whole fixture file (2-space indent vs the file's original 1-space convention), producing an 8000-line diff noise. Re-dump with indent=1 to match the existing style; diff is now scoped to the actual added rows. * fix(OMN-15737): declare the 2 newly-vendored node migrations in the manifest test_application_migration_manifest.py caught what the bundled vendor-sync fix (previous commit) missed: adding node_canary_score_reducer/0003 and node_projection_registration/0004 to the vendor tree without a matching declaration in docker/migrations/forward/_ledger/application-migrations.tsv left the manifest incomplete (94 declared vs 96 on disk). Domain classification follows the established, already-committed pattern for each node rather than inventing new policy: - node_canary_score_reducer/0003 (capability_scores tenant_id TEXT->UUID): domain=tenant, continuing sibling 0002's tenant domain for the same already-tenant-classified column. - node_projection_registration/0004 (node_service_registry NO FORCE RLS): domain=omninode_internal, matching the file's own inline OMN-15336 item-4 domain corroboration (contract.yaml db_io.schema=omninode_internal, 2026-08-02 operator ruling, OMN-15656 grants-derivation correction). Full impacted suite (scripts/ci/tests, scripts/tests, tests/ci, tests/scripts, tests/unit/scripts) re-run green: 3011 passed, 5 skipped. * fix(OMN-15737): dedupe stale vendor-migration TSV rows introduced by dev rebase The OMN-15732 deadlock-fix rebase auto-merged two intermediate commits' TSV additions for node_canary_score_reducer/0003 and node_projection_registration/0004 with stale checksums, alongside dev's already-correct rows for the same files (dev holds the adjudicated single-file 0003 since 211e81e). Take dev's TSV wholesale -- this PR has no legitimate TSV diff of its own.
… block on infra (#2973) OMN-16876 census item 2. Both checks ran on every omnibase_infra PR and could not block a merge. That is sharper here than in any other repo: `dev` requires exactly ONE status check, "CI Summary" (the OMN-4497 single-umbrella design), so EXPECTED_EXTERNAL_CONTEXTS in scripts/ci/ci_summary_gate.py IS the entire external enforcement surface. A context missing from that tuple has no second surface to fall back on and no branch-protection signal that it is missing. Both entries close a Done ticket that live state contradicts: - OMN-13328 ("receipt-honesty REQUIRED on all repos that produce receipts") -- its OCC contract asserted omnibase_infra, omniclaude and omnimarket "were already flipped". Live readback 2026-08-28: none of the three was enforced on any surface. - OMN-13326 ("contract-validation REQUIRED across all 6 repos") -- genuinely required on omnibase_core, omnibase_compat, omnibase_spi, omniintelligence and omnidash; wired on neither omnibase_infra nor omniclaude. Admission, measured not assumed: - #2955..#2970 (2026-08-28): 16/16 present, 16/16 green for both. - Backfilled BOTH existing merge-time fixtures with real API reads under the fixtures own merge-time filter (started_at <= mergedAt), not synthesised rows: omn15496 (#2546..#2567) and omn15979 (#2705..#2720). The second window independently measured 32/32 present, 32/32 green across both contexts. Three independent windows agree; no freeze-baseline ratchet is indicated. Non-vacuity (OMN-16876 finding 5) -- 16/16 green is also exactly the vacuous pass shape, so each was proven able to FAIL on real input: receipt-honesty gamed receipt (verifier == runner) -> exit 1; real -> 0 contract-validation schema-invalid contract -> exit 1; OMN-10041.yaml -> 0 Both producers declare pull_request AND merge_group and carry no `needs:` and no job-level `if:`, so no upstream failure can skip them; and this layer admits only "success", so a skip fails closed rather than passing. Evidence-Source: OCC#7433
Remediates the five adversarial-review findings against OMN-15395 as landed in
#2543 (squash-merged to
devasb2ca4faa). The first revision is already live ondev, and finding 1 below is a live regression there: against a managed (MSK)cluster the provisioner currently refuses to create every topic. This PR is the
fix, not a follow-up nicety.
Ticket: OMN-15395 —
fix: managed-staging topic provisioner must be contract-driven and reject RF1Finding-by-finding
1. HIGH · correctness/false-claim — refusing everything is not fail-closed, it is off
Verified claim, reproduced before changing anything, against the real
contracts_root(src/omnibase_infra/nodes) withModelTopicProvisioningPolicy.managed():The PR that landed deleted the only eleven
replication_factordeclarations in thetree and added none, so on MSK the provisioner could create nothing at all.
I also measured why it could not simply be fixed by declaring RF everywhere.
Across all 62 contract files:
topic_config)topic_configblockThose 75 appear only in
event_bus.subscribe_topics— they are produced byomniclaude / omnimarket / CLI relays. A literal refuse-on-undeclared policy makes
provisioning a permanent no-op for them with no in-repo contract that could ever
be fixed.
Fix. The managed profile now resolves an undeclared RF to
MANAGED_MINIMUM_REPLICATION_FACTOR(2) instead of refusing.This is not the deleted defect reintroduced. The defect was a module-level constant of
1 that overrode the broker's own RF2 default, environment-blind, everywhere. The new
value is profile-scoped, equal to the MSK broker's own default, and is the same constant
the managed-staging namespace catalog already binds
(
managed_staging_canary_catalog_namespace.yaml→default_replication_factor, wired tothis constant in
model_canary_namespace.py). Acceptance criterion (c) names thedivergence between that catalog's RF2 and the manager's RF1 as the defect; converging
both on one constant is the fix. An RF1 topic is unreachable through this path.
What stays fail-closed: a declared RF below the floor still aborts the entire pass
with zero
CreateTopicsissued. That path is now batch-scoped, not per-topic — theper-topic "refuse and carry on" branch (and the
refusedresult key) is gone, along withthe §"Honest note on a scoping decision" reasoning that justified it on the false premise
that compliant sibling topics existed.
The eleven deleted declarations are restored at RF2, and the ratchet
(
test_no_contract_declares_rf1.py) gained a second assertion: the declarations muststill exist. The previous revision checked only "none below the floor", so deleting
every declaration satisfied it — which is exactly what happened.
2. HIGH · correctness — undisclosed hard boot regression on the DLQ path
kernel_glue._provision_dlq_topics(kernel_glue.py:208) is the one caller with notry/except, and derived DLQ names are absent from the contract-derived specregistry. Verified:
onex.dlq.omnibase-infra.example-event.v1→IN SPEC REGISTRY: False,and
resolve_replication_factor(declared=None)raised. That propagated uncaught throughbuild_and_start_core_runtime(service_kernel.py:3268), so on MSK any DLQ topic notalready on the broker refused to start the S6 dispatch loop.
Resolved by finding 1's default and covered by two tests, one of which drives the real
_provision_dlq_topicshelper rather than a surrogate.3. MEDIUM · claim-vs-code — the fail-closed re-raise was never implemented
error_topic_provisioning.py:14-17claimed the distinct class existed "so theprovisioning call sites can re-raise it past their best-effort
except Exceptionboundaries". It was re-raised only inside
service_topic_manageritself. Implemented atall four external call sites:
runtime/auto_wiring/handler_wiring.py:5107runtime/service_kernel.py:1321runtime/service_kernel.py:1389runtime/runtime_host_process.py:3697Plus a static guard (
test_every_provisioning_call_site_reraises_the_policy_error)that reads the shipped source and fails if any
ensure_topic_exists/ensure_provisioned_topics_existcall sits behind a bareexcept Exceptionwithoutre-raising first — so a new best-effort call site cannot silently reintroduce the
swallow. A docstring promise is not a mechanism.
Note on blast radius, stated rather than buried:
_interleave_contractruns underasyncio.gather(...)withoutreturn_exceptions=True, so this re-raise aborts the bootsubscribe pass. That is the intent for a contract declaring RF1 against MSK, and the
static ratchet in
test_no_contract_declares_rf1.pyprevents such a contract fromlanding.
4. LOW · consistency — resolver output discarded at one creation site
service_topic_manager.py:755calledresolve_replication_factor(...)for its floorside-effect and then passed
config.replication_factortoNewTopic. It now passes theresolver's return value, and records a
ModelTopicSpecfor the readiness path."There is one resolver" only holds if every creation site uses what it returns.
5. LOW · dead-surface —
refresh_existing_topicsDeleted (zero callers, zero coverage). The snapshot lifecycle is now documented where the
cache is declared: re-fetched at the top of every
ensure_provisioned_topics_existpass,fetched lazily once for
ensure_topic_exists, folded forward on each create. The onlystaleness a live instance can observe is an out-of-band deletion, whose cost is a skipped
create the next full pass repairs.
Additional defect found while remediating (not in the review)
Restoring the eleven RF2 contract declarations broke
tests/ci/test_topic_config_contract_parity.py, which asserts field-by-field paritybetween each migrated topic's contract
topic_configand its transitionalModelTopicSpecentry inplatform_topic_suffixes.py. Nine registry entries still saidreplication_factor=None. Both sides now bindMANAGED_MINIMUM_REPLICATION_FACTOR, sothe seam cannot drift by editing one side.
This was caught only by the full unit suite on
.200, not by the focused run — theseam-matching rule earning its keep.
Seam definition — topic-spec fields end to end
The replication factor crosses five boundaries. Field-by-field:
published_events[].topic_configreplication_factor: intModelContractTopicEntryreplication_factor: int | NoneNoneModelTopicSpecreplication_factor: int | NoneNone= contract declared nothing (never 1)ModelTopicProvisioningPolicyminimum/default/capacity_replication_factordefault, or refused if the profile has noneaiokafka.admin.NewTopicreplication_factor: intNone— resolution precedes constructiondescribe_topics→evaluate_topic_readinesslen(partition["replicas"])Nonespec ⇒ no assertionThe same
ModelTopicSpecobject flows creation →_created_specs→ readiness, so afreshly created topic is confirmed against the spec it was created with. Pre-existing
topics are deliberately not spec-gated (the 519 legacy RF1 topics would flip healthy
topics to NOT-READY and block consumer attach); their drift is reported by
ensure_provisioned_topics_existand repaired by the operator-gated WS-M lane.New: self-hosted capacity ceiling
capacity_replication_factoris what makes a contract-declared RF2 landable at all. Asingle-node broker (local Redpanda, CI sandboxes, the
.201lanes) rejectsRF > broker countwithINVALID_REPLICATION_FACTOR, so without a ceiling all eleven restoreddeclarations would fail
CreateTopicson every non-managed lane. The ceiling only everreduces, and a model validator refuses a ceiling below the profile's durability floor —
otherwise a capacity reduction would be a bypass of the whole RF1 rejection, since
reduction happens before the floor check. Managed carries no ceiling.
Acceptance-criteria mapping
DEFAULT_EVENT_TOPIC_REPLICATION_FACTORdeleted; eleven contract declarations at RF2; single resolver. Deviation: an undeclared RF resolves to the managed floor rather than refusing — justified by the 75-orphan-topic readback above and documented in themodel_topic_provisioning_policymodule docstring, not hidden.CreateTopics; re-raised past all four best-effort call sites; static guard keeps it that way.node_remote_agent_invoke_effect/contract.yamlfixed (RF1 → RF2), not exempted.ensure_topic_exists(spec / bare-name / snapshot-config), readiness, canary catalog,managed_staging_topic_checkerall resolve through the one policy; the snapshot-config site now uses the resolver's output (finding 4).CreateTopicsdescribe_topicsper pass; only missing topics created; existing-topic drift reported, never mutated.ONEX_BOOT_UNIVERSE_PROVISIONstays 0dev: RF1 created silently, RF defaulted to 1, ~1,280 blind authorizations. Against revision 1: 0/168 resolvable, DLQ boot refusal, four swallowing call sites, RF2 unprovisionable on single-broker. Enumerated per-test in the test module docstring.Gates — run on
.200(stickybeatz-studio), patch-transfer verifiedPer rule 11a. Edits were authored locally, transferred by git bundle, and verified
identical before any gate ran — a gate on an unedited copy is a vacuous green.
.200HEAD746865732c45107f57ec876b717f035db195af20== local HEAD.shasum -a 256on all 17 changed files, local ==.200, byte-identical.uv run mypy src/omnibase_infra/— Success, 2628 source files.uv run ruff format --check+ruff check src/ tests/— clean.pre-commit— full hook set ran on commit (no--no-verify, no skip tokens). Twofindings were fixed rather than suppressed: a bare
MagicMock()on a transport surface(now
spec=ProtocolEventBusLike/spec=ProtocolDispatchEngine) and a ruff import-sortfix.
-knarrowing:23575 passed, 46 skipped, 4 failed.The 4 remaining failures are pre-existing full-suite env pollution, not this change
test_dependency_materializer::test_from_env,test_kafka_bootstrap_no_localhost_fallback::test_from_env_uses_env_var_when_set, and twotest_model_kafka_producer_config_max_request_size::test_from_env*. Evidence they are notmine:
tests/unit/runtime/alone at this HEAD: 5291 passed, zero failures.(
tests/unit/event_bus/ tests/unit/topics/+ the three victim modules): 886 passed.from_envconfig tests with no relationship to topic provisioning; the*_default_when_unsetcase failing means an earlier test left the var set inos.environ.Confirmed pre-existing by a
dev-baseline run, executed in the same.200worktreeand venv (detached checkout of
b2ca4faa, then restored):The same four, at
dev, without this PR. Per the "second failure of the same check is abug, not a flake" rule this is not being written off as transient — it is a real
cross-directory
os.environpollution bug, filed as OMN-15432 rather than folded inhere, because fixing it means finding and repairing an unrelated test's teardown.
Also filed from this lane: OMN-15433 — two files on
devcarry a2026SPDX yearagainst a validator pinned to
2025, makingpre-commit run --all-filesunconditionallyred on a clean checkout. Neither file is in this diff.
proof_class
receipt-bound(code + executed tests). Live MSK readback belongs to the separate,operator-gated WS-M repair legs, per the ticket's own scope note. No AWS mutation here.
Evidence-Source: OCC#5511
Evidence-Ticket: OMN-15395