feat(hooks): implement hooks extension mechanism#2352
Merged
DennisYu07 merged 4 commits intomainfrom Mar 18, 2026
Merged
Conversation
DennisYu07
requested review from
DragonnZhang,
LaZzyMan,
Mingholy,
gwinthis,
pomelo-nwu and
tanzhenxin
as code owners
Contributor
📋 Review SummaryThis 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 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
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. |
Collaborator
Author
ralph.lopp.mp4 |
This was referenced Mar 14, 2026
LaZzyMan
reviewed
Mar 17, 2026
Collaborator
LaZzyMan
left a comment
LaZzyMan
left a comment
There was a problem hiding this comment.
PR Review: feat(hooks): implement hooks extension mechanism
📋 Overview
This PR adds hooks support for extensions, enabling Claude plugin hooks conversion and supporting two hooks definition methods: inline in qwen-extension.json or via hooks/hooks.json file.
✅ What's Good
1. Feature Value
- 🔴 Necessary: The hooks extension mechanism fills an important gap in the extension system, allowing Claude plugins with hooks to be converted and used in Qwen Code.
- The
${CLAUDE_PLUGIN_ROOT}variable substitution is essential for cross-platform compatibility.
2. Implementation
- The code follows existing patterns in the codebase (similar to how MCP servers are handled).
- Good use of TypeScript types from
hooks/types.ts. - Proper error handling with
try-catchblocks anddebugLogger.warnfor non-critical failures.
🔴 Critical Issues
1. Code Duplication - substituteHookVariables Logic
The ${CLAUDE_PLUGIN_ROOT} substitution logic is duplicated in two places:
extensionManager.ts:substituteHookVariables()(lines 727-759)claude-converter.ts:convertClaudePluginPackage()(lines 508-530)
Recommendation: Extract this logic into a shared utility function.
// In a shared utility file
export function substituteHookVariables(
hooks: { [K in HookEventName]?: HookDefinition[] } | undefined,
basePath: string,
): { [K in HookEventName]?: HookDefinition[] } | undefined {
// ... shared implementation
}2. Missing Test Coverage
No tests were added for the new functionality. The following test cases should be covered:
- Loading hooks from
qwen-extension.json - Loading hooks from
hooks/hooks.json ${CLAUDE_PLUGIN_ROOT}variable substitution- Claude plugin hooks conversion
- Edge cases (invalid JSON, missing files, malformed hooks)
🟡 Suggestions
1. performVariableReplacement Side Effects
The performVariableReplacement method modifies files on disk during extension loading. This could cause issues:
- Files are modified in-place, which could affect source control if the extension is a linked extension
- No backup is created before modification
Recommendation: Consider whether this should be done during the conversion phase (for Claude plugins) rather than during every extension load.
2. Shell Script Transformation is Aggressive
The shell script transformations in performVariableReplacement are quite aggressive:
/"role":"assistant"/g → "type":"assistant"
/.claude/g → .qwenThese could potentially match unintended strings. For example:
.claudein a URL or comment would be replaced- The jq pattern replacement is very specific and might not cover all cases
Recommendation: Consider using more precise patterns or documenting the expected Claude plugin format more clearly.
3. shellProcessor.ts Change Scope
The addition of $ARGUMENTS placeholder support seems unrelated to the hooks feature:
.replaceAll('$ARGUMENTS', userArgsEscaped); // Replace $ARGUMENTSQuestion: Is this a Claude compatibility feature? If so, it should be documented in the commit message.
4. Hooks Loading Priority
The current logic:
if (config.hooks) {
extension.hooks = this.substituteHookVariables(config.hooks, ...);
}
if (!extension.hooks) {
// Load from hooks/hooks.json
}This means inline hooks take precedence over hooks/hooks.json. Is this the intended behavior? Consider documenting this priority.
🟢 Nice to Have
1. Type Safety for Hooks Parsing
The hooks parsing uses type assertions:
hooksData = parsedHooks.hooks as { [K in HookEventName]?: HookDefinition[] };Consider adding runtime validation using a schema validator (e.g., zod) or a custom validation function.
2. Documentation
Consider adding documentation for extension developers on how to define hooks in their extensions.
🔒 Security Considerations
No major security concerns, but note:
- Hooks execute shell commands with user permissions
- The
${CLAUDE_PLUGIN_ROOT}substitution could potentially be exploited if the path contains special characters (though this is mitigated by the extension installation process)
📊 Summary
| Category | Status |
|---|---|
| Build | ✅ Passes |
| Tests | |
| Code Style | ✅ Follows conventions |
| Documentation |
Recommendation: Request changes for:
- Extract duplicated
substituteHookVariableslogic - Add unit tests for the new functionality
The feature itself is valuable and the implementation is reasonable. With the above changes, this would be a solid addition to the extension system.
Collaborator
Code ReviewThanks for this PR! The hooks extension mechanism is a valuable addition that fills an important gap in the extension system. Here's my detailed review: ✅ What's Good
🔴 Critical Issues (Must Fix)1. Code DuplicationThe
Recommendation: Extract this into a shared utility function: // In a shared utility file
export function substituteHookVariables(
hooks: { [K in HookEventName]?: HookDefinition[] } | undefined,
basePath: string,
): { [K in HookEventName]?: HookDefinition[] } | undefined {
// ... shared implementation
}2. Missing Test CoverageNo unit tests were added for the new functionality. Should cover:
🟡 Suggestions1. File Modification Side Effects
fs.writeFileSync(filePath, updatedMdContent, 'utf8');Concerns:
Recommendation: Only execute during the conversion phase (for Claude plugins), or add a config option to control this behavior. 2. Aggressive Regex Patterns/.claude/g → .qwenThis could match unintended strings like:
Recommendation: Use more precise patterns, e.g., only match path references. 3. Scope Creep in
|
| Dimension | Rating | Notes |
|---|---|---|
| Feature Necessity | ✅ High | Fills important gap |
| Implementation | ✅ Sound | Architecture is correct |
| Code Quality | 🟡 Medium | Code duplication, missing tests |
| Security | ✅ Good | No major concerns |
| UX Impact | ✅ Positive | Enhances extensibility |
🎯 Recommendation
Request Changes - The feature is solid but needs:
- Deduplicated code (extract shared utility)
- Unit test coverage
Once these are addressed, this will be a great enhancement!
cc @DennisYu07
Collaborator
Author
|
Critical issue comment resolved, and suggestion 1, 2 resolved |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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