diff --git a/Makefile b/Makefile index 36e62b7a03..ac44044da4 100644 --- a/Makefile +++ b/Makefile @@ -277,9 +277,9 @@ lint-custom: sync-venv-if-uv ## per-branch Last-Known-Good (LKG) commit (or base branch when no ## LKG sidecar exists), and runs pytest on only that subset. Any ## sign of static-analysis fog (conftest / Makefile / pyproject / -## uv.lock / workflow / shared/tests / non-.py / gateway/*.py / -## dynamic-import / unresolvable-baseline / LKG-not-ancestor) widens -## to the full suite with an explicit trigger string on stderr. See +## uv.lock / workflow / shared/tests / non-.py / dynamic-import / +## unresolvable-baseline / LKG-not-ancestor) widens to the full +## suite with an explicit trigger string on stderr. See ## docs/guides/testing.md and scripts/select_tests.py for the full ## design. ## diff --git a/docs/guides/testing.md b/docs/guides/testing.md index 1e7bdb9a69..11947d1f46 100644 --- a/docs/guides/testing.md +++ b/docs/guides/testing.md @@ -103,9 +103,12 @@ by the `make test` recipe. The algorithm is: `from action_guards import …`) to fully-qualified grimp module ids, covering the test and production files that import via short names rather than fully-qualified package paths. Applies - to `shared.*`, `orchestrator.*`, and `sandbox.*`; `gateway.*` - is excluded — its importlib test-loader pattern is handled by - the `gateway/*.py` widening trigger (§7). + to `shared.*`, `orchestrator.*`, `sandbox.*`, AND `gateway.*` — + gateway tests reach production via `gateway/tests/conftest.py`'s + `importlib.spec_from_file_location` loader (which makes every + gateway production module importable by bare name), so the AST + resolver bridges those edges the same way it does for the + sys.path-injected packages. See §7 for the full rationale. 6. **Map modules → test files.** Intersect the downstream set with the pre-collected set of every `test_*.py` / `*_test.py` file in the graph. The selector emits the resulting set of test file @@ -183,7 +186,6 @@ suite, with the explicit trigger string written to stderr (e.g. | **`.github/workflows/test.yml` change** | The CI definition itself — running narrow risks misrepresenting CI's posture. | | **`shared/tests/**` change** | `shared/tests/conftest.py` is a universally-consumed cross-package fixture; v1 widens on any path under `shared/tests/` to avoid an allowlist that has not yet been audited. May narrow in a follow-up. | | **Any non-`.py` change** | Schemas, fixture data, scripts, YAML, Markdown — none are reachable via the import graph. Conservative v1 default; an allowlist of known-safe paths can be added later. | -| **`gateway/*.py` change** (production files directly under `gateway/`, NOT `gateway/tests/`) | **Known static-analysis blind spot.** `gateway/tests/conftest.py`'s `_load_module_with_replaced_imports` loads production modules via `importlib.util.spec_from_file_location`, so gateway tests import production by **bare name** (`from policy import ...`, never `from gateway.policy import ...`). Grimp cannot see those edges. Without this trigger, `gateway/policy.py` edits would silently select zero tests. The explicit trigger string is `gateway source change (importlib test-loader)`. See §7 for the long form. | | **Source file missing from grimp graph** | At graph-construction time, the selector enumerates every non-test `.py` under `gateway/`, `shared/`, `orchestrator/`, `sandbox/` (excluding `__pycache__`, `.venv`, test directories) and asserts each path resolves to a node in `graph.modules`. Any miss (PACKAGES drift, encoding quirk, grimp cache bug) widens to the full suite with trigger `source file missing from graph: `. | | **Dynamic-import-touched module** | During graph construction, the selector regex-scans each module for `importlib.{import_module,util,machinery}`, `__import__`, `SourceFileLoader`, and entry-point plugin patterns. If any changed module is in (or reverse-reachable from) that set, narrow analysis is unsafe. | | **Unresolvable changed path** | A changed path that cannot be mapped to an in-repo module (e.g. brand-new file not yet in grimp's graph, a `scripts/*.py` with no wheel binding). | @@ -191,9 +193,9 @@ suite, with the explicit trigger string written to stderr (e.g. | **LKG not ancestor of HEAD** | The recorded LKG sha is not reachable from `HEAD` (force-push, reset, history rewrite). | When a fallback fires, the stderr line uses the **explicit trigger -name** (e.g. `Makefile changed`, `gateway source change (importlib -test-loader)`), not generic wording — the trigger reason is the -single most useful diagnostic. +name** (e.g. `Makefile changed`, `dynamic-import reachability`), +not generic wording — the trigger reason is the single most useful +diagnostic. --- @@ -329,30 +331,39 @@ Static reverse import graphs are powerful, but they cannot see: do not have static import edges from their consumers. The dynamic-import scan picks up the consumer side; missed cases surface when CI runs `make test-all`. -- **Bare-name imports for non-gateway packages.** `shared.*`, - `orchestrator.*`, and `sandbox.*` modules are almost universally +- **Bare-name imports across packages.** `shared.*`, `orchestrator.*`, + `sandbox.*`, and `gateway.*` modules are almost universally imported by bare name throughout the codebase (e.g. `from egg_logging.signatures import …` rather than `from - shared.egg_logging.signatures import …`). Grimp registers modules - under fully-qualified names, so a plain grimp traversal misses - those edges. **Mitigation:** the bare-name AST resolver (step 5) - AST-scans every `.py` and maps bare-name targets back to - fully-qualified ids, making these edges visible to narrowing. -- **Gateway's importlib test-loader.** `gateway/tests/conftest.py` - defines `_load_module_with_replaced_imports`, which uses - `importlib.util.spec_from_file_location` to load production - modules and then injects mocks via `sys.modules`. Every gateway - test imports production by **bare name** (`from policy import - ...`), so grimp sees zero edges from `gateway/tests/test_*.py` - to `gateway/policy.py`. Without mitigation, a `gateway/policy.py` - edit would resolve an empty test set. **Mitigation:** any change - to a file matching `gateway/*.py` (production files directly - under `gateway/`, NOT `gateway/tests/`) widens to the full suite - with the explicit trigger string `gateway source change (importlib - test-loader)`. The bare-name AST resolver (step 5) intentionally - excludes `gateway.*` — the importlib loader pattern makes AST - edges unreliable there, so the widening trigger remains the stable - workaround. + shared.egg_logging.signatures import …`, or `from policy import …` + rather than `from gateway.policy import …`). Grimp registers + modules under fully-qualified names, so a plain grimp traversal + misses those edges. **Mitigation:** the bare-name AST resolver + (step 5) AST-scans every `.py` and maps bare-name targets back to + fully-qualified ids, making these edges visible to narrowing. The + resolver covers `gateway.*` even though `gateway/` is not on + sys.path during graph build — `gateway/tests/conftest.py`'s + `importlib.spec_from_file_location` loader makes every gateway + production module bare-name-importable at test time, and the AST + resolver only inspects source so the runtime importlib pattern + doesn't affect its view. + + Note: the AST resolver only delivers narrowing if the + dynamic-import fallback (R6, "dynamic-import reachability") does + not fire in its place. Because `_scan_dynamic_imports` regex-scans + every module's source for `__import__`, `importlib.util.*`, + `SourceFileLoader`, etc., any gateway module that legitimately + uses those primitives becomes a seed — and R6 widens for every + module reachable through that seed's `find_upstream_modules` + closure. To keep `gateway/*.py` edits narrowable, the importlib + bootstrap lives in `gateway/_module_loader.py`, a leaf module that + imports only stdlib; its upstream closure is empty, so R6 only + fires when the bootstrap itself is edited (which is the right + call). Keep this invariant in mind when adding any further + dynamic-import primitives anywhere in `gateway/` — adding them to + a module that is upstream of much of the gateway package would + silently re-disable narrowing for everything that flows through + it. These limits are the reason the fallback-trigger list in §4 is as broad as it is — narrowing trades coverage for speed, and any @@ -463,8 +474,6 @@ The stderr trigger string is the diagnostic. Common cases: sidecar. - `unresolvable baseline` — `origin/main` is missing or inaccessible. Check `git remote -v` and `git fetch origin`. -- `gateway source change (importlib test-loader)` — known blind - spot, see §7. The full suite is the right answer here. **"I want to force the full suite for a single run."** diff --git a/gateway/_module_loader.py b/gateway/_module_loader.py new file mode 100644 index 0000000000..6660203555 --- /dev/null +++ b/gateway/_module_loader.py @@ -0,0 +1,61 @@ +"""Sibling-module loader for gateway.gateway. + +Isolated into its own file so the ``__import__`` / ``importlib.util`` +primitives below do NOT mark the surrounding ``gateway/gateway.py`` +module as a dynamic-import seed for ``scripts/select_tests.py``. + +When ``gateway/gateway.py`` itself was a seed, every ``gateway/.py`` +edit reached the seed transitively through ``find_upstream_modules`` and +short-circuited ``make test`` to the full suite via the +``dynamic-import reachability`` trigger. Moving the importlib helpers +into this leaf module — which imports only stdlib — keeps the seed +set small enough that the bare-name AST resolver can actually narrow +gateway production edits. + +Do NOT add gateway-package imports here. The whole point of this +file is to keep the seed's ``find_upstream_modules`` closure free of +``gateway.*`` modules. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from typing import Any + + +def load_sibling_gateway_module(module_name: str) -> Any: + """Import a sibling gateway module regardless of test vs prod shape. + + Gateway modules are loaded two ways in this codebase: as a package + (``gateway.x``) in production and as flat top-level modules by the + test conftest (``__package__ == ""``). Plain ``import X`` works in + production when ``gateway/`` is on ``sys.path``, and in tests when + the conftest preloaded ``X`` into ``sys.modules``. For modules the + conftest does *not* preload — like the ones added in #1882 — we + fall back to loading the file by explicit path so the features are + still exercisable in tests without forcing a conftest edit by the + tester role. + """ + mod = sys.modules.get(module_name) or sys.modules.get(f"gateway.{module_name}") + if mod is not None: + return mod + try: + mod = __import__(module_name) + return mod + except ImportError: + pass + try: + mod_path = Path(__file__).parent / f"{module_name}.py" + if not mod_path.exists(): + return None + spec = importlib.util.spec_from_file_location(module_name, str(mod_path)) + if spec is None or spec.loader is None: + return None + mod = importlib.util.module_from_spec(spec) + sys.modules[module_name] = mod + spec.loader.exec_module(mod) + return mod + except Exception: # pragma: no cover - defensive + return None diff --git a/gateway/gateway.py b/gateway/gateway.py index 3845465780..1b59f32dbb 100644 --- a/gateway/gateway.py +++ b/gateway/gateway.py @@ -366,42 +366,16 @@ logger = get_logger("gateway") -def _load_sibling_gateway_module(module_name: str) -> Any: - """Import a sibling gateway module regardless of test vs prod shape. - - Gateway modules are loaded two ways in this codebase: as a package - (``gateway.x``) in production and as flat top-level modules by the - test conftest (``__package__ == ""``). Plain ``import X`` works in - production when ``gateway/`` is on ``sys.path``, and in tests when - the conftest preloaded ``X`` into ``sys.modules``. For modules the - conftest does *not* preload — like the ones added in #1882 — we - fall back to loading the file by explicit path so the features are - still exercisable in tests without forcing a conftest edit by the - tester role. - """ - mod = sys.modules.get(module_name) or sys.modules.get(f"gateway.{module_name}") - if mod is not None: - return mod - try: - mod = __import__(module_name) - return mod - except ImportError: - pass - try: - import importlib.util - - mod_path = Path(__file__).parent / f"{module_name}.py" - if not mod_path.exists(): - return None - spec = importlib.util.spec_from_file_location(module_name, str(mod_path)) - if spec is None or spec.loader is None: - return None - mod = importlib.util.module_from_spec(spec) - sys.modules[module_name] = mod - spec.loader.exec_module(mod) - return mod - except Exception: # pragma: no cover - defensive - return None +try: + # Production / package mode. + from ._module_loader import load_sibling_gateway_module as _load_sibling_gateway_module +except ImportError: + # Standalone-script mode (the test conftest loads gateway.py as + # a flat top-level module, in which case the relative import + # above raises ImportError before sys.modules has been seeded). + from _module_loader import ( # type: ignore[no-redef, import-untyped] + load_sibling_gateway_module as _load_sibling_gateway_module, + ) def _lookup_commit_observer_fn(name: str) -> Any: @@ -3377,7 +3351,7 @@ def _resolve_checkpoint_token(repo_path: str) -> str | None: _CHECKPOINT_SCRATCH_DIR = "/home/egg/.egg-worktrees/.checkpoint-scratch" -_checkpoint_scratch_lock = __import__("threading").Lock() +_checkpoint_scratch_lock = threading.Lock() def _ensure_checkpoint_scratch_repo() -> str | None: diff --git a/gateway/tests/conftest.py b/gateway/tests/conftest.py index 490c2aac39..e73f3384ed 100644 --- a/gateway/tests/conftest.py +++ b/gateway/tests/conftest.py @@ -88,6 +88,15 @@ def _load_module_with_replaced_imports( # Load modules in dependency order +# _module_loader holds the importlib bootstrap for gateway.py's +# sibling-loader. It has no relative imports. Loading it here makes +# `from _module_loader import ...` resolvable in flat-module test +# mode, mirroring how other gateway dependencies are pre-seeded. +module_loader = _load_module_with_replaced_imports( + "_module_loader", + GATEWAY_DIR / "_module_loader.py", +) + # github_client has no relative imports to other gateway modules github_client = _load_module_with_replaced_imports( "github_client", @@ -345,6 +354,7 @@ def _load_module_with_replaced_imports( "gateway", GATEWAY_DIR / "gateway.py", import_replacements={ + "from ._module_loader import": "from _module_loader import", "from .anthropic_credentials import": "from anthropic_credentials import", "from .auth import": "from auth import", "from .checkpoint_handler import": "from checkpoint_handler import", diff --git a/scripts/select_tests.py b/scripts/select_tests.py index c21aa526b3..3590ec82ec 100755 --- a/scripts/select_tests.py +++ b/scripts/select_tests.py @@ -146,16 +146,45 @@ # follow test→production edges in a codebase that grimp alone cannot # trace. # -# `gateway.` is intentionally absent: `gateway/` is NOT on sys.path -# during `build_graph` (the importlib test-loader pattern in -# `gateway/tests/conftest.py` would shadow grimp's view), and -# `gateway/*.py` changes are handled by their own dedicated widening -# trigger. +# `gateway.` is included even though `gateway/` is NOT on sys.path +# during `build_graph`: gateway tests reach production through +# `gateway/tests/conftest.py`'s `_load_module_with_replaced_imports` +# (importlib `spec_from_file_location`), which makes every gateway +# production module importable by its bare name (`from policy import +# X`, `import auth`). The AST resolver only inspects source — it +# does not import — so the runtime importlib pattern does not affect +# its view. Adding `gateway.` here lets the resolver record the +# test→production edges that grimp cannot see, replacing the +# previous blanket "any `gateway/*.py` edit widens to full suite" +# fallback. +# +# Important: parity with `shared.`/`orchestrator.`/`sandbox.` requires +# that the dynamic-import seed set (R6, `dynamic-import reachability`) +# does not also pull every gateway production module back into a +# full-suite fallback. Because `_scan_dynamic_imports` source-greps +# for `__import__(`, `importlib.util.spec_from_file_location`, etc., +# any gateway module that contains those primitives becomes a seed — +# and `is_dynamic_import_touched` then widens for every module in +# that seed's `find_upstream_modules` closure. When `gateway.gateway` +# itself was a seed (its source contained `__import__(module_name)` +# and `__import__("threading")`), the closure spanned ~32 of the 41 +# gateway production modules, so R6 fired in place of the deleted +# R1 — narrowing in name only. The fix is to keep the dynamic-import +# primitives in `gateway/_module_loader.py` (a leaf bootstrap that +# imports only stdlib), so its upstream closure is empty and R6 only +# fires when the loader itself is edited. See +# `tests/tools/test_select_tests_fallbacks.py:: +# test_gateway_source_change_does_not_widen_with_module_loader_seed` +# for the regression pin and +# `tests/tools/test_select_tests_fallbacks.py:: +# test_gateway_source_change_widens_if_gateway_gateway_becomes_seed` +# for the failure mode being guarded against. BARE_NAME_STRIP_PREFIXES: tuple[str, ...] = ( "shared.", "orchestrator.", "sandbox.tools.", # checked before "sandbox." so the longer prefix wins "sandbox.", + "gateway.", ) # Test-root directories (relative paths) the selector emits when @@ -1286,32 +1315,7 @@ def evaluate_fallback_triggers( if _fnmatch(raw_path, pattern): return trigger_string - # 3d. Gateway importlib-test-loader mapping (R1). Hits any - # `gateway/.py` that is NOT under `gateway/tests/`. Checked - # BEFORE the generic non-.py rule so a mixed diff names the - # specific blind spot. - # - # Layout assumption (locked by current repo as of this PR): gateway/ - # production source is FLAT — every .py production file is directly - # under `gateway/.py`, no subdirectories (verified with - # `ls gateway/*.py`). The TASK-2-3 spec phrases the rule as "any - # changed path matching `gateway/*.py`", which is what the - # `"/" not in raw_path[len("gateway/") :]` guard implements. If - # gateway production code is ever reorganised into subdirectories - # (e.g., `gateway/api/foo.py`), this check would NOT widen on those - # subdirectory edits — extend the guard to drop the `"/" not in` - # clause at that point. TASK-5-2's parametrized cases cover the - # current flat layout and would catch a change in semantics. - for raw_path in paths: - if ( - raw_path.startswith("gateway/") - and not raw_path.startswith("gateway/tests/") - and "/" not in raw_path[len("gateway/") :] - and raw_path.endswith(".py") - ): - return "gateway source change (importlib test-loader)" - - # 3e. Non-.py changes (decision-5) — the catch-all when none of + # 3d. Non-.py changes (decision-5) — the catch-all when none of # the more-specific path triggers fired. for raw_path in paths: if not raw_path.endswith(".py"): diff --git a/tests/tools/test_select_tests_bare_name.py b/tests/tools/test_select_tests_bare_name.py index 5d3facbea0..ea56b8868e 100644 --- a/tests/tools/test_select_tests_bare_name.py +++ b/tests/tools/test_select_tests_bare_name.py @@ -157,13 +157,14 @@ def test_index_strips_sandbox_tools_prefix() -> None: assert index["foo"] == {"sandbox.tools.foo"} -def test_index_does_not_strip_gateway_prefix() -> None: - """`gateway/` is NOT on sys.path during build_graph — gateway - bare-name imports are handled by the dedicated widening trigger, - not the AST resolver. The index should NOT include `policy` as - an alias for `gateway.policy`.""" +def test_index_strips_gateway_prefix() -> None: + """`gateway/tests/conftest.py` loads gateway production modules via + `importlib.util.spec_from_file_location`, so every gateway test + imports production by bare name (`from policy import X`). The + AST resolver bridges those edges, and the index records `policy` + as a bare-name alias for `gateway.policy`.""" index = selector.build_bare_name_index({"gateway.policy"}) - assert "policy" not in index + assert index["policy"] == {"gateway.policy"} assert index["gateway.policy"] == {"gateway.policy"} @@ -243,6 +244,28 @@ def test_upstream_edges_resolves_dotted_bare_name(tmp_path: Path) -> None: assert "tests.shared.egg_logging.test_signatures" in edges["shared.egg_logging.signatures"] +def test_upstream_edges_resolves_gateway_bare_name(tmp_path: Path) -> None: + """`gateway/tests/test_policy.py` does `from policy import X` — + grimp does not see this edge because gateway tests load + production via `importlib.spec_from_file_location` rather than + sys.path. The AST resolver bridges the gap via the `gateway.` + strip prefix.""" + _write( + tmp_path, + { + "gateway/policy.py": "X = 1\n", + "gateway/tests/test_policy.py": "from policy import X\n", + }, + ) + all_modules = { + "gateway.policy", + "gateway.tests.test_policy", + } + edges = selector.build_bare_name_upstream_edges(all_modules, tmp_path) + assert "gateway.policy" in edges + assert "gateway.tests.test_policy" in edges["gateway.policy"] + + def test_upstream_edges_drops_self_edge(tmp_path: Path) -> None: """A file that bare-name imports itself (uncommon, but possible via re-exports) doesn't produce a self-loop.""" diff --git a/tests/tools/test_select_tests_fallbacks.py b/tests/tools/test_select_tests_fallbacks.py index 29d7c99a77..07df610043 100644 --- a/tests/tools/test_select_tests_fallbacks.py +++ b/tests/tools/test_select_tests_fallbacks.py @@ -237,61 +237,112 @@ def test_static_path_triggers(path: str, expected: str) -> None: # ---------------------------------------------------------------------- -# R1 — gateway importlib test-loader mapping. +# Gateway production edits no longer widen — the AST resolver bridges +# the importlib test-loader pattern via `gateway.` in +# `BARE_NAME_STRIP_PREFIXES`, so a `gateway/.py` edit must +# resolve to None here and be handled by narrow analysis. +# +# Two tests pin both ends of the contract: +# * The R6 dynamic-import trigger MUST NOT fire for gateway production +# paths even when the seed set is non-empty AND `gateway.gateway` +# itself is a seed (modelling the production bundle's actual shape). +# If `gateway/gateway.py` regrew an importlib primitive and pulled +# all of `gateway/*.py` back into its upstream closure, this test +# would catch it. +# * The original "no seeds at all" stub-bundle case is kept as a +# baseline assertion below `test_gateway_source_change_does_not_widen`. # ---------------------------------------------------------------------- -@pytest.mark.parametrize( - "path", - [ - "gateway/policy.py", - "gateway/server.py", - "gateway/auth.py", - "gateway/credentials.py", - ], -) -def test_gateway_source_change_widens_to_full_suite(path: str) -> None: +_GATEWAY_PROD_PATHS = [ + "gateway/policy.py", + "gateway/auth.py", + "gateway/checkpoint_handler.py", + "gateway/worktree_manager.py", +] + + +@pytest.mark.parametrize("path", _GATEWAY_PROD_PATHS) +def test_gateway_source_change_does_not_widen(path: str) -> None: + """Baseline: with no dynamic-import seeds, `gateway/.py` edits + must fall through to narrow analysis (no full-suite trigger).""" + bundle = _StubBundle(all_modules={"gateway." + Path(path).stem}) trigger = selector.evaluate_fallback_triggers( paths=[path], - bundle=_StubBundle(all_modules={"gateway." + Path(path).stem}), + bundle=bundle, baseline_source="LKG", lkg_was_stale=False, ) - assert trigger == "gateway source change (importlib test-loader)" + assert trigger is None -def test_gateway_test_change_does_not_fire_gateway_rule() -> None: - """A change under ``gateway/tests/`` (not a production file - directly under ``gateway/``) must NOT fire the gateway rule — - those edits are tester-side and use the standard graph closure. +@pytest.mark.parametrize("path", _GATEWAY_PROD_PATHS) +def test_gateway_source_change_does_not_widen_with_module_loader_seed(path: str) -> None: + """Production-shape regression: the gateway dynamic-import seed is + `gateway._module_loader` (not `gateway.gateway`) so its + ``find_upstream_modules`` closure is empty for gateway production + files. Editing any `gateway/.py` must therefore NOT trigger + R6 even though the seed set is non-empty. + + If a future refactor reintroduces an importlib primitive into + `gateway/gateway.py` (or any module that transitively imports the + rest of `gateway/`), the seed's upstream closure would balloon and + R6 would fire for every gateway prod path. This test pins the + contract that the seed is leaf-shaped. """ + module = "gateway." + Path(path).stem bundle = _StubBundle( - all_modules={"gateway.tests.test_policy"}, - dynamic_import_modules=set(), - upstream_map={}, + all_modules={module, "gateway._module_loader"}, + # The seed exists, but its upstream closure does NOT include any + # gateway production module (the bootstrap loader only imports + # stdlib). This mirrors what `_scan_dynamic_imports` produces + # against the real gateway tree post-refactor. + dynamic_import_modules={"gateway._module_loader"}, + upstream_map={"gateway._module_loader": set()}, ) trigger = selector.evaluate_fallback_triggers( - paths=["gateway/tests/test_policy.py"], + paths=[path], bundle=bundle, baseline_source="LKG", lkg_was_stale=False, ) - assert trigger != "gateway source change (importlib test-loader)" + assert trigger is None + +@pytest.mark.parametrize("path", _GATEWAY_PROD_PATHS) +def test_gateway_source_change_widens_if_gateway_gateway_becomes_seed(path: str) -> None: + """Inverse of the above: documents what we are guarding against. -def test_nested_gateway_path_does_not_fire_gateway_rule() -> None: - """Files under a sub-directory of gateway (e.g. ``gateway/sub/x.py``) - are not directly under gateway/, so they don't trigger the - importlib rule (the rule's pattern is ``gateway/.py`` only). + If `gateway.gateway` re-acquires a dynamic-import primitive and + becomes a seed, every production module it transitively imports + will hit R6 (`dynamic-import reachability`) and widen to the full + suite. This test pins the diagnostic so a future regression is + obvious — failing here means you reintroduced the seed and the + gateway-production narrowing has silently reverted to full-suite + fallback for ~80% of files. """ - bundle = _StubBundle(all_modules={"gateway.sub.x"}) + module = "gateway." + Path(path).stem + # Model the pre-refactor production bundle: gateway.gateway is a + # seed AND its upstream closure includes the rest of gateway prod. + bundle = _StubBundle( + all_modules={module, "gateway.gateway"}, + dynamic_import_modules={"gateway.gateway"}, + upstream_map={ + "gateway.gateway": { + "gateway.policy", + "gateway.auth", + "gateway.checkpoint_handler", + "gateway.worktree_manager", + } + }, + ) trigger = selector.evaluate_fallback_triggers( - paths=["gateway/sub/x.py"], + paths=[path], bundle=bundle, baseline_source="LKG", lkg_was_stale=False, ) - assert trigger != "gateway source change (importlib test-loader)" + assert trigger == "dynamic-import reachability" # ---------------------------------------------------------------------- @@ -371,18 +422,16 @@ def test_dynamic_import_reachability_changed_module_in_seed_set() -> None: """A changed module that IS in the dynamic-import seed set fires the trigger directly.""" bundle = _StubBundle( - all_modules={"gateway.gateway", "gateway.policy"}, - dynamic_import_modules={"gateway.gateway"}, + all_modules={"sandbox.plugin_loader"}, + dynamic_import_modules={"sandbox.plugin_loader"}, ) trigger = selector.evaluate_fallback_triggers( - paths=["gateway/policy.py"], # gateway/*.py — fires R1 first + paths=["sandbox/plugin_loader.py"], bundle=bundle, baseline_source="LKG", lkg_was_stale=False, ) - # gateway/*.py rule fires before the dynamic-import rule, so we get - # the more-specific R1 string. This is intentional priority order. - assert trigger == "gateway source change (importlib test-loader)" + assert trigger == "dynamic-import reachability" def test_dynamic_import_reachability_via_upstream() -> None: diff --git a/tests/tools/test_select_tests_logging.py b/tests/tools/test_select_tests_logging.py index 555136cb25..69560d7c48 100644 --- a/tests/tools/test_select_tests_logging.py +++ b/tests/tools/test_select_tests_logging.py @@ -203,10 +203,9 @@ def test_patch_selection_record_malformed_json_returns_0_with_notice( r"\(baseline=[0-9a-f]{7}, trigger=diff\)$" ) _FULL_LINE_RE = re.compile( - # trigger= ... up to the FINAL closing paren of the line. The - # trigger string itself may legitimately contain `(...)` (e.g. - # ``gateway source change (importlib test-loader)``) so we anchor - # on the line end rather than the first close-paren. + # trigger= ... up to the FINAL closing paren of the line. Anchor + # on line end rather than the first close-paren so trigger strings + # that legitimately contain `(...)` round-trip through the regex. r"^select-tests: full suite \d+ tests \(trigger=.+\)$" ) @@ -236,7 +235,6 @@ def test_narrow_line_format_matches_regex() -> None: "unresolvable baseline", "dynamic-import reachability", "non-.py change", - "gateway source change (importlib test-loader)", "source file missing from graph: shared/egg_config/_orphan.py", "graph unavailable", ], diff --git a/tests/tools/test_select_tests_monorepo.py b/tests/tools/test_select_tests_monorepo.py index 97fdfe2e28..aec90ef056 100644 --- a/tests/tools/test_select_tests_monorepo.py +++ b/tests/tools/test_select_tests_monorepo.py @@ -235,6 +235,33 @@ def test_gateway_modules_marked_as_dynamic_imports(real_repo_graph) -> None: ) +def test_gateway_gateway_is_not_a_dynamic_import_seed(real_repo_graph) -> None: + """``gateway.gateway`` must remain leaf-shaped wrt dynamic-import seeds. + + The dynamic-import primitives (``__import__``, ``importlib.util.spec_*``) + live in ``gateway/_module_loader.py`` — a stdlib-only leaf bootstrap. + If a future change reintroduces one of those primitives into + ``gateway/gateway.py``, ``_scan_dynamic_imports`` will flag + ``gateway.gateway`` as a seed; ``is_dynamic_import_touched`` then + widens whenever any module in ``find_upstream_modules('gateway.gateway')`` + is edited. Because ``gateway.gateway`` transitively imports ~32 of + 41 gateway production modules, that regression silently reverts ~80% + of gateway production .py files to the full-suite fallback (R6) — + the exact regression issue #2320 was filed to prevent. + + This is a real-bundle pin against the production source shape; it + fails the moment the regression lands, not after the empirical- + narrowing claim is contradicted in a downstream PR. + """ + assert "gateway.gateway" not in real_repo_graph.dynamic_import_modules, ( + "gateway.gateway is back in dynamic_import_modules — an importlib " + "primitive was reintroduced. Move it to gateway/_module_loader.py " + "(or another stdlib-only leaf) to keep gateway production narrowing " + "intact. See gateway/_module_loader.py for the bootstrap pattern " + "and docs/guides/testing.md §7 for the seed-shape invariant." + ) + + # ---------------------------------------------------------------------- # Issue #2259 regression — `/scripts` on sys.path must not shadow # the top-level `tests/` package during graph construction.