Skip to content

Commit 3417f41

Browse files
authored
Dr. CI: only treat pytorch-bot's own comment as the Dr. CI comment (#8163)
## Problem Dr. CI keeps a single status comment per PR and finds "its" comment purely by scanning for the literal `<!-- drci-comment-start -->` marker, with **no author check**. Two selectors do this and neither filters by author: - `getDrciComment()` (probot/webhook path, `lib/drciUtils.ts`) — `listComments`, returns the first comment whose body includes the marker. - `getExistingDrCiComments()` (15-minute updater, `pages/api/drci/drci.ts`) — ClickHouse `issue_comment` `body like '%<!-- drci-comment-start -->%'`, with no `ORDER BY`, fed into `new Map()` (last row wins on duplicates). If any other comment on the PR embeds that marker (e.g. a bot/tool that quotes or forwards the Dr. CI status block), Dr. CI mistakes it for its own and overwrites it in place. Observed on #186611: a comment authored under a non-bot account embedded the marker and was repeatedly overwritten with Dr. CI's render, while the real `pytorch-bot[bot]` comment went stale. ## Fix Require the comment author to be `pytorch-bot[bot]` in both selectors. Dr. CI always creates/updates its comment through the pytorch-bot GitHub App installation, so its author login is always `pytorch-bot[bot]` — in the API handler the user token is only used for rate-limit identity; the actual mutation uses `getOctokit(org, repo)` (the app installation). So the guard doesn't affect the normal create/update flow. Also adds `order by id desc` to the ClickHouse query so selection stays deterministic (keeps the oldest bot comment, matching the chronologically-first match `getDrciComment` returns) in the unlikely event multiple bot-authored marker comments ever exist. ## Test plan - New regression test in `drciBot.test.ts`: a marker-bearing comment from a non-bot author is ignored and Dr. CI posts its own comment instead. - Updated the existing `drciBot.test.ts` fixtures to set the comment author to `pytorch-bot[bot]`. - `yarn jest test/drciBot.test.ts test/drci.test.ts` → both suites pass. - Verified the updated ClickHouse query against prod for #186611: without the author filter it returns 2 marker comments (`pytorch-bot[bot]` + the impostor); with the filter it returns exactly the `pytorch-bot[bot]` comment.
1 parent a30fc53 commit 3417f41

3 files changed

Lines changed: 66 additions & 1 deletion

File tree

torchci/lib/drciUtils.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,12 @@ export const NUM_MINUTES = 30;
2626
export const REPO: string = "pytorch";
2727
export const OWNER: string = "pytorch";
2828
export const DRCI_COMMENT_START = "<!-- drci-comment-start -->\n";
29+
// Dr. CI's comment is always created/updated through the pytorch-bot GitHub App
30+
// installation, so its author login is always this. Other tools (e.g. internal
31+
// diff-handoff bots) sometimes embed the DRCI_COMMENT_START marker inside their
32+
// own comments; without this author check Dr. CI mistakes such a comment for its
33+
// own and overwrites it. So match on the marker AND the author.
34+
export const DRCI_COMMENT_AUTHOR = "pytorch-bot[bot]";
2935
export const DOCS_URL = "https://docs-preview.pytorch.org";
3036
export const PYTHON_DOCS_PATH = "index.html";
3137
export const CPP_DOCS_PATH = "cppdocs/index.html";
@@ -121,7 +127,10 @@ export async function getDrciComment(
121127
issue_number: prNum,
122128
});
123129
for (const comment of commentsRes.data) {
124-
if (comment.body!.includes(DRCI_COMMENT_START)) {
130+
if (
131+
comment.user?.login === DRCI_COMMENT_AUTHOR &&
132+
comment.body!.includes(DRCI_COMMENT_START)
133+
) {
125134
return { id: comment.id, body: comment.body! };
126135
}
127136
}

torchci/pages/api/drci/drci.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { fetchJSON, isTime0 } from "lib/bot/utils";
55
import { queryClickhouse, queryClickhouseSaved } from "lib/clickhouse";
66
import {
77
CANCELLED_STEP_ERROR,
8+
DRCI_COMMENT_AUTHOR,
89
fetchPRLabels,
910
FLAKY_RULES_JSON,
1011
formDrciComment,
@@ -556,11 +557,18 @@ from
556557
default.issue_comment final
557558
where
558559
body like '%<!-- drci-comment-start -->%'
560+
and user.login = {drciCommentAuthor: String}
559561
and issue_url in {prUrls: Array(String)}
562+
-- Order so the oldest comment lands last; the Map below keeps the last entry
563+
-- per PR, so we deterministically pick the original Dr. CI comment if there
564+
-- ever are multiple bot-authored marker comments (matches getDrciComment,
565+
-- which returns the first match from chronologically-ordered listComments).
566+
order by id desc
560567
`;
561568
return new Map(
562569
(
563570
await queryClickhouse(existingCommentsQuery, {
571+
drciCommentAuthor: DRCI_COMMENT_AUTHOR,
564572
prUrls: Array.from(workflowsByPR.keys()).map(
565573
(prNumber) =>
566574
`https://api.github.com/repos/${repoFullName}/issues/${prNumber}`

torchci/test/drciBot.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ describe("verify-drci-functionality", () => {
101101
{
102102
id: comment_id,
103103
node_id: comment_node_id,
104+
user: { login: drciUtils.DRCI_COMMENT_AUTHOR },
104105
body: "<!-- drci-comment-start -->\nhello\n<!-- drci-comment-end -->\n",
105106
},
106107
])
@@ -124,6 +125,52 @@ describe("verify-drci-functionality", () => {
124125
handleScope(scope);
125126
});
126127

128+
test("Dr. CI ignores a marker-bearing comment from a non-bot author and posts its own", async () => {
129+
// Regression test: another tool (e.g. an internal diff-handoff bot) can post
130+
// a comment that embeds the DRCI_COMMENT_START marker. Dr. CI must not treat
131+
// that comment as its own and overwrite it; it should post a fresh comment.
132+
nock("https://api.github.com")
133+
.post("/app/installations/2/access_tokens")
134+
.reply(200, { token: "test" });
135+
136+
const payload = require("./fixtures/pull_request.opened")["payload"];
137+
payload["pull_request"]["user"]["login"] = some_user;
138+
payload["repository"]["owner"]["login"] = OWNER;
139+
payload["repository"]["name"] = REPO;
140+
141+
jest
142+
.spyOn(clickhouse, "queryClickhouse")
143+
.mockImplementation((query, params) => {
144+
return Promise.resolve([]);
145+
});
146+
147+
const scope = nock("https://api.github.com")
148+
.get(`/repos/${OWNER}/${REPO}/issues/31/comments`, (body) => {
149+
return true;
150+
})
151+
.reply(200, [
152+
{
153+
id: comment_id,
154+
node_id: comment_node_id,
155+
user: { login: some_user },
156+
body: "<!-- drci-comment-start -->\nhandoff\n<!-- drci-comment-end -->\n",
157+
},
158+
])
159+
// Must create a new comment rather than patch the impostor one.
160+
.post(`/repos/${OWNER}/${REPO}/issues/31/comments`, (body) => {
161+
const comment = body.body;
162+
expect(comment.includes(drciUtils.DRCI_COMMENT_START)).toBeTruthy();
163+
expect(
164+
comment.includes("See artifacts and rendered test results")
165+
).toBeTruthy();
166+
return true;
167+
})
168+
.reply(200);
169+
170+
await probot.receive({ name: "pull_request", payload: payload, id: "2" });
171+
handleScope(scope);
172+
});
173+
127174
test("Dr. CI does not comment when the PR is not open", async () => {
128175
nock("https://api.github.com")
129176
.post("/app/installations/2/access_tokens")
@@ -221,6 +268,7 @@ describe("verify-drci-functionality", () => {
221268
{
222269
id: comment_id,
223270
node_id: comment_node_id,
271+
user: { login: drciUtils.DRCI_COMMENT_AUTHOR },
224272
body: "<!-- drci-comment-start -->\nhello\n<!-- drci-comment-end -->\n",
225273
},
226274
])

0 commit comments

Comments
 (0)