Repository navigation
fix(OMN-15395): decide NewTopic policy-resolution at the call site, not the module — AST provenance guard - #2554
Conversation
…ot the module The static guard shipped in #2552 computed admissibility once per FILE: resolves = ("ModelTopicProvisioningPolicy" in text and _POLICY_RESOLVER_RE.search(text)) so every NewTopic(...) in any module that mentions the policy anywhere got a blanket pass unless its RF was an integer literal. Executed against the reconstructed b2ca4fa (#2543) tree it reported only the operator CLI and returned NOTHING for service_topic_manager.py:758's `replication_factor=config.replication_factor` -- that lineage's own defect -- because the module mentions the policy five times elsewhere. That is the same "the guard certified a property it could not see" failure the guard was introduced to remediate, reintroduced in its replacement. Admissibility is now computed from the NewTopic call's own argument expression by AST provenance. A replication factor is admissible only when it traces, via provenance-preserving operations (attribute, subscript, iteration, method call on a resolved receiver, container literal/comprehension, single-arg builtin repackaging), back to a resolve_spec / resolve_specs_for_creation / resolve_replication_factor call. A module-local wrapper qualifies by its BODY (every return resolved), never by its name, so a stub `_resolve_spec` that returns its argument confers nothing. Name lookup is line-ordered per lexical scope, which is what keeps managed_staging_topic_checker admissible: it binds `spec` twice, once from a raw walrus and once from the resolved mapping, and only the nearest preceding binding counts. Also now refused rather than waved through: a literal RF in positional slot 3 (NewTopic('t', 6, 1)), an omitted RF, and a **kwargs splat. Analysis is deliberately conservative: provenance is not tracked through a mutated accumulator. That shape is reported, and the documented remedy is the batch helper resolve_specs_for_creation -- not a loosening. Pinned by a test so it stays a decision rather than a surprise. Test-only change; no src/ or scripts/ behaviour is modified. Ticket: OMN-15395 Evidence-Ticket: OMN-15395
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 56 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 (1)
Comment |
OMN-15395 — remediation round 4
Fixes the MEDIUM adversarial review raised against PR #2552 (merged,
f420d5fa). #2552's branch was already merged, so this lands on a fresh branch offdev(f420d5fa) — the same pattern #2546 → #2550 → #2552 used.Headline: the static guard #2552 added to close the second
CreateTopicspath decided "does this site resolve through the policy?" at module scope. It could not see an unresolved value at a site inside a policy-aware module — which is the exact defect class this lineage keeps shipping.The defect
_raw_create_topics_offenderscomputed, once per FILE:So every
NewTopic(...)in any module that mentions the policy anywhere got a blanket pass unless its RF was an integer literal.service_topic_manager.pymentionsModelTopicProvisioningPolicy5× and calls a resolver 5×, and today carries 3NewTopicsites under that blanket pass — a 4th raw site there would have been invisible.The PR body of #2552 claimed the guard "scans
src/andscripts/for anyNewTopicconstruction that bypasses the policy or hardcodes a literal RF". The "bypasses the policy" arm did not exist as described.Proven by execution, not inferred
Both guards run against the reconstructed
b2ca4faa(#2543) tree (git show b2ca4faa:<path>into a scratch tree):service_topic_manager.py:758at that commit readreplication_factor=config.replication_factor— the raw, unresolved value handed straight toNewTopic, withresolve_replication_factorcalled three lines above and its result discarded. That is this same OMN-15395 lineage's own round-3 defect-3 / finding-4, and the shipped guard returned nothing for it.Site
:555is a conservative flag, disclosed rather than hidden: atb2ca4faa,_resolve_specs_for_creationreturnedtuple(resolved), tuple(sorted(refused))whereresolvedis a mutated accumulator list, which the analysis deliberately does not trace (see "Deliberate conservatism" below). The live tree does not use that shape.The fix
Admissibility is computed from the
NewTopiccall's own argument expression, by AST provenance:resolve_spec/resolve_specs_for_creation/resolve_replication_factortuple(...))returnresolved), never by its nameTopicProvisioner._resolve_spec/_resolve_specs_for_creationpass; a stubdef _resolve_spec(s): return sdoes notmanaged_staging_topic_checkerbindsspectwice — a raw walrus and the resolved mapping — and only the resolved one reachesNewTopicNewTopic('t', 6, 1)is now caught**kwargssplat, is reportedTest-only change. No
src/orscripts/behaviour is modified — the live tree returns[]under the new guard, so all five realNewTopicsites remain admissible.Controls
7 positive (
test_create_topics_guard_sees_a_planted_third_path) — flat literal; unresolved caller-supplied; policy-aware module with a raw site (the one this PR exists for); stub helper named like a resolver; positional literal;**kwargssplat; omitted RF.6 negative (
test_create_topics_guard_accepts_a_policy_resolved_site) — one per live creation shape: directresolve_spec; resolved scalar fromresolve_replication_factor; comprehension over a batch; dict-comprehension +.get()shadowing a raw walrus; module-local wrapper that genuinely delegates;tuple(...)-repackaged batch. Without these, "tighten the guard" degenerates into "flag everything", which is as useless as the blanket pass it replaces.1 pinned limitation (
test_create_topics_guard_is_conservative_about_accumulators) — see below.RED-before evidence (mutation proof, executed)
_raw_create_topics_offendersreverted in place to the #2552 module-scope body, guarding tests re-run:The other 11 failures are the remaining positive/negative controls, which the module-scope predecessor also gets wrong (in both directions). GREEN after: 60 passed in the file, 609 passed across
tests/unit/event_bus/.Deliberate conservatism (disclosed, pinned, not a silent gap)
Provenance is not tracked through a mutated accumulator (
out = []…out.append(policy.resolve_spec(spec))). That is genuinely correct code and the guard reports it anyway. The documented remedy is the batch helperresolve_specs_for_creation— which is what every live path already does — not a loosening of the analysis.test_create_topics_guard_is_conservative_about_accumulatorspins the behaviour so the next person to hit it reads it as a decision rather than a bug to widen away. A guard that is loose in order to avoid inconveniencing a refactor is the failure this one replaces.Acceptance-criteria mapping — re-verified, and where the record stands
#2550's body asserted (a) and (b) as satisfied; review proved both false at repository scope, and #2552 closed them in code. This PR does not re-open them — it closes the gap in the mechanism that certifies them.
NewTopicRF must trace to a resolver call at its own site.test_every_create_topics_site_resolves_through_the_policy(live tree →[]); guard executed againstb2ca4faa→ 3 offenders vs the predecessor's 1CreateTopicsservice_topic_manager.py's 3 sites would not have been caught.policy-aware-module-raw-sitecontrol (RED under M-R2:assert 0 == 1)tests/unit/event_bus/CreateTopicstest_capacity_is_measured_once_per_provisionerONEX_BOOT_UNIVERSE_PROVISIONuntouchedGates — run on
.200(stickybeatz-studio), patch-transfer verifiedLocal commit →
git format-patch→git amon the.200worktree. Patchsha256identical across the hop (4b62134f…), tree hash identical on both sides (65173cf48a8755736f3030bb1872c6e144776628), changed-filesha256identical (23e66613…) — the gates ran on the same bytes that were pushed, and the push was issued from.200.ruff format --check src/ tests/— 4575 files already formattedruff check src/ tests/— All checks passedmypy src/omnibase_infra/— Success, no issues in 2632 source filesdetect_test_paths.py) →{"selected_paths":["tests/unit/event_bus/"],"is_full_suite":false}— a single test file changed, no shared module touched, so the selector narrowed. No hand-typed-k.pytest tests/unit/event_bus/— 609 passedpre-commit run --all-filesHonest state of
pre-commit run --all-files— 3 failures, none from this diff:tests/ci/test_runner_routing_audit.pyandtests/scripts/test_deploy_runtime_core_contracts_resolution.pycarrySPDX-FileCopyrightText: 2026onorigin/devitself (verified:git show origin/dev:<path> | head -1→2026for both). Neither file is in this diff. Same two offenders fix(OMN-15395): close the SECOND CreateTopics path — contract-driven RF on the operator CLI, capped drift, memoized probe, loud RF refusal #2552 recorded; still unfixed ondev.check-required-env-vars— wantsGITHUB_TOKENin.200's~/.omnibase/.env. Machine env gap, not a repo defect.reject-required-check-skip-vector—ModuleNotFoundError: No module named 'yaml'fromomniclaude/.github/actions/required-check-skip-guard/validate_no_required_check_skip_vectors.py. The script lives in a different repo and its interpreter on.200lacks PyYAML. Environmental; new since fix(OMN-15395): close the SECOND CreateTopics path — contract-driven RF on the operator CLI, capped drift, memoized probe, loud RF refusal #2552's run.pre-commit run --files <this PR's only changed file>→ exit 0, every hook Passed. Commit-time hooks also ran clean (the commit was re-made after an initial-c core.hooksPath=.git/hooksinvocation was caught as a silent bypass — in a worktree.gitis a file, so that relative path resolves to nothing and skips every hook).No skip tokens, no
--no-verify, no-knarrowing.Ticket: OMN-15395
Evidence-Ticket: OMN-15395
Evidence-Source: OCC#5551