Skip to content
Merged
5 changes: 4 additions & 1 deletion packages/core/src/services/monitorRegistry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ describe('MonitorRegistry', () => {
it('completes a monitor and emits terminal notification', () => {
const callback = vi.fn();
registry.setNotificationCallback(callback);
registry.register(createEntry());
registry.register(createEntry({ command: 'grep "a&b" < /dev/null' }));

registry.complete('mon-1', 0);

Expand All @@ -290,6 +290,9 @@ describe('MonitorRegistry', () => {
const [displayText, modelText] = callback.mock.calls[0] as [string, string];
expect(displayText).toContain('completed');
expect(modelText).toContain('<status>completed</status>');
expect(modelText).toContain(
'<command>grep &quot;a&amp;b&quot; &lt; /dev/null</command>',
);
expect(modelText).toContain('Exited with code 0');
});

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/services/monitorRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@ export class MonitorRegistry {
`<status>${escapeXml(entry.status)}</status>`,
`<event-count>${entry.eventCount}</event-count>`,
`<summary>Monitor "${escapeXml(desc)}" ${statusText}. Total events: ${entry.eventCount}.${entry.droppedLines > 0 ? ` ${entry.droppedLines} lines dropped due to throttling.` : ''}</summary>`,
`<command>${escapeXml(stripDisplayControlChars(entry.command))}</command>`,
);
if (detail) {
xmlParts.push(
Expand Down
19 changes: 12 additions & 7 deletions packages/core/src/skills/bundled/loop/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,19 +43,24 @@ Examples:
Use this path only when the user supplied a prompt and no interval.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

- If the notification says the watched condition was met, cancel any pending fallback LoopWakeup with CronDelete if you still have its ID, then 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

3. Run the parsed prompt immediately now.
- If it is a slash command, invoke it via the Skill tool.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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).

Suggested change
- 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

- Otherwise, act on it directly.
3. Before ending the turn, decide whether another check is useful.
4. Before ending the turn, decide whether another check is useful.
- Call LoopWakeup only if continued follow-up is useful.
- 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.
4. 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).
- `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).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
- 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

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}` plus any state the next tick must preserve, such as `monitor restarted 1/1 time`.
- `reason`: a short reason for the chosen delay. Include the monitor restart count here when re-arming after an auto-stop.
6. 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 because a notification ended the loop, mention whether the stale fallback was cancelled; if the wakeup ID was lost, ignore or answer the stale wakeup briefly when it fires.

## Fixed-interval recurring path

Expand Down
32 changes: 32 additions & 0 deletions packages/core/src/skills/bundled/loop/SKILL.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,38 @@ describe('bundled loop skill', () => {
expect(body).not.toContain('delayMinutes');
});

it('teaches the self-paced loop to lean on monitor/background-task notifications', () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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

const { body } = loadLoopSkill();

expect(body).toContain('<task-notification>');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

expect(body).toContain('set LoopWakeup as a long fallback');
expect(body).toContain('auto-stop 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('cancel any pending fallback LoopWakeup');
expect(body).toContain('CronDelete');
expect(body).toContain('If a monitor auto-stopped');
expect(body).toContain('restart it once');
expect(body).toContain('monitor restarted 1/1 time');
expect(body).toContain('so it survives context compaction');
expect(body).toContain('report the repeated auto-stop to the user');
expect(body).toContain('report the restart count');
expect(body).toContain('If the signal is ambiguous');
expect(body).toContain('three consecutive ticks');
expect(body).toContain('Do not omit it just because something is watching');
expect(body).toContain('the work may hang');
expect(body).toContain(
'one owned by another agent routes its notification only to that agent',
);
expect(body).toContain('repeated monitor auto-stop');
expect(body).toContain('not a bare `/loop` wakeup prompt');
expect(body).toContain('stale fallback was cancelled');
});

it('keeps fixed-interval inputs on the recurring cron path', () => {
const { body } = loadLoopSkill();

Expand Down
16 changes: 16 additions & 0 deletions packages/core/src/tools/loop-wakeup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ describe('LoopWakeupTool', () => {
expect(tool.name).toBe('loop_wakeup');
});

it('documents the fallback-heartbeat semantics for monitor/background work', () => {
expect(tool.description).toContain('fallback heartbeat');
expect(tool.description).toContain('<task-notification>');
expect(tool.description).toContain('terminal `<task-notification>`');
expect(tool.description).not.toContain('per stdout line');
const params = tool.schema.parametersJsonSchema as {
properties: { delaySeconds: { description: string } };
};
const delay = params.properties.delaySeconds.description;
// Both sides of the rule: long fallback when something else wakes you,
// short poll only when you are the sole watcher.
expect(delay).toContain('1200-1800s');
expect(delay).toContain('60-270s');
expect(delay).toContain('Monitor');
});

it('uses ask permission because it schedules future model input', async () => {
const invocation = tool.build({
delaySeconds: 300,
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/tools/loop-wakeup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,14 @@ export class LoopWakeupTool extends BaseDeclarativeTool<
super(
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 sends a terminal `<task-notification>` on exit, failure, cancellation, or monitor auto-stop — keep this wakeup as a long fallback heartbeat rather than a poll; see `delaySeconds`.',
Kind.Other,
{
type: 'object',
properties: {
delaySeconds: {
type: 'number',
description: `Seconds from now to wake up. Clamped to [${WAKEUP_MIN_SECONDS}, ${WAKEUP_MAX_SECONDS}]. Prefer 60-270s for fast-changing state, 1200s+ when there is no reason to check sooner.`,
description: `Seconds from now to wake up. Clamped to [${WAKEUP_MIN_SECONDS}, ${WAKEUP_MAX_SECONDS}]. Use 60-270s only when actively polling external state that nothing else reports (a CI run, a remote queue) — staying inside the ~5-min prompt-cache window. When a background task you started will wake you via a \`<task-notification>\` once it finishes, that is the real wake signal — use 1200-1800s here as a fallback for when it never arrives (the task hangs, a Monitor auto-stops on idle or max-events, or another agent owns it). With no specific signal to watch, default to 1200s+.`,
},
prompt: {
type: 'string',
Expand Down
Loading