Merge pull request #2349 from modelcontextprotocol/v2/chore/2333-smoke-ci-budgets

chore: state the smoke-helper and CI wall-clock budgets (#2333)

.github/workflows/main.yml

@@ -17,9 +17,22 @@ on:
 permissions:
   contents: read
 
+# Every job below declares `timeout-minutes` (#2333). It is a HUNG-JOB GUARD,
+# not a flake remedy: GitHub runners are not the contended machine the local
+# gate runs on, and nothing measured implicates them — so no value here is
+# sized for load. The rule, for every job in this file, is roughly TWICE the
+# slowest run observed, rounded up to the next five minutes: real headroom for
+# a slow runner, and a hang cut off in minutes instead of at GitHub's
+# 360-minute default. Each job states the range it was read from; when one
+# legitimately outgrows its number, raise it here with the new range.
 jobs:
   build:
     runs-on: ubuntu-latest
+    # Observed 7.6–11.8 min across the 40 push runs before this landed
+    # (2026-09-11 → 09-13), all since the coverage split (#2159). A release run
+    # before that split reached 15.6 min, with the coverage gate still inside
+    # this job — that shape no longer exists.
+    timeout-minutes: 25
 
     steps:
       - name: Checkout code
@@ -150,6 +163,8 @@ jobs:
   # time tests out at the 5s default. Separate jobs get separate runners.
   coverage:
     runs-on: ubuntu-latest
+    # Observed 5.9–8.5 min across the same 40 push runs as `build`.
+    timeout-minutes: 20
 
     steps:
       - name: Checkout code
@@ -195,6 +210,11 @@ jobs:
     concurrency:
       group: publish-npm
       cancel-in-progress: false
+    # Observed 2.3–2.8 min across the last three releases (2.4.0–2.6.0), of
+    # which `pack:verify` is about half and the only step that reaches the
+    # registry. The publish itself is atomic on npm's side, so a cut-off here
+    # leaves nothing half-published.
+    timeout-minutes: 10
     permissions:
       contents: read
       # Required for npm provenance (`--provenance` mints a signed attestation
@@ -283,6 +303,9 @@ jobs:
     if: github.event_name == 'release'
     environment: release
     needs: [build, coverage]
+    # Observed 14.5–15.4 min across the last three releases (2.4.0–2.6.0);
+    # the linux/arm64 half of the build runs under QEMU and is most of it.
+    timeout-minutes: 35
     permissions:
       contents: read
       packages: write

AGENTS.md

@@ -472,8 +472,19 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race.
   per-project cap without re-running that measurement.
 - **The Playwright locator budgets the web smokes use are named constants in
   `scripts/lib/browser-timeouts.mjs`** — raise one there, not at a call site.
-  The *remaining* smoke-helper budgets and CI's missing `timeout-minutes` are
-  tracked on #2333 rather than settled here.
+  The shared flow helpers (`deep-link-connect.mjs`, `mcp-app-flow.mjs`)
+  default to the same constants, so a budget that moves there moves for every
+  smoke at once. The process-level budgets are stated the same way in their
+  own modules — `DEFAULTS` in `announced-child.mjs` and `render-smoke.mjs`,
+  `SCRIPT_PROBE_TIMEOUT_MS` in `pty.mjs` — each with the measurement that
+  sized it (#2333).
+- **Every CI job declares `timeout-minutes`, sized from observed runs.** It is
+  a hung-job guard, not a flake remedy — GitHub runners are not the contended
+  machine — so the rule is roughly twice the slowest run observed, rounded up
+  to the next five minutes, stated per job in `.github/workflows/main.yml`
+  with the range it was read from. A job that starts taking longer gets its
+  number raised there, with the new range; the 360-minute default it replaces
+  is how a hang blocks a runner for six hours.
 - **Do not scale a fixed sleep.** A `setTimeout(r, N)` with no condition is not
   a timeout: it always waits the full window, so raising it slows every passing
   run and still races on a loaded one. Replace one with a condition wait when it

scripts/lib/announced-child.mjs

@@ -23,6 +23,34 @@
 import { spawn } from "node:child_process";
 import { setTimeout as delay } from "node:timers/promises";
 
+/**
+ * The readiness budget and poll interval, stated once (#2333) — the same shape
+ * as `render-smoke.mjs`'s `DEFAULTS`, and for the same reason: a budget that
+ * lives in a parameter default is one nobody revisits.
+ *
+ * `timeoutMs` is a ceiling on a poll, not a sleep: the wait returns on the
+ * first poll that sees the announcement, so a passing start pays only the
+ * child's real boot time plus, at most, one `pollMs`. Every smoke that needs a
+ * test server goes through this, which makes it the widest of the smoke
+ * budgets — and the one with the most headroom. Measured on the composable
+ * test server (`server-composable.js --config mcp-app-http`) through this
+ * helper with a 25ms poll, ten starts on a machine at load average 15: boot to
+ * announce was 135ms median, 137ms max. 30s is more than two hundred times
+ * that; what it has to absorb is not a slow server but a machine several
+ * gates deep, where a Node process can take seconds to reach its first line.
+ *
+ * `pollMs` is the passing path's cost. The same measurement at the default
+ * interval landed at 251–254ms every run, which is one interval exactly: the
+ * child announces well inside the first poll, so a start pays ~250ms of which
+ * ~115ms is waiting to look. That is under a second across a whole gate, so
+ * it is stated rather than tuned — lowering it buys nothing anyone would
+ * notice and costs a tighter scan loop on the child's output.
+ */
+export const DEFAULTS = Object.freeze({
+  timeoutMs: 30_000,
+  pollMs: 250,
+});
+
 /**
  * @param {object} opts
  * @param {string} opts.command       Executable to spawn.
@@ -34,8 +62,8 @@ import { setTimeout as delay } from "node:timers/promises";
  *   Called synchronously with the child immediately after spawn, before any
  *   waiting. This is the caller's teardown handle.
  * @param {string} opts.what          Noun used in error messages ("MCP test server").
- * @param {number} [opts.timeoutMs]   Readiness budget (default 30s).
- * @param {number} [opts.pollMs]      Poll interval (default 250ms).
+ * @param {number} [opts.timeoutMs]   Readiness budget (default `DEFAULTS.timeoutMs`).
+ * @param {number} [opts.pollMs]      Poll interval (default `DEFAULTS.pollMs`).
  * @returns {Promise<{ child: import("node:child_process").ChildProcess, match: RegExpMatchArray }>}
  */
 export async function startAnnouncedChild({
@@ -45,8 +73,8 @@ export async function startAnnouncedChild({
   pattern,
   onSpawn,
   what,
-  timeoutMs = 30_000,
-  pollMs = 250,
+  timeoutMs = DEFAULTS.timeoutMs,
+  pollMs = DEFAULTS.pollMs,
 }) {
   const child = spawn(command, args, {
     cwd,

scripts/lib/browser-timeouts.mjs

@@ -8,7 +8,9 @@
  * same three or four decisions repeated, so they are named once here and the
  * scripts import them — the same shape `render-smoke.mjs`'s `DEFAULTS` already
  * has, and for the same reason: a budget nobody can find is a budget nobody
- * revisits.
+ * revisits. The shared flow helpers (`deep-link-connect.mjs`,
+ * `mcp-app-flow.mjs`) default their budgets to these too (#2333); they used to
+ * carry `ui`/`roundTrip` as literals of their own.
  *
  * Three of the four are ceilings on a poll rather than sleeps — `ui`,
  * `roundTrip` and `nested` each return the instant their locator condition

scripts/lib/deep-link-connect.mjs

@@ -25,8 +25,17 @@
  * Takes only the slice of Playwright's `page` it uses (`goto`, `locator` →
  * `waitFor`/`getAttribute`), which is what lets its failure branches be
  * unit-tested against a stand-in — a smoke only ever drives its happy path.
+ *
+ * The two budgets default to `BROWSER_TIMEOUTS` rather than restating them
+ * (#2333): `goto` and the status element attaching are the page loading and
+ * painting (`ui`), and `data-status="connected"` only appears after a round
+ * trip to the MCP server (`roundTrip`). `driveAppFlow` in `mcp-app-flow.mjs`
+ * passes the same two through, so the two helpers cannot drift apart — which
+ * is the point: they used to carry the same literals by hand.
  */
 
+import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs";
+
 /**
  * The deep link that connects to `mcpUrl`.
  *
@@ -57,8 +66,8 @@ export async function connectViaDeepLink({
   page,
   url,
   expectDeepLink = true,
-  gotoTimeoutMs = 30_000,
-  connectTimeoutMs = 45_000,
+  gotoTimeoutMs = BROWSER_TIMEOUTS.ui,
+  connectTimeoutMs = BROWSER_TIMEOUTS.roundTrip,
 }) {
   const response = await page.goto(url, {
     waitUntil: "domcontentloaded",

scripts/lib/deep-link-connect.test.mjs

@@ -17,24 +17,35 @@
 
 import assert from "node:assert/strict";
 import { describe, it } from "node:test";
+import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs";
 import {
   buildConnectDeepLink,
   connectViaDeepLink,
 } from "./deep-link-connect.mjs";
 
-/** Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior. */
+/**
+ * Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior.
+ * `gotoTimeouts` and `waits` record the budget each call was given, so a test
+ * can pin the defaults without a real browser.
+ */
 function fakePage(plan, { gotoStatus = 200 } = {}) {
   const gotos = [];
+  const gotoTimeouts = [];
+  const waits = {};
   return {
     gotos,
-    goto: async (url) => {
+    gotoTimeouts,
+    waits,
+    goto: async (url, opts) => {
       gotos.push(url);
+      gotoTimeouts.push(opts?.timeout);
       return { ok: () => gotoStatus === 200, status: () => gotoStatus };
     },
     locator: (selector) => {
       const entry = plan[selector] ?? {};
       return {
-        waitFor: async () => {
+        waitFor: async (opts) => {
+          waits[selector] = opts?.timeout;
           if (entry.waitFails) throw new Error(`timeout: ${selector}`);
         },
         getAttribute: async (name) => entry.attrs?.[name] ?? null,
@@ -96,6 +107,18 @@ describe("connectViaDeepLink", () => {
     assert.deepEqual(page.gotos, ["http://x/"]);
   });
 
+  it("defaults each budget to the shared BROWSER_TIMEOUTS entry (#2333)", async () => {
+    // These used to be 30_000 / 45_000 literals of the helper's own — the same
+    // values `browser-timeouts.mjs` names `ui` and `roundTrip`, kept in step by
+    // hand. Pinning the defaults to the constants is what makes a raise there
+    // reach this helper and `driveAppFlow` at once.
+    const page = fakePage(happyPlan());
+    await connectViaDeepLink({ page, url: "http://x/" });
+    assert.deepEqual(page.gotoTimeouts, [BROWSER_TIMEOUTS.ui]);
+    assert.equal(page.waits[STATUS], BROWSER_TIMEOUTS.ui);
+    assert.equal(page.waits[CONNECTED], BROWSER_TIMEOUTS.roundTrip);
+  });
+
   it("reports a non-200 document instead of waiting on selectors", async () => {
     await assert.rejects(
       connectViaDeepLink({

scripts/lib/mcp-app-flow.mjs

@@ -30,6 +30,7 @@
 
 import { join } from "node:path";
 import { startAnnouncedChild } from "./announced-child.mjs";
+import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs";
 import {
   buildConnectDeepLink,
   connectViaDeepLink,
@@ -167,6 +168,13 @@ export function buildAppDeepLink({
  * Takes only the small slice of Playwright's `page` it uses (`goto`, `locator`
  * → `waitFor`/`getAttribute`/`count`), which is what lets its failure paths be
  * unit-tested against a stand-in — a smoke only ever exercises its happy path.
+ *
+ * The three budgets are `BROWSER_TIMEOUTS` entries, not restated literals
+ * (#2333). The first two are handed straight to `connectViaDeepLink`, so the
+ * two helpers share one pair by construction. `ready` is a `roundTrip` too:
+ * the widget has to load inside the sandbox and answer through the bridge
+ * before the attribute can flip, which is the same shape as the connect wait,
+ * not a render of something already on screen (`nested`).
  */
 export async function driveAppFlow({
   page,
@@ -174,9 +182,9 @@ export async function driveAppFlow({
   expectDeepLink = true,
   what = "app",
   extraDiagnostics = null,
-  gotoTimeoutMs = 30_000,
-  connectTimeoutMs = 45_000,
-  readyTimeoutMs = 45_000,
+  gotoTimeoutMs = BROWSER_TIMEOUTS.ui,
+  connectTimeoutMs = BROWSER_TIMEOUTS.roundTrip,
+  readyTimeoutMs = BROWSER_TIMEOUTS.roundTrip,
 }) {
   // 1-2. Navigated, the deep link accepted by the token gate, and connected.
   await connectViaDeepLink({

scripts/lib/mcp-app-flow.test.mjs

@@ -16,6 +16,7 @@
 
 import assert from "node:assert/strict";
 import { describe, it } from "node:test";
+import { BROWSER_TIMEOUTS } from "./browser-timeouts.mjs";
 import {
   buildAppDeepLink,
   driveAppFlow,
@@ -23,9 +24,15 @@ import {
   sandboxProxyPageFor,
 } from "./mcp-app-flow.mjs";
 
-/** Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior. */
+/**
+ * Minimal Playwright `page` stand-in. `plan` maps a selector to its behavior.
+ * `waits` records the budget each `waitFor` was given, so a test can pin the
+ * defaults without a real browser.
+ */
 function fakePage(plan, { gotoStatus = 200 } = {}) {
+  const waits = {};
   return {
+    waits,
     goto: async () => ({
       ok: () => gotoStatus === 200,
       status: () => gotoStatus,
@@ -33,7 +40,8 @@ function fakePage(plan, { gotoStatus = 200 } = {}) {
     locator: (selector) => {
       const entry = plan[selector] ?? {};
       return {
-        waitFor: async () => {
+        waitFor: async (opts) => {
+          waits[selector] = opts?.timeout;
           if (entry.waitFails) throw new Error(`timeout: ${selector}`);
         },
         getAttribute: async (name) => entry.attrs?.[name] ?? null,
@@ -110,6 +118,17 @@ describe("driveAppFlow", () => {
     await driveAppFlow({ page: fakePage(happyPlan()), url: "http://x/" });
   });
 
+  it("shares its connect budgets with connectViaDeepLink and waits a roundTrip for ready (#2333)", async () => {
+    // The connect pair is handed straight through, so `ui`/`roundTrip` here
+    // are the same two `connectViaDeepLink` defaults to; `ready` is a round
+    // trip of its own (sandbox load + bridge handshake), not a `nested` render.
+    const page = fakePage(happyPlan());
+    await driveAppFlow({ page, url: "http://x/" });
+    assert.equal(page.waits[STATUS], BROWSER_TIMEOUTS.ui);
+    assert.equal(page.waits[CONNECTED], BROWSER_TIMEOUTS.roundTrip);
+    assert.equal(page.waits[READY], BROWSER_TIMEOUTS.roundTrip);
+  });
+
   it("reports a non-200 document instead of waiting on selectors", async () => {
     await assert.rejects(
       driveAppFlow({

scripts/lib/pty.mjs

@@ -108,6 +108,26 @@ export function ptyCommand({ command, args = [], flavor }) {
   throw new Error(`unknown script(1) flavor: ${flavor}`);
 }
 
+/**
+ * How long `script --version` gets to exit (#2333).
+ *
+ * A ceiling on a synchronous spawn, not a sleep: the probe returns the moment
+ * the process exits, and `script --version` exits at once on every flavor —
+ * BSD `script` with a usage error, util-linux and busybox with a version line.
+ * Measured here on darwin, twenty spawns with the machine at load average 4
+ * (recent peak 19): 1.3ms median, 3.6ms max. Five seconds is over a thousand
+ * times that, and what it has to absorb is process creation on a machine
+ * several gates deep, not the command.
+ *
+ * The number matters less than what its expiry means. `spawnSync` reports a
+ * timeout in the same `error` slot as ENOENT, and `probeScriptVersion` used to
+ * read both as "no `script(1)`" — so a starved probe turned `smoke:tui` into a
+ * documented **skip**, exit 0, with a message blaming the machine's PATH. A
+ * false skip on the one pre-push gate is worse than a false failure, which is
+ * why the timeout is now told apart and thrown instead (see below).
+ */
+export const SCRIPT_PROBE_TIMEOUT_MS = 5_000;
+
 /**
  * Probe the local `script(1)`: is it there, and what does it say about itself?
  *
@@ -124,23 +144,45 @@ export function ptyCommand({ command, args = [], flavor }) {
  * answers `illegal option -- -` plus its usage on stderr — which is exactly the
  * evidence `scriptFlavorFor` reads. Only `error` means "could not run it".
  *
- * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string, error?: Error }} [runner]
+ * A **timeout** is not unavailability either, and it is the one `error` that
+ * must not be folded into "not available": `script(1)` was found and started,
+ * and the machine did not let it finish. Reporting that as "no `script(1)` on
+ * PATH" would make `smoke:tui` skip — a green exit on a run that tested
+ * nothing — so it throws, naming the budget, and the caller fails loudly.
+ *
+ * @param {(cmd: string, args: string[]) => { stdout?: string, stderr?: string, error?: Error & { code?: string } }} [runner]
  *   Injected for tests; defaults to a real `spawnSync`.
  * @returns {{ available: boolean, output: string }}
+ * @throws {Error} when the probe timed out (`error.code === "ETIMEDOUT"`).
  */
 export function probeScriptVersion(
   runner = (cmd, args) =>
-    spawnSync(cmd, args, { encoding: "utf8", timeout: 5000 }),
+    spawnSync(cmd, args, {
+      encoding: "utf8",
+      timeout: SCRIPT_PROBE_TIMEOUT_MS,
+    }),
 ) {
+  let r;
   try {
-    const r = runner("script", ["--version"]);
-    // `error` covers ENOENT, EACCES and the timeout; a missing result at all
-    // means the runner told us nothing, which is not evidence of a working one.
-    if (!r || r.error) return { available: false, output: "" };
-    return { available: true, output: `${r.stdout ?? ""}${r.stderr ?? ""}` };
-  } catch {
-    return { available: false, output: "" };
+    r = runner("script", ["--version"]);
+  } catch (err) {
+    // A runner that throws outright (injected, or a future spawn shape) is
+    // normalized onto the same path as one that returns `{ error }`, so the
+    // timeout check below applies to both shapes — a thrown ETIMEDOUT must not
+    // slip back into "unavailable" (Copilot, #2333).
+    r = { error: err };
   }
+  if (r?.error?.code === "ETIMEDOUT") {
+    throw new Error(
+      `\`script --version\` did not exit within ${SCRIPT_PROBE_TIMEOUT_MS}ms — ` +
+        "a starved or hung probe, not a missing `script(1)`, so smoke:tui " +
+        "must not skip on it",
+    );
+  }
+  // `error` covers ENOENT and EACCES; a missing result at all means the runner
+  // told us nothing, which is not evidence of a working one.
+  if (!r || r.error) return { available: false, output: "" };
+  return { available: true, output: `${r.stdout ?? ""}${r.stderr ?? ""}` };
 }
 
 /**
@@ -151,11 +193,16 @@ export function probeScriptVersion(
  * invocation" send the reader somewhere different — so the failure carries a
  * `reason` rather than being a bare `null` the caller has to narrate.
  *
+ * A probe that **timed out** is neither — it throws through here untouched,
+ * because "the machine would not run `script`" is a failure to report, not a
+ * reason to skip (see `probeScriptVersion`).
+ *
  * @param {object} [opts]
  * @param {string} [opts.platform]
  * @param {() => { available: boolean, output: string }} [opts.probe]
  * @returns {{ ok: true, flavor: string, wrap: (spec: { command: string, args?: string[] }) => { command: string, args: string[] } }
  *          | { ok: false, reason: string }}
+ * @throws {Error} when the probe timed out.
  */
 export function resolvePtyWrapper({
   platform = process.platform,

scripts/lib/pty.test.mjs

@@ -4,6 +4,7 @@ import test from "node:test";
 import {
   ptyCommand,
   probeScriptVersion,
+  SCRIPT_PROBE_TIMEOUT_MS,
   resolvePtyWrapper,
   scriptFlavorFor,
   shellQuote,
@@ -120,7 +121,7 @@ test("shellQuote survives an embedded single quote", () => {
   assert.equal(shellQuote("plain"), "'plain'");
 });
 
-test("probeScriptVersion combines both streams and never throws", () => {
+test("probeScriptVersion combines both streams and reads a runner failure as unavailable", () => {
   assert.deepEqual(
     probeScriptVersion(() => ({ stdout: "out ", stderr: "err" })),
     { available: true, output: "out err" },
@@ -168,13 +169,46 @@ test("probeScriptVersion treats result.error as unavailable, not as empty output
     probeScriptVersion(() => ({ error: enoent, stdout: "", stderr: "" })),
     { available: false, output: "" },
   );
-  // The timeout shape reports the same way.
+  // An `error` with no recognisable code is still "could not run it".
   assert.equal(
-    probeScriptVersion(() => ({ error: new Error("ETIMEDOUT") })).available,
+    probeScriptVersion(() => ({ error: new Error("EACCES") })).available,
     false,
   );
 });
 
+// The one `error` that must NOT read as unavailable (#2333). `script(1)` was
+// found and started; the machine did not let it finish. Folded into
+// `available: false` it becomes a `smoke:tui` SKIP — exit 0, blaming PATH — on
+// exactly the loaded machine the gate is supposed to be believable on.
+test("probeScriptVersion throws on a timed-out probe instead of reporting no script(1)", () => {
+  const timedOut = Object.assign(new Error("spawnSync script ETIMEDOUT"), {
+    code: "ETIMEDOUT",
+  });
+  assert.throws(
+    () =>
+      probeScriptVersion(() => ({ error: timedOut, stdout: "", stderr: "" })),
+    new RegExp(`did not exit within ${SCRIPT_PROBE_TIMEOUT_MS}ms`),
+  );
+  // A runner that THROWS the same error is normalized onto the same path — the
+  // `catch` must not quietly turn a thrown ETIMEDOUT back into "unavailable".
+  assert.throws(
+    () =>
+      probeScriptVersion(() => {
+        throw timedOut;
+      }),
+    /did not exit within/,
+  );
+  // And it reaches the caller: resolvePtyWrapper must not turn it into a skip.
+  assert.throws(
+    () =>
+      resolvePtyWrapper({
+        platform: "darwin",
+        probe: () => probeScriptVersion(() => ({ error: timedOut })),
+      }),
+    /did not exit within/,
+  );
+});
+
 test("resolvePtyWrapper reports a missing script(1) rather than guessing", () => {
   // linux would otherwise be classified util-linux off the empty output.
   const r = resolvePtyWrapper({

scripts/smoke-tui.mjs

@@ -93,7 +93,16 @@ if (!existsSync(launcher)) {
 // No `script(1)` (Windows). Running without a PTY is not a weaker check, it is
 // a guaranteed failure — so say why rather than report a crash as a defect.
 // `node-pty` is the portable answer if this ever needs to run there.
-const pty = resolvePtyWrapper();
+let pty;
+try {
+  pty = resolvePtyWrapper();
+} catch (err) {
+  // The probe timed out: `script(1)` exists and the machine did not let it
+  // finish. That is a starved gate, not a platform without a PTY — reported as
+  // a failure so a load spike cannot quietly turn this smoke into a skip
+  // (#2333). The budget and the measurement are on `SCRIPT_PROBE_TIMEOUT_MS`.
+  fail(err instanceof Error ? err.message : String(err));
+}
 if (!pty.ok) {
   skip(
     `${pty.reason} — the Ink TUI needs raw mode, which requires a real ` +