fix(ci): tighten API error detection to avoid false positive on review prose - #7328
Conversation
…w prose The result-text classifier matched *"[API Error"* which hits review summaries that quote the pattern in prose (e.g. reviewing PR #7247 whose summary mentions "[API Error: ...]" and "quota … limit"). The quota grep then fired on the coincidental "quota … limit" substring, falsely reporting quota exhaustion on a successful review. Require a digit after "[API Error: " so only real API error messages (e.g. "[API Error: 429 …]") trigger the failure path.
|
Thanks for the PR! Template: headings differ from the repo template ("Motivation" / "Changes" / "How to verify" instead of "What this PR does" / "Why it's needed" / "Reviewer Test Plan"), but the content covers everything the template asks for — not blocking on naming. Problem: observed bug with concrete evidence — run 29727094401 failed despite the review completing and posting its comment on PR #7247. The root cause is clear: the Direction: aligned. This is a CI reliability fix — false positives in the review workflow waste runner time and create noise. The fix tightens detection to the actual error format rather than suppressing it. Size: not applicable — no core paths touched. 2 files, 114 additions / 3 deletions (mostly test scenarios). Approach: the scope feels right. Rather than just tightening the glob (which the PR title suggests), the actual fix is smarter — it strips known rate-limit suffixes, right-trims, and anchors the match to the end of the result text, matching how Moving on to code review. 🔍 中文说明感谢贡献！ 模板：标题与仓库模板不同（"Motivation" / "Changes" / "How to verify" 而非 "What this PR does" / "Why it's needed" / "Reviewer Test Plan"），但内容覆盖了模板要求的所有信息——不因命名问题阻塞。 问题：已观测到的 bug，有具体证据——run 29727094401 失败了，但 review 实际上已成功完成并在 PR #7247 上发布了评论。根因清晰： 方向：对齐。这是 CI 可靠性修复——review 工作流的误报浪费 runner 时间并制造噪音。修复将检测收紧到实际的错误格式，而非抑制检测。 规模：不适用——未触及核心路径。2 个文件，114 行新增 / 3 行删除（主要是测试场景）。 方案：范围合理。实际修复比标题暗示的更巧妙——剥离已知的 rate-limit 后缀、右修剪、将匹配锚定到 result text 的末尾，与 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: given the false positive, I'd anchor the What the PR does: exactly that. The implementation strips the three known rate-limit guidance suffixes (matching No correctness issues found. The comment explaining the trailing-anchor rationale is warranted — the invariant (adapter appends errors last) is non-obvious and load-bearing. One minor observation: the old comment "Same detection as before; only the disposition is new." was removed from the TestingRan the full test suite against the PR's changes: All 20 tests pass — 11 existing + 9 new scenarios covering:
中文说明代码审查独立方案： 针对误报，我会将 PR 的做法： 完全一致。实现剥离三个已知的 rate-limit 引导后缀（与 未发现正确性问题。解释尾部锚定原理的注释是必要的——不变量（adapter 最后追加错误）不显而易见且是关键依赖。 测试对 PR 的改动运行了完整测试套件：20 个测试全部通过（11 个已有 + 9 个新场景），覆盖了无状态码中断、长错误体、部分 review 后追加错误、rate-limit 后缀、原始误报场景、已知限制等。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 Clean, well-scoped fix for a real CI false positive with concrete evidence. The trailing-anchor approach correctly matches how the stream-json adapter appends errors, the suffix stripping handles the 429 rate-limit guidance that follows the closing bracket, and the sync test guards against drift with LGTM, approving. ✅ 中文说明置信度：5/5 干净、范围合理的修复，针对有具体证据的真实 CI 误报。尾部锚定方案正确匹配了 stream-json adapter 追加错误的方式，后缀剥离处理了 429 rate-limit 引导文本跟随闭合括号的情况，同步测试防止与 LGTM，批准。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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. |
ReviewWhat it does: tightens the aborted-review classifier's The bash semantics are right (the quoted span is literal, But I think the fix is net-negative as written: it trades a loud false positive for a silent false negative, and it doesn't actually close the false positive it targets. 1. Blocking: real aborts stop being detected at allThe premise — real API errors "always carry a 3-digit status code" after the colon — doesn't hold. Running the workflow's own classifier over the formats documented in
Fair caveat: under The suite doesn't catch this because every fixture uses the digit-leading shape. 2. It also doesn't close the false positive it targetsProse that quotes a status code still matches. This PR's own bot review is the counterexample — stage 1 and stage 2 both contain Fed as Worse, the diff adds 3. Suggested directionAnchor on position rather than shape — an abort renders the error last, prose quotes it mid-body. I tested this and it classifies every case above correctly, and rejects both real bot review bodies: # An aborted run renders the API error last; a review that *discusses* API
# errors quotes them mid-prose. Anchor on position, not on status-code shape.
RESULT_TAIL="$(printf '%s' "$RESULT_TEXT" | tail -c 600)"
case "$RESULT_TAIL" in
*"[API Error: "*)Note the window must be generous: 429s append a 110–140 char rate-limit suffix, so a real quota error is ~204 bytes and The durable fix, though, is a positive success signal instead of prose scraping. In 4. TestsGood structure — extracting and running the real bash beats a paraphrase, and the mutation check confirms teeth. Two gaps:
5. NitThe comment at line 732 — "Same detection as before; only the disposition is new" — is now stale; detection is exactly what changed. Recommendation: hold. §1 is a silent-failure regression on the path this block exists to guard. The tail anchor in §3 is a small change that fixes both directions and keeps the test you've already written. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
— qwen3.8-max-preview via Qwen Code /review
| fi | ||
| case "$RESULT_TEXT" in | ||
| *"[API Error"*) | ||
| *"[API Error: "[0-9]*) |
There was a problem hiding this comment.
[Critical] The narrowed glob drops the retry safety net for API error formats whose message does not begin with a digit — Failure scenario: model connection drops mid-review → CLI exits 0 in stream-json mode with subtype=success, is_error=false, RESULT_TEXT="[API Error: Connection error.]" → the AlreadyReportedError throw is guarded by outputFormat === OutputFormat.TEXT (nonInteractiveCli.ts:1651) and is never taken in stream-json → the case is the sole safety net → new glob requires [0-9] after [API Error: but Connection error. starts with C → falls through to OUTCOME='success' → review silently treated as successful, no retry, no comment posted. Same applies to [API Error: Rate limit exceeded], [API Error: An unknown error occurred.], and every other non-digit-prefixed format from parseAndFormatApiError.
Consider anchoring at the start of the string instead of requiring a digit, since the error format always begins with [API Error: :
| *"[API Error: "[0-9]*) | |
| "[API Error: "*) |
This matches real API errors (which always start with [API Error: ) while excluding prose that merely mentions the pattern mid-sentence.
— qwen3.8-max-preview via Qwen Code /review
… shape The status-code glob from the previous commit would silently miss real aborts whose message has no leading digit (Connection error, Status: suffix, Chinese rate-limit text) — trading a loud false positive for a silent false negative. Anchor on position instead: an aborted run renders the API error as (or at the very end of) the result text, while a successful review that discusses API errors quotes them mid-prose. Checking only the last 600 bytes separates the two without constraining the error message format. Also adds fixtures for the non-digit-leading shapes and for prose quoting a real status code mid-body, and removes a stale comment.
9927729
|
Addressed in 9927729 — switched to the tail-anchor approach from §3. §1 (silent false negative): Fixed. The §2 (prose quoting a status code): Fixed by position. A long review (>600 bytes) quoting §3 (tail anchor): Adopted as suggested. 600-byte window covers the longest real error (~200 bytes for 429 + rate-limit suffix) with headroom. Both the §4 (test gaps): Added fixtures for both gaps — non-digit-leading shapes and prose quoting a real status code mid-body. 14/14 pass. §5 (stale comment): Removed. The durable fix (positive success signal / upstream exit-code fix) is noted — worth a follow-up issue. |
Re-review (9927729)Thanks for turning this around. The blocking false-negative from the previous revision is confirmed fixed — I re-ran every real abort format from
Tests are 14/14 and the four new ones have real teeth — I mutated three ways and each mutation failed exactly the tests that should catch it: reverting to the original glob fails both prose tests, reverting to the digit glob fails both abort tests plus one prose test, and widening the window to 100000 fails both prose tests. I proposed the tail anchor, so let me be the one to report that it has its own counterexample. 1. The bilingual footer defeats the tail anchorqwen-code reviews end with a Running the updated production logic over the three review bodies posted on this PR:
The stage-3 trigger is in its trailing 600 bytes:
So the failure mode moves from Caveat, stated plainly: the classifier reads the agent's 2. Errors longer than the window still go silently greenIf the error message itself exceeds ~590 bytes, the
Cheap mitigation: also treat "the error IS the whole result" as an abort, which is length-independent and the shape of every existing fixture: IS_ABORT=0
case "$RESULT_TEXT" in "[API Error: "*) IS_ABORT=1 ;; esac # error is the entire result
case "$RESULT_TAIL" in *"[API Error: "*) IS_ABORT=1 ;; esac # error terminates a partial reviewI verified that arm keeps all six formats above correct and fixes the 758-byte case, without changing any prose verdict. 3. Moving the quota grep onto the tail changes disposition
4. The window's lower bound is untestedThe suite passes with No fixture includes the rate-limit suffix, so the constant the comment justifies ("~200 bytes for a 429 + rate-limit suffix") isn't actually pinned. Appending the real suffix to the 5. The bigger pointThis is the third heuristic for separating "model prose about API errors" from "an actual API error," and each one has fixed the previous counterexample while opening a new one — because both signals are the same model's free text. That will keep happening. The workflow already has ground truth available: in Recommendation: §2 is worth fixing before merge (three added lines). §1 I'd like confirmed against a real |
… quota Add a whole-result check (case "$RESULT_TEXT" in "[API Error: "*) alongside the tail check so errors whose body exceeds 600 bytes are still detected — the prefix falls outside the tail window but the result starts with it. Move the quota grep back onto the full RESULT_TEXT so a long error with quota wording early in the message is still classified as quota (not downgraded to retryable).
|
Addressed §2 and §3 in ee7b9fa. §2 (long error body): Added the whole-result check as suggested — §3 (quota grep scope): Moved the quota grep and §1 (bilingual footer): Acknowledged. The §4 (window lower bound) / §5 (ground truth): Agreed, follow-up. The positive success signal (checking if the bot comment landed at 15/15 tests pass. |
Re-review (ee7b9fa)Confirmed fixed from the last round: the whole-result arm catches long errors (mutating it away fails But digging into why the tail arm exists turned up something that changes the picture. 1. The production abort shape isn't the one being tested
const errorText = parseAndFormatApiError(event.value.error, …);
this.appendText(state, errorText, null);and the throw path in not The tail arm, its 600-byte constant, and the grep-scope fix are all unguarded. A future cleanup could delete the entire mechanism and CI would stay green. 2. Which means §2 is only half fixedIn the production shape, a long error still goes silently green — the prefix arm doesn't fire (the result starts with review prose) and the tail arm misses (the error exceeds the window):
Green job, no comment posted, no fallback — the case the whole block exists to catch. 3. My §3 suggestion made §1 worse — apologiesMoving the quota grep back to full text means the bilingual-footer false positive now lands in the quota bucket instead of retryable:
So a successful review now fails immediately with no retry and posts "model quota exhausted" — the exact #7247 symptom. I asked for §2 and §3 independently without checking the interaction; that's on me. 4. Suggestion: anchor on "ends with", not "within N bytes"Since # The adapter appends the API error last (BaseJsonOutputAdapter appendText),
# so an aborted run's result ENDS with the error — optionally followed by the
# rate-limit guidance suffix. A review that merely quotes the pattern keeps
# writing afterwards.
BODY="$RESULT_TEXT"
for S in "Possible quota limitations in place or slow response times detected. Please wait and try again later." \
"Please wait and try again later. To increase your limits, request a quota increase through AI Studio, or switch to another /auth method" \
"Please wait and try again later. To increase your limits, request a quota increase through Vertex, or switch to another /auth method"; do
BODY="${BODY%"$S"}"
done
BODY="${BODY%"${BODY##*[![:space:]]}"}" # rtrim
IS_ABORT=0
case "$BODY" in *"[API Error: "*"]") IS_ABORT=1 ;; esacI ran this against your suite and the real data:
Residual risk, stated honestly: a review that quotes the pattern and ends with Whichever way you go, please add a 5. Still worth doingThree revisions in, each heuristic has been defeated by a different property of the model's own prose. The Recommendation: §2's production shape is the one blocker — it's a silent green on a real abort. §4 fixes it, §1, and the untested-constant problem in one change, and I've verified it against your suite. Happy to be wrong about the appended-error shape if you have a real |
Replace the prefix+tail-window split with an ends-with check: the stream-json adapter appends the formatted API error last, so an aborted run's result ENDS with "[API Error: …]" optionally followed by a rate-limit guidance suffix. Strip the three known suffixes, rtrim, then match *"[API Error: "*"]". This catches the production abort shape (partial review + appended error) at any error length, without a byte-window constant to tune or leave untested. Prose that quotes the pattern mid-body keeps writing afterwards and does not end with "]". Adds fixtures for the production shape (appended error, long appended error, rate-limit suffix after the bracket).
|
Adopted §4 in 989fc23 — replaced the prefix+tail-window split with the ends-with approach. §1 (bilingual footer): Fixed by construction. The check is now §2 (production shape): Fixed. The production abort shape §3 (quota grep interaction): The quota grep still runs on full §4 (ends-with anchor): Implemented as suggested — strip the three known rate-limit suffixes, rtrim, match Mutation coverage:
18/18 tests pass. §5 (ground truth via comment check) noted for follow-up. |
Re-review (989fc23)This is the right shape. Everything from the last round checks out:
18/18 pass, and the mutation results line up with the fixtures rather than passing by luck. Two things left, both small. 1. Suffix drift is a silent-green vectorThe workflow hardcodes the three strings from The suite won't catch it, because Six lines in the existing test file closes it (it already reads the workflow YAML; this just adds the source): it('keeps the workflow rate-limit suffix list in sync with errorParsing.ts', () => {
const src = readFileSync('packages/core/src/utils/errorParsing.ts', 'utf8');
const blk = src.slice(src.indexOf('RATE_LIMIT_MESSAGE_BY_AUTH = {'), src.indexOf('} as const;'));
const suffixes = [...blk.matchAll(/'\\n([^']+)'/g)].map((m) => m[1]);
expect(suffixes).toHaveLength(3);
for (const s of suffixes) expect(workflow).toContain(s);
});I ran it against the current tree — 3/3 found, passes. 2. The
|
| successful review | rev 4 |
|---|---|
quotes [API Error: 429 quota exhausted], ends with a - [x] checklist |
quota |
quotes [API Error: 503 …], ends with a ref link [1] |
retryable |
ends with ], no API error anywhere |
success ✅ |
Mutating the pattern from *"[API Error: "*"]" to just *"]" leaves all 18 tests green — so the [API Error: half of the pattern isn't pinned by any fixture.
In practice the current review template ends with </details> + a <sub>Reviewed at …</sub> footer, which is why none of the 8 real bodies trip it — the footer is accidentally protecting us. That's worth a fixture so a future template change doesn't quietly remove the protection:
success_ends_with_bracket) r success false "Review of [API Error: 429 quota exhausted] handling. Checklist: - [x]" ;;
Note this one fails today — it's the known trade-off, not a regression. Either accept it and document it in the comment, or add a cheap disambiguator (e.g. require the closing ] to belong to the last [API Error: by checking that the segment after the final marker contains no newline).
Verdict
The important property is that rev 4 has no known silent-green case except §1's drift scenario — every remaining misclassification is a noisy false failure, which is the right direction to fail in. Compared to where this started (rev 1 turned six real abort formats into green jobs), that's a solid landing point.
LGTM to merge with §1 added — it's six lines and it's the last silent path. §2 I'd take as a follow-up or an accepted, documented limitation.
And the §5 point from earlier still stands as the eventual fix: once the workflow checks that the bot comment actually landed at $EXPECTED_HEAD_SHA, this entire classifier becomes advisory and neither §1 nor §2 can fail a good review.
…de-off Add a sync test that reads RATE_LIMIT_MESSAGE_BY_AUTH from errorParsing.ts and asserts all three suffixes appear in the workflow — prevents silent drift if someone rewords one. Add a KNOWN-limitation fixture documenting that prose ending with ] after quoting the pattern is a false positive (accepted trade-off; the durable fix is checking that the bot comment landed).
|
Done in 8edab22. §1 (suffix drift): Added the sync test — reads §2 ( 20/20 pass. |
Re-review (8edab22) — LGTM ✅Both remaining items are closed. No production code changed in this commit, so the behaviour matrix from the last round still holds; this is purely about pinning it. §1 sync guard — verified teeth in all three drift directions:
The length assertion is the part I'd have been most likely to leave out, and it's the one that catches a new auth type being added — nice. It also fails safe if prettier ever splits one of those strings across lines: the regex stops matching, the count drops, the test goes red rather than silently under-checking. Relative path resolution is correct — §2 documented rather than fixed — the right call. Final state, re-verified against the real data:
20/20 pass. Every remaining misclassification is a noisy failure; the only silent-green paths that existed across the earlier revisions are now either fixed or guarded by a test that fails on drift. That's the property that matters for this classifier. Worth recording what this took: rev 1 turned six real abort formats into green jobs, and each subsequent heuristic was defeated by a different property of the model's own prose — status-code shape, then the bilingual footer, then Which is why I'd still like the §5 follow-up eventually: in Approving. |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.20.1. |
|
PR #7328 is merged and released in v0.20.1. The last activity is yiliang114's "Running" comment (Jul 28) and the release bot confirming the tag. No action needed — acknowledged. ✅ completed |
4 participants
Motivation
The
review-prjob in run 29727094401 failed even though the review completed successfully and posted its comment to PR #7247.The result-text classifier uses a
casepattern*"[API Error"*to detect aborted reviews. When reviewing a PR about API error handling, the review summary naturally quotes the pattern (e.g. "detecting the[API Error: ...]pattern") and mentions regex keywords like`quota` and `rate.?limit`. The broadcasematched the prose, then the quota grep (quota.*(exhaust|exceed|limit|reset)) hit the coincidental "quota … limit" substring, falsely classifying a successful review as a quota exhaustion failure.Changes
Tighten the
caseglob from*"[API Error"*to*"[API Error: "[0-9]*so only real API error messages (which always carry a 3-digit status code, e.g.[API Error: 429 …]) trigger the failure path. Review prose that quotes the pattern without a status code no longer matches.How to verify
success_mentions_api_errorfeeds a result text containing[API Error: ...]and "quota … limit" in prose — it must classify assuccess, notquota.[API Error: 503 …],[API Error: 429 … quota exhausted], etc.) still classify correctly.npx vitest run scripts/tests/qwen-pr-review-workflow.test.js— 11/11 pass.中文说明
动机
run 29727094401 中
review-prjob 失败了，但 review 实际上已成功完成并在 PR #7247 上发布了评论。result-text 分类器使用
case模式*"[API Error"*来检测中断的 review。当 review 一个关于 API 错误处理的 PR 时，review 总结自然会引用该模式（如 "detecting the[API Error: ...]pattern"），并提及`quota` and `rate.?limit`等正则关键词。宽泛的case匹配到了正文，随后 quota grep（quota.*(exhaust|exceed|limit|reset)）命中了偶然出现的 "quota … limit" 子串，将一次成功的 review 误判为配额耗尽。改动
将
caseglob 从*"[API Error"*收紧为*"[API Error: "[0-9]*，要求冒号后跟数字状态码（真正的 API 错误格式如[API Error: 429 …]），review 散文中不带状态码的引用不再触发失败路径。验证方式
success_mentions_api_error：result text 包含[API Error: ...]和 "quota … limit" 散文——必须分类为success而非quota。[API Error: 503 …]、[API Error: 429 … quota exhausted]等）分类仍然正确。npx vitest run scripts/tests/qwen-pr-review-workflow.test.js— 11/11 通过。