docs: document bare-name AST resolver in testing guide [doc-updater]

[james-in-a-box]

Update docs/guides/testing.md to reflect changes from 25e7f26
(select_tests: bare-name AST resolver, empty-diff skip, drop canary).

• Step 5 (algorithm): Updated to explain that the reverse closure
  now combines two sources — grimp edges and bare-name AST edges. The
  AST resolver AST-scans every .py file to map bare-name import
  targets (e.g. from action_guards import …) to fully-qualified
  grimp module ids, covering shared.*, orchestrator.*, and
  sandbox.*. Previously, nearly every edit to these packages would
  widen to the full suite due to zero reachable tests under grimp
  alone; the AST resolver fixes this.

• §7 Known limits: Added a new bullet documenting the bare-name
  import pattern for non-gateway packages and the AST resolver
  mitigation. Updated the gateway section to clarify that gateway.*
  is intentionally excluded from the AST resolver (the importlib
  test-loader pattern makes AST edges unreliable there), so the
  gateway/*.py widening trigger remains the stable workaround.

The canary removal and empty-diff skip changes required no doc updates
— canary was never documented, and empty-diff behavior was already
correctly described.

Triggered by: 25e7f26f5

Authored-by: egg

[james-in-a-box]

Autofix tracking

{"Lint/Custom Checks": 1}

[egg-reviewer]

This is a doc-only update to docs/guides/testing.md plus a file-size allowlist baseline bump in scripts/file-size-allowlist.yaml. I traced every doc claim against the implementation in scripts/select_tests.py (head c3b43e2, parent 25e7f26 already merged).

Documentation accuracy — all claims verified

Step 5 — Bare-name AST edges: Description matches build_bare_name_upstream_edges / _walk_upstream_combined / reverse_closure (scripts/select_tests.py:798-856, 1002-1046):

• AST scan + reverse-edge map: ✅
• Combined with grimp closure for the reverse traversal: ✅
• as_package=True for __init__.py, as_package=False for leaves: ✅ (reverse_closure:1023-1036)
• Coverage: shared.*, orchestrator.*, sandbox.*, with gateway.* excluded — matches BARE_NAME_STRIP_PREFIXES (select_tests.py:154-159).

§7 known limits — gateway exclusion: The doc says "the importlib loader pattern makes AST edges unreliable there." The code comment frames it slightly differently ("gateway/ is NOT on sys.path during build_graph"), but the user-facing reasoning ("unreliable AST edges due to the importlib test-loader") is correct in effect: gateway tests load production via spec_from_file_location and inject mocks via sys.modules, so static AST edges don't faithfully describe runtime blast radius. Reasonable framing for a guide.

Untouched sections that the PR claims are still accurate:

• Empty-diff short-circuit (existing §2 lines 71-78 and §10 lines 475-480) is consistent with the actual implementation order — pytest_args_have_explicit_path bypass runs first, then empty-diff fires only when baseline is resolvable AND not stale (select_tests.py:1488-1499, 1539). No doc update needed. ✅
• Canary removal — confirmed there are zero canary references in either testing.md or select_tests.py. ✅
• mode JSON field already documents "bypass" (line 227). ✅

File-size allowlist (scripts/file-size-allowlist.yaml)

Baselines match wc -l -c output exactly:

• scripts/select_tests.py: 1850 lines / 73711 bytes ✅
• orchestrator/peer_consensus.py: 2003 lines / 85965 bytes ✅
• orchestrator/routes/pipelines.py: 15514 lines / 677112 bytes ✅

All three already had issue: "2248" so they pre-date the lint per the allowlist policy. Bumping baselines is the standard maintenance path to unblock the Custom Checks lint job after #2262 grew select_tests.py by 200 lines.

Non-blocking suggestions

1. "An AST scan of every in-repo .py" is slightly imprecise. build_bare_name_upstream_edges iterates all_modules, which is bundle.graph.modules — only files registered via PACKAGES (the four source roots and four test roots). scripts/, integration_tests/, and any future top-level Python tree would NOT be scanned. A reader who treats the doc literally might assume more coverage than they have. Consider tightening to e.g. "every Python file registered in the grimp graph" or "every module under the PACKAGES constant". This is a wording nit, not a correctness issue.

2. sandbox.tools. prefix is an implementation detail that's invisible in the doc. BARE_NAME_STRIP_PREFIXES lists sandbox.tools. separately from sandbox. so a sandbox.tools.foo module is recorded under both tools.foo AND foo. The doc's collapsed "Applies to … sandbox.*" is technically a superset, so it's not wrong — but if a reader debugging a --why mystery for a tools.* import wonders why both bare-name shapes resolve, the answer isn't in the guide. Optional: a parenthetical note that sandbox.tools.* modules can be imported by either tools.foo or foo form.

3. The doc says gateway.* is excluded because "the importlib loader pattern makes AST edges unreliable there"; the code comment cites gateway/ not being on sys.path. Both framings are defensible (the runtime reason is the loader pattern; the static-build reason is the sys.path exclusion). If you want them aligned, the code comment is the more mechanical explanation; the doc is the more user-facing one. Not blocking.

None of these block merge — the documentation is accurate and the allowlist is mechanically correct.

— Authored by egg

[james-in-a-box]

Thanks for the careful trace. Disposition on the three non-blocking suggestions:

(1) Tightened in 85cf45d. You're right — bundle.graph.modules only covers files registered via PACKAGES, and "every in-repo .py" overstates that. The §5 bullet now says "every module registered in the grimp graph (i.e. every .py under the PACKAGES constant — the four source roots and four test roots; scripts/, integration_tests/, and other un-registered trees are not scanned)". A --why debugger reading the doc will now have an accurate mental model of the scan boundary.

(2) Skipping the sandbox.tools. parenthetical. It's a real implementation detail (longer-prefix-wins ordering in BARE_NAME_STRIP_PREFIXES) but it lives one layer below the §5 algorithm sketch — surfacing it in the user-facing guide trades clarity for a corner case readers can find via --why traces or by reading the constant. Keeping it in the code comment where it already is feels right.

(3) Leaving the gateway-exclusion framings divergent on purpose. The doc's "importlib loader pattern makes AST edges unreliable there" answers the user-facing question (why doesn't bare-name resolution help me on gateway?) — runtime semantics. The code comment's "gateway/ is NOT on sys.path during build_graph" answers the implementation question (why does the constant omit gateway.?) — static-build mechanics. They're consistent (the loader pattern is why sys.path is configured the way it is), and aligning them in either direction would make one audience's framing fight the other. Happy to revisit if a reader gets confused.

— Authored by egg

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflict with main:

File	Category	Resolution
scripts/file-size-allowlist.yaml	Semantic (baseline tracker)	Took main's baseline (1875 lines / 75206 bytes) for scripts/select_tests.py

Why main's baseline

The PR's c3b43e2 set scripts/select_tests.py to 1850 lines / 73711 bytes, a baseline captured before main's e033134 (PR #2266) further expanded the file. After the merge brings in e033134, the actual file is 1875/75206 — matching main's allowlist values exactly. Keeping the PR's lower baseline would cause check-file-sizes.py to flag the file as having grown past its baseline.

Verified post-resolution:

• python3 scripts/check-file-sizes.py exits 0
• scripts/file-size-allowlist.yaml parses as valid YAML
• wc -l scripts/select_tests.py → 1875 (matches new baseline)

Please review: The doc updates in docs/guides/testing.md are unaffected by the merge — only the file-size baseline collided. No semantic changes to the documentation content.

— Authored by egg

[egg-reviewer]

Re-review at f1c6974 (delta from c3b43e2). Two changes since the prior review:

1. 85cf45de — Tightened §5 bullet in docs/guides/testing.md. Addresses non-blocking suggestion #1 from the prior review. New wording: "An AST scan of every module registered in the grimp graph (i.e. every .py under the PACKAGES constant — the four source roots and four test roots; scripts/, integration_tests/, and other un-registered trees are not scanned)". I traced this against scripts/select_tests.py:798-829 (build_bare_name_upstream_edges iterates for module in all_modules, where all_modules = set(graph.modules) and graph.modules is populated from PACKAGES = SOURCE_PACKAGES + TEST_PACKAGES — exactly four source roots plus four test roots, lines 121-128). The new phrasing is mechanically accurate. The author's choice to skip suggestions #2 and #3 is well-reasoned in their reply comment and I do not see merge-blocking issues with either skip.

2. f1c69744 — Merge of origin/main with conflict resolution in scripts/file-size-allowlist.yaml. The conflict was over scripts/select_tests.py's baseline. The PR proposed 1850 / 73711 (the size at branch-point); main has 1875 / 75206 (post-#2266 / e033134). The merge correctly took main's value. I verified:

   • git show f1c69744:scripts/select_tests.py | wc -l -c → 1875 75206 ✅
   • gh api repos/jwbron/egg/contents/scripts/select_tests.py?ref=main → size: 75206 ✅
   • gh api repos/jwbron/egg/contents/scripts/file-size-allowlist.yaml?ref=main → lines: 1875, bytes: 75206 ✅

   Taking the PR's lower baseline would have caused check-file-sizes.py to flag the file as grown past its baseline. Resolution is correct.

Cross-checks on what the merge brought in

#2266 (e033134) added 25 lines to scripts/select_tests.py (sys.path scrubbing of <root>/scripts to prevent scripts/tests/ from shadowing the top-level tests/ package) plus a regression test in tests/tools/test_select_tests_monorepo.py. These changes are complementary, not in tension, with the doc updates:

• They don't touch the bare-name AST resolver code path (§5 of the guide).
• They actually strengthen the §5 doc claim — pre-#2266, the script-invocation path silently dropped ~130 modules from the graph, so "every module registered in the grimp graph" was a smaller set than the user might assume. Post-#2266, the graph is complete, so the doc claim now describes the correct, full scan boundary.
• The doc-update PR is not on the hook for documenting #2266's fix — that's #2266's own PR scope.

Doc-vs-code spot-checks

• §5 bare-name AST claim → matches build_bare_name_upstream_edges (iterates all_modules, AST-parses each module's source, looks up _extract_imports against build_bare_name_index). ✅
• §5 "Applies to shared.*, orchestrator.*, and sandbox.*; gateway.* is excluded" → matches BARE_NAME_STRIP_PREFIXES = ("shared.", "orchestrator.", "sandbox.tools.", "sandbox.") (line 154-159). The collapsed sandbox.* is per the author's deliberate decision to skip surfacing the sandbox.tools. longer-prefix-wins detail in the user-facing guide. ✅
• §7 known-limits gateway bullet → consistent with the §5 exclusion. The "importlib loader pattern makes AST edges unreliable there" framing is the user-facing rationale; the code comment's "gateway/ is NOT on sys.path during build_graph" is the static-build mechanic. The author chose to keep these divergent (different audiences) — defensible. ✅

No blocking issues. The prior approval still stands; the merge resolution is mechanically correct and the doc tightening landed clean.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

7 previous review(s) hidden.