diff --git a/src/config.ts b/src/config.ts index e46adb3..bfdc801 100644 --- a/src/config.ts +++ b/src/config.ts @@ -1,9 +1,18 @@ +import type { RepoPolicyConfig } from "@query-doctor/core"; + export interface AnalyzerConfig { minimumCost: number; regressionThreshold: number; ignoredQueryHashes: string[]; acknowledgedQueryHashes: string[]; comparisonBranch?: string; + /** + * Per-condition CI policy overrides (#3500): condition key → `fail | warn | off`. + * Absent keys fall back to core's safe defaults, so `untested-data-access` blocks + * unless a repo softens it. Optional and empty until the Site repo config plumbs + * it through `getRepoConfig`; the gate honours it the moment it arrives. + */ + conditionPolicies?: RepoPolicyConfig; } export const DEFAULT_CONFIG: AnalyzerConfig = { diff --git a/src/gate/policy.test.ts b/src/gate/policy.test.ts new file mode 100644 index 0000000..e25eb58 --- /dev/null +++ b/src/gate/policy.test.ts @@ -0,0 +1,50 @@ +import { describe, expect, it } from "vitest"; +import { resolveVerdict } from "./policy.ts"; + +const untested = { + condition: "untested-data-access", + verdictClass: "uncertain-conservative-flag" as const, +}; + +describe("resolveVerdict", () => { + it("blocks an unverified data-access change by default", () => { + expect(resolveVerdict(untested)).toEqual({ + policy: "fail", + conclusion: "failure", + surfaced: true, + }); + }); + + it("softens to a surfaced, non-blocking neutral under a warn policy", () => { + expect( + resolveVerdict(untested, { "untested-data-access": "warn" }), + ).toEqual({ + policy: "warn", + conclusion: "neutral", + surfaced: true, + }); + }); + + it("suppresses the condition entirely under an off policy", () => { + expect( + resolveVerdict(untested, { "untested-data-access": "off" }), + ).toEqual({ + policy: "off", + conclusion: "success", + surfaced: false, + }); + }); + + it("keeps a passing verdict green even under a fail policy — the taxonomy drives the conclusion", () => { + expect( + resolveVerdict( + { condition: "untested-data-access", verdictClass: "pass" }, + { "untested-data-access": "fail" }, + ), + ).toEqual({ + policy: "fail", + conclusion: "success", + surfaced: true, + }); + }); +}); diff --git a/src/gate/policy.ts b/src/gate/policy.ts new file mode 100644 index 0000000..1b61cb0 --- /dev/null +++ b/src/gate/policy.ts @@ -0,0 +1,47 @@ +// Resolve a gate verdict to the CI conclusion it produces, using the shared +// failure taxonomy (#3498) and policy engine (#3500) from @query-doctor/core +// rather than a severity the analyzer decides for itself. +// +// The taxonomy fixes what a verdict class concludes: `uncertain-conservative-flag` +// concludes `failure`, so a data-access change Query Doctor could not verify +// blocks the check by default — an unverified result gates, framed as "could not +// check this, flagging to be safe", never as "your query is broken". The per-repo +// policy then decides whether this repo lets that condition fail (`fail`), softens +// it to a surfaced-but-non-blocking neutral (`warn`), or suppresses it (`off`). + +import { + conclusionForVerdict, + policyFor, + type CiConclusion, + type ConditionPolicy, + type RepoPolicyConfig, + type VerdictClass, +} from "@query-doctor/core"; + +export interface ResolvedVerdict { + /** The effective per-repo policy for the condition. */ + policy: ConditionPolicy; + /** CI conclusion after policy: `failure` blocks the check, `neutral`/`success` don't. */ + conclusion: CiConclusion; + /** False only when the policy is `off` — the condition is suppressed, not surfaced anywhere. */ + surfaced: boolean; +} + +/** + * Resolve a verdict's effective conclusion. `off` is reported as unsurfaced (the + * caller drops the condition — no comment block, no check annotation); every other + * policy surfaces it, with `warn` capping a taxonomy failure at a non-blocking + * neutral. The cap mirrors core's policy.ts `applyPolicy`, which core keeps private. + */ +export function resolveVerdict( + verdict: { condition: string; verdictClass: VerdictClass }, + config: RepoPolicyConfig = {}, +): ResolvedVerdict { + const policy = policyFor(verdict.condition, config); + if (policy === "off") { + return { policy, conclusion: "success", surfaced: false }; + } + const base = conclusionForVerdict(verdict); + const conclusion = policy === "warn" && base === "failure" ? "neutral" : base; + return { policy, conclusion, surfaced: true }; +} diff --git a/src/main.ts b/src/main.ts index 3836923..696e2ce 100644 --- a/src/main.ts +++ b/src/main.ts @@ -17,13 +17,14 @@ import { import { formatCost, queryPreview } from "./reporters/github/github.ts"; import { fetchPrChangedFiles } from "./gate/changed-files.ts"; import { evaluateTestPresence } from "./gate/test-presence.ts"; +import { resolveVerdict } from "./gate/policy.ts"; import { DEFAULT_CONFIG } from "./config.ts"; import { ApiClient } from "./remote/api-client.ts"; import { Remote } from "./remote/remote.ts"; import { ConnectionManager } from "./sync/connection-manager.ts"; import { PgbadgerSource } from "./sql/pgbadger.ts"; import type { RecentQuerySource } from "./sql/recent-query.ts"; -import type { FullSchema } from "@query-doctor/core"; +import type { FullSchema, RepoPolicyConfig } from "@query-doctor/core"; async function runInCI( targetPostgresUrl: Connectable, @@ -232,21 +233,46 @@ async function runInCI( } } + // Resolve the gate verdict's CI conclusion via the shared taxonomy (#3498) + // and policy engine (#3500) instead of a local severity. By default an + // unverified data-access change concludes `failure`, so it blocks the check — + // a "Warning" beside a green check is invisible to downstream reviewers + // (Site#3541). A repo can soften it to a non-blocking neutral (`warn`) or + // suppress it (`off`). Resolved before the report so `off` also drops the + // comment block, and so the comment can render the right framing. + if (reportContext.testPresenceVerdict) { + const policyConfig: RepoPolicyConfig = + ("conditionPolicies" in config ? config.conditionPolicies : undefined) ?? + {}; + const resolved = resolveVerdict( + reportContext.testPresenceVerdict, + policyConfig, + ); + if (resolved.surfaced) { + reportContext.testPresenceConclusion = resolved.conclusion; + } else { + reportContext.testPresenceVerdict = undefined; + } + } + console.log("Creating report...") // Generate PR comment with comparison data await runner.report(reportContext); - // The test-presence gate surfaces as a non-blocking warning, not a red X: - // it is a crude diff heuristic, so it warns loudly while the capture-based - // rungs (#3502/#3503) are built. Flipping it to a hard failure is opt-in via - // the per-repo policy (#3500). Framed as "unverified", never "your query is bad". + // Carry the verdict into the check itself: a blocking conclusion fails the + // run (red X), a softened one surfaces as a non-blocking annotation. Framed + // as "could not verify", never "your query is broken". if (reportContext.testPresenceVerdict) { const verdict = reportContext.testPresenceVerdict; const files = verdict.dataAccessFiles.map((f) => ` - ${f}`).join("\n"); - core.warning( + const message = `${verdict.reason}\n\nNext step: ${verdict.nextStep}\n\n` + - `Changed data-access files with no related data-layer test:\n${files}`, - ); + `Changed data-access files with no related data-layer test:\n${files}`; + if (reportContext.testPresenceConclusion === "failure") { + core.setFailed(message); + } else { + core.warning(message); + } } // Block PR if regressions exceed thresholds, or if a brand-new query ships diff --git a/src/reporters/github/github.test.ts b/src/reporters/github/github.test.ts index 6d7edef..3a20d3d 100644 --- a/src/reporters/github/github.test.ts +++ b/src/reporters/github/github.test.ts @@ -848,11 +848,17 @@ describe("test-presence verdict rendering", () => { dataAccessFiles: ["apps/api/src/users/user.repository.ts"], }; - test("renders the unverified banner, reason, next step, and flagged file", () => { - const output = renderTemplate(makeContext({ testPresenceVerdict: verdict })); - expect(output).toContain("[!WARNING]"); + test("renders a blocking caution block, reason, next step, and flagged file — never a Warning heading", () => { + const output = renderTemplate( + makeContext({ + testPresenceVerdict: verdict, + testPresenceConclusion: "failure", + }), + ); + expect(output).toContain("[!CAUTION]"); + expect(output).not.toContain("[!WARNING]"); expect(output).toContain( - "Unverified — this PR changes queries with no related data-layer test.", + "Could not verify — this PR changes queries with no related data-layer test.", ); expect(output).toContain(verdict.reason); expect(output).toContain(verdict.nextStep); @@ -860,8 +866,23 @@ describe("test-presence verdict rendering", () => { expect(output).toContain(verdict.triageHint); }); - test("omits the banner entirely when there is no verdict", () => { + test("renders a non-blocking note block when the policy softened the verdict to neutral", () => { + const output = renderTemplate( + makeContext({ + testPresenceVerdict: verdict, + testPresenceConclusion: "neutral", + }), + ); + expect(output).toContain("[!NOTE]"); + expect(output).not.toContain("[!CAUTION]"); + expect(output).not.toContain("[!WARNING]"); + expect(output).toContain(verdict.reason); + }); + + test("omits the block entirely when there is no verdict", () => { const output = renderTemplate(makeContext()); - expect(output).not.toContain("Unverified — this PR changes queries"); + expect(output).not.toContain( + "Could not verify — this PR changes queries", + ); }); }); diff --git a/src/reporters/github/success.md.j2 b/src/reporters/github/success.md.j2 index 06b86cc..9a8f023 100644 --- a/src/reporters/github/success.md.j2 +++ b/src/reporters/github/success.md.j2 @@ -23,8 +23,8 @@ {% endif %} {% if testPresenceVerdict %} -> [!WARNING] -> **Unverified — this PR changes queries with no related data-layer test.** +> [!{{ "CAUTION" if testPresenceConclusion == "failure" else "NOTE" }}] +> **Could not verify — this PR changes queries with no related data-layer test.** > {{ testPresenceVerdict.reason }} > > **Next step:** {{ testPresenceVerdict.nextStep }} diff --git a/src/reporters/reporter.ts b/src/reporters/reporter.ts index 806ba25..fb53378 100644 --- a/src/reporters/reporter.ts +++ b/src/reporters/reporter.ts @@ -1,4 +1,4 @@ -import type { ComputedStats, IndexIdentifier, StatisticsMode } from "@query-doctor/core"; +import type { CiConclusion, ComputedStats, IndexIdentifier, StatisticsMode } from "@query-doctor/core"; import type { CiRunMetadata, IngestFailureKind, @@ -113,10 +113,16 @@ export interface ReportContext { /** * The crude test-presence gate (#3496) flagged this PR: data-access code * changed with no data-layer test alongside it. Rendered as a conservative - * "could not verify" banner in the comment; also fails the check in `main`. + * "could not verify" block in the comment; also gates the check in `main`. * A pure diff heuristic — independent of the baseline comparison. */ testPresenceVerdict?: TestPresenceVerdict; + /** + * The gate verdict's resolved CI conclusion (taxonomy #3498 + policy #3500): + * `failure` renders the blocking framing, `neutral` the softened `warn` one. + * Absent when there is no surfaced verdict. + */ + testPresenceConclusion?: CiConclusion; } export interface IndexStatistic {