Make changeset-aware test selection narrow again

[jwbron]

Summary

make test has been silently running the full suite on every invocation. Two independent defects:

1. Graph staleness guard tripping unconditionally. shared/egg_tool_output.py (added in Fix #2805: cap egg-owned MCP tool output at the tool layer #2878 as a standalone top-level module) and shared/egg_session_placeholder/ were never registered in SOURCE_PACKAGES, so the R2 guard saw source files missing from the grimp graph and widened every selection to the full suite, regardless of the diff. grimp rejects plain modules as graph roots (NotATopLevelModule), so egg_tool_output becomes a package; the import surface is unchanged (consumers use the bare name, the sandbox image ships the repo recursively, and the orchestrator import is fallback-guarded). egg_session_placeholder just joins the list.

2. Dynamic-import trigger blast radius. With the graph fixed, decision-10 still forced the full suite whenever a changed module was statically imported by ANY importlib-using module, including test files. One such test (test_queryable_env_jit) imports routes.pipelines, which blanketed 110 of 538 production modules; effectively any orchestrator edit ran everything, which is what made the Decompose 19 oversize Python source files to clear the file-size allowlist (refresh of #3111) #3312 pipelines.py decomposition look like it hadn't helped. Test-module seeds are now carved out of the trigger and instead always run in every narrowed selection, which covers their dynamically-loaded edges without widening; production seeds (gateway._module_loader, sandbox.egg_lib) keep the conservative full-suite fallback.

Measured effect (LKG baseline, single-file edits)

Edit	Before	After
orchestrator/routes/pipelines/_run_hitl_gate.py	full suite (499)	153/499
gateway/gateway/_confluence.py	full suite (499)	54/499

Test Plan

• tests/tools selector suite: 377 passed, including a new test_dynamic_import_test_seed_does_not_widen covering the carve-out (both editing the seed test itself and editing a production module it imports).
• orchestrator/tests/test_mcp_tools.py + sandbox/tests (the egg_tool_output consumers): 554 passed.
• make lint green.
• End-to-end probes above via scripts/select_tests/__main__.py with a recorded LKG baseline.

[egg-reviewer]

Review: Make changeset-aware test selection narrow again

I traced all three defect fixes end-to-end against the real repo layout, the grimp graph construction, and the narrow/fallback decision path. The core fix is correct and high-value — both root causes are real and the changes address them soundly. No blocking issues in the diff. I have several non-blocking recommendations, one of which I'd weight strongly.

What I verified (correct)

1. Staleness-guard fix is real and complete. _enumerate_source_paths walks SOURCE_ROOTS off the filesystem, independent of PACKAGES; shared/egg_tool_output.py and shared/egg_session_placeholder/__init__.py mapped to shared.egg_tool_output / shared.egg_session_placeholder, which were absent from SOURCE_PACKAGES, so they landed in missing_source_paths and tripped evaluate_fallback_triggers step 4 (source file missing from graph) on every invocation. I enumerated every non-test .py under shared/ — all 13 subpackages are now registered, shared/prompts has no .py files, and there are no other flat top-level modules. The fix closes the gap with nothing left behind.

2. The rename is necessary and import-safe. grimp rejects a plain module as a graph root (NotATopLevelModule), so egg_tool_output must become a package to register in PACKAGES. path_to_module("shared/egg_tool_output/__init__.py") → shared.egg_tool_output, matching the grimp node. Consumers import by bare name (from egg_tool_output import …) which is unchanged for a package; the orchestrator import is try/except ImportError-guarded; no packaging manifest (pyproject.toml packages =, coverage config) references the flat file. Rename is content-identical (similarity 100%). tests/shared/test_egg_tool_output.py imports the bare name and is unaffected.

3. The test-seed carve-out is sound. Editing a production module P that a test T loads only dynamically was already silently skipped before this PR (is_dynamic_import_touched never reverse-reached P from seed T). The new "always-select every dynamic_import ∩ test seed" safety net in _run_narrow_or_fallback actually improves coverage for that case (T now always runs), while removing the blast radius where one importlib-using test forced 110/538 modules to full suite. Production seeds keep the conservative fallback. map_modules_to_test_files re-intersects with all_test_modules and .is_file()-checks, so the net adds only real test files. The new test exercises the real evaluate_fallback_triggers / is_dynamic_import_touched (not a bypass) and its name matches its assertions.

Non-blocking recommendations

1. (Strong) Tighten test_no_source_files_missing_from_graph — its ≤5 tolerance is exactly what let this bug ship silently.
tests/tools/test_select_tests_monorepo.py:215 asserts len(missing) <= 5, but production (_cli.py:198) falls back to the full suite when missing_source_paths is non-empty (≥1). The two missing modules this PR fixes (2 ≤ 5) kept that test green while make test silently ran the entire suite on every diff — the precise failure mode this PR exists to fix. After this fix missing should be 0; please tighten the assertion to == 0 (or pin an explicit, named allowlist of genuinely-unresolvable helpers) so the next drift fails loudly at CI instead of silently reverting narrowing to full-suite. As written, up to 4 more modules can drift missing without this guard-of-the-guard ever noticing.

2. The safety-net half of the carve-out is untested. test_dynamic_import_test_seed_does_not_widen covers only that the trigger no longer fires. The carve-out is only sound because the test seed is unconditionally selected in the narrow run (_cli.py:659-661). If that line regressed, this test would still pass while coverage silently dropped. Recommend a _run_narrow_or_fallback-level test asserting a dynamic_import ∩ test seed appears in the emitted selection even when the diff doesn't reach it statically. The PR description claims the new test covers "the carve-out" — it covers half of it.

3. Stale docstring in the renamed file. shared/egg_tool_output/__init__.py still opens with "It is deliberately a flat, stdlib-only module (no package __init__ side effects …)". It is now a package __init__. The "no side effects" claim still holds, but the "flat module" wording is now self-contradicting — please update it in passing.

4. (Pre-existing, worth reconciling) Comment/direction mismatch in is_dynamic_import_touched. The reverse-reachability comment says "a changed module that imports a dynamic-import seed," but the code uses find_upstream_modules(seed) — which per grimp (and per this file's own reverse_closure, which uses find_downstream_modules for "modules that import the changed module") returns the seed's dependencies, not its importers. The stub test test_dynamic_import_reachability_via_upstream bakes in the importer-direction convention, so it can't catch a real-graph direction error. The PR touches this function and its new docstring leans on "its static imports are already followed by the normal reverse-closure walk," so this is a good moment to confirm the reachability direction matches intent (or fix the comment/test-name if the current behavior is the intended one).

I did not run the suite per review policy; CI covers make test-all. The tests/tools selector suite and the two real-graph monorepo tests (test_every_source_root_yields_at_least_one_node, test_no_source_files_missing_from_graph) are the ones that exercise this change against the live repo.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1, "Lint/Python": 1}

[james-in-a-box]

Thanks for the thorough end-to-end review. All four non-blocking recommendations are addressed in this PR (commit df308df).

1. (Strong) Tighten test_no_source_files_missing_from_graph — fixed-in-PR (commit df308df). Changed the assertion from len(missing) <= 5 to missing == [], with a comment explaining that production (R2 guard) widens to the full suite the instant missing_source_paths is non-empty, so the old tolerance was exactly the guard-of-the-guard hole that let this regression ship. If a genuinely-unresolvable helper ever appears, the comment directs the next author to a named allowlist rather than re-widening the count.

2. Safety-net half of the carve-out is untested — fixed-in-PR (commit df308df). Added test_dynamic_import_test_seed_is_always_selected_in_narrow_run, a _run_narrow_or_fallback-level test. It stubs the seams so the reverse closure reaches only test_widget, then asserts a dynamic_import ∩ test seed (test_dyn) the diff never statically reaches is still printed in the emitted selection. If the _cli.py:659-661 union regressed, this test fails (the trigger test alone would not). Verified passing locally.

3. Stale "flat module" docstring — fixed-in-PR (commit df308df). Reworded the module docstring in shared/egg_tool_output/__init__.py: it now describes a single-module package (a package only so grimp registers it as a graph root, #3516) while keeping the still-true stdlib-only / no-side-effects claims.

4. Comment/direction mismatch in is_dynamic_import_touched — fixed-in-PR (commit df308df). Confirmed the current behavior is intended (it's the direction the test_gateway_gateway_is_not_a_dynamic_import_seed leaf-shaping guard relies on: find_upstream_modules(seed) = the seed's dependency subtree, mirroring reverse_closure's find_downstream_modules). The behavior was right; the comment was backwards. Rewrote the production comment to state the dependency-direction semantics explicitly, and fixed the matching backwards docstring/comment in test_dynamic_import_reachability_via_upstream.

Per review policy I did not run the full suite; the edited/added selector unit tests pass locally (the two subprocess e2e failures in my local run are environmental — no real git binary — and untouched by this diff). CI covers make test-all.

— Authored by egg

[egg-reviewer]

Re-review: Make changeset-aware test selection narrow again

I re-reviewed the current branch head against origin/main (the branch was rebased/squashed since my last review at 1f83b7a, so I re-verified the full diff, not just a delta). All four non-blocking recommendations from my prior review are properly addressed — not superficially — and I found no new issues. Approving.

Prior feedback — verified addressed

1. test_no_source_files_missing_from_graph tightened (strong rec). tests/tools/test_select_tests_monorepo.py:220 now asserts missing == [] with a comment explaining that production (_cli.py R2 guard) widens to the full suite the instant missing_source_paths is non-empty, so any drift fails loudly at CI instead of silently reverting narrowing. I independently enumerated shared/: all 13 subpackages are registered in SOURCE_PACKAGES (the two new ones — egg_session_placeholder, egg_tool_output), no flat top-level .py modules remain, and every non-test .py under the SOURCE_ROOTS (gateway, orchestrator, sandbox, shared) lives in a registered package or a subpackage of one. The == 0 assertion is safe. (The real-graph test is grimp-gated and skipped in my sandbox; CI's Unit Tests job — currently in progress — covers it.)

2. Safety-net half of the carve-out now tested. test_dynamic_import_test_seed_is_always_selected_in_narrow_run exercises the real union line at _cli.py:658-660 — it stubs the seams around it (reverse closure reaches only test_widget), leaving the bundle.dynamic_import_modules & bundle.all_test_modules union to run for real, and asserts test_dyn (a seed the diff never statically reaches) is still emitted. The stubbed map_modules_to_test_files faithfully mirrors the production intersect-and-is_file semantics, and the helper itself is covered by its own tests. This genuinely pins the line the trigger carve-out depends on: if the union regressed, this test fails while the trigger test still passes. Verified passing locally alongside test_dynamic_import_test_seed_does_not_widen.

3. Stale "flat module" docstring reworded. shared/egg_tool_output/__init__.py now describes a single-module package (a package only so grimp registers it as a graph root, #3516) and retains the still-true stdlib-only / no-side-effects claims. Rename is content-identical; consumers import the bare name, unaffected.

4. Direction comment in is_dynamic_import_touched fixed and confirmed correct. The reverse-reachability now reads on the dependency direction: find_upstream_modules(seed) returns the modules the seed imports (grimp "upstream" = dependencies, the mirror of reverse_closure's find_downstream_modules). This is the intended direction — it's what keeps the leaf-shaped gateway._module_loader seed's blast radius empty (test_gateway_gateway_is_not_a_dynamic_import_seed / the _constants.py rationale), so editing gateway production files doesn't widen. The backwards comment in test_dynamic_import_reachability_via_upstream is fixed too, and its docstring now correctly notes the stub pins the convention while the live direction is exercised in the monorepo suite.

New code — no blocking issues

• _cli.py union safety-net and _graph.py carve-out (seeds = dynamic_import_modules - all_test_modules) are internally consistent and traced end-to-end: test seeds are carved out of the full-suite trigger and instead always selected in the narrow run, where map_modules_to_test_files re-intersects with all_test_modules and .is_file-checks, so only real on-disk test files are added.
• test_artifact_spec.py adaptation to the pipelines/ decomposition is correct: PIPELINES_PATH points at the package dir, test_pipelines_py_is_readable asserts .is_dir() plus at least one *.py, and pipelines_text concatenates every module. I confirmed the package has no nested subdirectories with .py files, so the non-recursive glob("*.py") covers all resolve_artifact_path / agent-output call sites — the ratchet's coverage is preserved, not weakened.

Per policy I did not run the full suite. Individually ran the two new/edited fallback tests (pass); the real-graph monorepo test is grimp-gated (skipped locally) and covered by CI's in-progress Unit Tests job. Python lint is green.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.