feat(autofix): feed the gate's rejection back so the retry can fix what it broke - #7368
Conversation
…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 #7329 and #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.
Merge origin/main into the verification-gate crash retry branch. The conflict was in scripts/tests/qwen-autofix-workflow.test.js: - HEAD added gate-crash retry and reject_fix verdict tests - origin/main added re-arm marker and /retry authorization tests Both sets of tests are independent; kept all of them. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…at it broke #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. #7351's verdict test is retargeted to run_check.
|
Re-running the gate on the current head ( Template looks good ✓ — what/why/how, reviewer test plan, before/after evidence, risk & scope, linked issues, bilingual. Problem: observed, not theoretical. #7208 was handed to a human over a two-character TS4111 fix; the PR quotes the actual compiler output and the round-5/100 handoff. The reason the loop couldn't self-recover is well diagnosed — the rejection lived only in the bot's own handoff comment, which the feedback filter correctly excludes, so a retry re-read the original review points and re-made the same mistake. Direction: aligned. This is squarely on the autofix reliability line (#7330/#7350/#7351/#7355/#7358/#7364) and fixes a concrete "the loop gave up on a mechanical fix" failure. The stacked base #7351 is already merged into main, so the diff here is self-contained. Size: not applicable for core gating — the change touches Approach: the scope feels right. Capture the failing check's output → persist it with the verdict → carry it in the handoff → lift it back into the next round's feedback. That's the minimal path to close the loop, and it reuses the existing Moving on to code review. 🔍 中文说明在当前 head( 模板完整 ✓ —— what/why/how、评审验证计划、before/after 证据、风险与范围、关联 issue、双语齐全。 问题: 已观测,非理论。#7208 因为一个两字符的 TS4111 修复被交给人;PR 引用了真实的编译器输出和 round-5/100 的交接。环路无法自愈的原因诊断准确——拒绝理由只存在于 bot 自己的交接评论里,而反馈过滤器正确地排除了它,于是重试只重读原始评审意见、再犯同样的错。 方向: 对齐。完全在 autofix 可靠性主线上,修的是"环路在一个机械性修复上放弃"这一具体失败。叠置的 base #7351 已合入 main,所以这里的 diff 是自洽的。 规模: 核心门禁不适用——改动只涉及 方案: 范围合理。捕获失败检查的输出 → 连同判决一起持久化 → 带进交接评论 → 在下一轮反馈里取回。这是闭合环路的最小路径,且复用了既有的 进入代码审查。🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code reviewRead the full diff plus the surrounding
Beyond the two blockers, the design holds up:
No critical blockers, no convention violations. Reuse is good — it extends the existing Real-scenario testingThis is a CI-only change ( (The The new round-trip test in isolation: 87/87 green, driving the real extracted bash end to end. 中文说明代码审查读了完整 diff 及
两条 blocker 之外,设计也站得住: 无关键阻塞,无规范违反。复用良好——扩展既有模式而非另造工具。 真实场景测试这是纯 CI 改动,没有可在 tmux 里驱动的 CLI 产品面。相称的真实测试是 workflow 自带的测试框架——它从 YAML 抽出真实的 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; both prior blockers verifiably fixed at the file-byte level and exercised by a green suite; sound, minimal design. Stepping back: this closes a real, well-diagnosed gap — the gate's own compiler output was being thrown away, so a mechanical rejection became a human handoff. My independent take on the fix (capture the failing check's output, persist it with the verdict, carry it in the handoff, lift it back into the next round) is exactly what landed, and nothing extra came along for the ride. The two things that previously broke it are genuinely resolved, not papered over: the verdict is declared before the (non-fatal) detail write, and all five escape sites use the double-backslash sed form that actually works on GNU sed — I confirmed both against the raw file and empirically, and the suite drives the real extracted bash end to end (87/87). The security-sensitive bit — replaying captured build output as model input — is handled the same way the agent's own summary already is: truncated to 3 KB, The residual notes from earlier rounds are all non-blocking and, where declined, declined for the right reason (the 中文说明置信度:5/5 —— 每一阶段都干净;两条此前的 blocker 已在文件字节层面核实修复,并由全绿测试套实际驱动;设计稳妥、最小。 退一步看:这闭合了一个真实且诊断充分的缺口——门自己的编译器输出被丢掉,于是机械性拒绝变成了人工交接。我对修法的独立设想(捕获失败检查输出、连同判决持久化、带进交接、在下一轮取回)与最终落地完全一致,且没有夹带任何多余改动。 此前两处真正的问题是被切实解决、而非糊弄过去的:判决先于(非致命的)详情写入,全部五处转义点都用在 GNU sed 上确实有效的双反斜杠写法——我对照原始文件并实证核验了两者,测试套端到端驱动真实抽出的 bash(87/87)。安全敏感点——把捕获的构建输出作为模型输入回放——处理方式与 agent 自己的摘要一致:截断到 3 KB、转义 前几轮遗留的备注都非阻塞,被拒的也拒得有理( — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
One blocker: the pre-existing test "makes every known gate rejection declare its verdict" now fails because reject_fix writes to ${WORKDIR}/gate-rejection.md but the test doesn't set WORKDIR. The redirect to /gate-rejection.md fails under set -e, so outcome=failed is never written.
Fix: set WORKDIR to the test's temp dir in the bash env (same as the new test already does). Everything else looks good — the approach is clean and the new round-trip test is thorough.
Code reviewScope: the feedback-loop diff on top of The design is right: capture the check output, carry the reason across the handoff, lift it back on the retry. The round-trip is well-tested and the "newest 🔴 Blocking — the
|
Verification pass — adjudicating the two open findingsRan the suite at 1. Confirmed red — but the right fix is in the workflow, not the testThe triage's diagnosis is close, but its proposed fix is insufficient, and I'd argue it patches the wrong side:
Fix (verified): declare the verdict before the capture: reject_fix() {
echo "❌ ${1}"
echo "outcome=failed" >> "${GITHUB_OUTPUT}" # verdict first — unconditional
{
# ...capture...
} > "${WORKDIR}/gate-rejection.md"
exit 1
}With that reordering the suite is 74/74 with zero test edits (re-ran the full file against the patched YAML): the old test passes as-is under both uids, and the real gate path still writes the same 2. Escaping no-op: independently reproducedSame result as the earlier review on GNU sed 4.9: 3. Checked and clean
With the verdict-first reordering plus the 🤖 Reviewed with Claude Code · model |
This PR was stacked on ci/autofix-gate-crash-retry (#7351), which has since merged, so it now targets main directly. That turned the stale in-branch copy of #7351 into conflicts against its reviewed final form. Resolved by keeping this PR's own additions and taking main's version of everything that belongs to #7351 or #7364: - Gate helpers: kept this branch's GATE_LOG capture and run_check, which are what this PR adds; main's plain reject_fix was their predecessor. - Artifact listing: union of gate-rejection.md and resolved-comments.txt. - Handoff comments: took main's wording — it covers both no-verdict causes and is accurate that the agent's commit dies with the runner. - Tests: both sides added an it() at the same anchor. Kept both and supplied the seam terminator; the trailing rmSync in the shared context belongs to main's test, which has no cleanup of its own.
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. |
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.
|
CI was red on
{ … } > "${WORKDIR}/gate-rejection.md"
echo "outcome=failed" >> "${GITHUB_OUTPUT}"An empty Fixed: verdict first, detail write non-fatal. Why it was green on macOS and red on Linux. That is why the new guard is static, not only behavioural: expect(helper.indexOf('outcome=failed')).toBeLessThan(helper.indexOf('gate-rejection.md'));
expect(helper).toMatch(/gate-rejection\.md" \|\|\n/);Both mutation-verified red on macOS (swap the order → red; drop the non-fatal 76/76 locally, three runs (one hit the pre-existing 中文说明CI 在
而在本仓库中,job 失败时 已修:判决先写,详情写入改为非致命。 为什么 macOS 绿、Linux 红。 这正是新保护采用静态断言而不只是行为断言的原因(见上方 JS)。两条都在 macOS 上做了变异验证(调换顺序 → 红;去掉非致命的 本地 76/76,连跑三次(其中一次命中既有的 |
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
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 |
yiliang114
left a comment
There was a problem hiding this comment.
LGTM — well-structured fix for the #7208 blind-retry problem.
What's good:
- Verdict-before-detail ordering in
reject_fixis the right call; the static assertion in the test makes it bash-version-proof. - Four-backtick fence +
<!--escaping +iconv -c+head -c 3500is a solid defense-in-depth chain for arbitrary compiler output. - Round-trip test (write → handoff → extract) covers the full lifecycle.
One minor observation (non-blocking):
GATE_LOG accumulates output from all checks. If build passes with verbose output and typecheck fails with a short error, tail -c 3000 will correctly surface the typecheck error. But if the first check fails with >3000 bytes of output, the rejection detail loses the beginning of that error. In practice compiler errors repeat the key line, so this is fine — just noting the tradeoff.
No blocking issues found.
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution: PR #7368 into mainRoot causePR #7389 ( Textual or semanticTextual only. Both sides appended different filenames to a single for f in feedback.md address-summary.md no-action.md failure.md handoff.md gate-rejection.md agent-api-error agent-api-error-kind resolved-comments.txt pr.diff; doWhat is load-bearingNothing beyond the combined list. Each filename is guarded by What I could not verifyNo build or tests were run (per the merge-conflict resolution scope). The auto-merged file 中文说明合并冲突解决:PR #7368 合入 main根因PR #7389(修复 /resolve 报告被截断)和 PR #7247(重试模型 API 错误)在 main 上为 文本冲突还是语义冲突纯文本冲突。 双方只是在同一行 关键依赖无。每个文件名都有 未能验证的部分按合并冲突解决范围要求,未运行构建或测试。 |
674ac99
yiliang114
left a comment
There was a problem hiding this comment.
Nit (non-blocking): Prepare branch and feedback 里的 LAST_REJECTION="$(jq ...)" 在 set -e 下,如果 ic.json 缺失或损坏会中断整个 feedback 生成块。实际不会发生(ic.json 在前面必定已写入),但加个 || true 更健壮:
LAST_REJECTION="$(jq -r ... | sed -n ... | sed '1d;$d')" || true不影响 approve,纯防御性建议。
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] The existing blocker from @qwen-code-ci-bot (comment 3618484374) at .github/workflows/qwen-autofix.yml:3259 still stands — sed 's/<!--/<!\-\-/g' is a no-op on GNU sed (verified empirically). The pre-existing pattern at line 3244 uses \\-\\- correctly. This is the same finding reported independently by this review. Dropped as duplicate of the existing inline comment.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
|
Triage re-run completed without a new review. The stage comments above were updated with the latest result. View workflow run. |
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 #7368.
|
Confirmed and fixed in The other four publish sites already used the double-backslash form; only the gate-rejection site I added in this PR used the single. It now matches them. It was load-bearing, not cosmetic. The gate-rejection detail is Why the existing test missed it. expect(workflow.split("sed 's/<!--/<!\\\\-\\\\-/g'").length - 1).toBe(4);That counts how many sites are right; it says nothing about whether any are wrong. My fifth site did not match the counted string, so the count stayed at 4 and the test stayed green. It now enumerates every const escapeSites = workflow.match(/sed 's\/<!--\/[^']*\/g'/g) ?? [];
expect(escapeSites).toHaveLength(5);
for (const site of escapeSites) expect(site).toBe("sed 's/<!--/<!\\\\-\\\\-/g'");Mutation-verified: reverting that one line to Generalisable lesson I'm taking from this: a test that counts correct occurrences is not a test that forbids incorrect ones. Same shape as an allowlist with no deny rule. 中文说明已确认并在 另外四处发布点本就使用双反斜杠写法,只有本 PR 新增的门拒绝这一处用了单反斜杠,现已与其余一致。 这不是外观问题,是承重的。 门拒绝详情是 build/typecheck/lint/test 输出的 既有测试为何漏掉。 原断言统计的是"正确写法有几处"(等于 4),对"有没有错误写法"只字未提。我新增的第五处不匹配被统计的字符串,于是计数仍为 4、测试照样通过。现改为枚举全部 变异验证:把那一行改回 我从中提炼的通用教训:"统计正确出现次数"的测试,不等于"禁止错误出现"的测试 —— 形同只有白名单、没有拒绝规则。 |
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
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: Autofix review feedback addressedTriaged every feedback point on PR #7368. One suggestion needed a code change this round; the two Critical/ Feedback points and decisions1.
2.
3.
4.
Conflict notes
Verification results
中文说明已处理的 Autofix 评审反馈对 PR #7368 上的每条反馈做了分类处理。本轮有一项建议需要改动代码;两条 Critical/ 各反馈点与处理决定1.
2.
3.
4.
冲突说明
验证结果
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/模型 |
Code review — feed the gate's rejection back into the retryRead the full diff plus the surrounding What it does
Correctness — verified end to end
Minor observations (no change required)
Tests — strongThe new coverage drives the real extracted shell ( SecurityThe replayed text is model input, but it originates from the repo's own build/test tooling on a commit the agent itself wrote, is truncated to 3 KB, and has its comment-opening tokens escaped on the way into the PR comment — identical handling to the agent's own summary that already flows to the model. Bounded and reasonable. VerdictApprove. Correct, narrowly scoped, and the failure mode it removes is concrete. The residual notes above are all non-blocking. 🧠 Reviewed by Claude Code · model |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: Review feedback addressedReview from @wenshao: Approve — three minor observations, all explicitly non-blocking. 1.
|
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, ship it.
Verified: the byte-limit invariant holds (3500 head ≥ 3000 tail + label + fences), the sed escape is correct post-15270b5, and the verdict-before-detail static assertion is the right primary guard given bash 3.2/5 divergence on set -e through ||-invoked functions.
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
|
Released in v0.20.1. |
What this PR does
Carries the verification gate's rejection reason back into the loop, so a mechanical rejection gets fixed automatically instead of being handed to a human.
Why it's needed
#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:
…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:
### 1. Implemented … ### 2. Implemented), so neither a human nor the next round could see why it was refused;@qwen-code /retrywould 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.
How
run_checktees each check's output to a gate log;reject_fixwrites label + tail togate-rejection.md<!-- autofix-gate-rejection-start/end -->Prepare branch and feedbacklifts it back out of the bot's newest commentA four-backtick fence keeps captured ``` output from breaking out of the block when it is posted as a comment.
So a mechanical rejection now closes inside the loop — which is the point of takeover. A rejection the agent genuinely cannot fix still burns rounds and ends at the same handoff, bounded exactly as before.
Reviewer Test Plan
How to verify
npx vitest run scripts/tests/qwen-autofix-workflow.test.js— 74/74. The round trip is exercised end to end:error TS4111) lands ingate-rejection.mdwith its label,outcome=failedis declared, and the fence is four backticks;js-yamlparses; all 37run:blocks passbash -n; actionlint 1.7.12 clean; prettier clean.@qwen-code /retryon fix(core): validate goal judge terminal evidence #7208 — the next round's feedback should open with the TS4111 text, and the fix is['truncated']/['preview'].Evidence (Before & After)
Tested on
Risk & Scope
Linked Issues
Raised directly by #7208's TS4111 handoff. Stacked on #7351; part of the autofix reliability line: #7330, #7350, #7351, #7355, #7358, #7364.
中文说明
本 PR 做了什么
把验证门的拒绝理由带回环路,让机械性的拒绝能自动修好,而不是交给人。
为什么需要
#7208 因为一个两字符的修复被交给了人。 agent 实现了两条评审意见,门因为编译不过而拒绝:
环路就此停下 —— round 5/100,"A human should take over this PR"。
环路无法自行恢复,因为理由从未被带到任何环路读得到的地方:
### 1. Implemented … ### 2. Implemented),人和下一轮都看不到为什么被拒;@qwen-code /retry,也只是让同一个 agent 面对同样的输入,再写出同样编译不过的改动。编译器早就把问题说清楚了,环路把它扔掉了。
怎么做
run_check把每个检查的输出 tee 到 gate log;reject_fix把标签 + 输出尾部写入gate-rejection.md<!-- autofix-gate-rejection-start/end -->包住它Prepare branch and feedback从 bot 最新评论里把它取回外层用 4 个反引号围栏,保证被捕获的 ``` 输出不会破格。
这样机械性拒绝就在环路内部闭合 —— 这正是托管的意义。agent 确实修不了的拒绝仍会烧轮次、走到同样的 handoff,边界与此前完全一致。
评审验证
npx vitest run scripts/tests/qwen-autofix-workflow.test.js—— 74/74,端到端往返:失败检查的真实编译输出(error TS4111)带标签落入gate-rejection.md、声明outcome=failed、围栏为 4 反引号;handoff 正确包裹;prepare 步骤能从 bot 最新评论取回原文(标记已剥离),而"已推送"的一轮取回为空。run:块过bash -n;actionlint 1.7.12 clean;prettier clean。@qwen-code /retry,下一轮反馈应以 TS4111 开头,而修法就是['truncated']/['preview']。风险与范围
关联 Issue
由 #7208 的 TS4111 handoff 直接引出。叠在 #7351 之上;属 autofix 可靠性主线:#7330、#7350、#7351、#7355、#7358、#7364。