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
129 changes: 124 additions & 5 deletions packages/cli/src/acp-integration/session/Session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ describe('Session', () => {
let currentAuthType: AuthType;
let switchModelSpy: ReturnType<typeof vi.fn>;
let getAvailableCommandsSpy: ReturnType<typeof vi.fn>;
let mockToolRegistry: { getTool: ReturnType<typeof vi.fn> };
let mockToolRegistry: {
getTool: ReturnType<typeof vi.fn>;
ensureTool: ReturnType<typeof vi.fn>;
};
beforeEach(() => {
currentModel = 'qwen3-code-plus';
currentAuthType = AuthType.USE_OPENAI;
Expand All @@ -73,11 +76,21 @@ describe('Session', () => {
getHistory: vi.fn().mockReturnValue([]),
} as unknown as GeminiChat;

mockToolRegistry = { getTool: vi.fn() };
mockToolRegistry = {
getTool: vi.fn(),
// #executePrompt → #buildInitialSystemReminders calls
// getToolRegistry().ensureTool(ToolNames.AGENT) on every session.prompt(),
// so the default mock must provide it (#1151 / #3479).
ensureTool: vi.fn().mockResolvedValue(true),
};
const fileService = { shouldGitIgnoreFile: vi.fn().mockReturnValue(false) };

mockConfig = {
setApprovalMode: vi.fn(),
// #buildInitialSystemReminders branches on ApprovalMode.PLAN on every
// session.prompt(), so the default must be defined. Individual tests
// that care override via `mockConfig.getApprovalMode = vi.fn()...`.
getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT),
switchModel: switchModelSpy,
getModel: vi.fn().mockImplementation(() => currentModel),
getSessionId: vi.fn().mockReturnValue('test-session-id'),
Expand All @@ -91,6 +104,12 @@ describe('Session', () => {
recordToolResult: vi.fn(),
}),
getToolRegistry: vi.fn().mockReturnValue(mockToolRegistry),
// #buildInitialSystemReminders iterates listSubagents() on every
// session.prompt(). Default to an empty list so tests that don't
// exercise subagent reminders don't need to stub it (#1151 / #3479).
getSubagentManager: vi.fn().mockReturnValue({
listSubagents: vi.fn().mockResolvedValue([]),
}),
getFileService: vi.fn().mockReturnValue(fileService),
getFileFilteringRespectGitIgnore: vi.fn().mockReturnValue(true),
getEnableRecursiveFileSearch: vi.fn().mockReturnValue(false),
Expand Down Expand Up @@ -134,9 +153,7 @@ describe('Session', () => {
mockConfig = undefined as unknown as Config;
mockClient = undefined as unknown as AgentSideConnection;
mockSettings = undefined as unknown as LoadedSettings;
mockToolRegistry = undefined as unknown as {
getTool: ReturnType<typeof vi.fn>;
};
mockToolRegistry = undefined as unknown as typeof mockToolRegistry;
vi.restoreAllMocks();
vi.clearAllTimers();
});
Expand Down Expand Up @@ -1066,5 +1083,107 @@ describe('Session', () => {
});
});
});

describe('system reminders', () => {
// Captures the `message` parts fed into chat.sendMessageStream on the
// first turn so individual tests can assert what the model saw.
const captureFirstTurnMessage = () => {
const capture: { parts: Array<{ text?: string }> } = { parts: [] };
(mockChat.sendMessageStream as ReturnType<typeof vi.fn>) = vi
.fn()
.mockImplementation(async (_model, req) => {
capture.parts = req.message ?? [];
return createEmptyStream();
});
return capture;
};

const stubEmptySubagents = () => {
(mockConfig as unknown as Record<string, unknown>)[
'getSubagentManager'
] = vi.fn().mockReturnValue({
listSubagents: vi.fn().mockResolvedValue([]),
});
// ensureTool is called on the result of getToolRegistry(); add it.
(
mockToolRegistry as unknown as { ensureTool: () => Promise<boolean> }
).ensureTool = vi.fn().mockResolvedValue(true);
};

it('prepends plan-mode reminder when approval mode is PLAN (#1151)', async () => {
stubEmptySubagents();
mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.PLAN);
const capture = captureFirstTurnMessage();

await session.prompt({
sessionId: 'test-session-id',
prompt: [{ type: 'text', text: 'research this' }],
});

const reminderPart = capture.parts.find(
(p) => p.text && p.text.includes('Plan mode is active'),
);
expect(reminderPart).toBeTruthy();
expect(reminderPart!.text).toContain('exit_plan_mode');
// Reminder comes before the user text, matching client.ts ordering.
const reminderIdx = capture.parts.indexOf(reminderPart!);
const userIdx = capture.parts.findIndex(
(p) => p.text === 'research this',
);
expect(reminderIdx).toBeLessThan(userIdx);
});

it('does not prepend plan-mode reminder in default approval mode', async () => {
stubEmptySubagents();
mockConfig.getApprovalMode = vi
.fn()
.mockReturnValue(ApprovalMode.DEFAULT);
const capture = captureFirstTurnMessage();

await session.prompt({
sessionId: 'test-session-id',
prompt: [{ type: 'text', text: 'hi' }],
});

const hasPlanReminder = capture.parts.some(
(p) => p.text && p.text.includes('Plan mode is active'),
);
expect(hasPlanReminder).toBe(false);
});

it('prepends subagent reminder when user-level subagents exist', async () => {
(mockConfig as unknown as Record<string, unknown>)[
'getSubagentManager'
] = vi.fn().mockReturnValue({
listSubagents: vi.fn().mockResolvedValue([
{ name: 'researcher', level: 'user' },
{ name: 'planner', level: 'project' },
// builtin entries are filtered out, matching client.ts:853.
{ name: 'builtin-helper', level: 'builtin' },
]),
});
(
mockToolRegistry as unknown as { ensureTool: () => Promise<boolean> }
).ensureTool = vi.fn().mockResolvedValue(true);
mockConfig.getApprovalMode = vi
.fn()
.mockReturnValue(ApprovalMode.DEFAULT);
const capture = captureFirstTurnMessage();

await session.prompt({
sessionId: 'test-session-id',
prompt: [{ type: 'text', text: 'hi' }],
});

const reminder = capture.parts.find(
(p) =>
p.text &&
p.text.includes('researcher') &&
p.text.includes('planner'),
);
expect(reminder).toBeTruthy();
expect(reminder!.text).not.toContain('builtin-helper');
});
});
});
});
63 changes: 62 additions & 1 deletion packages/cli/src/acp-integration/session/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ import {
createHookOutput,
generateToolUseId,
MessageBusType,
getPlanModeSystemReminder,
getSubagentSystemReminder,
getArenaSystemReminder,
} from '@qwen-code/qwen-code-core';

import { RequestError } from '@agentclientprotocol/sdk';
Expand Down Expand Up @@ -400,6 +403,16 @@ export class Session implements SessionContext {
}
}

// Prepend session-level system reminders (plan mode / subagent /
// arena) so the model sees them, matching the behaviour of
// `GeminiClient.sendMessageStream` in the CLI/TUI path. Without this,
// plan mode in ACP has no effect because the model never learns it
// should avoid edits (#1151).
const systemReminders = await this.#buildInitialSystemReminders();
if (systemReminders.length > 0) {
parts = [...systemReminders, ...parts];
}

let nextMessage: Content | null = { role: 'user', parts };

while (nextMessage !== null) {
Expand Down Expand Up @@ -872,9 +885,12 @@ export class Session implements SessionContext {
_meta: { source: 'cron' },
});

// Prepend session-level system reminders (same rationale as the
// user-query path in #executePrompt).
const cronReminders = await this.#buildInitialSystemReminders();
let nextMessage: Content | null = {
role: 'user',
parts: [{ text: prompt }],
parts: [...cronReminders, { text: prompt }],
};

while (nextMessage !== null) {
Expand Down Expand Up @@ -1090,6 +1106,51 @@ export class Session implements SessionContext {
await this.sendUpdate(update);
}

/**
* Assemble the per-turn system reminders the model needs to see at the
* start of a user query or cron fire. Mirrors the subagent/plan/arena
* branches in `GeminiClient.sendMessageStream` (`client.ts:848-878`) —
* the ACP path bypasses that code, so without this helper plan mode is
* silently inert (#1151) and subagent/arena sessions lose context.
*
* Scope note: the `relevantAutoMemory` reminder is intentionally NOT
* included here. Managed auto-memory requires a prefetch pipeline that
* lives in `GeminiClient`, and porting it into the ACP path is tracked
* separately as part of the broader middleware-alignment work.
*/
async #buildInitialSystemReminders(): Promise<Part[]> {
const reminders: Part[] = [];

const hasAgentTool = await this.config
.getToolRegistry()
.ensureTool(ToolNames.AGENT);
const subagents = (await this.config.getSubagentManager().listSubagents())
.filter((subagent) => subagent.level !== 'builtin')
.map((subagent) => subagent.name);
if (hasAgentTool && subagents.length > 0) {
reminders.push({ text: getSubagentSystemReminder(subagents) });
}

if (this.config.getApprovalMode() === ApprovalMode.PLAN) {
reminders.push({
text: getPlanModeSystemReminder(this.config.getSdkMode?.()),
});
}

const arenaManager = this.config.getArenaManager?.();
if (arenaManager) {
try {
const sessionDir = arenaManager.getArenaSessionDir();
const configPath = `${sessionDir}/config.json`;
reminders.push({ text: getArenaSystemReminder(configPath) });
} catch {
// Arena config not yet initialized — skip (matches client.ts).
}
}

return reminders;
}

private async runTool(
abortSignal: AbortSignal,
promptId: string,
Expand Down
Loading