From 16f8651310e683a455a9d514bbb6b7663fd45329 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 01:16:54 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20an=20allowlist=20is=20authoritative=20?= =?UTF-8?q?=E2=80=94=20no=5Frun.yaml=20filters=20discovery=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #261 shipped the opposite rule ("no_run wins over the list"). Measured against the real workspaces before writing a single delegator, that rule would have SILENTLY DROPPED 13 SCRIPTS from smoke coverage: autogalaxy_workspace_test 9 autolens_workspace_test 2 autofit_workspace 1 autolens_workspace 1 all of which run in smoke today. The vendored runners read only smoke_tests.txt and have never opened no_run.yaml, so honouring it would have been a coverage regression disguised as a refactor — exactly what the task's "no repo loses behaviour it has today" criterion forbids. The rule was wrong because it conflated two policies for two different runs. no_run.yaml governs the release mega-run and notebook generation; an allowlist governs the PR smoke gate. A script legitimately appears in both — excluded from the full build, required in smoke. So with --list the allowlist is the policy and no_run.yaml is not consulted; without one, discovery is filtered by no_run.yaml exactly as before. Second fix, same cause: a missing config/build/no_run.yaml is no longer fatal under --list. autocti_workspace_test has none, and the autohands-level fallback path does not exist either, so both run_python.py and run.py crashed with FileNotFoundError before running anything — once at load and again in parse_no_run_reasons under --report-dir. Discovery still requires the file (a run with no exclusion policy is not a safe default) and now says why. Tests: the two that pinned the old rule are replaced by ones pinning the new, each carrying the measured 13-script rationale, plus a test that discovery is untouched. Suite 389 passed, 5 skipped. Verified end-to-end that a script named by BOTH the list and no_run.yaml now runs, and that the autocti shape (no no_run.yaml at all) completes and reports. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UpSFum81Jeq9KZ9wdKtaeZ --- autohands/build_util.py | 29 ++++++++++++++------- autohands/run.py | 23 ++++++++++++----- autohands/run_python.py | 34 ++++++++++++++++++------ bin/autohands | 9 ++++--- docs/internals.md | 15 ++++++++--- tests/test_script_list.py | 54 ++++++++++++++++++++++++++++----------- 6 files changed, 120 insertions(+), 44 deletions(-) diff --git a/autohands/build_util.py b/autohands/build_util.py index 60f49f4..b4c57a4 100644 --- a/autohands/build_util.py +++ b/autohands/build_util.py @@ -629,8 +629,9 @@ def execute_notebooks_in_folder( ``files`` selects the discovery model, mirroring :func:`execute_scripts_in_folder`: omitted, notebooks are discovered recursively (opt-out coverage); supplied by :func:`files_from_list`, exactly - those entries run in that order (opt-in). ``no_run_list`` applies either way - and wins over the list. + those entries run in that order (opt-in). ``no_run_list`` applies to + DISCOVERY ONLY — with an allowlist the list is authoritative, for the same + reason as :func:`execute_scripts_in_folder`. ``write_back`` and ``retry_from_scripts`` are passed through to :func:`execute_notebook`; a PR smoke gate wants ``write_back=False`` (leave @@ -641,6 +642,7 @@ def execute_notebooks_in_folder( infra_skip = ["__init__", "README"] no_run_list.extend(infra_skip) + files_are_allowlist = files is not None if files is None: files = sorted((Path.cwd() / directory).rglob("*.ipynb")) print(f"Found {len(files)} notebooks") @@ -659,7 +661,9 @@ def execute_notebooks_in_folder( ) ): continue - if should_skip(file, no_run_list): + # The allowlist is the policy when there is one: no_run.yaml governs + # OTHER runs (the release mega-run, notebook generation), not this one. + if not files_are_allowlist and should_skip(file, no_run_list): # Before the existence check, for the same reason as the script # runner: an excluded notebook that has also been deleted is still # excluded, and a failure would contradict the exclusion. @@ -866,17 +870,22 @@ def execute_scripts_in_folder(directory, no_run_list=None, report=None, skip_rea :func:`files_from_list`), exactly those entries run, in that order — coverage is opt-in. - ``no_run_list`` applies either way. An allowlisted script that is also - ``no_run``-listed is SKIPPED with its documented reason: an explicit - exclusion is the more specific statement of intent, and letting an - allowlist override it would resurrect a script that was deliberately - turned off. + ``no_run_list`` applies to DISCOVERY ONLY. When ``files`` is supplied the + list is authoritative and ``no_run.yaml`` is NOT consulted: the two express + policy for **different runs**. ``no_run.yaml`` governs the release mega-run + and notebook generation; an allowlist governs the PR smoke gate. A script + legitimately appears in both — excluded from the full build, required in + smoke — so filtering the list by ``no_run`` would silently delete coverage. + Measured before this was settled: honouring ``no_run`` over the workspaces' + allowlists would have dropped 13 scripts across four repos, every one of + which runs today. """ no_run_list = no_run_list or [] # Infrastructure files — always skip, never report infra_skip = ["__init__", "README"] no_run_list.extend(infra_skip) + files_are_allowlist = files is not None if files is None: files = find_scripts_in_folder(directory) print(f"Found {len(files)} scripts") @@ -886,7 +895,9 @@ def execute_scripts_in_folder(directory, no_run_list=None, report=None, skip_rea for file in files: if file.stem in infra_skip: continue - if should_skip(file, no_run_list): + # The allowlist is the policy when there is one: no_run.yaml governs + # OTHER runs (the release mega-run, notebook generation), not this one. + if not files_are_allowlist and should_skip(file, no_run_list): # Checked BEFORE existence: an excluded script that has also been # deleted is still excluded, and reporting it as a failure would # contradict the exclusion. diff --git a/autohands/run.py b/autohands/run.py index 9320607..1fe32f2 100644 --- a/autohands/run.py +++ b/autohands/run.py @@ -73,16 +73,21 @@ # build target owned its own files, so a missing config is a workspace bug and # is reported as one rather than silently resolving to someone else's rules. no_run_path = WORKSPACE_BUILD_CONFIG / "no_run.yaml" -if not no_run_path.exists(): +if no_run_path.exists(): + with open(no_run_path) as f: + no_run_list = yaml.safe_load(f) or [] +elif args.list_file: + # With an explicit list the allowlist IS the policy, so there is nothing for + # no_run.yaml to filter and its absence is not an error. + no_run_list = [] +else: raise FileNotFoundError( f"{no_run_path} not found. Every workspace must own its " - f"config/build/no_run.yaml (an empty file is valid and skips nothing). " + f"config/build/no_run.yaml (an empty file is valid and skips nothing), " + f"or pass --list to run an explicit set of notebooks. " f"Run from the workspace root, not from PyAutoHands." ) -with open(no_run_path) as f: - no_run_list = yaml.safe_load(f) or [] - if visualise: # A workspace with no visualise_notebooks.yaml has nothing marked for # visualisation; that is not an error, it just selects nothing. @@ -119,7 +124,13 @@ directory=directory, run_type="notebook", ) - skip_reasons = parse_no_run_reasons(no_run_path, project) + # Only when the policy file exists: with an explicit list it may be + # absent, and there are then no skip reasons to parse. + skip_reasons = ( + parse_no_run_reasons(no_run_path, project) + if no_run_path.exists() + else {} + ) env_config = None if env_config_path: diff --git a/autohands/run_python.py b/autohands/run_python.py index 10d3edb..7cdc5b2 100644 --- a/autohands/run_python.py +++ b/autohands/run_python.py @@ -48,14 +48,26 @@ if not no_run_path.exists(): no_run_path = AUTOHANDS_CONFIG / "no_run.yaml" -with open(no_run_path) as f: - no_run_data = yaml.safe_load(f) - -# Support both flat list (workspace) and keyed dict (legacy autohands) -if isinstance(no_run_data, dict): - no_run_list = no_run_data[project] +if no_run_path.exists(): + with open(no_run_path) as f: + no_run_data = yaml.safe_load(f) + # Support both flat list (workspace) and keyed dict (legacy autohands) + if isinstance(no_run_data, dict): + no_run_list = no_run_data.get(project, []) + else: + no_run_list = no_run_data or [] +elif args.list_file: + # With an explicit list the allowlist IS the policy, so there is nothing + # for no_run.yaml to filter and its absence is not an error. Discovery + # still requires it — without it "run everything under this directory" + # has no exclusion policy at all. + no_run_list = [] else: - no_run_list = no_run_data or [] + raise FileNotFoundError( + f"{no_run_path} not found. A discovery run needs an exclusion policy; " + f"pass --list to run an explicit set of scripts instead, or add " + f"config/build/no_run.yaml (an empty file is valid and skips nothing)." + ) # smoke profile: explicit flag > workspace config/build/profile_smoke.yaml > none env_config_path = None @@ -78,7 +90,13 @@ run_type="script", env_profile=(env_config_path.name if env_config_path else "none"), ) - skip_reasons = parse_no_run_reasons(no_run_path, project) + # Only when the policy file exists: with an explicit list it may be + # absent, and there are then no skip reasons to parse. + skip_reasons = ( + parse_no_run_reasons(no_run_path, project) + if no_run_path.exists() + else {} + ) env_config = None if env_config_path: diff --git a/bin/autohands b/bin/autohands index a2a2ec4..db25aa0 100755 --- a/bin/autohands +++ b/bin/autohands @@ -287,8 +287,9 @@ Arguments: --list Notebook list (e.g. a workspace's smoke_notebooks.txt). Given, only the listed notebooks run, in the list's order (opt-in coverage); omitted, notebooks are discovered - recursively (opt-out). no_run.yaml applies either way, and - wins over the list. + recursively (opt-out). no_run.yaml filters DISCOVERY only: + with --list the allowlist is authoritative, since the two are + policy for different runs. --no-write-back Execute a throwaway copy, leaving the committed notebooks untouched. What a PR smoke gate wants; the release pipeline omits it, because there the executed outputs ARE the product. @@ -320,7 +321,9 @@ Arguments: --list Script list (e.g. a workspace's smoke_tests.txt). Given, only the listed scripts run, in the list's order (opt-in coverage); omitted, scripts are discovered recursively under - (opt-out coverage). no_run.yaml applies either way, and wins. + (opt-out coverage). no_run.yaml filters DISCOVERY only: with + --list the allowlist is authoritative, since the two are policy + for different runs (release build vs PR smoke gate). Run from the workspace root. EOF diff --git a/docs/internals.md b/docs/internals.md index eb5741b..8d00abc 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -114,8 +114,8 @@ This workspace is often imported from `/mnt/c/...` and Codex may not be able to All scripts in `autohands/` are run from within a checked-out workspace directory (not from this repo root). They rely on `PYTHONPATH` including the PyAutoHands directory. -- **`run_python.py [--report-dir DIR] [--env-config FILE] [--list FILE]`** — Executes Python scripts in a workspace folder, skipping files listed in `config/no_run.yaml`. Coverage is opt-out by default (recursive discovery); `--list` switches it to opt-in, running exactly the entries of a script list such as a workspace's `smoke_tests.txt`, in that list's own order. `no_run.yaml` applies either way and wins over the list, so an explicitly excluded script stays excluded. A missing list file is an error, never an empty run. -- **`run.py [--visualise] [--report-dir DIR] [--env-config FILE] [--list FILE] [--no-write-back] [--retry-from DIR]`** — Executes Jupyter notebooks in a workspace folder, skipping files in `config/no_run.yaml`. `--list` switches coverage from opt-out discovery to opt-in (a workspace's `smoke_notebooks.txt`, in that list's order); `no_run.yaml` still wins over the list. `--no-write-back` executes a throwaway copy so the committed notebooks stay clean (a PR gate wants this; the release pipeline does not, since there the outputs are the product). `--retry-from ` regenerates a failing notebook from its source `.py` and retries ONCE — the stale-notebook recovery; timeouts are never retried. +- **`run_python.py [--report-dir DIR] [--env-config FILE] [--list FILE]`** — Executes Python scripts in a workspace folder, skipping files listed in `config/no_run.yaml`. Coverage is opt-out by default (recursive discovery); `--list` switches it to opt-in, running exactly the entries of a script list such as a workspace's `smoke_tests.txt`, in that list's own order. `no_run.yaml` filters **discovery only** — with `--list` the allowlist is authoritative, because the two are policy for different runs (`no_run.yaml` for the release mega-run and notebook generation, the allowlist for the PR smoke gate) and a script legitimately appears in both. A missing list file is an error, never an empty run; a missing `no_run.yaml` is an error for discovery but fine under `--list`. +- **`run.py [--visualise] [--report-dir DIR] [--env-config FILE] [--list FILE] [--no-write-back] [--retry-from DIR]`** — Executes Jupyter notebooks in a workspace folder, skipping files in `config/no_run.yaml`. `--list` switches coverage from opt-out discovery to opt-in (a workspace's `smoke_notebooks.txt`, in that list's order); `no_run.yaml` filters discovery only — with `--list` the allowlist is authoritative. `--no-write-back` executes a throwaway copy so the committed notebooks stay clean (a PR gate wants this; the release pipeline does not, since there the outputs are the product). `--retry-from ` regenerates a failing notebook from its source `.py` and retries ONCE — the stale-notebook recovery; timeouts are never retried. - **`generate.py `** — Converts Python scripts in `scripts/` to `.ipynb` notebooks in `notebooks/`, run from within the workspace root - **`generate_markdown.py [--only ] [--optimize-only]`** — Renders the curated scripts listed in the workspace's `config/build/markdown_examples.yaml` to **executed** markdown pages with output images under `markdown/`, plus an index, committed so examples are readable on GitHub. Manual / at-release only, never per-commit; refuses `PYAUTO_TEST_MODE` (truncated searches make wrong images — model-fit reruns instead resume from the completed `output/` cache); never renders `features/` scripts; restores tracked files a script modifies (e.g. simulators rewriting `dataset/`). Extracted figures are optimized on the way out, but only for the render in progress — `--optimize-only` renders nothing and puts already-committed `markdown/**/_files/` PNGs through the same optimizer, for pages built before it shipped. Rules and rationale in the module docstring. - **`script_matrix.py [project2 ...]`** — Outputs a JSON matrix of `{name, directory}` pairs for GitHub Actions matrix strategy @@ -205,7 +205,16 @@ none of those three sweeps, precisely because it holds no logic. The obstacle was never behaviour but *discovery model*: `run_python.py` was opt-out only, while the other two variants are opt-in allowlists. `--list ` closes that gap for the script leg, so the `workspace_test` variant can collapse to a HowTo- -shaped delegator. The notebook leg followed in the same change: +shaped delegator. + +Under `--list` the allowlist is **authoritative** and `no_run.yaml` is not +consulted. The two are policy for different runs — `no_run.yaml` for the release +mega-run and notebook generation, the allowlist for the PR smoke gate — and the +vendored runners have never read `no_run.yaml` at all. Measured before this was +settled: filtering the allowlists by `no_run` would have dropped **13 scripts** +across `autogalaxy_workspace_test` (9), `autolens_workspace_test` (2), +`autofit_workspace` (1) and `autolens_workspace` (1), every one of which runs in +smoke today. The notebook leg followed in the same change: `run.py` gained the matching `--list`, plus `--no-write-back` and `--retry-from`, so the `workspace` variant can collapse too. diff --git a/tests/test_script_list.py b/tests/test_script_list.py index 400744a..565dffd 100644 --- a/tests/test_script_list.py +++ b/tests/test_script_list.py @@ -95,12 +95,19 @@ def test_missing_list_file_is_an_error_not_an_empty_run(workspace): files_from_list("scripts", workspace / "does_not_exist.txt") -def test_no_run_wins_over_the_allowlist(workspace, capsys): +def test_the_allowlist_wins_over_no_run(workspace): """ - An allowlisted script that is also no_run-listed is SKIPPED, with its reason. + An allowlisted script runs even when no_run.yaml also names it. - The explicit exclusion is the more specific statement of intent; letting the - allowlist override it would resurrect a script someone deliberately turned off. + The two files are policy for DIFFERENT runs: no_run.yaml governs the release + mega-run and notebook generation, an allowlist governs the PR smoke gate. A + script legitimately appears in both — excluded from the full build, required + in smoke. + + This is measured, not theoretical. The vendored workspace runners read only + smoke_tests.txt and never open no_run.yaml, and across the four repos that + carry both files 13 allowlisted scripts are also no_run-listed. Filtering the + list by no_run would silently delete every one of them from smoke coverage. """ listing = workspace / "smoke_tests.txt" listing.write_text("top_level.py\na.py\n") @@ -110,10 +117,28 @@ def test_no_run_wins_over_the_allowlist(workspace, capsys): directory="scripts", no_run_list=["a"], report=report, - skip_reasons={"a": "deliberately off"}, + skip_reasons={"a": "excluded from the release build"}, files=files_from_list("scripts", listing), ) + by_name = {Path(r.file).name: r for r in report.results} + assert by_name["a.py"].status == Status.PASSED, ( + "an allowlisted script must run even when no_run.yaml names it" + ) + assert by_name["top_level.py"].status == Status.PASSED + + +def test_no_run_still_filters_discovery(workspace): + """The opt-out path is untouched: without a list, no_run.yaml still skips.""" + report = RunReport(project="p", directory="scripts", run_type="script") + + execute_scripts_in_folder( + directory="scripts", + no_run_list=["a"], + report=report, + skip_reasons={"a": "deliberately off"}, + ) + by_name = {Path(r.file).name: r for r in report.results} assert by_name["a.py"].status == Status.SKIPPED assert by_name["a.py"].skip_reason == "deliberately off" @@ -154,26 +179,25 @@ def test_absent_flag_leaves_discovery_untouched(workspace): assert Path(report.results[0].file).name == "simulator_script.py" -def test_no_run_wins_even_when_the_listed_file_is_missing(workspace): +def test_a_listed_missing_file_fails_even_if_no_run_names_it(workspace): """ - An excluded script that has also been deleted is SKIPPED, not FAILED. + With the list authoritative, a stale entry is a FAIL whatever no_run says. - Pins the check order: reporting a failure for a script someone deliberately - turned off would contradict the exclusion, and would redden a gate over a - file nobody intends to run. + no_run.yaml cannot rescue it: the list is the smoke policy, so an entry + naming a file that does not exist is a broken allowlist and must be visible. """ listing = workspace / "smoke_tests.txt" - listing.write_text("deleted_and_excluded.py\ntop_level.py\n") + listing.write_text("gone_and_excluded.py\ntop_level.py\n") report = RunReport(project="p", directory="scripts", run_type="script") execute_scripts_in_folder( directory="scripts", - no_run_list=["deleted_and_excluded"], + no_run_list=["gone_and_excluded"], report=report, - skip_reasons={"deleted_and_excluded": "retired"}, + skip_reasons={"gone_and_excluded": "retired"}, files=files_from_list("scripts", listing), ) by_name = {Path(r.file).name: r for r in report.results} - assert by_name["deleted_and_excluded.py"].status == Status.SKIPPED - assert by_name["deleted_and_excluded.py"].skip_reason == "retired" + assert by_name["gone_and_excluded.py"].status == Status.FAILED + assert "top_level.py" in by_name