fix(ci): match /review commands followed by a newline - #8723
Merged
Conversation
A comment of `@qwen-code /review` plus a newline and a body has never
triggered anything. The shape match tried to accept it with
startsWith(body, format('@qwen-code /review{0}', '\n'))
but GitHub expression string literals are NOT escape-processed: that
'\n' is a literal backslash + n, so the branch matched nothing. The
command was silently ignored — no run, no feedback, in a path whose
whole job is to be the manual escape hatch.
Measured on a live runner rather than assumed:
startsWith(<LF body>, format(…, '\n')) => false
startsWith(<LF body>, format(…, fromJSON('"\n"'))) => true
startsWith(<CRLF body>, format(…, fromJSON('"\n"'))) => false
startsWith(<CRLF body>, format(…, fromJSON('"\r"'))) => true
fromJSON parses JSON, which IS escape-processed, so it yields a real
newline. Both endings are needed: the REST API sends LF, the web UI
sends CRLF, and an LF pattern does not match a CRLF body. Applied to all
7 shape matches (6 /review, 1 /resolve).
The shell half had the matching gap: the command line is taken as
everything before the first LF, which on CRLF keeps a trailing CR. IFS
has no CR, so word splitting produced tokens like `--timeout=300<CR>`
that failed the numeric check with no visible cause. Strip it.
Three tests pin this: no shape match may use a non-escape-processed
literal, every shape match must carry both endings, and the CR strip
must follow the first-line split. All three fail against the workflow on
main. Mutation-tested: dropping the CR branches or the CR strip each
fails exactly its own test.
Found while re-triggering the PRs stranded by the #8648 outage — 19
multi-line trigger comments were accepted by `authorize`, then silently
dropped by this branch.
Collaborator
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
yiliang114
approved these changes
Aug 8, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The root cause checks out — expression string literals are not escape-processed, so the old '\n' branch was dead code and multi-line commands were authorized then silently dropped. The fix is applied consistently across all three shape-matching ifs (ack, review-pr, resolve) with KEEP IN SYNC notes, covers both LF and CRLF bodies, and the trailing-CR strip closes the --timeout=300<CR> token hole. Regression tests pin both the absence of literal '\n' matches and the LF+CR pair on every shape matcher. CI green on this head.
Collaborator
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical. The multi-line Direction: aligned. This is the manual escape hatch for when automatic review doesn't fire, and it has been a silent no-op since the literal- Size: 2 files — workflow YAML +33/−8 (production), tests +67. No core paths ( Approach: the scope feels exactly right. Risk: no elevated risk signals (no high-risk path matches). Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到，而非理论性的。多行形式的 方向：对齐。这是自动评审未触发时的手动兜底路径，自字面量 规模：2 个文件——workflow YAML +33/−8（生产代码），测试 +67。未触及核心路径（ 方案：范围恰好。 风险：无升级风险信号（未命中高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewMy independent proposal for "multi-line Verified against the diff and
No blockers, no convention violations. TestingUnattended CI run — PR code not executed. Evidence below is the PR's own CI, read from the GitHub API for the reviewed commit.
The only red check is the bot's own Not verified, and structurally cannot be pre-merge: the end-to-end multi-line trigger — 中文说明代码审查我对"多行 对照 diff 与
无阻塞项，无规范违规。 测试无人值守 CI 运行——未执行 PR 代码。以上证据为 PR 自身的 CI，通过 GitHub API 就所审提交读取。唯一标红的检查是 bot 自己的 未验证、且合入前结构性不可验证：端到端的多行触发—— — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — observed bug with measured evidence, minimal correct fix, tests that pin it, green CI; the one uncheckable box (post-merge end-to-end) is structural to any workflow change, not a doubt about this PR. This is the second follow-up this workflow has needed after the #8648 outage, and it is exactly the shape of fix I'd want: small, fully explained, measured on a live runner rather than assumed, and regression-pinned by tests that fail on 中文说明置信度：5/5 —— 已观测到的 bug、有实测证据、修复最小且正确、测试钉住改动、CI 全绿；唯一无法勾选的项（合入后的端到端验证）对任何 workflow 改动都是结构性不可验的，并非对本 PR 的疑虑。 这是 #8648 停摆后该 workflow 需要的第二次跟进修复，也正是我想要的那种修复：小、解释充分、在真实 runner 上实测而非假设、并用在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
approved these changes
Aug 8, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
commented
Aug 8, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not linted (tool limitation, not a blocker): the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted.
中文说明
已审查。 建议见行内评论。 未检查（工具限制，非阻断）：the executable-script lint — .github/workflows/qwen-code-pr-review.yml: actionlint embedded-shell source mapping is not yet supported — not linted。
— deepseek-v4-flash via Qwen Code /review (v0.21.7)
Comment on lines
+2238
to
+2240
| const withShape = ifs.filter(([, cond]) => | ||
| cond.includes("format('@qwen-code /"), | ||
| ); |
Collaborator
Author
There was a problem hiding this comment.
[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)
Comment on lines
+2272
to
+2274
| expect(firstLine).toBeGreaterThan(-1); | ||
| expect(stripCr).toBeGreaterThan(-1); | ||
| expect(stripCr).toBeGreaterThan(firstLine); |
Collaborator
Author
There was a problem hiding this comment.
[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)
Comment on lines
+469
to
+470
| # First line only, then drop a trailing CR: comments written in the | ||
| # GitHub web UI arrive CRLF-terminated, so without this the command |
Collaborator
Author
There was a problem hiding this comment.
[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)
Comment on lines
+330
to
+331
| # KEEP IN SYNC with ack-review-request.if (explicit-trigger branches) — | ||
| # including the fromJSON newline/CR pair, explained there. |
Collaborator
Author
There was a problem hiding this comment.
[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)
wenshao
pushed a commit
that referenced
this pull request
Aug 8, 2026
Resolves the conflict with #8723 (both PRs appended a describe to the same test file) and addresses the four review findings. R1-1 (Critical) — the guard never inspected the line it protects. The regex `<!--[^>]*-->` matched across newlines, so the `<!--` inside this PR's own explanatory comment opened a match that ran to the ACK_BODY marker's `-->`; the comment-line skip then discarded the whole span. Verified: with the comment retained and ACK_BODY reverted to the glued form, the old guard passed. `[^>\n]` confines a match to one line. R1-2 — the `\n` exemption was unconditional, but bash only expands `\n` in printf's format string or ANSI-C `$'…'`. In a plain double-quoted assignment it is a literal backslash-n, so the prose stays on the marker's physical line and still breaks. The exemption is now gated on the quoting form. R1-3 — the `"`/`'`/`)` whitelist treated "the marker closes its literal" as proof nothing follows. It now reads the remainder of the literal and, when that is empty, an adjacent literal concatenated onto it. R1-4 — the guard scanned one file; it now walks every workflow. Zero offenders repo-wide, so it starts clean rather than with a backlog. Rewritten around one question — does this marker OPEN a string literal, and what remains inside it — which is what separates BUILDING a comment body from REFERENCING a marker. That distinction is why `jq contains("<!-- m -->")` and `printf '<!-- m -->' "$VAR"` stay silent: 51 false positives under the first cut, 0 under this one. Known gap, documented in the test rather than papered over: a body split across printf arguments is not caught. Modelling which literal is the format string is required, and every cheap approximation flagged the legitimate `printf '<!-- m -->' "$VAR"` form. Mutation-tested against the review's own scenarios, 3 of 3 caught: the glued form with the comment block retained (which the old guard passed), the double-quoted `\n` form, and adjacent-string concatenation.
Collaborator
|
Released in v0.21.8. |
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes
@qwen-code /reviewfollowed by a newline and a body actually trigger a review, and strips the trailing CR that CRLF comments leave on the parsed command line.Why it's needed
This shape has never worked:
The
ifs tried to accept it with:startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n'))GitHub expression string literals are not escape-processed — that
'\n'is a literal backslash +n. The branch matched nothing, so a multi-line command was accepted byauthorize, then silently dropped byreview-prandack-review-request: no run, no comment, no error. Silent failure in the one path that exists as the manual escape hatch when the automatic review does not fire.I hit this for real while re-triggering the 40 PRs stranded by the #8648 outage: the first 19 trigger comments were written command-then-body and every one was a no-op (
authorizesucceeded withshould_review=true,review-prskipped).Measured, not assumed
A probe workflow on a live runner (deleted branch, results reproduced here):
startsWith(<LF body>, format('…{0}', '\n'))startsWith(<LF body>, format('…{0}', fromJSON('"\n"')))startsWith(<CRLF body>, format('…{0}', fromJSON('"\n"')))startsWith(<CRLF body>, format('…{0}', fromJSON('"\r"')))fromJSONparses JSON, which is escape-processed, so it yields a real newline. Both line endings are required: the REST API sends LF, the web UI sends CRLF, and an LF pattern does not match a CRLF body. Applied to all 7 shape matches (6 ×/review, 1 ×/resolve).authorizeis deliberately left alone — its loosestartsWithprefix is a filter to avoid spawning a job per comment, and it says so.The shell half
TRIGGER_COMMANDis everything before the first LF, which on a CRLF comment keeps a trailing CR.IFScontains no CR, so word splitting yielded tokens like--timeout=300<CR>, failing the numeric check with nothing to explain it. Fixing only theifwould have made that reachable for the first time, so it is fixed here too.Reviewer Test Plan
How to verify
Expected: 103/103. Full scripts suite: 50 files, 1056 passed.
.github/scripts/qwen-triage-workflow.test.mjs: 56/56.Check out
main's workflow with this branch's tests and all three new tests fail:End-to-end confirmation is available after merge: comment
@qwen-code /reviewwith a body on the next line andreview-prshould reachin_progress— onmaintoday it silently skips.Evidence (Before & After)
N/A for UI. Before is the probe table above plus 19 real no-op trigger comments; after is 103/103 and every shape match carrying both endings.
Mutation-tested — each fails exactly its own test, nothing over-broad:
main's literal'\n'formTested on
Risk & Scope
startsWith, so@qwen-code /reviewers …is still not a command, and the exact-match and space-suffix branches are untouched. Every widened path still passesauthorize(write+ permission) and thecontextstep's own^@qwen-code[[:space:]]+/review([[:space:]]|$)grep.issue_commentresolves the workflow from there. Called out rather than claimed.Linked Issues
Surfaced while recovering from the #8648 / #8720 review outage.
中文说明
What this PR does
让
@qwen-code /review后面跟换行和正文的评论真正触发评审，并去掉 CRLF 评论在命令行尾部残留的回车符。Why it's needed
下面这种写法从来就没生效过：
if里试图用这个来接受它：startsWith(github.event.comment.body, format('@qwen-code /review{0}', '\n'))但 GitHub 表达式的字符串字面量不做转义处理——那个
'\n'是字面的反斜杠加n。该分支什么都匹配不到，于是多行命令会被authorize放行，再被review-pr和ack-review-request静默丢弃：没有 run，没有评论，也没有报错。而这条路径的全部意义，恰恰是自动评审没触发时的手动兜底。我是在重新触发被 #8648 停摆卡住的 40 个 PR 时真实踩到的：最先发的 19 条触发评论都写成了"命令 + 正文"，全部是空操作（
authorize成功且should_review=true，review-pr却 skip）。实测，而非假设
在真实 runner 上跑的探针 workflow（分支已删除，结果如下）：
startsWith(<LF body>, format('…{0}', '\n'))startsWith(<LF body>, format('…{0}', fromJSON('"\n"')))startsWith(<CRLF body>, format('…{0}', fromJSON('"\n"')))startsWith(<CRLF body>, format('…{0}', fromJSON('"\r"')))fromJSON解析的是 JSON，而 JSON 会做转义处理，因此能得到真正的换行符。两种行尾都必须列出：REST API 发送的是 LF，网页 UI 发送的是 CRLF，而 LF 模式匹配不上 CRLF 的正文。已应用到全部 7 处形态匹配（6 处/review，1 处/resolve）。authorize有意保持原样——它那个宽松的startsWith前缀只是一个过滤器，用来避免每条评论都起一个 job，注释里也是这么写的。shell 那一半
TRIGGER_COMMAND取的是第一个 LF 之前的全部内容，在 CRLF 评论下会保留尾部的 CR。IFS不含 CR，因此分词会产出--timeout=300<CR>这样的 token，数字校验失败却没有任何可解释的现象。只修if会让这个坑第一次变得可达，所以一并修掉。Reviewer Test Plan
How to verify
预期 103/103。scripts 全量套件：50 个文件、1056 passed。
.github/scripts/qwen-triage-workflow.test.mjs：56/56。用
main的 workflow 配本分支的测试，三条新测试全部失败：端到端确认要等合入之后：发一条
@qwen-code /review且下一行带正文的评论，review-pr应当进入in_progress——在今天的main上它会静默 skip。Evidence (Before & After)
界面部分 N/A。Before 是上面的探针表格，外加 19 条真实的空操作触发评论；After 是 103/103，且每处形态匹配都带上了两种行尾。
变异测试 —— 每个变异都只让它对应的那条测试失败，没有过宽：
main的字面量'\n'形式Tested on
Risk & Scope
startsWith，所以@qwen-code /reviewers …依然不算命令，精确匹配和空格后缀两个分支也未改动。所有被放宽的路径仍然要过authorize（write+ 权限）以及contextstep 自己的^@qwen-code[[:space:]]+/review([[:space:]]|$)grep。issue_comment是从默认分支解析 workflow 的。这里如实说明，而不是宣称已验证。Linked Issues
在从 #8648 / #8720 的评审停摆中恢复时发现。