diff --git a/scripts/select_tests/_cli.py b/scripts/select_tests/_cli.py index 3135517d65..599a8c310a 100644 --- a/scripts/select_tests/_cli.py +++ b/scripts/select_tests/_cli.py @@ -651,7 +651,15 @@ def _run_narrow_or_fallback(repo_root: Path) -> int: for module, depth in rescue_depths.items(): if module not in closure_depths or depth < closure_depths[module]: closure_depths[module] = depth - test_files = map_modules_to_test_files(bundle, set(closure_depths), repo_root) + # Safety net for the test-seed carve-out in is_dynamic_import_touched: + # every dynamic-import TEST seed runs in every narrowed selection, so a + # change to a module such a test loads only dynamically (invisible to + # the static graph) can never be silently skipped. Cheap: a handful of + # extra test files versus the full-suite fallback they used to force. + selected_modules = set(closure_depths) | ( + bundle.dynamic_import_modules & bundle.all_test_modules + ) + test_files = map_modules_to_test_files(bundle, selected_modules, repo_root) # Emit direct-importers-first (#3182): pytest collects files in the # order given on the command line, so sorting by import distance # from the changed modules surfaces the most likely failure early diff --git a/scripts/select_tests/_constants.py b/scripts/select_tests/_constants.py index 2fdea9ad3a..0440d545e5 100644 --- a/scripts/select_tests/_constants.py +++ b/scripts/select_tests/_constants.py @@ -45,6 +45,8 @@ "shared.egg_orchestrator", "shared.egg_overseer", "shared.egg_restrictions", + "shared.egg_session_placeholder", + "shared.egg_tool_output", ) TEST_PACKAGES: tuple[str, ...] = ( diff --git a/scripts/select_tests/_graph.py b/scripts/select_tests/_graph.py index 5a8ac62dde..e2e48511f9 100644 --- a/scripts/select_tests/_graph.py +++ b/scripts/select_tests/_graph.py @@ -1071,13 +1071,32 @@ def reverse_closure(bundle: GraphBundle, module_path_pairs: Iterable[tuple[str, def is_dynamic_import_touched(bundle: GraphBundle, changed_modules: Iterable[str]) -> bool: """Return True iff any changed module is in (or reverse-reachable - from) the dynamic-import seed set.""" - seeds = bundle.dynamic_import_modules + from) a NON-test dynamic-import seed. + + Test-module seeds are excluded from the full-suite fallback: a test + that dynamically loads production code has its invisible edges + covered by the always-selected safety net in the narrow path (every + test seed runs in every narrowed selection), and its static imports + are already followed by the normal reverse-closure walk. Before this + carve-out a single importlib-using test that imported a hub module + (test_queryable_env_jit -> routes.pipelines) forced the full suite + for 110 of 538 production modules. + """ + seeds = bundle.dynamic_import_modules - bundle.all_test_modules for module in changed_modules: if module in seeds: return True - # Reverse-reachability: a changed module that imports a dynamic- - # import seed can also indirectly trigger dynamic loading. + # Reverse-reachability: ``find_upstream_modules(seed)`` returns the + # seed's own dependency subtree (grimp "upstream" == the modules the + # seed imports, the mirror of ``reverse_closure``'s + # ``find_downstream_modules`` == the modules that import the arg). + # Any changed module in that subtree is one the seed could load + # dynamically at runtime — an edge invisible to the static + # test->prod walk — so widen. This is the direction the gateway + # leaf-shaping guard (test_gateway_gateway_is_not_a_dynamic_import_seed) + # relies on: ``gateway.gateway`` transitively imports ~32 of 41 + # gateway modules, so a dynamic-import primitive there would widen on + # any of their edits. for seed in seeds: try: upstream = bundle.graph.find_upstream_modules(seed, as_package=False) diff --git a/shared/egg_contracts/tests/test_artifact_spec.py b/shared/egg_contracts/tests/test_artifact_spec.py index 3caec9f87e..2927019583 100644 --- a/shared/egg_contracts/tests/test_artifact_spec.py +++ b/shared/egg_contracts/tests/test_artifact_spec.py @@ -408,9 +408,12 @@ class TestConsistencyC_PromptDerivesFromSpec: (covered by Consistency-B above). """ - PIPELINES_PATH = ( - Path(__file__).resolve().parents[3] / "orchestrator" / "routes" / "pipelines.py" - ) + # ``pipelines.py`` was decomposed into the ``pipelines/`` package + # (the prompt-construction / ``resolve_artifact_path`` calls now live + # across ``_prompt_agent.py``, ``_populate.py``, ``_drafts.py``, …), + # so this invariant reads the concatenation of every module in the + # package rather than a single file. + PIPELINES_PATH = Path(__file__).resolve().parents[3] / "orchestrator" / "routes" / "pipelines" # Ratchet against a regression: forbid raw # ``.egg-state/agent-outputs/{_identifier}-…`` f-string literals @@ -426,10 +429,18 @@ class TestConsistencyC_PromptDerivesFromSpec: @pytest.fixture(scope="class") def pipelines_text(self) -> str: - return self.PIPELINES_PATH.read_text() + # Concatenate every module in the ``pipelines/`` package so the + # invariant covers the prompt builders wherever they live after + # the decomposition. + return "\n".join(path.read_text() for path in sorted(self.PIPELINES_PATH.glob("*.py"))) def test_pipelines_py_is_readable(self) -> None: - assert self.PIPELINES_PATH.exists(), f"missing: {self.PIPELINES_PATH} — has the file moved?" + assert self.PIPELINES_PATH.is_dir(), ( + f"missing: {self.PIPELINES_PATH} — has the package moved?" + ) + assert any(self.PIPELINES_PATH.glob("*.py")), ( + f"no modules under {self.PIPELINES_PATH} — has the package moved?" + ) def test_no_raw_agent_output_literals_remain(self, pipelines_text: str) -> None: # Slice-3 of #3077 removed every diff --git a/shared/egg_tool_output.py b/shared/egg_tool_output/__init__.py similarity index 96% rename from shared/egg_tool_output.py rename to shared/egg_tool_output/__init__.py index 78663e019b..86576b3ff8 100644 --- a/shared/egg_tool_output.py +++ b/shared/egg_tool_output/__init__.py @@ -12,9 +12,12 @@ hop, regardless of how large the SDK reader buffer is. This module is the shared helper referenced by #2805's "consistent across -tools" requirement. It is deliberately a flat, stdlib-only module (no -package ``__init__`` side effects, no ``claude_agent_sdk`` import) so both -sides of the boundary can import it once ``shared/`` is on ``sys.path``: +tools" requirement. It ships as a single-module package (``egg_tool_output/ +__init__.py``, a package rather than a flat ``egg_tool_output.py`` only so +grimp registers it as a graph root — #3516) and is deliberately kept +stdlib-only, with no ``claude_agent_sdk`` import and no import-time side +effects, so both sides of the boundary can import it once ``shared/`` is on +``sys.path``: * the **orchestrator** MCP server (operator-facing tools in ``orchestrator/mcp_tools.py``), and diff --git a/tests/tools/test_select_tests_fallbacks.py b/tests/tools/test_select_tests_fallbacks.py index a587a88903..29ac882d25 100644 --- a/tests/tools/test_select_tests_fallbacks.py +++ b/tests/tools/test_select_tests_fallbacks.py @@ -18,6 +18,7 @@ import subprocess import sys from pathlib import Path +from types import SimpleNamespace import pytest @@ -45,20 +46,22 @@ class _StubBundle: """Fake GraphBundle for trigger-eval tests. Only the fields ``evaluate_fallback_triggers`` reads are exposed: - ``all_modules``, ``dynamic_import_modules``, ``missing_source_paths``, - plus a stub ``graph.find_upstream_modules`` for the dynamic-import - reachability check. + ``all_modules``, ``all_test_modules``, ``dynamic_import_modules``, + ``missing_source_paths``, plus a stub ``graph.find_upstream_modules`` + for the dynamic-import reachability check. """ def __init__( self, *, all_modules: set[str] | None = None, + all_test_modules: set[str] | None = None, dynamic_import_modules: set[str] | None = None, missing_source_paths: list[str] | None = None, upstream_map: dict[str, set[str]] | None = None, ) -> None: self.all_modules = all_modules or set() + self.all_test_modules = all_test_modules or set() self.dynamic_import_modules = dynamic_import_modules or set() self.missing_source_paths = missing_source_paths or [] self.graph = _StubGraph(upstream_map or {}) @@ -438,14 +441,22 @@ def test_dynamic_import_reachability_changed_module_in_seed_set() -> None: def test_dynamic_import_reachability_via_upstream() -> None: - """A changed module that imports a dynamic-import seed (transitively) - fires the dynamic-import trigger.""" + """A changed module that a dynamic-import seed imports (transitively) + fires the dynamic-import trigger. + + grimp "upstream" is the dependency direction: ``find_upstream_modules( + seed)`` returns the modules the seed imports, so a changed module in + that set is one the seed could dynamically load at runtime. (The stub + graph below just returns the map verbatim, so it pins the convention + rather than the real grimp direction — the real direction is asserted + against the live graph in the monorepo suite.) + """ # Use a non-gateway module so R1 doesn't fire first. bundle = _StubBundle( all_modules={"sandbox.runner", "sandbox.plugin_loader"}, dynamic_import_modules={"sandbox.plugin_loader"}, - # `sandbox.runner` imports `sandbox.plugin_loader`, so plugin_loader's - # upstream set contains runner. + # `sandbox.plugin_loader` imports `sandbox.runner`, so runner is in + # plugin_loader's upstream (dependency) set. upstream_map={"sandbox.plugin_loader": {"sandbox.runner"}}, ) trigger = selector.evaluate_fallback_triggers( @@ -457,6 +468,99 @@ def test_dynamic_import_reachability_via_upstream() -> None: assert trigger == "dynamic-import reachability" +def test_dynamic_import_test_seed_does_not_widen() -> None: + """A TEST-module seed must not force the full suite (#3312 follow-up). + + A test that dynamically loads production code is covered by the + narrow path's always-selected safety net (every test seed runs in + every narrowed selection), so neither editing the seed test itself + nor editing a production module it statically imports widens the + run. Before the carve-out, one importlib-using test that imported a + hub module forced the full suite for most of the orchestrator. + """ + bundle = _StubBundle( + all_modules={"orchestrator.tests.test_jit", "orchestrator.hub"}, + all_test_modules={"orchestrator.tests.test_jit"}, + dynamic_import_modules={"orchestrator.tests.test_jit"}, + upstream_map={"orchestrator.tests.test_jit": {"orchestrator.hub"}}, + ) + for changed in ("orchestrator/tests/test_jit.py", "orchestrator/hub.py"): + trigger = selector.evaluate_fallback_triggers( + paths=[changed], + bundle=bundle, + baseline_source="LKG", + lkg_was_stale=False, + ) + assert trigger is None, changed + + +def test_dynamic_import_test_seed_is_always_selected_in_narrow_run( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture, tmp_path: Path +) -> None: + """The safety-net half of the carve-out: a ``dynamic_import ∩ test`` + seed appears in the emitted narrow selection even when the diff does + NOT statically reach it (#3516 reviewer non-blocking #2). + + The trigger carve-out (``test_dynamic_import_test_seed_does_not_widen``) + is only sound *because* ``_run_narrow_or_fallback`` unconditionally + unions every dynamic-import test seed into the selected set. This test + pins that line: the reverse closure below yields only ``test_widget``, + yet ``test_dyn`` (a dynamic-import test seed the diff never reaches) + must still be printed. If the safety-net union regressed, the trigger + test would still pass while ``test_dyn`` silently vanished — this one + fails instead. + """ + bundle = SimpleNamespace( + all_modules={"pkg.widget"}, + all_test_modules={"tests.test_widget", "tests.test_dyn"}, + dynamic_import_modules={"tests.test_dyn"}, + ) + + # Land squarely in the narrow path: resolvable LKG baseline, a single + # changed production file, no fallback trigger, and a rescue walk that + # already reaches a test (so the zero-downstream widen doesn't fire). + monkeypatch.setattr( + selector._cli, "resolve_baseline", lambda repo_root: ("a" * 40, "LKG", "main") + ) + monkeypatch.setattr(selector._cli, "lkg_is_stale", lambda repo_root: False) + monkeypatch.setattr(selector._io, "_run_git", lambda args, cwd=None: (0, "b" * 40 + "\n", "")) + monkeypatch.setattr( + selector._cli, "changed_files", lambda baseline_sha, repo_root=None: ["pkg/widget.py"] + ) + monkeypatch.setattr(selector._cli, "build_graph", lambda repo_root: bundle) + monkeypatch.setattr(selector._cli, "evaluate_fallback_triggers", lambda **kw: None) + monkeypatch.setattr( + selector._cli, "_walk_upstream_combined", lambda b, mods: {"tests.test_widget"} + ) + # Reverse closure reaches ONLY test_widget — test_dyn is invisible here. + monkeypatch.setattr( + selector._cli, + "reverse_closure_with_depth", + lambda b, pairs: {"pkg.widget": 0, "tests.test_widget": 1}, + ) + # Mirror the real map_modules_to_test_files semantics (intersect with + # all_test_modules, module id -> path) without touching disk. + monkeypatch.setattr( + selector._cli, + "map_modules_to_test_files", + lambda b, modules, root: sorted( + m.replace(".", "/") + ".py" for m in (set(modules) & b.all_test_modules) + ), + ) + monkeypatch.setattr(selector._cli, "write_selection_record", lambda **kw: None) + # No explicit PYTEST_ARGS path, or the bypass branch would fire first. + monkeypatch.delenv("PYTEST_ARGS_RAW", raising=False) + + rc = selector._cli._run_narrow_or_fallback(tmp_path) + assert rc == 0 + out = capsys.readouterr().out.splitlines() + assert "tests/test_dyn.py" in out, ( + f"dynamic-import test seed dropped from narrow selection: {out}" + ) + # Sanity: the statically-reached test is still there too. + assert "tests/test_widget.py" in out + + # ---------------------------------------------------------------------- # Negative case — narrow path is safe (no trigger). # ---------------------------------------------------------------------- diff --git a/tests/tools/test_select_tests_monorepo.py b/tests/tools/test_select_tests_monorepo.py index ab30b586ae..80f46c2238 100644 --- a/tests/tools/test_select_tests_monorepo.py +++ b/tests/tools/test_select_tests_monorepo.py @@ -209,12 +209,15 @@ def test_no_source_files_missing_from_graph(real_repo_graph) -> None: grimp graph node. An empty ``missing_source_paths`` list confirms PACKAGES covers the real repo layout.""" missing = real_repo_graph.missing_source_paths - # Real-world allowlist: deeply nested helper modules sometimes - # don't resolve via grimp's package-import logic. Tolerate up - # to a small handful, but anything > 5 indicates real drift. - assert len(missing) <= 5, ( - f"{len(missing)} source files missing from grimp graph: {missing[:10]}" - ) + # MUST be exactly zero. Production (`_cli.py` R2 guard) falls back + # to the full suite the instant `missing_source_paths` is non-empty, + # so any drift here silently reverts `make test` to the full suite on + # every diff — the precise failure mode #3516 fixed. The prior + # `<= 5` tolerance is what let two unregistered modules ship that + # regression unnoticed; assert `== 0` so the next drift fails loudly + # at CI instead. If a genuinely-unresolvable helper ever appears, + # pin it in a NAMED allowlist here rather than widening the count. + assert missing == [], f"{len(missing)} source files missing from grimp graph: {missing[:10]}" # ----------------------------------------------------------------------