fix(autofix): a no-output crash must not advance the review watermark - #7229
Merged
Conversation
When a review-address run crashes AFTER prepare (so NEWEST is set) but BEFORE the agent writes any verdict — no address-summary.md, no-action.md, or failure.md — the handoff stamped the marker with ts=NEWEST, advancing the feedback watermark as if the feedback had been evaluated. It hadn't. The next scan then saw 'nothing new since <NEWEST>' and never retried, stranding the PR on a purely transient crash. That is exactly what happened to #7219 during the #7165 SKILL-staging outage: the run crashed at promptFor (ENOENT) at 09:50, the handoff advanced the watermark to 09:50:56, and even after #7225 fixed the crash the loop considered all prior feedback 'addressed' and would not re-engage. Fix: on a no-output crash (NEWEST set, DETAIL_FILE empty) stamp the sentinel ts instead — it is excluded from EVAL_WM, so the watermark does not move and the next scan retries the same feedback. The round still increments, so a PERSISTENT crash is bounded by MAX_ROUNDS and ends in a terminal handoff rather than looping forever. Agent-produced handoffs (verify failed after real output) keep advancing the watermark as before. Replay test extended to assert BOTH MARK_TS and MARK_ROUND across all three shapes: output+verify-fail → advance; no-output crash → sentinel (retry); pre-prepare crash → terminal. 60/60 + 12/12.
Collaborator
|
Thanks for the PR! Template looks good ✓ (minor: "## The fix" stands in for "## Why it's needed" — content is all there, no issue). Problem: observed bug with clear evidence. #7219 was stranded during the #7165 → #7225 SKILL-staging outage: the run crashed at Direction: this is the project's own CI/autofix infrastructure — squarely in scope. The watermark-advance-on-no-output-crash is a genuine correctness gap. Size: not applicable (no core module paths; Approach: the scope is tight — one new Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓（小偏差："## The fix" 代替了 "## Why it's needed"——内容齐全，不影响）。 问题：已观测到的 bug，有明确证据。#7219 在 #7165 → #7225 SKILL-staging 故障期间被卡死：运行崩在 方向：这是项目自身的 CI/autofix 基础设施——完全在范围内。无产出崩溃时推进水位线是真实的正确性缺陷。 规模：不适用（未触及核心模块路径； 方案：范围紧凑——一个新的 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code ReviewIndependent proposal: for a no-output crash (NEWEST set, DETAIL_FILE empty), stamp the sentinel ts that EVAL_WM already excludes, so the watermark stays put and the next scan retries. Keep the round increment to bound persistent crashes. What the PR does: exactly that. One new No correctness issues, no security concerns, no convention violations. The sentinel exclusion is verified at two sites in the workflow (lines 1542 and 1987: TestingThis is a CI workflow change — no user-facing CLI behavior to drive in tmux. Verification is the behavioral test suite (which extracts and executes the real bash block) plus static checks. Unit tests (60/60 + 12/12)Behavioral replay (3 crash/handoff shapes)Shape 2 is the fix: sentinel ts means EVAL_WM ignores it, watermark stays, next scan retries. Round still increments (3), so persistent crashes are bounded by MAX_ROUNDS (5). Static checks中文说明代码审查独立方案： 对无产出崩溃（NEWEST 有值、DETAIL_FILE 空），用 EVAL_WM 已排除的哨兵 ts 打 marker，水位线不动，下次扫描重试。round 仍 +1，约束持续崩溃。 PR 实际做法： 完全一致。在已有的 无正确性问题、无安全隐患、无规范违反。哨兵排除在 workflow 两处验证（1542 行和 1987 行： 测试这是 CI workflow 变更——没有用户可见的 CLI 行为可在 tmux 中驱动。验证方式是行为测试套件（提取并执行真实 bash 块）加静态检查。 单元测试 60/60 + 12/12 全部通过。行为回放验证三种形态：有产出+verify 失败（水位推进）、无产出崩溃（哨兵，水位不动）、prepare 前崩溃（终态）。静态检查：YAML 可解析，36 个 run 块全部通过 bash -n。 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean fix for a real incident, minimal diff, behavioral tests cover all three shapes, would merge without hesitation. This is the kind of PR that's easy to review because it does exactly one thing and does it right. The #7219 stranding was a real incident with a clear causal chain — crash at promptFor, handoff stamped NEWEST, watermark advanced, loop went quiet. The fix reuses the existing sentinel mechanism (already excluded from EVAL_WM at two sites), so there's no new machinery, just a new branch that routes to it. The round counter still increments, so a persistent crash is bounded by MAX_ROUNDS and ends in a terminal handoff — no infinite retry loop. The test is the right kind of test: it extracts the actual bash block from the YAML and replays it under real bash with different env combinations, asserting both MARK_TS and MARK_ROUND. That catches regressions in either dimension, not just string-matching the workflow text. My independent proposal was identical to what the PR does — I couldn't find a simpler path. Every line in the diff serves the stated goal. The comment block in the workflow explains the why well enough that a future reader won't need to dig through the PR thread. 中文说明置信度：5/5 —— 针对真实事故的干净修复，diff 极小，行为测试覆盖三种形态，毫不犹豫地合并。 这个 PR 只做一件事而且做对了。#7219 被卡死是真实事故，因果链清晰——崩在 promptFor，handoff 用 NEWEST 打 marker，水位推进，循环静默。修复复用了已有的哨兵机制（EVAL_WM 两处已排除），没有新机制，只是一个新分支路由到哨兵。round 计数器仍 +1，持续崩溃受 MAX_ROUNDS 约束，最终走终态 handoff——不会无限重试。 测试是正确类型的测试：从 YAML 提取真实 bash 块，在不同环境变量组合下用真实 bash 回放，断言 MARK_TS 和 MARK_ROUND 两个值。这能捕获任一维度的回归，而非仅字符串匹配 workflow 文本。 我的独立方案与 PR 完全一致——找不到更简路径。diff 中每一行都服务于既定目标。workflow 中的注释块充分解释了原因，未来读者无需翻阅 PR 讨论。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 19, 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. ✅
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
wenshao
commented
Jul 19, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
Two review findings on the no-output-crash handoff: - The headline promised 'it will retry on the next scan' even on the final attempt, but at MARK_ROUND == MAX_ROUNDS the scan's round-cap gate skips the PR and the cap-reached notice is takeover-only — so a maintainer was told a retry was coming that never comes. The headline now branches: 'it will retry' only while MARK_ROUND < MAX_ROUNDS, otherwise 'this was the last automatic attempt; a human should take over'. - It embedded a Run log URL that the report block already appends to every handoff, duplicating it in the comment. Removed from the headline. Replay test extended: mid-attempt headline promises retry and carries no Run log; final-attempt headline says human-takeover and never 'retry'. 60/60 + 12/12.
qwen-code-ci-bot
approved these changes
Jul 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.8-max-preview via Qwen Code /review
yiliang114
approved these changes
Jul 19, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
The main flow looks right to me. I traced all three handoff shapes against the workflow source: on a no-output crash it stamps the sentinel ts (excluded from EVAL_WM) instead of NEWEST, so the watermark stays put and the next scan retries the same feedback, while MARK_ROUND still increments and MAX_ROUNDS bounds a persistent crash. The headline branch is consistent with the actual gate too — the address job's MAX_ROUNDS is matrix.target.max_rounds, which is the same EFF_MAX_ROUNDS the scan computes, so "it will retry" / "last automatic attempt" won't promise a retry the round-cap gate would then skip.
Non-blocking: a persistent (not transient) startup crash now posts up to MAX_ROUNDS handoff comments, one per scan, instead of one. The description already owns this as the intended trade for accurate attempt counts, so just flagging it — if the comment noise ever gets annoying, mid-attempt crashes could edit the marker in place and only post a fresh comment on the terminal one.
Nothing blocks merge. LGTM.
This was referenced Jul 19, 2026
qqqys
pushed a commit
to qqqys/qwen-code
that referenced
this pull request
Jul 21, 2026
…enLM#7247) * fix(autofix): retry a model API error instead of stranding the PR When the agent's qwen subprocess dies on a model-side [API Error] (403 access denied, a 429 quota, a 5xx), run-agent.mjs wrote a handoff/failure.md, so the handoff step treated it as an EVALUATED handoff — it advanced the watermark and the next scan saw 'nothing new', stranding the PR until a manual re-arm. But the agent never actually evaluated the feedback; the model was unreachable. QwenLM#7220 hit exactly this: fork-takeover engaged and ran the agent, the model returned '[API Error: 403 Model access denied]' (the autofix key lacks access to qwen3.8-max-preview), and the PR was left with an advanced watermark that will not retry. Fix, mirroring QwenLM#7229's no-output-crash handling: - run-agent.mjs extracts a [API Error: 4xx/5xx] from the captured output tail, includes it in failure.md, and drops an marker file. - The handoff step reads that marker and routes the failure to the sentinel-ts (retry) path — the watermark does NOT advance, so the next scan retries; the round still increments so a PERSISTENT model failure is bounded by MAX_ROUNDS. The headline names the model error and, on the final attempt, tells the maintainer to check the autofix model key/access and re-arm — instead of a generic crash message. Tests: run-agent.mjs flags a model [API Error] (marker + failure.md) and does NOT flag a generic failure; the handoff replay treats an API-error handoff as sentinel|retry (not a watermark advance) with a model-aware, cause-specific headline. 62/62 + 12/12. * fix(autofix): scope + broaden the retryable model-API detection (review) Addresses wenshao's review on QwenLM#7247: - Behavioral (1): the agent-api-error marker was written on ANY non-zero exit whose output tail contained an API-error string — so a loop guard, a timeout, or an agent-written failure.md (a real verdict) would wrongly retry and, worst case, silently discard a verdict. The write is now scoped to the bare-failure branch and guarded by !timedOut, so only an un-evaluated model failure retries. - Coverage (2): the old regex only matched a LEADING status digit, so it missed the canonical rate-limit render, the (Status: …) form, the bad-key 401, the Chinese quota text, and the unwrapped Qwen OAuth quota — i.e. most real errors this targets. Detection is now a whitelist of RECOVERABLE errors (401/402/403/429/5xx + rate-limit / quota / api-key / RESOURCE_EXHAUSTED / overloaded phrasings, plus the standalone OAuth-quota form); a 400/404 stays terminal. - Test gap (3): a writer↔reader contract test now runs the REAL run-agent.mjs to write the marker, then the extracted workflow reader block against that same workdir — a rename on either side (proven with the YAML-only mutation) now fails the suite. - Smaller: API_ERROR_DETAIL is comment-escaped (sed) and capped (cut -c1-200) since it derives from agent stdout; the marker match is single-line ([^]\n]) so a multi-line render can't smuggle a newline; agent-api-error is added to the run-artifacts list. Non-recoverable 4xx (400/404) deliberately stay terminal; the live 401/403 config cases retry and self-heal once the key/access is fixed. 79/79 across both suites. * test(autofix): cover the timeout guard and the OAuth-quota fallback (review) Two coverage gaps from the ci-bot review on QwenLM#7247: - The !result.timedOut guard was only asserted indirectly — no test emitted an [API Error] AND timed out. Added a case (spawnSync + QWEN_TIMEOUT_MS=100): qwen streams [API Error: 503] then hangs past the budget → killed → no marker. A refactor to !loopDetected now fails here. - The standalone Qwen-OAuth-quota fallback (unwrapped, no [API Error:]) had no test. Added a case emitting bare 'Qwen OAuth quota exceeded (limit: 100/min)' → marker written, wrapped as '[API Error: Qwen OAuth quota exceeded …]'. * fix(autofix): anchor the API-error code, split retry budget by cause, keep the headline UTF-8 Addresses the review on QwenLM#7247. Classifier (points 2 and 4): the status code is now read from its POSITION in the render (`[API Error: <code>`) instead of matched anywhere in the message. Matching anywhere retried permanent failures forever — `400 Invalid value for max_tokens: must be <= 512` matched a bare \b5\d\d\b and `400 context length exceeded` matched a bare `exceeded`. `exceeded` now only counts as part of `quota`. A 404 whose message says the model "does not exist or you do not have access to it" — the OpenAI-compatible render of what a 403 reports — is no longer terminal. Retry budget (point 3): the marker now carries the cause class. A transient 429/5xx self-heals and keeps the full round budget; an auth/access error that only a maintainer can fix is capped at API_AUTH_MAX_ROUNDS (3) and then goes terminal with the "check the autofix model key/access, then re-arm" headline — instead of ~100 agent runs and ~100 PR comments over ~17h on a takeover PR. The terminal round is stamped so the scan's round gate skips the PR while the sentinel ts keeps the feedback live for a re-arm. Headline (point 1): `cut -c` counts bytes under GNU coreutils and the classifier deliberately matches CJK renders, so the 200-byte cap could split a multi-byte character and emit invalid UTF-8. Guarded with `iconv -f utf-8 -t utf-8 -c || true`, matching the sibling publish site (the `|| true` is required — iconv -c exits 1 when it discards). Minor (point 5): documented that detection is best-effort because apiError is derived from the last 20 KB of output; `head -1` -> `head -n 1`; tests added for a permanent 400 carrying a 3-digit number >= 500 and for a >200-byte CJK render staying valid UTF-8. * test(autofix): cover the auth-capped retry budget and Chinese API-error patterns (QwenLM#7247) * fix(autofix): short-circuit 400 as terminal and classify only the last API error (QwenLM#7247) * fix(autofix): treat transport-level API failures as retryable QwenLM#7365 stranded at round 2/100 on this render: [API Error: terminated (cause: read ECONNRESET)] The connection to the model dropped mid-run. That is as transient as a 429, but the classifier never saw it that way: a transport failure never got far enough to have an HTTP status, so it fell through to the keyword arm, and the keyword arm only knew about rate limits and quotas. It was classified terminal, the watermark advanced, and a PR that needed nothing but a re-run was handed to a human. Verified against the shipped classifier before the fix — every transport render came back terminal: terminated (cause: read ECONNRESET) -> terminal fetch failed -> terminal socket hang up -> terminal connect ETIMEDOUT -> terminal Adds a transport arm to the code-less branch: ECONNRESET, ECONNREFUSED, ETIMEDOUT, EPIPE, EAI_AGAIN, socket hang up, fetch failed, terminated. ENOTFOUND is deliberately excluded. A hostname that does not resolve is a misconfigured endpoint, which repeats forever — the same reasoning that keeps a bad model name terminal. Coded errors are unaffected: the arm sits after the status-code branch, so the 400 short-circuit added in 719991a still runs first. * fix(autofix): address review — OAuth fallback override, comment accuracy, display clamp (QwenLM#7247) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: 易良 <1204183885@qq.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
Collaborator
|
Released in v0.20.1. |
Collaborator
|
Agent run timed out after 1800000ms ❌ failed |
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
Fixes a real correctness bug in the review-address loop: a crash that evaluated nothing must not advance the feedback watermark.
When a run crashes after prepare (so
NEWESTis set) but before the agent writes any verdict — noaddress-summary.md,no-action.md, orfailure.md— the handoff stamped its eval marker withts=NEWEST. That advances the watermark as if the feedback had been evaluated. It hadn't. The next scan then seesnothing new since <NEWEST>and never retries, stranding the PR on a purely transient crash.This is exactly what stranded #7219 during the #7165 SKILL-staging outage: the run crashed at
promptFor(ENOENT) at 09:50, the handoff advanced the watermark to09:50:56, and even after #7225 fixed the crash the loop treated all prior feedback as "addressed" and would not re-engage — a healthy loop still saw nothing to do.The fix
On a no-output crash (
NEWESTset,DETAIL_FILEempty), stamp the sentinel ts (9999-12-31T23:59:59Z) instead ofNEWEST. The sentinel is already excluded fromEVAL_WM, so the watermark does not move and the next scan retries the same feedback. The round still increments, so a persistent crash is bounded byMAX_ROUNDSand ends in a terminal handoff rather than looping forever.Nothing else changes:
ts=NEWEST), round increments. Unchanged.NEWESTempty) → still terminal (round=MAX_ROUNDS), so the scan skips instead of re-handing-off forever. Unchanged.Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 60/60. The handoff replay test now extracts the realMARK_TS/MARK_ROUNDblock and runs it under bash across all three shapes, asserting both values:NEWESTDETAIL_FILEMARK_TSMARK_ROUNDNEWEST(advance)ROUND+1ROUND+1WATERMARK/sentinelMAX_ROUNDS(terminal)npx vitest run scripts/tests/qwen-fleet-shepherd-workflow.test.js— 12/12 (untouched).Static: YAML parses; every
run:block passesbash -n.Evidence (Before & After)
Tested on
Risk & Scope
MAX_ROUNDStimes (≈ one per scan) before the terminal handoff, instead of handing off on the first crash. That's the intended trade — transient crashes self-heal, persistent ones are still bounded. The round counter treats each crash-attempt as a round, so a human sees an accurate attempt count.Linked Issues
Root-cause fix for the stranding observed on #7219 (after the #7165 → #7225 SKILL-staging incident).
中文说明
本 PR 做了什么
修复 review-address 循环的一个真实正确性 bug:一次什么都没评估的崩溃,不应推进反馈水位线。
当运行在 prepare 之后(
NEWEST已有值)、但 agent 写出任何结论之前崩溃 —— 没有address-summary.md/no-action.md/failure.md—— handoff 却用ts=NEWEST打了 marker,把水位线当作"反馈已评估"一样推进。实际并没有。于是下次扫描看到"自<NEWEST>起无新反馈"而永不重试,PR 被一次纯瞬时崩溃卡死。这正是 #7219 在 #7165 SKILL-staging 故障期间被卡死的原因:09:50 崩在
promptFor(ENOENT),handoff 把水位线推到09:50:56;即便 #7225 修好崩溃后,循环仍把之前所有反馈当作"已处理",不再介入 —— 健康的循环也无事可做。修复
崩溃前无产出(
NEWEST有值、DETAIL_FILE空)时,改用哨兵 ts(9999-12-31T23:59:59Z)。哨兵本就被EVAL_WM排除,水位线因此不动,下次扫描重试同一批反馈。round 仍 +1,所以持续崩溃受MAX_ROUNDS约束,最终走终态 handoff,不会无限循环。其余不变:agent 有产出但 verify 失败 → 仍推进水位(真实评估过的 handoff);prepare 前崩溃(
NEWEST空)→ 仍终态,扫描跳过而非反复 handoff。评审验证
npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 60/60。handoff replay 测试现在提取真实的MARK_TS/MARK_ROUND块并在 bash 下跑三种形态,断言两个值:有产出+verify 失败 →ts=NEWEST(推进)、round+1;无产出崩溃 → 哨兵(重试)、round+1;prepare 前崩溃 →WATERMARK/哨兵、MAX_ROUNDS(终态)。npx vitest run scripts/tests/qwen-fleet-shepherd-workflow.test.js—— 12/12(未改动)。run:块通过bash -n。风险与范围
MAX_ROUNDS次(约每次扫描一次)才走终态,而非首崩即 handoff。这是预期取舍 —— 瞬时崩溃自愈、持续崩溃仍有界。round 计数把每次崩溃计为一轮,人看到的尝试次数是准确的。关联 Issue
#7219 卡死(#7165 → #7225 SKILL-staging 事件后)的根因修复。