From 61a46fb9332ebd678f7462f3c7c6ea81b97b1a96 Mon Sep 17 00:00:00 2001 From: Abhijit Balaji Date: Thu, 19 Mar 2026 15:15:07 -0700 Subject: [PATCH] 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. --- packages/core/src/config/config.test.ts | 11 +++ packages/core/src/config/config.ts | 5 +- packages/core/src/policy/topic-policy.test.ts | 7 +- .../core/src/prompts/promptProvider.test.ts | 23 +++--- packages/core/src/prompts/promptProvider.ts | 3 +- packages/core/src/tools/topicTool.test.ts | 70 ++++++++++++------- packages/core/src/tools/topicTool.ts | 69 ++++++++++++------ 7 files changed, 123 insertions(+), 65 deletions(-) diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 162f82bb25..be0bae302e 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -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'); + }); }); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 5cd1525c03..6d86285889 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -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, () => diff --git a/packages/core/src/policy/topic-policy.test.ts b/packages/core/src/policy/topic-policy.test.ts index e0300ed443..7fc6197a72 100644 --- a/packages/core/src/policy/topic-policy.test.ts +++ b/packages/core/src/policy/topic-policy.test.ts @@ -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; diff --git a/packages/core/src/prompts/promptProvider.test.ts b/packages/core/src/prompts/promptProvider.test.ts index d9ef21c99f..978eec05bf 100644 --- a/packages/core/src/prompts/promptProvider.test.ts +++ b/packages/core/src/prompts/promptProvider.test.ts @@ -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).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); diff --git a/packages/core/src/prompts/promptProvider.ts b/packages/core/src/prompts/promptProvider.ts index f869bcfa48..169c394b5f 100644 --- a/packages/core/src/prompts/promptProvider.ts +++ b/packages/core/src/prompts/promptProvider.ts @@ -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}]`; } diff --git a/packages/core/src/tools/topicTool.test.ts b/packages/core/src/tools/topicTool.test.ts index 2d22359393..0bdf944183 100644 --- a/packages/core/src/tools/topicTool.test.ts +++ b/packages/core/src/tools/topicTool.test.ts @@ -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(); }); }); diff --git a/packages/core/src/tools/topicTool.ts b/packages/core/src/tools/topicTool.ts index ed7e7cab2e..c204dffee4 100644 --- a/packages/core/src/tools/topicTool.ts +++ b/packages/core/src/tools/topicTool.ts @@ -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 { 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, + ); } }