Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
236 changes: 235 additions & 1 deletion packages/cli/src/acp-integration/acpAgent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ vi.mock('@qwen-code/qwen-code-core', () => ({
_args: args,
})),
SessionService: vi.fn(),
SESSION_TITLE_MAX_LENGTH: 200,
tokenLimit: vi.fn(),
SessionStartSource: {
Startup: 'startup',
Expand All @@ -103,6 +104,16 @@ vi.mock('@qwen-code/qwen-code-core', () => ({
},
}));

vi.mock('./runtimeOutputDirContext.js', () => ({
runWithAcpRuntimeOutputDir: vi.fn(
async <T>(
_settings: unknown,
_cwd: string,
fn: () => T | Promise<T>,
): Promise<T> => fn(),
),
}));

vi.mock('./authMethods.js', () => ({ buildAuthMethods: vi.fn() }));
vi.mock('./service/filesystem.js', () => ({
AcpFileSystemService: vi.fn(),
Expand All @@ -126,7 +137,11 @@ import {
import type { Config } from '@qwen-code/qwen-code-core';
import type { LoadedSettings } from '../config/settings.js';
import type { CliArgs } from '../config/config.js';
import { SessionEndReason, MCPServerConfig } from '@qwen-code/qwen-code-core';
import {
SessionEndReason,
MCPServerConfig,
SessionService,
} from '@qwen-code/qwen-code-core';
import type { McpServer } from '@agentclientprotocol/sdk';
import { AgentSideConnection } from '@agentclientprotocol/sdk';
import { loadSettings } from '../config/settings.js';
Expand Down Expand Up @@ -894,3 +909,222 @@ describe('QwenAgent MCP SSE/HTTP support', () => {
await agentPromise;
});
});

// Regression coverage for the MR-review finding that ACP renameSession
// bypassed any live ChatRecordingService. The disk-only path left the
// recording service's in-memory `currentCustomTitle` stale, and the next
// re-anchor (every 32KB) or finalize() silently reverted the rename by
// re-emitting the cached old title at EOF.
describe('QwenAgent extMethod renameSession routing', () => {
type AgentSideConnectionLike = { closed: Promise<void> };
type AgentLike = {
initialize: (args: Record<string, unknown>) => Promise<unknown>;
newSession: (args: Record<string, unknown>) => Promise<unknown>;
extMethod: (
method: string,
params: Record<string, unknown>,
) => Promise<Record<string, unknown>>;
};

let capturedAgentFactory:
| ((conn: AgentSideConnectionLike) => AgentLike)
| undefined;
let mockConfig: Config;

// Live session sessionId is whatever `getSessionId()` on the inner config
// returns; matches the existing test scaffolding.
const liveSessionId = '550e8400-e29b-41d4-a716-446655440000';

beforeEach(() => {
vi.clearAllMocks();
mockConnectionState.reset();
capturedAgentFactory = undefined;

vi.mocked(AgentSideConnection).mockImplementation((factory: unknown) => {
capturedAgentFactory = factory as typeof capturedAgentFactory;
return {
get closed() {
return mockConnectionState.promise;
},
} as unknown as InstanceType<typeof AgentSideConnection>;
});

mockConfig = {
initialize: vi.fn().mockResolvedValue(undefined),
getHookSystem: vi.fn().mockReturnValue(undefined),
getDisableAllHooks: vi.fn().mockReturnValue(false),
hasHooksForEvent: vi.fn().mockReturnValue(false),
getModel: vi.fn().mockReturnValue('test-model'),
getModelsConfig: vi.fn().mockReturnValue({
getCurrentAuthType: vi.fn().mockReturnValue('api-key'),
}),
refreshAuth: vi.fn().mockResolvedValue(undefined),
} as unknown as Config;
});

function makeRecordingService() {
return {
recordCustomTitle: vi.fn().mockReturnValue(true),
flush: vi.fn().mockResolvedValue(undefined),
};
}

function makeLiveSessionInnerConfig(
recording: ReturnType<typeof makeRecordingService> | null,
) {
return {
initialize: vi.fn().mockResolvedValue(undefined),
getModelsConfig: vi.fn().mockReturnValue({
getCurrentAuthType: vi.fn().mockReturnValue('api-key'),
}),
refreshAuth: vi.fn().mockResolvedValue(undefined),
getModel: vi.fn().mockReturnValue('m'),
getContentGeneratorConfig: vi.fn().mockReturnValue({}),
getAvailableModels: vi.fn().mockReturnValue([]),
getModes: vi.fn().mockReturnValue([]),
getApprovalMode: vi.fn().mockReturnValue('default'),
getSessionId: vi.fn().mockReturnValue(liveSessionId),
getAuthType: vi.fn().mockReturnValue('api-key'),
getAllConfiguredModels: vi.fn().mockReturnValue([]),
getGeminiClient: vi.fn().mockReturnValue({
isInitialized: vi.fn().mockReturnValue(true),
initialize: vi.fn().mockResolvedValue(undefined),
}),
getFileSystemService: vi.fn().mockReturnValue(undefined),
setFileSystemService: vi.fn(),
getHookSystem: vi.fn().mockReturnValue(undefined),
getDisableAllHooks: vi.fn().mockReturnValue(true),
hasHooksForEvent: vi.fn().mockReturnValue(false),
getChatRecordingService: vi.fn().mockReturnValue(recording),
};
}

function makeAcpSettings() {
return {
merged: { mcpServers: {} },
getUserHooks: vi.fn().mockReturnValue({}),
getProjectHooks: vi.fn().mockReturnValue({}),
} as unknown as LoadedSettings;
}

async function bootAgent(
innerConfig: ReturnType<typeof makeLiveSessionInnerConfig>,
) {
vi.mocked(loadSettings).mockReturnValue(makeAcpSettings());
vi.mocked(loadCliConfig).mockResolvedValue(
innerConfig as unknown as Config,
);
vi.mocked(Session).mockImplementation(
() =>
({
getId: vi.fn().mockReturnValue(liveSessionId),
getConfig: vi.fn().mockReturnValue(innerConfig),
sendAvailableCommandsUpdate: vi.fn().mockResolvedValue(undefined),
replayHistory: vi.fn().mockResolvedValue(undefined),
installRewriter: vi.fn(),
}) as unknown as InstanceType<typeof Session>,
);

const agentPromise = runAcpAgent(
mockConfig,
makeAcpSettings(),
{} as CliArgs,
);
await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined());
const agent = capturedAgentFactory!({
get closed() {
return mockConnectionState.promise;
},
}) as AgentLike;
return { agent, agentPromise };
}

it('routes through ChatRecordingService.recordCustomTitle when the target session is live', async () => {
const recording = makeRecordingService();
const innerConfig = makeLiveSessionInnerConfig(recording);
const { agent, agentPromise } = await bootAgent(innerConfig);

// Populate `this.sessions` so the rename target is "live".
await agent.newSession({ cwd: '/tmp', mcpServers: [] });

const result = await agent.extMethod('renameSession', {
cwd: '/tmp',
sessionId: liveSessionId,
title: 'New Title',
});

expect(recording.recordCustomTitle).toHaveBeenCalledWith(
'New Title',
'manual',
);
// Awaited so the rename is durable before the response returns —
// a follow-up listSessions can't race the queued write.
expect(recording.flush).toHaveBeenCalledOnce();
// The disk-only fallback must NOT fire when a live session exists,
// otherwise we'd double-write (and the second writer would be the
// SessionService that lacks the in-memory cache update).
expect(SessionService).not.toHaveBeenCalled();
expect(result).toEqual({ success: true });

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

it('falls back to SessionService.renameSession when no live session matches the sessionId', async () => {
const recording = makeRecordingService();
const innerConfig = makeLiveSessionInnerConfig(recording);
const { agent, agentPromise } = await bootAgent(innerConfig);

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

const renameSpy = vi.fn().mockResolvedValue(true);
vi.mocked(SessionService).mockImplementation(
() =>
({
renameSession: renameSpy,
}) as unknown as InstanceType<typeof SessionService>,
);

const deadSessionId = '6ba7b810-9dad-11d1-80b4-00c04fd430c8';
const result = await agent.extMethod('renameSession', {
cwd: '/tmp',
sessionId: deadSessionId,
title: 'Renamed Offline',
});

expect(SessionService).toHaveBeenCalledWith('/tmp');
expect(renameSpy).toHaveBeenCalledWith(deadSessionId, 'Renamed Offline');
// The live recording belongs to a *different* sessionId; it must
// be left untouched, otherwise we'd corrupt an unrelated session's
// title cache.
expect(recording.recordCustomTitle).not.toHaveBeenCalled();
expect(result).toEqual({ success: true });

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

it('returns success=false when the live ChatRecordingService rejects the title (I/O error)', async () => {
const recording = makeRecordingService();
recording.recordCustomTitle.mockReturnValue(false);
const innerConfig = makeLiveSessionInnerConfig(recording);
const { agent, agentPromise } = await bootAgent(innerConfig);

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

const result = await agent.extMethod('renameSession', {
cwd: '/tmp',
sessionId: liveSessionId,
title: 'New Title',
});

// Even on failure we still flush so the writeChain settles before
// responding — keeps subsequent reads consistent and surfaces any
// queued earlier failure to the caller.
expect(recording.flush).toHaveBeenCalledOnce();
expect(result).toEqual({ success: false });

mockConnectionState.resolve();
await agentPromise;
});
});
17 changes: 17 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,23 @@ class QwenAgent implements Agent {
`Title too long (max ${SESSION_TITLE_MAX_LENGTH} chars)`,
);
}
// When the target session is currently live in this process, route
// through its ChatRecordingService so the in-memory `currentCustomTitle`
// stays in sync. Writing directly to disk via SessionService here
// would leave the live recording's cache stale; the next title
// re-anchor (every 32KB of writes) or finalize() would re-emit the
// old title and silently revert the rename. The disk-only path
// remains for the dead-session case (e.g., another client renaming
// a session that isn't active in this process).
const liveRecording = this.sessions
.get(sessionId)
?.getConfig()
.getChatRecordingService();
Comment thread
wenshao marked this conversation as resolved.
if (liveRecording) {
Comment thread
wenshao marked this conversation as resolved.
Comment thread
wenshao marked this conversation as resolved.
const ok = liveRecording.recordCustomTitle(title, 'manual');
await liveRecording.flush();
return { success: ok };
}
const success = await runWithAcpRuntimeOutputDir(
this.settings,
cwd,
Expand Down
13 changes: 11 additions & 2 deletions packages/cli/src/ui/components/SessionPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,15 @@ function SessionListItemView({
boldSelectedPrefix = true,
}: SessionListItemViewProps): React.JSX.Element {
const timeAgo = formatRelativeTime(session.mtime);
const messageText = formatMessageCount(session.messageCount);
// `messageCount` is now optional on `SessionListItem` because counting
// requires a full readline pass over the JSONL — far too expensive to do
// in the listing path. The row simply omits the "N messages" segment
// when the count isn't available; preview-style consumers that care can
// call `SessionService.countSessionMessages(sessionId)` lazily.
Comment thread
wenshao marked this conversation as resolved.
const messageText =
typeof session.messageCount === 'number'
Comment thread
wenshao marked this conversation as resolved.
? formatMessageCount(session.messageCount)
: undefined;

const showUpIndicator = isFirst && showScrollUp;
const showDownIndicator = isLast && showScrollDown;
Expand Down Expand Up @@ -139,7 +147,8 @@ function SessionListItemView({
</Box>
<Box paddingLeft={2}>
<Text color={theme.text.secondary}>
{timeAgo} · {messageText}
{timeAgo}
{messageText !== undefined && ` · ${messageText}`}
{session.gitBranch && ` · ${session.gitBranch}`}
</Text>
</Box>
Expand Down
22 changes: 22 additions & 0 deletions packages/cli/src/ui/components/SessionPreview.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,28 @@ describe('SessionPreview', () => {
expect(frame).toContain('feat/preview');
});

it('falls back to count from loaded conversation when messageCount prop is absent', async () => {
// listSessions() now omits messageCount, so the picker passes undefined
// through to SessionPreview. The footer must still show a count, derived
// from the loaded ResumedSessionData using unique user/assistant UUIDs.
const svc = mockService(fakeResumedData());
const { lastFrame } = render(
<KeypressProvider kittyProtocolEnabled={false}>
<SessionPreview
sessionService={svc}
sessionId="s1"
sessionTitle="My session"
onExit={vi.fn()}
onResume={vi.fn()}
/>
</KeypressProvider>,
);
await wait(100);
const frame = lastFrame() ?? '';
expect(frame).toMatch(/2\s*messages/);
expect(frame).not.toContain('undefined');
});

it('calls onExit when Escape is pressed', async () => {
const onExit = vi.fn();
const svc = mockService(fakeResumedData());
Expand Down
21 changes: 19 additions & 2 deletions packages/cli/src/ui/components/SessionPreview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,23 @@ export function SessionPreview(props: SessionPreviewProps) {
return buildResumedHistoryItems(data, null);
}, [data]);

// `listSessions` omits `messageCount` for perf, so the prop is usually
// undefined in practice. Compute the count from the loaded conversation
// using the same unique-user/assistant-uuid semantics as
// `SessionService.countSessionMessages` — the data is already in memory,
// so this is free and avoids an extra disk read.
const computedMessageCount = useMemo(() => {
if (!data) return undefined;
const seen = new Set<string>();
for (const msg of data.conversation.messages) {
if (msg.type === 'user' || msg.type === 'assistant') {
seen.add(msg.uuid);
}
}
return seen.size;
}, [data]);
const displayMessageCount = messageCount ?? computedMessageCount;

useKeypress(
(key) => {
const { name, ctrl } = key;
Expand All @@ -98,8 +115,8 @@ export function SessionPreview(props: SessionPreviewProps) {
const separatorWidth = Math.max(0, boxWidth - 2);

const metaParts: string[] = [];
if (typeof messageCount === 'number') {
metaParts.push(formatMessageCount(messageCount));
if (typeof displayMessageCount === 'number') {
metaParts.push(formatMessageCount(displayMessageCount));
}
if (typeof mtime === 'number') {
metaParts.push(formatRelativeTime(mtime));
Expand Down
Loading
Loading