From 33337a26ec708641035e651464d0fc9a1f0b58d7 Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Thu, 23 Jul 2026 19:34:51 +0100 Subject: [PATCH] feat(hygiene): detect orphan config files by reachability, not filename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/hygiene config` could not see orphan config FILES: `_hygiene_config.py` iterated only LIBRARY yamls and skipped any without a workspace counterpart, so a workspace file the libraries never shipped was structurally invisible. That blind spot let a dead `config/grids.yaml` survive in 10 repos for ~a year (autolens_workspace#317). Add an orphan-file pass beside the existing key-mirror diff. A workspace `config/**/*.yaml` with no library counterpart at the same relative path is an orphan. The right test of a config file is REACHABILITY (does anything read it), not filename similarity — a pure filename check is ~120 hits, nearly all legitimate, because whole subtrees are owned by something other than the libraries. Those owners are named in an explicit ORPHAN_OWNERS map and suppressed: `build/*` (PyAutoHands release tooling) and `priors/*` (JSONPriorConfig class-path resolution). Deliberately an in-checker map, NOT a per-repo `.hygieneignore` — 20 new files and a config surface that can itself go stale is the exact failure mode being fixed. The library's own shipped set encodes the verdict: it ships `non_linear/GridSearch.yaml` (live) but not `non_linear/{nest,mle,mcmc}.yaml` (dead), so the pass keeps the first and surfaces the rest with no per-file rule. Only repos whose config/ MIRRORS the library tree (share >=1 file) are scanned, which self-scopes to the workspace/test/assistant repos and excludes organ repos without a hardcoded list. Acceptance (all verified): flags the real pre-#317 `grids.yaml` blob, flags `non_linear/*` on today's tree, stays silent on `build/*` and `priors/*`. A check that cannot re-find the bug that motivated it is not validated. Folded into the same `count|summary` prescan line; stdlib+PyYAML only; the PyYAML-absent `sys.exit(1)` fallback is preserved. Co-Authored-By: Claude Opus 4.8 --- agents/conductors/hygiene/_hygiene_config.py | 136 +++++++++++++++++-- tests/test_hygiene_conductor.py | 56 ++++++++ 2 files changed, 178 insertions(+), 14 deletions(-) diff --git a/agents/conductors/hygiene/_hygiene_config.py b/agents/conductors/hygiene/_hygiene_config.py index c1e7c2c..df2d9d9 100755 --- a/agents/conductors/hygiene/_hygiene_config.py +++ b/agents/conductors/hygiene/_hygiene_config.py @@ -1,17 +1,35 @@ #!/usr/bin/env python3 -"""_hygiene_config.py — count library config keys missing from the matching -workspace config, for the hygiene conductor's `config` mode. - -The "mirror new library config keys into the workspace configs" chore: when a -library adds a key to one of its `config/*.yaml` files, the workspace config -(which overrides library defaults) should usually gain it too. This does a -*recursive* key-path diff (top-level-only would miss nested drift, which is -where it happens) for the shared config files in each library↔workspace pair. +"""_hygiene_config.py — the hygiene conductor's `config` prescan: two drift +signals over the workspace config tree, folded into one `count|summary` line. + +1. **Key-mirror drift** (`diff`): when a library adds a key to one of its + `config/*.yaml` files, the workspace config (which overrides library + defaults) should usually gain it too. A *recursive* key-path diff + (top-level-only would miss nested drift, which is where it happens) over the + shared config files in each library↔workspace pair. + +2. **Orphan config files** (`orphan_files`): a workspace `config/**/*.yaml` with + **no library counterpart** at the same relative path. This is the signal the + key-mirror diff is structurally blind to — it skips any file lacking a + workspace *or* library counterpart, so a workspace file the libraries never + shipped is invisible. That blind spot let a dead `config/grids.yaml` survive + in 10 repos for ~a year (autolens_workspace#317). + + The honest test of a config file is **reachability** — does anything read it + — not filename similarity. A pure filename check is far too noisy (~120 hits, + nearly all legitimate) because whole config subtrees are owned by something + *other* than the libraries and legitimately have no library default. Those + owners are named in ``ORPHAN_OWNERS`` and suppressed; what remains is the + review list. The library's *own* shipped set encodes the reachability verdict + for us: e.g. it ships ``non_linear/GridSearch.yaml`` (live — + ``conf.instance["non_linear"]["gridsearch"]``) but not + ``non_linear/{nest,mle,mcmc}.yaml`` (dead — searches take their defaults from + Python signatures), so the orphan pass keeps the first and surfaces the rest + without any per-file rule. Stdlib + PyYAML only — never imports the science stack. Emits one `count|summary` -line for the `config` prescan; exits non-zero (no output) if PyYAML is absent so -the conductor falls back gracefully. It is a *surface* signal: a missing key may -be an intentional workspace omission — the count is "keys to review", not bugs. +line; exits non-zero (no output) if PyYAML is absent so the conductor falls back +gracefully. It is a *surface* signal — the count is "items to review", not bugs. """ from __future__ import annotations @@ -32,6 +50,29 @@ ("PyAutoLens/autolens/config", "autolens_workspace/config"), ] +# The library packages whose shipped `config/` tree is the reachability +# reference: a workspace config file is an orphan iff no library ships one at the +# same relative path. (repo dir, package dir). +LIBRARIES = [ + ("PyAutoFit", "autofit"), + ("PyAutoGalaxy", "autogalaxy"), + ("PyAutoLens", "autolens"), + ("PyAutoArray", "autoarray"), + ("PyAutoCTI", "autocti"), + ("PyAutoNerves", "autonerves"), +] + +# Config subtrees owned by something OTHER than the libraries, so a workspace +# copy with no library counterpart is expected, not drift. Keyed by the top path +# segment under `config/`; the value names the owner and is documentation for the +# summary, not logic. Keep this list small and justified — every entry is a +# reachability claim ("something reads these, just not a library default"). +ORPHAN_OWNERS = { + "build": "PyAutoHands (release tooling reads workspace config/build/*)", + "priors": "JSONPriorConfig (prior configs resolved by class path; " + "workspace/user-defined classes have no library default)", +} + def key_paths(node, prefix: str = "") -> set[str]: """Every nested dict key path in `node` (dotted).""" @@ -75,13 +116,80 @@ def diff(root: str, pairs=PAIRS) -> tuple[int, list[str]]: return total, detail +def _yaml_relpaths(config_dir: str) -> set[str]: + """Relative paths of every `*.yaml` under `config_dir` (recursively), + forward-slash-normalised so a dict lookup is platform-stable.""" + out: set[str] = set() + for f in glob.glob(os.path.join(config_dir, "**", "*.yaml"), recursive=True): + out.add(os.path.relpath(f, config_dir).replace(os.sep, "/")) + return out + + +def library_config_relpaths(root: str, libraries=LIBRARIES) -> set[str]: + """The union of every config-file relative path the libraries ship — the + reachability reference the orphan check compares against.""" + out: set[str] = set() + for repo, pkg in libraries: + cfg = os.path.join(root, repo, pkg, "config") + if os.path.isdir(cfg): + out |= _yaml_relpaths(cfg) + return out + + +def _suppressed(relpath: str) -> bool: + """True if `relpath`'s top segment under config/ is an owned subtree + (ORPHAN_OWNERS) — a workspace copy there is expected, not drift.""" + return relpath.split("/")[0] in ORPHAN_OWNERS + + +def orphan_files(root: str, libraries=LIBRARIES, lib_relpaths=None, + owners=ORPHAN_OWNERS) -> tuple[int, list[str]]: + """Workspace config files with no library counterpart, after owner-map + suppression. + + Only repos whose `config/` *mirrors* the library tree (shares ≥1 file with + the library set) are scanned — that self-scopes to the workspace/tutorial/ + test/assistant repos and excludes organ repos (Brain/Heart/Mind) whose + `config/` is their own thing, without a hardcoded repo list to go stale. + """ + if lib_relpaths is None: + lib_relpaths = library_config_relpaths(root, libraries) + lib_repos = {repo for repo, _ in libraries} + total = 0 + detail: list[str] = [] + for name in sorted(os.listdir(root)): + if name in lib_repos: + continue + cfg = os.path.join(root, name, "config") + if not os.path.isdir(cfg): + continue + rels = _yaml_relpaths(cfg) + if not (rels & lib_relpaths): + continue # not a library-config mirror (organ-internal config) — skip + orphans = {r for r in (rels - lib_relpaths) + if not (r.split("/")[0] in owners)} + if orphans: + total += len(orphans) + detail.append(f"{name}:{len(orphans)}") + return total, detail + + def main() -> int: ap = argparse.ArgumentParser() ap.add_argument("--root", default=os.path.expanduser("~/Code/PyAutoLabs")) ns = ap.parse_args() - total, detail = diff(ns.root) - print(f"{total}|{total} library config keys absent downstream (review/mirror): " - f"{' '.join(detail)}".rstrip()) + keys, key_detail = diff(ns.root) + orphans, orphan_detail = orphan_files(ns.root) + total = keys + orphans + parts = [] + if keys: + parts.append(f"{keys} library config keys absent downstream " + f"(review/mirror): {' '.join(key_detail)}") + if orphans: + parts.append(f"{orphans} orphan config files with no library counterpart " + f"(review/remove): {' '.join(orphan_detail)}") + summary = "; ".join(parts) or "config in sync (no key drift or orphan files)" + print(f"{total}|{summary}") return 0 diff --git a/tests/test_hygiene_conductor.py b/tests/test_hygiene_conductor.py index 6ff18c8..b33098b 100644 --- a/tests/test_hygiene_conductor.py +++ b/tests/test_hygiene_conductor.py @@ -214,6 +214,62 @@ def test_config_helper_recursive_key_diff(tmp_path): assert total == 2 # 'a.y' and 'c' +def _fake_library(root, files): + """Write a fake PyAutoFit library config tree under `root`; `files` maps a + config-relative path to a trivial mapping.""" + import yaml + for rel, data in files.items(): + p = root / "PyAutoFit" / "autofit" / "config" / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(yaml.safe_dump(data)) + + +def _fake_workspace(root, name, files): + import yaml + for rel, data in files.items(): + p = root / name / "config" / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(yaml.safe_dump(data)) + + +def test_orphan_files_flags_unmirrored_and_suppresses_owned(tmp_path): + """The core reachability contract: a workspace config file with no library + counterpart is an orphan (flagged), UNLESS it lives under an owned subtree + (build/, priors/). This is the grids.yaml / non_linear regression: the + library ships non_linear/GridSearch.yaml (kept) but not nest.yaml (flagged). + """ + cfg = _load_config_helper() + _fake_library(tmp_path, { + "general.yaml": {"a": 1}, + "non_linear/GridSearch.yaml": {"grid": 1}, # the LIVE non_linear file + }) + _fake_workspace(tmp_path, "some_workspace", { + "general.yaml": {"a": 1}, # shared -> this IS a mirror + "grids.yaml": {"radial_minimum": 1}, # orphan -> FLAG (the 2025 bug) + "non_linear/nest.yaml": {"Nautilus": 1}, # orphan -> FLAG (dead) + "non_linear/GridSearch.yaml": {"grid": 1}, # mirrored -> keep (live) + "build/env_vars.yaml": {"X": 1}, # owned by Hands -> suppress + "priors/MyClass.yaml": {"p": 1}, # user class prior -> suppress + }) + total, detail = cfg.orphan_files(str(tmp_path)) + assert total == 2, detail # grids.yaml + non_linear/nest.yaml + assert detail == ["some_workspace:2"] + + +def test_orphan_files_skips_non_mirror_repos(tmp_path): + """A repo whose config/ shares nothing with the library set (an organ repo + like Brain/Heart with its own config) is not a mirror and is not scanned — + so its own files are never mis-flagged as orphans.""" + cfg = _load_config_helper() + _fake_library(tmp_path, {"general.yaml": {"a": 1}}) + _fake_workspace(tmp_path, "some_organ", { + "policy.yaml": {"own": 1}, # nothing shared with the library set + "internal.yaml": {"own": 2}, + }) + total, detail = cfg.orphan_files(str(tmp_path)) + assert total == 0 and detail == [] + + def test_help_lists_the_usage_block(tmp_path): r = _run(["--help"], tmp_path) assert r.returncode == 0