From 822c1bb047dbcab8246f0ba4f9addfd7a98876cf Mon Sep 17 00:00:00 2001 From: Michal Date: Mon, 10 Aug 2026 17:15:40 +0100 Subject: [PATCH] build: fail the release when smoke tests fail, and fix the SSE test that hung MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit release.sh printed `WARNING: Smoke tests failed!` and exited 0. That is how four broken readiness probes shipped unnoticed on 2026-08-10 — the warning scrolled past in the build log and the release reported success. It now exits 1, with `MCPCTL_ALLOW_SMOKE_FAILURE=1` as the escape hatch. The message is explicit that smoke runs LAST, against the installed binary: the package is already published and installed, so the failure reports fleet breakage rather than preventing a bad artifact. Turning the gate on required fixing a latent hang first, or every release would have blocked on it. `security.test.ts > /inspect SSE endpoint …` waited for a response body that by design never ends, so it could only settle via the socket's *inactivity* timeout — and /inspect relays every project's MCP traffic, so during a full smoke run it is never idle. Run alone it passed and looked flaky; run with the suite it failed every time. httpRequest gains `headersOnly`, which resolves on the response headers and hangs up. The assertion only ever needed the status line. Verified: full smoke suite 158/158 (was 157/158 with this test timing out); the gate block lifted verbatim from release.sh exits 1 with a stubbed failing smoke run, and exits 0 reaching subsequent code under MCPCTL_ALLOW_SMOKE_FAILURE=1. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_019wUmrfkVQR6CKcYKxENq7k --- docs/project-summary.md | 9 ++++++++ scripts/release.sh | 23 +++++++++++++++++-- src/mcplocal/tests/smoke/security.test.ts | 28 ++++++++++++++++++----- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/docs/project-summary.md b/docs/project-summary.md index 23e7a75..5c0a163 100644 --- a/docs/project-summary.md +++ b/docs/project-summary.md @@ -908,6 +908,15 @@ MCPCTL_BASE_BRANCH=release-2.x bash scripts/build-rpm.sh # compare to another b Offline it falls back to the last fetched `origin/main`, then to a local `main`, and says which it used; outside a git checkout it skips entirely. +**A failing smoke run fails the release.** It used to print +`WARNING: Smoke tests failed!` and exit 0 — which is exactly how four broken +readiness probes shipped unnoticed (see `docs/reliability.md`): the warning +scrolled past and the release reported success. Note what the gate does and does +not do — smoke runs *last*, against the installed binary, so the package is +already published and installed by the time it fails. It reports the breakage +rather than preventing it, so investigate the fleet rather than assuming the +artifact is bad. Override with `MCPCTL_ALLOW_SMOKE_FAILURE=1`. + Installs via nfpm: - `/usr/bin/mcpctl` — CLI binary (bun compiled) - `/usr/bin/mcpctl-local` — Local proxy binary (bun compiled) diff --git a/scripts/release.sh b/scripts/release.sh index fc6cfc8..16bbdd4 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -75,9 +75,28 @@ echo "==> Running smoke tests..." export PATH="$HOME/.npm-global/bin:$PATH" if pnpm test:smoke; then echo "==> Smoke tests passed!" +elif [ "${MCPCTL_ALLOW_SMOKE_FAILURE:-}" = "1" ]; then + echo "==> WARNING: Smoke tests failed, continuing (MCPCTL_ALLOW_SMOKE_FAILURE=1)." else - echo "==> WARNING: Smoke tests failed! Check mcplocal/mcpd are running." - echo " Continuing anyway — deployment is complete, but verify manually." + # This used to print a warning and exit 0. That is how four broken readiness + # probes shipped unnoticed on 2026-08-10: the warning scrolled past in the + # build log and the release reported success. A failing smoke run means + # something in the live fleet is genuinely broken — say so in the exit code. + # + # Note what this does and does not do: smoke runs LAST, against the installed + # binary, so the package is already published and installed by now. Failing + # here reports the breakage, it does not prevent it — investigate, do not + # assume the artifact is bad. + echo "" >&2 + echo "ERROR: smoke tests failed — the release is published and installed, but" >&2 + echo " something in the live fleet is broken. Investigate before relying" >&2 + echo " on this build; do not just re-run." >&2 + echo "" >&2 + echo " Common causes: mcplocal/mcpd not running, a readiness probe pointing at" >&2 + echo " a tool the upstream renamed, or an expired credential." >&2 + echo " Override: MCPCTL_ALLOW_SMOKE_FAILURE=1 $0" >&2 + echo "" >&2 + exit 1 fi echo "" diff --git a/src/mcplocal/tests/smoke/security.test.ts b/src/mcplocal/tests/smoke/security.test.ts index 19a4848..3e7ecbe 100644 --- a/src/mcplocal/tests/smoke/security.test.ts +++ b/src/mcplocal/tests/smoke/security.test.ts @@ -32,6 +32,18 @@ function httpRequest(opts: { headers?: Record; body?: string; timeout?: number; + /** + * Resolve as soon as the response headers arrive, then hang up, instead of + * waiting for the body to end. + * + * Required for a streaming endpoint: SSE responses never end, so the normal + * path can only settle via the socket's *inactivity* timeout — which never + * fires while the stream is busy. `/inspect` relays every project's MCP + * traffic, so during a full smoke run it is never idle, and the request hung + * until vitest killed the test. Alone it looked flaky; under load it failed + * every time. Reading the status does not need the body anyway. + */ + headersOnly?: boolean; }): Promise<{ status: number; headers: http.IncomingHttpHeaders; body: string }> { return new Promise((resolve, reject) => { const parsed = new URL(opts.url); @@ -46,6 +58,12 @@ function httpRequest(opts: { timeout: opts.timeout ?? 10_000, }, (res) => { + if (opts.headersOnly === true) { + resolve({ status: res.statusCode ?? 0, headers: res.headers, body: '' }); + res.destroy(); + req.destroy(); + return; + } const chunks: Buffer[] = []; res.on('data', (chunk: Buffer) => chunks.push(chunk)); res.on('end', () => { @@ -93,17 +111,15 @@ describe('Smoke: Security — mcplocal unauthenticated endpoints', () => { // /inspect streams ALL MCP traffic (tool calls, arguments, responses) // for ALL projects to any unauthenticated local client + // headersOnly: the stream never ends, and waiting for it to go idle is what + // made this hang whenever other suites were generating traffic. The status + // line is all this assertion needs. const res = await httpRequest({ url: `${MCPLOCAL_URL}/inspect`, method: 'GET', headers: { 'Accept': 'text/event-stream' }, timeout: 3_000, - }).catch((err) => { - // Timeout is expected (SSE keeps connection open) — still means endpoint is accessible - if ((err as Error).message.includes('timed out')) { - return { status: 200, headers: {} as http.IncomingHttpHeaders, body: '' }; - } - throw err; + headersOnly: true, }); // Should be accessible without auth (documenting the vulnerability)