diff --git a/.github/workflows/qwen-autofix-fork-signal.yml b/.github/workflows/qwen-autofix-fork-signal.yml index 2f9ef163ccf..e3c5a36b91e 100644 --- a/.github/workflows/qwen-autofix-fork-signal.yml +++ b/.github/workflows/qwen-autofix-fork-signal.yml @@ -68,25 +68,41 @@ jobs: # In-repo PRs keep their secrets and reach the review lane directly, so # signalling them would only dispatch a duplicate scan. The remaining # conjuncts MIRROR route's fork admission (qwen-autofix.yml's - # pull_request_review path): maintainer edits on, and the PR bot-authored - # or takeover-labeled — so an ordinary contributor's fork PR burns no - # signal/bridge/dispatched runs per review, and a takeover PR whose - # author turned maintainer edits off is never pushed into the credentialed - # lane. SKIP wins here as it wins everywhere: a skip-labeled PR's reviews - # would otherwise burn a signal + bridge run and a PR read before being - # discarded, repeating known-useless work on every review. The - # association list mirrors TRUSTED_ASSOC, the review-bot login mirrors + # pull_request_review path): the PR is bot-authored or takeover-labeled — + # so an ordinary contributor's fork PR burns no signal/bridge/dispatched + # runs per review. SKIP wins here as it wins everywhere: a skip-labeled + # PR's reviews would otherwise burn a signal + bridge run and a PR read + # before being discarded, repeating known-useless work on every review. + # The association list mirrors TRUSTED_ASSOC, the review-bot login mirrors # REVIEW_BOT, and the two labels mirror TAKEOVER_LABEL / SKIP_LABEL — all # pinned by the fork-bridge workflow test. The association is a WHEN # filter only: MEMBER/COLLABORATOR does not guarantee LIVE write # permission, so the bridge re-checks the reviewer live before # dispatching. + # + # MAINTAINER EDITS ARE NOT CHECKED HERE, deliberately, and this is the one + # conjunct that cannot live in this job. `pull_request_review` delivers + # the SIMPLE pull-request object, which carries no `maintainer_can_modify` + # — that field ships only on the full object the `pull_request` event + # sends. Reading it here yielded null on every delivery, `null == true` is + # false, and the gate could never open: across the 300 runs between this + # bridge shipping (#8676, 2026-08-07) and this fix, 290 skipped, 7 + # cancelled, 1 action_required, and NOT ONE reached the signal step — so + # fork reviews silently fell back to the throttled cron backstop this + # bridge exists to get ahead of, and when that backstop stalled they fell + # through entirely. The consent check itself is not lost: the bridge + # re-reads it LIVE (`gh pr view --json maintainerCanModify` and a + # `select(… .maintainerCanModify == true)`), which is the only place it + # can be read at all — this job holds `permissions: {}`, no secrets and no + # checkout, so it has no way to ask the API. Nor is it weakened by moving: + # consent can be withdrawn between the review and the dispatch, so the + # live read was always the authoritative one and a payload copy could only + # ever have been a stale early-out. if: |- ${{ github.repository == 'QwenLM/qwen-code' && github.event.pull_request.head.repo.full_name != github.repository && github.event.pull_request.base.ref == 'main' && github.event.pull_request.state == 'open' - && github.event.pull_request.maintainer_can_modify == true && (github.event.pull_request.user.login == (vars.AUTOFIX_BOT_LOGIN || 'qwen-code-dev-bot') || contains(toJSON(github.event.pull_request.labels.*.name), '"autofix/takeover"')) && !contains(toJSON(github.event.pull_request.labels.*.name), '"autofix/skip"') diff --git a/scripts/tests/qwen-autofix-fork-bridge-workflow.test.js b/scripts/tests/qwen-autofix-fork-bridge-workflow.test.js index d3135a3ecf7..0a6db234f5b 100644 --- a/scripts/tests/qwen-autofix-fork-bridge-workflow.test.js +++ b/scripts/tests/qwen-autofix-fork-bridge-workflow.test.js @@ -94,6 +94,25 @@ const expressionsIn = (text) => { const readsASecret = (text) => expressionsIn(text).some((expression) => /\bsecrets\./.test(expression)); +// GitHub Actions reaches a property through the documented `[ ]` index +// operator as readily as through `.`, on any segment of the path, so +// `pull_request['maintainer_can_modify']` resolves to the same absent field +// as the dot form and closes the gate just as permanently. Rewriting the +// index form to the dot form first means one matcher covers every +// combination of the two, at any depth, instead of enumerating spellings. +// (A `fromJSON(toJSON(github.event.pull_request))` round-trip still evades +// this — no textual guard catches that one; it needs a live payload.) +const asDotAccess = (text) => + text.replace(/\[\s*(['"])([A-Za-z_][A-Za-z0-9_]*)\1\s*\]/g, '.$2'); + +// Four of the full-object-only names below are strict PREFIXES of fields the +// simple object does deliver (`merged` ⊂ `merged_at`, `commits` ⊂ `commits_url`, +// `comments` ⊂ `comments_url`, `review_comments` ⊂ `review_comments_url`), so a +// substring test would also reject those legal conjuncts. Anchor on a word +// boundary so only a reference to the field itself matches. +const fullObjectOnlyReference = (field) => + new RegExp(`pull_request\\.${field}\\b`); + describe('qwen autofix fork bridge', () => { it('keeps the trigger name and dispatch target wired together', () => { // Cross-FILE contracts that no single file can be read to verify. Each @@ -219,7 +238,6 @@ describe('qwen autofix fork bridge', () => { '&& github.event.pull_request.head.repo.full_name != github.repository', "&& github.event.pull_request.base.ref == 'main'", "&& github.event.pull_request.state == 'open'", - '&& github.event.pull_request.maintainer_can_modify == true', `&& (github.event.pull_request.user.login == (vars.AUTOFIX_BOT_LOGIN || '${BOT_FALLBACK}')`, `|| contains(toJSON(github.event.pull_request.labels.*.name), '"${TAKEOVER_LABEL}"'))`, `&& !contains(toJSON(github.event.pull_request.labels.*.name), '"${SKIP_LABEL}"')`, @@ -256,6 +274,95 @@ describe('qwen autofix fork bridge', () => { ); }); + it('gates the signal only on fields the review payload actually delivers', () => { + // `pull_request_review` delivers the SIMPLE pull-request object. These + // fields exist only on the FULL object the `pull_request` event sends, so + // in this gate each one evaluates to null — and `null == true` is false, + // which closes the gate for every delivery rather than for the case the + // author meant to exclude. That is not hypothetical: `maintainer_can_modify` + // sat in this gate from #8676 (2026-08-07) until this test was written, and + // across 300 signal runs not one reached the signal step. + // + // A silent always-false is the worst failure this file can have. The gate + // has no observable output when it holds — the whole job is one echo — so a + // gate that never opens is indistinguishable from a repository where no fork + // review happened to qualify. Fork reviews just quietly fall back to the + // cron backstop, and when THAT stalls they fall through entirely. + // + // Anything on this list that the chain genuinely needs belongs in the + // bridge, which reads live PR state with credentials this job deliberately + // does not hold (`permissions: {}`, no secrets, no checkout). + const fullObjectOnlyFields = [ + 'maintainer_can_modify', + 'mergeable', + 'mergeable_state', + 'rebaseable', + 'merged', + 'merged_by', + 'additions', + 'deletions', + 'changed_files', + 'commits', + 'comments', + 'review_comments', + ]; + for (const field of fullObjectOnlyFields) { + expect(asDotAccess(signalJob.if)).not.toMatch( + fullObjectOnlyReference(field), + ); + } + }); + + // Every spelling GitHub Actions accepts for one field reference. The gate + // is written in the dot form today, but an edit in any of the others reaches + // the same absent field and closes the gate just as permanently, so the + // guard has to see through all of them — and through none of them reject a + // delivered field that a full-object name merely prefixes. + const referenceSpellings = (field) => [ + `github.event.pull_request.${field}`, + `github.event.pull_request['${field}']`, + `github.event.pull_request["${field}"]`, + // Actions tolerates whitespace inside the index brackets, and so does + // `asDotAccess`. Without a spelling that carries it, the helper's two + // `\s*` are unpinned: deleting them leaves this file green while a gate + // written in the spaced form reaches the same absent field again. + `github.event.pull_request[ '${field}' ]`, + `github.event['pull_request'].${field}`, + `github.event['pull_request']['${field}']`, + ]; + + it('rejects a full-object field without rejecting the fields it prefixes', () => { + const prefixPairs = [ + ['merged', 'merged_at'], + ['commits', 'commits_url'], + ['comments', 'comments_url'], + ['review_comments', 'review_comments_url'], + ]; + for (const [fullObjectOnly, delivered] of prefixPairs) { + for (const spelling of referenceSpellings(delivered)) { + expect(asDotAccess(`${spelling} == null`)).not.toMatch( + fullObjectOnlyReference(fullObjectOnly), + ); + } + for (const spelling of referenceSpellings(fullObjectOnly)) { + expect(asDotAccess(`${spelling} == false`)).toMatch( + fullObjectOnlyReference(fullObjectOnly), + ); + } + } + }); + + it('sees a full-object field through the index operator', () => { + // The incident field, in the five non-dot spellings. Each is a legal + // expression resolving to the same absent property, so a guard blind to + // any of them lets the always-false gate return with this file green. + for (const spelling of referenceSpellings('maintainer_can_modify')) { + expect(asDotAccess(`${spelling} == true`)).toMatch( + fullObjectOnlyReference('maintainer_can_modify'), + ); + } + }); + it('keeps every identity literal pinned to qwen-autofix.yml', () => { // The fork bridge cannot import qwen-autofix.yml's env, so the copies it // must make are pinned HERE: if the review-bot identity, the trust list,