Add InstructionsLoaded hook for instruction file loading#4665
Conversation
qqqys
added
roadmap/hooks-events
📋 Review SummaryThis PR introduces an 🔍 General FeedbackPositive aspects:
Architectural decisions:
Recurring themes:
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No high-confidence issues found. 7 low-confidence suggestions flagged for human review (callback duplication across 3 call sites, redundant triggerFilePath/parentFilePath fields, unused globs field, missing file path in error log, flat-format onFileImported untested, loadReason hardcoded to session_start, getMemoryType outside error-isolation wrapper). All deterministic checks (tsc, eslint) and 460 tests pass. — qwen3.7-max via Qwen Code /review
| logger.debug( | ||
| `Successfully read and processed imports: ${filePath} (Length: ${processedResult.content.length})`, | ||
| ); | ||
| await notifyInstructionsLoaded({ |
There was a problem hiding this comment.
[Suggestion] session_start notification fires AFTER processImports completes, which means hook consumers see child include events before the parent's session_start. For QWEN.md → included.md, hooks receive: included.md (include) → QWEN.md (session_start) — inverted from the natural causal order.
Additionally, if processImports throws unexpectedly, the parent's session_start notification is silently skipped while child include notifications have already fired, producing an orphaned notification stream.
| await notifyInstructionsLoaded({ | |
| await notifyInstructionsLoaded({ | |
| filePath, | |
| memoryType: getMemoryType(filePath), | |
| loadReason: 'session_start', | |
| }); | |
| const processedResult = await processImports( | |
| content, | |
| path.dirname(filePath), | |
| { | |
| processedFiles: new Set([path.resolve(filePath)]), | |
| maxDepth: 5, | |
| currentDepth: 0, | |
| currentFile: path.resolve(filePath), | |
| }, | |
| undefined, | |
| importFormat, | |
| { | |
| onFileImported: async (notification) => { | |
| await notifyInstructionsLoaded({ | |
| filePath: notification.filePath, | |
| memoryType: getMemoryType(notification.filePath), | |
| loadReason: 'include', | |
| triggerFilePath: notification.parentFilePath, | |
| parentFilePath: notification.parentFilePath, | |
| }); | |
| }, | |
| }, | |
| ); | |
| logger.debug( | |
| `Successfully read and processed imports: ${filePath} (Length: ${processedResult.content.length})`, | |
| ); |
— qwen3.7-max via Qwen Code /review
| filePath: notification.filePath, | ||
| memoryType: getMemoryType(notification.filePath), | ||
| loadReason: 'include', | ||
| triggerFilePath: notification.parentFilePath, |
There was a problem hiding this comment.
[Suggestion] triggerFilePath and parentFilePath are always set to the identical value (notification.parentFilePath). No code path ever sets them to different values, making triggerFilePath redundant. Hook consumers may incorrectly assume these carry distinct semantics.
Consider removing triggerFilePath from the notification type if it's semantically identical, or document the intended distinction and populate it differently (e.g., triggerFilePath = root file that initiated the import chain).
— qwen3.7-max via Qwen Code /review
| filePath: string; | ||
| memoryType: InstructionMemoryType; | ||
| loadReason: InstructionLoadReason; | ||
| globs?: string[]; |
There was a problem hiding this comment.
[Suggestion] The globs field is declared here and in InstructionsLoadedInput (hooks/types.ts:269), forwarded through all three call sites, but never populated with a value by any producer. Every notifyInstructionsLoaded call omits it, so hooks always receive globs: undefined.
Either remove this field until there's an actual producer, or add a comment marking it as reserved for future use to avoid confusing hook authors.
— qwen3.7-max via Qwen Code /review
| settings.merged.context?.importFormat || 'tree', // Use setting or default to 'tree' | ||
| config.getContextRuleExcludes(), | ||
| { | ||
| onInstructionsLoaded: async (notification) => { |
There was a problem hiding this comment.
[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
| try { | ||
| await fs.access(fullPath); | ||
| const fileContent = await fs.readFile(fullPath, 'utf-8'); | ||
| await options.onFileImported?.({ |
There was a problem hiding this comment.
[Suggestion] In tree mode, onFileImported fires after fs.readFile succeeds but BEFORE the recursive processImports call processes the imported content. If the recursive processing fails (circular import, I/O error, maxDepth exceeded), the hook consumer was told the file was "loaded" but its content may not appear in the final instruction set.
Consider moving the notification to after the recursive processImports call succeeds, so the hook only fires for files that are actually included in the final output.
— qwen3.7-max via Qwen Code /review
| ); | ||
| }); | ||
|
|
||
| it('notifies after importing a file', async () => { |
There was a problem hiding this comment.
[Suggestion] This test only exercises onFileImported with importFormat: 'tree'. The flat-format code path (memoryImportProcessor.ts:283) also calls onFileImported but with a different parentFilePath resolution (normalizedPath vs importState.currentFile ?? path.resolve(basePath)). Consider adding a parallel test with importFormat: 'flat' to cover both branches.
— qwen3.7-max via Qwen Code /review
| getHookSystem: () => HookSystem | undefined, | ||
| ) { | ||
| return async (notification: InstructionsLoadedNotification) => { | ||
| await getHookSystem()?.fireInstructionsLoadedEvent( |
There was a problem hiding this comment.
[Suggestion] fireInstructionsLoadedEvent returns an AggregatedHookResult that may contain continue: false or decision: 'block', but this callback discards the return value via await without checking. Users who configure hooks returning these fields will see no effect, creating a false sense of enforcement.
Consider either documenting that InstructionsLoaded is an informational-only event (and restricting the accepted output schema), or checking the result and acting on meaningful output fields.
— qwen3.7-max via Qwen Code /review
| : undefined; | ||
| } | ||
|
|
||
| async fireInstructionsLoadedEvent( |
There was a problem hiding this comment.
[Suggestion] No test for fireInstructionsLoadedEvent in hookSystem.test.ts. Every other fire*Event method (fireStopEvent, fireSessionStartEvent, fireUserPromptSubmitEvent, etc.) has a corresponding describe block verifying delegation to hookEventHandler and return-value mapping.
Add a test block following the existing pattern: mock hookEventHandler.fireInstructionsLoadedEvent, verify argument forwarding, and test both the result.finalOutput truthy path (returns DefaultHookOutput) and the falsy path (returns undefined).
— qwen3.7-max via Qwen Code /review
| import type { InstructionsLoadedNotification } from '../utils/memoryDiscovery.js'; | ||
| import type { HookSystem } from './hookSystem.js'; | ||
|
|
||
| export function createInstructionsLoadedCallback( |
There was a problem hiding this comment.
[Suggestion] No dedicated unit test for this new file. The only test reference is in config.test.ts where createInstructionsLoadedCallback is re-implemented by the mock rather than tested. The actual argument mapping (notification.triggerFilePath → triggerFilePath, etc.) is never exercised.
Consider adding instructionsLoadedCallback.test.ts covering: (1) correct argument forwarding to fireInstructionsLoadedEvent, (2) getHookSystem() returning undefined (optional chaining path), and (3) error propagation from fireInstructionsLoadedEvent.
— qwen3.7-max via Qwen Code /review
| ); | ||
| } | ||
|
|
||
| async fireInstructionsLoadedEvent( |
There was a problem hiding this comment.
[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.
| 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
| import type { InstructionsLoadedNotification } from '../utils/memoryDiscovery.js'; | ||
| import type { HookSystem } from './hookSystem.js'; | ||
|
|
||
| export function createInstructionsLoadedCallback( |
There was a problem hiding this comment.
[Suggestion] No explicit return type annotation. Other create* factory functions (createPromptHookRunner, createUrlValidator, createHookOutput) all declare explicit return types.
| export function createInstructionsLoadedCallback( | |
| export type InstructionsLoadedCallback = ( | |
| notification: InstructionsLoadedNotification, | |
| ) => Promise<void>; | |
| export function createInstructionsLoadedCallback( | |
| getHookSystem: () => HookSystem | undefined, | |
| ): InstructionsLoadedCallback { |
— qwen3.7-max via Qwen Code /review
| }; | ||
|
|
||
| case HookEventName.InstructionsLoaded: | ||
| return { kind: 'filePath', target: context?.filePath ?? '' }; |
There was a problem hiding this comment.
[Suggestion] The InstructionsLoaded matcher only exposes { kind: 'filePath', target: ... }. Users cannot configure hooks that selectively fire based on memory_type (e.g., only project files) or load_reason (e.g., only include events). The data IS available in the hook input JSON (memory_type, load_reason), but the matcher filtering layer cannot access it.
Consider adding memoryType and loadReason to HookEventContext and corresponding matcher kinds, following the existing pattern where events expose their distinguishing attributes (e.g., agentType for subagent events, notificationType for notification events).
— qwen3.7-max via Qwen Code /review
| await onInstructionsLoaded?.(notification); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| logger.warn(`InstructionsLoaded notification failed: ${message}`); |
There was a problem hiding this comment.
[Suggestion] The warning log captures error.message but omits notification.filePath. When a project has multiple instruction files (global, project, local, extension), a hook failure produces InstructionsLoaded notification failed: <message> with no way to identify which file triggered it.
| logger.warn(`InstructionsLoaded notification failed: ${message}`); | |
| logger.warn(`InstructionsLoaded notification failed for ${notification.filePath}: ${message}`); |
— qwen3.7-max via Qwen Code /review
Verification ReportEnvironment: macOS Darwin 25.4.0 (Apple Silicon), Node.js, tmux parallel execution Test Results
Code Review Observations
Build NoteThe core build failure is caused by a pre-existing dependency issue ( Verdict✅ Ready to merge (after rebase) — All runtime tests pass (561 total), lint clean, implementation follows established hook patterns. The build blocker is a pre-existing Verified by: wenshao |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
R4 (qwen3.7-max --comment, incremental since R3 at 9693ed6a3): APPROVED. Three small, focused commits land cleanly — 69ce8ac34 addresses the R3 suggestion about including filePath in the notification-failure warning, 372eb2cd3 removes a dead ternary branch in InputPrompt.tsx to align with the approval-visuals fix, and 604e741f5 bumps the display-event count to reflect the upstream PostToolBatch addition. Deterministic checks (tsc, eslint) clean; 235 core + 195 CLI tests pass. The remaining R3 suggestions (session_start ordering, triggerFilePath redundancy, unused globs field, callback dedup, onFileImported timing, JSDoc / return-type / matcher-scope items) are unchanged and still open for author triage — not re-posted here. — qwen3.7-max via Qwen Code /review
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Thanks for iterating on this — the latest commits cleaned up the event ordering, import de-duplication, the failure warning, and the test coverage, and the hook is genuinely observe-only and failure-isolated. Two metadata-accuracy gaps in the new contract are still open, though, and since InstructionsLoaded is brand-new public surface they're much cheaper to fix now than after hooks depend on them. Neither blocks functionality.
1. load_reason is always session_start, with no way to express a reload (severity: medium · confidence: high)
Every top-level instruction file is reported with load_reason: session_start, and the reason type only allows session_start or include — there's no refresh/reload value. But the same loading path runs on /memory refresh, adding a directory, and language/extension changes, so all of those re-fire as session_start. A hook that wants to act once at real session start will re-run on every refresh with no way to tell them apart. Worth adding a reason value and threading the actual trigger through from the call sites.
2. memory_type for imported files is derived from the imported file's own path (severity: medium · confidence: high)
When a file is pulled in via @import, its memory_type is computed from the imported file's location rather than inherited from the importer. So a user-level ~/QWEN.md that imports @./rules/foo.md reports that import as project rather than user, and the same happens for companions imported from a local QWEN.local.md. A hook that filters on memory_type — say, to treat user instructions differently from project ones — will mis-handle imports. The notification already carries the parent path, so the type can be inherited from it.
Minor
The InputPrompt.tsx prompt-width tweak on this branch looks unrelated to the hook and is a no-op (both branches of the removed ternary returned the same value) — probably a stray from the approval-visuals change and safe to drop from this PR.
Verdict
COMMENT — correct, safe, and observe-only; the two contract gaps are non-blocking but worth tightening before the InstructionsLoaded shape is something hooks rely on.
tanzhenxin
left a comment
tanzhenxin
left a comment
There was a problem hiding this comment.
Re-review — APPROVE
Thanks for the quick turnaround. c85b8e337 resolves both metadata-accuracy gaps from my earlier comment, verified end-to-end against the current head:
load_reasonnow distinguishes a real session start from reloads — the type carriesrefresh,initialize()tags the first loadsession_start, and/memory refresh,/directory add, language change, and extension change all flow through asrefresh. Covered by the newuses refresh load reason…test.- Import
memory_typenow inherits from the importer, so~/QWEN.mdimporting@./rules/foo.mdreportsuser. Covered by the newinherits memory type…test.
Two non-blocking nits, fine to leave or fold in later:
- The inheritance uses the immediate parent, so it still degrades at import depth ≥2 (
~/QWEN.md → ~/rules/foo.md → ~/rules/bar.mdreportsbar.mdasproject). The notification already carriestriggerFilePath(the chain root) — classifying by that would make it hold at any depth. - The
InputPrompt.tsxprompt-width tweak still looks unrelated to this PR and is a no-op (both branches of the removed ternary returned2) — probably a stray from the approval-visuals change.
Approving — observe-only, failure-isolated, CI green. Nice work.
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
| const parentFilePath = notification.parentFilePath; | ||
| await notifyInstructionsLoaded({ | ||
| filePath: notification.filePath, | ||
| memoryType: getMemoryType(filePath), |
There was a problem hiding this comment.
[Suggestion] The bare filePath in getMemoryType(filePath) refers to the outer closure variable (the root instruction file from batch.map(async (filePath) =>), not notification.filePath on the line above. While triggerFilePath: filePath on line 272 establishes the same closure reference, memoryType semantically describes the reported file — a future maintainer may misread this and "fix" it to getMemoryType(notification.filePath), silently changing behavior.
| memoryType: getMemoryType(filePath), | |
| // Inherit memoryType from the root instruction file, not the imported file. | |
| memoryType: getMemoryType(filePath), |
— qwen3.7-max via Qwen Code /review
| importFormat, | ||
| options, | ||
| ); | ||
| await options.onFileImported?.({ |
There was a problem hiding this comment.
[Suggestion] onFileImported is inside the try block (line 355), after processImports succeeds but before result += imported.content (line 378). If a direct caller of processImports provides a throwing onFileImported, the catch block discards the already-computed import content and replaces it with <!-- Import failed: ... -->. The current caller (readGeminiMdFiles) wraps the callback safely via notifyInstructionsLoaded, but processImports is a public export — any future direct consumer with a throwing callback would silently lose imported content.
| await options.onFileImported?.({ | |
| const imported = await processImports( | |
| fileContent, | |
| path.dirname(fullPath), | |
| newImportState, | |
| projectRoot, | |
| importFormat, | |
| options, | |
| ); | |
| result += `<!-- Imported from: ${importPath} -->\n${imported.content}\n<!-- End of import from: ${importPath} -->`; | |
| imports.push(imported.importTree); | |
| await options.onFileImported?.({ | |
| filePath: fullPath, | |
| parentFilePath: importState.currentFile ?? path.resolve(basePath), | |
| }); |
— qwen3.7-max via Qwen Code /review
| expect(result).not.toBeNull(); | ||
| }); | ||
|
|
||
| it('should match instruction loaded file paths with regex', () => { |
There was a problem hiding this comment.
[Suggestion] matchesFilePath (hookPlanner.ts:198-209) has a try/catch for invalid regex that falls back to exact match, but no test covers this branch. Other matchers with the same pattern (matchesToolName, matchesAgentType, matchesSessionTrigger) all have explicit invalid-regex fallback tests (lines 434, 452, 465, 827, 844). Consider adding:
it('should fallback to exact match for invalid regex on InstructionsLoaded', () => {
const entry: HookRegistryEntry = {
config: { type: HookType.Command, command: 'echo test' },
source: HooksConfigSource.Project,
eventName: HookEventName.InstructionsLoaded,
matcher: '[invalid(regex',
enabled: true,
};
vi.mocked(mockRegistry.getHooksForEvent).mockReturnValue([entry]);
const result = planner.createExecutionPlan(
HookEventName.InstructionsLoaded,
{ filePath: '/repo/QWEN.md' },
);
expect(result).toBeNull();
});— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('fireInstructionsLoadedEvent', () => { |
There was a problem hiding this comment.
[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
| newImportState, | ||
| projectRoot, | ||
| importFormat, | ||
| options, |
There was a problem hiding this comment.
[Suggestion] Tree mode clones processedFiles per branch (new Set(importState.processedFiles) at line 362), so diamond-shaped import graphs (A→B→D, A→C→D) process D twice and fire onFileImported twice — once with parentFilePath: B and once with parentFilePath: C. This causes duplicate InstructionsLoaded hook executions for the same physical file. Consider deduplicating notifications in the onFileImported wrapper within readGeminiMdFiles, or documenting the expected behavior for diamond imports.
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| if ( | ||
| resolvedPath.startsWith(`${globalQwenDir}${path.sep}`) || |
There was a problem hiding this comment.
[Suggestion] When the project root happens to be the user's home directory (e.g., cd ~ then run Qwen Code), path.dirname(resolvedPath) === resolvedHome matches project-level files like ~/QWEN.md, classifying them as 'user' instead of 'project'. The 'project' fallback is unreachable for any file directly in a home-as-project-root directory. Consider checking project-root membership before the user-level check:
if (resolvedRoot && isSubpath(resolvedRoot, resolvedPath)) {
return 'project';
}— qwen3.7-max via Qwen Code /review
| config.isTrustedFolder(), | ||
| settings.merged.context?.importFormat || 'tree', // Use setting or default to 'tree' | ||
| config.getContextRuleExcludes(), | ||
| { |
There was a problem hiding this comment.
[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.:
| { | |
| 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
3 participants
What this PR does
Adds an
InstructionsLoadedhook event that fires when instruction/context files are loaded during memory discovery and when files are loaded through@imports. The event payload includes the loaded file path, memory source type, load reason, and parent/trigger metadata for imports. Hook matchers can target the loaded file path, and the hooks management UI now describes the new event.Why it's needed
Automation and integrations need an observable point for instruction loading so they can audit or react to context sources without changing the existing discovery rules. This keeps the behavior event-only: no new discovery locations, no lazy rule changes, and no managed memory behavior changes.
Reviewer Test Plan
How to verify
Confirm a hook registered for
InstructionsLoadedreceivesfile_path,memory_type, andload_reasonfor startup-loaded instruction files and@imports. Confirm file-path matchers select only matching loaded paths. Confirm memory still loads if the notification callback fails.Evidence (Before & After)
N/A. This is a hook/event surface change covered by unit tests.
Tested on
Environment (optional)
Node.js local workspace. Verified with focused core and CLI hook test suites, full typecheck, full build, and lint.
Risk & Scope
Linked Issues
References #4664.
中文说明
What this PR does
新增
InstructionsLoadedhook event，在 memory discovery 加载 instruction/context 文件以及通过@import 加载文件时触发。事件 payload 包含被加载文件路径、memory 来源类型、加载原因，以及 import 的 parent/trigger 元数据。hook matcher 可以按被加载文件路径匹配，hooks 管理 UI 也补充了该事件说明。Why it's needed
自动化和集成需要一个可观测的 instruction 加载点，用来审计或响应上下文来源，同时不改变现有 discovery 规则。这个改动保持为纯事件能力：不新增 discovery 位置，不改变 lazy rule，也不改变 managed memory 行为。
Reviewer Test Plan
How to verify
确认注册到
InstructionsLoaded的 hook 能在启动加载 instruction 文件和@import 时收到file_path、memory_type、load_reason。确认 file-path matcher 只选择匹配的加载路径。确认通知回调失败时 memory 仍然正常加载。Evidence (Before & After)
N/A。这个改动是 hook/event surface，已通过单测覆盖。
Tested on
Environment (optional)
Node.js 本地工作区。已验证 core 和 CLI hook 相关测试、完整 typecheck、完整 build、lint。
Risk & Scope
Linked Issues
References #4664.