Skip to content
Closed
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;
});
});
23 changes: 22 additions & 1 deletion packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,11 @@ class QwenAgent implements Agent {
});

const sessions: SessionInfo[] = result.items.map((item) => ({
cwd: item.cwd,
// `cwd` is now optional on SessionListItem (lite tier omits it),
// but `listSessions` returns enriched items so it's populated in
// practice. Fall back to the request's `cwd` for any edge case
// where the head-record decode failed mid-page.
cwd: item.cwd ?? cwd,
sessionId: item.sessionId,
title: item.customTitle || item.prompt || '(session)',
updatedAt: new Date(item.mtime).toISOString(),
Expand Down Expand Up @@ -486,6 +490,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();
if (liveRecording) {
const ok = liveRecording.recordCustomTitle(title, 'manual');
await liveRecording.flush();
return { success: ok };
}
const success = await runWithAcpRuntimeOutputDir(
this.settings,
cwd,
Expand Down
26 changes: 22 additions & 4 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.
const messageText =
typeof session.messageCount === 'number'
? formatMessageCount(session.messageCount)
: undefined;

const showUpIndicator = isFirst && showScrollUp;
const showDownIndicator = isLast && showScrollDown;
Expand All @@ -101,11 +109,20 @@ function SessionListItemView({
? prefixChars.scrollDown
: prefixChars.normal;

const promptText = session.customTitle || session.prompt || '(empty prompt)';
// Pending rows have come back from `listSessionsLite` but enrichment
// hasn't filled in the title/prompt yet. Show a short sessionId
// prefix as a placeholder so the row is visibly "loading" rather
// than blank — once enrichment completes the row re-renders with
// the real title.
const isPending = session.pending === true;
const promptText = isPending
? `…${session.sessionId.slice(0, 8)}`
: session.customTitle || session.prompt || '(empty prompt)';
const truncatedPrompt = truncateText(promptText, maxPromptWidth);
// Dim auto-generated titles so users can distinguish a model guess from
// a title they chose themselves with `/rename`. Selected row keeps the
// accent color — legibility of the focused row wins over source hinting.
// Pending rows also dim — they're not actionable until enriched.
const isAutoTitle =
session.titleSource === 'auto' && Boolean(session.customTitle);

Expand All @@ -128,7 +145,7 @@ function SessionListItemView({
color={
isSelected
? theme.text.accent
: isAutoTitle
: isAutoTitle || isPending
? theme.text.secondary
: theme.text.primary
}
Expand All @@ -139,7 +156,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
Loading
Loading