reslove comment

packages/cli/src/services/prompt-processors/shellProcessor.ts

@@ -12,6 +12,7 @@ import {
   getShellConfiguration,
   ShellExecutionService,
   flatMapTextParts,
+  checkArgumentSafety,
 } from '@qwen-code/qwen-code-core';
 import type { AnyToolInvocation } from '@qwen-code/qwen-code-core';
 
@@ -101,6 +102,16 @@ export class ShellProcessor implements IPromptProcessor {
     const { shell } = getShellConfiguration();
     const userArgsEscaped = escapeShellArg(userArgsRaw, shell);
 
+    // Check safety of the value that will be used for $ARGUMENTS (after removing outer quotes)
+    let userArgsForArgumentsPlaceholder = userArgsRaw.replace(
+      /^'([\s\S]*?)'$/,
+      '$1',
+    );
+    const argumentSafety = checkArgumentSafety(userArgsForArgumentsPlaceholder);
+    if (!argumentSafety.isSafe) {
+      userArgsForArgumentsPlaceholder = userArgsEscaped;
+    }
+
     const resolvedInjections: ResolvedShellInjection[] = injections.map(
       (injection) => {
         const command = injection.content;
@@ -111,10 +122,7 @@ export class ShellProcessor implements IPromptProcessor {
 
         const resolvedCommand = command
           .replaceAll(SHORTHAND_ARGS_PLACEHOLDER, userArgsEscaped) // Replace {{args}}
-          .replaceAll(
-            '$ARGUMENTS',
-            userArgsRaw.replace(/^'([\s\S]*?)'$/, '$1'),
-          );
+          .replaceAll('$ARGUMENTS', userArgsForArgumentsPlaceholder);
         return { ...injection, resolvedCommand };
       },
     );

packages/core/src/extension/claude-converter.test.ts

@@ -511,66 +511,6 @@ describe('convertClaudePluginPackage', () => {
     // Clean up converted directory
     fs.rmSync(result.convertedDir, { recursive: true, force: true });
   });
-
-  it('should handle hooks defined directly in marketplace config', async () => {
-    // Setup: Create a plugin with hooks defined directly in marketplace config
-    const pluginSourceDir = path.join(testDir, 'direct-hooks-plugin');
-    fs.mkdirSync(pluginSourceDir, { recursive: true });
-
-    // Create marketplace.json with hooks defined directly
-    const marketplaceDir = path.join(pluginSourceDir, '.claude-plugin');
-    fs.mkdirSync(marketplaceDir, { recursive: true });
-
-    const marketplaceConfig: ClaudeMarketplaceConfig = {
-      name: 'test-marketplace',
-      owner: { name: 'Test Owner', email: 'test@example.com' },
-      plugins: [
-        {
-          name: 'direct-hooks-plugin',
-          version: '1.0.0',
-          source: './',
-          strict: false,
-          hooks: {
-            PreToolUse: [
-              {
-                matcher: '*', // Part of HookDefinition
-                sequential: true, // Part of HookDefinition
-                hooks: [
-                  // HookConfig[] array inside HookDefinition
-                  {
-                    type: HookType.Command,
-                    command: 'npm install',
-                  },
-                ],
-              },
-            ],
-          },
-        },
-      ],
-    };
-
-    fs.writeFileSync(
-      path.join(marketplaceDir, 'marketplace.json'),
-      JSON.stringify(marketplaceConfig, null, 2),
-      'utf-8',
-    );
-
-    // Execute: Convert the plugin
-    const result = await convertClaudePluginPackage(
-      pluginSourceDir,
-      'direct-hooks-plugin',
-    );
-
-    // Verify: The converted config should contain the hooks
-    expect(result.config.hooks).toBeDefined();
-    expect(result.config.hooks!['PreToolUse']).toHaveLength(1);
-    expect(result.config.hooks!['PreToolUse']![0].hooks![0].command).toBe(
-      'npm install',
-    );
-
-    // Clean up converted directory
-    fs.rmSync(result.convertedDir, { recursive: true, force: true });
-  });
 });
 
 describe('performVariableReplacement for Claude extensions', () => {
@@ -586,21 +526,6 @@ describe('performVariableReplacement for Claude extensions', () => {
     }
   });
 
-  it('should replace ${CLAUDE_PLUGIN_ROOT} in markdown files', () => {
-    const extDir = path.join(testDir, 'ext-md');
-    fs.mkdirSync(extDir, { recursive: true });
-
-    const mdContent = `# Test Extension
-      Run \`\${CLAUDE_PLUGIN_ROOT}/scripts/setup.sh\` to configure.`;
-    fs.writeFileSync(path.join(extDir, 'README.md'), mdContent, 'utf-8');
-
-    performVariableReplacement(extDir);
-
-    const result = fs.readFileSync(path.join(extDir, 'README.md'), 'utf-8');
-    expect(result).toContain(`${extDir}/scripts/setup.sh`);
-    expect(result).not.toContain('${CLAUDE_PLUGIN_ROOT}');
-  });
-
   it('should replace .claude with .qwen in shell scripts', () => {
     const extDir = path.join(testDir, 'ext-sh');
     fs.mkdirSync(extDir, { recursive: true });

packages/core/src/extension/extensionManager.ts

@@ -989,7 +989,8 @@ export class ExtensionManager {
         }
 
         // Perform variable replacement in extension files (e.g., ${CLAUDE_PLUGIN_ROOT}) for Claude extensions
-        if (originSource === 'Claude') {
+        const hooksDir = path.join(destinationPath, 'hooks');
+        if (originSource === 'Claude' || fs.existsSync(hooksDir)) {
           performVariableReplacement(destinationPath);
         }
 

packages/core/src/extension/variables.test.ts

@@ -406,4 +406,53 @@ describe('performVariableReplacement', () => {
       'content',
     );
   });
+
+  describe('regex boundary cases', () => {
+    it('should not replace incomplete variable syntax (missing brace) in markdown', () => {
+      const extDir = path.join(testDir, 'ext-incomplete');
+      fs.mkdirSync(extDir, { recursive: true });
+
+      // Note: performVariableReplacement only processes .md files
+      const content = 'Path: $CLAUDE_PLUGIN_ROOT/config.json';
+      fs.writeFileSync(path.join(extDir, 'test.md'), content, 'utf-8');
+
+      performVariableReplacement(extDir);
+
+      const result = fs.readFileSync(path.join(extDir, 'test.md'), 'utf-8');
+      // Should remain unchanged (no braces)
+      expect(result).toBe('Path: $CLAUDE_PLUGIN_ROOT/config.json');
+    });
+
+    it('should replace double dollar sign but keep first dollar', () => {
+      const extDir = path.join(testDir, 'ext-double-dollar');
+      fs.mkdirSync(extDir, { recursive: true });
+
+      // Note: performVariableReplacement only processes .md files
+      // The regex matches ${CLAUDE_PLUGIN_ROOT}, leaving first $ intact
+      const content = 'Path: $${CLAUDE_PLUGIN_ROOT}/config.json';
+      fs.writeFileSync(path.join(extDir, 'test.md'), content, 'utf-8');
+
+      performVariableReplacement(extDir);
+
+      const result = fs.readFileSync(path.join(extDir, 'test.md'), 'utf-8');
+      // First $ is preserved, variable is replaced
+      expect(result).toBe(`Path: $${extDir}/config.json`);
+    });
+
+    it('should replace variable in markdown comments', () => {
+      const extDir = path.join(testDir, 'ext-comment');
+      fs.mkdirSync(extDir, { recursive: true });
+
+      // Comments in markdown files should be processed
+      const content = '# TODO: Update ${CLAUDE_PLUGIN_ROOT} later';
+      fs.writeFileSync(path.join(extDir, 'test.md'), content, 'utf-8');
+
+      performVariableReplacement(extDir);
+
+      const result = fs.readFileSync(path.join(extDir, 'test.md'), 'utf-8');
+      // Should be replaced (comments in markdown are still processed)
+      expect(result).toContain(extDir);
+      expect(result).not.toContain('${CLAUDE_PLUGIN_ROOT}');
+    });
+  });
 });

packages/core/src/utils/shell-utils.test.ts

@@ -6,6 +6,7 @@
 
 import { expect, describe, it, beforeEach, vi, afterEach } from 'vitest';
 import {
+  checkArgumentSafety,
   checkCommandPermissions,
   escapeShellArg,
   getCommandRoots,
@@ -607,3 +608,136 @@ describe('isCommandNeedPermission', () => {
     expect(result.reason).toContain('requires permission to execute');
   });
 });
+
+describe('checkArgumentSafety', () => {
+  describe('command substitution patterns', () => {
+    it('should detect $() command substitution', () => {
+      const result = checkArgumentSafety('$(whoami)');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('$() command substitution');
+    });
+
+    it('should detect backtick command substitution', () => {
+      const result = checkArgumentSafety('`whoami`');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain(
+        'backtick command substitution',
+      );
+    });
+
+    it('should detect <() process substitution', () => {
+      const result = checkArgumentSafety('<(cat file)');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('<() process substitution');
+    });
+
+    it('should detect >() process substitution', () => {
+      const result = checkArgumentSafety('>(tee file)');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('>() process substitution');
+    });
+  });
+
+  describe('command separators', () => {
+    it('should detect semicolon separator', () => {
+      const result = checkArgumentSafety('arg1; rm -rf /');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('; command separator');
+    });
+
+    it('should detect pipe', () => {
+      const result = checkArgumentSafety('arg1 | cat file');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('| pipe');
+    });
+
+    it('should detect && operator', () => {
+      const result = checkArgumentSafety('arg1 && ls');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('&& AND operator');
+    });
+
+    it('should detect || operator', () => {
+      const result = checkArgumentSafety('arg1 || ls');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('|| OR operator');
+    });
+  });
+
+  describe('background execution', () => {
+    it('should detect background operator', () => {
+      const result = checkArgumentSafety('arg1 & ls');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('& background operator');
+    });
+  });
+
+  describe('input/output redirection', () => {
+    it('should detect output redirection', () => {
+      const result = checkArgumentSafety('arg1 > file');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('> output redirection');
+    });
+
+    it('should detect input redirection', () => {
+      const result = checkArgumentSafety('arg1 < file');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('< input redirection');
+    });
+
+    it('should detect append redirection', () => {
+      const result = checkArgumentSafety('arg1 >> file');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('> output redirection');
+    });
+  });
+
+  describe('safe inputs', () => {
+    it('should accept simple arguments', () => {
+      const result = checkArgumentSafety('arg1 arg2');
+      expect(result.isSafe).toBe(true);
+      expect(result.dangerousPatterns).toHaveLength(0);
+    });
+
+    it('should accept arguments with numbers', () => {
+      const result = checkArgumentSafety('file123.txt');
+      expect(result.isSafe).toBe(true);
+    });
+
+    it('should accept arguments with hyphens', () => {
+      const result = checkArgumentSafety('--flag=value');
+      expect(result.isSafe).toBe(true);
+    });
+
+    it('should accept arguments with underscores', () => {
+      const result = checkArgumentSafety('my_file_name');
+      expect(result.isSafe).toBe(true);
+    });
+
+    it('should accept arguments with dots', () => {
+      const result = checkArgumentSafety('path/to/file.txt');
+      expect(result.isSafe).toBe(true);
+    });
+
+    it('should accept empty string', () => {
+      const result = checkArgumentSafety('');
+      expect(result.isSafe).toBe(true);
+    });
+
+    it('should accept arguments with spaces (quoted)', () => {
+      const result = checkArgumentSafety('hello world');
+      expect(result.isSafe).toBe(true);
+    });
+  });
+
+  describe('multiple dangerous patterns', () => {
+    it('should detect multiple dangerous patterns', () => {
+      const result = checkArgumentSafety('$(whoami); rm -rf / &');
+      expect(result.isSafe).toBe(false);
+      expect(result.dangerousPatterns).toContain('$() command substitution');
+      expect(result.dangerousPatterns).toContain('; command separator');
+      expect(result.dangerousPatterns).toContain('& background operator');
+      expect(result.dangerousPatterns).toHaveLength(3);
+    });
+  });
+});

packages/core/src/utils/shell-utils.ts

@@ -913,3 +913,45 @@ export function isCommandNeedsPermission(command: string): {
     reason: 'Command requires permission to execute.',
   };
 }
+
+/**
+ * Checks user arguments for potentially dangerous shell characters.
+ * This is used to validate arguments before they are substituted into
+ * shell command templates (e.g., $ARGUMENTS placeholder).
+ *
+ * Note: This does NOT remove outer quotes - it validates the raw input.
+ * Use escapeShellArg() for safe shell argument escaping.
+ *
+ * @param args - The raw user arguments string
+ * @returns Object with isSafe flag and list of dangerous patterns found
+ */
+export function checkArgumentSafety(args: string): {
+  isSafe: boolean;
+  dangerousPatterns: string[];
+} {
+  const dangerousPatterns: string[] = [];
+
+  // Command substitution patterns
+  if (/\$\(/.test(args)) dangerousPatterns.push('$() command substitution');
+  if (/`/.test(args)) dangerousPatterns.push('backtick command substitution');
+  if (/<\(/.test(args)) dangerousPatterns.push('<() process substitution');
+  if (/>\(/.test(args)) dangerousPatterns.push('>() process substitution');
+
+  // Command separators (outside of quotes)
+  if (/;/.test(args)) dangerousPatterns.push('; command separator');
+  if (/\|/.test(args)) dangerousPatterns.push('| pipe');
+  if (/&&/.test(args)) dangerousPatterns.push('&& AND operator');
+  if (/\|\|/.test(args)) dangerousPatterns.push('|| OR operator');
+
+  // Background execution (space + &, with optional surrounding)
+  if (/\s+&/.test(args)) dangerousPatterns.push('& background operator');
+
+  // Input/Output redirection
+  if (/>\s+|\d*>/.test(args)) dangerousPatterns.push('> output redirection');
+  if (/<\s+|\d*</.test(args)) dangerousPatterns.push('< input redirection');
+
+  return {
+    isSafe: dangerousPatterns.length === 0,
+    dangerousPatterns,
+  };
+}