From 527f8b6360d66253a335f9601291013e05dec1bc Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Tue, 7 Jul 2026 06:11:53 -0400 Subject: [PATCH 1/2] feat(ci): add crude test-presence gate (#3496) The MVP tracer bullet for the CI-MCP verdict loop. In a PR, when data-access code changes but no real-DB (repository/integration) test changes alongside it, flag the run: Query Doctor never saw those queries (nothing runs them against Postgres), so a green check would misread as "safe". A pure diff heuristic over the PR's changed files (GitHub API) -- no capture, no query-to-site mapping. Conservative by design: it asserts *unverified*, never *bad*, and only fires when data-access changed with zero data-layer tests. Emits an inline v0 verdict payload (why + next step + triage hint), renders it as a "could not verify" banner in the PR comment, and fails the check. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/gate/changed-files.ts | 29 +++++ src/gate/test-presence.test.ts | 109 +++++++++++++++++ src/gate/test-presence.ts | 175 ++++++++++++++++++++++++++++ src/main.ts | 30 +++++ src/reporters/github/github.test.ts | 28 +++++ src/reporters/github/success.md.j2 | 15 +++ src/reporters/reporter.ts | 8 ++ 7 files changed, 394 insertions(+) create mode 100644 src/gate/changed-files.ts create mode 100644 src/gate/test-presence.test.ts create mode 100644 src/gate/test-presence.ts diff --git a/src/gate/changed-files.ts b/src/gate/changed-files.ts new file mode 100644 index 0000000..30686a7 --- /dev/null +++ b/src/gate/changed-files.ts @@ -0,0 +1,29 @@ +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 })); +} diff --git a/src/gate/test-presence.test.ts b/src/gate/test-presence.test.ts new file mode 100644 index 0000000..fe7ce39 --- /dev/null +++ b/src/gate/test-presence.test.ts @@ -0,0 +1,109 @@ +import { describe, expect, test } from "vitest"; +import { + classifyChangedFiles, + evaluateTestPresence, + type ChangedFile, +} from "./test-presence.ts"; + +const added = (path: string): ChangedFile => ({ path, status: "added" }); +const modified = (path: string): ChangedFile => ({ path, status: "modified" }); +const removed = (path: string): ChangedFile => ({ path, status: "removed" }); + +describe("classifyChangedFiles", () => { + test("buckets a repository and its spec into different surfaces", () => { + const surface = classifyChangedFiles([ + modified("apps/api/src/users/user.repository.ts"), + modified("apps/api/src/users/user.repository.spec.ts"), + ]); + expect(surface).toEqual({ + dataAccessChanged: ["apps/api/src/users/user.repository.ts"], + dataLayerTestChanged: ["apps/api/src/users/user.repository.spec.ts"], + }); + }); + + test("treats files under a dal/ directory as data-access", () => { + const surface = classifyChangedFiles([added("src/dal/orders.ts")]); + expect(surface.dataAccessChanged).toEqual(["src/dal/orders.ts"]); + expect(surface.dataLayerTestChanged).toEqual([]); + }); + + test("counts an integration test as a data-layer test", () => { + const surface = classifyChangedFiles([ + added("test/orders.integration.test.ts"), + ]); + expect(surface.dataLayerTestChanged).toEqual([ + "test/orders.integration.test.ts", + ]); + expect(surface.dataAccessChanged).toEqual([]); + }); + + test("ignores non-data-access files entirely", () => { + const surface = classifyChangedFiles([ + modified("apps/app/src/components/Button.tsx"), + modified("README.md"), + ]); + expect(surface).toEqual({ dataAccessChanged: [], dataLayerTestChanged: [] }); + }); + + test("does not count a removed data-access file as a change", () => { + const surface = classifyChangedFiles([ + removed("apps/api/src/users/user.repository.ts"), + ]); + expect(surface.dataAccessChanged).toEqual([]); + }); +}); + +describe("evaluateTestPresence", () => { + test("flags data-access change with no data-layer test", () => { + const verdict = evaluateTestPresence([ + modified("apps/api/src/users/user.repository.ts"), + ]); + expect(verdict).not.toBeNull(); + expect(verdict).toMatchObject({ + condition: "untested-data-access", + verdictClass: "uncertain-conservative-flag", + dataAccessFiles: ["apps/api/src/users/user.repository.ts"], + }); + }); + + test("passes when a data-layer test changes alongside the data-access code", () => { + const verdict = evaluateTestPresence([ + modified("apps/api/src/users/user.repository.ts"), + added("apps/api/src/users/user.repository.spec.ts"), + ]); + expect(verdict).toBeNull(); + }); + + test("passes when the PR changes no data-access code", () => { + const verdict = evaluateTestPresence([ + modified("apps/app/src/components/Button.tsx"), + added("docs/guide.md"), + ]); + expect(verdict).toBeNull(); + }); + + test("does not fire on a pure data-access deletion", () => { + const verdict = evaluateTestPresence([ + removed("apps/api/src/users/user.repository.ts"), + ]); + expect(verdict).toBeNull(); + }); + + test("frames the finding as unverified, not as a bad query", () => { + const verdict = evaluateTestPresence([ + added("src/dal/orders.ts"), + ]); + expect(verdict?.reason).toContain("could not verify"); + expect(verdict?.reason).toContain("flagged conservatively"); + expect(verdict?.nextStep).toContain("test"); + }); + + test("evaluates only diff-introduced surface, not pre-existing code", () => { + // A frontend-only PR: pre-existing repositories elsewhere are untouched and + // must not trip the gate, because they aren't in the diff. + const verdict = evaluateTestPresence([ + modified("apps/app/src/routes/dashboard.tsx"), + ]); + expect(verdict).toBeNull(); + }); +}); diff --git a/src/gate/test-presence.ts b/src/gate/test-presence.ts new file mode 100644 index 0000000..bdcde0c --- /dev/null +++ b/src/gate/test-presence.ts @@ -0,0 +1,175 @@ +// The crude test-presence gate (#3496) — the MVP tracer bullet for the CI⇄MCP +// verdict loop. It is a *pure diff heuristic*: given the files a PR changed, it +// asks one question — "did this PR touch data-access code without touching a +// real-DB (repository/integration) test?" — 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 actually runs against Postgres; a data-access 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 instead of letting silence read as +// safety. By design it *under-fires*: it only speaks up when data-access code +// changed and no data-layer test changed alongside it. + +/** 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; +} + +/** + * Path heuristics that decide what "data-access code" and "data-layer test" + * look like. Kept as data (not hard-coded regexes) so a later per-repo config + * (#3500) can override the defaults without touching the gate logic. + */ +export interface TestPresenceConfig { + /** Marks a path as a test file of any kind. */ + testFilePatterns: RegExp[]; + /** Marks a non-test path as data-access (query-emitting) code. */ + dataAccessPatterns: RegExp[]; + /** Marks a test path as a real-DB data-layer (repository/integration) test. */ + dataLayerTestPatterns: RegExp[]; +} + +// Conservative defaults, tuned to under-fire. `repository` / `dal` / +// `data-access` are the clearest data-access signals and match this project's +// own convention (`apps/api/**/*.repository.ts`). Anything narrower would never +// fire; anything broader risks reddening PRs over unrelated code. +export const DEFAULT_TEST_PRESENCE_CONFIG: TestPresenceConfig = { + testFilePatterns: [ + /\.(test|spec)\.[cm]?[jt]sx?$/i, // *.test.ts, *.spec.tsx, *.test.mjs, ... + /(^|\/)__tests__\//i, + /(^|\/)tests?\//i, + /(^|\/)test_[^/]+\.py$/i, // Python: test_foo.py + /_test\.(py|go|rb)$/i, // Go/Python/Ruby: foo_test.go + ], + dataAccessPatterns: [ + /(^|\/)[^/]*repositor(y|ies)[^/]*\.[cm]?[jt]sx?$/i, // user.repository.ts, repositories.ts + /(^|\/)[^/]*\.repo\.[cm]?[jt]sx?$/i, // user.repo.ts + /(^|\/)(dal|data-access)\//i, // src/dal/**, src/data-access/** + ], + dataLayerTestPatterns: [ + /repositor(y|ies)/i, // *.repository.spec.ts + /\.repo\./i, + /integration/i, // *.integration.test.ts + /(^|\/)(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); +} + +function isDataAccessFile(path: string, config: TestPresenceConfig): boolean { + return ( + !isTestFile(path, config) && matchesAny(path, config.dataAccessPatterns) + ); +} + +function isDataLayerTest(path: string, config: TestPresenceConfig): boolean { + return ( + isTestFile(path, config) && matchesAny(path, config.dataLayerTestPatterns) + ); +} + +export interface ChangedSurface { + /** Non-test data-access files the PR added or changed. */ + dataAccessChanged: string[]; + /** Real-DB data-layer tests the PR added or changed. */ + dataLayerTestChanged: string[]; +} + +/** + * Bucket a PR's changed files into the two surfaces the gate cares about. A + * repository *test* (`user.repository.spec.ts`) counts as a data-layer test, + * not as data-access code — so changing a repository and its test together + * satisfies the gate, while changing the repository alone does not. + */ +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 (isDataLayerTest(file.path, config)) { + dataLayerTestChanged.push(file.path); + } else if (isDataAccessFile(file.path, 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 that triggered the flag. */ + dataAccessFiles: string[]; +} + +const REASON = + "This PR changes data-access code but no real-DB (repository/integration) test " + + "changed alongside it, so Query Doctor could not verify the queries it " + + "introduces — nothing here exercises them against Postgres. This is flagged " + + "conservatively; it is not a claim that the query is wrong."; + +const NEXT_STEP = + "Add a repository/integration test that exercises the changed data-access " + + "code 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 data-access change intentionally needs no test, note why on the PR so " + + "the exception is auditable rather than silent."; + +/** + * Evaluate the gate. Returns a verdict when the PR trips the flag, or `null` + * when it passes — the two passing cases being "no data-access change" and + * "data-access change with a data-layer test alongside it". + */ +export function evaluateTestPresence( + files: ChangedFile[], + config: TestPresenceConfig = DEFAULT_TEST_PRESENCE_CONFIG, +): TestPresenceVerdict | null { + const { dataAccessChanged, dataLayerTestChanged } = classifyChangedFiles( + files, + config, + ); + if (dataAccessChanged.length === 0 || dataLayerTestChanged.length > 0) { + return null; + } + return { + condition: "untested-data-access", + verdictClass: "uncertain-conservative-flag", + reason: REASON, + nextStep: NEXT_STEP, + triageHint: TRIAGE_HINT, + dataAccessFiles: dataAccessChanged, + }; +} diff --git a/src/main.ts b/src/main.ts index d328178..67a99fb 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,38 @@ 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 is a coverage-gap condition, not a regression, so it + // fails the check on its own — independent of the comparison block below. + // Framed as "unverified", never as "your query is bad". + if (reportContext.testPresenceVerdict) { + const verdict = reportContext.testPresenceVerdict; + const files = verdict.dataAccessFiles.map((f) => ` - ${f}`).join("\n"); + core.setFailed( + `${verdict.reason}\n\nNext step: ${verdict.nextStep}\n\n` + + `Changed data-access files with no 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..ffb0674 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 data-access code with no 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 data-access code"); + }); +}); diff --git a/src/reporters/github/success.md.j2 b/src/reporters/github/success.md.j2 index 999e9f9..09fe6b9 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 data-access code with no data-layer test.** +> {{ testPresenceVerdict.reason }} +> +> **Next step:** {{ testPresenceVerdict.nextStep }} +> +> Changed data-access files with no 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 { From 73b5f3f58f2d3b2189a13b0e273472ee5e08fff7 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Sirois Date: Tue, 7 Jul 2026 11:40:53 -0400 Subject: [PATCH 2/2] feat(ci): detect queries by diff content + warn-only (#3496) Raises the gate's accuracy from a filename guess to reading the diff. - Content detection: classify a change as data-access when its added diff lines actually contain query code (ORM/query-builder calls or raw SQL), not when the filename looks like a repository. This kills the dominant false positives (a comment-only or whitespace edit to a repository file, a `UserRepositoryCard.tsx` component) and the dominant false negatives (a query added to a plainly-named service.ts / db.ts). Filename is now only a fallback for files whose patch is unavailable (large/binary). - Comment lines are stripped before matching, and only source files are inspected, so prose or docs mentioning SQL keywords don't trip it. - Relate test to code: a query change is satisfied only by a data-layer test in the same directory or carrying the file's stem -- you can no longer silence it by touching an unrelated repository test, and the comment names exactly which files lack a related test. - Warn-only: surfaces via core.warning, not core.setFailed. The heuristic is still crude, 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). Still a pure diff heuristic -- no runtime capture, no query-to-site mapping. Those remain #3502/#3503. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/gate/changed-files.ts | 6 +- src/gate/test-presence.test.ts | 137 +++++++++++------ src/gate/test-presence.ts | 221 ++++++++++++++++++++-------- src/main.ts | 11 +- src/reporters/github/github.test.ts | 4 +- src/reporters/github/success.md.j2 | 4 +- 6 files changed, 265 insertions(+), 118 deletions(-) diff --git a/src/gate/changed-files.ts b/src/gate/changed-files.ts index 30686a7..727b3fe 100644 --- a/src/gate/changed-files.ts +++ b/src/gate/changed-files.ts @@ -25,5 +25,9 @@ export async function fetchPrChangedFiles( pull_number: prNumber, per_page: 100, }); - return files.map((f) => ({ path: f.filename, status: f.status })); + 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 index fe7ce39..f10971a 100644 --- a/src/gate/test-presence.test.ts +++ b/src/gate/test-presence.test.ts @@ -2,63 +2,103 @@ import { describe, expect, test } from "vitest"; import { classifyChangedFiles, evaluateTestPresence, + patchAddsQueryCode, type ChangedFile, } from "./test-presence.ts"; -const added = (path: string): ChangedFile => ({ path, status: "added" }); -const modified = (path: string): ChangedFile => ({ path, status: "modified" }); -const removed = (path: string): ChangedFile => ({ path, status: "removed" }); +/** 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("buckets a repository and its spec into different surfaces", () => { + 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([ - modified("apps/api/src/users/user.repository.ts"), - modified("apps/api/src/users/user.repository.spec.ts"), + changed("apps/api/src/users/user.service.ts", "return db.select().from(users);"), ]); - expect(surface).toEqual({ - dataAccessChanged: ["apps/api/src/users/user.repository.ts"], - dataLayerTestChanged: ["apps/api/src/users/user.repository.spec.ts"], - }); + expect(surface.dataAccessChanged).toEqual(["apps/api/src/users/user.service.ts"]); }); - test("treats files under a dal/ directory as data-access", () => { - const surface = classifyChangedFiles([added("src/dal/orders.ts")]); - expect(surface.dataAccessChanged).toEqual(["src/dal/orders.ts"]); - expect(surface.dataLayerTestChanged).toEqual([]); + 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("counts an integration test as a data-layer test", () => { + test("classes a query-bearing test as a data-layer test", () => { const surface = classifyChangedFiles([ - added("test/orders.integration.test.ts"), + changed( + "apps/api/src/users/user.repository.spec.ts", + "expect(await db.select().from(users)).toHaveLength(1);", + ), ]); expect(surface.dataLayerTestChanged).toEqual([ - "test/orders.integration.test.ts", + "apps/api/src/users/user.repository.spec.ts", ]); expect(surface.dataAccessChanged).toEqual([]); }); - test("ignores non-data-access files entirely", () => { + test("falls back to the filename prior when a patch is unavailable", () => { const surface = classifyChangedFiles([ - modified("apps/app/src/components/Button.tsx"), - modified("README.md"), + { path: "apps/api/src/users/user.repository.ts", status: "modified" }, ]); - expect(surface).toEqual({ dataAccessChanged: [], dataLayerTestChanged: [] }); + expect(surface.dataAccessChanged).toEqual(["apps/api/src/users/user.repository.ts"]); }); - test("does not count a removed data-access file as a change", () => { + test("does not count a removed file as a change", () => { const surface = classifyChangedFiles([ - removed("apps/api/src/users/user.repository.ts"), + changed("apps/api/src/users/user.repository.ts", "db.select().from(users)", "removed"), ]); expect(surface.dataAccessChanged).toEqual([]); }); }); describe("evaluateTestPresence", () => { - test("flags data-access change with no data-layer test", () => { + test("flags a query change with no related test", () => { const verdict = evaluateTestPresence([ - modified("apps/api/src/users/user.repository.ts"), + changed("apps/api/src/users/user.repository.ts", "db.insert(users).values(u);"), ]); - expect(verdict).not.toBeNull(); expect(verdict).toMatchObject({ condition: "untested-data-access", verdictClass: "uncertain-conservative-flag", @@ -66,44 +106,51 @@ describe("evaluateTestPresence", () => { }); }); - test("passes when a data-layer test changes alongside the data-access code", () => { + test("passes when a related data-layer test changes alongside the query", () => { const verdict = evaluateTestPresence([ - modified("apps/api/src/users/user.repository.ts"), - added("apps/api/src/users/user.repository.spec.ts"), + 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("passes when the PR changes no data-access code", () => { + test("still flags when only an UNRELATED data-layer test changed", () => { const verdict = evaluateTestPresence([ - modified("apps/app/src/components/Button.tsx"), - added("docs/guide.md"), + 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", ]); - expect(verdict).toBeNull(); }); - test("does not fire on a pure data-access deletion", () => { + test("passes when the PR changes no query code", () => { const verdict = evaluateTestPresence([ - removed("apps/api/src/users/user.repository.ts"), + 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("frames the finding as unverified, not as a bad query", () => { + test("does not fire on a comment-only edit to a repository file", () => { const verdict = evaluateTestPresence([ - added("src/dal/orders.ts"), + changed("apps/api/src/users/user.repository.ts", "// rename for clarity"), ]); - expect(verdict?.reason).toContain("could not verify"); - expect(verdict?.reason).toContain("flagged conservatively"); - expect(verdict?.nextStep).toContain("test"); + expect(verdict).toBeNull(); }); - test("evaluates only diff-introduced surface, not pre-existing code", () => { - // A frontend-only PR: pre-existing repositories elsewhere are untouched and - // must not trip the gate, because they aren't in the diff. + test("frames the finding as unverified, not a bad query", () => { const verdict = evaluateTestPresence([ - modified("apps/app/src/routes/dashboard.tsx"), + changed("src/dal/orders.ts", "db.update(orders).set({ shipped: true });"), ]); - expect(verdict).toBeNull(); + 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 index bdcde0c..f44307c 100644 --- a/src/gate/test-presence.ts +++ b/src/gate/test-presence.ts @@ -1,66 +1,93 @@ -// The crude test-presence gate (#3496) — the MVP tracer bullet for the CI⇄MCP -// verdict loop. It is a *pure diff heuristic*: given the files a PR changed, it -// asks one question — "did this PR touch data-access code without touching a -// real-DB (repository/integration) test?" — and, if so, emits a conservative -// "we could not verify this" flag. +// 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 actually runs against Postgres; a data-access 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 instead of letting silence read as -// safety. By design it *under-fires*: it only speaks up when data-access code -// changed and no data-layer test changed alongside it. +// 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 heuristics that decide what "data-access code" and "data-layer test" - * look like. Kept as data (not hard-coded regexes) so a later per-repo config - * (#3500) can override the defaults without touching the gate logic. + * 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[]; - /** Marks a non-test path as data-access (query-emitting) code. */ - dataAccessPatterns: 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. */ - dataLayerTestPatterns: RegExp[]; + dataLayerTestPathPatterns: RegExp[]; } -// Conservative defaults, tuned to under-fire. `repository` / `dal` / -// `data-access` are the clearest data-access signals and match this project's -// own convention (`apps/api/**/*.repository.ts`). Anything narrower would never -// fire; anything broader risks reddening PRs over unrelated code. export const DEFAULT_TEST_PRESENCE_CONFIG: TestPresenceConfig = { testFilePatterns: [ - /\.(test|spec)\.[cm]?[jt]sx?$/i, // *.test.ts, *.spec.tsx, *.test.mjs, ... + /\.(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 ], - dataAccessPatterns: [ - /(^|\/)[^/]*repositor(y|ies)[^/]*\.[cm]?[jt]sx?$/i, // user.repository.ts, repositories.ts - /(^|\/)[^/]*\.repo\.[cm]?[jt]sx?$/i, // user.repo.ts - /(^|\/)(dal|data-access)\//i, // src/dal/**, src/data-access/** + 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, ], - dataLayerTestPatterns: [ - /repositor(y|ies)/i, // *.repository.spec.ts + dataLayerTestPathPatterns: [ + /repositor(y|ies)/i, /\.repo\./i, - /integration/i, // *.integration.test.ts + /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. + * 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"; @@ -74,31 +101,98 @@ function isTestFile(path: string, config: TestPresenceConfig): boolean { return matchesAny(path, config.testFilePatterns); } -function isDataAccessFile(path: string, config: TestPresenceConfig): boolean { - return ( - !isTestFile(path, config) && matchesAny(path, config.dataAccessPatterns) - ); +/** 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); } -function isDataLayerTest(path: string, config: TestPresenceConfig): boolean { +/** 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(path, config) && matchesAny(path, config.dataLayerTestPatterns) + 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 data-access files the PR added or changed. */ + /** Non-test files whose diff added/altered query code. */ dataAccessChanged: string[]; /** Real-DB data-layer tests the PR added or changed. */ dataLayerTestChanged: string[]; } -/** - * Bucket a PR's changed files into the two surfaces the gate cares about. A - * repository *test* (`user.repository.spec.ts`) counts as a data-layer test, - * not as data-access code — so changing a repository and its test together - * satisfies the gate, while changing the repository alone does not. - */ export function classifyChangedFiles( files: ChangedFile[], config: TestPresenceConfig = DEFAULT_TEST_PRESENCE_CONFIG, @@ -107,9 +201,9 @@ export function classifyChangedFiles( const dataLayerTestChanged: string[] = []; for (const file of files) { if (!isChanged(file.status)) continue; - if (isDataLayerTest(file.path, config)) { - dataLayerTestChanged.push(file.path); - } else if (isDataAccessFile(file.path, config)) { + if (isTestFile(file.path, config)) { + if (isDataLayerTest(file, config)) dataLayerTestChanged.push(file.path); + } else if (changedQueryCode(file, config)) { dataAccessChanged.push(file.path); } } @@ -128,30 +222,30 @@ export interface TestPresenceVerdict { reason: string; nextStep: string; triageHint: string; - /** The changed data-access files that triggered the flag. */ + /** The changed data-access files with no related data-layer test — what to cover. */ dataAccessFiles: string[]; } const REASON = - "This PR changes data-access code but no real-DB (repository/integration) test " + - "changed alongside it, so Query Doctor could not verify the queries it " + - "introduces — nothing here exercises them against Postgres. This is flagged " + + "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 a repository/integration test that exercises the changed data-access " + - "code 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."; + "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 data-access change intentionally needs no test, note why on the PR so " + - "the exception is auditable rather than silent."; + "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 when the PR trips the flag, or `null` - * when it passes — the two passing cases being "no data-access change" and - * "data-access change with a data-layer test alongside it". + * 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[], @@ -161,15 +255,16 @@ export function evaluateTestPresence( files, config, ); - if (dataAccessChanged.length === 0 || dataLayerTestChanged.length > 0) { - return null; - } + 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: dataAccessChanged, + dataAccessFiles: untested, }; } diff --git a/src/main.ts b/src/main.ts index 67a99fb..13c38bf 100644 --- a/src/main.ts +++ b/src/main.ts @@ -224,15 +224,16 @@ async function runInCI( // Generate PR comment with comparison data await runner.report(reportContext); - // The test-presence gate is a coverage-gap condition, not a regression, so it - // fails the check on its own — independent of the comparison block below. - // Framed as "unverified", never as "your query is bad". + // 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.setFailed( + core.warning( `${verdict.reason}\n\nNext step: ${verdict.nextStep}\n\n` + - `Changed data-access files with no data-layer test:\n${files}`, + `Changed data-access files with no related data-layer test:\n${files}`, ); } diff --git a/src/reporters/github/github.test.ts b/src/reporters/github/github.test.ts index ffb0674..f5fc760 100644 --- a/src/reporters/github/github.test.ts +++ b/src/reporters/github/github.test.ts @@ -813,7 +813,7 @@ describe("test-presence verdict rendering", () => { const output = renderTemplate(makeContext({ testPresenceVerdict: verdict })); expect(output).toContain("[!WARNING]"); expect(output).toContain( - "Unverified — this PR changes data-access code with no data-layer test.", + "Unverified — this PR changes queries with no related data-layer test.", ); expect(output).toContain(verdict.reason); expect(output).toContain(verdict.nextStep); @@ -823,6 +823,6 @@ describe("test-presence verdict rendering", () => { test("omits the banner entirely when there is no verdict", () => { const output = renderTemplate(makeContext()); - expect(output).not.toContain("Unverified — this PR changes data-access code"); + 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 09fe6b9..cb2ab8b 100644 --- a/src/reporters/github/success.md.j2 +++ b/src/reporters/github/success.md.j2 @@ -22,12 +22,12 @@ {% endif %} {% if testPresenceVerdict %} > [!WARNING] -> **Unverified — this PR changes data-access code with no data-layer test.** +> **Unverified — this PR changes queries with no related data-layer test.** > {{ testPresenceVerdict.reason }} > > **Next step:** {{ testPresenceVerdict.nextStep }} > -> Changed data-access files with no data-layer test: +> Changed data-access files with no related data-layer test: {% for f in testPresenceVerdict.dataAccessFiles %} > - `{{ f }}` {% endfor %}