diff --git a/.github/owners b/.github/owners new file mode 100644 index 0000000..189efa1 --- /dev/null +++ b/.github/owners @@ -0,0 +1,6 @@ +# Owners: the people who decide what the maintainer works on. One GitHub login per line. +# +# The maintainer agent and `issue-to-pr.yml` work only on an issue an owner has labelled +# `maintainer`. `issue-triage.yml` tags these owners when a new issue needs that decision. +# Read from `main`, and on `issue-to-pr.yml`'s forbidden paths, so a PR cannot add itself. +bbertucc diff --git a/.github/scripts/ask-owners.sh b/.github/scripts/ask-owners.sh new file mode 100755 index 0000000..7102eef --- /dev/null +++ b/.github/scripts/ask-owners.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# Usage: ask-owners.sh +# +# Tags the owners in .github/owners once on an open issue that no owner has labelled `maintainer`, +# so they can decide whether the maintainer should work on it. Posts nothing on a closed issue, on +# one labelled `maintainer` or with one of issue-to-pr.yml's SKIP_LABELS, or on one it already asked on. +set -euo pipefail + +n="$1" +here="$(dirname "$0")" +marker='' + +gh issue view "$n" --json state,labels,comments > /tmp/ask-owners.json +if [ "$(jq -r .state /tmp/ask-owners.json)" != "OPEN" ]; then + echo "#$n is not open — not asking." + exit 0 +fi +skip=$(jq -r '[.labels[].name] | map(select(IN("maintainer", "wontfix", "invalid", "duplicate", "question", "no-auto-pr"))) | join(", ")' /tmp/ask-owners.json) +if [ -n "$skip" ]; then + echo "#$n is labelled $skip — not asking." + exit 0 +fi +if jq -e --arg m "$marker" 'any(.comments[]; .body | contains($m))' /tmp/ask-owners.json >/dev/null; then + echo "#$n was already asked on — not asking again." + exit 0 +fi + +owners=$(grep -v -e '^[[:space:]]*#' -e '^[[:space:]]*$' "$here/../owners" | sed 's/^/@/' | paste -sd' ' -) || true +if [ -z "$owners" ]; then + echo ".github/owners lists no one — not asking." + exit 0 +fi +{ + printf '%s: this issue is ready for an owner to review. The maintainer works on it only after an owner adds the `maintainer` label.\n\n' "$owners" + printf '%s\n' "$marker" +} > /tmp/ask-owners.md +gh issue comment "$n" --body-file /tmp/ask-owners.md diff --git a/.github/scripts/owner-approved.sh b/.github/scripts/owner-approved.sh new file mode 100755 index 0000000..4581019 --- /dev/null +++ b/.github/scripts/owner-approved.sh @@ -0,0 +1,30 @@ +#!/usr/bin/env bash +# Usage: owner-approved.sh +# +# Exits 0 and prints the owner's login when the last `maintainer` label event on the issue is an +# add by an owner in .github/owners. Exits 1 otherwise, including when the events can't be read. +# It does not read the current labels; issue-to-pr.yml checks those. +# +# Only accounts with triage access or higher can label an issue. The label event's actor is checked +# too, because that access is wider than the owners list. +set -euo pipefail + +n="$1" +repo="${GITHUB_REPOSITORY:-$(gh repo view --json nameWithOwner --jq .nameWithOwner)}" +owners_file="$(dirname "$0")/../owners" + +last=$(gh api --paginate "repos/$repo/issues/$n/events" \ + | jq -r '.[] | select((.event == "labeled" or .event == "unlabeled") and .label.name == "maintainer") + | "\(.event) \(.actor.login)"' \ + | tail -n 1) + +case "$last" in + "labeled "*) login="${last#labeled }" ;; + *) exit 1 ;; +esac + +if grep -v '^[[:space:]]*#' "$owners_file" | grep -qxF "$login"; then + echo "$login" + exit 0 +fi +exit 1 diff --git a/.github/workflows/code-review.yml b/.github/workflows/code-review.yml index e33847a..fa67773 100644 --- a/.github/workflows/code-review.yml +++ b/.github/workflows/code-review.yml @@ -1038,7 +1038,8 @@ jobs: 9. **CI and workflow security — conditional, and when it applies it comes FIRST.** Skip this item entirely unless the diff touches `.github/workflows/**`, - `.github/scripts/**` or `.github/actions/**`. `.github/scripts/` counts + `.github/scripts/**`, `.github/actions/**` or `.github/owners`, which decides + whose `maintainer` label sends an issue to `issue-to-pr.yml`. `.github/scripts/` counts because a workflow may keep part of itself there — GitHub refuses a `run:` block past 21000 characters, so `issue-triage.yml` holds its enforcement step in `.github/scripts/triage-decide.sh`, and that file decides whether an diff --git a/.github/workflows/issue-to-pr.yml b/.github/workflows/issue-to-pr.yml index 153809a..3175aa4 100644 --- a/.github/workflows/issue-to-pr.yml +++ b/.github/workflows/issue-to-pr.yml @@ -25,6 +25,9 @@ name: Issue to PR # "Claims" means a closing keyword or an `issue-` branch, not a passing `#` # in prose; see `Pick an issue` for why, and for every reason a run is skipped. # +# It works only on an issue an owner in .github/owners has labelled `maintainer`, +# a dispatch by number included. issue-triage.yml tags the owners for that call. +# # Requires the same two repo settings as code-review.yml, and nothing new: # - secret AWS_BEDROCK_ROLE_ARN — the OIDC role. Its trust policy covers this # repo's `refs/heads/*` subjects, which is what a `schedule` or @@ -316,7 +319,8 @@ jobs: # already made the "is this worth doing" call, so skip labels and a # past rejection do not veto them — asking again by hand is exactly # how you overrule a rejection. A second PR for an issue that is - # still in the queue is just a mess, so that one still applies. + # still in the queue is just a mess, so that one still applies. So + # does the owner's `maintainer` label, checked below for both paths. if grep -qxF "$FORCED_ISSUE" /tmp/open-auto-issues.txt; then echo "should_run=false" >> "$GITHUB_OUTPUT" printf 'Skipped: issue #%s already has an open `iris-auto` PR.\n' \ @@ -366,6 +370,23 @@ jobs: echo "forced=false" >> "$GITHUB_OUTPUT" fi + # Only issues an owner approved: an owner in .github/owners added the `maintainer` label. + # This applies to a dispatch by number too, and fails closed if the events can't be read. + # The label filter goes first, so only labelled issues cost an events call. + jq '[.[] | select(any(.labels[]; .name == "maintainer"))]' /tmp/candidates.json > /tmp/labelled.json + jq -r '.[].number' /tmp/labelled.json | while read -r n; do + if .github/scripts/owner-approved.sh "$n" >/dev/null; then echo "$n"; fi + done | jq -R 'tonumber' | jq -s . > /tmp/approved.json + jq --slurpfile ok /tmp/approved.json '[.[] | select(.number as $n | $ok[0] | index($n))]' \ + /tmp/labelled.json > /tmp/candidates-approved.json + mv /tmp/candidates-approved.json /tmp/candidates.json + if [ -n "${FORCED_ISSUE:-}" ] && [ "$(jq length /tmp/candidates.json)" -eq 0 ]; then + echo "should_run=false" >> "$GITHUB_OUTPUT" + printf 'Skipped: no owner has labelled issue #%s `maintainer`.\n' "$FORCED_ISSUE" >> "$GITHUB_STEP_SUMMARY" + echo "::error::Issue #$FORCED_ISSUE needs the \`maintainer\` label from an owner in .github/owners." + exit 1 + fi + COUNT=$(jq length /tmp/candidates.json) echo "candidate issues: $COUNT" if [ "$COUNT" -eq 0 ]; then @@ -379,8 +400,8 @@ jobs: echo echo 'Every open issue is linked by an open pull request (anyone'"'"'s — see the table' echo 'above if there is one), already attempted by this workflow (open or' - echo 'previously-rejected PR), or carries a skip label. Nothing was run — no model' - echo 'call, no PR, no comment.' + echo 'previously-rejected PR), carries a skip label, or has no `maintainer` label' + echo 'from an owner. Nothing was run — no model call, no PR, no comment.' } >> "$GITHUB_STEP_SUMMARY" echo "::notice::No eligible open issue — nothing to do." exit 0 @@ -902,7 +923,7 @@ jobs: # an issue closes — into `.github/scripts/triage-decide.sh`. A privilege # boundary drawn at a directory name follows the privilege, not the name, # so anywhere a workflow's own logic lives belongs in this pattern. - FORBIDDEN='^(\.github/workflows/|\.github/scripts/|\.github/CODEOWNERS|LICENSE|infra/|\.env)' + FORBIDDEN='^(\.github/workflows/|\.github/scripts/|\.github/CODEOWNERS|\.github/owners|LICENSE|infra/|\.env)' RESULT=/tmp/iris-auto-result.json CLAIMED_PR="" diff --git a/.github/workflows/issue-triage.yml b/.github/workflows/issue-triage.yml index 1eadb6e..3922a5d 100644 --- a/.github/workflows/issue-triage.yml +++ b/.github/workflows/issue-triage.yml @@ -1015,3 +1015,24 @@ jobs: # `.github/scripts/` is also in `issue-to-pr.yml`'s forbidden paths and in # `code-review.yml`'s CI-security gate, for the same reason. run: .github/scripts/triage-decide.sh + + # Tags the owners in .github/owners on an issue triage left open, so they can decide whether the + # maintainer should work on it. Its own job so that none of triage's early exits skip it, and it + # waits for triage to succeed, so a failed triage doesn't use up the one ask. + ask-owners: + needs: triage + if: ${{ needs.triage.result == 'success' && inputs.dry_run != true }} + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + issues: write + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + - name: Ask the owners + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + NUMBER: ${{ github.event.issue.number || inputs.issue_number }} + run: .github/scripts/ask-owners.sh "$NUMBER" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e2b0ec..499446e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -21,13 +21,14 @@ By participating you agree to our [Code of Conduct](CODE_OF_CONDUCT.md). one per content type". `chartDataAgent.md` is the shape that earns its place. - **Code** — bug fixes and improvements via pull request. -A well-written issue may get a pull request without you doing anything else. A scheduled workflow -ranks the open issues Sun–Wed and opens one PR for the most pressing one it can finish well +A well-written issue may get a pull request without you doing anything else. An owner first labels +it `maintainer` (a bot tags them when it's time). A scheduled workflow then ranks those issues +Sun–Wed and opens one PR for the most pressing one it can finish well ([details](docs/ci.md#scheduled-issue-triage)) — accessibility barriers rank first, and small user-visible fixes reported against the demo rank well because they review cleanly. It never touches an issue labelled `no-auto-pr`, never files a second PR for an issue it has already tried, and stops entirely when nothing is eligible. If you'd rather own the fix yourself, say so on the -issue and add that label. +issue and ask for the `no-auto-pr` label. **You get the credit for it.** A PR from that workflow names you in its body and carries a `Co-authored-by` trailer for your account on the commit, so the merged commit is attributed to you diff --git a/docs/ci.md b/docs/ci.md index 4a18193..d584513 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -112,6 +112,8 @@ that human is reading, not whether they read it. opened or reopened**. It reads the new issue, finds the open issue it most resembles, and — only when a second, independent session fails to refute the claim — closes the new one as a duplicate with a comment naming the survivor. Anything short of that is commented and reported, never closed. +An issue it leaves open, with no skip label, gets one comment tagging the owners in +`.github/owners`, asking for the `maintainer` label. It exists because the dedupe already in the app cannot do this, and was never trying to. `src/github/issue.ts` refuses to file an `Agent update proposal:` whose title exactly matches an @@ -261,6 +263,10 @@ UTC**. It reads the open issues, ranks them by what most improves Iris, and open **one** pull request for the top issue it can finish well, with a review requested from **@bbertucc**. +It works only on an issue an owner listed in [`.github/owners`](../.github/owners) has labelled +`maintainer`, a dispatch by number included. [Closing duplicate issues](#closing-duplicate-issues) +tags the owners on each new issue it leaves open, so they can make that call. + [Automated code review](#automated-code-review) raised the ceiling on how much review this maintainership can absorb; this spends some of that headroom on the other side of the same bottleneck — issues that are correct, small, and never picked up. A reported barrier that sits open @@ -378,8 +384,8 @@ A deployment nobody else runs must never be able to turn this project's `main` r [`.github/workflows/quality-report.yml`](../.github/workflows/quality-report.yml) runs **Saturdays at 20:00 UTC**. It reads `GET /v1/quality` on a live deployment, compares a handful of rates against thresholds held in that workflow file, and opens one issue per crossed threshold. -[Scheduled issue triage](#scheduled-issue-triage) then ranks those issues with everything else and -may open a PR against one. +Once an owner labels one `maintainer`, [Scheduled issue triage](#scheduled-issue-triage) ranks it +with everything else and may open a PR against it. Everything before this depended on somebody typing. An issue, or a session's feedback — the loop is good, but a person has to start it. Meanwhile Iris grades itself on every single run: how many diff --git a/test/owner-gate.test.ts b/test/owner-gate.test.ts new file mode 100644 index 0000000..9ec43a7 --- /dev/null +++ b/test/owner-gate.test.ts @@ -0,0 +1,100 @@ +// The maintainer works on an issue only after an owner labels it `maintainer`. These run the two +// scripts that enforce and announce that against a fake `gh`, and pin the workflow lines that call them. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { spawnSync } from "node:child_process"; +import { copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync, chmodSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { parse } from "yaml"; + +const ROOT = join(import.meta.dirname, ".."); +const SCRIPTS = join(ROOT, ".github", "scripts"); + +const FAKE_GH = `#!/usr/bin/env bash +set -euo pipefail +case "$1 $2" in + "api --paginate") [ -n "\${FAKE_FAIL:-}" ] && exit 1; cat "$FAKE_DIR/events.json" ;; + "issue view") cat "$FAKE_DIR/issue.json" ;; + "issue comment") cp "$5" "$FAKE_DIR/posted.md" ;; + *) echo "fake gh: unexpected $*" >&2; exit 2 ;; +esac +`; + +// `owners` swaps in an owners file by running a copy of the script from the temp dir. +function run(script: string, files: Record, env: Record = {}, owners?: string) { + const dir = mkdtempSync(join(tmpdir(), "owner-gate-")); + try { + writeFileSync(join(dir, "gh"), FAKE_GH); + chmodSync(join(dir, "gh"), 0o755); + for (const [name, value] of Object.entries(files)) writeFileSync(join(dir, name), JSON.stringify(value)); + let path = join(SCRIPTS, script); + if (owners !== undefined) { + mkdirSync(join(dir, "scripts")); + path = join(dir, "scripts", script); + copyFileSync(join(SCRIPTS, script), path); + writeFileSync(join(dir, "owners"), owners); + } + const r = spawnSync(path, ["7"], { + encoding: "utf8", + env: { ...process.env, PATH: `${dir}:${process.env.PATH}`, FAKE_DIR: dir, GITHUB_REPOSITORY: "o/r", ...env }, + }); + const posted = existsSync(join(dir, "posted.md")) ? readFileSync(join(dir, "posted.md"), "utf8") : null; + return { status: r.status, stdout: r.stdout.trim(), posted }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +const ev = (event: string, login: string, label = "maintainer") => ({ event, actor: { login }, label: { name: label } }); + +test("an issue is approved only while an owner's `maintainer` label is on it", () => { + const approved = (events: unknown[], env?: Record) => run("owner-approved.sh", { "events.json": events }, env); + assert.deepEqual(approved([ev("labeled", "bbertucc")]), { status: 0, stdout: "bbertucc", posted: null }); + assert.equal(approved([ev("labeled", "someone-else")]).status, 1, "not an owner"); + assert.equal(approved([ev("labeled", "bbertucc"), ev("unlabeled", "bbertucc")]).status, 1, "removed again"); + assert.equal(approved([ev("labeled", "bbertucc", "bug")]).status, 1, "another label"); + assert.equal(approved([ev("labeled", "someone-else"), ev("unlabeled", "x"), ev("labeled", "bbertucc")]).status, 0); + assert.equal(approved([ev("labeled", "bbertucc"), ev("labeled", "someone-else")]).status, 1, "the last label event decides"); + assert.equal(approved([]).status, 1); + assert.equal(approved([ev("labeled", "bbertucc")], { FAKE_FAIL: "1" }).status, 1, "fails closed"); +}); + +test("triage tags the owners once on an open issue no owner has approved", () => { + const ask = (issue: unknown) => run("ask-owners.sh", { "issue.json": issue }); + const open = { state: "OPEN", labels: [], comments: [] }; + const first = ask(open); + assert.equal(first.status, 0); + assert.match(first.posted ?? "", /^@bbertucc: .*`maintainer` label/); + assert.match(first.posted ?? "", //); + assert.equal(ask({ ...open, state: "CLOSED" }).posted, null); + for (const name of ["maintainer", "duplicate", "wontfix", "invalid", "question", "no-auto-pr"]) { + assert.equal(ask({ ...open, labels: [{ name }] }).posted, null, name); + } + assert.equal(ask({ ...open, labels: [{ name: "bug" }] }).posted !== null, true); + assert.equal(ask({ ...open, comments: [{ body: first.posted }] }).posted, null, "already asked"); + const none = run("ask-owners.sh", { "issue.json": open }, {}, "# no one\n\n"); + assert.deepEqual([none.status, none.posted], [0, null], "no owners to ask"); + assert.match(run("ask-owners.sh", { "issue.json": open }, {}, "# x\na\n \nb\n").posted ?? "", /^@a @b: /); +}); + +test("issue-to-pr keeps only approved issues, and triage asks once triage succeeds", () => { + const itp = parse(readFileSync(join(ROOT, ".github", "workflows", "issue-to-pr.yml"), "utf8")); + const pick: string = itp.jobs.propose.steps.find((s: { id?: string }) => s.id === "triage").run; + const built = pick.lastIndexOf("> /tmp/candidates.json"); + const label = pick.indexOf('select(any(.labels[]; .name == "maintainer"))'); + const gate = pick.indexOf(".github/scripts/owner-approved.sh"); + const counted = pick.indexOf("COUNT=$(jq length /tmp/candidates.json)"); + assert.ok(built < label && label < gate, "the current label is checked first, after both paths build the candidates"); + assert.ok(gate < counted, "before they are counted"); + assert.match(readFileSync(join(ROOT, ".github", "workflows", "issue-to-pr.yml"), "utf8"), /FORBIDDEN='[^']*\\\.github\/owners/); + const skip = pick.match(/SKIP_LABELS="([^"]*)"/)![1]!.split(" "); + const asks = readFileSync(join(SCRIPTS, "ask-owners.sh"), "utf8"); + for (const name of skip) assert.ok(asks.includes(`"${name}"`), `ask-owners skips ${name} too`); + + const triage = parse(readFileSync(join(ROOT, ".github", "workflows", "issue-triage.yml"), "utf8")); + const job = triage.jobs["ask-owners"]; + assert.equal(job.needs, "triage"); + assert.match(job.if, /needs\.triage\.result == 'success'/); + assert.match(job.steps.at(-1).run, /^\.github\/scripts\/ask-owners\.sh /); +});