feat(scheduled-tasks): gate an isolated run behind a precondition#6619
Conversation
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.
|
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:
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。构成:
方案:对于功能范围来说,scope 紧凑。每一层(数据模型 → scheduler → session 触发路径 → REST 验证 → UI)都只取最小所需变更。PATCH 的组合状态验证(判断 patch 后的 condition+runMode 而非 patch 本身)是正确选择——防止 condition 被从任一方向搁置在 shared 任务上。"Run now 就是 run now"的决策(第 4 轮)理由充分:客户端无法观察到判定结果,所以客户端守卫从构造上就不可靠。 无关键问题。进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewRead 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 Verdict parser ( Fail-closed completeness: every failure mode is handled — disposed session, token-limit drain, incomplete outcome (truncated tool loop), non-ok outcome, and the
PATCH combined-state validation: judges post-patch
i18n: seven strings translated in en/zh/zh-TW. Reuse check: the TestingCI Tmux real-scenario testing is not practical here — this is a daemon feature requiring 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。无关键阻断——实现干净、测试充分、遵循项目规范。 独立基线: 我会用相同方式——给 判定解析器( Fail-closed 完整性: 每种失败模式都有处理——已销毁会话、token 限制、不完整结果(截断的工具循环)、非 ok 结果、以及
i18n: en/zh/zh-TW 翻译七个字符串。 测试CI Tmux 真实场景测试在此不适用——这是需要 测试套件声称六层 51 个新测试(核心文件格式、scheduler、ACP session、REST 路由、Web Shell 对话框、Markdown 渲染),33 个 mutation 在已验证的绿色基线上全部被杀死。三个空转通过的测试被发现并重写——这种自我批评使测试套件可信。 — Qwen Code · qwen3.7-max |
|
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, 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 LGTM, approving. 中文说明整体来看:这是一个构思良好的功能,实现紧凑。在绑定会话中评估前置条件的设计决策——将其 transcript 变为决策日志——是一种让功能既更简单又更有用的洞察(相比显而易见的方案:一次性评估会话)。PR 作者显然深入思考了失败模式:歧义时 fail-closed、检查时隐藏命令、PATCH 的组合状态验证、手动搁置的任务仍可编辑。 范围恰好是功能所需——四层(数据模型、scheduler、session 触发路径、REST + UI)各取最小改动。51 个新测试通过,包括 33 个在已验证绿色基线上经过 mutation testing 验证的守卫。构建、类型检查和 CI 全部干净。 审查历史显示四轮实质性反馈,每轮都发现真实 bug(截断的工具循环被读为正常完成、含糊的判定被接受、missed carrier 继承了守卫状态、手动 "Run now" 假装评估了守卫、 1,208 生产行触及核心路径,需维护者关注——但变更是增量的(新字段、新验证、新函数),没有重构现有逻辑。跨仓库来源和 LGTM,批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
There was a problem hiding this comment.
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.
| @@ -533,8 +562,22 @@ export function registerScheduledTasksRoutes( | |||
| // legacy / default tasks — absent means 'shared'). | |||
| patch.runMode = body['runMode'] === 'isolated' ? 'isolated' : undefined; | |||
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Two review findings could not be placed inline because GitHub's review API returned [Critical] Update [Critical] Skip — GPT-5 via Qwen Code /review |
Suggestions — commit
|
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| 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.
Review round 1 handled —
|
wenshao
left a comment
There was a problem hiding this comment.
Downgraded from Request changes to Comment: self-PR; CI still running.
— GPT-5 via Qwen Code /review
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.
CI:
|
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.
…/qwen-code into feat/scheduled-task-precondition
Correction — the workflow-test failure is already fixed upstreamMy earlier note offered to send a separate patch for the two red #6608 ( A/B against the two heads of this branch: So the red |
`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.
|
| 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 |
…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.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
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.Why the check runs in the bound session
Not in a throwaway sub-session, deliberately:
sharedfire — same tools, same approval mode — so it introduces no new permission surface.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
missedfire 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.Verification
1. A live end-to-end run against a real
qwen servedaemonDriven 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 runningtest -f …throughrun_shell_commandand reading the result. The screenshot below is the captured terminal output of that run.The task's own session after the two fires — this is the decision log the design is built around:
The sub-session spawned by the second fire. It starts from zero context: the precondition is nowhere in its transcript, only the command.
2. The tests
51 new tests across the six layers (core file format, scheduler, ACP session, REST route, Web Shell dialog).
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.
Three of the tests I first wrote passed vacuously and were rewritten after their mutation survived:
!part.thoughtguard 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.outcome !== 'ok'killed nothing. The stream now emitsDECISION: YESand 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?" }conditionis isolated-only.POSTandPATCHreject 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 touchingconditionorrunMode, so a hand-edited stranded task stays editable.isValidTaskrequires 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
withheldinruns[]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.
A truncated tool loop read as a clean turn.
stopAfterPermissionCancelandloopDetectedreturn mid-tool-loop without aborting the turn's signal or setting the error flag, soonCompletesaw'ok'. A model that emitsDECISION: YESas 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;stopAfterPermissionCancelis only set by nested agent-tool cancels.loopDetectedreaches it from any tool loop, so the finding stands.)The batched
missedcarrier inherited the first task's guard state. Worse than reported:missed: truehas 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 fromrunnable[0]. ANOon that task's precondition would have suppressed the notice for its siblings, whichremoveMissedFromDiskhas already deleted. Fixed at both ends: the scheduler stripscondition/runModefrom the synthetic carrier, and the session refuses to judge amissedfire. This was my own error — I chose to judge missed fires without checking whatmissedactually carries.A hedged verdict was accepted.
\bafter the verdict letDECISION: YES, but I could not verify itpass 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.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 thatPATCH 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
debugLoggerwrites 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)The verdict was read from anywhere in the reply, not from its final line.
DECISION: YES\n\nBut I could not verify itreleased the fire. My own test had pinned that as correct, which locked the bug in rather than catching it; it is now inverted.isCronConditionMetreads only the last non-empty line — which also subsumes the reasoning-out-loud case, since scratch work never lands there.A second way to truncate a turn read as a clean one. Last round I marked
loopDetected; its siblingrepeatedDuplicateProviderToolCalltakes the quiet exit —#buildNextMessageAfterToolRunreturns 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.The headless and TUI
onFireconsumers ignoredcondition, 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-writtenconditionon an unbound task — aconditionrequires 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.)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.withheldis stamped after the verdict by the evaluating session, addressed by the fire's own minute (processJobassignsjob.lastFiredAtbefore callingonFire, and the persist writesruns[].atfrom 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 boundsession 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.
All seven strings are translated in
en/zh/zh-TW— the three localescheck-i18nholds to strict key parity.
Session.tshad no i18n import before this.Both screenshots are live captures against a real
qwen servedaemon: the task fires oncewith 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 onscreen — 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 testpins 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.tsrecords a cron prompt viarecordCronPrompt(message, displayText)— exactly the display/model split this needs — butonly 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
manualrun record lied.onRunPromptresolves at admission, sorunScheduledTaskappended a
manualrun 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 branchconsumes (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.
wrapIsolatedRunPromptdoes not take thecondition; "Run now" means run now. The
manualrecord and the one-shot consumption aretruthful 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,
withheldstamping, 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 aline 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 sanitizesevery href through
defaultUrlTransform, which rewrites any scheme outsidehttp(s)/irc(s)/mailto/xmppto'', socomponents.asaw an empty href and rendered aninert anchor.
Dead code since #6535 (
ac2f371c4) introduced it — mine. It went unnoticed becausecreate_sub_session's link works through a different path:ToolGroupparses it out of plaintool 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
urlTransformthat passes the scheme through and defers everything else to thedefault sanitizer. The interception branch never writes the scheme to the DOM — it renders
href="#"and dispatches the id — and the per-componentisSafeHref/isSafeImageSrcguardsare untouched. Verified end to end: clicking navigates from the anchor session to the spawned
sub-session.