-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add InstructionsLoaded hook for instruction file loading #4665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 13 commits
c954b47
0769aeb
3fe3b2a
12f874d
2479f7e
743f306
8ddb096
2373477
9693ed6
69ce8ac
c8bbc14
372eb2c
604e741
c85b8e3
34b0d50
73ae1ab
a52a6b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,6 +168,52 @@ describe('HookEventHandler', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('fireInstructionsLoadedEvent', () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This test passes 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([]); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] The
onInstructionsLoadedcallback creation is duplicated verbatim across three call sites (here,directoryCommand.tsx:245, and theconfig.tsrefresh path). Each constructs an identicalcreateInstructionsLoadedCallback(() => config.getHookSystem())with the sameloadReason: 'refresh'.Consider extracting a shared helper, e.g.:
where
createRefreshMemoryOptionslives alongsidecreateInstructionsLoadedCallbackand 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