Skip to content

Commit 31421ab

Browse files
authored
Add nitpicker-style sticky PR comment bot (#8053)
## Summary - Ports the logic of [ethanis/nitpicker](https://github.com/ethanis/nitpicker) into the existing torchci probot app: when `.github/nitpick.yml` is present, the bot evaluates each rule's `pathFilter` against the PR's changed files and posts a single sticky comment. - Patterns use the upstream syntax: `+pat` includes, `-pat` excludes, no-prefix is treated as include; matching is via `minimatch` (already a dep). Comment is wrapped in `<!-- nitpick-bot-start -->` / `<!-- nitpick-bot-end -->` markers, updated on `pull_request.synchronize`, and deleted when no rules match. - Gated to `pytorch/pytorch` via `isPyTorchPyTorch` initially. Easy to broaden to `isPyTorchbotSupportedOrg` later if other repos want to opt in. ### Notes - Config is fetched directly via `repos.getContent` rather than `context.config()` because `@probot/octokit-plugin-config` deep-merges as objects and silently flattens top-level YAML arrays (the upstream nitpicker schema). - `js-yaml` was already a transitive dep through probot; added it explicitly to `torchci/package.json` along with `@types/js-yaml`. ### Example `.github/nitpick.yml` ```yaml - markdown: | ## Did you update the docs? Please update docs when changing the public API. pathFilter: - "+torch/**" - "-torch/**/*.test.py" ``` ## Test plan - [x] `npx tsc --noEmit` clean - [x] `npx jest test/nitpickBot.test.ts` — 12 new tests pass (parser, file matcher, comment formation, plus probot integration covering: posts on match, no-op when no match, exclude rules suppress matches, updates existing comment, deletes stale comment when nothing matches, skips when no `nitpick.yml`, skips for non-pytorch/pytorch repos) - [ ] After merge, drop a `.github/nitpick.yml` into `pytorch/pytorch` and observe behavior on a draft PR
1 parent 37d7d97 commit 31421ab

4 files changed

Lines changed: 495 additions & 0 deletions

File tree

torchci/lib/bot/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import checkLabelsBot from "./checkLabelsBot";
77
import ciflowPushTrigger from "./ciflowPushTrigger";
88
import codevNoWritePerm from "./codevNoWritePermBot";
99
import drciBot from "./drciBot";
10+
import nitpickBot from "./nitpickBot";
1011
import pytorchBot from "./pytorchBot";
1112
import retryBot from "./retryBot";
1213
import stripApprovalBot from "./stripApprovalBot";
@@ -22,6 +23,7 @@ export default function bot(app: Probot) {
2223
ciflowPushTrigger(app);
2324
codevNoWritePerm(app);
2425
drciBot(app);
26+
nitpickBot(app);
2527
pytorchBot(app);
2628
retryBot(app);
2729
stripApprovalBot(app);

torchci/lib/bot/nitpickBot.ts

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
import * as yaml from "js-yaml";
2+
import { minimatch } from "minimatch";
3+
import { Context, Probot } from "probot";
4+
import { getFilesChangedByPr, isPyTorchPyTorch } from "./utils";
5+
6+
// Implements logic similar to https://github.com/ethanis/nitpicker.
7+
// Reads `.github/nitpick.yml` from the repo's default branch and posts
8+
// (or updates) a single sticky comment on PRs whose changed files match
9+
// any of the configured rules.
10+
//
11+
// Config format (top-level YAML list):
12+
//
13+
// - markdown: |
14+
// ## Did you update the migration?
15+
// Reminder text shown to the PR author.
16+
// pathFilter:
17+
// - "+migrations/**"
18+
// - "-migrations/test/**"
19+
//
20+
// Each pattern is evaluated with `minimatch`. A pattern beginning with `+`
21+
// (or with no prefix) is an include pattern; a pattern beginning with `-`
22+
// is an exclude pattern. A file matches the rule if it matches any include
23+
// pattern and no exclude pattern. The rule fires (its `markdown` is added
24+
// to the comment) when at least one changed file matches.
25+
26+
export const NITPICK_COMMENT_START = "<!-- nitpick-bot-start -->";
27+
export const NITPICK_COMMENT_END = "<!-- nitpick-bot-end -->";
28+
export const NITPICK_CONFIG_PATH = ".github/nitpick.yml";
29+
30+
export interface NitpickRule {
31+
markdown: string;
32+
pathFilter: string[];
33+
}
34+
35+
export function parseNitpickConfig(text: string): NitpickRule[] {
36+
const parsed = yaml.load(text);
37+
if (!Array.isArray(parsed)) {
38+
return [];
39+
}
40+
const rules: NitpickRule[] = [];
41+
for (const entry of parsed) {
42+
if (
43+
entry &&
44+
typeof entry.markdown === "string" &&
45+
Array.isArray(entry.pathFilter)
46+
) {
47+
rules.push({
48+
markdown: entry.markdown,
49+
pathFilter: entry.pathFilter.filter((p: any) => typeof p === "string"),
50+
});
51+
}
52+
}
53+
return rules;
54+
}
55+
56+
export function fileMatchesRule(file: string, rule: NitpickRule): boolean {
57+
let included = false;
58+
for (const pat of rule.pathFilter) {
59+
if (pat.startsWith("-")) {
60+
if (minimatch(file, pat.slice(1))) {
61+
return false;
62+
}
63+
} else {
64+
const glob = pat.startsWith("+") ? pat.slice(1) : pat;
65+
if (minimatch(file, glob)) {
66+
included = true;
67+
}
68+
}
69+
}
70+
return included;
71+
}
72+
73+
export function getMatchingRules(
74+
files: string[],
75+
rules: NitpickRule[]
76+
): NitpickRule[] {
77+
return rules.filter((rule) =>
78+
files.some((file) => fileMatchesRule(file, rule))
79+
);
80+
}
81+
82+
export function formNitpickComment(rules: NitpickRule[]): string {
83+
if (rules.length === 0) {
84+
return "";
85+
}
86+
const body = rules.map((r) => r.markdown.trim()).join("\n\n---\n\n");
87+
return `${NITPICK_COMMENT_START}\n${body}\n${NITPICK_COMMENT_END}`;
88+
}
89+
90+
async function findExistingNitpickComment(
91+
context: Context<"pull_request">,
92+
owner: string,
93+
repo: string,
94+
prNum: number
95+
): Promise<{ id: number; body: string }> {
96+
const res = await context.octokit.issues.listComments({
97+
owner,
98+
repo,
99+
issue_number: prNum,
100+
});
101+
for (const c of res.data) {
102+
if (c.body && c.body.includes(NITPICK_COMMENT_START)) {
103+
return { id: c.id, body: c.body };
104+
}
105+
}
106+
return { id: 0, body: "" };
107+
}
108+
109+
async function loadNitpickConfig(
110+
context: Context<"pull_request">,
111+
owner: string,
112+
repo: string
113+
): Promise<NitpickRule[] | null> {
114+
try {
115+
const res = await context.octokit.repos.getContent({
116+
owner,
117+
repo,
118+
path: NITPICK_CONFIG_PATH,
119+
});
120+
const data = res.data as { content?: string; encoding?: string };
121+
if (!data.content) {
122+
return null;
123+
}
124+
const text = Buffer.from(
125+
data.content,
126+
(data.encoding as BufferEncoding) ?? "base64"
127+
).toString("utf8");
128+
return parseNitpickConfig(text);
129+
} catch (err: any) {
130+
if (err.status === 404) {
131+
return null;
132+
}
133+
throw err;
134+
}
135+
}
136+
137+
export default function nitpickBot(app: Probot): void {
138+
app.on(
139+
[
140+
"pull_request.opened",
141+
"pull_request.reopened",
142+
"pull_request.synchronize",
143+
],
144+
async (context) => {
145+
const owner = context.payload.repository.owner.login;
146+
const repo = context.payload.repository.name;
147+
// Limit to pytorch/pytorch initially.
148+
if (!isPyTorchPyTorch(owner, repo)) {
149+
context.log(
150+
`${__filename} only runs on pytorch/pytorch (got ${owner}/${repo})`
151+
);
152+
return;
153+
}
154+
if (context.payload.pull_request.state !== "open") {
155+
return;
156+
}
157+
const prNum = context.payload.pull_request.number;
158+
159+
const rules = await loadNitpickConfig(context, owner, repo);
160+
if (rules == null) {
161+
context.log(`${NITPICK_CONFIG_PATH} not found, skipping`);
162+
return;
163+
}
164+
165+
const filesChanged = await getFilesChangedByPr(
166+
context.octokit,
167+
owner,
168+
repo,
169+
prNum
170+
);
171+
const matched = getMatchingRules(filesChanged, rules);
172+
const newBody = formNitpickComment(matched);
173+
const existing = await findExistingNitpickComment(
174+
context,
175+
owner,
176+
repo,
177+
prNum
178+
);
179+
180+
if (newBody === "") {
181+
if (existing.id !== 0) {
182+
await context.octokit.issues.deleteComment({
183+
owner,
184+
repo,
185+
comment_id: existing.id,
186+
});
187+
}
188+
return;
189+
}
190+
191+
if (existing.id === 0) {
192+
await context.octokit.issues.createComment({
193+
owner,
194+
repo,
195+
issue_number: prNum,
196+
body: newBody,
197+
});
198+
} else if (existing.body !== newBody) {
199+
await context.octokit.issues.updateComment({
200+
owner,
201+
repo,
202+
comment_id: existing.id,
203+
body: newBody,
204+
});
205+
}
206+
}
207+
);
208+
}

torchci/package.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
"echarts-for-react": "^3.0.2",
4646
"eslint-plugin-unused-imports": "^3.2.0",
4747
"jaro-winkler-typescript": "^1.0.1",
48+
"js-yaml": "^4.1.0",
4849
"lodash": "^4.17.21",
4950
"lz-string": "^1.5.0",
5051
"minimatch": "^9.0.3",
@@ -77,6 +78,7 @@
7778
"@types/d3": "^7.4.3",
7879
"@types/echarts": "^4.9.14",
7980
"@types/jest": "^29.5.14",
81+
"@types/js-yaml": "^4.0.5",
8082
"@types/jsdom": "^16.2.14",
8183
"@types/lodash": "^4.14.182",
8284
"@types/node": "^17.0.40",

0 commit comments

Comments
 (0)