feat(core): make self-paced /loop lean on monitor/background-task notifications - #5844
Conversation
…ifications A self-paced /loop only knew how to keep itself alive by scheduling a timer wakeup, even when a Monitor or a backgrounded agent it started re-invokes the session on its own via a <task-notification> once it finishes. Teach the LoopWakeup tool description and the bundled loop skill that such work makes the wakeup a long fallback heartbeat (1200-1800s), not a short poll: a short poll wastes a prompt-cache window, and the notification already wakes the loop the moment the work terminates. Keep the fallback rather than omitting it — a monitor can auto-stop on idle/max-events, be owned by another agent, or the work can hang. Guidance only: qwen already auto-wakes the session on these notifications; this just stops the loop from polling work that reports on its own. Closes QwenLM#5841 Co-Authored-By: Qwen-Coder <noreply@qwen.ai>
|
Thanks for the PR! Re-running triage after maintainer verification surfaced something Stage 1 should have caught. Template looks good ✓ On direction: still solid. Teaching On scope — correction from the prior run: the PR body says "guidance only; it adds no runtime wiring", but that's no longer accurate. Commit On approach: focused. The 8-commit stack is a clean narrative (clarify → persist restart count → include command → cap ambiguous follow-ups → cover in tests), no drive-by refactors, no unrelated file touches. Moving on to code review. 🔍 中文说明感谢贡献！本次为验证者复核触发的重跑，Stage 1 该抓住但没抓到的问题现在补上。 模板完整 ✓ 方向：仍然对齐。让 范围——对上一轮的修正：PR 正文写 "guidance only; it adds no runtime wiring"，但这已不准确。提交 方案：聚焦。8 个提交脉络清晰（厘清 → 保留重启计数 → 包含命令 → 截短模糊续作 → 测试覆盖），无顺手重构、无无关文件改动。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): To solve "self-paced loops short-poll when a monitor/background task is already watching," I would (1) update Diff comparison: the PR matches the independent proposal on all three guidance/test items, and additionally makes the terminal notification carry a Correctness / reuse:
Optional (non-blocking): TestingUnit tests (PR branch, main working tree): All 80 pass, including the new Runtime change, observed from the diff: @@ monitorRegistry.ts
- `<summary>Monitor "${escapeXml(desc)}" ${statusText}. Total events: ${entry.eventCount}.${…}</summary>`,
+ `<summary>Monitor "${escapeXml(desc)}" ${statusText}. Total events: ${entry.eventCount}.${…}</summary>`,
+ `<command>${escapeXml(stripDisplayControlChars(entry.command))}</command>`,Terminal Real-scenario tmux pass: the maintainer (@wenshao) independently ran a comprehensive tmux verification on the same branch (commit 中文说明代码审查独立方案（读 diff 之前）： 要解决"自定步循环在已有 monitor/后台任务时仍短轮询"，我会 (1) 更新 Diff 对比： 上述三项 guidance/测试 PR 全部匹配，并额外让终止通知带上 正确性/复用：
可选（不阻塞）： 测试单元测试（PR 分支，主工作树）： 三个涉及套件的 全部 80 通过，包括新增的 运行时改动（来自 diff）： 终止 真实场景 tmux： 验证者 @wenshao 已独立在同分支（commit — Qwen Code · qwen3.7-max |
|
Reflection on the rerun. Going back to my independent proposal from Stage 2: I would have done the same guidance updates in the same files, but I would have missed the Net assessment:
Am I approving because it's genuinely good, or because I ran out of reasons to say no? Genuinely good. The runtime change is small, safe, well-tested, and makes the guidance actionable; the guidance is accurate against the code; the optional nits (command cap, body wording) are things the maintainer can choose to land or not. The maintainer's own verdict is "recommend merge" after a verification pass more thorough than mine — I have no basis to disagree. Approving. ✅ 中文说明复核反思。 回到 Stage 2 的独立方案：guidance 部分我会做一样的改动，但我会漏掉 总体评估：
是因为真的好而批准，还是因为找不到反对理由而批准？真的好。运行时改动小、安全、测试充分、让 guidance 可执行；guidance 与代码一致；可选小问题（command 上限、正文措辞）由验证者决定是否落地。验证者自己跑过比我更彻底的验证后给出"建议合并"——我没有理由不同意。 批准 ✅ — 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. ✅
…p-monitor-fallback
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x).
— qwen3.7-max via Qwen Code /review
| LoopWakeupTool.Name, | ||
| ToolDisplayNames.LOOP_WAKEUP, | ||
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h.', | ||
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h. When a background task you started will wake you on its own — a backgrounded agent or a Monitor emits a `<task-notification>` when it finishes — keep this wakeup as a long fallback heartbeat rather than a poll; see `delaySeconds`.', |
There was a problem hiding this comment.
[Suggestion] The tool description (and SKILL.md step 3) says a Monitor "emits a <task-notification> when it finishes," but monitorRegistry.ts actually emits a <task-notification> for every stdout line (emitNotification with status=running at line ~498) in addition to the terminal notification (emitTerminalNotification at line ~533). A chatty monitor (e.g., tail -f on a busy log) will wake the agent many times, not once at finish.
This matters because a model following this guidance may start a monitor expecting a single wake-up event, then get flooded with per-line notifications.
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h. When a background task you started will wake you on its own — a backgrounded agent or a Monitor emits a `<task-notification>` when it finishes — keep this wakeup as a long fallback heartbeat rather than a poll; see `delaySeconds`.', | |
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h. When a background task you started will wake you on its own — a backgrounded agent or a Monitor emits `<task-notification>` events as it runs (per stdout line) and a final one on termination — keep this wakeup as a long fallback heartbeat rather than a poll; see `delaySeconds`.', |
— qwen3.7-max via Qwen Code /review
| - `prompt`: `/loop ${original prompt}` | ||
| - `reason`: a short reason for the chosen delay. | ||
| 5. Briefly tell the user what was done now. If a wakeup was scheduled, include when the next check is expected. If no wakeup was scheduled, say the loop is complete or not continuing. | ||
| 5. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event first, then decide whether to re-arm the fallback wakeup or end the loop. |
There was a problem hiding this comment.
[Suggestion] Step 5 says "handle that event first" but provides no guidance on how to distinguish the four <task-notification> types a monitor can produce: per-line status=running, terminal status=completed, idle-timeout auto-stop, and max-events auto-stop. All arrive as <task-notification> with different semantics — an idle-timeout auto-stop should trigger re-arming, while a successful completion should end the loop.
Consider adding a sub-bullet with decision heuristics, e.g.:
- If the notification reports the watched condition was met → end the loop.
- If the monitor auto-stopped (idle timeout or max-events) → restart the monitor and re-arm the fallback.
- If ambiguous → re-arm with a shorter delay and investigate next tick.
— qwen3.7-max via Qwen Code /review
| it('teaches the self-paced loop to lean on monitor/background-task notifications', () => { | ||
| const { body } = loadLoopSkill(); | ||
|
|
||
| expect(body).toContain('<task-notification>'); |
There was a problem hiding this comment.
[Suggestion] The new test asserts four substrings but misses the most load-bearing behavioral directive added by this PR: "Do not omit it just because something is watching". This sentence overrides a plausible model inference ("a monitor is watching, so I don't need a fallback wakeup"). The step 5 qualifier "not a bare \/loop` wakeup prompt"` is also untested.
If a future edit softens or removes these directives, the test still passes. Consider adding:
expect(body).toContain('Do not omit it just because something is watching');
expect(body).toContain('not a bare `/loop` wakeup prompt');— qwen3.7-max 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.
Two Critical findings verified against the Session code (see inline), plus two Suggestions. The per-line-notification issue is load-bearing: the model following this guidance would expect to be woken by every stdout line, but Session.ts #registerBackgroundNotificationCallbacks explicitly drops status=running monitor events — only terminal notifications re-invoke the session.
| - Do not call LoopWakeup if the task is complete. | ||
| - Do not call LoopWakeup if the task is blocked on user input or external state that cannot be checked later. | ||
| - Do not call LoopWakeup just to keep polling when no useful next check exists. | ||
| - If you started a background agent or a Monitor, it wakes you via `<task-notification>` events as it runs (per stdout line) and a final one on termination — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, a Monitor auto-stops on idle or max-events, and one owned by another agent never notifies you. Omit LoopWakeup only on the terminal conditions above (complete, or blocked). |
There was a problem hiding this comment.
[Critical] The claim "a backgrounded agent or a Monitor emits <task-notification> events as it runs (per stdout line) and a final one on termination" is inaccurate on two counts, verified against the actual Session wiring:
-
Per-line monitor events are dropped at the Session boundary.
packages/cli/src/acp-integration/session/Session.tsline ~2638 in#registerBackgroundNotificationCallbackshas an explicitif (meta.status === 'running') { return; }guard on the monitor registry callback. Only terminal events (completed / failed / cancelled / idle-timeout / max-events) frommonitorRegistry.emitTerminalNotificationreach#enqueueBackgroundNotificationand re-invoke the model. Every per-lineemitNotificationcall is silently discarded. -
Background agents never emit per-line notifications.
BackgroundTaskRegistry.emitNotificationis only called fromcomplete(),fail(), andfinalizeCancelled()— all terminal transitions. A backgrounded agent is completely silent to the parent until it reaches a terminal state.
The delaySeconds description on line 56 actually gets this right ("once it finishes"), but this bullet and the loop-wakeup.ts tool description promise a mechanism that does not exist. A model that sets a 1200–1800s fallback expecting to be woken "per stdout line" will sleep past a watched condition that appears mid-output — it will only be woken when the monitor terminates or auto-stops.
| - If you started a background agent or a Monitor, it wakes you via `<task-notification>` events as it runs (per stdout line) and a final one on termination — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, a Monitor auto-stops on idle or max-events, and one owned by another agent never notifies you. Omit LoopWakeup only on the terminal conditions above (complete, or blocked). | |
| - If you started a background agent or a Monitor, it wakes you via a single terminal `<task-notification>` (process exit, failure, cancellation, or a Monitor's auto-stop on idle or max-events) — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, a Monitor auto-stops on idle or max-events, and one owned by another agent never notifies you. Omit LoopWakeup only on the terminal conditions above (complete, or blocked). |
Also apply the same fix to the matching sentence in packages/core/src/tools/loop-wakeup.ts line 128.
— qwen3.7-max via Qwen Code /review
| LoopWakeupTool.Name, | ||
| ToolDisplayNames.LOOP_WAKEUP, | ||
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h.', | ||
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h. When a background task you started will wake you on its own — a backgrounded agent or a Monitor emits `<task-notification>` events as it runs (per stdout line) and a final one on termination — keep this wakeup as a long fallback heartbeat rather than a poll; see `delaySeconds`.', |
There was a problem hiding this comment.
[Critical] The tool description claims "a backgrounded agent or a Monitor emits <task-notification> events as it runs (per stdout line) and a final one on termination." Verified against the code: this is wrong on both counts.
packages/cli/src/acp-integration/session/Session.ts#registerBackgroundNotificationCallbackshas an explicitif (meta.status === 'running') { return; }guard on the monitor registry callback. Per-lineemitNotificationevents frommonitorRegistry.tsare silently dropped — only terminalemitTerminalNotificationevents (completed / failed / cancelled / idle-timeout / max-events) re-invoke the session.BackgroundTaskRegistry.emitNotificationis called only fromcomplete(),fail(), andfinalizeCancelled(). Background agents never emit per-line notifications.
Your own delaySeconds description on line 135 already gets this right ("once it finishes"). A model following the tool description will expect per-line wake-ups that will never arrive, and will sleep past a watched condition that appears mid-output.
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h. When a background task you started will wake you on its own — a backgrounded agent or a Monitor emits `<task-notification>` events as it runs (per stdout line) and a final one on termination — keep this wakeup as a long fallback heartbeat rather than a poll; see `delaySeconds`.', | |
| 'Schedule when to resume work in a self-paced loop iteration (always pass the `prompt` arg). Call this before ending the turn to keep the loop alive; omit the call to end the loop. Session-only and one-shot — it does not persist or recur. A self-paced wakeup chain may run for at most 24h. When a background task you started will wake you on its own — a backgrounded agent or a Monitor emits a single terminal `<task-notification>` (process exit, failure, cancellation, or a Monitor auto-stop on idle or max-events) — keep this wakeup as a long fallback heartbeat rather than a poll; see `delaySeconds`.', |
— qwen3.7-max via Qwen Code /review
| - `prompt`: `/loop ${original prompt}` | ||
| - `reason`: a short reason for the chosen delay. | ||
| 5. Briefly tell the user what was done now. If a wakeup was scheduled, include when the next check is expected. If no wakeup was scheduled, say the loop is complete or not continuing. | ||
| 5. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event first, then decide whether to re-arm the fallback wakeup or end the loop. |
There was a problem hiding this comment.
[Suggestion] "handle that event first" contradicts the step ordering. Steps 2–4 tell the model to "Run the parsed prompt immediately now" and decide whether another check is useful (including scheduling a LoopWakeup). Step 5 is reached only after the prompt has been executed. On a notification-driven re-invocation, a model reading the numbered sequence top-to-bottom will re-run the prompt and potentially schedule a new fallback wakeup before handling the incoming <task-notification>. The word "first" in step 5 contradicts its sequential position.
This leads to duplicated work (re-reading a deploy log when the notification already says "deploy succeeded") and unnecessary tool calls on each notification-driven tick.
Either move this step above step 2, or add an explicit guard at the top of the self-paced path, e.g.:
| 5. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event first, then decide whether to re-arm the fallback wakeup or end the loop. | |
| On a tick that opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), skip straight to step 5 before running the prompt. |
— qwen3.7-max via Qwen Code /review
| 5. Briefly tell the user what was done now. If a wakeup was scheduled, include when the next check is expected. If no wakeup was scheduled, say the loop is complete or not continuing. | ||
| 5. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event first, then decide whether to re-arm the fallback wakeup or end the loop. | ||
| - If the notification says the watched condition was met, finish the loop. | ||
| - If a monitor auto-stopped on idle or max-events, restart it if the watch is still useful and re-arm the fallback. |
There was a problem hiding this comment.
[Suggestion] "restart it if the watch is still useful and re-arm the fallback" has no circuit-breaker. If a monitor keeps auto-stopping on the same trigger (a noisy log source that always hits maxEvents within seconds, or a stable file that idle-times-out immediately), the model will restart it, receive another auto-stop notification, restart again, and loop indefinitely — each cycle consuming a full model turn with tool calls. The loop can only terminate when the 24h wakeup-chain limit is reached, the token budget runs out, or the user intervenes. This is the exact class of misbehavior that step 3's terminal conditions are meant to prevent.
| - If a monitor auto-stopped on idle or max-events, restart it if the watch is still useful and re-arm the fallback. | |
| - If a monitor auto-stopped on idle or max-events, restart it once if the watch is still useful and re-arm the fallback. If it auto-stops again on the next tick, end the loop and report the repeated auto-stop to the user. |
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-review after commit 49091d89 ("clarify loop task notifications").
The commit addressed one of the prior Suggestions — the test now asserts "Do not omit it just because something is watching" and "not a bare \/loop` wakeup prompt"`, closing the coverage gap flagged earlier. The other four prior findings (1 Critical + 3 Suggestions) remain unresolved on the current HEAD.
The Critical per-line-notification concern is still valid and is load-bearing for the guidance:
- Registry layer (
monitorRegistry.ts~line 498):emitNotificationdoes emit a<task-notification>with<status>running</status>for every stdout line — this part is correct. - Daemon path (
Session.ts~line 2638,#registerBackgroundNotificationCallbacks): the monitor callback has an explicitif (meta.status === 'running') { return; }guard — all per-line running-status events are unconditionally dropped. The model never sees them; only terminal (completed/failed/cancelled) notifications re-invoke the session. - TUI (
useGeminiStream.ts~line 3234), non-interactive CLI (nonInteractiveCli.ts~line 648), headless SDK (nonInteractive/session.ts~line 203): running events ARE delivered (with a stale-entry guard). Per-line notifications reach the model.
So the PR's wording — "emits <task-notification> events as it runs (per stdout line)" — is true for three of four paths but false for the daemon path, which is where /loop most commonly runs. A model following this guidance would expect to be woken by every stdout line, but in the daemon path it will only be woken on termination. The long-fallback-heartbeat advice is still directionally right, but the per-line justification overclaims. Suggested reword (both the tool description at loop-wakeup.ts:128 and SKILL.md step 3): drop the "per stdout line" qualifier and say "a Monitor or backgrounded agent emits a terminal <task-notification> when it completes, fails, or is cancelled" — accurate across all paths without leaking a per-line detail that only applies outside the daemon.
The three unresolved Suggestions are unchanged from the prior review:
- SKILL.md step 5 "handle that event first" still contradicts the top-to-bottom ordering of steps 2–4, which commit the model to running the prompt and deciding on a LoopWakeup before step 5 is reached.
- SKILL.md step 5's restart-the-monitor branch still has no circuit-breaker for a monitor that keeps auto-stopping on the same trigger.
- SKILL.md step 5 still doesn't distinguish the four
<task-notification>kinds a monitor can produce (status=runningper-line, terminalcompleted, idle-timeout auto-stop, max-events auto-stop).
Deterministic checks clean: tsc --noEmit and eslint pass on the 4 changed files; vitest run src/tools/loop-wakeup.test.ts src/skills/bundled/loop/SKILL.test.ts passes 24/24.
— qwen3.7-max via Qwen Code /review
| expect(body).not.toContain('delayMinutes'); | ||
| }); | ||
|
|
||
| it('teaches the self-paced loop to lean on monitor/background-task notifications', () => { |
There was a problem hiding this comment.
[Suggestion] The new test asserts 15+ substrings from SKILL.md but misses the two core safety justifications added at SKILL.md line 58: the work may hang and one owned by another agent never notifies you. These are the primary reasons LoopWakeup must not be omitted — the central behavioral argument of this entire PR — yet they have no test guard. A future edit could soften or drop them without breaking any assertion.
| it('teaches the self-paced loop to lean on monitor/background-task notifications', () => { | |
| it('teaches the self-paced loop to lean on monitor/background-task notifications', () => { | |
| const { body } = loadLoopSkill(); | |
| expect(body).toContain('<task-notification>'); | |
| expect(body).toContain('set LoopWakeup as a long fallback'); | |
| expect(body).toContain('auto-stops on idle or max-events'); | |
| expect(body).toContain('handle that event before re-running the prompt'); | |
| expect(body).toContain('terminal `<task-notification>`'); | |
| expect(body).not.toContain('per stdout line'); | |
| expect(body).toContain( | |
| 'If the notification says the watched condition was met', | |
| ); | |
| expect(body).toContain('If a monitor auto-stopped'); | |
| expect(body).toContain('restart it once'); | |
| expect(body).toContain('report the repeated auto-stop to the user'); | |
| expect(body).toContain('If the signal is ambiguous'); | |
| expect(body).toContain('Do not omit it just because something is watching'); | |
| expect(body).toContain('the work may hang'); | |
| expect(body).toContain('another agent never notifies you'); | |
| expect(body).toContain('not a bare `/loop` wakeup prompt'); | |
| }); |
— qwen3.7-max via Qwen Code /review
| 2. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event before re-running the prompt. | ||
| - If the notification says the watched condition was met, finish the loop. | ||
| - If a monitor auto-stopped on idle or max-events, restart it once if the watch is still useful and re-arm the fallback. If it auto-stops again on the next tick, end the loop and report the repeated auto-stop to the user. | ||
| - If the signal is ambiguous, re-arm a shorter follow-up and investigate on the next tick. |
There was a problem hiding this comment.
[Suggestion] "Restart it once" has no state-persistence mechanism. Loop ticks are 1200–1800s apart (20–30 min), and context truncation could erase the prior turn's reasoning. The model on a later tick sees a fresh <task-notification> about a monitor auto-stop with no history that it already used its one restart — it restarts again, creating a silent restart loop that never escalates to the user.
Consider instructing the model to include the restart count in its user-facing summary (step 6) so it survives context truncation — e.g., "When reporting, include the restart count (e.g., 'monitor restarted 1/1 time') so the next tick can read it from the conversation history."
— qwen3.7-max via Qwen Code /review
| - If you started a background agent or a Monitor, it wakes you via a terminal `<task-notification>` on exit, failure, cancellation, or monitor auto-stop — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, a Monitor auto-stops on idle or max-events, and one owned by another agent never notifies you. Omit LoopWakeup only on the terminal conditions above (complete, or blocked). | ||
| 5. When scheduling a continuation, call LoopWakeup with: | ||
| - `delaySeconds`: the next useful delay in seconds. The runtime clamps to 60–3600 (1–60 min); follow the tool's own guidance on picking a value — it accounts for the prompt-cache window and for the fallback-heartbeat case when a background task will wake you. | ||
| - `prompt`: `/loop ${original prompt}` |
There was a problem hiding this comment.
[Suggestion] "one owned by another agent never notifies you" is factually wrong for background agents. The BackgroundTaskRegistry has no per-agent routing — it uses a single session-level notificationCallback (background-tasks.ts:966), so ALL background agent completions are delivered to the session regardless of which agent started them. Only the MonitorRegistry has per-agent routing via ownerAgentId. Since this sentence explicitly references both ("a background agent or a Monitor"), the claim misleads the model for background agents.
Also, the parenthetical "(complete, or blocked)" omits the repeated-auto-stop terminal condition introduced in step 2.
| - `prompt`: `/loop ${original prompt}` | |
| - If you started a background agent or a Monitor, it wakes you via a terminal `<task-notification>` on exit, failure, cancellation, or monitor auto-stop — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, a Monitor auto-stops on idle or max-events (and one owned by another agent routes its notification only to that agent). Omit LoopWakeup only on the terminal conditions above (complete, blocked, or repeated monitor auto-stop). |
— qwen3.7-max via Qwen Code /review
| - If a monitor auto-stopped on idle or max-events, restart it once if the watch is still useful, re-arm the fallback, and report the restart count to the user. If it auto-stops again on the next tick, end the loop and report the repeated auto-stop to the user. | ||
| - If the signal is ambiguous, re-arm a shorter follow-up and investigate on the next tick. | ||
| 3. Run the parsed prompt immediately now. | ||
| - If it is a slash command, invoke it via the Skill tool. |
There was a problem hiding this comment.
[Suggestion] The ambiguous-signal branch has no retry cap, unlike the other two branches: "condition met" terminates immediately, "auto-stopped" restarts once then terminates, but "ambiguous" re-arms with no limit. If monitored resources consistently produce ambiguous output, the loop re-arms indefinitely (up to the 24h chain cap).
| - If it is a slash command, invoke it via the Skill tool. | |
| - If the signal is ambiguous, re-arm a shorter follow-up and investigate on the next tick. If the signal remains ambiguous for three consecutive ticks, end the loop and report to the user that the watch could not reach a clear conclusion. |
— qwen3.7-max 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.
No review findings. Downgraded from Approve to Comment: CI still running.
The previous round's Critical findings (per-line notification inaccuracy) have been correctly addressed — the guidance now accurately describes terminal-only <task-notification> delivery and the restart-once policy. The 6 stale comments from earlier commits are superseded by the current code.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-review after commit db93f36d (two commits since last review at 49091d89).
The new commits address the two Critical findings from the prior review — "per stdout line" is fully replaced with "terminal <task-notification> on exit, failure, cancellation, or monitor auto-stop," and the step ordering is fixed by moving notification handling to step 2. Tests are updated accordingly and all 24 pass.
The prior Suggestions about restart state persistence and ambiguous-signal retry cap were partially addressed: "restart it once" + "end the loop on second auto-stop" adds a natural-language circuit breaker, and "report the restart count" surfaces the tracking intent. However, neither concern has a code-level backing — the restart count lives only in conversation memory (vulnerable to compaction over 1200-1800s ticks), and the ambiguous-signal branch still lacks explicit bounds.
One new finding below: the "restart it once" guidance assumes the model can reconstruct the monitor command, but the terminal <task-notification> XML from monitorRegistry.ts doesn't include it.
— qwen3.7-max via Qwen Code /review
| 2. Run the parsed prompt immediately now. | ||
| 2. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event before re-running the prompt. | ||
| - If the notification says the watched condition was met, finish the loop. | ||
| - If a monitor auto-stopped on idle or max-events, restart it once if the watch is still useful, re-arm the fallback, and report the restart count to the user. If it auto-stops again on the next tick, end the loop and report the repeated auto-stop to the user. |
There was a problem hiding this comment.
[Suggestion] The "restart it once" guidance assumes the model can (a) track that it already restarted across ticks 1200–1800s apart, and (b) reconstruct the original monitor command. Both are fragile:
- State persistence: the restart count lives only in conversation memory. Over 1200–1800s ticks, context compaction can erase it, causing the model to either restart indefinitely or end the loop prematurely. Consider encoding the count in the LoopWakeup
reasonfield or the continuation prompt so it survives compaction. - Command availability: the terminal
<task-notification>XML emitted bymonitorRegistry.ts(emitTerminalNotification) includes<task-id>,<status>,<event-count>, and<summary>— but not the originalcommand. The<summary>contains only the truncateddescription(≤80 chars), not a runnable command. After context compaction, the model may not be able to reconstruct the exact command to restart.
Consider adding the original command to the terminal notification XML so the model has it available for restart without relying on conversation history.
— qwen3.7-max via Qwen Code /review
| const [displayText, modelText] = callback.mock.calls[0] as [string, string]; | ||
| expect(displayText).toContain('completed'); | ||
| expect(modelText).toContain('<status>completed</status>'); | ||
| expect(modelText).toContain('<command>tail -f /var/log/app.log</command>'); |
There was a problem hiding this comment.
[Suggestion] The <command> assertion uses the safe default tail -f /var/log/app.log. No test covers XML escaping of the command field — if escapeXml were accidentally removed from line 565 of monitorRegistry.ts, commands containing <, >, or & would produce malformed XML without any test failing.
Consider adding a test that registers a monitor with command: 'grep "a&b" < /dev/null' and asserts the escaped output:
expect(modelText).toContain('<command>grep "a&b" < /dev/null</command>');— qwen3.7-max via Qwen Code /review
| - `prompt`: `/loop ${original prompt}` | ||
| - `reason`: a short reason for the chosen delay. | ||
| 5. Briefly tell the user what was done now. If a wakeup was scheduled, include when the next check is expected. If no wakeup was scheduled, say the loop is complete or not continuing. | ||
| - If you started a background agent or a Monitor, it wakes you via a terminal `<task-notification>` on exit, failure, cancellation, or monitor auto-stop — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, or a Monitor may auto-stop on idle or max-events (and one owned by another agent routes its notification only to that agent). Omit LoopWakeup only on the terminal conditions above (complete, blocked, or repeated monitor auto-stop). |
There was a problem hiding this comment.
[Suggestion] When a task-notification arrives and the loop ends (condition met or repeated auto-stop), the previously scheduled 1200–1800s fallback wakeup is still pending in the CronScheduler. It will fire 20–30 minutes later as a stale prompt, and sessionSize counts pending wakeups so it keeps the headless process alive. With the previous 60–270s delays this was minor; at 1200–1800s it's a meaningful resource hold.
Consider adding a note here to cancel the pending fallback wakeup with CronDelete when ending the loop. If the wakeup ID was compacted away, the stale wakeup is self-limiting (fires once and is deleted), so a brief "ignore or answer briefly" note for unexpected wakeups would suffice.
— qwen3.7-max via Qwen Code /review
| 2. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event before re-running the prompt. | ||
| - If the notification says the watched condition was met, finish the loop. | ||
| - If a monitor auto-stopped on idle or max-events, restart it once if the watch is still useful, re-arm the fallback, report the restart count to the user, and include that count in the LoopWakeup prompt or reason (for example, `monitor restarted 1/1 time`) so it survives context compaction. If it auto-stops again on the next tick, end the loop and report the repeated auto-stop to the user. | ||
| - If the signal is ambiguous, re-arm a shorter follow-up and investigate on the next tick. If the signal remains ambiguous for three consecutive ticks, end the loop and report that the watch could not reach a clear conclusion. |
There was a problem hiding this comment.
[Suggestion] The three-tick ambiguity cap has no persistence mechanism, unlike the adjacent restart counter which explicitly carries state via monitor restarted 1/1 time in the LoopWakeup prompt. After context compaction (likely across 1200–1800s ticks), the model may lose track of how many consecutive ticks were ambiguous and never trigger the cap — the loop keeps re-arming indefinitely.
Consider mirroring the restart-branch pattern: add an instruction to include the ambiguity count in the LoopWakeup prompt (e.g., ambiguous tick 2/3), and add it as an example in step 5's prompt field alongside monitor restarted 1/1 time.
— qwen3.7-max via Qwen Code /review
✅ Local verification report — PR #5844I built this branch and ran a real-runtime verification locally (Linux, in Verdict: behaviour is correct and precisely guarded by the new tests. Recommend merge. One minor wording nit in the PR body (the change is no longer purely guidance — see Note 1), and two optional/neutral observations. What I verified & how
The runtime change, observed (real compiled
|
| Guidance claim | Code | Verdict |
|---|---|---|
Monitor auto-stops on idle or max-events, emitting a terminal <task-notification> |
monitorRegistry.ts idle timer + eventCount >= maxEvents, both call emitTerminalNotification |
✅ |
| A monitor owned by another agent routes its notification only to that agent | dispatchNotification → agentNotificationCallbacks.get(ownerAgentId) else default |
✅ |
| Only a terminal notification wakes the loop (no per-line poll) | ACP path drops status==='running'; PR also removes any "per stdout line" wording |
✅ |
delaySeconds clamped to [60, 3600] |
WAKEUP_MIN_SECONDS=60, WAKEUP_MAX_SECONDS=3600 |
✅ |
| Notification re-invokes the session | nonInteractiveCli.ts local-queue drain + Session.ts notification-queue drain |
✅ |
Notes (none blocking)
- Body wording: the description says "guidance only; it adds no runtime wiring." A later commit (
9aec4ad6a) adds a real—though additive and safely escaped—runtime change: the<command>element in the terminal notification payload. It's not new wiring, but it is a behaviour change worth one line in the body so reviewers don't skip it. - Optional (low):
entry.commandis not length-capped in the<command>element, whereasdescriptionis capped at 80 and event lines at 2000. Commands are normally short, so the risk is small; a cap would be symmetric/defensive. - Neutral: running notifications omitting
<command>is by design (terminal-only) and correct for the loop's wake semantics — flagged only so the asymmetry is intentional on record.
Recommendation: ✅ Looks good to merge. The new behaviour is correct, well-escaped, and exactly covered by the added tests; the model-facing guidance is accurate against the runtime.
中文说明
我在本地拉取该分支做了真实运行时验证（Linux，全程在 tmux 中执行并留存日志），不是只读代码。先给结论，证据在下。
结论：行为正确，且被新增测试精确覆盖，建议合并。 PR 正文有一处措辞需微调（本改动已不再是纯 guidance，见注 1），另有两条可选/中性观察。
验证内容与方法
- 分支/提交：
feat/loop-monitor-fallback-heartbeat@343374421，packages/core由源码构建通过。 - 手段： PR 自带 vitest 套件；一个驱动
packages/core/dist中真实编译MonitorRegistry的 harness；一轮 RED/GREEN 隔离验证；以及对照真实唤醒机制对 guidance 文本逐条 fact-check。所有测试均在tmux会话内运行并捕获。
| # | 检查项 | 结果 |
|---|---|---|
| 1 | PR 单测 monitorRegistry + loop-wakeup + SKILL |
80/80 通过（56 + 19 + 5） |
| 2 | RED/GREEN：把 3 个源文件回退到 merge-base、保留 PR 的测试 | 恰好 3 个失败、77 通过——每个文件各失败一个、无连带；恢复后转绿 |
| 3 | 真实 MonitorRegistry harness：complete(0/2)、fail、cancel、max-events 自停、idle 自停、owner-scoped 投递 |
20/20 通过 |
| 4 | 对那一行运行时改动做 before/after（pre-fix dist） | 各终止路径的 <command> 全部缺失（8 个存在性断言翻转）；running 通知两侧都不含 command |
| 5 | guidance 文本对照真实代码 | 准确（见下表） |
实际观察到的运行时改动（真实编译 MonitorRegistry）
harness 喂入一个同时包含五个 XML 元字符、外加 C0 控制符（BEL）、ANSI ESC 序列、bidi RLO 覆盖符（U+202E）的命令，逐一驱动各终止路径。每个终止 <task-notification> 现都带上经正确转义 + 控制符/bidi 剥离的 <command>：
<summary>Monitor "watch app logs" completed. Total events: 0.</summary>
+ <command>tail -f /var/log/app.log</command> ← 本 PR 新增（仅终止通知）
<result>Exited with code 0</result>- 转义/剥离精确无误：
grep "a&b" < … ' \x07\x1b[31m \u202e…→grep "a&b" < … ' [31m …，BEL/ESC/RLO 字节被去除。这把 PR 自带的转义测试（覆盖" & <）扩展到了控制符/bidi 的 "Trojan Source" 类——同样通过。 - running 通知刻意不带
<command>，仅终止通知带——这与"自定步循环只由终止通知唤醒"一致。 - owner-scoped 监视器的终止通知（含
<command>）只投递给所属 agent 的回调，绝不投给父级——与 guidance 中"被别的 agent 持有"那条吻合。
Guidance fact-check（断言 ↔ 代码）
| guidance 断言 | 代码 | 结论 |
|---|---|---|
Monitor 在 idle 或 max-events 自停并发终止 <task-notification> |
monitorRegistry.ts idle timer + eventCount >= maxEvents，均调用 emitTerminalNotification |
✅ |
| 被别的 agent 持有的监视器，其通知只投给那个 agent | dispatchNotification → agentNotificationCallbacks.get(ownerAgentId)，否则默认 |
✅ |
| 只有终止通知会唤醒循环（不逐行轮询） | ACP 路径丢弃 status==='running'；PR 也删除了任何 "per stdout line" 措辞 |
✅ |
delaySeconds 钳制到 [60, 3600] |
WAKEUP_MIN_SECONDS=60、WAKEUP_MAX_SECONDS=3600 |
✅ |
| 通知会重新唤起会话 | nonInteractiveCli.ts 本地队列 drain + Session.ts 通知队列 drain |
✅ |
备注（均不阻塞）
- 正文措辞： 描述写的是 "guidance only; it adds no runtime wiring"。后续提交（
9aec4ad6a）加入了一处真实——但属增量且已安全转义——的运行时改动：终止通知里的<command>。它不算新"接线"，但确是行为变化，正文里补一句更稳妥，免得 reviewer 跳过。 - 可选（低）：
entry.command在<command>里未做长度截断，而description截到 80、事件行截到 2000。命令通常很短、风险不大；加个上限会更对称/防御。 - 中性： running 通知不带
<command>是有意为之（仅终止带），对循环唤醒语义是正确的——仅备案说明该不对称是刻意的。
建议： ✅ 可以合并。新行为正确、转义到位、被新增测试精确覆盖；面向模型的 guidance 与运行时一致。
🤖 Generated with Claude Code — Claude Opus 4.8 (1M context)
|
@qwen-code /triage |
| - `prompt`: `/loop ${original prompt}` | ||
| - `reason`: a short reason for the chosen delay. | ||
| 5. Briefly tell the user what was done now. If a wakeup was scheduled, include when the next check is expected. If no wakeup was scheduled, say the loop is complete or not continuing. | ||
| - If you started a background agent or a Monitor, it wakes you via a terminal `<task-notification>` on exit, failure, cancellation, or monitor auto-stop — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, or a Monitor may auto-stop on idle or max-events (and one owned by another agent routes its notification only to that agent). Omit LoopWakeup only on the terminal conditions above (complete, blocked, or repeated monitor auto-stop). |
There was a problem hiding this comment.
[Suggestion] This line says "a background agent or a Monitor" but omits background shells (run_shell_command with is_background: true). BackgroundShellRegistry emits the same terminal <task-notification> pattern (verified at backgroundShellRegistry.ts:442-476 with <kind>shell</kind>), so a loop watching a background shell (e.g., npm run build in background) should also set a long fallback heartbeat. The tool description in loop-wakeup.ts uses the broader "a background task you started" which covers all three — consider matching that wording here for consistency.
| - If you started a background agent or a Monitor, it wakes you via a terminal `<task-notification>` on exit, failure, cancellation, or monitor auto-stop — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, or a Monitor may auto-stop on idle or max-events (and one owned by another agent routes its notification only to that agent). Omit LoopWakeup only on the terminal conditions above (complete, blocked, or repeated monitor auto-stop). | |
| - If you started a background task (a background agent, a Monitor, or a background shell), it wakes you via a terminal `<task-notification>` on exit, failure, cancellation, or monitor auto-stop — so set LoopWakeup as a long fallback rather than a short poll. Do not omit it just because something is watching: the work may hang, or a Monitor may auto-stop on idle or max-events (and one owned by another agent routes its notification only to that agent). Omit LoopWakeup only on the terminal conditions above (complete, blocked, or repeated monitor auto-stop). |
— qwen3.7-max via Qwen Code /review
|
|
||
| 1. Do not call CronCreate for this path. | ||
| 2. Run the parsed prompt immediately now. | ||
| 2. If this tick opens with a `<task-notification>` block (a monitor or background event re-invoked you, not a bare `/loop` wakeup prompt), handle that event before re-running the prompt. |
There was a problem hiding this comment.
[Suggestion] After handling a notification in step 2, the model continues to step 3 ("Run the parsed prompt immediately now") unconditionally. But the three sub-cases describe different outcomes:
- Condition met: "finish the loop" — running the prompt again is unnecessary.
- Auto-stopped: monitor restarted, fallback re-armed — running the prompt could launch a duplicate watcher.
- Ambiguous: "investigate on the next tick" — running the prompt now contradicts the deferral.
Consider making the short-circuit explicit, e.g.: "handle that event and end the turn (do not continue to step 3)" or restructuring so steps 3–6 only run when there is no notification.
— qwen3.7-max 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.
LGTM, looks ready to ship. ✅ The guidance is accurate against the runtime, the <command> addition is small and well-escaped, and 80/80 unit tests pass. One non-blocking ask: update the PR body to mention the runtime <command> change (currently reads as guidance-only when it isn't). Optional: consider a length cap on entry.command for symmetry with MAX_DESCRIPTION_LENGTH and EVENT_LINE_TRUNCATE.
3 participants
What this PR does
A self-paced
/looppreviously knew only one way to stay alive — schedule a timer wakeup withLoopWakeup— so a model running a loop that had started a Monitor or a backgrounded agent would tend to set a short wakeup to "check progress." This teaches the loop that such work re-invokes the session on its own (a Monitor or a backgrounded agent emits a<task-notification>when it finishes), so the wakeup should be a long fallback heartbeat (1200–1800s), not a short poll. It also tells the loop to keep that fallback rather than omit it (the work can hang, a Monitor auto-stops on idle or max-events, or it may be owned by another agent that never notifies the loop), and to handle a notification-triggered tick's event before deciding whether to re-arm.Why it's needed
A short poll while a
<task-notification>will already wake the loop the moment the work terminates is wasted work, and a sub-5-minute wakeup also throws away the prompt-cache window for nothing. The substrate already exists —Session.tsandnonInteractiveCli.tswire both the monitor registry and the background-task registry notification callbacks into the prompt/notification drain that re-invokes the session — the loop guidance just never mentioned it, so the model had no reason to prefer the event-driven wake. This is primarily guidance; it also adds a small runtime payload improvement by including an escaped<command>element in terminal Monitor notifications, so a notification-driven loop can identify the monitor that finished. It adds no new notification wiring.Reviewer Test Plan
How to verify
Automated —
npm run test --workspace packages/core -- src/tools/loop-wakeup.test.ts src/skills/bundled/loop/SKILL.test.ts: asserts theLoopWakeuptool/delaySecondsdescription carries both sides of the rule (1200-1800s fallback, 60-270s poll) and that the bundled loopSKILL.mddocuments the monitor/background-task fallback and the "handle that event first" ordering.Reading — the change is model-facing guidance text in
packages/core/src/tools/loop-wakeup.ts(tool +delaySecondsdescriptions) andpackages/core/src/skills/bundled/loop/SKILL.md(self-paced path). A reviewer should confirm the wording is accurate against the wake mechanism: a Monitor wakes the loop only on a terminal<task-notification>(running per-line output is dropped), and only work the loop itself started is delivered to it.Evidence (Before & After)
Before — loop guidance said nothing about monitors/background tasks, so a loop watching work it had backgrounded would schedule short polling wakeups and re-check on a timer. After — the guidance steers the model to set a long fallback heartbeat and rely on the
<task-notification>for the real wake, while keeping the fallback so the loop survives the work auto-stopping or hanging. No TUI/pixel change — model-steering text only, so screenshot Before/After is N/A.Tested on
Environment (optional)
Unit tests only (vitest); no live runtime needed.
Risk & Scope
<command>field on terminal Monitor notifications; no new notification wiring. The risk is that the wording mis-steers the model or the extra payload is malformed; reviewed for accuracy and covered by escaping tests.LoopWakeupdescription strings and bundled loopSKILL.mdbody change, and terminal Monitor notifications include an additive<command>element; no API or wiring change.Linked Issues
Closes #5841
中文说明
这个 PR 做了什么
自定步
/loop此前只会用LoopWakeup排一个定时 wakeup 来续命,所以模型在起了 Monitor 或后台 agent 的循环里会倾向排个短 wakeup 去"看进度"。本改动告诉循环:这类工作会自己 re-invoke 会话(Monitor 或后台 agent 在完成时发<task-notification>),所以 wakeup 应当是长兜底心跳(1200–1800s),而不是短轮询。还告诉循环要保留这个兜底而非省略(工作可能 hang、Monitor 会因 idle/max-events 自停、或被别的 agent 持有而永不通知本循环),并在由通知触发的 tick 里先处理事件再决定是否重排。为什么需要
当
<task-notification>已经会在工作终止的瞬间唤醒循环时,再短轮询就是浪费;而短于 5 分钟的 wakeup 还白白丢掉 prompt-cache 窗口。基建本就存在——Session.ts和nonInteractiveCli.ts已把 monitor registry 与后台任务 registry 的通知回调接进会重新唤起会话的 drain——只是循环 guidance 从没提过,模型没理由去优先用事件驱动唤醒。本改动主要是 guidance；同时在 Monitor 终止通知里加入一个已转义的<command>元素，让通知触发的循环能识别完成的是哪个 monitor。不新增通知接线。Reviewer 验证计划
如何验证
自动——
npm run test --workspace packages/core -- src/tools/loop-wakeup.test.ts src/skills/bundled/loop/SKILL.test.ts:断言LoopWakeup工具/delaySeconds描述同时带上规则两面(1200-1800s 兜底、60-270s 轮询),且 bundled loopSKILL.md记录了 monitor/后台任务兜底与"先处理事件"的次序。阅读——改动是面向模型的 guidance 文本,在
packages/core/src/tools/loop-wakeup.ts(工具 +delaySeconds描述)与packages/core/src/skills/bundled/loop/SKILL.md(自定步路径)。reviewer 应核对措辞是否符合唤醒机制:Monitor 只在终止的<task-notification>唤醒循环(逐行 running 输出被丢弃),且只有循环自己起的工作才会投递给它。证据(Before & After)
Before——循环 guidance 完全没提 monitor/后台任务,所以盯着自己后台化工作的循环会排短轮询 wakeup、靠定时器复查。After——guidance 引导模型排长兜底心跳、把真正唤醒交给
<task-notification>,同时保留兜底以便工作自停/hang 时循环仍存活。无 TUI/像素变化——纯模型引导文本,故 Before/After 截图为 N/A。测试平台
🍏 macOS ✅(单测)· 🪟 Windows⚠️ 未测(CI)· 🐧 Linux ⚠️ 未测(CI)。
运行环境(可选)
仅单元测试(vitest),无需实时运行时。
风险与范围
<command>字段；不新增通知接线。风险在措辞是否误导模型或新增负载是否格式错误；已就准确性审过并用转义测试覆盖。LoopWakeup描述串与 bundled loopSKILL.md正文，并让 Monitor 终止通知包含一个增量<command>元素；无 API 或接线改动。