Make the quality gates check facts, not prose - #21
Merged
Merged
Conversation
The gates had drifted from checking invariants into freezing the current implementation, and several of them could not fail. Delete the checks that asserted authoring choices: README and homepage copy frozen as Python literals, a wordmark check whose two branches both reported an error, and the per-chapter counts (exactly one .tex, exactly three figure PDFs) that froze the current 14 chapters. Keep the invariants that can break: every .tex has its compiled PDF beside it, every notebook parses, every TOC entry resolves, every local link resolves, language attributes match. Fix the checks that judged at the wrong layer: - check_tex ran its regexes over raw TeX, so the template's commented-out example was a false positive and the fix had been a filename exemption (`fig1_xxx`). Strip comments first, counting preceding backslashes so `\%` stays a literal, and drop the exemption. - check_bilingual only enumerated English assets, so a Chinese asset with a missing English twin was never looked at. Pair every asset with its twin. - check_site re-derived the Chinese-page rule that breakrl_locale owns, and the language toggle kept a 120-line page map by hand where the repository already enforces a naming convention. Both now read the one rule. - prepare_site_assets kept the file suffix and the base64 flag in two tables. Pin the Colab bootstrap to the release tag that ships it, replacing the hand-maintained `?v=2` counter, and stop importing repo-internal helpers in a module delivered alone by raw URL into an empty runtime: runpy does not add the script's directory to sys.path, so the import failed exactly where it mattered. That import is the only notebook change: 1 line per notebook, 28 files. sync_colab_bootstrap keeps its in-place text edit. An nbformat round-trip is content-preserving but rewrites 1178 lines across 20 of 28 notebooks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The build emitted `<html lang="zh_CN">` on every Chinese page. That is a Python locale name, not a BCP 47 tag, and Sphinx writes the value into `<html lang>` and `docsearch:language` verbatim. Two consequences followed. `check_site` listed `zh_CN` as an accepted value, so the gate could not see it. The language toggle derived each page's own language from the filename instead of reading the tag the build wrote, so on a built page it disagreed with the document and read Chinese pages as English. The build now writes `zh-CN` from one constant in breakrl_locale, the gate accepts only real tags, and the toggle reads `<html lang>` rather than answering the same question a second way. Chapter PDFs were linked as `github.com/.../blob/main/...` so readers left the site to read them. Link them at the paths the book builds instead. Jupyter Book rewrites and copies files that markdown links point at, so nothing has to be mirrored by hand. The embedded-PDF copy step kept a hand-written pair of paths for the one chapter that uses `<object data=...>`. Jupyter Book copies neither those nor their targets, so the list would silently rot the moment another chapter embedded a PDF. Read the targets out of the page markup instead. Two facts that lived only in prose now have gates: every text page the toggle pairs by hand has a book source, and the entry path still links the minimum demo, Failure Atlas #8, and the Offline RL chapter. Each check asks for link targets and leaves the wording to the author. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sync_colab_bootstrap found the first code cell by searching for the literal text of a code-cell opening brace, located its closing brace with a hand-written string-aware bracket matcher, then reformatted a cell dict back into 2-space-indented JSON at the right character offset. All of that work exists to avoid parsing the notebook. Read and write it as a notebook instead: a read, a source assignment, and a write. nbformat is already a dependency of the two gates that validate these files, so this removes code rather than adding a dependency. One trap the old path did not have to face. nbformat.write opens the target with newline translation on, so writing by path turns every line of the notebook into CRLF on Windows — 809 lines in a single file. Write through an explicit newline="\n" handle instead. A notebook already carrying the wanted cell is still not written at all, so the script stays idempotent and a no-op run leaves the tree untouched. Verified on the committed notebooks: all 28 report unchanged and git status stays empty. The churn this could have caused is real — an nbformat round-trip reindents 1178 lines across 20 of the 28 — but the short-circuit keeps it from ever landing. Both write paths behave as before, checked against a copy of a real notebook: a stale bootstrap is replaced in place, keeping its cell id, its position and its neighbours; a missing one is inserted before the first code cell with a fresh id and the remove-cell tag. Notebooks keep LF endings either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2d2225932
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| REPO_URL = f"https://github.com/{GITHUB_REPO}.git" | ||
| RAW_SETUP_URL = ( | ||
| f"https://raw.githubusercontent.com/{GITHUB_REPO}/main/scripts/colab_setup.py" | ||
| f"https://raw.githubusercontent.com/{GITHUB_REPO}/{RELEASE_TAG}/scripts/colab_setup.py" |
There was a problem hiding this comment.
Every regenerated notebook now downloads this pinned URL, but the v1.2.2 version of scripts/colab_setup.py imports the repository-only paths module at load time. In a fresh Colab, only /content/_breakrl_colab_setup.py exists and the repository has not yet been cloned, so runpy.run_path raises ModuleNotFoundError before setup() can run. The new standalone test copies the current working-tree helper rather than the pinned artifact, so it misses this mismatch; pin to a revision containing the standalone-import fix.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Three commits that make the same point from three directions: the quality gates checked prose and froze the current implementation, while missing facts that could actually break. Fixing that exposed two real defects.
The gates were checking the wrong things
check_flagship_ringfroze README marketing copy as Python literals — editing a sentence required editing a script.check_flagship_lookhad a branch that reported an error whether or not the file existed.check_consistencyasserted "exactly one.tex, exactly three figure PDFs, exactly one notebook" per chapter, which is a description of the current 14 chapters rather than an invariant. A JS test asserted the source text of the toggle, so an equivalent refactor would fail it.Those are gone. What stayed is what can actually break: every
.texhas its compiled PDF beside it, every notebook parses, every TOC entry resolves, every local link resolves, language attributes match.Three checks were judging at the wrong layer and were repaired rather than deleted:
check_texran its regexes over raw TeX, so a commented-out\includegraphicsin the template was a false positive and the fix had been a filename exemption (fig1_xxx). Strip comments first, counting preceding backslashes so\%stays a literal, and drop the exemption.check_bilingualonly enumerated English assets, so a Chinese asset with a missing English twin was never examined.check_sitere-derived a rulebreakrl_localealready owned, andprepare_site_assetskept the file suffix and the base64 flag in two tables.What that exposed
Every Chinese page was built with
<html lang="zh_CN">. That is a Python locale name, not a BCP 47 tag, and Sphinx writes it into<html lang>anddocsearch:languageverbatim. Two things hid it: the gate listedzh_CNas an accepted value, and the language toggle derived each page's language from its filename instead of reading the tag the build emitted. On a real page the toggle therefore disagreed with the document and reported Chinese pages as English. The build now writeszh-CNfrom one constant, the gate accepts only real tags, and the toggle reads<html lang>.Readers left the site to open a chapter PDF. 13 table rows linked
github.com/.../blob/main/...pdf. They now point at the paths the book builds. Jupyter Book rewrites and copies files that markdown links point at, so nothing is mirrored by hand — and the embedded-PDF step now reads its<object data=...>targets out of the page markup instead of keeping a hand-written pair of paths for the one chapter that uses them.sync_colab_bootstrapfound the first code cell by searching for the literal text of a JSON brace, then matched brackets by hand, then re-indented a cell dict back into the right character offset — all to avoid parsing the notebook. It reads and writes notebooks withnbformatnow. One trap that comes with it:nbformat.write(nb, path)opens the target with newline translation on, so writing by path turns every line into CRLF on Windows. It writes through an explicitnewline="\n"handle.Two facts that lived only in prose now have gates: every text page the toggle pairs by hand has a book source, and the reader's entry path still links the minimum demo, Failure Atlas #8, and the Offline RL chapter. Both ask for link targets and leave the wording to the author.
Verification
All four gates pass:
check_consistency(14 chapters, 28 notebooks, 29 TeX files, 42 figures),test_colab_setup,test_lang_toggle(17 explicit pairs),check_site(37 HTML pages).Beyond the gates:
enon a Chinese page redirects to the English edition preserving the query string; storedzhon/lands on/index-zh.html; the unpaireddemo.htmlneither navigates nor claims a language. Each new gate was mutation-tested — reintroduce the defect and confirm it fails.nbformatwrite paths, which the committed notebooks never reach: a stale cell is replaced in place keeping its id, position and neighbours' outputs; a missing one is inserted before the first code cell with a fresh id and theremove-celltag.sync_colab_bootstrapleavesgit statusempty. Annbformatround-trip of these notebooks reindents 1178 lines across 20 of the 28, but the write-only-if-changed path means that churn never lands.jupyter-book clean --all:book/_staticand the build's_staticmatch by SHA-256 on all 10 files; sampledlangattributes are correct; no github blob PDF links remain; 26 chapter PDFs and 2 embedded PDFs are present in the build.Residual risk
_downloads/<hash>/dqn_en.pdf), so regenerating a PDF changes its URL. In-site navigation always points at the current hash, and the site previously offered no stable on-site PDF URL to break, but external deep links to a chapter PDF would not survive a regeneration._is_bootstrapis still a prefix heuristic — a cell starting withBREAKRL_CHAPTER =is treated as the bootstrap and wholly replaced. Unchanged from before.codex/v2-rebuildbranches are untouched.🤖 Generated with Claude Code