Add an AI_NOT_RELATED bucket for advisor-cleared failures

torchci/lib/advisor/advisorComment.ts

@@ -10,59 +10,52 @@ import {
   AdvisorLineVerdict,
   selectAdvisorLines,
 } from "lib/advisor/advisorBadge";
-import { isAdvisorEnabled } from "lib/advisor/advisorConfig";
 import {
   readDispatchStates,
   signalKeyForJob,
 } from "lib/advisor/advisorDispatch";
+import { advisorCommentEnabled } from "lib/advisor/advisorFlags";
 import {
   AdvisorVerdictRow,
-  deduplicateVerdicts,
+  headRowsBySignalKey,
+  resolveVerdict,
 } from "lib/advisorVerdictUtils";
-import { queryClickhouseSaved } from "lib/clickhouse";
 import { RecentWorkflowsData } from "lib/types";
 
-// Gate the inline verdict rendering behind its own flag so it ships dark and
-// can be enabled per deployment (Vercel env var), independently of the
-// auto-dispatch flag. Display-only, so it doesn't also require VERCEL_ENV
-// (unlike auto-dispatch, which fires real workflow_dispatches).
-export function advisorCommentEnabled(owner: string, repo: string): boolean {
-  return (
-    process.env.DRCI_ADVISOR_COMMENT_ENABLED === "true" &&
-    isAdvisorEnabled(owner, repo)
-  );
-}
-
 /**
  * Build the per-job "AI verdict:" line for a PR's new/unclassified failures.
  * Returns job.id -> rendered HTML (empty map when the comment flag is off, the
  * repo isn't advisor-enabled, or there are no jobs). The caller wraps this so a
  * ClickHouse error can never break the Dr.CI comment.
+ *
+ * Takes the PR's verdict rows rather than reading them, so this line and the
+ * suppression gate describe the same rows resolved the same way. A signal key
+ * whose newest rows disagree resolves to nothing here too, and the job falls
+ * through to the pending/in-progress treatment below -- an unusable answer
+ * should not render as a confident badge.
  */
 export async function buildAdvisorVerdictLines(
   hudBaseUrl: string,
   owner: string,
   repo: string,
   prNumber: number,
   headSha: string,
-  jobs: RecentWorkflowsData[]
+  jobs: RecentWorkflowsData[],
+  verdictRows: AdvisorVerdictRow[]
 ): Promise<Map<number, string>> {
   if (!advisorCommentEnabled(owner, repo) || jobs.length === 0) {
     return new Map();
   }
 
   // Finalized verdicts for this PR, keyed by signal_key for the head commit.
-  const verdictRows = (await queryClickhouseSaved("advisor_verdicts_for_pr", {
-    repo: `${owner}/${repo}`,
-    prNumber,
-  })) as AdvisorVerdictRow[];
   const verdictByKey = new Map<string, AdvisorLineVerdict>();
-  for (const v of deduplicateVerdicts(verdictRows)) {
-    if (v.sha === headSha) {
-      verdictByKey.set(v.signalKey, {
-        verdict: v.verdict,
-        confidence: v.confidence,
-        summary: v.summary,
+  for (const [signalKey, rows] of headRowsBySignalKey(verdictRows, headSha)) {
+    const resolved = resolveVerdict(rows);
+    if (resolved !== null) {
+      verdictByKey.set(signalKey, {
+        verdict: resolved.verdict,
+        confidence: resolved.confidence,
+        summary: resolved.summary,
       });
     }
   }

torchci/lib/advisor/advisorFlags.ts

@@ -0,0 +1,42 @@
+// Deployment flags for the two Dr.CI advisor consumers.
+//
+// Separate from advisorConfig.ts because that module is pure data with no
+// server-only behavior and is imported by React components; these read
+// process.env, which a client caller would see as unset. Separate from the
+// consumers themselves so that deciding whether to read verdicts at all costs
+// no import of the modules that consume them -- advisorComment reaches the AWS
+// SDK through its dispatch-state dependency.
+
+import { isAdvisorEnabled } from "lib/advisor/advisorConfig";
+
+// Render the inline "AI verdict:" line in the Dr.CI comment. Its own flag so it
+// ships dark and can be enabled per deployment (Vercel env var), independently
+// of the auto-dispatch flag. Display-only, so it doesn't also require
+// VERCEL_ENV (unlike auto-dispatch, which fires real workflow_dispatches).
+export function advisorCommentEnabled(owner: string, repo: string): boolean {
+  return (
+    process.env.DRCI_ADVISOR_COMMENT_ENABLED === "true" &&
+    isAdvisorEnabled(owner, repo)
+  );
+}
+
+// Move advisor-cleared failures out of the blocking set. Kept separate from the
+// display flag above because this one decides whether a merge is allowed, not
+// just what the comment says, so the display half can be enabled on its own
+// while the merge gate stays off.
+//
+// REQUIRES the comment flag, and is inert without it. Suppression with the
+// comment off would move a job into the non-blocking section while
+// buildAdvisorVerdictLines returns nothing, so the reader is told the AI
+// cleared the job but never which verdict cleared it or why. That combination
+// has no use -- a merge gate the comment cannot account for -- so it is
+// unreachable by construction rather than left to deployment discipline.
+export function advisorSuppressionEnabled(
+  owner: string,
+  repo: string
+): boolean {
+  return (
+    process.env.DRCI_ADVISOR_SUPPRESSION_ENABLED === "true" &&
+    advisorCommentEnabled(owner, repo)
+  );
+}

torchci/lib/advisor/advisorSuppression.ts

@@ -0,0 +1,199 @@
+// Decides which NEW/unclassified failures the AI CI Advisor has cleared well
+// enough to stop blocking a merge.
+//
+// Deliberately separate from advisorComment.ts. That module renders a display
+// line and is gated by a display flag; this one moves a failure out of the
+// blocking set, so it gets its own flag and a strictly narrower predicate.
+// Everything here is fail-safe: any missing, stale, ambiguous or low-confidence
+// verdict leaves the job blocking.
+
+import dayjs from "dayjs";
+import utc from "dayjs/plugin/utc";
+import {
+  confidenceBucket,
+  drciSignalKeyForJob,
+} from "lib/advisor/advisorBadge";
+import { advisorSuppressionEnabled } from "lib/advisor/advisorFlags";
+import {
+  AdvisorVerdictRow,
+  AdvisorVerdictType,
+  headRowsBySignalKey,
+  resolveVerdict,
+} from "lib/advisorVerdictUtils";
+import { isTime0 } from "lib/bot/utils";
+import { RecentWorkflowsData } from "lib/types";
+
+dayjs.extend(utc);
+
+// Every advisor verdict and whether it stops a failure from blocking.
+//
+// Written as a TOTAL map rather than a list of the cleared ones: adding a member
+// to AdvisorVerdictType then fails to compile here until someone decides which
+// side it falls on, which is the decision most likely to be skipped.
+//
+//   not_related  the failure exists independently of this change
+//   infra_issue  the environment broke -- credentials, image pull, runner loss
+//   garbage      the signal itself is noise: it flips red/green across unrelated
+//                commits, and the baselines fail the same way
+//   related      the opposite claim -- this change caused it
+//   revert       the trunk-side spelling of `related`
+//   unsure       no claim was reached
+//
+// `garbage` sits with the cleared ones rather than with `unsure` because it is
+// an EVIDENCED claim about the signal, not an absence of one: the advisor
+// reaches it by comparing the job against its own baselines. `unsure` is the
+// real "cannot tell", and it blocks.
+//
+// LIMIT, stated because the confidence and freshness gates do not cover it: a
+// verdict says the failure looks environmental, not that the PR is innocent of
+// causing it. A change to CI config, a Dockerfile, or a submodule pin can
+// produce a genuine infrastructure-shaped failure that IS the PR's fault, and
+// `infra_issue` is where that lands. `producedATestOutcome` below narrows this
+// -- it gates on the job's own conclusion rather than the advisor's opinion, so
+// an infra fault that left the job `cancelled` never reaches here -- but it does
+// not close it, since a job can conclude `failure` having tested nothing.
+// Nothing on this side bounds how many such jobs one merge may skip; a
+// per-merge cap is proposed in pytorch/pytorch#195503. Unless such a cap is
+// deployed, the flag being off is what holds this residue.
+const VERDICT_DISPOSITION: Record<AdvisorVerdictType, "clear" | "block"> = {
+  not_related: "clear",
+  infra_issue: "clear",
+  garbage: "clear",
+  related: "block",
+  revert: "block",
+  unsure: "block",
+};
+
+export const SUPPRESSIBLE_VERDICTS: ReadonlySet<string> = new Set(
+  Object.entries(VERDICT_DISPOSITION)
+    .filter(([, disposition]) => disposition === "clear")
+    .map(([verdict]) => verdict)
+);
+
+// Clear only what the badge scale calls high confidence. Asks advisorBadge for
+// the bucket rather than restating its threshold, so retuning the scale moves
+// the gate with it.
+//
+// That keeps badge and gate in step for `not_related` -- the only cleared
+// verdict whose label is hedged by confidence, so it is the only one that could
+// read "probably not related" while this suppressed. `verdictBadge` returns for
+// `garbage` and `infra_issue` before it consults the bucket, so those labels
+// never hedge: a sub-threshold `infra_issue` shows a flat "infra issue" beside a
+// job that still blocks, and about 6% of `infra_issue` rows in a recent 30-day
+// sample sit below the bar. Fixing that means changing labels in advisorBadge.ts,
+// which is on the already-live comment path -- a follow-up, not this change.
+export function confidentEnoughToSuppress(confidence: number): boolean {
+  return confidenceBucket(confidence) === "high";
+}
+
+// A verdict describes one execution of a job, but is keyed only by (sha, job
+// name), so a rerun at the same head would otherwise inherit the previous run's
+// verdict. Requiring the verdict to be strictly newer than the job's completion
+// rejects the common case: the rerun finishes after the old verdict was
+// written, so the job blocks until a fresh analysis lands. Equality is treated
+// as stale because these timestamps are truncated and a tie cannot be told
+// apart from the unsafe ordering.
+//
+// KNOWN GAP, and the reason this ships behind a flag: a verdict for execution A
+// that lands after execution B has finished still passes this test. Closing it
+// needs the analyzed job id recorded on the verdict row -- the row's `run_id`
+// is the advisor's own dispatch run, not the job's.
+function verdictDescribesThisRun(
+  job: RecentWorkflowsData,
+  verdictTimestamp: string
+): boolean {
+  // isTime0 covers the epoch sentinel AND unparseable input (it NaN-checks), so
+  // a malformed timestamp on either side blocks rather than passing.
+  if (isTime0(job.completed_at) || isTime0(verdictTimestamp)) {
+    return false;
+  }
+  return dayjs.utc(verdictTimestamp).isAfter(dayjs.utc(job.completed_at));
+}
+
+// Only a conclusive `failure` is eligible. `cancelled`, `timed_out`,
+// `action_required` and friends did not produce a test outcome, and the advisor
+// labels much of that class `not_related` anyway -- see the shadow-mode
+// adjudication. This narrows that exposure; it does not close it, because a
+// job can also fail without a real outcome (runner lost, driver fault) while
+// still concluding `failure`. That residue is what the manual adjudication
+// before enforcement is for.
+export function producedATestOutcome(job: RecentWorkflowsData): boolean {
+  return job.conclusion === "failure";
+}
+
+/** Whether one job's verdict clears it to stop blocking. Pure, for testing. */
+export function isSuppressible(
+  job: RecentWorkflowsData,
+  rows: AdvisorVerdictRow[]
+): boolean {
+  if (!producedATestOutcome(job)) {
+    return false;
+  }
+  const resolved = resolveVerdict(rows);
+  if (resolved === null) {
+    return false;
+  }
+  return (
+    SUPPRESSIBLE_VERDICTS.has(resolved.verdict) &&
+    confidentEnoughToSuppress(resolved.confidence) &&
+    verdictDescribesThisRun(job, resolved.timestamp)
+  );
+}
+
+/**
+ * Job ids among `jobs` that the advisor has cleared. Returns an empty set when
+ * the flag is off, so the caller needs no separate check.
+ *
+ * Takes the PR's verdict rows rather than reading them: drci.ts reads once and
+ * shares them with the badge line, so the two cannot resolve the same job to
+ * different verdicts. They can still reach different DISPOSITIONS -- the gate
+ * adds the confidence, freshness and conclusion tests the badge does not, so a
+ * confident-looking badge beside a still-blocking job is expected. Rows for any
+ * other commit are dropped here, so a verdict from an earlier head can never
+ * clear a job at this one.
+ */
+export function suppressibleJobIds(
+  owner: string,
+  repo: string,
+  headSha: string,
+  jobs: RecentWorkflowsData[],
+  verdictRows: AdvisorVerdictRow[]
+): Set<number> {
+  if (!advisorSuppressionEnabled(owner, repo) || jobs.length === 0) {
+    return new Set();
+  }
+
+  const rowsByKey = headRowsBySignalKey(verdictRows, headSha);
+
+  const suppressible = new Set<number>();
+  for (const job of jobs) {
+    if (!job.name) {
+      continue;
+    }
+    const rows = rowsByKey.get(drciSignalKeyForJob(job.name)) ?? [];
+    if (isSuppressible(job, rows)) {
+      suppressible.add(job.id);
+    }
+  }
+  return suppressible;
+}
+
+/**
+ * Move cleared jobs out of `blocking` and return them, mutating the array in
+ * place. In place because drci.ts hands the same array object to both the
+ * failures dict and the comment renderer, and CRCR L4 pushes into it later --
+ * rebinding would silently desync those.
+ */
+export function extractSuppressed(
+  blocking: RecentWorkflowsData[],
+  suppressibleIds: Set<number>
+): RecentWorkflowsData[] {
+  const extracted: RecentWorkflowsData[] = [];
+  for (let i = blocking.length - 1; i >= 0; i--) {
+    if (suppressibleIds.has(blocking[i].id)) {
+      extracted.unshift(blocking[i]);
+      blocking.splice(i, 1);
+    }
+  }
+  return extracted;
+}

torchci/lib/advisor/advisorVerdictSource.ts

@@ -0,0 +1,46 @@
+// The single read of a PR's advisor verdicts.
+//
+// Both consumers in the Dr.CI request -- the inline badge line and the
+// suppression gate -- need the same rows for the same PR and head. Reading
+// twice cost a ClickHouse round trip per PR and, worse, let a verdict landing
+// between the two reads make the comment and the merge gate describe the same
+// job differently. drci.ts reads once here and hands the rows to both.
+
+import {
+  advisorCommentEnabled,
+  advisorSuppressionEnabled,
+} from "lib/advisor/advisorFlags";
+import { AdvisorVerdictRow } from "lib/advisorVerdictUtils";
+import { queryClickhouseSaved } from "lib/clickhouse";
+import { RecentWorkflowsData } from "lib/types";
+
+/**
+ * Whether this PR is worth a verdict read at all.
+ *
+ * Both consumers bail on an empty job list, so a PR with no new or
+ * unclassified failures -- the common case -- must not cost a query. Each
+ * consumer re-checks its own flag; this only decides whether to read.
+ */
+export function shouldReadAdvisorVerdicts(
+  owner: string,
+  repo: string,
+  jobs: RecentWorkflowsData[]
+): boolean {
+  if (jobs.length === 0) {
+    return false;
+  }
+  return (
+    advisorCommentEnabled(owner, repo) || advisorSuppressionEnabled(owner, repo)
+  );
+}
+
+export async function fetchAdvisorVerdictRows(
+  owner: string,
+  repo: string,
+  prNumber: number
+): Promise<AdvisorVerdictRow[]> {
+  return (await queryClickhouseSaved("advisor_verdicts_for_pr", {
+    repo: `${owner}/${repo}`,
+    prNumber,
+  })) as AdvisorVerdictRow[];
+}