mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-06-12 20:37:08 -07:00
refactor(core): transition topic management from singleton to session-scoped TopicState
- Move topic tracking from global TopicManager to a session-scoped TopicState within Config. - Implement sanitization to strip newlines and carriage returns from topic titles. - Reject empty or whitespace-only topic titles for improved robustness. - Update CreateNewTopicTool and PromptProvider to use the session-scoped state. - Disable topicUpdateNarration by default in .gemini/settings.json. - Enhance test coverage for state independence and sanitization.
This commit is contained in:
@@ -1694,6 +1694,17 @@ describe('Server Config (config.ts)', () => {
|
||||
const excluded = config.getExcludeTools();
|
||||
expect(excluded!.has(CREATE_NEW_TOPIC_TOOL_NAME)).toBe(true);
|
||||
});
|
||||
|
||||
it('should have independent TopicState across instances', () => {
|
||||
const config1 = new Config(baseParams);
|
||||
const config2 = new Config(baseParams);
|
||||
|
||||
config1.topicState.setTopic('Topic 1');
|
||||
config2.topicState.setTopic('Topic 2');
|
||||
|
||||
expect(config1.topicState.getTopic()).toBe('Topic 1');
|
||||
expect(config2.topicState.getTopic()).toBe('Topic 2');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -34,7 +34,7 @@ import { WebFetchTool } from '../tools/web-fetch.js';
|
||||
import { MemoryTool, setGeminiMdFilename } from '../tools/memoryTool.js';
|
||||
import { WebSearchTool } from '../tools/web-search.js';
|
||||
import { AskUserTool } from '../tools/ask-user.js';
|
||||
import { CreateNewTopicTool } from '../tools/topicTool.js';
|
||||
import { CreateNewTopicTool, TopicState } from '../tools/topicTool.js';
|
||||
import { ExitPlanModeTool } from '../tools/exit-plan-mode.js';
|
||||
import { EnterPlanModeTool } from '../tools/enter-plan-mode.js';
|
||||
import { GeminiClient } from '../core/client.js';
|
||||
@@ -699,6 +699,7 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
private clientVersion: string;
|
||||
private fileSystemService: FileSystemService;
|
||||
private trackerService?: TrackerService;
|
||||
readonly topicState = new TopicState();
|
||||
private contentGeneratorConfig!: ContentGeneratorConfig;
|
||||
private contentGenerator!: ContentGenerator;
|
||||
readonly modelConfigService: ModelConfigService;
|
||||
@@ -3198,7 +3199,7 @@ export class Config implements McpContext, AgentLoopContext {
|
||||
};
|
||||
|
||||
maybeRegister(CreateNewTopicTool, () =>
|
||||
registry.registerTool(new CreateNewTopicTool(this.messageBus)),
|
||||
registry.registerTool(new CreateNewTopicTool(this, this.messageBus)),
|
||||
);
|
||||
|
||||
maybeRegister(LSTool, () =>
|
||||
|
||||
@@ -6,18 +6,15 @@
|
||||
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import * as path from 'node:path';
|
||||
import { fileURLToPath } from 'node:url';
|
||||
import { loadPoliciesFromToml } from './toml-loader.js';
|
||||
import { PolicyEngine } from './policy-engine.js';
|
||||
import { ApprovalMode, PolicyDecision } from './types.js';
|
||||
import { CREATE_NEW_TOPIC_TOOL_NAME } from '../tools/tool-names.js';
|
||||
|
||||
const __filename = fileURLToPath(import.meta.url);
|
||||
const __dirname = path.dirname(__filename);
|
||||
|
||||
describe('Topic Tool Policy', () => {
|
||||
async function loadDefaultPolicies() {
|
||||
const policiesDir = path.resolve(__dirname, 'policies');
|
||||
// Path relative to packages/core root
|
||||
const policiesDir = path.resolve(process.cwd(), 'src/policy/policies');
|
||||
const getPolicyTier = () => 1; // Default tier
|
||||
const result = await loadPoliciesFromToml([policiesDir], getPolicyTier);
|
||||
return result.rules;
|
||||
|
||||
@@ -16,7 +16,7 @@ import { ApprovalMode } from '../policy/types.js';
|
||||
import { DiscoveredMCPTool } from '../tools/mcp-tool.js';
|
||||
import { MockTool } from '../test-utils/mock-tool.js';
|
||||
import { CREATE_NEW_TOPIC_TOOL_NAME } from '../tools/tool-names.js';
|
||||
import { TopicManager } from '../tools/topicTool.js';
|
||||
import { TopicState } from '../tools/topicTool.js';
|
||||
import type { CallableTool } from '@google/genai';
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
import type { ToolRegistry } from '../tools/tool-registry.js';
|
||||
@@ -55,6 +55,7 @@ describe('PromptProvider', () => {
|
||||
).getToolRegistry?.() as unknown as ToolRegistry;
|
||||
},
|
||||
getToolRegistry: vi.fn().mockReturnValue(mockToolRegistry),
|
||||
topicState: new TopicState(),
|
||||
getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true),
|
||||
storage: {
|
||||
getProjectTempDir: vi.fn().mockReturnValue('/tmp/project-temp'),
|
||||
@@ -240,25 +241,23 @@ describe('PromptProvider', () => {
|
||||
|
||||
describe('Topic & Update Narration', () => {
|
||||
beforeEach(() => {
|
||||
TopicManager.getInstance().reset();
|
||||
mockConfig.topicState.reset();
|
||||
vi.mocked(mockConfig.isTopicUpdateNarrationEnabled).mockReturnValue(true);
|
||||
(mockConfig.getToolRegistry as ReturnType<typeof vi.fn>).mockReturnValue({
|
||||
getAllToolNames: vi.fn().mockReturnValue([CREATE_NEW_TOPIC_TOOL_NAME]),
|
||||
getAllTools: vi
|
||||
.fn()
|
||||
.mockReturnValue([
|
||||
new MockTool({
|
||||
name: CREATE_NEW_TOPIC_TOOL_NAME,
|
||||
displayName: 'Topic',
|
||||
}),
|
||||
]),
|
||||
getAllTools: vi.fn().mockReturnValue([
|
||||
new MockTool({
|
||||
name: CREATE_NEW_TOPIC_TOOL_NAME,
|
||||
displayName: 'Topic',
|
||||
}),
|
||||
]),
|
||||
});
|
||||
vi.mocked(mockConfig.getHasAccessToPreviewModel).mockReturnValue(true);
|
||||
vi.mocked(mockConfig.getGemini31LaunchedSync).mockReturnValue(true);
|
||||
});
|
||||
|
||||
it('should include active topic context when narration is enabled', () => {
|
||||
TopicManager.getInstance().setTopic('Active Chapter');
|
||||
mockConfig.topicState.setTopic('Active Chapter');
|
||||
const provider = new PromptProvider();
|
||||
const prompt = provider.getCoreSystemPrompt(mockConfig);
|
||||
|
||||
@@ -269,7 +268,7 @@ describe('PromptProvider', () => {
|
||||
vi.mocked(mockConfig.isTopicUpdateNarrationEnabled).mockReturnValue(
|
||||
false,
|
||||
);
|
||||
TopicManager.getInstance().setTopic('Active Chapter');
|
||||
mockConfig.topicState.setTopic('Active Chapter');
|
||||
const provider = new PromptProvider();
|
||||
const prompt = provider.getCoreSystemPrompt(mockConfig);
|
||||
|
||||
|
||||
@@ -32,7 +32,6 @@ import { resolveModel, supportsModernFeatures } from '../config/models.js';
|
||||
import { DiscoveredMCPTool } from '../tools/mcp-tool.js';
|
||||
import { getAllGeminiMdFilenames } from '../tools/memoryTool.js';
|
||||
import type { AgentLoopContext } from '../config/agent-loop-context.js';
|
||||
import { TopicManager } from '../tools/topicTool.js';
|
||||
|
||||
/**
|
||||
* Orchestrates prompt generation by gathering context and building options.
|
||||
@@ -246,7 +245,7 @@ export class PromptProvider {
|
||||
|
||||
// Context Reinjection (Active Topic)
|
||||
if (context.config.isTopicUpdateNarrationEnabled()) {
|
||||
const activeTopic = TopicManager.getInstance().getTopic();
|
||||
const activeTopic = context.config.topicState.getTopic();
|
||||
if (activeTopic) {
|
||||
sanitizedPrompt += `\n\n[Active Topic: ${activeTopic}]`;
|
||||
}
|
||||
|
||||
@@ -5,49 +5,73 @@
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, vi } from 'vitest';
|
||||
import { TopicManager, CreateNewTopicTool } from './topicTool.js';
|
||||
import { TopicState, CreateNewTopicTool } from './topicTool.js';
|
||||
import { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
import type { PolicyEngine } from '../policy/policy-engine.js';
|
||||
import {
|
||||
CREATE_NEW_TOPIC_TOOL_NAME,
|
||||
TOPIC_PARAM_TITLE,
|
||||
} from './definitions/base-declarations.js';
|
||||
import type { Config } from '../config/config.js';
|
||||
|
||||
describe('TopicState', () => {
|
||||
let state: TopicState;
|
||||
|
||||
describe('TopicManager', () => {
|
||||
beforeEach(() => {
|
||||
TopicManager.getInstance().reset();
|
||||
state = new TopicState();
|
||||
});
|
||||
|
||||
it('should store and retrieve topic title', () => {
|
||||
const manager = TopicManager.getInstance();
|
||||
expect(manager.getTopic()).toBeUndefined();
|
||||
expect(state.getTopic()).toBeUndefined();
|
||||
const success = state.setTopic('Test Topic');
|
||||
expect(success).toBe(true);
|
||||
expect(state.getTopic()).toBe('Test Topic');
|
||||
});
|
||||
|
||||
manager.setTopic('Test Topic');
|
||||
expect(manager.getTopic()).toBe('Test Topic');
|
||||
it('should sanitize newlines and carriage returns', () => {
|
||||
state.setTopic('Topic\nWith\r\nLines');
|
||||
expect(state.getTopic()).toBe('Topic With Lines');
|
||||
});
|
||||
|
||||
it('should trim whitespace', () => {
|
||||
state.setTopic(' Spaced Topic ');
|
||||
expect(state.getTopic()).toBe('Spaced Topic');
|
||||
});
|
||||
|
||||
it('should reject empty or whitespace-only titles', () => {
|
||||
expect(state.setTopic('')).toBe(false);
|
||||
expect(state.setTopic(' ')).toBe(false);
|
||||
expect(state.setTopic('\n\n')).toBe(false);
|
||||
});
|
||||
|
||||
it('should reset topic', () => {
|
||||
const manager = TopicManager.getInstance();
|
||||
manager.setTopic('Test Topic');
|
||||
manager.reset();
|
||||
expect(manager.getTopic()).toBeUndefined();
|
||||
state.setTopic('Test Topic');
|
||||
state.reset();
|
||||
expect(state.getTopic()).toBeUndefined();
|
||||
});
|
||||
|
||||
it('should be a singleton', () => {
|
||||
const manager1 = TopicManager.getInstance();
|
||||
const manager2 = TopicManager.getInstance();
|
||||
expect(manager1).toBe(manager2);
|
||||
it('should be independent across instances', () => {
|
||||
const state1 = new TopicState();
|
||||
const state2 = new TopicState();
|
||||
state1.setTopic('Topic 1');
|
||||
state2.setTopic('Topic 2');
|
||||
expect(state1.getTopic()).toBe('Topic 1');
|
||||
expect(state2.getTopic()).toBe('Topic 2');
|
||||
});
|
||||
});
|
||||
|
||||
describe('CreateNewTopicTool', () => {
|
||||
let tool: CreateNewTopicTool;
|
||||
let mockMessageBus: MessageBus;
|
||||
let mockConfig: Config;
|
||||
|
||||
beforeEach(() => {
|
||||
mockMessageBus = new MessageBus(vi.mocked({} as PolicyEngine));
|
||||
tool = new CreateNewTopicTool(mockMessageBus);
|
||||
TopicManager.getInstance().reset();
|
||||
// Mock enough of Config to satisfy the tool
|
||||
mockConfig = {
|
||||
topicState: new TopicState(),
|
||||
} as unknown as Config;
|
||||
tool = new CreateNewTopicTool(mockConfig, mockMessageBus);
|
||||
});
|
||||
|
||||
it('should have correct name and display name', () => {
|
||||
@@ -55,21 +79,19 @@ describe('CreateNewTopicTool', () => {
|
||||
expect(tool.displayName).toBe('Create New Topic');
|
||||
});
|
||||
|
||||
it('should update TopicManager on execute', async () => {
|
||||
it('should update TopicState on execute', async () => {
|
||||
const invocation = tool.build({ [TOPIC_PARAM_TITLE]: 'New Chapter' });
|
||||
const result = await invocation.execute(new AbortController().signal);
|
||||
|
||||
expect(result.llmContent).toBe('Current topic: "New Chapter"');
|
||||
expect(TopicManager.getInstance().getTopic()).toBe('New Chapter');
|
||||
expect(mockConfig.topicState.getTopic()).toBe('New Chapter');
|
||||
});
|
||||
|
||||
it('should return error if title is missing', async () => {
|
||||
const invocation = tool.build({ [TOPIC_PARAM_TITLE]: '' } as {
|
||||
[TOPIC_PARAM_TITLE]: string;
|
||||
});
|
||||
it('should return error if title is invalid after sanitization', async () => {
|
||||
const invocation = tool.build({ [TOPIC_PARAM_TITLE]: ' \n ' });
|
||||
const result = await invocation.execute(new AbortController().signal);
|
||||
|
||||
expect(result.error).toBeDefined();
|
||||
expect(TopicManager.getInstance().getTopic()).toBeUndefined();
|
||||
expect(mockConfig.topicState.getTopic()).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -18,25 +18,35 @@ import { ToolErrorType } from './tool-error.js';
|
||||
import type { MessageBus } from '../confirmation-bus/message-bus.js';
|
||||
import { debugLogger } from '../utils/debugLogger.js';
|
||||
import { getCreateNewTopicDeclaration } from './definitions/dynamic-declaration-helpers.js';
|
||||
import type { Config } from '../config/config.js';
|
||||
|
||||
/**
|
||||
* Singleton to manage the current active topic title.
|
||||
* Manages the current active topic title for a session.
|
||||
* Hosted within the Config instance for session-scoping.
|
||||
*/
|
||||
export class TopicManager {
|
||||
private static instance: TopicManager;
|
||||
export class TopicState {
|
||||
private activeTopicTitle?: string;
|
||||
|
||||
private constructor() {}
|
||||
/**
|
||||
* Sanitizes and sets the topic title.
|
||||
* @returns true if the title was valid and set, false otherwise.
|
||||
*/
|
||||
setTopic(title: string): boolean {
|
||||
if (!title) return false;
|
||||
|
||||
static getInstance(): TopicManager {
|
||||
if (!TopicManager.instance) {
|
||||
TopicManager.instance = new TopicManager();
|
||||
// 1. Trim whitespace
|
||||
let sanitized = title.trim();
|
||||
|
||||
// 2. Security: Strip newlines and carriage returns to prevent prompt injection/breakout
|
||||
sanitized = sanitized.replace(/[\r\n]+/g, ' ');
|
||||
|
||||
// 3. Robustness check: Ensure it's not empty after sanitization
|
||||
if (sanitized.length === 0) {
|
||||
return false;
|
||||
}
|
||||
return TopicManager.instance;
|
||||
}
|
||||
|
||||
setTopic(title: string): void {
|
||||
this.activeTopicTitle = title;
|
||||
this.activeTopicTitle = sanitized;
|
||||
return true;
|
||||
}
|
||||
|
||||
getTopic(): string | undefined {
|
||||
@@ -56,6 +66,15 @@ class CreateNewTopicInvocation extends BaseToolInvocation<
|
||||
CreateNewTopicParams,
|
||||
ToolResult
|
||||
> {
|
||||
constructor(
|
||||
params: CreateNewTopicParams,
|
||||
messageBus: MessageBus,
|
||||
toolName: string,
|
||||
private readonly config: Config,
|
||||
) {
|
||||
super(params, messageBus, toolName);
|
||||
}
|
||||
|
||||
getDescription(): string {
|
||||
return `Create new topic: "${this.params[TOPIC_PARAM_TITLE]}"`;
|
||||
}
|
||||
@@ -63,10 +82,12 @@ class CreateNewTopicInvocation extends BaseToolInvocation<
|
||||
async execute(): Promise<ToolResult> {
|
||||
const title = this.params[TOPIC_PARAM_TITLE];
|
||||
|
||||
if (!title) {
|
||||
const success = this.config.topicState.setTopic(title);
|
||||
|
||||
if (!success) {
|
||||
return {
|
||||
llmContent: 'Error: A valid topic title is required.',
|
||||
returnDisplay: 'Error: A valid topic title is required.',
|
||||
llmContent: 'Error: A valid, non-empty topic title is required.',
|
||||
returnDisplay: 'Error: A valid, non-empty topic title is required.',
|
||||
error: {
|
||||
message: 'A valid topic title is required.',
|
||||
type: ToolErrorType.INVALID_TOOL_PARAMS,
|
||||
@@ -74,12 +95,12 @@ class CreateNewTopicInvocation extends BaseToolInvocation<
|
||||
};
|
||||
}
|
||||
|
||||
debugLogger.log(`[TopicTool] Changing topic to: "${title.trim()}"`);
|
||||
TopicManager.getInstance().setTopic(title.trim());
|
||||
const setTopic = this.config.topicState.getTopic()!;
|
||||
debugLogger.log(`[TopicTool] Changing topic to: "${setTopic}"`);
|
||||
|
||||
return {
|
||||
llmContent: `Current topic: "${title.trim()}"`,
|
||||
returnDisplay: `Current topic: **${title.trim()}**`,
|
||||
llmContent: `Current topic: "${setTopic}"`,
|
||||
returnDisplay: `Current topic: **${setTopic}**`,
|
||||
};
|
||||
}
|
||||
}
|
||||
@@ -91,7 +112,10 @@ export class CreateNewTopicTool extends BaseDeclarativeTool<
|
||||
CreateNewTopicParams,
|
||||
ToolResult
|
||||
> {
|
||||
constructor(messageBus: MessageBus) {
|
||||
constructor(
|
||||
private readonly config: Config,
|
||||
messageBus: MessageBus,
|
||||
) {
|
||||
const declaration = getCreateNewTopicDeclaration();
|
||||
super(
|
||||
CREATE_NEW_TOPIC_TOOL_NAME,
|
||||
@@ -107,6 +131,11 @@ export class CreateNewTopicTool extends BaseDeclarativeTool<
|
||||
params: CreateNewTopicParams,
|
||||
messageBus: MessageBus,
|
||||
): CreateNewTopicInvocation {
|
||||
return new CreateNewTopicInvocation(params, messageBus, this.name);
|
||||
return new CreateNewTopicInvocation(
|
||||
params,
|
||||
messageBus,
|
||||
this.name,
|
||||
this.config,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user