Skip to content

fix(core): persist Goal turn endings outside model history #11924

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.

Sign up for GitHub

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

Open
qqqys wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(core): persist Goal turn endings outside model history #11924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1dd94e7
fix(core): persist Goal turn endings outside model history
qqqys Sep 15, 2026
139ae5d
test(core): update telemetry swap chat mock for Goal recovery
qqqys Sep 15, 2026
c13af5f
Merge branch 'main' into fix/goal-turn-end-record
qqqys Sep 15, 2026
9d585ca
test(cli): complete session swap chat mock
qqqys Sep 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 134 additions & 39 deletions packages/cli/src/acp-integration/session/Session.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,7 @@ describe('Session', () => {
recordTurnResult: ReturnType<typeof vi.fn>;
recordUserMessage: ReturnType<typeof vi.fn>;
recordGoalRuntimeMessage: ReturnType<typeof vi.fn>;
recordGoalTurnEnd: ReturnType<typeof vi.fn>;
recordMidTurnUserMessage: ReturnType<typeof vi.fn>;
recordUiTelemetryEvent: ReturnType<typeof vi.fn>;
recordToolResult: ReturnType<typeof vi.fn>;
Expand Down Expand Up @@ -753,10 +754,15 @@ describe('Session', () => {
});

const getHistoryMock = vi.fn().mockReturnValue([]);
let completedToolCallIds: readonly string[] = [];
mockChat = {
sendMessageStream: vi.fn(),
addHistory: vi.fn(),
getHistory: getHistoryMock,
setCompletedToolCallIds: vi.fn((ids: readonly string[]) => {
completedToolCallIds = ids;
}),
getCompletedToolCallIds: vi.fn(() => completedToolCallIds),
// continueLastTurn classifies from a bounded tail; delegate to getHistory
// so tests that set getHistory drive detection (fixtures are small).
getHistoryTail: vi.fn(() => getHistoryMock()),
Expand Down Expand Up @@ -871,6 +877,7 @@ describe('Session', () => {
recordTurnResult: vi.fn(),
recordUserMessage: vi.fn(),
recordGoalRuntimeMessage: vi.fn(),
recordGoalTurnEnd: vi.fn().mockResolvedValue(undefined),
recordMidTurnUserMessage: vi.fn(),
recordUiTelemetryEvent: vi.fn(),
recordToolResult: vi.fn(),
Expand Down Expand Up @@ -28199,63 +28206,151 @@ describe('Session', () => {
},
]);

it('ends a Goal turn without another model request', async () => {
// The proposal only reaches the verifier at a turn boundary, so a
// continuation that keeps the turn alive parks it indefinitely:
// the objective is already met, and the runtime refuses every
// later proposal for the same turn.
it.each([false, true])(
'ends a Goal turn without another model request (recording fails: %s)',
async (recordingFails) => {
// The proposal only reaches the verifier at a turn boundary, so a
// continuation that keeps the turn alive parks it indefinitely:
// the objective is already met, and the runtime refuses every
// later proposal for the same turn.
const permit: core.GoalTurnPermit = {
goalId: 'goal-1',
revision: 1,
turnId: 'turn-terminating-tool',
};
const turnKey = 'goal-runtime:turn-terminating-tool';
mockGoalRuntime.getSnapshot.mockReturnValue(activeGoalSnapshot);
mockGoalRuntime.permitForTurn.mockImplementation((key: string) =>
key === turnKey ? permit : undefined,
);
agentTelemetry.getActiveInteractionSpan.mockReturnValue(
agentTelemetry.span,
);
mockToolsWithTerminatingUpdateGoal();
if (recordingFails) {
mockChatRecordingService.recordGoalTurnEnd.mockRejectedValue(
new Error('writer failed'),
);
}
mockChat.sendMessageStream = vi
.fn()
.mockResolvedValueOnce(
streamCalling({ id: 'call-update', name: 'update_goal' }),
)
.mockResolvedValue(createEmptyStream());

expect(boundGoalHost).toBeDefined();
await boundGoalHost!.startGoalTurn({
permit,
continuationContext: 'write a poem',
});

await vi.waitFor(() => {
expect(mockGoalRuntime.finishTurn).toHaveBeenCalledWith(permit);
});
expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(1);
// Settled as a completed iteration, not paused as a failure.
expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled();
// The turn ends, but its own tool response still has to reach the
// transcript, or the next request carries a call with no result.
expect(mockChat.addHistory).toHaveBeenCalledWith({
role: 'user',
parts: expect.arrayContaining([
expect.objectContaining({
functionResponse: expect.objectContaining({
id: 'call-update',
}) as unknown,
}),
]) as unknown,
});
expect(mockClient.extNotification).toHaveBeenCalledWith(
'_qwencode/end_turn',
expect.objectContaining({ reason: 'end_turn', source: 'goal' }),
);
expect(
agentTelemetry.captures[0]?.writeToSpan,
).toHaveBeenCalledWith(agentTelemetry.span);
expect(
mockChatRecordingService.recordGoalTurnEnd,
).toHaveBeenCalledWith('call-update', permit);
const history = vi
.mocked(mockChat.addHistory)
.mock.calls.map(([entry]) => entry);
vi.mocked(mockChat.getHistory).mockReturnValue(history);
expect(session.getRecoveryStatus()).toEqual({
kind: recordingFails ? 'interrupted_prompt' : 'clean',
canContinue: recordingFails,
});
expect(
mockChatRecordingService.recordGoalRuntimeMessage,
).toHaveBeenCalledTimes(1);
history.push({
role: 'user',
parts: [{ text: 'new unanswered request' }],
});
expect(session.getRecoveryStatus()).toEqual({
kind: 'interrupted_prompt',
canContinue: true,
});
},
);

it('does not record a clean boundary when cancellation arrives during tool-result rewriting', async () => {
const permit: core.GoalTurnPermit = {
goalId: 'goal-1',
revision: 1,
turnId: 'turn-terminating-tool',
turnId: 'cancelled-tool-end',
};
const turnKey = 'goal-runtime:turn-terminating-tool';
const turnKey = `goal-runtime:${permit.turnId}`;
mockGoalRuntime.getSnapshot.mockReturnValue(activeGoalSnapshot);
mockGoalRuntime.permitForTurn.mockImplementation((key: string) =>
key === turnKey ? permit : undefined,
);
agentTelemetry.getActiveInteractionSpan.mockReturnValue(
agentTelemetry.span,
);
mockToolsWithTerminatingUpdateGoal();
let releaseRewrite!: () => void;
const waitForPendingRewrites = vi.fn(
() =>
new Promise<void>((resolve) => {
releaseRewrite = resolve;
}),
);
session.messageRewriter = {
interceptUpdate: vi.fn().mockResolvedValue(undefined),
flushTurn: vi.fn().mockResolvedValue(undefined),
waitForPendingRewrites,
} as unknown as Session['messageRewriter'];
mockChat.sendMessageStream = vi
.fn()
.mockResolvedValueOnce(
streamCalling({ id: 'call-update', name: 'update_goal' }),
)
.mockResolvedValue(createEmptyStream());

expect(boundGoalHost).toBeDefined();
);
await boundGoalHost!.startGoalTurn({
permit,
continuationContext: 'write a poem',
});

await vi.waitFor(() => {
expect(mockGoalRuntime.finishTurn).toHaveBeenCalledWith(permit);
});
expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(1);
// Settled as a completed iteration, not paused as a failure.
expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled();
// The turn ends, but its own tool response still has to reach the
// transcript, or the next request carries a call with no result.
expect(mockChat.addHistory).toHaveBeenCalledWith({
role: 'user',
parts: expect.arrayContaining([
expect.objectContaining({
functionResponse: expect.objectContaining({
id: 'call-update',
}) as unknown,
}),
]) as unknown,
continuationContext: 'finish the goal',
});
expect(mockClient.extNotification).toHaveBeenCalledWith(
'_qwencode/end_turn',
expect.objectContaining({ reason: 'end_turn', source: 'goal' }),
await vi.waitFor(() =>
expect(waitForPendingRewrites).toHaveBeenCalled(),
);
expect(agentTelemetry.captures[0]?.writeToSpan).toHaveBeenCalledWith(
agentTelemetry.span,
await session.cancelPendingPrompt();
releaseRewrite();
await vi.waitFor(() =>
expect(mockClient.extNotification).toHaveBeenCalledWith(
'_qwencode/end_turn',
expect.objectContaining({ reason: 'cancelled', source: 'goal' }),
),
);
expect(
mockChatRecordingService.recordGoalTurnEnd,
).not.toHaveBeenCalled();
expect(mockChat.setCompletedToolCallIds).not.toHaveBeenCalled();
expect(mockChat.sendMessageStream).toHaveBeenCalledOnce();
vi.mocked(mockChat.getHistory).mockReturnValue(
vi.mocked(mockChat.addHistory).mock.calls.map(([entry]) => entry),
);
expect(session.getRecoveryStatus()).toEqual({
kind: 'interrupted_prompt',
canContinue: true,
});
});

it('runs managed memory effects after an early Goal turn end', async () => {
Expand Down
28 changes: 28 additions & 0 deletions packages/cli/src/acp-integration/session/Session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,6 +649,7 @@ interface AcpGoalTurn extends GoalContinuationTurn {
controller: AbortController;
origin: 'runtime' | 'user';
modelStarted: boolean;
endingToolCallId?: string;
}

function sameGoalPermit(
Expand Down Expand Up @@ -2787,6 +2788,29 @@ export class Session implements SessionContext {
const cancelledByUser =
result?.stopReason === 'cancelled' &&
turn.controller.signal.reason === USER_CANCEL_ABORT_REASON;
if (
turn.endingToolCallId &&
result?.stopReason === 'end_turn' &&
failureMessage === undefined &&
!turn.controller.signal.aborted
Comment on lines +2793 to +2795

@qwen-code-ci-bot qwen-code-ci-bot Sep 15, 2026

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] R1-2: The new cancellation test never makes !turn.controller.signal.aborted the deciding clause, so this guard can be deleted with the whole suite still green. In that test the abort lands before the terminating-tool exit computes its stop reason, so getAbortAwareEndTurnStopReason returns 'cancelled' and the preceding result?.stopReason === 'end_turn' clause already blocks the recording. The clause is load-bearing only for the narrower race it was presumably written for — a cancel landing after the stop reason was computed, for example during the settlement path's await ...flush() just above — and nothing covers that window.

The remap hazard is not hypothetical: this file holds 21 bare return { stopReason: 'end_turn' } exits against 14 abort-aware ones, so a future remap of the terminating-tool exit onto a bare one would persist a goal_turn_end boundary for a turn the user cancelled. On resume getRecoveryStatus() would report clean / canContinue: false, and the cancelled-but-unanswered prompt would be silently dropped instead of offered for continuation.

Witness:

mutation: delete `!turn.controller.signal.aborted` from the guard
npx vitest run src/acp-integration/session/Session.test.ts -t 'a tool that ends the turn'
  -> Tests 17 passed | 998 skipped (1015)     # the mutant survives, cancellation test included

static premises, quoted at the reviewed commit:
  Session.ts:577-581    return signal.aborted ? 'cancelled' : 'end_turn';
  Session.ts:7954       stopReason: getAbortAwareEndTurnStopReason(pendingSend.signal)
  Session.ts:8720-8724  endingToolCallId assigned after `await this.messageRewriter?.waitForPendingRewrites()`
  exit census in Session.ts: 21 bare `{ stopReason: 'end_turn' }` vs 14 abort-aware

Add a case that lets the terminating tool exit normally (so getAbortAwareEndTurnStopReason returns 'end_turn') and aborts the turn controller while the settlement's recording flush is in flight, then asserts that neither recordGoalTurnEnd nor setCompletedToolCallIds was called.

getAbortAwareEndTurnStopReason already folds an abort into the stop reason (return signal.aborted ? 'cancelled' : 'end_turn';, Session.ts:580), so the new test must abort after that call rather than before it, or it re-pins the stopReason clause and leaves this one still uncovered. That new case must go red when !turn.controller.signal.aborted is removed — the mutation that survives today.

中文说明

新增的取消测试从未让 !turn.controller.signal.aborted 成为决定性条件，因此删掉这个守卫，整个测试套件仍然是绿的。在该测试中，中止发生在终止型工具退出计算 stop reason 之前，所以 getAbortAwareEndTurnStopReason 返回 'cancelled'，前一个 result?.stopReason === 'end_turn' 条件已经拦住了记录写入。这个子句真正起作用的只有一个更窄的竞态——中止发生在 stop reason 计算之后，例如就在上方的结算路径 await ...flush() 期间——而这个窗口没有任何覆盖。

“被改写”的风险并非假设：本文件中有 21 处裸 return { stopReason: 'end_turn' } 退出，对应 14 处感知中止的退出。因此将来若把终止型工具退出改接到某个裸退出上，就会为一个用户已取消的回合持久化 goal_turn_end 边界。恢复时 getRecoveryStatus() 会报告 clean / canContinue: false，那条已取消但未获答复的输入会被静默丢弃，而不再被提供“继续执行”。

请补一个用例：让终止型工具正常退出（使 getAbortAwareEndTurnStopReason 返回 'end_turn'），并在结算的记录 flush 进行中中止 turn controller，然后断言 recordGoalTurnEnd 与 setCompletedToolCallIds 都未被调用。

注意 getAbortAwareEndTurnStopReason 已经把中止折进 stop reason（return signal.aborted ? 'cancelled' : 'end_turn';，Session.ts:580），所以新用例必须在该调用之后才中止，否则只是重新钉住了 stopReason 条件，本子句仍未被覆盖。该新用例在移除 !turn.controller.signal.aborted 时必须变红——也就是今天能够存活的那个变异。

— qwen3.8-max via Qwen Code /review (v0.23.3)

) {
const recorder = this.config.getChatRecordingService();
if (recorder) {
try {
await recorder.recordGoalTurnEnd(
turn.endingToolCallId,
turn.permit,
);
Comment on lines +2800 to +2803

@qwen-code-ci-bot qwen-code-ci-bot Sep 15, 2026

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] R1-1: This strict append runs before runtime.finishTurn(turn.permit), so a transient failure of what is optional recovery metadata costs the turn commit that is essential. If the goal_turn_end append fails — ENOSPC, EIO, or a writer lease taken over by a second process — enterWriteFailure latches the recorder and rethrows; the try/catch here swallows that throw and logs a warn, so execution continues into finishTurn. finishTurn journals through recordGoalState → appendRecordStrict, which opens with if (this.writeFailure) throw this.writeFailure;, so it throws too, and the settlement catch arm runs releaseTurn(turn.turnKey) with the default requeue. The turn that had already finished is then never committed: no goal_state record, and no token or noProgressTurns accounting, since both are applied inside finishTurn only after the journal write. The Goal stays active and churns permits — the requeued continuation's prompt() rejects at assertCanStartTurn() before any model request, #drainGoalQueueExclusive's catch settles it with modelStarted: false, and the cycle repeats until the user cancels. Moving this block behind the settlement try/catch costs only the boundary on a write failure, which is the conservative outcome the description claims.

For scale: this widens a hazard the file already documents rather than introducing a new failure mode — the merge base carries the same latched-write fallback for finishTurn's own strict write, and a transient failure there produces the identical lost settlement. That is why this is a Suggestion rather than a blocker, but the fix is a pure move.

Witness:

P0 (real ChatRecordingService, one transient writeLine rejection on the goal_turn_end append):
  {"probe":"P0","writesWhenGoalTurnEndFailed":1,"recordGoalState":"THREW EIO once",
   "flush":"THREW EIO once","writesAfterRecordGoalState":1,"writerTouchedBySettlementWrite":false}
P1 (latch modelled causally, unmodified PR):
  {"probe":"P1","recordGoalTurnEndCalled":1,"finishTurnCalled":1,"finishTurnThrew":true,
   "releaseTurnCalls":[["goal-runtime:turn-latch-probe"]],"dispatchCalled":0}
P1 with the block moved behind the settlement try/catch — the probe flips:
  {"probe":"P1","recordGoalTurnEndCalled":1,"finishTurnCalled":1,"finishTurnThrew":false,
   "releaseTurnCalls":[],"dispatchCalled":0}

Transcript ordering stays valid after the move: the intervening goal_state record is a system record the reader keeps out of modelSet, and in SessionApiHistoryAccumulator.add a non-goal_turn_end system record returns before touching lastMaterialRecord, so the previous?.type === 'tool_result' adjacency check still sees the terminating tool result.

The fix must not assume the recorder is healthy at settlement time: appendRecordStrict re-throws a latched failure forever (packages/core/src/services/chatRecordingService.ts:1449-1452), the latch is set by enterWriteFailure (chatRecordingService.ts:1326-1346), and finishTurn shares that path via recordGoalState (chatRecordingService.ts:1895, reached from goal-runtime.ts:1913). Extend the existing recordingFails: true case in Session.test.ts so the mocked runtime models the latch — make mockGoalRuntime.finishTurn reject once recordGoalTurnEnd has rejected — and assert expect(mockGoalRuntime.releaseTurn).not.toHaveBeenCalled(); that assertion is red today and green once the record moves behind settlement. Please then remove the move again and confirm the assertion reds.

中文说明

这个 strict append 位于 runtime.finishTurn(turn.permit) 之前，因此一次可选恢复元数据的瞬时写入失败，会连带牺牲掉真正必要的回合提交。若 goal_turn_end 追加失败（ENOSPC、EIO，或写入租约被第二个进程接管），enterWriteFailure 会把记录器置为锁定并重新抛出；此处的 try/catch 吞掉该异常只记一条 warn，于是执行继续进入 finishTurn。finishTurn 经 recordGoalState → appendRecordStrict 写日志，而后者开头就是 if (this.writeFailure) throw this.writeFailure;，因此它同样抛出，结算的 catch 分支随即以默认 requeue 调用 releaseTurn(turn.turnKey)。结果是：本已完成的回合永远不会被提交——没有 goal_state 记录，也没有 token 与 noProgressTurns 记账（两者都在 finishTurn 内部、日志写入之后才生效）。Goal 会保持 active 并不断空转许可——被重新排队的 continuation 在 prompt() 中于 assertCanStartTurn() 处被拒（尚未发出任何模型请求），#drainGoalQueueExclusive 的 catch 以 modelStarted: false 结算，如此循环直到用户取消。把这一整块移到结算 try/catch 之后，写入失败时只损失边界，这正是描述中所声称的保守结果。

关于严重程度：这是在扩大文件中已记录的风险，而非引入新的失败模式——merge base 对 finishTurn 自身的 strict 写入已有同样的锁定回退，那里的瞬时失败会产生完全相同的“提交丢失”。因此这是 Suggestion 而非阻塞项，但修复只是一次纯粹的代码移动。

移动后记录顺序依然成立：中间插入的 goal_state 是 system 记录，读取侧不会将其纳入 modelSet；而在 SessionApiHistoryAccumulator.add 中，非 goal_turn_end 的 system 记录会在触及 lastMaterialRecord 之前返回，所以 previous?.type === 'tool_result' 的相邻性检查仍能看到那个终止型工具结果。

修复时不能假定结算那一刻记录器是健康的：appendRecordStrict 会永久重抛锁定失败（packages/core/src/services/chatRecordingService.ts:1449-1452），锁定由 enterWriteFailure 设置（chatRecordingService.ts:1326-1346），而 finishTurn 经 recordGoalState 共用这条路径（chatRecordingService.ts:1895，由 goal-runtime.ts:1913 抵达）。请把 Session.test.ts 中已有的 recordingFails: true 用例扩展为让 mock runtime 模拟该锁定——在 recordGoalTurnEnd 拒绝之后让 mockGoalRuntime.finishTurn 也拒绝——并断言 expect(mockGoalRuntime.releaseTurn).not.toHaveBeenCalled()；该断言在今天为红，移动之后转绿。随后请再撤销这次移动，确认断言重新变红。

— qwen3.8-max via Qwen Code /review (v0.23.3)

const chat = this.#getCurrentChat();
chat.setCompletedToolCallIds([
...chat.getCompletedToolCallIds(),
turn.endingToolCallId,
]);
} catch (error) {
debugLogger.warn('Failed to record ACP Goal turn end:', error);
}
}
}
// A turn preempted by a newly arrived user prompt is a handoff, not a
// failure. `this.pendingPrompt` is the goal turn's own controller while
// a goal turn is in flight, so a new prompt aborts it with
Expand Down Expand Up @@ -5437,6 +5461,7 @@ export class Session implements SessionContext {
: undefined);
return buildSessionRecoveryPlanFromApiHistory({
sessionId: this.sessionId,
completedToolCallIds: chat.getCompletedToolCallIds?.(),
apiHistory: fullHistory
? chat.getHistory()
: (chat.getHistoryTailShallow?.(TURN_INTERRUPTION_HISTORY_TAIL_COUNT) ??
Expand Down Expand Up @@ -8694,6 +8719,9 @@ export class Session implements SessionContext {
true,
);
await this.messageRewriter?.waitForPendingRewrites();
goalTurn.endingToolCallId = toolRun.parts.findLast(
(part) => part.functionResponse?.id,
)?.functionResponse?.id;
return true;
}

Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/nonInteractive/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,7 @@ class Session {
const recoveryPlan = buildSessionRecoveryPlanFromApiHistory({
sessionId: this.sessionId,
apiHistory: historyTail,
completedToolCallIds: chat.getCompletedToolCallIds?.(),
});
debugLogger.info('[Session] requestContinueLastTurn recovery', {
sessionId: this.sessionId,
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/nonInteractiveCli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1141,6 +1141,7 @@ export async function runNonInteractive(
const recoveryPlan = buildSessionRecoveryPlanFromApiHistory({
sessionId,
apiHistory: llmClient.getChat().getHistory(),
completedToolCallIds: llmClient.getChat().getCompletedToolCallIds?.(),
});
debugLogger.info('[runNonInteractive] continueInterrupted recovery', {
kind: recoveryPlan.kind,
Expand Down
7 changes: 4 additions & 3 deletions packages/cli/src/serve/prompt-terminal-ledger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import type { Content } from '@google/genai';
import { closeSync, openSync, readSync, statSync } from 'node:fs';
import {
buildApiHistoryFromConversation,
buildSessionHistoryFromConversation,
detectTurnInterruption,
SessionService,
TURN_INTERRUPTION_HISTORY_TAIL_COUNT,
Expand Down Expand Up @@ -258,9 +258,10 @@ export async function reconcileDanglingPromptTerminals(
) {
return;
}
const apiHistory = buildApiHistoryFromConversation(resumed.conversation);
const { apiHistory, completedToolCallIds } =
buildSessionHistoryFromConversation(resumed.conversation);

@qwen-code-ci-bot qwen-code-ci-bot Sep 15, 2026

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] R1-6: One of six sites where the new completedToolCallIds threading has no test that reddens when the threading at that site is removed. reconcileDanglingPromptTerminals now feeds recorded boundary ids into detectTurnInterruption, but prompt-terminal-ledger.test.ts — 28 cases for this function — contains no goal_turn_end fixture and no completedToolCallIds reference, so the new classification is untested at this call site.

Daemon restart while a Goal turn's prompt terminal is still dangling, with a transcript ending in a recorded goal_turn_end: reverting this to buildApiHistoryFromConversation plus detectTurnInterruption(historyTail) leaves the suite green, and the behaviour that then ships is that the trailing user-role functionResponse entry is classified interrupted_prompt, so the ledger is stamped { terminal: 'interrupted', code: 'daemon_lost' } instead of { terminal: 'completed', stopReason: 'reconstructed_from_transcript' } — a Goal prompt that finished cleanly is reported to SSE clients as lost. This is the load-bearing path for the banner reported in the linked issue, so it is the one site here worth pinning first.

Witness:

mutation: revert to `detectTurnInterruption(historyTail)` (drop completedToolCallIds)
  npx vitest run src/serve/prompt-terminal-ledger.test.ts -> 34 passed (34)   # mutant survives
LIVENESS CONTROL: injected `throw new Error('PROBE-LIVENESS-CONTROL')` in the same
  function surfaced repeatedly in that suite's output, proving the run executes the mutation
mechanical: `rg 'goal_turn_end|completedToolCallIds' prompt-terminal-ledger.test.ts` -> 0 hits

Add a ledger case whose transcript is assistant functionCall → tool_result functionResponse (same goalContext) → system/goal_turn_end with a matching permit, and assert the appended terminal record is completed.

The accumulator only honours a boundary when the immediately preceding material record is the tool_result carrying the same permit — previous?.type === 'tool_result' plus matching goalId / revision / turnId (packages/core/src/services/session-api-history.ts:110-126) — so the fixture's goal_turn_end record must directly follow its tool_result. That new case must go red when completedToolCallIds is dropped from the detectTurnInterruption(historyTail, ...) call.

中文说明

这是六处“新增 completedToolCallIds 传递没有被任何测试钉住”的站点之一。reconcileDanglingPromptTerminals 现在会把记录到的边界 ids 传给 detectTurnInterruption，但 prompt-terminal-ledger.test.ts（该函数有 28 个用例）中没有任何 goal_turn_end fixture，也没有任何 completedToolCallIds 引用，因此这个新的分类在该调用点未被测试。

场景：守护进程重启时某个 Goal 回合的 prompt terminal 仍悬空，且记录尾部是一条已写入的 goal_turn_end。把此处改回 buildApiHistoryFromConversation 加 detectTurnInterruption(historyTail)，套件仍然全绿；随之上线的行为是：尾部的 user 角色 functionResponse 条目被判定为 interrupted_prompt，账本被写入 { terminal: 'interrupted', code: 'daemon_lost' } 而不是 { terminal: 'completed', stopReason: 'reconstructed_from_transcript' }——一个干净结束的 Goal 提示会被当作“丢失”上报给 SSE 客户端。这正是所关联 issue 中横幅问题的关键路径，因此这几处里最值得优先钉住。

请补一个账本用例，其记录序列为：assistant functionCall → tool_result functionResponse（相同 goalContext）→ 携带匹配许可的 system/goal_turn_end，并断言追加的 terminal 记录为 completed。

注意累加器只有在紧邻的上一条实质记录是携带同一许可的 tool_result 时才承认边界——previous?.type === 'tool_result' 加上 goalId / revision / turnId 匹配（packages/core/src/services/session-api-history.ts:110-126）——因此 fixture 中的 goal_turn_end 记录必须紧跟在其 tool_result 之后。从 detectTurnInterruption(historyTail, ...) 调用中去掉 completedToolCallIds 时，该新用例必须变红。

— qwen3.8-max via Qwen Code /review (v0.23.3)

const historyTail = apiHistory.slice(-TURN_INTERRUPTION_HISTORY_TAIL_COUNT);
const verdict = detectTurnInterruption(historyTail);
const verdict = detectTurnInterruption(historyTail, completedToolCallIds);
// Id-less tool-call guard: `detectTurnInterruption` ignores functionCalls
// without an id (they cannot be paired on the wire), but reconciliation
// needs no wire pairing — a model tail holding ANY functionCall means the
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/ui/hooks/session-swap-telemetry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ function makeFakeEnv() {
const fakeChat = {
seedResumeTokenCounts: vi.fn(),
setLastPromptTokenCount: vi.fn(),
setCompletedToolCallIds: vi.fn(),
};

// One shared session-service object: every getSessionService() call sees
Expand Down
5 changes: 4 additions & 1 deletion packages/cli/src/ui/hooks/use-llm-stream.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4516,7 +4516,10 @@ export const useLlmStream = (
};
const orphanedEntries: Part[][] = [];
try {
const history = llmClient?.getHistoryShallow?.() ?? [];
const history =
llmClient?.getChat?.()?.getHistoryForRecovery?.() ??

@qwen-code-ci-bot qwen-code-ci-bot Sep 15, 2026

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] R1-3: One of six sites where the new completedToolCallIds threading has no test that reddens when the threading at that site is removed, so a regression here ships green. getHistoryForRecovery appears in zero files matching packages/cli/src/**/*.test.ts{,x}, and no fake client in use-llm-stream.test.tsx exposes getChat, so this new first ?? rung always evaluates to undefined under test and all ~10 retry-debt tests exercise the getHistoryShallow fallback instead.

Because the call is optional-chained, a rename or removal of the core method degrades silently to the old full-history scan with nothing failing. The cost is that this hunk's whole purpose — making the CLI's orphaned-envelope scan stop at the same completed-tool-call boundary that core's stripOrphanedUserEntriesFromHistory now stops at — stays unpinned: an envelope sitting below the boundary would be classified as a trailing orphan and re-attached to the retry payload even though the core strip no longer pops it, duplicating teammate envelope text into the retried prompt.

Witness:

mechanical, at the reviewed commit:
  rg -l 'getHistoryForRecovery' 'packages/cli/src/**/*.test.ts' '**/*.test.tsx'  -> 0 files
  only cli source reference: packages/cli/src/ui/hooks/use-llm-stream.ts:4520 (this hunk)
  the cited retry-debt tests do exist and drive the fallback, e.g.
    client.getHistoryShallow = vi.fn().mockReturnValue([   at 2044, 2142, 2259, 2327, 2373, 3115, ...
so reverting this rung is a semantic no-op in tests.

Add one case to the existing retry-debt describe whose fake client exposes getChat: () => ({ getHistoryForRecovery: () => historyFromBoundary }), where a pre-boundary user entry carries the envelope text, and assert the envelope is not re-attached — while keeping getHistoryShallow returning a list that would match, so the test fails if the chain regresses to the fallback.

The scan matches by byte equality (return JSON.stringify(a) === JSON.stringify(b);, use-llm-stream.ts:4483) while getHistoryForRecovery() returns copyContentContainer copies, so a hand-built fixture must round-trip identically or the test would pass by never matching. That new case must go red when this getHistoryForRecovery?.() rung is deleted and the scan falls back to getHistoryShallow.

中文说明

这是六处“新增 completedToolCallIds 传递没有被任何测试钉住”的站点之一：移除该处的传递，测试仍然全绿。getHistoryForRecovery 在 packages/cli/src/**/*.test.ts{,x} 中出现 0 次，且 use-llm-stream.test.tsx 中没有任何 fake client 暴露 getChat，因此这个新的第一级 ?? 在测试中永远求值为 undefined，约 10 个 retry-debt 用例实际走的都是 getHistoryShallow 回退分支。

由于该调用使用了可选链，核心方法一旦被重命名或移除，就会静默退回到旧的全历史扫描而不会有任何失败。代价是本 hunk 的全部意图——让 CLI 的孤儿 envelope 扫描停在 core 的 stripOrphanedUserEntriesFromHistory 现在所停的同一个已完成工具调用边界上——完全没有被钉住：位于边界之下的 envelope 会被判定为尾部孤儿并重新挂到重试载荷上，而 core 的 strip 已不再弹出它，于是队友 envelope 文本会在重试提示中重复出现。

请在已有的 retry-debt describe 中补一个用例：让 fake client 暴露 getChat: () => ({ getHistoryForRecovery: () => historyFromBoundary })，其中边界之前的 user 条目携带该 envelope 文本，并断言 envelope 没有被重新挂上；同时让 getHistoryShallow 返回一个会匹配的列表，这样一旦链路退化到回退分支，用例就会失败。

注意该扫描按字节相等匹配（return JSON.stringify(a) === JSON.stringify(b);，use-llm-stream.ts:4483），而 getHistoryForRecovery() 返回的是 copyContentContainer 副本，因此手工构造的 fixture 必须能逐字节往返一致，否则用例会因为“从不匹配”而假绿。删除这一级 getHistoryForRecovery?.() 使扫描退回 getHistoryShallow 时，该新用例必须变红。

— qwen3.8-max via Qwen Code /review (v0.23.3)

llmClient?.getHistoryShallow?.() ??
[];
for (let i = history.length - 1; i >= 0; i--) {
const entry = history[i];
if (!entry || entry.role !== 'user') break;
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/core/client.telemetrySwap.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ function makeEnv() {
const fakeChat = {
seedResumeTokenCounts: vi.fn(),
setLastPromptTokenCount: vi.fn(),
setCompletedToolCallIds: vi.fn(),
} as unknown as LlmChat;
const startChat = vi
.spyOn(client, 'startChat')
Expand Down
Loading
Loading