|
| 1 | +# pre_build.sh failure-class audit — and what its local commit is for |
| 2 | + |
| 3 | +**Status:** audit deliverable for PyAutoBuild#156 (build-chain campaign #155, Phase 1). |
| 4 | +**Method:** every claim below is measured (2026-07-16), read-only (`git add -n`, |
| 5 | +sed-to-stdout, `black --check`, origin-verified), against the real repos — both |
| 6 | +`.gitignore` dialect groups. Re-measure before relying on any number here. |
| 7 | + |
| 8 | +## 1. The verified map (division of labour, as it actually is) |
| 9 | + |
| 10 | +What actually modifies files during a `pre_build.sh` run, and who commits what: |
| 11 | + |
| 12 | +| artifact | modified by (local) | staged by (local) | committed on the runner | net owner | |
| 13 | +|---|---|---|---|---| |
| 14 | +| `scripts/**/*.py` formatting | `black .` | `git add scripts/` | never | **pre_build (genuine)** | |
| 15 | +| notebooks (`*.ipynb`) | `generate.py` | `git add notebooks/` | regenerated + committed explicitly (`release_workspaces` → `git add *.ipynb`) | runner (local copy redundant) | |
| 16 | +| `llms-full.txt`, `workspace_index.json` | `generate.py` (root) | **never** — `:88` is dead (§2) | swept by `git add -A` inside the *"bump Colab URL tag refs"* commit | **unowned — rides along, mislabeled** | |
| 17 | +| README Colab URLs | not touched locally | — | `bump_colab_urls.sh` + the same `git add -A` | runner | |
| 18 | +| README version pin (`<pkg> vX`) | `:55` sed (edits where a pin exists) | **never** — `:88` is dead | **nothing** — the runner-side step was deliberately removed (#120/#121; see the comment in `release.yml`) | **orphaned** | |
| 19 | +| `dataset/`, `config/` | nothing in the run | `git add dataset|config/` | never | vestigial (stages only pre-existing human dirt) | |
| 20 | +| black collateral outside staged dirs | `black .` | never (no rule covers them) | never | perpetual churn (§4) | |
| 21 | + |
| 22 | +**Proven consequence of the orphaned README pin** (origin-verified): after the |
| 23 | +2026.7.15.1 release, `autofit_workspace` README.md pins `PyAutoFit v2026.7.9.1` |
| 24 | +(the 07-13 hand-bump value); `autofit_workspace_test` and `autolens_workspace_test` |
| 25 | +pin `v2026.5.14.1` (~2 months stale). The other 7 repos passed a `readme_pkg` |
| 26 | +but have **no pin pattern in README.md at all** — for them the banner + sed are |
| 27 | +semantically empty on every run. The `"Bumping README version → …"` banner has |
| 28 | +printed a false claim, 13/13 repos, on every release for months. |
| 29 | + |
| 30 | +## 2. Complete enumeration of the failure class |
| 31 | + |
| 32 | +The shape: a command whose failure is handled wrongly in one of two opposite |
| 33 | +directions — fatal under `set -e` when a no-op is legitimate, or swallowed by |
| 34 | +`|| true`/`2>/dev/null`/ignored return when a real failure matters. |
| 35 | + |
| 36 | +| site | direction | measured (13 repos) | |
| 37 | +|---|---|---| |
| 38 | +| `:55` `sed … README.rst README.md 2>/dev/null \|\| true` | swallow | exit **2 in 13/13** (no repo has `README.rst`); `\|\| true` permanently load-bearing; real sed failure indistinguishable | |
| 39 | +| `:79` `git add dataset/ 2>/dev/null \|\| true` | swallow (post-#154) | exit 1 in 5 repos (whole-dir ignore), 0 in 4 (allowlist dialect), nodir in 4 — correct direction *today*, but a real failure (index.lock, perms) is also silent | |
| 40 | +| `:82` `[ -d "$d" ] && git add "$d/"` (config/notebooks/scripts) | **fatal, latent** | exit 0 wherever present today; a present-but-fully-ignored dir = bug-1 redux, kills the release | |
| 41 | +| `:88` root-glob `git add -- *.py *.md … 2>/dev/null \|\| true` | swallow | exit **128 in 13/13** — a **global no-op**. Six patterns (`*.cfg *.ini *.toml *.yml *.yaml setup*`) match nothing in ANY repo; each unmatched glob stays literal and poisons the whole pathspec list | |
| 42 | +| `:50` `black .` | fatal | plus unstaged collateral (§4) | |
| 43 | +| `:60` `generate.py` | fatal | pre_build calls it with `report=None` → any single script conversion error raises and kills the release | |
| 44 | +| `generate.py:107` `os.system(f"git add -f {notebook}")` | **ignored return** | a hidden 4th tolerant site, in a callee, using `-f` — staging responsibility is smeared across two files | |
| 45 | +| `:125` `git add -A` on PyAutoBuild, current branch | — | commits onto whatever branch is checked out (documented trap) | |
| 46 | +| `:142` `gh workflow run` | fatal at the end | after 14 repos already pushed — no atomicity anywhere: any mid-sequence fatal leaves earlier repos released-committed and later repos untouched | |
| 47 | + |
| 48 | +Original brief's "three sites, three hits" **understated it**: with `:82`'s |
| 49 | +latent class, `generate.py:107`, and the atomicity gap, the file has ~6 members |
| 50 | +of the class, and the two live "safety nets" (`:55`, `:88`) are both dead weight |
| 51 | +(one masks an orphaned artifact, one is a global no-op). |
| 52 | + |
| 53 | +## 3. The design question, answered per artifact |
| 54 | + |
| 55 | +**"If release.yml regenerates and commits these artifacts on the runner anyway, |
| 56 | +what is pre_build's local commit for?"** — it decomposes; the aggregate answer |
| 57 | +is *mostly redundant, one genuine product, one broken orphan*: |
| 58 | + |
| 59 | +- **Genuine:** committing black's reformat of `scripts/` (and `slam_pipeline/`). |
| 60 | + This is the only artifact the run itself produces that only the local commit |
| 61 | + ships. (The runner regenerates notebooks *from the pushed scripts*, so the |
| 62 | + formatting does reach notebooks via the runner.) |
| 63 | +- **Redundant:** notebooks staging — the runner rebuilds and commits them. |
| 64 | +- **Broken/unowned:** everything on the root-glob line. `llms-full.txt` / |
| 65 | + `workspace_index.json` reach main only as **accidental `git add -A` sweep** |
| 66 | + inside the runner's Colab commit (works only for `bump_colab_urls: true` |
| 67 | + repos — autofit/ag/al workspace + HowTo*; euclid + assistants opted out, and |
| 68 | + correspondingly `generate_notebooks: false` there, so nothing goes stale). |
| 69 | + The README version pin reaches main **never** (§1). |
| 70 | +- **Vestigial:** `dataset/` and `config/` staging — nothing in the run modifies |
| 71 | + them; they exist to sweep pre-existing human-uncommitted work into a release |
| 72 | + commit. That "feature" is how #126 leaked simulated datasets; the organism |
| 73 | + has been deliberately retiring it (#150). |
| 74 | + |
| 75 | +## 4. Black collateral (measured) |
| 76 | + |
| 77 | +`black --check` on current mains: **13 files** are reformatted by every release |
| 78 | +and never staged by any rule — `.github/scripts/check_tutorials_complete.py` in |
| 79 | +each HowTo repo (3) and 10 `autoassistant/*.py` files in `autolens_assistant`. |
| 80 | +They churn dirty-local every release until a clean-slate wipes them, forever. |
| 81 | +Answer to the brief's open question: this is a **third instance of the class** |
| 82 | +(work done, claim implied, result dropped), not intended behaviour. |
| 83 | + |
| 84 | +## 5. Target design (proposal — argue with it) |
| 85 | + |
| 86 | +Principles the enumeration supports: a no-op and a failure must be different |
| 87 | +events; a log line must not claim work that did not happen; one side owns each |
| 88 | +artifact; if the runner is the real producer, the local script stops pretending. |
| 89 | + |
| 90 | +**Recommended shape (the #47 / delete-the-trap outcome), as three landable steps:** |
| 91 | + |
| 92 | +1. **Delete the dead lines** (no behaviour change, measured): the `:88` root |
| 93 | + glob (a 13/13 no-op — deleting it changes nothing on main) and the `:55` |
| 94 | + README bump (its artifact is orphaned; deleting the *false signal* beats |
| 95 | + keeping a banner that lies). Decide the README pin's fate in Phase 4 of the |
| 96 | + campaign (`release_version_sync_back_to_main.md`): either the runner owns it |
| 97 | + again (one sed in `release_workspaces`, next to the Colab bump, committed |
| 98 | + explicitly) or the pins come out of the READMEs in favour of "install the |
| 99 | + latest release" + floors. **Either way the owner is the runner or nobody — |
| 100 | + not a local sed whose output nothing stages.** |
| 101 | +2. **Give the swept artifacts an owner:** in `release_workspaces`, replace the |
| 102 | + Colab step's `git add -A` with explicit paths (`llms-full.txt`, |
| 103 | + `workspace_index.json`, README, notebooks dir) and an honest commit message. |
| 104 | + `git add -A` on a runner checkout is the same unowned-sweep idiom that made |
| 105 | + bug 2 invisible for months. |
| 106 | +3. **Shrink pre_build to what it genuinely produces:** black + generate + |
| 107 | + allowlist check + stage the named dirs the run actually modified |
| 108 | + (`scripts/`, `slam_pipeline/`, `notebooks/` if kept) + explicit |
| 109 | + "matched nothing (expected)" vs "failed" handling on the dataset add. Move |
| 110 | + the black-collateral files inside a staged rule or stop black'ing them |
| 111 | + (black a fixed dir list, not `.`). Fix `generate.py:107` to use a checked |
| 112 | + call, not `os.system`, and decide deliberately whether `-f` is needed. |
| 113 | + |
| 114 | +**Not proposed:** `shopt -s nullglob` on `:88` — it would make a redundant line |
| 115 | +silently succeed, which is the trap wearing a new hat (the brief's own |
| 116 | +rejection reasoning, confirmed: the line stages nothing anyone needs). |
| 117 | + |
| 118 | +## 6. Rejected / open decisions for the human |
| 119 | + |
| 120 | +- **Rejected: hardening every line with better error handling.** The class |
| 121 | + keeps reappearing because the file does jobs it doesn't own; fewer jobs beats |
| 122 | + more armour, and 2 of its 3 "safety nets" were dead weight on measurement. |
| 123 | +- **Open (Phase 4 fork):** where the README version pin should live, if |
| 124 | + anywhere. This audit only proves nobody owns it today. |
| 125 | +- **Open:** whether pre_build's "sweep human dirt in dataset/config into the |
| 126 | + release" vestige should be deleted (recommended — it is #126's mechanism) or |
| 127 | + kept deliberately. Deleting it makes releases require clean mains, which |
| 128 | + Heart already checks. |
| 129 | +- **Open:** atomicity — a mid-sequence fatal leaves a half-pushed release |
| 130 | + surface. Worth a fail-fast pre-pass (all repos validated before any push)? |
| 131 | + Costed as a follow-up, not this PR. |
| 132 | + |
| 133 | +## Trust nothing here |
| 134 | + |
| 135 | +Written by an agent, same day as the measurements. Three instrument errors were |
| 136 | +made *during this audit* and caught only by re-checking (a piped `$?` that read |
| 137 | +`head`'s exit status instead of grep's, twice, and a `black --check -q` whose |
| 138 | +silence was mistaken for cleanliness). Every table above is cheap to re-measure |
| 139 | +and the commands are stated — re-measure before acting. |
0 commit comments