diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e54360..ee3954b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to `@testsprite/testsprite-cli` are documented here. The for ## [Unreleased] +### Fixed + +- **`ci run` on `deployment_status` gates only an open pull request whose head is the deployed commit** (#550). A redeploy of a merged or closed pull request's head commit, or an open pull request that merely contains the commit, is now skipped with a `::notice::` and exit 0 (`no_pull_request`) instead of cloning an environment and gating it. +- **The `deployment_status` pull-request lookup honors `--request-timeout` and survives a non-JSON reply** (#551). A GitHub API that never answers is exit 10 within the request budget instead of hanging the job, and a 200 with an HTML body (proxy, captive portal) is exit 10 with a clear message instead of an uncaught exception. 401/403 keep exit 5. + ## [0.15.0] - 2026-10-17 ### Added diff --git a/src/commands/ci.ts b/src/commands/ci.ts index 9e3eec2..fa5819a 100644 --- a/src/commands/ci.ts +++ b/src/commands/ci.ts @@ -5,6 +5,7 @@ import { Command } from 'commander'; import { makeHttpClient, parseRequestTimeoutFlag, + resolveRequestTimeoutMs, type CommonOptions as FactoryCommonOptions, } from '../lib/client-factory.js'; import { ApiError, CLIError, localValidationError } from '../lib/errors.js'; @@ -1327,7 +1328,10 @@ export async function runCiRun(opts: CiRunOptions, deps: TestDeps = {}): Promise throw err; } - const github = await readGithubRunContext(env, { fetchImpl: deps.fetchImpl }); + const github = await readGithubRunContext(env, { + fetchImpl: deps.fetchImpl, + requestTimeoutMs: resolveRequestTimeoutMs(opts, env), + }); let previewName: string | undefined; let githubTrigger: CiGithubTrigger | undefined; if (github.kind === 'pull_request') { diff --git a/src/lib/github-context.test.ts b/src/lib/github-context.test.ts index b5c5944..6a5d540 100644 --- a/src/lib/github-context.test.ts +++ b/src/lib/github-context.test.ts @@ -116,6 +116,134 @@ describe('readGithubRunContext', () => { }, ); expect(closed.kind).toBe('no_pull_request'); + // Nor is a merged/closed pull request whose head IS the deployed commit (a redeploy after merge). + for (const state of ['closed', 'merged']) { + const done = await readGithubRunContext( + actionsEnv({ GITHUB_EVENT_NAME: 'deployment_status', GITHUB_TOKEN: 't' }), + { + ...eventFile({ deployment: { sha: SHA } }), + fetchImpl: githubApi([{ number: 7, state, head: { sha: SHA, ref: 'feat/done' } }]), + }, + ); + expect(done).toMatchObject({ kind: 'no_pull_request', event: 'deployment_status' }); + } + // Nor an open PR that only contains the commit (branched off it): its head is another commit. + const containing = await readGithubRunContext( + actionsEnv({ GITHUB_EVENT_NAME: 'deployment_status', GITHUB_TOKEN: 't' }), + { + ...eventFile({ deployment: { sha: SHA } }), + fetchImpl: githubApi([ + { number: 7, state: 'closed', head: { sha: SHA, ref: 'feat/done' } }, + { number: 8, state: 'open', head: { sha: OTHER, ref: 'feat/next' } }, + ]), + }, + ); + expect(containing).toMatchObject({ kind: 'no_pull_request', event: 'deployment_status' }); + }); + + it('deployment_status: an open PR wins over a closed one with the same head commit', async () => { + const ctx = await readGithubRunContext( + actionsEnv({ GITHUB_EVENT_NAME: 'deployment_status', GITHUB_TOKEN: 't' }), + { + ...eventFile({ deployment: { sha: SHA } }), + fetchImpl: githubApi([ + { number: 7, state: 'closed', head: { sha: SHA, ref: 'feat/old' } }, + { number: 9, state: 'open', head: { sha: SHA, ref: 'feat/new' } }, + ]), + }, + ); + expect(ctx).toMatchObject({ kind: 'pull_request', trigger: { prNumber: 9 } }); + }); + + it('deployment_status: a lookup that never answers times out with exit 10 instead of hanging', async () => { + const hang = vi.fn( + (_url: string | URL | Request, init?: RequestInit) => + new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(init.signal?.reason)); + }), + ) as unknown as typeof globalThis.fetch; + const started = Date.now(); + await expect( + readGithubRunContext( + actionsEnv({ GITHUB_EVENT_NAME: 'deployment_status', GITHUB_TOKEN: 't' }), + { + ...eventFile({ deployment: { sha: SHA } }), + fetchImpl: hang, + requestTimeoutMs: 50, + }, + ), + ).rejects.toMatchObject({ + exitCode: 10, + message: expect.stringContaining('did not answer within'), + }); + expect(Date.now() - started).toBeLessThan(5_000); + }); + + it('deployment_status: a 200 whose body stalls past the timeout is reported as a timeout, exit 10', async () => { + const stall = vi.fn((_url: string | URL | Request, init?: RequestInit) => { + const body = new ReadableStream({ + start(controller) { + init?.signal?.addEventListener('abort', () => controller.error(init.signal?.reason)); + }, + }); + return Promise.resolve( + new Response(body, { status: 200, headers: { 'content-type': 'application/json' } }), + ); + }) as unknown as typeof globalThis.fetch; + await expect( + readGithubRunContext( + actionsEnv({ GITHUB_EVENT_NAME: 'deployment_status', GITHUB_TOKEN: 't' }), + { ...eventFile({ deployment: { sha: SHA } }), fetchImpl: stall, requestTimeoutMs: 50 }, + ), + ).rejects.toMatchObject({ + exitCode: 10, + message: expect.stringContaining('did not finish answering within'), + }); + }); + + it('deployment_status: a 200 that is not JSON (proxy, captive portal) is exit 10, not an uncaught crash', async () => { + const html = vi.fn( + async () => + new Response('sign in to the proxy', { + status: 200, + headers: { 'content-type': 'text/html' }, + }), + ) as unknown as typeof globalThis.fetch; + await expect( + readGithubRunContext( + actionsEnv({ GITHUB_EVENT_NAME: 'deployment_status', GITHUB_TOKEN: 't' }), + { + ...eventFile({ deployment: { sha: SHA } }), + fetchImpl: html, + }, + ), + ).rejects.toMatchObject({ exitCode: 10, message: expect.stringContaining('not JSON') }); + }); + + it('deployment_status: a body that breaks off mid-stream is exit 10 with its cause, not "not JSON"', async () => { + const broken = vi.fn( + async () => + new Response( + new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode('[{"number":')); + controller.error(new Error('socket hang up')); + }, + }), + { status: 200, headers: { 'content-type': 'application/json' } }, + ), + ) as unknown as typeof globalThis.fetch; + const failure = readGithubRunContext( + actionsEnv({ GITHUB_EVENT_NAME: 'deployment_status', GITHUB_TOKEN: 't' }), + { ...eventFile({ deployment: { sha: SHA } }), fetchImpl: broken }, + ); + await expect(failure).rejects.toMatchObject({ + exitCode: 10, + message: expect.stringContaining('could not be read: socket hang up'), + }); + await expect(failure).rejects.not.toMatchObject({ + message: expect.stringContaining('not JSON'), + }); }); it('deployment_status: needs GITHUB_TOKEN, and a failed lookup never falls back to a guess', async () => { diff --git a/src/lib/github-context.ts b/src/lib/github-context.ts index 7153107..83fdd42 100644 --- a/src/lib/github-context.ts +++ b/src/lib/github-context.ts @@ -23,8 +23,13 @@ export interface GithubContextDeps { /** Reads `$GITHUB_EVENT_PATH`; injectable so tests never touch the runner. */ readFile?: (path: string) => string; fetchImpl?: FetchImpl; + /** Budget for the GitHub pull-request lookup, as `--request-timeout` gives every other call. */ + requestTimeoutMs?: number; } +/** Same default as the HttpClient's per-request budget when no flag or env var sets one. */ +const DEFAULT_LOOKUP_TIMEOUT_MS = 30_000; + /** The temporary environment a pull request's runs live in — shared with the GitHub App. */ export function previewEnvironmentName(prNumber: number): string { return `github-pr-${prNumber}`; @@ -86,14 +91,20 @@ export async function readGithubRunContext( ); } const pulls = await listPullsForCommit( - { env, repoOwner, repoName, sha, token }, + { + env, + repoOwner, + repoName, + sha, + token, + timeoutMs: deps.requestTimeoutMs ?? DEFAULT_LOOKUP_TIMEOUT_MS, + }, deps.fetchImpl ?? globalThis.fetch, ); - // The PR whose head IS this commit is the one this deployment belongs to. - // Anything looser (several open PRs, a merged one) is not a gate target. - const exact = pulls.filter(p => p.headSha === sha); - const open = pulls.filter(p => p.state === 'open'); - const chosen = exact[0] ?? (open.length === 1 ? open[0] : undefined); + // Only an OPEN PR whose head IS this commit is the one this deployment belongs to. + // Anything looser (an open PR that merely contains the commit, a merged or closed one) is + // not a gate target: a redeploy of a merged PR's head commit must not clone an environment. + const chosen = pulls.find(p => p.headSha === sha && p.state === 'open'); if (!chosen) { return { kind: 'no_pull_request', @@ -152,7 +163,14 @@ interface PullSummary { } async function listPullsForCommit( - args: { env: NodeJS.ProcessEnv; repoOwner: string; repoName: string; sha: string; token: string }, + args: { + env: NodeJS.ProcessEnv; + repoOwner: string; + repoName: string; + sha: string; + token: string; + timeoutMs: number; + }, fetchImpl: FetchImpl, ): Promise { const base = (args.env.GITHUB_API_URL ?? 'https://api.github.com').replace(/\/+$/, ''); @@ -166,10 +184,17 @@ async function listPullsForCommit( authorization: `Bearer ${args.token}`, 'x-github-api-version': '2022-11-28', }, + // Without a budget a GitHub API that never answers hangs the job until the runner kills it. + signal: AbortSignal.timeout(args.timeoutMs), }); } catch (err) { throw new CLIError( - `Could not look up the pull request for commit ${args.sha.slice(0, 7)}: ${err instanceof Error ? err.message : String(err)}`, + `Could not look up the pull request for commit ${args.sha.slice(0, 7)}: ` + + (isTimeout(err) + ? `GitHub did not answer within ${Math.round(args.timeoutMs / 1000)}s` + : err instanceof Error + ? err.message + : String(err)), 10, ); } @@ -182,7 +207,23 @@ async function listPullsForCommit( response.status === 401 || response.status === 403 ? 5 : 10, ); } - const body: unknown = await response.json(); + let body: unknown; + try { + body = await response.json(); + } catch (err) { + // A proxy or captive portal answering 200 with HTML must be a transient failure, not an uncaught crash; so is + // a body that stalls until the timeout aborts it, which is reported as the timeout it is, and a connection that + // drops mid-body, which is reported with its own cause rather than as "not JSON". + throw new CLIError( + `Could not look up the pull request for commit ${args.sha.slice(0, 7)}: ` + + (isTimeout(err) + ? `GitHub did not finish answering within ${Math.round(args.timeoutMs / 1000)}s` + : err instanceof SyntaxError + ? `GitHub answered ${response.status} with a body that is not JSON` + : `GitHub's ${response.status} response body could not be read: ${err instanceof Error ? err.message : String(err)}`), + 10, + ); + } if (!Array.isArray(body)) return []; return body.flatMap((item): PullSummary[] => { const pr = asRecord(item); @@ -205,3 +246,7 @@ function asRecord(value: unknown): Record | undefined { ? (value as Record) : undefined; } + +function isTimeout(err: unknown): boolean { + return err instanceof Error && (err.name === 'TimeoutError' || err.name === 'AbortError'); +}