Skip to content

guards(reach): a declare() discover reaches for repo_root() instead of the root it was handed, so the empty-tree mutation proves less than it reads as #17940

Description

@mrveiss

What

repo_tests/_reach.py's discover callable is handed a root on purpose: reach_declarations_test.py::test_no_guard_can_succeed_against_an_empty_tree and ::test_discovery_honours_the_root_it_is_given hand every declaration an empty directory and require the sweep to come back empty, which is the one mechanical proof that a floor can fire.

Some discover callables reach for repo_root() part-way down instead of using that root, so part of their discovery answers a question about the REAL tree while the rest answers one about the root they were given. Where that happens the empty-tree mutation still passes — but for the wrong reason, and it reads identically to the honest case.

Confirmed instance

repo_tests/docs_liquid_tags_are_defined_17930_test.py:

  • _liquid_rendered_docs(root) → _non_excluded_docs(root) → excluded_dirs()
  • excluded_dirs() reads repo_root() / "docs" / "_config.yml", never root / ...

Against the empty tree the sweep still returns [], so both meta-tests pass — but because (empty / "docs").rglob(...) matches nothing, not because the exclude list was read from the tree under test. The root is honoured by the walk and ignored by the predicate that shapes it.

Why this is not a drive-by fix

Changing it to excluded_dirs(root) makes the empty tree a case the function has to answer for, and the obvious answer is a silent one:

  • on the empty tree root / "docs" / "_config.yml" does not exist, so read_text raises FileNotFoundError;
  • Reach.population converts any raise into ReachDiscoveryError, and the empty-tree test needs an empty RESULT, not an exception — so the fix fails the meta-test it was meant to strengthen;
  • the obvious remedy — catch and return frozenset() — introduces a silent-empty path in the REAL tree too: an unreadable or malformed _config.yml would then mean "nothing is excluded", docs/archives/ (107 files, carrying tags Liquid does not define) would enter the population, and the guard would start reporting files the site never renders. That is the correct-about-the-text / wrong-about-the-build failure the guard's own docstring exists to prevent.

The honest fix has to keep "this root has no config" and "this config could not be read" distinguishable. That is a design decision about _reach's contract, not a one-liner, which is why it is filed rather than folded into the PR that found it.

Scope — a class, not an instance

A transitive AST sweep of repo_tests/*.py for declarations whose discover (or anything it calls) invokes repo_root() returns 7 candidates:

guard discover reaches repo_root() in
async_sqlalchemy_declares_greenlet_17432_test.py _declares_sqlalchemy itself
docs_liquid_tags_are_defined_17930_test.py _liquid_rendered_docs excluded_dirs
dpkg_tests_installed_state_not_knowledge_17387_test.py _code_lines_using_dpkg_list itself
dpkg_tests_installed_state_not_knowledge_17387_test.py _dpkg_registrations itself
one_claim_registry_16653_test.py _iter_sources itself
slm_frontend_bare_ui_literals_test.py _components itself
slm_frontend_i18n_parity_test.py _en_message_keys itself

These are candidates, not confirmed defects. Only the second was checked by hand. A repo_root() call inside a discover is legitimate where it names a reference the root is not supposed to change; each of the other six needs reading before it is called anything.

Two measurement notes on that table, because the sweep is the kind of instrument this repo keeps catching:

  • The first version followed call edges one hop and returned 6 — it missed the very instance that prompted the issue, which sits two hops down (_liquid_rendered_docs → _non_excluded_docs → excluded_dirs). The table above is the transitive version. A one-hop answer here is correct, complete, and about a different question.
  • It only sees calls by bare name within one module. A helper imported from another module, or reached through an attribute, is invisible to it, so 7 is a floor on the candidate set and not a count of it.

Acceptance criteria

  • _reach.declare's contract states what a discover must do when the root it is given has no configuration to read, and how that differs from a configuration it cannot read
  • docs_liquid_tags_are_defined_17930_test.py honours its root end to end, with a test proving the malformed-config case still FAILS rather than yielding an empty exclude set
  • each of the other six candidates is read and either fixed or recorded as legitimate, with the reason
  • a check prevents the next one, or its absence is recorded with the reason

Context

Found while fixing the same guard's population under #17930. Related: #15826 (tree-scanning guards that can report clean having examined nothing), #17844 (declare()'s what= is unverified free text, so a guard can be floored correctly over the wrong population) — all three are the same family: the reach machinery proves something adjacent to the thing it reads as proving.

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions