From bbdcdfa1311ae04cb25070ad198094faed24b21b Mon Sep 17 00:00:00 2001 From: caesarsage Date: Sat, 31 Oct 2026 10:51:34 +0100 Subject: [PATCH 1/2] ci: add Prow-style triage and review slash commands Adds a triage bot, run by GitHub Actions, that applies Kubernetes-style slash commands from issue and pull request comments. New issues get needs-triage. Contributors and maintainers triage with /triage, /kind, /area, /platform, /good-first-issue and /help, and contributors pick up accepted issues with /assign. Maintainers also have /priority, /milestone, /keep-open, /close duplicate|wontfix|invalid and the /remove-* commands. On pull requests, /lgtm, /approve, /hold, /unhold, /retest, /cc and /uncc are available; review commands only set labels, so merging still needs a code owner review. A hold check fails while do-not-merge/hold is set. The bot lives in hack/triage (parse.js, plan.js, run.js) with all permission rules in the pure plan() function, and is tested with node --test in the new triage-tests workflow. It only checks out hack/triage from the default branch and never runs pull request code. The welcome messages now point first-time contributors to the triage flow. Commands are documented in documentation/triage-commands.md. Refs #603 Signed-off-by: caesarsage --- .github/workflows/hold.yml | 29 ++ .github/workflows/triage-commands.yml | 69 +++++ .github/workflows/triage-tests.yml | 29 ++ .github/workflows/welcome-new-users.yml | 8 +- documentation/triage-commands.md | 74 +++++ hack/triage/parse.js | 65 ++++ hack/triage/plan.js | 384 ++++++++++++++++++++++++ hack/triage/run.js | 230 ++++++++++++++ hack/triage/test/parse.test.js | 51 ++++ hack/triage/test/plan.test.js | 301 +++++++++++++++++++ hack/triage/test/run.test.js | 239 +++++++++++++++ 11 files changed, 1477 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/hold.yml create mode 100644 .github/workflows/triage-commands.yml create mode 100644 .github/workflows/triage-tests.yml create mode 100644 documentation/triage-commands.md create mode 100644 hack/triage/parse.js create mode 100644 hack/triage/plan.js create mode 100644 hack/triage/run.js create mode 100644 hack/triage/test/parse.test.js create mode 100644 hack/triage/test/plan.test.js create mode 100644 hack/triage/test/run.test.js diff --git a/.github/workflows/hold.yml b/.github/workflows/hold.yml new file mode 100644 index 00000000..f0633b59 --- /dev/null +++ b/.github/workflows/hold.yml @@ -0,0 +1,29 @@ +# Fails while a pull request has the do-not-merge/hold label (set with /hold). +# Labels set by the triage bot don't start workflows, so after /hold or /unhold +# the bot re-runs this workflow (see hack/triage/run.js); that is why the label +# is read from the API rather than from the event payload. + +name: hold + +on: + pull_request: + types: [opened, reopened, synchronize, labeled, unlabeled] + +permissions: {} + +jobs: + hold: + runs-on: ubuntu-latest + permissions: + pull-requests: read + steps: + - name: Fail while the pull request is on hold + env: + GH_TOKEN: ${{ github.token }} + PR: ${{ github.event.pull_request.number }} + run: | + labels=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/labels" --jq '.[].name') + if grep -qx 'do-not-merge/hold' <<<"$labels"; then + echo "::error::This pull request is on hold. Comment /unhold to release it." + exit 1 + fi diff --git a/.github/workflows/triage-commands.yml b/.github/workflows/triage-commands.yml new file mode 100644 index 00000000..6671544b --- /dev/null +++ b/.github/workflows/triage-commands.yml @@ -0,0 +1,69 @@ +# Prow-style slash commands for issues and PRs: /triage accepted, /kind bug, /assign, +# and on PRs /lgtm, /approve, /hold, /retest, /cc, ... +# See documentation/triage-commands.md for the command list. +# +# Security: both events run in the context of the default branch, and the job only +# checks out hack/triage from it. Pull request code is never checked out or run; +# commands are read from the comment text only. + +name: triage-commands + +on: + issues: + types: [opened] + issue_comment: + types: [created] + +# Explicitly disable all default GITHUB_TOKEN permissions at the workflow level. +# Each job then declares only the minimal required permissions. +permissions: {} + +jobs: + label-new-issue: + if: github.event_name == 'issues' + runs-on: ubuntu-latest + permissions: + contents: read + issues: write + steps: + - name: Checkout triage scripts + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 + with: + sparse-checkout: hack/triage + persist-credentials: false + + - name: Add needs-triage label + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const { labelNewIssue } = require('./hack/triage/run.js') + await labelNewIssue({ github, context }) + + commands: + if: >- + github.event_name == 'issue_comment' && + github.event.comment.user.type != 'Bot' && + contains(github.event.comment.body, '/') + runs-on: ubuntu-latest + permissions: + contents: read + issues: write + pull-requests: write + # /retest re-runs failed workflow runs of a pull request. + actions: write + # No concurrency group: GitHub keeps only one pending run per group and + # cancels the others, which silently drops commands from quick comments. + # Each run reads the current labels instead. + steps: + - name: Checkout triage scripts + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 + with: + sparse-checkout: hack/triage + persist-credentials: false + + - name: Apply slash commands + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const { run } = require('./hack/triage/run.js') + await run({ github, context, core }) diff --git a/.github/workflows/triage-tests.yml b/.github/workflows/triage-tests.yml new file mode 100644 index 00000000..ec2c32bd --- /dev/null +++ b/.github/workflows/triage-tests.yml @@ -0,0 +1,29 @@ +# Unit tests for the triage bot in hack/triage. They only use Node's built-in +# test runner, so no dependencies are installed. + +name: triage-tests + +on: + push: + paths: + - 'hack/triage/**' + - '.github/workflows/triage-tests.yml' + pull_request: + paths: + - 'hack/triage/**' + - '.github/workflows/triage-tests.yml' + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + steps: + - name: Checkout Code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #v7.0.1 + with: + persist-credentials: false + + - name: Run triage tests + run: node --test hack/triage/test/*.test.js diff --git a/.github/workflows/welcome-new-users.yml b/.github/workflows/welcome-new-users.yml index 1a1e6d2e..bf445160 100644 --- a/.github/workflows/welcome-new-users.yml +++ b/.github/workflows/welcome-new-users.yml @@ -32,7 +32,9 @@ jobs: Welcome to the Microcks community! 💖 Thanks and congrats 🎉 for opening your first issue here! Be sure to follow the issue template or please update it accordingly. - + + 🏷️ A maintainer will triage it soon. If you'd like to work on it, please wait for the `triage/accepted` label, then comment `/assign`. See the [triage commands](https://github.com/microcks/microcks-cli/blob/master/documentation/triage-commands.md). + 📢 If you're using Microcks in your organization, please [add your company name to this list](https://github.com/microcks/.github/blob/main/ADOPTERS.md). 🙏 It really helps the project to gain momentum and credibility. It's a small contribution back to the project with a big impact. If you need to know why and how to add yourself to the list, please read the blog post ["Join the Microcks Adopters list and Empower the vibrant open source Community 🙌"](https://microcks.io/blog/join-adopters-list/) @@ -60,7 +62,9 @@ jobs: Welcome to the Microcks community! 💖 Thanks and congrats 🎉 for opening your first pull request here! Be sure to follow the pull request template or please update it accordingly. - + + 🏷️ Please link the issue this fixes; we review pull requests for issues labelled `triage/accepted` first. See the [triage commands](https://github.com/microcks/microcks-cli/blob/master/documentation/triage-commands.md). + Hope you have a great time there! welcome-pr-merged: diff --git a/documentation/triage-commands.md b/documentation/triage-commands.md new file mode 100644 index 00000000..e15bd0c0 --- /dev/null +++ b/documentation/triage-commands.md @@ -0,0 +1,74 @@ +# Triage commands + +Issues and pull requests in this repository can be triaged with Prow-style slash commands, similar to Kubernetes. Write the command at the start of a line in a comment. You can put several commands in one comment, one per line. The bot reacts with 👍 when the commands are applied, and replies if one could not be applied. + +## Triage flow + +1. Every new issue gets the `needs-triage` label. +2. A maintainer or contributor checks it and comments `/triage accepted`, or `/triage needs-information` if more details are needed from the reporter. +3. Once an issue has `triage/accepted`, contributors can pick it up with `/assign` and open a pull request that references it. + +Please wait for `triage/accepted` before starting work. It avoids spending time on a fix or feature that the maintainers would not merge. + +## Commands for everyone + +| Command | Effect | +| --- | --- | +| `/assign` | Assigns yourself. On issues, only once the issue has `triage/accepted`. | +| `/unassign` | Unassigns yourself. | +| `/close`, `/reopen` | Closes or reopens an issue or pull request you opened. You can only reopen it if you closed it yourself; if a maintainer closed it, ask them. | + +## Commands for contributors and maintainers + +Contributors are people who already have commits in this repository. They can help triage issues and pull requests opened by others, but not their own. + +| Command | Effect | +| --- | --- | +| `/triage accepted` | Adds `triage/accepted` and removes `needs-triage`. | +| `/triage needs-information` | Adds `triage/needs-information` and removes `needs-triage`. | +| `/kind bug\|feature\|enhancement\|task\|question` | Adds the matching `kind/*` label. | +| `/area cli\|connectors\|build\|docs\|packaging` | Adds the matching `component/*` label. | +| `/platform windows\|linux\|macos` | Adds the matching `platform/*` label. | +| `/good-first-issue` | Adds `good first issue`. On issues, needs `triage/accepted` first. | +| `/help` | Adds `help wanted`. On issues, needs `triage/accepted` first. | + +## Commands for maintainers + +Maintainers are people with write access to this repository. They can use every command above, including on their own issues, and the following ones to correct or manage triage. + +| Command | Effect | +| --- | --- | +| `/remove-triage` | Removes the `triage/*` labels and adds `needs-triage` back. | +| `/remove-kind`, `/remove-area`, `/remove-platform` | Removes the matching label, for example `/remove-kind bug`. | +| `/remove-good-first-issue`, `/remove-help` | Removes `good first issue` or `help wanted`. | +| `/keep-open` | Adds `keep-open`, so the stale bot skips the item. `/remove-keep-open` removes it. | +| `/assign @user`, `/unassign @user` | Assigns or unassigns someone else. GitHub only lets you assign people who have commented on the issue or have access to the repository. | +| `/close`, `/reopen` | Closes or reopens any issue or pull request. `/reopen` also removes the `status/*` label. | +| `/close duplicate\|wontfix\|invalid` | Closes with a reason and adds the matching `status/*` label. On issues it also sets GitHub's close reason (duplicate or not planned). | +| `/priority critical-urgent\|important-soon\|important-longterm\|backlog` | Sets the `priority/*` label, replacing any other priority. `/remove-priority` removes it. | +| `/milestone ` | Sets an open GitHub milestone, for example `/milestone 1.0.3`. `/milestone clear` or `/remove-milestone` removes it. | + +## Pull request commands + +These commands only work in pull request comments. `lgtm`, `approved` and `do-not-merge/hold` are review signals for maintainers: merging still needs an approving GitHub review from a code owner. + +| Command | Who | Effect | +| --- | --- | --- | +| `/lgtm` | Contributors and maintainers, not on their own pull request | Adds `lgtm`. | +| `/approve` | Maintainers, not on their own pull request | Adds `approved`. | +| `/remove-lgtm` (`/lgtm cancel`), `/remove-approve` (`/approve cancel`) | Maintainers | Removes `lgtm` or `approved`. | +| `/hold` | Contributors, maintainers and the author | Adds `do-not-merge/hold`. The `hold` check fails while the label is set. | +| `/unhold` (`/hold cancel`) | Maintainers | Removes `do-not-merge/hold`. | +| `/retest` | Maintainers and the author | Re-runs the failed checks on the latest commit. | +| `/cc @user`, `/uncc @user` | Contributors, maintainers and the author | Requests or removes a review from someone. | + +Commands inside code blocks, quoted lines (`> /kind bug`) and HTML comments are ignored, and unknown commands are ignored silently. If a command can't be applied, the bot reacts with 😕 and explains why, once per person and issue. + +## Example + +```text +/triage accepted +/kind bug +/area connectors +/good-first-issue +``` diff --git a/hack/triage/parse.js b/hack/triage/parse.js new file mode 100644 index 00000000..31640b79 --- /dev/null +++ b/hack/triage/parse.js @@ -0,0 +1,65 @@ +/* + * Copyright The Microcks Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// @ts-check + +// Reads slash commands from comment text. + +const MAX_COMMANDS = 10; + +/** + * @typedef {object} Command + * @property {string} name command name without the slash, lowercased + * @property {string[]} args arguments, with a leading "@" removed + */ + +/** + * Extracts slash commands from a comment body. Commands must start a line; + * lines inside fenced code blocks and quoted lines ("> /kind bug") are + * ignored so that quoting a previous comment does not re-run it. + * + * @param {string | null | undefined} body + * @returns {Command[]} + */ +function parseCommands(body) { + const commands = []; + let inFence = false; + // GitHub doesn't render HTML comments, so commands inside them would run + // without reviewers seeing them. An unclosed comment hides the rest. + const visible = String(body || '').replace(/<!--[\s\S]*?(-->|$)/g, ''); + for (const raw of visible.split(/\r?\n/)) { + const line = raw.trim(); + if (line.startsWith('```') || line.startsWith('~~~')) { + inFence = !inFence; + continue; + } + if (inFence || !line.startsWith('/')) { + continue; + } + const [head, ...args] = line.split(/\s+/); + const name = head.slice(1).toLowerCase(); + if (!/^[a-z][a-z-]*$/.test(name)) { + continue; + } + commands.push({ name, args: args.map((a) => a.replace(/^@/, '')).filter(Boolean) }); + if (commands.length >= MAX_COMMANDS) { + break; + } + } + return commands; +} + +module.exports = { parseCommands }; diff --git a/hack/triage/plan.js b/hack/triage/plan.js new file mode 100644 index 00000000..c43021d8 --- /dev/null +++ b/hack/triage/plan.js @@ -0,0 +1,384 @@ +/* + * Copyright The Microcks Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// @ts-check + +// Decides what a set of commands does. All permission rules live here; no +// GitHub API calls are made. + +const NEEDS_TRIAGE = 'needs-triage'; +const TRIAGE_ACCEPTED = 'triage/accepted'; +// Pull request review signals. They are labels only: merging still needs an +// approving GitHub review from a code owner, and a maintainer merges by hand. +// TODO: merge automatically once a PR has lgtm and approved, no hold and green +// checks, like Prow's Tide. The master ruleset requires a code owner review, +// so the bot would need a bypass there (or a GitHub App) first; that needs +// maintainer agreement. +const LGTM = 'lgtm'; +const APPROVED = 'approved'; +const HOLD = 'do-not-merge/hold'; +const CANCEL = { lgtm: 'remove-lgtm', approve: 'remove-approve', hold: 'unhold' }; + +// Command argument -> repository label. Only these labels can be set. +const TRIAGE = { + accepted: TRIAGE_ACCEPTED, + 'needs-information': 'triage/needs-information', +}; +const KIND = { + bug: 'kind/bug', + feature: 'kind/feature', + enhancement: 'kind/enhancement', + task: 'kind/task', + question: 'kind/question', +}; +const AREA = { + cli: 'component/cli', + connectors: 'component/connectors', + build: 'component/build', + docs: 'component/documentation', + packaging: 'component/packaging', +}; +const PLATFORM = { + windows: 'platform/windows', + linux: 'platform/linux', + macos: 'platform/macos', +}; +const FLAGS = { + help: 'help wanted', + 'good-first-issue': 'good first issue', + 'keep-open': 'keep-open', +}; +const PRIORITY = { + 'critical-urgent': 'priority/critical-urgent', + 'important-soon': 'priority/important-soon', + 'important-longterm': 'priority/important-longterm', + backlog: 'priority/backlog', +}; +const CLOSE_REASON = { + duplicate: { label: 'status/duplicate', stateReason: 'duplicate' }, + wontfix: { label: 'status/wontfix', stateReason: 'not_planned' }, + invalid: { label: 'status/invalid', stateReason: 'not_planned' }, +}; + +/** + * @typedef {object} Context + * @property {string} commenter login of the person who wrote the comment + * @property {string | undefined} author login of the issue or pull request author + * @property {boolean} isMaintainer commenter has write access or more + * @property {boolean} isContributor commenter already has commits in the repository + * @property {boolean} isPullRequest the comment is on a pull request + * @property {string[]} labels current labels of the issue or pull request + * @property {string} [closedBy] login of the person who last closed it, if any + */ + +/** + * @typedef {object} Plan + * @property {string[]} addLabels + * @property {string[]} removeLabels + * @property {string[]} assign + * @property {string[]} unassign + * @property {string[]} reviewers review requests to add + * @property {string[]} unreviewers review requests to remove + * @property {boolean} retest re-run the failed checks of the pull request + * @property {'open' | 'closed' | null} state + * @property {'duplicate' | 'not_planned' | null} stateReason why an issue is closed + * @property {{ title: string | null } | null} milestone milestone to set (title null clears it), or null for no change + * @property {string[]} errors explanations for commands that were not applied + */ + +/** + * Turns parsed commands into the changes to apply. Pure function so it can be + * unit tested without the GitHub API. + * + * Maintainers can use every command. Contributors (people with commits in the + * repository) can add triage and label values, but not on their own issues; + * removing labels stays with maintainers so they can correct a triage. + * + * @param {import('./parse').Command[]} commands + * @param {Context} ctx + * @returns {Plan} + */ +function plan(commands, ctx) { + const labels = new Set(ctx.labels); + /** @type {Plan} */ + const out = { + addLabels: [], removeLabels: [], assign: [], unassign: [], reviewers: [], unreviewers: [], retest: false, state: null, stateReason: null, milestone: null, errors: [], + }; + + const add = (label) => { + if (!labels.has(label)) { + labels.add(label); + out.addLabels.push(label); + } + out.removeLabels = out.removeLabels.filter((l) => l !== label); + }; + const remove = (label) => { + if (labels.has(label)) { + labels.delete(label); + out.removeLabels.push(label); + } + out.addLabels = out.addLabels.filter((l) => l !== label); + }; + const isAuthor = Boolean(ctx.author) && ctx.author.toLowerCase() === ctx.commenter.toLowerCase(); + const needMaintainer = (cmd) => { + if (!ctx.isMaintainer) { + out.errors.push(`\`/${cmd}\` can only be used by maintainers.`); + return false; + } + return true; + }; + const needTriager = (cmd) => { + if (ctx.isMaintainer) return true; + if (!ctx.isContributor) { + out.errors.push(`\`/${cmd}\` can be used by maintainers and by people who have contributed to this repository.`); + return false; + } + if (isAuthor) { + out.errors.push(`\`/${cmd}\` can't be used on your own ${ctx.isPullRequest ? 'pull request' : 'issue'}. Another contributor or a maintainer will triage it.`); + return false; + } + return true; + }; + const needPullRequest = (cmd) => { + if (!ctx.isPullRequest) { + out.errors.push(`\`/${cmd}\` can only be used on pull requests.`); + return false; + } + return true; + }; + // Maintainers, contributors and the pull request author. + const needParticipant = (cmd) => { + if (ctx.isMaintainer || ctx.isContributor || isAuthor) return true; + out.errors.push(`\`/${cmd}\` can be used by maintainers, by people who have contributed to this repository, and by the author.`); + return false; + }; + const mapped = (cmd, table, arg) => { + const label = table[String(arg || '').toLowerCase()]; + if (!label) { + out.errors.push(`\`/${cmd}\` expects one of: ${Object.keys(table).map((k) => `\`${k}\``).join(', ')}.`); + } + return label; + }; + const labelCommand = (name, table, args) => { + const removing = name.startsWith('remove-'); + const base = removing ? name.slice('remove-'.length) : name; + if (!(removing ? needMaintainer(name) : needTriager(name))) return; + if (args.length === 0) { + out.errors.push(`\`/${name}\` expects one of: ${Object.keys(table).map((k) => `\`${k}\``).join(', ')}.`); + return; + } + for (const arg of args) { + const label = mapped(base, table, arg); + if (label) (removing ? remove : add)(label); + } + }; + + for (const command of commands) { + const { args } = command; + const name = CANCEL[command.name] && String(args[0]).toLowerCase() === 'cancel' ? CANCEL[command.name] : command.name; + switch (name) { + case 'triage': { + if (!needTriager(name)) break; + const label = mapped(name, TRIAGE, args[0]); + if (!label) break; + for (const l of Object.values(TRIAGE)) if (l !== label) remove(l); + remove(NEEDS_TRIAGE); + add(label); + break; + } + case 'remove-triage': + if (!needMaintainer(name)) break; + for (const l of Object.values(TRIAGE)) remove(l); + add(NEEDS_TRIAGE); + break; + case 'kind': + case 'remove-kind': + labelCommand(name, KIND, args); + break; + case 'area': + case 'remove-area': + labelCommand(name, AREA, args); + break; + case 'platform': + case 'remove-platform': + labelCommand(name, PLATFORM, args); + break; + case 'help': + case 'good-first-issue': + case 'keep-open': { + // keep-open exempts an item from the stale bot, so it stays with maintainers. + if (!(name === 'keep-open' ? needMaintainer(name) : needTriager(name))) break; + if (name !== 'keep-open' && !ctx.isPullRequest && !labels.has(TRIAGE_ACCEPTED)) { + out.errors.push(`\`/${name}\` needs the issue to be accepted first: use \`/triage accepted\`.`); + break; + } + add(FLAGS[name]); + break; + } + case 'remove-help': + case 'remove-good-first-issue': + case 'remove-keep-open': + if (!needMaintainer(name)) break; + remove(FLAGS[name.slice('remove-'.length)]); + break; + case 'assign': { + const targets = args.length ? args : [ctx.commenter]; + const others = targets.filter((u) => u.toLowerCase() !== ctx.commenter.toLowerCase()); + if (others.length && !ctx.isMaintainer) { + out.errors.push('Only maintainers can assign other people. Use `/assign` on its own to assign yourself.'); + break; + } + if (!ctx.isMaintainer && !ctx.isPullRequest && !labels.has(TRIAGE_ACCEPTED)) { + out.errors.push('This issue has not been accepted yet. A maintainer will triage it first; please wait for the `triage/accepted` label before picking it up.'); + break; + } + if (!ctx.isMaintainer && ctx.isPullRequest) { + out.errors.push('Only maintainers can assign pull requests.'); + break; + } + out.assign.push(...targets); + break; + } + case 'unassign': { + const targets = args.length ? args : [ctx.commenter]; + const others = targets.filter((u) => u.toLowerCase() !== ctx.commenter.toLowerCase()); + if (others.length && !ctx.isMaintainer) { + out.errors.push('Only maintainers can unassign other people. Use `/unassign` on its own to unassign yourself.'); + break; + } + out.unassign.push(...targets); + break; + } + case 'close': + case 'reopen': { + if (!ctx.isMaintainer && !isAuthor) { + out.errors.push(`\`/${name}\` can only be used by maintainers or the author.`); + break; + } + // Like GitHub itself, the author can only reopen what they closed, not + // an item a maintainer closed (for example as out of scope). + const closedBySomeoneElse = Boolean(ctx.closedBy) && ctx.closedBy.toLowerCase() !== ctx.commenter.toLowerCase(); + if (name === 'reopen' && !ctx.isMaintainer && closedBySomeoneElse) { + out.errors.push('`/reopen` can\'t reopen an item a maintainer closed. Please ask a maintainer if you think it should be reopened.'); + break; + } + if (name === 'reopen') { + for (const { label } of Object.values(CLOSE_REASON)) remove(label); + out.state = 'open'; + break; + } + if (args.length) { + const reason = CLOSE_REASON[args[0].toLowerCase()]; + if (!reason) { + out.errors.push(`\`/close\` expects nothing or one of: ${Object.keys(CLOSE_REASON).map((k) => `\`${k}\``).join(', ')}.`); + break; + } + // A reason is a maintainer's judgement, even on the author's own item. + if (!ctx.isMaintainer) { + out.errors.push('`/close` with a reason can only be used by maintainers. Use `/close` on its own.'); + break; + } + add(reason.label); + if (!ctx.isPullRequest) out.stateReason = reason.stateReason; + } + out.state = 'closed'; + break; + } + case 'priority': { + if (!needMaintainer(name)) break; + const label = mapped(name, PRIORITY, args[0]); + if (!label) break; + for (const l of Object.values(PRIORITY)) if (l !== label) remove(l); + add(label); + break; + } + case 'milestone': + case 'remove-milestone': { + if (!needMaintainer(name)) break; + // "/milestone clear" is the Prow spelling of "/remove-milestone". + const clear = name === 'remove-milestone' || (args.length === 1 && args[0].toLowerCase() === 'clear'); + if (!clear && args.length === 0) { + out.errors.push('`/milestone` expects a milestone title, for example `/milestone 1.0.3`, or `clear`.'); + break; + } + // The title is checked against the repository's open milestones when applied. + out.milestone = { title: clear ? null : args.join(' ') }; + break; + } + case 'remove-priority': + if (!needMaintainer(name)) break; + for (const l of Object.values(PRIORITY)) remove(l); + break; + case 'lgtm': + if (!needPullRequest(name)) break; + if (isAuthor) { + out.errors.push('`/lgtm` can\'t be used on your own pull request.'); + break; + } + if (!needTriager(name)) break; + add(LGTM); + break; + case 'approve': + if (!needPullRequest(name) || !needMaintainer(name)) break; + // GitHub doesn't let authors approve their own pull request either. + if (isAuthor) { + out.errors.push('`/approve` can\'t be used on your own pull request. Another maintainer will review it.'); + break; + } + add(APPROVED); + break; + case 'remove-lgtm': + case 'remove-approve': + if (!needPullRequest(name) || !needMaintainer(name)) break; + remove(name === 'remove-lgtm' ? LGTM : APPROVED); + break; + case 'hold': + if (!needPullRequest(name) || !needParticipant(name)) break; + add(HOLD); + break; + case 'unhold': + // Anyone involved can ask to stop a merge, but only a maintainer lifts + // it, so a contributor can't undo a maintainer's hold. + if (!needPullRequest(name) || !needMaintainer(name)) break; + remove(HOLD); + break; + case 'retest': + if (!needPullRequest(name)) break; + if (!ctx.isMaintainer && !isAuthor) { + out.errors.push('`/retest` can only be used by maintainers or the author.'); + break; + } + out.retest = true; + break; + case 'cc': + case 'uncc': + if (!needPullRequest(name) || !needParticipant(name)) break; + if (args.length === 0) { + out.errors.push(`\`/${name}\` expects one or more users, for example \`/${name} @user\`.`); + break; + } + (name === 'cc' ? out.reviewers : out.unreviewers).push(...args); + break; + default: + // Unknown commands are ignored, like Prow does, so "/path/to/file" + // or other text starting with a slash does not trigger a reply. + break; + } + } + return out; +} + +module.exports = { plan, NEEDS_TRIAGE, HOLD }; diff --git a/hack/triage/run.js b/hack/triage/run.js new file mode 100644 index 00000000..f8a47fe8 --- /dev/null +++ b/hack/triage/run.js @@ -0,0 +1,230 @@ +/* + * Copyright The Microcks Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// @ts-check + +// Entry points called from actions/github-script. They read the event, +// ask plan() what to do and apply it through the GitHub API. + +const { parseCommands } = require('./parse'); +const { plan, NEEDS_TRIAGE, HOLD } = require('./plan'); + +/** + * The parts of actions/github-script's arguments that are used here. The + * Octokit client is left untyped so no type packages need to be installed. + * + * @typedef {{ rest: any, paginate: any }} GitHub + * @typedef {{ repo: { owner: string, repo: string }, payload: any }} Context + * @typedef {{ warning: (message: string) => void }} Core + */ + +const DOCS_URL = 'https://github.com/microcks/microcks-cli/blob/master/documentation/triage-commands.md'; +const HOLD_WORKFLOW = 'hold.yml'; + +const MAINTAINER_PERMISSIONS = ['admin', 'maintain', 'write']; +const CONTRIBUTOR_ASSOCIATIONS = ['CONTRIBUTOR', 'COLLABORATOR', 'MEMBER', 'OWNER']; + +/** + * @param {GitHub} github + * @param {string} owner + * @param {string} repo + * @param {string} username + * @returns {Promise<boolean>} + */ +async function isMaintainer(github, owner, repo, username) { + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ owner, repo, username }); + return MAINTAINER_PERMISSIONS.includes(data.permission); + } catch (err) { + // 404 means the user is not a collaborator. + if (err.status === 404) return false; + throw err; + } +} + +/** + * Applies the slash commands of an issue_comment event. + * + * @param {{ github: GitHub, context: Context, core: Core }} args + */ +async function run({ github, context, core }) { + const { comment, issue } = context.payload; + if (!comment || !issue || comment.user.type === 'Bot') return; + + const commands = parseCommands(comment.body); + if (commands.length === 0) return; + + const { owner, repo } = context.repo; + const issue_number = issue.number; + const commenter = comment.user.login; + + const maintainer = await isMaintainer(github, owner, repo, commenter); + // The event payload is a snapshot from when the comment was posted; an + // earlier comment's run may have changed labels since, so read them again. + const { data: current } = await github.rest.issues.get({ owner, repo, issue_number }); + const result = plan(commands, { + commenter, + author: issue.user && issue.user.login, + isMaintainer: maintainer, + isContributor: maintainer || CONTRIBUTOR_ASSOCIATIONS.includes(comment.author_association), + isPullRequest: Boolean(issue.pull_request), + labels: (current.labels || []).map((l) => (typeof l === 'string' ? l : l.name)), + closedBy: current.closed_by ? current.closed_by.login : undefined, + }); + + const failures = [...result.errors]; + const attempt = async (what, fn) => { + try { + await fn(); + } catch (err) { + core.warning(`${what}: ${err.message}`); + failures.push(`Could not ${what}: ${err.message}`); + } + }; + + for (const name of result.removeLabels) { + await attempt(`remove label \`${name}\``, () => github.rest.issues.removeLabel({ owner, repo, issue_number, name })); + } + if (result.addLabels.length) { + await attempt(`add ${result.addLabels.map((l) => `\`${l}\``).join(', ')}`, () => + github.rest.issues.addLabels({ owner, repo, issue_number, labels: result.addLabels })); + } + if (result.unassign.length) { + await attempt('unassign', () => github.rest.issues.removeAssignees({ owner, repo, issue_number, assignees: result.unassign })); + } + if (result.assign.length) { + await attempt('assign', async () => { + const { data } = await github.rest.issues.addAssignees({ owner, repo, issue_number, assignees: result.assign }); + const assigned = new Set((data.assignees || []).map((a) => a.login.toLowerCase())); + const missing = result.assign.filter((u) => !assigned.has(u.toLowerCase())); + if (missing.length) { + throw new Error(`${missing.map((u) => `@${u}`).join(', ')} cannot be assigned. They need to comment on this issue first.`); + } + }); + } + if (result.unreviewers.length) { + await attempt('remove review requests', () => + github.rest.pulls.removeRequestedReviewers({ owner, repo, pull_number: issue_number, reviewers: result.unreviewers })); + } + if (result.reviewers.length) { + await attempt('request reviews', () => + github.rest.pulls.requestReviewers({ owner, repo, pull_number: issue_number, reviewers: result.reviewers })); + } + if (result.retest) { + await attempt('re-run failed checks', async () => { + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: issue_number }); + const { data } = await github.rest.actions.listWorkflowRunsForRepo({ owner, repo, head_sha: pr.head.sha, per_page: 100 }); + const failed = data.workflow_runs.filter((r) => ['failure', 'cancelled', 'timed_out'].includes(r.conclusion)); + if (failed.length === 0) { + throw new Error('there are no failed checks on the latest commit.'); + } + for (const r of failed) { + await github.rest.actions.reRunWorkflowFailedJobs({ owner, repo, run_id: r.id }); + } + }); + } + if (result.state) { + await attempt(result.state === 'closed' ? 'close' : 'reopen', () => + github.rest.issues.update({ + owner, repo, issue_number, state: result.state, ...(result.stateReason ? { state_reason: result.stateReason } : {}), + })); + } + // Labels set with GITHUB_TOKEN don't start workflows, so re-run the hold + // check by hand for it to see the new label. + if ([...result.addLabels, ...result.removeLabels].includes(HOLD)) { + await attempt('update the hold check', async () => { + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number: issue_number }); + const { data } = await github.rest.actions.listWorkflowRuns({ + owner, repo, workflow_id: HOLD_WORKFLOW, head_sha: pr.head.sha, per_page: 1, + }); + const [latest] = data.workflow_runs; + if (latest) { + await github.rest.actions.reRunWorkflow({ owner, repo, run_id: latest.id }); + } + }); + } + + if (result.milestone) { + const { title } = result.milestone; + await attempt(title === null ? 'clear the milestone' : 'set the milestone', async () => { + let number = null; + if (title !== null) { + const open = await github.paginate(github.rest.issues.listMilestones, { owner, repo, state: 'open', per_page: 100 }); + const match = open.find((m) => m.title.toLowerCase() === title.toLowerCase()); + if (!match) { + // The title is not echoed back, so a comment can't inject text into the reply. + throw new Error(`no open milestone has that title. Open milestones: ${open.map((m) => `\`${m.title}\``).join(', ') || 'none'}.`); + } + number = match.number; + } + await github.rest.issues.update({ owner, repo, issue_number, milestone: number }); + }); + } + + const changed = result.addLabels.length || result.removeLabels.length || result.assign.length + || result.unassign.length || result.reviewers.length || result.unreviewers.length || result.retest || result.state + || result.milestone; + if (changed && failures.length === 0) { + await github.rest.reactions.createForIssueComment({ owner, repo, comment_id: comment.id, content: '+1' }); + } + if (failures.length) { + await github.rest.reactions.createForIssueComment({ owner, repo, comment_id: comment.id, content: 'confused' }); + const greeting = `@${commenter} thanks! Some commands were not applied`; + // Reply once per person and issue, so repeated commands can't make the + // bot spam the thread; later failures only get the reaction. + if (!maintainer && await alreadyReplied(github, owner, repo, issue_number, greeting)) return; + const lines = failures.map((f) => `- ${f}`).join('\n'); + await github.rest.issues.createComment({ + owner, + repo, + issue_number, + body: `${greeting}:\n\n${lines}\n\nSee the [triage commands](${DOCS_URL}) for details.`, + }); + } +} + +/** + * @param {GitHub} github + * @param {string} owner + * @param {string} repo + * @param {number} issue_number + * @param {string} greeting start of the bot's reply to this commenter + * @returns {Promise<boolean>} + */ +async function alreadyReplied(github, owner, repo, issue_number, greeting) { + const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number, per_page: 100 }); + return comments.some((c) => c.user && c.user.type === 'Bot' && String(c.body).startsWith(greeting)); +} + +/** + * Adds needs-triage to newly opened issues, unless a maintainer already + * triaged them through a label. + * + * @param {{ github: GitHub, context: Context }} args + */ +async function labelNewIssue({ github, context }) { + const { issue } = context.payload; + if (!issue) return; + const labels = (issue.labels || []).map((l) => (typeof l === 'string' ? l : l.name)); + if (labels.some((l) => l.startsWith('triage/'))) return; + await github.rest.issues.addLabels({ + ...context.repo, + issue_number: issue.number, + labels: [NEEDS_TRIAGE], + }); +} + +module.exports = { run, labelNewIssue }; diff --git a/hack/triage/test/parse.test.js b/hack/triage/test/parse.test.js new file mode 100644 index 00000000..e52dc843 --- /dev/null +++ b/hack/triage/test/parse.test.js @@ -0,0 +1,51 @@ +/* + * Copyright The Microcks Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Run with: node --test hack/triage/test/*.test.js + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { parseCommands } = require('../parse'); + +test('parseCommands reads commands at the start of lines', () => { + assert.deepEqual(parseCommands('Looks good.\n/triage accepted\n /kind bug\n/assign @dave'), [ + { name: 'triage', args: ['accepted'] }, + { name: 'kind', args: ['bug'] }, + { name: 'assign', args: ['dave'] }, + ]); +}); + +test('parseCommands ignores code blocks, quotes, inline slashes and paths', () => { + const body = [ + '```', '/close', '```', + '> /kind bug', + 'please run `/assign`', + 'see cmd/test.go', + '/usr/local/bin/microcks', + ].join('\n'); + assert.deepEqual(parseCommands(body), []); +}); + +test('parseCommands ignores commands hidden in HTML comments', () => { + assert.deepEqual(parseCommands('Nice!\n<!--\n/lgtm\n-->\n/kind bug\n<!-- /hold cancel -->\n<!--\n/approve'), [ + { name: 'kind', args: ['bug'] }, + ]); +}); + +test('parseCommands caps the number of commands', () => { + assert.equal(parseCommands(Array(50).fill('/kind bug').join('\n')).length, 10); +}); + diff --git a/hack/triage/test/plan.test.js b/hack/triage/test/plan.test.js new file mode 100644 index 00000000..4f264992 --- /dev/null +++ b/hack/triage/test/plan.test.js @@ -0,0 +1,301 @@ +/* + * Copyright The Microcks Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Run with: node --test hack/triage/test/*.test.js + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { parseCommands } = require('../parse'); +const { plan } = require('../plan'); + +const maintainer = { commenter: 'alice', author: 'bob', isMaintainer: true, isContributor: true, isPullRequest: false, labels: [] }; +// A newcomer without commits in the repository. +const contributor = { ...maintainer, commenter: 'carol', isMaintainer: false, isContributor: false }; +// Someone who already has commits in the repository. +const pastContributor = { ...contributor, commenter: 'dan', isContributor: true }; + +test('/triage accepted replaces needs-triage and other triage labels', () => { + const r = plan(parseCommands('/triage accepted'), { ...maintainer, labels: ['needs-triage', 'triage/needs-information'] }); + assert.deepEqual(r.addLabels, ['triage/accepted']); + assert.deepEqual(r.removeLabels.sort(), ['needs-triage', 'triage/needs-information']); + assert.deepEqual(r.errors, []); +}); + +test('/remove-triage puts needs-triage back', () => { + const r = plan(parseCommands('/remove-triage'), { ...maintainer, labels: ['triage/accepted'] }); + assert.deepEqual(r.removeLabels, ['triage/accepted']); + assert.deepEqual(r.addLabels, ['needs-triage']); +}); + +test('label commands map to existing repository labels', () => { + const r = plan(parseCommands('/kind bug\n/area docs\n/platform windows'), maintainer); + assert.deepEqual(r.addLabels, ['kind/bug', 'component/documentation', 'platform/windows']); +}); + +test('/remove-kind removes only labels that are present', () => { + const r = plan(parseCommands('/remove-kind bug\n/remove-kind task'), { ...maintainer, labels: ['kind/bug'] }); + assert.deepEqual(r.removeLabels, ['kind/bug']); + assert.deepEqual(r.errors, []); +}); + +test('unknown label values are reported', () => { + const r = plan(parseCommands('/kind nonsense\n/kind'), maintainer); + assert.equal(r.addLabels.length, 0); + assert.equal(r.errors.length, 2); + assert.match(r.errors[0], /expects one of/); +}); + +test('newcomers cannot triage or label', () => { + const r = plan(parseCommands('/triage accepted\n/kind bug\n/keep-open\n/good-first-issue'), contributor); + assert.equal(r.addLabels.length, 0); + assert.equal(r.errors.length, 4); + assert.match(r.errors[0], /people who have contributed/); + assert.match(r.errors[2], /only be used by maintainers/); +}); + +test('past contributors can triage and label other people\'s issues', () => { + const r = plan(parseCommands('/triage accepted\n/kind bug\n/area cli\n/good-first-issue'), { ...pastContributor, labels: ['needs-triage'] }); + assert.deepEqual(r.addLabels, ['triage/accepted', 'kind/bug', 'component/cli', 'good first issue']); + assert.deepEqual(r.removeLabels, ['needs-triage']); + assert.deepEqual(r.errors, []); +}); + +test('past contributors cannot triage their own issue', () => { + const r = plan(parseCommands('/triage accepted\n/kind bug'), { ...pastContributor, author: 'dan' }); + assert.equal(r.addLabels.length, 0); + assert.equal(r.errors.length, 2); + assert.match(r.errors[0], /can't be used on your own issue/); +}); + +test('maintainers can triage their own issue', () => { + const r = plan(parseCommands('/triage accepted'), { ...maintainer, author: 'alice' }); + assert.deepEqual(r.addLabels, ['triage/accepted']); +}); + +test('only maintainers can remove labels or use /keep-open', () => { + const labels = ['triage/accepted', 'kind/bug', 'good first issue']; + const r = plan(parseCommands('/remove-triage\n/remove-kind bug\n/remove-good-first-issue\n/keep-open'), { ...pastContributor, labels }); + assert.deepEqual(r.removeLabels, []); + assert.deepEqual(r.addLabels, []); + assert.equal(r.errors.length, 4); + r.errors.forEach((e) => assert.match(e, /only be used by maintainers/)); +}); + +test('/good-first-issue and /help need triage/accepted on issues', () => { + const before = plan(parseCommands('/good-first-issue\n/help'), maintainer); + assert.equal(before.addLabels.length, 0); + assert.equal(before.errors.length, 2); + + const after = plan(parseCommands('/good-first-issue\n/help'), { ...maintainer, labels: ['triage/accepted'] }); + assert.deepEqual(after.addLabels, ['good first issue', 'help wanted']); +}); + +test('triaging and flagging in the same comment works', () => { + const r = plan(parseCommands('/triage accepted\n/good-first-issue'), { ...maintainer, labels: ['needs-triage'] }); + assert.deepEqual(r.addLabels, ['triage/accepted', 'good first issue']); + assert.deepEqual(r.errors, []); +}); + +test('contributors can self-assign only accepted issues', () => { + const pending = plan(parseCommands('/assign'), contributor); + assert.deepEqual(pending.assign, []); + assert.match(pending.errors[0], /not been accepted yet/); + + const accepted = plan(parseCommands('/assign'), { ...contributor, labels: ['triage/accepted'] }); + assert.deepEqual(accepted.assign, ['carol']); + assert.deepEqual(accepted.errors, []); +}); + +test('contributors cannot assign others or pull requests', () => { + const others = plan(parseCommands('/assign @dave'), { ...contributor, labels: ['triage/accepted'] }); + assert.deepEqual(others.assign, []); + assert.match(others.errors[0], /Only maintainers can assign other people/); + + const pr = plan(parseCommands('/assign'), { ...contributor, isPullRequest: true }); + assert.deepEqual(pr.assign, []); + assert.match(pr.errors[0], /pull requests/); +}); + +test('maintainers can assign anyone, before triage', () => { + const r = plan(parseCommands('/assign @dave @erin'), maintainer); + assert.deepEqual(r.assign, ['dave', 'erin']); +}); + +test('anyone can unassign themselves but not others', () => { + assert.deepEqual(plan(parseCommands('/unassign'), contributor).unassign, ['carol']); + const r = plan(parseCommands('/unassign @dave'), contributor); + assert.deepEqual(r.unassign, []); + assert.equal(r.errors.length, 1); +}); + +test('/close and /reopen work for maintainers and the author only', () => { + assert.equal(plan(parseCommands('/close'), maintainer).state, 'closed'); + assert.equal(plan(parseCommands('/reopen'), { ...contributor, commenter: 'bob' }).state, 'open'); + const r = plan(parseCommands('/close'), contributor); + assert.equal(r.state, null); + assert.equal(r.errors.length, 1); +}); + +test('authors can reopen only what they closed themselves', () => { + const author = { ...contributor, commenter: 'bob' }; + assert.equal(plan(parseCommands('/reopen'), { ...author, closedBy: 'bob' }).state, 'open'); + + const r = plan(parseCommands('/reopen'), { ...author, closedBy: 'alice' }); + assert.equal(r.state, null); + assert.match(r.errors[0], /a maintainer closed/); + + assert.equal(plan(parseCommands('/reopen'), { ...maintainer, closedBy: 'erin' }).state, 'open'); +}); + +test('unknown commands are ignored without errors', () => { + const r = plan(parseCommands('/test all\n/meow'), contributor); + assert.deepEqual(r, { + addLabels: [], removeLabels: [], assign: [], unassign: [], reviewers: [], unreviewers: [], retest: false, state: null, stateReason: null, milestone: null, errors: [], + }); +}); + +const prMaintainer = { ...maintainer, isPullRequest: true }; +const prPastContributor = { ...pastContributor, isPullRequest: true }; +const prNewcomer = { ...contributor, isPullRequest: true }; + +test('pull request commands are rejected on issues', () => { + const r = plan(parseCommands('/lgtm\n/approve\n/hold\n/retest\n/cc @dave'), maintainer); + assert.deepEqual(r.addLabels, []); + assert.equal(r.retest, false); + assert.equal(r.errors.length, 5); + r.errors.forEach((e) => assert.match(e, /only be used on pull requests/)); +}); + +test('/lgtm is for maintainers and past contributors, never on their own pull request', () => { + assert.deepEqual(plan(parseCommands('/lgtm'), prPastContributor).addLabels, ['lgtm']); + assert.deepEqual(plan(parseCommands('/lgtm'), prMaintainer).addLabels, ['lgtm']); + + const newcomer = plan(parseCommands('/lgtm'), prNewcomer); + assert.deepEqual(newcomer.addLabels, []); + assert.match(newcomer.errors[0], /people who have contributed/); + + const own = plan(parseCommands('/lgtm'), { ...prMaintainer, author: 'alice' }); + assert.deepEqual(own.addLabels, []); + assert.match(own.errors[0], /your own pull request/); +}); + +test('/approve is maintainer-only, never on their own pull request', () => { + assert.deepEqual(plan(parseCommands('/approve'), prMaintainer).addLabels, ['approved']); + + const own = plan(parseCommands('/approve'), { ...prMaintainer, author: 'alice' }); + assert.deepEqual(own.addLabels, []); + assert.match(own.errors[0], /your own pull request/); + + const r = plan(parseCommands('/approve'), prPastContributor); + assert.deepEqual(r.addLabels, []); + assert.match(r.errors[0], /only be used by maintainers/); +}); + +test('/remove-lgtm, /remove-approve and the cancel spelling are maintainer-only', () => { + const labels = ['lgtm', 'approved']; + const r = plan(parseCommands('/lgtm cancel\n/remove-approve'), { ...prMaintainer, labels }); + assert.deepEqual(r.removeLabels, ['lgtm', 'approved']); + assert.deepEqual(r.addLabels, []); + + const denied = plan(parseCommands('/remove-lgtm\n/approve cancel'), { ...prPastContributor, labels }); + assert.deepEqual(denied.removeLabels, []); + assert.equal(denied.errors.length, 2); +}); + +test('/hold works for participants and the author, /unhold only for maintainers', () => { + assert.deepEqual(plan(parseCommands('/hold'), prPastContributor).addLabels, ['do-not-merge/hold']); + assert.deepEqual(plan(parseCommands('/hold'), { ...prNewcomer, commenter: 'bob' }).addLabels, ['do-not-merge/hold']); + assert.equal(plan(parseCommands('/hold'), prNewcomer).errors.length, 1); + + const held = ['do-not-merge/hold']; + assert.deepEqual(plan(parseCommands('/hold cancel'), { ...prMaintainer, labels: held }).removeLabels, held); + assert.deepEqual(plan(parseCommands('/unhold'), { ...prMaintainer, labels: held }).removeLabels, held); + for (const who of [prPastContributor, { ...prNewcomer, commenter: 'bob' }]) { + const r = plan(parseCommands('/unhold\n/hold cancel'), { ...who, labels: held }); + assert.deepEqual(r.removeLabels, []); + assert.equal(r.errors.length, 2); + } +}); + +test('/priority is maintainer-only and keeps a single priority', () => { + const r = plan(parseCommands('/priority important-soon'), { ...maintainer, labels: ['priority/backlog'] }); + assert.deepEqual(r.addLabels, ['priority/important-soon']); + assert.deepEqual(r.removeLabels, ['priority/backlog']); + + assert.deepEqual(plan(parseCommands('/remove-priority'), { ...maintainer, labels: ['priority/backlog'] }).removeLabels, ['priority/backlog']); + assert.match(plan(parseCommands('/priority urgent'), maintainer).errors[0], /expects one of/); + + const denied = plan(parseCommands('/priority critical-urgent\n/remove-priority'), { ...pastContributor, labels: ['priority/backlog'] }); + assert.deepEqual(denied.addLabels, []); + assert.deepEqual(denied.removeLabels, []); + assert.equal(denied.errors.length, 2); +}); + +test('/close with a reason labels the item and is maintainer-only', () => { + const r = plan(parseCommands('/close wontfix'), maintainer); + assert.equal(r.state, 'closed'); + assert.equal(r.stateReason, 'not_planned'); + assert.deepEqual(r.addLabels, ['status/wontfix']); + + // Pull requests have no state_reason. + const pr = plan(parseCommands('/close duplicate'), prMaintainer); + assert.deepEqual(pr.addLabels, ['status/duplicate']); + assert.equal(pr.stateReason, null); + + const author = plan(parseCommands('/close invalid'), { ...contributor, commenter: 'bob' }); + assert.equal(author.state, null); + assert.match(author.errors[0], /with a reason can only be used by maintainers/); + + const typo = plan(parseCommands('/close dupe'), maintainer); + assert.equal(typo.state, null); + assert.match(typo.errors[0], /expects nothing or one of/); +}); + +test('/milestone is maintainer-only and can be cleared', () => { + assert.deepEqual(plan(parseCommands('/milestone 1.0.3'), maintainer).milestone, { title: '1.0.3' }); + assert.deepEqual(plan(parseCommands('/milestone Next Release'), maintainer).milestone, { title: 'Next Release' }); + assert.deepEqual(plan(parseCommands('/milestone clear'), maintainer).milestone, { title: null }); + assert.deepEqual(plan(parseCommands('/remove-milestone'), maintainer).milestone, { title: null }); + assert.match(plan(parseCommands('/milestone'), maintainer).errors[0], /expects a milestone title/); + + const denied = plan(parseCommands('/milestone 1.0.3\n/remove-milestone'), pastContributor); + assert.equal(denied.milestone, null); + assert.equal(denied.errors.length, 2); +}); + +test('/reopen removes the close reason', () => { + const r = plan(parseCommands('/reopen'), { ...maintainer, labels: ['status/duplicate', 'kind/bug'] }); + assert.equal(r.state, 'open'); + assert.deepEqual(r.removeLabels, ['status/duplicate']); +}); + +test('/retest works for maintainers and the author only', () => { + assert.equal(plan(parseCommands('/retest'), prMaintainer).retest, true); + assert.equal(plan(parseCommands('/retest'), { ...prNewcomer, commenter: 'bob' }).retest, true); + const r = plan(parseCommands('/retest'), prPastContributor); + assert.equal(r.retest, false); + assert.match(r.errors[0], /maintainers or the author/); +}); + +test('/cc and /uncc request and remove reviewers', () => { + const r = plan(parseCommands('/cc @dave @erin\n/uncc @frank'), prPastContributor); + assert.deepEqual(r.reviewers, ['dave', 'erin']); + assert.deepEqual(r.unreviewers, ['frank']); + + assert.match(plan(parseCommands('/cc'), prMaintainer).errors[0], /expects one or more users/); + assert.equal(plan(parseCommands('/cc @dave'), prNewcomer).errors.length, 1); +}); + diff --git a/hack/triage/test/run.test.js b/hack/triage/test/run.test.js new file mode 100644 index 00000000..9fac760c --- /dev/null +++ b/hack/triage/test/run.test.js @@ -0,0 +1,239 @@ +/* + * Copyright The Microcks Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Run with: node --test hack/triage/test/*.test.js + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const { run, labelNewIssue } = require('../run'); + +// run() and labelNewIssue() against a fake GitHub client. + +// labels and closedBy describe the issue as GitHub returns it now, which can +// differ from the event payload. +function fakeGithub({ permission = 'read', assignable = () => true, runs = [], labels = [], closedBy = null, comments = [], milestones = [] } = {}) { + const calls = []; + const record = (name, fn = () => ({ data: {} })) => async (args) => { calls.push([name, args]); return fn(args); }; + const github = { + paginate: async (method, args) => { + if (method === github.rest.issues.listMilestones) { + calls.push(['listMilestones', args]); + return milestones; + } + calls.push(['listComments', args]); + return comments; + }, + rest: { + repos: { + getCollaboratorPermissionLevel: record('permission', () => { + if (permission === null) { const e = new Error('Not Found'); e.status = 404; throw e; } + return { data: { permission } }; + }), + }, + issues: { + get: record('getIssue', () => ({ data: { labels: labels.map((name) => ({ name })), closed_by: closedBy && { login: closedBy } } })), + listMilestones: () => {}, + addLabels: record('addLabels'), + removeLabel: record('removeLabel'), + addAssignees: record('addAssignees', ({ assignees }) => ({ data: { assignees: assignees.filter(assignable).map((login) => ({ login })) } })), + removeAssignees: record('removeAssignees'), + update: record('update'), + createComment: record('createComment'), + }, + reactions: { createForIssueComment: record('reaction') }, + pulls: { + get: record('getPull', () => ({ data: { head: { sha: 'abc123' } } })), + requestReviewers: record('requestReviewers'), + removeRequestedReviewers: record('removeRequestedReviewers'), + }, + actions: { + listWorkflowRunsForRepo: record('listRuns', () => ({ data: { workflow_runs: runs } })), + reRunWorkflowFailedJobs: record('rerun'), + listWorkflowRuns: record('listHoldRuns', () => ({ data: { workflow_runs: [{ id: 42 }] } })), + reRunWorkflow: record('rerunHold'), + }, + }, + }; + return { github, calls }; +} + +const core = { warning: () => {} }; + +function contextFor(body, { login = 'alice', type = 'User', labels = [], pr = false, association = 'NONE' } = {}) { + return { + repo: { owner: 'microcks', repo: 'microcks-cli' }, + payload: { + comment: { id: 99, body, user: { login, type }, author_association: association }, + issue: { + number: 7, + user: { login: 'bob' }, + labels: labels.map((name) => ({ name })), + assignees: [], + ...(pr ? { pull_request: {} } : {}), + }, + }, + }; +} + +test('run applies maintainer commands and reacts with +1', async () => { + const { github, calls } = fakeGithub({ permission: 'write', labels: ['needs-triage'] }); + await run({ github, context: contextFor('/triage accepted\n/kind bug'), core }); + const names = calls.map(([n]) => n); + assert.deepEqual(names, ['permission', 'getIssue', 'removeLabel', 'addLabels', 'reaction']); + assert.deepEqual(calls[3][1].labels, ['triage/accepted', 'kind/bug']); + assert.equal(calls[4][1].content, '+1'); +}); + +test('run treats non-collaborators (404) as contributors and explains', async () => { + const { github, calls } = fakeGithub({ permission: null }); + await run({ github, context: contextFor('/triage accepted', { login: 'carol' }), core }); + const names = calls.map(([n]) => n); + assert.deepEqual(names, ['permission', 'getIssue', 'reaction', 'listComments', 'createComment']); + assert.equal(calls[2][1].content, 'confused'); + assert.match(calls[4][1].body, /@carol thanks! Some commands were not applied/); + assert.match(calls[4][1].body, /triage-commands\.md/); +}); + +test('run lets past contributors triage using the comment author association', async () => { + const { github, calls } = fakeGithub({ permission: 'read', labels: ['needs-triage'] }); + await run({ github, context: contextFor('/triage accepted', { login: 'dan', association: 'CONTRIBUTOR' }), core }); + assert.deepEqual(calls.map(([n]) => n), ['permission', 'getIssue', 'removeLabel', 'addLabels', 'reaction']); + assert.deepEqual(calls[3][1].labels, ['triage/accepted']); +}); + +test('run uses current labels, not the event payload snapshot', async () => { + // A maintainer accepted the issue after this comment's event was created. + const { github, calls } = fakeGithub({ permission: 'read', labels: ['triage/accepted'] }); + await run({ github, context: contextFor('/assign', { login: 'carol', labels: ['needs-triage'] }), core }); + assert.deepEqual(calls.find(([n]) => n === 'addAssignees')[1].assignees, ['carol']); + assert.equal(calls.at(-1)[1].content, '+1'); +}); + +test('run passes who closed the item, so authors cannot reopen a maintainer close', async () => { + const { github, calls } = fakeGithub({ permission: 'read', closedBy: 'alice' }); + await run({ github, context: contextFor('/reopen', { login: 'bob' }), core }); + assert.equal(calls.find(([n]) => n === 'update'), undefined); + assert.match(calls.find(([n]) => n === 'createComment')[1].body, /a maintainer closed/); +}); + +test('run reports users GitHub would not assign', async () => { + const { github, calls } = fakeGithub({ permission: 'admin', assignable: (u) => u !== 'ghost' }); + await run({ github, context: contextFor('/assign @ghost'), core }); + const comment = calls.find(([n]) => n === 'createComment'); + assert.match(comment[1].body, /@ghost cannot be assigned/); +}); + +test('run re-runs only failed workflow runs on the pull request head', async () => { + const runs = [{ id: 1, conclusion: 'failure' }, { id: 2, conclusion: 'success' }, { id: 3, conclusion: 'timed_out' }]; + const { github, calls } = fakeGithub({ permission: 'write', runs }); + await run({ github, context: contextFor('/retest', { pr: true }), core }); + assert.equal(calls.find(([n]) => n === 'listRuns')[1].head_sha, 'abc123'); + assert.deepEqual(calls.filter(([n]) => n === 'rerun').map(([, a]) => a.run_id), [1, 3]); + assert.equal(calls.at(-1)[1].content, '+1'); +}); + +test('run explains when there is nothing to retest', async () => { + const { github, calls } = fakeGithub({ permission: 'write', runs: [{ id: 2, conclusion: 'success' }] }); + await run({ github, context: contextFor('/retest', { pr: true }), core }); + assert.match(calls.find(([n]) => n === 'createComment')[1].body, /no failed checks/); +}); + +test('run requests reviewers on pull requests', async () => { + const { github, calls } = fakeGithub({ permission: 'write' }); + await run({ github, context: contextFor('/cc @dave', { pr: true }), core }); + const req = calls.find(([n]) => n === 'requestReviewers'); + assert.deepEqual(req[1], { owner: 'microcks', repo: 'microcks-cli', pull_number: 7, reviewers: ['dave'] }); +}); + +test('run re-runs the hold check after /hold', async () => { + const { github, calls } = fakeGithub({ permission: 'write' }); + await run({ github, context: contextFor('/hold', { pr: true }), core }); + const list = calls.find(([n]) => n === 'listHoldRuns')[1]; + assert.equal(list.workflow_id, 'hold.yml'); + assert.equal(list.head_sha, 'abc123'); + assert.equal(calls.find(([n]) => n === 'rerunHold')[1].run_id, 42); + assert.equal(calls.at(-1)[1].content, '+1'); +}); + +test('run replies to a non-maintainer only once per issue', async () => { + const earlier = [{ user: { type: 'Bot' }, body: '@carol thanks! Some commands were not applied:\n\n- ...' }]; + const { github, calls } = fakeGithub({ permission: null, comments: earlier }); + await run({ github, context: contextFor('/kind bug', { login: 'carol' }), core }); + assert.equal(calls.find(([n]) => n === 'createComment'), undefined); + assert.equal(calls.at(-1)[0], 'listComments'); + assert.equal(calls.find(([n]) => n === 'reaction')[1].content, 'confused'); +}); + +test('run always replies to maintainers', async () => { + const earlier = [{ user: { type: 'Bot' }, body: '@alice thanks! Some commands were not applied:\n\n- ...' }]; + const { github, calls } = fakeGithub({ permission: 'write', comments: earlier }); + await run({ github, context: contextFor('/kind nonsense'), core }); + assert.ok(calls.find(([n]) => n === 'createComment')); + assert.equal(calls.find(([n]) => n === 'listComments'), undefined); +}); + +test('run closes issues with a reason and labels them', async () => { + const { github, calls } = fakeGithub({ permission: 'write' }); + await run({ github, context: contextFor('/close duplicate'), core }); + assert.deepEqual(calls.find(([n]) => n === 'addLabels')[1].labels, ['status/duplicate']); + const update = calls.find(([n]) => n === 'update')[1]; + assert.equal(update.state, 'closed'); + assert.equal(update.state_reason, 'duplicate'); +}); + +test('run sets an open milestone by title', async () => { + const { github, calls } = fakeGithub({ permission: 'write', milestones: [{ number: 15, title: '1.0.3' }, { number: 17, title: 'Next' }] }); + await run({ github, context: contextFor('/milestone next'), core }); + assert.equal(calls.find(([n]) => n === 'listMilestones')[1].state, 'open'); + assert.equal(calls.find(([n]) => n === 'update')[1].milestone, 17); + assert.equal(calls.at(-1)[1].content, '+1'); +}); + +test('run explains an unknown milestone without echoing the title', async () => { + const { github, calls } = fakeGithub({ permission: 'write', milestones: [{ number: 17, title: 'Next' }] }); + await run({ github, context: contextFor('/milestone **pwned**'), core }); + assert.equal(calls.find(([n]) => n === 'update'), undefined); + const body = calls.find(([n]) => n === 'createComment')[1].body; + assert.match(body, /no open milestone has that title\. Open milestones: `Next`/); + assert.doesNotMatch(body, /pwned/); +}); + +test('run clears the milestone', async () => { + const { github, calls } = fakeGithub({ permission: 'write' }); + await run({ github, context: contextFor('/milestone clear'), core }); + assert.equal(calls.find(([n]) => n === 'listMilestones'), undefined); + assert.equal(calls.find(([n]) => n === 'update')[1].milestone, null); +}); + +test('run ignores bots and comments without commands', async () => { + const bot = fakeGithub({ permission: 'admin' }); + await run({ github: bot.github, context: contextFor('/close', { login: 'dependabot[bot]', type: 'Bot' }), core }); + assert.equal(bot.calls.length, 0); + + const plain = fakeGithub({ permission: 'admin' }); + await run({ github: plain.github, context: contextFor('Thanks for the fix!'), core }); + assert.equal(plain.calls.length, 0); +}); + +test('labelNewIssue adds needs-triage unless already triaged', async () => { + const fresh = fakeGithub(); + await labelNewIssue({ github: fresh.github, context: contextFor('', { labels: ['kind/bug'] }) }); + assert.deepEqual(fresh.calls[0][1].labels, ['needs-triage']); + + const triaged = fakeGithub(); + await labelNewIssue({ github: triaged.github, context: contextFor('', { labels: ['triage/accepted'] }) }); + assert.equal(triaged.calls.length, 0); +}); From ece52f4b94ccefb179d8deac357c67f32c68c833 Mon Sep 17 00:00:00 2001 From: caesarsage <destinyerhabor6@gmail.com> Date: Wed, 4 Nov 2026 10:00:23 +0100 Subject: [PATCH 2/2] ci: make /assign maintainer-only and explain /retest Microcks doesn't assign issues to contributors: the first complete pull request is accepted. Contributors now get a short explanation instead of being assigned, and the welcome message no longer mentions /assign. Document why /retest exists: re-running checks from the Actions tab needs write access. Signed-off-by: caesarsage <destinyerhabor6@gmail.com> --- .github/workflows/welcome-new-users.yml | 2 +- documentation/triage-commands.md | 8 ++---- hack/triage/plan.js | 28 ++++-------------- hack/triage/test/plan.test.js | 38 +++++++------------------ hack/triage/test/run.test.js | 4 +-- 5 files changed, 21 insertions(+), 59 deletions(-) diff --git a/.github/workflows/welcome-new-users.yml b/.github/workflows/welcome-new-users.yml index bf445160..b3e2517d 100644 --- a/.github/workflows/welcome-new-users.yml +++ b/.github/workflows/welcome-new-users.yml @@ -33,7 +33,7 @@ jobs: Thanks and congrats 🎉 for opening your first issue here! Be sure to follow the issue template or please update it accordingly. - 🏷️ A maintainer will triage it soon. If you'd like to work on it, please wait for the `triage/accepted` label, then comment `/assign`. See the [triage commands](https://github.com/microcks/microcks-cli/blob/master/documentation/triage-commands.md). + 🏷️ A maintainer will triage it soon. If you'd like to work on it, please wait for the `triage/accepted` label before opening a pull request. See the [triage commands](https://github.com/microcks/microcks-cli/blob/master/documentation/triage-commands.md). 📢 If you're using Microcks in your organization, please [add your company name to this list](https://github.com/microcks/.github/blob/main/ADOPTERS.md). 🙏 It really helps the project to gain momentum and credibility. It's a small contribution back to the project with a big impact. diff --git a/documentation/triage-commands.md b/documentation/triage-commands.md index e15bd0c0..0fca0f0b 100644 --- a/documentation/triage-commands.md +++ b/documentation/triage-commands.md @@ -6,7 +6,7 @@ Issues and pull requests in this repository can be triaged with Prow-style slash 1. Every new issue gets the `needs-triage` label. 2. A maintainer or contributor checks it and comments `/triage accepted`, or `/triage needs-information` if more details are needed from the reporter. -3. Once an issue has `triage/accepted`, contributors can pick it up with `/assign` and open a pull request that references it. +3. Once an issue has `triage/accepted`, anyone can open a pull request that references it. Issues are not assigned: the first complete pull request is accepted. If one is already open, please collaborate there instead of opening another. Please wait for `triage/accepted` before starting work. It avoids spending time on a fix or feature that the maintainers would not merge. @@ -14,8 +14,6 @@ Please wait for `triage/accepted` before starting work. It avoids spending time | Command | Effect | | --- | --- | -| `/assign` | Assigns yourself. On issues, only once the issue has `triage/accepted`. | -| `/unassign` | Unassigns yourself. | | `/close`, `/reopen` | Closes or reopens an issue or pull request you opened. You can only reopen it if you closed it yourself; if a maintainer closed it, ask them. | ## Commands for contributors and maintainers @@ -42,7 +40,7 @@ Maintainers are people with write access to this repository. They can use every | `/remove-kind`, `/remove-area`, `/remove-platform` | Removes the matching label, for example `/remove-kind bug`. | | `/remove-good-first-issue`, `/remove-help` | Removes `good first issue` or `help wanted`. | | `/keep-open` | Adds `keep-open`, so the stale bot skips the item. `/remove-keep-open` removes it. | -| `/assign @user`, `/unassign @user` | Assigns or unassigns someone else. GitHub only lets you assign people who have commented on the issue or have access to the repository. | +| `/assign [@user]`, `/unassign [@user]` | Assigns or unassigns yourself or someone else, for example for mentorship work. GitHub only lets you assign people who have commented on the issue or have access to the repository. | | `/close`, `/reopen` | Closes or reopens any issue or pull request. `/reopen` also removes the `status/*` label. | | `/close duplicate\|wontfix\|invalid` | Closes with a reason and adds the matching `status/*` label. On issues it also sets GitHub's close reason (duplicate or not planned). | | `/priority critical-urgent\|important-soon\|important-longterm\|backlog` | Sets the `priority/*` label, replacing any other priority. `/remove-priority` removes it. | @@ -59,7 +57,7 @@ These commands only work in pull request comments. `lgtm`, `approved` and `do-no | `/remove-lgtm` (`/lgtm cancel`), `/remove-approve` (`/approve cancel`) | Maintainers | Removes `lgtm` or `approved`. | | `/hold` | Contributors, maintainers and the author | Adds `do-not-merge/hold`. The `hold` check fails while the label is set. | | `/unhold` (`/hold cancel`) | Maintainers | Removes `do-not-merge/hold`. | -| `/retest` | Maintainers and the author | Re-runs the failed checks on the latest commit. | +| `/retest` | Maintainers and the author | Re-runs the failed checks on the latest commit. Re-running from the Actions tab needs write access, so this lets contributors retry a flaky check (for example a network error during `go mod download`) without pushing an empty commit. | | `/cc @user`, `/uncc @user` | Contributors, maintainers and the author | Requests or removes a review from someone. | Commands inside code blocks, quoted lines (`> /kind bug`) and HTML comments are ignored, and unknown commands are ignored silently. If a command can't be applied, the bot reacts with 😕 and explains why, once per person and issue. diff --git a/hack/triage/plan.js b/hack/triage/plan.js index c43021d8..94a3db7f 100644 --- a/hack/triage/plan.js +++ b/hack/triage/plan.js @@ -234,32 +234,14 @@ function plan(commands, ctx) { if (!needMaintainer(name)) break; remove(FLAGS[name.slice('remove-'.length)]); break; - case 'assign': { - const targets = args.length ? args : [ctx.commenter]; - const others = targets.filter((u) => u.toLowerCase() !== ctx.commenter.toLowerCase()); - if (others.length && !ctx.isMaintainer) { - out.errors.push('Only maintainers can assign other people. Use `/assign` on its own to assign yourself.'); - break; - } - if (!ctx.isMaintainer && !ctx.isPullRequest && !labels.has(TRIAGE_ACCEPTED)) { - out.errors.push('This issue has not been accepted yet. A maintainer will triage it first; please wait for the `triage/accepted` label before picking it up.'); - break; - } - if (!ctx.isMaintainer && ctx.isPullRequest) { - out.errors.push('Only maintainers can assign pull requests.'); - break; - } - out.assign.push(...targets); - break; - } + // Microcks doesn't assign issues to contributors: the first complete pull request is accepted. + case 'assign': case 'unassign': { - const targets = args.length ? args : [ctx.commenter]; - const others = targets.filter((u) => u.toLowerCase() !== ctx.commenter.toLowerCase()); - if (others.length && !ctx.isMaintainer) { - out.errors.push('Only maintainers can unassign other people. Use `/unassign` on its own to unassign yourself.'); + if (!ctx.isMaintainer) { + out.errors.push('Issues are not assigned here. Once an issue has `triage/accepted`, open a pull request that references it; the first complete one is accepted.'); break; } - out.unassign.push(...targets); + out[name].push(...(args.length ? args : [ctx.commenter])); break; } case 'close': diff --git a/hack/triage/test/plan.test.js b/hack/triage/test/plan.test.js index 4f264992..ea7c668e 100644 --- a/hack/triage/test/plan.test.js +++ b/hack/triage/test/plan.test.js @@ -109,36 +109,18 @@ test('triaging and flagging in the same comment works', () => { assert.deepEqual(r.errors, []); }); -test('contributors can self-assign only accepted issues', () => { - const pending = plan(parseCommands('/assign'), contributor); - assert.deepEqual(pending.assign, []); - assert.match(pending.errors[0], /not been accepted yet/); - - const accepted = plan(parseCommands('/assign'), { ...contributor, labels: ['triage/accepted'] }); - assert.deepEqual(accepted.assign, ['carol']); - assert.deepEqual(accepted.errors, []); -}); - -test('contributors cannot assign others or pull requests', () => { - const others = plan(parseCommands('/assign @dave'), { ...contributor, labels: ['triage/accepted'] }); - assert.deepEqual(others.assign, []); - assert.match(others.errors[0], /Only maintainers can assign other people/); - - const pr = plan(parseCommands('/assign'), { ...contributor, isPullRequest: true }); - assert.deepEqual(pr.assign, []); - assert.match(pr.errors[0], /pull requests/); -}); - -test('maintainers can assign anyone, before triage', () => { - const r = plan(parseCommands('/assign @dave @erin'), maintainer); - assert.deepEqual(r.assign, ['dave', 'erin']); +test('non-maintainers cannot assign or unassign, even on accepted issues', () => { + for (const body of ['/assign', '/assign @dave', '/unassign', '/unassign @dave']) { + const r = plan(parseCommands(body), { ...contributor, labels: ['triage/accepted'] }); + assert.deepEqual([...r.assign, ...r.unassign], []); + assert.match(r.errors[0], /Issues are not assigned here/); + } }); -test('anyone can unassign themselves but not others', () => { - assert.deepEqual(plan(parseCommands('/unassign'), contributor).unassign, ['carol']); - const r = plan(parseCommands('/unassign @dave'), contributor); - assert.deepEqual(r.unassign, []); - assert.equal(r.errors.length, 1); +test('maintainers can assign and unassign anyone, before triage', () => { + assert.deepEqual(plan(parseCommands('/assign @dave @erin'), maintainer).assign, ['dave', 'erin']); + assert.deepEqual(plan(parseCommands('/assign'), maintainer).assign, [maintainer.commenter]); + assert.deepEqual(plan(parseCommands('/unassign @dave'), maintainer).unassign, ['dave']); }); test('/close and /reopen work for maintainers and the author only', () => { diff --git a/hack/triage/test/run.test.js b/hack/triage/test/run.test.js index 9fac760c..29c41a30 100644 --- a/hack/triage/test/run.test.js +++ b/hack/triage/test/run.test.js @@ -117,8 +117,8 @@ test('run lets past contributors triage using the comment author association', a test('run uses current labels, not the event payload snapshot', async () => { // A maintainer accepted the issue after this comment's event was created. const { github, calls } = fakeGithub({ permission: 'read', labels: ['triage/accepted'] }); - await run({ github, context: contextFor('/assign', { login: 'carol', labels: ['needs-triage'] }), core }); - assert.deepEqual(calls.find(([n]) => n === 'addAssignees')[1].assignees, ['carol']); + await run({ github, context: contextFor('/help', { login: 'carol', labels: ['needs-triage'], association: 'CONTRIBUTOR' }), core }); + assert.deepEqual(calls.find(([n]) => n === 'addLabels')[1].labels, ['help wanted']); assert.equal(calls.at(-1)[1].content, '+1'); });