From e94e842dfe2266d0099a91a38fa46556e8f7729f Mon Sep 17 00:00:00 2001 From: Andrey Epifancev Date: Sat, 24 Oct 2026 23:50:09 +0400 Subject: [PATCH] =?UTF-8?q?fix(dsh):=20verify=20=E2=80=94=20=D1=81=D1=82?= =?UTF-8?q?=D1=80=D0=BE=D0=B3=D0=B0=D1=8F=20=D0=BF=D1=80=D0=BE=D0=B2=D0=B5?= =?UTF-8?q?=D1=80=D0=BA=D0=B0=20scripts/*,=20=D0=B4=D1=83=D0=B1=D0=BB?= =?UTF-8?q?=D0=B8=D0=BA=D0=B0=D1=82=D1=8B=20frontmatter,=20=D1=82=D0=B5?= =?UTF-8?q?=D1=81=D1=82=D1=8B=20(T2.1-T2.6,=20#69)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dsh/sync/verify-skills.mjs | 57 ++++---- .../scripts/review-package | 2 + .../scripts/sdd-workspace | 2 + dsh/tests/verify.test.mjs | 129 +++++++++++++++++- 4 files changed, 166 insertions(+), 24 deletions(-) create mode 100644 dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/review-package create mode 100644 dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/sdd-workspace diff --git a/dsh/sync/verify-skills.mjs b/dsh/sync/verify-skills.mjs index a1d8693..bb20bc8 100644 --- a/dsh/sync/verify-skills.mjs +++ b/dsh/sync/verify-skills.mjs @@ -14,18 +14,22 @@ * the scalar one vendored from LayneChai's `parseFrontmatter` (MIT); no * provider is imported, because on the DeepSeek Harness the registry * belongs to the built-in `skill-filesystem`, not to this repository. + * A duplicate scalar key is an error too: the last value silently wins + * in a naive parser, and a name/description pair that appears twice is + * a broken skill file, not a style choice. * 2. The body carries no leftover upstream `superpowers:` namespace * prefix — the port strips it during sync, and the DSH registry * addresses skills by bare name. * 3. Every relative reference in the body resolves inside ``: * markdown links (`[x](..//y.md)`) and path-like inline code - * (`references/y.md`, `..//references/y.md`, `scripts/y`). A - * sibling skill's `scripts/` invocation only needs that skill directory - * to exist: the reduced vendor tree under `dsh/tests/fixtures/` keeps - * the upstream command without shipping the executable, and a mistyped - * skill name still fails. Names that are illustrative examples rather - * than real files (`scripts/tool.sh`, …) are skipped, as in the - * vendored reference implementation. + * (`references/y.md`, `..//references/y.md`, `scripts/y`, + * `..//scripts/y`). Both the skill directory and the file must + * exist — a renamed or dropped upstream script is exactly the + * regression this check exists for (T2.1/T2.2). Names that are + * illustrative examples rather than real files (`scripts/tool.sh`, …) + * are skipped, as in the vendored reference implementation; that + * whitelist is a deliberate compromise, not a general escape hatch + * (T2.3 in the SDD ledger). * * The prefix check is scoped to SKILL.md bodies on purpose: reference files * such as `using-superpowers/references/dsh-tools.md` quote `superpowers:` in @@ -53,6 +57,11 @@ const TRAILING_PUNCTUATION = /[.,;:)\]]+$/ /** * Names that appear only as illustrative examples in prose, never as real * files — the same list the vendored reference implementation keeps. + * + * Deliberate compromise: upstream `writing-skills` shows `bash scripts/tool.sh` + * as an example, and no such file ships (see T2.3 in the SDD ledger). The list + * is closed and exact-path matched, so it cannot hide a real dangling link in + * another skill. */ const ILLUSTRATIVE = new Set(['scripts/tool.sh', 'scripts/tool.js', 'scripts/helper.py', 'scripts/helper.sh']) @@ -61,8 +70,11 @@ const ILLUSTRATIVE = new Set(['scripts/tool.sh', 'scripts/tool.js', 'scripts/hel * `parseFrontmatter`: the file starts with `---`, the block ends at the first * line that starts with `---`, and each `key: value` line becomes a string * (matching quotes stripped). Anything else is left for a real YAML parser. + * Keys seen more than once are collected in `duplicates`: the vendored parser + * would let the last value win silently, and a duplicated `name` or + * `description` is a broken skill file. * - * @returns {{metadata: Record, body: string} | null} + * @returns {{metadata: Record, body: string, duplicates: string[]} | null} */ function parseFrontmatter(text) { if (!text.startsWith('---')) return null @@ -71,6 +83,7 @@ function parseFrontmatter(text) { const block = text.slice(3, end) const body = text.slice(end + 4).replace(/^\n+/, '') const metadata = {} + const duplicates = [] for (const line of block.split('\n')) { const match = /^([A-Za-z][\w-]*):\s*(.*)$/.exec(line.trim()) if (!match) continue @@ -78,9 +91,10 @@ function parseFrontmatter(text) { if ((value.startsWith('"') && value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'"))) { value = value.slice(1, -1) } + if (Object.hasOwn(metadata, match[1])) duplicates.push(match[1]) metadata[match[1]] = value } - return { metadata, body } + return { metadata, body, duplicates } } /** Map every line inside a ``` / ~~~ fence to an empty line: examples are not prose. */ @@ -116,20 +130,16 @@ async function isDirectory(path) { } /** - * Classify one whitespace-separated token of an inline code span. + * Return the in-tree reference a whitespace-separated inline-code token names, + * or `null` when it is prose (`foo`, `PLAN_FILE`, `skills/…` from the checkout + * root) rather than a path relative to the skill directory. * - * `file` — a path that must resolve; `sibling-script` — a `..//scripts/` - * invocation, where only the skill directory has to exist (see the header). - * - * @returns {{kind: 'file' | 'sibling-script', path: string} | null} + * @returns {string | null} */ function inlineReference(token) { - if (/^(?:references|scripts)\//.test(token)) return { kind: 'file', path: token } - if (!token.startsWith('../')) return null - const parts = token.split('/') - const scripts = parts.indexOf('scripts') - if (scripts > 0) return { kind: 'sibling-script', path: parts.slice(0, scripts).join('/') } - return { kind: 'file', path: token } + if (/^(?:references|scripts)\//.test(token)) return token + if (token.startsWith('../')) return token + return null } function fail(skill, reason) { @@ -151,6 +161,7 @@ async function verifySkill(skillsRoot, skill) { const parsed = parseFrontmatter(text) if (parsed === null) fail(skill, 'missing YAML frontmatter') + if (parsed.duplicates.length > 0) fail(skill, `duplicate key "${parsed.duplicates[0]}" in frontmatter`) if (!parsed.metadata.name) fail(skill, 'empty name in frontmatter') if (!parsed.metadata.description) fail(skill, 'empty description in frontmatter') @@ -180,9 +191,9 @@ async function verifySkill(skillsRoot, skill) { if (token === '' || ILLUSTRATIVE.has(token)) continue const reference = inlineReference(token) if (reference === null) continue - const target = resolve(skillDir, reference.path) - const resolved = reference.kind === 'sibling-script' ? await isDirectory(target) : await exists(target) - if (!resolved) fail(skill, `dangling relative reference: ${token} (line ${lineNumber})`) + if (!(await exists(resolve(skillDir, reference)))) { + fail(skill, `dangling relative reference: ${token} (line ${lineNumber})`) + } } } } diff --git a/dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/review-package b/dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/review-package new file mode 100644 index 0000000..d1005a9 --- /dev/null +++ b/dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/review-package @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +echo "review-package: stub, dsh tests only" diff --git a/dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/sdd-workspace b/dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/sdd-workspace new file mode 100644 index 0000000..5e7b620 --- /dev/null +++ b/dsh/tests/fixtures/superpowers-min/skills/subagent-driven-development/scripts/sdd-workspace @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +echo "sdd-workspace: stub, dsh tests only" diff --git a/dsh/tests/verify.test.mjs b/dsh/tests/verify.test.mjs index f13b8d0..9ab9b00 100644 --- a/dsh/tests/verify.test.mjs +++ b/dsh/tests/verify.test.mjs @@ -1,6 +1,6 @@ import assert from 'node:assert/strict' import { spawnSync } from 'node:child_process' -import { cpSync, readFileSync } from 'node:fs' +import { cpSync, readFileSync, rmSync } from 'node:fs' import { dirname, join, resolve } from 'node:path' import test from 'node:test' import { fileURLToPath } from 'node:url' @@ -138,3 +138,130 @@ test('verify ok when superpowers: survives in a references file (T1.5)', () => { cleanup(work) } }) + +test('verify fails when a sibling skill script is missing', () => { + const work = tempDir('dsh-verify-missing-script-') + try { + const dir = syncedCopy(work, syncedTree(work)) + rmSync(join(dir, 'subagent-driven-development', 'scripts', 'sdd-workspace')) + + const result = verify(dir) + + assert.equal(result.status, 1) + assert.match(result.stderr, /executing-plans\/SKILL\.md/) + assert.match(result.stderr, /sdd-workspace/) + } finally { + cleanup(work) + } +}) + +test('verify fails on a mistyped sibling skill name', () => { + const work = tempDir('dsh-verify-mistyped-skill-') + try { + const dir = syncedCopy(work, syncedTree(work)) + const file = join(dir, 'executing-plans', 'SKILL.md') + write( + file, + `${readFileSync(file, 'utf8')}\nRun \`bash ../subagent-driven-developmen/scripts/sdd-workspace PLAN_FILE\`.\n`, + ) + + const result = verify(dir) + + assert.equal(result.status, 1) + assert.match(result.stderr, /subagent-driven-developmen\/scripts\/sdd-workspace/) + } finally { + cleanup(work) + } +}) + +test('verify fails on a duplicate frontmatter key', () => { + const work = tempDir('dsh-verify-duplicate-key-') + try { + const dir = syncedCopy(work, syncedTree(work)) + const file = join(dir, 'brainstorming', 'SKILL.md') + write(file, readFileSync(file, 'utf8').replace(/^name: brainstorming$/m, 'name: brainstorming\nname: duplicate')) + + const result = verify(dir) + + assert.equal(result.status, 1) + assert.match(result.stderr, /brainstorming\/SKILL\.md/) + assert.match(result.stderr, /duplicate key/) + } finally { + cleanup(work) + } +}) + +test('verify fails without frontmatter delimiters', () => { + const work = tempDir('dsh-verify-no-frontmatter-') + try { + const dir = syncedCopy(work, syncedTree(work)) + write(join(dir, 'brainstorming', 'SKILL.md'), '# Brainstorming\n\nNo frontmatter here.\n') + + const result = verify(dir) + + assert.equal(result.status, 1) + assert.match(result.stderr, /brainstorming\/SKILL\.md/) + assert.match(result.stderr, /missing YAML frontmatter/) + } finally { + cleanup(work) + } +}) + +test('verify CLI without a directory exits 2', () => { + const result = verify('') + + assert.equal(result.status, 2) + assert.match(result.stderr, /usage:/) +}) + +test('verify an empty skills directory reports zero skills', () => { + const work = tempDir('dsh-verify-empty-') + try { + const dir = join(work, 'empty') + write(join(dir, '.keep'), '') + + const result = verify(dir) + + assert.equal(result.status, 0, `stderr: ${result.stderr}`) + assert.match(result.stdout, /verified 0 skills/) + } finally { + cleanup(work) + } +}) + +test('verify ignores prefixes and paths inside fenced examples', () => { + const work = tempDir('dsh-verify-fenced-') + try { + const dir = syncedCopy(work, syncedTree(work)) + const file = join(dir, 'writing-plans', 'SKILL.md') + const fence = [ + '```', + 'See superpowers:brainstorming, [x](../brainstorming/nope.md) and `references/nope.md`.', + '```', + ].join('\n') + write(file, `${readFileSync(file, 'utf8')}\n${fence}\n`) + + const result = verify(dir) + + assert.equal(result.status, 0, `stderr: ${result.stderr}`) + assert.match(result.stdout, /verified 15 skills/) + } finally { + cleanup(work) + } +}) + +test('verify does not flag a resolving inline reference', () => { + const work = tempDir('dsh-verify-inline-reference-') + try { + const dir = syncedCopy(work, syncedTree(work)) + const file = join(dir, 'using-superpowers', 'SKILL.md') + write(file, `${readFileSync(file, 'utf8')}\nSee \`references/dsh-tools.md\` again.\n`) + + const result = verify(dir) + + assert.equal(result.status, 0, `stderr: ${result.stderr}`) + assert.match(result.stdout, /verified 15 skills/) + } finally { + cleanup(work) + } +})