fix(review): make agent launches and cleanup resilient - #7259
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with evidence — a real Direction: aligned. Making Size: core paths touched ( Approach: scope feels right. Three focused fixes that each address a distinct failure mode: (1) accept redundant Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，有证据——一次真实 方向：对齐。让 规模：触及核心路径（ 方案：范围合理。三个聚焦修复分别对应不同失败模式：(1) 当 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: given the problem (strict providers send both Findings: no critical blockers. The The lease service ( The The Real-Scenario TestingBuilt the PR ( Lease created on fetch, cleared on cleanup; worktree and branch removed. ✓ Unit tests: all 646 tests pass across the 4 changed test files (agent 170, scheduler 299, lease+cleanup 20, useGeminiStream 157). 中文说明代码审查独立方案： 给定问题（strict provider 同时发送 发现： 无关键阻塞。
lease 服务具有完善的安全防护：
真实场景测试构建 PR 后在 tmux 中驱动 单元测试： 4 个变更测试文件共 646 个测试全部通过。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 — solid fix for a real observed failure; clean implementation with comprehensive tests and proper safety rails on the lease service. This PR does three things well: it makes the Agent tool tolerant of strict providers that send redundant schema fields (the immediate cause of the The implementation matches what I'd propose independently — no simpler path was missed. The lease service is the largest new addition but it earns its complexity: file-based leases survive process crashes that would skip Only nit: the 中文说明置信度：4/5 — 对真实观测失败的扎实修复；实现干净，测试全面，lease 服务有完善的安全防护。 此 PR 做好了三件事：让 Agent 工具容忍 strict provider 发送冗余 schema 字段（ 实现与我的独立方案一致——没有遗漏更简路径。lease 服务是最大的新增，但其复杂度物有所值：基于文件的 lease 能在进程崩溃（跳过 唯一小问题： — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed diff-only — the PR’s existing discussion could not be fetched, so this is not an approval and not a no-blockers claim. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: issue-fidelity — pr-context failed (HTTP 503), Agent 0 skipped. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: Agent 0: Issue fidelity & root-cause ownership — its prompt was built, but no agent on record was launched with it.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
🤝 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 冲突，直到移除标签或达到轮次上限。移除 |
|
@qwen-code /resolve |
Resolve conflict in coreToolScheduler.ts by combining PR's recordBatchRetryableToolError deduplication helper with main's plan-mode entry boundary skip logic and indexed loop iteration.
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #7259ConflictFile: A single conflict region around the tool-processing loop in PR branch (HEAD) added:
main added:
ResolutionCombined both sides:
No other files were modified — all other files auto-merged cleanly. Commit |
…' into fix/review-agent-launch-cleanup
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: chunk 3, chunk 2, chunk 1, chunk 4 — launched with a prompt that is not the one the CLI built. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Test coverage matrix (whole-diff), 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 — 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. Not reviewed: verification — the review posts findings, but no verifier was launched with a prompt this skill builds — they were ruled on, if at all, without the verdict bar its brief carries.
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: Review feedback addressedCommit: Feedback points
Verification
ConflictNo conflict ( 中文说明已处理的审查反馈提交： 反馈要点
验证结果
冲突无冲突（ 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/模型 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/29743342886)._ |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: No action needed — all suggestions already addressedAll three suggestions from this review round are already covered by the current HEAD ( 1. Success-path test for
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. 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.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
LGTM. 增量 review 无 Critical 缺陷。
核心确认：
- Agent worktree 兼容：
createInvocation中isolation: undefined覆盖复用 caller-owned worktree，不修改原始 params - Lease 机制：多层验证（
validTarget防路径穿越、branch 匹配reviewBranch、worktree 必须在REVIEW_TMP_DIR下、sessionId+promptId匹配防子 prompt 释放父资源）。Fallback 路径rmSync+git worktree prune处理git worktree remove失败 - Lease 生命周期：
fetch-pr在 worktree 创建前注册 lease；TUI UserCancelled/loopDetected/catch 设 cleanup flag + finally 释放；headlessregisterCleanupfatal exit + finally 显式调用 + unregister 防重复 - 验证错误去重：
recordBatchRetryableToolError使用 batch-scoped Map，同一 response 中相同错误只计 1 次，batch map 不跨 turn 泄漏 - 小修复：
finalizeAssistMessage移入 try 块、UserCancelledbreak→return干净退出
实现质量高，防御性编程到位。
— qwen3.7-plus via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: No action needed — all feedback already addressedEvery feedback point on PR #7259 was verified against the current HEAD (
No commits were made in this round. 中文说明无需操作 — 所有反馈已在先前提交中解决PR #7259 上的每条反馈均已对照当前 HEAD（
本轮未产生任何提交。 Base-conflict check: no conflict with main. 🧠 Handled by Qwen Code · model/模型 |
Review 总结变更概述修复 代码质量评估Agent worktree 兼容（
Review worktree lease 机制（
Lease 生命周期集成：
验证错误去重（
小修复：
结论无 Critical 缺陷，防御性编程到位，多层验证确保资源安全清理。可以合并。👍 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Nice work — the security model here is solid (path traversal via relative() checks, branch name validation against qwen-review/pr-<N>, execFileSync with argument arrays throughout), and the test coverage is comprehensive.
Three non-blocking observations:
-
useGeminiStream.ts~L2966: the cleanup path usesprompt_id!non-null assertion. Ifprompt_idis undefined, the lease cleanup silently skips — which is a bit ironic for code whose purpose is preventing silent skips. A defensive guard (if (!prompt_id) return) would make the intent explicit. -
fetch-pr.ts~L145: the lease is intentionally created before the worktree exists. The cleanup handles this correctly (traced throughremoveLeaseWorktree), but a one-line comment documenting the ordering would save future readers the same trace. -
coreToolScheduler.tsL2120–2124: the batch dedup's duplicate branch deletes all retry counts for a tool, not just the repeated key. In a mixed batch this can lose counts for non-repeated errors. Consistent with the existing design and covered by tests, but worth a comment for maintainability.
Nothing blocking merge.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: No action neededThe review from @ytahdn is an approval with a detailed summary of the changes. It identifies no Critical defects, no inline comments, and no failed CI checks. The reviewer explicitly concludes the PR can be merged. No code changes are required. Base-conflict check: no conflict with main. 🧠 Handled by Qwen Code · model/模型 |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
…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>
|
Released in v0.20.1. |
|
[API Error: Connection error. (cause: fetch failed)] ❌ failed |
6 participants
What this PR does
This PR makes
/reviewresilient when a provider supplies both a caller-ownedworking_dirand the redundantisolation: "worktree"field. The Agent tool now reuses the validated caller-owned worktree, identical validation failures in one model response count as one retry, and review worktrees are protected by prompt-scoped leases that are released on normal completion, TUI failures, loop termination, and headless fatal cancellation. It also tightens Issue Fidelity guidance so references to motivating incidents are not misclassified as target issues.Why it's needed
A real
/reviewsession successfully prepared its PR context and roster but every review agent failed because the model sent both workspace fields. Three identical validation failures in the same response exhausted the retry budget, skipped the remaining agents, and left the review worktree and branch behind. Prompt-only prohibitions are not sufficient for strict providers, and the cancellation path exits through the global cleanup chain before a function-levelfinallycan reliably protect these resources.Reviewer Test Plan
How to verify
Run the focused Agent and scheduler tests and confirm that a real caller-owned worktree is reused when both fields are supplied, no additional isolation worktree is created, and repeated validation errors are counted once per model response. Run the focused CLI review and cleanup tests and confirm that leases are prompt-scoped, child prompts cannot release parent resources, explicit cleanup disarms the finalizer, and a lease registered before worktree creation still protects the completed worktree. For end-to-end verification, run
/reviewwith the repository's executable CLI entry, interrupt the active prompt with oneSIGINTafter the review worktree is created, and confirm that the lease, worktree, and temporary branch are all removed before exit.Evidence (Before & After)
Before: a strict-provider Agent launch with
working_dirplusisolation: "worktree"failed validation; same-response failures exhausted the retry budget; abnormal review termination could leave.qwen/tmp/review-pr-*andqwen-review/pr-*behind.After: the caller-owned worktree is reused without creating another worktree, one correction turn remains available, and a real
SIGINTE2E ended withprocess=no lease=no worktree=no branch=no.Tested on
Environment (optional)
Node.js 22+, local production bundle built with
npm run build && npm run bundle; real-model headless E2E usedscripts/cli-entry.jsso nested review commands executed the same build.Risk & Scope
Linked Issues
Related to #7235; this adds runtime resilience beyond the prompt-level prohibition.
中文说明
此 PR 做了什么
此 PR 提升
/review在 provider 同时提供 caller-ownedworking_dir与冗余isolation: "worktree"时的韧性。Agent 工具现在会复用已校验的 caller-owned worktree；同一个模型响应中的相同参数校验错误只计作一次重试；review worktree 由 prompt-scoped lease 保护，并会在正常完成、TUI 失败、loop 终止和 headless 致命取消时释放。同时收紧 Issue Fidelity 指引，避免将用于说明动机的历史事件引用误判为目标 issue。为什么需要
一次真实
/review会话成功准备了 PR 上下文和 roster，但模型同时发送两个 workspace 字段，导致所有 review agent 失败。同一响应中的三个相同校验错误耗尽了重试预算，后续 agent 被跳过，并遗留 review worktree 与分支。仅依靠 prompt 禁止不足以约束 strict provider，而且取消路径会通过全局 cleanup 链退出，不能只依赖函数级finally保护这些资源。Reviewer Test Plan
如何验证
运行 Agent 与 scheduler 定向测试，确认两个字段同时提供时会复用真实 caller-owned worktree，不创建额外 isolation worktree，并且重复校验错误按每个模型响应只计一次。运行 CLI review 与 cleanup 定向测试，确认 lease 按 prompt 隔离、child prompt 无法释放 parent 资源、显式 cleanup 会解除 finalizer，并且在 worktree 创建前登记的 lease 仍能保护随后创建完成的 worktree。端到端验证时，使用仓库的可执行 CLI 入口运行
/review，在 review worktree 创建后向活跃 prompt 发送一次SIGINT，确认退出前 lease、worktree 和临时分支均已删除。证据（之前与之后）
之前：strict provider 发送
working_dir与isolation: "worktree"时 Agent 启动校验失败；同一响应中的失败会耗尽重试预算；review 异常终止可能遗留.qwen/tmp/review-pr-*与qwen-review/pr-*。之后：caller-owned worktree 被直接复用且不会创建额外 worktree，模型仍有一次纠错机会，真实
SIGINTE2E 最终状态为process=no lease=no worktree=no branch=no。测试平台
环境（可选）
Node.js 22+，通过
npm run build && npm run bundle构建本地生产 bundle；真实模型 headless E2E 使用scripts/cli-entry.js，确保嵌套 review 命令执行同一份构建。风险与范围
关联 Issue
与 #7235 相关；本 PR 在 prompt 级禁止之外增加 runtime 韧性。