Skip to content

fix(core): preserve host-confirmed user answers in auto classification #11084

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

Merged
qqqys merged 5 commits into from
Sep 6, 2026
Merged

fix(core): preserve host-confirmed user answers in auto classification #11084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34eecbf
fix(core): preserve trusted user answers in auto classification
qqqys Sep 5, 2026
43a33e5
fix(core): address trusted user answer review feedback (#11019)
qqqys Sep 5, 2026
6f1af1d
chore: merge origin/main to retire @qwen-code/webui from the build
qqqys Sep 5, 2026
820cb02
chore: keep the merged skill doc byte-identical to main
qqqys Sep 5, 2026
de879de
Merge branch 'main' into fix/issue-11019-trusted-user-answers
qwen-code-dev-bot Sep 5, 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
149 changes: 149 additions & 0 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 @@ -517,6 +517,12 @@ describe('Session', () => {
};
let mockLlmClient: {
getChat: ReturnType<typeof vi.fn>;
getHistoryTail: ReturnType<typeof vi.fn>;
getTrustedUserAnswers: ReturnType<typeof vi.fn>;
recordTrustedUserAnswers: ReturnType<typeof vi.fn>;
setHistory: ReturnType<typeof vi.fn>;
stripOrphanedUserEntriesFromHistory: ReturnType<typeof vi.fn>;
truncateHistory: ReturnType<typeof vi.fn>;
isInitialized: ReturnType<typeof vi.fn>;
refreshSystemInstruction: ReturnType<typeof vi.fn>;
setTools: ReturnType<typeof vi.fn>;
Expand Down Expand Up @@ -745,6 +751,16 @@ describe('Session', () => {
} as unknown as LlmChat;
mockLlmClient = {
getChat: vi.fn().mockReturnValue(mockChat),
getHistoryTail: vi.fn().mockReturnValue([]),
getTrustedUserAnswers: vi.fn().mockReturnValue([]),
recordTrustedUserAnswers: vi.fn(),
setHistory: vi.fn((history: Content[]) => mockChat.setHistory(history)),

@yiliang114 yiliang114 Sep 5, 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.

Non-blocking: these client-level spies delegate to the same mockChat spies the pre-existing assertions target, so the routing change they exist to cover isn't actually pinned.

Session.ts:4332/4387/5303/5321 were moved from chat.* to llmClient.* specifically so rewind/restore/retry invalidate the store. But reverting all four to the exact pre-PR calls — which bypasses trusted-answer invalidation, FileReadCache clearing, and IDE-context forcing — passes every current test, because mockLlmClient.setHistory / truncateHistory / stripOrphanedUserEntriesFromHistory are never asserted directly (grep: zero matches) and just forward to mockChat (:4633, :4670, :6099, :6172, :6202, :6266, :6297, :6443).

Asserting the client-level spies in those existing rewind/restore/retry tests would catch the literal pre-PR code.

@qwen-code-dev-bot qwen-code-dev-bot Sep 5, 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.

Deferred again — still open, not dropped.

The gap is real and survives this round: the four Session.ts routings (truncateHistory, stripOrphanedUserEntriesFromHistory, and the two setHistory sites) are still load-bearing clear sites, and reverting them to their pre-PR chat.* calls would still pass every current test because the client-level spies only forward to the same mockChat spies the existing assertions target.

Deferred for the same reason as the other two: this round was a same-run verification repair whose mandate was the deterministic build rejection, resolved by merging origin/main.

This finding needs a re-read before it is implemented. That merge brought 685 new lines into Session.test.ts from main, so the mock harness this thread cites (the mockLlmClient forwarding at :4633, :4670, :6099, :6172, :6202, :6266, :6297, :6443) has moved since the finding was written. The line references above are pre-merge; the shape of the fix — asserting the client-level spies directly in the existing rewind/restore/retry tests so the literal pre-PR code fails — is unchanged.

中文说明

再次顺延 —— 线程保持 open，未被丢弃。

这个缺口是真实存在的，并且在本轮之后依然存在：Session.ts 的四路由（truncateHistory、stripOrphanedUserEntriesFromHistory，以及两处 setHistory）仍然是承重的清理点，而把它们回退成 PR 之前的 chat.* 调用，仍然会通过当前所有测试，因为客户端层的 spy 只是转发给既有断言所针对的同一批 mockChat spy。

顺延原因与另外两条相同：本轮是一次同轮验证修复，任务是处理确定性的 build 拒绝，而该拒绝通过合并 origin/main 解决。

这条问题在实施前需要重新阅读。 那次合并从 main 给 Session.test.ts 带入了 685 行新代码，因此本线程引用的 mock 脚手架（:4633、:4670、:6099、:6172、:6202、:6266、:6297、:6443 处的 mockLlmClient 转发）自该问题写下之后已经移动。上面的行号是合并前的；但修复的形态没有变 —— 在既有的 rewind/restore/retry 测试中直接断言客户端层的 spy，使 PR 之前的原始代码会失败。

stripOrphanedUserEntriesFromHistory: vi.fn(() =>
mockChat.stripOrphanedUserEntriesFromHistory(),
),
truncateHistory: vi.fn((count: number) =>
mockChat.truncateHistory(count),
),
isInitialized: vi.fn().mockReturnValue(true),
refreshSystemInstruction: vi.fn().mockResolvedValue(undefined),
setTools: vi.fn().mockResolvedValue(undefined),
Expand Down Expand Up @@ -30896,6 +30912,139 @@ describe('Session', () => {
);
}

const trustedAnswerQuestions = [
{
question: 'Create the marker?',
header: 'Marker',
options: [
{ label: 'Yes', description: 'Create only /tmp/marker.' },
{ label: 'No', description: 'Do not create it.' },
],
},
];

class AskUserQuestionTool {
readonly name = core.ToolNames.ASK_USER_QUESTION;
readonly kind = core.Kind.Think;
readonly displayName = this.name;
readonly description = this.name;
readonly canUpdateOutput = false;
readonly isOutputMarkdown = true;
readonly build = vi.fn().mockReturnValue({
params: { questions: trustedAnswerQuestions },
execute: vi.fn().mockResolvedValue({
llmContent:
'User has provided the following answers:\n\n**Marker**: Yes',
}),
getDefaultPermission: vi.fn().mockResolvedValue('ask'),
requiresUserInteraction: vi.fn().mockReturnValue(true),
getConfirmationDetails: vi.fn().mockResolvedValue({
type: 'ask_user_question',
title: 'Please answer the following question(s):',
questions: trustedAnswerQuestions,
onConfirm: vi.fn().mockResolvedValue(undefined),
}),
getDescription: vi.fn().mockReturnValue(this.name),
toolLocations: vi.fn().mockReturnValue([]),
});
}

function useBuiltinAskUserQuestionTool() {
mockToolRegistry.getTool.mockReturnValue(new AskUserQuestionTool());
}

async function runAskUserQuestion(signal = new AbortController().signal) {
return (session as unknown as ToolCallInternals).runToolCalls(
signal,
'prompt-auq',
[
{
id: 'call-auq',
name: core.ToolNames.ASK_USER_QUESTION,
args: { questions: trustedAnswerQuestions },
},
],
);
}

it('records an accepted built-in ask_user_question host answer', async () => {
useBuiltinAskUserQuestionTool();
vi.mocked(mockClient.requestPermission).mockResolvedValue({
outcome: { outcome: 'selected', optionId: 'proceed_once' },
answers: { '0': 'Yes' },
});

await runAskUserQuestion();

expect(mockLlmClient.recordTrustedUserAnswers).toHaveBeenCalledOnce();
expect(mockLlmClient.recordTrustedUserAnswers).toHaveBeenCalledWith(
'call-auq',
trustedAnswerQuestions,
{ '0': 'Yes' },
);
});

it.each([
[
'cancelled',
async () => {
vi.mocked(mockClient.requestPermission).mockResolvedValue({
outcome: { outcome: 'cancelled' },
});
await runAskUserQuestion();
},
],
[
'permission error',
async () => {
vi.mocked(mockClient.requestPermission).mockRejectedValue(
new Error('host unavailable'),
);
await runAskUserQuestion();
},
],
[
'aborted response',
async () => {
const controller = new AbortController();
vi.mocked(mockClient.requestPermission).mockImplementation(
async () => {
controller.abort();
return {
outcome: { outcome: 'selected', optionId: 'proceed_once' },
answers: { '0': 'Yes' },
};
},
);
await runAskUserQuestion(controller.signal);
},
],
])(
'does not record a %s ask_user_question response',
async (_name, run) => {
useBuiltinAskUserQuestionTool();
await run();
expect(mockLlmClient.recordTrustedUserAnswers).not.toHaveBeenCalled();
},
);

it('does not trust a same-named shadow ask_user_question tool', async () => {
mockToolRegistry.getTool.mockReturnValue(
mockConfirmingTool(
core.ToolNames.ASK_USER_QUESTION,
vi.fn().mockResolvedValue({ llmContent: 'shadow result' }),
),
);
vi.mocked(mockClient.requestPermission).mockResolvedValue({
outcome: { outcome: 'selected', optionId: 'proceed_once' },
answers: { '0': 'Yes' },
});

await runAskUserQuestion();

expect(mockLlmClient.recordTrustedUserAnswers).not.toHaveBeenCalled();
});

it('blocks standalone worktree actions before building the tool', async () => {
recreateStandaloneSession();
const builds: Array<ReturnType<typeof vi.fn>> = [];
Expand Down
42 changes: 33 additions & 9 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 @@ -4320,7 +4320,8 @@ export class Session implements SessionContext {
);
}

const chat = this.config.getLlmClient()!.getChat();
const llmClient = this.config.getLlmClient()!;
const chat = llmClient.getChat();
const apiHistory = chat.getHistoryShallow();
const apiTruncateIndex = this.#computeApiTruncationIndexForUserTurn(
apiHistory,
Expand All @@ -4334,7 +4335,7 @@ export class Session implements SessionContext {
);
}

chat.truncateHistory(apiTruncateIndex);
llmClient.truncateHistory(apiTruncateIndex);
chat.stripThoughtsFromHistory();
this.clearActiveTodoPlanRevision();
const preserveQueuedPromptPriority = this.todoStopGuardQueuedPromptPriority;
Expand Down Expand Up @@ -4389,7 +4390,7 @@ export class Session implements SessionContext {
);
}

this.config.getLlmClient()!.getChat().setHistory(structuredClone(history));
this.config.getLlmClient()!.setHistory(structuredClone(history));
this.clearActiveTodoPlanRevision();
this.#clearTodoStopGuardTrustAndDrainAutomaticQueues();
}
Expand Down Expand Up @@ -5305,8 +5306,9 @@ export class Session implements SessionContext {
}
if (recoveryPlan.continuation.mode === 'retry_user_parts') {
strippedOrphanEntries =
this.#getCurrentChat().stripOrphanedUserEntriesFromHistory() ??
null;
this.config
.getLlmClient()!
.stripOrphanedUserEntriesFromHistory() ?? null;
orphanPushCountSnapshot =
this.#getCurrentChat().getUserContentPushCount?.() ?? 0;
continuationParts = recoveryPlan.continuation.parts;
Expand All @@ -5322,7 +5324,7 @@ export class Session implements SessionContext {
// The orphaned content is already persisted; recording a new user
// message would duplicate the turn in the transcript.
} else if (isRetry) {
this.#getCurrentChat().stripOrphanedUserEntriesFromHistory();
this.config.getLlmClient()!.stripOrphanedUserEntriesFromHistory();
} else if (!isSlashInput || slashCommandName !== 'advisor') {
// record user message for session management. Only `/advisor`
// defers its record to after command resolution below — a
Expand Down Expand Up @@ -11916,6 +11918,13 @@ export class Session implements SessionContext {
// The VS Code extension is just a UI layer for requestPermission.
const isAskUserQuestionTool =
policyToolName === ToolNames.ASK_USER_QUESTION;
// Core keeps built-in tool classes lazy-loaded. The bundle's
// keepNames preserves this class check; name and kind also reject
// MCP and registry shadows.
const isTrustedAskUserQuestionTool =
isAskUserQuestionTool &&
tool.kind === Kind.Think &&
tool.constructor.name === 'AskUserQuestionTool';
// ---- L3→L4: Shared permission flow ----
let toolParams = invocation.params as Record<string, unknown>;
const flowResult =
Expand Down Expand Up @@ -12104,15 +12113,17 @@ export class Session implements SessionContext {
// exactly that tail rather than triggering a `structuredClone`
// of the whole session on every non-fast-path AUTO call.
// Parallels coreToolScheduler.ts.
const llmClient = this.config.getLlmClient?.();
const messages =
this.config
.getLlmClient?.()
?.getHistoryTail(MAX_TRANSCRIPT_MESSAGES, false) ?? [];
llmClient?.getHistoryTail(MAX_TRANSCRIPT_MESSAGES, false) ?? [];
const trustedUserAnswers =
llmClient?.getTrustedUserAnswers?.() ?? [];
const decision = await evaluateAutoMode({
ctx: pmCtx,
pmForcedAsk,
toolParams,
messages,
trustedUserAnswers,
config: this.config,
signal: abortSignal,
skipClassifierReason: fallback.fallback
Expand Down Expand Up @@ -12730,6 +12741,19 @@ export class Session implements SessionContext {
if (confirmationCancellation) {
return confirmationCancellation;
}
if (
isTrustedAskUserQuestionTool &&
isApproveOutcome(outcome) &&
confirmationDetails.type === 'ask_user_question'
) {
this.config
.getLlmClient?.()
?.recordTrustedUserAnswers(
callId,
confirmationDetails.questions,
output.answers,
);
}
} catch (error) {
if (outcome !== ToolConfirmationOutcome.Cancel) {
throw error;
Expand Down
38 changes: 38 additions & 0 deletions packages/core/src/core/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,36 @@ describe('Gemini Client (client.ts)', () => {
expect(enableSpy).toHaveBeenCalledTimes(2);
});

it('clears trusted user answers when a chat is rebuilt', async () => {
client.recordTrustedUserAnswers('ask-1', [{ question: 'Continue?' }], {
'0': 'No',
});
expect(client.getTrustedUserAnswers()).toHaveLength(1);

await client.startChat(
[{ role: 'user', parts: [{ text: 'resumed' }] }],
SessionStartSource.Resume,
);

expect(client.getTrustedUserAnswers()).toEqual([]);
});

it('keeps trusted user answers when the chat replaces history in place', async () => {
await client.startChat();
client.recordTrustedUserAnswers('ask-1', [{ question: 'Continue?' }], {
'0': 'No',
});

// Pre-send microcompaction, compression, the hard-rescue rollback, and
// the startup-prelude refresh all replace history through LlmChat
// without dropping the ask_user_question pair the projection anchors on.
client
.getChat()
.setHistory([{ role: 'user', parts: [{ text: 'compacted' }] }]);

expect(client.getTrustedUserAnswers()).toHaveLength(1);
});

it('passes startup, resume, and clear sources to the profiler', async () => {
await client.startChat();
await client.startChat([{ role: 'user', parts: [{ text: 'hi' }] }]);
Expand Down Expand Up @@ -3184,10 +3214,14 @@ describe('Gemini Client (client.ts)', () => {
client['chat'] = {
setHistory: vi.fn(),
} as unknown as LlmChat;
client.recordTrustedUserAnswers('ask-1', [{ question: 'Continue?' }], {
'0': 'No',
});

client.setHistory([{ role: 'user', parts: [{ text: 'replaced' }] }]);

expect(cacheClear).toHaveBeenCalled();
expect(client.getTrustedUserAnswers()).toEqual([]);
});

/**
Expand All @@ -3209,10 +3243,14 @@ describe('Gemini Client (client.ts)', () => {
it('truncateHistory clears the cache when entries are actually removed', () => {
const cacheClear = mockFileReadCacheClear();
client['chat'] = mockChatWithLengths(3, 2);
client.recordTrustedUserAnswers('ask-1', [{ question: 'Continue?' }], {
'0': 'No',
});

client.truncateHistory(2);

expect(cacheClear).toHaveBeenCalled();
expect(client.getTrustedUserAnswers()).toEqual([]);
});

it('truncateHistory does NOT clear the cache when nothing was removed (keepCount >= history length)', () => {
Expand Down
Loading
Loading