aglinxinyuan commented on code in PR #8678:
URL: https://github.com/apache/texera/pull/8678#discussion_r4201406705
##########
.github/workflows/pr-assignment.yml:
##########
@@ -24,12 +24,20 @@ permissions:
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
Review Comment:
The checkout doesn't fail here. apache/texera is public, so a token without
the Contents scope can still fetch it. `contributor-checks.yml` has the same
`issues`/`pull-requests: write` permissions block and runs the same sparse
`actions/checkout@v7`. Its log in [run
37376276601](https://github.com/apache/texera/actions/runs/37376276601) shows
`GITHUB_TOKEN Permissions: Issues: write, Metadata: read, PullRequests: write`,
and the checkout succeeds.
I've still added `contents: read` in 911f890fa7. The checkout is now the
first step of the only job, so if it ever failed, every later step would be
skipped too, including the existing self-assign, sync, unassign and credit
steps. Declaring the scope means that no longer depends on the repo being
public. `suggest-reviewers.yml` declares the same three scopes.
##########
.github/workflows/pr-assignment.yml:
##########
@@ -139,6 +147,97 @@ 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.
+ 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 { CLAIM_MARKER, findClaimConflicts, renderClaimComment } =
+ require('./.github/scripts/issue-claims.js');
+ const { owner, repo } = context.repo;
+ const pr = context.payload.pull_request;
+ const issue_number = pr.number;
+
+ 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: 20) {
+ nodes { number state author { login } repository {
nameWithOwner } }
Review Comment:
In 911f890fa7, both reads (here and in the credit query) now use `first:
100`, the most this connection allows (`first: 101` returns
`EXCESSIVE_PAGINATION`). Across all 3,002 issues in apache/texera, no issue has
more than 5 PRs in `closedByPullRequestsReferences` with these default
arguments (open plus merged), or 7 counting closed ones. So the page has about
20x headroom, and the query's rate-limit cost doesn't change.
I didn't add cursor pagination. github-script v9 doesn't bundle a GraphQL
paginator, and this connection is nested inside
`closingIssuesReferences.nodes[]`, so it can't be paginated automatically. Full
pagination would need a hand-written follow-up query per issue in both steps.
If an issue ever went past 100, the effect is limited: assignees are read in
full (GitHub allows at most 10), so an assigned claimant is still flagged, and
the credit step would fall back to how it behaved before this PR.
##########
.github/workflows/pr-assignment.yml:
##########
@@ -139,6 +147,97 @@ 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.
+ 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 { CLAIM_MARKER, findClaimConflicts, renderClaimComment } =
+ require('./.github/scripts/issue-claims.js');
+ const { owner, repo } = context.repo;
+ const pr = context.payload.pull_request;
+ const issue_number = pr.number;
+
+ 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: 20) {
+ 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('; ')}`);
+ }
+ if (!conflicts.length && context.payload.action === 'opened') {
+ core.info(`No linked issue is claimed by someone else.`);
+ return;
+ }
+
+ let existing = null;
+ try {
+ const comments = await github.paginate(
+ github.rest.issues.listComments,
+ { owner, repo, issue_number, per_page: 100 },
+ );
+ existing = comments.find((c) => c.user?.type === 'Bot' &&
(c.body || '').includes(CLAIM_MARKER));
+ } catch (e) {
+ // Without the comment list we can't safely de-dupe; bail to
+ // avoid posting a second copy.
+ core.warning(`listComments on #${issue_number} failed:
${e.message}`);
+ return;
+ }
+
+ try {
+ if (conflicts.length) {
+ const body = renderClaimComment(conflicts);
+ if (!existing) {
+ await github.rest.issues.createComment({ owner, repo,
issue_number, body });
Review Comment:
Agreed, the race is real. `contributor-checks.yml` uses the same
list-then-create pattern and has left two copies posted in the same second on
#8756 and #6408.
Fixed in 911f890fa7 by making the update converge rather than adding a
concurrency group. Each run keeps the oldest marker comment, updates it if it's
stale, and deletes any other copies. A run that has just posted lists the
comments again immediately, so two racing runs end up keeping the same comment.
Each delete has its own try/catch because the other run may delete that copy
first. The decision is the pure function `claimCommentChanges` in
`issue-claims.js`, covered by unit tests. I also modelled two racing runs and
tried every ordering of their API calls: whenever both see the same state,
exactly one comment is left. A copy that a lagging re-list misses is removed on
the next edit.
I didn't use a concurrency group because, with the default queue, a newer
event cancels the run that's waiting. The sync step's unassign reads each
`edited` event's `changes.body.from`, and the credit step runs only once, on
`closed`. A dropped run would therefore leave a stale assignee or skip the
merge credit, and runner waits here have reached an hour. `queue: max` would
avoid the drops, but it's new and nothing in this repo uses it yet, and an
invalid key would break every step in this workflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
