Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
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