Skip to content
91 changes: 87 additions & 4 deletions packages/cli/src/acp-integration/acpAgent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -844,9 +844,10 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
sendAvailableCommandsUpdate: vi.fn().mockResolvedValue(undefined),
replayHistory: vi.fn().mockResolvedValue(undefined),
installRewriter: vi.fn(),
captureHistorySnapshot: vi
.fn()
.mockReturnValue([{ role: 'user', parts: [{ text: 'before' }] }]),
captureHistorySnapshot: vi.fn().mockReturnValue({
history: [{ role: 'user', parts: [{ text: 'before' }] }],
modelFacingUserTurnCount: 1,
}),
restoreHistory: vi.fn(),
rewindToTurn: vi
.fn()
Expand Down Expand Up @@ -1719,7 +1720,10 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
expect(lastSessionMock?.rewindToTurn).toHaveBeenCalledWith(1);
expect(response).toEqual({
success: true,
historyBeforeRewind: [{ role: 'user', parts: [{ text: 'before' }] }],
historyBeforeRewind: {
history: [{ role: 'user', parts: [{ text: 'before' }] }],
modelFacingUserTurnCount: 1,
},
targetTurnIndex: 1,
apiTruncateIndex: 2,
});
Expand Down Expand Up @@ -1843,6 +1847,85 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
await agentPromise;
});

it('restoreSessionHistory extension method restores history snapshots', async () => {
const sessionId = '11111111-1111-1111-1111-111111111111';
await setupSessionMocks(sessionId);

const agentPromise = runAcpAgent(
mockConfig,
makeSessionSettings(),
mockArgv,
);
await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined());

const agent = capturedAgentFactory!({
get closed() {
return mockConnectionState.promise;
},
}) as AgentLike;

await agent.newSession({ cwd: '/tmp', mcpServers: [] });
const snapshot = {
history: [{ role: 'user', parts: [{ text: 'restored' }] }],
modelFacingUserTurnCount: 1,
};
const response = await agent.extMethod('restoreSessionHistory', {
sessionId,
history: snapshot,
cwd: '/tmp',
});

expect(lastSessionMock?.restoreHistory).toHaveBeenCalledWith(snapshot);
expect(response).toEqual({ success: true });

mockConnectionState.resolve();
await agentPromise;
});

it('restoreSessionHistory rejects invalid history snapshot turn counts', async () => {
const sessionId = '11111111-1111-1111-1111-111111111111';
await setupSessionMocks(sessionId);

const agentPromise = runAcpAgent(
mockConfig,
makeSessionSettings(),
mockArgv,
);
await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined());

const agent = capturedAgentFactory!({
get closed() {
return mockConnectionState.promise;
},
}) as AgentLike;

await agent.newSession({ cwd: '/tmp', mcpServers: [] });

for (const modelFacingUserTurnCount of [
NaN,
Infinity,
-Infinity,
-1,
1.5,
Number.MAX_SAFE_INTEGER + 1,
]) {
await expect(
agent.extMethod('restoreSessionHistory', {
sessionId,
history: {
history: [],
modelFacingUserTurnCount,
},
}),
).rejects.toThrow('Invalid or missing history');
}

expect(lastSessionMock?.restoreHistory).not.toHaveBeenCalled();

mockConnectionState.resolve();
await agentPromise;
});

it('restoreSessionHistory rejects invalid session ids', async () => {
await setupSessionMocks('11111111-1111-1111-1111-111111111111');

Expand Down
27 changes: 24 additions & 3 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ import type { ApprovalModeValue } from './session/types.js';
import { z } from 'zod';
import type { CliArgs } from '../config/config.js';
import { loadCliConfig } from '../config/config.js';
import { Session, buildAvailableCommandsSnapshot } from './session/Session.js';
import {
Session,
buildAvailableCommandsSnapshot,
type HistorySnapshot,
} from './session/Session.js';
import {
formatAcpModelId,
parseAcpBaseModelId,
Expand Down Expand Up @@ -1541,7 +1545,24 @@ class QwenAgent implements Agent {
'Invalid or missing sessionId',
);
}
if (!Array.isArray(history)) {
const isHistorySnapshot =
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.

[Critical] isHistorySnapshot guard accepts NaN, Infinity, -Infinity, negative, and non-integer values for modelFacingUserTurnCount. typeof NaN === 'number' is true in JS. A malformed ACP client can send {history: [...], modelFacingUserTurnCount: NaN} which permanently poisons the session — Math.max(NaN, ...) propagates NaN through all downstream arithmetic in #computeApiTruncationIndexForUserTurn, making every future rewindToTurn fail silently.

Suggested change
const isHistorySnapshot =
const isHistorySnapshot =
!!history &&
typeof history === 'object' &&
!Array.isArray(history) &&
Array.isArray((history as { history?: unknown }).history) &&
Number.isInteger(
(history as { modelFacingUserTurnCount?: unknown })
.modelFacingUserTurnCount,
) &&
((history as { modelFacingUserTurnCount?: unknown })
.modelFacingUserTurnCount as number) >= 0;

Number.isInteger() rejects NaN, Infinity, floats, and the >= 0 rejects negatives — matching the existing validation pattern for targetTurnIndex at line 1004.

— mimo-v2.5-pro via Qwen Code /review

!!history &&
typeof history === 'object' &&
!Array.isArray(history) &&
Array.isArray((history as { history?: unknown }).history) &&
Number.isInteger(
(history as { modelFacingUserTurnCount?: unknown })
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.

[Critical] 空历史快照被接受,可静默清空整个会话。

isHistorySnapshot 接受 { history: [], modelFacingUserTurnCount: 0 }。该快照会通过 session.restoreHistory 调用 chat.setHistory([]) 静默清空整个会话历史。当前校验没有对 history 数组设最小长度限制。

Suggested change
(history as { modelFacingUserTurnCount?: unknown })
// 在 restoreHistory 或 isHistorySnapshot 后添加:
if (Array.isArray(history) && history.length === 0 && modelFacingUserTurnCount > 0) {
throw new Error('Inconsistent snapshot: empty history with non-zero turn count');
}

— DeepSeek/deepseek-v4-pro via Qwen Code /review

.modelFacingUserTurnCount,
) &&
Number.isFinite(
(history as { modelFacingUserTurnCount?: unknown })
.modelFacingUserTurnCount as number,
) &&
((history as { modelFacingUserTurnCount?: unknown })
.modelFacingUserTurnCount as number) >= 0 &&
((history as { modelFacingUserTurnCount?: unknown })
.modelFacingUserTurnCount as number) <= Number.MAX_SAFE_INTEGER;
if (!Array.isArray(history) && !isHistorySnapshot) {
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.

Critical: No upper bound validation for modelFacingUserTurnCount

The validation checks Number.isInteger(modelFacingUserTurnCount) && >= 0 but does NOT check an upper bound. An external caller (e.g., IDE companion extension) can pass an arbitrarily large value.

If modelFacingUserTurnCount > actualTurnCount, then:

  • compressedTurnCount = modelFacingUserTurnCount - apiTailUserIndices.length becomes too large
  • All rewind targets resolve to -1 (blocked), making rewind completely non-functional

Fix: Add an upper bound check, e.g.:

&& ((history as { modelFacingUserTurnCount?: unknown })
  .modelFacingUserTurnCount as number) <= MAX_REASONABLE_TURN_COUNT

Or validate against history.history.length at restore time.


Reviewed by mimo-v2.5-pro

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.

Critical: No upper bound validation for modelFacingUserTurnCount

The validation checks Number.isInteger(modelFacingUserTurnCount) && >= 0 but does NOT check an upper bound. An external caller (e.g., IDE companion extension) can pass an arbitrarily large value.

If modelFacingUserTurnCount > actualTurnCount, then:

  • compressedTurnCount = modelFacingUserTurnCount - apiTailUserIndices.length becomes too large
  • All rewind targets resolve to -1 (blocked), making rewind completely non-functional

Fix: Add an upper bound check, e.g.:

&& ((history as { modelFacingUserTurnCount?: unknown })
  .modelFacingUserTurnCount as number) <= MAX_REASONABLE_TURN_COUNT

Or validate against history.history.length at restore time.


Reviewed by mimo-v2.5-pro

throw RequestError.invalidParams(
undefined,
'Invalid or missing history',
Expand All @@ -1555,7 +1576,7 @@ class QwenAgent implements Agent {
);
}

session.restoreHistory(history as Content[]);
session.restoreHistory(history as Content[] | HistorySnapshot);
return { success: true };
}
case 'getAccountInfo': {
Expand Down
Loading
Loading