Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions packages/cli/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
FileDiscoveryService,
getAllGeminiMdFilenames,
loadServerHierarchicalMemory,
type LoadServerHierarchicalMemoryOptions,
type LoadServerHierarchicalMemoryResponse,
setGeminiMdFilename as setServerGeminiMdFilename,
resolveTelemetrySettings,
Expand Down Expand Up @@ -1130,6 +1131,7 @@ export async function loadHierarchicalGeminiMemory(
folderTrust: boolean,
memoryImportFormat: 'flat' | 'tree' = 'tree',
contextRuleExcludes: string[] = [],
options: LoadServerHierarchicalMemoryOptions = {},
): Promise<LoadServerHierarchicalMemoryResponse> {
// FIX: Use real, canonical paths for a reliable comparison to handle symlinks.
const realCwd = fs.realpathSync(path.resolve(currentWorkingDirectory));
Expand All @@ -1149,6 +1151,7 @@ export async function loadHierarchicalGeminiMemory(
folderTrust,
memoryImportFormat,
contextRuleExcludes,
options,
);
}

Expand Down
16 changes: 16 additions & 0 deletions packages/cli/src/ui/AppContainer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1307,6 +1307,22 @@ export const AppContainer = (props: AppContainerProps) => {
config.isTrustedFolder(),
settings.merged.context?.importFormat || 'tree', // Use setting or default to 'tree'
config.getContextRuleExcludes(),
{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The onInstructionsLoaded callback creation is duplicated verbatim across three call sites (here, directoryCommand.tsx:245, and the config.ts refresh path). Each constructs an identical createInstructionsLoadedCallback(() => config.getHookSystem()) with the same loadReason: 'refresh'.

Consider extracting a shared helper, e.g.:

Suggested change
{
createRefreshMemoryOptions(config),

where createRefreshMemoryOptions lives alongside createInstructionsLoadedCallback and returns the { loadReason, onInstructionsLoaded } object. This avoids drift if the callback shape changes and makes the three call sites trivially consistent.

— qwen3.7-max via Qwen Code /review

onInstructionsLoaded: async (notification) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The onInstructionsLoaded callback is duplicated verbatim across three call sites (here, directoryCommand.tsx:245, and config.ts:1869). Each destructures the same notification fields and forwards them identically to fireInstructionsLoadedEvent. If the callback shape changes, all three must be updated in lockstep.

Consider extracting a shared helper:

export function createInstructionsLoadedCallback(hookSystem: HookSystem | undefined) {
  return async (notification: InstructionsLoadedNotification) => {
    await hookSystem?.fireInstructionsLoadedEvent(
      notification.filePath,
      notification.memoryType,
      notification.loadReason,
      { globs: notification.globs, triggerFilePath: notification.triggerFilePath, parentFilePath: notification.parentFilePath },
    );
  };
}

— qwen3.7-max via Qwen Code /review

await config
.getHookSystem()
?.fireInstructionsLoadedEvent(
notification.filePath,
notification.memoryType,
notification.loadReason,
{
globs: notification.globs,
triggerFilePath: notification.triggerFilePath,
parentFilePath: notification.parentFilePath,
},
);
},
},
);

config.setUserMemory(memoryContent);
Expand Down
16 changes: 16 additions & 0 deletions packages/cli/src/ui/commands/directoryCommand.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,22 @@ export const directoryCommand: SlashCommand = {
context.services.settings.merged.context?.importFormat ||
'tree', // Use setting or default to 'tree'
config.getContextRuleExcludes(),
{
onInstructionsLoaded: async (notification) => {
await config
.getHookSystem()
?.fireInstructionsLoadedEvent(
notification.filePath,
notification.memoryType,
notification.loadReason,
{
globs: notification.globs,
triggerFilePath: notification.triggerFilePath,
parentFilePath: notification.parentFilePath,
},
);
},
},
);
config.setUserMemory(memoryContent);
config.setGeminiMdFileCount(fileCount);
Expand Down
32 changes: 30 additions & 2 deletions packages/cli/src/ui/components/hooks/constants.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ describe('hooks constants', () => {
expect(exitCodes).toHaveLength(3);
});

it('should return exit codes for InstructionsLoaded event', () => {
const exitCodes = getHookExitCodes(HookEventName.InstructionsLoaded);
expect(exitCodes).toHaveLength(2);
});

it('should return empty array for unknown event', () => {
const exitCodes = getHookExitCodes('unknown_event' as HookEventName);
expect(exitCodes).toEqual([]);
Expand Down Expand Up @@ -110,6 +115,11 @@ describe('hooks constants', () => {
expect(desc).toBe('When a new session is started');
});

it('should return description for InstructionsLoaded', () => {
const desc = getHookShortDescription(HookEventName.InstructionsLoaded);
expect(desc).toBe('When instruction files are loaded');
});

it('should return empty string for unknown event', () => {
const desc = getHookShortDescription('unknown_event' as HookEventName);
expect(desc).toBe('');
Expand All @@ -133,6 +143,13 @@ describe('hooks constants', () => {
expect(desc).toBe('');
});

it('should return description for InstructionsLoaded', () => {
const desc = getHookDescription(HookEventName.InstructionsLoaded);
expect(desc).toContain('file_path');
expect(desc).toContain('memory_type');
expect(desc).toContain('load_reason');
});

it('should return empty string for unknown event', () => {
const desc = getHookDescription('unknown_event' as HookEventName);
expect(desc).toBe('');
Expand Down Expand Up @@ -179,10 +196,11 @@ describe('hooks constants', () => {
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.PermissionDenied);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.TodoCreated);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.TodoCompleted);
expect(DISPLAY_HOOK_EVENTS).toContain(HookEventName.InstructionsLoaded);
});

it('should have 17 events', () => {
expect(DISPLAY_HOOK_EVENTS).toHaveLength(17);
it('should have 18 events', () => {
expect(DISPLAY_HOOK_EVENTS).toHaveLength(18);
});
});

Expand Down Expand Up @@ -242,5 +260,15 @@ describe('hooks constants', () => {
expect(info.exitCodes).toHaveLength(3);
expect(info.configs).toEqual([]);
});

it('should create empty info for InstructionsLoaded', () => {
const info = createEmptyHookEventInfo(HookEventName.InstructionsLoaded);

expect(info.event).toBe(HookEventName.InstructionsLoaded);
expect(info.shortDescription).toBe('When instruction files are loaded');
expect(info.description).toContain('file_path');
expect(info.exitCodes).toHaveLength(2);
expect(info.configs).toEqual([]);
});
});
});
8 changes: 8 additions & 0 deletions packages/cli/src/ui/components/hooks/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ export function getHookExitCodes(eventName: string): HookExitCode[] {
{ code: 0, description: t('stdout/stderr not shown') },
{ code: 'Other', description: t('show stderr to user only') },
],
[HookEventName.InstructionsLoaded]: [
{ code: 0, description: t('stdout/stderr not shown') },
{ code: 'Other', description: t('show stderr to user only') },
],
[HookEventName.UserPromptSubmit]: [
{ code: 0, description: t('stdout shown to Qwen') },
{
Expand Down Expand Up @@ -127,6 +131,7 @@ export function getHookShortDescription(eventName: string): string {
[HookEventName.PostToolUse]: t('After tool execution'),
[HookEventName.PostToolUseFailure]: t('After tool execution fails'),
[HookEventName.Notification]: t('When notifications are sent'),
[HookEventName.InstructionsLoaded]: t('When instruction files are loaded'),
[HookEventName.UserPromptSubmit]: t('When the user submits a prompt'),
[HookEventName.SessionStart]: t('When a new session is started'),
[HookEventName.Stop]: t('Right before Qwen Code concludes its response'),
Expand Down Expand Up @@ -168,6 +173,9 @@ export function getHookDescription(eventName: string): string {
[HookEventName.Notification]: t(
'Input to command is JSON with notification message and type.',
),
[HookEventName.InstructionsLoaded]: t(
'Input to command is JSON with file_path, memory_type, load_reason, and optional globs, trigger_file_path, and parent_file_path.',
),
[HookEventName.UserPromptSubmit]: t(
'Input to command is JSON with original user prompt text.',
),
Expand Down
49 changes: 47 additions & 2 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import { ToolNames } from '../tools/tool-names.js';
import { fireNotificationHook } from '../core/toolHookTriggers.js';
import type { MessageBus } from '../confirmation-bus/message-bus.js';
import { loadServerHierarchicalMemory } from '../utils/memoryDiscovery.js';
import type { LoadServerHierarchicalMemoryOptions } from '../utils/memoryDiscovery.js';
import { readAutoMemoryIndex } from '../memory/store.js';
import { ExtensionManager } from '../extension/extensionManager.js';
import { SkillManager } from '../skills/skill-manager.js';
Expand Down Expand Up @@ -1440,7 +1441,7 @@ describe('Server Config (config.ts)', () => {
await config.refreshHierarchicalMemory();

const lastCall = vi.mocked(loadServerHierarchicalMemory).mock.calls.at(-1);
expect(lastCall?.at(-1)).toEqual({ explicitOnly: true });
expect(lastCall?.at(-1)).toMatchObject({ explicitOnly: true });
expect(lastCall?.[1]).toEqual([]);
expect(readAutoMemoryIndex).not.toHaveBeenCalled();
expect(config.getUserMemory()).toContain('Project rules');
Expand Down Expand Up @@ -1468,7 +1469,51 @@ describe('Server Config (config.ts)', () => {

const lastCall = vi.mocked(loadServerHierarchicalMemory).mock.calls.at(-1);
expect(lastCall?.[1]).toEqual([explicitDir]);
expect(lastCall?.at(-1)).toEqual({ explicitOnly: true });
expect(lastCall?.at(-1)).toMatchObject({ explicitOnly: true });
});

it('refreshHierarchicalMemory should fire InstructionsLoaded hooks from memory notifications', async () => {
const config = new Config(baseParams);
const fireInstructionsLoadedEvent = vi.fn().mockResolvedValue(undefined);
config['hookSystem'] = {
fireInstructionsLoadedEvent,
} as unknown as HookSystem;

vi.mocked(loadServerHierarchicalMemory).mockResolvedValue({
memoryContent: '--- Context from: QWEN.md ---\nProject rules',
fileCount: 1,
ruleCount: 0,
conditionalRules: [],
projectRoot: '/tmp',
});

await config.refreshHierarchicalMemory();

const lastCall = vi.mocked(loadServerHierarchicalMemory).mock.calls.at(-1);
const options = lastCall?.at(-1) as
| LoadServerHierarchicalMemoryOptions
| undefined;
expect(options?.onInstructionsLoaded).toEqual(expect.any(Function));

await options?.onInstructionsLoaded?.({
filePath: '/tmp/project/QWEN.md',
memoryType: 'project',
loadReason: 'include',
globs: ['**/*.ts'],
triggerFilePath: '/tmp/project/AGENTS.md',
parentFilePath: '/tmp/project/AGENTS.md',
});

expect(fireInstructionsLoadedEvent).toHaveBeenCalledWith(
'/tmp/project/QWEN.md',
'project',
'include',
{
globs: ['**/*.ts'],
triggerFilePath: '/tmp/project/AGENTS.md',
parentFilePath: '/tmp/project/AGENTS.md',
},
);
});

it('Config constructor should call setGeminiMdFilename with contextFileName if provided', () => {
Expand Down
16 changes: 15 additions & 1 deletion packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1848,7 +1848,21 @@ export class Config {
this.isTrustedFolder(),
this.getImportFormat(),
this.contextRuleExcludes,
{ explicitOnly: this.getBareMode() },
{
explicitOnly: this.getBareMode(),
onInstructionsLoaded: async (notification) => {
await this.hookSystem?.fireInstructionsLoadedEvent(
notification.filePath,
notification.memoryType,
notification.loadReason,
{
globs: notification.globs,
triggerFilePath: notification.triggerFilePath,
parentFilePath: notification.parentFilePath,
},
);
},
},
);
if (this.getManagedAutoMemoryEnabled()) {
const managedAutoMemoryIndex = await readAutoMemoryIndex(
Expand Down
46 changes: 46 additions & 0 deletions packages/core/src/hooks/hookEventHandler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,52 @@ describe('HookEventHandler', () => {
});
});

describe('fireInstructionsLoadedEvent', () => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This test passes parentFilePath but never triggerFilePath. The camelCase→snake_case mapping (options.triggerFilePath → input.trigger_file_path at hookEventHandler.ts:144) has zero test coverage. Consider adding a second test case that passes triggerFilePath and asserts input.trigger_file_path:

it('should include trigger_file_path when provided', async () => {
  // ...same setup...
  await hookEventHandler.fireInstructionsLoadedEvent(
    '/repo/QWEN.md', 'project', 'include',
    { triggerFilePath: '/repo/AGENTS.md', parentFilePath: '/repo/QWEN.md' },
  );
  const input = mockCalls[0][2] as InstructionsLoadedInput;
  expect(input.trigger_file_path).toBe('/repo/AGENTS.md');
  expect(input.parent_file_path).toBe('/repo/QWEN.md');
});

— qwen3.7-max via Qwen Code /review

it('should include instruction load metadata in hook input', async () => {
const mockPlan = createMockExecutionPlan([
{
type: HookType.Command,
command: 'echo test',
source: HooksConfigSource.Project,
},
]);
vi.mocked(mockHookPlanner.createExecutionPlan).mockReturnValue(mockPlan);
vi.mocked(mockHookRunner.executeHooksParallel).mockResolvedValue([]);
vi.mocked(mockHookAggregator.aggregateResults).mockReturnValue(
createMockAggregatedResult(true),
);

await hookEventHandler.fireInstructionsLoadedEvent(
'/repo/.qwen/QWEN.local.md',
'local',
'include',
{
parentFilePath: '/repo/QWEN.md',
},
);

expect(mockHookPlanner.createExecutionPlan).toHaveBeenCalledWith(
HookEventName.InstructionsLoaded,
{
filePath: '/repo/.qwen/QWEN.local.md',
},
);

const mockCalls = (mockHookRunner.executeHooksParallel as Mock).mock
.calls;
const input = mockCalls[0][2] as {
file_path: string;
memory_type: string;
load_reason: string;
parent_file_path?: string;
};
expect(input.file_path).toBe('/repo/.qwen/QWEN.local.md');
expect(input.memory_type).toBe('local');
expect(input.load_reason).toBe('include');
expect(input.parent_file_path).toBe('/repo/QWEN.md');
});
});

describe('fireStopEvent', () => {
it('should execute hooks for Stop event', async () => {
const mockPlan = createMockExecutionPlan([]);
Expand Down
34 changes: 34 additions & 0 deletions packages/core/src/hooks/hookEventHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ import type {
TodoCompletedInput,
TodoItem,
TodoStatus,
InstructionsLoadedInput,
InstructionMemoryType,
InstructionLoadReason,
} from './types.js';
import { HookPhase, PermissionMode } from './types.js';
import { createDebugLogger } from '../utils/debugLogger.js';
Expand Down Expand Up @@ -116,6 +119,37 @@ export class HookEventHandler {
);
}

async fireInstructionsLoadedEvent(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] fireInstructionsLoadedEvent is the only fire*Event method in HookEventHandler without a JSDoc comment. Every other public fire method has a /** Fire a ... event */ doc block.

Suggested change
async fireInstructionsLoadedEvent(
/**
* Fire an InstructionsLoaded event
* Called when instruction/context files are loaded during session startup or import resolution
*/
async fireInstructionsLoadedEvent(

— qwen3.7-max via Qwen Code /review

filePath: string,
memoryType: InstructionMemoryType,
loadReason: InstructionLoadReason,
options: {
globs?: string[];
triggerFilePath?: string;
parentFilePath?: string;
} = {},
signal?: AbortSignal,
): Promise<AggregatedHookResult> {
const input: InstructionsLoadedInput = {
...this.createBaseInput(HookEventName.InstructionsLoaded),
file_path: filePath,
memory_type: memoryType,
load_reason: loadReason,
globs: options.globs,
trigger_file_path: options.triggerFilePath,
parent_file_path: options.parentFilePath,
};

return this.executeHooks(
HookEventName.InstructionsLoaded,
input,
{
filePath,
},
signal,
);
}

/**
* Fire a Stop event
* Called by handleHookExecutionRequest - executes hooks directly
Expand Down
Loading
Loading