From ef5627eecee5ea0b9f46c158c72020e43cb664b0 Mon Sep 17 00:00:00 2001 From: Christian Gunderman Date: Mon, 16 Mar 2026 16:24:27 +0000 Subject: [PATCH] Disallow Object.create() and reflect. (#22408) --- eslint.config.js | 18 +++- packages/core/src/agents/agent-scheduler.ts | 12 +-- packages/core/src/agents/local-executor.ts | 15 +--- packages/core/src/agents/registry.ts | 66 ++++++++++---- .../src/confirmation-bus/message-bus.test.ts | 86 +++++++++++++++++++ .../core/src/confirmation-bus/message-bus.ts | 31 +++++++ packages/core/src/tools/tool-registry.ts | 9 ++ packages/core/src/utils/stdio.ts | 52 ++++++----- packages/sdk/src/session.ts | 6 +- 9 files changed, 229 insertions(+), 66 deletions(-) diff --git a/eslint.config.js b/eslint.config.js index 150a50d2b7..99b1b28f4b 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -51,6 +51,7 @@ export default tseslint.config( 'evals/**', 'packages/test-utils/**', '.gemini/skills/**', + '**/*.d.ts', ], }, eslint.configs.recommended, @@ -206,11 +207,26 @@ export default tseslint.config( { // Rules that only apply to product code files: ['packages/*/src/**/*.{ts,tsx}'], - ignores: ['**/*.test.ts', '**/*.test.tsx'], + ignores: ['**/*.test.ts', '**/*.test.tsx', 'packages/*/src/test-utils/**'], rules: { '@typescript-eslint/no-unsafe-type-assertion': 'error', '@typescript-eslint/no-unsafe-assignment': 'error', '@typescript-eslint/no-unsafe-return': 'error', + 'no-restricted-syntax': [ + 'error', + ...commonRestrictedSyntaxRules, + { + selector: + 'CallExpression[callee.object.name="Object"][callee.property.name="create"]', + message: + 'Avoid using Object.create() in product code. Use object spread {...obj}, explicit class instantiation, structuredClone(), or copy constructors instead.', + }, + { + selector: 'Identifier[name="Reflect"]', + message: + 'Avoid using Reflect namespace in product code. Do not use reflection to make copies. Instead, use explicit object copying or cloning (structuredClone() for values, new instance/clone function for classes).', + }, + ], }, }, { diff --git a/packages/core/src/agents/agent-scheduler.ts b/packages/core/src/agents/agent-scheduler.ts index d0f4d4004b..852e25b4c1 100644 --- a/packages/core/src/agents/agent-scheduler.ts +++ b/packages/core/src/agents/agent-scheduler.ts @@ -57,18 +57,8 @@ export async function scheduleAgentTools( } = options; // Create a proxy/override of the config to provide the agent-specific tool registry. - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const agentConfig: Config = Object.create(config); - agentConfig.getToolRegistry = () => toolRegistry; - agentConfig.getMessageBus = () => toolRegistry.messageBus; - // Override toolRegistry property so AgentLoopContext reads the agent-specific registry. - Object.defineProperty(agentConfig, 'toolRegistry', { - get: () => toolRegistry, - configurable: true, - }); - const schedulerContext = { - config: agentConfig, + config, promptId: config.promptId, toolRegistry, messageBus: toolRegistry.messageBus, diff --git a/packages/core/src/agents/local-executor.ts b/packages/core/src/agents/local-executor.ts index fccd95aed6..0ec7c80e9e 100644 --- a/packages/core/src/agents/local-executor.ts +++ b/packages/core/src/agents/local-executor.ts @@ -26,7 +26,6 @@ import { } from '../tools/mcp-tool.js'; import { CompressionStatus } from '../core/turn.js'; import { type ToolCallRequestInfo } from '../scheduler/types.js'; -import { type Message } from '../confirmation-bus/types.js'; import { ChatCompressionService } from '../services/chatCompressionService.js'; import { getDirectoryContextString } from '../utils/environmentContext.js'; import { promptIdContext } from '../utils/promptIdContext.js'; @@ -128,19 +127,7 @@ export class LocalAgentExecutor { const parentMessageBus = context.messageBus; // Create an override object to inject the subagent name into tool confirmation requests - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - const subagentMessageBus = Object.create( - parentMessageBus, - ) as typeof parentMessageBus; - subagentMessageBus.publish = async (message: Message) => { - if (message.type === 'tool-confirmation-request') { - return parentMessageBus.publish({ - ...message, - subagent: definition.name, - }); - } - return parentMessageBus.publish(message); - }; + const subagentMessageBus = parentMessageBus.derive(definition.name); // Create an isolated tool registry for this agent instance. const agentToolRegistry = new ToolRegistry( diff --git a/packages/core/src/agents/registry.ts b/packages/core/src/agents/registry.ts index 6eb642da72..23cf912055 100644 --- a/packages/core/src/agents/registry.ts +++ b/packages/core/src/agents/registry.ts @@ -520,23 +520,55 @@ export class AgentRegistry { return definition; } - // Use Object.create to preserve lazy getters on the definition object - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const merged: LocalAgentDefinition = Object.create(definition); - - if (overrides.runConfig) { - merged.runConfig = { - ...definition.runConfig, - ...overrides.runConfig, - }; - } - - if (overrides.modelConfig) { - merged.modelConfig = ModelConfigService.merge( - definition.modelConfig, - overrides.modelConfig, - ); - } + // Preserve lazy getters on the definition object by wrapping in a new object with getters + const merged: LocalAgentDefinition = { + get kind() { + return definition.kind; + }, + get name() { + return definition.name; + }, + get displayName() { + return definition.displayName; + }, + get description() { + return definition.description; + }, + get experimental() { + return definition.experimental; + }, + get metadata() { + return definition.metadata; + }, + get inputConfig() { + return definition.inputConfig; + }, + get outputConfig() { + return definition.outputConfig; + }, + get promptConfig() { + return definition.promptConfig; + }, + get toolConfig() { + return definition.toolConfig; + }, + get processOutput() { + return definition.processOutput; + }, + get runConfig() { + return overrides.runConfig + ? { ...definition.runConfig, ...overrides.runConfig } + : definition.runConfig; + }, + get modelConfig() { + return overrides.modelConfig + ? ModelConfigService.merge( + definition.modelConfig, + overrides.modelConfig, + ) + : definition.modelConfig; + }, + }; return merged; } diff --git a/packages/core/src/confirmation-bus/message-bus.test.ts b/packages/core/src/confirmation-bus/message-bus.test.ts index 34e36167a9..8f5c51d7d5 100644 --- a/packages/core/src/confirmation-bus/message-bus.test.ts +++ b/packages/core/src/confirmation-bus/message-bus.test.ts @@ -262,4 +262,90 @@ describe('MessageBus', () => { ); }); }); + + describe('derive', () => { + it('should receive responses from parent bus on derived bus', async () => { + vi.spyOn(policyEngine, 'check').mockResolvedValue({ + decision: PolicyDecision.ASK_USER, + }); + + const subagentName = 'test-subagent'; + const subagentBus = messageBus.derive(subagentName); + + const request: Omit = { + type: MessageBusType.TOOL_CONFIRMATION_REQUEST, + toolCall: { name: 'test-tool', args: {} }, + }; + + const requestPromise = subagentBus.request< + ToolConfirmationRequest, + ToolConfirmationResponse + >(request, MessageBusType.TOOL_CONFIRMATION_RESPONSE, 2000); + + // Wait for request on root bus and respond + await new Promise((resolve) => { + messageBus.subscribe( + MessageBusType.TOOL_CONFIRMATION_REQUEST, + (msg) => { + if (msg.subagent === subagentName) { + void messageBus.publish({ + type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, + correlationId: msg.correlationId, + confirmed: true, + }); + resolve(); + } + }, + ); + }); + + await expect(requestPromise).resolves.toEqual( + expect.objectContaining({ + type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, + confirmed: true, + }), + ); + }); + + it('should correctly chain subagent names for nested subagents', async () => { + vi.spyOn(policyEngine, 'check').mockResolvedValue({ + decision: PolicyDecision.ASK_USER, + }); + + const subagentBus1 = messageBus.derive('agent1'); + const subagentBus2 = subagentBus1.derive('agent2'); + + const request: Omit = { + type: MessageBusType.TOOL_CONFIRMATION_REQUEST, + toolCall: { name: 'test-tool', args: {} }, + }; + + const requestPromise = subagentBus2.request< + ToolConfirmationRequest, + ToolConfirmationResponse + >(request, MessageBusType.TOOL_CONFIRMATION_RESPONSE, 2000); + + await new Promise((resolve) => { + messageBus.subscribe( + MessageBusType.TOOL_CONFIRMATION_REQUEST, + (msg) => { + if (msg.subagent === 'agent1/agent2') { + void messageBus.publish({ + type: MessageBusType.TOOL_CONFIRMATION_RESPONSE, + correlationId: msg.correlationId, + confirmed: true, + }); + resolve(); + } + }, + ); + }); + + await expect(requestPromise).resolves.toEqual( + expect.objectContaining({ + confirmed: true, + }), + ); + }); + }); }); diff --git a/packages/core/src/confirmation-bus/message-bus.ts b/packages/core/src/confirmation-bus/message-bus.ts index 33aa10355b..5495996d25 100644 --- a/packages/core/src/confirmation-bus/message-bus.ts +++ b/packages/core/src/confirmation-bus/message-bus.ts @@ -40,6 +40,37 @@ export class MessageBus extends EventEmitter { this.emit(message.type, message); } + /** + * Derives a child message bus scoped to a specific subagent. + */ + derive(subagentName: string): MessageBus { + const bus = new MessageBus(this.policyEngine, this.debug); + + bus.publish = async (message: Message) => { + if (message.type === MessageBusType.TOOL_CONFIRMATION_REQUEST) { + return this.publish({ + ...message, + subagent: message.subagent + ? `${subagentName}/${message.subagent}` + : subagentName, + }); + } + return this.publish(message); + }; + + // Delegate subscription methods to the parent bus + bus.subscribe = this.subscribe.bind(this); + bus.unsubscribe = this.unsubscribe.bind(this); + bus.on = this.on.bind(this); + bus.off = this.off.bind(this); + bus.emit = this.emit.bind(this); + bus.once = this.once.bind(this); + bus.removeListener = this.removeListener.bind(this); + bus.listenerCount = this.listenerCount.bind(this); + + return bus; + } + async publish(message: Message): Promise { if (this.debug) { debugLogger.debug(`[MESSAGE_BUS] publish: ${safeJsonStringify(message)}`); diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index bc8e85462a..7e1faffb42 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -233,6 +233,15 @@ export class ToolRegistry { return this.messageBus; } + /** + * Creates a shallow clone of the registry and its current known tools. + */ + clone(): ToolRegistry { + const clone = new ToolRegistry(this.config, this.messageBus); + clone.allKnownTools = new Map(this.allKnownTools); + return clone; + } + /** * Registers a tool definition. * diff --git a/packages/core/src/utils/stdio.ts b/packages/core/src/utils/stdio.ts index 66abbe6ade..ca262b4784 100644 --- a/packages/core/src/utils/stdio.ts +++ b/packages/core/src/utils/stdio.ts @@ -77,43 +77,55 @@ export function patchStdio(): () => void { }; } +/** + * Type guard to check if a property key exists on an object. + */ +function isKey( + key: string | symbol | number, + obj: T, +): key is keyof T { + return key in obj; +} + /** * Creates proxies for process.stdout and process.stderr that use the real write methods * (writeToStdout and writeToStderr) bypassing any monkey patching. * This is used to write to the real output even when stdio is patched. */ export function createWorkingStdio() { - const inkStdout = new Proxy(process.stdout, { - get(target, prop, receiver) { + const stdoutHandler: ProxyHandler = { + get(target, prop) { if (prop === 'write') { return writeToStdout; } - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const value = Reflect.get(target, prop, receiver); - if (typeof value === 'function') { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return value.bind(target); + if (isKey(prop, target)) { + const value = target[prop]; + if (typeof value === 'function') { + return value.bind(target); + } + return value; } - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return value; + return undefined; }, - }); + }; + const inkStdout = new Proxy(process.stdout, stdoutHandler); - const inkStderr = new Proxy(process.stderr, { - get(target, prop, receiver) { + const stderrHandler: ProxyHandler = { + get(target, prop) { if (prop === 'write') { return writeToStderr; } - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const value = Reflect.get(target, prop, receiver); - if (typeof value === 'function') { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return value.bind(target); + if (isKey(prop, target)) { + const value = target[prop]; + if (typeof value === 'function') { + return value.bind(target); + } + return value; } - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return value; + return undefined; }, - }); + }; + const inkStderr = new Proxy(process.stderr, stderrHandler); return { stdout: inkStdout, stderr: inkStderr }; } diff --git a/packages/sdk/src/session.ts b/packages/sdk/src/session.ts index bc4a82320d..001d528817 100644 --- a/packages/sdk/src/session.ts +++ b/packages/sdk/src/session.ts @@ -243,10 +243,10 @@ export class GeminiCliSession { const loopContext: AgentLoopContext = this.config; const originalRegistry = loopContext.toolRegistry; - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const scopedRegistry: ToolRegistry = Object.create(originalRegistry); + const scopedRegistry: ToolRegistry = originalRegistry.clone(); + const originalGetTool = scopedRegistry.getTool.bind(scopedRegistry); scopedRegistry.getTool = (name: string) => { - const tool = originalRegistry.getTool(name); + const tool = originalGetTool(name); if (tool instanceof SdkTool) { return tool.bindContext(context); }