Skip to content

feat(scheduled-tasks): gate an isolated run behind a precondition#6619

Merged
wenshao merged 8 commits into
QwenLM:mainfrom
wenshao:feat/scheduled-task-precondition
Jul 10, 2026
Merged

feat(scheduled-tasks): gate an isolated run behind a precondition#6619
wenshao merged 8 commits into
QwenLM:mainfrom
wenshao:feat/scheduled-task-precondition

Conversation

@wenshao

@wenshao wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

An isolated scheduled task can now carry an optional precondition alongside its prompt. On every fire the task's bound session evaluates the precondition as an ordinary cron turn, and only dispatches the prompt into a fresh sub-session when the verdict is DECISION: YES.

fire → evaluate condition in the task's own session (tools, workspace approval mode)
        ├─ YES  → spawn a fresh sub-session, run the prompt there
        └─ else → skip this run

Why the check runs in the bound session

Not in a throwaway sub-session, deliberately:

  • It has exactly the semantics of a shared fire — same tools, same approval mode — so it introduces no new permission surface.
  • The bound session of an isolated task is otherwise empty, so its transcript becomes the task's decision log: the record of why a fire did or did not happen.
  • No session is minted for a run that never occurs.

Everything that is not a YES skips the fire: NO, an unparseable answer, a tool-loop error, a cancelled or timed-out turn. A precondition exists to withhold an unattended run, so an ambiguous answer must withhold it too.

A missed fire is never judged: that job is the scheduler's synthetic carrier — one batched notification covering every one-shot missed in this load, whose prompt is a notice rather than any task's command. Otherwise a precondition changes whether a fire runs, never how. The Web Shell's Run now does not evaluate the condition: the verdict is only observable inside the session that computes it, so a client-side guard could never learn its own outcome (see round 4). Run now means run now, and a guarded task's tooltip says so.

UI

The field appears only for isolated (the daemon rejects a condition on a shared task), and the guard is shown on the task card.

Precondition field

Task card

Verification

1. A live end-to-end run against a real qwen serve daemon

Driven against a real daemon (cron * * * * *) with a scripted mock provider, so the model's output is deterministic while the scheduler, the tool loop, the dispatch and the filesystem effects are all real. The condition asks whether a release-flag file exists; the model answers by actually running test -f … through run_shell_command and reading the result. The screenshot below is the captured terminal output of that run.

Live run

The task's own session after the two fires — this is the decision log the design is built around:

Decision log

The sub-session spawned by the second fire. It starts from zero context: the precondition is nowhere in its transcript, only the command.

Sub-session

2. The tests

51 new tests across the six layers (core file format, scheduler, ACP session, REST route, Web Shell dialog).

Test suites

3. Every test checked against a source mutation

A passing test proves nothing until you have seen it fail for its own reason. Each mutation below was applied to the real source, the suite re-run, and the source restored — 33 mutations, 33 killed.

Mutation testing

Three of the tests I first wrote passed vacuously and were rewritten after their mutation survived:

  • "ignores a verdict in the model's reasoning" — deleting the !part.thought guard still passed, because the planted verdict was mid-line (the parser is line-anchored) and last-match-wins let the answer's verdict win regardless of order. The thought must carry a line-anchored verdict and the answer none.
  • "reads the verdict off the final round" — did not exist; the per-round reset of the accumulated text was untested. Now driven by a real two-round tool loop: round 1 says YES on its way to a tool call, round 2 comes back undecided.
  • "skips when the turn errors" — passed because the text was empty (fail-closed), not because of the outcome check; deleting outcome !== 'ok' killed nothing. The stream now emits DECISION: YES and then throws.

API

POST /scheduled-tasks
{ "cron": "0 9 * * *", "runMode": "isolated",
  "prompt":    "Summarize what landed and write the digest.",
  "condition": "Has anything landed on main since yesterday?" }

condition is isolated-only. POST and PATCH reject it on a shared task (condition_requires_isolated), judging the combined post-patch state so a condition can be stranded from neither side — adding one to a shared task, or switching a guarded task back to shared without dropping it, both fail rather than silently discarding what the user typed. That check is gated on the request actually touching condition or runMode, so a hand-edited stranded task stays editable. isValidTask requires a non-empty string, since the fire path gates on truthiness and an empty condition would silently un-guard the task.

Known characteristic

The bound session accumulates one check turn per fire, so a guarded task's context grows across runs (auto-compression applies, as on any other cron turn). That accumulation is the decision log — it is the point of the design, not a side effect. A withheld fire is now marked withheld in runs[] and tagged skipped (precondition) in the Web Shell history, so the run list no longer reports a clean run for a task that deliberately did nothing.


Review round 1 — four findings, all confirmed and fixed (cf0e87b4c)

Every one reproduced against the code before being acted on; each fix is pinned by a test that fails without it.

  1. A truncated tool loop read as a clean turn. stopAfterPermissionCancel and loopDetected return mid-tool-loop without aborting the turn's signal or setting the error flag, so onComplete saw 'ok'. A model that emits DECISION: YES as text in the same streaming round as its tool call would then release the fire on a verdict it never got to revise. Added an 'incomplete' outcome, set at both early returns. (The reviewer's stated trigger — a plain tool's permission timeout — actually routes elsewhere; stopAfterPermissionCancel is only set by nested agent-tool cancels. loopDetected reaches it from any tool loop, so the finding stands.)

  2. The batched missed carrier inherited the first task's guard state. Worse than reported: missed: true has exactly one producer, and its prompt is a notification about every one-shot missed in this load ("Do NOT execute these prompts yet — ask the user first"), spread from runnable[0]. A NO on that task's precondition would have suppressed the notice for its siblings, which removeMissedFromDisk has already deleted. Fixed at both ends: the scheduler strips condition/runMode from the synthetic carrier, and the session refuses to judge a missed fire. This was my own error — I chose to judge missed fires without checking what missed actually carries.

  3. A hedged verdict was accepted. \b after the verdict let DECISION: YES, but I could not verify it pass as a YES. The prompt asks for a line that is exactly one of the two, so the verdict must now be the whole line; closing markdown and terminal punctuation are still tolerated, everything else fails closed.

  4. A condition could be stranded on an unbound task. The check runs in the task's own session — that is what makes its transcript a decision log. A task with no sessionId (tool-created, or created with no bridge) fires through the shared per-project durable owner, so its check would be injected into whichever session holds that lock. Create and update now reject a condition on such a task (condition_requires_bound_session) rather than quietly relocating the check. The related observation that PATCH runMode: 'isolated' does not bind a session is real but pre-existing (it predates this PR) and left alone here.

Also addressed: the decision point now logs. A non-ok outcome additionally reaches stderr, because the scheduler has already booked the fire as a run and debugLogger writes nothing unless a debug log session is active.

Two further findings posted on this PR (.github/workflows/qwen-autofix.yml, packages/core/src/services/sessionService.ts) target files this PR does not touch, and are not addressed here.


Review round 2 — four more findings, all confirmed and fixed (ca72ae956)

  1. The verdict was read from anywhere in the reply, not from its final line. DECISION: YES\n\nBut I could not verify it released the fire. My own test had pinned that as correct, which locked the bug in rather than catching it; it is now inverted. isCronConditionMet reads only the last non-empty line — which also subsumes the reasoning-out-loud case, since scratch work never lands there.

  2. A second way to truncate a turn read as a clean one. Last round I marked loopDetected; its sibling repeatedDuplicateProviderToolCall takes the quiet exit — #buildNextMessageAfterToolRun returns null and the loop ends with no error and no abort. Marked at the choke point (nextMessage === null) instead of per-flag, because enumerating the reasons is exactly how the sibling was missed.

  3. The headless and TUI onFire consumers ignored condition, firing a guarded task with its guard ignored. Both now fail closed; only the ACP/daemon session, which owns the sub-session dispatch the verdict gates, evaluates one. (Reachable only via a hand-written condition on an unbound task — a condition requires a bound session, and a bound task fires only in its own — but the failure direction is fire-open, which a precondition must never take.)

  4. The run history could not tell a withheld fire from a real one. Previously waved off as a documented limitation; that was a cop-out. CronTaskRun.withheld is stamped after the verdict by the evaluating session, addressed by the fire's own minute (processJob assigns job.lastFiredAt before calling onFire, and the persist writes runs[].at from that same stamp), and the Web Shell tags the entry skipped (precondition).

On the test suite itself

An interrupted mutation run left a mutation in the working tree, and the next sweep reported every mutation as killed — because the baseline was already failing. The harness now verifies a green baseline for all six suites before it trusts a single result. With that in place, four mutations that the corrupted run had scored as killed were exposed as surviving: two of them were pre-existing tests that finding #1's stricter parser had quietly subsumed. Both guards are still load-bearing (a thought streamed after the answer; a final round that emits no text), so those tests were re-pointed at the cases that still discriminate. 33/33 now, on a verified baseline.


Round 3 — the bound session was unreadable, and untranslated (02260dfb6)

The bound session is the feature's decision log, but every fire echoed the whole
instruction wrapper the model receives — five paragraphs of "end your reply with a final
line that is exactly one of…"
— and none of it went through t().

It now shows a compact label and the user's own condition (whitespace-collapsed, capped at
280 chars, surrogate-safe), and one line saying what the check decided. On a YES that line
carries a qwen-session:// link to the sub-session doing the work — without it, the bound
session of an isolated task shows nothing at all for a fire that did run.

The model still receives the full wrapper; only the echo changed.

Anchor session, English

All seven strings are translated in en / zh / zh-TW — the three locales check-i18n
holds to strict key parity. Session.ts had no i18n import before this.

Anchor session, Chinese

Both screenshots are live captures against a real qwen serve daemon: the task fires once
with the flag absent (DECISION: NO → skipped) and once with it present (DECISION: YES
sub-session link), in one transcript.

A bug the screenshot caught and the tests did not. The status line is an
agent_message_chunk, which the client appends to the assistant message already on
screen — and that message ends on the verdict with no trailing newline. The first capture
rendered DECISION: NO⏰ Precondition not met…. It now opens with a blank line, and a test
pins that.

Known limitation, pre-existing. The ACP cron path persists no user record at all, so the
echo and the status lines are live-only: after a reload the transcript shows the model's
answers with no question above them. client.ts records a cron prompt via
recordCronPrompt(message, displayText) — exactly the display/model split this needs — but
only on the core send path, which the ACP session does not use. Worth fixing separately.


Round 4 — a manual run cannot evaluate the guard (6a0c1c95a)

Two findings from @BZ-D, both confirmed against the code:

The manual run record lied. onRunPrompt resolves at admission, so runScheduledTask
appended a manual run entry even when the model went on to decide the condition was false —
and unlike a scheduled fire, nothing could stamp withheld. Worse, the one-shot branch
consumes (deletes) the task before the prompt is enqueued, so a false precondition destroyed
the task without ever running it.

The manual wrapper was not fail-closed. It offered only "holds" / "does not hold". The
scheduled path treats "cannot determine" as NO and machine-parses a whole-line verdict, and
withholds on an errored, cancelled or truncated turn. Here the model owned the dispatch
decision, so a tool failure had no branch and could still reach create_sub_session.

Both are symptoms of one root cause: the verdict is only observable inside the session that
computes it.
The dialog hands off a prompt and gets back an admission ack. Any client-side
guard is unsound by construction.

So the manual run no longer evaluates the guard. wrapIsolatedRunPrompt does not take the
condition; "Run now" means run now. The manual record and the one-shot consumption are
truthful again, there is no second decision protocol to drift, and a guarded task's button reads
"Run now (runs immediately, ignoring the precondition)" (en/zh). A human clicking ▶ on a card
that shows If: … is overriding the guard on purpose.

This was my design error, chosen early on for the stated reason that Run now would double as a
way to test a condition — without noticing the client cannot see the answer.

A guarded manual run that genuinely reproduces a scheduled fire (real verdict, withheld
stamping, one-shot consumed only on YES) has to be dispatched daemon-side, through the same
internal fire path (#deliverCronFire + #markCronRunWithheld). That is a follow-up, not a
line to add here.

Also fixed: qwen-session:// links were never clickable (a2ac49df5)

The precondition's "Running this scheduled task in a new session: 🧵 …" link did nothing.
MarkdownLink's interception branch for that scheme has never run: react-markdown sanitizes
every href through defaultUrlTransform, which rewrites any scheme outside
http(s)/irc(s)/mailto/xmpp to '', so components.a saw an empty href and rendered an
inert anchor.

Dead code since #6535 (ac2f371c4) introduced it — mine. It went unnoticed because
create_sub_session's link works through a different path: ToolGroup parses it out of plain
tool output with its own regex, never touching react-markdown. A precondition status line is the
first such link to reach an assistant message.

Fixed with a urlTransform that passes the scheme through and defers everything else to the
default sanitizer. The interception branch never writes the scheme to the DOM — it renders
href="#" and dispatches the id — and the per-component isSafeHref / isSafeImageSrc guards
are untouched. Verified end to end: clicking navigates from the anchor session to the spawned
sub-session.

An isolated scheduled task now takes an optional `condition` alongside its
prompt. On every fire the task's bound session evaluates the condition as an
ordinary cron turn, and only dispatches the prompt into a fresh sub-session
when that turn's verdict is YES.

The check deliberately runs in the bound session rather than a throwaway
sub-session:

  - It has exactly the semantics of a `shared` fire — same tools, same
    workspace approval mode — so it introduces no new permission surface.
  - The bound session of an isolated task is otherwise empty, so its
    transcript becomes the task's decision log: the record of why a fire did
    or did not happen.
  - No session is minted for a run that never occurs.

Everything that is not a YES skips the fire: NO, an unparseable answer, a
tool-loop error, a cancelled or timed-out turn. A precondition exists to
withhold an unattended run, so an ambiguous answer must withhold it too.

A `missed` (late-delivered) fire is judged the same way and then keeps its
existing in-session path — a precondition changes whether a fire runs, never
how. The Web Shell's "Run now" evaluates the condition before relaying the
dispatch through the model, so a manual run reproduces a scheduled one and
doubles as the way to test that a condition is written correctly.

The field is isolated-only. `POST`/`PATCH /scheduled-tasks` reject a condition
on a shared task, judging the combined post-patch state so a condition can be
stranded from neither side; the check is gated on the request actually
touching `condition` or `runMode`, so a hand-edited stranded task stays
editable. `isValidTask` requires a non-empty string, since the fire path gates
on truthiness and an empty condition would silently un-guard the task.
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template: the body is thorough — design rationale, API spec, verification screenshots, mutation testing — but doesn't follow the repo template headings (What this PR does / Why it's needed / Reviewer Test Plan / Risk & Scope). All the information is there and more, so not blocking on this, but a future revision aligning to the template would help reviewers navigate faster.

Problem: this is a new feature, not a bug fix — the problem-existence check is about whether users actually need conditional scheduled-task execution. The answer is clearly yes: "run the nightly report, but only if something landed" is a real workflow, and the current scheduler fires unconditionally. The design decision to evaluate the precondition in the bound session (turning its transcript into a decision log) is elegant — simpler than a separate evaluation session, and the transcript becomes useful rather than empty.

Direction: aligned. The scheduler already has the isolated/shared run-mode split; adding a precondition on top is a natural extension. No new infrastructure is needed — the bound session, the cron turn, the sub-session dispatch are all reused. This is how a feature should land: extending existing seams rather than introducing new ones.

Size: 23 files changed, +2404/-52. Breakdown:

  • Production logic: ~1,208 lines (Session.ts +409/-42, cronTasksFile.ts +71/-1, scheduled-tasks.ts +146/-1, cronScheduler.ts +23/-1, Markdown.tsx +18/-1, useGeminiStream.ts +25/-1, others)
  • Tests: ~1,525 lines (Session.test.ts +761, scheduled-tasks.test.ts +254, ScheduledTasksDialog.test.tsx +157, Markdown.test.ts +81, others)
  • Touches core paths (packages/core/src/services/cronScheduler.ts, packages/core/src/services/cronTasksFile.ts) — flagged for maintainer awareness at 1,200+ production lines, but the changes are additive (new field, new validation, new markCronRunWithheld function) with no restructuring of existing logic.

Approach: scope is tight for what the feature does. Every layer (data model → scheduler → session fire path → REST validation → UI) gets the minimum change needed. The combined-state PATCH validation (judging post-patch condition+runMode together rather than the patch alone) is the right call — prevents a condition from being stranded on a shared task from either direction. The "Run now means run now" decision (round 4) is well-reasoned: the client cannot observe the verdict, so a client-side guard is unsound by construction.

No critical concerns. Moving on to code review. 🔍

中文说明

感谢贡献!

模板:PR 正文非常详尽——设计理由、API 规格、验证截图、mutation testing——但没有遵循仓库模板的标题格式(What this PR does / Why it's needed / Reviewer Test Plan / Risk & Scope)。所有信息都在,不因此阻断,但后续修订对齐模板会让审阅者更快定位。

问题:这是一个新功能而非 bug 修复——问题存在性检查是关于用户是否真的需要条件化定时任务执行。答案是明确的肯定:"跑 nightly report,但只有当有东西 landing 时才跑"是真实场景,当前 scheduler 是无条件触发的。在绑定会话中评估前置条件的设计(将其 transcript 变为决策日志)很精巧——比独立评估会话更简单,且 transcript 变得有用而非空。

方向:对齐。scheduler 已有 isolated/shared run-mode 拆分;在此基础上加前置条件是自然延伸。不需要新基础设施——绑定会话、cron turn、子会话分发全部复用。这是功能应有的落地方式:扩展现有接缝而非引入新的。

规模:23 文件变更,+2404/-52。构成:

  • 生产逻辑:~1,208 行(Session.ts +409/-42, cronTasksFile.ts +71/-1, scheduled-tasks.ts +146/-1, cronScheduler.ts +23/-1, Markdown.tsx +18/-1, useGeminiStream.ts +25/-1 等)
  • 测试:~1,525 行(Session.test.ts +761, scheduled-tasks.test.ts +254, ScheduledTasksDialog.test.tsx +157, Markdown.test.ts +81 等)
  • 触及核心路径(cronScheduler.tscronTasksFile.ts)——1,200+ 生产行需维护者关注,但变更是增量的(新字段、新验证、新 markCronRunWithheld 函数),没有重构现有逻辑。

方案:对于功能范围来说,scope 紧凑。每一层(数据模型 → scheduler → session 触发路径 → REST 验证 → UI)都只取最小所需变更。PATCH 的组合状态验证(判断 patch 后的 condition+runMode 而非 patch 本身)是正确选择——防止 condition 被从任一方向搁置在 shared 任务上。"Run now 就是 run now"的决策(第 4 轮)理由充分:客户端无法观察到判定结果,所以客户端守卫从构造上就不可靠。

无关键问题。进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Read the full diff against an independent proposal. No critical blockers found — the implementation is clean, well-tested, and follows project conventions.

Independent baseline: I'd have approached this the same way — add condition to DurableCronTask, validate at REST boundaries, wrap the condition into a verdict-prompt, parse the last line for DECISION: YES/NO, gate the dispatch. The PR matches or exceeds this baseline in every respect: the verdict regex is line-anchored with markdown-noise tolerance, the PATCH validates combined state, and the missed-carrier strips guard state.

Verdict parser (CRON_CONDITION_VERDICT_RE): line-anchored, markdown-tolerant, hedged-verdict-rejecting. isCronConditionMet reads only the last non-empty line — correctly subsumes the reasoning-out-loud case. The fail-closed default (no parseable verdict = skip) is the right choice for an unattended guard.

Fail-closed completeness: every failure mode is handled — disposed session, token-limit drain, incomplete outcome (truncated tool loop), non-ok outcome, and the missed carrier (which the scheduler strips and the session refuses to judge). The headless and TUI onFire consumers both fail closed when they encounter a guarded task they can't evaluate. Defensive and correct.

markCronRunWithheld: fire-and-forget, matched on the exact fire timestamp (not "newest run"), idempotent. Best-effort cosmetic marker that can't fail a fire — right tradeoff.

PATCH combined-state validation: judges post-patch condition + runMode together, gated on the request actually touching one of the two fields. Prevents stranding from either direction while keeping hand-edited stranded tasks editable. Well-designed.

qwen-session:// link fix (a2ac49df5): real bug from #6535, correctly diagnosed — defaultUrlTransform strips non-standard schemes before components.a runs. The urlTransform fix passes qwen-session:// through and defers everything else. The interception branch never writes the scheme to DOM (href="#"). Five new tests, three mutations.

i18n: seven strings translated in en/zh/zh-TW. Session.ts had no i18n import before — the buildCronConditionEcho and cronOutcomeSkipMessage functions correctly use t().

Reuse check: the scheduledTaskSessionName function's surrogate-pair-safe truncation pattern is reused by buildCronConditionEcho — same approach, separate implementation (not worth extracting for two call sites). No duplication concern.

Testing

CI Test (ubuntu-latest, Node 22.x) passes on the current head (b86ff9018). The two previously red qwen-autofix-workflow.test.js tests were a pre-existing failure on main (from #6609), confirmed by the author's A/B test and resolved upstream.

Tmux real-scenario testing is not practical here — this is a daemon feature requiring qwen serve with a live model backend and minute-level cron scheduling. The PR provides comprehensive live evidence: a real daemon run with a scripted mock provider, showing the condition evaluated via run_shell_command (test -f), the verdict stamped, and the sub-session dispatched. Screenshots of the decision log, sub-session, and Web Shell UI are all in the PR body.

The test suite claims 51 new tests across six layers (core file format, scheduler, ACP session, REST route, Web Shell dialog, Markdown rendering), with 33 mutations all killed against a verified green baseline. Three vacuously-passing tests were caught and rewritten — that's the kind of self-critique that makes a test suite trustworthy.

中文说明

代码审查

基于独立方案对比完整 diff。无关键阻断——实现干净、测试充分、遵循项目规范。

独立基线: 我会用相同方式——给 DurableCronTaskcondition,在 REST 边界验证,将 condition 包装为判定提示,解析最后一行 DECISION: YES/NO,控制分发。PR 在每个方面都匹配或超越此基线:判定正则行锚定且容忍 markdown 噪音,PATCH 验证组合状态,missed carrier 剥离守卫状态。

判定解析器CRON_CONDITION_VERDICT_RE):行锚定、markdown 容忍、拒绝含糊判定。isCronConditionMet 只读最后非空行——正确涵盖了 reasoning-out-loud 场景。Fail-closed 默认值(无可解析判定 = 跳过)对于无人值守的守卫是正确选择。

Fail-closed 完整性: 每种失败模式都有处理——已销毁会话、token 限制、不完整结果(截断的工具循环)、非 ok 结果、以及 missed carrier(scheduler 剥离 + session 拒绝判断)。headless 和 TUI 的 onFire 消费者在遇到无法评估的受守卫任务时都 fail closed。防御性且正确。

markCronRunWithheld:fire-and-forget,精确匹配触发时间戳(而非"最新 run"),幂等。尽力标记的装饰性标记,不会导致触发失败——正确的权衡。

qwen-session:// 链接修复a2ac49df5):#6535 引入的真实 bug——defaultUrlTransformcomponents.a 运行前就剥离了非标准 scheme。urlTransform 修复放行 qwen-session:// 并延迟其他一切到默认清理器。拦截分支永不将 scheme 写入 DOM(href="#")。五个新测试,三个 mutation。

i18n: en/zh/zh-TW 翻译七个字符串。Session.ts 之前没有 i18n 导入——buildCronConditionEchocronOutcomeSkipMessage 正确使用 t()

测试

CI Test (ubuntu-latest, Node 22.x) 在当前 head(b86ff9018)上通过。之前两个红色的 qwen-autofix-workflow.test.js 测试是 main 上的预存故障(来自 #6609),作者已通过 A/B 测试确认并在上游修复。

Tmux 真实场景测试在此不适用——这是需要 qwen serve + 活跃模型后端 + 分钟级 cron 调度的 daemon 功能。PR 提供了完整的实时证据:真实 daemon 运行,使用脚本化 mock provider,显示条件通过 run_shell_commandtest -f)评估,判定结果被标记,子会话被分发。决策日志、子会话和 Web Shell UI 的截图都在 PR 正文中。

测试套件声称六层 51 个新测试(核心文件格式、scheduler、ACP session、REST 路由、Web Shell 对话框、Markdown 渲染),33 个 mutation 在已验证的绿色基线上全部被杀死。三个空转通过的测试被发现并重写——这种自我批评使测试套件可信。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Stepping back: this is a well-conceived feature with a tight implementation. The design decision to evaluate preconditions in the bound session — turning its transcript into a decision log — is the kind of insight that makes a feature both simpler and more useful than the obvious approach (a throwaway evaluation session). The PR author clearly thought hard about the failure modes: fail-closed on ambiguity, the command hidden from the checking turn, combined-state validation on PATCH, hand-stranded tasks still editable.

The scope is exactly what the feature needs — four layers (data model, scheduler, session fire path, REST + UI) each get the minimum change. 51 new tests pass, including 33 mutation-tested guards verified against a green baseline. Build, typecheck, and CI all clean.

The review history shows four rounds of substantive feedback, each one finding real bugs (truncated tool loops reading as clean turns, hedged verdicts being accepted, the missed carrier inheriting guard state, manual "Run now" pretending to evaluate the guard, qwen-session:// links being dead code). Every finding was reproduced against the code before being acted on, and each fix is pinned by a test that fails without it. That's the kind of iterative rigor that makes a PR trustworthy.

The 1,208 production lines touch core paths, which warrants maintainer awareness — but the changes are additive (new field, new validation, new function) with no restructuring of existing logic. The cross-repo origin and feat type don't trigger the refactor guardrail.

LGTM, approving.

中文说明

整体来看:这是一个构思良好的功能,实现紧凑。在绑定会话中评估前置条件的设计决策——将其 transcript 变为决策日志——是一种让功能既更简单又更有用的洞察(相比显而易见的方案:一次性评估会话)。PR 作者显然深入思考了失败模式:歧义时 fail-closed、检查时隐藏命令、PATCH 的组合状态验证、手动搁置的任务仍可编辑。

范围恰好是功能所需——四层(数据模型、scheduler、session 触发路径、REST + UI)各取最小改动。51 个新测试通过,包括 33 个在已验证绿色基线上经过 mutation testing 验证的守卫。构建、类型检查和 CI 全部干净。

审查历史显示四轮实质性反馈,每轮都发现真实 bug(截断的工具循环被读为正常完成、含糊的判定被接受、missed carrier 继承了守卫状态、手动 "Run now" 假装评估了守卫、qwen-session:// 链接是死代码)。每个发现都在代码上复现后才被处理,每个修复都由一个没有它就会失败的测试锁定。这种迭代严谨性使 PR 可信。

1,208 生产行触及核心路径,需维护者关注——但变更是增量的(新字段、新验证、新函数),没有重构现有逻辑。跨仓库来源和 feat 类型不触发重构阻断规则。

LGTM,批准。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

LGTM, looks ready to ship. ✅

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Downgraded from Request changes to Comment: PR author appears to match the authenticated account, and GitHub does not allow requesting changes on your own PR.

Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/core/src/services/cronScheduler.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
@@ -533,8 +562,22 @@ export function registerScheduledTasksRoutes(
// legacy / default tasks — absent means 'shared').
patch.runMode = body['runMode'] === 'isolated' ? 'isolated' : undefined;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Critical] PATCH can set runMode: 'isolated' on an existing task without checking that the task has a dedicated sessionId or minting one. Legacy/tool-created tasks can therefore become "isolated" while still being owned by the shared durable lock owner; their precondition transcript is not the task's decision log, and in embeddings without a sub-session spawner the fire can fall back to running in-session.

Reject isolated mode/preconditions for unbound tasks, or create and persist the bound thread session before accepting this patch.

— GPT-5 via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed for the precondition, and fixed in cf0e87b4c. Split into two claims, because they have different answers:

A condition on an unbound task — real. The check runs in the task's own session; that is exactly what makes its transcript a decision log. An unbound task fires through the shared per-project durable owner, so its check would be injected into whichever session holds that lock — someone's live conversation. Both create and update now reject it (condition_requires_bound_session), rather than quietly relocating the check. That closes the two ways in: PATCH onto a cron_create-made task, and POST in an embedding with no bridge to bind one.

PATCH runMode: 'isolated' not minting a session — real, but pre-existing: that path was added with the isolated run mode itself and is unchanged by this PR. An unbound isolated task already dispatches a sub-session from whichever session fires it. I've left it alone here rather than fold a behaviour change to runMode into a PR about condition; happy to take it as a follow-up.

Pinned by rejects a condition on an UNBOUND task via PATCH (seeded straight to disk, since no writer produces that shape) and rejects a condition when no bridge can bind the task a session, which also asserts that a bridge-less isolated-without-condition create still succeeds — so the guard is scoped to the condition and does not silently tighten runMode.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Leaving this thread unresolved on purpose.

The precondition half of the remedy is done in cf0e87b4c — a condition on an unbound task is now rejected at both create and update (condition_requires_bound_session).

The other half — PATCH runMode: 'isolated' accepting a task with no bound sessionId, and not minting one — is untouched, and deliberately so: that path shipped with the isolated run mode itself and is not changed by this PR, so tightening it here would fold a behaviour change to runMode into a PR about condition. An unbound isolated task already dispatches a sub-session from whichever session fires it, exactly as before.

So the thread stays open as the record of that residual. Happy to take it as a follow-up PR, or to include it here if you'd rather it not ship separately.

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Two review findings could not be placed inline because GitHub's review API returned Path could not be resolved for those paths, so I’m posting them here with exact file/line references.

[Critical] .github/workflows/qwen-autofix.yml:275 leaves the targeted workflow test red. Running npx vitest run scripts/tests/qwen-autofix-workflow.test.js --config scripts/tests/vitest.config.ts fails because the tests still expect the old global group: 'qwen-autofix-issue' and still assert the route step never assigns ROUTE_ISSUE="${ISSUE_NUMBER}". Until those expectations are updated, the workflow test suite fails and the new assigned-issue/per-issue routing behavior is not locked in.

Update scripts/tests/qwen-autofix-workflow.test.js to assert the new per-issue concurrency group and to allow ROUTE_ISSUE="${ISSUE_NUMBER}" only in the assigned-to-autofix-bot branch, including its open-issue guard.

[Critical] packages/core/src/services/sessionService.ts:990 skips artifact records but still treats tail-neutral custom_title records as the active replay leaf. When a title reanchor is followed by a persisted session_artifact_event at the tail, loading from the title makes includeActiveSideArtifactRecords reject the later artifact because it is parented to the preceding conversation record, not to the title. That can silently omit restorable artifacts or tombstones on resume/fork even though they were written to JSONL.

Skip isTailNeutralSideRecord(record) in lastConversationRecordUuid, or otherwise keep tail-neutral records from becoming the replay leaf.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Suggestions — commit cf0e87b4c83b280a19d04a9a3d64918dc67e89f2

  • packages/web-shell/client/components/dialogs/ScheduledTasksDialog.tsx:60"Run now" leaks the task prompt into the condition evaluation. wrapIsolatedRunPrompt concatenates both the precondition text and the task prompt into a single model message ('If it DOES hold, then: ' + dispatch where dispatch contains --- Task ---\n + the task prompt). The scheduled-fire path (wrapCronConditionPrompt in Session.ts) deliberately withholds the task prompt to prevent the model from rubber-stamping YES. The "Run now" path biases the verdict toward YES because the model sees the work it would dispatch. Fix: restructure so the condition evaluation commits to a verdict before seeing the task text, or split "Run now" into a two-stage flow mirroring the scheduled path.

— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.

@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Suggestions — commit 8b7de1d5fed614a5600f549f67252f4f9585c2be

File Issue Suggested fix
Session.ts:3008-3016 No structured logging in onComplete skip paths — oncall cannot tell WHY a precondition withheld a fire without reading raw transcripts Add debugLogger.info/warn for each outcome branch (disposed, token-limit, outcome≠ok, verdict=NO)
cronScheduler.ts:~1330 Run history (CronTaskRun) doesn't distinguish dispatched vs precondition-skipped fires — UI shows green entries for skipped fires Add result?: 'dispatched' | 'skipped' to CronTaskRun, update from onComplete
Session.ts:~3489-3497 onComplete throw-swallow try/catch has no regression test — removing it silently kills the cron drain loop Add a test where onComplete throws, verify drain continues
Session.ts:642,654,678 Verdict format (DECISION: YES/NO) coupled across prompt, regex, and parser with no shared constant — rephrasing the prompt silently breaks parsing Extract VERDICT_KEYWORD/VERDICT_YES/VERDICT_NO constants used by both prompt and regex
scheduled-tasks.ts:177 + Session.ts:3007 toView and onFire independently gate condition with runMode === 'isolated' — changing one without the other creates UI/runtime mismatch Extract shared isConditionActive(task) predicate
Session.ts:3006-3019 No condition-specific tool-call budget — complex conditions on frequent schedules burn unbounded API quota on evaluations alone Consider per-condition tool-call cap or wall-clock timeout
Session.ts:3012-3013 disposed/tokenLimit guards in onComplete untested — regression removing them would dispatch fires into disposed sessions Add tests for both guard conditions during precondition turn
scheduled-tasks.test.ts PATCH route non-string condition untested — create route covers it but PATCH has more complex control flow Add PATCH with { condition: 42 } expecting 400
ScheduledTasksDialog.tsx:40-65 "Run now" combines condition + task in single message, diverging from scheduled fire's two-turn approach — model may do the work inline instead of dispatching Split into two steps (condition check → dispatch) matching the scheduled path

— qwen3.7-max via Qwen Code /review

…dings

Never judge a `missed` fire. That job is the scheduler's synthetic carrier:
one batched notification covering every one-shot missed in this load, built
from a spread of the first task, whose prompt is a notice ("these were missed
— ask the user before running them") rather than any task's command. Gating it
on the first task's precondition let a `NO` silently suppress the notice for
its siblings, which `removeMissedFromDisk` has already deleted. The scheduler
now strips per-task guard state from the carrier, and the session refuses to
judge a missed fire — the same contract enforced from both ends.

Distinguish a truncated turn from a clean one. A permission cancel or a
detected tool loop returns mid-tool-loop without aborting the turn's signal or
recording an error, so it reached `onComplete` as `'ok'`. A model that emits
`DECISION: YES` as text in the same streaming round as its tool call would then
release the fire on a verdict it never got to revise. Add an `'incomplete'`
outcome, set at both early returns.

Require the verdict to be the whole line. `\b` after the verdict accepted
`DECISION: YES, but I could not verify it` as a YES. The prompt asks for a line
that is exactly one of the two; a hedged answer is not a decision, and a
precondition must fail closed on an answer it cannot trust. Closing markdown
and terminal punctuation are still tolerated.

Require a bound session for a condition. The check is evaluated in the task's
own session — that is what makes its transcript a decision log. A task with no
`sessionId` (tool-created, or created with no bridge to bind one) fires through
the shared per-project durable owner, so its check would be injected into
whichever session holds that lock. Both create and update now reject a
condition on such a task instead of quietly relocating the check.

Also log the decision point: a non-ok outcome reaches stderr, since the
scheduler has already booked the run and `debugLogger` writes nothing unless a
debug log session is active.
@wenshao
wenshao requested a review from Copilot July 9, 2026 21:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review round 1 handled — cf0e87b4c

All four inline findings reproduced against the code and were fixed. Details are in each thread; the PR description now carries a summary. Each fix is pinned by a test that fails without it — the mutation matrix is up to 26 mutations, 26 killed (the four new fixes added five of them), and the suite is at 39 new tests.

Two corrections to the findings as filed, both in the authors' favour on the substance:

  • The truncated-turn finding names stopAfterPermissionCancel via a plain tool's permission timeout, but that flag is only set by a nested agent-tool cancel. The bug is real via loopDetected, which any tool loop can reach. Both returns are now flagged.
  • The missed-carrier finding understates it. missed: true has exactly one producer, and its prompt is a notification about every one-shot missed in that load — not runnable[0]'s command. A NO would have suppressed the notice for siblings the scheduler had already deleted from disk. Fixed at both ends: the scheduler strips per-task guard state from the synthetic carrier, and the session refuses to judge a missed fire at all. That one was my own mistake — I chose to judge missed fires without checking what missed carries.

Not addressed here

Two findings target files this PR does not touch. .github/workflows/qwen-autofix.yml:275 belongs to #6609 (a6902c5d4) and packages/core/src/services/sessionService.ts:990 to #6557 (53243de0c); neither path appears in git diff e06d3be2b..HEAD. They look like real issues — worth filing against those PRs — but they are not this PR's to fix.

PATCH runMode: 'isolated' does not mint a bound session. Real, but pre-existing: it shipped with the isolated run mode and is unchanged here. I've rejected an unbound condition (that is this PR's invariant) and deliberately not tightened runMode, so a bridge-less runMode: 'isolated' create still succeeds exactly as before. Happy to take it as a follow-up.

runs[] records kind: 'scheduled' before the precondition is evaluated, so a skipped fire is indistinguishable from a dispatched one in the run history. Known, and called out under "Known characteristic". The scheduler persists the run at fire time and cannot know the verdict; marking it would need a post-hoc write back from the session. The transcript of the task's own session is the record of why — which is the whole reason the check runs there.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Downgraded from Request changes to Comment: self-PR; CI still running.

— GPT-5 via Qwen Code /review

Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/core/src/services/cronScheduler.ts
Comment thread packages/core/src/services/cronScheduler.ts
Read the verdict off the final non-empty line, not from anywhere in the text.
The prompt asks the model to *end* its reply with the verdict, so
`DECISION: YES\n\nBut I could not verify it` is not a decision — scanning the
whole reply took the conclusion off the wrong line and released the fire.

Mark a cut-short tool loop at its choke point. `loopDetected` was flagged, but
its sibling `repeatedDuplicateProviderToolCall` takes the quiet exit: it makes
`#buildNextMessageAfterToolRun` return null, ending the turn with no error and
no abort. A model that streamed `DECISION: YES` in that same round then
released the fire on an investigation it never finished. Both cases (and any
future one) are now marked where the follow-up message comes back null, rather
than by enumerating flags — enumerating them is how the sibling was missed.

Fail closed in the two consumers that cannot evaluate a precondition. The
headless and TUI `onFire` callbacks read only `prompt`/`cronExpr`/`missed`, so
a guarded task fired there with its guard ignored — the exact outcome the
precondition exists to prevent. Both now skip such a fire; only the ACP/daemon
session, which owns the sub-session dispatch the verdict gates, runs it.

Distinguish a withheld fire in the run history. The scheduler books the run the
moment it fires, before any verdict exists, so a task that deliberately did
nothing reported "ran at 02:00". `CronTaskRun.withheld` is stamped afterwards
by the evaluating session, addressed by the fire's own minute (the scheduler
writes `runs[].at` from the very `lastFiredAt` it hands to `onFire`), and the
Web Shell tags the entry. Best-effort and never awaited: losing a cosmetic
marker must not affect a fire that has already been decided.
@wenshao
wenshao requested a review from Copilot July 10, 2026 00:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao

wenshao commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

CI: Test (ubuntu-latest, Node 22.x) is red on a pre-existing failure, not this PR

The job reports Tests 2 failed | 388 passed | 9 skipped. Both failures are in scripts/tests/qwen-autofix-workflow.test.js:

FAIL  qwen-autofix workflow > keeps label-triggered issue routing guarded and diagnosable
FAIL  qwen-autofix workflow > does not expose comment-triggered autofix commands

Neither scripts/tests/qwen-autofix-workflow.test.js nor .github/workflows/qwen-autofix.yml appears in git diff e06d3be2b..HEAD --name-only for this branch. Zero failures land in any file this PR touches.

Reproduced on an untouched origin/main worktree (e06d3be2b, which is this branch's base):

$ git worktree add /tmp/probe origin/main --detach
$ npx vitest run scripts/tests/qwen-autofix-workflow.test.js --config scripts/tests/vitest.config.ts
  × keeps label-triggered issue routing guarded and diagnosable
  × does not expose comment-triggered autofix commands
  Tests  2 failed | 46 passed (48)

Root cause is what the earlier out-of-scope review comment described: #6609 (a6902c5d4) moved the workflow to per-issue concurrency and added an assigned trigger, but the test still asserts the old global group and still asserts the route step never assigns ROUTE_ISSUE:

AssertionError: expected 'name: \'Qwen Autofix\'…' to contain "group: 'qwen-autofix-issue'"

So this is #6609's stale test, inherited by every PR branched off main since. It should be fixed on main (or in a follow-up), not folded into a feature PR about scheduled-task preconditions — happy to send that patch separately if you'd like.

wenshao added 2 commits July 10, 2026 09:51
The bound session of a guarded task is the feature's decision log, but it read
like a debug dump: every fire echoed the whole instruction wrapper the model
receives — five paragraphs of "end your reply with a final line that is exactly
one of…" — and nothing in it was translatable.

Echo a compact label instead. `CronQueueItem` gains an optional `echoText`: the
text the client shows when the text sent to the model is not fit to read. A
precondition turn now shows "⏰ Precondition check" and the user's own condition,
whitespace-collapsed and capped at 280 characters (surrogate-safe). The model
still receives the full wrapper.

Say what the check decided. The model's answer explains its reasoning but cannot
state the consequence, so the scheduler adds one line: the run was skipped
(precondition not met, or the check was cancelled / interrupted / failed), or it
is running — with a `qwen-session://` link to the sub-session that is doing the
work. Without that link the bound session of an isolated task shows nothing at
all for a fire that DID run: the work happens in a sibling the user cannot
reach from here.

The status line opens with a blank line. It is an `agent_message_chunk`, which
the client appends to the assistant message already on screen, and that message
ends on the verdict with no trailing newline — without the break the transcript
renders `DECISION: NO⏰ Precondition not met…`. A screenshot caught that; the
assertions did not, so there is now a test for it.

All seven strings go through `t()` and are translated in en/zh/zh-TW (the three
locales `check-i18n` holds to strict key parity). Session.ts had no i18n import
before this; `t()` is initialized on the ACP path by `gemini.tsx`.

Not addressed: the ACP cron path persists no user record at all, so the echo and
the status lines are live-only and a reload shows the model's answers with no
question above them. That is pre-existing — `client.ts` records a cron prompt via
`recordCronPrompt(message, displayText)` only on the core send path, which the
ACP session does not use.
@wenshao
wenshao requested a review from Copilot July 10, 2026 02:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

…/qwen-code into feat/scheduled-task-precondition
@wenshao
wenshao requested a review from Copilot July 10, 2026 02:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao
wenshao requested a review from Copilot July 10, 2026 02:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao

wenshao commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Correction — the workflow-test failure is already fixed upstream

My earlier note offered to send a separate patch for the two red
scripts/tests/qwen-autofix-workflow.test.js tests. That is no longer needed, and the offer
is withdrawn.

#6608 (55311c3b5) updated both .github/workflows/qwen-autofix.yml and its test in the
same commit, and it has since landed on main and been merged into this branch (8ec33cef2).

A/B against the two heads of this branch:

ca72ae956  (pre-merge, the head the red CI run tested)   Tests  2 failed | 46 passed (48)
b86ff9018  (current head, after the main merge)          Tests  48 passed (48)

So the red Test (ubuntu-latest, Node 22.x) on this PR was against the old head and is
stale. The run for b86ff9018 is queued.

`MarkdownLink` has an interception branch for `qwen-session://<id>` that renders
a button and dispatches `qwen:open-session` so the app shell can navigate. It
has never run.

react-markdown sanitizes every href through `defaultUrlTransform`, which allows
only `http(s)`, `irc(s)`, `mailto` and `xmpp` and rewrites everything else to
`''`. So the scheme was stripped before `components.a` was called: the branch
saw an empty href, fell through, and rendered a plain anchor with no href.
Clicking it did nothing.

Add a `urlTransform` that passes `qwen-session://` through and defers every
other url to the default sanitizer. Letting the scheme through is safe — the
interception branch never puts it in the DOM, it renders `href="#"` and
dispatches the id as an event — and the per-component `isSafeHref` /
`isSafeImageSrc` guards are unchanged.

Dead since QwenLM#6535 (ac2f371) introduced it: the `create_sub_session` tool's link
works only because `ToolGroup` parses `[text](qwen-session://id)` out of plain
tool output with its own regex, never touching react-markdown. A precondition's
"running this task in a new session" line is the first such link to reach an
assistant message, which is how this surfaced.
@wenshao
wenshao requested a review from Copilot July 10, 2026 03:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao

wenshao commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

qwen-session:// links were never clickable — fixed in a2ac49df5

The precondition's "Running this scheduled task in a new session: 🧵 …" line renders a
qwen-session://<id> markdown link. It did nothing when clicked.

MarkdownLink has an interception branch for that scheme — it renders a button and
dispatches qwen:open-session so the app shell navigates. The branch has never run.
react-markdown sanitizes every href through defaultUrlTransform, which allows only
http(s) / irc(s) / mailto / xmpp and rewrites everything else to '':

> defaultUrlTransform('qwen-session://abc123')
''

So components.a was called with an empty href, the branch fell through, and the anchor
rendered with no href at all.

This is dead code from #6535 (ac2f371c4), the PR that introduced it — mine. It went
unnoticed because the create_sub_session tool's link works through a different path:
ToolGroup parses [text](qwen-session://id) out of plain tool output with its own regex
and never touches react-markdown. A precondition's status line is the first such link to
reach an assistant message.

Fix: a urlTransform that passes qwen-session:// through and defers everything else to
the default sanitizer. Safe by construction — the interception branch never writes the scheme
to the DOM (it renders href="#" and dispatches the id), and the per-component isSafeHref /
isSafeImageSrc guards are untouched.

Verified end to end against a live daemon: the YES fire emits the line, the link renders as
<a role="button" href="#">, and clicking it navigates from the anchor session
(/session/530aa910…) to the spawned sub-session (/session/76d7e4c3…), which then shows the
task's own transcript.

Five new tests. Three mutations, each killing exactly its own test:

mutation kills
remove the urlTransform prop the two DOM tests (back to the dead-code state)
pass every url through defers every other url to react-markdown's sanitizer
never let qwen-session through all three link tests

Comment thread packages/web-shell/client/components/dialogs/ScheduledTasksDialog.tsx Outdated
Comment thread packages/web-shell/client/components/dialogs/ScheduledTasksDialog.tsx Outdated
…ondition

"Run now" wrapped the task's condition into the relayed prompt and let the model
decide whether to dispatch. That cannot be made correct from the client, and it
produced three defects:

  - `onRunPrompt` resolves at ADMISSION, so `runScheduledTask` appended an
    ordinary `manual` run record even when the model went on to decide the
    condition was false. The history reported a successful run for work that was
    never done, and — unlike a scheduled fire — nothing could stamp `withheld`.

  - The one-shot branch consumes the task (`/run` deletes it) BEFORE the prompt
    is even enqueued. A false precondition therefore destroyed the task without
    ever running it.

  - The manual wrapper offered only "holds" / "does not hold". The scheduled path
    treats an inability to determine the condition as NO and machine-parses a
    final verdict; here the model owned the dispatch decision, so a tool failure
    or an ambiguous answer had no branch at all and could still reach
    `create_sub_session`.

The verdict is only observable inside the session that computes it. So a manual
run no longer evaluates the guard: "Run now" means run now. The `manual` run
record and the one-shot consumption are truthful again, and there is no second
decision protocol to drift from the scheduler's.

The button says so — a guarded task's tooltip reads "Run now (runs immediately,
ignoring the precondition)", translated in en/zh.

A guarded manual run that reproduces a scheduled fire (verdict, `withheld`
stamping, one-shot consumed only on YES) would have to be dispatched daemon-side,
where the outcome exists. That is a separate change.
@wenshao
wenshao requested a review from Copilot July 10, 2026 03:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@wenshao

wenshao commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

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.

LGTM, looks ready to ship. ✅

@BZ-D BZ-D left a comment

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.

LGTM

@wenshao
wenshao added this pull request to the merge queue Jul 10, 2026
Merged via the queue into QwenLM:main with commit dd62c3a Jul 10, 2026
71 of 73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants