Skip to content

Commit cec6396

Browse files
Joibelclaude
andauthored
feat(ci): add PR readiness helper bot (argoproj#16231)
Signed-off-by: Alan Clucas <alan@clucas.org> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 9577e45 commit cec6396

17 files changed

Lines changed: 1407 additions & 0 deletions
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
Description: PR readiness helper bot guides contributors through fixing CI failures
2+
Authors: [Alan Clucas](https://github.com/Joibel)
3+
Component: Build and Development
4+
Issues: 16231
5+
6+
A bot now helps contributors get their PRs ready for review.
7+
When CI completes on a PR it maintains a single comment listing the contributor-fixable problems — lint, codegen, UI, build, docs, PR title format, missing feature files, DCO sign-off and an unfilled PR description — each with the command to fix it.
8+
PRs with blocking problems are moved to draft; mark the PR ready for review again once they are fixed.
9+
The comment updates as checks change and shows all-clear when everything contributor-fixable is resolved.
10+
Unit and E2E test results are not covered by the bot.
11+
Maintainers can tune the covered checks and guidance in `.github/pr-readiness/checks.config.json`.

.github/pr-readiness/.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
node_modules/

.github/pr-readiness/README.md

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
# PR Readiness Helper
2+
3+
A standalone bot ([`pr-readiness.yaml`](../workflows/pr-readiness.yaml)) that lowers maintainer burden: when CI finishes on a PR, it keeps **one sticky comment** telling the contributor exactly how to fix contributor-fixable problems, moves not-ready PRs to **draft**, and gets out of the way once everything is green.
4+
5+
## What it covers
6+
7+
| Signal | Check run | Guidance |
8+
|---|---|---|
9+
| Lint | `Lint` (CI) | `make pre-commit -B` |
10+
| Codegen | `Codegen` (CI) | `make codegen -B` |
11+
| UI | `UI` (CI) | `yarn --cwd ui …` |
12+
| Build | `Build Binaries (cli/controller)` (CI) | `make cli` / `make controller` |
13+
| Docs | `docs` (Docs) | `make docs` |
14+
| PR title | `title-check` (PR Title Check) | conventional commits |
15+
| Feature file | `feature-pr-handling` (PR Feature Check) | `make feature-new` / `make features-validate` (per failing step) |
16+
| DCO | `DCO` (dco app) | `git rebase --signoff` |
17+
| PR description | template check (no CI signal) | fill in the template |
18+
19+
**Deliberately not covered:** unit tests, Windows unit tests, E2E tests — they are too flaky to be a readiness signal and are never mentioned.
20+
21+
Tune guidance, add or remove signals in [`checks.config.json`](checks.config.json) — it is pure data; no logic changes needed. The `match.check` value is the **check-run name** (= the job's `name:`, or the job *id* when it has none — not a step name).
22+
23+
## Behavior
24+
25+
- Fires on `workflow_run: completed` of CI / Docs / PR Title Check / PR Feature Check. Title and feature checks re-run on PR `edited`, so title/description edits re-evaluate too. `/retest` re-runs CI and therefore re-evaluates.
26+
- **Never posts** on a PR that never had a covered issue.
27+
- While issues exist: one comment listing only the failing items, each with a fix command and a log link. Pending checks are not mentioned.
28+
- Blocking issues (any covered check failure, or a description that doesn't follow the template) also **convert the PR to draft**. The bot **never** marks ready-for-review — that's the contributor's call — and it drafts at most once per head SHA, so a human re-marking it ready is respected until new commits arrive.
29+
- Draft conversion needs a **GitHub App token**: the default Actions token cannot toggle draft state (`Resource not accessible by integration` — verified live). Provision an app with **Pull requests: Read & write** only (do not reuse the cherry-pick app, which can push code), install it on the repo, and set the `PR_READINESS_APP_ID` / `PR_READINESS_APP_PRIVATE_KEY` secrets — the same `actions/create-github-app-token` pattern as `cherry-pick-single.yml`. Without the secrets the bot comments but does not draft.
30+
- When issues are resolved but other covered checks are still running: the comment shows a short "waiting" state.
31+
- When everything is terminal and green: the comment is edited to a short ✅ all-clear.
32+
- Skipped: PRs by anyone in [`OWNERS`](../../OWNERS) (owners/approvers/reviewers) and by bots.
33+
34+
## PR description check
35+
36+
A deterministic check ([`template.ts`](template.ts)) compares the description against [the PR template](../pull_request_template.md): it flags an empty body, the unreplaced `Fixes #TODO` placeholder, and any required `###` section that is missing or left empty (template placeholder comments don't count as content). The required sections are derived from the template itself, so the check follows the template if it changes. It deliberately does **not** judge prose quality.
37+
38+
> An earlier version used GitHub Models (an LLM) to judge description quality. That was removed: GitHub Models is not available to `argoproj` under the CNCF enterprise, and the deterministic check covers the common "didn't fill it in" cases without a model, a network call, data leaving the repo, or `models:` permissions. If Models is ever enabled and richer judgment is wanted, it could be reintroduced behind a flag.
39+
40+
## Security model
41+
42+
- `workflow_run` workflows execute the **default branch's** definition with the base-repo token — a fork PR cannot alter what runs here.
43+
- The job **never checks out or executes PR-head code**; the checkout step takes the default branch only. Keep it that way.
44+
- `permissions: {}` at the top; the job grants only `pull-requests: write`, `contents: read`, `actions: read`. No secrets beyond `GITHUB_TOKEN` (and the optional draft-app secrets).
45+
- `workflow_run.pull_requests` is empty for fork PRs, so the PR is found by matching `head_sha` against open PRs; no match → exit (a newer push superseded the run).
46+
- PR title/body/branch are attacker-controlled: they are only ever handled as data, never interpolated into shell or scripts. The comment never echoes contributor-supplied text — only the bot's own guidance, check titles/URLs, and template section names.
47+
- All actions are pinned to full commit SHAs (enforced by repo lint).
48+
49+
## Dry run & rollout
50+
51+
`DRY_RUN: "true"` in the workflow renders the would-be comment and decisions to the job's **step summary** instead of commenting or drafting. Roll out: merge with dry-run on → watch summaries on real PRs (correct PR resolution, author gating, sensible text) → flip to `"false"`.
52+
53+
## Maintenance notes
54+
55+
- **Check renamed?** The signal silently stops matching (fail-safe — no false positives) and any failing unmapped check from a covered app is logged as a warning ("unmapped failing check") so you notice. Update `checks.config.json`.
56+
- **Workflow renamed?** Keep `on.workflow_run.workflows` in `pr-readiness.yaml` in sync with the `name:` fields of `ci-build.yaml`, `docs.yaml`, `pr.yaml`, `pr-feature.yaml`.
57+
- **Known limitation:** first-time contributors whose workflows need approval get no help until a maintainer approves the run (nothing completes, so nothing fires).
58+
59+
## Code & local development
60+
61+
The logic is TypeScript (ESM) under this directory. `main.ts` is the entry point — a single `run()` called from one `actions/github-script` step; the pure, unit-tested logic lives in `classify.ts`, `comment.ts`, `template.ts`, with shared types in `types.ts`.
62+
63+
At runtime there are **no dependencies**: `actions/github-script` runs on Node 24, which strips the TypeScript types when it `require()`s `main.ts` — no build step and nothing compiled is committed. Because stripping does not *type-check*, [`pr-readiness-test.yaml`](../workflows/pr-readiness-test.yaml) runs `tsc --noEmit` and the unit tests on every PR that touches this directory. `typescript` and `@types/node` are dev-only (CI/editor), never shipped to the runner.
64+
65+
```sh
66+
cd .github/pr-readiness
67+
npm ci
68+
npm run typecheck # tsc --noEmit
69+
npm test # node --test test/*.test.ts
70+
```
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
{
2+
"comment": "Classification of contributor-fixable PR checks. See README.md. 'match.check' is the CHECK-RUN name (= job name or job id), not a step name.",
3+
"signals": [
4+
{
5+
"id": "lint",
6+
"match": { "check": "Lint" },
7+
"title": "Lint",
8+
"guidance": "Run `make pre-commit -B` locally to auto-fix most lint issues, then commit and push the result."
9+
},
10+
{
11+
"id": "codegen",
12+
"match": { "check": "Codegen" },
13+
"title": "Codegen",
14+
"guidance": "Generated files are out of date with your changes. Run `make codegen -B` (or `make pre-commit -B`) locally and commit the regenerated files."
15+
},
16+
{
17+
"id": "ui",
18+
"match": { "check": "UI" },
19+
"title": "UI",
20+
"guidance": "The UI build/lint failed. Run `yarn --cwd ui install && yarn --cwd ui build && yarn --cwd ui lint` locally and commit any fixes."
21+
},
22+
{
23+
"id": "build-cli",
24+
"match": { "check": "Build Binaries (cli)" },
25+
"title": "Build (CLI)",
26+
"guidance": "The CLI failed to compile. Reproduce with `make cli` and fix the build error — the compiler output is in the linked log."
27+
},
28+
{
29+
"id": "build-controller",
30+
"match": { "check": "Build Binaries (controller)" },
31+
"title": "Build (controller)",
32+
"guidance": "The controller failed to compile. Reproduce with `make controller` and fix the build error — the compiler output is in the linked log."
33+
},
34+
{
35+
"id": "docs",
36+
"match": { "check": "docs" },
37+
"title": "Docs",
38+
"guidance": "The documentation build/lint failed. Run `make docs` locally (it auto-fixes some issues) and commit the result."
39+
},
40+
{
41+
"id": "pr-title",
42+
"match": { "check": "title-check" },
43+
"title": "PR title",
44+
"guidance": "Your PR title must follow [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/), e.g. `fix: ...`, `feat: ...`, `docs: ...`. Edit the PR title (no push needed)."
45+
},
46+
{
47+
"id": "features",
48+
"match": { "check": "feature-pr-handling" },
49+
"title": "Feature description (.features/)",
50+
"guidance": "Feature PRs need a valid `.features/pending/*.md` file. Run `make feature-new` to create one, edit it, then check it with `make features-validate`.",
51+
"stepGuidance": {
52+
"No ./.features/*.md addition": "This is a `feat:` PR but no feature description file was added. Run `make feature-new` and edit the generated file under `.features/pending/`.",
53+
"Validate ./.features/*.md changes": "Your `.features/pending/*.md` file failed validation. Run `make features-validate` locally and fix the reported problems."
54+
}
55+
},
56+
{
57+
"id": "dco",
58+
"match": { "check": "DCO", "app": "dco" },
59+
"title": "DCO (sign-off)",
60+
"guidance": "One or more commits are missing the `Signed-off-by` line. Sign off (`git commit --amend --signoff` for the last commit, or `git rebase --signoff main`) and force-push. See the [DCO app](https://github.com/apps/dco) for details."
61+
}
62+
],
63+
"ignoreChecks": [
64+
"Unit Tests",
65+
"Windows Unit Tests",
66+
"E2E Tests*",
67+
"argo-images*",
68+
"Get changed files",
69+
"review"
70+
],
71+
"coveredApps": ["github-actions", "dco"]
72+
}

.github/pr-readiness/classify.ts

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
// Core decision logic for the PR-readiness helper. Pure functions here are
2+
// unit-tested (test/classify.test.ts); the API-calling orchestration lives in
3+
// main.ts.
4+
5+
import type { CheckRun, Config, Decision, GitHubUser, JobStep, Signal, SignalMatch, SignalState } from './types.ts';
6+
7+
const FAILURE_CONCLUSIONS = new Set(['failure', 'timed_out', 'action_required']);
8+
const NOT_APPLICABLE_CONCLUSIONS = new Set(['skipped', 'cancelled']);
9+
10+
function matchesIgnore(name: string, patterns: string[]): boolean {
11+
return patterns.some((p) => (p.endsWith('*') ? name.startsWith(p.slice(0, -1)) : name === p));
12+
}
13+
14+
function findRun(checkRuns: CheckRun[], match: SignalMatch): CheckRun | undefined {
15+
return checkRuns.find(
16+
(r) => r.name === match.check && (!match.app || (r.app != null && r.app.slug === match.app))
17+
);
18+
}
19+
20+
function runState(run: CheckRun | undefined): SignalState {
21+
if (!run) {
22+
return 'not-applicable';
23+
}
24+
if (run.status !== 'completed') {
25+
return 'pending';
26+
}
27+
if (run.conclusion !== null && FAILURE_CONCLUSIONS.has(run.conclusion)) {
28+
return 'failure';
29+
}
30+
if (run.conclusion !== null && NOT_APPLICABLE_CONCLUSIONS.has(run.conclusion)) {
31+
return 'not-applicable';
32+
}
33+
return 'success'; // success, neutral
34+
}
35+
36+
// Maps the covered signals from checks.config.json onto the live check runs
37+
// for a head SHA. Uncovered checks (unit/E2E tests etc.) are invisible.
38+
export function classifySignals(checkRuns: CheckRun[], config: Config): Signal[] {
39+
return config.signals.map((signal) => {
40+
const run = findRun(checkRuns, signal.match);
41+
return {
42+
id: signal.id,
43+
title: signal.title,
44+
guidance: signal.guidance,
45+
stepGuidance: signal.stepGuidance ?? null,
46+
state: runState(run),
47+
url: run ? run.html_url : null,
48+
};
49+
});
50+
}
51+
52+
// Drift detection: failing check runs from apps we cover that match neither a
53+
// signal nor the ignore list — typically a renamed job. Logged, never posted.
54+
export function diagnostics(checkRuns: CheckRun[], config: Config): { unmapped: string[] } {
55+
const unmapped = checkRuns
56+
.filter(
57+
(r) =>
58+
r.status === 'completed' &&
59+
r.conclusion !== null &&
60+
FAILURE_CONCLUSIONS.has(r.conclusion) &&
61+
r.app != null &&
62+
config.coveredApps.includes(r.app.slug) &&
63+
!config.signals.some((s) => findRun([r], s.match)) &&
64+
!matchesIgnore(r.name, config.ignoreChecks)
65+
)
66+
.map((r) => r.name);
67+
return { unmapped };
68+
}
69+
70+
interface DecideArgs {
71+
signals: ReadonlyArray<{ id: string; state: string }>;
72+
templateVerdict: { compliant: boolean } | null;
73+
existingState: { draftedSha?: string | null } | null;
74+
hasExistingComment: boolean;
75+
pr: { draft: boolean; headSha: string };
76+
}
77+
78+
// The convergence rules. See README.md for the decision table.
79+
export function decide({ signals, templateVerdict, existingState, hasExistingComment, pr }: DecideArgs): Decision {
80+
const failing = signals.filter((s) => s.state === 'failure').map((s) => s.id);
81+
const templateBlocking = Boolean(templateVerdict && templateVerdict.compliant === false);
82+
const blocking = failing.length > 0 || templateBlocking;
83+
const anyPending = signals.some((s) => s.state === 'pending');
84+
85+
let variant: Decision['variant'] = null;
86+
let shouldComment = false;
87+
if (blocking) {
88+
variant = 'issues';
89+
shouldComment = true;
90+
} else if (hasExistingComment) {
91+
variant = anyPending ? 'waiting' : 'allclear';
92+
shouldComment = true;
93+
}
94+
95+
const alreadyDraftedThisSha = Boolean(existingState && existingState.draftedSha === pr.headSha);
96+
const shouldDraft = blocking && !pr.draft && !alreadyDraftedThisSha;
97+
98+
return { variant, shouldComment, shouldDraft, failing, templateBlocking };
99+
}
100+
101+
// OWNERS is a small YAML subset: three keys, each a list of logins.
102+
export function parseOwners(yamlText: string): string[] {
103+
const sections = new Set(['owners', 'approvers', 'reviewers']);
104+
const logins: string[] = [];
105+
let current: string | null = null;
106+
for (const line of yamlText.split('\n')) {
107+
const key = line.match(/^(\w+):/);
108+
if (key) {
109+
current = key[1];
110+
continue;
111+
}
112+
const item = line.match(/^-\s*(\S+)/);
113+
if (item && current !== null && sections.has(current)) {
114+
logins.push(item[1]);
115+
}
116+
}
117+
return logins;
118+
}
119+
120+
export function isExemptAuthor(user: GitHubUser, ownersYaml: string): boolean {
121+
if (user.type === 'Bot' || /\[bot\]$/i.test(user.login)) {
122+
return true;
123+
}
124+
const login = user.login.toLowerCase();
125+
return parseOwners(ownersYaml).some((l) => l.toLowerCase() === login);
126+
}
127+
128+
export function findPullRequest<T extends { head: { sha: string } }>(openPrs: T[], headSha: string): T | null {
129+
return openPrs.find((pr) => pr.head.sha === headSha) ?? null;
130+
}
131+
132+
// For checks with per-step guidance (the feature-pr-handling job), pick the
133+
// guidance of the failing step; fall back to the signal's generic guidance.
134+
export function pickStepGuidance(
135+
signal: { guidance: string; stepGuidance?: Record<string, string> | null },
136+
steps: JobStep[] | null
137+
): string {
138+
if (signal.stepGuidance && Array.isArray(steps)) {
139+
const failed = steps.find((s) => s.conclusion === 'failure' && signal.stepGuidance![s.name]);
140+
if (failed) {
141+
return signal.stepGuidance[failed.name];
142+
}
143+
}
144+
return signal.guidance;
145+
}

0 commit comments

Comments
 (0)