fix(autofix): retry a model API error instead of stranding the PR - #7247
Conversation
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. #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 #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.
|
Thanks for the PR! Template looks good ✓ (the "Why it's needed" section is folded into "What this PR does" + "The fix" — content is all there). Problem: observed bug with clear evidence. #7220 is a real incident — the autofix key lacked access to Direction: aligned. This is the same "transient failure should self-heal" line as #7229 (crash), #7230/#7231 (flaky), #7233 (review API retry). A model API error is a transient/config failure the agent didn't evaluate through — retrying instead of stranding is the correct behavior. The auth/transient split with a reduced retry budget for auth errors is a sensible refinement. Size: not applicable — changes are in Approach: the scope feels right. The marker-file pattern mirrors #7229's existing approach, the classification logic is position-based (status code read from Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓（"Why it's needed" 内容融入了 "What this PR does" + "The fix"——内容齐全）。 问题：已观测到的 bug，有明确证据。#7220 是真实事件——autofix 密钥没有 方向：对齐。与 #7229（崩溃）、#7230/#7231（flaky）、#7233（review API 重试）同一"瞬时失败应自愈"主线。模型 API 错误是 agent 没跑通的瞬时/配置失败——重试而非搁置是正确行为。auth/transient 分类 + auth 错误缩减重试预算是合理的细化。 规模：不适用——改动在 方案：范围合理。标记文件模式对齐 #7229 的既有方案，分类逻辑基于位置（状态码从 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: I would detect Comparison with the diff: the PR does exactly this, and more carefully than I would have:
No critical blockers. No AGENTS.md violations. The code follows the existing marker-file pattern from #7229 and the test style is consistent with the rest of the suite. TestingThis is a CI workflow change (GitHub Actions YAML + autofix scripts), not user-facing CLI behavior. The test suite exercises the actual bash blocks via New behavioral coverage (7 new test cases):
中文说明代码审查独立方案： 我会检测 agent 输出尾部的 与 diff 对比： PR 正是这样做的，而且比我设想的更细致：
无关键阻塞。无 AGENTS.md 违规。 测试这是 CI 工作流变更（GitHub Actions YAML + autofix 脚本），非用户可见的 CLI 行为。测试套件通过 83/83 + 12/12 全部通过。7 个新测试用例覆盖了 API 错误标记、非 API 失败、终态守卫、无状态码渲染、原因分类、最后错误优先、UTF-8 安全、写↔读合约。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This PR solves a real, observed problem (#7220, #7365) with a focused, well-tested fix that follows the existing marker-file pattern from #7229. The classification logic is the right level of sophistication — position-based status code extraction avoids the false-positive trap that the previous /review round caught, and the auth/transient budget split is a practical refinement that prevents burning agent runs on a 403 only a maintainer can fix. Going back to my independent proposal: the PR matches it and exceeds it. The guard layering (timeout, loop detection, agent-written verdict), the last-error-wins semantics, the UTF-8 safety on CJK byte-cap splits, and the writer↔reader contract test are all things I would have wanted but might not have thought to add on the first pass. The test coverage is thorough — 7 new cases covering the happy path, every guard, the classification matrix, ordering, and the end-to-end contract. Every change in the diff is necessary for the stated goal. No drive-by refactors, no scope creep. If I had to maintain this in six months, the comments and tests would tell me exactly why each decision was made. The stale CHANGES_REQUESTED review (bare 中文说明置信度：5/5——每个阶段都干净；毫不犹豫合并。 这个 PR 用聚焦、充分测试的修复解决了一个真实观测到的问题（#7220、#7365），遵循了 #7229 的既有标记文件模式。分类逻辑是正确的复杂度级别——基于位置的状态码提取避免了上一轮 /review 发现的误报陷阱，auth/transient 预算分配是实用的细化，防止在只有维护者才能修的 403 上烧 agent 运行。 与我的独立方案对比：PR 匹配并超越了它。守卫分层、最后错误优先语义、CJK 字节截断的 UTF-8 安全、写↔读合约测试都是我想要但可能第一次不会想到加的。测试覆盖全面——7 个新用例覆盖了正常路径、每个守卫、分类矩阵、排序和端到端合约。 diff 中每个改动都是既定目标所必需的。无顺手重构，无范围蔓延。 过时的 CHANGES_REQUESTED review（裸 — 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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
|
Qwen Code review timed out. Qwen review timed out after 300 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
ReviewThe diagnosis is right and the shape of the fix matches #7229's precedent: a model-side error means the agent evaluated nothing, so the watermark must not advance. Cause-aware headlines are a genuine UX win over the generic "a human should take over". Three things I think need addressing before merge — one behavioral, one coverage-scope, one test gap. 1. The marker is written on failure paths the PR says stay terminalIn
B, C and D each contradict an invariant the PR body states as unchanged. C is the worst: the agent produced a real verdict, and the watermark now silently does not advance, so that verdict is discarded and the same feedback re-runs. B is the most expensive: loop-guard runs burn the full tool-call cap, and they'd now repeat up to Reachability is the part worth weighing. Note the } else {
writeFailure(options.workdir, `Qwen failed during ${options.mode}: ${detail}.${
result.apiError ? ` ${result.apiError}` : ''
}`);
// Only a bare, un-evaluated API failure is retryable. A loop guard, a
// timeout, or an agent-written failure.md is a real verdict — leave those
// terminal even if an API error string appears in the tail.
if (result.apiError && !result.timedOut) {
writeFileSync(file(options.workdir, 'agent-api-error'), `${result.apiError}\n`);
}
}2. The regex misses most real
|
Addresses wenshao's review on #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.
|
All three main points + the smaller ones addressed in 1. Marker scoped to a bare, un-evaluated failure. The write moved inside the same 2. Detection is now a RECOVERABLE whitelist, not a leading-digit scan. Extraction is single-line One deliberate deviation from your suggestion: you proposed 3. Writer↔reader contract test. New test runs the real Smaller:
79/79 across both suites. Ready for re-review. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
…review) Two coverage gaps from the ci-bot review on #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 …]'.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const RECOVERABLE_API_ERROR = | ||
| /\b(?:401|402|403|429|5\d\d)\b|rate.?limit|quota|exceeded|RESOURCE_EXHAUSTED|overloaded|temporarily|too many requests|api key|\u901f\u7387\u9650\u5236|\u914d\u989d|\u670d\u52a1(?:\u7e41\u5fd9|\u4e0d\u53ef\u7528)/i; |
There was a problem hiding this comment.
[Suggestion] The bare exceeded keyword matches permanent 400 errors whose message text contains that word (e.g., [API Error: 400 Request exceeded maximum token limit]), contradicting the comment's stated intent that "a 400 malformed request or a plain 404 fails identically forever and stays terminal." The workflow retries up to MAX_ROUNDS times on a failure that will never self-heal.
Concrete cost: quota and rate.?limit already cover messages where "exceeded" appears ("Quota exceeded", "Rate limit exceeded"). Removing the standalone exceeded loses no coverage and eliminates the false-positive retry loop on permanent context/payload size errors.
| const RECOVERABLE_API_ERROR = | |
| /\b(?:401|402|403|429|5\d\d)\b|rate.?limit|quota|exceeded|RESOURCE_EXHAUSTED|overloaded|temporarily|too many requests|api key|\u901f\u7387\u9650\u5236|\u914d\u989d|\u670d\u52a1(?:\u7e41\u5fd9|\u4e0d\u53ef\u7528)/i; | |
| const RECOVERABLE_API_ERROR = | |
| /\b(?:401|402|403|429|5\d\d)\b|rate.?limit|quota|RESOURCE_EXHAUSTED|overloaded|temporarily|too many requests|api key|\u901f\u7387\u9650\u5236|\u914d\u989d|\u670d\u52a1(?:\u7e41\u5fd9|\u4e0d\u53ef\u7528)/i; |
— qwen3.7-max via Qwen Code /review
|
Qwen Code review paused — model quota exhausted. Qwen review stopped: the model API quota is exhausted. Transient errors auto-retry, but a quota reset is too far out to wait on a runner. Re-run once it resets by commenting |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/29752456855)._ |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
— qwen3.8-max-preview via Qwen Code /review
| const RECOVERABLE_API_ERROR = | ||
| /\b(?:401|402|403|429|5\d\d)\b|rate.?limit|quota|exceeded|RESOURCE_EXHAUSTED|overloaded|temporarily|too many requests|api key|\u901f\u7387\u9650\u5236|\u914d\u989d|\u670d\u52a1(?:\u7e41\u5fd9|\u4e0d\u53ef\u7528)/i; |
There was a problem hiding this comment.
[Critical] The bare exceeded alternative matches non-transient 400 errors (e.g. "context length exceeded", "maximum token limit exceeded"), classifying them as recoverable and triggering retries that fail identically every time — contradicting the comment on line 76 that "A 400 malformed request … fails identically forever and stays terminal (no match)." — Failure scenario: the model returns [API Error: 400 Bad request: context length exceeded] → exceeded matches → agent-api-error marker written → workflow stamps sentinel ts → retries up to MAX_ROUNDS on a permanent error, wasting CI minutes and delaying human takeover. The existing test only covers [API Error: 400 Bad request: malformed], which lacks the word "exceeded".
| const RECOVERABLE_API_ERROR = | |
| /\b(?:401|402|403|429|5\d\d)\b|rate.?limit|quota|exceeded|RESOURCE_EXHAUSTED|overloaded|temporarily|too many requests|api key|\u901f\u7387\u9650\u5236|\u914d\u989d|\u670d\u52a1(?:\u7e41\u5fd9|\u4e0d\u53ef\u7528)/i; | |
| const RECOVERABLE_API_ERROR = | |
| /\b(?:401|402|403|429|5\d\d)\b|rate.?limit|quota|RESOURCE_EXHAUSTED|overloaded|temporarily|too many requests|api key|\u901f\u7387\u9650\u5236|\u914d\u989d|\u670d\u52a1(?:\u7e41\u5fd9|\u4e0d\u53ef\u7528)/i; |
— qwen3.8-max-preview via Qwen Code /review
| for (const render of [ | ||
| '[API Error: Rate limit exceeded (Status: RESOURCE_EXHAUSTED)]', | ||
| '[API Error: 401 Incorrect API key provided.]', | ||
| ]) { |
There was a problem hiding this comment.
[Suggestion] The four Chinese-language patterns in RECOVERABLE_API_ERROR (速度限制, 配额, 服务繁忙, 服务不可用) have zero test coverage. A typo in any Unicode escape would silently disable retry for Chinese-locale model errors — the exact bug this PR fixes, but for Chinese error renders. — Concrete cost: a contributor reformats the regex and corrupts \u901f → \u90lf; [API Error: 服务繁忙，请稍后重试] no longer matches → no marker → watermark advances → PR stranded.
| for (const render of [ | |
| '[API Error: Rate limit exceeded (Status: RESOURCE_EXHAUSTED)]', | |
| '[API Error: 401 Incorrect API key provided.]', | |
| ]) { | |
| for (const render of [ | |
| '[API Error: Rate limit exceeded (Status: RESOURCE_EXHAUSTED)]', | |
| '[API Error: 401 Incorrect API key provided.]', | |
| '[API Error: 服务繁忙，请稍后重试]', | |
| ]) { |
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| if (result.apiError && !result.timedOut) { | ||
| writeFileSync( | ||
| file(options.workdir, 'agent-api-error'), | ||
| `${result.apiError}\n`, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Two guard issues at the agent-api-error marker write:
-
Marker written despite success verdict. When the agent writes a real success output (
address-summary.mdorno-action.md) then encounters an API error on a subsequent call and exits non-zero, the outer!existsSync(failure.md)gate passes (nofailure.mdexists — the agent wroteaddress-summary.md). A syntheticfailure.mdis created, and theagent-api-errormarker is written. The workflow's[[ -z "${DETAIL_FILE}" || -n "${API_ERROR_DETAIL}" ]]then routes to the sentinel/retry path, discarding the agent's real verdict. — Failure scenario: agent commits a fix, writesaddress-summary.md, then hits a transient API error on a follow-up call → PR comment says "could not reach the model" instead of reporting the actual fix; watermark doesn't advance; same feedback retried. -
Guard omits
!result.loopDetected. The comment declares all three terminal conditions (loop guard, timeout, agent-written verdict) should suppress the marker, but the guard only checks!result.timedOut. The loop-guard case is prevented today by the outerif/elsebranch (Branch A vs B), not by this guard — making the contract fragile to future restructuring.
| if (result.apiError && !result.timedOut) { | |
| writeFileSync( | |
| file(options.workdir, 'agent-api-error'), | |
| `${result.apiError}\n`, | |
| ); | |
| if (result.apiError && !result.timedOut && !result.loopDetected) { | |
| const hasVerdict = spec.outputs.some( | |
| (f) => existsSync(file(options.workdir, f)), | |
| ); | |
| if (!hasVerdict) { | |
| writeFileSync( | |
| file(options.workdir, 'agent-api-error'), | |
| `${result.apiError}\n`, | |
| ); | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
|
Qwen Code review timed out. Qwen review timed out after 300 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
…w prose (QwenLM#7328) * fix(ci): tighten API error detection to avoid false positive on review prose The result-text classifier matched *"[API Error"* which hits review summaries that quote the pattern in prose (e.g. reviewing PR QwenLM#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. * fix(ci): anchor API error detection on tail position, not status-code 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. * fix(ci): catch aborts longer than the tail window; grep full text for 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). * fix(ci): anchor abort detection on trailing shape, not byte window 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). * test(ci): pin suffix sync with errorParsing.ts; document ]-ending trade-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).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-13/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-13--work-qwen-code-qwen-code/subagents/5bf833ae-4ea6-45ba-9a8f-7e4a83259bfc (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-13/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-13--work-qwen-code-qwen-code/subagents/5bf833ae-4ea6-45ba-9a8f-7e4a83259bfc'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read. Not reviewed: verification — could not check that Step 4 and Step 5 ran (no subagent transcripts at /home/github-runner/actions-runner-13/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-13--work-qwen-code-qwen-code/subagents/5bf833ae-4ea6-45ba-9a8f-7e4a83259bfc (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-13/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-13--work-qwen-code-qwen-code/subagents/5bf833ae-4ea6-45ba-9a8f-7e4a83259bfc'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.).
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| for (const render of wrapped.reverse()) { | ||
| const kind = classifyApiError(render); | ||
| if (kind) return { error: render, kind }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] recoverableApiError iterates matches last-to-first and returns the first recoverable one, skipping terminal errors (kind = ''). This violates the NOTE comment's claim of "never a wrongly-retried permanent failure."
Failure scenario: Output tail contains [API Error: 429 Too Many Requests] then [API Error: 400 Bad request: malformed]. After .reverse(), the 400 is checked first (terminal, skipped), then the 429 is returned as 'transient'. The workflow retries up to MAX_ROUNDS on a permanent error — each retry hits the same 400, producing the same mixed output, classified transient again.
Consider classifying only the last [API Error: ...] render — it represents the terminal state:
if (wrapped.length > 0) {
const last = wrapped[wrapped.length - 1];
const kind = classifyApiError(last);
if (kind) return { error: last, kind };
}— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: Review feedback addressed1. [Suggestion] 400 errors with "does not exist" misclassified as auth (@doudouOUC)Decision: Implemented. A 400 Bad Request is always a malformed client request that reproduces identically on every retry. The Change: Added Tests added: Two new 400 fixtures in the permanent-failure test: 2. [Suggestion]
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
#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.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
Two conflicts, both in the review-address report step, both resolved by combining the two sides rather than choosing one: - Artifact listing: kept this branch's agent-api-error markers AND main's resolved-comments.txt. - Retry routing: main (#7351) added a gate-crash route and this branch added a model-API route to the SAME if-chain. The condition now covers all three no-verdict causes, and the cause naming runs most-specific first — a model death also leaves OUTCOME empty on a failed job, so testing the API error first keeps a provider blip from being reported as a gate problem. The cause-split retry budget (auth capped below MAX_ROUNDS) applies across the merged chain. Test env for the marker-name contract now sets JOB_STATUS=success: an agent that reached a verdict leaves the job green, so without it every case in that test would have read as a gate crash.
|
Merged 1. Artifact listing — union: this branch's 2. Retry routing — the no-verdict condition now covers all three causes, and the cause naming runs most-specific first: if [[ -z "${DETAIL_FILE}" || -n "${API_ERROR_DETAIL}" || "${GATE_CRASHED}" == 'true' ]]; then
if [[ -n "${API_ERROR_DETAIL}" ]]; then # model — check key/access, re-arm
elif [[ -z "${DETAIL_FILE}" ]]; then # no output — human takes over
else # gate — check gate logs, re-armOrdering is load-bearing: a model death also leaves Verification of the seam — 83/83, run three times (one run hit the pre-existing
One test-harness fix: the marker-name contract test now sets 中文说明已合并 1. 产物清单 —— 取并集:本分支的 2. 重试路由 —— 无判定条件现覆盖三种成因,且成因判定从最具体开始(见上方代码块)。 顺序是关键:模型侧死亡在 job 失败时同样会留下空的 接缝验证 —— 83/83,连跑三次(其中一次命中既有的 另修正一处测试夹具:marker 名称契约测试现显式设置 |
ReviewLGTM with three minor notes. The core routing is correct and genuinely needed. I traced the actual strand mechanism: on a model API death, Verified locally at 0216f01
Findings (all minor)
For the record: bare 🤖 Review by Claude Code · model |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
|
@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. ✅
…acy, display clamp (#7247)
|
🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on: Review feedback addressedAll three findings implemented in one commit. 1. OAuth quota fallback overrides terminal wrapped-error verdict ([Suggestion] rc:3619571992 + Nit 1)Decision: Implemented. Change: In Test added: A stub that emits both 2. Two comments mis-state the GATE_CRASHED mechanism (Nit 2)Decision: Implemented. Change: Reworded both comments (workflow YAML + test file) to accurately describe today's behavior: run-agent writes 3. Terminal auth headline can read "attempt 4/3" (Nit 3)Decision: Implemented. Change: Added Test added: Verification
中文说明已处理的评审反馈三条发现均已在一次提交中实现。 1. OAuth 配额回退覆盖终态包装错误判定（[Suggestion] rc:3619571992 + Nit 1）决定： 已实现。 变更： 在 新增测试： 一个同时在 stderr 输出 2. 两处注释错误描述了 GATE_CRASHED 机制（Nit 2）决定： 已实现。 变更： 重写了 workflow YAML 和测试文件中的两处注释，准确描述当前行为：run-agent 在 API 死亡路径上写入 3. 终态 auth 标题可能显示 "attempt 4/3"（Nit 3）决定： 已实现。 变更： 在标题构造前添加了 新增测试： 验证
Base-conflict check: no conflict with main. Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. 🧠 Handled by Qwen Code · model/模型 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — the classifier design (status-code-first, 400 unconditional short-circuit, last-error-wins, AUTH-before-TRANSPORT for code-less renders) is well-reasoned and the test coverage is thorough (CJK byte-split, writer↔reader marker contract, timeout Case D, OAuth standalone, ordering permutations).
Two non-blocking observations, no action needed:
-
408 gap in
classifyApiError. A 408 (Request Timeout) with a status code falls through to theAUTH_API_ERRORkeyword check and defaults to terminal, but it is transient by nature. Extremely rare for LLM APIs so no practical impact today — noting in case the catch-all for coded statuses ever gets aTRANSIENT_API_ERRORkeyword pass too. -
terminatedis the broadest keyword inTRANSPORT_API_ERROR. The AUTH-first ordering and the 400 short-circuit prevent the realistic false positives (e.g. "invalid API key" is caught by AUTH before TRANSPORT is consulted), so this is fine as-is. Just flagging that a future code-less render like[API Error: Process terminated: <permanent reason>]would classify transient — the single-line constraint and last-error-wins rule keep the blast radius small.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
|
Released in v0.20.1. |
|
[API Error: Connection error. (cause: fetch failed)] ❌ failed |
5 participants
What this PR does
Stops a model API error from stranding a managed PR. When the agent's qwen subprocess dies on a model-side
[API Error]— a403access denied, a429quota, a5xx— the agent never actually evaluated the feedback, yet the failure was treated as an evaluated handoff:run-agent.mjswrotefailure.md/handoff.md, so the handoff step advanced the feedback watermark. The next scan then saw "nothing new" and never retried — the PR sat stranded until a manual re-arm.This is exactly what happened on #7220: fork-takeover engaged and ran the agent, the model returned
[API Error: 403 Model access denied](the autofix key lacks access toqwen3.8-max-preview), and the PR was left with an advanced watermark that won't retry.The fix
Mirrors #7229's no-output-crash handling — a model API error is a transient/config failure the agent didn't evaluate through, so it should retry, not advance the watermark:
run-agent.mjsextracts a[API Error: 4xx/5xx]from the captured output tail, includes it infailure.md(so the human-visible handoff names the real error, not a barestatus 1), and drops anagent-api-errormarker file.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 "a human should take over."Unchanged: a real evaluated handoff (the agent wrote a verdict, verify failed) still advances the watermark; a pre-verdict crash (no output) still retries via #7229's path; a genuine timeout stays terminal.
Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 62/62. New behavioral coverage:[API Error: 403 …]and exits 1 produces theagent-api-errormarker and afailure.mdnaming the error; a stub that fails on stderr (no API error) produces no marker.MARK_TSblock): an API-error handoff yieldssentinel|round+1(retry, no watermark advance), and the headline says "could not reach the model — [API Error: …]" (retry mid-attempt; "check the autofix model key/access" on the final attempt).npx vitest run scripts/tests/qwen-fleet-shepherd-workflow.test.js— 12/12.run:block passesbash -n;node --checkon run-agent.mjs.Evidence (Before & After)
Tested on
Risk & Scope
MAX_ROUNDStimes (one per scan) before the terminal handoff, instead of stranding on the first. The round cap bounds it, and the final-attempt headline points at the operator fix. For a takeover PR (MAX_ROUNDS=100) that is more retries — but a 403 model error is a fast, cheap failure (the sandbox exits immediately), and it self-heals the moment access/quota is restored.qwen3.8-max-preview, or point autofix at a model the key can use) is a maintainer action this PR does not change — it only makes the loop self-heal once that's done, instead of needing a manual re-arm.Linked Issues
Motivated by #7220 (403 Model access denied). Same "transient failure should self-heal" line as #7229 (crash), #7230/#7231 (flaky), #7233 (review API retry).
中文说明
本 PR 做了什么
阻止模型 API 错误搁置被托管的 PR。当 agent 的 qwen 子进程死于模型端
[API Error](403拒绝、429配额、5xx)时,agent 其实没有评估任何反馈,却被当成已评估的 handoff:run-agent.mjs写了failure.md/handoff.md,handoff 步骤便推进了反馈水位线。下次扫描看到"无新反馈"就不再重试 —— PR 被搁置,直到人工重新武装。#7220 正是如此:fork 托管engage并跑了 agent,模型返回
[API Error: 403 Model access denied](autofix 密钥没有qwen3.8-max-preview的权限),PR 被留在一个前进过的水位线上、不会重试。修复
对齐 #7229 的"无产出崩溃"处理 —— 模型 API 错误是 agent 没跑通的瞬时/配置失败,应当重试而非推进水位线:
run-agent.mjs从捕获的输出尾部提取[API Error: 4xx/5xx]、纳入failure.md(让人可见的 handoff 写出真实错误而非空洞的status 1),并落一个agent-api-error标记文件。MAX_ROUNDS约束。标题写出模型错误,并在末次尝试时提示维护者检查 autofix 模型密钥/权限并重新武装 —— 而非泛泛的"交人工"。不变:真正评估过的 handoff(写了结论、verify 失败)仍推进水位线;产出前崩溃(无输出)仍走 #7229 重试;真超时仍终态。
评审验证
npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 62/62:run-agent.mjs 实跑(stub qwen)—— 打印[API Error: 403 …]并 exit 1 的 stub 产出agent-api-error标记 + 写出错误的failure.md;仅 stderr 失败(无 API 错误)不产出标记。handoff 回放(对提取的MARK_TS块实跑 bash)—— API-error handoff 得sentinel|round+1(重试、不推水位),标题为"could not reach the model — [API Error: …]"。npx vitest run scripts/tests/qwen-fleet-shepherd-workflow.test.js—— 12/12。run:块过bash -n;run-agent.mjs 过node --check。风险与范围
MAX_ROUNDS次(每次扫描一次)才走终态,而非首次即搁置。round 上限有界,末次标题指向操作修复。takeover PR(MAX_ROUNDS=100)重试更多,但 403 是快速廉价的失败(沙箱立即退出),且权限/配额一恢复即自愈。qwen3.8-max-preview,或让 autofix 用密钥可访问的模型)是维护者动作,本 PR 不改 —— 只让循环在修好后自愈,不再需要人工重新武装。关联 Issue
由 #7220(403 Model access denied)驱动。与 #7229(崩溃)、#7230/#7231(flaky)、#7233(review API 重试)同一"瞬时失败应自愈"主线。