fix(plan-gate): isolate gate agent AbortSignal from parent signal chain#5185
Conversation
The Plan Approval Gate's review agent inherited the parent round's AbortController signal. When transient parent-side issues occurred (stream NO_FINISH_REASON retries, round cleanup), the abort cascaded into the gate agent, causing it to terminate with mode=CANCELLED. The runAgentWithRetry loop then saw signal.aborted=true and immediately skipped all remaining retry attempts, leaving the user stuck in plan mode with a repeated 'Gate review agent unavailable' message. Changes: - runGateAgent: create an independent AbortController with its own timeout (5 min) instead of inheriting the parent signal. This isolates the gate agent from transient parent-side aborts. - runAgentWithRetry: add a 1s delay between retries to allow transient issues to settle. - Improved log messages to distinguish attempt numbers in retry loop. Fixes: exit_plan_mode getting stuck when NO_FINISH_REASON stream errors cascade through the abort signal chain.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adjusts the Plan Approval Gate agent’s cancellation/timeout behavior to reduce failures caused by parent-signal aborts and adds a small delay between retries.
Changes:
- Add early “parent aborted” skip logic and a 1s delay between gate-agent retries.
- Isolate the gate agent from the parent
AbortSignalby using a dedicatedAbortControllerwith a timeout. - Update logging and inline documentation around abort/timeout behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/core/src/plan-gate/planApprovalGate.ts | Adds parent-abort pre-check logging and a fixed delay between retry attempts. |
| packages/core/src/plan-gate/gateReviewAgents.ts | Introduces an isolated abort controller + timeout for the gate agent and updates related docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Signal isolation: The gate agent creates its own independent AbortController | ||
| * with a timeout, rather than directly inheriting the parent's signal. This | ||
| * prevents transient parent-side issues (stream errors, round cleanup) from | ||
| * cascading into the gate agent. The parent signal is only monitored for | ||
| * genuine user-initiated cancellations. | ||
| * |
There was a problem hiding this comment.
- Fix docstring: parent signal is pre-checked only, not monitored
during execution. Matches the actual isolation behavior. - Remove signal.aborted check from retry loop. The gate agent is now
fully signal-isolated with its own timeout, so parent-side aborts
(typically transient) should not short-circuit retries. The caller
discards the result if the parent is truly gone. - Replace plain setTimeout delay with abortableSleep() that returns
early if the parent signal aborts during the 1s wait. Prevents
wasted shutdown time on genuine cancellation. - Fix timeout comment: gate agent typically completes in 1-2 minutes;
the 5-minute ceiling matches runConfig.max_time_minutes.
| // Fast-fail if the parent context is already cancelled before we start | ||
| if (parentSignal.aborted) { | ||
| throw new Error('Parent signal already aborted before gate agent start'); | ||
| } |
There was a problem hiding this comment.
- Fix docstring: parent signal is pre-checked only, not monitored
during execution. Matches the actual isolation behavior. - Remove signal.aborted check from retry loop. The gate agent is now
fully signal-isolated with its own timeout, so parent-side aborts
(typically transient) should not short-circuit retries. The caller
discards the result if the parent is truly gone. - Replace plain setTimeout delay with abortableSleep() that returns
early if the parent signal aborts during the 1s wait. Prevents
wasted shutdown time on genuine cancellation. - Fix timeout comment: gate agent typically completes in 1-2 minutes;
the 5-minute ceiling matches runConfig.max_time_minutes.
| // Check if the parent signal is already aborted before starting. | ||
| // Note: This check is for genuine user-initiated cancellations. | ||
| // Transient parent-side aborts (e.g., stream errors) should not | ||
| // prevent retries since runGateAgent now isolates the gate agent | ||
| // from parent signal propagation. | ||
| if (signal.aborted) { | ||
| debugLogger.warn('Gate agent skipped: signal already aborted'); | ||
| debugLogger.warn( | ||
| `Gate agent skipped on attempt ${attempt}: parent signal already aborted`, | ||
| ); | ||
| // If the signal is aborted, don't retry — the user/session is gone | ||
| return null; | ||
| } |
There was a problem hiding this comment.
- Fix docstring: parent signal is pre-checked only, not monitored
during execution. Matches the actual isolation behavior. - Remove signal.aborted check from retry loop. The gate agent is now
fully signal-isolated with its own timeout, so parent-side aborts
(typically transient) should not short-circuit retries. The caller
discards the result if the parent is truly gone. - Replace plain setTimeout delay with abortableSleep() that returns
early if the parent signal aborts during the 1s wait. Prevents
wasted shutdown time on genuine cancellation. - Fix timeout comment: gate agent typically completes in 1-2 minutes;
the 5-minute ceiling matches runConfig.max_time_minutes.
| // Add a small delay between retries to allow transient issues to settle | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); |
There was a problem hiding this comment.
- Fix docstring: parent signal is pre-checked only, not monitored
during execution. Matches the actual isolation behavior. - Remove signal.aborted check from retry loop. The gate agent is now
fully signal-isolated with its own timeout, so parent-side aborts
(typically transient) should not short-circuit retries. The caller
discards the result if the parent is truly gone. - Replace plain setTimeout delay with abortableSleep() that returns
early if the parent signal aborts during the 1s wait. Prevents
wasted shutdown time on genuine cancellation. - Fix timeout comment: gate agent typically completes in 1-2 minutes;
the 5-minute ceiling matches runConfig.max_time_minutes.
| /** | ||
| * Timeout for the gate agent in milliseconds. The gate agent should complete | ||
| * within a reasonable time (typically 1-2 minutes for a plan review). | ||
| */ | ||
| const GATE_AGENT_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes, matching runConfig.max_time_minutes |
There was a problem hiding this comment.
- Fix docstring: parent signal is pre-checked only, not monitored
during execution. Matches the actual isolation behavior. - Remove signal.aborted check from retry loop. The gate agent is now
fully signal-isolated with its own timeout, so parent-side aborts
(typically transient) should not short-circuit retries. The caller
discards the result if the parent is truly gone. - Replace plain setTimeout delay with abortableSleep() that returns
early if the parent signal aborts during the 1s wait. Prevents
wasted shutdown time on genuine cancellation. - Fix timeout comment: gate agent typically completes in 1-2 minutes;
the 5-minute ceiling matches runConfig.max_time_minutes.
- Fix docstring: parent signal is pre-checked only, not monitored during execution. Matches the actual isolation behavior. - Remove signal.aborted check from retry loop. The gate agent is now fully signal-isolated with its own timeout, so parent-side aborts (typically transient) should not short-circuit retries. The caller discards the result if the parent is truly gone. - Replace plain setTimeout delay with abortableSleep() that returns early if the parent signal aborts during the 1s wait. Prevents wasted shutdown time on genuine cancellation. - Fix timeout comment: gate agent typically completes in 1-2 minutes; the 5-minute ceiling matches runConfig.max_time_minutes.
| parentSignal: AbortSignal, | ||
| ): Promise<GateAgentResult> { | ||
| // Fast-fail if the parent context is already cancelled before we start | ||
| if (parentSignal.aborted) { | ||
| throw new Error('Parent signal already aborted before gate agent start'); | ||
| } |
| function abortableSleep(ms: number, signal: AbortSignal): Promise<void> { | ||
| if (signal.aborted) return Promise.resolve(); | ||
| return new Promise<void>((resolve) => { | ||
| const onAbort = () => { | ||
| clearTimeout(timer); | ||
| resolve(); | ||
| }; | ||
| const timer = setTimeout(() => { | ||
| signal.removeEventListener('abort', onAbort); | ||
| resolve(); | ||
| }, ms); | ||
| signal.addEventListener('abort', onAbort, { once: true }); | ||
| }); | ||
| } |
- Remove parentSignal.aborted pre-check from runGateAgent. The hard-fail conflicted with the isolation goal: a transient parent abort would throw, abortableSleep would resolve immediately, and retries would spin in a rapid loop. The gate agent's own 5-minute timeout is the sole cancellation mechanism; the parent signal is intentionally unused. - Fix race window in abortableSleep: attach the abort listener first, then re-check signal.aborted to handle the case where abort fires between the initial check and listener registration. Eliminates the edge case where the sleep would wait the full duration despite an already-aborted signal.
| // Abort-aware delay: wait 1s between retries, but bail out early | ||
| // if the parent signal is aborted during the wait. | ||
| await abortableSleep(1000, signal); |
Replace custom abortableSleep with the existing delay() utility from utils/retry.ts, which was already battle-tested for exactly this pattern: - delay() REJECTS (not resolves) when the signal is aborted, so the retry loop can catch the rejection and cleanly exit — no rapid-fire retry spin after cancellation. - delay() already handles the TOCTOU race window (attach listener first, re-check signal.aborted after) — addresses the second review comment. - Also exports delay() from retry.ts since planApprovalGate needs it. This eliminates the risk of abortableSleep resolving early during abort and immediately proceeding to another gate agent attempt, which would cause rapid successive retries and log spam.
| try { | ||
| await delay(1000, signal); | ||
| } catch { | ||
| // Signal aborted during delay — stop retrying | ||
| debugLogger.warn( | ||
| `Gate agent retry loop cancelled during backoff delay (attempt ${attempt}/${MAX_AGENT_RETRIES})`, | ||
| ); | ||
| return null; | ||
| } |
| for (let attempt = 1; attempt <= MAX_AGENT_RETRIES; attempt++) { | ||
| if (signal.aborted) { | ||
| debugLogger.warn('Gate agent skipped: signal already aborted'); | ||
| return null; | ||
| } | ||
| // Note: We deliberately do NOT check signal.aborted here. | ||
| // The gate agent is signal-isolated (see runGateAgent), so parent-side | ||
| // aborts are typically transient (stream errors, round cleanup). The | ||
| // gate agent has its own 5-minute timeout as the safety net. If the | ||
| // parent is truly gone, the caller will discard the result anyway. |
| * Timeout for the gate agent in milliseconds. The gate agent typically | ||
| * completes within 1–2 minutes; this 5-minute ceiling accounts for the | ||
| * runConfig.max_time_minutes limit and provides a safety net against hangs. | ||
| */ | ||
| const GATE_AGENT_TIMEOUT_MS = 5 * 60 * 1000; |
- Add entry-time signal.aborted check to avoid launching gate agent on obvious cancellations - Only delay between retries when attempt < MAX_AGENT_RETRIES (not after final failure) - Clarify timeout comment: 5-min ceiling is fixed, coincides with runConfig but independent
| for (let attempt = 1; attempt <= MAX_AGENT_RETRIES; attempt++) { | ||
| if (signal.aborted) { | ||
| debugLogger.warn('Gate agent skipped: signal already aborted'); | ||
| return null; | ||
| } | ||
| try { | ||
| return await runGateAgent(config, bundle, signal); | ||
| } catch (error) { |
| try { | ||
| await delay(1000, signal); | ||
| } catch { | ||
| // Signal aborted during delay — stop retrying | ||
| debugLogger.warn( | ||
| `Gate agent retry loop cancelled during backoff delay (attempt ${attempt}/${MAX_AGENT_RETRIES})`, | ||
| ); | ||
| return null; | ||
| } |
| const gateAbortController = new AbortController(); | ||
|
|
||
| // Add a timeout so the gate agent doesn't hang indefinitely | ||
| const timeoutId = setTimeout(() => { | ||
| debugLogger.warn( | ||
| `[runGateAgent] Gate agent timed out after ${GATE_AGENT_TIMEOUT_MS}ms`, | ||
| ); | ||
| gateAbortController.abort(new Error('Gate agent timeout')); | ||
| }, GATE_AGENT_TIMEOUT_MS); |
| // Pass the isolated gate signal instead of the parent signal | ||
| await subagent.execute(contextState, gateAbortController.signal); |
|
Thanks for the thorough review. I'd like to address these comments, but I want to flag up front that the four comments contain contradictory suggestions that would undo the bug fix if applied together. I'll walk through each one. The contradictionThe comments fall into two groups: Group A ( Group B ( If we do both A and B, the system behaves exactly like the pre-fix code — the very bug this PR is fixing. Here's why:
This is the exact failure sequence from the bug report. The debug logs in the PR description show this happening repeatedly. The two groups of comments are pulling in opposite directions because they're each independently correct in isolation, but together they recreate the cascading-abort failure mode. The design rationaleWhy signal isolation is intentional (Group B)The gate agent uses an independent // agent.ts line 2205
const bgAbortController = new AbortController();
// ...
await bgSubagent.execute(contextState, bgAbortController.signal);The comment there says: "Create an independent AbortController — background agents survive ESC cancellation of the parent's current turn." The gate agent has the same requirement: it must survive transient parent-side aborts. The difference is only the reason for the abort, but
Without a way to distinguish these, we have to choose: either isolate (resilient to transient aborts) or propagate (responsive to explicit cancellation). We cannot do both with a bare Why per-iteration abort checks are intentionally absent (Group A)The retry loop deliberately does not check If we check per iteration, a transient parent abort during or between attempts immediately terminates the retry loop — even though the gate agent itself is isolated and would have completed successfully on a retry. The entry-time check (lines 148–154) catches obvious pre-existing cancellations without interfering with retries: // Entry-time check only — not per-iteration
if (signal.aborted) {
debugLogger.warn('Gate agent skipped: parent signal already aborted at entry');
return null;
}
for (let attempt = 1; attempt <= MAX_AGENT_RETRIES; attempt++) {
// No signal.aborted check here — intentionally
try {
return await runGateAgent(config, bundle, signal);
} catch (error) {
// ...
try { await delay(1000, signal); } catch { return null; }
}
}The The 5-minute wait tradeoffThe Group B comment raises a valid concern: if the parent is genuinely cancelled (user closes the session), the gate agent will continue running for up to 5 minutes. This is an acknowledged tradeoff. In practice:
If this tradeoff proves unacceptable in production, the correct fix is not to propagate the parent signal (which reintroduces the bug) but to add abort reason semantics — either via Summary
The current implementation deliberately trades responsiveness-to-cancellation for resilience-to-transient-aborts. This is the same tradeoff the codebase already makes for background agents. I believe this is the correct choice for now. |
| // Entry-time check: if the parent signal is already aborted before we start, | ||
| // respect it to avoid launching a 5-minute gate agent for an obvious cancellation. | ||
| // This is the ONLY place we check signal.aborted — during execution, the gate | ||
| // agent is signal-isolated (see runGateAgent) to prevent transient parent-side | ||
| // aborts from cascading. |
There was a problem hiding this comment.
Good catch, fixed in b1baf06. Reworded to clarify this is the only synchronous pre-run check; delay() still provides abort-awareness between retries.
- Clarify entry-time comment: it's the only synchronous pre-run check; delay() still provides abort-awareness between retries. - Guard gateAbortController.abort() in finally: only abort if not already aborted, avoiding unnecessary abort events on success.
| for (let attempt = 1; attempt <= MAX_AGENT_RETRIES; attempt++) { | ||
| if (signal.aborted) { | ||
| debugLogger.warn('Gate agent skipped: signal already aborted'); | ||
| return null; | ||
| } | ||
| try { | ||
| return await runGateAgent(config, bundle, signal); | ||
| } catch (error) { |
| // Abort-aware delay: wait 1s between retries (not after the final attempt). | ||
| // Uses the existing `delay()` from utils/retry.ts, which rejects when the | ||
| // signal is aborted. A cancellation during the wait breaks the loop | ||
| // immediately rather than proceeding to another rapid-fire attempt. | ||
| try { | ||
| await delay(1000, signal); | ||
| } catch { | ||
| // Signal aborted during delay — stop retrying | ||
| debugLogger.warn( | ||
| `Gate agent retry loop cancelled during backoff delay (attempt ${attempt}/${MAX_AGENT_RETRIES})`, | ||
| ); | ||
| return null; | ||
| } | ||
| } | ||
| } |
| export async function runGateAgent( | ||
| config: Config, | ||
| bundle: EvidenceBundle, | ||
| signal: AbortSignal, | ||
| // parentSignal is accepted for API consistency but intentionally unused: | ||
| // the gate agent is fully isolated from parent-side aborts. | ||
| _parentSignal: AbortSignal, | ||
| ): Promise<GateAgentResult> { |
| const gateAbortController = new AbortController(); | ||
|
|
||
| // Add a timeout so the gate agent doesn't hang indefinitely | ||
| const timeoutId = setTimeout(() => { | ||
| debugLogger.warn( | ||
| `[runGateAgent] Gate agent timed out after ${GATE_AGENT_TIMEOUT_MS}ms`, | ||
| ); | ||
| gateAbortController.abort(new Error('Gate agent timeout')); | ||
| }, GATE_AGENT_TIMEOUT_MS); |
| // Pass the isolated gate signal instead of the parent signal | ||
| await subagent.execute(contextState, gateAbortController.signal); |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Suggestion] Missing test coverage for new behavior introduced by this PR:
- (a) Abort during retry backoff delay (
delay(1000, signal)catch block) — the scenario the PR is explicitly designed to handle - (b) Parent signal isolation — no test verifies the gate agent continues running when parent signal aborts
- (c) Pre-aborted signal test doesn't assert
runGateAgentwas never called (the key optimization is unverified — addexpect(mockRunGateAgent).not.toHaveBeenCalled()) - (d) Gate agent 5-minute timeout (
setTimeout+ abort)
— qwen3.7-max via Qwen Code /review
| } | ||
| try { | ||
| return await runGateAgent(config, bundle, signal); | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] Timeout errors are retried identically to transient errors. When the gate agent's 5-minute timeout fires (gateReviewAgents.ts:165), the error propagates here and the retry loop launches another 5-minute attempt. With MAX_AGENT_RETRIES=3, worst case = ~15 minutes of blocked waiting.
A timeout signals the operation is too expensive — retrying it at the same budget defeats the safety net. Consider distinguishing timeout errors:
| } catch (error) { | |
| } catch (error) { | |
| const msg = error instanceof Error ? error.message : String(error); | |
| if (msg.includes('Gate agent timeout')) { | |
| debugLogger.error( | |
| `[runAgentWithRetry] Gate agent timed out; not retrying (attempt ${attempt}/${MAX_AGENT_RETRIES})`, | |
| ); | |
| return null; | |
| } |
— qwen3.7-max via Qwen Code /review
| // because: | ||
| // 1. The gate agent is a critical review step that should complete if possible | ||
| // 2. Parent aborts are often transient (stream retries, round cleanup) | ||
| // 3. The gate agent's own timeout provides a safety net |
There was a problem hiding this comment.
[Suggestion] _parentSignal is accepted but never observed. If the parent aborts during gate agent execution, there's zero log output connecting the parent abort to the gate agent's continued execution. For debugging "I cancelled but it kept going" reports, oncall needs a breadcrumb.
Consider adding a one-time listener before the AbortController:
| // 3. The gate agent's own timeout provides a safety net | |
| _parentSignal.addEventListener( | |
| 'abort', | |
| () => { | |
| debugLogger.warn( | |
| '[runGateAgent] Parent signal aborted — gate agent continuing independently (own timeout is sole cancellation)', | |
| ); | |
| }, | |
| { once: true }, | |
| ); | |
| const gateAbortController = new AbortController(); |
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
The signal isolation design is sound — independent AbortController with its own timeout correctly prevents transient parent-side aborts from cascading into the gate agent, while the entry-time signal.aborted check and abort-aware delay() between retries still provide cancellation points. Timer/listener cleanup in the finally block is correct. The delay export from retry.ts is clean.
— qwen3.7-max via Qwen Code /review
✅ Maintainer verification — bug reproduced on merge-base, fixed on PR head (real-code A/B) + integrated TUI runThis fixes the Setup: PR head The fix in one paragraph
1. Real-code A/B — reproduces the bug, proves the fix (the decisive evidence)I wrote a behaviour test that drives the real execute: async (_ctx, signal) => {
parentToAbort.abort(); // transient parent-side abort during the gate run
await tick;
seen.abortedAtDecision = signal.aborted;
terminateMode = signal.aborted ? CANCELLED : GOAL; // real subagent observes ITS OWN signal
}
// ...
const decision = await runPlanApprovalGate(config, bundle, parent.signal);
expect(decision.kind).toBe('approved'); // pre-fix this was 'unavailable'Same test, run against both trees:
The merge-base failure is the reported bug: the gate is cancelled by the inherited transient abort, retries are skipped, and the call returns 2. Existing suites + typecheck (PR head)
All green (the author had never run them). Note: 3. Integrated TUI run — real
|
| 树 | runGateAgent（隔离） |
runPlanApprovalGate（用户可见） |
|---|---|---|
PR head b1baf061 |
✅ gate 在隔离signal 下运行（!== parent、未 abort）→ 返回 pass |
✅ kind: 'approved'，1 次尝试 |
merge-base 1cb0d7194（修复前） |
❌ gate 继承父 signal → 被 abort → CANCELLED（抛错） |
❌ expected 'unavailable' to be 'approved' |
merge-base 的失败正是所报告的 bug：gate 被继承的瞬态 abort 取消，重试被跳过，调用返回 unavailable —— 这正是导致模型反复重新调用 exit_plan_mode 而卡住的根因。PR 把两者都翻成绿。（我的 verify 测试是维护者编写的脚手架，不属于 PR。）
2. 既有套件 + typecheck（PR head）
| 文件 | 测试数 |
|---|---|
plan-gate/planApprovalGate.test.ts |
22 ✅ |
plan-gate/gateReviewAgents.test.ts |
12 ✅ |
utils/retry.test.ts |
84 ✅ |
tsc --noEmit（core，仅 PR 代码） |
无报错 ✅ |
全绿（作者从未跑过）。注意：planApprovalGate.test.ts 现在墙钟约 4 秒，因为两个重试测试会触发 PR 真实的 1 秒退避 —— 功能没问题，只是更慢；可以考虑是否接受测试中用真实 delay()（而非 fake timers）。
3. 集成 TUI 跑通 —— 真实 exit_plan_mode → gate → 批准
在 tmux 下驱动真实二进制（YOLO）。模型进入 plan 模式（prePlanMode=YOLO、创建 gate state），调用 exit_plan_mode，真实 gate 子代理在隔离 controller 下被 spawn，经 mock 模型评审返回 pass，gate 批准并恢复 YOLO：
✓ EnterPlanMode Entered plan mode.
✓ ExitPlanMode Gate approved.
1. Create src/Button.tsx with a Button component
2. Wire it into App.tsx
3. Add a unit test in Button.test.tsx
✦ ✅ Plan approved by the gate and plan mode exited.
这跑通了真实集成路径（ExitPlanModeTool → runPlanApprovalGate → runGateAgent → SubagentManager.createAgentHeadless）且隔离改动已生效 —— happy path 未被破坏。（abort 竞态本身由 §1 确定性证明；此处 mock 模型瞬时返回，不发生瞬态 abort。）
4. 一个需要权衡的设计取舍（非正确性 bug）
该修复完全隔离了 gate 与父 signal，因此 gate 运行期间真正的用户取消（ESC）不再被感知 —— gate 会一直跑到它自己的 5 分钟超时。作者将其定位为有意为之，与 agent.ts 中后台代理的隔离模式一致，并指出 AbortSignal 是布尔量，无法区分瞬态 abort（NO_FINISH_REASON/round cleanup）与真实用户取消。该论证成立，但最坏情况是真实 ESC 后出现约 5 分钟不可取消的 gate —— 值得明确签字确认，并最好在运行时将来能暴露 reason/type 时做个后续区分。
结论
该修复解决了所报告的 bug（A/B 证明：merge-base 复现 unavailable，PR head 返回 approved），通过全部既有 plan-gate/retry 测试 + typecheck，且集成 exit_plan_mode → gate 路径在 PR 构建上端到端可用。就正确性而言可合并 —— 唯一需要明确决定的是 §4 中「gate 运行期间 ESC」的隔离取舍。👍
Verified locally in an isolated environment at head b1baf061. The A/B test drives the real runGateAgent/runPlanApprovalGate; only the subagent (LLM) boundary is mocked. Bug reproduced on merge-base 1cb0d7194, fixed on PR head.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @CubeLander — thanks for the PR and the detailed root-cause analysis. The fix itself looks well-reasoned.
Before we can proceed with review, the PR body needs to follow our PR template. The current description uses custom headings (Bug Description, Fix, Changes, Testing, Patch File) that don't match the required structure. Could you restructure the body to include these sections?
- What this PR does — describe the change in prose
- Why it's needed — the motivation / user-facing problem (your root-cause analysis fits well here)
- Reviewer Test Plan — how a reviewer can verify the fix: reproduction steps, expected vs observed behavior, and evidence (before/after). The
Testingsection you already have is a good start. - Risk & Scope — main tradeoff (you already discuss the ESC-during-gate 5-min wait — this belongs here), what's out of scope, breaking changes
- Linked Issues — any related issue references
- 中文说明 — Chinese translation of the above in a
<details>block
The technical content is solid — this is purely a formatting step so reviewers can navigate consistently. Happy to help if anything in the template is unclear.
中文说明
@CubeLander 你好——感谢 PR 和详尽的根因分析，修复本身思路清晰。
在进入代码审查之前，PR 正文需要按照我们的 PR 模板 来组织。当前描述使用了自定义标题（Bug Description、Fix、Changes、Testing、Patch File），与模板要求的结构不匹配。请补充以下章节：
- What this PR does — 用文字描述改动
- Why it's needed — 动机 / 用户侧问题（你的根因分析很适合放在这里）
- Reviewer Test Plan — 审查者如何验证：复现步骤、期望与实际行为、证据（已有
Testing部分是好的起点） - Risk & Scope — 主要取舍（你已讨论了 ESC 期间 5 分钟等待——放在这里），不在范围内的内容，破坏性变更
- Linked Issues — 相关 issue 引用
- 中文说明 — 以上内容的中文翻译，放在
<details>折叠块中
技术内容很好——这纯粹是格式调整，方便审查者统一浏览。如模板有疑问随时沟通。
— Qwen Code · qwen3.7-max
|
@qwen-code /triage |
|
Thanks for the PR, @CubeLander! Template looks good ✓ On direction: this is a clear-cut bug fix — users getting permanently stuck in plan mode under AUTO/YOLO pre-plan is a real, reproducible problem. The root cause (transient parent aborts cascading into the gate agent via shared AbortSignal) is well-diagnosed, and the fix follows an established codebase pattern: On approach: the scope is tight — 3 files, 74 additions, every change directly serves the stated goal. The three-part split (signal isolation in Moving on to code review. 🔍 中文说明感谢贡献，@CubeLander！ 模板完整 ✓ 方向：这是一个明确的 bug 修复 —— AUTO/YOLO pre-plan 下用户永久卡在 plan 模式是真实的、可复现的问题。根因（瞬态父级 abort 通过共享 AbortSignal 级联进 gate agent）诊断准确，修复遵循了代码库已有的模式： 方案：范围精简 —— 3 个文件，74 行新增，每处改动都直接服务于声明的目标。三部分拆分（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewNo critical blockers found. The implementation matches what I'd independently propose — isolate the gate agent with its own One observation for the maintainer to weigh in on: the PR ships no tests for the new branches — abort during the 1s backoff delay, parent-signal isolation (gate keeps running when parent aborts mid-flight), pre-aborted entry path, and the 5-minute timeout path. The existing suites still pass, but those new code paths are uncovered. The previous review round already flagged this; the author's position is that the existing tests cover the retry/gate paths. Worth a maintainer call on whether follow-up tests should block merge or land separately. Test ResultsNote: Real-Scenario TestingThis PR fixes internal signal/retry logic — the bug requires a transient Maintainer @wenshao independently verified this with three methods:
That verification is the decisive evidence for this PR. 中文说明代码审查未发现关键阻断问题。实现与我的独立方案一致 —— 用独立的 一个需要维护者权衡的观察：PR 未为新分支补测试 —— 1 秒退避 delay 期间的 abort、父级 signal 隔离（父级中途 abort 时 gate 仍继续运行）、预先 abort 的入口路径、以及 5 分钟超时路径。既有套件仍然通过，但这些新代码路径未被覆盖。上一轮审查已提出此问题；作者的立场是既有测试覆盖了重试/gate 路径。需要维护者决定后续测试应该阻断合并还是可以单独提交。 测试结果注意： 真实场景测试本 PR 修复内部 signal/重试逻辑 —— bug 需要在 维护者 @wenshao 已用三种方法独立验证：
该验证是本 PR 的决定性证据。 — Qwen Code · qwen3.7-max |
|
Stepping back: this is a well-executed bug fix for a critical issue. The The implementation is exactly what I'd have proposed independently. Three focused changes, each serving the stated goal, no scope creep. The signal isolation pattern mirrors what the codebase already does for background agents in All 118 tests pass, typecheck is clean, build succeeds. The maintainer A/B verification is thorough and decisive — bug reproduced on merge-base, fixed on PR head, integrated TUI run confirms the happy path. The one open question is test coverage for the new branches (abort during backoff, parent-signal isolation, timeout path). That's a real gap worth addressing, but it doesn't block this fix — the correctness is proven by the maintainer's A/B test, and follow-up tests can land separately. Approving. ✅ 中文说明退后一步看：这是一个针对关键问题的出色 bug 修复。AUTO/YOLO pre-plan 模式下的 实现与我的独立方案完全一致。三处聚焦的改动，每处服务于声明的目标，无范围蔓延。signal 隔离模式与代码库中 全部 118 个测试通过，typecheck 干净，构建成功。维护者的 A/B 验证彻底且决定性 —— bug 在 merge-base 上复现，在 PR head 上修复，集成 TUI 运行确认了 happy path。 唯一的开放问题是新分支的测试覆盖（退避期间 abort、父级 signal 隔离、超时路径）。这是一个值得解决的真实缺口，但不应阻断此修复 —— 正确性已由维护者的 A/B 测试证明，后续测试可以单独提交。 批准 ✅ — Qwen Code · qwen3.7-max |
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. ✅
What this PR does
Isolates the Plan Approval Gate review agent from the parent abort-signal chain so transient parent-side aborts can no longer cancel it, and makes the gate's retry loop genuinely retry instead of collapsing to a single attempt. Three changes: (1)
gateReviewAgents.ts—runGateAgentnow runs the gate subagent under its own dedicatedAbortControllerwith a fixed 5-minute timeout (GATE_AGENT_TIMEOUT_MS) instead of the inherited parent signal; the parent-signal parameter is kept for signature stability but intentionally unused (renamed_parentSignal), and thefinallyblock clears the timeout and aborts the controller if it hasn't already fired so no timers or listeners leak. (2)planApprovalGate.ts—runAgentWithRetryreplaces the per-iterationsignal.abortedcheck with a single entry-time check (skip launching a gate agent at all if the parent is already aborted), and adds an abort-awaredelay(1000, signal)backoff between attempts that breaks the loop immediately if the parent is cancelled during the wait. (3)retry.ts— the existingdelay()helper is exported so the gate can reuse it (additive, no behavior change).Why it's needed
In AUTO/YOLO pre-plan mode, calling
exit_plan_modecould leave the user permanently stuck in plan mode through an infinite retry loop. The gate review agent inherited the parent's abort signal all the way down the chain (masterAbortController → roundAbortController → processFunctionCalls → ExitPlanModeToolInvocation.execute → runPlanApprovalGate → runGateAgent → subagent.execute). When the parent hit a transient condition — e.g. aNO_FINISH_REASONstream retry or per-round cleanup — the abort cascaded into the gate agent and cancelled it (mode=CANCELLED, no usable output). Compounding this,runAgentWithRetrycheckedsignal.abortedat the top of every loop iteration, so once the signal was aborted the remaining attempts were skipped instantly — "3 retries" effectively became "1 retry." The gate then reported unavailable,exit_plan_modereturned an error, the model re-calledexit_plan_mode, and the same transient failure repeated indefinitely. Representative logs:Giving the gate agent its own signal + timeout breaks the cascade, and the abort-aware backoff restores real retries while still honoring genuine cancellation at the loop boundaries.
Reviewer Test Plan
How to verify
This is non-user-visible signal/retry logic; confirm via the plan-gate suites, typecheck, and build (from repo root):
Behaviorally: enter plan mode under AUTO/YOLO pre-plan mode and trigger
exit_plan_modewhile the parent stream hits a transientNO_FINISH_REASONretry. Before — the gate agent is cancelled, retries are skipped, the gate returns "unavailable," andexit_plan_modeloops without ever exiting plan mode. After — the gate agent runs under its own signal, survives the transient parent abort, completes its review, and plan mode exits.A maintainer independently verified this on a real build — a real-code A/B that reproduces the exact bug on the merge-base and confirms the fix on the PR head, plus an integrated TUI run: #5185 (comment)
Evidence (Before & After)
Before —
exit_plan_modeinfinite-retry loop; Plan Approval Gate reports "unavailable"; user cannot leave plan mode (log signature above). After — the gate agent is isolated from transient parent aborts, completes, and plan mode exits cleanly. Internal signal-handling fix with no UI surface; the full before/after is captured in the maintainer A/B verification linked above.Tested on
The signal/timeout logic is platform-agnostic (
AbortController/setTimeout); CI exercises macOS, Windows, and Linux.Environment (optional)
Plan-gate unit suites +
tsc --noEmit+packages/corebuild. No special runtime required.Risk & Scope
signal.abortedcheck) and between retries (abort-awaredelay), but not mid-gate. This is the deliberate cost of making the gate resilient to transient parent aborts.runGateAgentis not called when pre-aborted, and the 5-minute timeout path. Windows/Linux not exercised locally.runGateAgent's third parameter is retained (renamed_parentSignal) so callers are unaffected; exportingdelayfromutils/retry.tsis additive.Linked Issues
No separate tracking issue — the root cause and reproduction are documented above (the
exit_plan_mode/ Plan Approval Gate stuck-loop under AUTO/YOLO pre-plan mode).中文说明
这个 PR 做了什么
让 Plan Approval Gate 评审 agent 从父级的 abort 信号链中隔离出来,使父级的瞬时 abort 不再能取消它,并让 gate 的重试循环真正重试,而不是退化成只试一次。共三处改动:(1)
gateReviewAgents.ts——runGateAgent现在让 gate 子 agent 跑在它自己独立的AbortController下,带一个固定的 5 分钟超时(GATE_AGENT_TIMEOUT_MS),不再使用继承来的父级 signal;父级 signal 参数为保持签名稳定而保留,但被刻意弃用(重命名为_parentSignal),并在finally中清除定时器、若控制器尚未触发则将其 abort,避免定时器/监听器泄漏。(2)planApprovalGate.ts——runAgentWithRetry把"每轮循环顶部检查signal.aborted"改成单次的入口检查(如果进入时父级已 abort,就根本不启动 gate agent),并在两次尝试之间加入一个 abort 感知的delay(1000, signal)退避;若退避期间父级被取消,会立即中断循环。(3)retry.ts—— 把已有的delay()辅助函数导出,供 gate 复用(纯新增,无行为变化)。为什么需要它
在 AUTO/YOLO pre-plan 模式下,调用
exit_plan_mode可能让用户永久卡在 plan 模式里,陷入无限重试循环。gate 评审 agent 沿整条链继承了父级的 abort 信号(masterAbortController → roundAbortController → processFunctionCalls → ExitPlanModeToolInvocation.execute → runPlanApprovalGate → runGateAgent → subagent.execute)。当父级遇到瞬时情况 —— 例如NO_FINISH_REASON流重试或每轮清理 —— abort 会级联进 gate agent 并把它取消(mode=CANCELLED,无可用输出)。雪上加霜的是,runAgentWithRetry在每轮循环顶部检查signal.aborted,所以信号一旦被 abort,剩余尝试就被瞬间跳过 —— "3 次重试"实际变成"1 次重试"。于是 gate 报告 unavailable,exit_plan_mode返回错误,模型再次调用exit_plan_mode,同样的瞬时失败无限重复。典型日志:给 gate agent 自己的 signal + 超时打断了这条级联,abort 感知的退避恢复了真正的重试,同时仍在循环边界处尊重真实的取消。
评审者测试计划
如何验证
这是非用户可见的 signal/重试逻辑;通过 plan-gate 测试套件、typecheck、构建来确认(在仓库根目录执行):
行为层面:在 AUTO/YOLO pre-plan 模式下进入 plan 模式,并在父级流遇到瞬时
NO_FINISH_REASON重试时触发exit_plan_mode。修复前 —— gate agent 被取消、重试被跳过、gate 返回 "unavailable",exit_plan_mode一直循环、始终无法退出 plan 模式。修复后 —— gate agent 跑在自己的 signal 下,扛过瞬时的父级 abort,完成评审,plan 模式正常退出。维护者已在真实构建上独立验证 —— 用真实代码 A/B 在 merge-base 上复现了该 bug、并在 PR head 上确认了修复,外加一次集成 TUI 运行:#5185 (comment)
证据(前后对比)
修复前 ——
exit_plan_mode无限重试循环;Plan Approval Gate 报告 "unavailable";用户无法离开 plan 模式(日志特征见上)。修复后 —— gate agent 与瞬时父级 abort 隔离、完成、plan 模式干净退出。这是内部 signal 处理的修复,无 UI 表层;完整前后对比见上方链接的维护者 A/B 验证。测试平台
该 signal/超时逻辑与平台无关(
AbortController/setTimeout);CI 会覆盖 macOS、Windows、Linux 三个平台。环境(可选)
plan-gate 单元测试套件 +
tsc --noEmit+packages/core构建。无需特殊运行环境。风险与范围
signal.aborted检查)和两次重试之间(abort 感知的delay)被尊重,但在 gate 运行中途不会。这是让 gate 对瞬时父级 abort 具备韧性所付出的有意代价。runGateAgent未被调用、以及 5 分钟超时路径。Windows/Linux 未在本地实测。runGateAgent的第三个参数被保留(重命名为_parentSignal),调用方不受影响;把delay从utils/retry.ts导出是纯新增。关联 Issue
无单独的跟踪 issue —— 根因与复现已在上文说明(AUTO/YOLO pre-plan 模式下
exit_plan_mode/ Plan Approval Gate 卡死循环)。