fix(channels): deliver background agent replies - #7336
Merged
Conversation
Collaborator
Author
|
Verification completed on Linux (Node.js v22.23.0):
|
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. |
qqqys
marked this pull request as ready for review
July 20, 2026 12:41
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with solid evidence. #7334 documents a concrete reproduction (background subagent completes after the parent turn ends → the model's final Direction: aligned. Delivering a background agent's final reply back to the originating Channel chat is squarely within the daemon/channels feature area, which is under active development. It correctly preserves the #7223 isolation (the internal Size: cross-package ( Approach: the scope feels right and the design is the obvious one — it mirrors the existing Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，证据充分。#7334 给出了具体复现（后台 subagent 在父请求结束后才完成 → 模型最终的 方向：对齐。把后台 agent 的最终回复投递回发起会话的 Channel，完全属于 daemon/channels 功能领域，且该领域正在活跃开发。它正确保留了 #7223 的隔离（内部 规模：跨包（ 方案：范围合理，设计也是最自然的一种——完全复刻现有 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Code reviewMy independent take before reading the diff: the bridges discard every The PR does exactly this, and does it cleanly. No correctness blockers, no convention violations. Notes from the review:
sequenceDiagram
participant P1 as Bridge (Acp or Daemon)
participant P2 as ChannelBase listener
participant P3 as Background relay
participant P4 as SessionRouter
participant P5 as Channel adapter
Note over P1: agent_message_chunk, source background_notification_response
P1->>P1: skip if rewritten is true (dedupe)
P1->>P2: backgroundResponse (standalone, no router)
P1->>P3: backgroundResponse (gateway or daemon, router supplied)
P2->>P4: getTarget sessionId
P3->>P4: getTarget sessionId
P4-->>P3: target with matching channelName
P3->>P5: dispatchBackgroundResponse
P5->>P5: proactive push if supported, else sendResponseMessage
Real-scenario testingA live end-to-end run against a real third-party Channel platform (e.g. DingTalk) with a background subagent completing after the parent turn isn't possible in this environment — it needs platform credentials and a live model call. So I verified the behavior the same way the issue itself does: the PR's regression tests encode the exact before/after, and I ran them in tmux at the reviewed head both with the fix and with the two bridge files reverted to After (this PR, head c271ff1): Before (bridges reverted to main, bug present) — tmux capture-pane: The before/after is unambiguous: with the buggy bridges the discrete final response never surfaces ( 中文说明代码审查我在看 diff 之前的独立想法：桥接层会丢弃所有 PR 正是这么做的，而且很干净。没有正确性阻塞项，也没有违反项目约定。审查要点：
（时序图见上方英文部分。） 真实场景测试本环境无法对真实第三方 Channel 平台做端到端实测（需要平台凭据和真实模型调用）。因此用 issue 本身采用的方式验证：PR 的回归测试恰好编码了 before/after，我在 tmux 中于所审提交分别在带修复和把两个桥接文件回退到 before/after 明确：带 bug 的桥接层下离散最终回复无法浮现（ — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. Stepping back: this fixes a real, reproduced bug (#7334) — a background agent's final answer was being silently dropped because the bridges discarded every discrete message. The fix is the obvious correct one and nothing more: special-case the one The newest commit earned its place: I read My independent proposal matched the PR's approach; I didn't find a simpler path it missed. Every change in the diff is load-bearing — no drive-by refactors, no scope creep. The before/after is unambiguous: revert the bridges to 中文说明置信度：5/5 —— 每个阶段都干净，毫不犹豫地合并。 退一步看：这修复了一个真实且可复现的 bug（#7334）——后台 agent 的最终答案因为桥接层丢弃所有离散消息而被静默丢掉。修复正是最自然的正确做法，且不多余：只对关键的那一个 最新一次提交有其价值：我读了 我的独立方案与 PR 一致，没找到它遗漏的更简路径。diff 中每处改动都是必需的——没有顺手重构，没有范围蔓延。before/after 明确：把桥接层回退到 — 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
+430
to
+434
| if (this.supportsProactiveSend() && this.supportsProactiveTarget(target)) { | ||
| await this.pushProactive(target, text); | ||
| return; | ||
| } | ||
| await this.sendResponseMessage(target.chatId, text, sessionId); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The sendResponseMessage fallback path (when supportsProactiveSend() returns false) is not covered by any test. — Concrete cost: if the fallback's arguments were changed incorrectly (e.g. dropping sessionId), no test would detect the regression.
Add a test with proactiveSupported = false that asserts ch.sent contains the expected { chatId, text, sessionId } and ch.proactive remains empty.
— qwen3.7-max via Qwen Code /review
Comment on lines
+312
to
+313
| describe('registerBackgroundResponseRelay', () => { | ||
| it('routes the final background response without joining the active prompt', async () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] registerBackgroundResponseRelay has three guard/error branches (no route → log, no channel → log, dispatch throws → catch+log) but only the happy path is tested. — Failure scenario: if the !target null guard were removed or inverted, target.channelName would throw inside the EventEmitter listener, crashing the relay for all subsequent events. The analogous registerPermissionRelay in the same file has a 'logs before cancelling permission requests with no channel' test covering its error branch.
Add tests for (1) router.getTarget returning undefined — assert stderr contains 'No route for background response'; (2) channel not found in the map — assert stderr contains 'No channel'.
— 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 addressedSuggestion 1: Cover
|
This was referenced Jul 20, 2026
Merged
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.7-max via Qwen Code /review
Comment on lines
+387
to
+388
| expect(backgroundResponses).toEqual([['s-1', 'Background final answer.']]); | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The test asserts backgroundResponse is emitted but does not assert that textChunk is NOT emitted for the same message. — Failure scenario: if the break at line 331 of AcpBridge.ts were removed, the background response text would be emitted as both backgroundResponse (delivered via Channel proactive send) and textChunk (accumulated into the active turn), causing duplicate message delivery. This test would still pass because it only checks the positive assertion.
Suggested change
| expect(backgroundResponses).toEqual([['s-1', 'Background final answer.']]); | |
| }); | |
| expect(backgroundResponses).toEqual([['s-1', 'Background final answer.']]); | |
| // Also verify the message is NOT accumulated into the active turn | |
| expect(textChunks).toEqual([]); | |
| }); |
— qwen3.7-max via Qwen Code /review
Comment on lines
+209
to
+213
| void channel | ||
| .dispatchBackgroundResponse(sessionId, text) | ||
| .catch((err: unknown) => { | ||
| writeStderrLine( | ||
| `[Channel] Background response relay failed for session ${sanitizeLogText(sessionId, 128)}: ${err instanceof Error ? sanitizeLogText(err.message, 512) : sanitizeLogText(String(err), 512)}`, |
Collaborator
There was a problem hiding this comment.
[Suggestion] The .catch() error path is untested — the existing tests mock dispatchBackgroundResponse with mockResolvedValue(undefined), so it never rejects. — Failure scenario: pushProactive or sendResponseMessage can reject on network errors. Without this .catch(), the rejection becomes an unhandled promise rejection that can terminate the daemon. A future refactor could remove the error handling without any test catching the regression.
Suggested change
| void channel | |
| .dispatchBackgroundResponse(sessionId, text) | |
| .catch((err: unknown) => { | |
| writeStderrLine( | |
| `[Channel] Background response relay failed for session ${sanitizeLogText(sessionId, 128)}: ${err instanceof Error ? sanitizeLogText(err.message, 512) : sanitizeLogText(String(err), 512)}`, | |
| void channel | |
| .dispatchBackgroundResponse(sessionId, text) | |
| .catch((err: unknown) => { | |
| writeStderrLine( | |
| `[Channel] Background response relay failed for session ${sanitizeLogText(sessionId, 128)}: ${err instanceof Error ? sanitizeLogText(err.message, 512) : sanitizeLogText(String(err), 512)}`, | |
| ); | |
| }); | |
| // Test: mock dispatchBackgroundResponse to reject, assert stderr contains 'Background response relay failed' |
— qwen3.7-max via Qwen Code /review
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.
— qwen3.7-max via Qwen Code /review
Comment on lines
+323
to
+330
| if (meta?.['qwenDiscreteMessage'] === true) { | ||
| if ( | ||
| meta['source'] === 'background_notification_response' && | ||
| content?.type === 'text' && | ||
| content.text | ||
| ) { | ||
| this.emit('backgroundResponse', sessionId, content.text); | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] When MessageRewriteMiddleware is enabled, the same background_notification_response update is emitted through the bridge twice — once as the original (sent immediately at MessageRewriteMiddleware.ts:89) and once as the rewritten copy (flushed at line 170). The rewritten copy carries the same source: 'background_notification_response' and qwenDiscreteMessage: true metadata (confirmed by MessageRewriteMiddleware.test.ts:245–291 and captureTurnMeta at line 109, since REWRITE_META_EXCLUDED_KEYS is intentionally empty). Both satisfy this guard, so this.emit('backgroundResponse', …) fires twice — the user on the channel receives two messages for one background-task completion. — Failure scenario: enable message-rewrite middleware (production default for most sessions), run a background agent to completion after the parent turn ends → duplicate delivery. Fix: either (a) add && meta['rewritten'] !== true here and in DaemonChannelBridge.ts:656–659, or (b) exclude source === 'background_notification_response' from accumulation in the middleware (like slash_command at line 94), since a discrete background response has no meaningful rewritten form.
— qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /triage |
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 2/5). What changed, and what I pushed back on: Review feedback addressed1. [Suggestion] Add negative assertion for
|
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.
LGTM, looks ready to ship. ✅
Collaborator
✅ Local real-build verification — merge referenceI built and ran this PR from a clean checkout of the head commit (
Reviewer Test Plan — behaviors confirmedSteps 3 & 4 of the plan (the ones that actually matter) were verified directly against the production classes, not just re-running the shipped unit tests:
Independent end-to-end (this is not the author's tests)I wrote a throwaway integration test that drives the whole chain through the real objects — no mocks of the code under test:
It covers both wiring modes: the standalone channel (self-registered bridge events) and the multi-channel gateway (the real
Negative control: with only Notes for the merge decision
Verdict: from a build-and-behavior standpoint this is ready to merge. 👍 中文说明✅ 本地真实构建验证 —— 合并参考我从 PR head 提交（
（截图见英文版顶部。） Reviewer 测试计划 —— 已确认的行为计划中的第 3、4 步（真正关键的部分）我直接针对生产类做了验证，而不是仅仅重跑作者自带的单测：
独立端到端验证（不是作者自带的测试）我写了一个一次性的集成测试，让整条链路都跑在真实对象上——对被测代码不做任何 mock：
它覆盖了两种接线方式：独立频道（自注册 bridge 事件）和多频道网关（daemon/gateway 启动路径实际使用的真实 反向对照： 仅把 合并决策相关说明
结论：从构建与行为的角度看，本 PR 已具备合并条件。 👍 🤖 Verified locally with Claude Code (Opus 4.8). |
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.
— qwen3.7-max via Qwen Code /review
Comment on lines
+422
to
+429
| const target = this.router.getTarget(sessionId); | ||
| if ( | ||
| !target || | ||
| target.channelName !== this.name || | ||
| text.trim().length === 0 | ||
| ) { | ||
| return; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] dispatchBackgroundResponse has three early-return guards (null target, cross-channel name mismatch, empty/whitespace text) — none are tested. — Failure scenario: if the target.channelName !== this.name guard were removed or inverted, every bridge-listening channel would deliver backgroundResponse for sessions routed to a different channel, producing duplicate cross-channel message delivery.
Consider adding tests for each guard branch: (1) session with a different channelName target → no delivery; (2) whitespace-only text → no delivery; (3) missing route → no crash.
— qwen3.7-max via Qwen Code /review
Comment on lines
+211
to
+215
| .catch((err: unknown) => { | ||
| writeStderrLine( | ||
| `[Channel] Background response relay failed for session ${sanitizeLogText(sessionId, 128)}: ${err instanceof Error ? sanitizeLogText(err.message, 512) : sanitizeLogText(String(err), 512)}`, | ||
| ); | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The .catch() error path is untested — the existing tests mock dispatchBackgroundResponse with mockResolvedValue(undefined), so it never rejects. — Failure scenario: pushProactive or sendResponseMessage can reject on network failures; without the .catch() handler, the rejected promise becomes an unhandled rejection, crashing the daemon process.
Consider adding a test where dispatchBackgroundResponse is mocked to reject, then verify stderr contains the expected error log message.
— qwen3.7-max via Qwen Code /review
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: 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 — its prompt was built, but no agent on record was launched with it. Not reviewed: reverse audit — an auditor ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and what the agent was actually asked is not what this skill certifies.
— qwen3.7-max via Qwen Code /review
Comment on lines
+323
to
+333
| if (meta?.['qwenDiscreteMessage'] === true) { | ||
| if ( | ||
| meta['source'] === 'background_notification_response' && | ||
| meta['rewritten'] !== true && | ||
| content?.type === 'text' && | ||
| content.text | ||
| ) { | ||
| this.emit('backgroundResponse', sessionId, content.text); | ||
| } | ||
| break; | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] The 4-condition background response detection predicate (qwenDiscreteMessage + source === 'background_notification_response' + rewritten !== true + text-exists) is copy-pasted between AcpBridge and DaemonChannelBridge. — Failure scenario: when the detection criteria change (e.g., a new exclusion field in _meta), both copies must be updated in lockstep. A change to one bridge without the other silently causes channel-specific behavioral divergence — one bridge delivers background responses that the other drops.
Consider extracting the meta-conditions into a shared predicate (e.g., isBackgroundResponse(meta): boolean). The text-existence check can remain caller-side since it depends on each bridge's content model.
— qwen3.7-max via Qwen Code /review
doudouOUC
reviewed
Jul 20, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Not reviewed: chunk 1 — no agent reported covering these; nobody read them. Not reviewed: every dimension — none of the 10 required agents is on record as launched with a prompt this skill built, so this diff was reviewed, if at all, from prompts the run wrote for itself: no record shows the severity bar, the finding format or this project's own rules reaching an agent. Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
— qwen3.7-max via Qwen Code /review
This was referenced Jul 20, 2026
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
4 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 a dedicated Channel delivery path for model responses generated after a background task notification wakes an idle ACP session. The bridges continue to exclude internal discrete notifications from the completed parent prompt, but emit
background_notification_responsetext through a separate event. Channel runtimes route that event back to the original session target and use proactive delivery when the adapter supports it.The relay is registered for standalone Channel instances, single- and multi-channel gateway startup, gateway restarts, and daemon workers. Route ownership is checked again before delivery, and missing routes, missing channels, and delivery failures are handled without contaminating another prompt.
Why it's needed
ACP already wakes an idle session and asks the model to process a completed background task. However, the Channel bridges currently discard every message marked
qwenDiscreteMessage, including the model's finalbackground_notification_response. Users therefore see an interim acknowledgement and never receive the promised result. This preserves the isolation introduced by #7223 while delivering only the generated follow-up response asynchronously.Reviewer Test Plan
How to verify
agenttask in background mode.background_notificationis not appended to the parent response.background_notification_responseis sent as a new message to the original session target.npm testfrompackages/channels/baseand../../node_modules/.bin/vitest run src/commands/channelfrompackages/cli.npm run typecheckfrom the repository root.Evidence (Before & After)
Before: focused regression tests received the initial response but observed no
backgroundResponseevent after injecting a discretebackground_notification_responseupdate.After: the Channel Base suite passes 883/883 tests, all CLI Channel tests pass 258/258, the repository-wide TypeScript check passes, and an independent read-only test review confirmed standalone, gateway restart, and daemon-worker routing without duplicate listeners.
Tested on
Environment (optional)
Linux 6.8.0-124-generic x86_64, Node.js v22.23.0, clean
npm ciinstall.Risk & Scope
Linked Issues
Fixes #7334
中文说明
本 PR 做了什么
为后台任务通知唤醒空闲 ACP 会话后生成的模型回复增加独立的 Channel 投递路径。桥接层仍会从已结束的父请求中排除内部离散通知，但会通过单独事件发送
background_notification_response文本。Channel 运行时把该事件路由回原会话目标，并在适配器支持时使用主动消息投递。该转发逻辑已接入独立 Channel 实例、单 Channel 与多 Channel 网关启动、网关重启以及 daemon worker。投递前会再次检查路由归属；缺少路由、缺少频道和投递失败均会被安全处理，不会污染其他请求。
为什么需要
ACP 已经能够唤醒空闲会话，并让模型处理已完成的后台任务。但是 Channel 桥接层目前会丢弃所有带
qwenDiscreteMessage标记的消息，其中也包括模型最终生成的background_notification_response。因此用户只能看到中间确认话术，之后收不到承诺的结果。本修复保留 #7223 引入的消息隔离，同时只异步投递模型生成的后续回复。Reviewer 测试计划
如何验证
agent任务。background_notification没有追加到父请求回复中。background_notification_response作为新消息发送到原会话目标。packages/channels/base中运行npm test，并在packages/cli中运行../../node_modules/.bin/vitest run src/commands/channel。npm run typecheck。修复前后证据
修复前：定向回归测试能收到初始回复，但注入离散的
background_notification_response更新后观察不到backgroundResponse事件。修复后：Channel Base 测试套件 883/883 通过，全部 CLI Channel 测试 258/258 通过，全仓 TypeScript 检查通过；独立只读测试审核还确认了 standalone、网关重启和 daemon-worker 路由均生效且没有重复监听。
测试平台
环境（可选）
Linux 6.8.0-124-generic x86_64、Node.js v22.23.0、干净的
npm ci安装。风险与范围
关联 Issue
修复 #7334