diff --git a/src/env.ts b/src/env.ts index 905dc62..7caf577 100644 --- a/src/env.ts +++ b/src/env.ts @@ -24,6 +24,11 @@ const envSchema = z.object({ SITE_API_ENDPOINT: z.url().default("https://api.querydoctor.com"), TOKEN: z.string().optional(), GITHUB_REPOSITORY: z.string().optional(), + // Fail the check when the API rejects a computed run (4xx other than auth). + // Off by default: that's usually an analyzer/API contract issue the user + // can't fix, so we surface it loudly without blocking their PR. Auth failures + // always fail regardless; transient failures never do. + FAIL_ON_INGEST_ERROR: z.stringbool().default(false), }); // we want to avoid asking for ALL env permissions if possible diff --git a/src/main.ts b/src/main.ts index fbb380a..d328178 100644 --- a/src/main.ts +++ b/src/main.ts @@ -7,6 +7,8 @@ import { Connectable } from "./sync/connectable.ts"; import { shutdownController } from "./shutdown.ts"; import { buildQueries, + type CiRunResult, + classifyIngestFailure, compareRuns, fetchPreviousRun, gateEligibleNewQueries, @@ -116,9 +118,37 @@ async function runInCI( // POST to Site API first so we get the run ID (for baseline exclusion) and // the unified CI-signal metadata for the PR comment. - let runResult = null; + let runResult: CiRunResult | null = null; if (siteApiEndpoint) { - runResult = await postToSiteApi(siteApiEndpoint, queries, reportContext.statisticsMode, reportContext.computedStats, syncedSchema); + const outcome = await postToSiteApi(siteApiEndpoint, queries, reportContext.statisticsMode, reportContext.computedStats, syncedSchema); + if (outcome.ok) { + runResult = outcome.result; + } else { + // Ingestion failed: the run was computed but not saved. Without this it + // silently vanishes — no dashboard link, a degraded PR comment that + // looks like a normal empty result, and a green check. Surface it in the + // comment, and pick the Actions severity by who can act and whether it + // recovers (see classifyIngestFailure). + const kind = classifyIngestFailure(outcome.failure.status); + reportContext.ingestError = { kind, ...outcome.failure }; + const statusText = outcome.failure.status + ? ` (HTTP ${outcome.failure.status})` + : ""; + const base = `Query Doctor could not record this run${statusText}; it was not saved to the dashboard.`; + if (kind === "auth") { + // The user's to fix and means CI isn't actually running — fail loudly. + core.setFailed(`${base} The project TOKEN is missing or invalid.`); + } else if (kind === "rejected") { + // The API refused a computed run (likely analyzer/API skew) — our bug, + // not theirs. Red and loud, but don't block the PR unless opted in. + const msg = `${base} The API rejected the submission; re-running won't help.`; + if (env.FAIL_ON_INGEST_ERROR) core.setFailed(msg); + else core.error(msg); + } else { + // Transient (network/timeout/5xx) — recoverable, so warn and move on. + core.warning(`${base} Query Doctor was unreachable — re-run to retry.`); + } + } } const runId: string | null = runResult?.id ?? null; diff --git a/src/reporters/github/github.test.ts b/src/reporters/github/github.test.ts index b977811..bf24c2d 100644 --- a/src/reporters/github/github.test.ts +++ b/src/reporters/github/github.test.ts @@ -402,6 +402,46 @@ describe("template rendering", () => { const output = renderTemplate(ctx); expect(output).toContain("3 queries analyzed"); }); + + test("renders a rejected-ingest banner with status and details", () => { + const ctx = makeContext({ + ingestError: { + kind: "rejected", + status: 400, + message: "ZodError: invalid constraintType", + }, + }); + const output = renderTemplate(ctx); + expect(output).toContain("Query Doctor couldn't record this run"); + expect(output).toContain("HTTP 400"); + expect(output).toContain("re-running won't help"); + expect(output).toContain("ZodError: invalid constraintType"); + }); + + test("renders auth-specific copy for an auth-kind ingest failure", () => { + const ctx = makeContext({ + ingestError: { kind: "auth", status: 401, message: "Unauthorized" }, + }); + const output = renderTemplate(ctx); + expect(output).toContain("authentication failed"); + expect(output).toContain("Set a valid `TOKEN`"); + }); + + test("renders retry copy for a transient ingest failure", () => { + const ctx = makeContext({ + ingestError: { kind: "transient", status: null, message: "network down" }, + }); + const output = renderTemplate(ctx); + expect(output).toContain("re-run the check to retry"); + }); + + test("omits the failure banner when ingestion succeeded", () => { + const ctx = makeContext({ + queryStats: { analyzed: 3, matched: 1, optimized: 0, errored: 0 }, + }); + const output = renderTemplate(ctx); + expect(output).not.toContain("Query Doctor couldn't record this run"); + }); }); function makeMetadata(overrides: Partial = {}): CiRunMetadata { diff --git a/src/reporters/github/success.md.j2 b/src/reporters/github/success.md.j2 index 57fe1f9..ec6f755 100644 --- a/src/reporters/github/success.md.j2 +++ b/src/reporters/github/success.md.j2 @@ -1,5 +1,25 @@ ### Query Doctor Analysis +{% if ingestError %} +> [!CAUTION] +{% if ingestError.kind == "auth" %} +> **Query Doctor couldn't record this run — authentication failed{% if ingestError.status %} (HTTP {{ ingestError.status }}){% endif %}.** The project token is missing or invalid, so nothing was saved to the dashboard. Set a valid `TOKEN` in your workflow. +{% elif ingestError.kind == "transient" %} +> **Query Doctor couldn't record this run.** The API was unreachable{% if ingestError.status %} (HTTP {{ ingestError.status }}){% endif %}, so it wasn't saved this time. This is usually temporary — re-run the check to retry. +{% else %} +> **Query Doctor couldn't record this run.** The API rejected the submission{% if ingestError.status %} (HTTP {{ ingestError.status }}){% endif %}, so it wasn't saved to the dashboard and the results below may be incomplete. This usually means the analyzer and Query Doctor are out of sync — re-running won't help; if it persists, it's a bug on our side. +{% endif %} +{% if ingestError.message %} +
Details + +``` +{{ ingestError.message }} +``` + +
+{% endif %} + +{% endif %} {% if hasComparison %} {{ queryStats.analyzed | default('?') }} queries analyzed {% elif comparisonUnavailable %} diff --git a/src/reporters/reporter.ts b/src/reporters/reporter.ts index dc313e9..5226ac8 100644 --- a/src/reporters/reporter.ts +++ b/src/reporters/reporter.ts @@ -1,5 +1,9 @@ import type { ComputedStats, IndexIdentifier, StatisticsMode } from "@query-doctor/core"; -import type { CiRunMetadata, RunComparison } from "./site-api.ts"; +import type { + CiRunMetadata, + IngestFailureKind, + RunComparison, +} from "./site-api.ts"; export interface Reporter { provider(): string; @@ -90,6 +94,16 @@ export interface ReportContext { * "no baseline / add a push trigger" copy (Site#3287). */ comparisonUnavailable?: boolean; + /** + * `POST /ci/runs` failed, so the run wasn't saved to the dashboard. Drives a + * prominent failure banner in the comment so the run doesn't silently vanish; + * `kind` tailors the copy (transient → retry, auth → fix token, rejected → our bug). + */ + ingestError?: { + kind: IngestFailureKind; + status: number | null; + message: string; + }; /** The run page link (`metadata.url` from `POST /ci/runs`). Absent when the repo isn't linked. */ runUrl?: string; /** Unified CI-signal metadata: roll-up line, footer, per-query links, docs link, icon keys. */ diff --git a/src/reporters/site-api.test.ts b/src/reporters/site-api.test.ts index 05302a5..4d87c22 100644 --- a/src/reporters/site-api.test.ts +++ b/src/reporters/site-api.test.ts @@ -1,5 +1,6 @@ import { test, expect, describe, afterEach, vi } from "vitest"; import { + classifyIngestFailure, compareRuns, fetchPreviousRun, gateEligibleNewQueries, @@ -268,6 +269,84 @@ describe("postToSiteApi payload baseBranch", () => { }); }); +describe("classifyIngestFailure", () => { + test("treats no-response and 5xx as transient (recoverable)", () => { + expect(classifyIngestFailure(null)).toBe("transient"); + expect(classifyIngestFailure(500)).toBe("transient"); + expect(classifyIngestFailure(503)).toBe("transient"); + }); + + test("treats 401/403 as auth (user must fix the token)", () => { + expect(classifyIngestFailure(401)).toBe("auth"); + expect(classifyIngestFailure(403)).toBe("auth"); + }); + + test("treats other 4xx as a rejected run (contract skew)", () => { + expect(classifyIngestFailure(400)).toBe("rejected"); + expect(classifyIngestFailure(422)).toBe("rejected"); + }); +}); + +describe("postToSiteApi outcome", () => { + afterEach(() => { + vi.unstubAllGlobals(); + vi.unstubAllEnvs(); + }); + + test("returns ok with the run on a 2xx response", async () => { + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue( + new Response(JSON.stringify({ id: "run-1", url: null }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }), + ), + ); + + const outcome = await postToSiteApi("https://api.querydoctor.com", [ + makeQuery("hash-a"), + ]); + + expect(outcome).toEqual({ + ok: true, + result: { id: "run-1", url: null, metadata: null }, + }); + }); + + test("returns a failure carrying the status and body on a non-2xx response", async () => { + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue( + new Response("ZodError: invalid constraintType", { status: 400 }), + ), + ); + + const outcome = await postToSiteApi("https://api.querydoctor.com", [ + makeQuery("hash-a"), + ]); + + expect(outcome).toEqual({ + ok: false, + failure: { status: 400, message: "ZodError: invalid constraintType" }, + }); + }); + + test("returns a failure with a null status when the request never completes", async () => { + vi.stubGlobal( + "fetch", + vi.fn().mockRejectedValue(new Error("network down")), + ); + + const outcome = await postToSiteApi("https://api.querydoctor.com", [ + makeQuery("hash-a"), + ]); + + expect(outcome.ok).toBe(false); + expect(outcome).toMatchObject({ failure: { status: null } }); + }); +}); + describe("gateEligibleNewQueries", () => { function withRecommendation( hash: string, diff --git a/src/reporters/site-api.ts b/src/reporters/site-api.ts index 4c3713b..fbb8936 100644 --- a/src/reporters/site-api.ts +++ b/src/reporters/site-api.ts @@ -149,6 +149,47 @@ export interface CiRunResult { metadata: CiRunMetadata | null; } +/** + * Why `POST /ci/runs` didn't save the run. `status` is the HTTP status, or null + * when the request never completed (network/timeout). `message` is the (capped) + * response body or error text, surfaced in the PR comment so the failure isn't + * silent. + */ +export interface PostRunFailure { + status: number | null; + message: string; +} + +export type PostRunOutcome = + | { ok: true; result: CiRunResult } + | { ok: false; failure: PostRunFailure }; + +/** + * How a rejected ingest should be treated, by recipient and recoverability: + * - `transient`: network/timeout/5xx — recoverable, re-run to retry. + * - `auth`: 401/403 — CI is misconfigured; the user must fix the token. + * - `rejected`: other 4xx — the API refused a computed run (e.g. analyzer/API + * contract skew); not the user's to fix and re-running won't help. + */ +export type IngestFailureKind = "transient" | "auth" | "rejected"; + +export function classifyIngestFailure(status: number | null): IngestFailureKind { + if (status === null || status >= 500) return "transient"; + if (status === 401 || status === 403) return "auth"; + return "rejected"; +} + +// Response bodies (e.g. a ZodError) can be large; cap what we echo into the PR +// comment so a failure banner stays readable. +const MAX_FAILURE_MESSAGE_LENGTH = 600; + +function truncateFailureMessage(message: string): string { + const trimmed = message.trim(); + return trimmed.length > MAX_FAILURE_MESSAGE_LENGTH + ? trimmed.slice(0, MAX_FAILURE_MESSAGE_LENGTH) + "…" + : trimmed; +} + export interface RunComparison { previousRunId: string; previousBranch: string; @@ -360,7 +401,7 @@ export async function postToSiteApi( statisticsMode?: StatisticsMode, computedStats?: ComputedStats, schema?: FullSchema, -): Promise { +): Promise { const payload: CiRunPayload = { repo: process.env.GITHUB_REPOSITORY ?? "", branch: process.env.GITHUB_HEAD_REF || process.env.GITHUB_REF_NAME || "", @@ -402,7 +443,13 @@ export async function postToSiteApi( if (!response.ok) { const text = await response.text(); console.warn(`Site API responded with ${response.status}: ${text}`); - return null; + return { + ok: false, + failure: { + status: response.status, + message: truncateFailureMessage(text), + }, + }; } const body = (await response.json()) as { id: string; @@ -411,13 +458,19 @@ export async function postToSiteApi( }; console.log(`Site API ingestion successful: ${JSON.stringify(body)}`); return { - id: body.id, - url: body.url ?? null, - metadata: body.metadata ?? null, + ok: true, + result: { + id: body.id, + url: body.url ?? null, + metadata: body.metadata ?? null, + }, }; } catch (err) { console.warn(`Failed to POST to Site API: ${err}`); - return null; + return { + ok: false, + failure: { status: null, message: truncateFailureMessage(String(err)) }, + }; } }