Skip to content

feat(gate): explain why a file was flagged - #220

Merged
veksen merged 2 commits into
mainfrom
feat-gate-evidence
Aug 5, 2026
Merged

feat(gate): explain why a file was flagged#220
veksen merged 2 commits into
mainfrom
feat-gate-evidence

Conversation

@veksen

@veksen veksen commented Aug 5, 2026

Copy link
Copy Markdown
Member

Goal

Make the untested-data-access gate explain itself, so a false positive can be judged from the PR comment instead of by cloning the analyzer.

The gate has taken six precision fixes in four weeks (Query-Doctor/Site#3531, #3550, #3615, #3650, plus two cases nobody filed). Every one was diagnosed the same way: check out the analyzer, write a throwaway script, replay the PR's changed-file list, print which pattern matched what text. The module had that information and discarded it before anyone could see it.

An audit of all 15 PRs the gate has ever fired on, across Site, Nutcracker, this repo and veksen-enterprises/d2armory, found 2 correct flags. One false positive is still open: apps/api/src/db/test-db.ts on Query-Doctor/Site#3530 and #3573.

What

Before, the PR comment listed paths:

  • apps/api/src/db/test-db.ts

After, it names the cause:

  • apps/api/src/db/test-db.ts — matched drizzle-init on line 2: drizzle(

That line is enough to see the flag is the testcontainers harness being read as production data access. It is the open false positive above, diagnosed without leaving the PR.

How

Read src/gate/test-presence.ts first, then the two rendering sites.

The 20 anonymous regexes in the content matcher become named rules: raw-select-from, drizzle-sql-tag, ddl-create-index, and so on. findQueryCode returns the first that matches, with the line and an excerpt. patchAddsQueryCode stays as the boolean form, defined in terms of it. Rule order is precedence, so a line matching both .execute( and the raw select shape reports the first, which names the cause better.

Line numbers required a change to the three strippers. They removed comment, import and block-comment text, which shifted every later line, so an offset into the stripped text pointed at the wrong place. They now replace that text with spaces and keep the newlines. This is the one behavioural risk in the change: blanking leaves whitespace where deletion closed the gap, and the raw select shape has a 300-character budget between select and from, so blanking is marginally more conservative. I checked that against the corpus below.

dataAccessFiles becomes {path, evidence} rather than strings. Evidence is absent when GitHub supplied no patch, which happens for large and binary files: the filename prior decides those alone, and the line renders as a bare path.

The check annotation in main.ts renders separately from the template rather than sharing a formatter. That predates this change and I left it alone.

Tests

Six new cases in src/gate/test-presence.test.ts, each written before its implementation and observed failing:

  • rule name and line number for a match
  • precedence when several rules match
  • correct line number when comment and import lines precede the match, which is what the blanking change buys
  • null when nothing matches
  • the verdict carries evidence through to what a reader sees
  • evidence omitted when the path prior decided it

Full suite 429/429. npm run typecheck clean.

Beyond the unit tests, I replayed 42 real PRs from Site, Nutcracker, this repo and d2armory through classifyChangedFiles. The set of files classified as data access is identical before and after, so the stripper change altered no outcome. I also rendered the comment for Site#3530 and #3573 through the reporter's own nunjucks config, with a patchless file appended so both branches of the new loop render.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query Doctor — 6 successful checks

 Cost regression — No query went up more than 5%
 Untested data access — No changed data-access file without a test
 New query — No new queries
 New query with index recommendation — No new query ships an index recommendation
 Schema drift — No schema changes
 High-value nudge — No index or rewrite past the threshold


More details via MCP → get_ci_run({ runId: "019fd2a1-cab4-7c41-aefd-a6fae4da71ff" }) · view run · docs
3 queries read against main on assumed statistics of 10,000,000 rows per table. Sync production stats for costs measured against your real data.

@veksen
veksen force-pushed the refact-gate-drop-config-seam branch from 99771cb to bb8a796 Compare August 5, 2026 15:37
@veksen
veksen force-pushed the feat-gate-evidence branch from f128c15 to 33857b2 Compare August 5, 2026 15:37
@veksen
veksen force-pushed the refact-gate-drop-config-seam branch from bb8a796 to 916551d Compare August 5, 2026 15:47
@veksen
veksen force-pushed the feat-gate-evidence branch from 33857b2 to c3868d8 Compare August 5, 2026 15:47
@veksen
veksen changed the base branch from refact-gate-drop-config-seam to main August 5, 2026 15:52
veksen added 2 commits August 5, 2026 12:52
The gate decided but could not explain. Six precision fixes in four weeks were
each diagnosed the same way: clone the analyzer, write a throwaway script,
replay the PR's changed-file list, and print which pattern matched what text.
The module had that information and dropped it on the way out.

Give every content pattern a stable name and return the match. findQueryCode
reports the rule, the line within the added lines, and an excerpt of the text.
patchAddsQueryCode stays as the boolean form, now defined in terms of it.

Rule order is precedence, so a line matching both `.execute(` and the raw
select shape reports the first, which names the cause better.

Line numbers required a change to the three strippers. They removed comment,
import and block-comment text, which shifted every later line, so an offset in
the stripped text pointed at the wrong place. They now replace that text with
spaces and keep the newlines, leaving positions intact.

No behaviour change. The full suite passes, and replaying 42 PRs from Site,
Nutcracker, this repo and d2armory classifies the same files as before.

Two of those PRs now explain themselves. The unresolved false positive on
apps/api/src/db/test-db.ts reports rule `drizzle-init` at line 2 on the text
`drizzle(`, which is the testcontainers harness being read as production data
access. The one true positive in the corpus, project-queries.repository.ts,
reports `drizzle-sql-tag` at line 35.
The gate now knows which rule matched and where, but kept it to itself: the
verdict carried a list of paths, so a reader still had to clone the analyzer to
find out why their file was flagged.

Carry the evidence through. `dataAccessFiles` becomes a list of `{path,
evidence}` rather than strings, and both renderings use it. The PR comment for
Site#3530 goes from

  - `apps/api/src/db/test-db.ts`

to

  - `apps/api/src/db/test-db.ts` — matched `drizzle-init` on line 2: `drizzle(`

which is enough to see that the flag is the testcontainers harness being read
as production data access, without leaving the PR.

Evidence is absent when GitHub supplied no patch, which happens for large and
binary files. The filename prior decides those alone, there is no matched text
to point at, and the line renders as a bare path. A test covers each branch.

The check annotation built in main.ts gets the same treatment. It renders
separately from the template, which is its own problem, but not this change's.

Full suite 429/429, typecheck clean. Verified by rendering the real comment for
Site#3530 and #3573 through the reporter's own nunjucks config.
@veksen
veksen force-pushed the feat-gate-evidence branch from c3868d8 to 5b8eff7 Compare August 5, 2026 15:53
@veksen
veksen merged commit 793be9e into main Aug 5, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant