You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR adds hooks support for extensions, enabling extensions to define lifecycle hooks that can execute commands at specific events. The implementation includes:
ExtensionManager: Loads and processes hooks from qwen-extension.json or hooks/hooks.json, with variable substitution for ${CLAUDE_PLUGIN_ROOT} Claude Converter: Converts Claude plugin hooks to Qwen format, supporting both inline hooks and file-based hooks
Dive Deeper
Previously, extensions had no way to hook into lifecycle events. Claude plugins supported hooks but were not converted properly when imported.
Solution:
Hook Loading: Extensions can now define hooks in two ways:
Inline in qwen-extension.json under the hooks field
Via a separate hooks/hooks.json file (auto-discovered)
Variable Substitution: The ${CLAUDE_PLUGIN_ROOT} placeholder is automatically replaced with the actual extension path in:
Hook command definitions
All markdown files in the extension
Claude Plugin Conversion: The converter now properly handles:
Inline hooks objects in plugin.json
External hooks file paths (loaded and merged during conversion)
Variable substitution for ${CLAUDE_PLUGIN_ROOT} in hook commands
This PR implements a hooks extension mechanism that enables extensions to define lifecycle hooks that execute commands at specific events. The implementation includes hook loading from qwen-extension.json or hooks/hooks.json, variable substitution for ${CLAUDE_PLUGIN_ROOT}, and Claude plugin hook conversion. Overall, the code is well-structured and follows existing patterns, but there are several areas that need attention before merging.
🔍 General Feedback
The implementation follows existing code patterns and conventions well
Good use of TypeScript types with proper imports from hooks/types.js
Error handling is present but could be more robust in some areas
The variable substitution approach is consistent with how other extension resources are handled
Code duplication exists between the Claude converter and ExtensionManager for hook processing
🎯 Specific Feedback
🔴 Critical
File: packages/core/src/extension/extensionManager.ts:114 - The ExtensionConfig interface adds hooks property, but the base interface definition around line 100-115 needs to be checked for consistency. The diff shows hooks being added to both Extension and ExtensionConfig interfaces, but the actual file content shown doesn't reflect these changes yet. Ensure the type definitions are properly aligned.
File: packages/core/src/extension/claude-converter.ts:472-527 - The hooks file parsing logic has a potential security issue: when parsing external hooks files, there's no validation of the hook structure beyond basic type checking. Malicious hook configurations could execute arbitrary commands. Consider adding schema validation for hook definitions.
🟡 High
File: packages/core/src/extension/extensionManager.ts:668-710 - The substituteHookVariables function uses JSON.parse(JSON.stringify(hooks)) for deep cloning. This approach fails with undefined values and functions in objects. Consider using a proper deep clone utility or structuredClone() for better reliability.
File: packages/core/src/extension/extensionManager.ts:713-755 - The performVariableReplacement function modifies markdown files in-place during extension loading. This could cause issues with:
Read-only extensions
Extensions installed in protected directories
Multiple loads of the same extension (idempotency concern)
Consider making this a one-time conversion during installation rather than every load.
File: packages/core/src/extension/claude-converter.ts:316-329 - The hooks type conversion assumes that if claudeConfig.hooks is not a string, it's already in the correct format. This lacks validation and could lead to runtime errors if the Claude plugin uses a different hook structure. Add proper type guards and validation.
🟢 Medium
File: packages/core/src/extension/claude-converter.ts:487-492 - The hooks file parsing checks for both parsedHooks.hooks and direct hooks object. This logic could be extracted into a helper function for better reusability and testing, as the same pattern appears in extensionManager.ts.
File: packages/core/src/extension/extensionManager.ts - The hooks loading logic (lines 665-710) and the Claude converter hooks handling (lines 472-527) have significant code duplication. Consider extracting the common hook parsing and variable substitution logic into a shared utility module.
File: packages/core/src/extension/claude-converter.ts:316 - The comment "If it's a string, it's a file path, we handle it later" is informal. Consider more professional documentation-style comments.
🔵 Low
File: packages/core/src/extension/extensionManager.ts:101-114 - Consider adding JSDoc comments to the new hooks properties in the Extension and ExtensionConfig interfaces to explain the hook structure and purpose.
File: packages/core/src/extension/claude-converter.ts - The step numbering in comments jumps from "Step 6.1" to "Step 9.1" to "Step 10". Consider renumbering for clarity or removing step numbers if they're not maintained.
Testing Gap - The PR description mentions a testing matrix but all entries are marked with ❓ (unknown). Before merging, ensure proper test coverage is added for:
Hook loading from both inline config and external files
Variable substitution in hook commands
Claude plugin hook conversion
Error handling for malformed hook configurations
✅ Highlights
Good type safety: Proper use of TypeScript types with HookEventName and HookDefinition imports
Comprehensive variable substitution: The ${CLAUDE_PLUGIN_ROOT} substitution covers both hook commands and markdown files
Flexible hook loading: Support for both inline hooks in config and external hooks/hooks.json files
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
This PR adds hooks support for
extensions, enablingextensionsto define lifecyclehooksthat can execute commands at specific events. The implementation includes:ExtensionManager: Loads and processes hooks from qwen-extension.json or hooks/hooks.json, with variable substitution for ${CLAUDE_PLUGIN_ROOT}
Claude Converter: Converts Claude plugin hooks to Qwen format, supporting both inline hooks and file-based
hooksDive Deeper
Previously, extensions had no way to hook into lifecycle events. Claude plugins supported hooks but were not converted properly when imported.
Solution:
plugin.jsonReviewer Test Plan
Testing Matrix
Linked issues / bugs