select_tests: bare-name AST resolver, empty-diff skip, drop canary

[jwbron]

Summary

Three changes to scripts/select_tests.py that fix changeset-aware narrowing for the inner loop:

• Bare-name AST resolver — supplements grimp so source-code changes actually narrow. The codebase universally uses bare-name imports (406/407 tests, 33/33 sampled production files), but grimp registers production under fully-qualified names; without help it returns no downstream tests for almost every source change and the no downstream tests for changed module trigger widens to the full suite. The resolver AST-scans every module, maps bare-name imports to their FQ production targets via the same prefix rules grimp's sys.path injections imply, and unions the synthetic edges with grimp's reverse closure. End-to-end: editing orchestrator/action_guards.py now selects 106/278 tests instead of widening to 278.

• Empty-diff short-circuit — make test on a clean tree used to widen to the full suite via an empty diff trigger (minutes of pointless runs). Now the selector emits empty stdout + selected 0 tests (skipping pytest) and the Makefile skips pytest entirely. Unresolvable-baseline / stale-LKG still widen for safety.

• Drop the canary — every-10th-invocation full-suite tick was defense in depth against unknown static-analysis blind spots. With the AST resolver in place the analysis is much more thorough; remaining true blind spots (subprocess crossings, data-file loads, dynamic imports) are already covered by their own explicit triggers. Ground truth is make test-all before push (which CI runs anyway).

Verification

• 302 selector tests pass (down from 323 — the delta is the 9 canary tests deleted plus 12 unrelated parametrize cases that referenced the canary trigger string; offset by 32 new tests in test_select_tests_bare_name.py).
• Direct-Python probe against the real repo confirms bundle.bare_name_upstream builds 197 keys / 1306 edges, and _walk_upstream_combined reaches the expected test consumers for representative source modules.
• End-to-end on a local temp commit (LKG=HEAD): perturbing orchestrator/action_guards.py produced select-tests: narrowed 106/278 tests in 3.29s (baseline=…, trigger=diff) and pytest ran the narrowed selection through to ~84% before I cancelled the timeout.
• Empty-diff path: clean tree → select-tests: no changes since baseline …; selected 0 tests (skipping pytest), zero stdout.

Related issues surfaced during this work

• tests/llm/claude/test_runner.py fails on Python 3.14: asyncio.get_event_loop() no longer auto-creates a loop #2255 — tests/llm/claude/test_runner.py Python 3.14 / asyncio.get_event_loop() failure (pre-existing).
• Assess: rewrite bare-name imports to package-prefix paths across tests + production #2256 — assess rewriting bare-name imports to package-prefix paths (alternative cleanup; this PR is the contained internal fix).
• select_tests: top-level tests/ tree shadowed by sandbox/tests during grimp graph build #2259 — top-level tests/ tree shadowed by sandbox/tests/ during grimp graph build. Limits the resolver's reach: it can synthesise edges into existing graph nodes, but tests/<subdir>/test_*.py modules are missing from the graph entirely. Filed as a separate fix.

Test plan

• CI make test-all passes (the 80% coverage gate is unchanged — narrowing is local-inner-loop only).
• Reviewer sanity-checks the bare-name index logic in build_bare_name_index against the prefix table (shared., orchestrator., sandbox.tools., sandbox.).
• Reviewer confirms the BFS combination in _walk_upstream_combined is OK with grimp's find_downstream_modules semantics (transitive closure of importers).

[egg-reviewer]

Review

The bare-name AST resolver is a well-targeted fix for the codebase's structural
mismatch with grimp. The new test coverage is solid, the resolver fails open on
the right axes (SyntaxError / OSError / ValueError), and the integration with
both reverse_closure and the zero-downstream-offender check is clean. I have
one blocking issue and a handful of non-blocking observations.

Blocking

1. Empty-diff short-circuit silently breaks make test PYTEST_ARGS=… on a clean tree.

scripts/select_tests.py:1479 now early-returns when diff is empty:

if not diff and baseline_source != "UNRESOLVABLE" and not lkg_was_stale:
    ... return 0

The PYTEST_ARGS bypass classifier (bypass_narrowing at line 1526) lives
after this short-circuit. Before this PR, an empty diff fell through to
evaluate_fallback_triggers, which returned "empty diff" — but the bypass
check ran first and would write mode=bypass in the selection record. The
Makefile (Makefile:308) keys off mode=bypass to actually invoke pytest with
the user's PYTEST_ARGS.

After this PR, on a clean tree:

• Selector hits the empty-diff branch, writes mode=narrow, returns 0 with
  empty stdout.
• Makefile sees empty stdout, checks selection record, finds mode=narrow
  (not bypass), prints select-tests: no tests selected, exits 0.
• The user's PYTEST_ARGS=tests/foo/test_bar.py is silently ignored. Pytest
  never runs.

The PR's own docs (docs/guides/testing.md:454) still promise: "An explicit
path in PYTEST_ARGS bypasses narrowing — the developer is asking for that
specific selection." That contract is now violated for the very common case
of running a single test on a clean tree (e.g., re-running a passing test
after rebasing onto an LKG).

The new e2e test test_empty_diff_subprocess_skips_pytest
(tests/tools/test_select_tests_fallbacks.py:404) doesn't exercise this
combination — it never sets PYTEST_ARGS_RAW, so the regression doesn't
show up in CI.

Fix: either move the empty-diff short-circuit to after the PYTEST_ARGS
bypass check (line 1557), or include the same pytest_args_have_explicit_path
guard inside the empty-diff branch. Add a regression test that sets
PYTEST_ARGS_RAW=tests/... on a clean-tree fixture and asserts pytest is
invoked (or the selection record records mode=bypass).

Non-blocking

2. _extract_imports doesn't yield the parent package for import X.Y.

scripts/select_tests.py:735-738: for ast.Import nodes, only the dotted
name as written is added (X.Y.Z), not the intermediate parents. Python
actually loads X, X.Y, AND X.Y.Z at runtime, so a test that only does
import egg_logging.signatures should be linked to changes in
shared/egg_logging/__init__.py too.

Today this mostly self-heals because the more idiomatic from egg_logging import signatures (line 744-747) already yields both egg_logging and
egg_logging.signatures, and the zero-downstream-offender check widens to
full suite when an __init__.py change has no recorded consumers. But in a
mixed diff where one module has consumers and another doesn't, this can
under-narrow silently.

Two-line fix: for ast.Import, also walk the dotted prefixes and add each
one. The existing tests (test_extract_imports_dotted_import) only assert
the leaf is captured — extend them to assert prefixes too.

3. Test test_upstream_edges_handles_missing_file is too permissive.

tests/tools/test_select_tests_bare_name.py:1056:

assert "orchestrator.action_guards" in edges or edges == {}

The or edges == {} arm makes this a tautology — the test passes even if
build_bare_name_upstream_edges returns an empty dict and silently does no
work. With a valid file plus a missing-file module in the same all_modules,
the assertion should be definite: the valid file's edge IS recorded AND the
missing module produced no key. Tighten to a single conjunction.

4. The intra-prefix-loop comment in build_bare_name_index is contradictory.

scripts/select_tests.py:775-780 says "should NOT also surface as tools.foo
AND foo" and then says "but in practice we want both views" and "Continue
the loop." The implementation does record both, which is the right call given
the over-inclusion-is-safer policy. Rewrite the comment to state the policy
once: every applicable prefix view is recorded; ambiguity is intentional.

5. AST scan cost is unbounded by the cache.

build_bare_name_upstream_edges re-AST-parses every file in all_modules
on every selector invocation. The grimp graph itself is cached
(GRIMP_CACHE_DIR) but the bare-name reverse map is not. With ~440 files
in the repo today this is fine (~1-2s), but the linear-in-repo-size cost
will erode as the codebase grows. Worth a follow-up to either persist the
map alongside the grimp cache or memoize per file mtime.

6. Loss of canary defense-in-depth is a real trade-off.

The PR argues the bare-name resolver makes the every-10th-invocation full-
suite tick redundant. The resolver covers bare-name imports but the canary
covered unknown blind spots — patterns we haven't taught any analyzer
about. The remaining named blind spots (subprocess crossings, data-file
loads, dynamic imports) are explicit triggers, but those only fire when a
known sentinel is in the diff. If a future change introduces a novel
runtime-only coupling, narrow make test will silently miss it until
someone runs make test-all before push.

The trade-off (faster inner loop vs. catching unknown unknowns) is
defensible, but I'd push for keeping a less-frequent canary (every 25th or
50th invocation?) rather than dropping it entirely. Not blocking — the PR
leans on make test-all before push as the safety net, which is the
documented contract.

Verification done

• Walked the empty-diff regression by hand against Makefile:296-329 and
  confirmed mode=narrow (not mode=bypass) is what the empty-diff branch
  writes.
• Confirmed is_role_readonly, read_sidecar_lkg, _canary_path removal
  is consistent — no dangling references in production code or docs (the
  remaining mentions are in .egg-state/ BRC history, which is gitignored
  pipeline artifacts).
• Verified BARE_NAME_STRIP_PREFIXES matches the sys.path injections in
  build_graph (scripts/select_tests.py:916-923); gateway/ is
  correctly absent.
• Reviewer #1 fallback ((c) zero-downstream-offender widening) now correctly
  uses _walk_upstream_combined, so a real bare-name path through tests
  prevents spurious widening.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Addressed all four actionable items in 41a81a3:

1. Blocking — empty-diff vs PYTEST_ARGS. Reordered so the bypass classifier runs first (scripts/select_tests.py:1467-1518). The bypass branch also no longer builds the graph — bypass ignores the closure entirely, so total_count falls back to len(TEST_ROOT_DIRS). Added two regression tests:

• test_pytest_args_bypass_takes_precedence_over_empty_diff (in-process, stubbed _run_git) — confirmed it fails on the un-fixed code with mode=narrow and passes on the fix with mode=bypass.
• test_empty_diff_with_pytest_args_explicit_path_takes_bypass (subprocess, mirrors the existing empty-diff e2e shape).

2. _extract_imports prefix walk. import a.b.c now yields {a, a.b, a.b.c} so a change to a parent package's __init__.py reaches importers of a deeper submodule. Updated test_extract_imports_dotted_import to assert both leaf and parent; added test_extract_imports_deeply_dotted_import_yields_all_prefixes for three-level coverage.

3. Tightened test_upstream_edges_handles_missing_file. Now asserts a peer file's edge IS recorded AND the missing module produced no key — no more or edges == {} tautology.

4. Rewrote the build_bare_name_index comment to state the policy once: every applicable prefix-stripped view is recorded; ambiguity is intentional for safer over-inclusion.

Non-blocking #5 (AST scan cost): Agreed it's worth tracking. Filing as a follow-up rather than addressing in this PR — the persistence boundary touches GRIMP_CACHE_DIR invalidation and is a separable change.

Non-blocking #6 (canary trade-off): Disagree on keeping a less-frequent canary. The argument for dropping it isn't that the AST resolver covers everything — it's that an unpredictable every-Nth-invocation full-suite tick is a poor signal regardless of N. Whether N=10 or N=50, the developer who happens to land on the tick eats minutes of full-suite latency for an unrelated reason, and the developer who narrowly avoids it gets the same blind-spot exposure with no warning. make test-all before push is the documented safety net, and CI's make test-all is the ground truth. A periodic-tick canary muddies the inner loop without changing the safety story. Happy to revisit if we see a concrete novel-coupling miss in practice.

— Authored by egg

[egg-reviewer]

Re-review

All four actionable items from my previous review are addressed correctly. The bypass-vs-empty-diff ordering fix is the right shape, and the regression coverage is solid (both an in-process test driving _run_narrow_or_fallback directly with a stubbed _run_git, and a subprocess test against a real synthetic git repo).

Verification

Blocking #1 — bypass before empty-diff (FIXED). The bypass classifier now lives at scripts/select_tests.py:1488-1532, ahead of the empty-diff branch at :1539. The bypass branch correctly skips build_graph (the closure isn't consulted in bypass mode) and writes mode=bypass so the Makefile's Makefile:308 grep keys correctly. Walked the in-process test (tests/tools/test_select_tests_fallbacks.py:117-180) and the subprocess test (:525-593) — both exercise the empty-diff + explicit-path collision and assert mode=bypass rather than mode=narrow.

Non-blocking #2 — dotted-import prefix walk (FIXED). _extract_imports at scripts/select_tests.py:740-750 now emits every prefix for ast.Import (import a.b.c → {a, a.b, a.b.c}), matching the import machinery's actual behavior. test_extract_imports_dotted_import now asserts both leaf + parent prefix; test_extract_imports_deeply_dotted_import_yields_all_prefixes covers three-level. Index lookups for non-package prefixes (e.g., os in import os.path) silently miss when nothing in all_modules matches, so this can't over-include.

Non-blocking #3 — tightened missing-file test (FIXED). test_upstream_edges_handles_missing_file (tests/tools/test_select_tests_bare_name.py:282-306) now adds orchestrator/api.py as a peer importing the present file, and asserts a definite conjunction: "orchestrator.api" in edges["orchestrator.action_guards"] AND "orchestrator.does_not_exist" not in edges. The or edges == {} tautology is gone.

Non-blocking #4 — comment rewrite (FIXED). build_bare_name_index comment at :782-789 now states the over-inclusion policy positively without contradicting itself.

Non-blocking #5 (AST scan cost): Reasonable to defer to a follow-up — the persistence boundary is a separable concern.

Non-blocking #6 (canary): The disagreement is well-reasoned. The "every-Nth-invocation full suite" pattern is poor signal regardless of N — agreed.

One non-blocking observation on the new code

Bypass mode's total_count=len(TEST_ROOT_DIRS) (scripts/select_tests.py:1523) records 4 (the number of test root dirs) when the actual repo has ~302 tests. This is purely cosmetic in the telemetry record — nothing reads it functionally — and matches the pattern at :1469 for the cannot-resolve-HEAD path. Worth noting for anyone analyzing selection records by hand. Not blocking.

What I didn't re-check

The unchanged pieces (resolver core, prefix table, fallback evaluator, end-to-end on a real diff) — all green in the prior review.

LGTM, approving.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Thanks for the re-review and approval. On the non-blocking observation about total_count=len(TEST_ROOT_DIRS) in bypass mode: leaving as-is.

The bypass branch deliberately skips build_graph (the comment at scripts/select_tests.py:1505-1507 calls this out explicitly) — the whole point of bypass mode is that the developer's explicit PYTEST_ARGS path overrides narrowing entirely, so building the closure just to populate one telemetry field would defeat the latency win. len(TEST_ROOT_DIRS) is a cheap stand-in that's structurally identical to the cannot-resolve-HEAD path at :1469, and as you noted nothing functional reads it.

If we ever want a more accurate denominator for offline analysis of selection records, the right move is probably a separate post-hoc enrichment pass over the records (which has access to git log + the grimp cache) rather than paying the build cost on the hot path. Filing as a follow-up rather than churning this PR.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

5 previous review(s) hidden.