From 9f47b38dc8a0dfb8050ecaf1f47fabffbd84a4bb Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 25 Aug 2026 13:53:38 -0400 Subject: [PATCH] feat(web): documentation is linked, never embedded -- and the anchors are checked (#539) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every documentation reference is now an outbound keeltrading.com link. The app fetches, bundles and caches nothing, and `/glossary` -- a page that has been EMPTY in every installed deployment keel has ever shipped -- is deleted. ── THE SHIPPED BUG THIS CLOSES ──────────────────────────────────────────────── `docs/` lives at the REPOSITORY root, outside `keel/`, and `uv_build` packages the module root. No wheel has ever carried it. `help_console.load_glossary` says so in its own docstring: "an installed deployment has no docs/ checkout, and the help screen renders that notice as its empty state." It is not fixable with a packaging glob -- measured at #535, building with `artifacts` set and with `artifacts = []` produces byte-identical wheels. Linking out is not the cheaper option here; it is the only one that reaches an install. ── A BROKEN DEEP LINK IS NOT AN ERROR, SO A TEST HAS TO BE ──────────────────── `…/glossary/#rial` opens the glossary at the TOP of the page, looking exactly like a link to a term that happens not to scroll, and nothing anywhere reports it. keel's `docs/` is the source and the site is the mirror, so this repository is the only place a rename and the links depending on it can be compared -- and `tests/web/test_doc_links.py` is that comparison, in both directions: * every `(slug, anchor)` the client can emit resolves to a heading that exists in the named document in `docs/`; * every LABEL in the table is one a view actually puts on screen, because `kv` links by lookup -- a label renamed in `render.js` would silently stop being a link rather than break. ── THE TABLE IS KEYED BY LABEL, AND IS DELIBERATELY INCOMPLETE ──────────────── `kv` consults `docs.TERMS` for every pair it builds, so a label that names a term is a link wherever it appears and no call site knows. The alternative -- tagging each site with a term -- spreads the decision across six views and makes "which words on this screen are defined somewhere" unanswerable without reading all of them. Two labels are left out on purpose and pinned as such. `mode` reads `paper` or `live`: one label, two definitions, no way to pick. `evidence required` sits on the CAPABILITY gates, not the promotion gate, so `#promotion-gate` would be confidently and invisibly wrong. A missing link costs a reader one search; a wrong one costs them their trust in every other link on the page. ── THE FIRST PAINT WAITS FOR THE BUILD, AND `?v=` IS NOT `build` ────────────── `show()` moved inside the `/api/config` callback: a link built before the build is known carries no version until the next poll -- fifteen seconds of links that do not say which build the reader is running, on the first screen they see. `/api/config` opens no database, and `api.read` resolves rather than rejecting, so this cannot hang the app with nothing on the port. The rendered nav links out too, and its first spelling put `cfg.build` in the query -- which is the footer's human-readable LINE, not a version. It rendered `?v=keel%200.11.2%2Bc1634a3fa17f%20%28DIRTY%29%20%5Bcheckout%5D`. Caught by reading the href of a running server, not by a test: both are non-empty strings and every assertion about "the link carries a version" passed. `server._docs_version` now reads `full_version`, the same field `/api/config` hands the client, and a test passes both strings so the two can never be confused again. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2 --- keel/web/render.py | 68 ++--- keel/web/server.py | 29 +- keel/web/static/css/keel.css | 16 ++ keel/web/static/index.html | 17 ++ keel/web/static/js/docs.js | 141 ++++++++++ keel/web/static/js/main.js | 23 +- keel/web/static/js/render.js | 10 +- keel/web/static/sw.js | 1 + tests/commands/test_console_thinness.py | 11 +- tests/web/test_client_assets.py | 33 ++- tests/web/test_doc_links.py | 335 ++++++++++++++++++++++++ tests/web/test_pwa.py | 5 +- tests/web/test_render.py | 22 +- tests/web/test_server.py | 14 +- 14 files changed, 651 insertions(+), 74 deletions(-) create mode 100644 keel/web/static/js/docs.js create mode 100644 tests/web/test_doc_links.py diff --git a/keel/web/render.py b/keel/web/render.py index fbd8419..36a6158 100644 --- a/keel/web/render.py +++ b/keel/web/render.py @@ -24,8 +24,20 @@ from collections.abc import Iterable, Sequence from decimal import Decimal from typing import Any +from urllib.parse import quote + +#: The published documentation root. Spelled here and in `static/js/docs.js`, and +#: `tests/web/test_doc_links.py` pins that the two agree. +DOCS_URL = "https://keeltrading.com/en/docs/" #: Nav order, and the labels. `/` first because the status page is the answer to "is it alive". +#: +#: **The eighth entry is an OUTBOUND link, and used to be a page (#539).** `/glossary` rendered +#: `docs/glossary.md` read from the working directory -- which no installed deployment has, since +#: `docs/` sits at the repository root, outside the `keel/` module `uv_build` packages. Every +#: install therefore rendered an empty glossary, and `help_console.load_glossary`'s docstring said +#: so. It is a link now, for the same reason the client's is: linking is the only form of this +#: that reaches an installed deployment at all. NAV: tuple[tuple[str, str], ...] = ( ("/", "Status"), ("/setup", "Setup"), @@ -34,7 +46,7 @@ ("/rules", "Rules"), ("/venues", "Venues"), ("/gates", "Gates"), - ("/glossary", "Glossary"), + (DOCS_URL, "Docs"), ) _STYLE = """ @@ -304,6 +316,7 @@ def page( path: str, body: str, build: str = "", + version: str = "", refresh_sec: int | None = None, ) -> str: """The document shell. `refresh_sec` emits a `` -- a zero-JS @@ -313,7 +326,29 @@ def page( nav_items = [] for href, label in NAV: on = ' class="on"' if href == path else "" - nav_items.append(f'{esc(label)}') + # An outbound entry is navigation, not a subresource, so `default-src 'none'` does not + # reach it. `noopener` still does matter: a new tab opened without it holds a + # `window.opener` handle back to a trading console on a token-bearing origin. + # + # `?v=` carries the running build, exactly as `static/js/docs.js` does for the client: + # the site pins `main` while an operator runs a tagged release, so a linked page can + # describe behaviour their build does not have. That skew is made VISIBLE rather than + # solved -- the build ends up in the URL bar of the page they are reading. `quote`, not + # an f-string, because a full version is `0.11.2+c1634a3fa17f` and a raw `+` in a query + # string decodes to a space. + # + # `version`, NOT `build`, and the two are different strings: `build` is the footer's + # human-readable LINE -- `keel 0.11.2+c1634a3fa17f (DIRTY) [checkout]` -- and the first + # spelling of this used it, putting that whole sentence percent-encoded into the query. + # Caught by looking at the rendered href, not by a test: both forms are non-empty + # strings and every assertion about "the link carries a version" passed. + away = "" + target = href + if href.startswith("https://"): + away = ' target="_blank" rel="noopener noreferrer"' + if version: + target = href + "?v=" + quote(version, safe="") + nav_items.append(f'{esc(label)}') nav = "".join(nav_items) meta_refresh = ( f'' if refresh_sec else "" @@ -948,34 +983,5 @@ def render_gates(gates: Sequence[Any], capabilities: Sequence[Any]) -> str: return "".join(parts) -def render_glossary(terms: Sequence[Any]) -> str: - parts = [ - '

Glossary

keel\'s vocabulary, and the fiqh terms it anchors to

', - '
', - ] - for term in terms: - marker = ' fiqh' if term.fiqh else "" - if term.fiqh and not term.stated: - marker = ' not stated in fiqh-basis' - parts.append(f"
{esc(term.term)}{marker}
") - parts.append(f"
{esc(term.definition)}
") - source = term.citation or term.source - if source: - parts.append(f'
{esc(source)}
') - parts.append("
") - if not terms: - # The normal state of an INSTALLED deployment, not a bug: `docs/glossary.md` is read from - # the working directory, and a deployment folder is a config, a database and an .env -- - # there is no docs checkout beside them. The TUI's help screen shows the same empty state - # for the same reason. Packaging the docs inside the artifact is D5's business (#438); - # until then this says which file is missing rather than implying the glossary is empty. - parts.append( - '

No glossary here. keel reads docs/glossary.md from ' - "the folder it is run in, and an installed deployment has no docs checkout beside " - "its config and database.

" - ) - return "".join(parts) - - def render_message(heading: str, detail: str) -> str: return f'

{esc(heading)}

{esc(detail)}

' diff --git a/keel/web/server.py b/keel/web/server.py index 65acdc8..4031740 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -280,17 +280,11 @@ def page_gates(_cfg: ServeConfig, _query: dict[str, list[str]]) -> tuple[str, st return "Gates", render.render_gates(GATES, CAPABILITIES), None -def page_glossary(_cfg: ServeConfig, _query: dict[str, list[str]]) -> tuple[str, str, int | None]: - from keel.commands.help_console import load_glossary - - return "Glossary", render.render_glossary(load_glossary()), None - - ROUTES: dict[str, Callable[[ServeConfig, dict[str, list[str]]], tuple[str, str, int | None]]] = { # First-run detection (#437): every page that reads the database serves the checklist when # there is no database to read, rather than a 500 whose real cause is that the user has not - # set anything up yet. `/venues`, `/gates` and `/glossary` are not wrapped -- none of them - # touches the deployment, and all three are useful before one exists. + # set anything up yet. `/venues` and `/gates` are not wrapped -- neither touches the + # deployment, and both are useful before one exists. "/": needs_database(page_status), "/setup": page_setup, "/activity": needs_database(page_activity), @@ -298,7 +292,6 @@ def page_glossary(_cfg: ServeConfig, _query: dict[str, list[str]]) -> tuple[str, "/rules": needs_database(page_rules), "/venues": page_venues, "/gates": page_gates, - "/glossary": page_glossary, } @@ -466,6 +459,22 @@ def run_setup_action(cfg: ServeConfig, key: str, form: dict[str, str]) -> Any: _JSON_CONTENT_TYPE = "application/json; charset=utf-8" +def _docs_version(cfg: ServeConfig) -> str: + """The build the nav's documentation link should report, or `""`. + + Read off `build_info` rather than off `cfg.build`, because those are different strings and + only one of them is a version: `cfg.build` is the footer's human-readable LINE + (`keel 0.11.2+c1634a3fa17f (DIRTY) [checkout]`), and putting it in a query string produced + `?v=keel%200.11.2%2B...%20%28DIRTY%29%20%5Bcheckout%5D`. This is the same field `/api/config` + hands the client for the same purpose (`payload.config_document`'s `"build"`), so both + front-ends report the identical string while both exist. + + `""` when there is no build info at all -- an unversioned link is honest, and a link claiming + `?v=unknown` is not. + """ + return str(getattr(cfg.build_info, "full_version", "") or "") + + def _static_headers(content_type: str) -> tuple[tuple[str, str], ...]: """`_STATIC_BASE_HEADERS` plus CSP, but ONLY when `content_type` is one of `_CSP_CONTENT_TYPES` -- see the comments on `_STATIC_BASE_HEADERS` and `_CSP_CONTENT_TYPES` @@ -571,6 +580,7 @@ def _refuse(self, code: int, heading: str, detail: str) -> None: path="", body=render.render_message(heading, detail), build=self.cfg.build, + version=_docs_version(self.cfg), ), ) @@ -906,6 +916,7 @@ def do_GET(self) -> None: # noqa: N802 - stdlib's naming, not ours path=parsed.path, body=body, build=self.cfg.build, + version=_docs_version(self.cfg), refresh_sec=refresh, ), ) diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index db9962e..de68061 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -181,6 +181,22 @@ h2 { font-size: 1.05rem; margin: 2rem 0 0.6rem; } } .kv .v { font-size: 1.05rem; font-variant-numeric: tabular-nums; } +/* #539: a label that names a documented term links out to its definition. + `--muted`, NOT `--accent`: the label is the quietest text on the card and a link colour there + would pull the eye off the figure beside it, which is the thing the operator came to read. The + dotted underline is what marks it as a link -- and it is an underline rather than colour alone, + because #532's whole finding was that this palette had been separating meanings by hue. + `text-decoration-thickness` is set because a dotted border-bottom would sit below the descenders + and read as a divider instead. */ +.kv .k .doclink { + color: inherit; + text-decoration: underline dotted; + text-underline-offset: 0.2em; + text-decoration-thickness: 1px; +} +.kv .k .doclink:hover, +.kv .k .doclink:focus-visible { color: var(--fg); text-decoration-style: solid; } + .tablewrap { overflow-x: auto; /* A scrollable region must be reachable by keyboard. `tabindex="0"` on the wrapper (set in diff --git a/keel/web/static/index.html b/keel/web/static/index.html index 52b1923..f6ec75c 100644 --- a/keel/web/static/index.html +++ b/keel/web/static/index.html @@ -71,6 +71,23 @@
  • Rules
  • Venues
  • Gates
    • + +
  • Docs
    • diff --git a/keel/web/static/js/docs.js b/keel/web/static/js/docs.js new file mode 100644 index 0000000..adeb0e1 --- /dev/null +++ b/keel/web/static/js/docs.js @@ -0,0 +1,141 @@ +// @ts-check +/** + * Outbound documentation links (#539). **The app fetches, bundles and caches nothing.** + * + * ── WHY LINKING IS THE ONLY OPTION, NOT THE CHEAP ONE ─────────────────────────────────────── + * `docs/` lives at the REPOSITORY root, outside `keel/`, and `uv_build` packages the module root + * -- so no wheel has ever carried it. Every installed deployment, the signed desktop bundle + * included, renders an empty glossary today, and `keel/commands/help_console.py` says so in its + * own docstring: "an installed deployment has no docs/ checkout, and the help screen renders + * that notice as its empty state." + * + * That is not fixable by adding a packaging glob -- it is structural. Measured at #535: building + * with `artifacts` set and with `artifacts = []` produces byte-identical wheels, because + * `uv_build` ships the whole module root regardless of the key. Linking out is the only option + * that reaches an installed deployment at all. + * + * ── NO OFFLINE FALLBACK, DELIBERATELY ─────────────────────────────────────────────────────── + * No inline definitions, no cached snapshot, no entry in the service worker's `PRECACHE`. An + * operator running a trading engine has network by definition, and #538's whole argument is that + * a cached copy of something authoritative is worse than no copy: a definition that has since + * changed, presented as current, with nothing on screen to say which it is. + * + * ── THE ANCHOR CONTRACT, AND WHY A TEST HOLDS IT ──────────────────────────────────────────── + * `docs/glossary.md` states its own rule -- "Each entry is a `## term` heading, a definition, + * and a `Source:` line" -- and Astro emits kebab-cased IDs for those headings, so the anchor for + * a term is its heading kebab-cased. Nothing in either repository enforces that from the other + * side: a heading renamed upstream would break every deep link here **silently**, because a bad + * fragment is not an error, it is a page that opens at the top. + * + * `tests/web/test_doc_links.py` closes that by parsing this table and asserting every anchor + * exists as a heading in the named document, in this repository, where `docs/` is the source. + */ + +/** The published documentation root. One string, spelled once. */ +const SITE = "https://keeltrading.com/en/docs/"; + +/** + * The running build, for `?v=`. + * + * **Version skew is made VISIBLE here, not solved.** The site pins `main` while an operator runs + * a tagged release, so a linked page can describe behaviour their build does not have. Per- + * version documentation paths were rejected: `keeltrading.com/en/docs/v0.11.0/glossary` 404s + * today, and building versioned trees is work in the other repository plus a retention policy, + * across three languages and a sitemap. Carrying the version in the query string costs nothing + * and puts the operator's build in the URL bar of the page they are reading. + * + * Module state, written exactly once, at boot, by `main.js` -- the alternative is threading a + * version string through every render function to reach the four places that build a link. + */ +let version = ""; + +/** + * Record the build every documentation link should carry. Called once from `main.js`, from the + * same `/api/config` read that fills the footer. + * + * @param {string} build + */ +export function rememberVersion(build) { + version = typeof build === "string" ? build : ""; +} + +/** + * The URL for one document, optionally at one anchor. + * + * The trailing slash on the slug is not cosmetic: the site serves `…/docs/glossary/index.html`, + * and the un-slashed form is a redirect that some browsers resolve by dropping the fragment -- + * a deep link that lands at the top of the page, which is exactly the failure this module's + * anchor table exists to prevent. + * + * @param {string} slug a document slug, or `""` for the documentation index. + * @param {string} anchor a heading anchor, or `""` for the top of the page. + * @returns {string} + */ +export function documentUrl(slug, anchor) { + let url = SITE; + if (slug) url = url + slug + "/"; + if (version) url = url + "?v=" + encodeURIComponent(version); + if (anchor) url = url + "#" + anchor; + return url; +} + +/** The documentation index, for the header's outbound link. @returns {string} */ +export function indexUrl() { + return documentUrl("", ""); +} + +/** + * The labels this client puts on screen that name a term the documentation defines, mapped to + * where it is defined. + * + * **Keyed by the LABEL, not by the term.** The alternative -- tagging each call site with a term + * name -- spreads the decision across six views and makes "which words on this screen are + * defined somewhere" unanswerable without reading all of them. Keyed by label, the whole answer + * is this table, and `kv` consults it for every pair it builds, so a label that names a term is + * a link wherever it appears without a call site knowing. + * + * **Deliberately NOT exhaustive, and the omissions are the point.** `mode` reads `paper` or + * `live` and would need two different targets for one label; `evidence required` sits on the + * CAPABILITY gates (`keel.capabilities.GATES`), not the promotion gate, and linking it to + * `#promotion-gate` would be confidently wrong. A missing link costs a reader one search. A + * wrong one costs them their trust in every other link on the page. + * + * @type {Record} + */ +export const TERMS = { + autonomy: { slug: "glossary", anchor: "autonomy" }, + "autonomy configured": { slug: "glossary", anchor: "autonomy" }, + "autonomy lapses": { slug: "glossary", anchor: "autonomy" }, + "kill switch": { slug: "glossary", anchor: "kill-switch" }, + "rail 11": { slug: "glossary", anchor: "rail" }, + "withdrawal attestation (rail 17)": { slug: "glossary", anchor: "attestation" }, + "market session": { slug: "glossary", anchor: "market-clock" }, + session: { slug: "glossary", anchor: "session-bound-venue" }, + "paper stage": { slug: "glossary", anchor: "paper-mode" }, +}; + +/** + * A label as an outbound link to its definition, or `null` if the label names no term. + * + * `rel="noopener"` with `target="_blank"`: a new tab opened without it gets a `window.opener` + * handle back to this page, and this page is a trading console on a token-bearing origin. + * `noreferrer` too -- the server already sends `Referrer-Policy: no-referrer`, and a link that + * states it as well is one that keeps holding if this markup is ever read somewhere the header + * is not sent. + * + * @param {string} label + * @returns {HTMLAnchorElement | null} + */ +export function termLink(label) { + const target = Object.prototype.hasOwnProperty.call(TERMS, label) ? TERMS[label] : null; + if (!target) return null; + const anchor = document.createElement("a"); + anchor.className = "doclink"; + anchor.textContent = label; + anchor.href = documentUrl(target.slug, target.anchor); + anchor.target = "_blank"; + anchor.rel = "noopener noreferrer"; + // Named for a reader who arrives on the link out of context, and hears only the link text. + anchor.title = "Definition on keeltrading.com — opens in a new tab"; + return anchor; +} diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index be84587..bbd9eb2 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -34,6 +34,7 @@ */ import { read } from "./api.js"; +import { indexUrl, rememberVersion } from "./docs.js"; import { available, subscribe } from "./live.js"; import { activityView, @@ -127,6 +128,8 @@ const engineNode = must("engine"); const contentNode = must("content"); /** @type {HTMLElement} */ const buildNode = must("build"); +/** The header's outbound documentation link (#539); its href gains `?v=` once the build is known. */ +const docsNode = /** @type {HTMLAnchorElement} */ (must("docs-link")); /** * An element that `index.html` guarantees. Throwing beats rendering half a page: the two files @@ -619,16 +622,26 @@ function registerWorker(config) { } /** - * The footer's build line, read once. + * The build, read once, and the three things that depend on it. * * Once, not per poll: `/api/config` describes the binary that is answering, and that cannot * change without the process restarting -- at which point the session token is new, every fetch * is a 403, and the banner says so. A version string re-read four times a minute would be four * times a minute spent confirming a constant. + * + * **The first view is painted from INSIDE this callback (#539), and that ordering is deliberate.** + * Every documentation link carries `?v=` (`docs.rememberVersion`), and a link built before + * the build is known would carry no version until the next poll -- fifteen seconds of links that + * quietly do not say which build the reader is running, on exactly the first screen they see. + * `/api/config` is the one endpoint that opens no database, so this costs a single round trip on + * a loopback socket, and it cannot hang the app: `api.read` resolves with a stopped reading + * rather than rejecting, so `show` runs even with nothing listening on the port. */ void read("config").then((reading) => { - buildLine(buildNode, reading.data); - registerWorker(reading.data); + const config = reading.data; + rememberVersion((config && (config.build || config.version)) || ""); + docsNode.href = indexUrl(); + buildLine(buildNode, config); + registerWorker(config); + show(booted, false); }); - -show(booted, false); diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index 64f7d11..dd7efe2 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -56,6 +56,7 @@ */ import { equityChart } from "./chart.js"; +import { termLink } from "./docs.js"; import { instant } from "./format.js"; /** @@ -158,7 +159,14 @@ function plain(value) { */ function kv(label, value) { const wrap = el("div", "kv"); - wrap.append(el("span", "k", label)); + // #539: a label that names a documented term becomes an outbound link to its definition, + // here rather than at each call site. `docs.TERMS` is the whole answer to "which words on + // this screen are defined somewhere", and a label it does not know stays plain text. + const linked = termLink(label); + const key = el("span", "k"); + if (linked) key.append(linked); + else key.textContent = label; + wrap.append(key); const holder = el("span", "v"); if (value instanceof Node) holder.append(value); else if (typeof value === "string") holder.textContent = value; diff --git a/keel/web/static/sw.js b/keel/web/static/sw.js index 82507f5..a7c463c 100644 --- a/keel/web/static/sw.js +++ b/keel/web/static/sw.js @@ -70,6 +70,7 @@ const PRECACHE = [ `${BASE}css/keel.css`, `${BASE}js/api.js`, `${BASE}js/chart.js`, + `${BASE}js/docs.js`, `${BASE}js/format.js`, `${BASE}js/live.js`, `${BASE}js/main.js`, diff --git a/tests/commands/test_console_thinness.py b/tests/commands/test_console_thinness.py index 0491a49..51cbc01 100644 --- a/tests/commands/test_console_thinness.py +++ b/tests/commands/test_console_thinness.py @@ -102,10 +102,17 @@ def _console_module_paths() -> list[str]: #: hand-rolling percent-decoding on attacker-influenced input, which is a strictly worse trade #: than one named, scoped allowance. #: -#: Scoped to the module and the exact import, so it cannot widen: `urllib.request` in either file +#: `render` joined them at #539, for the same reason in the other direction: it BUILDS a query +#: string rather than splitting one. The nav's documentation link carries `?v=`, a full +#: version is `0.11.2+c1634a3fa17f`, and a raw `+` in a query string decodes to a space -- so the +#: choice was `quote(build, safe="")` or a hand-rolled encoder for "the characters a version +#: string might contain", which is a guess about an alphabet rather than a rule about one. +#: (`render` is deleted at #540 and this entry goes with it.) +#: +#: Scoped to the module and the exact import, so it cannot widen: `urllib.request` in any of them #: still fails, and `urllib.parse` anywhere else still fails. RULE5_IMPORT_ALLOWLIST: frozenset[tuple[str, str]] = frozenset( - {("server", "urllib.parse"), ("staticfiles", "urllib.parse")} + {("server", "urllib.parse"), ("staticfiles", "urllib.parse"), ("render", "urllib.parse")} ) diff --git a/tests/web/test_client_assets.py b/tests/web/test_client_assets.py index 29d80b8..22e46f1 100644 --- a/tests/web/test_client_assets.py +++ b/tests/web/test_client_assets.py @@ -52,17 +52,16 @@ #: appearing under `js/` without a test author noticing fails `test_the_client_ships_exactly_the_ #: declared_modules` rather than shipping unexamined. #: -#: `chart.js` and `live.js` arrived at #537, which is what the previous revision of this line -#: predicted ("a fifth module is a design decision (#537 adds `chart`, `live`, `docs`, `sw`) and -#: should arrive with the list updated, not silently"). `docs` (#539) is still to come, and its -#: absence here is what will make it arrive the same way. +#: `chart.js` and `live.js` arrived at #537 and `docs.js` at #539, which is what the first +#: revision of this line predicted ("a fifth module is a design decision (#537 adds `chart`, +#: `live`, `docs`, `sw`) and should arrive with the list updated, not silently"). #: #: **`sw` arrived at #538 and is deliberately NOT in this list**, because it is not under `js/`: #: a service worker's scope is its own directory, so `js/sw.js` would be scoped to `/static/js/` #: and could not answer a navigation to `/static/insights`. It sits at the static root instead, #: and `tests/web/test_pwa.py::test_the_worker_is_served_from_the_scope_it_must_control` asserts #: that placement rather than leaving it to whoever next reads the spec's file list. -_MODULES = ("main.js", "api.js", "render.js", "chart.js", "live.js", "format.js") +_MODULES = ("main.js", "api.js", "render.js", "chart.js", "live.js", "format.js", "docs.js") #: The modules held to "no arithmetic, no judgement, no derived display string", and the ONLY two. #: @@ -567,9 +566,31 @@ def test_the_client_loads_nothing_from_a_third_party_origin() -> None: a browser. This asserts we never ship the attempt, so the header is never the only thing standing between the page and an external request.""" html = _markup_only(_INDEX.read_text(encoding="utf-8")) - remote = re.findall(r'(?:src|href)="((?:https?:)?//[^"]*)"', html) + + # **`` is navigation, not a load, and #539 makes that distinction load-bearing.** The + # documentation link opens keeltrading.com in a new tab; nothing is fetched into this page, + # nothing is bundled and nothing is cached, so `default-src 'self'` is untouched by it (the + # spec: "Outbound links are navigation, not connections"). Every OTHER way a URL can appear in + # this markup is a subresource, and those stay same-origin: `src` on a script or an image, + # and `href` on a `` -- the stylesheet, the manifest and the icons. + # + # The check is narrowed rather than dropped. An earlier spelling of this test matched `href` + # anywhere, which would now pass only by listing the docs URL as an exception -- and an + # exception list is what turns a rule into a habit of adding to a list. + loads = re.findall(r'<(?:script|img|iframe|source|embed)\b[^>]*\bsrc="([^"]*)"', html) + loads += re.findall(r']*\bhref="([^"]*)"', html) + remote = [url for url in loads if url.startswith(("http://", "https://", "//"))] assert remote == [], f"index.html loads from another origin: {remote}" + # And what IS allowed outbound is exactly one link, to the documentation, opened safely. + anchors = re.findall(r"]*>", html) + outbound = [tag for tag in anchors if re.search(r'href="(?:https?:)?//', tag)] + assert len(outbound) == 1, f"expected one outbound link, found {len(outbound)}: {outbound}" + assert 'href="https://keeltrading.com/en/docs/"' in outbound[0], outbound[0] + # `noopener` is the one that matters: a tab opened without it holds a `window.opener` handle + # back to a trading console on a token-bearing origin. + assert 'rel="noopener noreferrer"' in outbound[0], outbound[0] + css = _CSS.read_text(encoding="utf-8") assert "@import" not in css, "a stylesheet that imports another can import a remote one" # `url()` covers fonts, background images and cursors in one check. There are none today, and diff --git a/tests/web/test_doc_links.py b/tests/web/test_doc_links.py new file mode 100644 index 0000000..ba5ece4 --- /dev/null +++ b/tests/web/test_doc_links.py @@ -0,0 +1,335 @@ +"""Documentation is linked, never embedded (#539) -- and the links are checked against `docs/`. + +**Why this module exists at all.** A broken deep link is not an error. `…/glossary/#rial` opens +the glossary at the top of the page, looking exactly like a link to a term that happens not to +scroll, and nothing anywhere reports it. keel's `docs/` is the SOURCE and keeltrading.com is the +mirror (`engine-docs.manifest.json` pins `CodeGateSoftware/keel@main`), so this repository is the +one place where a rename and the links that depend on it can be compared at all -- and this is +the comparison. + +The anchor contract is the one `docs/glossary.md` states about itself: "Each entry is a `## term` +heading, a definition, and a `Source:` line." Astro's slugger kebab-cases those headings into +ids, and `_slug` below reproduces that transformation. If the site ever changes slugger, this +module is what fails. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +from keel.web import render, staticfiles + +_REPO = Path(__file__).resolve().parents[2] +_DOCS = _REPO / "docs" +_DOCS_JS = staticfiles.STATIC_ROOT / "js" / "docs.js" +_RENDER_JS = staticfiles.STATIC_ROOT / "js" / "render.js" +_INDEX = staticfiles.STATIC_ROOT / "index.html" + +#: Slug -> the document in THIS repository it is published from. Mirrors +#: `keeltrading.com/engine-docs.manifest.json`, which is the site's own pin of the same pairs. +#: Only the slugs the app links to need an entry; a link to a slug absent here fails below, +#: which is the correct outcome for a link to a document nobody has confirmed is published. +_PUBLISHED: dict[str, str] = { + "glossary": "glossary.md", + "fiqh-basis": "fiqh-basis.md", + "operator-runbook": "operator-runbook.md", + "go-live-runbook": "go-live-runbook.md", +} + + +def _slug(heading: str) -> str: + """A `## heading` as the id the built site gives it. + + Lowercase, drop everything that is not a letter, digit, space or hyphen, then spaces to + hyphens -- GitHub's slugger, which is what `rehype-slug` implements and what the built site + was verified to emit (`id="rail"`, `id="instrument-attestation"`, `id="kill-switch"`). + """ + text = heading.strip().lower() + text = re.sub(r"[^\w\s-]", "", text, flags=re.UNICODE) + return re.sub(r"[\s_]+", "-", text).strip("-") + + +def _anchors(document: str) -> set[str]: + """Every anchor a document offers, from its `##`-and-deeper headings.""" + source = (_DOCS / document).read_text(encoding="utf-8") + return {_slug(match) for match in re.findall(r"^#{2,6}\s+(.+)$", source, re.MULTILINE)} + + +def _terms() -> dict[str, tuple[str, str]]: + """`docs.js`'s `TERMS`, as `{label: (slug, anchor)}`. + + Parsed rather than imported, for the same reason `test_client_assets.py` parses `main.js`'s + route table: there is no JavaScript runtime here. The parse is narrow on purpose, and + `test_the_term_parser_actually_found_something` is what stops a rewritten table from silently + matching nothing. + """ + source = _DOCS_JS.read_text(encoding="utf-8") + block = re.search(r"export const TERMS = \{(.*?)\n\};", source, re.DOTALL) + assert block is not None, "TERMS is not in the shape this parser understands" + found: dict[str, tuple[str, str]] = {} + pattern = re.compile( + r'^\s*(?:"(?P[^"]+)"|(?P[A-Za-z_][\w$]*))\s*:\s*' + r'\{\s*slug:\s*"(?P[^"]+)"\s*,\s*anchor:\s*"(?P[^"]+)"\s*\}\s*,\s*$' + ) + for line in block.group(1).splitlines(): + if not line.strip() or line.strip().startswith("//"): + continue + match = pattern.match(line) + assert match is not None, f"unparsed TERMS entry: {line}" + label = match.group("quoted") or match.group("bare") + found[label] = (match.group("slug"), match.group("anchor")) + return found + + +# -- the acceptance criterion ---------------------------------------------------------------------- + + +def test_every_anchor_the_app_emits_exists_in_the_source_document() -> None: + """**The acceptance criterion, and the reason a rename upstream cannot break a link quietly.** + + Each `(slug, anchor)` the client can emit is resolved to a document in `docs/` and checked + against the headings that document actually has. + """ + for label, (slug, anchor) in sorted(_terms().items()): + assert slug in _PUBLISHED, ( + f"{label!r} links to slug {slug!r}, which is not a published document -- add it to " + "_PUBLISHED here and to keeltrading.com's engine-docs.manifest.json, or link " + "somewhere that exists" + ) + available = _anchors(_PUBLISHED[slug]) + assert anchor in available, ( + f"{label!r} links to #{anchor} in docs/{_PUBLISHED[slug]}, which has no such heading. " + f"A heading was probably renamed; the link would open the page at the top and report " + f"nothing. Closest available: {sorted(a for a in available if anchor[:4] in a)}" + ) + + +def test_the_anchor_check_would_notice_a_renamed_heading() -> None: + """Mutation: the assertion above compares against real headings, not against anything. + + Without this, a `_anchors` that returned everything -- or a regex that matched nothing and so + made the set empty in a way `in` happened to tolerate -- would leave the criterion green and + meaningless. + """ + real = _anchors("glossary.md") + assert "kill-switch" in real, "the heading parser found no known term" + assert "kill-switch-renamed-upstream" not in real + + +def test_the_term_parser_actually_found_something() -> None: + """Guards every assertion that iterates `TERMS` against an empty parse.""" + terms = _terms() + assert len(terms) >= 5, terms + assert terms["kill switch"] == ("glossary", "kill-switch") + + +def test_the_slugger_matches_the_ids_the_built_site_emits() -> None: + """The transformation, pinned against ids observed in the built site rather than assumed. + + `dist/en/docs/glossary/index.html` was checked at #531 and carries `id="rail"`, + `id="attestation"`, `id="instrument-attestation"`, `id="kill-switch"`, `id="qabd"`, + `id="riba"`. These are those cases run backwards through `_slug`. + """ + assert _slug("rail") == "rail" + assert _slug("instrument attestation") == "instrument-attestation" + assert _slug("kill switch") == "kill-switch" + assert _slug("qabd") == "qabd" + assert _slug("DCA benchmark") == "dca-benchmark" + assert _slug("session-bound venue") == "session-bound-venue" + + +# -- the table cannot rot ---------- + + +def test_every_linked_label_is_a_label_the_client_actually_puts_on_screen() -> None: + """The other direction, and the one that keeps the table honest as views change. + + `kv` links a label by looking it up, so a label renamed in `render.js` does not break -- it + just silently stops being a link, and the entry here becomes an entry for a label that no + longer exists. This is what turns that into a failure. + """ + source = _RENDER_JS.read_text(encoding="utf-8") + emitted = set(re.findall(r'kv\("([^"]*)"', source)) + assert emitted, "no kv labels found -- this test would prove nothing" + for label in sorted(_terms()): + assert label in emitted, ( + f"{label!r} is in docs.TERMS but no view emits it as a kv label; it was probably " + "renamed in render.js, where the link would have vanished without a word" + ) + + +def test_the_ambiguous_labels_are_deliberately_absent() -> None: + """A wrong link costs more than a missing one, and these two are the wrong ones. + + `mode` reads `paper` or `live` -- one label, two definitions, and no way to pick. `evidence + required` sits on the CAPABILITY gates (`keel.capabilities.GATES`), not the promotion gate, so + `#promotion-gate` would be confidently and invisibly wrong. Pinned so that "the table looks + incomplete" never becomes a reason to complete it. + """ + terms = _terms() + assert "mode" not in terms + assert "evidence required" not in terms + + +# -- nothing is fetched, bundled or cached ---------- + + +def test_the_app_fetches_no_documentation() -> None: + """`docs.js` builds URLs and returns anchors. It opens no connection. + + `test_client_assets.py::test_fetch_appears_in_exactly_one_client_module` already pins that + `fetch` lives only in `api.js`; this is the narrower statement the issue asks for -- the + documentation module in particular never reads a document. + """ + source = _DOCS_JS.read_text(encoding="utf-8") + code = "\n".join( + line for line in source.splitlines() if not line.lstrip().startswith(("*", "/*", "//")) + ) + for forbidden in ("fetch(", "XMLHttpRequest", "EventSource", "import("): + assert forbidden not in code, f"docs.js reaches the network via {forbidden}" + + +def test_no_documentation_is_precached() -> None: + """#538's worker must not hold a copy either. A cached definition that has since changed, + presented as current, is the same failure as a cached balance in a milder register.""" + precache = (staticfiles.STATIC_ROOT / "sw.js").read_text(encoding="utf-8") + assert "keeltrading.com" not in precache + for name in sorted(p.name for p in _DOCS.glob("*.md")): + assert name not in precache, f"{name} is precached; documentation is linked, not shipped" + + +def test_no_documentation_prose_ships_inside_the_client() -> None: + """The definitions themselves stay in `docs/`. A copy in the client is a second source that + drifts, and drifts silently, because nothing compares them.""" + glossary = (_DOCS / "glossary.md").read_text(encoding="utf-8") + definitions = [ + line.strip() + for line in glossary.splitlines() + if len(line.strip()) > 60 and not line.startswith(("#", "Source:", "-", ">")) + ] + assert definitions, "no definitions found in the glossary -- this test would prove nothing" + client = "\n".join( + path.read_text(encoding="utf-8") + for path in sorted(staticfiles.STATIC_ROOT.rglob("*")) + if path.is_file() and path.suffix in (".js", ".html", ".css") + ) + for definition in definitions: + assert definition not in client, ( + f"a glossary definition is embedded in the client: {definition[:60]!r}" + ) + + +# -- the version, and the one URL ---------- + + +def test_links_carry_the_running_version() -> None: + """Version skew is made visible rather than solved: the site pins `main` while an operator + runs a tagged release, so the build goes in the URL bar of the page they are reading.""" + source = _DOCS_JS.read_text(encoding="utf-8") + assert 'url = url + "?v=" + encodeURIComponent(version)' in source + main = (staticfiles.STATIC_ROOT / "js" / "main.js").read_text(encoding="utf-8") + assert "rememberVersion(" in main, "nothing ever tells docs.js which build is running" + + +def test_the_first_paint_already_knows_the_version() -> None: + """`show` is called from inside the `/api/config` callback, so no link is ever built before + the build is known -- otherwise the first screen an operator sees carries links that do not + say which build they are reading about, for a whole poll interval.""" + main = (staticfiles.STATIC_ROOT / "js" / "main.js").read_text(encoding="utf-8") + config_block = main[main.index('void read("config")') :] + assert "show(booted, false);" in config_block, ( + "the first paint no longer waits for the build; documentation links would be unversioned " + "until the first poll" + ) + assert main.count("show(booted, false);") == 1, "the first paint happens twice" + + +def test_the_rendered_pages_version_their_documentation_link_too(running) -> None: # type: ignore[no-untyped-def] + """Both front-ends exist until #540, and the criterion is not "the client carries `?v=`". + + `quote(..., safe="")` rather than an f-string: a full version is `0.11.2+c1634a3fa17f`, and a + raw `+` in a query string decodes to a space. + """ + from tests.web.test_server import _request, _session + + _status, _headers, body = _request(running, "/", cookie=_session(running)) + assert render.DOCS_URL in body + # The fixture's build may be empty, in which case no `?v=` is correct -- an empty version is + # not a version to report. Asserted through the renderer instead, where a build is present. + versioned = render.page( + title="t", + path="/", + body="", + # The two are DIFFERENT strings, and passing both here is the point of this assertion: + # `build` is the footer's human-readable line and the first spelling of this feature put + # it in the query, yielding `?v=keel%200.11.2%2B...%20%28DIRTY%29%20%5Bcheckout%5D`. + build="keel 0.11.2+c1634a3fa17f (DIRTY) [checkout]", + version="0.11.2+c1634a3fa17f", + ) + assert render.DOCS_URL + "?v=0.11.2%2Bc1634a3fa17f" in versioned, ( + "the rendered nav's documentation link carries no version, or carries a raw `+`" + ) + assert "keel%200.11.2" not in versioned, "the build LINE leaked into the query string" + assert 'rel="noopener noreferrer"' in versioned + + +def test_the_documentation_root_is_spelled_the_same_in_both_front_ends() -> None: + """`render.py` and `docs.js` both link out while both front-ends exist. Two spellings is two + things to update at #540, and one of them would be missed.""" + js = _DOCS_JS.read_text(encoding="utf-8") + site = re.search(r'const SITE = "([^"]+)"', js) + assert site is not None + assert site.group(1) == render.DOCS_URL + assert render.DOCS_URL in _INDEX.read_text(encoding="utf-8") + + +# -- the deletions ---------- + + +def test_the_glossary_page_is_gone() -> None: + """`/glossary` rendered a file no installed deployment has. A link replaced it.""" + from keel.web import server + + assert "/glossary" not in server.ROUTES + assert not hasattr(render, "render_glossary") + assert not hasattr(server, "page_glossary") + + +def test_the_web_layer_no_longer_reads_the_glossary_file() -> None: + """`help_console.load_glossary` stays for the TUI until #541, but nothing under `keel/web/` + calls it any more -- the whole point being that the file is not there to read. + + Read through `ast` rather than by substring, and the difference is not fastidiousness: the + NAV comment in `render.py` explains this deletion by NAMING `load_glossary`, and a substring + scan would fail on the prose that documents the change. An AST sees identifiers, so a + docstring can say the word and only a call can fail the test. + """ + import ast + + for path in sorted((_REPO / "keel" / "web").rglob("*.py")): + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + used = { + node.id if isinstance(node, ast.Name) else node.attr + for node in ast.walk(tree) + if isinstance(node, (ast.Name, ast.Attribute)) + } + used |= { + alias.name + for node in ast.walk(tree) + if isinstance(node, ast.ImportFrom) + for alias in node.names + } + assert "load_glossary" not in used, path + assert "parse_glossary" not in used, path + + +@pytest.mark.parametrize("path", ["/glossary", "/static/glossary"]) +def test_the_glossary_path_is_a_404_on_both_front_ends(path: str, running) -> None: # type: ignore[no-untyped-def] + """Over the wire, on the rendered pages and on the client's own prefix.""" + from tests.web.test_server import _request, _session + + status, _headers, _body = _request(running, path, cookie=_session(running)) + assert status == 404, path diff --git a/tests/web/test_pwa.py b/tests/web/test_pwa.py index 9653268..e4e08f8 100644 --- a/tests/web/test_pwa.py +++ b/tests/web/test_pwa.py @@ -332,12 +332,13 @@ def test_the_client_registers_the_worker_only_after_a_successful_config_read() - leave the installed worker alone: it is the thing letting the operator read the page. """ main = (_STATIC / "js" / "main.js").read_text(encoding="utf-8") - assert "registerWorker(reading.data)" in main, ( + config_block = main[main.index('void read("config")') :] + assert "registerWorker(config)" in config_block, ( "the worker is not registered from the config read" ) assert re.search( r"const build = \(config && \(config\.build \|\| config\.version\)\) \|\| \"\";", main - ) + ), "registerWorker no longer derives the build from the config document" assert "if (!build) return;" in main, "a failed config read must register nothing" diff --git a/tests/web/test_render.py b/tests/web/test_render.py index f2999e2..a8f3cb4 100644 --- a/tests/web/test_render.py +++ b/tests/web/test_render.py @@ -11,7 +11,6 @@ from decimal import Decimal from keel.commands.brokers import BrokerInfo -from keel.commands.help_console import GlossaryTerm from keel.web import render XSS = '' @@ -70,20 +69,13 @@ def test_a_failed_adapter_row_shows_the_error_and_not_the_placeholders() -> None assert "WIRED" not in html -def test_a_fiqh_term_that_fiqh_basis_does_not_state_says_so() -> None: - """`stated=False` means fiqh-basis does not define the term and the glossary entry says that - rather than substituting a help-authored summary. Losing that marker in a new front-end would - turn a disclaimed gap into an apparent citation.""" - term = GlossaryTerm( - term="something", - definition="fiqh-basis does not state this.", - source="", - citation=None, - fiqh=True, - stated=False, - ) - html = render.render_glossary([term]) - assert "not stated in fiqh-basis" in html +# `test_a_fiqh_term_that_fiqh_basis_does_not_state_says_so` lived here and went with +# `render_glossary` at #539. The property it protected did NOT go: the "not stated" disclaimer is +# written into the definition text in `docs/glossary.md` itself -- which is why +# `help_console.parse_glossary` can DERIVE `stated` from it (`stated = "not stated" not in +# source.lower()`), and why `tests/commands/test_help_console.py` asserts it on the gharar entry. +# A reader following the deep link lands on that prose. What was deleted is a renderer for a file +# no installed deployment has ever had. def test_utc_is_used_and_a_broken_timestamp_does_not_raise() -> None: diff --git a/tests/web/test_server.py b/tests/web/test_server.py index fa2051d..714f1a7 100644 --- a/tests/web/test_server.py +++ b/tests/web/test_server.py @@ -35,7 +35,6 @@ "/rules", "/venues", "/gates", - "/glossary", ) @@ -969,10 +968,19 @@ def test_the_printed_url_is_the_one_that_carries_the_token( def test_the_nav_and_the_routing_table_agree() -> None: """A page with no nav entry is unreachable; a nav entry with no page is a 404 the user is - invited to click. Neither is caught by testing either side alone.""" + invited to click. Neither is caught by testing either side alone. + + **The nav has one entry that is deliberately not a route (#539).** `Docs` links out to + keeltrading.com, because `docs/` has never shipped inside a wheel and the page that used to + render it was empty in every installed deployment. It is separated here by its scheme rather + than by its label, so a second outbound entry needs no edit and an internal entry that loses + its route still fails.""" from keel.web import render - assert {href for href, _label in render.NAV} == set(web_server.ROUTES) + internal = {href for href, _label in render.NAV if not href.startswith("https://")} + outbound = {href for href, _label in render.NAV if href.startswith("https://")} + assert internal == set(web_server.ROUTES) + assert outbound == {render.DOCS_URL}, "an unexpected outbound nav entry" assert set(ROUTES) == set(web_server.ROUTES), "this test module's list drifted from the server"