Make changeset-aware test selection narrow again

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

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, ...] = (

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)

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

shared/egg_tool_output.py → 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

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).
 # ----------------------------------------------------------------------

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]}"
 
 
 # ----------------------------------------------------------------------