Skip to content
Merged
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@
"undici": "^6.22.0",
"uuid": "^9.0.1",
"web-tree-sitter": "^0.24.7",
"ws": "^8.18.0"
"ws": "^8.18.0",
"yaml": "^2.8.1"
},
"optionalDependencies": {
"@lydell/node-pty": "1.2.0-beta.10",
Expand Down
120 changes: 120 additions & 0 deletions packages/core/src/skills/skill-load.real-parser.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
/**
* @license
* Copyright 2026 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, expect, it } from 'vitest';
import { parseSkillContent } from './skill-load.js';

describe('parseSkillContent with real YAML parser', () => {
const testPath = '/test/extension/skills/test-skill/SKILL.md';

it('parses folded block scalar descriptions (>)', () => {
const markdown = `---
name: test-skill
description: >
This is a folded
multiline description.
---

Body.
`;
const config = parseSkillContent(markdown, testPath);
expect(config.name).toBe('test-skill');
expect(config.description).toBe(
'This is a folded multiline description.\n',
);
});

it('parses literal block scalar descriptions (|)', () => {
const markdown = `---
name: test-skill
description: |
Line one.
Line two.
---
Body.
`;
const config = parseSkillContent(markdown, testPath);
expect(config.description).toBe('Line one.\nLine two.\n');
});

it('parses strip-chomped folded block scalar (>-)', () => {
const markdown = `---
name: test-skill
description: >-
No trailing newline.
---
Body.
`;
const config = parseSkillContent(markdown, testPath);
expect(config.description).toBe('No trailing newline.');
});

it('does not coerce date-like values to Date objects', () => {
const markdown = `---
name: test-skill
description: A skill created on 2024-01-01
---
Body.
`;
const config = parseSkillContent(markdown, testPath);
expect(typeof config.description).toBe('string');
});

it('handles allowedTools array correctly', () => {
const markdown = `---
name: test-skill
description: A test skill
allowedTools:
- read_file
- write_file
---
Body.
`;
const config = parseSkillContent(markdown, testPath);
expect(config.allowedTools).toEqual(['read_file', 'write_file']);
});

it('handles complex frontmatter with mixed field types', () => {
const markdown = `---
name: test-skill
description: >
Manage the full lifecycle of
cloud resources.
allowedTools:
- read_file
- write_file
model: qwen-max
argument-hint: "[resource-type]"
priority: 10
disable-model-invocation: true
---
Body content here.
`;
const config = parseSkillContent(markdown, testPath);
expect(config.name).toBe('test-skill');
expect(config.description).toContain('Manage the full lifecycle');
expect(config.allowedTools).toEqual(['read_file', 'write_file']);
expect(config.model).toBe('qwen-max');
expect(config.argumentHint).toBe('[resource-type]');
expect(config.priority).toBe(10);
expect(config.disableModelInvocation).toBe(true);
});

it('falls back gracefully for malformed YAML', () => {
// Unclosed flow mapping triggers a yaml.parse error; the simple
// parser ignores it and still extracts name + description.
const markdown = `---
name: test-skill
description: a test skill
extra: {key: [nested unclosed
---
Body.
`;
const config = parseSkillContent(markdown, testPath);
expect(config.name).toBe('test-skill');
expect(config.description).toBe('a test skill');
});
});
12 changes: 2 additions & 10 deletions packages/core/src/skills/skill-load.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
type SkillConfig,
type SkillValidationResult,
parseAllowedToolsField,
parseModelField,
parsePathsField,
validateSkillName,
Expand Down Expand Up @@ -132,16 +133,7 @@ export function parseSkillContent(
const description = String(descriptionRaw);

// Extract optional fields
const allowedToolsRaw = frontmatter['allowedTools'] as unknown[] | undefined;
let allowedTools: string[] | undefined;

if (allowedToolsRaw !== undefined) {
if (Array.isArray(allowedToolsRaw)) {
allowedTools = allowedToolsRaw.map(String);
} else {
throw new Error('"allowedTools" must be an array');
}
}
const allowedTools = parseAllowedToolsField(frontmatter);

// Extract optional model field
const model = parseModelField(frontmatter);
Expand Down
36 changes: 10 additions & 26 deletions packages/core/src/skills/skill-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import * as os from 'os';
import { watch as watchFs, type FSWatcher } from 'chokidar';
import { resolveBundleDir } from '../utils/bundlePaths.js';
import { parse as parseYaml } from '../utils/yaml-parser.js';
import * as yaml from 'yaml';
import type {
SkillConfig,
SkillLevel,
Expand All @@ -22,6 +21,7 @@ import type {
import {
SkillError,
SkillErrorCode,
parseAllowedToolsField,
parseModelField,
parsePathsField,
validateSkillName,
Expand Down Expand Up @@ -688,34 +688,18 @@ export class SkillManager {
const description = String(descriptionRaw);

// Extract optional fields
const allowedToolsRaw = frontmatter['allowedTools'] as
| unknown[]
| undefined;
let allowedTools: string[] | undefined;

if (allowedToolsRaw !== undefined) {
if (Array.isArray(allowedToolsRaw)) {
allowedTools = allowedToolsRaw.map(String);
} else {
throw new Error('"allowedTools" must be an array');
}
}
const allowedTools = parseAllowedToolsField(frontmatter);

// Extract hooks configuration
// Use full YAML parser for hooks as they have nested structures
let hooks: SkillHooksSettings | undefined;
if (frontmatterYaml.includes('hooks:')) {
// Re-parse with full YAML parser to get nested hooks structure
const fullFrontmatter = yaml.parse(frontmatterYaml) as Record<
string,
unknown
>;
const hooksRaw = fullFrontmatter['hooks'] as
| Record<string, unknown>
| undefined;
if (hooksRaw !== undefined) {
hooks = this.parseHooksConfig(hooksRaw);
}
const hooksRaw = frontmatter['hooks'];
if (
hooksRaw !== undefined &&
typeof hooksRaw === 'object' &&
hooksRaw !== null &&
!Array.isArray(hooksRaw)
) {
hooks = this.parseHooksConfig(hooksRaw as Record<string, unknown>);
}

// Set skillRoot to the directory containing SKILL.md
Expand Down
18 changes: 18 additions & 0 deletions packages/core/src/skills/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,24 @@ export function parsePathsField(
*/
export const SKILL_NAME_PATTERN = /^[\p{L}\p{N}_:.-]+$/u;

/**
* Parse the `allowedTools` field from skill frontmatter.
* Returns `undefined` when the field is omitted. Throws when the field is
* present but not an array.
*/
export function parseAllowedToolsField(
frontmatter: Record<string, unknown>,
): string[] | undefined {
const raw = frontmatter['allowedTools'];
if (raw == null) {
return undefined;
}
if (!Array.isArray(raw)) {
throw new Error('"allowedTools" must be an array');
}
return raw.map(String);
}

/**
* Validate that a skill `name` is safe to embed into prompts and reminders
* verbatim. Throws with a descriptive message if not — the surrounding
Expand Down
2 changes: 0 additions & 2 deletions packages/core/src/subagents/subagent-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
import * as fs from 'fs/promises';
import * as path from 'path';
import * as os from 'os';
// Note: yaml package would need to be added as a dependency
// For now, we'll use a simple YAML parser implementation
import {
parse as parseYaml,
stringify as stringifyYaml,
Expand Down
118 changes: 118 additions & 0 deletions packages/core/src/utils/yaml-parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,124 @@ describe('yaml-parser', () => {
},
});
});

it('should parse YAML folded block scalar (>)', () => {
const input =
'name: test-skill\ndescription: >\n This is a folded\n multiline description.';
const result = parse(input);
expect(result['name']).toBe('test-skill');
expect(result['description']).toBe(
'This is a folded multiline description.\n',
);
});

it('should parse YAML literal block scalar (|)', () => {
const input =
'name: test-skill\ndescription: |\n Line one.\n Line two.';
const result = parse(input);
expect(result['name']).toBe('test-skill');
expect(result['description']).toBe('Line one.\nLine two.\n');
});

it('should parse YAML block scalar with strip chomping (>-)', () => {
const input =
'name: test-skill\ndescription: >-\n Folded without trailing newline.';
const result = parse(input);
expect(result['name']).toBe('test-skill');
expect(result['description']).toBe('Folded without trailing newline.');
});

it('should not coerce date-like strings into Date objects', () => {
const input = 'name: test\ncreated: 2024-01-01';
const result = parse(input);
expect(typeof result['created']).toBe('string');
expect(result['created']).toBe('2024-01-01');
});

it('should strip bare keys with no value', () => {
const input = 'name: test\nhooks:';
const result = parse(input);
expect(result['name']).toBe('test');
expect(result['hooks']).toBeUndefined();
});

it('should strip explicit null and tilde values', () => {
const input = 'a: null\nb: ~';
const result = parse(input);
expect(result['a']).toBeUndefined();
expect(result['b']).toBeUndefined();
});

it('should treat yes/no as strings in YAML 1.2 core schema', () => {
const input = 'answer: yes\nother: no';
const result = parse(input);
expect(result['answer']).toBe('yes');
expect(result['other']).toBe('no');
Comment thread
yiliang114 marked this conversation as resolved.
});

it('should fall back to simple parser on invalid YAML', () => {
// Unclosed flow sequence triggers a yaml.parse error
const input = 'name: test\nallowedTools: [unclosed';
const result = parse(input);
expect(result['name']).toBe('test');
});

it('should strip null values in fallback path same as main path', () => {
// Unclosed flow forces fallback to parseSimple; explicit null
// must be stripped so callers can use `!== undefined` consistently.
const input = 'name: test\noptional: null\nbroken: [unclosed';
const result = parse(input);
expect(result['name']).toBe('test');
expect(result['optional']).toBeUndefined();
expect('optional' in result).toBe(false);
});

it('should not allow prototype pollution via simple parser fallback', () => {
// Crafted to fail yaml.parse (unclosed flow) and trigger parseSimple,
// where __proto__ as a nested-object key could pollute the prototype.
const input =
'__proto__:\n polluted: true\nname: test\nbroken: [unclosed';
const result = parse(input);
expect(result['name']).toBe('test');
const clean: Record<string, unknown> = {};
expect(clean['polluted']).toBeUndefined();
expect(Object.getPrototypeOf(result)).toBeNull();
});

it('should handle empty input gracefully', () => {
const result = parse('');
expect(result).toEqual({});
});

it('should handle comment-only input gracefully', () => {
const result = parse('# just a comment');
expect(result).toEqual({});
});

it('should not allow prototype pollution via __proto__ key', () => {
const input = 'name: legit\n__proto__:\n polluted: true';
const result = parse(input);
expect(result['name']).toBe('legit');
// result uses null prototype — __proto__ is a plain own property
expect(Object.getPrototypeOf(result)).toBeNull();
expect(Object.hasOwn(result, '__proto__')).toBe(true);
});

it('should not resolve !!timestamp explicit tags', () => {
const input = 'name: test\ncreated: !!timestamp 2024-01-01';
const result = parse(input);
expect(typeof result['created']).toBe('string');
});

it('should sanitize nested objects recursively', () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The PR description calls out "Date/Uint8Array coercion guards" but only !!timestamp/Date has test coverage. The !!binaryUint8Array path through sanitizeValue() is untested. Add a test alongside this one:

    it('should not resolve !!binary explicit tags', () => {
      const input = 'name: test\ndata: !!binary SGVsbG8=';
      const result = parse(input);
      expect(typeof result['data']).toBe('string');
    });

Without this, the Uint8Array branch in sanitizeValue() (line 64) is a security guard with no regression test.

— qwen3.7-plus via Qwen Code /review

const input =
'name: test\nmetadata:\n created: !!timestamp 2024-01-01\n note: hello';
const result = parse(input);
const metadata = result['metadata'] as Record<string, unknown>;
expect(typeof metadata['created']).toBe('string');
expect(metadata['note']).toBe('hello');
expect(Object.getPrototypeOf(metadata)).toBeNull();
});
});

describe('stringify', () => {
Expand Down
Loading
Loading