Skip to content

Commit ff43099

Browse files
izaitsevfbIvan Zaitsev
andauthored
[drci] Re-render advisor comment until the verdict concludes (alt sentinel) (#8235)
## Problem The inline AI advisor verdict line (#8202) has two render states under each new/unclassified failure: - **in-progress** — a bare `AI verdict:` line + badge image, and - **concluded** — the line wrapped in `<details>` with the reasoning. The badge **image** flips server-side the moment the verdict lands (the `<img>` points at `/api/drci/advisorBadge`, which camo refetches). But the concluded `<details>` **expand is comment-body text** — it only appears when the Dr.CI comment is re-rendered. The 15-min cron (`updateDrciComments`) only re-renders a PR that has workflow activity in the last `NUM_MINUTES` (30) **or** whose comment still shows `\d Pending` jobs. A verdict can land seconds *after* the last render and the PR's CI then go quiet — so the comment stays stuck on the in-progress line indefinitely. The `body === comment` idempotency check is fine; it's just never reached, because the PR drops out of the candidate set. Observed live on pytorch/pytorch#188479: the verdict landed **44s after** the comment's last render, and never expanded. ## Fix Give the advisor `<img>` an `alt` that encodes the outcome: - in-progress → `alt="AI verdict: pending"` (a sentinel), - concluded → `alt="AI verdict: <label>"` (`related`, `not related`, `probably not related`, …). Then broaden the cron candidate query (`getPRsWithPendingJobInComment` → `getPRsNeedingCommentRefresh`) to **also** keep open PRs whose comment still carries the pending sentinel. They re-render every tick until the verdict concludes; the moment it does, the alt changes (no longer `pending`) and the PR **self-clears** from the set. Same open-PR + 1-month freshness guards as the existing `\d Pending` branch, so growth is bounded the same way. The query matches the **full `alt="AI verdict: pending"` attribute**, not the bare phrase: advisor summaries are HTML-escaped, so a `"` in a summary becomes `&quot;` and can never reproduce the real double-quoted attribute → no false-match. A single shared `ADVISOR_PENDING_ALT_ATTR` constant drives both the emitted line and the query so they can't drift. **Bonus:** the verdict outcome is now machine-readable text in the comment (useful for AI agents reading the comment, or humans with images off), not only inside the badge SVG. ## Notes - Forward-looking: comments **already** stuck (rendered before this change, with no `alt`) won't self-heal — they need a one-time `@pytorchbot drci` nudge or they age out via the 1-month guard. - Tests: `advisorBadge.test.ts` + `drci.test.ts` pass; `tsc --noEmit` clean; lintrunner clean. Co-authored-by: Ivan Zaitsev <izaitsevfb@meta.com>
1 parent 67f681a commit ff43099

5 files changed

Lines changed: 100 additions & 10 deletions

File tree

torchci/lib/advisor/advisorBadge.ts

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,31 @@ export function drciSignalKeyForJob(fullJobName: string): string {
1818
return `${DRCI_SIGNAL_KEY_PREFIX}${fullJobName}`;
1919
}
2020

21+
// Each advisor <img> carries an alt of the form `AI verdict: <outcome>`. This
22+
// does two jobs: (1) it encodes the verdict outcome as machine-readable text in
23+
// the comment body (an AI agent reading the comment, or a human with images
24+
// off, gets the verdict without fetching the badge SVG), and (2) the
25+
// not-yet-concluded sentinel below is what the Dr.CI cron matches on to keep
26+
// re-rendering a PR until its verdict lands -- the badge image flips
27+
// server-side via camo, but the concluded <details> expand is comment-body text
28+
// that only appears on a re-render, so an unconcluded PR must stay a candidate.
29+
export const ADVISOR_ALT_PREFIX = "AI verdict: ";
30+
// Sentinel alt for the in-progress line (no concluded verdict at render time).
31+
// No verdict label contains "pending", so a concluded line never matches this.
32+
export const ADVISOR_PENDING_ALT = `${ADVISOR_ALT_PREFIX}pending`;
33+
// The exact attribute the in-progress line emits. The cron candidate query
34+
// matches THIS (the full `alt="..."` form, not the bare phrase) so a
35+
// model-generated summary that happens to contain the words can't false-match:
36+
// summaries are HTML-escaped, so a literal `"` in one becomes `&quot;` and can
37+
// never reproduce the real double-quoted attribute.
38+
export const ADVISOR_PENDING_ALT_ATTR = `alt="${ADVISOR_PENDING_ALT}"`;
39+
40+
// The alt text for a concluded verdict line: `AI verdict: <label>`, where the
41+
// label is the same human-readable outcome shown on the badge pill.
42+
export function advisorVerdictAlt(verdict: string, confidence: number): string {
43+
return `${ADVISOR_ALT_PREFIX}${verdictBadge(verdict, confidence).label}`;
44+
}
45+
2146
export interface AdvisorBadge {
2247
label: string;
2348
// Hex fill, e.g. "#2da44e".
@@ -156,7 +181,10 @@ export function renderInProgressLine(
156181
): string {
157182
const badge = advisorBadgeUrl(hudBaseUrl, owner, repo, sha, jobName);
158183
const link = hudPrUrl(hudBaseUrl, owner, repo, prNumber, jobId);
159-
return ` AI verdict: <a href="${link}"><img src="${badge}"></a>\n`;
184+
// alt = the pending sentinel: it marks this PR for re-rendering until the
185+
// verdict lands (see ADVISOR_PENDING_ALT_ATTR) and reads as "AI verdict:
186+
// pending". Emit the shared attr constant so the cron's match stays in lockstep.
187+
return ` AI verdict: <a href="${link}"><img ${ADVISOR_PENDING_ALT_ATTR} src="${badge}"></a>\n`;
160188
}
161189

162190
// Concluded line: "AI verdict:" plain text toggles the expand; the badge links
@@ -169,17 +197,24 @@ export function renderVerdictLine(
169197
sha: string,
170198
jobName: string,
171199
jobId: number,
200+
verdict: string,
201+
confidence: number,
172202
summary: string
173203
): string {
174204
const badge = advisorBadgeUrl(hudBaseUrl, owner, repo, sha, jobName);
175205
const link = hudPrUrl(hudBaseUrl, owner, repo, prNumber, jobId);
206+
// alt encodes the concluded outcome (`AI verdict: related`, etc.) so the
207+
// verdict is machine-readable text in the comment AND no longer matches the
208+
// pending sentinel, dropping the PR from the re-render candidate set. The
209+
// label comes from our own verdictBadge map, but escape defensively anyway.
210+
const altText = _.escape(advisorVerdictAlt(verdict, confidence));
176211
// The advisor summary is model-generated from (attacker-influenceable) PR
177212
// content, so HTML-escape it before embedding in the comment: collapse
178213
// newlines (can't break the blockquote) and neutralize markup so it can't
179214
// close the <details>/<blockquote> or inject tags.
180215
const oneLine = _.escape((summary || "").replace(/\s*\n\s*/g, " ").trim());
181216
return (
182-
` <details><summary>AI verdict: <a href="${link}"><img src="${badge}"></a></summary><blockquote>\n\n` +
217+
` <details><summary>AI verdict: <a href="${link}"><img alt="${altText}" src="${badge}"></a></summary><blockquote>\n\n` +
183218
` ${oneLine}\n\n` +
184219
` <a href="${link}">Full reasoning on HUD &rarr;</a>\n` +
185220
` </blockquote></details>\n`
@@ -193,6 +228,9 @@ export interface AdvisorLineJob {
193228
name: string;
194229
}
195230
export interface AdvisorLineVerdict {
231+
// verdict + confidence drive the badge label baked into the alt text.
232+
verdict: string;
233+
confidence: number;
196234
summary: string;
197235
}
198236

@@ -225,6 +263,8 @@ export function selectAdvisorLines(
225263
headSha,
226264
job.name,
227265
job.id,
266+
verdict.verdict,
267+
verdict.confidence,
228268
verdict.summary
229269
)
230270
);

torchci/lib/advisor/advisorComment.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,11 @@ export async function buildAdvisorVerdictLines(
5959
const verdictByKey = new Map<string, AdvisorLineVerdict>();
6060
for (const v of deduplicateVerdicts(verdictRows)) {
6161
if (v.sha === headSha) {
62-
verdictByKey.set(v.signalKey, { summary: v.summary });
62+
verdictByKey.set(v.signalKey, {
63+
verdict: v.verdict,
64+
confidence: v.confidence,
65+
summary: v.summary,
66+
});
6367
}
6468
}
6569

torchci/lib/advisor/advisorDispatch.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,7 @@ const SKIP_DRAFT_PRS = true;
398398
/**
399399
* Fetch the minimal PR state needed to gate auto-dispatch, from the
400400
* default.pull_request ClickHouse mirror rather than the GitHub API. The rest
401-
* of the advisor path already reads from CH (and getPRsWithPendingJobInComment
401+
* of the advisor path already reads from CH (and getPRsNeedingCommentRefresh
402402
* in drci.ts already reads pull_request.state the same way), so this keeps the
403403
* Dr.CI cron off the GitHub rate limit. The mirror lags GitHub by ~1 minute,
404404
* well within the 15-minute cron cadence. A PR not yet mirrored (no row) is

torchci/pages/api/drci/drci.ts

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { PutObjectCommand } from "@aws-sdk/client-s3";
22
import dayjs from "dayjs";
33
import utc from "dayjs/plugin/utc";
4+
import { ADVISOR_PENDING_ALT_ATTR } from "lib/advisor/advisorBadge";
45
import { buildAdvisorVerdictLines } from "lib/advisor/advisorComment";
56
import { autoDispatchAdvisorForNewFailures } from "lib/advisor/advisorDispatch";
67
import { fetchJSON, isTime0 } from "lib/bot/utils";
@@ -158,7 +159,7 @@ export async function updateDrciComments(
158159
? []
159160
: fetchRecentWorkflows(
160161
`${owner}/${repo}`,
161-
await getPRsWithPendingJobInComment(`${owner}/${repo}`),
162+
await getPRsNeedingCommentRefresh(`${owner}/${repo}`),
162163
NUM_MINUTES + ""
163164
),
164165
]);
@@ -403,7 +404,20 @@ function removeFailureContext(failure: {
403404
* @param repo The repository to search for PRs in. E.g. "pytorch/pytorch"
404405
* @returns A list of PR numbers
405406
*/
406-
async function getPRsWithPendingJobInComment(repo: String): Promise<number[]> {
407+
// PRs whose Dr.CI comment is in a transient state that warrants a re-render
408+
// even with no recent (< NUM_MINUTES) workflow activity. Two cases:
409+
// - `\d Pending`: the comment still shows pending jobs that may resolve.
410+
// - the advisor pending sentinel: a NEW/unclassified failure was dispatched
411+
// to the AI advisor but its verdict had not landed at the last render, so
412+
// the comment carries the in-progress `alt="AI verdict: pending"` line. The
413+
// verdict can land seconds after a render and the PR's CI then go quiet,
414+
// so without this clause the concluded <details> expand would never get
415+
// written. Once the verdict renders, the alt becomes `AI verdict: <label>`
416+
// (no "pending"), so the PR self-clears from this set. We match the full
417+
// alt attribute (not the bare phrase) so an escaped model summary can't
418+
// false-match.
419+
// Both branches stay gated by the open-PR + 1-month freshness guards below.
420+
async function getPRsNeedingCommentRefresh(repo: String): Promise<number[]> {
407421
const query = `
408422
select
409423
issue_comment.issue_url
@@ -412,12 +426,15 @@ from
412426
join default.pull_request on issue_comment.issue_url = pull_request.issue_url
413427
where
414428
body like '<!-- drci-comment-start -->%'
415-
and match(body, '\\d Pending')
429+
and (match(body, '\\d Pending') or position(body, {pendingAltAttr: String}) > 0)
416430
and issue_comment.updated_at > now() - interval 1 month
417431
and issue_url like {repo: String }
418432
and pull_request.state = 'open'
419433
`;
420-
const results = await queryClickhouse(query, { repo: `%${repo}%` });
434+
const results = await queryClickhouse(query, {
435+
repo: `%${repo}%`,
436+
pendingAltAttr: ADVISOR_PENDING_ALT_ATTR,
437+
});
421438
return results.map((v) => parseInt(v.issue_url.split("/").pop()));
422439
}
423440

torchci/test/advisorBadge.test.ts

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import {
2+
ADVISOR_PENDING_ALT,
23
advisorBadgeUrl,
34
ANALYZING_BADGE,
45
confidenceBucket,
@@ -100,7 +101,7 @@ describe("advisorBadgeUrl", () => {
100101
});
101102

102103
describe("renderInProgressLine / renderVerdictLine", () => {
103-
it("in-progress: badge only, no expand", () => {
104+
it("in-progress: badge only, no expand, pending alt", () => {
104105
const line = renderInProgressLine(
105106
HUD,
106107
"pytorch",
@@ -114,6 +115,8 @@ describe("renderInProgressLine / renderVerdictLine", () => {
114115
expect(line).toContain("/api/drci/advisorBadge?");
115116
expect(line).not.toContain("<details>");
116117
expect(line).toContain("/pr/pytorch/pytorch/123#42");
118+
// the pending sentinel alt is what the cron matches to keep re-rendering
119+
expect(line).toContain(`alt="${ADVISOR_PENDING_ALT}"`);
117120
});
118121

119122
it("concluded: AI verdict text toggles a details expand with reasoning", () => {
@@ -125,6 +128,8 @@ describe("renderInProgressLine / renderVerdictLine", () => {
125128
"abc",
126129
"trunk / x / test",
127130
42,
131+
"related",
132+
0.95,
128133
"Line one.\n Line two."
129134
);
130135
expect(line).toContain("<details><summary>AI verdict:");
@@ -133,6 +138,25 @@ describe("renderInProgressLine / renderVerdictLine", () => {
133138
// multi-line summary collapsed to one line inside the blockquote
134139
expect(line).toContain("Line one. Line two.");
135140
expect(line).not.toContain("Line one.\n");
141+
// alt encodes the concluded outcome and is NOT the pending sentinel
142+
expect(line).toContain('alt="AI verdict: related"');
143+
expect(line).not.toContain(ADVISOR_PENDING_ALT);
144+
});
145+
146+
it("concluded alt carries the confidence-bucketed label", () => {
147+
const line = renderVerdictLine(
148+
HUD,
149+
"pytorch",
150+
"pytorch",
151+
123,
152+
"abc",
153+
"trunk / x / test",
154+
42,
155+
"not_related",
156+
0.8,
157+
"summary"
158+
);
159+
expect(line).toContain('alt="AI verdict: probably not related"');
136160
});
137161

138162
it("escapes HTML in the summary so it can't break out of the expand", () => {
@@ -144,6 +168,8 @@ describe("renderInProgressLine / renderVerdictLine", () => {
144168
"abc",
145169
"trunk / x / test",
146170
42,
171+
"related",
172+
0.95,
147173
"</blockquote></details><img src=x onerror=alert(1)>"
148174
);
149175
expect(line).not.toContain("</blockquote></details><img");
@@ -163,7 +189,10 @@ describe("selectAdvisorLines", () => {
163189

164190
it("prefers a finalized verdict, else in-progress, else nothing", () => {
165191
const verdictByKey = new Map([
166-
[drciSignalKeyForJob("trunk / a / test"), { summary: "done" }],
192+
[
193+
drciSignalKeyForJob("trunk / a / test"),
194+
{ verdict: "related", confidence: 0.95, summary: "done" },
195+
],
167196
]);
168197
const inProgress = new Set([drciSignalKeyForJob("trunk / b / test")]);
169198

0 commit comments

Comments
 (0)