diff --git a/docs/pre_build_failure_audit.md b/docs/pre_build_failure_audit.md index ff10e45..dc793d7 100644 --- a/docs/pre_build_failure_audit.md +++ b/docs/pre_build_failure_audit.md @@ -138,9 +138,27 @@ rejection reasoning, confirmed: the line stages nothing anyone needs). release" vestige should be deleted (recommended — it is #126's mechanism) or kept deliberately. Deleting it makes releases require clean mains, which Heart already checks. -- **Open:** atomicity — a mid-sequence fatal leaves a half-pushed release - surface. Worth a fail-fast pre-pass (all repos validated before any push)? - Costed as a follow-up, not this PR. +- **RESOLVED — atomicity.** Was: "a mid-sequence fatal leaves a half-pushed + release surface. Worth a fail-fast pre-pass (all repos validated before any + push)?" Answered yes and implemented: `pre_build.sh` now walks every repo in + `WORKSPACE_SPECS` before the first is touched, aborting if any checkout is + missing or carries untracked files under the directories the run reformats + and stages (`notebooks/`, `scripts/`, `slam_pipeline/`). + + The trigger was a near-miss during the 2026-08-07 release: `git add /` + stages *untracked* files, so an uncommitted script in a workspace's + `scripts/` would be black-formatted and pushed inside the "pre build" commit + — the same leak class as #126, which §3 fixed for `dataset/` and `config/` + while leaving the `scripts/` path open. It was caught only because the + operator moved the file out by hand. Reproduced against the pre-fix script on + fixture repos: the private file was committed and pushed, exit 0, silently. + + Staging was narrowed in the same change — `git add -u` for tracked edits and + deletions, plus newly created files added by explicit path — so the + directory-wide form that causes this cannot return. Both legs are covered by + `tests/test_pre_build_staging.py`, which runs the real script against + throwaway git fixtures. There is deliberately **no** `--allow-dirty` + override: an override is precisely the operator vigilance this replaces. ## Trust nothing here diff --git a/pre_build.sh b/pre_build.sh index a9a0e09..95f124b 100644 --- a/pre_build.sh +++ b/pre_build.sh @@ -50,6 +50,89 @@ if command -v gh >/dev/null 2>&1; then bash "$PYAUTOBASE/PyAutoBrain/bin/ensure_workspace_labels.sh" fi +# Positional fields: repo project [generate=true] [slam=false] +# Declared as data, not as a call list, because TWO passes read it: the +# uncommitted-work preflight below and the execution loop at the bottom. A +# second hand-maintained list would drift out of step with this one, and the +# preflight would then silently skip a repo it is meant to protect. +# The repo names are checked against PyAutoMind/repos.yaml (the body map) by +# `repos_sync.py --check`; the flags are Build policy and live only here. +# (The former readme_pkg arg / README version bump was deleted per the audit in +# docs/pre_build_failure_audit.md: its sed edit was never staged and the runner +# side was removed under #120. Phase 4 task 4 of the build-chain campaign +# (#155) then resolved the pins themselves: the three surviving ` vX` lines +# were REMOVED from the READMEs in favour of "install the latest release" plus +# the `version.minimum_library_version` floor, which Heart's version_skew check +# actually verifies. Do not re-add a README version bump here or on the runner — +# an unowned pin is what went 2 months stale.) +# The last entry is the AI assistant repo. No notebook generation; release.yml's +# release_workspaces job stamps its workspace version and regenerates +# wiki/core/api_audit_baseline.json against the released wheels. +WORKSPACE_SPECS=( + "autofit_workspace autofit true false" + "autogalaxy_workspace autogalaxy true false" + "autolens_workspace autolens true true" + "autofit_workspace_test autofit false false" + "autogalaxy_workspace_test autogalaxy false false" + "autolens_workspace_test autolens false false" + "euclid_strong_lens_modeling_pipeline - false false" + "HowToGalaxy howtogalaxy true false" + "HowToLens howtolens true false" + "HowToFit howtofit true false" + "autofit_workspace_developer - false false" + "autolens_workspace_developer - false false" + "autolens_assistant autolens false false" +) + +# The directories run_workspace reformats with black and stages. Anything +# untracked under them BEFORE a run is human work, never run output. +MUTATED_DIRS=(notebooks scripts slam_pipeline) + +# Preflight: no workspace may carry uncommitted work under MUTATED_DIRS. +# +# run_workspace both black-formats and `git add`s those directories, and both +# operations reach untracked files — so a human's in-progress script would be +# reformatted on disk and pushed inside the "pre build" commit. That is the +# same leak class as the tracked-dataset leak (#126), which was fixed for +# dataset/ and config/ by dropping their staging; the scripts/ path still had +# the hole, and it was caught by hand during the 2026-08-07 release only +# because the operator happened to notice. +# +# This runs over EVERY repo before the first one is touched, mirroring the +# PyAutoHands gate above. run_workspace commits and pushes each repo before +# moving to the next, so a per-repo check that aborted midway would leave the +# earlier repos already published. +echo "" +echo "=== Checking workspaces for uncommitted work ===" +WIP_REPORT="" +for spec in "${WORKSPACE_SPECS[@]}"; do + # `read` rather than `set --`: this loop runs at top level, where `set --` + # would clobber the script's own positional parameters. + read -r wip_repo _ <<< "$spec" + wip_dir="$PYAUTOBASE/$wip_repo" + # Checked here so a missing checkout fails with a clear message during the + # preflight, rather than as a bare `cd` error partway through the run once + # earlier repos have already been committed and pushed. + if [ ! -d "$wip_dir/.git" ]; then + echo "ABORT: $wip_repo is missing or is not a git repo ($wip_dir)." >&2 + exit 1 + fi + # `ls-files --others` tolerates pathspecs that match nothing (unlike + # `git add`), so the dirs need no per-repo existence guard here. + # `--exclude-standard` honours .gitignore, keeping output/ and friends out. + wip="$(git -C "$wip_dir" ls-files --others --exclude-standard -- "${MUTATED_DIRS[@]}")" + if [ -n "$wip" ]; then + WIP_REPORT="${WIP_REPORT} ${wip_repo}:"$'\n'"$(printf '%s\n' "$wip" | sed 's/^/ /')"$'\n' + fi +done +if [ -n "$WIP_REPORT" ]; then + echo "ABORT: uncommitted work under directories pre_build formats and commits." >&2 + printf '%s' "$WIP_REPORT" >&2 + echo "Commit, stash or move these before releasing — pre_build must not author them." >&2 + exit 1 +fi +echo " Clean: no untracked files under ${MUTATED_DIRS[*]} in any workspace." + run_workspace() { local repo="$1" local project="$2" @@ -88,11 +171,24 @@ run_workspace() { # release commits, which is the mechanism that leaked simulated datasets # (#126). Releases require clean mains (Heart gates on it); human work is # committed by humans. See docs/pre_build_failure_audit.md §3/§6 (#156). + local stage_dirs=() for d in notebooks scripts; do - if [ -d "$d" ]; then git add "$d/"; fi + if [ -d "$d" ]; then stage_dirs+=("$d"); fi done if [ "$slam" = "true" ] && [ -d "slam_pipeline" ]; then - git add slam_pipeline/ + stage_dirs+=("slam_pipeline") + fi + if [ ${#stage_dirs[@]} -gt 0 ]; then + # Tracked edits and deletions: black's reformatting, regenerated and + # retired notebooks. + git add -u -- "${stage_dirs[@]}" + # Plus what this run CREATED — a new notebook from generate.py. The + # preflight proved these directories held no untracked files before the + # run, so anything untracked now is run output. Added by explicit path + # rather than as `git add /`, which also sweeps in untracked files + # and would re-open the hole the preflight closes. + git ls-files --others --exclude-standard -z -- "${stage_dirs[@]}" \ + | xargs -0 --no-run-if-empty git add -- fi # Root-level artifacts (llms-full.txt, workspace_index.json, README Colab # URLs) are produced and committed by release.yml's release_workspaces job @@ -108,33 +204,15 @@ run_workspace() { fi } -# Positional args: repo project [generate=true] [slam=false] -# The repo names are checked against PyAutoMind/repos.yaml (the body map) by -# `repos_sync.py --check`; the flags are Build policy and live only here. -# (The former readme_pkg arg / README version bump was deleted per the audit in -# docs/pre_build_failure_audit.md: its sed edit was never staged and the runner -# side was removed under #120. Phase 4 task 4 of the build-chain campaign -# (#155) then resolved the pins themselves: the three surviving ` vX` lines -# were REMOVED from the READMEs in favour of "install the latest release" plus -# the `version.minimum_library_version` floor, which Heart's version_skew check -# actually verifies. Do not re-add a README version bump here or on the runner — -# an unowned pin is what went 2 months stale.) -run_workspace "autofit_workspace" "autofit" true false -run_workspace "autogalaxy_workspace" "autogalaxy" true false -run_workspace "autolens_workspace" "autolens" true true -run_workspace "autofit_workspace_test" "autofit" false false -run_workspace "autogalaxy_workspace_test" "autogalaxy" false false -run_workspace "autolens_workspace_test" "autolens" false false -run_workspace "euclid_strong_lens_modeling_pipeline" "" false false -run_workspace "HowToGalaxy" "howtogalaxy" true false -run_workspace "HowToLens" "howtolens" true false -run_workspace "HowToFit" "howtofit" true false -run_workspace "autofit_workspace_developer" "" false false -run_workspace "autolens_workspace_developer" "" false false -# The AI assistant repo. No notebook generation; release.yml's -# release_workspaces job stamps its workspace version and regenerates -# wiki/core/api_audit_baseline.json against the released wheels. -run_workspace "autolens_assistant" "autolens" false false +# Execute. Same list the preflight above walked — see WORKSPACE_SPECS for the +# field meanings and the policy notes. +for spec in "${WORKSPACE_SPECS[@]}"; do + # Unquoted on purpose, as in the preflight: the fields are + # whitespace-separated and none contains a space. A no-generate repo carries + # `-` in the project field — word splitting cannot express an empty field, + # and run_workspace never reads project when generate is false. + run_workspace $spec +done # Release readiness (version skew, including the version.txt-ahead crash that # used to be checked here) is now Heart's job, not Build's: PyAutoHands is a diff --git a/skills/pre_build/pre_build.md b/skills/pre_build/pre_build.md index b5c38b9..9c42ee6 100644 --- a/skills/pre_build/pre_build.md +++ b/skills/pre_build/pre_build.md @@ -55,10 +55,11 @@ bash $HOME/Code/PyAutoLabs/PyAutoHands/bin/autohands pre_build The script handles every mechanical step of the pre-build flow: 1. Fails before any side effects unless PyAutoHands is on clean `main`; the run produces no PyAutoHands files and never stages or commits that repository. -2. Ensures the canonical `pending-release` label exists on each release-window repo. -3. For every workspace, runs black on the staged dirs (`scripts/`, `slam_pipeline/`), runs `generate.py` for projects with a notebook target, and stages only what the run itself produced (`notebooks/`, `scripts/`, plus `slam_pipeline/` for `autolens_workspace`). It does not stage `dataset/` or `config/` — nothing in the run modifies them, and sweeping pre-existing human work into release commits was the #126 leak mechanism. Root-level artifacts and README Colab URLs are committed by `release.yml` on the runner, not here. -4. Commits and pushes each workspace (skipping if no changes are staged). -5. Dispatches `gh workflow run release.yml --repo PyAutoLabs/PyAutoHands --field minor_version=`. +2. Sweeps **every** workspace for untracked files under the directories it reformats and stages (`notebooks/`, `scripts/`, `slam_pipeline/`) and aborts, naming each repo and path, if any exist. This runs before the first repo is touched, because the script commits and pushes each workspace before moving to the next — a check that aborted midway would leave earlier repos already published. Remedy is to commit, stash or move the files; there is deliberately no override flag, since an override is exactly the operator vigilance this replaces. +3. Ensures the canonical `pending-release` label exists on each release-window repo. +4. For every workspace, runs black on the staged dirs (`scripts/`, `slam_pipeline/`), runs `generate.py` for projects with a notebook target, and stages only what the run itself produced (`notebooks/`, `scripts/`, plus `slam_pipeline/` for `autolens_workspace`) — tracked edits and deletions via `git add -u`, plus newly created files by explicit path. It never runs `git add /`, which also sweeps in untracked files; that is what committed and pushed a human's uncommitted script during the 2026-08-07 release rehearsal, and is the same leak class as #126. It does not stage `dataset/` or `config/` — nothing in the run modifies them. Root-level artifacts and README Colab URLs are committed by `release.yml` on the runner, not here. +5. Commits and pushes each workspace (skipping if no changes are staged). +6. Dispatches `gh workflow run release.yml --repo PyAutoLabs/PyAutoHands --field minor_version=`. Release-readiness — including the version-skew check that used to run here (`verify_workspace_versions.sh`) — is gated **upstream by PyAutoHeart** diff --git a/tests/test_pre_build_skill.py b/tests/test_pre_build_skill.py index 92f77b5..d8e1c24 100644 --- a/tests/test_pre_build_skill.py +++ b/tests/test_pre_build_skill.py @@ -10,7 +10,11 @@ def test_pre_build_skill_checks_every_executor_repo(): script = (ROOT / "pre_build.sh").read_text() body = (ROOT / "skills" / "pre_build" / "pre_build.md").read_text() - executor_repos = set(re.findall(r'^run_workspace "([^"]+)"', script, re.MULTILINE)) + # Repos come from the WORKSPACE_SPECS array — the single list that both the + # uncommitted-work preflight and the execution loop read. + executor_repos = set( + re.findall(r'^\s+"(\S+)\s+\S+\s+\S+\s+\S+"', script, re.MULTILINE) + ) fixed_dependencies = set(re.findall(r'\$PYAUTOBASE/([^/"$]+)', script)) preflight = body.split("Check that all required repositories exist", 1)[1] preflight = preflight.split("For each, verify", 1)[0] @@ -27,3 +31,33 @@ def test_pre_build_guards_pyautohands_instead_of_staging_it(): assert 'if [ "$HANDS_BRANCH" != "main" ] || [ -n "$HANDS_STATUS" ]' in script assert script.index(guard) < script.index("=== Ensuring pending-release labels ===") assert "git add -A" not in script + + +def test_pre_build_never_stages_a_directory(): + """`git add /` also stages untracked files. + + That is how a human's uncommitted script was reformatted and pushed inside + a "pre build" commit during the 2026-08-07 release. Staging must name the + tracked set (`git add -u`) and add created files by explicit path. + """ + script = (ROOT / "pre_build.sh").read_text() + + assert not re.search(r"git add\s+[\"']?\$?\w+/", script) + assert 'git add -u -- "${stage_dirs[@]}"' in script + + +def test_pre_build_wip_preflight_precedes_every_mutation(): + """The preflight must sweep all repos before the first is touched. + + run_workspace commits and pushes each workspace before moving to the next, + so a per-repo check that aborted midway would leave earlier repos already + published. + """ + script = (ROOT / "pre_build.sh").read_text() + + preflight = script.index("=== Checking workspaces for uncommitted work ===") + assert preflight < script.index("run_workspace() {") + # The invocations themselves, not the words — both appear in prose above. + assert preflight < script.index('black "$d/"') + assert preflight < script.index("git add -u --") + assert "ABORT: uncommitted work" in script diff --git a/tests/test_pre_build_staging.py b/tests/test_pre_build_staging.py new file mode 100644 index 0000000..d30b434 --- /dev/null +++ b/tests/test_pre_build_staging.py @@ -0,0 +1,223 @@ +"""End-to-end behaviour tests for `pre_build.sh`'s staging and WIP preflight. + +`test_pre_build_skill.py` asserts against the *text* of the script. That cannot +prove the preflight actually fires or that staging is exact, so these tests run +the real script against a throwaway PYAUTOBASE of fixture git repos, with +`black`, `python`/`python3` and `gh` stubbed on PATH. Nothing real is touched +and nothing is pushed anywhere but a local bare remote. + +The behaviour under test is the 2026-08-07 near-miss: `git add /` also +stages *untracked* files, so a human's in-progress script in a workspace's +`scripts/` would be reformatted by black and pushed inside the "pre build" +commit. +""" + +import os +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + + +ROOT = Path(__file__).resolve().parents[1] +PRE_BUILD = ROOT / "pre_build.sh" + +# The repos pre_build walks, parsed from the script so this fixture cannot +# drift away from the executor the way a second hand-written list would. +SPECS = re.findall( + r'^\s+"(\S+)\s+(\S+)\s+(\S+)\s+(\S+)"', PRE_BUILD.read_text(), re.MULTILINE +) + + +def _git(repo, *args): + return subprocess.run( + ["git", "-C", str(repo), *args], + check=True, + capture_output=True, + text=True, + ).stdout + + +def _init_repo(path): + path.mkdir(parents=True, exist_ok=True) + _git(path, "init", "-q", "-b", "main", ".") + _git(path, "config", "user.email", "fixture@example.com") + _git(path, "config", "user.name", "fixture") + return path + + +def _commit_all(repo, message): + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", message) + + +@pytest.fixture +def base(tmp_path): + """A throwaway PYAUTOBASE: PyAutoHands + every workspace pre_build walks.""" + assert SPECS, "failed to parse WORKSPACE_SPECS out of pre_build.sh" + + pyautobase = tmp_path / "PyAutoLabs" + + # PyAutoHands itself, holding the script under test. pre_build requires it + # on a clean main before it will do anything. + hands = _init_repo(pyautobase / "PyAutoHands") + (hands / "autohands").mkdir() + shutil.copy(PRE_BUILD, hands / "pre_build.sh") + _commit_all(hands, "fixture hands") + + for repo, _project, _generate, _slam in SPECS: + work = _init_repo(pyautobase / repo) + (work / "scripts").mkdir() + (work / "scripts" / "committed.py").write_text("x = 1\n") + (work / "notebooks").mkdir() + (work / "notebooks" / "committed.ipynb").write_text("{}\n") + _commit_all(work, "fixture workspace") + + # A real bare remote, so the script's `git push` is exercised rather + # than stubbed away. + remote = tmp_path / "remotes" / f"{repo}.git" + remote.parent.mkdir(exist_ok=True) + subprocess.run( + ["git", "init", "-q", "--bare", str(remote)], check=True + ) + _git(work, "remote", "add", "origin", str(remote)) + _git(work, "push", "-q", "-u", "origin", "main") + + return pyautobase + + +@pytest.fixture +def stub_bin(tmp_path): + """Stubs for the external commands pre_build shells out to.""" + bindir = tmp_path / "stubbin" + bindir.mkdir() + + # black must be a no-op that still exits 0. `gh` must not dispatch a real + # release workflow. `python`/`python3` stand in for generate.py and + # check_dataset_allowlist.py; generate.py is asked to create a notebook so + # the "new file this run produced" path is covered. + (bindir / "black").write_text("#!/bin/bash\nexit 0\n") + (bindir / "gh").write_text("#!/bin/bash\nexit 0\n") + stub_python = ( + "#!/bin/bash\n" + 'case "$*" in\n' + " *generate.py*) mkdir -p notebooks && " + 'printf "{}\\n" > notebooks/generated.ipynb ;;\n' + "esac\n" + "exit 0\n" + ) + for name in ("python", "python3"): + (bindir / name).write_text(stub_python) + + for f in bindir.iterdir(): + f.chmod(0o755) + return bindir + + +def _run(base, stub_bin): + env = dict(os.environ) + env["PATH"] = f"{stub_bin}:{env['PATH']}" + # pre_build only runs ensure_workspace_labels.sh when gh is on PATH; it is + # stubbed, so give it a real (no-op) script to find. + brain_bin = base / "PyAutoBrain" / "bin" + brain_bin.mkdir(parents=True, exist_ok=True) + labels = brain_bin / "ensure_workspace_labels.sh" + labels.write_text("#!/bin/bash\nexit 0\n") + labels.chmod(0o755) + + return subprocess.run( + ["bash", str(base / "PyAutoHands" / "pre_build.sh")], + capture_output=True, + text=True, + env=env, + cwd=str(base), + ) + + +def test_untracked_wip_aborts_before_anything_is_touched(base, stub_bin): + """A human's uncommitted script blocks the release and is left alone.""" + victim = base / "autolens_assistant" / "scripts" / "wip_private.py" + victim.write_text("secret = 1\n") + original = victim.read_bytes() + + result = _run(base, stub_bin) + + assert result.returncode != 0, result.stdout + assert "ABORT: uncommitted work" in result.stderr + # Names the repo and the exact path, so one run surfaces the whole problem. + assert "autolens_assistant" in result.stderr + assert "scripts/wip_private.py" in result.stderr + + # Untouched on disk: the abort precedes black. + assert victim.read_bytes() == original + + # And nothing was committed anywhere — the abort precedes the first repo. + for repo, *_ in SPECS: + log = _git(base / repo, "log", "--oneline") + assert "pre build" not in log + + +def test_wip_in_any_repo_is_reported_together(base, stub_bin): + """The preflight sweeps every repo before aborting, not just the first.""" + (base / SPECS[0][0] / "scripts" / "a_wip.py").write_text("a = 1\n") + (base / SPECS[-1][0] / "notebooks" / "z_wip.ipynb").write_text("{}\n") + + result = _run(base, stub_bin) + + assert result.returncode != 0 + assert SPECS[0][0] in result.stderr + assert SPECS[-1][0] in result.stderr + + +def test_gitignored_files_do_not_block_a_release(base, stub_bin): + """`--exclude-standard` keeps output/ and other ignored cruft out.""" + work = base / "autolens_workspace" + (work / ".gitignore").write_text("scripts/scratch/\n") + _commit_all(work, "fixture gitignore") + (work / "scripts" / "scratch").mkdir() + (work / "scripts" / "scratch" / "junk.py").write_text("junk = 1\n") + + result = _run(base, stub_bin) + + assert result.returncode == 0, result.stderr + + +def test_generated_notebooks_are_still_staged(base, stub_bin): + """The narrower staging must not drop genuinely new run output.""" + result = _run(base, stub_bin) + assert result.returncode == 0, result.stderr + + # generate.py runs for the repos flagged generate=true; each must have + # committed the notebook the stub created. + generating = [repo for repo, _p, generate, _s in SPECS if generate == "true"] + assert generating, "fixture expects at least one generate=true repo" + for repo in generating: + tracked = _git(base / repo, "ls-files", "notebooks") + assert "notebooks/generated.ipynb" in tracked, repo + assert "pre build" in _git(base / repo, "log", "--oneline") + + +def test_missing_checkout_aborts_in_the_preflight(base, stub_bin): + """A missing repo fails clearly up front, not as a bare `cd` error midway.""" + shutil.rmtree(base / "HowToFit") + + result = _run(base, stub_bin) + + assert result.returncode != 0 + assert "HowToFit is missing or is not a git repo" in result.stderr + # Nothing was published before the failure was noticed. + assert "pre build" not in _git(base / "autofit_workspace", "log", "--oneline") + + +def test_tracked_deletions_are_staged(base, stub_bin): + """`git add -u` must carry a retired notebook's deletion into the commit.""" + work = base / "autolens_workspace" + (work / "notebooks" / "committed.ipynb").unlink() + + result = _run(base, stub_bin) + assert result.returncode == 0, result.stderr + + assert "notebooks/committed.ipynb" not in _git(work, "ls-files", "notebooks") + assert "pre build" in _git(work, "log", "--oneline")