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
89 changes: 75 additions & 14 deletions .github/workflows/qwen-autofix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -274,21 +274,53 @@ jobs:
if [[ "${EVENT_NAME}" == 'schedule' ]]; then
DO_ISSUE=true
fi
# Real-time review triggers: only process in-repo bot PRs with
# reviews from trusted senders (collaborators or the review bot).
# This prevents arbitrary commenters from forcing expensive
# Real-time review triggers: process the SAME managed set the
# scheduled scan does, so feedback is picked up seconds after the
# review instead of waiting for a schedule GitHub throttles hard
# (the */10 cron actually lands every 40-70min on this repo).
# Reviews must come from trusted senders (collaborators or the
# review bot) so arbitrary commenters cannot force expensive
# review-scan runs. Only pull_request_review:submitted triggers
# (not per-comment events) to avoid redundant runs on multi-comment
# reviews.
# (not per-comment events) to avoid redundant runs on
# multi-comment reviews.
if [[ "${EVENT_NAME}" == 'pull_request_review' ]]; then
DO_ISSUE=false
if [[ "${PR_AUTHOR}" != "${AUTOFIX_BOT}" ]]; then
echo "🧭 review event ignored: PR author '${PR_AUTHOR}' is not ${AUTOFIX_BOT}"
elif [[ "${PR_HEAD_REPO}" != "${REPO}" ]]; then
echo "🧭 review event ignored: PR is a fork (${PR_HEAD_REPO} != ${REPO})"
elif [[ "${PR_BASE_REF}" != "main" ]]; then
pr_is_managed=false
if [[ "${PR_BASE_REF}" != "main" ]]; then
echo "🧭 review event ignored: PR targets '${PR_BASE_REF}' not 'main'"
elif [[ "${PR_HEAD_REPO}" == "${REPO}" ]]; then
if [[ "${PR_AUTHOR}" == "${AUTOFIX_BOT}" ]]; then
pr_is_managed=true
else
echo "🧭 review event ignored: PR author '${PR_AUTHOR}' is not ${AUTOFIX_BOT}"
fi
else
# Fork PR. The scheduled scan already admits these for
# takeover, so real-time pickup applies the SAME admission
# (allow-edits on, and either the bot's own fork or an
# explicit TAKEOVER_LABEL) rather than making the takeover
# PRs — the ones a maintainer is actively waiting on — sit
# through a throttled schedule. This event runs in BASE-repo
# context, and review-address independently re-verifies
# allow-edits, a live write+ author and a matching live head
# repo before it touches the branch, so this only decides
# WHEN that same gated work happens, never whether it may.
fork_meta=''
if fork_meta="$(gh pr view "${PR_NUMBER_EVENT}" --repo "${REPO}" --json labels,maintainerCanModify 2> /dev/null)"; then
fork_allows_edits="$(jq -r '.maintainerCanModify == true' <<< "${fork_meta}")"
fork_has_takeover="$(jq -r --arg t "${TAKEOVER_LABEL}" '[.labels[]?.name] | index($t) != null' <<< "${fork_meta}")"
if [[ "${fork_allows_edits}" != 'true' ]]; then
echo "🧭 review event ignored: fork PR #${PR_NUMBER_EVENT} does not allow maintainer edits"
elif [[ "${PR_AUTHOR}" == "${AUTOFIX_BOT}" || "${fork_has_takeover}" == 'true' ]]; then
pr_is_managed=true
else
echo "🧭 review event ignored: fork PR #${PR_NUMBER_EVENT} is neither ${AUTOFIX_BOT}'s own fork nor ${TAKEOVER_LABEL}-labeled"
fi
else
echo "🧭 review event ignored: could not read fork PR #${PR_NUMBER_EVENT} metadata"
fi
fi
if [[ "${pr_is_managed}" == 'true' ]]; then
# Verify the reviewer/commenter is trusted (prompt-injection gate).
sender_permission=''
sender_is_trusted=false
Expand Down Expand Up @@ -1492,15 +1524,44 @@ jobs:
# input and silently green-no-op'd all forced dispatches.
if [[ -n "${FORCED_PR}" ]]; then
META="$(gh pr view "${FORCED_PR}" --repo "${REPO}" \
--json number,state,author,headRefName,isCrossRepository,baseRefName,labels 2> /dev/null || echo '{}')"
--json number,state,author,headRefName,isCrossRepository,baseRefName,labels,maintainerCanModify 2> /dev/null || echo '{}')"
# Same admission as the scheduled scan below. In-repo PRs fail
# CLOSED on a missing isCrossRepository field (`.isCrossRepository
# == false`, never a `// true | not` default — jq's // treats false
# as empty, so that form is false for EVERY input and silently
# green-no-op'd all forced dispatches). Fork PRs are admitted under
# the scan's OWN fork rules (allow-edits on; the live write+ author
# gate runs in the shell case just below, mirroring the scan's
# per-candidate permission call) so the real-time route's fork
# pickup is not silently discarded here.
OK="$(jq -r --arg ab "${AUTOFIX_BOT}" --arg take "${TAKEOVER_LABEL}" --arg skip "${SKIP_LABEL}" \
'(((.state // "") == "OPEN")
and (((.author.login // "") == $ab) or ([.labels[]?.name] | index($take) != null))
and ([.labels[]?.name] | index($skip) | not)
and (.isCrossRepository == false)
and ((.baseRefName // "") == "main"))' <<< "${META}")"
and ((.baseRefName // "") == "main")
and (if (.isCrossRepository == true)
then (.maintainerCanModify == true)
else (.isCrossRepository == false)
end))' <<< "${META}")"
# Fork only: the author must hold write+ RIGHT NOW (the same
# live-privilege rule the scan applies per candidate and
# review-address re-checks before pushing). In-repo PRs are gated
# by author/label alone.
if [[ "${OK}" == 'true' && "$(jq -r '.isCrossRepository == true' <<< "${META}")" == 'true' ]]; then
FORK_AUTHOR="$(jq -r '.author.login // ""' <<< "${META}")"
FPERM="$(gh api "repos/${REPO}/collaborators/${FORK_AUTHOR}/permission" --jq '.permission // ""' 2> /dev/null || echo '')"
case "${FPERM}" in
admin|maintain|write)
echo "🌿 forced fork PR #${FORCED_PR} admitted (author ${FORK_AUTHOR}=${FPERM})"
;;
*)
echo "🧭 forced fork PR #${FORCED_PR} rejected: author ${FORK_AUTHOR} permission='${FPERM:-none}' below write"
OK='false'
;;
esac
fi
if [[ "${OK}" != "true" ]]; then
echo "❌ #${FORCED_PR} is not an open in-repo main-targeting PR owned by ${AUTOFIX_BOT} or labeled ${TAKEOVER_LABEL} (or it carries ${SKIP_LABEL}); fork takeover PRs are engaged by the scheduled scan, not manual dispatch"
echo "❌ #${FORCED_PR} is not an open main-targeting PR owned by ${AUTOFIX_BOT} or labeled ${TAKEOVER_LABEL} (or it carries ${SKIP_LABEL}); a fork PR additionally needs maintainer edits allowed and a live write+ author"
echo "targets=[]" >> "${GITHUB_OUTPUT}"
echo "has_targets=false" >> "${GITHUB_OUTPUT}"
exit 0
Expand Down
182 changes: 173 additions & 9 deletions scripts/tests/qwen-autofix-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ describe('qwen-autofix workflow', () => {
expect(workflow).toContain("MAX_ROUNDS: '5'");
expect(workflow).toContain("MAX_OPEN_AUTOFIX_PRS: '5'");
expect(reviewScanJob).toContain('isCrossRepository');
expect(reviewScanJob).toContain('not an open in-repo main-targeting PR');
expect(reviewScanJob).toContain('not an open main-targeting PR');
// Candidates fail CLOSED on the fork field, matching the forced path
// and the NOTE that documents the jq // false trap.
expect(reviewScanJob).toContain('select(.isCrossRepository == false)');
Expand Down Expand Up @@ -731,21 +731,28 @@ describe('qwen-autofix workflow', () => {
);
});

it('routes submitted review events only for trusted in-repo bot PRs', () => {
it('routes submitted review events only for trusted managed PRs', () => {
expect(routeStep).toContain('PR_AUTHOR');
expect(routeStep).toContain('PR_NUMBER_EVENT');
expect(routeStep).toContain(
'if [[ "${EVENT_NAME}" == \'pull_request_review\' ]]; then',
);
expect(routeStep).toContain('"${PR_AUTHOR}" != "${AUTOFIX_BOT}"');
expect(routeStep).toContain('"${PR_HEAD_REPO}" != "${REPO}"');
// In-repo PRs are managed only when the bot authored them; forks only
// under the scan's takeover rules (allow-edits + bot fork or the label).
expect(routeStep).toContain('"${PR_AUTHOR}" == "${AUTOFIX_BOT}"');
expect(routeStep).toContain('"${PR_HEAD_REPO}" == "${REPO}"');
expect(routeStep).toContain('"${PR_BASE_REF}" != "main"');
expect(routeStep).toContain('.maintainerCanModify == true');
expect(routeStep).toContain('index($t) != null');
expect(routeStep).toContain(
'ROUTE_PR="$(sanitize_number "${PR_NUMBER_EVENT}")',
);
expect(routeStep).toContain(
"review event ignored: PR author '${PR_AUTHOR}' is not ${AUTOFIX_BOT}",
);
expect(routeStep).toContain(
'review event ignored: fork PR #${PR_NUMBER_EVENT} does not allow maintainer edits',
);
});

it('keeps label-triggered issue routing guarded and diagnosable', () => {
Expand Down Expand Up @@ -1588,7 +1595,10 @@ describe('qwen-autofix workflow', () => {
it('behaviorally validates forced targets against author, takeover, and skip', () => {
// Extract the forced-PR OK predicate VERBATIM and replay it: the bot's
// own PRs pass; a human PR passes only with the takeover label; skip
// vetoes even a takeover-labeled PR; closed and fork PRs never pass.
// vetoes even a takeover-labeled PR; closed PRs never pass. A fork PR
// passes the structural predicate only with maintainer edits allowed — the
// live write+ author gate is a shell step below (asserted separately),
// mirroring the scheduled scan's per-candidate fork admission.
const okProgram = reviewScanJob.match(
/OK="\$\(jq -r --arg ab "\$\{AUTOFIX_BOT\}" --arg take "\$\{TAKEOVER_LABEL\}" --arg skip "\$\{SKIP_LABEL\}" \\\n\s+'([\s\S]*?)'/,
)?.[1];
Expand Down Expand Up @@ -1629,6 +1639,26 @@ describe('qwen-autofix workflow', () => {
expect(ok(meta('human', ['autofix/takeover'], { state: 'CLOSED' }))).toBe(
'false',
);
// Fork PRs: admitted structurally only when maintainer edits are allowed
// (the bot's own fork or a takeover-labelled fork). The live write+ author
// check is the shell gate asserted below; without allow-edits a fork still
// fails closed here.
expect(
ok(
meta('human', ['autofix/takeover'], {
isCrossRepository: true,
maintainerCanModify: true,
}),
),
).toBe('true');
expect(
ok(
meta('qwen-code-dev-bot', [], {
isCrossRepository: true,
maintainerCanModify: true,
}),
),
).toBe('true');
expect(
ok(meta('human', ['autofix/takeover'], { isCrossRepository: true })),
).toBe('false');
Expand All @@ -1641,6 +1671,16 @@ describe('qwen-autofix workflow', () => {
expect(ok(missing)).toBe('false');
expect(reviewScanJob).toContain('.isCrossRepository == false');
expect(reviewScanJob).not.toContain('(.isCrossRepository // true) | not');
// The forced path queries maintainerCanModify and re-checks a fork author's
// live permission exactly like the scheduled scan's per-candidate gate, so
// a fork the route admitted in real time is not silently discarded here.
expect(reviewScanJob).toContain(
'--json number,state,author,headRefName,isCrossRepository,baseRefName,labels,maintainerCanModify',
);
expect(reviewScanJob).toContain('forced fork PR #${FORCED_PR} admitted');
expect(reviewScanJob).toContain(
'gh api "repos/${REPO}/collaborators/${FORK_AUTHOR}/permission"',
);
});

it('exposes exactly one comment command: label-toggle takeover sugar', () => {
Expand Down Expand Up @@ -1829,16 +1869,19 @@ describe('qwen-autofix workflow', () => {
});

it('gates real-time review triggers on bot author, trusted sender, and in-repo PR', () => {
// Route step must check PR author against AUTOFIX_BOT for review events.
expect(routeStep).toContain('"${PR_AUTHOR}" != "${AUTOFIX_BOT}"');
// Route step must check PR author against AUTOFIX_BOT for review events
// (an in-repo PR is managed only when the bot authored it).
expect(routeStep).toContain('"${PR_AUTHOR}" == "${AUTOFIX_BOT}"');
// Must verify sender is trusted (collaborator or review bot).
expect(routeStep).toContain('"${SENDER_LOGIN}" == "${REVIEW_BOT}"');
expect(routeStep).toContain(
'gh api "repos/${REPO}/collaborators/${SENDER_LOGIN}/permission"',
);
// Must reject fork PRs and non-main targets.
expect(routeStep).toContain('"${PR_HEAD_REPO}" != "${REPO}"');
// Non-main targets are rejected; forks are admitted only under the scan's
// own takeover rules (allow-edits + bot fork or takeover label).
expect(routeStep).toContain('"${PR_BASE_REF}" != "main"');
expect(routeStep).toContain('"${PR_HEAD_REPO}" == "${REPO}"');
expect(routeStep).toContain('--json labels,maintainerCanModify');
// Must set ROUTE_PR from the event payload.
expect(routeStep).toContain(
'ROUTE_PR="$(sanitize_number "${PR_NUMBER_EVENT}")"',
Expand All @@ -1858,6 +1901,127 @@ describe('qwen-autofix workflow', () => {
);
});

it('admits managed fork PRs to the real-time review trigger, not just in-repo bot PRs', () => {
// The */10 schedule is throttled to 40-70min on this repo, so a takeover PR
// that only the scan could pick up waited up to an hour for feedback the
// event already carried. Real-time pickup now applies the scan's OWN fork
// admission (allow-edits + the bot's own fork or an explicit takeover
// label); review-address still re-verifies allow-edits, a live write+
// author and a matching head repo before touching the branch.
const block = routeStep.match(
/if \[\[ "\$\{EVENT_NAME\}" == 'pull_request_review' \]\]; then[\s\S]*?\n {14}fi/,
)?.[0];
expect(block).toBeTruthy();

const run = ({
headRepo,
author,
base = 'main',
sender = 'alice',
allowEdits = true,
labels = [],
perm = 'write',
metaOk = true,
}) => {
const dir = mkdtempSync(join(tmpdir(), 'route-'));
const bin = join(dir, 'bin');
mkdirSync(bin);
const meta = JSON.stringify({
maintainerCanModify: allowEdits,
labels: labels.map((name) => ({ name })),
});
writeFileSync(
join(bin, 'gh'),
[
'#!/usr/bin/env bash',
`if [[ "$*" == *"--json labels,maintainerCanModify"* ]]; then ${
metaOk ? `printf '%s' ${JSON.stringify(meta)}; exit 0` : 'exit 1'
}; fi`,
`if [[ "$*" == *permission* ]]; then printf '%s' ${JSON.stringify(perm)}; exit 0; fi`,
'exit 1',
].join('\n'),
);
chmodSync(join(bin, 'gh'), 0o755);
const out = execFileSync(
'bash',
[
'-c',
[
'set -uo pipefail',
'sanitize_number() { printf "%s" "${1//[^0-9]/}"; }',
'DO_ISSUE=true; DO_REVIEW=false; ROUTE_PR=""',
block,
'printf "DO_REVIEW=%s ROUTE_PR=%s" "${DO_REVIEW}" "${ROUTE_PR}"',
].join('\n'),
],
{
env: {
...process.env,
PATH: `${bin}:${process.env.PATH}`,
EVENT_NAME: 'pull_request_review',
REPO: 'QwenLM/qwen-code',
AUTOFIX_BOT: 'qwen-code-dev-bot',
REVIEW_BOT: 'qwen-code-ci-bot',
TAKEOVER_LABEL: 'autofix/takeover',
PR_NUMBER_EVENT: '7259',
PR_HEAD_REPO: headRepo,
PR_AUTHOR: author,
PR_BASE_REF: base,
SENDER_LOGIN: sender,
},
encoding: 'utf8',
},
);
rmSync(dir, { recursive: true, force: true });
return out;
};

const IN_REPO = 'QwenLM/qwen-code';
const FORK = 'wenshao/qwen-code';
// Unchanged: an in-repo bot PR is admitted, a human in-repo PR is not.
expect(run({ headRepo: IN_REPO, author: 'qwen-code-dev-bot' })).toContain(
'DO_REVIEW=true',
);
expect(run({ headRepo: IN_REPO, author: 'someone' })).toContain(
'DO_REVIEW=false',
);
// NEW: the bot's own fork, and a takeover-labelled human fork, are admitted
// in real time and route to that exact PR.
expect(run({ headRepo: FORK, author: 'qwen-code-dev-bot' })).toContain(
'DO_REVIEW=true',
);
expect(
run({ headRepo: FORK, author: 'wenshao', labels: ['autofix/takeover'] }),
).toContain('DO_REVIEW=true');
expect(run({ headRepo: FORK, author: 'qwen-code-dev-bot' })).toContain(
'ROUTE_PR=7259',
);
// Still rejected: no allow-edits, an unlabelled human fork, a non-main
// base, and an untrusted sender.
expect(
run({ headRepo: FORK, author: 'qwen-code-dev-bot', allowEdits: false }),
).toContain('DO_REVIEW=false');
expect(run({ headRepo: FORK, author: 'wenshao' })).toContain(
'DO_REVIEW=false',
);
expect(
run({ headRepo: IN_REPO, author: 'qwen-code-dev-bot', base: 'release' }),
).toContain('DO_REVIEW=false');
expect(
run({
headRepo: FORK,
author: 'wenshao',
labels: ['autofix/takeover'],
perm: 'read',
}),
).toContain('DO_REVIEW=false');
// A metadata read failure fails CLOSED: the event is ignored rather than
// admitting a fork whose allow-edits/labels could not be verified.
expect(
run({ headRepo: FORK, author: 'qwen-code-dev-bot', metaOk: false }),
).toContain('DO_REVIEW=false');
});

it('treats Suggestion-level review findings as actionable feedback', () => {
// AGENTS.md: Suggestions ARE addressed during a PR's first ~5 review
// rounds; only past that are they deferred with a recorded reason. The
Expand Down
Loading