Skip to content

feat(scheduled-tasks): add isolated run mode via create_sub_session tool#6535

Merged
wenshao merged 7 commits into
QwenLM:mainfrom
wenshao:feat/isolated-scheduled-tasks-sub-session
Jul 9, 2026
Merged

feat(scheduled-tasks): add isolated run mode via create_sub_session tool#6535
wenshao merged 7 commits into
QwenLM:mainfrom
wenshao:feat/isolated-scheduled-tasks-sub-session

Conversation

@wenshao

@wenshao wenshao commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Introduce a new create_sub_session tool (daemon-only) that spawns a fresh top-level sub-session with its own clean context and transcript. Wire it into the cron scheduler as an isolated run mode so each scheduled fire dispatches its prompt into a fresh sub-session instead of accumulating in one shared transcript.

Changes

  • New create_sub_session tool — spawns a sibling session and runs a prompt in it. Two completion modes: first-turn (waits for result) and sent (fire-and-forget). Daemon-only; reports itself unavailable in TUI/headless.
  • SubSessionLauncher in cli/serve — handles the daemon-side spawn via bridge.spawnOrAttach, subscribes to event stream for first-turn text, with concurrency cap (5 per caller), 5-minute timeout, and 32K char truncation.
  • ACP bridge extMethod dispatch — new qwen/control/create-sub-session method routes child→daemon requests with parameter validation.
  • runMode field (shared | isolated) added to DurableCronTask, CronJob, and daemon API types. Defaults to shared for backward compatibility; unknown values go through fix-or-delete validation.
  • Session integrationSession.onFire dispatches an isolated fire straight into a fresh sub-session (daemon-side, completion: 'sent') instead of running it inline. The scheduled fire deliberately does not go through the create_sub_session tool: that tool's permission is 'ask', and an unattended fire under the default approval mode would block on requestPermission with no one to answer, until the daemon's 5-minute timeout cancelled it. The tool keeps 'ask' for model-initiated calls, and the attended "Run now" button keeps relaying through it.
  • Web Shell UI — radio picker in ScheduledTasksDialog for choosing run mode; history button works the same for both modes.
  • Minor fixAuthMessage placeholder now uses i18n key instead of hardcoded string.

Evidence (Before & After)

Run mode picker — before/after

Left: main, no run-mode control. Middle/right: this PR's radio group in the create form, with Shared session as the default and each option's hint. Captured from the real ScheduledTasksDialog (real i18n, real CSS modules) in headless Chromium; only the daemon SDK is stubbed. The BEFORE panel is the same harness with the three touched client files checked out at the merge-base.

Editing an existing isolated task re-selects Isolated in the form, so the runMode API round-trip is visible in the UI.

Note: the task list itself shows no run-mode indicator — a shared and an isolated card render identically. Combined with the known limitation below (an isolated task's View conversation opens its empty anchor session), that is worth a follow-up.

Reviewer Test Plan

How to verify

  1. Create a scheduled task with isolated run mode via the web shell dialog — confirm the radio picker appears, defaults to shared, and saves isolated correctly.
  2. Fire an isolated task under qwen serve with the default approval mode — verify the prompt runs in a fresh sub-session (check session list for the 🧵 prefixed session) rather than the bound task session, and that no permission request is raised.
  3. Fire a shared task — confirm behavior is unchanged (runs in the bound session, accumulates transcript).
  4. API round-tripPOST /scheduled-tasks with runMode: 'isolated', then GET to confirm normalization.

Before/After

  • Before: All scheduled tasks run in one bound session; context accumulates across fires.
  • After: Users can choose isolated mode so each fire gets a clean context in a fresh sub-session.
中文说明

概述

新增 create_sub_session 工具(仅 daemon 模式可用),可创建一个全新的顶级子会话,拥有独立的上下文和会话记录。将其集成到 cron 调度器中作为 isolated 运行模式,使每次定时触发都将 prompt 调度到全新的子会话中执行,而非在同一个共享会话中累积上下文。

变更内容

  • 新增 create_sub_session 工具 — 创建兄弟会话并在其中运行 prompt。两种完成模式:first-turn(等待结果返回)和 sent(fire-and-forget,发送后立即返回)。仅 daemon 模式可用;在 TUI/无头模式下自动报告不可用。
  • SubSessionLaunchercli/serve)— 通过 bridge.spawnOrAttach 处理 daemon 侧的子会话创建,订阅事件流获取 first-turn 文本,具备并发上限(每个调用者 5 个)、5 分钟超时和 32K 字符截断。
  • ACP bridge extMethod 分派 — 新增 qwen/control/create-sub-session 方法,带参数校验地路由子→daemon 请求。
  • runMode 字段shared | isolated)— 添加到 DurableCronTaskCronJob 和 daemon API 类型中。默认 shared 保持向后兼容;未知值走 fix-or-delete 校验。
  • Session 集成Session.onFireisolated 触发直接派发到全新子会话(daemon 侧,completion: 'sent'),不再内联执行。定时触发经过 create_sub_session 工具:该工具权限为 'ask',在默认审批模式下无人值守的触发会卡在 requestPermission 上无人应答,直到 daemon 的 5 分钟超时将其取消。模型主动调用该工具时仍保留 'ask';有人值守的「Run now」按钮也仍走工具中继。
  • Web Shell UIScheduledTasksDialog 中新增运行模式单选框;历史按钮在两种模式下行为一致。
  • 小修复AuthMessage placeholder 改用 i18n key 替代硬编码字符串。

效果对比

运行模式选择器 — 修改前/后

左:main,没有运行模式控件。中/右:本 PR 在创建表单中新增的单选组,默认 Shared session,并各带一行说明。截图来自 headless Chromium 中渲染的真实 ScheduledTasksDialog(真实 i18n、真实 CSS module),仅 stub 了 daemon SDK;BEFORE 面板是同一套 harness、把三个受影响的 client 文件切到 merge-base 后拍的。

编辑一个已有的 isolated 任务时,表单会正确回显 Isolated,所以 runMode 的 API 往返在 UI 上可见。

注意:任务列表本身不显示运行模式——sharedisolated 的卡片长得一模一样。结合下面的已知限制(isolated 任务的「查看对话」会打开空白的锚点会话),这值得后续跟进。

验证方法

  1. 创建 isolated 运行模式的定时任务 — 通过 web shell 对话框确认单选框出现,默认 shared,可正确保存 isolated
  2. 触发 isolated 任务 — 在 qwen serve 默认审批模式下验证 prompt 在全新子会话中运行(会话列表中出现 🧵 前缀的会话),且不会弹出权限请求。
  3. 触发 shared 任务 — 确认行为不变(在绑定会话中运行,记录累积)。
  4. API 往返POST /scheduled-tasksrunMode: 'isolated',再 GET 确认规范化。

对比

  • 修改前:所有定时任务在同一个绑定会话中运行;上下文跨触发累积。
  • 修改后:用户可选择 isolated 模式,每次触发在全新子会话中获得干净的上下文。

Spawn-boundary hardening

  • create_sub_session honors the caller's AbortSignal, so cancelling a turn no longer pins the tool loop until the daemon's 5-minute first-turn ceiling. The sub-session itself is not cancelled (no abort seam) and keeps its concurrency slot — releasing it would over-admit against a still-running sub-session.
  • Nesting is capped at one level: the launcher refuses to spawn from a session it spawned. Without this, one prompt could fan out 5ⁿ sub-sessions until maxSessions ran dry.
  • callerSessionId — which keys the per-caller concurrency bucket — is authenticated at the bridge against the connection's ownsSession, so a child cannot forge a fresh bucket or burn a victim session's slots.
  • prompt is capped at 100,000 chars (matching the scheduled-task REST route) and name at 200, enforced at the bridge trust boundary and in the tool's own validation.

Known limitation

runs[].sessionId in the task history still records the bound (anchor) session, not the spawned sub-session, so clicking a run of an isolated task opens an empty transcript. The scheduler persists the run record at fire time, before the spawn resolves; linking the sub-session id needs a second write. Tracked as follow-up.

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Re-triage (post-review update)

This PR has gone through two rounds of code review with substantial hardening. Updating the Stage 1 assessment to reflect the current state.

Template: Now complete ✓ — has Summary, Changes, Evidence (Before & After) with real screenshots, Reviewer Test Plan, Spawn-boundary hardening, Known limitation, and Chinese translation. All required sections present.

Problem: Real and well-evidenced. Context accumulating across repeated scheduled-task fires in a single bound session is a legitimate usability issue. The independent E2E verification (comment by @wenshao) confirmed this with concrete before/after data: merge-base silently drops runMode, PR correctly persists and dispatches to fresh sub-sessions. Not a theoretical concern.

Direction: Aligned. Scheduled tasks and daemon infrastructure have seen heavy investment this cycle (#6389, #6348, #6453). Isolated run mode is a natural next step — users scheduling recurring agent tasks want clean context per fire.

Size:

  • Core paths (packages/core/src/): 409 production lines (new tool + config getter + cron scheduler/tasks file), 226 test lines, 0 generated/schema lines
  • Total across 5 packages: 1542 production lines, 1291 test lines
  • Cross-package scope: acp-bridge, cli/serve, core, web-shell, webui
  • Maintainer awareness flagged for the 1000+ production lines advisory and cross-package breadth.

Approach: The scope is justified — each component (reusable tool, daemon launcher, bridge dispatch, UI picker, type additions, hardening) serves the stated goal. The post-review hardening is impressive: depth-1 nesting cap, authenticated callerSessionId, 100K prompt cap, raceCancellation thunk for proper abort handling, workspace-wide concurrency backstop, and orphan session cleanup. Every review finding was addressed with code changes and regression tests.

One outstanding item from the independent E2E verification: the [object Object] error formatting bug (ACP SDK rejects with raw JSON-RPC error objects, not Error instances). A 12-line describeSpawnError fix was proposed but not yet applied. Non-blocking but worth folding in before merge.

Moving on to code review. 🔍

中文说明

重新评估(审查后更新)

此 PR 经过两轮代码审查,已进行大量加固。更新 Stage 1 评估以反映当前状态。

模板: 现已完整 ✓ — 包含 Summary、Changes、Evidence (Before & After) 附真实截图、Reviewer Test Plan、Spawn-boundary hardening、Known limitation 和中文翻译。

问题: 真实且有充分证据。定时任务在同一绑定会话中反复触发导致上下文累积是实际的可用性问题。独立 E2E 验证用具体数据证实了这一点。

方向: 对齐。定时任务和守护进程基础设施在本版本周期投入很大。隔离运行模式是自然的下一步。

规模: 核心路径 409 行生产代码、226 行测试代码。总计 5 个包 1542 行生产代码、1291 行测试代码。已标记维护者关注

方案: 范围合理。审查后的加固令人印象深刻:深度 1 嵌套上限、已认证的 callerSessionId、100K prompt 上限、正确的取消处理等。每个审查发现都通过代码变更和回归测试得到了解决。一个待处理项:E2E 验证发现的 [object Object] 错误格式化 bug,已提出 12 行修复但尚未应用。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code Review (re-triage — post-review update)

The PR has been substantially revised since the initial review. Key design change: isolated fires no longer go through the model. Session.onFire now dispatches isolated fires directly to the daemon via #dispatchIsolatedCronFire (completion: 'sent'), bypassing the 'ask'-gated create_sub_session tool entirely. This was the correct fix — the old path would have blocked unattended fires on a permission request nobody was there to answer.

Independent proposal vs. PR's approach: My initial proposal was a direct daemon API call from the cron fire handler — no new tool. The PR's approach (reusable create_sub_session tool + direct dispatch for scheduler fires) gives both: the tool is available for model-initiated calls and future features, while the scheduler bypasses the permission gate. This exceeds my proposal.

Hardening review (all post-initial-review):

  • raceCancellation thunk — takes () => Promise<T>, not a pre-evaluated promise. Pre-cancelled turns create no daemon work. spawnStarted flag distinguishes the two cancellation outcomes. Regression test asserts spawner is never called on pre-aborted signal. ✓
  • Depth-1 nesting gatespawnedSessionIds Set with FIFO eviction at 1024. Sub-sessions cannot spawn further sub-sessions. Verified by test + E2E. ✓
  • callerSessionId authentication — required at the bridge, validated against ownsSession. A child cannot forge another session's bucket. E2E-verified with a forged id. ✓
  • Input boundsprompt capped at 100K chars at both tool and bridge boundaries; name at 200 at bridge, 60 at display. Consistent MAX_SUB_SESSION_PROMPT_CHARS constant shared between tool and bridge. ✓
  • Workspace-wide concurrency backstopMAX_CONCURRENT_SUB_SESSIONS_TOTAL = 20 holds regardless of callerSessionId honesty. ✓
  • Orphan session cleanup — if spawn succeeds but a later step fails, closeSession is wrapped in both sync try/catch and async .catch(). The dual-guard pattern is load-bearing (CI confirmed this). ✓
  • closeSession error isolation — sync throw and async rejection both caught, never replaces the real launch error. Regression test for both paths. ✓

No critical blockers found. The code is correct, well-secured, and follows project conventions. Every hardening measure has a corresponding regression test that was mutation-checked.

One outstanding item: The [object Object] error formatting bug identified during independent E2E verification — ACP SDK rejects extMethod with raw JSON-RPC error objects ({ code, message, data }), not Error instances. The error instanceof Error ? error.message : String(error) pattern produces [object Object] for all daemon-side rejections. The 12-line describeSpawnError fix proposed in the verification comment is correct and should be folded in. Same pattern also exists in Session.#dispatchIsolatedCronFire's catch. Non-blocking for merge but a real UX issue — the model can't read error reasons.

Test Results

Package Test File Result
core create-sub-session.test.ts 11/11 ✓
core cronTasksFile.test.ts 40/40 ✓
core cronScheduler.test.ts 121/121 ✓
cli create-sub-session.test.ts 17/17 ✓
cli scheduled-tasks.test.ts 52/52 ✓
cli Session.test.ts 232/232 ✓
Total 473/473 ✓

Build: ✓ clean. Typecheck: ✓ clean.

(acp-bridge bridgeClient.test.ts and web-shell ScheduledTasksDialog.test.tsx could not run in this CI worktree due to module resolution issues in the test environment — not a code bug. The initial triage ran these successfully: 52/52 and 22/22 respectively.)

Independent E2E Verification

A comprehensive E2E verification was performed by @wenshao (see comment) using a real qwen serve daemon, real web shell with Playwright, and a merge-base A/B build. Key findings:

  • All PR claims confirmed: isolated fires spawn fresh sub-sessions, shared fires unchanged, no permission requests raised, depth-1 cap enforced, concurrency cap enforced, callerSessionId authentication verified
  • UI picker works correctly, API round-trips correctly, runMode validation rejects unknown values with 400
  • One real bug found ([object Object] — see above)
  • Recommendation: merge (with the [object Object] fix folded in)

Smoke Test

$ npm run build
> tsc -b packages/acp-bridge packages/core packages/cli packages/web-shell packages/webui
✓ all packages built successfully

$ npm run typecheck
> tsc --noEmit (per package)
✓ all packages typecheck clean
中文说明

代码审查(重新评估——审查后更新)

PR 自初次审查以来进行了大幅修订。关键设计变更:isolated 触发不再经过模型Session.onFire 现在通过 #dispatchIsolatedCronFire(completion: 'sent')直接将 isolated 触发派发到守护进程,完全绕过了 'ask' 权限门控的 create_sub_session 工具。这是正确的修复。

加固审查(均为审查后新增):

  • raceCancellation thunk:预取消的 turn 不创建任何守护进程工作 ✓
  • 深度 1 嵌套门控:子会话不能再创建子会话 ✓
  • callerSessionId 认证:在 bridge 侧验证,防止伪造 ✓
  • 输入边界:prompt 100K、name 200/60,双重检查 ✓
  • 工作区级并发后手:总量上限 20 ✓
  • 孤儿会话清理:双守卫模式(同步 + 异步)✓

未发现关键阻碍问题。

一个待处理项: [object Object] 错误格式化 bug——ACP SDK 用原始 JSON-RPC 错误对象而非 Error 实例来 reject。已提出 12 行修复,应在合并前应用。

测试结果

473 个测试全部通过。构建和类型检查均通过。独立 E2E 验证确认了所有 PR 声明,并发现了一个非阻塞 bug。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Reflection (re-triage)

This is a standout PR. Let me be direct about what I see after reviewing the full picture:

The problem is real and well-scoped. Scheduled tasks accumulating context across fires in a single bound session is a genuine usability gap. The before/after evidence from the independent E2E verification (merge-base silently drops runMode, PR correctly spawns fresh sub-sessions) leaves no room for doubt.

The implementation exceeds what I would have proposed. I would have added a direct daemon dispatch path for isolated fires — no reusable tool. The PR gives both: a reusable create_sub_session tool for model-initiated calls and future features, AND a direct daemon dispatch path for scheduler fires that correctly bypasses the 'ask' permission gate. The design pivoted mid-review when the author discovered the original approach (routing through the model) would block unattended fires — and the pivot was correct.

The hardening is genuine, not cosmetic. Every review finding (closeSession sync throw, AbortSignal propagation, nesting depth, callerSessionId auth, prompt/name caps, workspace-wide concurrency backstop) was addressed with code changes AND mutation-checked regression tests. The raceCancellation thunk pattern — taking a lazy function instead of an eager promise to prevent pre-aborted turns from creating daemon work — is the kind of fix that shows real understanding of the problem.

473 unit tests pass, build clean, typecheck clean. Independent E2E verification with a real qwen serve daemon confirms every behavioral claim. The author did more testing than most maintainers would.

What I'd flag before merging:
The [object Object] error formatting bug (ACP SDK rejects with raw JSON-RPC error objects, not Error instances) is a real UX issue — the model can't read daemon-side error reasons (concurrency cap reached, nesting denied, etc.). The 12-line describeSpawnError fix proposed in the verification comment is correct and should be folded in. Same pattern exists in Session.#dispatchIsolatedCronFire's catch. Non-blocking but worth a follow-up commit if not included before merge.

Size context: 409 core production lines (under the 500 threshold), 1542 total production lines across 5 packages. The cross-package scope is inherent to daemon features — the tool lives in core, the launcher in cli/serve, the dispatch in the bridge. Every file serves the feature.

Verdict: Approve. The code is correct, well-tested, and solves a real problem. The [object Object] fix is the only loose thread, and it's non-blocking.

中文说明

反思(重新评估)

这是一个出色的 PR。

问题真实且范围明确。 定时任务在同一绑定会话中跨触发累积上下文是真实的可用性缺口。独立 E2E 验证的 before/after 证据毫无疑问。

实现超越了我的方案。 加固是实质性的,不是表面文章。每个审查发现都通过代码变更和经过突变检查的回归测试得到了解决。473 个单元测试通过,构建和类型检查均通过。

合并前建议: [object Object] 错误格式化 bug 应在合并前修复(已提出 12 行修复),但不阻塞。

结论:批准。 代码正确、经过充分测试、解决了真实问题。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot added daemon scope/session-management Session state and persistence scope/web-shell status/in-review This issue is currently in review. type/feature-request New feature or enhancement request labels Jul 8, 2026
@wenshao wenshao force-pushed the feat/isolated-scheduled-tasks-sub-session branch from 6fe8d64 to 85384f2 Compare July 8, 2026 10:38
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@wenshao

wenshao commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Overview

Adds a daemon-only create_sub_session tool (child → daemon over a new qwen/control/create-sub-session extMethod), a SubSessionLauncher host handler in cli/serve, and a runMode: 'shared' | 'isolated' field threaded through DurableCronTaskCronJobSession.onFire → the daemon REST API → the web-shell dialog. Isolated fires are implemented by wrapping the prompt so the model chooses to call create_sub_session.

The back-compat work is careful and the new test suites are real (I ran both new files — 14 tests, all pass; they exercise timeout, truncation, turn_error, the cap, and stop()). Two things block, though: a verified i18n regression, and a concurrency cap that doesn't apply to the mode this feature actually uses.


Blocking

1. Six i18n keys don't exist — the UI renders raw key strings

ScheduledTasksDialog.tsx:462,474,479 and AuthMessage.tsx:648 call t() with keys that are in no locale map. getTranslator falls back to messages[key] ?? EN[key] ?? key, so the literal key is rendered. Verified at runtime against packages/web-shell/client/i18n.tsx on this branch:

scheduledTasks.runMode              ->  "scheduledTasks.runMode"
scheduledTasks.runMode.shared       ->  "scheduledTasks.runMode.shared"
scheduledTasks.runMode.isolated     ->  "scheduledTasks.runMode.isolated"
scheduledTasks.runMode.shared.hint  ->  "scheduledTasks.runMode.shared.hint"
scheduledTasks.runMode.isolated.hint->  "scheduledTasks.runMode.isolated.hint"
auth.modelsPlaceholder              ->  "auth.modelsPlaceholder"
scheduledTasks.prompt               ->  "Prompt"        <- control, translator works

So the new radio group renders as scheduledTasks.runMode with options scheduledTasks.runMode.shared / scheduledTasks.runMode.isolated.

The AuthMessage change described as a "minor fix" is a strict regression: it replaced a working literal ('model-id-1, model-id-2') with a key that resolves to auth.modelsPlaceholder. (auth.modelsPrompt right above it does exist — easy to miss.)

Please add all six to both EN and zh-CN. Note the existing dialog tests don't catch this because findRunModeRadio selects by input[value=…], never by label text — an assertion on the rendered label would have.

2. The per-caller concurrency cap is a no-op for completion: 'sent' — the mode the isolated-cron path uses

In create-sub-session.ts, the slot is released in a finally (:266) that runs as soon as launch() returns, and sent returns immediately at :248. Only first-turn holds a slot. But wrapIsolatedTaskPrompt (Session.ts:221) instructs the model to use completion "sent".

Runtime check — one caller, sequential sent launches, MAX_CONCURRENT_SUB_SESSIONS_PER_CALLER = 5:

cap=5   sessions actually spawned=50

The doc comment claims "parallel tool calls from one caller must not spawn unbounded sub-sessions", but for sent nothing bounds them. The only real backstop is the bridge-wide maxSessions (default DEFAULT_MAX_SESSIONS = 20), which is shared with the user's interactive sessions — so a looping or misbehaving isolated task can exhaust the daemon's whole session pool and make new user sessions fail with SessionLimitExceededError.

Suggest holding the slot for the sub-session's lifetime in sent mode (release on session close), or adding a separate cap on live sub-sessions.


Design concern

3. isolated is best-effort, but the UI presents it as a mode

Routing through the model ('do NOT perform the task yourself in this session') means a fire can silently degrade to shared behavior if the model ignores the instruction, paraphrases the prompt, or truncates it. Two concrete consequences worth answering in the PR:

  • Unattended approval. getDefaultPermission() returns 'ask' (correct, and consistent with cron_create). But a cron fire is unattended. Under ApprovalMode.DEFAULT the tool call raises session/request_permission on the anchor session; with no attached client the mediator resolves via DEFAULT_PERMISSION_TIMEOUT_MS (5 min) and cancels. Net effect: a scheduled task that previously needed zero approvals (plain text prompt) now needs one, and may never run. Did you verify an isolated fire end-to-end under the default approval mode?
  • No provenance. The sub-session's raw first-turn text is spliced into the caller's context. Standard subagent risk, just worth a note.

A deterministic daemon-side path — scheduler routes isolated fires straight to SubSessionLauncher, no model in the loop — would make the radio button mean what it says. If the model-relay is intentional for v1, please say so in the UI hint and the DurableCronTask.runMode doc.


Should fix

4. create_sub_session is missing from PermissionManager.CORE_TOOLS

permission-manager.ts:549 lists every analogous tool: cron_create, cron_list, cron_delete, loop_wakeup. isToolEnabled() bypasses the coreTools allowlist for non-core tools, so a user who narrows coreTools to a safe subset still gets a tool that spawns an agent with full tool access. Explicit deny rules still work, so it's a hardening gap rather than a hole — but of all the tools in that set, this is the one you'd most want gated. Please add it.

5. "Run now" ignores runMode

ScheduledTasksDialog.tsx:333,350 call onRunPrompt(fresh.prompt, fresh.sessionId) with the raw prompt. Manually running an isolated task executes it inline in the bound session — the opposite of what the radio promises. Wrap it there too, or annotate the button.

6. sent-mode dispatch failures are invisible

void turn.catch(() => {}) (create-sub-session.ts:246) swallows a rejected sendPrompt. Since sent is the cron path, an isolated fire that fails to dispatch reports success to the model, which then writes its "one-line confirmation" into the anchor transcript. Log the rejection (log.debug / writeStderrLine) at minimum.

7. POST/PATCH persistence asymmetry

POST omits runMode when 'shared' ("so tool-created / default tasks stay byte-identical"), but PATCH writes it explicitly (scheduled-tasks.ts:532) — and the dialog always sends runMode on edit. So editing any legacy task materializes the field. Harmless, but the stated invariant isn't upheld; consider delete patch.runMode when 'shared'.


Nits

  • Truncation cap is soft. MAX_RESULT_CHARS = 32_000, but '\n[…output truncated]' is appended after the slice → 32,021 chars. The test only asserts < 40_000. Also turn_error's [turn error] … is appended outside the cap.
  • subscribeEvents without maxQueued. A chatty sub-session that overruns the default 256-event backlog gets client_evicted; the loop then ends with no turn_complete, and awaitFirstTurn returns stopReason: 'incomplete' with partial text — indistinguishable from a closed bus. Consider handling the client_evicted frame explicitly.
  • Type cast hides the contract. as Parameters<AcpSessionBridge['sendPrompt']>[1] — prefer importing PromptRequest and constructing it typed, so a future required field is a compile error rather than a runtime surprise.
  • BridgeClient is now a 12-positional-arg constructor, with onCreateSubSession the third consecutive optional callback. Ordering is correct here (I checked the bridge.ts call site), but a deps object would remove a whole class of silent mis-wiring.
  • subSessionName('') (prompt of only control chars) yields "🧵 " with a trailing space. The surrogate-pair guard itself is right — charCodeAt(cut - 1) is exactly the last included unit.
  • Doc reflow in bridgeClient.ts leaves an over-long line and an awkward sentence ("the ACP child calls the last one between tool batches…"). Worth a re-read.
  • Model inheritance. spawnOrAttach gets only workspaceCwd + optional modelServiceId, so an isolated fire runs under the daemon default model, not the model the anchor session was using. Probably intended — worth documenting.

What's good

  • Back-compat is handled properly at every layer: absent runMode'shared'; unknown value routes through fix-or-delete in cronTasksFile.ts rather than silently degrading; toView normalizes so the client never sees undefined.
  • Capturing lastEventId before sendPrompt to avoid early-chunk loss is correct, and it holds against the real eventBus (abort closes the iterator cleanly rather than throwing, so the timeout path really does return partial text). The reaper's promptActive guard also means a sent sub-session won't be killed mid-turn — the module doc's claim checks out.
  • Bidi + terminal-control sanitization on the display name, built from a string literal so no invisible chars land in source. Nice touch.
  • toAutoClassifierInput surfacing the delegated prompt is exactly right — without it the classifier would be blind to what the sub-session gets asked to do.
  • Test suites have teeth: timeout, truncation, turn_error, cap overflow, stop(), and the "no subscribe in sent mode" assertion.

@wenshao

wenshao commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Suggestions — commit 7983c8d05

File Issue Suggested fix
packages/cli/src/serve/create-sub-session.ts:424-449 Orphaned event subscription when turnError wins the first-turn race — awaitFirstTurn's internal AbortController is never aborted, so the for await loop consumes events until the 5-min timeout fires Create a caller-owned AbortController, compose it into the signal passed to awaitFirstTurn, and abort it in the finally block
packages/acp-bridge/src/bridgeClient.ts:1029-1037 model parameter silently dropped when > 128 chars — conditional spread omits the field without error, while prompt and name throw explicit RequestError.invalidParams Throw RequestError.invalidParams for consistency, or at minimum log a warning when silently dropped
packages/cli/src/acp-integration/session/Session.ts:2887-2908 Isolated cron fire failures silently dropped — when dispatch fails (concurrency cap, bridge unavailable), fire is permanently lost with only debugLogger.warn Log at error level and record the failure in the task's run history for user visibility
packages/cli/src/acp-integration/session/Session.ts:2902-2917 #dispatchIsolatedCronFire has no timeout on the spawner call — if spawnOrAttach hangs, the promise never settles Wrap in Promise.race with a 30-second timeout
packages/core/src/tools/create-sub-session.ts:283-299 Tool-layer name length validation missing — validateToolParamValues checks prompt against MAX_SUB_SESSION_PROMPT_CHARS but omits name length check Add name length validation mirroring the bridge's MAX_SUB_SESSION_NAME_CHARS check

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

⚠️ Downgraded from Request Changes to Comment: self-PR; CI failing: review-pr.

Build: ✅ passed · Tests: ❌ 227 failures in Session.test.ts (222) and Session.worktree.test.ts (5) — this.config.setSubSessionSpawner is not a function. The mock Config in both files is missing setSubSessionSpawner: vi.fn(). Add the stub to the mockConfig object in both test files.

Suggestion-level recommendations are in the Suggestion summary comment below.

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

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

Copy link
Copy Markdown
Collaborator

Suggestions — commit 2da58d63d6a

File Issue Suggested fix
packages/acp-bridge/src/bridgeClient.ts:1031 model parameter silently dropped when invalid (non-string, empty, or >128 chars) instead of rejecting with invalidParams like prompt and name Validate and reject invalid model values explicitly with RequestError.invalidParams, or at minimum log a warning. Extract 128 to a named constant.
packages/web-shell/client/components/messages/ToolGroup.tsx:1306-1337 renderWithSessionLinks regex-scans ALL tool result strings for qwen-session:// links, not just create_sub_session output. Any tool output containing that markdown pattern gets silently transformed into clickable session-navigation links Scope renderWithSessionLinks to only create_sub_session tool results by checking tool.name before calling it
packages/cli/src/acp-integration/session/Session.ts:2905 Silent fallback to shared mode when runMode === 'isolated' but spawner is undefined — no log or metric Add a debugLogger.warn() when falling through to shared mode so the fallback is observable
packages/cli/src/serve/create-sub-session.ts:345 sendPrompt payload cast with as Parameters<...>[1] bypasses structural type checking — future refactors of sendPrompt's signature won't flag this call site Import the parameter type explicitly or extract a typed helper
packages/cli/src/serve/create-sub-session.ts:425 First-turn path returns stopReason to caller but never logs it daemon-side — abnormal outcomes (timeout, error, shutdown) leave no daemon log Log abnormal stopReasons (!== 'end_turn') before returning
packages/cli/src/acp-integration/session/Session.ts + cronScheduler.ts Isolated cron fire sub-sessions get prompt-derived display names instead of the human-readable task name — #dispatchIsolatedCronFire doesn't pass name to the spawner Thread name through CronJobdurableTaskToJob → spawner call
packages/cli/src/serve/create-sub-session.test.ts No test covers sent-mode drain releasing its concurrency slot when sendPrompt rejects Add a test using sendPromptRejects in sent mode; verify all 5 slots are freed after rejection
packages/acp-bridge/src/bridgeClient.test.ts No test exercises ownsSession() => false — the session-forgery defense is dead code from the test suite's perspective Add a test that passes ownsSession() => false and asserts rejection

— qwen3.7-max via Qwen Code /review

@wenshao wenshao force-pushed the feat/isolated-scheduled-tasks-sub-session branch 2 times, most recently from 8d80086 to c772160 Compare July 8, 2026 12:03
@wenshao

wenshao commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Review: feat(scheduled-tasks): add isolated run mode via create_sub_session tool

Nice shape overall — the seam (Config.setSubSessionSpawnerSession.extMethodBridgeClient.handleCreateSubSessionSubSessionLauncher) is clean, the daemon-only gate falls out of "no spawner wired" rather than a mode flag, runMode is absent-means-shared on disk so legacy tasks stay byte-identical, and the getDefaultPermission(): 'ask' + toAutoClassifierInput pairing correctly mirrors cron_create's reasoning about AUTO-mode short-circuit.

Three blocking items below (two are red CI, one I reproduced with a probe), plus some design concerns about isolated being model-mediated rather than enforced.

How I verified: worktree at 8d80086, ran the touched suites. The 6 bridgeClient.test.ts failures (artifact ingress, reverse tool channel) also fail at the merge-base 271664b — pre-existing, not this PR. The two failures below are new.


🔴 Blocking

1. CI is red — two of this PR's own new tests fail.

  • packages/cli/src/serve/create-sub-session.test.ts:130

    // 'sent' must NOT wait on / subscribe to the event stream.
    expect(fake.subscribeCalls()).toBe(0);   // AssertionError: expected 1 to be +0
    

    The implementation deliberately does subscribe in sent mode (create-sub-session.ts:267-283) to hold the concurrency slot. The test is stale, not the code — and so is the module docblock, which still says only 'first-turn' subscribes:

    *  - 'sent'       — dispatch the prompt and return { sessionId } immediately;
    *  - 'first-turn' — subscribe to the sub-session's event stream, ...
    

    Fix the assertion (toBe(1)), the test name ((no subscribe)), and the docblock together.

  • packages/web-shell/client/components/dialogs/ScheduledTasksDialog.test.tsx:242

    click(findButton('View history (1)'));   // Error: click target not found
    

    The real label is View conversation (1) (see the passing test at :479). The test throws before reaching expect(onOpenSession).toHaveBeenCalledWith('anchor-1'), so the behavior it claims to cover is currently unverified.


2. stop() does not tear down in-flight first-turn awaits — but the comment says it does.

create-sub-session.ts:187-190:

// Shared AbortController — stop() aborts it, tearing down every active
// subscription (first-turn awaits AND sent-mode background drains). This
// prevents shutdown from waiting up to 5 min per in-flight session.
const stopAc = new AbortController();

stopAc.signal reaches the sent drain (:267) but is never passed to awaitFirstTurn — that function builds its own AbortController from timeoutMs alone (:105). Probe (fake bridge, firstTurnTimeoutMs: 10_000):

PROBE A: after stop(), first-turn launch still pending = true

So stopSubSessionLauncher() in run-qwen-serve.ts buys nothing for the case the comment is about: on daemon shutdown an in-flight first-turn caller hangs for the full FIRST_TURN_TIMEOUT_MS (5 min). Suggested fix — compose the signals and distinguish the reason:

const ac = new AbortController();
const composed = AbortSignal.any([ac.signal, stopSignal]);   // pass stopAc.signal in
// ...
if (stopReason === undefined) {
  stopReason = stopSignal.aborted ? 'shutdown' : ac.signal.aborted ? 'timeout' : 'incomplete';
}

Relatedly, sendPrompt is called with undefined for the abort signal (:245-250), so the sub-session's turn is never cancelled either. Worth stating that explicitly in the docblock rather than implying stop() cleans up.


3. The per-caller concurrency slot is released twice when awaitFirstTurn rejects → the cap over-admits.

bridge.subscribeEvents() is a plain function, not an async generator — it throws SessionNotFoundError synchronously (bridge.ts:3023), and EventBus.subscribe() throws SubscriberLimitExceededError (eventBus.ts:515). Both propagate out of awaitFirstTurn, hitting both release sites:

try {                                        // outer, :210
  ...
  try {
    const { result, stopReason } = await awaitFirstTurn(...);   // throws
    return { sessionId, result, stopReason };
  } finally {
    release(key);                            // :299  release #1
  }
} catch (err) {
  release(key);                              // :303  release #2  ← double
  ...
}

Probe, with 4 launches genuinely in flight and one failing:

BASELINE: cap holds — 6th concurrent first-turn launch is rejected     ✓
BUG: after 1 failure with 4 in flight → extra1 admitted=true, extra2 admitted=true

Two more admitted where one should have been rejected — 6 concurrent under a cap of 5. Each failure loosens the cap by one slot; repeated failures drive the counter to 0 while N sub-sessions are live. Make the release idempotent per acquire:

let released = false;
const releaseOnce = () => { if (!released) { released = true; release(key); } };

Second-order: that failing launch already completed spawnOrAttach and sendPrompt, so a real sub-session is running with its prompt dispatched — yet launch() reports failure and frees the slot. Consider bridge.closeSession(sessionId) on the post-spawn error path so it isn't orphaned.


🟡 Design concerns

4. isolated is model-mediated, not enforced. Session.onFire only wraps the prompt text asking the model to call create_sub_session. If the model ignores the instruction — or the tool call is denied, or rejected by the cap (see #8) — the task runs inline in the shared session, i.e. exactly what isolated promises not to do, with no signal to the user and no visible failure. For a persisted, user-chosen config field that's a weak contract. Options: dispatch deterministically from onFire via the spawner (no model in the loop), or at minimum detect an isolated fire that completed without a create_sub_session call and surface it.

The wrapper text is also duplicated verbatim in two places — Session.ts:221 (wrapIsolatedTaskPrompt) and ScheduledTasksDialog.tsx:31 (wrapIsolatedRunPrompt, whose own comment says "Mirrors the server-side ..."). A load-bearing prompt that must stay byte-identical across a package boundary should be one exported constant, not two copies.

5. Unattended fires have to clear an ask permission. getDefaultPermission(): 'ask' is right for the AUTO-classifier reason you document, but a cron fire in an anchor session has no human attached. In DEFAULT approval mode the create_sub_session call goes to the permission mediator with no voter and times out → the isolated task silently doesn't run. Please document the required approval mode (AUTO/YOLO) or add a test pinning the behavior.

6. Nothing reads the sub-session's output, and the UI can't reach it. Both wrappers instruct completion: "sent", and the new web-shell test asserts the history button opens the anchor session. So an isolated task's actual work lands in a transcript the user has no navigation path to; the anchor transcript contains only "dispatched" confirmations. The 🧵 prefix helps in the session list, but a link from the run entry would be much better.

7. packages/webui/src/daemon/workspace/types.ts:191-194 documents behavior that doesn't exist.

* `'isolated'` spawns a fresh session per fire
* (each run recorded with its own `runs[].sessionId`).

Neither half holds: the daemon spawns nothing (the model does, #4), and cronScheduler.ts:1326 always records ...(this.sessionId ? { sessionId: this.sessionId } : {}) — the anchor. Your own cronScheduler.test.ts asserts sessionId: 'session-1' for an isolated task, and the web-shell test asserts the button opens anchor-1. Fix the comment before it becomes the spec someone implements against.

8. The cap is keyed on the anchor session id, which is constant across fires. With sent mode holding a slot for the whole sub-session turn, an isolated task that fires faster than its runs complete will hit Too many concurrent sub-sessions on the 6th fire — and the model's likely fallback is to do the work inline (#4). Worth calling out in the runMode.isolated hint, or reconsidering the key/eviction policy.


🟢 Nits

  • create-sub-session.ts:267AbortSignal.any([stopAc.signal]) over a single signal; just pass stopAc.signal.
  • create-sub-session.ts:239-242lastEventId is hard-coded to 0 for sent. It happens to be correct (a fresh bus starts at id 1, and EventBus.subscribe gates replay on lastEventId !== undefined, so 0 replays from the start and the drain can't miss an early turn_complete). That's subtle and load-bearing; call getSessionLastEventId(sessionId) unconditionally instead.
  • appendChunk (:120-128) can slice through a surrogate pair at MAX_RESULT_CHARSsubSessionName already guards this for names.
  • config.ts:6385 registers the tool unconditionally, so in TUI/headless the model can call a tool that always errors. shouldDefer: true caps the token cost, but a wasted turn seems worse than the tool being absent. Consider gating on daemon mode.
  • The PR body's "Minor fixAuthMessage placeholder now uses i18n key" doesn't correspond to anything in the diff — stale body.

中文小结

整体结构很好:Config.setSubSessionSpawnerSession.extMethodBridgeClientSubSessionLauncher 这条 seam 很干净,daemon-only 由"未接线 spawner"自然导出,runMode 缺省即 shared(磁盘表示不变),getDefaultPermission(): 'ask' + toAutoClassifierInputcron_create 的推理一致。

阻塞项(3 个)

  1. CI 红 —— 本 PR 新增的两个测试自身失败。 create-sub-session.test.ts:130 断言 sent 模式不订阅事件流,但实现为了持有并发槽确实会订阅(:267-283)——测试和模块文档注释是过时的,代码是对的ScheduledTasksDialog.test.tsx:242 找的按钮文案是 View history (1),实际是 View conversation (1)(见 :479 的通过用例),点击前就抛错,该用例声称覆盖的行为其实未被验证。

  2. stop() 并不会中断进行中的 first-turn,但注释声称会。 stopAc.signal 只传给了 sent 模式的后台 drain,从未传入 awaitFirstTurn(后者只用自己基于 timeoutMs 的 controller)。探针验证:stop() 之后 launch promise 仍 pending。守护进程关闭时调用方会挂满 5 分钟超时。建议 AbortSignal.any([ac.signal, stopSignal]),并用独立的 'shutdown' stopReason 区分。

  3. awaitFirstTurn 抛错时并发槽被释放两次,导致 cap 超发。 bridge.subscribeEvents() 是普通函数,SessionNotFoundError同步抛出的(bridge.ts:3023),SubscriberLimitExceededError 同理(eventBus.ts:515)。内层 finally:299)和外层 catch:303)各释放一次。探针:4 个真正在飞 + 1 次失败后,本应只能再进 1 个,实际进了 2 个(cap 5 → 并发 6)。建议按 acquire 做幂等释放。另外该次失败已经 spawnOrAttach + sendPrompt 过了,子会话在跑却被当成失败且释放了槽位 —— 建议在 spawn 之后的错误路径上 closeSession

设计层面

  1. isolated 是"靠模型自觉",不是强制的:onFire 只是包了一段提示词。模型不照做 / 工具被拒 / 撞到并发上限时,任务会在共享会话里内联执行,用户无感知。且这段提示词在 Session.ts:221ScheduledTasksDialog.tsx:31 逐字重复了两遍,应抽成一个共享常量。
  2. 定时触发无人值守,但 create_sub_session 默认 ask;DEFAULT 审批模式下没有投票方,权限请求会超时 —— 隔离任务会静默不执行。请文档化所需审批模式或补测试。
  3. 两处 wrapper 都用 completion: "sent",子会话的产出没人读;UI 的历史按钮打开的是 anchor 会话(新测试自己断言了这点),用户没有路径查看隔离运行的真实结果。
  4. packages/webui/.../types.ts:191-194 的注释("每次 fire 新建会话""每条 run 记录自己的 runs[].sessionId")与实现不符 —— cronScheduler.ts:1326 永远记 anchor 的 this.sessionId。请先修注释。
  5. cap 以 anchor sessionId 为 key(跨 fire 恒定),sent 模式又整轮持有槽位;高频 + 慢任务会在第 6 次触发时报 Too many concurrent sub-sessions

小问题:单元素 AbortSignal.anysent 分支硬编码 lastEventId: 0(碰巧正确且是 load-bearing,建议统一调 getSessionLastEventId);appendChunk 截断可能切断代理对;config.ts:6385 在 TUI/headless 无条件注册一个必然报错的工具;PR 描述里的 AuthMessage i18n 修复在 diff 中不存在。

@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 failing: review-pr, Test (ubuntu-latest, Node 22.x).

Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/cli/src/serve/run-qwen-serve.ts
Comment thread packages/acp-bridge/src/bridgeClient.ts
Comment thread packages/cli/src/serve/create-sub-session.test.ts Outdated
Comment thread packages/web-shell/client/components/dialogs/ScheduledTasksDialog.test.tsx Outdated
@wenshao wenshao force-pushed the feat/isolated-scheduled-tasks-sub-session branch from c772160 to 1ed248d Compare July 8, 2026 12:29

@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 failing: review-pr, Test (ubuntu-latest, Node 22.x).

Comment thread packages/cli/src/serve/create-sub-session.ts Outdated

@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 failing: Test (ubuntu-latest, Node 22.x).

Comment thread packages/cli/src/serve/create-sub-session.ts Outdated
Comment thread packages/cli/src/serve/create-sub-session.ts Outdated
@wenshao wenshao force-pushed the feat/isolated-scheduled-tasks-sub-session branch from 1ed248d to 9f38ed5 Compare July 8, 2026 13:19

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

No new review findings in the incremental diff since the last reviewed commit. Downgraded from Approve to Comment: self-PR; CI still running.

— GPT-5 via Qwen Code /review

Comment thread packages/acp-bridge/src/bridgeClient.ts
Comment thread packages/cli/src/serve/create-sub-session.ts
Comment thread packages/cli/src/serve/create-sub-session.ts Outdated
@wenshao wenshao force-pushed the feat/isolated-scheduled-tasks-sub-session branch from 9f38ed5 to a2bf76b Compare July 8, 2026 14:36

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

— qwen3.7-max via Qwen Code /review

Comment thread packages/cli/src/serve/create-sub-session.ts
Comment thread packages/cli/src/serve/create-sub-session.ts Outdated
Comment thread packages/cli/src/serve/create-sub-session.ts Outdated
@wenshao wenshao force-pushed the feat/isolated-scheduled-tasks-sub-session branch from a2bf76b to 2026267 Compare July 8, 2026 16:43

@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 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. One Critical finding is posted inline — the latest commit turns create-sub-session.test.ts red on a clean tree.

— Opus 4.8 (1M context) via Qwen Code /qreview

Comment thread packages/cli/src/serve/create-sub-session.ts Outdated
Comment thread packages/cli/src/serve/create-sub-session.test.ts Outdated
Comment thread packages/core/src/tools/create-sub-session.ts Outdated
Comment thread packages/cli/src/serve/create-sub-session.test.ts
Comment thread packages/core/src/permissions/permission-manager.ts
Comment thread packages/core/src/tools/create-sub-session.ts Outdated
…e model

An `isolated` fire was relayed through the model: the fired prompt was
wrapped with an instruction to call `create_sub_session`. That tool's
default permission is `'ask'`, so under `ApprovalMode.DEFAULT` an
unattended fire reached `client.requestPermission`, found no SSE
subscriber, and was cancelled by the daemon's 5-minute permission
timeout. The task never ran, and the cancel was booked as a successful
run — the headline use case of a scheduled task was broken.

Route isolated fires straight to the daemon instead: the cron `onFire`
handler in `Session` calls the sub-session spawner directly, with no
model relay and no tool-permission gate. The prompt was already approved
when the task was created; laundering it back through the model only
re-opened that gate. `create_sub_session` keeps `'ask'` for
model-initiated calls, and the attended "Run now" button keeps its relay
(a user is present to answer the prompt).

Also fix orphan-session cleanup in the launcher. `closeSession` was
guarded only by `.catch()`, which covers an async rejection but not a
synchronous throw; because the call sits inside the launcher's own
`catch (err)` block, a sync throw escaped and replaced the real launch
error. Guard both shapes.

Tests:
- Cover isolated routing: dispatch, in-session fallback with no spawner,
  missed one-shot, dispatch failure (dropped, never run inline), and
  shared mode.
- Cover the orphan close, including a `closeSession` that throws.
- Replace the sent-mode concurrency test, which only asserted the slot
  was eventually released (moving the release to the drain's *start*
  kept it green) with one that asserts the slot is HELD while the drain
  runs, plus one that asserts it is released at `turn_complete`.

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

Reviewed — no new blockers found in this round. Build passes, 285 tests pass (0 failures). Several Critical issues from prior review rounds remain open (recursive nesting depth, unbounded prompt size, callerSessionId validation, AbortSignal propagation). Suggestion-level recommendations are in the Suggestion summary comment below.

— qwen3.7-max via Qwen Code /review

…pawn boundary

Four findings from review, all in the model-initiated `create_sub_session`
path (the scheduled `isolated` dispatch reaches none of them).

`execute()` took no parameters, so it silently dropped the parent turn's
`AbortSignal`. `Session.ts` awaits `invocation.execute(signal)` without
racing the abort itself, so cancelling a turn with a `first-turn`
sub-session in flight pinned the caller's tool loop until the daemon's
5-minute ceiling. Accept the signal and return as soon as it fires.

The sub-session is deliberately NOT cancelled and deliberately KEEPS its
concurrency slot: `sendPrompt` has no abort seam, so the sub-session runs
on. Releasing its slot on cancel — as the review suggested — would let the
caller over-admit against sub-sessions that are still consuming a bridge
session and model quota.

`handleCreateSubSession` trusted the child-supplied `callerSessionId`
verbatim, and that id keys the launcher's per-caller concurrency bucket: a
fabricated id starts a fresh bucket at zero (cap evasion) and a victim's id
burns their slots (DoS). Validate it with the connection's existing
`ownsSession` seam.

Every daemon session wires a spawner, sub-sessions included, and each gets
its own cap-sized bucket — so one prompt could fan out 5ⁿ sub-sessions until
`maxSessions` ran dry. Gate nesting at one level: the launcher remembers the
sessions it spawned and refuses to spawn from them. With `callerSessionId`
now authenticated, the gate cannot be sidestepped.

Cap the prompt at 100,000 chars (matching the scheduled-task REST route) and
the display name at 200, both at the bridge trust boundary and, for the
prompt, in the tool's own validation so the model gets an actionable error.

Not changed: `create_sub_session` stays in `PermissionManager.CORE_TOOLS`.
Membership there SUBJECTS a tool to the `coreTools` allowlist; it does not
exempt it. Removing it — as the review suggested — is what would let the
tool bypass a user's allowlist, the way `agent` and `send_message` do today.
@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Review round handled — 96760f710 + c1e3ef03d

I've taken this PR over. Two pushes: one that changes how isolated fires are dispatched, one that closes the outstanding review findings.

The design changed: isolated fires no longer go through the model

@LaZzyMan — your blocking finding is fixed, though not with either of the two suggested forms. void bridge.closeSession(id).catch(() => {}) is what the previous commit already did, and it is what turned CI red: the call sits inside the launcher's own catch (err) block, so .catch() handles the async rejection but a synchronous throw escapes and replaces the real launch error. Both guards are needed:

try {
  void bridge.closeSession(spawnedSessionId).catch(() => {});
} catch (closeErr) {
  log.debug('sub-session: closeSession threw', spawnedSessionId, closeErr);
}

Beyond that: the isolated run mode was implemented by wrapping the fired prompt with an instruction for the model to call create_sub_session. That tool's permission is 'ask', and needsConfirmation('ask', DEFAULT, …) is true, so an unattended fire under the default approval mode reached client.requestPermission, found no SSE subscriber, and was cancelled by the daemon's 5-minute permission timeout — then booked as a successful run. The headline use case of a scheduled task did not work.

Session's cron onFire now calls the sub-session spawner directly (completion: 'sent'): no model relay, no tool-permission gate. wrapIsolatedTaskPrompt is deleted. The tool keeps 'ask' for model-initiated calls, and the attended "Run now" button keeps its relay (a user is present to answer). Full rationale is in the PR body.

Findings closed this round

Finding Outcome
closeSession sync throw masks the real error (CI red) Fixed; regression test for sync and async failure, plus the orphan-close assertion that was missing entirely
sent-mode cap test never asserts the hold Split into hold + release tests; both mutation-checked
execute() drops the parent turn's AbortSignal Fixed — and it was worse than reported: Session.ts:5087 is a bare await, so this pinned the caller's tool loop, not just a slot
Unbounded recursive nesting (5ⁿ fan-out) Fixed — depth-1 gate in the launcher
callerSessionId trusted verbatim Fixed — validated against the connection's ownsSession
Unbounded prompt size Fixed — 100,000 chars at the bridge boundary and in tool validation; name capped at 200
create_sub_session in CORE_TOOLS bypasses the allowlist Refuted — inverted; see the inline reply
Release the concurrency slot on cancel Refuted — the sub-session keeps running, so releasing would over-admit; see the inline reply

Three of those (nesting, callerSessionId, prompt size) were sitting in threads marked resolved with no corresponding code change. They are actually fixed now.

Every new test was mutation-checked: reverting the fix it guards makes exactly that test fail. The execute() cancellation test times out at 5s against the old code.

Known limitation

runs[].sessionId still records the bound (anchor) session, not the spawned sub-session, so clicking a run of an isolated task opens an empty transcript. The scheduler persists the run record at fire time, before the spawn resolves.

@LaZzyMan could you re-review?

@LaZzyMan LaZzyMan 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.

Follow-up review on head c1e3ef03d:

  • The previous async closeSession() cleanup blocker is fixed: the code now keeps both the synchronous try/catch and async .catch() guard.

New blocking finding:

  • packages/core/src/tools/create-sub-session.ts:138-145 still starts the spawn before honoring an already-aborted tool signal. spawner({...}) is evaluated before raceCancellation() runs, so a pre-aborted AbortSignal still calls the daemon spawner and may create/dispatch a sub-session, then returns Cancelled. That violates the cancellation fix this round is meant to provide: if the parent turn was already cancelled before execute() starts, the tool should not create new daemon work. This is not guarded by Session; it directly awaits invocation.execute(activeToolAbortSignal). Please check signal.aborted before invoking spawner, or pass a thunk into the race helper so the spawn starts only after the pre-abort check. Add a regression test with an already-aborted signal asserting the spawner is not called.

Verification context: PR head c1e3ef03d against refreshed origin/main c9a80996; GitHub Ubuntu test is passing and review-pr is still in progress. Local focused vitest runs could not start in this worktree because package-local dependencies are missing (vitest/config resolution fails before test collection), so I did not use local test failures as PR evidence.

`raceCancellation(spawner({…}), signal)` evaluated the spawner as an
argument, so the spawn started before the abort was ever checked. A turn
cancelled before `execute()` ran still created a sub-session on the daemon
— and it kept a concurrency slot — while the tool reported itself
cancelled.

Take a thunk instead, so the pre-abort check happens before any daemon work
is started. Track whether the spawn actually began, and say so: "cancelled
before it started, no sub-session was created" is a different fact from
"a sub-session may already have been created and is not cancelled".

Regression test asserts the spawner is never called for a pre-aborted
signal; it fails against the eager-argument form.
@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

@LaZzyMan good catch — confirmed and fixed in c7bf29ade.

You're right that the abort check ran too late. raceCancellation(spawner({…}), signal) evaluates spawner({…}) as an argument, so the spawn was already in flight by the time the helper looked at signal.aborted. A pre-cancelled turn created a sub-session on the daemon (and consumed a concurrency slot) and then reported itself Cancelled.

Fixed by taking a thunk, so nothing starts before the check:

async function raceCancellation<T>(
  start: () => Promise<T>,
  signal: AbortSignal,
): Promise<T | typeof CANCELLED> {
  if (signal.aborted) return CANCELLED;   // no daemon work started
  const spawn = start();
  
}

Two follow-ons from your finding:

  • The two cancellation outcomes are now distinguished. A spawnStarted flag, set inside the thunk, decides between "cancelled before it started. No sub-session was created." and "a sub-session may already have been created; it runs independently and is not cancelled." The old single message hedged in the case where we know for certain nothing exists.
  • Regression test added as you asked: never calls the spawner when the signal is already aborted asserts spawner is not called, that returnDisplay is Cancelled, and that the message does not hedge. Mutation-checked — moving const spawn = start() back above the signal.aborted check fails exactly that test and nothing else.

On the local-test note: vitest/config fails to resolve because a fresh worktree has no node_modules. cp -al <main-checkout>/node_modules <worktree>/node_modules (plus the same for packages/*/node_modules) hardlinks them in seconds — but only if the worktree is on the same filesystem as the source checkout; across filesystems cp -al silently degrades and you get a nested node_modules/node_modules.

Could you take another look?

@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 failing: review-pr.

Two new Critical findings posted inline; prior open comments from earlier rounds still apply. Suggestion-level recommendations are in the Suggestion summary comment below.

Comment thread packages/cli/src/serve/create-sub-session.ts Outdated
Comment thread packages/acp-bridge/src/bridgeClient.ts Outdated
…ream close

Two findings from review.

`awaitFirstTurn`'s `'incomplete'` stopReason was unreachable. The cleanup
`finally` calls `ac.abort()` unconditionally to tear the subscription down,
so by the time the stopReason ternary read `ac.signal.aborted` it was always
true. An event stream that closed before the turn finished (bridge teardown,
WS drop) was reported as a 5-minute wall-clock `'timeout'` — indistinguishable
from a real one. Track the timer firing in its own flag.

`callerSessionId` was validated only when present. Omitting it handed the
launcher `undefined`, which minted an `anon:<uuid>` bucket — a fresh
concurrency bucket per call, so no cap — and skipped the depth-1 nesting gate
(`info.callerSessionId !== undefined && …`). Authenticating the id closed
forgery but not omission. It is now required at the bridge boundary, and
required in `CreateSubSessionInfo`, so the launcher's anonymous fallback and
the gate's presence check are both gone. Every real caller has a session id —
the tool only ever runs inside a session's turn.

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

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

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

Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/serve/create-sub-session.ts

@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. One Critical finding is posted inline.

— GPT-5 via Qwen Code /review

Comment thread packages/acp-bridge/src/bridgeClient.ts
@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Independent E2E verification — real qwen serve daemon, real web shell

I verified this end-to-end against a real qwen serve daemon (only the OpenAI backend is mocked), plus a merge-base A/B build. Everything the PR description claims is true. I also found one real bug, described at the bottom — non-blocking, and I include a fix.

Recommendation: merge, ideally with the [object Object] fix folded in.

Setup: PR head 7983c8d05 vs merge-base 271664b34, each npm ci-built in its own worktree. Isolated $HOME, mock OpenAI SSE endpoint, qwen serve --port 8801 --workspace <tmp>. Cron tasks use * * * * *; the daemon is restarted after task creation so boot rehydration arms the anchor schedulers.


1. The headline behavior: isolated really does spawn a fresh sub-session per fire

Same two tasks, same mock, same cron — only the build differs.

A/B terminal

merge-base PR 6535
POST runMode:'isolated' silently accepted and dropped (runMode absent from view and from disk) persisted, echoed as "isolated"
POST runMode:'bogus' 201 — unknown field ignored 400 invalid_run_mode
isolated task's anchor transcript, after 2 fires 2 turns 0 turns
shared task's anchor transcript, after 2 fires 2 turns 2 turns (unchanged)
fresh 🧵 sub-sessions spawned 0 2 — one per fire

Each isolated fire lands in its own session with exactly one clean turn (ISOLATED_TASK_PROMPTISOLATED_RUN_OK), while the bound anchor session's transcript stays completely empty. The shared task is byte-for-byte unchanged: runMode is omitted from disk when 'shared', and switching a task to isolated and back via PATCH clears the field again.

No permission request is ever raised. Verified in a dedicated run with tools.approvalMode left unset (i.e. default): two unattended isolated fires, two fresh sub-sessions, anchor at 0 turns, and 0 requestPermission calls in the daemon log. This is the part that confirms dispatching the scheduled fire through Session.#dispatchIsolatedCronFire — rather than laundering it back through the 'ask'-gated tool — is load-bearing.

Also verified:

  • Corrupt runMode on diskGET /scheduled-tasks returns 500 scheduled_tasks_read_failed, i.e. the fix-or-delete contract, not a silent downgrade to shared. Repairing the value restores service.
  • runs[].sessionId records the anchor, not the sub-session — the known limitation, reproduced. All four run records for the isolated task point at the empty anchor.

2. The create_sub_session tool

claim result
daemon-only outside qwen serve ✅ headless -p run returns the DAEMON_ONLY_MESSAGE verbatim and exits cleanly
first-turn returns the child's output ✅ parent's tool result: Sub-session [🧵 c38d2f35](qwen-session://…) first-turn result (stopReason: end_turn):\n\nHELLO_FROM_CHILD
sent returns immediately … created and the prompt was dispatched
depth-1 nesting cap ✅ a sub-session's create_sub_session call is refused; no grandchild session is created
per-caller concurrency cap of 5 ✅ 6 sent spawns from one session with hanging children → exactly 5 sub-sessions created, 6th refused. sent mode genuinely holds its slot via the background drain
callerSessionId authenticated at the bridge ✅ patched the compiled child to send 'forged-1234-not-mine' → daemon replies -32602 Invalid params: \callerSessionId` is required and must name a session owned by this connection`, no session created
name used for the session title ✅ session list shows 🧵 probe child, not the prompt

3. Web Shell UI — driven with Playwright against the real packages/web-shell/dist

run mode picker before/after

Two radios, Shared session checked by default, hint text swaps on selection. Editing an existing isolated task re-selects Isolated (isolated checked = true), so the runMode API round-trip is visible in the UI.

The qwen-session:// deep link works end-to-end — the anchor renders in the expanded tool card, a real click dispatches qwen:open-session, and App.tsx navigates to the sub-session's own transcript:

session deep link

Confirming your own note: a shared and an isolated card render identically in the task list — there is no run-mode indicator. Combined with runs[].sessionId pointing at the empty anchor, a user who picks isolated gets a task list that never shows the mode and a View conversation button that always opens an empty transcript. Worth the follow-up you filed.

task list


4. Bug found: every daemon-side rejection reaches the model as [object Object]

object Object finding

@agentclientprotocol/sdk rejects extMethod with the raw JSON-RPC error object, not an Error:

// node_modules/@agentclientprotocol/sdk/dist/acp.js:850
else if ("error" in response) { pendingResponse.reject(response.error); }

Session.#registerSubSessionSpawner passes that rejection straight through, and the tool's catch does:

// packages/core/src/tools/create-sub-session.ts:200
const message = error instanceof Error ? error.message : String(error);
//                                                        ^^^^^^^^^^^^^ String({…}) === "[object Object]"

So while the daemon logs the real reason —

code: -32603, message: 'Internal error',
data: { details: 'Too many concurrent sub-sessions for this session (cap 5); wait for one to finish.' }

— the model is told:

Error creating sub-session: [object Object]

Everything that can reject is affected: the depth-1 nesting gate, the per-caller concurrency cap, callerSessionId authentication, the 100k prompt cap, the 200-char name cap, and methodNotFound. That last one matters for the doc claim in bridgeOptions.ts"Omitted by tests / Mode A / non-daemon embeds — the tool then reports itself unavailable (daemon-only)" — an embed that injects deps.bridge (so onCreateSubSession is never wired) gets [object Object], not the daemon-only message. The DAEMON_ONLY_MESSAGE path only fires when getSubSessionSpawner() is undefined, which is TUI/headless only.

The concurrency cap is the sharpest case: its message is deliberately written to be actionable (wait for one to finish), and the launcher's comment says it is "surfaced as the tool's error, never silently dropped" — but the model can't read it.

Why the existing tests don't catch it: create-sub-session.test.ts's surfaces a spawner error as a tool error rejects with new Error('spawn boom'). The real spawner never rejects with an Error. The fake and the real spawner have different rejection contracts.

Note the same formatter appears in Session.#dispatchIsolatedCronFire's catch (err instanceof Error ? err.message : String(err)), so a dropped isolated fire logs the fire was dropped [session …]: [object Object] too. That call site I confirmed by reading, not by executing.

Suggested fix (12 lines) — red→green, and the PR's own 11 tests still pass
/**
 * The ACP SDK rejects `extMethod` with the RAW JSON-RPC error object
 * (`{ code, message, data }`), never an `Error`. `String(obj)` is
 * "[object Object]", which would hide every daemon-side reason. A thrown
 * `Error` puts the reason in `data.details`; `invalidParams` in `message`.
 */
function describeSpawnError(error: unknown): string {
  if (error instanceof Error) return error.message;
  if (error && typeof error === 'object') {
    const e = error as { message?: unknown; data?: { details?: unknown } };
    const d = e.data?.details;
    if (typeof d === 'string' && d.length > 0) return d;
    if (typeof e.message === 'string' && e.message.length > 0) return e.message;
  }
  return String(error);
}

then in execute()'s catch: const message = describeSpawnError(error);

With that applied, the live 6th spawn returns:

Error creating sub-session: Too many concurrent sub-sessions for this session (cap 5); wait for one to finish.

A regression test should reject with the real shape, not an Error:

const spawner = vi.fn(async () => {
  throw { code: -32603, message: 'Internal error',
          data: { details: 'Too many concurrent sub-sessions …' } };
});
expect(res.error?.message).toContain('Too many concurrent sub-sessions');

Minor notes

  • ownsSession is bound to the channel's session set (sessionIds.has(...), bridge.ts:1706), and one qwen --acp child multiplexes a workspace's sessions. So a compromised child could name a sibling session's id in callerSessionId — it cannot cross workspaces. The hardening note ("a child cannot forge a fresh bucket or burn a victim session's slots") holds against a malicious prompt (which never controls the field), which is the threat that matters; it's slightly stronger than the cross-session guarantee. No action needed, just calibrating the claim.
  • model longer than 128 chars is silently dropped at the bridge rather than rejected, unlike prompt/name which 400. Cosmetic.
中文版

独立端到端验证 —— 真实 qwen serve daemon + 真实 Web Shell

我用真实的 qwen serve daemon(只 mock 了 OpenAI 后端)加上 merge-base 对照构建,做了完整的端到端验证。PR 描述中的所有声明都属实。 另外发现一个真实 bug,写在最后 —— 不阻塞合并,并附上修复。

结论:建议合并,最好把 [object Object] 的修复一并带上。

环境:PR head 7983c8d05 vs merge-base 271664b34,各自独立 worktree 真实 npm ci 构建。隔离的 $HOME、mock OpenAI SSE 端点、qwen serve --port 8801 --workspace <tmp>。定时任务用 * * * * *;创建任务后重启 daemon,让启动时的 rehydration 装载锚点会话的调度器。

1. 核心行为:isolated 确实为每次触发创建全新子会话

同样两个任务、同样的 mock、同样的 cron,只有构建不同。

merge-base PR 6535
POST runMode:'isolated' 静默接受并丢弃(视图和磁盘上都没有 runMode 正确持久化,回显 "isolated"
POST runMode:'bogus' 201 —— 未知字段被忽略 400 invalid_run_mode
两次触发后,isolated 任务锚点会话的对话轮数 2 0
两次触发后,shared 任务锚点会话的对话轮数 2 2(行为不变)
新建的 🧵 子会话数 0 2 —— 每次触发一个

每次 isolated 触发都落在自己的会话里,只有一轮干净的对话(ISOLATED_TASK_PROMPTISOLATED_RUN_OK),而绑定的锚点会话记录完全为空。shared 任务字节级不变:'shared' 时磁盘上省略 runModePATCH 切到 isolated 再切回来,字段会被清除。

从未触发权限请求。 这一点用一次专门的运行验证:tools.approvalMode 保持未设置(即 default),两次无人值守的 isolated 触发、两个全新子会话、锚点会话 0 轮对话、daemon 日志中 requestPermission 调用数为 0。这证明了「定时触发走 Session.#dispatchIsolatedCronFire 而不是绕回 'ask' 权限的工具」这个设计是必要的。

另外验证了:

  • 磁盘上 runMode 损坏GET /scheduled-tasks 返回 500 scheduled_tasks_read_failed,即 fix-or-delete 契约,而非静默降级为 shared。修复该值后服务恢复。
  • runs[].sessionId 记录的是锚点会话而非子会话 —— 已知限制,已复现。isolated 任务的四条运行记录全部指向那个空的锚点会话。

2. create_sub_session 工具

声明 结果
qwen serve 环境下仅 daemon 可用 ✅ headless -p 运行原样返回 DAEMON_ONLY_MESSAGE 并正常退出
first-turn 返回子会话输出 ✅ 父会话工具结果:Sub-session [🧵 c38d2f35](qwen-session://…) first-turn result (stopReason: end_turn):\n\nHELLO_FROM_CHILD
sent 立即返回 … created and the prompt was dispatched
嵌套深度上限为 1 ✅ 子会话调用 create_sub_session 被拒绝,没有创建孙会话
每调用方并发上限 5 ✅ 同一会话发起 6 次 sent 且子会话挂起 → 恰好创建 5 个子会话,第 6 个被拒。sent 模式确实通过后台 drain 占住了并发槽
callerSessionId 在 bridge 侧鉴权 ✅ 我修改编译后的 child 发送 'forged-1234-not-mine' → daemon 返回 -32602 Invalid params: \callerSessionId` is required and must name a session owned by this connection`,未创建任何会话
name 用作会话标题 ✅ 会话列表显示 🧵 probe child,而非 prompt

3. Web Shell UI —— Playwright 驱动真实的 packages/web-shell/dist

两个单选框,默认选中 Shared session,选择时提示文案随之切换。编辑已有的 isolated 任务会正确回显 Isolatedisolated checked = true),所以 runMode 的 API 往返在 UI 上可见。

qwen-session:// 深链完整可用 —— 链接渲染在展开的工具卡片中,真实点击会派发 qwen:open-sessionApp.tsx 跳转到子会话自己的记录。

确认你自己的注记:sharedisolated 的任务卡片渲染完全一致,没有运行模式指示。再加上 runs[].sessionId 指向空的锚点会话,用户选了 isolated 后,任务列表永远看不出模式,而「查看对话」按钮永远打开一个空白记录。你标记的后续跟进是必要的。

4. 发现的 bug:daemon 侧所有拒绝原因传到模型时都变成 [object Object]

@agentclientprotocol/sdk原始 JSON-RPC 错误对象(而非 Error)来 reject extMethod

// node_modules/@agentclientprotocol/sdk/dist/acp.js:850
else if ("error" in response) { pendingResponse.reject(response.error); }

Session.#registerSubSessionSpawner 把这个 rejection 原样透传,而工具的 catch 是:

// packages/core/src/tools/create-sub-session.ts:200
const message = error instanceof Error ? error.message : String(error);
//                                                        ^^^^^^^^^^^^^ String({…}) === "[object Object]"

于是 daemon 日志里有真实原因:

code: -32603, message: 'Internal error',
data: { details: 'Too many concurrent sub-sessions for this session (cap 5); wait for one to finish.' }

而模型收到的是:

Error creating sub-session: [object Object]

所有可能的拒绝路径都受影响:嵌套深度门、每调用方并发上限、callerSessionId 鉴权、100k prompt 上限、200 字符 name 上限,以及 methodNotFound。最后一个还牵涉到 bridgeOptions.ts 的文档声明 ——「Omitted by tests / Mode A / non-daemon embeds — the tool then reports itself unavailable (daemon-only)」—— 注入 deps.bridge 的 embed(因而从未接上 onCreateSubSession)得到的是 [object Object],而不是 daemon-only 提示。DAEMON_ONLY_MESSAGE 分支只在 getSubSessionSpawner()undefined 时触发,也就是仅限 TUI/headless。

并发上限这一条最尖锐:它的消息是特意写成可执行的(wait for one to finish),launcher 的注释也说它「surfaced as the tool's error, never silently dropped」—— 但模型根本读不到。

为什么现有测试没抓到: create-sub-session.test.ts 里的 surfaces a spawner error as a tool errornew Error('spawn boom') 来 reject。而真实的 spawner 永远不会用 Error reject。假 spawner 和真 spawner 的 rejection 契约不一致。

同样的格式化代码也出现在 Session.#dispatchIsolatedCronFire 的 catch 里(err instanceof Error ? err.message : String(err)),所以被丢弃的 isolated 触发在日志里同样是 the fire was dropped [session …]: [object Object]。这处调用点我是读代码确认的,没有实际执行。

修复见上方英文折叠块(12 行 describeSpawnError,红转绿,且 PR 自带的 11 个测试仍全部通过)。

其他小点

  • ownsSession 绑定的是channel 的会话集合(sessionIds.has(...)bridge.ts:1706),而一个 qwen --acp child 会复用同一 workspace 的多个会话。所以一个被攻陷的 child 可以在 callerSessionId 里填同 workspace 兄弟会话的 id —— 但跨不了 workspace。加固说明(「a child cannot forge a fresh bucket or burn a victim session's slots」)对恶意 prompt(永远控制不了这个字段)是成立的,这也是真正重要的威胁模型;只是措辞比跨会话的实际保证略强。无需改动,仅作校准。
  • 超过 128 字符的 model 在 bridge 侧被静默丢弃,而不像 prompt/name 那样返回 400。属于观感问题。

…ons per workspace

Three findings from review.

A dropped `isolated` scheduled fire left no trace. `debugLogger.warn` writes
nothing unless a debug log session is active, and the scheduler persists the
fire as a run before dispatch — so a nightly task could fail forever while its
history claimed it ran. It now also writes to stderr, which the daemon forwards
from the child.

A sent-mode drain that hit its 30-minute ceiling was equally silent: the catch
saw `drainAc.signal.aborted` and skipped logging, the `finally` freed the
concurrency slot, and the sub-session — which the abort does not cancel — kept
burning a bridge session and model quota. The timer now records its own firing
(the controller cannot: `finally` aborts it on every exit path) and the timeout
is written to stderr. The drain ceiling is injectable for tests, mirroring
`firstTurnTimeoutMs`.

The per-caller concurrency cap trusts `callerSessionId`, and the bridge can only
authenticate that id as "a session on this channel". Every session of a
workspace shares one child process, so nothing at the transport can prove which
of them issued the call — and a per-session secret would be readable by the
whole process anyway. Rather than pretend otherwise, add a workspace-wide
ceiling on concurrent sub-sessions that holds no matter which bucket a launch is
charged to.
@wenshao

wenshao commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@LaZzyMan LaZzyMan 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.

Follow-up review on head 94c66d6ba:\n\n- My previous blocker is fixed: create_sub_session now checks an already-aborted signal before invoking the spawner, and the regression test asserts the spawner is not called.\n- Reviewed the follow-up fixes for required callerSessionId, early stream-close stopReason, isolated scheduled-fire stderr logging, sent-mode drain-timeout logging, and the workspace-wide sub-session cap. I did not find a new blocking issue in this round.\n\nVerification context: GitHub Test (ubuntu-latest, Node 22.x) and web-shell E2E Smoke are passing on the current head. Local focused vitest commands for core/cli/acp-bridge could not start in this worktree because vitest/config is not resolvable before test collection. The automatic review-pr job is still in progress.

@qqqys qqqys 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.

One follow-up review comment on the current head.

— GPT-5 via Qwen Code /review

returnDisplay: `${sessionLink} completed${stop}`,
};
} catch (error) {
const message = error instanceof Error ? error.message : String(error);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] This still loses the useful JSON-RPC error details coming back from the bridge. Session.#registerSubSessionSpawner() awaits client.extMethod(...) directly, and ACP failures such as invalidParams, methodNotFound, or the launcher’s concurrency/depth-limit errors can reject as structured values like { code, message: 'Internal error', data: { details: 'Too many concurrent sub-sessions…' } }. For those, String(error) becomes [object Object], so the model sees Error creating sub-session: [object Object] instead of the actionable reason it needs to stop or wait.

Please normalize the same error shape the bridge already handles in extractErrorMessage: prefer data.details / data.message, then message, then String(error). The isolated scheduled-fire catch in Session.#dispatchIsolatedCronFire should use the same helper too, otherwise dropped isolated fires can still log [object Object] on these bridge rejection paths.

— GPT-5 via Qwen Code /review

@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. Solid feature PR — well-hardened through two rounds of review, 473 unit tests pass, independent E2E verification confirms all claims. One non-blocking suggestion: fold in the describeSpawnError fix for the [object Object] error formatting bug before merge. ✅

@wenshao wenshao added this pull request to the merge queue Jul 9, 2026
Merged via the queue into QwenLM:main with commit ac2f371 Jul 9, 2026
108 of 109 checks passed
wenshao added a commit to wenshao/qwen-code that referenced this pull request Jul 10, 2026
`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.
ytahdn pushed a commit to chiga0/qwen-code that referenced this pull request Jul 10, 2026
…enLM#6619)

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

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.

* fix(scheduled-tasks): harden the precondition against four review findings

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.

* fix(scheduled-tasks): close four more precondition gaps from 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.

* feat(scheduled-tasks): make the precondition readable, and translate it

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.

* fix(web-shell): make qwen-session:// links actually clickable

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

* fix(scheduled-tasks): stop pretending a manual run evaluates the precondition

"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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

daemon scope/session-management Session state and persistence scope/web-shell status/in-review This issue is currently in review. type/feature-request New feature or enhancement request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants