docs: prevent broken links in published mdBook

[sonalisrisivani]

Fixes #371.

Summary

• Repoint the four links to repository-root files at the canonical GitHub URLs.
• Add the three existing but previously unlisted documents to docs/SUMMARY.md, preserving the distinction between the two user-stories.md files.
• Add scripts/check-mdbook-links.sh, which walks generated HTML, checks internal .html targets, and fails when it examines zero links.
• Add a release regression test covering valid links, missing generated pages, zero-link false passes, and the real mdBook build when the pinned tools are available.
• Run the guard in the GitHub Pages docs workflow after the mdBook build.

Validation

• tests/release/mdbook-links.test.sh fixture checks pass.
• Pinned mdBook 0.4.40 + mdbook-admonish 1.18.0 build passes and the guard checks 1,173 internal generated HTML links.
• bash -n scripts/check-mdbook-links.sh tests/release/mdbook-links.test.sh passes.
• git diff --check passes.

[sonalisrisivani]

Implemented and validated in commit c23da60.

The PR fixes all seven published-book links, adds the three omitted pages to SUMMARY.md, and adds a generated HTML link guard with regression fixtures. The pinned mdBook 0.4.40 + mdbook-admonish 1.18.0 build completed locally; the guard checked 1,173 internal .html links with no broken targets. Fixture checks, shell syntax, and git diff --check also pass.

[vladimirrott]

Reviewed at c23da602e532dc1ed712b6fc9678e1f65a3decf8.

The checker is the right idea and it is built correctly. It walks the rendered HTML rather than the Markdown source, which is the only place these seven links are broken, and it counts what it inspected so a build that produces no internal links fails instead of passing over nothing. That if checked == 0 line is the part that makes it a guard rather than a decoration, and you wrote it without being asked.

I drove it directly on three fixtures instead of taking the test's exit code for it. All of this ran in rootless podman with the tree mounted read-only and no network.

A valid pair of pages:

$ podman run --rm --network=none -v "$PWD/trees/p379t:/repo:ro" -v "$PWD/fixture379:/fx:ro" -w /repo localhost/sk-gate:1 bash scripts/check-mdbook-links.sh /fx/book
mdbook-links: checked 1 internal .html links
exit=0

The target removed:

$ podman run --rm --network=none -v "$PWD/trees/p379t:/repo:ro" -v "$PWD/fixture379b:/fx:ro" -w /repo localhost/sk-gate:1 bash scripts/check-mdbook-links.sh /fx/book
mdbook-links: broken generated links:
/fx/book/index.html	guide.html
exit=1

Nothing internal left to check:

$ podman run --rm --network=none -v "$PWD/trees/p379t:/repo:ro" -v "$PWD/fixture379c:/fx:ro" -w /repo localhost/sk-gate:1 bash scripts/check-mdbook-links.sh /fx/book
mdbook-links: no internal .html links were checked
exit=1

Your own test passes, and it reaches the real script rather than restating its logic, so a revert of the production script would take it down with it:

$ podman run --rm --network=none -v "$PWD/trees/p379t:/repo:z" -w /repo localhost/sk-gate:1 bash tests/release/mdbook-links.test.sh
mdbook-links: checked 1 internal .html links
mdbook-links: SKIP real mdBook build (mdbook and mdbook-admonish are required)
exit=0

Shellcheck is clean at CI's severity, and at --severity=style as well:

$ shellcheck --severity=warning /tmp/sc_*.sh
$ echo "SHELLCHECK_EXIT=$?"
SHELLCHECK_EXIT=0

The three pages you wire into SUMMARY.md all exist, so none of the new entries points at nothing:

$ for f in docs/user-stories.md docs/contributing/ubuntu-vm-testing.md docs/testing/user-stories.md; do git cat-file -e p379:$f 2>/dev/null && echo "OK $f" || echo "MISSING $f"; done
OK docs/user-stories.md
OK docs/contributing/ubuntu-vm-testing.md
OK docs/testing/user-stories.md

I read the .github/workflows/docs.yml hunk in full. Three added lines, one step, and it touches no permissions block, no trigger, no action pin and no secret.

Blocking: the guard never runs on a pull request

docs.yml fires on push to main and on workflow_dispatch, and on nothing else:

$ git show p379:.github/workflows/docs.yml | grep -n "pull_request" || echo "NO pull_request TRIGGER IN docs.yml (confirmed)"
NO pull_request TRIGGER IN docs.yml (confirmed)

So the observable behaviour is not what the title promises. A pull request that breaks a generated link gets a green board and merges. The check then runs on the merge commit, sits ahead of Upload Pages artifact, and fails, so main goes red and the documentation stops publishing until somebody fixes it forward. That is worse than the current state, where the link is broken but the site still deploys.

There is a second consequence, and it is the one that will bite first. #377 is open right now and adds a gate requiring every tests/release/*.test.sh to be named in ci.yml, e2e.yml, release.yml or ci-local.sh. Your test is named only in docs.yml:

$ for f in .github/workflows/ci.yml .github/workflows/e2e.yml .github/workflows/release.yml .github/workflows/docs.yml scripts/ci-local.sh; do echo -n "$f: "; git show p379:$f 2>/dev/null | grep -c "mdbook-links.test.sh"; done
.github/workflows/ci.yml: 0
.github/workflows/e2e.yml: 0
.github/workflows/release.yml: 0
.github/workflows/docs.yml: 1
scripts/ci-local.sh: 0

and their gate names it:

$ podman run --rm --network=none -v "$PWD/trees/merged1:/repo:ro" -w /repo docker.io/library/debian:stable-slim bash scripts/check_test_reachability.sh
test-reachability: test is not invoked by a gate: tests/release/mdbook-links.test.sh
exit=1

One change fixes both: run tests/release/mdbook-links.test.sh from the docs-and-hygiene job in ci.yml, and add it to run_hygiene_group in scripts/ci-local.sh, next to public-claims.test.sh. Then a pull request that breaks a link goes red before it merges, which is what #371 was about, and #377's gate finds it wired. Keep the docs.yml step too if you like; it costs nothing and it guards the deploy.

That is the only change I am asking for. This is not a criticism of the check itself, which is good, and the interaction with #377 is a scheduling accident rather than anything you did wrong. I will hold the merge order so neither of you inherits a red board from the other.

Two things I could not verify, and why

I could not reproduce the real build. The workflow pins mdBook 0.4.40 and mdbook-admonish 1.18.0; this host has mdBook 0.5.2, and my review container has no network to fetch the pinned pair:

$ podman run --rm --network=none -v "$PWD/trees/p379t:/repo:z" -v "$HOME/.cargo/bin:/cb:ro" -w /repo -e PATH=/cb:/usr/local/bin:/usr/bin:/bin localhost/sk-book:1 bash -c 'mdbook --version; mdbook-admonish install . >/dev/null 2>&1; mdbook build 2>&1 | tail -2; echo "--- checker against the book/ directory docs.yml uploads ---"; bash scripts/check-mdbook-links.sh book'
mdbook v0.5.2
 WARN Error writing the RenderContext to the backend, Broken pipe (os error 32)
ERROR The "admonish" preprocessor exited unsuccessfully with exit status: 1 status
mdbook-links: book directory does not exist: book

So your "checks 1,173 internal generated HTML links" and the claim that all seven of #371's links are gone are unverified by me. I have no reason to doubt either; I am saying which command failed rather than implying I ran it. Your own run and the hosted job are the evidence for those.

Second, the step runs the test, and the test builds its own copy of the book into a temporary directory. The book/ that Upload Pages artifact publishes with path: book is never the thing inspected. Same source and same config, so divergence is unlikely, but run: scripts/check-mdbook-links.sh book after the build step would check the bytes that ship. Worth folding into the same push.

Optional

Two of the repointed links now leave the book for GitHub to reach a page that is inside the book. docs/cli.md's user-stories.md and docs/contributing/testing.md's ubuntu-vm-testing.md are both in your SUMMARY.md additions, so mdBook writes them and a relative link resolves. Sending a reader to github.com/blob/main for a page one click away is a worse read, and it also hands those two links to ci.yml's external checker and its network retries instead of your deterministic one. The four genuine root-file links (README.md, SECURITY.md twice, HACKING.md) do need the absolute form.

tests/release/mdbook-links.test.sh runs mdbook-admonish install "$repo_root", which writes into the working tree. Disposable in CI, less so for whoever runs it locally. Installing into the fixture instead would keep it self-contained.

docs/contributing/CONTRIBUTING.md:44 gained a third leading space on the continuation line. Cosmetic.

One process note

I did not approve the queued workflow runs on this pull request, and that is a rule rather than a judgement about you: approving a run for a fork PR that touches .github/workflows/** is the control that stops a stranger's workflow executing in CI, so it needs a person to read the workflow diff first. I do that myself. Nothing is waiting on you for it.

[vladimirrott]

Checking in on this one. The checker itself is finished work and I would rather land it than let it age.

The only thing outstanding is still the wiring: docs.yml fires on push to main and workflow_dispatch, so a pull request that breaks a generated link merges green and main goes red afterwards. Running tests/release/mdbook-links.test.sh from the docs-and-hygiene job in ci.yml, and adding it to run_hygiene_group in scripts/ci-local.sh next to public-claims.test.sh, fixes that and also gets it found by #377's reachability gate, which now parses ci.yml, e2e.yml and release.yml and does not look at docs.yml.

Your branch is behind main by eight days of commits, so it needs an update either way. If you would rather I made that change myself on top of your work and merged it, say so and I will; the review credit is yours regardless. If you want to push it, I will re-review the same day.

One thing that is not on you: security-audit is red on main for RUSTSEC-2026-0285 in rustls. A lockfile bump clears it and is already in flight.

[sonalisrisivani]

Thank you for the detailed and constructive review!

I have rebased the branch on latest main and addressed all of your feedback:

1. CI Guard Wiring: Added tests/release/mdbook-links.test.sh to the docs-and-hygiene job in .github/workflows/ci.yml. In the updated scripts/ci-local.sh, tests/release/*.test.sh is already discovered and executed automatically as part of the shell tests suite.
2. Docs Workflow: Updated .github/workflows/docs.yml to run scripts/check-mdbook-links.sh book directly on the generated build output before uploading the Pages artifact.
3. Fixture Isolation: Updated tests/release/mdbook-links.test.sh so mdbook-admonish install and mdbook build run entirely within a temporary fixture directory without modifying the local working tree.
4. Internal Book Links: Switched docs/cli.md (user-stories.md) and docs/contributing/testing.md (ubuntu-vm-testing.md) back to relative links so readers stay within the generated mdBook.
5. Formatting: Fixed the indentation on docs/contributing/CONTRIBUTING.md:44.

All checks and tests pass cleanly. Ready for your review!

[vladimirrott]

Reviewed at 766cb886.

Thank you for taking every item from my last review, the optional ones included. Running mdbook-admonish install inside the fixture keeps a local run out of the working tree, and docs.yml now checks the same book/ it uploads. Both are what I asked for, and the relative links for user-stories.md and ubuntu-vm-testing.md read better.

One blocking item, and the gap is mine

I asked you to run tests/release/mdbook-links.test.sh from docs-and-hygiene so a pull request that breaks a link goes red before merge. You wired it as I said. I missed that the job installs no mdBook, so the test runs its synthetic fixtures and then skips the real build. The hosted log for this PR (run 35880972750, job docs-and-hygiene):

mdbook-links: checked 1 internal .html links
mdbook-links: SKIP real mdBook build (mdbook and mdbook-admonish are required)

I reproduced that in a container with no mdBook, then pointed the user-stories.md link in docs/cli.md at no-such-page.md:

mdbook on PATH: none
=== clean rc=0
mdbook-links: checked 1 internal .html links
mdbook-links: SKIP real mdBook build (mdbook and mdbook-admonish are required)
mutated lines: 1
=== broken-link rc=0
mdbook-links: checked 1 internal .html links
mdbook-links: SKIP real mdBook build (mdbook and mdbook-admonish are required)

A broken link in docs/ still merges green, and main goes red on the next docs.yml run. Two changes close it:

1. In docs-and-hygiene, before your new step, add the Install mdBook and plugins step from docs.yml, copied verbatim: both versions, both SHA-256 values, both sha256sum --check --strict lines. Keep the hashes, since that job holds the workflow token.
2. In the test, refuse to skip under CI. When CI is set and either binary is missing, print why and exit 1. Local runs without mdBook keep the SKIP. Without this, anyone who later drops the install step turns the check back into a fixture test with no warning.

With both in, the broken-link case above should fail naming cli.html. I will run that mutation against your next push.

What I could not run

I still cannot build the book at the pinned versions. This host has mdBook 0.5.2, and my environment refused the download of the 0.4.40 release. Your 1,173-link figure rests on your run, and on the hosted job once step 1 lands.

Optional

scripts/check-mdbook-links.sh ends without a trailing newline, and the test calls $checker unquoted inside its two $(...) captures. Neither breaks anything today.

Once this lands, #464 lives in the same book: the contributing guide tells you to run the pre-commit framework that the developer guide forbids. Say so there and I will hold it for you.

[sonalisrisivani]

Thank you again for the precise and thorough re-review — the gap you identified (the test skipping silently in CI when mdBook is absent) is exactly the kind of thing that makes the check a decoration rather than a guard. Both items are now fixed in 034f92d.

What changed:

1. ci.yml — Install mdBook before running the test. Added the Install mdBook and plugins step verbatim from docs.yml (same two versions, same two SHA-256 values, same sha256sum --check --strict lines) immediately before the Check generated mdBook HTML links step. The hashes are preserved since this job holds the workflow token.

2. tests/release/mdbook-links.test.sh — Refuse to skip under CI. When CI is set and either binary is missing, the script now prints why and exits 1. Local runs without mdBook keep the SKIP path unchanged.

3. scripts/check-mdbook-links.sh — Trailing newline. Added the missing final newline.

4. $checker quoting — Quoted both $() captures around $checker as "$checker" per your optional note.

With the install step in place, the mutation test you described (pointing user-stories.md at a nonexistent page) should now fail the docs-and-hygiene job, naming the broken link in cli.html, before the PR merges. Ready for your re-review!

[sonalisrisivani]

I checked the recent CI failure in docs-and-hygiene (specifically the failing with Failed to read "/tmp/.../src/docs/images/social-preview.png").

It turns out docs/images/social-preview.png is a symlink pointing to ../../assets/social-preview.png. Because my test setup was only copying the docs directory over to the temporary fixture, the symlink resolving to assets broke.

I just pushed commit 48dddd4 which appropriately copies the assets folder (cp -r "$repo_root/assets" "$src_dir/assets") into the fixture so that mdbook can resolve the symlink and run cleanly. The build works perfectly on my end now!

[vladimirrott]

Reviewed at 48dddd44. Nothing here blocks.

I had a review half-written about the social-preview.png symlink when your 48dddd4 landed with the same diagnosis and the fix. Both items from my last review are done too. Your CI check turned the silent SKIP into a failure, and that failure is how the symlink surfaced at all. The hosted log for this head shows the real book building and being checked for the first time:

$ job=$(gh api repos/lacs-project/sysknife/actions/runs/36421459302/jobs --jq '.jobs[] | select(.name=="docs-and-hygiene") | "\(.id) \(.conclusion)"'); echo "$job"; out="$(gh run view 36421459302 --repo lacs-project/sysknife --job ${job%% *} --log 2>&1)"; echo "rc=$?"; grep -n 'mdbook-links\|Rendering failed\|mdbook::book' <<<"$out" | sed 's/^\([0-9]*:\)docs-and-hygiene\t[^\t]*\t/\1/' | cut -c1-200
108924939109 success
rc=0
...
1001:2026-09-28T12:26:04.4698962Z mdbook-links: checked 1 internal .html links
1011:2026-09-28T12:26:04.5336181Z 2026-09-28 12:26:04 [INFO] (mdbook::book): Book building has started
1012:2026-09-28T12:26:04.5479517Z 2026-09-28 12:26:04 [INFO] (mdbook::book): Running the html backend
1013:2026-09-28T12:26:04.7959245Z mdbook-links: checked 1244 internal .html links

At the previous head, the same step failed with Failed to read ".../src/docs/images/social-preview.png". docs/images/social-preview.png is a symlink to ../../assets/social-preview.png, and copying assets/ next to docs/ gives it a target. I replayed your fixture layout with the files from your branch and the link resolves:

$ d=$(mktemp -d); git archive p379 docs assets | tar -x -C "$d"; mkdir "$d/src"; cp -r "$d/docs" "$d/src/docs"; cp -r "$d/assets" "$d/src/assets"; test -e "$d/src/docs/images/social-preview.png"; echo "PR fixture layout: exists rc=$?"
PR fixture layout: exists rc=0

Workflow change

The new step sits in docs-and-hygiene, runs only curl, sha256sum --check --strict and tar, and pins the same two versions and SHA-256 values as docs.yml. I read it in full and have no concerns with it.

Before I approve

maintainer screen marks this PR as executable, and my container sandbox was unavailable today, so I have not run the checker myself on this pass. Before I approve I will break one generated link in a real mdBook build and require the test to fail, which is the proof I could not get at your last head. Your branch is also behind main. That comes from my repository moving under you, and I will update it when I merge.

One thing I owe you

In my last review I pointed you at #464 and said I would hold it for you. Two hours later I offered it to someone else, and they have a pull request up for it now. That was my mistake. If you want another one in the same area, #460 is open: sixteen assertions in release-rehearsal.test.sh exit 1 with no output when their anchor moves, the same "a check that fails without telling you why" problem you fixed here. Once this lands, comment on #460 if you want it and I will assign it to you there.

[vladimirrott]

Approved at bb1c5b4b.

Two things settled since my last pass.

The rebase carries nothing new. Your contribution at 48dddd44 and at bb1c5b4b is the same patch:

$ for f in docs/SUMMARY.md docs/cli.md docs/contributing/CONTRIBUTING.md \
    docs/contributing/testing.md docs/the-audit-chain.md \
    scripts/check-mdbook-links.sh tests/release/mdbook-links.test.sh \
    .github/workflows/docs.yml; do
    [ "$(git rev-parse 48dddd44:$f)" = "$(git rev-parse bb1c5b4b:$f)" ] && echo "same  $f"; done
same  docs/SUMMARY.md ... same  tests/release/mdbook-links.test.sh ... same  .github/workflows/docs.yml
$ git diff origin/main...bb1c5b4b --stat | tail -1
 9 files changed, 138 insertions(+), 4 deletions(-)

ci.yml is the one file whose blob moved, because main moved under it. Your three-dot diff against main hashes to 3cecdec7 at both heads.

The install block you copied into docs-and-hygiene is byte-for-byte the one already in docs.yml on main, comments aside, so this adds no new download and no new hash to trust:

$ diff <(main docs.yml, install block, comments stripped) <(bb1c5b4b ci.yml, same)
17d16
<

One blank line. Both pin mdBook 0.4.40 and mdbook-admonish 1.18.0 by sha256 and check before tar.

The mutation proof I owed you:

$ maintainer-merge verify 379 bb1c5b4b tests/release/mdbook-links.test.sh \
    's%if not (page.parent / target).resolve().exists()%if False%' shell
  running 'tests/release/mdbook-links.test.sh' unmutated
  shell suite: evidence of 1 executed unit(s), passing unmutated
  applying the mutation and re-running
  receipt recorded for #379 at bb1c5b4b (observed: clean pass, mutated fail)

That makes the checker call every target present, and your missing-page case goes red. The checked == 0 refusal is the assertion I would ask for if it were absent: a checker that walks a book nobody built reports the same silence as a book with no broken links, and yours refuses instead.

Board: 12 pass, container-smoke skipping, branch current with main. Three approved pull requests are ahead of you in the queue and main requires an up-to-date branch, so each of those merges puts this one behind again. I will run the branch update and merge it myself; you do not need to touch it. The CHANGELOG entry is mine.

The #460 offer from this morning stands: sixteen assertions in release-rehearsal.test.sh exit 1 with no output when their anchor moves. Same family as the zero-link refusal you wrote here. Comment there and I will assign it.

[vladimirrott]

Merged as c0f60925, and the check earned its place inside the hour.

#515 landed before this one and added [HACKING.md](../../HACKING.md) to the testing guide. HACKING.md is not in docs/SUMMARY.md, so mdBook rewrote the target to ../../HACKING.html and published a link to a page it never wrote. Your checker, running on the merge of your branch with main, said so:

mdbook-links: broken generated links:
/tmp/tmp.cz4fLM2e0I/real-book/contributing/testing.html	../../HACKING.html

I approved #515 without looking at that link, so the break is mine and not yuee3's or yours. I fixed it on main in 7224b15c, rewrote it as an absolute URL the way your patch does for the other one on the same page, and your branch went green on the next update.

Nothing in the repository caught this before today. markdown-link-check reads the Markdown source, where ../../HACKING.md resolves fine on disk; the gap was always between the source and what mdBook publishes, which is exactly the gap you named in #371.

Four branch updates and four CI cycles stood between your approval and this merge, because main requires an up-to-date branch and three other pull requests landed ahead of you. That queue is mine to run, not yours to wait on, and your patch was unchanged through all of it.

The #460 offer stands: sixteen assertions in release-rehearsal.test.sh exit 1 with no output when their anchor moves. Comment there and I will assign it.