diff --git a/keel/web/server.py b/keel/web/server.py
index 6533141..7e1c457 100644
--- a/keel/web/server.py
+++ b/keel/web/server.py
@@ -814,10 +814,30 @@ class KeelServer(ThreadingHTTPServer):
def build_server(cfg: ServeConfig) -> KeelServer:
- handler = type("BoundKeelHandler", (KeelHandler,), {"cfg": cfg})
- family = socket.AF_INET6 if ":" in cfg.host else socket.AF_INET
- server_type = type("BoundKeelServer", (KeelServer,), {"address_family": family})
- return server_type((cfg.host, cfg.port), handler) # type: ignore[return-value]
+ """A server bound to `cfg`, with the address family its host implies.
+
+ **Nested subclasses rather than three-argument `type()`.** Both bind a per-run config onto a
+ handler class and an address family onto a server class, and both are correct at runtime --
+ but `type(name, bases, namespace)` is declared to return plain `type`, so calling it produced
+ an `Any` that flowed straight out of a function annotated `-> KeelServer`. The
+ `# type: ignore[return-value]` that used to sit here did not even name the right error: mypy
+ reported `no-any-return` and noted the code was not covered, which is a suppression that
+ silences nothing while looking like it silences something. Written as classes, the types are
+ checked rather than asserted, and `keel/web/` type-checks under full `--strict`.
+
+ A fresh pair per call is deliberate: `address_family` and `cfg` are class attributes that
+ `http.server` reads during `__init__`, so binding them to the shared classes would make two
+ concurrently-built servers overwrite each other's configuration.
+ """
+ bound_cfg = cfg
+
+ class BoundKeelHandler(KeelHandler):
+ cfg = bound_cfg
+
+ class BoundKeelServer(KeelServer):
+ address_family = socket.AF_INET6 if ":" in cfg.host else socket.AF_INET
+
+ return BoundKeelServer((cfg.host, cfg.port), BoundKeelHandler)
def serve(cfg: ServeConfig, *, echo: Callable[[str], None] = print) -> int:
diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css
index 1bb56c4..c9d2ee8 100644
--- a/keel/web/static/css/keel.css
+++ b/keel/web/static/css/keel.css
@@ -564,3 +564,21 @@ button.run:disabled { cursor: progress; opacity: 0.7; }
it is present on every card from the moment the card is drawn. */
.action-outcome { margin: 0.5rem 0 0; }
.action-outcome:empty { margin: 0; }
+
+/* The "a newer build is ready" offer (#538). Sized like the footer text it sits in rather than
+ like the action buttons: it is an offer an operator may ignore indefinitely, and a page that
+ shouts about an available upgrade while somebody is reading a drawdown has its priorities
+ backwards. `--warn` because it reports a real difference between what is running and what is
+ installed, without claiming anything is broken. */
+button.update {
+ background: none;
+ border: 1px solid var(--control-line);
+ border-radius: 4px;
+ color: var(--warn);
+ font: inherit;
+ font-size: 0.85rem;
+ padding: 0.15rem 0.5rem;
+ cursor: pointer;
+}
+button.update:hover { color: var(--fg); }
+button.update:disabled { cursor: progress; opacity: 0.7; }
diff --git a/keel/web/static/index.html b/keel/web/static/index.html
index 6ac1615..b401d8c 100644
--- a/keel/web/static/index.html
+++ b/keel/web/static/index.html
@@ -136,6 +136,15 @@
This page needs JavaScript
diff --git a/keel/web/static/js/actions.js b/keel/web/static/js/actions.js
new file mode 100644
index 0000000..424ef2a
--- /dev/null
+++ b/keel/web/static/js/actions.js
@@ -0,0 +1,149 @@
+// @ts-check
+/**
+ * The write boundary: this session's write token, the actions performed through it, and what each
+ * one reported.
+ *
+ * ── WHY THIS MODULE EXISTS, AND WHOSE SHAPE IT IS ───────────────────────────────────────────
+ * The design spec's reference implementation is
+ * [youperiod.app](https://github.com/getify/youperiod.app), and its client is five modules with
+ * one job each: `main.js` attaches event listeners, `data-manager.js` owns the storage boundary
+ * behind a `get`/`set` pair, `utils.js` holds shared helpers. keel's client was built to that
+ * shape -- `api.js` is the single `fetch` wrapper, `render.js` the only view builder,
+ * `format.js` the helpers -- and then #540 gave the browser a WRITE surface and put all of it in
+ * `main.js`: the token, the submit handler, the outcome memory, and the sentence-picking. That
+ * is the module the reference keeps emptiest, and it had grown a second job.
+ *
+ * So this is keel's `data-manager.js`. It owns everything about performing an action except the
+ * two things that are not its business: the `fetch` (that is `api.js`, still the only module
+ * that opens a connection) and the DOM (that is `render.js` and the listener in `main.js`).
+ *
+ * ── WHAT IT HIDES ───────────────────────────────────────────────────────────────────────────
+ * A caller needs to know an action's key and the operator's answers. It does not need to know
+ * that a write carries a session-scoped HMAC token in `X-Keel-CSRF` beside `X-Keel-Client`, that
+ * the token arrives on `/api/setup` and nowhere else, that the result document distinguishes
+ * "done" from "already done" through a `changed` flag rather than an error, or that the sentence
+ * to show comes from `data.message.display` on success and `error.detail` on refusal. All of
+ * that is here, and none of it is in the listener.
+ *
+ * ── THE MEMORY IS SESSION STATE, AND IT IS THE ONLY STATE THIS CLIENT KEEPS ─────────────────
+ * Every other thing on screen is a server document re-read on a timer. "What did I just do" is
+ * the one fact no document can answer: `paint` rebuilds the view from `/api/*` on every poll,
+ * every tick and after every action, so an outcome written into a card is gone at the next
+ * rebuild. It is deliberately NOT persisted and dies with the page -- a message about an action
+ * from an hour ago, presented as current, is the same failure #538 refuses to make with a cached
+ * balance, in a much milder register.
+ */
+
+import { runAction } from "./api.js";
+
+/**
+ * @typedef {import("./api.js").Reading} Reading
+ */
+
+/**
+ * This session's write token, from `/api/setup`'s `data.csrf`.
+ *
+ * `""` until a setup document has been read, and that is correct rather than a gap: the only
+ * thing that can submit an action is a form this client drew, and it draws them only on the view
+ * that just supplied the token. An empty token reaches the server and is refused, which is the
+ * right answer for a submission that could not have come from a rendered action card.
+ *
+ * @type {string}
+ */
+let token = "";
+
+/**
+ * What each action last reported, by action key.
+ * @type {Map}
+ */
+const results = new Map();
+
+/**
+ * Take the write token off a `/api/setup` document.
+ *
+ * Called from the view that reads it, once per read. `payload.setup_payload` sends it as a bare
+ * string rather than a `Field` precisely because it is a credential the client SENDS and never
+ * displays; treating it as one here means it never reaches `render.js`.
+ *
+ * @param {any} data `/api/setup`'s `data`, or `null`.
+ */
+export function remember(data) {
+ token = data && typeof data.csrf === "string" ? data.csrf : "";
+}
+
+/**
+ * Whether a write can be attempted at all.
+ *
+ * Exported so a caller can ask rather than infer from a failure. Nothing uses it to HIDE a
+ * button -- "a client that hides a button is not a gate" is the spec's own sentence, and the
+ * server refuses what is not in `keel.commands.setup.ACTIONS` regardless of what this returns.
+ *
+ * @returns {boolean}
+ */
+export function available() {
+ return token !== "";
+}
+
+/**
+ * Perform one declared action and record what it reported.
+ *
+ * @param {string} key an action key from `/api/setup`'s `actions`.
+ * @param {Record} values the declared fields, by name.
+ * @returns {Promise} the sentence to show. Never rejects.
+ */
+export async function perform(key, values) {
+ const reading = await runAction(key, values, token);
+ const outcome = describe(reading);
+ results.set(key, outcome);
+ return outcome;
+}
+
+/**
+ * What an action last reported, or `""` if it has not been run in this session.
+ *
+ * @param {string} key
+ * @returns {string}
+ */
+export function outcomeFor(key) {
+ return results.get(key) ?? "";
+}
+
+/**
+ * Every recorded outcome, for a view being rebuilt.
+ *
+ * A copy, not the live map: a caller iterating this while an action completes would otherwise be
+ * iterating a collection that changed underneath it.
+ *
+ * @returns {Map}
+ */
+export function recorded() {
+ return new Map(results);
+}
+
+/**
+ * The sentence for a finished action -- the SERVER's own words in every branch.
+ *
+ * `data.message.display` when it ran, `error.detail` when it did not. This function chooses which
+ * field to read and never composes a sentence of its own, which is the same rule `render.js`
+ * follows for every value on screen, applied to the one outcome that arrives outside a view.
+ *
+ * **`changed` is appended because it is not a success flag.** Every action is idempotent, so a
+ * repeated submission succeeds and reports `already done -- nothing to change`, which is a true
+ * statement about the deployment rather than a soft failure. Dropping it would make a re-run look
+ * identical to a first run, and the difference is the whole reason `keel.commands.setup` carries
+ * the field.
+ *
+ * @param {Reading} reading
+ * @returns {string}
+ */
+function describe(reading) {
+ const data = reading.data;
+ if (data && data.message && typeof data.message.display === "string") {
+ const changed = data.changed && data.changed.display ? data.changed.display : "";
+ return changed ? data.message.display.concat(" — ", changed) : data.message.display;
+ }
+ const error = reading.error;
+ const detail = error && error.detail ? error.detail : "";
+ const title = error && error.title ? error.title : reading.engine.display;
+ return detail ? title.concat(" — ", detail) : title;
+}
diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js
index 57f9d3b..caecb4d 100644
--- a/keel/web/static/js/main.js
+++ b/keel/web/static/js/main.js
@@ -32,7 +32,8 @@
* it is worth paying.
*/
-import { read, runAction } from "./api.js";
+import { perform, recorded, remember } from "./actions.js";
+import { read } from "./api.js";
import { indexUrl, rememberVersion } from "./docs.js";
import { available, subscribe } from "./live.js";
import {
@@ -129,18 +130,8 @@ const contentNode = must("content");
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"));
-
-/**
- * This session's write token (#540), read off `/api/setup` whenever the setup view mounts.
- *
- * `""` until then, and that is correct rather than a gap: the only thing that can submit a form
- * is a form this client drew, and it draws them only on the view that just supplied the token.
- * An empty token would be refused by the server, which is the right outcome for a submission
- * that could not have come from a rendered action card.
- *
- * @type {string}
- */
-let csrf = "";
+/** Where the "a newer build is ready" offer goes (#538). Empty until there is one. */
+const updateNode = must("update");
/**
* An element that `index.html` guarantees. Throwing beats rendering half a page: the two files
@@ -367,7 +358,7 @@ function mount(route, readings) {
// The write token for this session, kept for the submit handler below. It is read off the
// document that was just fetched rather than stored anywhere: it dies with the process that
// minted it, and a stale one produces a 403 the view shows rather than a silent no-op.
- csrf = (data && typeof data.csrf === "string") ? data.csrf : "";
+ remember(data);
return setupView(data);
}
if (route.name === "activity") {
@@ -567,55 +558,41 @@ contentNode.addEventListener("submit", (event) => {
setBusy(form, true);
showOutcome(key, "Running…");
- void runAction(key, values, csrf).then((reading) => {
- // Recorded BEFORE the repaint, and re-applied after it. Writing straight into the node would
- // put the sentence into an element `paint` is about to replace -- which is exactly what the
- // first version of this did, and the message was never visible for a single frame. Found by
- // clicking the button in a browser; nothing in the suite could have seen it.
- outcomes.set(key, outcomeText(reading));
- showOutcome(key, outcomes.get(key));
+ // The token, the headers, the idempotency semantics and the sentence all live in `actions.js`.
+ // What is left here is what is genuinely this file's job: read the form, show the answer,
+ // repaint. See that module's note on whose shape it is.
+ void perform(key, values).then((outcome) => {
+ showOutcome(key, outcome);
// Re-read the whole view: a step that ran has changed the deployment this page describes, and
- // patching one card would leave the checklist above it saying the opposite.
+ // patching one card would leave the checklist above it saying the opposite. The outcome
+ // survives that rebuild because `rebuildInto` re-applies every recorded one.
void paint(routeFor(window.location.pathname), true, false);
});
});
-/**
- * What each action last reported, by action key.
- *
- * Kept here rather than in the view because the view does not survive: `paint` rebuilds it from
- * the server's documents on every poll, every tick and after every action, and an outcome written
- * into a card is gone at the next rebuild. This is the small amount of state that belongs to the
- * SESSION rather than to the deployment -- "what did I just do", which no document can answer.
- *
- * Not persisted, and cleared by a reload: it describes this sitting at this machine, and a
- * message about an action from an hour ago presented as current is the same failure #538 refuses
- * to make with a cached balance, in a much milder register.
- *
- * @type {Map}
- */
-const outcomes = new Map();
-
/**
* Put an action's outcome into its card, if that card is on screen.
*
* @param {string} key
- * @param {Node | string} text
+ * @param {string} text
*/
function showOutcome(key, text) {
const form = contentNode.querySelector('form[data-action="'.concat(key, '"]'));
const node = form ? form.querySelector(".action-outcome") : null;
if (!node) return;
- node.replaceChildren(typeof text === "string" ? document.createTextNode(text) : text);
+ node.replaceChildren(document.createTextNode(text));
}
-/** Re-apply every remembered outcome after a rebuild has replaced the cards. */
+/**
+ * Re-apply every remembered outcome after a rebuild has replaced the cards.
+ *
+ * Strings rather than nodes, since the memory moved into `actions.js`. That is not only tidier:
+ * a `Node` can be in one place in a document at a time, so a map of nodes had to be cloned on
+ * every restore or the second rebuild would move the only copy out of the map's reach. A string
+ * has no such property, and the bug it invites cannot be written.
+ */
function restoreOutcomes() {
- for (const [key, text] of outcomes) {
- // `cloneNode`: a `Node` can only be in one place in a document, and this map outlives any
- // number of rebuilds. Inserting the same node twice would move it out of the map's reach.
- showOutcome(key, text.cloneNode(true));
- }
+ for (const [key, text] of recorded()) showOutcome(key, text);
}
/**
@@ -630,30 +607,6 @@ function setBusy(form, busy) {
}
}
-/**
- * What to show for a finished action.
- *
- * The SERVER's own words in every branch: `data.message.display` when it ran, `error.detail` when
- * it did not. This function chooses which field to read and never composes a sentence -- the same
- * rule `render.js` follows, applied to the one outcome that arrives outside a view.
- *
- * @param {import("./api.js").Reading} reading
- * @returns {Node}
- */
-function outcomeText(reading) {
- const data = reading.data;
- if (data && data.message && typeof data.message.display === "string") {
- const changed = data.changed && data.changed.display ? data.changed.display : "";
- return document.createTextNode(
- changed ? data.message.display.concat(" — ", changed) : data.message.display,
- );
- }
- const error = reading.error;
- const detail = error && error.detail ? error.detail : "";
- const title = error && error.title ? error.title : reading.engine.display;
- return document.createTextNode(detail ? title.concat(" — ", detail) : title);
-}
-
document.addEventListener("visibilitychange", () => {
if (document.visibilityState === "visible") void paint(current, false);
});
@@ -761,9 +714,82 @@ function registerWorker(config) {
// install an optional cache would be trading a working page for a nicety.
void navigator.serviceWorker
.register(`${BASE}sw.js?v=${encodeURIComponent(build)}`, { scope: BASE })
+ .then((registration) => watchForUpdate(registration))
.catch(() => {});
}
+/**
+ * Offer the operator a new build once one has finished installing (#538, corrected).
+ *
+ * **Why there is a prompt at all.** The worker used to call `skipWaiting()` the moment its cache
+ * was full, which takes over the page currently on screen -- a document rendered by the OLD
+ * build's JavaScript, whose later requests are then answered from the NEW build's cache. `sw.js`
+ * carries the full argument. The fix is that the new worker waits, and this is what tells the
+ * operator it is waiting.
+ *
+ * **Three states, because a worker can already be waiting when this runs.** A registration whose
+ * `waiting` is populated has an update that installed during a previous visit; `updatefound` plus
+ * `installed` catches one that arrives while the page is open. Both funnel to the same offer.
+ *
+ * `navigator.serviceWorker.controller` is the test for "is this an UPDATE or a first install".
+ * Without it, the very first visit -- where a worker installs and waits with nothing to replace
+ * -- would offer the operator a reload for a build they are already running.
+ *
+ * **It gates the OFFER, not the watching, and the first spelling got that wrong.** Returning
+ * early when there is no controller meant that on a first visit -- the one load where there
+ * reliably is none -- the `updatefound` listener was never attached at all, so an update arriving
+ * later in that same session went unnoticed. The window was narrow (an update needs a server
+ * restart, which invalidates the session token, which the banner reports) but the code was saying
+ * something it did not mean. Found by driving the flow in a browser rather than by reading it.
+ *
+ * @param {ServiceWorkerRegistration} registration
+ */
+function watchForUpdate(registration) {
+ if (registration.waiting && navigator.serviceWorker.controller) {
+ offerUpdate(registration.waiting);
+ }
+ registration.addEventListener("updatefound", () => {
+ const installing = registration.installing;
+ if (!installing) return;
+ installing.addEventListener("statechange", () => {
+ if (installing.state === "installed" && navigator.serviceWorker.controller) {
+ offerUpdate(installing);
+ }
+ });
+ });
+}
+
+/**
+ * The offer itself: one line in the footer, and a button that takes it.
+ *
+ * In the FOOTER rather than over the view, and not in the engine banner. The banner is the page's
+ * one `aria-live` region and it answers "is keel running"; an upgrade notice is neither urgent nor
+ * about the engine's state, and putting it there would interrupt a screen reader mid-table to say
+ * something that can wait indefinitely. It sits beside the build line it is about to change.
+ *
+ * The reload is driven by `controllerchange` rather than fired straight after the message: the
+ * new worker has to actually take over before a reload gets the new build, and reloading first
+ * would fetch the old one again and leave the offer standing.
+ *
+ * @param {ServiceWorker} waiting
+ */
+function offerUpdate(waiting) {
+ if (updateNode.childElementCount !== 0) return;
+
+ const button = document.createElement("button");
+ button.className = "update";
+ button.setAttribute("type", "button");
+ button.append(document.createTextNode("A newer build is ready — reload"));
+ button.addEventListener("click", () => {
+ button.disabled = true;
+ navigator.serviceWorker.addEventListener("controllerchange", () => {
+ window.location.reload();
+ });
+ waiting.postMessage("SKIP_WAITING");
+ });
+ updateNode.replaceChildren(button);
+}
+
/**
* The build, read once, and the three things that depend on it.
*
diff --git a/keel/web/static/sw.js b/keel/web/static/sw.js
index 22a5762..97063d0 100644
--- a/keel/web/static/sw.js
+++ b/keel/web/static/sw.js
@@ -74,6 +74,7 @@ const PRECACHE = [
SHELL,
`${BASE}manifest.webmanifest`,
`${BASE}css/keel.css`,
+ `${BASE}js/actions.js`,
`${BASE}js/api.js`,
`${BASE}js/chart.js`,
`${BASE}js/docs.js`,
@@ -88,13 +89,26 @@ const PRECACHE = [
];
/**
- * Install: fill this build's cache, then take over immediately.
+ * Install: fill this build's cache, and then WAIT.
*
- * `skipWaiting` rather than waiting for every tab to close, and the version key is what makes
- * that safe: the new worker serves the new build's cache, the old one is deleted in `activate`,
- * and a client that reloads gets a consistent set. Waiting would leave an upgraded engine being
- * read by the previous shell for as long as one tab stayed open -- exactly the failure the
- * version key exists to prevent.
+ * ── THIS CALLED `skipWaiting()` UNCONDITIONALLY, AND THAT WAS WRONG ─────────────────────────
+ * The argument was that the version-keyed cache made it safe: the new worker serves the new
+ * build's cache, the old one is deleted in `activate`, so nothing stale survives. That reasoning
+ * is sound about CACHES and silent about the thing that actually breaks -- the page already on
+ * screen. `skipWaiting()` plus `clients.claim()` takes over a document that was parsed and
+ * rendered by the OLD build's JavaScript, and every request it makes afterwards is answered from
+ * the NEW build's cache. One page, two builds, no indication.
+ *
+ * keel had a second line of defence that made this hard to notice: a new build means the process
+ * restarted, which means a new session token, which means every `/api/*` call from the old page
+ * is a 403 the banner reports. So the window was narrow and loud rather than wide and quiet. It
+ * was still a window, and "another layer happens to cover it" is not a reason to keep a hazard
+ * that costs one message to remove.
+ *
+ * So the new worker installs its cache and stays in `waiting`. `main.js` notices, tells the
+ * operator a new build is ready, and only a click sends `SKIP_WAITING` -- at which point the page
+ * reloads into the build it just accepted. Nothing is ever half-upgraded, and the operator finds
+ * out an upgrade happened, which they could not before.
*
* `cache: "reload"` on every request: the server sends `Cache-Control: no-store` on static
* assets (`server._STATIC_BASE_HEADERS`), but the HTTP cache is not the only thing between here
@@ -105,11 +119,21 @@ self.addEventListener("install", (event) => {
event.waitUntil(
caches
.open(CACHE)
- .then((cache) => cache.addAll(PRECACHE.map((path) => new Request(path, { cache: "reload" }))))
- .then(() => self.skipWaiting()),
+ .then((cache) => cache.addAll(PRECACHE.map((path) => new Request(path, { cache: "reload" })))),
);
});
+/**
+ * The one message this worker answers: "the operator accepted the update, take over".
+ *
+ * A message rather than a timer or a heuristic, because the decision is not the worker's to make.
+ * `skipWaiting()` here is safe in the way it was not in `install`: the page that sent it is about
+ * to reload, so there is no document left running the old build for the new one to serve.
+ */
+self.addEventListener("message", (event) => {
+ if (event.data === "SKIP_WAITING") void self.skipWaiting();
+});
+
/**
* Activate: delete every other keel cache, then claim open clients.
*
diff --git a/pyproject.toml b/pyproject.toml
index 3b2d536..c05e074 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -147,6 +147,15 @@ warn_redundant_casts = true
# fails the build when this list and mypy's own `--strict` bundle drift apart.
[[tool.mypy.overrides]]
module = [
+ # `keel.web.*` is not a new package, and it is here for the same reason the new packages are:
+ # it was ALREADY clean under these flags. Measured rather than assumed -- `mypy --strict
+ # keel/web` reported two errors, both in one function, and both were the same defect: a
+ # three-argument `type()` call returning `Any` into a function annotated `-> KeelServer`,
+ # under a `type: ignore[return-value]` that did not name the error mypy was actually
+ # reporting. Fixing that left zero, so gating it costs nothing today and is what stops the
+ # next edit from costing something. It is also the layer where it matters most: everything
+ # under `keel/web/` is reachable from a socket.
+ "keel.web.*",
"keel_broker_api.*",
"keel_broker_alpaca.*",
"keel_broker_coinbase.*",
diff --git a/tests/test_packaging.py b/tests/test_packaging.py
index 834698f..8232601 100644
--- a/tests/test_packaging.py
+++ b/tests/test_packaging.py
@@ -160,11 +160,22 @@ def _mypy_overrides() -> list[dict]:
def _strict_modules() -> list[str]:
- """Import packages the root `[tool.mypy]` config checks in strict mode.
+ """DISTRIBUTION ROOTS the root `[tool.mypy]` config checks in strict mode.
Read from the config rather than listed here, so that tightening a package (giving it the
strict flag block) automatically brings it under the marker rule below instead of requiring
someone to remember this file.
+
+ **Dotted patterns are skipped, and the distinction is the whole point of the rule below.**
+ PEP 561's marker is a promise to whoever INSTALLS a distribution: it tells their type checker
+ that this package's annotations are real. A submodule of a distribution -- `keel.web.*`, made
+ strict at the same time as this filter -- has no installer of its own to make that promise to,
+ and the place a marker would go (`keel/py.typed`) covers `keel.*`, which is still
+ `ignore_errors`. Shipping it would be exactly what the docstring below warns against: "a
+ marker on unchecked code promises a guarantee nothing verifies."
+
+ So the rule keeps its teeth where it was aimed -- every distribution under `packages/` -- and
+ stops misfiring on internal modules that are strict for their own sake.
"""
modules: list[str] = []
for override in _mypy_overrides():
@@ -172,10 +183,25 @@ def _strict_modules() -> list[str]:
continue
entry = override["module"]
for pattern in [entry] if isinstance(entry, str) else entry:
- modules.append(pattern.removesuffix(".*"))
+ root = pattern.removesuffix(".*")
+ if "." in root:
+ continue
+ modules.append(root)
return sorted(modules)
+def test_the_strict_module_reader_still_sees_the_distributions() -> None:
+ """Mutation guard on the filter above: skipping dotted patterns must not skip everything.
+
+ A filter that quietly emptied this list would make `test_strictly_typed_packages_ship_a_
+ py_typed_marker` parametrise over nothing and pass by having no cases -- the exact shape of
+ vacuous green this file exists to prevent elsewhere."""
+ roots = _strict_modules()
+ assert "keel_broker_api" in roots, roots
+ assert len(roots) >= 6, roots
+ assert not [r for r in roots if "." in r], roots
+
+
@pytest.mark.parametrize("module", _strict_modules())
def test_strictly_typed_packages_ship_a_py_typed_marker(module):
"""A package mypy checks strictly must declare that fact to whoever installs it (PEP 561).
diff --git a/tests/web/test_client_assets.py b/tests/web/test_client_assets.py
index 7c90e8d..ce9da97 100644
--- a/tests/web/test_client_assets.py
+++ b/tests/web/test_client_assets.py
@@ -59,12 +59,27 @@ def _P(rest: str) -> str:
#: 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").
#:
+#: `actions.js` is the write boundary, split out of `main.js` after the reference implementation's
+#: own shape: youperiod.app keeps `main.js` to "DOM manipulation and event listener attachment"
+#: and puts the storage boundary in `data-manager.js` behind a small interface. #540 gave keel a
+#: write surface and left all of it -- token, submit handler, outcome memory, sentence-picking --
+#: in `main.js`, which is the module that reference keeps emptiest.
+#:
#: **`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", "docs.js")
+_MODULES = (
+ "main.js",
+ "api.js",
+ "actions.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.
#:
@@ -394,6 +409,61 @@ def test_no_client_module_can_write_markup() -> None:
assert sink not in _code_only(source), f"{name} can write markup via {sink}"
+def test_the_write_token_lives_only_in_the_write_boundary() -> None:
+ """**The module split this file's `_MODULES` note describes, asserted rather than intended.**
+
+ The design spec's reference implementation keeps `main.js` to "DOM manipulation and event
+ listener attachment" and puts the storage boundary in a module of its own behind a small
+ interface. #540 gave keel a write surface and left every part of it in `main.js`: the session
+ write token, the submit handler, the memory of what each action reported, and the choice of
+ which server field to show. `actions.js` owns all of that now.
+
+ The token is the sharpest part to pin, and the rule is about where it is HELD rather than
+ where the word appears. `api.js` names it because it is the parameter it puts in a header --
+ that module is the only one allowed to open a connection, so it is the only one that can send
+ it -- but it takes it as an argument and keeps nothing. `actions.js` is the one module with a
+ variable holding it between calls.
+
+ Stated that way rather than "the word appears once", because a credential that acquires a
+ second home is a credential the next thing added there will read, and that is what this
+ catches. A parameter passed straight into a header is not a home.
+ """
+ # Asserted as BEHAVIOUR, not as prose. `actions.js` names `X-Keel-CSRF` in a comment listing
+ # what it hides from its callers, and a raw-source search would read that explanation as a
+ # violation of the thing it explains -- the same trap `_markup_only` exists for on
+ # `index.html`. What actually matters is that the boundary builds no headers at all: the
+ # transport is `api.js`'s job and nothing here should be assembling a request.
+ boundary = _code_only(_source("actions.js"))
+ assert "headers" not in boundary.lower(), (
+ "the write boundary assembles a request; that belongs to api.js, which sends it"
+ )
+ assert "let token" in boundary, "the write boundary holds no token"
+
+ assert "X-Keel-CSRF" in _source("api.js"), "nothing sends the token"
+ transport = _code_only(_source("api.js"))
+ assert "let csrf" not in transport and "const csrf" not in transport, (
+ "api.js stores the write token; it takes one as an argument and keeps nothing"
+ )
+
+ for name in _MODULES:
+ if name in ("actions.js", "api.js"):
+ continue
+ assert "csrf" not in _code_only(_source(name)).lower(), (
+ f"{name} names the write token; it is held by actions.js and sent by api.js"
+ )
+
+
+def test_the_write_boundary_opens_no_connection_and_builds_no_dom() -> None:
+ """It sits between the two modules that do, and touches neither's job.
+
+ `api.js` is still the only module that calls `fetch` (pinned below), and the DOM is
+ `render.js`'s and the listener's. A boundary module that reached into either would be a third
+ place to look for behaviour that already has two homes."""
+ code = _code_only(_source("actions.js"))
+ for forbidden in ("fetch(", "document.", "querySelector", "createElement", "innerHTML"):
+ assert forbidden not in code, f"actions.js reaches past its boundary via {forbidden}"
+
+
def test_fetch_appears_in_exactly_one_client_module() -> None:
"""`api` is the single `fetch` wrapper.
diff --git a/tests/web/test_pwa.py b/tests/web/test_pwa.py
index 8d031f0..b2ab288 100644
--- a/tests/web/test_pwa.py
+++ b/tests/web/test_pwa.py
@@ -10,6 +10,8 @@
running `keel serve` driven by a real Chromium; the results are in the PR body:
* that the worker installs, activates and reaches `controlling` state;
+ * that a second build leaves the new worker WAITING and the offer appears in the footer;
+ * that taking the offer swaps the controller and reloads into the new build;
* that the precache is populated and holds exactly `PRECACHE`;
* that the shell still paints after `keel serve` is killed, with no figures on it;
* that a changed build string swaps the cache and deletes the old one;
@@ -286,6 +288,93 @@ def test_the_worker_never_writes_to_a_cache_outside_install() -> None:
assert code.count(".addAll(") == 1, "the precache should be the only thing written"
+def test_the_worker_never_takes_over_a_page_it_did_not_render() -> None:
+ """**This is a correction: the worker called `skipWaiting()` unconditionally at install.**
+
+ The original argument was that the version-keyed cache made it safe -- new worker, new cache,
+ old one deleted. That is true about CACHES and silent about the page already on screen:
+ `skipWaiting()` with `clients.claim()` takes over a document parsed and rendered by the OLD
+ build, and answers its later requests from the NEW build's cache. One page, two builds.
+
+ keel had a second line of defence that made it hard to notice -- a new build means a restarted
+ process, a new session token, and a 403 the banner reports -- but a hazard covered by an
+ unrelated layer is still a hazard.
+
+ So: `skipWaiting` appears only inside the message handler, never in `install`.
+ """
+ source = _sw_source()
+ code = "\n".join(
+ line for line in source.splitlines() if not line.lstrip().startswith(("*", "/*", "//"))
+ )
+ # Sliced to the install handler ALONE, not "install up to activate": the message handler now
+ # sits between them and it is the one place `skipWaiting` legitimately appears, so the wider
+ # slice would find it there and report the opposite of the truth.
+ start = code.index('addEventListener("install"')
+ install = code[start : code.index("self.addEventListener(", start + 1)]
+ assert "skipWaiting" not in install, (
+ "the worker takes over at install again; see this test's docstring before restoring it"
+ )
+ assert 'if (event.data === "SKIP_WAITING") void self.skipWaiting();' in code, (
+ "the consent path is gone -- an update could never be applied"
+ )
+
+
+def test_the_client_offers_the_update_rather_than_applying_it() -> None:
+ """The other half: a waiting worker is useless if nothing tells the operator it is waiting.
+
+ Both arrival paths are pinned. `registration.waiting` catches an update that installed during
+ a previous visit; `updatefound` + `installed` catches one that arrives while the page is open.
+ A build that handled only the second would leave an update stranded forever for anyone who
+ closed the tab at the wrong moment."""
+ main = (_STATIC / "js" / "main.js").read_text(encoding="utf-8")
+ assert "registration.waiting" in main, "an update installed on a previous visit is stranded"
+ assert '"updatefound"' in main, "an update arriving while the page is open is missed"
+ assert 'postMessage("SKIP_WAITING")' in main
+
+
+def test_the_first_install_is_not_announced_as_an_update() -> None:
+ """`navigator.serviceWorker.controller` is the test for "update" versus "first install".
+
+ Without it the very first visit -- a worker installing with nothing to replace -- would offer
+ the operator a reload for the build they are already running, which teaches them the notice
+ means nothing.
+
+ **And it gates the OFFER, never the watching.** An earlier spelling returned early from
+ `watchForUpdate` when there was no controller, which meant that on a first visit -- the one
+ load where there reliably is none -- the `updatefound` listener was never attached. This
+ asserts the listener is registered unconditionally."""
+ main = (_STATIC / "js" / "main.js").read_text(encoding="utf-8")
+ watcher = main[main.index("function watchForUpdate(") : main.index("function offerUpdate(")]
+ assert "navigator.serviceWorker.controller" in watcher, (
+ "a first install is announced as an update"
+ )
+ assert "return;" not in watcher.split('addEventListener("updatefound"')[0], (
+ "watchForUpdate returns before attaching its listener; a first visit would never watch"
+ )
+
+
+def test_the_reload_waits_for_the_new_worker_to_take_over() -> None:
+ """Reloading straight after `postMessage` races: the new worker may not be controlling yet, so
+ the reload fetches the old build again and leaves the offer standing."""
+ main = (_STATIC / "js" / "main.js").read_text(encoding="utf-8")
+ controller_change = main.index('"controllerchange"')
+ post = main.index('postMessage("SKIP_WAITING")')
+ assert controller_change < post, (
+ "the reload listener is registered after the message is sent -- the takeover can land first"
+ )
+
+
+def test_the_update_offer_is_not_in_the_live_region() -> None:
+ """The engine banner is the page's one `aria-live` region and it answers "is keel running".
+
+ An upgrade notice is neither urgent nor about the engine, and putting it there would interrupt
+ a screen reader mid-table to say something that can wait indefinitely."""
+ html = _INDEX.read_text(encoding="utf-8")
+ banner = html[html.index('id="engine"') : html.index('id="content"')]
+ assert 'id="update"' not in banner
+ assert 'id="update"' in html, "the offer has nowhere to go"
+
+
def test_the_cache_name_is_keyed_to_the_build() -> None:
"""A `CacheFirst` shell with a fixed cache name survives an engine upgrade and answers the new
API with the old contract -- everything renders, only the fields are wrong."""