Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions .github/workflows/qwen-autofix-fork-signal.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"')
Expand Down
109 changes: 108 additions & 1 deletion scripts/tests/qwen-autofix-fork-bridge-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
Comment thread
qqqys marked this conversation as resolved.

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
Expand Down Expand Up @@ -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}"')`,
Expand Down Expand Up @@ -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}']`,
Comment thread
qqqys marked this conversation as resolved.
`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'),
);
}
Comment thread
qqqys marked this conversation as resolved.
});

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,
Expand Down
Loading