Repository navigation
fix(OMN-16709): restore sys.modules in the scaffold fixture so the unrestored-del ratchet passes - #1601
Merged
jonahgabriel merged 1 commit intoAug 27, 2026
Conversation
…N-14985 ratchet passes `Tests Gate` -> `CI Summary` (a REQUIRED check on dev) has been failing closed on every omnibase_core branch since #1598 (OMN-16679) merged at 2026-08-27T00:00:29Z. The failing test is test_no_unrestored_del_sys_modules_in_tests, a static scan of the whole tests/ tree, so it fails regardless of what the PR under test changes -- observed on the docs-only #1600 (OMN-16581), reproduced byte-identically twice. The flagged fixture is NOT the OMN-14944 contaminator class. That class evicts PRE-EXISTING omnibase_core.* entries and never restores them. This fixture does the opposite: it deletes only `set(sys.modules) - original_modules`, i.e. names that did not exist before the test, which is the scaffold-package purge that keeps a temp package from leaking across xdist-shared workers. The ratchet's AST scan cannot see that set-difference semantics, so it is a true-positive lexical hit on a semantically correct fixture. Deliberately NOT fixed by wrapping the block in isolated_sys_modules(): that helper evicts pre-existing entries and restores them on exit while explicitly LEAVING newly-imported entries in place. Substituting it would satisfy the guard while deleting the purge this fixture exists to perform, reintroducing the cross-worker leak OMN-16679 guarded against. Instead: snapshot sys.modules as a dict, keep the newly-added-name purge exactly as-is, then sys.modules.update(original_modules). The purge behavior is unchanged, the fixture additionally restores any pre-existing entry a scaffold import displaced (strictly stronger than before), and the restore is the real one the ratchet's _function_restores_sys_modules looks for -- not a no-op added to appease the guard. Verified on the .201 gate host before commit: tests/unit/test_omn_14944_sys_modules_purge_regression.py -> 3 passed tests/unit/cli/test_cli_new_scaffold_runnable.py -> 8 passed i.e. the ratchet is green and every OMN-16679 acceptance test still passes. OMN-16709
jonahgabriel
enabled auto-merge (squash)
August 27, 2026 04:30
Contributor
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 126 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
jonahgabriel
pushed a commit
to OmniNode-ai/onex_change_control
that referenced
this pull request
Aug 27, 2026
…e_core#1601 (#7260) * evidence: OCC companion pass 1 for OmniNode-ai/omnibase_core#1601 * evidence: OCC companion self-bind for #7260 --------- Co-authored-by: node-occ-companion-effect <occ-companion-effect@omninode.ai>
jonahgabriel
deleted the
jonah/omn-16709-scaffold-fixture-sys-modules-restore
branch
August 27, 2026 05:02
jonahgabriel
restored the
jonah/omn-16709-scaffold-fixture-sys-modules-restore
branch
August 27, 2026 06:37
This was referenced Sep 13, 2026
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
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.
OMN-16709 — unblocks every omnibase_core PR on
devCI Summaryis a required status check onomnibase_coredevand fail-closes behindTests Gate.Tests Gatehas been red on every branch cut fromdevsince #1598(OMN-16679) merged at 2026-08-27T00:00:29Z (
240ca9f205c35274094dff7ca133f576b1ef17db),because the failing test is a static AST scan of the whole
tests/tree — it does not carewhat the PR under test changed.
Net effect: no omnibase_core PR could merge to
dev, regardless of content. Observed live onthe docs-only #1600 (OMN-16581), reproduced byte-identically across jobs 98394415201 and
98400912799 — deterministic, not flaky.
Root cause
The
importable_srcfixture added by #1598 ends with a baredel sys.modules[name]loop. TheOMN-14985 ratchet (
_function_restores_sys_modules) only accepts adel sys.modules[...]whenthe enclosing function also contains a
with isolated_sys_modules(...)block, asys.modules[key] = ...assignment, or asys.modules.update(...)call. The fixture has noneof the three.
This was a false positive — and the obvious fix is the wrong one
The OMN-14944 leak class is: evict pre-existing
omnibase_core.*entries and never restorethem. This fixture does the opposite — it deletes only
set(sys.modules) - original_modules,i.e. entries that did not exist before the test. That is cleanup of the scaffolded temp
package, which genuinely must not leak across xdist-shared workers.
isolated_sys_moduleswas deliberately not used: it has different semantics (it evictspre-existing entries and restores them on exit, while explicitly leaving in place entries
newly imported inside the block). Swapping it in would have satisfied the guard while silently
deleting the scaffold-package purge this fixture exists to perform, reintroducing the very
cross-worker leak #1598 was guarding against.
Fix
Snapshot
sys.modulesas a dict, keep the newly-added-name purge exactly as-is, then restorethe pre-existing entries explicitly:
Strictly stronger than before: the purge is unchanged, and any pre-existing entry a scaffold
import displaced is now also restored. It satisfies the ratchet through the
sys.modules.update(...)branch as an honest restore, not a guard-appeasement no-op.One file, +9/-2, tests only. No source change.
dod_evidence
tests/unit/test_omn_14944_sys_modules_purge_regression.py— 3 passed; repo-wideviolations list empty.
tests/unit/cli/test_cli_new_scaffold_runnable.py— 8 passed (the OMN-16679 acceptancetests still pass, so the fixture's purge behavior is unchanged).
11 passed in 5.53s.is_full_suite=False,full_suite_reason=None, 44 selected paths, and no baretests/ortests/unit/entry (so the OMN-15408 whole-suite-equivalent guard correctly did not fire).env | grep -Ei 'PREPUSH|ENABLE_SMART_TESTS'→ none present. No--no-verify, noPREPUSH_FULL_SUITE, noENABLE_SMART_TESTS, noPREPUSH_ALLOW_*.Tests Gate+CI Summary, plus previously-blocked docs(OMN-16581): close Wave-3 audit residuals A+B — Makefile ci.yml rename leftover + stale omnidash handshake template #1600(OMN-16581) going green on a re-run with no change to its own diff.
Fixes OMN-16709
Evidence-Ticket: OMN-16709
Evidence-Source: OCC#7260