fix(dingtalk): retry transient emotion failures - #7329
Merged
Conversation
Collaborator
Author
Verification reportBaseline reproduction on Post-fix verification:
The required read-only test-engineer verification independently confirmed the same three retry outcomes and the complete 74/74 adapter test result. Repository-wide build/typecheck was attempted but is not reported as passing: this isolated worktree reused dependencies from an older checkout, producing unrelated current-main dependency errors for packages including 验证报告在 修复后验证：
按仓库要求运行的只读 test-engineer 也独立确认了上述三个重试结果以及完整适配器测试 74/74 通过。 已尝试仓库级 build/typecheck，但不将其报告为通过：隔离 worktree 复用了较旧检出目录的依赖，因此在 |
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-dev-bot
marked this pull request as ready for review
July 20, 2026 11:11
qqqys
previously approved these changes
Jul 20, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the complete diff and the shared reply/recall call path. The retry classification, bounded attempt count, final-error sanitization, and best-effort behavior are consistent with the issue. I independently ran the full DingTalk adapter test file (74/74) and the three focused retry cases (3/3); no blocking issues found. One non-blocking inline suggestion would close the remaining branch-coverage gap.
| }); | ||
| if (resp.ok) return; | ||
|
|
||
| const isTransient = resp.status === 429 || resp.status >= 500; |
Collaborator
There was a problem hiding this comment.
Suggestion: please add a focused 429 retry case. The new predicate has a distinct status === 429 branch, while the current recovery test only exercises 500 responses, so a future regression could silently drop the rate-limit behavior this PR promises. A short 429 → 200 case should be enough.
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with real evidence. Issue #7327 captures an actual transient Direction: aligned. This is a best-effort acknowledgement reaction in the DingTalk channel adapter; a short bounded retry recovers from transient server errors without changing the "never break message flow" guarantee. It doesn't touch auth, sandbox, model selection, telemetry, or any public contract. CHANGELOG has no direct reference, but the area (channel reliability) is squarely in scope. Size: not a core-infrastructure path ( Approach: the scope feels right and minimal — an inline bounded loop (3 attempts, 250/500 ms backoff), retry only on 429/5xx, fail fast on other 4xx, terminal failure logged once. No drive-by changes, no new abstraction or dependency. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，有真实证据。Issue #7327 记录了钉钉 emotion 接口一次真实的临时性 方向：对齐。这是钉钉通道适配器中一个尽力而为的确认表情；短暂且有上限的重试可以从临时性服务端错误中恢复，同时不改变"绝不中断消息流"的保证。它不触及鉴权、沙箱、模型选择、遥测或任何公共契约。CHANGELOG 无直接引用，但该领域（通道可靠性）完全在范围内。 规模：非核心基础设施路径（ 方案：范围合理且最小化——内联的有上限循环（3 次尝试，250/500 毫秒退避），仅对 429/5xx 重试，其他 4xx 快速失败，最终失败只记录一次。无顺手改动，无新抽象或依赖。进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code reviewMy independent take before reading the diff: a transient A few things I checked specifically:
No correctness, security, or regression concerns found. The two new tests (recovery on Real-scenario testingOne honesty note up front: this retry path only fires on live DingTalk channel traffic — an inbound webhook message drives Before (pre-fix adapter,
|
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. Stepping back: this is a tight, well-motivated fix. The problem is real and reproduced — #7327 captured an actual transient My independent proposal before reading the code was essentially what landed here, so there's no simpler path I think was missed. If I had to maintain this in six months I'd thank the author: two well-named constants, an obvious loop, and deterministic fake-timer tests covering recovery, the retry bound, and the non-transient fast-fail. The only thing I'd note is purely informational and predates this PR — the success path doesn't consume the response body — but that's the existing behavior of a fire-and-forget reaction call and not something this change should take on. LGTM, ships the fix cleanly. ✅ 中文说明置信度：5/5 —— 每个阶段都干净，毫不犹豫地合并。 退一步看：这是一个紧凑、动机充分的修复。问题真实且已复现——#7327 捕获了 emotion 接口一次真实的临时性 读代码之前我的独立方案基本就是这里落地的样子，所以我认为没有遗漏更简的路径。如果半年后由我维护，我会感谢作者：两个命名清晰的常量、一个显而易见的循环，以及用假定时器确定性覆盖恢复、重试上限和非临时快速失败的测试。唯一要提的纯属信息性、且早于本 PR——成功路径没有消费响应体——但那是即发即忘表情请求的既有行为，不该由本次改动承担。 LGTM，干净地交付了修复。✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 20, 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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
Comment on lines
+829
to
+832
| emotionAttempts++; | ||
| return Promise.resolve( | ||
| new Response('{}', { status: emotionAttempts < 3 ? 500 : 200 }), | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The isTransient predicate has a distinct resp.status === 429 branch, but all three retry tests exercise only the >= 500 path with status 500. A future refactor that simplifies the condition to resp.status >= 500 would pass every existing test while silently dropping rate-limit retry support. — Failure scenario: 429 responses stop retrying, DingTalk rate-limit errors immediately log failure instead of backing off.
Consider adding a test that returns 429 on the first attempt and 200 on the second, asserting emotionAttempts === 2, or parameterize the existing retry test over [429, 500, 502].
— qwen3.7-max via Qwen Code /review
Comment on lines
714
to
717
| const detail = sanitizeLogText(await resp.text().catch(() => ''), 500); | ||
| process.stderr.write( | ||
| `[DingTalk:${this.name}] emotion/${endpoint} failed: ${resp.status} ${detail}\n`, | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The terminal error log is identical whether the failure happened on the first attempt (non-transient 4xx) or after exhausting all 3 retries (persistent 500). An oncall engineer seeing [DingTalk:bot] emotion/reply failed: 500 {...} cannot tell from the log alone whether retries ran. — Failure scenario: sustained DingTalk outage → every log line looks like a single failure → misdiagnosis of whether the retry mechanism is active.
Suggested change
| const detail = sanitizeLogText(await resp.text().catch(() => ''), 500); | |
| process.stderr.write( | |
| `[DingTalk:${this.name}] emotion/${endpoint} failed: ${resp.status} ${detail}\n`, | |
| ); | |
| const detail = sanitizeLogText(await resp.text().catch(() => ''), 500); | |
| process.stderr.write( | |
| `[DingTalk:${this.name}] emotion/${endpoint} failed after ${attempt + 1}/${EMOTION_MAX_ATTEMPTS} attempts: ${resp.status} ${detail}\n`, | |
| ); |
— qwen3.7-max via Qwen Code /review
Comment on lines
+933
to
+939
| const emotionCalls = fetchSpy.mock.calls.filter((call) => | ||
| String(call[0]).startsWith( | ||
| 'https://api.dingtalk.com/v1.0/robot/emotion/reply', | ||
| ), | ||
| ); | ||
| expect(emotionCalls).toHaveLength(3); | ||
| expect(stderr).toHaveBeenCalledOnce(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The expect(emotionCalls).toHaveLength(3) assertion couples this sanitization test to retry-count behavior. The dedicated retry test already asserts emotionAttempts === 3; this test's stated purpose is to verify log content sanitization. — Failure scenario: if a future change makes status 500 non-transient, this test fails with expected 3 to be 1 instead of reaching the sanitization assertions, misidentifying the regression.
Consider removing the emotionCalls length assertion from this test and keeping it focused on log content.
— qwen3.7-max via Qwen Code /review
Comment on lines
+705
to
+712
| const isTransient = resp.status === 429 || resp.status >= 500; | ||
| if (isTransient && attempt < EMOTION_MAX_ATTEMPTS - 1) { | ||
| await resp.body?.cancel(); | ||
| await new Promise((resolve) => | ||
| setTimeout(resolve, EMOTION_RETRY_BASE_DELAY_MS * 2 ** attempt), | ||
| ); | ||
| continue; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] If a fetch() call inside the retry loop throws a network error (ECONNRESET, DNS failure), the outer catch {} swallows it silently — but the retry loop has now introduced a multi-fetch sequence where a prior transient 500 that triggered the retry is also never logged. — Failure scenario: first call returns 500 → retry sleeps 250ms → second fetch() throws → outer catch swallows → neither the 500 nor the network error appears in any log, making it harder to diagnose why a reaction emoji was never attached.
Consider wrapping the per-iteration fetch in its own try-catch that logs the error on the last attempt, so the outer catch only fires for truly unexpected errors.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 1/5). What changed, and what I pushed back on: Review feedback addressed1. Add a focused 429 retry test (@qqqys + @qwen-code-ci-bot)Decision: Implemented. 2. Include attempt count in terminal error log (@qwen-code-ci-bot)Decision: Implemented. 3. Remove
|
qwen-code-dev-bot
dismissed stale reviews from qwen-code-ci-bot and qqqys
via
July 20, 2026 15:48
72a4689
qwen-code-ci-bot
approved these changes
Jul 20, 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.7-max via Qwen Code /review
This was referenced Jul 20, 2026
Merged
Collaborator
✅ Local verification — ready to mergeI built and ran this branch ( Environment: Linux x86_64 · Node v22.22.2 · vitest 3.2.4 What I ran
Independent real-timer cross-checkThe PR's tests use
The measured timings match the documented backoff exactly, transient (
Notes
Verdict: behavior independently confirmed, all CI gates green locally. LGTM to merge. 👍 中文说明✅ 本地验证 —— 可以合并我在隔离 worktree 中基于当前 环境： Linux x86_64 · Node v22.22.2 · vitest 3.2.4 执行的检查
独立真实定时器交叉验证PR 的测试使用
实测耗时与文档中的退避策略完全一致：临时性（ （截图见上方英文部分） 备注
结论： 行为已独立验证，本地所有 CI 门禁均为绿色。同意合并。👍 🤖 Verified locally with Claude Code (Opus 4.8). |
wenshao
approved these changes
Jul 20, 2026
doudouOUC
reviewed
Jul 20, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: issue-fidelity — gh CLI authentication expired, PR context unavailable. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Agent 1a: Line-by-line correctness, Agent 2: Security, Agent 3: Code quality, Agent 4: Performance & efficiency, Agent 5: Test coverage, Agent 6a: Undirected audit — attacker mindset, Agent 6b: Undirected audit — 3 AM oncall mindset, Agent 6c: Undirected audit — six-months-later maintainer, Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — its prompt was built, but no agent was launched with it — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries, and cannot be certified. Not reviewed: verification — its prompt was built, but no agent was launched with it, so the posted findings cannot be counted as verified.
— qwen3.7-max via Qwen Code /review
Comment on lines
+705
to
+707
| const isTransient = resp.status === 429 || resp.status >= 500; | ||
| if (isTransient && attempt < EMOTION_MAX_ATTEMPTS - 1) { | ||
| await resp.body?.cancel(); |
Collaborator
There was a problem hiding this comment.
[Suggestion] 429 responses are retried without honouring the server's Retry-After header. — Failure scenario: DingTalk returns 429 with Retry-After: 10. The code retries in 250ms, gets another 429, retries in 500ms, gets a third 429, and gives up — all within 750ms. Three rapid-fire requests in violation of the server's rate limit may escalate enforcement (temporary IP/token ban).
Suggested change
| const isTransient = resp.status === 429 || resp.status >= 500; | |
| if (isTransient && attempt < EMOTION_MAX_ATTEMPTS - 1) { | |
| await resp.body?.cancel(); | |
| const isTransient = resp.status === 429 || resp.status >= 500; | |
| if (isTransient && attempt < EMOTION_MAX_ATTEMPTS - 1) { | |
| await resp.body?.cancel(); | |
| let delay = EMOTION_RETRY_BASE_DELAY_MS * 2 ** attempt; | |
| if (resp.status === 429) { | |
| const retryAfter = resp.headers.get('Retry-After'); | |
| if (retryAfter) { | |
| const parsed = Number(retryAfter); | |
| if (Number.isFinite(parsed) && parsed > 0) { | |
| delay = Math.min(parsed * 1000, 5000); | |
| } | |
| } | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, delay)); |
— qwen3.7-max via Qwen Code /review
Comment on lines
+706
to
+710
| if (isTransient && attempt < EMOTION_MAX_ATTEMPTS - 1) { | ||
| await resp.body?.cancel(); | ||
| await new Promise((resolve) => | ||
| setTimeout(resolve, EMOTION_RETRY_BASE_DELAY_MS * 2 ** attempt), | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Intermediate retry attempts produce no log output — only the final failure is logged. — Concrete cost: at 3 AM, an oncall engineer sees emotion/reply failed after 3/3 attempts: 500 {"errmsg":"system busy"} but cannot tell whether all 3 attempts hit 500 or whether the error changed between attempts. A one-line log per retry (with attempt number and status) makes this trivially diagnosable.
Suggested change
| if (isTransient && attempt < EMOTION_MAX_ATTEMPTS - 1) { | |
| await resp.body?.cancel(); | |
| await new Promise((resolve) => | |
| setTimeout(resolve, EMOTION_RETRY_BASE_DELAY_MS * 2 ** attempt), | |
| ); | |
| if (isTransient && attempt < EMOTION_MAX_ATTEMPTS - 1) { | |
| await resp.body?.cancel(); | |
| process.stderr.write( | |
| `[DingTalk:${this.name}] emotion/${endpoint} attempt ${attempt + 1}/${EMOTION_MAX_ATTEMPTS} failed (${resp.status}), retrying\n`, | |
| ); | |
| await new Promise((resolve) => | |
| setTimeout(resolve, EMOTION_RETRY_BASE_DELAY_MS * 2 ** attempt), | |
| ); |
— qwen3.7-max via Qwen Code /review
pull Bot
pushed a commit
to bit-cook/qwen-code
that referenced
this pull request
Jul 20, 2026
… deleting a marker (QwenLM#7354) * feat(autofix): re-arm a stranded PR with @qwen-code /retry instead of deleting a marker Recovering a stranded managed PR meant running `gh api -X DELETE` against the bot's own autofix-eval marker comment. That needed raw API access and the comment id, erased the audit trail, and was undiscoverable unless you had read the workflow — it came up twice while triaging QwenLM#7246, QwenLM#7329 and QwenLM#7336. `@qwen-code /retry` now posts a single `<!-- autofix-rearm -->` marker, which does both halves of what the deletion did: - The scan's watermark ignores eval markers written BEFORE the newest re-arm, so the feedback those markers buried is read again. The watermark stays global otherwise — this is an explicit, maintainer-issued exception, which is exactly what the deletion was, only recorded instead of destructive. - The marker also opens a fresh counting window (it joins the engage ack in REARM_KEY), so the round counter resets and a terminal round stops skipping the PR. That also means the existing "a re-arm supersedes queued old-window jobs" guard covers /retry for free. The address job's live recheck mirrors both, so a run selected before a re-arm still discards itself instead of stamping an old-sequence marker. Authorization is the takeover command's, unchanged and reused rather than reinvented: exact body match, live permission lookup, in-repo-only author privilege. The route prefilter now admits the second command. The job verifies CI_DEV_BOT_PAT authenticates as the bot before commenting, because both scanners only count markers authored by it. The marker is registered as a control comment so the agent never sees the re-arm as feedback to address. Tests: the real extracted scan block is replayed over synthetic comment fixtures — stranded (watermark held, round 2), after /retry (watermark released, window reset, round 0), a marker written after the re-arm counting again, and a re-arm from a non-bot author correctly ignored. Both halves mutation-verified. * test(autofix): add behavioral test for address-side re-arm stale check (QwenLM#7354) * fix(autofix): generalize remaining command-ignored messages and assert all filter sites (QwenLM#7354) * test(autofix): add behavioral test for the retry-command re-arm marker job (QwenLM#7354) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
pchaganti
pushed a commit
to pchaganti/qx-qwen-code
that referenced
this pull request
Jul 21, 2026
…agent's fix (QwenLM#7351) * fix(autofix): retry a verification-gate crash instead of burying the agent's fix A gate failure had two very different meanings collapsed into one outcome. When the gate DECLARES a verdict (outcome=failed) it evaluated the agent's attempt and rejected it, so advancing the watermark is right — the same feedback would reproduce the same rejection, and MAX_ROUNDS bounds it. But when the gate dies WITHOUT a verdict it never judged the work at all, and advancing buries a fix the agent had already written: the next scan sees "nothing new" and the PR sits until a human deletes the marker by hand. That is exactly how the nested-package ENOENT stranded QwenLM#7329 and QwenLM#7336. Both agents had implemented the review feedback — the handoff even quoted the implemented changes — but the gate crashed on its own bug while resolving packages/channels/*, the commit was discarded, and the PRs read as "Could not address the latest feedback automatically". Two halves: - The review-address gate now declares every rejection it can legitimately reach: build, typecheck, lint and the per-package tests each call a `reject_fix` helper that writes outcome=failed before exiting. (The resolver call is deliberately left undeclared — a resolver error IS a gate bug.) - The handoff treats an EMPTY outcome on a non-success job as the gate's own crash and routes it to the existing sentinel/retry path, so the feedback stays live and the next scan retries. The round still increments, so a persistently crashing gate is bounded exactly as before, and the headline names the real cause ("hit a verification-gate error before reaching a verdict") and, on the final attempt, points at the gate logs. Unchanged: a declared rejection still advances and reads as before, a no-output crash keeps its own wording and retry, and a crash before the feedback was read stays terminal. Tests: the real extracted decision block is replayed under bash across declared rejection (advances to NEWEST), gate crash (sentinel + retry + round+1), no output (sentinel, original wording), the round cap (operator fix), and a successful job (never a crash); plus the reject_fix helper is driven for real to prove a rejection writes outcome=failed. Both mutation-verified — dropping the crash arm, or unwiring one known rejection, turns them red. * fix(autofix): clarify retry-branch comments per review nits (QwenLM#7351) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com>
pull Bot
pushed a commit
to edisplay/qwen-code
that referenced
this pull request
Jul 21, 2026
…wenLM#7355) * feat(autofix): render the managed fleet into the scan's run summary Seeing whether the loop was healthy meant reconstructing it by hand: list the bot's PRs, fetch each one's comments, regex the autofix-eval markers for round and watermark, then cross-check gh pr checks and the fork/takeover state. That is how today's triage of QwenLM#7246, QwenLM#7259, QwenLM#7329, QwenLM#7333 and QwenLM#7336 was done, and it is why a stalled PR stayed invisible until somebody went looking for it. The scan already computes every one of those facts while deciding what to process — it just wrote them to a job log nobody reads. Each per-PR terminal decision now also records a row, and the step renders one markdown table into the run summary: | PR | State | Detail | | QwenLM#7329 | SELECTED | 1 review + 5 inline new (round 0/5) | | QwenLM#7333 | idle | nothing new since 2026-07-20T13:54:18Z | | QwenLM#7262 | waiting | active checks in flight | | QwenLM#7208 | round-capped | round 100/100 - needs a human or @qwen-code /retry | States cover every branch that ends a PR's inspection: busy, skipped, unknown, waiting, round-capped, idle and SELECTED — so a PR cannot drop out of the table by returning early, which is exactly the invisibility this fixes. No new API calls (the data is already in hand), no writes outside the run summary, and the helper is defined at the top of the step so it stays clear of the BUSY_PRS/INSPECTED proximity guard that keeps the free busy-skip from consuming the inspection budget. Tests: the real helper and render block are replayed over fixtures (table structure, one row per state, and an empty fleet still rendering a table), plus each decision branch is pinned to its fleet_row. Mutation-verified: dropping one branch's row turns it red. * fix(autofix): use temp file for fleet test replay; cover fork-head skip (QwenLM#7355) * test(autofix): assert each skipped fleet_row call site individually (QwenLM#7355) * fix(autofix): record fleet rows for both budget-break paths (QwenLM#7355) The candidate-inspection budget break incremented INSPECTED but never called fleet_row, so the PR that tripped the budget was silently absent from the fleet table. The target-budget break left all remaining candidates invisible with no truncation signal. Add a per-PR deferred row before the inspection-budget break and a summary deferred row before the target-budget break so the fleet table stays complete in both cases. * fix(autofix): harden fleet summary render and clean up temp file (QwenLM#7355) Address review feedback: - Escape '|' in detail values to prevent broken table columns - Render budget summary row (PR '-') as em dash instead of '#-' - Add trap for FLEET_FILE cleanup on early exit paths - Document deferred summary row semantics in test comment * fix(autofix): use summary row for candidate-inspection budget break (QwenLM#7355) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
RehDen
pushed a commit
to RehDen/qwen-code
that referenced
this pull request
Jul 21, 2026
…at it broke (QwenLM#7368) * fix(autofix): retry a verification-gate crash instead of burying the agent's fix A gate failure had two very different meanings collapsed into one outcome. When the gate DECLARES a verdict (outcome=failed) it evaluated the agent's attempt and rejected it, so advancing the watermark is right — the same feedback would reproduce the same rejection, and MAX_ROUNDS bounds it. But when the gate dies WITHOUT a verdict it never judged the work at all, and advancing buries a fix the agent had already written: the next scan sees "nothing new" and the PR sits until a human deletes the marker by hand. That is exactly how the nested-package ENOENT stranded QwenLM#7329 and QwenLM#7336. Both agents had implemented the review feedback — the handoff even quoted the implemented changes — but the gate crashed on its own bug while resolving packages/channels/*, the commit was discarded, and the PRs read as "Could not address the latest feedback automatically". Two halves: - The review-address gate now declares every rejection it can legitimately reach: build, typecheck, lint and the per-package tests each call a `reject_fix` helper that writes outcome=failed before exiting. (The resolver call is deliberately left undeclared — a resolver error IS a gate bug.) - The handoff treats an EMPTY outcome on a non-success job as the gate's own crash and routes it to the existing sentinel/retry path, so the feedback stays live and the next scan retries. The round still increments, so a persistently crashing gate is bounded exactly as before, and the headline names the real cause ("hit a verification-gate error before reaching a verdict") and, on the final attempt, points at the gate logs. Unchanged: a declared rejection still advances and reads as before, a no-output crash keeps its own wording and retry, and a crash before the feedback was read stays terminal. Tests: the real extracted decision block is replayed under bash across declared rejection (advances to NEWEST), gate crash (sentinel + retry + round+1), no output (sentinel, original wording), the round cap (operator fix), and a successful job (never a crash); plus the reject_fix helper is driven for real to prove a rejection writes outcome=failed. Both mutation-verified — dropping the crash arm, or unwiring one known rejection, turns them red. * feat(autofix): feed the gate's rejection back so the retry can fix what it broke QwenLM#7208 was handed to a human over a two-character fix. The agent implemented two review findings, the gate refused the commit because it did not compile (TS4111: `truncated` comes from an index signature, use `['truncated']`), and the loop stopped there — round 5/100, "A human should take over this PR". Nothing in the loop could have recovered on its own, because the reason was never carried anywhere the loop could read it: - the handoff comment showed only the agent's optimistic summary, so neither a human nor the next round could see WHY it was refused; - the feedback filter (correctly) excludes the bot's own comments, so a retry re-read only the original review points; - so `@qwen-code /retry` would have re-run the same agent against the same input and produced the same non-compiling change. The compiler had already said exactly what was wrong. The loop just threw it away. Three pieces carry it instead: - Each deterministic check now runs through `run_check`, which tees its output to a gate log; `reject_fix` writes the label plus the tail of that output to gate-rejection.md. (A four-backtick fence keeps captured ``` output from breaking out when this is posted as a comment.) - The handoff comment carries that block between `<!-- autofix-gate-rejection-start/end -->` markers, so a human sees the real reason next to the summary instead of a report that reads like success. - `Prepare branch and feedback` lifts it back out of the bot's newest comment and puts it at the top of the next round's feedback: "Your previous attempt was REJECTED by the verification gate — fix this first." So a mechanical rejection now closes inside the loop, which is the point of takeover. A rejection the agent cannot fix still burns rounds and ends at the same handoff, bounded exactly as before. Tests: the round trip is exercised end to end — a failing check's compiler output lands in gate-rejection.md with its label, the handoff delimits it, and the prepare step recovers the text (markers stripped) from the newest bot comment while a round that pushed yields nothing to replay. Both halves mutation-verified. QwenLM#7351's verdict test is retargeted to run_check. * fix(autofix): declare the gate verdict before writing its detail file CI caught this and macOS could not: reject_fix wrote gate-rejection.md first and outcome=failed second, so a failure to write the detail took the verdict with it. An empty outcome on a failed job is the signal for "the gate never reached a verdict" — a crash, which is RETRIED — so a clean rejection whose detail write failed would be re-attempted every round instead of being reported once. The verdict is now written first and the detail write is non-fatal. The ordering is pinned by a STATIC assertion, not only the behavioural one: bash 3.2 suspends set -e through a `||`-invoked function and bash 5 does not, so the wrong order runs clean on macOS and aborts on a Linux runner. That is exactly how it shipped green locally and red in CI, and a guard that depends on the reviewer's bash would let it happen again. * fix(autofix): escape the gate-rejection detail for real The gate-rejection publish site used `sed 's/<!--/<!\-\-/g'` — single backslashes, which sed reads as escaped literal `-`, so the replacement is byte-identical to the match and the whole command is a no-op on both GNU and BSD sed. The other four publish sites use `\\-\\-` correctly. That mattered: the detail is `tail -c 3000` of build/typecheck/lint/test output, published verbatim in a bot-authored comment. The scan parses markers by matching the literal `<!-- autofix-eval ts=`, and it only counts markers in bot-authored comments — so any check output containing that string would have been parsed as a real eval marker. The existing test counted the CORRECT spelling and asserted there were four of them. A fifth site with the wrong spelling did not match the counted string, so the count stayed at four and the test stayed green. It now asserts every `s/<!--/…/g` site is byte-identical to the correct form, which fails on exactly this bug. Reported by qwen-code-ci-bot on PR QwenLM#7368. * chore(autofix): correct stale "ALL FOUR" escape-site comment to five (QwenLM#7368) * chore(autofix): document the head/tail byte-limit invariant (QwenLM#7368) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
|
Released in v0.20.1. |
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
5 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
Adds bounded retries for DingTalk acknowledgement and recall reactions when the emotion API returns HTTP 429 or a 5xx response. Each request gets at most three attempts with short exponential delays of 250 ms and 500 ms. Non-transient 4xx responses still fail immediately, and a terminal failure is logged only once.
Why it's needed
DingTalk can occasionally return a transient
system.errHTTP 500 from the emotion endpoint. Previously the adapter made one request and gave up, so users saw no acknowledgement reaction even though message processing continued normally. A short bounded retry recovers from temporary service failures without delaying the main workflow indefinitely.Reviewer Test Plan
How to verify
Confirm that an emotion request receiving
500, 500, 200makes three attempts and completes without an error log. Confirm that a persistent 500 stops after three attempts and emits one sanitized final error. Confirm that a 400 response is attempted once and is not retried. The complete DingTalk adapter unit suite should remain green.Evidence (Before & After)
Before: a focused
500, 500, 200regression test observed one request and failed withexpected 1 to be 3. After: the recovery, retry-bound, and non-transient cases pass, and the complete adapter test file passes 74/74 tests.Tested on
Environment (optional)
Node.js v22.23.0 on Linux x86_64. Package build, package TypeScript check, focused lint, formatting check, and the complete DingTalk adapter unit test file pass locally. Repository-wide build/typecheck could not be used as evidence because the isolated worktree reused a dependency installation from an older checkout, which is missing or mismatches current-main packages such as
mime,shiki, and the newer Ink exports.Risk & Scope
Linked Issues
Fixes #7327
中文说明
本 PR 做了什么
当钉钉确认表情或撤回表情所调用的 emotion API 返回 HTTP 429 或 5xx 时，增加有限重试。每个请求最多尝试三次，采用 250 毫秒和 500 毫秒的短指数退避。非临时性的 4xx 仍立即失败，最终失败只记录一次日志。
为什么需要
钉钉 emotion 接口偶尔会返回临时性的
system.errHTTP 500。此前适配器只请求一次就放弃，因此即使消息处理主流程正常继续，用户也看不到确认表情。短暂且有上限的重试可以从临时服务故障中恢复，同时不会无限拖延主流程。Reviewer 测试计划
如何验证
确认 emotion 请求依次收到
500、500、200时共尝试三次，并且成功后不记录错误日志。确认持续返回 500 时在三次后停止，并只输出一次经过清理的最终错误。确认 400 响应只尝试一次且不会重试。完整的钉钉适配器单元测试应保持通过。证据（修复前与修复后）
修复前：聚焦的
500、500、200回归测试只观察到一次请求，并以expected 1 to be 3失败。修复后：恢复、重试上限和非临时错误用例均通过，完整适配器测试文件 74/74 通过。测试平台
环境（可选）
Linux x86_64，Node.js v22.23.0。本地包构建、包级 TypeScript 检查、聚焦 lint、格式检查和完整钉钉适配器单元测试均通过。仓库级 build/typecheck 未作为验证依据，因为隔离 worktree 复用了较旧检出目录的依赖安装，其中缺失或不匹配当前 main 所需的
mime、shiki和新版 Ink 导出等依赖。风险与范围
关联 Issue
修复 #7327