This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8678-b76048948b64e9aa87509c627700f5573c955508 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 2cd44ee0d8219a7cc0f16e8c4ee869c0bad9feec Author: Xinyuan Lin <[email protected]> AuthorDate: Wed Oct 7 05:52:38 2026 +0000 ci: flag PRs that close an issue someone else claimed (#8678) ### What changes were proposed in this PR? Adds a check to `pr-assignment.yml` that flags a PR closing an issue someone else has already claimed. It also stops the merge-time credit step from unassigning a claimant whose own PR is still open. ``` Before: #8267 open (claimant) ── #8339 "Closes #8149" ── (silence) ── #8339 merges ── claimant unassigned from #8149, #8267 left with 0 changed files After: #8267 open (claimant) ── #8339 "Closes #8149" ── comment on #8339 names the claimant + #8267 ── #8339 merges ── claimant stays assigned while #8267 is open ``` | Where | Change | | --- | --- | | New step "Flag linked issues someone else has claimed" (PR `opened` / `edited`) | For each open same-repo issue in `closingIssuesReferences`, flags it when **(a)** it has an assignee other than the PR opener, or **(b)** its `closedByPullRequestsReferences` lists another `OPEN` PR by a different author. Posts or updates one `<!-- texera:issue-claim-conflict -->` comment with an issue / claimant / PR table, and deletes the comment once nothing matches. If overlapping runs each post a copy, every run keeps the oldest and deletes the others, so they end up keeping the same comment (`claimCommentChanges`). A run can also read the PR before an edit and write after a newer run has finished, so after any write it reads the PR again and repeats until a pass changes nothing (`syncClaimComment`, at most 3 passes). The last run to write is then also the last to read, and it corrects a stale write. | | "Credit issue assignees on PR merge" | Keeps an assignee who has their own open PR on the issue. If that PR later closes without merging, the existing close-without-merge step still unassigns them. | | `.github/scripts/issue-claims.js` | The decision logic, which both steps `require`. It is checked out from the base branch, which is safe under `pull_request_target`. The workflow now declares `contents: read` for that checkout. | **(b) is needed on its own.** GitHub silently drops the opener self-assign for an outside contributor who never commented on the issue. For example, #6674 shows no assignee but has the open PR #8603. **It posts a comment and does not fail a check:** | Reason | Detail | | --- | --- | | Both PRs match | The claimant's own PR matches the rule too: #8267 would flag #8339 back. A failing check would also land on the person the rule protects. | | Some takeovers are agreed | Only a person can tell a duplicate from an agreed takeover. One of the 2 PRs it would flag today (below), #8484, is an agreed takeover, and a failing check would block it. | | It would go stale | The check runs again only when this PR is opened or edited. Closing the other PR or unassigning someone doesn't re-run it, so a failed check would stay red until someone edits the PR. | On its first post, the comment @-mentions the claimant, so they get a notification. The #8149 claimant never got one. Closed issues are skipped, because the risk is two PRs in progress at once. Both queries read up to 100 linked PRs per issue. That is the GraphQL maximum; no issue in this repo has more than 5. There is no concurrency group: with the default queue, a group cancels a waiting run, and the body-edit unassign and the merge credit need every event. One known limitation: the comment is refreshed only when this PR is edited, not when the other PR closes. ### Any related issues, documentation, discussions? Closes #8676 Complements #8675, which adds an AGENTS.md rule to check claims before coding. This PR catches the cases that rule misses. ### How was this PR tested? Unit tests: 32 pass. Two of them replay the #8149 and #6674 incidents as GraphQL fixtures. ```bash bash .github/scripts/test_issue_claims.sh # tests 42, pass 42, fail 0 ``` The `infra` job picks these up through its `test_*.sh` discovery. Mutation check: reverting the credit fix fails the #8149 merge test, and dropping (b) fails the #6674 test. For `claimCommentChanges`, removing any one of the id sort, the Bot-only filter, the deletion of extra copies or the unchanged-body check fails at least one test. For `syncClaimComment`, limiting it to one pass fails both delayed-run tests. `pull_request_target` runs the workflow from `main`, so this PR can't run its own steps. Instead, I ran both edited `github-script` steps, extracted from the YAML, in a local harness. GraphQL calls went to the live API (read-only). Every REST write was recorded, not sent. | Scenario | Result | | --- | --- | | #8339 opened, fixture = #8149 as of 2026-09-02 | posts 1 comment: `#8149 · <claimant> (assignee) · #8267` | | edited, with an older bot comment and a human comment quoting the marker | updates only the bot comment | | edited again, comment already current | no write | | edited after `Closes #8149` is removed | deletes the bot comment; the human comment is left alone | | live: #8603 opened (#6674) | no conflict, no writes | | live: #8484 edited | posts 1 comment naming the #6939 assignee | | credit step, fixture = #8339's merge on 2026-09-25 | #8149 keeps its claimant (`kept (own open PR)=[…]`); an assignee without a PR on #8338 is still removed | | credit step, live query on #8339 | query valid, no writes | | race: this run posts, then its re-list shows an older copy from another run | deletes its own copy (and first updates the older one if it is stale) | | re-list after posting fails, or a delete returns 404 | logs a warning and stops; a 404 skips only that one delete | | edited, with two copies left by an earlier race | updates the oldest, deletes the other | | delayed run: reads the claim, a newer run removes it, then the delayed run posts | its re-read finds nothing to flag and deletes its own post | | delayed run: writes old text over a newer run's comment | its re-read restores the newer text | | live: guard and credit queries with `first: 100` | both valid against the schema | I also modelled two racing runs against one shared comment list and tried every ordering of their API calls, using the real `claimCommentChanges`. Whenever both runs see the same state, exactly one up-to-date comment is left; 104 orderings for an open followed by a quick edit. When both runs find nothing to flag, no comment is left. Dry run over every open PR: 218 open, 188 with closing references, **2 flagged**. Both are real claims: | PR | Issue | Claim | | --- | --- | --- | | #5558 (draft) | #5422 | the issue's author assigned themselves | | #8484 | #6939 | a `/take` that was never released; a maintainer later told #8484's author to go ahead | ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5.5) --------- Co-authored-by: Claude Opus 5.5 <[email protected]> --- .github/scripts/issue-claims.js | 206 +++++++++++++++ .github/scripts/issue-claims.test.js | 494 +++++++++++++++++++++++++++++++++++ .github/scripts/test_issue_claims.sh | 29 ++ .github/workflows/pr-assignment.yml | 109 +++++++- 4 files changed, 830 insertions(+), 8 deletions(-) diff --git a/.github/scripts/issue-claims.js b/.github/scripts/issue-claims.js new file mode 100644 index 0000000000..4659183fe5 --- /dev/null +++ b/.github/scripts/issue-claims.js @@ -0,0 +1,206 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +// Issue-claim decisions for .github/workflows/pr-assignment.yml. The workflow +// runs the GraphQL queries and makes every API call; these functions only +// decide, so test_issue_claims.sh can unit-test them. Each `issue` is a +// `closingIssuesReferences` node carrying `state`, `repository`, `assignees` +// and `closedByPullRequestsReferences`, as the workflow's queries fetch it; +// `comments` are the PR's REST issue comments. + +"use strict"; + +const CLAIM_MARKER = "<!-- texera:issue-claim-conflict -->"; + +// Open same-repo PRs, other than `prNumber`, that also close `issue`, as +// author login -> [PR numbers]. A deleted account's PR has a null author, +// which GitHub displays as "ghost". +function otherOpenPrs(issue, repo, prNumber) { + const byAuthor = new Map(); + for (const p of issue.closedByPullRequestsReferences.nodes) { + if (p.state !== "OPEN" || p.number === prNumber || p.repository.nameWithOwner !== repo) { + continue; + } + const login = p.author?.login ?? "ghost"; + byAuthor.set(login, [...(byAuthor.get(login) || []), p.number]); + } + return byAuthor; +} + +// Open same-repo issues this PR closes that someone other than `opener` has +// claimed, either by being assigned or through an open PR of their own. The +// PR half is needed on its own: GitHub silently drops the opener self-assign +// for outside contributors who never commented on the issue, leaving their +// PR as the only trace of the claim. +// Returns [{ issue, claimants: [{ login, assigned, prs }] }]. +function findClaimConflicts(issues, { repo, prNumber, opener }) { + const conflicts = []; + for (const issue of issues) { + if (issue.state !== "OPEN" || issue.repository.nameWithOwner !== repo) continue; + const claimants = new Map(); + for (const { login } of issue.assignees.nodes) { + if (login !== opener) claimants.set(login, { login, assigned: true, prs: [] }); + } + for (const [login, prs] of otherOpenPrs(issue, repo, prNumber)) { + if (login !== opener) claimants.set(login, { login, assigned: claimants.has(login), prs }); + } + if (claimants.size) conflicts.push({ issue: issue.number, claimants: [...claimants.values()] }); + } + return conflicts; +} + +function renderClaimComment(conflicts) { + const rows = conflicts.flatMap(({ issue, claimants }) => + claimants.map(({ login, assigned, prs }) => { + const who = `@${login}${assigned ? " (assignee)" : ""}`; + return `| #${issue} | ${who} | ${prs.map((n) => `#${n}`).join(", ") || "—"} |`; + }), + ); + return [ + CLAIM_MARKER, + "### Linked issue already claimed", + "", + "This PR closes an issue that someone else has already claimed:", + "", + "| Issue | Claimed by | Their open PR |", + "| --- | --- | --- |", + ...rows, + "", + "Please check with them before this merges. Merging closes the issue, and any " + + "work they have in progress is left with nothing to fix.", + "", + "- If this PR takes over, say so here. Credit any of their work it includes, for " + + "example with a `Co-authored-by:` trailer.", + "- If their claim is stale, they can release it with `/untake`, or a maintainer " + + "can unassign them.", + "- Otherwise, remove that issue's closing keyword from this PR's description.", + "", + "_Refreshed whenever this PR is edited; deleted once nothing here applies._", + ].join("\n"); +} + +// Assignee changes for `issue` when PR `prNumber` merges: the merged PR's +// authors (`credited`) replace the current assignees, except that anyone who +// still has their own open PR on the issue stays assigned. Unassigning them +// would hide a claim that is still in review (#8149 lost its /take claimant +// this way). If that PR later closes unmerged, the close-without-merge step +// unassigns them. +function creditChanges(issue, { repo, prNumber, credited }) { + const current = issue.assignees.nodes.map((n) => n.login); + const stillOpen = otherOpenPrs(issue, repo, prNumber); + const uncredited = current.filter((l) => !credited.includes(l)); + return { + current, + toRemove: uncredited.filter((l) => !stillOpen.has(l)), + toAdd: credited.filter((l) => !current.includes(l)), + kept: uncredited.filter((l) => stillOpen.has(l)), + }; +} + +// What to do with the PR's comments so that exactly one claim comment holding +// `body` is left, or none when `body` is null. Overlapping opened/edited runs +// can each post a copy before seeing the other's, so every run keeps the +// oldest copy and deletes the rest, and racing runs settle on the same one. +// Returns { create, update: comment id or null, remove: [comment ids] }. +function claimCommentChanges(comments, body) { + const ours = comments + .filter((c) => c.user?.type === "Bot" && (c.body || "").includes(CLAIM_MARKER)) + .sort((a, b) => a.id - b.id); + if (body === null) return { create: false, update: null, remove: ours.map((c) => c.id) }; + const [keep, ...extras] = ours; + return { + create: !keep, + update: keep && keep.body !== body ? keep.id : null, + remove: extras.map((c) => c.id), + }; +} + +// Bring the PR's claim comment in line with `readBody()`, the comment body for +// the live PR state (null when nothing is claimed). Runs for opened/edited +// events overlap, and a run can read the state before an edit and write after +// a newer run has finished, so after any write it reads the state again and +// repeats until a pass changes nothing. The run that writes last then also +// reads last, after every edit, and corrects a stale write. `skipIfClear` +// skips listing comments when a first read finds nothing to flag (a new PR +// has no claim comment yet). REST calls go through the injected functions; +// write failures are logged and the pass goes on. +async function syncClaimComment({ + readBody, + listComments, + createComment, + updateComment, + deleteComment, + log, + skipIfClear = false, + maxPasses = 3, +}) { + let posted = false; + for (let pass = 1; pass <= maxPasses; pass++) { + const body = await readBody(); + if (pass === 1 && body === null && skipIfClear) { + log.info("No linked issue is claimed by someone else."); + return; + } + let changes; + try { + changes = claimCommentChanges(await listComments(), body); + } catch (e) { + // Without the comment list we can't safely de-dupe; bail to avoid + // posting a second copy. + log.warning(`Listing comments failed: ${e.message}`); + return; + } + // Never post twice: the list can lag behind our own comment. + if (posted) changes.create = false; + if (!changes.create && !changes.update && !changes.remove.length) { + log.info(body ? "Claim comment is current." : "No claim comment needed."); + return; + } + try { + if (changes.create) { + await createComment(body); + posted = true; + log.info("Posted claim comment."); + } + if (changes.update) { + await updateComment(changes.update, body); + log.info(`Updated claim comment ${changes.update}.`); + } + } catch (e) { + log.warning(`Writing the claim comment failed: ${e.message}`); + } + for (const id of changes.remove) { + try { + await deleteComment(id); + log.info(`Deleted claim comment ${id}.`); + } catch (e) { + // A racing run may have deleted it first. + log.warning(`Deleting claim comment ${id} failed: ${e.message}`); + } + } + } + log.warning(`Claim comment still changing after ${maxPasses} passes; the next edit refreshes it.`); +} + +module.exports = { + CLAIM_MARKER, + findClaimConflicts, + renderClaimComment, + creditChanges, + claimCommentChanges, + syncClaimComment, +}; diff --git a/.github/scripts/issue-claims.test.js b/.github/scripts/issue-claims.test.js new file mode 100644 index 0000000000..536eaa2fcf --- /dev/null +++ b/.github/scripts/issue-claims.test.js @@ -0,0 +1,494 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +// Unit tests for issue-claims.js, run by test_issue_claims.sh. Fixtures mirror +// the GraphQL nodes and REST issue comments pr-assignment.yml fetches; the +// named cases replay the real incidents from #8676. + +"use strict"; + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const { + CLAIM_MARKER, + findClaimConflicts, + renderClaimComment, + creditChanges, + claimCommentChanges, + syncClaimComment, +} = require("./issue-claims.js"); + +const REPO = "apache/texera"; + +function pr(number, author, { state = "OPEN", repo = REPO } = {}) { + return { + number, + state, + author: author === null ? null : { login: author }, + repository: { nameWithOwner: repo }, + }; +} + +function issue(number, { state = "OPEN", repo = REPO, assignees = [], prs = [] } = {}) { + return { + number, + state, + repository: { nameWithOwner: repo }, + assignees: { nodes: assignees.map((login) => ({ login })) }, + closedByPullRequestsReferences: { nodes: prs }, + }; +} + +const opened = (prNumber, opener) => ({ repo: REPO, prNumber, opener }); + +test("findClaimConflicts", async (t) => { + await t.test("#8339 opening on #8149 flags the /take claimant and their PR", () => { + // State when #8339 opened: the claimant is assigned and #8267 is open. + const issues = [ + issue(8149, { + assignees: ["Alwaysgaurav1", "aglinxinyuan"], + prs: [pr(8267, "Alwaysgaurav1"), pr(8339, "aglinxinyuan")], + }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(8339, "aglinxinyuan")), [ + { + issue: 8149, + claimants: [{ login: "Alwaysgaurav1", assigned: true, prs: [8267] }], + }, + ]); + }); + + await t.test("an open PR is a claim without any assignee (#6674 / #8603)", () => { + // GitHub silently dropped #8603's opener self-assign, so the issue has + // no assignee and the open PR is the only trace of the claim. + const issues = [ + issue(6674, { + prs: [pr(6675, "Ma77Ball", { state: "CLOSED" }), pr(8603, "suyashj1231")], + }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(9000, "someone-else")), [ + { + issue: 6674, + claimants: [{ login: "suyashj1231", assigned: false, prs: [8603] }], + }, + ]); + }); + + await t.test("an assignee with no PR yet is a claim", () => { + const issues = [issue(1, { assignees: ["claimer"] })]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), [ + { issue: 1, claimants: [{ login: "claimer", assigned: true, prs: [] }] }, + ]); + }); + + await t.test("the check is symmetric: the claimant's PR flags the later one", () => { + const issues = [ + issue(8149, { + assignees: ["Alwaysgaurav1", "aglinxinyuan"], + prs: [pr(8267, "Alwaysgaurav1"), pr(8339, "aglinxinyuan")], + }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(8267, "Alwaysgaurav1")), [ + { + issue: 8149, + claimants: [{ login: "aglinxinyuan", assigned: true, prs: [8339] }], + }, + ]); + }); + + await t.test("the opener's own assignment and own PRs are not conflicts", () => { + const issues = [ + issue(1, { + assignees: ["opener"], + prs: [pr(2, "opener"), pr(3, "opener")], + }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), []); + }); + + await t.test("this PR itself is never a conflict, whoever GitHub lists as author", () => { + const issues = [issue(1, { prs: [pr(2, "someone-else")] })]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), []); + }); + + await t.test("merged and closed PRs are not claims", () => { + const issues = [ + issue(1, { + prs: [pr(3, "a", { state: "MERGED" }), pr(4, "b", { state: "CLOSED" })], + }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), []); + }); + + await t.test("closed issues are skipped", () => { + const issues = [ + issue(1, { state: "CLOSED", assignees: ["claimer"], prs: [pr(3, "claimer")] }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), []); + }); + + await t.test("cross-repo issues and cross-repo PRs are skipped", () => { + const issues = [ + issue(1, { repo: "other/repo", assignees: ["claimer"] }), + issue(5, { prs: [pr(3, "claimer", { repo: "other/repo" })] }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), []); + }); + + await t.test("no closing issues, or unclaimed ones, flag nothing", () => { + assert.deepEqual(findClaimConflicts([], opened(2, "opener")), []); + assert.deepEqual(findClaimConflicts([issue(1)], opened(2, "opener")), []); + }); + + await t.test("an assigned claimant with several PRs is listed once", () => { + const issues = [ + issue(1, { assignees: ["claimer"], prs: [pr(3, "claimer"), pr(4, "claimer")] }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), [ + { issue: 1, claimants: [{ login: "claimer", assigned: true, prs: [3, 4] }] }, + ]); + }); + + await t.test("claimants keep their order across several issues", () => { + const issues = [ + issue(1, { assignees: ["b", "opener"], prs: [pr(4, "c"), pr(3, "b")] }), + issue(5), + issue(6, { prs: [pr(7, "d")] }), + ]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), [ + { + issue: 1, + claimants: [ + { login: "b", assigned: true, prs: [3] }, + { login: "c", assigned: false, prs: [4] }, + ], + }, + { issue: 6, claimants: [{ login: "d", assigned: false, prs: [7] }] }, + ]); + }); + + await t.test("a deleted account's PR is attributed to ghost", () => { + const issues = [issue(1, { prs: [pr(3, null)] })]; + assert.deepEqual(findClaimConflicts(issues, opened(2, "opener")), [ + { issue: 1, claimants: [{ login: "ghost", assigned: false, prs: [3] }] }, + ]); + }); +}); + +test("renderClaimComment", async (t) => { + await t.test("starts with the marker and lists one row per claimant", () => { + const body = renderClaimComment([ + { + issue: 8149, + claimants: [{ login: "Alwaysgaurav1", assigned: true, prs: [8267] }], + }, + { + issue: 6674, + claimants: [ + { login: "suyashj1231", assigned: false, prs: [8603, 8610] }, + { login: "claimer", assigned: true, prs: [] }, + ], + }, + ]); + assert.ok(body.startsWith(`${CLAIM_MARKER}\n`)); + const rows = body.split("\n").filter((l) => /^\| #\d/.test(l)); + assert.deepEqual(rows, [ + "| #8149 | @Alwaysgaurav1 (assignee) | #8267 |", + "| #6674 | @suyashj1231 | #8603, #8610 |", + "| #6674 | @claimer (assignee) | — |", + ]); + }); + + await t.test("the marker appears exactly once", () => { + const body = renderClaimComment([ + { issue: 1, claimants: [{ login: "a", assigned: true, prs: [] }] }, + ]); + assert.equal(body.split(CLAIM_MARKER).length, 2); + }); +}); + +test("creditChanges", async (t) => { + const merged = (prNumber, credited) => ({ repo: REPO, prNumber, credited }); + + await t.test("#8339 merging keeps the #8149 claimant whose #8267 is still open", () => { + const i = issue(8149, { + assignees: ["Alwaysgaurav1", "aglinxinyuan"], + prs: [pr(8339, "aglinxinyuan", { state: "MERGED" }), pr(8267, "Alwaysgaurav1")], + }); + assert.deepEqual(creditChanges(i, merged(8339, ["aglinxinyuan"])), { + current: ["Alwaysgaurav1", "aglinxinyuan"], + toRemove: [], + toAdd: [], + kept: ["Alwaysgaurav1"], + }); + }); + + await t.test("an assignee without an open PR is replaced by the credited authors", () => { + const i = issue(1, { assignees: ["triager"], prs: [pr(2, "author", { state: "MERGED" })] }); + assert.deepEqual(creditChanges(i, merged(2, ["author", "coauthor"])), { + current: ["triager"], + toRemove: ["triager"], + toAdd: ["author", "coauthor"], + kept: [], + }); + }); + + await t.test("a closed, merged, or cross-repo PR does not keep its author", () => { + const i = issue(1, { + assignees: ["a", "b", "c"], + prs: [ + pr(3, "a", { state: "CLOSED" }), + pr(4, "b", { state: "MERGED" }), + pr(5, "c", { repo: "other/repo" }), + ], + }); + assert.deepEqual(creditChanges(i, merged(2, ["author"])).toRemove, ["a", "b", "c"]); + }); + + await t.test("the merged PR itself does not keep anyone, even if still listed OPEN", () => { + // The merge event can race GitHub's own state update. + const i = issue(1, { assignees: ["opener"], prs: [pr(2, "opener")] }); + assert.deepEqual(creditChanges(i, merged(2, ["author"])).toRemove, ["opener"]); + }); + + await t.test("an open PR by someone not assigned adds nobody", () => { + const i = issue(1, { prs: [pr(3, "outsider")] }); + assert.deepEqual(creditChanges(i, merged(2, ["author"])), { + current: [], + toRemove: [], + toAdd: ["author"], + kept: [], + }); + }); + + await t.test("an issue already assigned to exactly the credited authors is a no-op", () => { + const i = issue(1, { assignees: ["author"] }); + assert.deepEqual(creditChanges(i, merged(2, ["author"])), { + current: ["author"], + toRemove: [], + toAdd: [], + kept: [], + }); + }); +}); + +test("claimCommentChanges", async (t) => { + const comment = (id, body, type = "Bot") => ({ id, body, user: { type } }); + const BODY = renderClaimComment([ + { issue: 1, claimants: [{ login: "a", assigned: true, prs: [] }] }, + ]); + const STALE = `${CLAIM_MARKER}\nold text`; + const none = { create: false, update: null, remove: [] }; + + await t.test("no claim comment yet: create one", () => { + assert.deepEqual(claimCommentChanges([], BODY), { create: true, update: null, remove: [] }); + }); + + await t.test("a current claim comment is left alone", () => { + assert.deepEqual(claimCommentChanges([comment(5, BODY)], BODY), none); + }); + + await t.test("a stale claim comment is rewritten in place", () => { + assert.deepEqual(claimCommentChanges([comment(5, STALE)], BODY), { + create: false, + update: 5, + remove: [], + }); + }); + + await t.test("copies posted by racing runs keep the oldest, whatever the list order", () => { + assert.deepEqual(claimCommentChanges([comment(9, BODY), comment(5, BODY)], BODY), { + create: false, + update: null, + remove: [9], + }); + assert.deepEqual(claimCommentChanges([comment(9, BODY), comment(5, STALE)], BODY), { + create: false, + update: 5, + remove: [9], + }); + }); + + await t.test("nothing to flag: every copy is removed", () => { + assert.deepEqual(claimCommentChanges([comment(9, BODY), comment(5, STALE)], null), { + create: false, + update: null, + remove: [5, 9], + }); + }); + + await t.test("nothing to flag and no copy: no-op", () => { + assert.deepEqual(claimCommentChanges([], null), none); + }); + + await t.test("human comments, unmarked bot comments and empty bodies are never ours", () => { + const others = [ + comment(1, `quoting ${CLAIM_MARKER} in a reply`, "User"), + comment(2, "<!-- texera:template-compliance -->\nunrelated bot note"), + comment(3, null), + { id: 4, body: CLAIM_MARKER, user: null }, + ]; + assert.deepEqual(claimCommentChanges(others, BODY), { create: true, update: null, remove: [] }); + assert.deepEqual(claimCommentChanges(others, null), none); + }); +}); + +// An in-memory PR conversation with the REST calls syncClaimComment makes. +// `lag` hides comments from listComments, like a list that hasn't caught up. +function fakeConversation({ lag = new Set() } = {}) { + const comments = []; + let nextId = 100; + const warnings = []; + const api = { + listComments: async () => comments.filter((c) => !lag.has(c.id)).map((c) => ({ ...c })), + createComment: async (body) => { + const c = { id: nextId++, body, user: { type: "Bot" } }; + comments.push(c); + return c.id; + }, + updateComment: async (id, body) => { + const c = comments.find((x) => x.id === id); + if (!c) throw new Error(`comment ${id} not found`); + c.body = body; + }, + deleteComment: async (id) => { + const i = comments.findIndex((x) => x.id === id); + if (i < 0) throw new Error(`comment ${id} not found`); + comments.splice(i, 1); + }, + log: { info: () => {}, warning: (m) => warnings.push(m) }, + }; + return { comments, api, warnings }; +} + +test("syncClaimComment", async (t) => { + const claim = (login) => + renderClaimComment([{ issue: 1, claimants: [{ login, assigned: true, prs: [] }] }]); + const OLD = claim("old-claimant"); + const NEW = claim("new-claimant"); + const bodies = (conv) => conv.comments.map((c) => c.body); + + await t.test("posts the claim comment once and stops when it is current", async () => { + const conv = fakeConversation(); + let reads = 0; + await syncClaimComment({ ...conv.api, readBody: async () => (reads++, NEW) }); + assert.deepEqual(bodies(conv), [NEW]); + assert.equal(reads, 2, "re-reads the state once after writing, then stops"); + }); + + await t.test("a current comment is left alone after one read", async () => { + const conv = fakeConversation(); + await conv.api.createComment(NEW); + let reads = 0; + await syncClaimComment({ ...conv.api, readBody: async () => (reads++, NEW) }); + assert.deepEqual(bodies(conv), [NEW]); + assert.equal(reads, 1); + }); + + await t.test("a delayed run does not leave a warning the newer run removed", async () => { + // Run A reads the state before an edit drops the closing keyword; run B + // reads it after, finds nothing to flag, and finishes before A writes. + const conv = fakeConversation(); + let state = OLD; + let aReads = 0; + await syncClaimComment({ + ...conv.api, + readBody: async () => { + if (aReads++ > 0) return state; + const seen = state; + state = null; + await syncClaimComment({ ...conv.api, readBody: async () => state }); + return seen; + }, + }); + assert.deepEqual(bodies(conv), []); + }); + + await t.test("a delayed run does not overwrite the newer run's text", async () => { + // Run B posts NEW between run A reading OLD and A writing it. + const conv = fakeConversation(); + let state = OLD; + let aReads = 0; + await syncClaimComment({ + ...conv.api, + readBody: async () => { + if (aReads++ > 0) return state; + const seen = state; + state = NEW; + await syncClaimComment({ ...conv.api, readBody: async () => state }); + return seen; + }, + }); + assert.deepEqual(bodies(conv), [NEW]); + }); + + await t.test("never posts twice when the re-list lags behind its own comment", async () => { + const conv = fakeConversation({ lag: new Set([100]) }); + await syncClaimComment({ ...conv.api, readBody: async () => NEW }); + assert.deepEqual(bodies(conv), [NEW]); + }); + + await t.test("skipIfClear: nothing to flag means no list and no writes", async () => { + const conv = fakeConversation(); + let listed = false; + await syncClaimComment({ + ...conv.api, + listComments: async () => ((listed = true), []), + readBody: async () => null, + skipIfClear: true, + }); + assert.equal(listed, false); + }); + + await t.test("a failed list bails without writing", async () => { + const conv = fakeConversation(); + await syncClaimComment({ + ...conv.api, + listComments: async () => { + throw new Error("boom"); + }, + readBody: async () => NEW, + }); + assert.deepEqual(bodies(conv), []); + assert.equal(conv.warnings.length, 1); + }); + + await t.test("a copy a racing run already deleted is only a warning", async () => { + const conv = fakeConversation(); + await conv.api.createComment(NEW); + await conv.api.createComment(NEW); + const deleteComment = async (id) => { + await conv.api.deleteComment(id); + throw new Error("already gone"); + }; + await syncClaimComment({ ...conv.api, deleteComment, readBody: async () => NEW }); + assert.deepEqual(bodies(conv), [NEW]); + }); + + await t.test("gives up after maxPasses if the state keeps changing", async () => { + const conv = fakeConversation(); + let reads = 0; + await syncClaimComment({ + ...conv.api, + readBody: async () => (reads++ % 2 ? OLD : NEW), + maxPasses: 3, + }); + assert.equal(reads, 3); + assert.equal(conv.comments.length, 1); + assert.match(conv.warnings.at(-1), /3 passes/); + }); +}); diff --git a/.github/scripts/test_issue_claims.sh b/.github/scripts/test_issue_claims.sh new file mode 100755 index 0000000000..d0b05c252b --- /dev/null +++ b/.github/scripts/test_issue_claims.sh @@ -0,0 +1,29 @@ +#!/usr/bin/env bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you 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. + +# Runs the unit tests for issue-claims.js, the issue-claim logic behind +# pr-assignment.yml. The infra CI job discovers this file by its test_*.sh +# name; the cases themselves live in issue-claims.test.js and use Node's +# built-in test runner, which the ubuntu and macOS runner images ship with. + +set -euo pipefail + +command -v node >/dev/null || { echo "node is required to run these tests" >&2; exit 1; } + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +node --test "$script_dir/issue-claims.test.js" diff --git a/.github/workflows/pr-assignment.yml b/.github/workflows/pr-assignment.yml index 0f362a40eb..6f0444820b 100644 --- a/.github/workflows/pr-assignment.yml +++ b/.github/workflows/pr-assignment.yml @@ -20,16 +20,25 @@ on: types: [opened, edited, closed] permissions: + contents: read issues: write pull-requests: write jobs: - # All three behaviors live as steps under one job so the PR Checks - # tab shows a single entry per event instead of two-or-three skipped - # siblings. Step-level if-guards keep the actual work scoped. + # All behaviors live as steps under one job so the PR Checks tab shows + # a single entry per event instead of several skipped siblings. + # Step-level if-guards keep the actual work scoped. pr-assignment: runs-on: ubuntu-latest steps: + # pull_request_target resolves to the trusted base branch, never the + # fork head, so requiring this script from the steps below is safe. + - uses: actions/checkout@v7 + with: + persist-credentials: false + sparse-checkout: .github/scripts/issue-claims.js + sparse-checkout-cone-mode: false + - name: Self-assign PR author to the PR if: >- github.event.action == 'opened' @@ -139,6 +148,82 @@ jobs: core.info(`Body unchanged on edit; skipping removal-detection.`); } + - name: Flag linked issues someone else has claimed + # A PR that closes an issue someone else has claimed (they are + # assigned, or have an open PR on it) gets one sticky comment + # naming them and their PR, so the overlap is settled in review. + # Otherwise the first merge closes the issue and can strand the + # other PR: #8339 merged over #8267 on #8149 (see #8676). + # + # It comments instead of failing a check. Both PRs match (the + # claimant's own PR flags this one back), so a red check would + # also land on the person being protected. Only a human can tell + # an agreed takeover from a duplicate. And nothing re-runs this + # when the other PR closes, so a failure would go stale. + # + # Overlapping opened/edited runs can each post a copy before seeing + # the other's. Every run keeps the oldest copy and deletes the rest + # (claimCommentChanges), so racing runs settle on one comment. A run + # can also read the state before an edit and write after a newer + # run, so after writing it reads the state again and repeats until + # nothing changes (syncClaimComment). A concurrency group isn't + # used: with the default queue it cancels a waiting run, and the + # sync and credit steps need every event. + if: >- + contains(fromJSON('["opened","edited"]'), github.event.action) + && github.event.pull_request.state == 'open' + && github.event.pull_request.user.type != 'Bot' + uses: actions/github-script@v9 + with: + script: | + const { findClaimConflicts, renderClaimComment, syncClaimComment } = + require('./.github/scripts/issue-claims.js'); + const { owner, repo } = context.repo; + const pr = context.payload.pull_request; + const issue_number = pr.number; + + // The claim comment for the PR as it is now, not as this event saw it. + const readBody = async () => { + const { repository: { pullRequest: prq } } = await github.graphql(` + query($owner: String!, $repo: String!, $pr: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + closingIssuesReferences(first: 50) { + nodes { + number + state + repository { nameWithOwner } + assignees(first: 20) { nodes { login } } + closedByPullRequestsReferences(first: 100) { + nodes { number state author { login } repository { nameWithOwner } } + } + } + } + } + } + }`, { owner, repo, pr: issue_number }); + const conflicts = findClaimConflicts(prq.closingIssuesReferences.nodes, { + repo: `${owner}/${repo}`, prNumber: issue_number, opener: pr.user.login, + }); + for (const { issue, claimants } of conflicts) { + core.info(`Issue #${issue} claimed by: ${claimants.map((c) => `${c.login} (assigned=${c.assigned}, prs=[${c.prs.join(',')}])`).join('; ')}`); + } + return conflicts.length ? renderClaimComment(conflicts) : null; + }; + + await syncClaimComment({ + readBody, + listComments: () => github.paginate( + github.rest.issues.listComments, + { owner, repo, issue_number, per_page: 100 }, + ), + createComment: (body) => github.rest.issues.createComment({ owner, repo, issue_number, body }), + updateComment: (comment_id, body) => github.rest.issues.updateComment({ owner, repo, comment_id, body }), + deleteComment: (comment_id) => github.rest.issues.deleteComment({ owner, repo, comment_id }), + log: core, + skipIfClear: context.payload.action === 'opened', + }); + - name: Unassign PR opener from linked issues on PR close without merge # When a PR is closed without merging, the opener was added to # linked issues by the "Sync PR opener" step on open/edit. Mirror @@ -187,10 +272,16 @@ jobs: } - name: Credit issue assignees on PR merge + # Replace each linked issue's assignees with the merged PR's human + # authors, except anyone who still has their own open PR on the + # issue: unassigning them would hide a claim that is still in + # review. If that PR later closes unmerged, the step above + # unassigns them. if: github.event.action == 'closed' && github.event.pull_request.merged uses: actions/github-script@v9 with: script: | + const { creditChanges } = require('./.github/scripts/issue-claims.js'); const { owner, repo } = context.repo; const pr = context.payload.pull_request; const opener = pr.user; @@ -206,6 +297,9 @@ jobs: number repository { nameWithOwner } assignees(first: 20) { nodes { login } } + closedByPullRequestsReferences(first: 100) { + nodes { number state author { login } repository { nameWithOwner } } + } } } commits(first: 250) { @@ -227,7 +321,6 @@ jobs: } } const credited = [...authors].slice(0, 10); - const creditedSet = new Set(credited); core.info(`Credited authors (max 10, [bot] filtered): [${credited.join(', ') || '(none)'}]`); if (!credited.length) { core.info(`No human authors to credit; skipping all linked issues.`); @@ -238,11 +331,11 @@ jobs: core.info(`Linked same-repo issues to credit: [${sameRepoIssues.map((i) => `#${i.number}`).join(', ') || '(none)'}]`); for (const issue of sameRepoIssues) { - const current = issue.assignees.nodes.map(n => n.login); - const toRemove = current.filter(l => !creditedSet.has(l)); - const toAdd = credited.filter(l => !current.includes(l)); + const { current, toRemove, toAdd, kept } = creditChanges(issue, { + repo: `${owner}/${repo}`, prNumber: pr.number, credited, + }); const args = { owner, repo, issue_number: issue.number }; - core.info(`Issue #${issue.number}: current=[${current.join(',')}] credited=[${credited.join(',')}] toRemove=[${toRemove.join(',')}] toAdd=[${toAdd.join(',')}]`); + core.info(`Issue #${issue.number}: current=[${current.join(',')}] credited=[${credited.join(',')}] toRemove=[${toRemove.join(',')}] toAdd=[${toAdd.join(',')}] kept (own open PR)=[${kept.join(',')}]`); try { if (toRemove.length) { await github.rest.issues.removeAssignees({ ...args, assignees: toRemove });
