diff --git a/src/gate/changed-files.ts b/src/gate/changed-files.ts new file mode 100644 index 0000000..727b3fe --- /dev/null +++ b/src/gate/changed-files.ts @@ -0,0 +1,33 @@ +import * as github from "@actions/github"; +import type { ChangedFile } from "./test-presence.ts"; + +/** + * Fetch the PR's changed files from the GitHub API. Returns `null` when the run + * isn't a pull request (e.g. a push to the comparison branch) — the gate is a + * PR-diff heuristic, so with no PR there is nothing to evaluate. + * + * We use the API rather than `git diff` because a default `actions/checkout` is + * shallow: the base ref often isn't fetched, so a local diff would be unreliable. + * The `GITHUB_TOKEN` the action already receives has the `pull_requests: read` + * scope this needs. + */ +export async function fetchPrChangedFiles( + token: string, +): Promise { + const prNumber = github.context.payload.pull_request?.number; + if (typeof prNumber === "undefined") { + return null; + } + const octokit = github.getOctokit(token); + const files = await octokit.paginate(octokit.rest.pulls.listFiles, { + owner: github.context.repo.owner, + repo: github.context.repo.repo, + pull_number: prNumber, + per_page: 100, + }); + return files.map((f) => ({ + path: f.filename, + status: f.status, + patch: f.patch, + })); +} diff --git a/src/gate/test-presence.test.ts b/src/gate/test-presence.test.ts new file mode 100644 index 0000000..f10971a --- /dev/null +++ b/src/gate/test-presence.test.ts @@ -0,0 +1,156 @@ +import { describe, expect, test } from "vitest"; +import { + classifyChangedFiles, + evaluateTestPresence, + patchAddsQueryCode, + type ChangedFile, +} from "./test-presence.ts"; + +/** A minimal patch whose single added line is `line`. */ +function addPatch(line: string): string { + return `@@ -1,0 +1,1 @@\n+${line}`; +} + +const changed = ( + path: string, + line: string, + status = "modified", +): ChangedFile => ({ path, status, patch: addPatch(line) }); + +describe("patchAddsQueryCode", () => { + test("detects an ORM query call in added lines", () => { + expect(patchAddsQueryCode(addPatch("const u = await db.select().from(users);"))).toBe( + true, + ); + }); + + test("detects a raw SQL string", () => { + expect(patchAddsQueryCode(addPatch('await client.query("INSERT INTO users VALUES (1)");'))).toBe( + true, + ); + }); + + test("ignores a comment that merely mentions SQL keywords", () => { + expect(patchAddsQueryCode(addPatch("// TODO: select the user from the list"))).toBe( + false, + ); + }); + + test("ignores non-query code", () => { + expect(patchAddsQueryCode(addPatch("const total = items.length + 1;"))).toBe(false); + }); + + test("only looks at added lines, not removed ones", () => { + const patch = "@@ -1,1 +1,1 @@\n-await db.select().from(users);\n+const x = 1;"; + expect(patchAddsQueryCode(patch)).toBe(false); + }); + + test("is false for a missing patch", () => { + expect(patchAddsQueryCode(undefined)).toBe(false); + }); +}); + +describe("classifyChangedFiles", () => { + test("classes a query-bearing non-test file as data-access, wherever it lives", () => { + // Not named like a repository — content is what counts now. + const surface = classifyChangedFiles([ + changed("apps/api/src/users/user.service.ts", "return db.select().from(users);"), + ]); + expect(surface.dataAccessChanged).toEqual(["apps/api/src/users/user.service.ts"]); + }); + + test("does NOT flag a comment-only edit to a repository file", () => { + const surface = classifyChangedFiles([ + changed("apps/api/src/users/user.repository.ts", "// clarify the join order"), + ]); + expect(surface.dataAccessChanged).toEqual([]); + }); + + test("classes a query-bearing test as a data-layer test", () => { + const surface = classifyChangedFiles([ + changed( + "apps/api/src/users/user.repository.spec.ts", + "expect(await db.select().from(users)).toHaveLength(1);", + ), + ]); + expect(surface.dataLayerTestChanged).toEqual([ + "apps/api/src/users/user.repository.spec.ts", + ]); + expect(surface.dataAccessChanged).toEqual([]); + }); + + test("falls back to the filename prior when a patch is unavailable", () => { + const surface = classifyChangedFiles([ + { path: "apps/api/src/users/user.repository.ts", status: "modified" }, + ]); + expect(surface.dataAccessChanged).toEqual(["apps/api/src/users/user.repository.ts"]); + }); + + test("does not count a removed file as a change", () => { + const surface = classifyChangedFiles([ + changed("apps/api/src/users/user.repository.ts", "db.select().from(users)", "removed"), + ]); + expect(surface.dataAccessChanged).toEqual([]); + }); +}); + +describe("evaluateTestPresence", () => { + test("flags a query change with no related test", () => { + const verdict = evaluateTestPresence([ + changed("apps/api/src/users/user.repository.ts", "db.insert(users).values(u);"), + ]); + expect(verdict).toMatchObject({ + condition: "untested-data-access", + verdictClass: "uncertain-conservative-flag", + dataAccessFiles: ["apps/api/src/users/user.repository.ts"], + }); + }); + + test("passes when a related data-layer test changes alongside the query", () => { + const verdict = evaluateTestPresence([ + changed("apps/api/src/users/user.repository.ts", "db.insert(users).values(u);"), + changed( + "apps/api/src/users/user.repository.spec.ts", + "expect(await db.select().from(users)).toEqual([u]);", + ), + ]); + expect(verdict).toBeNull(); + }); + + test("still flags when only an UNRELATED data-layer test changed", () => { + const verdict = evaluateTestPresence([ + changed("apps/api/src/orders/order.repository.ts", "db.insert(orders).values(o);"), + changed( + "apps/api/src/users/user.repository.spec.ts", + "expect(await db.select().from(users)).toEqual([u]);", + ), + ]); + expect(verdict?.dataAccessFiles).toEqual([ + "apps/api/src/orders/order.repository.ts", + ]); + }); + + test("passes when the PR changes no query code", () => { + const verdict = evaluateTestPresence([ + changed("apps/app/src/components/Button.tsx", "const label = props.label;"), + changed("docs/guide.md", "Some prose about SELECT and FROM."), + ]); + expect(verdict).toBeNull(); + }); + + test("does not fire on a comment-only edit to a repository file", () => { + const verdict = evaluateTestPresence([ + changed("apps/api/src/users/user.repository.ts", "// rename for clarity"), + ]); + expect(verdict).toBeNull(); + }); + + test("frames the finding as unverified, not a bad query", () => { + const verdict = evaluateTestPresence([ + changed("src/dal/orders.ts", "db.update(orders).set({ shipped: true });"), + ]); + expect(verdict?.reason).toContain("could not verify"); + expect(verdict?.reason).toContain("flagged conservatively"); + expect(verdict?.nextStep).toContain("test"); + }); +}); diff --git a/src/gate/test-presence.ts b/src/gate/test-presence.ts new file mode 100644 index 0000000..f44307c --- /dev/null +++ b/src/gate/test-presence.ts @@ -0,0 +1,270 @@ +// The test-presence gate (#3496) — the MVP tracer bullet for the CI⇄MCP verdict +// loop. It asks one question of a PR's diff: "did this change add or alter a +// query without a real-DB (repository/integration) test alongside it?" — and, if +// so, emits a conservative "we could not verify this" flag. +// +// It never claims a query is *bad*. Query Doctor only analyses SQL that a +// real-DB test runs against Postgres; a query change with no such test produces +// no captured query, so CI would go green having never seen it. This gate +// reports that blind spot honestly. +// +// It is a pure *diff* heuristic — it reads the diff's added lines to decide +// whether a query changed (still no runtime capture, no query-to-site mapping; +// those are the later capture-based rungs, #3502/#3503). Reading the diff's +// content, rather than guessing from the filename, is what keeps it from +// reddening a comment-only edit to a repository file or missing a query added to +// a plainly-named `service.ts`. It ships warn-only until the capture-based rungs +// make it precise enough to block. + +/** One entry from the PR's changed-file list (GitHub's `pulls.listFiles`). */ +export interface ChangedFile { + path: string; + /** GitHub file status: added | modified | removed | renamed | copied | changed | unchanged. */ + status: string; + /** Unified-diff hunks for the file; absent for large or binary files. */ + patch?: string; +} + +/** + * Path and content heuristics. Kept as data (not hard-coded regexes) so a later + * per-repo config (#3500) can override the defaults without touching the gate. + */ +export interface TestPresenceConfig { + /** Marks a path as a test file of any kind. */ + testFilePatterns: RegExp[]; + /** Extensions worth inspecting for query code — a doc/config file never is. */ + sourceFilePatterns: RegExp[]; + /** Content signals that an added diff line is (part of) a query. */ + queryCodePatterns: RegExp[]; + /** Fallback data-access signal, used only when a file's patch is unavailable. */ + dataAccessPathPatterns: RegExp[]; + /** Marks a test path as a real-DB data-layer (repository/integration) test. */ + dataLayerTestPathPatterns: RegExp[]; +} + +export const DEFAULT_TEST_PRESENCE_CONFIG: TestPresenceConfig = { + testFilePatterns: [ + /\.(test|spec)\.[cm]?[jt]sx?$/i, // *.test.ts, *.spec.tsx, ... + /(^|\/)__tests__\//i, + /(^|\/)tests?\//i, + /(^|\/)test_[^/]+\.py$/i, // Python: test_foo.py + /_test\.(py|go|rb)$/i, // Go/Python/Ruby: foo_test.go + ], + sourceFilePatterns: [/\.([cm]?[jt]sx?|py|go|rb|java|kt|rs|php|scala|cs|sql)$/i], + // Prefer high-precision ORM / query-builder calls; a small set of raw-SQL + // shapes catches string queries. Comment lines are stripped before matching, + // so a code comment mentioning "select" won't trip it. + queryCodePatterns: [ + /\bdb\.(select|insert|update|delete)\b/i, + /\.(execute|query)\s*\(/i, + /\bsql`/, // drizzle sql`...` tag + /\bdrizzle\s*\(/i, + /\bknex\b/i, + /\bprisma\.\w+\.(find\w*|create|update|delete|upsert|count|aggregate)\b/i, + /\.createQueryBuilder\s*\(/i, + /\bgetRepository\s*\(/i, + /\.\$(queryRaw|executeRaw)/, + /\.(leftJoin|innerJoin|rightJoin)\s*\(/i, + /\binsert\s+into\b/i, + /\bdelete\s+from\b/i, + /\bupdate\b[^\n]{0,80}\bset\b/i, + /\bselect\b[\s\S]{0,300}?\bfrom\b/i, + /\b(create|alter|drop)\s+(table|index|view|materialized\s+view)\b/i, + ], + dataAccessPathPatterns: [ + /(^|\/)[^/]*repositor(y|ies)[^/]*\.[cm]?[jt]s$/i, + /(^|\/)[^/]*\.repo\.[cm]?[jt]s$/i, + /(^|\/)(dal|data-access)\//i, + ], + dataLayerTestPathPatterns: [ + /repositor(y|ies)/i, + /\.repo\./i, + /integration/i, + /(^|\/)(dal|data-access)\//i, + ], +}; + +/** + * A changed file "changed" for gating purposes when its content could have + * introduced or altered a query. A pure deletion removes surface, it doesn't add + * an unverified query, so `removed`/`unchanged` never trip the gate. + */ +function isChanged(status: string): boolean { + return status !== "removed" && status !== "unchanged"; +} + +function matchesAny(path: string, patterns: RegExp[]): boolean { + return patterns.some((p) => p.test(path)); +} + +function isTestFile(path: string, config: TestPresenceConfig): boolean { + return matchesAny(path, config.testFilePatterns); +} + +/** The added lines of a unified diff, with the leading `+` removed. */ +function addedLines(patch: string): string { + return patch + .split("\n") + .filter((line) => line.startsWith("+") && !line.startsWith("+++")) + .map((line) => line.slice(1)) + .join("\n"); +} + +/** Drop lines that are plainly comments, so prose mentioning SQL keywords doesn't match. */ +function stripCommentLines(text: string): string { + return text + .split("\n") + .filter((line) => { + const trimmed = line.trim(); + return !( + trimmed.startsWith("//") || + trimmed.startsWith("*") || + trimmed.startsWith("/*") || + trimmed.startsWith("#") || + trimmed.startsWith("--") + ); + }) + .join("\n"); +} + +/** True when the diff's *added* lines contain query code. */ +export function patchAddsQueryCode( + patch: string | undefined, + config: TestPresenceConfig = DEFAULT_TEST_PRESENCE_CONFIG, +): boolean { + if (!patch) return false; + const added = stripCommentLines(addedLines(patch)); + return matchesAny(added, config.queryCodePatterns); +} + +/** + * Did this non-test change add or alter a query? Uses the diff content when + * available; falls back to the filename prior only when the patch is missing + * (large/binary files), where content can't be inspected. + */ +function changedQueryCode( + file: ChangedFile, + config: TestPresenceConfig, +): boolean { + if (!matchesAny(file.path, config.sourceFilePatterns)) return false; + if (file.patch !== undefined) return patchAddsQueryCode(file.patch, config); + return matchesAny(file.path, config.dataAccessPathPatterns); +} + +/** A test counts as a data-layer test if it exercises query code or is named like one. */ +function isDataLayerTest(file: ChangedFile, config: TestPresenceConfig): boolean { + return ( + isTestFile(file.path, config) && + (patchAddsQueryCode(file.patch, config) || + matchesAny(file.path, config.dataLayerTestPathPatterns)) + ); +} + +function directory(path: string): string { + const slash = path.lastIndexOf("/"); + return slash === -1 ? "" : path.slice(0, slash); +} + +/** Filename without directory, extension, or a `.test`/`.spec` suffix. */ +function baseStem(path: string): string { + const name = path.slice(path.lastIndexOf("/") + 1); + return name + .replace(/\.(test|spec)\./i, ".") + .replace(/\.[cm]?[jt]sx?$/i, "") + .replace(/\.(py|go|rb)$/i, ""); +} + +/** + * Whether a data-layer test plausibly covers a changed data-access file: same + * directory, or the test's name carries the file's stem (`user.repository.ts` ↔ + * `user.repository.spec.ts`, `orders.ts` ↔ `orders.integration.test.ts`). + * Lenient on purpose — a loose match makes the gate under-fire, the safe side. + */ +function isRelated(dataAccessPath: string, testPath: string): boolean { + if (directory(dataAccessPath) === directory(testPath)) return true; + const stem = baseStem(dataAccessPath); + return stem.length > 0 && baseStem(testPath).includes(stem); +} + +export interface ChangedSurface { + /** Non-test files whose diff added/altered query code. */ + dataAccessChanged: string[]; + /** Real-DB data-layer tests the PR added or changed. */ + dataLayerTestChanged: string[]; +} + +export function classifyChangedFiles( + files: ChangedFile[], + config: TestPresenceConfig = DEFAULT_TEST_PRESENCE_CONFIG, +): ChangedSurface { + const dataAccessChanged: string[] = []; + const dataLayerTestChanged: string[] = []; + for (const file of files) { + if (!isChanged(file.status)) continue; + if (isTestFile(file.path, config)) { + if (isDataLayerTest(file, config)) dataLayerTestChanged.push(file.path); + } else if (changedQueryCode(file, config)) { + dataAccessChanged.push(file.path); + } + } + return { dataAccessChanged, dataLayerTestChanged }; +} + +/** + * The v0 inline verdict payload (#3496). A precursor to the versioned, shared + * verdict contract (#3497) — deliberately inline here so the MVP ships without a + * cross-repo dependency on the published contract. + */ +export interface TestPresenceVerdict { + condition: "untested-data-access"; + /** The honest epistemic state: not "bad", but "we could not check this". */ + verdictClass: "uncertain-conservative-flag"; + reason: string; + nextStep: string; + triageHint: string; + /** The changed data-access files with no related data-layer test — what to cover. */ + dataAccessFiles: string[]; +} + +const REASON = + "This PR adds or changes queries in data-access code, but no related real-DB " + + "(repository/integration) test changed, so Query Doctor could not verify them " + + "— nothing here exercises them against Postgres. This is flagged " + + "conservatively; it is not a claim that the query is wrong."; + +const NEXT_STEP = + "Add or update a repository/integration test that exercises the changed " + + "queries against a real database, following your project's testing " + + "conventions — or, if a test genuinely isn't needed (a revert, generated " + + "code, a column-drop migration), triage it."; + +const TRIAGE_HINT = + "If this change intentionally needs no test, note why on the PR so the " + + "exception is auditable rather than silent."; + +/** + * Evaluate the gate. Returns a verdict listing the changed data-access files + * that have no related data-layer test, or `null` when the PR passes — no query + * change, or every query change has a related test alongside it. + */ +export function evaluateTestPresence( + files: ChangedFile[], + config: TestPresenceConfig = DEFAULT_TEST_PRESENCE_CONFIG, +): TestPresenceVerdict | null { + const { dataAccessChanged, dataLayerTestChanged } = classifyChangedFiles( + files, + config, + ); + const untested = dataAccessChanged.filter( + (path) => !dataLayerTestChanged.some((test) => isRelated(path, test)), + ); + if (untested.length === 0) return null; + return { + condition: "untested-data-access", + verdictClass: "uncertain-conservative-flag", + reason: REASON, + nextStep: NEXT_STEP, + triageHint: TRIAGE_HINT, + dataAccessFiles: untested, + }; +} diff --git a/src/main.ts b/src/main.ts index d328178..13c38bf 100644 --- a/src/main.ts +++ b/src/main.ts @@ -15,6 +15,8 @@ import { postToSiteApi, } from "./reporters/site-api.ts"; import { formatCost, queryPreview } from "./reporters/github/github.ts"; +import { fetchPrChangedFiles } from "./gate/changed-files.ts"; +import { evaluateTestPresence } from "./gate/test-presence.ts"; import { DEFAULT_CONFIG } from "./config.ts"; import { ApiClient } from "./remote/api-client.ts"; import { Remote } from "./remote/remote.ts"; @@ -202,10 +204,39 @@ async function runInCI( ); } + // Crude test-presence gate (#3496): flag data-access changes that ship with + // no data-layer test. A pure diff heuristic — independent of capture and of + // the baseline comparison, so it runs even on a brand-new PR with no + // baseline. Best-effort: a GitHub API hiccup must never sink the whole run. + if (env.GITHUB_TOKEN) { + try { + const changedFiles = await fetchPrChangedFiles(env.GITHUB_TOKEN); + if (changedFiles) { + reportContext.testPresenceVerdict = + evaluateTestPresence(changedFiles) ?? undefined; + } + } catch (err) { + log.warn(`Test-presence gate skipped: ${err}`, "main"); + } + } + 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". + if (reportContext.testPresenceVerdict) { + const verdict = reportContext.testPresenceVerdict; + const files = verdict.dataAccessFiles.map((f) => ` - ${f}`).join("\n"); + core.warning( + `${verdict.reason}\n\nNext step: ${verdict.nextStep}\n\n` + + `Changed data-access files with no related data-layer test:\n${files}`, + ); + } + // Block PR if regressions exceed thresholds, or if a brand-new query ships // with a high-confidence index recommendation (#3281). New queries have no // baseline so they can never regress; the new-query gate catches the missing diff --git a/src/reporters/github/github.test.ts b/src/reporters/github/github.test.ts index e16f4af..f5fc760 100644 --- a/src/reporters/github/github.test.ts +++ b/src/reporters/github/github.test.ts @@ -798,3 +798,31 @@ describe("schema change section", () => { expect(output).not.toContain("1 schema changes"); }); }); + +describe("test-presence verdict rendering", () => { + const verdict = { + condition: "untested-data-access" as const, + verdictClass: "uncertain-conservative-flag" as const, + reason: "This PR changes data-access code but could not verify it.", + nextStep: "Add a repository/integration test that exercises it.", + triageHint: "Note why on the PR if no test is needed.", + 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]"); + expect(output).toContain( + "Unverified — this PR changes queries with no related data-layer test.", + ); + expect(output).toContain(verdict.reason); + expect(output).toContain(verdict.nextStep); + expect(output).toContain("`apps/api/src/users/user.repository.ts`"); + expect(output).toContain(verdict.triageHint); + }); + + test("omits the banner entirely when there is no verdict", () => { + const output = renderTemplate(makeContext()); + expect(output).not.toContain("Unverified — this PR changes queries"); + }); +}); diff --git a/src/reporters/github/success.md.j2 b/src/reporters/github/success.md.j2 index 999e9f9..cb2ab8b 100644 --- a/src/reporters/github/success.md.j2 +++ b/src/reporters/github/success.md.j2 @@ -19,6 +19,21 @@ {% endif %} +{% endif %} +{% if testPresenceVerdict %} +> [!WARNING] +> **Unverified — this PR changes queries with no related data-layer test.** +> {{ testPresenceVerdict.reason }} +> +> **Next step:** {{ testPresenceVerdict.nextStep }} +> +> Changed data-access files with no related data-layer test: +{% for f in testPresenceVerdict.dataAccessFiles %} +> - `{{ f }}` +{% endfor %} +> +> _{{ testPresenceVerdict.triageHint }}_ + {% endif %} {% if hasComparison %} {{ queryStats.analyzed | default('?') }} queries analyzed diff --git a/src/reporters/reporter.ts b/src/reporters/reporter.ts index 5226ac8..9334249 100644 --- a/src/reporters/reporter.ts +++ b/src/reporters/reporter.ts @@ -4,6 +4,7 @@ import type { IngestFailureKind, RunComparison, } from "./site-api.ts"; +import type { TestPresenceVerdict } from "../gate/test-presence.ts"; export interface Reporter { provider(): string; @@ -108,6 +109,13 @@ export interface ReportContext { runUrl?: string; /** Unified CI-signal metadata: roll-up line, footer, per-query links, docs link, icon keys. */ runMetadata?: CiRunMetadata; + /** + * 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`. + * A pure diff heuristic — independent of the baseline comparison. + */ + testPresenceVerdict?: TestPresenceVerdict; } export interface IndexStatistic {