-
Notifications
You must be signed in to change notification settings - Fork 3k
ci(workflows): remind authors not to force-push active PRs #6035
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bf2c244
1266743
b7aa71f
48b6c46
6868e97
3ba3169
be9e2db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| name: 'PR Force-Push Reminder' | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: | ||
| - 'synchronize' | ||
|
|
||
| permissions: | ||
| contents: 'read' | ||
| issues: 'write' | ||
| pull-requests: 'write' | ||
|
wenshao marked this conversation as resolved.
|
||
|
|
||
| # No concurrency group on purpose. GitHub keeps at most one pending run per | ||
| # group, so a burst of pushes can cancel a still-pending run that was about to | ||
| # post — dropping the very reminder this workflow exists to deliver. Letting | ||
| # every synchronize event run independently guarantees each force-push is | ||
| # evaluated; idempotency comes from the once-per-PR marker checked in the script | ||
| # (not from serializing runs). A rare double-post on two near-simultaneous | ||
| # first force-pushes is the acceptable cost of never silently missing one. | ||
|
|
||
| jobs: | ||
| remind-on-force-push: | ||
| name: 'Remind on force-push' | ||
| timeout-minutes: 5 | ||
| if: |- | ||
| ${{ github.repository == 'QwenLM/qwen-code' }} | ||
| runs-on: 'ubuntu-latest' | ||
| steps: | ||
| - name: 'Detect force-push and post reminder' | ||
| uses: 'actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3' # v9.0.0 | ||
| with: | ||
| github-token: '${{ secrets.GITHUB_TOKEN }}' | ||
| script: | | ||
| const pr = context.payload.pull_request; | ||
| const before = context.payload.before; | ||
| const after = context.payload.after; | ||
|
|
||
| // A `synchronize` event should always carry both SHAs; bail out if | ||
| // either is missing or `before` is the all-zero (no parent) SHA. | ||
| if (!before || !after || /^0+$/.test(before)) { | ||
| console.log('No usable before/after SHA; nothing to do.'); | ||
| return; | ||
| } | ||
|
|
||
| // Skip automation-driven updates. A GitHub App push has | ||
| // sender.type === 'Bot', but the repo's own autofix bot pushes | ||
| // (including force-pushes) via a PAT as the user account | ||
| // qwen-code-dev-bot, which arrives with sender.type === 'User' — so | ||
| // also skip known automation logins (mirrors qwen-autofix.yml's | ||
| // KNOWN_BOTS). Only human contributors should be reminded. | ||
| const sender = context.payload.sender; | ||
| // KEEP IN SYNC with KNOWN_BOTS in .github/workflows/qwen-autofix.yml. | ||
| const KNOWN_AUTOMATION = new Set([ | ||
|
wenshao marked this conversation as resolved.
|
||
| 'qwen-code-ci-bot', | ||
| 'qwen-code-dev-bot', | ||
| 'github-actions', | ||
| 'github-actions[bot]', | ||
| 'gemini-cli-robot', | ||
| ]); | ||
| if (sender?.type === 'Bot' || KNOWN_AUTOMATION.has(sender?.login)) { | ||
| console.log(`Push made by automation "${sender?.login}" (${sender?.type}); skipping.`); | ||
| return; | ||
| } | ||
|
|
||
| // Compare the old tip (`before`) with the new tip (`after`): | ||
| // ahead -> new commits added on top of the old tip (normal push) | ||
| // identical -> no change | ||
| // behind -> reset to an older commit (force-push) | ||
| // diverged -> history rewritten, e.g. rebase/amend (force-push) | ||
| let status; | ||
| try { | ||
| const cmp = await github.rest.repos.compareCommitsWithBasehead({ | ||
| owner: context.repo.owner, | ||
|
wenshao marked this conversation as resolved.
|
||
| repo: context.repo.repo, | ||
| basehead: `${before}...${after}`, | ||
|
wenshao marked this conversation as resolved.
wenshao marked this conversation as resolved.
|
||
| }); | ||
| status = cmp.data.status; | ||
| } catch (err) { | ||
| // A 404 means the old tip (`before`) is no longer reachable — it | ||
| // was orphaned by the force-push and already GC'd. Skip | ||
| // conservatively rather than risk a false accusation. Any other | ||
| // error (403/429/5xx) is a real failure: rethrow so the run goes | ||
| // red and the outage is visible instead of a silent no-op. | ||
| if (err.status === 404) { | ||
| console.log(`Old tip ${before} no longer reachable (404); skipping.`); | ||
| return; | ||
| } | ||
| throw err; | ||
|
wenshao marked this conversation as resolved.
|
||
| } | ||
|
|
||
| console.log(`Compare ${before.slice(0, 7)}...${after.slice(0, 7)} => ${status}`); | ||
| if (status === 'ahead' || status === 'identical') { | ||
| console.log('Fast-forward push (not a force-push); nothing to do.'); | ||
| return; | ||
| } | ||
|
|
||
| // Force-push confirmed. Post the reminder at most once per PR; the | ||
| // hidden marker lets us detect a reminder we already left. | ||
| const MARKER = '<!-- pr-force-push-reminder -->'; | ||
| let comments; | ||
| try { | ||
| comments = await github.paginate(github.rest.issues.listComments, { | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: pr.number, | ||
| per_page: 100, | ||
| }); | ||
| } catch (err) { | ||
| core.error(`Failed to list comments on PR #${pr.number}: ${err.status} ${err.message}`); | ||
| throw err; | ||
| } | ||
| // Only trust the marker on our own bot's comment — otherwise anyone | ||
| // could permanently suppress reminders by pasting the marker string. | ||
| if ( | ||
| comments.some( | ||
| (c) => | ||
| c.user?.type === 'Bot' && | ||
| c.user?.login === 'github-actions[bot]' && | ||
| c.body && | ||
| c.body.includes(MARKER), | ||
| ) | ||
| ) { | ||
| console.log('Reminder already posted by the bot on this PR; skipping.'); | ||
| return; | ||
| } | ||
|
|
||
| const english = | ||
| 'Please do not rebase or force-push to an active PR as it invalidates ' + | ||
| 'existing review comments. Note for future reference, the bots always ' + | ||
| 'squash all changes into a single commit automatically as part of the ' + | ||
| 'integration.'; | ||
| const chinese = | ||
| '请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。' + | ||
| '另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动' + | ||
| '压缩(squash)为单个提交。'; | ||
| const body = [ | ||
| MARKER, | ||
| '', | ||
| english, | ||
| '', | ||
| '<details>', | ||
| '<summary>中文</summary>', | ||
| '', | ||
| chinese, | ||
| '', | ||
| '</details>', | ||
| ].join('\n'); | ||
|
|
||
| try { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: pr.number, | ||
| body, | ||
| }); | ||
| console.log(`Posted force-push reminder on PR #${pr.number}.`); | ||
| } catch (err) { | ||
| // Surface auth/rate-limit/transient failures with context and let | ||
| // the run go red instead of failing silently. | ||
| core.error(`Failed to comment on PR #${pr.number}: ${err.status} ${err.message}`); | ||
| throw err; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2025 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { readFileSync } from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| const repoRoot = path.resolve( | ||
| path.dirname(fileURLToPath(import.meta.url)), | ||
| '../..', | ||
| ); | ||
|
|
||
| describe('pr force-push reminder workflow', () => { | ||
| const workflow = readFileSync( | ||
| path.join(repoRoot, '.github/workflows/pr-force-push-reminder.yml'), | ||
| 'utf8', | ||
| ); | ||
|
|
||
| it('triggers only on pull_request_target synchronize', () => { | ||
| expect(workflow).toContain('pull_request_target:'); | ||
| expect(workflow).toContain("- 'synchronize'"); | ||
| // Must not check out or run PR code: a github-script-only job needs no | ||
| // checkout, which is what keeps pull_request_target safe from pwn-requests. | ||
| expect(workflow).not.toContain('actions/checkout'); | ||
| }); | ||
|
|
||
| it('only runs on the upstream repo', () => { | ||
| expect(workflow).toContain("github.repository == 'QwenLM/qwen-code'"); | ||
| }); | ||
|
|
||
| it('grants the permissions the comment endpoints need', () => { | ||
| expect(workflow).toContain("contents: 'read'"); | ||
| expect(workflow).toContain("issues: 'write'"); | ||
| expect(workflow).toContain("pull-requests: 'write'"); | ||
| }); | ||
|
|
||
| it('uses no concurrency group so no push event is ever dropped', () => { | ||
| // GitHub keeps at most one pending run per concurrency group, so a group | ||
| // could cancel a still-pending force-push run before it posts. Idempotency | ||
| // comes from the marker instead, so there must be no concurrency block. | ||
| expect(workflow).not.toContain('concurrency:'); | ||
| expect(workflow).not.toContain('cancel-in-progress'); | ||
| }); | ||
|
|
||
| it('bounds the job and pins the github-script action by SHA', () => { | ||
| expect(workflow).toContain('timeout-minutes: 5'); | ||
| expect(workflow).toContain( | ||
| 'actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3', | ||
| ); | ||
| }); | ||
|
|
||
| it('guards against missing or zero before/after SHAs', () => { | ||
| expect(workflow).toContain('!before || !after || /^0+$/.test(before)'); | ||
| }); | ||
|
|
||
| it('skips bot and known-automation pushes', () => { | ||
| // GitHub Apps arrive as sender.type Bot; the autofix bot force-pushes via a | ||
| // PAT as a User account, so its login must be skipped explicitly. | ||
| expect(workflow).toContain( | ||
| "sender?.type === 'Bot' || KNOWN_AUTOMATION.has(sender?.login)", | ||
| ); | ||
| expect(workflow).toContain( | ||
| 'KEEP IN SYNC with KNOWN_BOTS in .github/workflows/qwen-autofix.yml', | ||
| ); | ||
| // Mechanically enforce the sync: read qwen-autofix.yml's KNOWN_BOTS and | ||
| // assert every login is also skipped here, so adding a bot there without | ||
| // updating this list fails the test rather than silently drifting. | ||
| const autofix = readFileSync( | ||
| path.join(repoRoot, '.github/workflows/qwen-autofix.yml'), | ||
| 'utf8', | ||
| ); | ||
| const match = autofix.match(/KNOWN_BOTS:\s*'(\[.*\])'/); | ||
| expect(match, 'KNOWN_BOTS not found in qwen-autofix.yml').not.toBeNull(); | ||
| for (const login of JSON.parse(match[1])) { | ||
| expect(workflow).toContain(`'${login}'`); | ||
| } | ||
| }); | ||
|
|
||
| it('detects force-pushes with a 3-dot compare on the base repo', () => { | ||
| // Verified against the live REST API: 3-dot returns diverged/behind for | ||
| // force-pushes and resolves fork-PR commits, while 2-dot 404s. Do not | ||
| // "simplify" this to two dots. | ||
| expect(workflow).toContain('basehead: `${before}...${after}`'); | ||
| expect(workflow).not.toContain('basehead: `${before}..${after}`'); | ||
| // The base repo resolves fork-PR commits via refs/pull/N/head, so the | ||
| // compare targets context.repo, not the (possibly deleted) head repo. | ||
| expect(workflow).not.toContain('pr.head.repo'); | ||
| // Only ahead/identical is a normal push; behind/diverged is a force-push. | ||
| expect(workflow).toContain("status === 'ahead' || status === 'identical'"); | ||
| }); | ||
|
|
||
| it('skips on a 404 compare but surfaces other errors', () => { | ||
| // A 404 means the old tip was orphaned by the force-push; anything else | ||
| // (403/429/5xx) must fail the run loudly instead of a silent green no-op. | ||
| expect(workflow).toContain('if (err.status === 404)'); | ||
| expect(workflow).toContain('throw err;'); | ||
|
wenshao marked this conversation as resolved.
|
||
| expect(workflow).not.toContain('Could not compare'); | ||
| }); | ||
|
|
||
| it('only trusts the dedup marker on its own bot comment', () => { | ||
| // Otherwise any user could suppress all future reminders by pasting the | ||
| // marker string into a comment. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The dedup-marker test asserts the marker string, bot identity checks, and the skip message, but never asserts that comment listing uses expect(workflow).toContain('github.paginate(');— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 中文已在 |
||
| expect(workflow).toContain('<!-- pr-force-push-reminder -->'); | ||
| expect(workflow).toContain("c.user?.type === 'Bot'"); | ||
| expect(workflow).toContain("c.user?.login === 'github-actions[bot]'"); | ||
| // Assert the skip path itself, so deleting the guard fails a test. | ||
| expect(workflow).toContain( | ||
| 'Reminder already posted by the bot on this PR; skipping.', | ||
| ); | ||
| }); | ||
|
|
||
| it('wraps every GitHub write/read in error logging that rethrows', () => { | ||
|
wenshao marked this conversation as resolved.
|
||
| // listComments, compare (via the 404 branch), and createComment must all | ||
| // surface failures rather than swallowing them. | ||
| expect(workflow).toContain('Failed to list comments on PR #${pr.number}'); | ||
| expect(workflow).toContain('Failed to comment on PR #${pr.number}'); | ||
| expect(workflow).toContain('core.error('); | ||
| }); | ||
|
|
||
| it('posts a bilingual reminder', () => { | ||
| expect(workflow).toContain('Please do not rebase or force-push'); | ||
| expect(workflow).toContain('squash all changes into a single commit'); | ||
| expect(workflow).toContain('<summary>中文</summary>'); | ||
| expect(workflow).toContain('请勿对活跃的 PR 执行 rebase 或 force-push'); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.