Merge PR #108: fail the release when smoke tests fail
Some checks failed
Some checks failed
This commit was merged in pull request #108.
This commit is contained in:
@@ -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`,
|
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.
|
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:
|
Installs via nfpm:
|
||||||
- `/usr/bin/mcpctl` — CLI binary (bun compiled)
|
- `/usr/bin/mcpctl` — CLI binary (bun compiled)
|
||||||
- `/usr/bin/mcpctl-local` — Local proxy binary (bun compiled)
|
- `/usr/bin/mcpctl-local` — Local proxy binary (bun compiled)
|
||||||
|
|||||||
@@ -75,9 +75,28 @@ echo "==> Running smoke tests..."
|
|||||||
export PATH="$HOME/.npm-global/bin:$PATH"
|
export PATH="$HOME/.npm-global/bin:$PATH"
|
||||||
if pnpm test:smoke; then
|
if pnpm test:smoke; then
|
||||||
echo "==> Smoke tests passed!"
|
echo "==> Smoke tests passed!"
|
||||||
|
elif [ "${MCPCTL_ALLOW_SMOKE_FAILURE:-}" = "1" ]; then
|
||||||
|
echo "==> WARNING: Smoke tests failed, continuing (MCPCTL_ALLOW_SMOKE_FAILURE=1)."
|
||||||
else
|
else
|
||||||
echo "==> WARNING: Smoke tests failed! Check mcplocal/mcpd are running."
|
# This used to print a warning and exit 0. That is how four broken readiness
|
||||||
echo " Continuing anyway — deployment is complete, but verify manually."
|
# 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
|
fi
|
||||||
echo ""
|
echo ""
|
||||||
|
|
||||||
|
|||||||
@@ -32,6 +32,18 @@ function httpRequest(opts: {
|
|||||||
headers?: Record<string, string>;
|
headers?: Record<string, string>;
|
||||||
body?: string;
|
body?: string;
|
||||||
timeout?: number;
|
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 }> {
|
}): Promise<{ status: number; headers: http.IncomingHttpHeaders; body: string }> {
|
||||||
return new Promise((resolve, reject) => {
|
return new Promise((resolve, reject) => {
|
||||||
const parsed = new URL(opts.url);
|
const parsed = new URL(opts.url);
|
||||||
@@ -46,6 +58,12 @@ function httpRequest(opts: {
|
|||||||
timeout: opts.timeout ?? 10_000,
|
timeout: opts.timeout ?? 10_000,
|
||||||
},
|
},
|
||||||
(res) => {
|
(res) => {
|
||||||
|
if (opts.headersOnly === true) {
|
||||||
|
resolve({ status: res.statusCode ?? 0, headers: res.headers, body: '' });
|
||||||
|
res.destroy();
|
||||||
|
req.destroy();
|
||||||
|
return;
|
||||||
|
}
|
||||||
const chunks: Buffer[] = [];
|
const chunks: Buffer[] = [];
|
||||||
res.on('data', (chunk: Buffer) => chunks.push(chunk));
|
res.on('data', (chunk: Buffer) => chunks.push(chunk));
|
||||||
res.on('end', () => {
|
res.on('end', () => {
|
||||||
@@ -93,17 +111,15 @@ describe('Smoke: Security — mcplocal unauthenticated endpoints', () => {
|
|||||||
|
|
||||||
// /inspect streams ALL MCP traffic (tool calls, arguments, responses)
|
// /inspect streams ALL MCP traffic (tool calls, arguments, responses)
|
||||||
// for ALL projects to any unauthenticated local client
|
// 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({
|
const res = await httpRequest({
|
||||||
url: `${MCPLOCAL_URL}/inspect`,
|
url: `${MCPLOCAL_URL}/inspect`,
|
||||||
method: 'GET',
|
method: 'GET',
|
||||||
headers: { 'Accept': 'text/event-stream' },
|
headers: { 'Accept': 'text/event-stream' },
|
||||||
timeout: 3_000,
|
timeout: 3_000,
|
||||||
}).catch((err) => {
|
headersOnly: true,
|
||||||
// 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;
|
|
||||||
});
|
});
|
||||||
|
|
||||||
// Should be accessible without auth (documenting the vulnerability)
|
// Should be accessible without auth (documenting the vulnerability)
|
||||||
|
|||||||
Reference in New Issue
Block a user