From e341e9ae3710f89affbeed6c25bd2e78787fd64b Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 10 Sep 2025 16:24:59 +0800 Subject: [PATCH] fix: tests fail on Windows --- .../src/subagents/subagent-manager.test.ts | 58 +++++- .../core/src/subagents/subagent-manager.ts | 38 ++-- packages/core/src/utils/yaml-parser.test.ts | 193 ++++++++++++++++++ packages/core/src/utils/yaml-parser.ts | 64 ++++-- 4 files changed, 315 insertions(+), 38 deletions(-) create mode 100644 packages/core/src/utils/yaml-parser.test.ts diff --git a/packages/core/src/subagents/subagent-manager.test.ts b/packages/core/src/subagents/subagent-manager.test.ts index cd361d88..0a7272db 100644 --- a/packages/core/src/subagents/subagent-manager.test.ts +++ b/packages/core/src/subagents/subagent-manager.test.ts @@ -108,6 +108,12 @@ describe('SubagentManager', () => { if (yamlString.includes('name: agent3')) { return { name: 'agent3', description: 'Third agent' }; } + if (yamlString.includes('name: 11')) { + return { name: 11, description: 333 }; // Numeric values test case + } + if (yamlString.includes('name: true')) { + return { name: true, description: false }; // Boolean values test case + } if (!yamlString.includes('name:')) { return { description: 'A test subagent' }; // Missing name case } @@ -248,6 +254,46 @@ You are a helpful assistant. expect(config.runConfig).toEqual({ max_time_minutes: 5, max_turns: 10 }); }); + it('should handle numeric name and description values', () => { + const markdownWithNumeric = `--- +name: 11 +description: 333 +--- + +You are a helpful assistant. +`; + + const config = manager.parseSubagentContent( + markdownWithNumeric, + validConfig.filePath, + ); + + expect(config.name).toBe('11'); + expect(config.description).toBe('333'); + expect(typeof config.name).toBe('string'); + expect(typeof config.description).toBe('string'); + }); + + it('should handle boolean name and description values', () => { + const markdownWithBoolean = `--- +name: true +description: false +--- + +You are a helpful assistant. +`; + + const config = manager.parseSubagentContent( + markdownWithBoolean, + validConfig.filePath, + ); + + expect(config.name).toBe('true'); + expect(config.description).toBe('false'); + expect(typeof config.name).toBe('string'); + expect(typeof config.description).toBe('string'); + }); + it('should determine level from file path', () => { const projectPath = '/test/project/.qwen/agents/test-agent.md'; const userPath = '/home/user/.qwen/agents/test-agent.md'; @@ -358,7 +404,7 @@ You are a helpful assistant. await manager.createSubagent(validConfig, { level: 'project' }); expect(fs.mkdir).toHaveBeenCalledWith( - path.dirname(validConfig.filePath), + path.normalize(path.dirname(validConfig.filePath)), { recursive: true }, ); expect(fs.writeFile).toHaveBeenCalledWith( @@ -428,7 +474,7 @@ You are a helpful assistant. expect(config).toBeDefined(); expect(config!.name).toBe('test-agent'); expect(fs.readFile).toHaveBeenCalledWith( - '/test/project/.qwen/agents/test-agent.md', + path.normalize('/test/project/.qwen/agents/test-agent.md'), 'utf8', ); }); @@ -443,7 +489,7 @@ You are a helpful assistant. expect(config).toBeDefined(); expect(config!.name).toBe('test-agent'); expect(fs.readFile).toHaveBeenCalledWith( - '/home/user/.qwen/agents/test-agent.md', + path.normalize('/home/user/.qwen/agents/test-agent.md'), 'utf8', ); }); @@ -507,7 +553,7 @@ You are a helpful assistant. await manager.deleteSubagent('test-agent', 'project'); expect(fs.unlink).toHaveBeenCalledWith( - '/test/project/.qwen/agents/test-agent.md', + path.normalize('/test/project/.qwen/agents/test-agent.md'), ); }); @@ -518,10 +564,10 @@ You are a helpful assistant. expect(fs.unlink).toHaveBeenCalledTimes(2); expect(fs.unlink).toHaveBeenCalledWith( - '/test/project/.qwen/agents/test-agent.md', + path.normalize('/test/project/.qwen/agents/test-agent.md'), ); expect(fs.unlink).toHaveBeenCalledWith( - '/home/user/.qwen/agents/test-agent.md', + path.normalize('/home/user/.qwen/agents/test-agent.md'), ); }); diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index dfd18759..5a15e71c 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -121,9 +121,9 @@ export class SubagentManager { return BuiltinAgentRegistry.getBuiltinAgent(name); } - const path = this.getSubagentPath(name, level); + const filePath = this.getSubagentPath(name, level); try { - const config = await this.parseSubagentFile(path); + const config = await this.parseSubagentFile(filePath); return config; } catch (_error) { return null; @@ -378,18 +378,22 @@ export class SubagentManager { // Parse YAML frontmatter const frontmatter = parseYaml(frontmatterYaml) as Record; - // Extract required fields - const name = frontmatter['name'] as string; - const description = frontmatter['description'] as string; + // Extract required fields and convert to strings + const nameRaw = frontmatter['name']; + const descriptionRaw = frontmatter['description']; - if (!name || typeof name !== 'string') { - throw new Error('Missing or invalid "name" in frontmatter'); + if (nameRaw == null || nameRaw === '') { + throw new Error('Missing "name" in frontmatter'); } - if (!description || typeof description !== 'string') { - throw new Error('Missing or invalid "description" in frontmatter'); + if (descriptionRaw == null || descriptionRaw === '') { + throw new Error('Missing "description" in frontmatter'); } + // Convert to strings (handles numbers, booleans, etc.) + const name = String(nameRaw); + const description = String(descriptionRaw); + // Extract optional fields const tools = frontmatter['tools'] as string[] | undefined; const modelConfig = frontmatter['modelConfig'] as @@ -400,11 +404,19 @@ export class SubagentManager { | undefined; const color = frontmatter['color'] as string | undefined; - // Determine level from file path - // Project level paths contain the project root, user level paths are in home directory + // Determine level from file path using robust, cross-platform check + // A project-level agent lives under /.qwen/agents + const projectAgentsDir = path.join( + this.config.getProjectRoot(), + QWEN_CONFIG_DIR, + AGENT_CONFIG_DIR, + ); + const rel = path.relative( + path.normalize(projectAgentsDir), + path.normalize(filePath), + ); const isProjectLevel = - filePath.includes(this.config.getProjectRoot()) && - filePath.includes(`/${QWEN_CONFIG_DIR}/${AGENT_CONFIG_DIR}/`); + rel !== '' && !rel.startsWith('..') && !path.isAbsolute(rel); const level: SubagentLevel = isProjectLevel ? 'project' : 'user'; const config: SubagentConfig = { diff --git a/packages/core/src/utils/yaml-parser.test.ts b/packages/core/src/utils/yaml-parser.test.ts new file mode 100644 index 00000000..1997a12f --- /dev/null +++ b/packages/core/src/utils/yaml-parser.test.ts @@ -0,0 +1,193 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { parse, stringify } from './yaml-parser.js'; + +describe('yaml-parser', () => { + describe('parse', () => { + it('should parse simple key-value pairs', () => { + const yaml = 'name: test\ndescription: A test config'; + const result = parse(yaml); + expect(result).toEqual({ + name: 'test', + description: 'A test config', + }); + }); + + it('should parse arrays', () => { + const yaml = 'tools:\n - file\n - shell'; + const result = parse(yaml); + expect(result).toEqual({ + tools: ['file', 'shell'], + }); + }); + + it('should parse nested objects', () => { + const yaml = 'modelConfig:\n temperature: 0.7\n maxTokens: 1000'; + const result = parse(yaml); + expect(result).toEqual({ + modelConfig: { + temperature: 0.7, + maxTokens: 1000, + }, + }); + }); + }); + + describe('stringify', () => { + it('should stringify simple objects', () => { + const obj = { name: 'test', description: 'A test config' }; + const result = stringify(obj); + expect(result).toBe('name: test\ndescription: A test config'); + }); + + it('should stringify arrays', () => { + const obj = { tools: ['file', 'shell'] }; + const result = stringify(obj); + expect(result).toBe('tools:\n - file\n - shell'); + }); + + it('should stringify nested objects', () => { + const obj = { + modelConfig: { + temperature: 0.7, + maxTokens: 1000, + }, + }; + const result = stringify(obj); + expect(result).toBe( + 'modelConfig:\n temperature: 0.7\n maxTokens: 1000', + ); + }); + + describe('string escaping security', () => { + it('should properly escape strings with quotes', () => { + const obj = { key: 'value with "quotes"' }; + const result = stringify(obj); + expect(result).toBe('key: "value with \\"quotes\\""'); + }); + + it('should properly escape strings with backslashes', () => { + const obj = { key: 'value with \\ backslash' }; + const result = stringify(obj); + expect(result).toBe('key: "value with \\\\ backslash"'); + }); + + it('should properly escape strings with backslash-quote sequences', () => { + // This is the critical security test case + const obj = { key: 'value with \\" sequence' }; + const result = stringify(obj); + // Should escape backslashes first, then quotes + expect(result).toBe('key: "value with \\\\\\" sequence"'); + }); + + it('should handle complex escaping scenarios', () => { + const testCases = [ + { + input: { path: 'C:\\Program Files\\"App"\\file.txt' }, + expected: 'path: "C:\\\\Program Files\\\\\\"App\\"\\\\file.txt"', + }, + { + input: { message: 'He said: \\"Hello\\"' }, + expected: 'message: "He said: \\\\\\"Hello\\\\\\""', + }, + { + input: { complex: 'Multiple \\\\ backslashes \\" and " quotes' }, + expected: + 'complex: "Multiple \\\\\\\\ backslashes \\\\\\" and \\" quotes"', + }, + ]; + + testCases.forEach(({ input, expected }) => { + const result = stringify(input); + expect(result).toBe(expected); + }); + }); + + it('should maintain round-trip integrity for escaped strings', () => { + const testStrings = [ + 'simple string', + 'string with "quotes"', + 'string with \\ backslash', + 'string with \\" sequence', + 'path\\to\\"file".txt', + 'He said: \\"Hello\\"', + 'Multiple \\\\ backslashes \\" and " quotes', + ]; + + testStrings.forEach((testString) => { + // Force quoting by adding a colon + const originalObj = { key: testString + ':' }; + const yamlString = stringify(originalObj); + const parsedObj = parse(yamlString); + expect(parsedObj).toEqual(originalObj); + }); + }); + + it('should not quote strings that do not need quoting', () => { + const obj = { key: 'simplevalue' }; + const result = stringify(obj); + expect(result).toBe('key: simplevalue'); + }); + + it('should quote strings with colons', () => { + const obj = { key: 'value:with:colons' }; + const result = stringify(obj); + expect(result).toBe('key: "value:with:colons"'); + }); + + it('should quote strings with hash symbols', () => { + const obj = { key: 'value#with#hash' }; + const result = stringify(obj); + expect(result).toBe('key: "value#with#hash"'); + }); + + it('should quote strings with leading/trailing whitespace', () => { + const obj = { key: ' value with spaces ' }; + const result = stringify(obj); + expect(result).toBe('key: " value with spaces "'); + }); + }); + + describe('numeric string handling', () => { + it('should parse unquoted numeric values as numbers', () => { + const yaml = 'name: 11\ndescription: 333'; + const result = parse(yaml); + expect(result).toEqual({ + name: 11, + description: 333, + }); + expect(typeof result['name']).toBe('number'); + expect(typeof result['description']).toBe('number'); + }); + + it('should parse quoted numeric values as strings', () => { + const yaml = 'name: "11"\ndescription: "333"'; + const result = parse(yaml); + expect(result).toEqual({ + name: '11', + description: '333', + }); + expect(typeof result['name']).toBe('string'); + expect(typeof result['description']).toBe('string'); + }); + + it('should handle mixed numeric and string values', () => { + const yaml = 'name: "11"\nage: 25\ndescription: "333"'; + const result = parse(yaml); + expect(result).toEqual({ + name: '11', + age: 25, + description: '333', + }); + expect(typeof result['name']).toBe('string'); + expect(typeof result['age']).toBe('number'); + expect(typeof result['description']).toBe('string'); + }); + }); + }); +}); diff --git a/packages/core/src/utils/yaml-parser.ts b/packages/core/src/utils/yaml-parser.ts index 9d58074d..2ae8dea7 100644 --- a/packages/core/src/utils/yaml-parser.ts +++ b/packages/core/src/utils/yaml-parser.ts @@ -20,33 +20,36 @@ export function parse(yamlString: string): Record { const lines = yamlString .split('\n') - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith('#')); + .filter((line) => line.trim() && !line.trim().startsWith('#')); const result: Record = {}; let currentKey = ''; - let currentArray: string[] = []; + let currentArray: unknown[] = []; let inArray = false; let currentObject: Record = {}; let inObject = false; let objectKey = ''; - for (const line of lines) { + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + // Handle array items - if (line.startsWith('- ')) { + if (line.startsWith(' - ')) { if (!inArray) { inArray = true; currentArray = []; } - currentArray.push(line.substring(2).trim()); + const itemRaw = line.substring(4).trim(); + currentArray.push(parseValue(itemRaw)); continue; } // End of array - if (inArray && !line.startsWith('- ')) { + if (inArray && !line.startsWith(' - ')) { result[currentKey] = currentArray; inArray = false; currentArray = []; + currentKey = ''; } // Handle nested object items (simple indentation) @@ -62,6 +65,7 @@ export function parse(yamlString: string): Record { result[objectKey] = currentObject; inObject = false; currentObject = {}; + objectKey = ''; } // Handle key-value pairs @@ -72,17 +76,25 @@ export function parse(yamlString: string): Record { if (value === '') { // This might be the start of an object or array currentKey = key.trim(); - // Check if next lines are indented (object) or start with - (array) - continue; + + // Look ahead to determine if this is an array or object + if (i + 1 < lines.length) { + const nextLine = lines[i + 1]; + if (nextLine.startsWith(' - ')) { + // Next line is an array item, so this will be handled in the next iteration + continue; + } else if (nextLine.startsWith(' ')) { + // Next line is indented, so this is an object + inObject = true; + objectKey = currentKey; + currentObject = {}; + currentKey = ''; + continue; + } + } } else { result[key.trim()] = parseValue(value); } - } else if (currentKey && !inArray && !inObject) { - // This might be the start of an object - inObject = true; - objectKey = currentKey; - currentObject = {}; - currentKey = ''; } } @@ -114,7 +126,7 @@ export function stringify( if (Array.isArray(value)) { lines.push(`${key}:`); for (const item of value) { - lines.push(` - ${item}`); + lines.push(` - ${formatValue(item)}`); } } else if (typeof value === 'object' && value !== null) { lines.push(`${key}:`); @@ -140,6 +152,13 @@ function parseValue(value: string): unknown { if (value === 'null') return null; if (value === '') return ''; + // Handle quoted strings + if (value.startsWith('"') && value.endsWith('"') && value.length >= 2) { + const unquoted = value.slice(1, -1); + // Unescape quotes and backslashes + return unquoted.replace(/\\"/g, '"').replace(/\\\\/g, '\\'); + } + // Try to parse as number const num = Number(value); if (!isNaN(num) && isFinite(num)) { @@ -155,9 +174,16 @@ function parseValue(value: string): unknown { */ function formatValue(value: unknown): string { if (typeof value === 'string') { - // Quote strings that might be ambiguous - if (value.includes(':') || value.includes('#') || value.trim() !== value) { - return `"${value.replace(/"/g, '\\"')}"`; + // Quote strings that might be ambiguous or contain special characters + if ( + value.includes(':') || + value.includes('#') || + value.includes('"') || + value.includes('\\') || + value.trim() !== value + ) { + // Escape backslashes THEN quotes + return `"${value.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`; } return value; }