-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): fire active-todo reminders at delegation boundaries and user turns #10963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5575,9 +5575,19 @@ export class Session implements SessionContext { | |
| if (!continuesCurrentWorkChain && !this.todoStopGuard.enabled) { | ||
| this.#resetTodoStopGuardBackgroundLineage(); | ||
| } | ||
| // A registered reminder means the previous chain's plan still | ||
| // has unfinished items (todo_write deletes it on completion): | ||
| // continue that chain instead of discarding its context with the | ||
| // very turn that may be asking about it (#10953). | ||
| const continuesTodoWorkChain = | ||
|
Comment on lines
+5580
to
+5582
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-5: This behaviour change leaves Witness: 中文说明此行为变更使 — qwen3.8-max via Qwen Code /review (v0.23.0) |
||
| continuesCurrentWorkChain || | ||
| (this.activeTodoWorkChainPromptId !== undefined && | ||
| this.config.getActiveTodoReminder( | ||
| this.activeTodoWorkChainPromptId, | ||
| ) !== undefined); | ||
| this.config.startActiveTodoWorkChain( | ||
| promptId, | ||
| continuesCurrentWorkChain | ||
| continuesTodoWorkChain | ||
| ? this.activeTodoWorkChainPromptId | ||
| : undefined, | ||
| ); | ||
|
|
@@ -7908,7 +7918,18 @@ export class Session implements SessionContext { | |
| if (hadMidTurnUserInput) { | ||
| this.todoStopGuard.acceptMidTurnUserInput(); | ||
| } | ||
| const activeTodoReminder = this.config.takeActiveTodoReminder(promptId); | ||
| // A top-level Agent tool result means a delegated execution just | ||
| // returned (#10953): real work advanced while the parent earned a | ||
| // single tool turn, so the turn budget cannot come due on its own. | ||
| // Force the reminder exactly where the progress information arrives. | ||
| const carriesAgentToolResult = toolRun.parts.some( | ||
| (part) => | ||
| canonicalToolName(part.functionResponse?.name ?? '') === | ||
| ToolNames.AGENT, | ||
| ); | ||
|
Comment on lines
+7925
to
+7929
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R1-8: On an ACP user turn with a registered reminder, the reminder is now injected twice into permanent history: the pre-existing turn-start force take (Session.ts:5698) resets the counter to 0 but does not delete the reminder, and this new agent-result force takes it again when the delegation returns — an identical second copy in the same turn. Pre-PR the second take was cadence-gated (elapsed = 1 < 3 → undefined), so such a turn stored one copy. This exceeds the PR's own Risk-section accounting ("at most one reminder injection per completed top-level delegation") and the cadence's documented purpose of bounding permanent history growth; core Retry/Cron/Notification/Teammate turns that delegate have the same double shape (client.ts:3842 + 3932). Note the new Session.test.ts force test already exhibits this: the first send receives the reminder via the turn-start force take, yet the test only asserts the last send contains it. Suppress the agent-result force when this turn already force-injected the reminder: capture whether the turn-start take returned a reminder and pass that into Witness: 中文说明在提醒已注册的 ACP 用户轮中,提醒现在会被注入两次进永久历史:既有的轮首强制取用(Session.ts:5698)把计数器重置为 0 但并不删除提醒,而新增的 Agent 结果强制取用在委派返回时再次取用——同一轮内向历史追加一份完全相同的副本。修复前,第二次取用受节奏限制(elapsed = 1 < 3 → undefined),因此这类轮只存一份。这超出了本 PR Risk 一节自己的核算("每次完成的顶层委派至多一次提醒注入"),也超出了节奏机制文档中"约束永久历史增长"的设计目的;core 侧会委派的 Retry/Cron/Notification/Teammate 轮同样是双份形态(client.ts:3842 + 3932)。注意 Session.test.ts 的新增强制注入测试已经暴露了这一点:第一次发送经由轮首强制取用收到了提醒,而测试只断言最后一次发送包含它。建议:当本轮已经强制注入过提醒时,抑制 Agent 结果强制注入——记录轮首取用是否返回了提醒,并将其传入 — qwen3.8-max via Qwen Code /review (v0.23.0) |
||
| const activeTodoReminder = carriesAgentToolResult | ||
| ? this.config.takeActiveTodoReminder(promptId, true) | ||
| : this.config.takeActiveTodoReminder(promptId); | ||
| if (abortSignal.aborted) { | ||
| return { | ||
| message: { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R1-3: This force test only exercises the canonical
'agent'name (both the registry mock and the streamedfunctionCalls), but Session.ts:7927 callscanonicalToolName()specifically to also catch the legacytaskalias — and nothing on the ACP side witnesses that alias (the twin TUI test pins both names viait.each(['agent', 'task'])). DroppingcanonicalToolName()at Session.ts:7927 and comparing the raw name leaves the entire ACP suite green, while tool-result batches whose functionResponse carries the legacy nametask— still supported perToolNamesMigration— silently stop forcing the reminder due, re-freezing exactly the delegation-heavy sessions of #10953 on the ACP frontend. Parameterize the test like the core one:it.each(['agent', 'task'])('forces the active todo reminder due when a %s tool result returns', ...), feeding the name into bothmockToolRegistry.getTool.mockReturnValue({ name: ... })and the streamed chunk. The alias the parameterized case must use istask: ToolNames.AGENTinToolNamesMigration(packages/core/src/tools/tool-names.ts). Acceptance: the'task'case assertingtakeActiveTodoReminderis called with(promptId, true)— removingcanonicalToolNameat Session.ts:7927 must make it fail.Witness:
中文说明
该强制注入测试只覆盖了规范名
'agent'(registry mock 与流式functionCalls都是),但 Session.ts:7927 特意调用canonicalToolName()以同时捕获旧名task别名——而 ACP 侧没有任何测试见证该别名(TUI 的对应测试通过it.each(['agent', 'task'])钉住了两个名字)。若在 Session.ts:7927 去掉canonicalToolName()改为直接比较原始名字,整个 ACP 测试套件仍然全绿,而 functionResponse 携带旧名task的工具结果批次(ToolNamesMigration仍然支持该名字)将不再强制提醒到期,在 ACP 前端重新冻结 #10953 所针对的委派密集会话。建议像 core 一样参数化该测试:it.each(['agent', 'task'])('forces the active todo reminder due when a %s tool result returns', ...),并将该名字同时传入mockToolRegistry.getTool.mockReturnValue({ name: ... })与流式数据块。参数化用例必须使用的别名是ToolNamesMigration(packages/core/src/tools/tool-names.ts)中的task: ToolNames.AGENT。验收标准:'task'用例断言takeActiveTodoReminder以(promptId, true)被调用——移除 Session.ts:7927 的canonicalToolName后该断言必须失败。— qwen3.8-max via Qwen Code /review (v0.23.0)