Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f1879c0
feat(loop): inject a .qwen/loop.md task file at fire time via sentinels
qqqys Jun 26, 2026
7b630fe
fix(cli): remove unused loop mode import
qqqys Jun 26, 2026
f36a5c8
fix(loop): rename loop task-file/tick-resolver modules to kebab-case
qqqys Jun 26, 2026
9b70f16
fix(loop): address re-review suggestions on loop.md injection
qqqys Jun 26, 2026
91759a8
fix(loop): address loop.md re-review — bounded read, cached realpath,…
qqqys Jun 26, 2026
a6da917
fix(loop): address re-review — heading test, absent heading, source l…
qqqys Jun 26, 2026
6c7466d
fix(loop): keep loop.md confinement root internal; harden bounded-rea…
qqqys Jun 26, 2026
0e2128f
Merge branch 'main' into feat/loop-md-injection
wenshao Jun 26, 2026
a278fd0
test(core): cover loop.md project-root realpath cache eviction on tra…
qqqys Jun 26, 2026
89f5ff8
fix(loop): harden loop.md against symlink exfiltration, FIFO hang, an…
qqqys Jun 26, 2026
1ad5362
fix(loop): bracket-access process.env in Session test setFakeHome
qqqys Jun 26, 2026
3e19ba5
fix(loop): fail-secure loop.md default + root confinement, with coverage
qqqys Jun 26, 2026
60edea8
fix(loop): harden loop.md reader/resolver per CI review
qqqys Jun 27, 2026
c0a962a
fix(loop): address CI review on loop.md reader/resolver
qqqys Jun 27, 2026
d0561f0
fix(loop): re-evaluate folder trust per tick; guard durable loop.md i…
qqqys Jun 27, 2026
565398e
fix(loop): honor skipDurableFire in deliverPending missed branch
qqqys Jun 27, 2026
e77e66e
fix(loop): address loop.md injection review (durable guards, logs, QW…
qqqys Jun 27, 2026
d9db495
test(loop): cover all-filtered missed batch and working-dir resolver …
qqqys Jun 27, 2026
f402bae
fix(loop): report real home loop.md path in absent tick; cover resolv…
qqqys Jun 27, 2026
300060c
fix(loop): sanitize loop.md resolve error and guard handle close
qqqys Jun 27, 2026
5acd11a
fix(loop): align skip guards, resolve sentinel pendingRemoval limbo, …
qqqys Jun 28, 2026
eb0f082
fix(loop): never leak an absolute $QWEN_HOME path in the home loop.md…
qqqys Jun 28, 2026
44f45c0
fix(loop): guard UTF-8 truncation boundary and scope resolve-error to…
qqqys Jun 28, 2026
21aebdd
test(loop): lock that one-shot sentinel jobs are not deleted by the h…
qqqys Jun 28, 2026
2f43aeb
fix(loop): guard empty homedir confinement root; interpolate dynamic …
qqqys Jun 28, 2026
75c6c37
fix(loop): reject hard-linked loop.md and keep dynamic loops alive on…
qqqys Jun 28, 2026
9271c43
test(loop): hoist expects out of conditional branches to satisfy vite…
qqqys Jun 28, 2026
54082f0
fix(loop): normalize home label trailing slash; distinguish transient…
qqqys Jun 28, 2026
2116715
fix(loop): only degrade transient fs errors in dynamic mode; log tran…
qqqys Jun 28, 2026
73df2ef
fix(loop): treat EISDIR/ENOTDIR as transient in dynamic mode
qqqys Jun 28, 2026
0198307
test(loop): lock the setSkipDurableFire sentinel-predicate wiring in …
qqqys Jun 28, 2026
4ab4012
fix(loop): keep separator for root QWEN_HOME; distinct heading for tr…
qqqys Jun 28, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1,619 changes: 1,619 additions & 0 deletions packages/cli/src/acp-integration/session/Session.test.ts

Large diffs are not rendered by default.

186 changes: 184 additions & 2 deletions packages/cli/src/acp-integration/session/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* SPDX-License-Identifier: Apache-2.0
*/

import * as os from 'node:os';
import * as path from 'node:path';
import type {
Content,
FunctionCall,
Expand All @@ -28,11 +30,14 @@ import type {
GoalTerminalEvent,
ToolCallRequestInfo,
ToolCallResponseInfo,
LoopTickResult,
} from '@qwen-code/qwen-code-core';
import {
AuthType,
ApprovalMode,
CompressionStatus,
detectLoopSentinel,
LoopTickResolver,
convertToFunctionResponse,
createDuplicateProviderToolCallResponse,
findRepeatedDuplicateProviderToolCall,
Expand Down Expand Up @@ -212,6 +217,23 @@ const MAX_MID_TURN_RESOURCE_TEXT_LENGTH = 100_000;
// conforming-but-busy client, while a client that never answers stops
// costing a stall per tool batch after a few batches.
const MID_TURN_QUEUE_DRAIN_MAX_TIMEOUT_STRIKES = 3;
// fs codes that let a `dynamic` (self-paced) loop treat a THROWN loop.md
// sentinel-resolution as transient — degrade to a no-op re-arm tick so the loop
// survives — instead of re-throwing (which ends it: the firing wakeup is already
// consumed, so only an end-of-turn re-arm keeps it alive). readLoopTaskFile only
// re-throws EACCES/EIO/EBUSY/EPERM (it skips ENOENT/EISDIR/ENOTDIR/ELOOP/… to its
// own `missing` → no-op path); EISDIR/ENOTDIR stay here as defense-in-depth for
// the lstat→open TOCTOU race (path swapped to a dir/non-dir mid-read) should that
// internal skip ever narrow. ENOENT is omitted on purpose: "absent" is not a
// transient read failure and can never reach this catch.
const TRANSIENT_FS_CODES: readonly string[] = [

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] TRANSIENT_FS_CODES is missing 'EISDIR' and 'ENOTDIR'.

readLoopTaskFile performs a pre-open lstat() on the project candidate. If the path is replaced with a directory (or a non-directory) between the lstat and the subsequent fs.open, fs.open throws EISDIR or ENOTDIR. Neither code is in the transient whitelist, so the error propagates and permanently terminates the dynamic loop instead of degrading to a no-op tick.

This is a narrow TOCTOU race but the fix is trivial:

const TRANSIENT_FS_CODES: readonly string[] = [
  'EACCES',
  'EIO',
  'EBUSY',
  'EPERM',
  'ENOENT',
  'EISDIR',
  'ENOTDIR',
];

— qwen3.7-max via Qwen Code /review

'EACCES',
'EIO',
'EBUSY',
'EPERM',
'EISDIR',
'ENOTDIR',
];

type DrainedMidTurnMessage =
| { kind: 'text'; message: string }
Expand Down Expand Up @@ -667,6 +689,13 @@ export class Session implements SessionContext {
private cronQueue: CronQueueItem[] = [];
private cronProcessing = false;
private cronAbortController: AbortController | null = null;
// Resolves the `<<loop.md>>` / `<<loop.md-dynamic>>` sentinels at fire time.
// Lazily created on the first loop tick; its content cache is reset on
// compaction (see #sendMessageStreamWithAutoCompression) and it is rebuilt if
// the working dir changes (e.g. /cd) so it always reads the current project's
// loop.md.
private loopTickResolver: LoopTickResolver | null = null;
private loopTickResolverRoot: string | null = null;
private cronCompletion: Promise<void> | null = null;
private cronDisabledByTokenLimit = false;
private lastPromptTokenCount = 0;
Expand Down Expand Up @@ -1927,6 +1956,10 @@ export class Session implements SessionContext {
compressionInfo = compressed;
this.#recordCompressionTokenCount(compressed);
if (compressed.compressionStatus === CompressionStatus.COMPRESSED) {
// Context was just compacted; a loop.md tick must re-deliver the full

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.

[Critical] This cache reset only covers automatic sends. ACP /compress and /compress-fast call geminiClient.tryCompressChat / tryCompressChatFast directly from the slash-command handlers and return through #processSlashCommandResult, so this Session's loopTickResolver cache survives a successful manual compression. The next unchanged loop tick can then send only a short reminder even though the full block was summarized away. Centralize a compression-completed hook, or reset loopTickResolver after successful ACP compression commands too.

— GPT-5 via Qwen Code /review

// task block (a short reminder refers back to a message that is no
// longer in context).
this.loopTickResolver?.resetCache();

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.

[Critical] When auto-compression triggers during the first turn's send of a loop tick's full block, the ordering is:

  1. resolve() sets #pendingContent = content and returns the full block
  2. #sendMessageStreamWithAutoCompression fires compression → calls resetCache() here, clearing both #lastContent and #pendingContent to null
  3. markDelivered() (line 2565) then sees #pendingContent === null → does nothing
  4. Result: #lastContent stays null, so the next tick sees null !== content and redundantly re-delivers the full 25 KB task block

This is a realistic scenario since accumulated chat + tool screenshots are exactly what triggers compression in long-running sessions.

Suggested fix: change resetCache() to only clear #lastContent while leaving #pendingContent intact, so markDelivered() can still commit it. Or move markDelivered() to before the send call.

— 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] resetCache() after compaction is not tested at the Session level

The LoopTickResolver.resetCache() method is tested in isolation (loop-tick-resolver.test.ts), but the Session-level wiring — that compaction actually calls resetCache() so the next tick re-delivers the full task block — has no test.

If this call were removed, a post-compaction tick would send a dangling short reminder pointing to a task block that was just evicted from context.

Suggested: add a Session test that (1) fires a loop.md tick (full delivery), (2) triggers compaction, (3) fires the sentinel again, (4) asserts the second tick is also full: true.

const reasonClause =
compressed.triggerReason === 'image_overflow'
? `accumulated enough tool screenshots to trigger compaction for ${this.config.getModel()}`
Expand Down Expand Up @@ -2445,6 +2478,44 @@ export class Session implements SessionContext {
}
}

#getLoopTickResolver(): LoopTickResolver {

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] Several Session-level integration paths introduced by this PR lack tests:

  1. #getLoopTickResolver() working-dir-change rebuild (lines 2445–2456) — no test verifies the resolver is rebuilt and re-reads from the new project after /cd.
  2. markDelivered() timing at line 2565 — no test fires two consecutive sentinel ticks to verify that the first delivers full and the second delivers a short reminder, or that an abort before turnCount === 1 leaves the cache uncommitted.
  3. Compaction → resetCache() at line 1943 — no Session test triggers compaction during a loop tick and verifies the next tick re-delivers full.
  4. The <<loop.md>> (cron) sentinel — only <<loop.md-dynamic>> is tested at the Session level. The cron sentinel produces different reminder text ("do not call LoopWakeup") that should be verified.

The unit tests in loopTickResolver.test.ts cover these in isolation, but the Session wiring that connects them to the cron processing loop is untested.

— 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] No Session-level test exercises the working-dir-change rebuild path here. All existing Session loop tests use a single fixed getWorkingDir() mock, so the this.loopTickResolverRoot !== root trigger is never exercised through the integration layer.

A regression that drops the loopTickResolverRoot tracking field would silently keep reading the old project's .qwen/loop.md after a /cd, feeding stale tasks from a previous workspace.

Test needed: Fire two loop ticks with getWorkingDir() returning different dirs between them (each with a distinct loop.md). Assert the second tick delivers the NEW project's full block.

— qwen3.7-max via Qwen Code /review

const root = this.config.getWorkingDir();
// Rebuild if the working dir changed (e.g. /cd) so loop.md resolves against
// the current project; a fresh resolver also correctly re-delivers full.
if (!this.loopTickResolver || this.loopTickResolverRoot !== root) {
// Resolve the home/global loop.md from the QWEN_HOME-aware global dir (the
// rest of Qwen honors QWEN_HOME for `.qwen`); reading raw os.homedir() here
// would always hit the real `~/.qwen` and ignore a relocated config home.
const homeQwenDir = Storage.getGlobalQwenDir();
// Confinement root for the home candidate's resolved target: $QWEN_HOME
// when set (it IS the global dir), else $HOME — keeps the earlier
// confinement (an in-root dotfile symlink resolves; an escape is refused).
// The `|| path.dirname(homeQwenDir)` guards an empty os.homedir() (minimal
// containers with no HOME): an empty root makes isWithin('', target) always
// true, trivially bypassing the symlink confinement. homeQwenDir
// (Storage.getGlobalQwenDir()) is always non-empty, so its parent is a
// sound non-empty fallback root.
const homeConfineRoot =
(process.env['QWEN_HOME'] ? homeQwenDir : os.homedir()) ||
path.dirname(homeQwenDir);
this.loopTickResolver = new LoopTickResolver({

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.

[Critical] LoopTickResolver is constructed from the current working directory unconditionally, so even when folder trust is enabled and this folder is untrusted, readLoopTaskFile still consumes project-controlled .qwen/loop.md. Other project-controlled surfaces such as workspace hooks, saved workflows, and MCP discovery are skipped in untrusted folders; this path should apply the same boundary. Please skip the project candidate when !config.isTrustedFolder() and allow only the home candidate, or pass an explicit trust flag into the reader.

— GPT-5 via Qwen Code /review

projectRoot: root,
homeDir: homeConfineRoot,

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.

[Medium] Home confinement scope is $HOME, not $HOME/.qwen — symlinks to ~/.ssh/id_rsa pass the check.

When QWEN_HOME is not set (the common case), homeConfineRoot is os.homedir() — the entire home directory. A symlink ~/.qwen/loop.md -> ~/.ssh/id_rsa resolves within $HOME and passes isWithin. The SSH private key would then be read as UTF-8 and delivered to the model as authoritative task instructions on every loop tick.

The home candidate is always read regardless of workspace trust, making this exploitable without any trust-gate bypass. An attacker who can create or replace ~/.qwen/loop.md (malicious dotfiles installer, compromised npm postinstall, shared machine) can exfiltrate any file under $HOME.

Trade-off: Narrowing confinement to homeQwenDir ($HOME/.qwen) would also block legitimate dotfile-manager symlinks like ~/.qwen/loop.md -> ~/dotfiles/qwen/loop.md (the target would escape $HOME/.qwen). Worth a deliberate design decision — perhaps document the intentional scope and add a warning in the absent reminder when the home candidate's symlink target is outside $HOME/.qwen.

homeQwenDir,
// The project `.qwen/loop.md` is repo-controlled, so an untrusted folder
// must not read it and feed it to the model (mirrors getProjectHooks()'s
// trust gate). The home/global `~/.qwen/loop.md` is user-owned and stays
// allowed. Pass a getter, not a snapshot: isTrustedFolder() can flip
// mid-session on an IDE workspace-trust update, and the resolver outlives
// a single tick — re-read it on every resolve() so a trusted→untrusted
// flip stops reading the project file immediately.
allowProjectFile: () => this.config.isTrustedFolder(),
});
this.loopTickResolverRoot = root;
}
return this.loopTickResolver;
}

/**
* Executes a single cron-fired prompt: echoes it as a user message with
* `_meta.source='cron'`, streams the model response, and handles tool calls.
Expand Down Expand Up @@ -2478,10 +2549,114 @@ export class Session implements SessionContext {
async () => {
let turnCount = 0;
try {
// A `<<loop.md>>` / `<<loop.md-dynamic>>` sentinel is expanded at
// fire time into the loop.md task block — full on the first or a
// changed fire, a short reminder when unchanged. Non-sentinel
// prompts pass through untouched.
const loopMode = detectLoopSentinel(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.

[Critical] This expansion happens after the scheduled prompt has already been approved/classified as the harmless literal sentinel. Because loop.md is reread at fire time and can change after scheduling, a project/home file edit can replace an approved sentinel with new model instructions that execute with cron/loop tool access without going through the same future-prompt scrutiny. This is separate from prompt framing: even delimiters would not re-approve changed file contents. Run the resolved full block through the same approval/classifier path whenever it is first delivered or changes, or require explicit user confirmation before sending changed loop.md contents.

— GPT-5 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] loopMode is derived only from the sentinel string, but the authoritative schedule type is already in scope as item.source ('loop' for a @wakeup/LoopWakeup job, 'cron' for a recurring CronCreate — set at line 2413). The re-arm guidance the model receives is selected solely by the sentinel, so the two can silently disagree:

  • A @wakeup job (item.source === 'loop', one-shot) whose prompt is <<loop.md>>mode = 'cron' → the reminder says "the recurring cron fires the next tick automatically — do not call LoopWakeup". Nothing re-fires, so the self-paced loop dies silently after one tick.
  • A recurring cron (item.source === 'cron') whose prompt is <<loop.md-dynamic>>mode = 'dynamic' → the reminder tells the model to re-arm LoopWakeup, which then double-fires on top of the auto-firing cron.

The re-arm instruction is about the firing mechanism (does this job auto-repeat?), which item.source already knows — not the model's sentinel choice. SKILL.md pairs them correctly, but nothing enforces it and a mis-pairing fails with no diagnostic. (This is distinct from the already-noted literal-collision concern on detectLoopSentinel.) Derive the mode from item.source (or assert it agrees with the sentinel):

Suggested change
const loopMode = detectLoopSentinel(prompt);
const loopMode =
detectLoopSentinel(prompt) === null
? null
: item.source === 'loop'
? 'dynamic'
: 'cron';

— claude-opus-4-8 via Qwen Code /qreview

let loopTick: LoopTickResult | null = null;
if (loopMode) {
const resolver = this.#getLoopTickResolver();
// Capture folder-trust ONCE for this tick and thread it through
// both the resolve probe and the error path. isTrustedFolder()
// can flip mid-tick (an IDE workspace-trust update), so two
// separate reads could let the sanitized error name a different
// candidate set than resolve() actually probed.
const trustedAtResolve = this.config.isTrustedFolder();
try {
loopTick = await resolver.resolve(loopMode, trustedAtResolve);
} catch (resolveErr) {
// resolve() reads .qwen/loop.md (project or home/global); an
// EACCES/EIO here is a sentinel-RESOLUTION failure, not a
// model-call failure — tag it so the two are distinguishable
// in logs.
const code =
(resolveErr as NodeJS.ErrnoException).code ?? 'unknown';
// Full detail — including the raw fs error's ABSOLUTE loop.md
// path (OS username + dir layout) — stays in this LOCAL debug
// log only; debug logs are never sent to the ACP client.
debugLogger.warn(
`loop.md sentinel resolution failed (mode=${loopMode}, code=${code}) — check .qwen/loop.md permissions/IO`,
resolveErr,
);
if (
loopMode === 'dynamic' &&
TRANSIENT_FS_CODES.includes(code)
) {
// A `dynamic` (self-paced) loop is kept alive ONLY by the
// model re-arming LoopWakeup at the end of each turn; the
// firing wakeup was already consumed, so throwing here (no
// turn → no re-arm) would silently kill the loop forever on a
// transient hiccup (EACCES/EIO, or a Windows editor/AV briefly

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 dynamic branch catches ALL errors from resolver.resolve() and unconditionally degrades to buildTransientErrorTick. If resolve() throws a non-fs error (TypeError, programming bug, assertion failure), code becomes 'unknown', the model gets "could not be read this tick (unknown)" with re-arm instructions, and the loop enters an infinite degraded cycle — every tick is a no-op but the loop never dies, and no error ever surfaces.

Consider filtering to known fs error codes and re-throwing unexpected errors:

Suggested change
// transient hiccup (EACCES/EIO, or a Windows editor/AV briefly
if (loopMode === 'dynamic' && ['EACCES', 'EIO', 'EBUSY', 'EPERM', 'ENOENT'].includes(code)) {
// A `dynamic` (self-paced) loop is kept alive ONLY by the
// model re-arming LoopWakeup at the end of each turn; the
// firing wakeup was already consumed, so throwing here (no
// turn → no re-arm) would silently kill the loop forever on a
// transient hiccup (EACCES/EIO, or a Windows editor/AV briefly
// locking the file). Degrade to a no-op tick mirroring the
// absent path so the model still re-arms and the loop survives.
// (`cron` re-fires on its own next interval, so it still
// throws below.) The captured trust names the SAME candidate
// set the probe used; the errno (no absolute path) is noted.
loopTick = resolver.buildTransientErrorTick(
loopMode,
trustedAtResolve,
code,
);
} else {

— qwen3.7-max via Qwen Code /review

// locking the file). Degrade to a no-op tick mirroring the
// absent path so the model still re-arms and the loop survives.
// (`cron` re-fires on its own next interval, so it still
// throws below.) The captured trust names the SAME candidate
// set the probe used; the errno (no absolute path) is noted.
// Only KNOWN-transient codes degrade: an unexpected error
// (TypeError / assertion → code 'unknown') falls through to the
// throw so the real bug surfaces instead of an infinite no-op
// cycle.
loopTick = resolver.buildTransientErrorTick(
loopMode,
trustedAtResolve,
code,
);
} else {
// Reached by `cron` (re-fires on its own next interval) and by
// `dynamic` with an UNEXPECTED (non-transient) error — both
// surface rather than silently degrade. Re-throw a SANITIZED
// error: the outer catch forwards error.message verbatim to the
// client via emitAgentMessage,
// so re-throwing the raw fs error would leak that absolute
// path. Surface only the candidate labels + errno code via the
// shared absentLocations() — reusing the QWEN_HOME-aware home
// label (never a hardcoded `~/.qwen`) and naming the project
// candidate only when it was actually read (the captured trust
// matches the resolve() probe, so an untrusted folder can't
// falsely claim `(project)`).
throw new Error(
`loop.md resolution failed (${code}) for ${resolver.absentLocations(
trustedAtResolve,
)}`,
);
}
}
}
const modelText = loopTick ? loopTick.modelText : 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.

[Critical] modelText is built before #sendMessageStreamWithAutoCompression can compact. On an unchanged tick, resolve() returns only the short reminder; if compression happens in the send path, resetCache() runs but the already-built short reminder is still sent into the new compressed context that no longer contains the full task block. The next tick will recover, but this tick silently loses the task state. Run compression before resolving the sentinel, or have the send path report COMPRESSED so the cron path can reset and re-resolve before sending.

— GPT-5 via Qwen Code /review

if (loopTick) {
debugLogger.debug(

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.

[Critical] Absolute filesystem path leaks to the ACP client via the error propagation channel.

throw resolveErr re-throws the raw Node.js fs error (e.g., EACCES: permission denied, open '/home/alice/project/.qwen/loop.md'). The outer cron catch at line 2697 extracts error.message verbatim and sends it to the ACP client via emitAgentMessage(\[${item.source} error] ${msg}`). This leaks the OS username and directory layout through a channel the rest of this PR carefully sanitizes (echoTextuses relative labels,sourceLabel` uses relative labels, debug logs avoid paths).

The inner catch's debugLogger.warn correctly avoids paths (logs only mode + code), but the re-thrown error reaches the outer catch untouched. The existing Session test constructs a mock error with a relative-path message, which does not match what Node.js actually produces in production.

Suggested change
debugLogger.debug(
throw new Error(
`loop.md resolution failed (mode=${loopMode}, code=${
(resolveErr as NodeJS.ErrnoException).code ?? 'unknown'
})`,
);

This preserves the diagnostic code while stripping the embedded absolute path.

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 debug log here reports delivery=absent for transient errors (where loopTick exists but loopTick.kind === 'transient_error'). Since the resolver did produce a result (just an error one), this is misleading. Consider logging delivery=error or delivery=transient instead.

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 tick delivery debug log cannot distinguish a transient-error tick from a genuinely-absent file. Both buildTransientErrorTick and the genuine-absent path produce full: false and sourceLabel: undefined, so the log prints delivery=absent source=none for both cases. The transientError flag on the result is never consulted in the log expression. An oncall engineer tailing debug logs would see delivery=absent and conclude "the file doesn't exist," when the real situation is "the file exists but can't be read."

Add a transient field so a single log line is self-explanatory:

Suggested change
debugLogger.debug(
debugLogger.debug(
`loop tick: mode=${loopMode} delivery=${
loopTick.full
? 'full'
: loopTick.sourceLabel
? 'reminder'
: 'absent'
} source=${loopTick.sourceLabel ?? 'none'} transient=${loopTick.transientError ?? false}`,
);

— qwen3.7-max via Qwen Code /review

`loop tick: mode=${loopMode} delivery=${
loopTick.full
? 'full'
: loopTick.sourceLabel
? 'reminder'
: 'absent'
} source=${loopTick.sourceLabel ?? 'none'} transient=${
loopTick.transientError ?? false
}`,
);
}
// For a loop tick echo a stable, relative label — never the bare
// sentinel or the full task dump (and the resolver never hands back
// the absolute path, which would leak the OS username / dir layout
// into the ACP client UI); otherwise echo the prompt verbatim.
const echoText = loopTick

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] Three-level nested ternary is hard to parse at a glance. Consider extracting to if/else:

Suggested change
const echoText = loopTick
let echoText = prompt;
if (loopTick) {
echoText = loopTick.sourcePath
? `Loop tick — tasks from ${loopTick.sourcePath}`
: 'Loop tick — loop.md not present';
}

— qwen3.7-max via Qwen Code /review

? loopTick.sourceLabel
? `Loop tick — tasks from ${loopTick.sourceLabel}`

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 loopTick exists but has no sourceLabel, the echo says "loop.md not present" — but this branch also fires for transient-error ticks produced by buildTransientErrorTick, which do have a resolved file but deliberately omit sourceLabel.

The user would see "Loop tick — loop.md not present" when in reality the file exists but there was a temporary read error. This is misleading.

Consider distinguishing the two cases:

: loopTick.sourceLabel
  ? `Loop tick — tasks from ${loopTick.sourceLabel}`
  : loopTick.error
    ? 'Loop tick — loop.md temporarily unavailable'
    : 'Loop tick — loop.md not present'

Or, have buildTransientErrorTick set a sentinel sourceLabel (e.g. 'error') so this ternary doesn't conflate "absent" with "error".

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 loopTick.sourcePath is set but loopTick.kind === 'transient_error', the echoText here reads "loop.md not present, skipping". This is misleading — the file is present but had a read error (EACCES, EIO, etc.). Consider changing the transient-error branch to say something like "loop.md could not be read, skipping" so the user can distinguish absence from error.

: // A transient-error tick (buildTransientErrorTick) resolved a
// file but couldn't read it this tick; it deliberately omits
// sourceLabel, so don't conflate it with a genuinely-absent
// loop.md. No errno/path here — those stay in the model text.
loopTick.transientError
? 'Loop tick — loop.md temporarily unavailable'
: 'Loop tick — loop.md not present'
: prompt;

// Echo the cron prompt as a user message so the client sees it
await this.sendUpdate({
sessionUpdate: 'user_message_chunk',
content: { type: 'text', text: prompt },
content: { type: 'text', text: echoText },
_meta: { source: item.source },
});

Expand All @@ -2490,7 +2665,7 @@ export class Session implements SessionContext {
const cronReminders = await this.#buildInitialSystemReminders();
let nextMessage: Content | null = {
role: 'user',
parts: [...cronReminders, { text: prompt }],
parts: [...cronReminders, { text: modelText }],
};

while (nextMessage !== null) {
Expand Down Expand Up @@ -2519,6 +2694,13 @@ export class Session implements SessionContext {
return;
}
const responseStream = sendResult.responseStream;
if (loopTick && turnCount === 1) {
// The block reached the model (the send started); commit it so
// the next tick can detect "unchanged". Deferring the commit
// to here keeps an abort before delivery from poisoning the
// cache into a dangling short reminder.
this.loopTickResolver?.markDelivered();

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] markDelivered() fires here immediately after the stream object is returned, before the for await loop actually consumes any bytes. If the stream fails mid-iteration (network drop, API timeout, provider error), the content is committed to #lastContent but the model may have only partially received it. Subsequent ticks would get a SHORT_REMINDER referencing "the loop.md contents established earlier" — context the model never actually received.

Consider deferring markDelivered() until after the first successful response chunk is consumed, or adding a completion flag that the for await loop sets.

— 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] markDelivered() + resetCache() ordering trap: when auto-compression fires during the first turn of a loop tick (inside #sendMessageStreamWithAutoCompression at line 1943), resetCache() clears both #pendingContent and #lastContent. Then this markDelivered() call finds #pendingContent null and commits nothing — even though the full task block successfully reached the model. Every subsequent tick re-delivers the full block (instead of the cheap one-line reminder) until the file content actually changes, silently burning tokens on cron-mode loops.

Consider having resetCache() promote #pendingContent to #lastContent (rather than discarding both), so that post-compression ticks correctly see "unchanged" content. Alternatively, add a comment at both call sites warning that the ordering is load-bearing.

— qwen3.7-max via Qwen Code /review

}
nextMessage = null;

for await (const resp of responseStream) {
Expand Down
112 changes: 111 additions & 1 deletion packages/cli/src/nonInteractiveCli.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import type {
Config,
CronJob,
ToolRegistry,
ServerGeminiStreamEvent,
SessionMetrics,
Expand All @@ -22,9 +23,15 @@ import {
ApprovalMode,
SendMessageType,
LoopType,
CronScheduler,
LOOP_SENTINEL_CRON,
LOOP_SENTINEL_DYNAMIC,
} from '@qwen-code/qwen-code-core';
import type { Part } from '@google/genai';
import { runNonInteractive } from './nonInteractiveCli.js';
import {
runNonInteractive,
skipHeadlessLoopSentinel,
} from './nonInteractiveCli.js';
import { vi, type Mock, type MockInstance } from 'vitest';
import * as fs from 'node:fs/promises';
import * as os from 'node:os';
Expand Down Expand Up @@ -73,6 +80,73 @@ vi.mock('./services/CommandService.js', () => ({
},
}));

describe('skipHeadlessLoopSentinel', () => {
it('deletes a recurring session loop.md sentinel job so sessionSize reaches 0', () => {
// A recurring SESSION (non-durable) loop.md job left in the scheduler keeps
// sessionSize > 0, so the headless hold-open never resolves and the run
// hangs. Skipping the sentinel must delete the job, not just no-op the tick.
const scheduler = new CronScheduler();
const job = scheduler.create('*/5 * * * *', LOOP_SENTINEL_CRON, true);

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] All three skipHeadlessLoopSentinel tests use LOOP_SENTINEL_CRON only. LOOP_SENTINEL_DYNAMIC (<<loop.md-dynamic>>) is never passed to skipHeadlessLoopSentinel in any test. If the function regressed to only recognize the cron sentinel (e.g., a direct === LOOP_SENTINEL_CRON comparison instead of calling detectLoopSentinel), these tests would not catch it — the dynamic sentinel job would remain in the scheduler and pin the headless run open.

Add a mirror test using LOOP_SENTINEL_DYNAMIC:

it('deletes a recurring session dynamic-sentinel job', () => {
  const scheduler = new CronScheduler();
  const job = scheduler.create('*/5 * * * *', LOOP_SENTINEL_DYNAMIC, true);
  expect(scheduler.sessionSize).toBe(1);
  expect(skipHeadlessLoopSentinel(scheduler, job)).toBe(true);
  expect(scheduler.sessionSize).toBe(0);
});

— 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] All four skipHeadlessLoopSentinel tests create jobs with recurring: true. No test exercises a sentinel-prompt job with recurring: false. The production guard at nonInteractiveCli.ts:168 is job.recurring && !job.durable — the deletion branch requires BOTH conditions. If someone simplifies the guard to !job.durable, a one-shot sentinel job would be incorrectly deleted, and no test would catch the regression.

Suggested change
const job = scheduler.create('*/5 * * * *', LOOP_SENTINEL_CRON, true);
it('does not delete a non-recurring sentinel job (one-shot stays in the scheduler)', () => {
const scheduler = new CronScheduler();
const job = scheduler.create('*/5 * * * *', LOOP_SENTINEL_CRON, false);
const deleteSpy = vi.spyOn(scheduler, 'delete');
expect(skipHeadlessLoopSentinel(scheduler, job)).toBe(true);
expect(deleteSpy).not.toHaveBeenCalled();
});

— qwen3.7-max via Qwen Code /review

expect(scheduler.sessionSize).toBe(1);

expect(skipHeadlessLoopSentinel(scheduler, job)).toBe(true);

expect(scheduler.sessionSize).toBe(0);
expect(scheduler.list()).toHaveLength(0);
});

it('also cleans up a recurring session job for the dynamic sentinel', () => {
// Mirror of the cron case for `<<loop.md-dynamic>>`. skipHeadlessLoopSentinel
// must route through detectLoopSentinel (which matches BOTH sentinels), not a
// `=== LOOP_SENTINEL_CRON` comparison — otherwise a dynamic loop.md job would
// pin sessionSize > 0 and hang the headless run.
const scheduler = new CronScheduler();
const job = scheduler.create('*/5 * * * *', LOOP_SENTINEL_DYNAMIC, true);
expect(scheduler.sessionSize).toBe(1);

expect(skipHeadlessLoopSentinel(scheduler, job)).toBe(true);

expect(scheduler.sessionSize).toBe(0);
expect(scheduler.list()).toHaveLength(0);
});

it('returns false and keeps a non-sentinel job', () => {
const scheduler = new CronScheduler();
scheduler.create('*/5 * * * *', 'do real work', true);
const job = scheduler.list()[0] as CronJob;

expect(skipHeadlessLoopSentinel(scheduler, job)).toBe(false);

expect(scheduler.sessionSize).toBe(1);
});

it('does not delete a durable sentinel job (it persists for a future session)', () => {
// Durable jobs live under ~/.qwen and never count toward sessionSize, so
// they don't pin the run; deleting one would wrongly remove it from disk.
const scheduler = new CronScheduler();
const job = scheduler.create('*/5 * * * *', LOOP_SENTINEL_CRON, true);
job.durable = true;
const deleteSpy = vi.spyOn(scheduler, 'delete');

expect(skipHeadlessLoopSentinel(scheduler, job)).toBe(true);

expect(deleteSpy).not.toHaveBeenCalled();
});

it('does not delete a non-recurring sentinel job (one-shot stays in the scheduler)', () => {
// The deletion branch requires BOTH `recurring && !durable`. A one-shot
// sentinel job is already removed by the scheduler before it fires, so this
// guard must NOT delete it — a `!durable`-only guard would wrongly evict it.
const scheduler = new CronScheduler();
const job = scheduler.create('*/5 * * * *', LOOP_SENTINEL_CRON, false);
const deleteSpy = vi.spyOn(scheduler, 'delete');

expect(skipHeadlessLoopSentinel(scheduler, job)).toBe(true);

expect(deleteSpy).not.toHaveBeenCalled();
});
});

describe('runNonInteractive', () => {
let mockConfig: Config;
let mockSettings: LoadedSettings;
Expand Down Expand Up @@ -3137,6 +3211,42 @@ describe('runNonInteractive', () => {
);
});

it('installs a skipDurableFire predicate that classifies loop.md sentinels in headless mode', async () => {
// Locks the wiring at the scheduler-enable site: runNonInteractive must
// hand the scheduler a predicate that skips durable loop.md sentinels
// (which a headless run can't expand), while still letting non-sentinel
// durable jobs fire. Both halves are covered alone — detectLoopSentinel via
// skipHeadlessLoopSentinel above, the filter via cronScheduler tests — but
// nothing pins that runNonInteractive actually connects them. A refactor
// dropping or rewriting this call would otherwise silently fire raw
// `<<loop.md>>` sentinels at the model (or skip real durable jobs), uncaught.
setupMetricsMock();
// Real scheduler with no projectRoot: enableDurable() short-circuits (no
// filesystem/lock work) and, with no jobs, the headless cron hold-open
// resolves immediately, so runNonInteractive returns without hanging.
const scheduler = new CronScheduler();
const skipSpy = vi.spyOn(scheduler, 'setSkipDurableFire');
mockConfig.isCronEnabled = vi.fn().mockReturnValue(true);
mockConfig.getCronScheduler = vi.fn().mockReturnValue(scheduler);
mockGeminiClient.sendMessageStream.mockReturnValue(
createStreamFromEvents([
{ type: GeminiEventType.Content, value: 'ok' },
{
type: GeminiEventType.Finished,
value: { reason: undefined, usageMetadata: { totalTokenCount: 1 } },
},
]),
);

await runNonInteractive(mockConfig, mockSettings, 'test', 'p-cron-wiring');

expect(skipSpy).toHaveBeenCalledOnce();
const predicate = skipSpy.mock.calls[0][0];
expect(predicate({ prompt: LOOP_SENTINEL_CRON } as CronJob)).toBe(true);
expect(predicate({ prompt: LOOP_SENTINEL_DYNAMIC } as CronJob)).toBe(true);
expect(predicate({ prompt: 'regular cron job' } as CronJob)).toBe(false);
});

describe('--json-schema structured output', () => {
// Helper: walk an emitted event and extract the first tool_use_id when
// it represents a tool_result block. Returns undefined for any other
Expand Down
Loading
Loading