diff --git a/.github/ISSUE_TEMPLATE/bug_issue.md b/.github/ISSUE_TEMPLATE/bug_issue.md index 9a5ff41..de51da9 100644 --- a/.github/ISSUE_TEMPLATE/bug_issue.md +++ b/.github/ISSUE_TEMPLATE/bug_issue.md @@ -16,6 +16,7 @@ Instructions: **Target branch:** ## Observed behavior + @@ -39,6 +40,7 @@ Please wrap errors in triple backticks for clean formatting like this: … ## Expected behavior + @@ -53,6 +55,7 @@ Implications and real-world consequences for learners, coaches, admins, and othe … ## Steps to reproduce + @@ -60,6 +63,7 @@ Precise steps that someone else can follow in order to see this behavior … ## Context + ## Current behavior + ## Desired behavior + ## Value add diff --git a/.github/ISSUE_TEMPLATE/feature_issue.md b/.github/ISSUE_TEMPLATE/feature_issue.md index f040360..8a2eb6f 100644 --- a/.github/ISSUE_TEMPLATE/feature_issue.md +++ b/.github/ISSUE_TEMPLATE/feature_issue.md @@ -14,16 +14,21 @@ type: Tracking Issue --> ## Requirements + ### What problem are we solving? + ### Why this? Why now? + ## Outcomes + - [Specific, measurable outcome] ## Constraints + - [Budget, deadline, technical limitation] @@ -48,12 +53,14 @@ type: Tracking Issue - [Explicit exclusion] ## Context + ### Relevant Feedback - [Feedback from partners, users, clearinghouse, teammates] ### Technical Context + - [Relevant systems, prior art, technical considerations] diff --git a/.github/ISSUE_TEMPLATE/other_issue.md b/.github/ISSUE_TEMPLATE/other_issue.md index abde6bf..56c6482 100644 --- a/.github/ISSUE_TEMPLATE/other_issue.md +++ b/.github/ISSUE_TEMPLATE/other_issue.md @@ -10,6 +10,7 @@ about: For issues that don't fit any other category --> ## Description + diff --git a/.github/ISSUE_TEMPLATE/product_issue.md b/.github/ISSUE_TEMPLATE/product_issue.md index c5e2694..4b54d6c 100644 --- a/.github/ISSUE_TEMPLATE/product_issue.md +++ b/.github/ISSUE_TEMPLATE/product_issue.md @@ -13,19 +13,23 @@ type: Tracking Issue _[General guidance: Product issues should describe the feature, but are not actually technically specced for dev work.]_ ## Overview + _[First, the workflow in one sentence]_ ### Description and outcomes + _[Then, more detail. Provide a short summary and/or list key outcomes that the user should be able to accomplish with this feature.]_ ### Resources + - _[Link Gherkin Scenarios]_ - _[Link Figma Scenarios]_ ### Accessibility Requirements + ## AI usage diff --git a/.github/ISSUE_TEMPLATE/technical_task_issue.md b/.github/ISSUE_TEMPLATE/technical_task_issue.md index 00f1bb1..7cd45a2 100644 --- a/.github/ISSUE_TEMPLATE/technical_task_issue.md +++ b/.github/ISSUE_TEMPLATE/technical_task_issue.md @@ -13,6 +13,7 @@ Instructions: --> ## Overview + ### The Change + ### Acceptance Criteria + ## Overview + ### Scope + ` comment in ``` +A `###` heading can be marked too. A section runs to the next heading of the same or higher level, so a `##` section includes its `###` subsections. A body's heading matches the template's at either level and in any case. + A template that marks no section requires every level-two heading, so that a repo is strict rather than silently unchecked. To change what a repo requires, edit the markers in that repo's template. No change is required here. Four states fail, and the log and comment name every one of them rather than stopping at the first: @@ -108,9 +110,14 @@ These two automations ask `rtibblesbot` to validate claims in contributor-submit ## `issue-validation` -Runs on `issues: [opened]`. It proceeds only if the repository is in `ISSUE_VALIDATION_REPOS`, the issue opener is a contributor, and the issue does not already have the `community-issue-review` label. +Runs on `issues: [opened, edited, reopened]` for open issues whose opener is a contributor, in public repositories. `edited` and `reopened` only run while the issue carries `needs-template`, so issues opened before the check existed are never flagged. + +It first checks the description against this repo's `.github/ISSUE_TEMPLATE/*.md`, using the `` rules of `pull-request-template`. The issue does not record which template it came from, so it passes if it satisfies any one of them. -It posts a comment mentioning `@rtibblesbot` and adds the label. If the label does not exist in the repository, the add-label call creates it. +- **Fails:** posts one comment naming the template it shares the most headings with and what that template still needs, and adds `needs-template`. No validation is requested. +- **Passes:** deletes that comment, removes `needs-template`, and requests validation, unless the issue already carries `community-issue-review`. + +Validation is requested by a comment mentioning `@rtibblesbot` and the `community-issue-review` label. If a label does not exist in the repository, the add-label call creates it. **The mention is essential.** The agent polls its GitHub notifications and has no inbound endpoint, so a mention is the only way to reach it. Adding a label does not generate a notification, and a handle inside backticks, as in `BOT_MESSAGE_RTIBBLESBOT_REVIEW`, does not trigger one either. @@ -145,18 +152,22 @@ The weekly `unassign-inactive-issues` run also sweeps stalled validations throug If the agent never responds, for example, because the service is down or all available tokens have reached their limits, the label would otherwise remain indefinitely. The sweep removes it when the issue's `labeled` event is older than `ISSUE_REVIEW_MAX_DAYS` days. +The same run closes, as not planned, every open issue labelled `needs-template` for longer than `ISSUE_TEMPLATE_MAX_DAYS` days, with `BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED`. To keep an issue open, a maintainer fixes its description or removes the label. Reopening a closed issue reruns the check and restarts the clock. + ## Scope and configuration -Neither automation closes issues or acts on the validation findings. The agent's comment recommends whether the issue should be closed; a maintainer must request that action in a reply. +Neither automation acts on the validation findings. The agent's comment recommends whether the issue should be closed; a maintainer must request that action in a reply. Configure the following constants in `scripts/constants.js`: -- `ISSUE_VALIDATION_REPOS`: The repositories where validation runs, which are the community repositories plus the sandbox the automations are tested in. - `LABEL_COMMUNITY_ISSUE_REVIEW`: The label applied while validation is in progress. - `ISSUE_VALIDATION_MARKER`: The marker that signals validation completion, shared with `learningequality/coding-agent`. - `ISSUE_REVIEW_MAX_DAYS`: The maximum age of the label before the sweep clears it. - `BOT_MESSAGE_ISSUE_VALIDATION`: The comment text posted to request validation. - `ISSUE_VALIDATION_FOLLOW_UPS`: The six verdicts and the follow-up each one asks a maintainer for, used in the Slack line. +- `LABEL_NEEDS_TEMPLATE`: The label on an issue that fails the template check. +- `ISSUE_TEMPLATE_MAX_DAYS`: How long that label stays on before the sweep closes the issue. +- `BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED`: The comment posted when the sweep closes an issue. # `holiday-message` diff --git a/scripts/constants.js b/scripts/constants.js index b77e167..58bdb8f 100644 --- a/scripts/constants.js +++ b/scripts/constants.js @@ -186,6 +186,15 @@ const BOT_MESSAGE_ISSUE_VALIDATION = `- @rtibblesbot's comments are LLM-generated, so please evaluate them critically.\n\n` + `A maintainer decides what happens next. This check does not close the issue.`; +const LABEL_NEEDS_TEMPLATE = 'needs-template'; +const ISSUE_TEMPLATE_MAX_DAYS = 7; +const ISSUE_TEMPLATE_MARKER = ''; + +const BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED = + `**Closing this issue because its description still doesn't follow an issue template ` + + `after ${ISSUE_TEMPLATE_MAX_DAYS} days.** Once you've edited it to follow one, ` + + `comment here and a maintainer can reopen it.`; + // Repositories to include in PR statistics reports const PR_STATS_REPOS = [ 'kolibri', @@ -200,10 +209,6 @@ const PR_STATS_REPOS = [ // Repositories in which we accept open-source contributions const COMMUNITY_REPOS = [...PR_STATS_REPOS]; -// No other automation gates on the repo, so all of them already run in the -// sandbox. See docs/testing-automations.md. -const ISSUE_VALIDATION_REPOS = [...COMMUNITY_REPOS, 'test-actions']; - // Every place GitHub reads a pull request template from. The contents API is // case sensitive, so both spellings are needed. const PR_TEMPLATE_PATHS = [ @@ -244,7 +249,6 @@ module.exports = { LABEL_COMMUNITY_REVIEW, BOT_MESSAGE_COMMUNITY_REVIEW, LABEL_COMMUNITY_ISSUE_REVIEW, - ISSUE_VALIDATION_REPOS, ISSUE_VALIDATION_MARKER, ISSUE_VALIDATION_DUPLICATE_VERDICT, ISSUE_VALIDATION_FOLLOW_UPS, @@ -252,6 +256,10 @@ module.exports = { ISSUE_VALIDATION_NO_VERDICT_FOLLOW_UP, ISSUE_REVIEW_MAX_DAYS, BOT_MESSAGE_ISSUE_VALIDATION, + LABEL_NEEDS_TEMPLATE, + ISSUE_TEMPLATE_MAX_DAYS, + ISSUE_TEMPLATE_MARKER, + BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED, TEAMS_WITH_CLOSE_CONTRIBUTORS, HOLIDAY_MESSAGE, PR_STATS_REPOS, diff --git a/scripts/issue-review-sweep.js b/scripts/issue-review-sweep.js index 2abf681..147df20 100644 --- a/scripts/issue-review-sweep.js +++ b/scripts/issue-review-sweep.js @@ -1,14 +1,20 @@ // See docs/community-automations.md -const { LABEL_COMMUNITY_ISSUE_REVIEW, ISSUE_REVIEW_MAX_DAYS } = require('./constants'); +const { + LABEL_COMMUNITY_ISSUE_REVIEW, + ISSUE_REVIEW_MAX_DAYS, + LABEL_NEEDS_TEMPLATE, + ISSUE_TEMPLATE_MAX_DAYS, + BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED, +} = require('./constants'); const { removeLabel } = require('./utils'); const MS_PER_DAY = 24 * 60 * 60 * 1000; /** - * When the label was last applied, or null when the timeline carries no such - * event. The timeline returns events oldest first, so the last match is the - * most recent application. + * When the label was last applied, or the issue last reopened if later, or null + * when the timeline carries no such label event. The timeline returns events + * oldest first. */ async function labeledAt(name, owner, repo, issueNumber, github, core) { try { @@ -18,55 +24,61 @@ async function labeledAt(name, owner, repo, issueNumber, github, core) { issue_number: issueNumber, per_page: 100, }); - const applied = events.filter( + const applied = events.findLastIndex( event => event.event === 'labeled' && event.label?.name?.toLowerCase() === name.toLowerCase(), ); - if (!applied.length) { + if (applied === -1) { return null; } - return new Date(applied[applied.length - 1].created_at); + const reopened = events.slice(applied).findLast(event => event.event === 'reopened'); + return new Date((reopened ?? events[applied]).created_at); } catch (error) { core.warning(`Failed to read the timeline of issue #${issueNumber}: ${error.message}`); return null; } } -/** - * Removes the review label from every open issue that carried it for longer - * than ISSUE_REVIEW_MAX_DAYS, and returns the issue numbers it cleared. A stuck - * label means the agent never answered, so it tells a maintainer nothing. - */ -async function sweepIssueReviewLabels(owner, repo, { github, core }, now = Date.now()) { +async function staleIssues(name, maxDays, owner, repo, { github, core }, now) { let issues = []; try { issues = await github.paginate(github.rest.issues.listForRepo, { owner, repo, state: 'open', - labels: LABEL_COMMUNITY_ISSUE_REVIEW, + labels: name, per_page: 100, }); } catch (error) { - core.warning(`Failed to list issues under review in ${owner}/${repo}: ${error.message}`); + core.warning(`Failed to list issues labelled ${name} in ${owner}/${repo}: ${error.message}`); return []; } - const cleared = []; + const stale = []; for (const issue of issues) { - const applied = await labeledAt( - LABEL_COMMUNITY_ISSUE_REVIEW, - owner, - repo, - issue.number, - github, - core, - ); - if (!applied) { - continue; - } - if (now - applied.getTime() <= ISSUE_REVIEW_MAX_DAYS * MS_PER_DAY) { - continue; + const applied = await labeledAt(name, owner, repo, issue.number, github, core); + if (applied && now - applied.getTime() > maxDays * MS_PER_DAY) { + stale.push({ issue, applied }); } + } + return stale; +} + +/** + * Removes the review label from every open issue that carried it for longer + * than ISSUE_REVIEW_MAX_DAYS, and returns the issue numbers it cleared. A stuck + * label means the agent never answered, so it tells a maintainer nothing. + */ +async function sweepIssueReviewLabels(owner, repo, { github, core }, now = Date.now()) { + const stale = await staleIssues( + LABEL_COMMUNITY_ISSUE_REVIEW, + ISSUE_REVIEW_MAX_DAYS, + owner, + repo, + { github, core }, + now, + ); + const cleared = []; + for (const { issue, applied } of stale) { const removed = await removeLabel( LABEL_COMMUNITY_ISSUE_REVIEW, owner, @@ -86,4 +98,40 @@ async function sweepIssueReviewLabels(owner, repo, { github, core }, now = Date. return cleared; } -module.exports = { sweepIssueReviewLabels }; +async function closeIssuesMissingTemplate(owner, repo, { github, core }, now = Date.now()) { + const stale = await staleIssues( + LABEL_NEEDS_TEMPLATE, + ISSUE_TEMPLATE_MAX_DAYS, + owner, + repo, + { github, core }, + now, + ); + const closed = []; + for (const { issue, applied } of stale.filter(({ issue }) => !issue.pull_request)) { + try { + await github.rest.issues.update({ + owner, + repo, + issue_number: issue.number, + state: 'closed', + state_reason: 'not_planned', + }); + await github.rest.issues.createComment({ + owner, + repo, + issue_number: issue.number, + body: BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED, + }); + closed.push(issue.number); + core.info( + `Closed issue #${issue.number}, labelled ${LABEL_NEEDS_TEMPLATE} ${applied.toISOString()}.`, + ); + } catch (error) { + core.warning(`Failed to close issue #${issue.number}: ${error.message}`); + } + } + return closed; +} + +module.exports = { sweepIssueReviewLabels, closeIssuesMissingTemplate }; diff --git a/scripts/issue-review-sweep.test.js b/scripts/issue-review-sweep.test.js index 79dc5df..e069e6d 100644 --- a/scripts/issue-review-sweep.test.js +++ b/scripts/issue-review-sweep.test.js @@ -1,8 +1,14 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { sweepIssueReviewLabels } = require('./issue-review-sweep'); -const { LABEL_COMMUNITY_ISSUE_REVIEW, ISSUE_REVIEW_MAX_DAYS } = require('./constants'); +const { sweepIssueReviewLabels, closeIssuesMissingTemplate } = require('./issue-review-sweep'); +const { + LABEL_COMMUNITY_ISSUE_REVIEW, + ISSUE_REVIEW_MAX_DAYS, + LABEL_NEEDS_TEMPLATE, + ISSUE_TEMPLATE_MAX_DAYS, + BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED, +} = require('./constants'); const NOW = Date.parse('2026-10-26T12:00:00Z'); const MS_PER_DAY = 24 * 60 * 60 * 1000; @@ -256,5 +262,128 @@ test('a listing this run cannot read sweeps nothing', async () => { assert.deepEqual(cleared, []); assert.equal(core.warnings.length, 1); - assert.match(core.warnings[0], /Failed to list issues under review/); + assert.match(core.warnings[0], /Failed to list issues labelled/); +}); + +const templateIssue = number => ({ number, labels: [LABEL_NEEDS_TEMPLATE] }); + +function closingGithub(options) { + const github = fakeGithub(options); + github.commented = []; + github.closed = []; + github.rest.issues.createComment = async ({ issue_number, body }) => { + github.commented.push({ issue_number, body }); + }; + github.rest.issues.update = async ({ issue_number, state, state_reason }) => { + github.closed.push({ issue_number, state, state_reason }); + }; + return github; +} + +test('an issue missing its template past the limit is closed with a comment', async () => { + const github = closingGithub({ + issues: [templateIssue(7)], + timelines: { 7: [labeled(daysAgo(ISSUE_TEMPLATE_MAX_DAYS + 1), LABEL_NEEDS_TEMPLATE)] }, + }); + const closed = await closeIssuesMissingTemplate( + 'learningequality', + 'kolibri', + { github, core: fakeCore() }, + NOW, + ); + + assert.deepEqual(closed, [7]); + assert.deepEqual(github.commented, [ + { issue_number: 7, body: BOT_MESSAGE_ISSUE_TEMPLATE_CLOSED }, + ]); + assert.deepEqual(github.closed, [ + { issue_number: 7, state: 'closed', state_reason: 'not_planned' }, + ]); +}); + +test('an issue missing its template within the limit stays open', async () => { + const github = closingGithub({ + issues: [templateIssue(7)], + timelines: { 7: [labeled(daysAgo(ISSUE_TEMPLATE_MAX_DAYS - 1), LABEL_NEEDS_TEMPLATE)] }, + }); + const closed = await closeIssuesMissingTemplate( + 'learningequality', + 'kolibri', + { github, core: fakeCore() }, + NOW, + ); + + assert.deepEqual(closed, []); + assert.deepEqual(github.commented, []); +}); + +test('an issue whose template label a maintainer removed stays open', async () => { + const github = closingGithub({ + issues: [{ number: 7, labels: [] }], + timelines: { 7: [labeled(daysAgo(30), LABEL_NEEDS_TEMPLATE)] }, + }); + const closed = await closeIssuesMissingTemplate( + 'learningequality', + 'kolibri', + { github, core: fakeCore() }, + NOW, + ); + + assert.deepEqual(closed, []); +}); + +test('a close this run cannot make warns and is not counted', async () => { + const github = closingGithub({ + issues: [templateIssue(7)], + timelines: { 7: [labeled(daysAgo(30), LABEL_NEEDS_TEMPLATE)] }, + }); + github.rest.issues.update = async () => { + throw new Error('forbidden'); + }; + const core = fakeCore(); + const closed = await closeIssuesMissingTemplate( + 'learningequality', + 'kolibri', + { github, core }, + NOW, + ); + + assert.deepEqual(closed, []); + assert.deepEqual(github.commented, []); + assert.match(core.warnings[0], /Failed to close issue #7/); +}); + +test('a reopened issue still missing its template gets the full limit again', async () => { + const github = closingGithub({ + issues: [templateIssue(7)], + timelines: { + 7: [ + labeled(daysAgo(30), LABEL_NEEDS_TEMPLATE), + { event: 'reopened', created_at: daysAgo(ISSUE_TEMPLATE_MAX_DAYS - 1) }, + ], + }, + }); + const closed = await closeIssuesMissingTemplate( + 'learningequality', + 'kolibri', + { github, core: fakeCore() }, + NOW, + ); + + assert.deepEqual(closed, []); +}); + +test('a pull request carrying the template label is not closed', async () => { + const github = closingGithub({ + issues: [{ ...templateIssue(7), pull_request: {} }], + timelines: { 7: [labeled(daysAgo(30), LABEL_NEEDS_TEMPLATE)] }, + }); + const closed = await closeIssuesMissingTemplate( + 'learningequality', + 'kolibri', + { github, core: fakeCore() }, + NOW, + ); + + assert.deepEqual(closed, []); }); diff --git a/scripts/issue-template.js b/scripts/issue-template.js new file mode 100644 index 0000000..1db8d99 --- /dev/null +++ b/scripts/issue-template.js @@ -0,0 +1,88 @@ +const fs = require('fs'); +const path = require('path'); + +const { + headingKey, + sections, + requiredSections, + failures, + syncComment, +} = require('./template-check'); +const { + LABEL_NEEDS_TEMPLATE, + ISSUE_TEMPLATE_MARKER, + ISSUE_TEMPLATE_MAX_DAYS, +} = require('./constants'); +const { addLabel } = require('./utils'); + +const TEMPLATE_DIR = path.join(__dirname, '..', '.github', 'ISSUE_TEMPLATE'); +const TEMPLATE_URL = 'https://github.com/learningequality/.github/blob/main/.github/ISSUE_TEMPLATE'; +const FRONT_MATTER_NAME = /^name:\s*["']?(.+?)["']?\s*$/m; + +function readTemplates(dir = TEMPLATE_DIR) { + return fs + .readdirSync(dir) + .filter(file => file.endsWith('.md')) + .sort() + .map(file => { + const text = fs.readFileSync(path.join(dir, file), 'utf8'); + return { + file, + name: FRONT_MATTER_NAME.exec(text)?.[1] ?? file, + headings: sections(text).map(section => headingKey(section.heading)), + required: requiredSections(text), + }; + }); +} + +function closestFailure(body, templates) { + const present = new Set(sections(body).map(section => headingKey(section.heading))); + const scored = templates.map(template => ({ + template, + problems: failures(body, template.required), + shared: template.headings.filter(heading => present.has(heading)).length, + })); + if (scored.some(({ problems }) => !problems.length)) return null; + scored.sort((a, b) => b.shared - a.shared || a.problems.length - b.problems.length); + return scored[0]; +} + +function commentBody({ template, problems }, { owner, repo }) { + return [ + ISSUE_TEMPLATE_MARKER, + `**📝 This issue doesn't follow one of our ` + + `[issue templates](https://github.com/${owner}/${repo}/issues/new/choose) yet.** ` + + `It is closest to [${template.name}](${TEMPLATE_URL}/${template.file}). ` + + 'Please edit the description to fix:', + '', + ...problems.map(problem => `- ${problem}`), + '', + 'The check runs again when you save, and this comment goes away once it passes. ' + + 'We review the issue after that. ' + + `**If it still doesn't follow a template in ${ISSUE_TEMPLATE_MAX_DAYS} days, ` + + 'it will be closed.**', + ].join('\n'); +} + +async function checkIssueTemplate({ github, context, core }, issue, templates = readTemplates()) { + const { owner, repo } = context.repo; + const failure = closestFailure(issue.body, templates); + if (!failure) { + core.info(`Issue #${issue.number} follows a template.`); + await syncComment({ github, context, core }, issue.number, ISSUE_TEMPLATE_MARKER, null); + return true; + } + core.info( + `Issue #${issue.number} is closest to ${failure.template.file}: ` + failure.problems.join(' '), + ); + await syncComment( + { github, context, core }, + issue.number, + ISSUE_TEMPLATE_MARKER, + commentBody(failure, context.repo), + ); + await addLabel(LABEL_NEEDS_TEMPLATE, owner, repo, issue.number, github, core); + return false; +} + +module.exports = { checkIssueTemplate, readTemplates }; diff --git a/scripts/issue-template.test.js b/scripts/issue-template.test.js new file mode 100644 index 0000000..c107bab --- /dev/null +++ b/scripts/issue-template.test.js @@ -0,0 +1,99 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); + +const { checkIssueTemplate, readTemplates } = require('./issue-template'); +const { LABEL_NEEDS_TEMPLATE } = require('./constants'); + +const TEMPLATE_DIR = path.join(__dirname, '..', '.github', 'ISSUE_TEMPLATE'); +const TEMPLATE_FILES = fs.readdirSync(TEMPLATE_DIR).filter(file => file.endsWith('.md')); + +function fakeGithub() { + const comments = []; + const added = []; + return { + comments, + added, + paginate: async () => [], + rest: { + issues: { + listComments: async () => [], + createComment: async ({ body }) => { + comments.push(body); + }, + addLabels: async ({ labels }) => { + added.push(...labels); + }, + }, + }, + }; +} + +const context = { repo: { owner: 'learningequality', repo: 'kolibri' } }; +const core = { info() {}, warning() {} }; + +const check = (body, github = fakeGithub()) => + checkIssueTemplate({ github, context, core }, { number: 7, body }); + +function fillIn(template) { + return template + .split('\n') + .flatMap(line => { + if (line.trim() === '…') return ['Filled in.']; + if (/^\s*\s*$/.test(line)) return [line, 'Filled in.']; + return [line]; + }) + .join('\n'); +} + +for (const file of TEMPLATE_FILES) { + const template = fs.readFileSync(path.join(TEMPLATE_DIR, file), 'utf8'); + + test(`${file} filled in passes`, async () => { + assert.equal(await check(fillIn(template)), true); + }); + + test(`${file} left as written fails`, async () => { + assert.equal(await check(template), false); + }); + + test(`${file} without AI usage fails`, async () => { + const withoutAiUsage = fillIn(template).split('## AI usage')[0]; + assert.equal(await check(withoutAiUsage), false); + }); +} + +test('the templates are read with their names', () => { + const names = readTemplates().map(template => template.name); + assert.ok(names.includes('Report a bug')); +}); + +test('a failure comments, labels, and names the closest template', async () => { + const github = fakeGithub(); + const body = '## Steps to reproduce\n\n1. Sync\n\n## AI usage\n\nNone.\n'; + assert.equal(await check(body, github), false); + + assert.deepEqual(github.added, [LABEL_NEEDS_TEMPLATE]); + assert.match(github.comments[0], /closest to \[Report a bug\]/); + assert.match(github.comments[0], /`## Observed behavior` is missing/); + assert.ok(!github.comments[0].includes('Steps to reproduce')); +}); + +test('a required subsection is checked even when its parent is filled', async () => { + const template = fs.readFileSync(path.join(TEMPLATE_DIR, 'technical_tracking_issue.md'), 'utf8'); + const withoutScope = fillIn(template).replace('### Scope', '### Boundaries'); + const github = fakeGithub(); + assert.equal(await check(withoutScope, github), false); + assert.match(github.comments[0], /`### Scope` is missing/); +}); + +test('a filled template matches whatever heading level the body uses', async () => { + // kolibri#15703 followed the bug template with `###` headings throughout. + const template = fs.readFileSync(path.join(TEMPLATE_DIR, 'bug_issue.md'), 'utf8'); + assert.equal(await check(fillIn(template).replace(/^## /gm, '### ')), true); +}); + +test('an empty body fails rather than throwing', async () => { + assert.equal(await check(null), false); +}); diff --git a/scripts/issue-validation.js b/scripts/issue-validation.js index 4b441f9..988607e 100644 --- a/scripts/issue-validation.js +++ b/scripts/issue-validation.js @@ -1,11 +1,12 @@ // See docs/community-automations.md const { - ISSUE_VALIDATION_REPOS, LABEL_COMMUNITY_ISSUE_REVIEW, + LABEL_NEEDS_TEMPLATE, BOT_MESSAGE_ISSUE_VALIDATION, } = require('./constants'); -const { sendBotMessage, hasLabel, addLabel } = require('./utils'); +const { sendBotMessage, addLabel, removeLabel } = require('./utils'); +const { checkIssueTemplate } = require('./issue-template'); module.exports = async ({ github, context, core }) => { try { @@ -13,12 +14,41 @@ module.exports = async ({ github, context, core }) => { const repo = context.repo.repo; const issueNumber = context.payload.issue.number; - if (!ISSUE_VALIDATION_REPOS.includes(repo)) { - core.info(`Issue validation does not run in ${repo}, so none is requested.`); + if (context.payload.repository?.private) { + core.info(`Issue validation does not run in private repo ${repo}.`); return; } - if (await hasLabel(LABEL_COMMUNITY_ISSUE_REVIEW, owner, repo, issueNumber, github, core)) { + const { data: issue } = await github.rest.issues.get({ + owner, + repo, + issue_number: issueNumber, + }); + if (issue.state !== 'open') { + core.info(`Issue #${issueNumber} is ${issue.state}.`); + return; + } + + const labels = (issue.labels || []).map(label => + (typeof label === 'string' ? label : label.name).toLowerCase(), + ); + const flagged = labels.includes(LABEL_NEEDS_TEMPLATE); + if (context.payload.action !== 'opened' && !flagged) { + core.info(`Issue #${issueNumber} is not waiting on the template.`); + return; + } + + if (!(await checkIssueTemplate({ github, context, core }, issue))) { + return; + } + if ( + flagged && + !(await removeLabel(LABEL_NEEDS_TEMPLATE, owner, repo, issueNumber, github, core)) + ) { + return; + } + + if (labels.includes(LABEL_COMMUNITY_ISSUE_REVIEW)) { core.info(`Issue #${issueNumber} already carries ${LABEL_COMMUNITY_ISSUE_REVIEW}.`); return; } diff --git a/scripts/issue-validation.test.js b/scripts/issue-validation.test.js index 99c12df..16ce0fd 100644 --- a/scripts/issue-validation.test.js +++ b/scripts/issue-validation.test.js @@ -2,39 +2,90 @@ const test = require('node:test'); const assert = require('node:assert/strict'); const issueValidation = require('./issue-validation'); -const { LABEL_COMMUNITY_ISSUE_REVIEW, COMMUNITY_REPOS } = require('./constants'); - -const SANDBOX = 'test-actions'; - -function fakeGithub({ labels = [], addFails = false } = {}) { - const comments = []; +const { + LABEL_COMMUNITY_ISSUE_REVIEW, + LABEL_NEEDS_TEMPLATE, + BOT_MESSAGE_ISSUE_VALIDATION, + ISSUE_TEMPLATE_MARKER, + LE_BOT_USERNAME, +} = require('./constants'); + +const FOLLOWS_TEMPLATE = '## Description\n\nThe sync fails.\n\n## AI usage\n\nI used no AI.\n'; +// Shaped like kolibri#15779, which an agent wrote without a template. +const FREESTYLED = [ + '## Summary', + '', + 'The sync fails.', + '', + '## Steps to reproduce', + '', + '1. Sync', + '', + '## Suggested fix', + '', + 'Retry.', +].join('\n'); + +function fakeGithub({ + body = FOLLOWS_TEMPLATE, + labels = [], + state = 'open', + comments = [], + addFails = false, + removeFails = false, +} = {}) { + const created = []; + const updated = []; + const deleted = []; const added = []; + const removed = []; + const listComments = async () => comments; return { - comments, + created, + updated, + deleted, added, + removed, paginate: async (fn, params) => fn(params), rest: { issues: { - listLabelsOnIssue: async () => labels.map(name => ({ name })), + get: async () => ({ + data: { number: 7, body, state, labels: labels.map(name => ({ name })) }, + }), + listComments, addLabels: async ({ labels: names }) => { if (addFails) { throw new Error('Resource not accessible by integration'); } added.push(...names); }, - createComment: async ({ body }) => { - comments.push(body); + removeLabel: async ({ name }) => { + if (removeFails) { + const error = new Error('server error'); + error.status = 500; + throw error; + } + removed.push(name); + }, + createComment: async ({ body: text }) => { + created.push(text); return { data: { html_url: 'https://github.com/learningequality/kolibri/issues/7' } }; }, + updateComment: async ({ comment_id, body: text }) => { + updated.push({ comment_id, body: text }); + }, + deleteComment: async ({ comment_id }) => { + deleted.push(comment_id); + }, }, }, }; } -function fakeContext({ repo = COMMUNITY_REPOS[0] } = {}) { +function fakeContext({ action = 'opened', isPrivate = false } = {}) { return { - repo: { owner: 'learningequality', repo }, - payload: { issue: { number: 7 } }, + repo: { owner: 'learningequality', repo: 'kolibri' }, + payload: { action, issue: { number: 7 }, repository: { private: isPrivate } }, }; } @@ -55,12 +106,14 @@ function fakeCore() { }; } -test('a new issue in a community repo gets one comment and the label', async () => { +const templateComment = { id: 3, user: { login: LE_BOT_USERNAME }, body: ISSUE_TEMPLATE_MARKER }; + +test('a new issue following a template gets one comment and the review label', async () => { const github = fakeGithub(); const core = fakeCore(); await issueValidation({ github, context: fakeContext(), core }); - assert.equal(github.comments.length, 1); + assert.deepEqual(github.created, [BOT_MESSAGE_ISSUE_VALIDATION]); assert.deepEqual(github.added, [LABEL_COMMUNITY_ISSUE_REVIEW]); assert.deepEqual(core.failures, []); }); @@ -71,32 +124,98 @@ test('the comment mentions rtibblesbot outside backticks, which is the trigger', // A backticked handle sends no notification, and the mention is the only way // to reach the agent. See learningequality/coding-agent#1229. - assert.match(github.comments[0], /(^|[^`])@rtibblesbot/); + assert.match(github.created[0], /(^|[^`])@rtibblesbot/); }); -test('a repo outside the community list gets neither', async () => { - const github = fakeGithub(); - const core = fakeCore(); - await issueValidation({ github, context: fakeContext({ repo: 'internal-tooling' }), core }); +test('a new freestyled issue is flagged and not sent for validation', async () => { + const github = fakeGithub({ body: FREESTYLED }); + await issueValidation({ github, context: fakeContext(), core: fakeCore() }); - assert.deepEqual(github.comments, []); - assert.deepEqual(github.added, []); - assert.deepEqual(core.failures, []); + assert.equal(github.created.length, 1); + assert.ok(github.created[0].includes(ISSUE_TEMPLATE_MARKER)); + assert.match(github.created[0], /closest to \[Report a bug\]/); + assert.match(github.created[0], /`## AI usage` is missing/); + assert.ok( + !github.created[0].includes('@rtibblesbot'), + 'the template comment must not summon the agent', + ); + assert.deepEqual(github.added, [LABEL_NEEDS_TEMPLATE]); }); -test('the sandbox is in scope, so the automation can be tested there', async () => { - const github = fakeGithub(); - await issueValidation({ github, context: fakeContext({ repo: SANDBOX }), core: fakeCore() }); +test('an edit that makes a flagged issue pass clears it and requests validation', async () => { + const github = fakeGithub({ labels: [LABEL_NEEDS_TEMPLATE], comments: [templateComment] }); + await issueValidation({ github, context: fakeContext({ action: 'edited' }), core: fakeCore() }); - assert.equal(github.comments.length, 1); + assert.deepEqual(github.deleted, [3]); + assert.deepEqual(github.removed, [LABEL_NEEDS_TEMPLATE]); + assert.deepEqual(github.created, [BOT_MESSAGE_ISSUE_VALIDATION]); assert.deepEqual(github.added, [LABEL_COMMUNITY_ISSUE_REVIEW]); }); -test('an issue already under review gets no second comment', async () => { +test('a flagged issue whose label cannot come off is not sent for validation', async () => { + const github = fakeGithub({ labels: [LABEL_NEEDS_TEMPLATE], removeFails: true }); + await issueValidation({ github, context: fakeContext({ action: 'edited' }), core: fakeCore() }); + + assert.deepEqual(github.created, []); + assert.deepEqual(github.added, []); +}); + +test('an edit that leaves a flagged issue failing updates the comment', async () => { + const github = fakeGithub({ + body: FREESTYLED, + labels: [LABEL_NEEDS_TEMPLATE], + comments: [templateComment], + }); + await issueValidation({ github, context: fakeContext({ action: 'edited' }), core: fakeCore() }); + + assert.deepEqual(github.created, []); + assert.equal(github.updated.length, 1); + assert.deepEqual(github.removed, []); +}); + +test('an edit to an issue never flagged is not checked', async () => { + const github = fakeGithub({ body: FREESTYLED }); + await issueValidation({ github, context: fakeContext({ action: 'edited' }), core: fakeCore() }); + + assert.deepEqual(github.created, []); + assert.deepEqual(github.added, []); +}); + +test('reopening a flagged issue that now passes requests validation', async () => { + const github = fakeGithub({ labels: [LABEL_NEEDS_TEMPLATE] }); + await issueValidation({ + github, + context: fakeContext({ action: 'reopened' }), + core: fakeCore(), + }); + + assert.deepEqual(github.removed, [LABEL_NEEDS_TEMPLATE]); + assert.deepEqual(github.created, [BOT_MESSAGE_ISSUE_VALIDATION]); +}); + +test('an edit to a closed issue does nothing until it is reopened', async () => { + const github = fakeGithub({ labels: [LABEL_NEEDS_TEMPLATE], state: 'closed' }); + await issueValidation({ github, context: fakeContext({ action: 'edited' }), core: fakeCore() }); + + assert.deepEqual(github.created, []); + assert.deepEqual(github.removed, []); +}); + +test('an issue in a private repo gets neither', async () => { + const github = fakeGithub({ body: FREESTYLED }); + const core = fakeCore(); + await issueValidation({ github, context: fakeContext({ isPrivate: true }), core }); + + assert.deepEqual(github.created, []); + assert.deepEqual(github.added, []); + assert.deepEqual(core.failures, []); +}); + +test('an issue already under review gets no second request', async () => { const github = fakeGithub({ labels: [LABEL_COMMUNITY_ISSUE_REVIEW] }); await issueValidation({ github, context: fakeContext(), core: fakeCore() }); - assert.deepEqual(github.comments, []); + assert.deepEqual(github.created, []); assert.deepEqual(github.added, []); }); @@ -104,7 +223,7 @@ test('the label match ignores case', async () => { const github = fakeGithub({ labels: ['Community-Issue-Review'] }); await issueValidation({ github, context: fakeContext(), core: fakeCore() }); - assert.deepEqual(github.comments, []); + assert.deepEqual(github.created, []); }); test('a failed label add warns and leaves the run passing', async () => { @@ -112,7 +231,7 @@ test('a failed label add warns and leaves the run passing', async () => { const core = fakeCore(); await issueValidation({ github, context: fakeContext(), core }); - assert.equal(github.comments.length, 1); + assert.equal(github.created.length, 1); assert.deepEqual(core.failures, []); assert.equal(core.warnings.length, 1); assert.match(core.warnings[0], /Failed to add/); diff --git a/scripts/pull-request-template.js b/scripts/pull-request-template.js index 43d65de..e72d334 100644 --- a/scripts/pull-request-template.js +++ b/scripts/pull-request-template.js @@ -7,65 +7,13 @@ const fs = require('fs'); const path = require('path'); const { isContributor } = require('./utils'); -const { LE_BOT_USERNAME, PR_TEMPLATE_PATHS, PR_TEMPLATE_TARGET } = require('./constants'); +const { requiredSections, failures, syncComment } = require('./template-check'); +const { PR_TEMPLATE_PATHS, PR_TEMPLATE_TARGET } = require('./constants'); const ORG_TEMPLATE_PATH = path.join(__dirname, '..', PR_TEMPLATE_TARGET); -const HEADING = /^## [^#]/; -const REQUIRED_MARKER = /^\s*\s*$/; -const HTML_COMMENT = //g; -const PLACEHOLDER = '…'; const COMMENT_MARKER = ''; -function sections(markdown) { - const out = []; - for (const line of (markdown || '').split(/\r?\n/)) { - if (HEADING.test(line)) out.push({ heading: line.trim(), lines: [] }); - else if (out.length) out[out.length - 1].lines.push(line); - } - return out; -} - -function isMarked(section) { - const first = section.lines.find(line => line.trim() !== ''); - return first !== undefined && REQUIRED_MARKER.test(first); -} - -// A template that marks nothing requires every section, so a repo whose template -// has no markers is strict rather than unchecked. -function requiredSections(template) { - const all = sections(template); - const marked = all.filter(isMarked); - return marked.length ? marked : all; -} - -function content(section) { - return section.lines.join('\n').replace(HTML_COMMENT, '').trim(); -} - -function failures(body, required) { - const found = new Map(sections(body).map(section => [section.heading.toLowerCase(), section])); - const problems = []; - for (const { heading, lines } of required) { - const section = found.get(heading.toLowerCase()); - if (!section) { - problems.push(`\`${heading}\` is missing.`); - continue; - } - const text = content(section); - if (text === '') { - problems.push(`\`${heading}\` is empty.`); - } else if (text.split('\n').some(line => line.trim() === PLACEHOLDER)) { - // The template puts the placeholder on a line of its own, so an ellipsis - // inside a sentence is prose rather than an unfilled section. - problems.push(`\`${heading}\` still holds the \`${PLACEHOLDER}\` placeholder.`); - } else if (text === content({ lines })) { - problems.push(`\`${heading}\` still holds the text from the template.`); - } - } - return problems; -} - /** * The repo's own template, `null` if none exists, or `{ unreadable }` if a * candidate path returns anything other than a 404. An unreadable path may @@ -97,34 +45,6 @@ async function readRepoTemplate({ github, context, core }, ref) { return { template: null }; } -async function syncComment({ github, context, core }, issueNumber, body) { - const { owner, repo } = context.repo; - try { - const comments = await github.paginate(github.rest.issues.listComments, { - owner, - repo, - issue_number: issueNumber, - per_page: 100, - }); - const ours = comments.filter( - comment => comment.user?.login === LE_BOT_USERNAME && comment.body?.includes(COMMENT_MARKER), - ); - if (body === null) { - await Promise.all( - ours.map(comment => - github.rest.issues.deleteComment({ owner, repo, comment_id: comment.id }), - ), - ); - } else if (!ours.length) { - await github.rest.issues.createComment({ owner, repo, issue_number: issueNumber, body }); - } else if (ours[0].body !== body) { - await github.rest.issues.updateComment({ owner, repo, comment_id: ours[0].id, body }); - } - } catch (error) { - core.warning(`Could not update the template comment: ${error.message}`); - } -} - function commentBody(problems, templateUrl) { return [ COMMENT_MARKER, @@ -167,7 +87,7 @@ module.exports = async ({ github, context, core }) => { const problems = failures(pullRequest.body, required); if (!problems.length) { core.info(`The body carries all ${required.length} required sections.`); - await syncComment({ github, context, core }, pullRequest.number, null); + await syncComment({ github, context, core }, pullRequest.number, COMMENT_MARKER, null); return; } @@ -177,6 +97,7 @@ module.exports = async ({ github, context, core }) => { await syncComment( { github, context, core }, pullRequest.number, + COMMENT_MARKER, commentBody(problems, templateUrl), ); diff --git a/scripts/template-check.js b/scripts/template-check.js new file mode 100644 index 0000000..75fee97 --- /dev/null +++ b/scripts/template-check.js @@ -0,0 +1,93 @@ +const { LE_BOT_USERNAME } = require('./constants'); + +const HEADING = /^(#{2,3}) [^#]/; +const REQUIRED_MARKER = /^\s*\s*$/; +const HTML_COMMENT = //g; +const PLACEHOLDER = '…'; + +function headingKey(heading) { + return heading.replace(/^#+\s*/, '').toLowerCase(); +} + +function sections(markdown) { + const lines = (markdown || '').split(/\r?\n/); + const headings = []; + lines.forEach((line, index) => { + const match = HEADING.exec(line); + if (match) headings.push({ heading: line.trim(), level: match[1].length, index }); + }); + return headings.map(({ heading, level, index }, i) => { + const end = headings.slice(i + 1).find(next => next.level <= level); + return { heading, level, lines: lines.slice(index + 1, end ? end.index : lines.length) }; + }); +} + +function isMarked(section) { + const first = section.lines.find(line => line.trim() !== ''); + return first !== undefined && REQUIRED_MARKER.test(first); +} + +// A template that marks nothing requires every top-level section, so a repo +// whose template has no markers is strict rather than unchecked. +function requiredSections(template) { + const all = sections(template); + const marked = all.filter(isMarked); + return marked.length ? marked : all.filter(section => section.level === 2); +} + +function content(section) { + return section.lines.join('\n').replace(HTML_COMMENT, '').trim(); +} + +function failures(body, required) { + const found = new Map(sections(body).map(section => [headingKey(section.heading), section])); + const problems = []; + for (const { heading, lines } of required) { + const section = found.get(headingKey(heading)); + if (!section) { + problems.push(`\`${heading}\` is missing.`); + continue; + } + const text = content(section); + if (text === '') { + problems.push(`\`${heading}\` is empty.`); + } else if (text.split('\n').some(line => line.trim() === PLACEHOLDER)) { + // The template puts the placeholder on a line of its own, so an ellipsis + // inside a sentence is prose rather than an unfilled section. + problems.push(`\`${heading}\` still holds the \`${PLACEHOLDER}\` placeholder.`); + } else if (text === content({ lines })) { + problems.push(`\`${heading}\` still holds the text from the template.`); + } + } + return problems; +} + +async function syncComment({ github, context, core }, issueNumber, marker, body) { + const { owner, repo } = context.repo; + try { + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: issueNumber, + per_page: 100, + }); + const ours = comments.filter( + comment => comment.user?.login === LE_BOT_USERNAME && comment.body?.includes(marker), + ); + if (body === null) { + await Promise.all( + ours.map(comment => + github.rest.issues.deleteComment({ owner, repo, comment_id: comment.id }), + ), + ); + } else if (!ours.length) { + await github.rest.issues.createComment({ owner, repo, issue_number: issueNumber, body }); + } else if (ours[0].body !== body) { + await github.rest.issues.updateComment({ owner, repo, comment_id: ours[0].id, body }); + } + } catch (error) { + core.warning(`Could not update the template comment: ${error.message}`); + } +} + +module.exports = { headingKey, sections, requiredSections, failures, syncComment }; diff --git a/scripts/unassign-inactive-issues.js b/scripts/unassign-inactive-issues.js index f5f804e..0be01de 100644 --- a/scripts/unassign-inactive-issues.js +++ b/scripts/unassign-inactive-issues.js @@ -1,4 +1,4 @@ -const { sweepIssueReviewLabels } = require('./issue-review-sweep'); +const { sweepIssueReviewLabels, closeIssuesMissingTemplate } = require('./issue-review-sweep'); const formatUnassignments = unassignments => { if (unassignments.length === 0) return ''; @@ -259,6 +259,7 @@ module.exports = async ({ github, context, core }) => { } await sweepIssueReviewLabels(owner, repo, { github, core }); + await closeIssuesMissingTemplate(owner, repo, { github, core }); const issues = await getAllIssues(github, owner, repo); //console.log(`Processing ${issues.length} open issues`);