diff --git a/config/scripts/generate-bundled-skill-guides.test.mjs b/config/scripts/generate-bundled-skill-guides.test.mjs index d2743d319d3..8c0a2a782c8 100644 --- a/config/scripts/generate-bundled-skill-guides.test.mjs +++ b/config/scripts/generate-bundled-skill-guides.test.mjs @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process' -import { cp, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { cp, mkdir, mkdtemp, readFile, readdir, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import path from 'node:path' import { promisify } from 'node:util' @@ -351,3 +351,55 @@ describe('bundled skill guide generator', () => { await expect(buildArtifacts(root)).rejects.toThrow('Guide reference is empty') }) }) + +// Why generalized: `orchestration-skill-guidance.test.mjs` pins this both-directions routing for +// orchestration alone. Any guide that grows a `references/` directory needs the same contract, or a +// reference can ship unroutable or a gate can route a file that does not exist. +describe('guide reference routing', () => { + async function guidesWithReferences() { + const guideRoot = path.join(projectDir, 'skill-guides') + const entries = await readdir(guideRoot, { withFileTypes: true }) + const owners = [] + for (const entry of entries.filter((candidate) => candidate.isDirectory())) { + const referenceRoot = path.join(guideRoot, entry.name, 'references') + const shipped = await readdir(referenceRoot).catch(() => null) + if (shipped === null) { + continue + } + owners.push({ + name: entry.name, + referenceRoot, + shipped: shipped.filter((file) => file.endsWith('.md')).sort() + }) + } + return owners + } + + it('routes every shipped reference from its own guide, in both directions', async () => { + const owners = await guidesWithReferences() + // A vacuous loop would pass forever; orchestration is the guide that owns references today. + expect(owners.map((owner) => owner.name)).toContain('orchestration') + + const mismatches = [] + for (const owner of owners) { + const guidePath = path.join(projectDir, 'skill-guides', `${owner.name}.md`) + const guide = await readFile(guidePath, 'utf8').catch(() => null) + if (guide === null) { + mismatches.push(`${owner.name}: references/ exists with no ${owner.name}.md beside it`) + continue + } + const routed = [ + ...new Set([...guide.matchAll(/`references\/([^`]+\.md)`/gu)].map((match) => match[1])) + ].sort() + const unshipped = routed.filter((file) => !owner.shipped.includes(file)) + const unrouted = owner.shipped.filter((file) => !routed.includes(file)) + if (unshipped.length > 0) { + mismatches.push(`${owner.name}: routes missing ${unshipped}`) + } + if (unrouted.length > 0) { + mismatches.push(`${owner.name}: ships unrouted ${unrouted}`) + } + } + expect(mismatches).toEqual([]) + }) +}) diff --git a/config/scripts/skill-description-length.test.mjs b/config/scripts/skill-description-length.test.mjs index e7a9db79541..b39af4b6da5 100644 --- a/config/scripts/skill-description-length.test.mjs +++ b/config/scripts/skill-description-length.test.mjs @@ -7,6 +7,10 @@ const skillsDir = resolve(import.meta.dirname, '../../skills') // Why: the Agent Skills spec caps `description` at 1024 chars and conforming installers // reject the whole skill (#17935); the frontmatter is what the installer parses, so check it. const MAX_DESCRIPTION_LENGTH = 1024 +// Why raw, not backtick-stripped: NVIDIA SkillEvaluator rejects `` in a description as a +// schema error, and Cowork's validator parses descriptions as HTML and fails the whole plugin +// silently (compound-engineering #602). Neither honors backticks, so placeholders belong in the body. +const ANGLE_BRACKET_TOKEN = /<[A-Za-z][\w.-]*>/u function readDescription(skillName) { const skillMarkdown = readFileSync(join(skillsDir, skillName, 'SKILL.md'), 'utf8') @@ -36,4 +40,13 @@ describe('bundled skill descriptions', () => { `${name}: description is ${description.length} chars` ).toBeLessThanOrEqual(MAX_DESCRIPTION_LENGTH) }) + + it.each(skillNames)('%s keeps angle-bracket placeholders out of its description', (name) => { + const token = ANGLE_BRACKET_TOKEN.exec(readDescription(name) ?? '') + + expect( + token?.[0], + `${name}: rephrase or move "${token?.[0] ?? ''}" into the skill body` + ).toBeUndefined() + }) }) diff --git a/config/scripts/skill-guide-size-budget.test.mjs b/config/scripts/skill-guide-size-budget.test.mjs new file mode 100644 index 00000000000..13ee7d7f186 --- /dev/null +++ b/config/scripts/skill-guide-size-budget.test.mjs @@ -0,0 +1,74 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' + +const guideRoot = resolve(import.meta.dirname, '../../skill-guides') + +/** + * Provenance: the Agent Skills spec's "keep your main SKILL.md under 500 lines" is an explicit + * recommendation, not a limit, and nothing rejects a longer guide. 300 is the tighter bound this + * repo already practices — six of eight guides sit under it, and `orchestration.md` holds 201 lines + * by routing detail into `references/`, which is the restructure this budget is meant to push. + * A line count is not a token count; treat a green run as a shape check, not a context-budget proof. + */ +const MAX_GUIDE_LINES = 300 + +/** + * Guides that already exceed the bound, with the size they may not grow past. Recorded sizes are a + * ratchet ceiling, not a target: shrink them freely and delete the entry once the guide fits. + * A name may leave this set. A name may never join it — split the guide into `references/` instead. + */ +const OVER_BUDGET = new Map([ + ['orca-cli', 424], + ['orca-per-workspace-env', 794] +]) + +/** Matches `wc -l`: a trailing newline ends the last line rather than starting a new one. */ +function lineCount(contents) { + const lines = contents.split(/\r?\n/u) + return lines.at(-1) === '' ? lines.length - 1 : lines.length +} + +function guideSizes() { + return new Map( + readdirSync(guideRoot, { withFileTypes: true }) + .filter((entry) => entry.isFile() && entry.name.endsWith('.md')) + .map((entry) => [ + entry.name.replace(/\.md$/u, ''), + lineCount(readFileSync(join(guideRoot, entry.name), 'utf8')) + ]) + ) +} + +describe('always-loaded skill guide size budget', () => { + const sizes = guideSizes() + + it('measures every shipped guide', () => { + expect(sizes.size).toBeGreaterThanOrEqual(8) + expect(sizes.get('orchestration')).toBeGreaterThan(0) + }) + + it('keeps every guide outside OVER_BUDGET under the bound', () => { + const violations = [...sizes] + .filter(([name, size]) => size > MAX_GUIDE_LINES && !OVER_BUDGET.has(name)) + .map(([name, size]) => `${name}: ${size} lines > ${MAX_GUIDE_LINES}`) + + expect(violations).toEqual([]) + }) + + it('never lets an OVER_BUDGET guide grow past its recorded size', () => { + const grown = [...OVER_BUDGET] + .filter(([name, ceiling]) => (sizes.get(name) ?? 0) > ceiling) + .map(([name, ceiling]) => `${name}: ${sizes.get(name)} lines > recorded ${ceiling}`) + + expect(grown).toEqual([]) + }) + + it('drops OVER_BUDGET entries that now fit, so the set only ratchets down', () => { + const stale = [...OVER_BUDGET.keys()].filter( + (name) => !sizes.has(name) || (sizes.get(name) ?? 0) <= MAX_GUIDE_LINES + ) + + expect(stale).toEqual([]) + }) +}) diff --git a/src/cli/skill-guide-cli-parity.test.ts b/src/cli/skill-guide-cli-parity.test.ts new file mode 100644 index 00000000000..11a2ad15c8d --- /dev/null +++ b/src/cli/skill-guide-cli-parity.test.ts @@ -0,0 +1,177 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { join, relative, resolve } from 'node:path' +import { describe, expect, it } from 'vitest' +import { CLI_GLOBAL_FLAGS } from '../shared/cli-argument-boundary' +import { specPaths } from './command-spec' +import { COMMAND_SPECS } from './specs' + +// Why: a guide is the version-matched surface for the binary that shipped it, so a command +// path or flag it names must exist in COMMAND_SPECS. `orca emulator camera --webcam` was +// documented for months without ever existing (#16904 review C1). + +// Why __dirname: it works under both Vitest and the CommonJS tsc emit that build:cli type-checks +// this file against; import.meta.dirname does not (TS1470). +const projectDir = resolve(__dirname, '..', '..') +const guideRoot = join(projectDir, 'skill-guides') +const MAX_COMMAND_DEPTH = 3 + +type Invocation = { file: string; line: number; text: string } + +function guideFiles(directory: string): string[] { + return readdirSync(directory, { withFileTypes: true }).flatMap((entry) => { + const full = join(directory, entry.name) + if (entry.isDirectory()) { + return guideFiles(full) + } + return entry.isFile() && entry.name.endsWith('.md') ? [full] : [] + }) +} + +/** + * The invocation span is the command text only — never the surrounding prose or table cell. + * `skill-guides/orca-emulator.md` describes serve-sim's own `--detach` in a Notes column beside + * an `ORCA ...` cell, and that is correct prose a line-scoped check would flag. + */ +function invocationSpans(contents: string, file: string): Invocation[] { + const found: Invocation[] = [] + let inFence = false + contents.split(/\r?\n/u).forEach((line, index) => { + if (/^\s*(?:```|~~~)/u.test(line)) { + inFence = !inFence + return + } + const spans = inFence ? [line] : [...line.matchAll(/`([^`]+)`/gu)].map((match) => match[1]) + for (const span of spans) { + const starts = [...span.matchAll(/\bORCA\b/gu)].map((match) => match.index) + starts.forEach((start, position) => { + found.push({ + file, + line: index + 1, + text: span.slice(start, starts[position + 1] ?? span.length).trim() + }) + }) + } + }) + return found +} + +/** Blank out quoted values so a nested `--model` inside `--command "codex --model ..."` is not read as a flag. */ +function maskQuotedValues(text: string): string { + let masked = '' + let quote: string | null = null + for (const character of text) { + if (quote) { + masked += character === quote ? character : ' ' + if (character === quote) { + quote = null + } + } else if (character === '"' || character === "'") { + quote = character + masked += character + } else { + masked += character + } + } + return masked +} + +const specByPath = new Map() +const pathPrefixes = new Set() +for (const spec of COMMAND_SPECS) { + for (const path of specPaths(spec)) { + specByPath.set(path.join(' '), spec) + for (let length = 1; length < path.length; length += 1) { + pathPrefixes.add(path.slice(0, length).join(' ')) + } + } +} + +function longestKnownPrefix(tokens: string[]): string | null { + for (let length = tokens.length; length >= 1; length -= 1) { + const candidate = tokens.slice(0, length).join(' ') + if (specByPath.has(candidate) || pathPrefixes.has(candidate)) { + return candidate + } + } + return null +} + +function allowedFlagsFor(prefix: string): Set { + const exact = specByPath.get(prefix) + const flags = new Set(CLI_GLOBAL_FLAGS) + const specs = exact + ? [exact] + : COMMAND_SPECS.filter((spec) => + specPaths(spec).some((path) => path.join(' ').startsWith(`${prefix} `)) + ) + for (const spec of specs) { + for (const flag of spec.allowedFlags) { + flags.add(flag) + } + } + return flags +} + +function describeFailure(invocation: Invocation, detail: string): string { + const location = `${relative(projectDir, invocation.file)}:${invocation.line}` + return `${location}: ${detail}\n ${invocation.text}` +} + +function parityFailures(invocation: Invocation): string[] { + const masked = maskQuotedValues(invocation.text).replace(/\s#.*$/u, '') + const tokens: string[] = [] + for (const token of masked.slice('ORCA'.length).trim().split(/\s+/u)) { + if (!/^[a-z][a-z0-9-]*$/u.test(token) || tokens.length === MAX_COMMAND_DEPTH) { + break + } + tokens.push(token) + } + if (tokens.length === 0) { + return [] + } + + const failures: string[] = [] + let command: string | null = null + for (let length = tokens.length; length >= 1 && command === null; length -= 1) { + const candidate = tokens.slice(0, length).join(' ') + if (specByPath.has(candidate)) { + command = candidate + } + } + if (command === null) { + // An incomplete reference like `ORCA emulator ...` names a real prefix and pins no flags. + if (pathPrefixes.has(tokens.join(' '))) { + return failures + } + failures.push( + describeFailure(invocation, `no COMMAND_SPECS path or alias for "${tokens.join(' ')}"`) + ) + command = longestKnownPrefix(tokens) + if (command === null) { + return failures + } + } + + const allowed = allowedFlagsFor(command) + for (const match of masked.matchAll(/--([a-z][a-z0-9-]*)/gu)) { + if (!allowed.has(match[1])) { + failures.push(describeFailure(invocation, `--${match[1]} is not a flag of "${command}"`)) + } + } + return failures +} + +describe('skill guides only name commands and flags the CLI defines', () => { + const invocations = guideFiles(guideRoot).flatMap((file) => + invocationSpans(readFileSync(file, 'utf8'), file) + ) + + it('extracts invocations from every guide and reference', () => { + expect(invocations.length).toBeGreaterThan(150) + expect(new Set(invocations.map((invocation) => invocation.file)).size).toBeGreaterThan(8) + }) + + it('resolves every ORCA invocation against COMMAND_SPECS', () => { + expect(invocations.flatMap(parityFailures)).toEqual([]) + }) +})