diff --git a/.github/workflows/pkgdown.yaml b/.github/workflows/pkgdown.yaml index bfc9f4d..a7e9d11 100644 --- a/.github/workflows/pkgdown.yaml +++ b/.github/workflows/pkgdown.yaml @@ -25,6 +25,15 @@ jobs: steps: - uses: actions/checkout@v4 + # pkgdown renders EVERY root-level .md except a hardcoded allowlist (README, + # LICENSE, NEWS and two GitHub templates), and there is no config option to + # exclude one. `.Rbuildignore` does not help — it governs `R CMD build`, not the + # docs site. Removing the file before the build is the only thing that stops all + # three copies being produced: the rendered page, a verbatim copy of the source, + # and the full-text search index. + - name: Keep internal notes out of the published site + run: rm -f CLAUDE.md + - uses: r-lib/actions/setup-pandoc@v2 - uses: r-lib/actions/setup-r@v2 @@ -40,10 +49,73 @@ jobs: run: pkgdown::build_site_github_pages(new_process = FALSE, install = FALSE) shell: Rscript {0} + # The step above stops the file being built. This stops the NEXT one being built + # without anyone noticing — a root-level .md added a year from now would otherwise + # publish silently, which is exactly how this went unnoticed here. + # + # Add a name to `allowed` only after deciding the page should be public. An entry + # here is a decision, not a formality. + - name: Fail if an unexpected page reached the site + run: | + set -eu + allowed="404 authors index LICENSE LICENSE-text" + + # An empty result set is not a pass. A loop over nothing exits 0, so a build + # that produced no pages at all would report "all declared" — the guard + # asserting success about work that never happened. Check the population + # exists before checking what is in it. + [ -f docs/index.html ] || { echo "docs/index.html missing — the build produced no site"; exit 1; } + + # Exact token match. `case " $allowed " in *" $b "*)` reads equivalent and is + # not: it matches a contiguous SUBSTRING of the list, so a page named + # "authors index" passes. That fails toward pass, which is the direction that + # matters in a guard. + is_allowed() { + for a in $allowed; do + if [ "$a" = "$1" ]; then return 0; fi + done + return 1 + } + + # Both extensions. pkgdown writes a rendered .html AND a markdown twin for + # every root page, so checking only one leaves the other free to leak. Strip + # the suffix that actually matched — stripping both unconditionally reduces + # "index.md.html" to "index". + unexpected="" + for f in docs/*.html docs/*.md; do + [ -e "$f" ] || continue + b=$(basename "$f") + case "$f" in + *.html) b="${b%.html}" ;; + *.md) b="${b%.md}" ;; + esac + if is_allowed "$b"; then continue; fi + seen=0 + for u in $unexpected; do + if [ "$u" = "$b" ]; then seen=1; break; fi + done + if [ "$seen" = "0" ]; then unexpected="$unexpected $b"; fi + done + + if [ -n "$unexpected" ]; then + echo "Unexpected root pages reached the site:$unexpected" + echo "If one of these should be public, add it to \`allowed\` in this workflow." + exit 1 + fi + echo "Root pages are all declared." + - name: Deploy to GitHub pages 🚀 if: github.event_name != 'pull_request' uses: JamesIves/github-pages-deploy-action@v4.5.0 with: - clean: false + # `clean: false` never deletes, so every file ever deployed stays on gh-pages + # whether or not the source still produces it. Removing CLAUDE.md from the + # build would leave the published copies untouched. With `clean: true` the + # site equals what the build produced, which is also what makes it auditable. + # + # Checked before flipping: no CNAME on gh-pages (the domain comes from the org + # site repo), no dev/ versioned docs, and docs/ is untracked so nothing on the + # branch is a hand-added asset. + clean: true branch: gh-pages folder: docs diff --git a/planning/archive/2026-08-issue-42-pkgdown-claude-md/README.md b/planning/archive/2026-08-issue-42-pkgdown-claude-md/README.md new file mode 100644 index 0000000..6830fbf --- /dev/null +++ b/planning/archive/2026-08-issue-42-pkgdown-claude-md/README.md @@ -0,0 +1,21 @@ +# fly#42 — keep CLAUDE.md off the pkgdown site + +No PWF: one workflow file. Kept for the review, because the interesting part is that the +guard shipped in a weaker form than it read. + +`rm -f CLAUDE.md` alone would not have worked — the deploy ran `clean: false`, so the +action never deletes and the published copies would have stayed put while the change +claimed to have removed them. Three parts were needed: pre-build removal, an allowlist +gate, and `clean: true`. + +The gate went through two rounds. The first draft looped over `docs/*.html` and exited 0 +on an empty `docs/` — an affirmative claim of success about a build that produced nothing. +The second used `case " $allowed " in *" $b "*)`, which is a **substring** test rather +than a token test, so a page named `authors index` passed. Both fail toward pass. Neither +was visible by reading; both were found by running the script against inputs that should +have failed it. + +Final gate tested against nine cases, extracted verbatim from the YAML that ships rather +than retyped: clean site, `CLAUDE.html`, `CLAUDE.md`, both, empty `docs/`, the substring +bypass, a double suffix, a glob metacharacter in a filename, and a plausible future root +page. diff --git a/planning/archive/2026-08-issue-42-pkgdown-claude-md/review.md b/planning/archive/2026-08-issue-42-pkgdown-claude-md/review.md new file mode 100644 index 0000000..1e59698 --- /dev/null +++ b/planning/archive/2026-08-issue-42-pkgdown-claude-md/review.md @@ -0,0 +1,178 @@ +# Review — `b98b863` "Keep CLAUDE.md off the pkgdown site (#42)" + +Reviewer: code-review subagent. Date: 2026-08-31. +Scope: `.github/workflows/pkgdown.yaml` (the only file the commit touches, and the only +workflow in the repo that deploys to `gh-pages`). + +Verdict: **the change is correct and does what it claims.** Two low-probability +false-pass paths in the allowlist gate, both verified by executing the shipped script. +One factual claim in the commit message / issue body is wrong in a way that does not +affect the remedy. + +--- + +## Findings + +- **[fragile]** `.github/workflows/pkgdown.yaml:78-81` — `case " $allowed " in *" $b "*)` + is a **substring** test, not a token test, so a page whose name is a contiguous + multi-word substring of the allowlist passes silently. + + Verified by running the gate script extracted verbatim from the YAML: + + | `docs/` contents | expected | actual | + |---|---|---| + | `index.html`, `authors index.html` | fail | **`rc=0`, "Root pages are all declared."** | + | `index.html`, `404 authors.md` | fail | **`rc=0`, "Root pages are all declared."** | + + `allowed` is `"404 authors index LICENSE LICENSE-text"`, so `" authors index "` and + `" 404 authors "` are both substrings of `" $allowed "`. This is exactly the + escape-hatch class CLAUDE.md warns about ("A guard's escape hatches are where it goes + to die") and it fails toward **pass**, which is the dangerous direction. + + Exploitability here is low: reaching it needs a `docs/` root page whose filename + contains a space, which in this repo can only come from a root `.md` with a space in + its name — and `R CMD check` rejects those outright ("checking for portable file + names"). So this is not a live leak, it is a guard that is weaker than it reads. + + Fix is one line, and it also removes the `${b%.html}`/`${b%.md}` interaction below: + + ```sh + ok=0 + for a in $allowed; do [ "$a" = "$b" ] && { ok=1; break; }; done + [ "$ok" = 1 ] || case " $unexpected " in *" $b "*) ;; *) unexpected="$unexpected $b" ;; esac + ``` + + (Note the same substring shape is used for the `$unexpected` de-duplication on + line 80. There it only affects whether a name is *repeated* in the message, so it is + cosmetic — but it is the same construct and should be changed with it.) + +- **[fragile]** `.github/workflows/pkgdown.yaml:77` — the two suffix strips are applied + unconditionally in sequence (`b="${b%.html}"; b="${b%.md}"`), so `index.md.html` + reduces to `index` and passes. Verified: `docs/index.md.html` present ⇒ `rc=0`. + Strictly more contrived than the finding above (pkgdown produces no such name), and + the token-comparison fix does not remove it — strip only one suffix, matched to which + glob produced the file, if it is worth closing at all. + +### Not findings, but worth recording + +- **Factual error in the commit message and issue #42 body.** Both state the second copy + is "a verbatim copy of the source, served as-is". It is not. `gh-pages:CLAUDE.md` is a + pandoc-regenerated markdown twin — pkgdown 2.2 emits an `.md` alongside every root + `.html` for `llms.txt`, and the published file is re-wrapped, not byte-identical to + the source: + + ``` + source : "...Estimate ground footprints from airphoto centroids and scale," (~110 col) + gh-pages: "...Estimate ground\nfootprints from airphoto centroids and scale," (~72 col) + ``` + + The content is still fully published, so the remedy (`rm` before build) is unchanged + and the "three copies" count is right. Per the "issue bodies get edited, not appended" + convention, the #42 body is worth a one-word correction; the commit message is + immutable history and stays as is. + +- **`clean: true` changes what a degraded build costs.** With `clean: false` a build that + silently produced fewer pages left the old ones published; now it deletes them. The + gate's population check is only `[ -f docs/index.html ]`, so it does not catch a + partial site. This is the tradeoff CLAUDE.md's pkgdown convention explicitly endorses + (auditability), the loss is recoverable from `gh-pages` history and from the next good + build, and pkgdown errors rather than half-building — so it is context, not a defect. + +- **Gate scope is root pages only.** `docs/articles/`, `docs/reference/`, + `docs/search.json` and `docs/llms.txt` are not inspected. That is the right scope + (the mechanism being closed is `package_mds()`), and because the gate runs *before* the + deploy step, a failure blocks publication of the index and `llms.txt` too — nothing + half-publishes. + +--- + +## Verification performed + +Everything below was measured, not reasoned. + +**1. The gate's shell, run verbatim from the YAML.** All six of the committer's-plus-mine +cases behaved as documented: + +| case | result | +|---|---| +| real gh-pages root set minus CLAUDE (`404/authors/index/LICENSE/LICENSE-text`, `.html`+`.md`) | pass, `rc=0` | +| + `CLAUDE.html` and `CLAUDE.md` | fail, `rc=1`, "CLAUDE" named **once** | +| empty `docs/` | fail, `rc=1`, "the build produced no site" | +| non-page assets only (`llms.txt`, `sitemap.xml`, `pkgdown.yml`, `search.json`, `logo.png`, `katex-auto.js`, `lightswitch.js`, `link.svg`) | pass — correctly outside the `docs/*.html docs/*.md` globs | +| subdirectory content (`docs/reference/foo.html`) | pass — correctly outside the top-level glob | +| filename containing a newline (`LICENSE\nevil.md`) | fail, `rc=1` | + +Other shell properties checked and found sound: +- Unmatched globs are handled (`[ -e "$f" ] || continue`); no `nullglob` dependency. +- Glob metacharacters in a filename are **not** re-interpreted — `" $b "` is a quoted + portion of the `case` pattern, so it is literal. `docs/foo*.html` is correctly + reported as unexpected rather than matching anything. +- `set -eu`: every variable (`allowed`, `unexpected`, `f`, `b`) is assigned before use; + `basename` failing aborts the step (fails toward abort, correct for a guard). +- BSD/GNU portability is not in play — this runs on `ubuntu-latest` bash only — and the + script uses no `sed`/`grep` extensions regardless. +- No `ls` parsing, no `2>/dev/null` on a mutating command, no `;`-chained mutations. +- The empty-population trap CLAUDE.md names ("a loop over nothing exits 0") is explicitly + closed by the `docs/index.html` precondition, and the failing case was demonstrated. + +**2. The `allowed` list is exactly right for this repo — neither too broad nor too +narrow.** Currently published root files on `gh-pages`: + +``` +404.html 404.md authors.html authors.md index.html index.md +LICENSE.html LICENSE.md LICENSE-text.html LICENSE-text.md +CLAUDE.html CLAUDE.md <- the leak +.nojekyll katex-auto.js lightswitch.js link.svg llms.txt +logo.png pkgdown.js pkgdown.yml search.json sitemap.xml +articles/ deps/ news/ reference/ +``` + +`allowed = "404 authors index LICENSE LICENSE-text"` covers every `.html`/`.md` root page +except `CLAUDE`, and no more. Every file listed in the review brief (`llms.txt`, +`sitemap.xml`, `logo.png`, `search.json`, `pkgdown.yml`, `katex-auto.js`, +`lightswitch.js`, `link.svg`) falls outside the `docs/*.html docs/*.md` globs and is +correctly out of scope — confirmed by execution, not by reading. `404.md` is a real +published file and is covered. + +**3. `pkgdown:::package_mds()` returns `CLAUDE.md` and nothing else** for this repo. +Verified directly against the working tree with pkgdown 2.2.0. Root `.md` files are +`CLAUDE.md`, `LICENSE.md`, `NEWS.md`, `README.md`; the latter three are pkgdown's +hardcoded exclusions. There is no `.github/CODE_OF_CONDUCT.md` or `SUPPORT.md`, and no +`CITATION.cff` / `inst/CITATION`, so no additional root page is generated. + +**4. `clean: true` is safe.** +- **No `CNAME` on `gh-pages`** (confirmed by API listing, count 0). Independently + corroborated at the source: `pkgdown:::build_github_pages()` writes a `CNAME` only when + `cname_url(url)` is non-`NULL`, and `_pkgdown.yml`'s `url` is + `https://newgraphenvironment.github.io/fly/` — a `github.io` URL, which yields `NULL`. + The `www.newgraphenvironment.com/fly/` address is the org site's apex domain serving + the project site as a subpath, so nothing on this branch owns it. +- **No `dev/`** (count 0). `_pkgdown.yml` sets no `development: mode:`. +- **`.nojekyll` is regenerated by the build** — `build_github_pages()` calls + `write_if_different(pkg, "", ".nojekyll")` unconditionally. So `clean: true` cannot + strand the site behind Jekyll processing. +- **`docs/` is untracked**: `.gitignore:6` lists `docs`, `git check-ignore -v docs` + confirms, and `git ls-files docs` returns nothing. Nothing in the deployed tree is + hand-added, so everything `clean` could delete is CI-regenerable. The commit's claim + holds. +- The only file `clean: true` will remove that the build no longer produces is + `pkgdown.js` (superseded by `deps/`), plus the two `CLAUDE.*` copies — which is the + point. + +**5. `rm -f CLAUDE.md` runs on every triggering event.** The step carries no `if:`, so it +executes for `push` to main/master, `pull_request`, `release: published` and +`workflow_dispatch` alike. It is placed immediately after `actions/checkout@v4` and +before `setup-r-dependencies` (`local::.`) and `Build site`, so neither the install nor +the build ever sees the file. Nothing else references it: no hit for `CLAUDE` in +`README.Rmd`, `README.md`, `_pkgdown.yml`, or `vignettes/`, and `.Rbuildignore` already +excluded it from `R CMD build`. `rm -f` cannot fail the step if the file is renamed or +absent — that silent no-op is exactly what the allowlist gate exists to backstop, so the +two steps are correctly layered rather than redundant. + +**6. Ordering is right.** `Build site` -> gate -> `Deploy`. A gate failure aborts the job +before `github-pages-deploy-action` runs, so there is no path to a half-published site. +The gate also runs on `pull_request` builds, where deploy is skipped — so an offending +root page reddens the PR rather than only being caught at merge. + +**7. `search.json` claim spot-checked**: 227 occurrences of `CLAUDE` in the currently +published index. Matches the issue body exactly.