fix(docs): pin the engine docs to the released tag, and say so when a reader is elsewhere (#85) - #92
Merged
Merged
Conversation
… reader is elsewhere (#85) Closes #85. Both options in the issue, because they compose: pinning narrows the gap, the banner covers what remains. ### Why pinning, not just a banner This failure mode is not hypothetical here. PR #88 shipped a false claim because the install page described `scripts/install.sh` on `main`; keel then raised its Python floor from 3.11 to 3.14 in `2d3934f`, and the page went stale with nobody touching this repo. Docs pinned to `main` carry the same fault silently. An operator runs a published release, so the documents this site renders should be the documents that release shipped. That is FR-9 applied upstream of the prose rather than to it. `scripts/lib/release-tag.mjs` resolves the tag once: a fresh `data/release.json` first (`npm run fetch` now runs `fetch-release.mjs` first, so the normal path costs no extra API call), then the releases API, then the last-known tag when the API is unreachable. **There is deliberately no fallback to `main`** — a silent fallback would restore exactly the skew this exists to remove. When no tag resolves at all, the build stops. The manifest keeps the policy legible: `"ref": "latest-release"` plus a `refNote` stating it in the file. A literal branch, tag or SHA is still honored verbatim, so pinning to something specific stays a one-line edit. FR-4 is intact — a document missing at the pinned ref still exits non-zero, naming the doc and the ref. ### The `?v=` notice keel's browser UI will deep-link as `/en/docs/glossary/?v=0.11.2#qabd`. The site is `output: "static"`, so the comparison happens in the browser: the markup ships hidden and complete, and one small inline script fills a text node and unhides it. Because the element is parsed before the fragment scroll and the script never touches `location.hash`, the anchor still lands correctly. Anything unexpected in `?v=` renders nothing — an unrecognized value is not evidence of skew. The value is regex-gated, length-capped and written with `textContent`, never `innerHTML`. ### One correction to the issue Acceptance asked for the notice "in all three locales". That is not achievable: `src/pages/en/docs/[slug].astro` is the only per-document route — ar and fr ship a docs index only, so there is no page for it to render on. The banner is EN-only. Arabic and French strings are in `ui.ts` so the chrome is complete if that changes. ### Also `src/components/docs/nav.ts` had `FALLBACK_META.ref = "main"`. A hard-coded branch there would now be a false claim on any page that fell back to it, so it is empty and the notice's `comparable` guard suppresses itself. `LAUNCH-CHECKLIST.md` inline-script count goes two to three — `1e194b9` was a commit correcting that same count, so it is kept honest here. ### Not fixed here — filed as #91 `blob/main` still appears in hand-written links across `Footer.astro`, `HomePage`, `CompliancePage`, `ComparePage`, `InstallPage` and — most seriously — `features.ts`, whose "Verify in the repository" links are how this site asks to be checked rather than believed. Same skew class, much wider surface, and it deserves its own review rather than riding along here. ### Verification - `npm run check` — 0 errors, 0 warnings - `npm run build` — `docs ref: v0.11.2 (from data/release.json, written this build)`, 8 documents, 56 pages - `data/docs-meta.json` records `"ref": "v0.11.2"`; every `sourceUrl` is `blob/v0.11.2/` - Rendered `dist/en/docs/glossary/index.html`: notice present and `hidden`, source link on the tag, no `blob/main` doc link left on the page Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cBJ2syTN4VoE73F9sBNGW
…eaches the log (#85) SonarCloud's quality gate failed on #92 with `new_security_rating: 2`. Two findings, and the first is worth more than its MINOR severity suggests. ### jssecurity:S5145 — user-controlled data reaching the log `resolved.tag` and `resolved.source` both originate at the GitHub releases API, and `source` quotes a network error verbatim. The tag is also interpolated into `raw.githubusercontent.com` fetch URLs and GitHub blob URLs, so this was never only a logging concern. Fixed at both ends: - **`release-tag.mjs` validates before returning.** `TAG_PATTERN` accepts a plain version tag and nothing else; all three resolution paths — fresh cache, API, stale cache — now go through `trustedTag()`, so an untrusted value is treated as unresolved and the caller fails the build. Verified against: `v0.11.2`, `0.11.2`, `v1.2.3.4`, `v0.11.2-rc.1` trusted; `main`, `../../evil`, `v1;rm -rf`, `""`, an embedded newline, and `%0aInjected` all rejected. Note `main` is rejected by the pattern, which is a second, independent guard on the property this whole change exists to protect. - **`fetch-engine-docs.mjs` sanitizes at the log boundary.** `forLog()` strips control characters and caps length, so no remote string can forge a line in a build log that is read to decide whether a deploy is trustworthy. ### javascript:S7773 — `Number.NaN` over `NaN` Trivial, applied. ### Verification - `npm run check` — 0 errors, 0 warnings - `npm run build` — `docs ref: v0.11.2`, 56 pages, complete - Tag-pattern behaviour exercised directly against the ten cases above Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cBJ2syTN4VoE73F9sBNGW
…s in its docs (#85) Independent review returned MERGE WITH FIXES. It could not break the code — 25 hostile `?v=` inputs, nine hostile cache states, and a real FR-4 404 test all behaved. Every finding was a claim that overstated what is true. ### The PR body was wrong about its own output It asserted "no `blob/main` doc link on the page." There are two, on every docs page, from `Footer.astro:20`: $ grep -oh 'keel/\(blob\|tree\)/[^/"]*' dist/en/docs/glossary/index.html | sort | uniq -c 2 keel/blob/main 2 keel/blob/v0.11.2 They are the footer's link to the experiment record, squarely in #91's scope and not this PR's job to fix — but the claim should not have been made. Nothing is currently misrepresented by them: that document is byte-identical at `v0.11.2` and `origin/main` today. The PR body is corrected. ### The last-known-tag tier is a local convenience, not CI resilience Three places described it as a safety net. `data/` is gitignored and no workflow caches it, so on the machine that ships the site `data/release.json` never pre-exists — `fetch-release.mjs` writes it seconds earlier or writes nothing usable. In CI the behaviour is binary: the releases API answers, or the build stops. That is the right policy; it was just described as more forgiving than it is. Said plainly now in `release-tag.mjs` and `DEPLOYMENT.md`. ### Two DEPLOYMENT.md bullets contradicted each other One said the docs pin fails the build when no tag resolves; the next still said "Release / Discussions fetch failures degrade, not fail." Both were written true, and this PR made them incompatible: the release fetch's *tag* is now load-bearing. Reconciled, including the part that matters operationally — the live site is unaffected, because Pages keeps serving the last good deployment. ### `comparable` was unanchored `/^v?\d+(\.\d+)*/` accepted `1d1799e` and `2.0-dev`. Harmless today, since `TAG_PATTERN` already guarantees a plain version tag or a dead build. It bites the moment someone uses the documented escape hatch and puts a literal SHA in `manifest.ref`: a SHA starting with a digit would render "these pages describe 1d1799e", one starting with a letter would render nothing. Anchored. ### The inline-script count was a count of behaviors An English docs page ships four `<script>` blocks, not three — the light/dark choice is a bootstrap plus a toggle. Consistent with how `1e194b9` counted, but that commit exists *because* this number was wrong once. Now states both. ### Worth recording: the pin removed a live self-contradiction Of the eight mirrored documents, exactly one differs between `v0.11.2` and `origin/main` — `docs/operator-runbook.md`. `main`'s version says the TUI "went with the dashboard" and that `keel serve` is the only interactive surface. But `keel/commands/tui.py` is present at `v0.11.2` and absent on `main`, so a released operator *has* `keel tui` — and this site's own guides ship `code: "keel tui"` with screenshots from a real session. Before this PR the site told operators, on one page, that a command it demonstrates on another page does not exist. Pinning is not a trade here; it fixed that. ### Verification - `npm run check` — 0 errors, 0 warnings - `npm run build` — `docs ref: v0.11.2`, 56 pages - Notice still renders and still ships `hidden`; anchored regex in output - `dist/en/docs/operator-runbook/index.html` — 7 occurrences of `keel tui` Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cBJ2syTN4VoE73F9sBNGW
|
eaitbrahim
added a commit
that referenced
this pull request
Aug 27, 2026
Two ~340-line `.playwright-mcp/page-*.yml` files were committed by #92. They are accessibility-tree dumps written by the browser verification of the `?v=` notice — scratch output from checking the work, not part of it. Removed from the index and added to `.gitignore` so the next browser check cannot repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cBJ2syTN4VoE73F9sBNGW
eaitbrahim
added a commit
that referenced
this pull request
Aug 27, 2026
… to verify (#91) (#96) * fix(links): point the "verify this" links at the release they ask you to verify (#91) Closes #91. #85 pinned the docs pipeline to the latest published release tag. Hand-written links kept pointing at `main`, which is 17+ commits ahead of the newest tag. The serious half is `features.ts`. Each feature carries a "Verify in the repository" link — the mechanism by which this site asks to be checked rather than believed — and those landed on `keel/compliance/screen.py`, `keel/execution/guards.py` and `keel/agent.py` at `main`. A reader who takes the invitation was reading code that may not be in the release they run. That is worse than a broken link: a broken link announces itself, while a verification link that verifies the wrong thing does not. Not hypothetical for these exact files. `docs/operator-runbook.md` gained packaged-install text in keel `1d1799e`, on `main`, unreleased — so a v0.11.2 operator clicking "verify" read behaviour their build does not have. ### The scope was wider than the issue said Issue #91 listed the five component call sites and `features.ts`. It missed `compliance.ts`, which carries four links in each of three locales — twelve more, including `docs/fiqh-basis.md#scholarly-review-status`. That fragment is load-bearing: a helper that appended a path to a base URL would silently drop it and land the reader at the top of the document. ### One place the ref lives `src/lib/engine-url.ts` builds every engine URL from the ref in `data/docs-meta.json` — the one `scripts/fetch-engine-docs.mjs` resolved. Copy files carry a repo-relative `path` (plus optional `hash`), and the component builds the href, matching the convention `community.ts`, `compliance.ts`'s `links.items` and #94's `FaqItem.link` already follow. `grep -rn "blob/main" src/ | grep -v "src/content/"` returning nothing is now the lint that keeps it true. ### It fails the build rather than guessing `nav.ts`'s `FALLBACK_META` sets `ref: ""` — #92 made it empty precisely so a hard-coded branch could not become a false claim. `engineSourceUrl` throws when the ref is empty rather than emitting `blob//docs/...` or falling back to `main`, because a silent fallback restores exactly the skew this removes. Proven, not asserted: blanking `ref` in `data/docs-meta.json` makes `astro build` exit **1** with the message naming the path and the fix; restoring it exits **0**. ### FR-8: revs deliberately not bumped No translated prose changed — every `label` and `note` string is byte-identical, and only the href construction moved. #94's reviewer settled the same question for an `id` attribute: markup is not copy. The diff is 35 `href:` lines removed (33 links plus two type declarations) against 33 `path:` and 3 `hash:` lines added. ### Out of scope, deliberately `src/content/changelog/*.md` still contains `blob/main` links. That directory is gitignored and rewritten by `fetch-release.mjs` — those links are quotes from release notes as published, and the site must not rewrite what a release said. `docs/desktop-install.md` is pinned like the rest, but still walks a reader through downloading a `.dmg` v0.11.2 does not carry. That is #87. ### Verification - `npm run check` — 0 errors, 0 warnings - `npm run build` — `docs ref: v0.11.2`, 56 pages - `grep blob/main` in `src/` excluding `src/content/` — 0 - No `blob/main` in any built page outside the changelog - `#scholarly-review-status` resolves at `blob/v0.11.2/` - Verify links pinned in en, ar and fr - Empty ref → build exit 1; real ref → exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cBJ2syTN4VoE73F9sBNGW * fix(links): make the "lint" real, and close the whitespace hole (#91) Independent review returned MERGE and confirmed the substance: all eleven pinned paths exist at v0.11.2, both fragments resolve to real headings, and a dist-vs-dist diff against main is exactly 154 hrefs removed and 154 added, 1:1, with the compliance and features pages byte-identical once the ref is substituted. Two findings were worth acting on. ### The PR claimed an enforcement that did not exist Both the PR body and `engine-url.ts`'s header said `grep blob/main` "is the lint that keeps it that way". It was not wired to CI, `npm run check`, or any script — it was a sentence. On a site with FR-9, claiming a guard that does not run is the wrong kind of wrong. Worse, the named pattern would not have caught two shapes this repo already contained before #91: `tree/main/packages` and `raw.githubusercontent.../keel/main/...`. So the sentence is now a program. `scripts/check-engine-refs.mjs` fails on any engine URL naming `main`, `master` or `HEAD` across `blob/`, `tree/`, `raw/` and raw.githubusercontent.com, run by `npm run check` and as its own CI step so the failure is legible in Checks. Proven by injecting all three shapes into `Footer.astro`: exit 1, all three reported by file and line; removed, exit 0. Two exceptions are allowlisted in the script with their reasons, not silently skipped: - `scripts/install.sh` — does not exist at v0.11.2 (`git cat-file -e` fails). It lives on the default branch only, so pinning it would turn the install page's primary command into a 404. - `src/content/` — fetched release-note prose whose links are quotes; rewriting them would edit what a release said. ### A whitespace ref shipped a malformed URL on a green build `if (!ref || !repo)` tested falsiness. A ref of `" "` passed it and produced `blob/ /keel/compliance/screen.py`. Reachable: `fetch-engine-docs.mjs:46` short-circuits on a hand-set manifest ref (`if (manifest.ref !== LATEST_RELEASE) return manifest.ref;`), returning it verbatim without `release-tag.mjs`'s `TAG_PATTERN` validation. Both values are trimmed before the guard now — a whitespace ref exits 1 like an empty one. ### Two corrections to the record `main` is 25 commits ahead of v0.11.2, not the "17+" the previous message said. `docs/desktop-install.md` is pinned like the rest, but the section containing it does not currently render at all — `InstallPage.astro:216` gates it behind `hasPlatformBundle`, and v0.11.2 ships none, so `grep -c desktop-install dist/en/install/index.html` is 0. The earlier note read as though that link were live to a reader today. It is not, which makes #87 the only thing standing between it and a reader. ### Verification - `npm run check` — 0 errors, 0 warnings (now runs the guard first) - `npm run build` — `docs ref: v0.11.2`, 56 pages - Guard: clean on HEAD; exit 1 on each injected shape - Empty ref exit 1, whitespace ref exit 1, real ref exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015cBJ2syTN4VoE73F9sBNGW --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #85. Implements both options — they compose: pinning narrows the gap, the banner covers what remains. Decision recorded on the issue.
Why pinning, not just a banner
Not hypothetical here. PR #88 shipped a false claim because the install page described
scripts/install.shonmain; keel then raised its Python floor 3.11 → 3.14 in2d3934f, and the page went stale with nobody touching this repo. Docs pinned tomaincarry the same fault, silently.An operator runs a published release, so the documents this site renders should be the documents that release shipped.
Resolution order, and the one thing it will not do
scripts/lib/release-tag.mjsresolves the tag once: freshdata/release.json→ releases API → last-known tag when the API is unreachable.npm run fetchnow runsfetch-release.mjsfirst, so the normal build path costs zero extra API calls.There is deliberately no fallback to
main. A silent fallback would restore exactly the skew this removes. No tag → build stops. FR-4 is intact: a doc missing at the pinned ref still exits non-zero, naming the doc and the ref.The manifest keeps the policy legible —
"ref": "latest-release"plus arefNote. A literal branch, tag or SHA is still honored verbatim.The
?v=noticeStatic output means no query parameter at build time, so the comparison happens in the browser: markup ships hidden and complete, one small inline script fills a text node and unhides it. The element is parsed before fragment scrolling and the script never touches
location.hash, so#qabdstill lands correctly.Unexpected input renders nothing — an unrecognized value is not evidence of skew. Regex-gated, length-capped,
textContentneverinnerHTML.Correction to the issue
Acceptance asked for the notice "in all three locales" — not achievable.
src/pages/en/docs/[slug].astrois the only per-document route; ar and fr ship a docs index only, so there is no page for it to render on. EN-only. Arabic and French strings are inui.tsso the chrome is complete if that changes.Not fixed here — filed as #91
blob/mainstill appears in hand-written links acrossFooter,HomePage,CompliancePage,ComparePage,InstallPageand — most seriously —features.ts, whose "Verify in the repository" links are how this site asks to be checked rather than believed. Same class, much wider surface, deserves its own review.Verification
npm run check— 0 errors, 0 warningsnpm run build— 8 documents, 56 pages, completedata/docs-meta.json→"ref": "v0.11.2"; everysourceUrlonblob/v0.11.2/dist/en/docs/glossary/index.html— notice present andhidden, source link on the tagblob/mainlink remained on the page. Two do, fromFooter.astro:20— tracked in The "Verify in the repository" links point at main, not the release they ask you to verify #91, not fixed here. Nothing is misrepresented by them today: that experiment document is byte-identical atv0.11.2andorigin/main.Reviewer attention
mainstops appearing here until it releases. That is the intent, but it is a real trade while the project moves fast.LAUNCH-CHECKLISTupdated;1e194b9was a commit correcting that same count).🤖 Generated with Claude Code
https://claude.ai/code/session_015cBJ2syTN4VoE73F9sBNGW