Skip to content

fix(ci): match /review commands followed by a newline #8723

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.

Sign up for GitHub

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

Merged
wenshao merged 1 commit into from
Aug 8, 2026
Merged

fix(ci): match /review commands followed by a newline #8723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 33 additions & 8 deletions .github/workflows/qwen-code-pr-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ jobs:
# KEEP IN SYNC with review-pr.if (explicit-trigger branches).
# Authorization is delegated to the `authorize` job (write+ permission);
# this `if` only matches the /review command shape.
#
# The command may be followed by a newline and a body, so the shape match
# accepts that — via fromJSON, because expression string literals are NOT
# escape-processed: '\n' there is a literal backslash + n, and the branch
# written that way never matched anything. fromJSON('"\n"') is JSON, which
# IS escape-processed, so it yields a real newline. Both line endings are
# listed: the API sends LF, the web UI sends CRLF, and startsWith with an
# LF pattern does not match a CRLF body. Measured on a live runner, not
# assumed — see the PR that introduced this.
needs: ['authorize']
if: |-
!cancelled() &&
Expand All @@ -87,17 +96,20 @@ jobs:
github.event.issue.state == 'open' &&
(github.event.comment.body == '@qwen-code /review' ||
startsWith(github.event.comment.body, '@qwen-code /review ') ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n')))) ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"'))))) ||
(github.event_name == 'pull_request_review_comment' &&
github.event.pull_request.state == 'open' &&
(github.event.comment.body == '@qwen-code /review' ||
startsWith(github.event.comment.body, '@qwen-code /review ') ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n')))) ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"'))))) ||
(github.event_name == 'pull_request_review' &&
github.event.pull_request.state == 'open' &&
(github.event.review.body == '@qwen-code /review' ||
startsWith(github.event.review.body, '@qwen-code /review ') ||
startsWith(github.event.review.body, format('@qwen-code /review{0}', '\n')))))
startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) ||
startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\r"'))))))
concurrency:
group: 'qwen-pr-ack-${{ github.event.issue.number || github.event.pull_request.number }}'
cancel-in-progress: false
Expand Down Expand Up @@ -315,7 +327,8 @@ jobs:
# review_requested events check the requester and skip delay):
# - opened/synchronize uses delay-automatic-review
# - reopened/ready_for_review runs immediately
# KEEP IN SYNC with ack-review-request.if (explicit-trigger branches).
# KEEP IN SYNC with ack-review-request.if (explicit-trigger branches) —
# including the fromJSON newline/CR pair, explained there.
Comment on lines +330 to +331

@wenshao wenshao Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Nothing pins the ack-review-request.if ↔ review-pr.if shape contract to each other; test 2 verifies each job's LF/CR pair independently, so a one-job shape edit desyncs the pair with the suite green.

Failure scenario: the same /review shapes live in two jobs (ack-review-request and review-pr) plus resolve-pr — 7 matches, guarded only by "KEEP IN SYNC" comments. A future contributor who adds a trigger branch or edits the shape list in only one of the two jobs leaves the pair desynced while test 2 still passes. ack-review-request then posts "Review queued" for a body that review-pr.if no longer matches — the review silently never starts, the exact silent-ignore bug class this PR fixes, with all three new tests green. This PR is itself the edit class: it changed the same shapes in two jobs in one commit.

Suggested change
# KEEP IN SYNC with ack-review-request.if (explicit-trigger branches) —
# including the fromJSON newline/CR pair, explained there.
const shapeOf = (id) =>
ifs
.filter(([jid]) => jid === id)
.flatMap(([, cond]) =>
[...cond.matchAll(/format\('@qwen-code \/(\w+)\{0\}'/g)].map((m) => m[1]),
);
expect(shapeOf('review-pr')).toEqual(shapeOf('ack-review-request'));
中文说明

/review 的形状匹配同时存在于 ack-review-request 和 review-pr 两个 job（加上 resolve-pr 共 7 处），目前只靠 "KEEP IN SYNC" 注释维系。测试 2 只独立检查每个 job 的 LF/CR 对，因此未来只改其中一个 job（新增触发分支或修改形状列表）会让两者失同步而套件依然全绿：ack-review-request 对某个 body 发“Review queued”，而 review-pr.if 已不再匹配——评审静默不启动，正是本 PR 修复的静默忽略类 bug。本 PR 本身就是这类编辑：一个 commit 里改了同一形状在两处 job。建议加断言：两个 job 的显式触发形状集合必须完全一致。

— deepseek-v4-flash via Qwen Code /review (v0.21.7)

if: |-
!cancelled() &&
((github.event_name == 'workflow_dispatch' &&
Expand All @@ -335,19 +348,22 @@ jobs:
github.event.issue.state == 'open' &&
(github.event.comment.body == '@qwen-code /review' ||
startsWith(github.event.comment.body, '@qwen-code /review ') ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n'))) &&
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"')))) &&
needs.authorize.outputs.should_review == 'true') ||
(github.event_name == 'pull_request_review_comment' &&
github.event.pull_request.state == 'open' &&
(github.event.comment.body == '@qwen-code /review' ||
startsWith(github.event.comment.body, '@qwen-code /review ') ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n'))) &&
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) ||
startsWith(github.event.comment.body, format('@qwen-code /review{0}', fromJSON('"\r"')))) &&
needs.authorize.outputs.should_review == 'true') ||
(github.event_name == 'pull_request_review' &&
github.event.pull_request.state == 'open' &&
(github.event.review.body == '@qwen-code /review' ||
startsWith(github.event.review.body, '@qwen-code /review ') ||
startsWith(github.event.review.body, format('@qwen-code /review{0}', '\n'))) &&
startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\n"'))) ||
startsWith(github.event.review.body, format('@qwen-code /review{0}', fromJSON('"\r"')))) &&
needs.authorize.outputs.should_review == 'true'))
# The per-review budget auto-scales to QWEN_REVIEW_MAX_TIMEOUT_MINUTES
# for any non-small PR (see "Run review"), and the shared retry budget
Expand Down Expand Up @@ -450,7 +466,13 @@ jobs:
# review" is an explicit ask (authorize write-permission-checks the
# requester for exactly that action), so it is excluded below.
AUTO_REVIEW=false
# First line only, then drop a trailing CR: comments written in the
# GitHub web UI arrive CRLF-terminated, so without this the command
Comment on lines +469 to +470

@wenshao wenshao Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The new LF/CR shape matches make multi-line /review commands run for the first time, but the pre-existing first-line truncation TRIGGER_COMMAND="${TRIGGER_BODY%%$'\n'*}" discards everything after the first line break before the for token in $TRIGGER_COMMAND loop, so --timeout=/timeout= options written on line 2+ are silently dropped.

Failure scenario: a commenter writes @qwen-code /review and --timeout=30 on the next line — web UI CRLF body or API/bot LF body, both newly accepted by this PR. Before this PR such a body matched no branch and nothing ran. Now the job runs, the truncation keeps only @qwen-code /review, the token loop never sees --timeout=30, and the review proceeds with the default/size-aware budget (180 min, or up to QWEN_REVIEW_MAX_TIMEOUT_MINUTES for non-small PRs) instead of the requested 30 — the commenter's explicit resource cap is silently ignored with no warning (probe-verified: line-2 options yield TIMEOUT_MINUTES=180 TIMEOUT_EXPLICIT=false; parsing the remainder flips to the requested 30).

Suggested change
# First line only, then drop a trailing CR: comments written in the
# GitHub web UI arrive CRLF-terminated, so without this the command
TRIGGER_COMMAND="${TRIGGER_BODY%%$'\n'*}"
TRIGGER_COMMAND="${TRIGGER_COMMAND%$'\r'}"
REMAINDER="${TRIGGER_BODY#*$'\n'}"
case "$REMAINDER" in
*--timeout=*|*timeout=*)
echo "::warning::--timeout on a later line is ignored; put it on the command line" >&2 ;;
esac
中文说明

本 PR 让多行 /review 命令第一次真正生效，但既有的“只取第一行”截断 TRIGGER_COMMAND="${TRIGGER_BODY%%$'\n'*}" 会把第二行及之后的内容全部丢弃——写在第二行的 --timeout=30 等选项永远不会进入 token 循环。写“命令 + 下一行选项”的评论（网页端 CRLF 或 API 的 LF，本 PR 新接受的两种形态）会在没有任何提示的情况下按默认/按规模预算运行（180 分钟，非小型 PR 最高可达 QWEN_REVIEW_MAX_TIMEOUT_MINUTES），而不是用户要求的 30 分钟，最多浪费 6-12 倍算力。建议截断后检查被丢弃的剩余部分，要么解析其中的 --timeout=，要么至少输出一条可见警告。

— deepseek-v4-flash via Qwen Code /review (v0.21.7)

# line keeps a `\r`. That rides through word splitting (IFS has no
# CR) into tokens like `--timeout=300<CR>`, which then fails the
# numeric check for no visible reason.
TRIGGER_COMMAND="${TRIGGER_BODY%%$'\n'*}"
TRIGGER_COMMAND="${TRIGGER_COMMAND%$'\r'}"

if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
PR_NUMBER="${{ github.event.inputs.pr_number }}"
Expand Down Expand Up @@ -1604,6 +1626,8 @@ jobs:

resolve-pr:
needs: ['authorize']
# The /resolve shape match uses the same fromJSON newline/CR pair as
# ack-review-request.if, and for the same reason — see the note there.
if: |-
!cancelled() &&
github.repository == 'QwenLM/qwen-code' &&
Expand All @@ -1616,7 +1640,8 @@ jobs:
github.event.issue.state == 'open' &&
(github.event.comment.body == '@qwen-code /resolve' ||
startsWith(github.event.comment.body, '@qwen-code /resolve ') ||
startsWith(github.event.comment.body, format('@qwen-code /resolve{0}', '\n'))))
startsWith(github.event.comment.body, format('@qwen-code /resolve{0}', fromJSON('"\n"'))) ||
startsWith(github.event.comment.body, format('@qwen-code /resolve{0}', fromJSON('"\r"')))))
)
# Pinned to an ephemeral hosted runner. The conflict-resolution agent step
# runs with `sandbox: true`, which on Linux needs docker or podman to launch
Expand Down
67 changes: 67 additions & 0 deletions scripts/tests/qwen-pr-review-workflow.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2207,3 +2207,70 @@ describe('workflow expression length', () => {
expect(runReviewStep()).not.toContain('${{');
});
});

describe('command shape matching', () => {
// A comment may be `@qwen-code /review` followed by a newline and a body.
// The `if`s tried to accept that with format('…{0}', '\n'), but expression
// string literals are NOT escape-processed: that '\n' is a literal
// backslash + n, so the branch matched nothing and every multi-line command
// was silently ignored — no run, no feedback. Measured on a live runner:
// startsWith(<LF body>, format('…{0}', '\n')) => false
// startsWith(<LF body>, format('…{0}', fromJSON('"\n"'))) => true
// startsWith(<CRLF body>, format('…{0}', fromJSON('"\n"'))) => false
// startsWith(<CRLF body>, format('…{0}', fromJSON('"\r"'))) => true
// Hence fromJSON (JSON *is* escape-processed) and both line endings: the
// REST API sends LF, the web UI sends CRLF.
const doc = parse(workflow);
const ifs = Object.entries(doc.jobs)
.filter(([, job]) => typeof job?.if === 'string')
.map(([id, job]) => [id, job.if]);

it('never matches a command shape with a non-escaped literal newline', () => {
const broken = ifs.filter(([, cond]) => /'\\[nr]'/.test(cond));
expect(broken.map(([id]) => id)).toEqual([]);
});

it('accepts both LF and CRLF after the command in every shape match', () => {
// `authorize` deliberately matches only a loose prefix — it is a filter to
// avoid spawning a job per comment, and delegates the exact shape to the
// downstream jobs. Jobs that do the shape match are the ones that use
// format('@qwen-code /<cmd>{0}', …), so key off that.
const withShape = ifs.filter(([, cond]) =>
cond.includes("format('@qwen-code /"),
);
Comment on lines +2238 to +2240

@wenshao wenshao Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Test 2 verifies the LF/CR pair per job with a single includes over the whole multi-branch if string, so a regression in any one of the three trigger branches per job (issue_comment, pull_request_review_comment, pull_request_review) passes all three new tests.

Failure scenario: mutation-demonstrated — replacing the CR variant with the LF variant in review-pr's pull_request_review branch, or deleting the CR line from ack-review-request's issue_comment branch, both leave tests 1 and 2 green, because the CR form still exists in the other two branches. A web-UI CRLF /review comment on that one trigger is then silently ignored again — the exact bug class this PR fixes — with the suite green.

Suggested change
const withShape = ifs.filter(([, cond]) =>
cond.includes("format('@qwen-code /"),
);
const missing = [];
for (const [id, cond] of withShape) {
for (const cmd of ['review', 'resolve']) {
// Only check commands this job actually matches on.
if (!cond.includes(`format('@qwen-code /${cmd}{0}'`)) continue;
const lf = (cond.match(/fromJSON\('"\\n"'\)/g) ?? []).length;
const cr = (cond.match(/fromJSON\('"\\r"'\)/g) ?? []).length;
if (lf !== cr || lf === 0) missing.push(`${id}/${cmd} (LF:${lf} CR:${cr})`);
}
}
expect(missing).toEqual([]);
中文说明

测试 2 对每个 job 用一次 includes 检查 LF/CR 对，只看“存在”而非“每个分支都有”。实测变异验证：把 review-pr 的 pull_request_review 分支里的 CR 变体换成 LF，或删掉 ack-review-request 的 issue_comment 分支的 CR 行，测试 1 和测试 2 仍然全绿——因为另外两个分支里还有 CR 形式。后果：该触发分支上的网页端 CRLF /review 评论会再次被静默忽略——正是本 PR 修复的那类 bug，而套件依然全绿。建议改为按 (job, command) 统计 LF/CR 出现次数并断言两者相等且非零（如 lfCount === crCount && lfCount > 0），上述两种单分支变异都会让计数失衡从而失败。

— deepseek-v4-flash via Qwen Code /review (v0.21.7)

expect(withShape.length).toBeGreaterThan(0);
const missing = [];
for (const [id, cond] of withShape) {
for (const cmd of ['review', 'resolve']) {
// Only check commands this job actually matches on.
if (!cond.includes(`format('@qwen-code /${cmd}{0}'`)) continue;
const lf = cond.includes(
`format('@qwen-code /${cmd}{0}', fromJSON('"\\n"'))`,
);
const cr = cond.includes(
`format('@qwen-code /${cmd}{0}', fromJSON('"\\r"'))`,
);
if (!lf || !cr) missing.push(`${id}/${cmd} (LF:${lf} CR:${cr})`);
}
}
expect(missing).toEqual([]);
});

it('strips a trailing CR before parsing command tokens', () => {
// Word splitting uses IFS, which has no CR, so a CRLF comment would carry
// `\r` into tokens like `--timeout=300` and fail the numeric check.
// The command is parsed in "Resolve PR context", not in "Run review".
const run = parse(workflow).jobs['review-pr'].steps.find(
(s) => s.id === 'context',
).run;
const firstLine = run.indexOf(
'TRIGGER_COMMAND="${TRIGGER_BODY%%$\'\\n\'*}"',
);
const stripCr = run.indexOf(
'TRIGGER_COMMAND="${TRIGGER_COMMAND%$\'\\r\'}"',
);
expect(firstLine).toBeGreaterThan(-1);
expect(stripCr).toBeGreaterThan(-1);
expect(stripCr).toBeGreaterThan(firstLine);
Comment on lines +2272 to +2274

@wenshao wenshao Aug 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Test 3 ("strips a trailing CR before parsing command tokens") only pins the strip after the first-line extraction — it never pins the strip BEFORE the for token in $TRIGGER_COMMAND word-splitting loop, which is the ordering the fix actually exists for.

Failure scenario: the bug this PR fixes is specifically that a trailing CR survives into tokens (--timeout=300<CR>) and fails the *[!0-9]* numeric check at qwen-code-pr-review.yml:984-986. A future refactor that moves TRIGGER_COMMAND="${TRIGGER_COMMAND%$'\r'}" below the token loop would silently re-break every CRLF web-UI /review --timeout=N comment while all three new tests stay green (probe-verified: the mutation keeps all 3 tests green while the timeout value carries a trailing \r and the numeric check fails).

Suggested change
expect(firstLine).toBeGreaterThan(-1);
expect(stripCr).toBeGreaterThan(-1);
expect(stripCr).toBeGreaterThan(firstLine);
const parseLoop = run.indexOf('for token in $TRIGGER_COMMAND');
expect(firstLine).toBeGreaterThan(-1);
expect(stripCr).toBeGreaterThan(-1);
expect(stripCr).toBeGreaterThan(firstLine);
expect(stripCr).toBeLessThan(parseLoop);
中文说明

测试 3 的标题声称“在解析命令 token 之前剥离尾部 CR”，但它只断言了剥离发生在取首行之后，从未断言剥离发生在 for token in $TRIGGER_COMMAND 分词循环之前——而这才是修复真正要保证的顺序。一旦未来有人把 TRIGGER_COMMAND="${TRIGGER_COMMAND%$'\r'}" 挪到 token 循环之后，所有网页端 CRLF 的 /review --timeout=N 评论都会悄悄复发，而三条新测试依然全绿（探针已验证：该变异下测试全过，但 timeout 值带着 \r 并触发数字校验失败）。建议加一条断言：剥离行必须出现在分词循环之前。

— deepseek-v4-flash via Qwen Code /review (v0.21.7)

});
});
Loading