From eae68f2fc624a12744cca0af5c6afcd080fc149a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 16 Sep 2026 08:58:21 -0400 Subject: [PATCH 1/4] test(web): await the theme observer instead of a wall-clock budget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The AppRenderer "live theme flip" test had its inner `waitFor` raised twice (bare assertion -> 1000ms -> 5000ms) and failed a third time at 5032ms under `local:gate` load. The budget was never the problem. Root cause: happy-dom 20.11.1's `MutationObserverListener` stores its dispatch callback as `new WeakRef((record) => this.report(record))` — an arrow function nothing else holds strongly. A GC between `observe()` and the mutation collects it, `callback.deref()` returns undefined in `Node[reportMutation]`, and the mutation is dropped silently and permanently. That is why the failure tracks machine load rather than any wall-clock value, and why no raise ever fixed it. Reproduced directly against 20.11.1: an observer on `documentElement`, three forced `global.gc()` passes, then `setAttribute` — callback count 0. Without the forced GC, 1. Upstream holds the arrow strongly in a private field from 20.11.2 onward; the same probe against the bumped copy reports 1. So: - bump `happy-dom`'s floor to `^20.11.2` (lockfile resolves 20.14.5), which is the actual fix; - replace the 5000ms `waitFor` with `flipThemeAttributeAndAwaitObservers`, an awaited condition per AGENTS.md: a probe MutationObserver registered after the component's is notified after it, so resolving on the probe's callback proves the component's already ran. No polling, no timeout, nothing load-sensitive. - use the same helper in the two negative theme tests, which is what makes their "not called" a real gate rather than a vacuous pass on a flip that was never delivered. The file's 52 tests now run in ~120ms. Closes #2384 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- clients/web/package-lock.json | 8 +- clients/web/package.json | 2 +- .../elements/AppRenderer/AppRenderer.test.tsx | 102 ++++++++---------- 3 files changed, 52 insertions(+), 60 deletions(-) diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 9af37724..c666dc79 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -46,7 +46,7 @@ "eslint-plugin-react-hooks": "^7.1.1", "eslint-plugin-react-refresh": "^0.5.2", "eslint-plugin-storybook": "^10.5.5", - "happy-dom": "^20.9.0", + "happy-dom": "^20.11.2", "playwright": "^1.58.2", "storybook": "^10.5.5", "tsup": "^8.5.1", @@ -5264,9 +5264,9 @@ "optional": true }, "node_modules/happy-dom": { - "version": "20.11.1", - "resolved": "https://registry.npmjs.org/happy-dom/-/happy-dom-20.11.1.tgz", - "integrity": "sha512-XSt8tMzbW9ymE7687xztkO1ckR7qJNQ3LywY9vlYGhGi3zXrGBHuUo2Cl1ztZaICW+1eAGdkLbj6iwVqDT33kg==", + "version": "20.14.5", + "resolved": "https://registry.npmjs.org/happy-dom/-/happy-dom-20.14.5.tgz", + "integrity": "sha512-x/RzkpWO40bTjIoT30iQtt64FLLmH/iRcUCN2X//bLx7H3ifkdfPXyqsro/OYtqzIAhiLMMA7mmiOR9C3NOKjQ==", "dev": true, "license": "MIT", "dependencies": { diff --git a/clients/web/package.json b/clients/web/package.json index 421c8b99..4c8cb795 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -70,7 +70,7 @@ "eslint-plugin-react-hooks": "^7.1.1", "eslint-plugin-react-refresh": "^0.5.2", "eslint-plugin-storybook": "^10.5.5", - "happy-dom": "^20.9.0", + "happy-dom": "^20.11.2", "playwright": "^1.58.2", "storybook": "^10.5.5", "tsup": "^8.5.1", diff --git a/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx b/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx index b6efc1ed..1dd1660e 100644 --- a/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx +++ b/clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx @@ -1,5 +1,5 @@ import { createRef, StrictMode } from "react"; -import { act, waitFor } from "@testing-library/react"; +import { act } from "@testing-library/react"; import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import type { AppBridge } from "@modelcontextprotocol/ext-apps/app-bridge"; import type { CallToolResult, Tool } from "@modelcontextprotocol/client"; @@ -79,6 +79,34 @@ async function flushAsync(): Promise { }); } +// Runs `mutate` and resolves once every MutationObserver that was already +// watching `data-mantine-color-scheme` on `documentElement` has had its +// callback delivered — which is what the AppRenderer theme push rides on. +// +// This is an awaited condition rather than a wall-clock budget, per #2384. +// A probe observer registered *after* the component's is notified after it: +// observers are queued in registration order and each delivery is an ordinary +// microtask, so by the time the probe's callback runs the component's has +// already run and the assertion below is settled. No polling and no timeout is +// involved, so nothing here is load-sensitive. +async function flipThemeAttributeAndAwaitObservers( + value: string, +): Promise { + await act(async () => { + await new Promise((resolve) => { + const probe = new MutationObserver(() => { + probe.disconnect(); + resolve(); + }); + probe.observe(document.documentElement, { + attributes: true, + attributeFilter: ["data-mantine-color-scheme"], + }); + document.documentElement.setAttribute("data-mantine-color-scheme", value); + }); + }); +} + describe("AppRenderer", () => { it("renders an iframe with the sandbox path and tool title", () => { const bridge = createMockBridge(); @@ -538,47 +566,17 @@ describe("AppRenderer", () => { // flip we trigger below. bridge.sendHostContextChange.mockClear(); - await act(async () => { - document.documentElement.setAttribute( - "data-mantine-color-scheme", - "dark", - ); - // MutationObserver callbacks are delivered on a microtask. - await Promise.resolve(); - }); - // `waitFor`, not a bare assertion after the single microtask above: how - // many ticks happy-dom takes to deliver the MutationObserver callback is - // load-dependent, so under a full parallel run the observer can still be - // pending here. Observed failing exactly once that way, with 0 calls. - // - // The default 1000ms then turned out not to be enough either — it failed - // twice in a row in `npm run local:gate` (at ~1021ms, still 0 calls) while - // passing every time this file runs alone, which is the signature of a - // budget that is too tight rather than of a broken observer. Raised - // rather than retried: a `waitFor` that is generous costs nothing on the - // passing path, since it returns as soon as the assertion holds. - // - // #2323 briefly deleted this argument as one of its "a budget inside an - // equal budget can never be observed" cases, which it was: 5000 inside a - // 5000ms test could never win. That is fixed from the other end now — the - // enclosing budget is 15000 — so the raise is observable again and has to - // stay, because the value it beats is a measured failure rather than a - // default. It does NOT ride on `asyncUtilTimeout`, which is pinned at - // 1000 precisely because raising it globally was measured worse - // (Copilot). - await waitFor( - () => - expect(bridge.sendHostContextChange).toHaveBeenCalledWith( - expect.objectContaining({ - theme: "dark", - styles: expect.objectContaining({ - variables: expect.objectContaining({ - "--color-background-primary": "#101113", - }), - }), + await flipThemeAttributeAndAwaitObservers("dark"); + + expect(bridge.sendHostContextChange).toHaveBeenCalledWith( + expect.objectContaining({ + theme: "dark", + styles: expect.objectContaining({ + variables: expect.objectContaining({ + "--color-background-primary": "#101113", }), - ), - { timeout: 5000 }, + }), + }), ); } finally { getComputedStyleSpy.mockRestore(); @@ -599,13 +597,10 @@ describe("AppRenderer", () => { // No `initialized` emitted — the theme observer is gated, like the // container and displayMode pushes, so a pre-handshake flip is dropped. bridge.sendHostContextChange.mockClear(); - await act(async () => { - document.documentElement.setAttribute( - "data-mantine-color-scheme", - "dark", - ); - await Promise.resolve(); - }); + // Same awaited-delivery helper as the positive case: it is what makes this + // "not called" a real gate rather than a vacuous pass on a flip that was + // never delivered at all. + await flipThemeAttributeAndAwaitObservers("dark"); expect(bridge.sendHostContextChange).not.toHaveBeenCalled(); document.documentElement.removeAttribute("data-mantine-color-scheme"); }); @@ -627,13 +622,10 @@ describe("AppRenderer", () => { }); bridge.sendHostContextChange.mockClear(); - await act(async () => { - document.documentElement.setAttribute( - "data-mantine-color-scheme", - "dark", - ); - await Promise.resolve(); - }); + // Same awaited-delivery helper as the positive case: it is what makes this + // "not called" a real gate rather than a vacuous pass on a flip that was + // never delivered at all. + await flipThemeAttributeAndAwaitObservers("dark"); expect(bridge.sendHostContextChange).not.toHaveBeenCalled(); document.documentElement.removeAttribute("data-mantine-color-scheme"); }); From 3463304fbb06ef9948a21b024c9ac026fa84a777 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 16 Sep 2026 09:13:14 -0400 Subject: [PATCH 2/4] docs: pack:verify is not a local:gate stage; name it as its own release step (#2380) The release skill's step 2a claimed pack:verify runs inside local:gate; it does not. Make it an explicit step there, and correct pre-push-gate's claim that CI never runs it (the publish job does, on a published release). Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/pre-push-gate/SKILL.md | 8 +++++--- .claude/skills/release/SKILL.md | 13 ++++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index ea8bcb64..fc151b5e 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -207,6 +207,8 @@ it — the split is the design. `npm run pack:verify` builds, packs the real tarball, installs it into a clean throwaway consumer, and drives the installed `mcp-inspector` bin end to end. It -needs **network access**, so it is a local/release check and is in neither -`local:gate` nor CI. Run it when touching packaging (the `files` allowlist, -a bundler `external` list, anything read from disk at runtime). +needs **network access**, so it is a local/release check: it is **not** a +`local:gate` stage, and CI runs it only in the `publish` job on a published +GitHub Release — never on a push or PR. Run it when touching packaging (the +`files` allowlist, a bundler `external` list, anything read from disk at +runtime), and as its own step when cutting a release (the `release` skill). diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index ee526531..d460096d 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -143,9 +143,16 @@ Then drive it. Work from a **dedicated worktree** with its own full `npm install` (a symlinked `node_modules` passes lint and tests and then fails every story file), run `npm run local:gate` there, and exercise the app from the **production build** — the packaged bin and the built bundles, not `vite dev`. -The `local-dev`, `test-servers` and `pre-push-gate` skills cover the mechanics; -`pack:verify` inside the gate is what proves the tarball a consumer installs -actually resolves. +The `local-dev`, `test-servers` and `pre-push-gate` skills cover the mechanics. + +**Then run `npm run pack:verify` there as its own step.** It is what proves the +tarball a consumer installs actually resolves, and ⚠️ **`local:gate` does not +run it** — `local:gate:stages` has no packaging stage, and a green gate says +nothing about the published tarball (#2380). CI runs it only in the `publish` +job, which fires on the published GitHub Release — after the tag exists — so +skipping it here means the first signal of a broken package arrives too late to +stop the release. It needs network access; record its result (tarball size and +the `pack:verify OK` line) for the ledger below. **Every contribution closed in the milestone gets driven, not read.** The bar is observed behavior from the running app — a rendered panel, a status attribute, a From 35436777095d9458ea4a42ef8e03b9f79e49c946 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 16 Sep 2026 09:16:57 -0400 Subject: [PATCH 3/4] docs: add pack:verify to the release summary table and ledger verdict (#2387 review) Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/release/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index d460096d..5d40bd39 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -36,7 +36,7 @@ other. | Branch | `v2/chore/-bump-`, cut from `origin/v2/main` | the milestone-merge branch, cut from `origin/main` | | Base | **`v2/main`** | **`main`** | | Carries | the `npm audit` report, **any fixes the audit forces**, and the **version bump** — all three, one PR | the milestone's work, arriving whole from `v2/main`. **No commits of its own.** | -| Verified by | `npm run local:gate` | `npm run local:gate` **plus** a hand-driven smoke of every contribution in the milestone, from the **production build**, written up as a **ledger artifact** | +| Verified by | `npm run local:gate` | `npm run local:gate`, **plus** `npm run pack:verify` (not a gate stage), **plus** a hand-driven smoke of every contribution in the milestone, from the **production build**, written up as a **ledger artifact** | | Merged when | reviewed and green | the ledger is reviewed by the maintainers and clean | Then, and only then, a maintainer tags and publishes the **GitHub Release** @@ -172,7 +172,7 @@ and link it from PR 2. Shape it like the standfirst saying what tree was tested and that its hash matches `origin/v2/main`, plus whether the milestone payload is complete (the only issue left open should be the merge itself). -- **Verdict band** — `local:gate` result, milestone issues verified as `N / N`, +- **Verdict band** — `local:gate` and `pack:verify` results, milestone issues verified as `N / N`, distinct test count, regressions found. - **The automated gate** — one cell per stage with its number (file counts, test counts, smoke count, `pack:verify` size), and a note on what is new this From 389bb17d2a65af2d20f3ac2999d76e5ee59f169a Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 16 Sep 2026 09:20:44 -0400 Subject: [PATCH 4/4] docs: give pack:verify its own ledger cell, apart from the gate stages (#2387 review) Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: cliffhall --- .claude/skills/release/SKILL.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.claude/skills/release/SKILL.md b/.claude/skills/release/SKILL.md index 5d40bd39..d409acb5 100644 --- a/.claude/skills/release/SKILL.md +++ b/.claude/skills/release/SKILL.md @@ -172,11 +172,13 @@ and link it from PR 2. Shape it like the standfirst saying what tree was tested and that its hash matches `origin/v2/main`, plus whether the milestone payload is complete (the only issue left open should be the merge itself). -- **Verdict band** — `local:gate` and `pack:verify` results, milestone issues verified as `N / N`, - distinct test count, regressions found. -- **The automated gate** — one cell per stage with its number (file counts, - test counts, smoke count, `pack:verify` size), and a note on what is new this +- **Verdict band** — `local:gate` and `pack:verify` results, milestone issues + verified as `N / N`, distinct test count, regressions found. +- **The automated gate** — one cell per `local:gate` stage with its number + (file counts, test counts, smoke count), and a note on what is new this milestone. +- **The packaging check** — `pack:verify` in its own cell, apart from the gate + stages because it is not one of them: its result and the tarball size. - **One section per theme**, each a table of *Issue · What was driven · Observed · Status*. One row per closed issue, issue-linked, with the actual output in the Observed cell.