mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-05-12 21:03:05 -07:00
fix(core): suppress duplicate hook failure warnings during streaming (#17727)
This commit is contained in:
@@ -4,6 +4,10 @@
|
|||||||
* SPDX-License-Identifier: Apache-2.0
|
* SPDX-License-Identifier: Apache-2.0
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
import type {
|
||||||
|
GenerateContentParameters,
|
||||||
|
GenerateContentResponse,
|
||||||
|
} from '@google/genai';
|
||||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||||
import { HookEventHandler } from './hookEventHandler.js';
|
import { HookEventHandler } from './hookEventHandler.js';
|
||||||
import type { Config } from '../config/config.js';
|
import type { Config } from '../config/config.js';
|
||||||
@@ -776,6 +780,68 @@ describe('HookEventHandler', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('failure suppression', () => {
|
||||||
|
it('should suppress duplicate feedback for the same failing hook and request context', async () => {
|
||||||
|
const mockHook: HookConfig = {
|
||||||
|
type: HookType.Command,
|
||||||
|
command: './fail.sh',
|
||||||
|
name: 'failing-hook',
|
||||||
|
};
|
||||||
|
const mockResults: HookExecutionResult[] = [
|
||||||
|
{
|
||||||
|
success: false,
|
||||||
|
duration: 10,
|
||||||
|
hookConfig: mockHook,
|
||||||
|
eventName: HookEventName.AfterModel,
|
||||||
|
error: new Error('Failed'),
|
||||||
|
},
|
||||||
|
];
|
||||||
|
const mockAggregated = {
|
||||||
|
success: false,
|
||||||
|
allOutputs: [],
|
||||||
|
errors: [new Error('Failed')],
|
||||||
|
totalDuration: 10,
|
||||||
|
};
|
||||||
|
|
||||||
|
vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue({
|
||||||
|
eventName: HookEventName.AfterModel,
|
||||||
|
hookConfigs: [mockHook],
|
||||||
|
sequential: false,
|
||||||
|
});
|
||||||
|
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue(
|
||||||
|
mockResults,
|
||||||
|
);
|
||||||
|
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
|
||||||
|
mockAggregated,
|
||||||
|
);
|
||||||
|
|
||||||
|
const llmRequest = { model: 'test', contents: [] };
|
||||||
|
const llmResponse = { candidates: [] };
|
||||||
|
|
||||||
|
// First call - should emit feedback
|
||||||
|
await hookEventHandler.fireAfterModelEvent(
|
||||||
|
llmRequest as unknown as GenerateContentParameters,
|
||||||
|
llmResponse as unknown as GenerateContentResponse,
|
||||||
|
);
|
||||||
|
expect(mockCoreEvents.emitFeedback).toHaveBeenCalledTimes(1);
|
||||||
|
|
||||||
|
// Second call with SAME request - should NOT emit feedback
|
||||||
|
await hookEventHandler.fireAfterModelEvent(
|
||||||
|
llmRequest as unknown as GenerateContentParameters,
|
||||||
|
llmResponse as unknown as GenerateContentResponse,
|
||||||
|
);
|
||||||
|
expect(mockCoreEvents.emitFeedback).toHaveBeenCalledTimes(1);
|
||||||
|
|
||||||
|
// Third call with DIFFERENT request - should emit feedback again
|
||||||
|
const differentRequest = { model: 'different', contents: [] };
|
||||||
|
await hookEventHandler.fireAfterModelEvent(
|
||||||
|
differentRequest as unknown as GenerateContentParameters,
|
||||||
|
llmResponse as unknown as GenerateContentResponse,
|
||||||
|
);
|
||||||
|
expect(mockCoreEvents.emitFeedback).toHaveBeenCalledTimes(2);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('createBaseInput', () => {
|
describe('createBaseInput', () => {
|
||||||
it('should create base input with correct fields', async () => {
|
it('should create base input with correct fields', async () => {
|
||||||
const mockPlan = [
|
const mockPlan = [
|
||||||
|
|||||||
@@ -49,6 +49,13 @@ export class HookEventHandler {
|
|||||||
private readonly hookRunner: HookRunner;
|
private readonly hookRunner: HookRunner;
|
||||||
private readonly hookAggregator: HookAggregator;
|
private readonly hookAggregator: HookAggregator;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Track reported failures to suppress duplicate warnings during streaming.
|
||||||
|
* Uses a WeakMap with the original request object as a key to ensure
|
||||||
|
* failures are only reported once per logical model interaction.
|
||||||
|
*/
|
||||||
|
private readonly reportedFailures = new WeakMap<object, Set<string>>();
|
||||||
|
|
||||||
constructor(
|
constructor(
|
||||||
config: Config,
|
config: Config,
|
||||||
hookPlanner: HookPlanner,
|
hookPlanner: HookPlanner,
|
||||||
@@ -210,7 +217,12 @@ export class HookEventHandler {
|
|||||||
llm_request: defaultHookTranslator.toHookLLMRequest(llmRequest),
|
llm_request: defaultHookTranslator.toHookLLMRequest(llmRequest),
|
||||||
};
|
};
|
||||||
|
|
||||||
return this.executeHooks(HookEventName.BeforeModel, input);
|
return this.executeHooks(
|
||||||
|
HookEventName.BeforeModel,
|
||||||
|
input,
|
||||||
|
undefined,
|
||||||
|
llmRequest,
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -227,7 +239,12 @@ export class HookEventHandler {
|
|||||||
llm_response: defaultHookTranslator.toHookLLMResponse(llmResponse),
|
llm_response: defaultHookTranslator.toHookLLMResponse(llmResponse),
|
||||||
};
|
};
|
||||||
|
|
||||||
return this.executeHooks(HookEventName.AfterModel, input);
|
return this.executeHooks(
|
||||||
|
HookEventName.AfterModel,
|
||||||
|
input,
|
||||||
|
undefined,
|
||||||
|
llmRequest,
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -242,7 +259,12 @@ export class HookEventHandler {
|
|||||||
llm_request: defaultHookTranslator.toHookLLMRequest(llmRequest),
|
llm_request: defaultHookTranslator.toHookLLMRequest(llmRequest),
|
||||||
};
|
};
|
||||||
|
|
||||||
return this.executeHooks(HookEventName.BeforeToolSelection, input);
|
return this.executeHooks(
|
||||||
|
HookEventName.BeforeToolSelection,
|
||||||
|
input,
|
||||||
|
undefined,
|
||||||
|
llmRequest,
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -253,6 +275,7 @@ export class HookEventHandler {
|
|||||||
eventName: HookEventName,
|
eventName: HookEventName,
|
||||||
input: HookInput,
|
input: HookInput,
|
||||||
context?: HookEventContext,
|
context?: HookEventContext,
|
||||||
|
requestContext?: object,
|
||||||
): Promise<AggregatedHookResult> {
|
): Promise<AggregatedHookResult> {
|
||||||
try {
|
try {
|
||||||
// Create execution plan
|
// Create execution plan
|
||||||
@@ -311,7 +334,13 @@ export class HookEventHandler {
|
|||||||
this.processCommonHookOutputFields(aggregated);
|
this.processCommonHookOutputFields(aggregated);
|
||||||
|
|
||||||
// Log hook execution
|
// Log hook execution
|
||||||
this.logHookExecution(eventName, input, results, aggregated);
|
this.logHookExecution(
|
||||||
|
eventName,
|
||||||
|
input,
|
||||||
|
results,
|
||||||
|
aggregated,
|
||||||
|
requestContext,
|
||||||
|
);
|
||||||
|
|
||||||
return aggregated;
|
return aggregated;
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
@@ -354,6 +383,7 @@ export class HookEventHandler {
|
|||||||
input: HookInput,
|
input: HookInput,
|
||||||
results: HookExecutionResult[],
|
results: HookExecutionResult[],
|
||||||
aggregated: AggregatedHookResult,
|
aggregated: AggregatedHookResult,
|
||||||
|
requestContext?: object,
|
||||||
): void {
|
): void {
|
||||||
const failedHooks = results.filter((r) => !r.success);
|
const failedHooks = results.filter((r) => !r.success);
|
||||||
const successCount = results.length - failedHooks.length;
|
const successCount = results.length - failedHooks.length;
|
||||||
@@ -364,15 +394,33 @@ export class HookEventHandler {
|
|||||||
.map((r) => this.getHookNameFromResult(r))
|
.map((r) => this.getHookNameFromResult(r))
|
||||||
.join(', ');
|
.join(', ');
|
||||||
|
|
||||||
|
let shouldEmit = true;
|
||||||
|
if (requestContext) {
|
||||||
|
let reportedSet = this.reportedFailures.get(requestContext);
|
||||||
|
if (!reportedSet) {
|
||||||
|
reportedSet = new Set<string>();
|
||||||
|
this.reportedFailures.set(requestContext, reportedSet);
|
||||||
|
}
|
||||||
|
|
||||||
|
const failureKey = `${eventName}:${failedNames}`;
|
||||||
|
if (reportedSet.has(failureKey)) {
|
||||||
|
shouldEmit = false;
|
||||||
|
} else {
|
||||||
|
reportedSet.add(failureKey);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
debugLogger.warn(
|
debugLogger.warn(
|
||||||
`Hook execution for ${eventName}: ${successCount} succeeded, ${errorCount} failed (${failedNames}), ` +
|
`Hook execution for ${eventName}: ${successCount} succeeded, ${errorCount} failed (${failedNames}), ` +
|
||||||
`total duration: ${aggregated.totalDuration}ms`,
|
`total duration: ${aggregated.totalDuration}ms`,
|
||||||
);
|
);
|
||||||
|
|
||||||
|
if (shouldEmit) {
|
||||||
coreEvents.emitFeedback(
|
coreEvents.emitFeedback(
|
||||||
'warning',
|
'warning',
|
||||||
`Hook(s) [${failedNames}] failed for event ${eventName}. Press F12 to see the debug drawer for more details.\n`,
|
`Hook(s) [${failedNames}] failed for event ${eventName}. Press F12 to see the debug drawer for more details.\n`,
|
||||||
);
|
);
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
debugLogger.debug(
|
debugLogger.debug(
|
||||||
`Hook execution for ${eventName}: ${successCount} hooks executed successfully, ` +
|
`Hook execution for ${eventName}: ${successCount} hooks executed successfully, ` +
|
||||||
|
|||||||
Reference in New Issue
Block a user