diff --git a/packages/harness/README.md b/packages/harness/README.md index 3ea85263..06de37ab 100644 --- a/packages/harness/README.md +++ b/packages/harness/README.md @@ -48,11 +48,14 @@ Both `Ok` and `Err` carry a `report`, so a failed run still reports its cost. A token count is `undefined` when the adapter does not report it. If the run fails before the Agent starts, `steps` is 0. `report.warnings` names each source file the Harness left out for size, each `onEvent` failure, and each cleanup problem. -An `InvalidSkill` error lists each failed output check in `issues`. +If the output fails a check, the Harness sends the issues back to the Agent in the same session, up to two times. +Each repair turn adds its steps and usage to the report, and a warning names the issues. +An `InvalidSkill` error lists each check that still fails in `issues`. The Harness checks the rules that the generation Skills state: - The frontmatter contains only `name` and `description`. +- The description is one plain YAML line without double quotes, backticks, or `%`. - `SKILL.md` links every file under `references/`, by a Markdown link or an inline code path. - A package Skill keeps `SKILL.md` under 500 lines and writes at most eight reference files. - A project Skill points only at project files and gives at least one search command. diff --git a/packages/harness/src/harness.ts b/packages/harness/src/harness.ts index 59f86787..4756a418 100644 --- a/packages/harness/src/harness.ts +++ b/packages/harness/src/harness.ts @@ -1,5 +1,5 @@ import type { HarnessV1NetworkSandboxSession, HarnessV1Skill } from '@ai-sdk/harness' -import type { SandboxSession } from './internal/output/collect.ts' +import type { CollectedFile, SandboxSession } from './internal/output/collect.ts' import type { Result } from './internal/result.ts' import type { PreparedSource } from './internal/source/host.ts' import type { FetchClient } from './internal/source/npm.ts' @@ -125,8 +125,23 @@ function toUsage(usage: AgentUsage): SkillRunUsage { } } -function cancelled(): RunOutcome { - return err({ _tag: 'Cancelled', message: 'Skill run was cancelled.' }) +/** An unknown count in any turn keeps the total unknown. */ +function sumUsage(a: SkillRunUsage, b: SkillRunUsage): SkillRunUsage { + const sum = (x: number | undefined, y: number | undefined) => x === undefined || y === undefined ? undefined : x + y + return { inputTokens: sum(a.inputTokens, b.inputTokens), cachedInputTokens: sum(a.cachedInputTokens, b.cachedInputTokens), outputTokens: sum(a.outputTokens, b.outputTokens) } +} + +/** Turns the Agent gets to fix output that failed the deterministic checks, in the same session. */ +const OUTPUT_REPAIR_TURNS = 2 + +function repairRequest(outputPath: string, issues: ReadonlyArray): string { + return [ + `The Harness checked the output at \`${outputPath}\` and found these problems:`, + '', + ...issues.map(issue => `- ${issue}`), + '', + `Fix each problem in place. Write no files outside \`${outputPath}\`. Finish when the output passes these checks.`, + ].join('\n') } async function prepareCurrentSkill(destination: SkillDestination, policy: SkillOutputPolicy, signal?: AbortSignal): Promise> { @@ -263,10 +278,14 @@ async function writePreparedSource(active: ActiveSandbox, prepared: PreparedRun, }) } -function toAgentError(cause: unknown, signal?: AbortSignal): RunOutcome { +function agentError(cause: unknown, signal?: AbortSignal): SkillRunError { return signal?.aborted - ? cancelled() - : err({ _tag: 'AgentFailed', message: 'Harness Agent failed during the Skill run.', cause }) + ? { _tag: 'Cancelled', message: 'Skill run was cancelled.' } + : { _tag: 'AgentFailed', message: 'Harness Agent failed during the Skill run.', cause } +} + +function toAgentError(cause: unknown, signal?: AbortSignal): RunOutcome { + return err(agentError(cause, signal)) } export function createSkillHarness(options: CreateSkillHarnessOptions): SkillHarness { @@ -352,44 +371,70 @@ export function createSkillHarness(options: CreateSkillHarnessOptions): SkillHar const currentSkillPath = posix.join(active.workDir, 'input/current-skill') const outputPath = posix.join(active.workDir, 'skilld-output', prepared.outputName) const prompt = renderRequest(requestContent(skill), sourcePath, currentSkillPath, outputPath, prepared.outputName) + const outputSandbox = active.sandbox + // Steps count across turns, so a repair turn continues the numbering. let step = 0 - const generated = await agent.generate({ - session, - prompt, - abortSignal: signal, - onStepStart: (event) => { - step = event.stepNumber - log.emit({ _tag: 'StepStart', step }) - }, - onToolExecutionStart: ({ toolCall }) => { - log.emit({ _tag: 'ToolCall', step, toolName: toolCall.toolName, toolCallId: toolCall.toolCallId, input: toolCall.input }) - }, - onStepEnd: (result) => { - log.emit({ _tag: 'StepFinish', step: result.stepNumber, finishReason: result.finishReason, usage: toUsage(result.usage) }) - }, - }).then(ok, cause => err(cause)) - if (generated._tag === 'Err') - return toAgentError(generated.error, signal) - log.settle(toUsage(generated.value.totalUsage), generated.value.steps.length) - if (signal?.aborted) - return cancelled() + let stepOffset = 0 + let usage: SkillRunUsage | undefined + const turn = async (text: string): Promise> => { + const generated = await agent.generate({ + session, + prompt: text, + abortSignal: signal, + onStepStart: (event) => { + step = stepOffset + event.stepNumber + log.emit({ _tag: 'StepStart', step }) + }, + onToolExecutionStart: ({ toolCall }) => { + log.emit({ _tag: 'ToolCall', step, toolName: toolCall.toolName, toolCallId: toolCall.toolCallId, input: toolCall.input }) + }, + onStepEnd: (result) => { + log.emit({ _tag: 'StepFinish', step: stepOffset + result.stepNumber, finishReason: result.finishReason, usage: toUsage(result.usage) }) + }, + }).then(ok, cause => err(cause)) + if (generated._tag === 'Err') + return err(agentError(generated.error, signal)) + const turnUsage = toUsage(generated.value.totalUsage) + usage = usage ? sumUsage(usage, turnUsage) : turnUsage + stepOffset += generated.value.steps.length + log.settle(usage, stepOffset) + return signal?.aborted ? err({ _tag: 'Cancelled', message: 'Skill run was cancelled.' }) : ok(undefined) + } + const check = async (): Promise, SkillRunError>> => { + const collected = await collectSandboxOutput(outputSandbox, outputPath, policy, signal) + if (collected._tag === 'Err') + return collected + const validated = prepared.skillName === 'review-skill' + ? validateSkillReview(collected.value) + : validateGeneratedSkill( + prepared.outputName, + collected.value, + prepared.skillName === 'generate-project-skill' + ? { _tag: 'ProjectSkill', projectPaths: prepared.source.files.map(file => file.path) } + : { _tag: 'PackageSkill' }, + ) + return validated._tag === 'Err' ? validated : ok(collected.value) + } - const collected = await collectSandboxOutput(active.sandbox, outputPath, policy, signal) - if (collected._tag === 'Err') - return collected + const first = await turn(prompt) + if (first._tag === 'Err') + return first + let checked = await check() + // A failed check goes back to the Agent, which still holds its context, instead of failing the run. + for (let repair = 1; repair <= OUTPUT_REPAIR_TURNS && checked._tag === 'Err' && checked.error._tag === 'InvalidSkill'; repair++) { + log.warn(`Output checks failed; repair turn ${repair} of ${OUTPUT_REPAIR_TURNS}: ${checked.error.issues.join(' ')}`) + const repaired = await turn(repairRequest(outputPath, checked.error.issues)) + if (repaired._tag === 'Err') + return repaired + checked = await check() + } + if (checked._tag === 'Err') + return checked + const collected = checked if (prepared.skillName === 'review-skill') return validateSkillReview(collected.value) - const validated = validateGeneratedSkill( - prepared.outputName, - collected.value, - prepared.skillName === 'generate-project-skill' - ? { _tag: 'ProjectSkill', projectPaths: prepared.source.files.map(file => file.path) } - : { _tag: 'PackageSkill' }, - ) - if (validated._tag === 'Err') - return validated if (!prepared.destination) return err({ _tag: 'InvalidInput', message: 'Skill destination is required.' }) const promoted = await promoteSkill( diff --git a/packages/harness/src/internal/output/validate.ts b/packages/harness/src/internal/output/validate.ts index e24ae5dc..76b1c77f 100644 --- a/packages/harness/src/internal/output/validate.ts +++ b/packages/harness/src/internal/output/validate.ts @@ -1,7 +1,7 @@ import type { SkillReview, SkillReviewFinding, SkillRunError } from '../../types.ts' import type { Result } from '../result.ts' import type { CollectedFile } from './collect.ts' -import { parseDocument } from 'yaml' +import { isScalar, parseDocument, Scalar } from 'yaml' import { isSkillName, normalizeOutputPath } from '../paths.ts' import { checkProjectSkill } from './project.ts' import { err, ok } from '../result.ts' @@ -15,6 +15,12 @@ const packageSkillLineLimit = 500 /** `generate-package-skill`: "Write at most eight reference files." */ const packageSkillReferenceLimit = 8 +/** + * Skill loaders and viewers parse frontmatter with different YAML parsers. + * A plain one-line description without these characters reads the same in all of them. + */ +const riskyDescription = /["`%]/ + /** * The Skill that produced the output decides which of its written rules apply. * A project Skill also carries the project paths its pointers must resolve to. @@ -125,6 +131,9 @@ export const validateGeneratedSkill = ( issues.push('Frontmatter name is invalid.') if (typeof frontmatter.description !== 'string' || frontmatter.description.trim().length === 0 || frontmatter.description.length > 1024) issues.push('Frontmatter description must contain 1 to 1024 characters.') + const description = document.get('description', true) + if (typeof frontmatter.description === 'string' && (!isScalar(description) || description.type !== Scalar.PLAIN || riskyDescription.test(frontmatter.description))) + issues.push('Frontmatter description must be one plain line without double quotes, backticks, or %. Name symptoms in plain words.') issues.push(...structureIssues(source, files, contract)) return issues.length === 0 ? ok(undefined) : invalid(issues) diff --git a/packages/harness/test/e2e/harness.test.ts b/packages/harness/test/e2e/harness.test.ts index 74bb0374..d8d724a8 100644 --- a/packages/harness/test/e2e/harness.test.ts +++ b/packages/harness/test/e2e/harness.test.ts @@ -1,4 +1,4 @@ -import { mkdir, mkdtemp, readFile, symlink, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { createSkillHarness } from '../../src/index.ts' @@ -237,6 +237,8 @@ describe('createSkillHarness', () => { async onPrompt({ workDir }) { const outputDir = join(workDir, 'skilld-output/example-project') await mkdir(outputDir, { recursive: true }) + // Every repair turn links the file again. + await rm(join(outputDir, 'SKILL.md'), { force: true }) await symlink(outside, join(outputDir, 'SKILL.md')) }, }) diff --git a/packages/harness/test/e2e/output-checks.test.ts b/packages/harness/test/e2e/output-checks.test.ts index 05da0acb..58e0e202 100644 --- a/packages/harness/test/e2e/output-checks.test.ts +++ b/packages/harness/test/e2e/output-checks.test.ts @@ -58,6 +58,49 @@ function linking(paths: ReadonlyArray): string { return paths.map(path => `- [${path}](${path})`).join('\n') } +const promptText = (prompt: unknown): string => typeof prompt === 'string' ? prompt : JSON.stringify(prompt) + +/** Runs a package Skill whose Agent writes `outputs[n]` on its nth turn, repeating the last one. */ +async function runTurns(outputs: ReadonlyArray) { + const packageDir = await mkdtemp(join(tmpdir(), 'skilld-checks-')) + await writeFile(join(packageDir, 'package.json'), '{"name":"example-package"}\n') + const destinationRoot = await mkdtemp(join(tmpdir(), 'skilld-checks-out-')) + let turn = 0 + const fake = createFakeHarness({ + async onPrompt({ sandbox, workDir }) { + const output = outputs[Math.min(turn++, outputs.length - 1)]! + for (const [path, content] of Object.entries(output)) + await sandbox.writeTextFile({ path: join(workDir, 'skilld-output/example-package', path), content }) + }, + }) + const result = await createSkillHarness({ harness: fake.harness, sandbox: createFakeSandboxProvider() }).run({ + _tag: 'PackageSkill', + source: { _tag: 'LocalPackage', rootDir: packageDir, packageDir: '.' }, + destination: { rootDir: destinationRoot, name: 'example-package' }, + }) + return { result, prompts: fake.capture.prompts.map(prompt => promptText(prompt.prompt)) } +} + +describe('output check feedback', () => { + const invalid = { 'SKILL.md': `${frontmatter('example-package', 'license: MIT\n')}# Example\n` } + const valid = { 'SKILL.md': `${frontmatter('example-package')}# Example\n` } + + it('sends failed checks back to the Agent in the same session and promotes the repaired Skill', async () => { + const { result, prompts } = await runTurns([invalid, valid]) + + expect(result).toMatchObject({ _tag: 'Ok', value: { _tag: 'GeneratedSkill' } }) + expect(prompts).toHaveLength(2) + expect(prompts[1]).toContain('Frontmatter field is not supported: license') + }) + + it('returns the remaining issues after two repair turns', async () => { + const { result, prompts } = await runTurns([invalid]) + + expect(issues(result)).toContain('Frontmatter field is not supported: license') + expect(prompts).toHaveLength(3) + }) +}) + describe('generated Skill output checks', () => { it('promotes a package Skill that links eight references', async () => { const files = references(8) @@ -115,6 +158,17 @@ describe('generated Skill output checks', () => { expect(result).toMatchObject({ _tag: 'Ok' }) }) + it.each([ + ['Use it when a task reports "Nuxt not detected".', 'quotes'], + ['Use it when %siteName prints in the title.', 'percent'], + ['Use it when `useHead` throws.', 'backtick'], + ['>\n Use it for head tags.', 'block'], + ])('rejects a description a Skill loader can misread (%s)', async (description) => { + const result = await runPackageSkill({ 'SKILL.md': `---\nname: example-package\ndescription: ${description}\n---\n\n# Example\n` }) + + expect(issues(result)).toContain('Frontmatter description must be one plain line without double quotes, backticks, or %. Name symptoms in plain words.') + }) + it('applies the frontmatter and link rules to a project Skill', async () => { const result = await runProjectSkill({ 'SKILL.md': `${frontmatter('example-project', 'license: MIT\n')}${projectSkillBody()}`, diff --git a/packages/harness/test/e2e/run-events.test.ts b/packages/harness/test/e2e/run-events.test.ts index 2f64c37a..ff3e73a7 100644 --- a/packages/harness/test/e2e/run-events.test.ts +++ b/packages/harness/test/e2e/run-events.test.ts @@ -159,7 +159,7 @@ describe('failed skill run report', () => { }) }) - it('reports usage and steps when the output check fails', async () => { + it('sums usage and steps across repair turns when the output check still fails', async () => { const { harness } = createFakeHarness({ async onPrompt({ sandbox, workDir }) { await sandbox.writeTextFile({ @@ -179,7 +179,14 @@ describe('failed skill run report', () => { expect(result).toMatchObject({ _tag: 'Err', error: { _tag: 'InvalidSkill' }, - report: { steps: 1, usage: { outputTokens: 30 }, warnings: [] }, + report: { + steps: 3, + usage: { outputTokens: 90 }, + warnings: [ + 'Output checks failed; repair turn 1 of 2: Frontmatter name must match the Skill directory name.', + 'Output checks failed; repair turn 2 of 2: Frontmatter name must match the Skill directory name.', + ], + }, }) }) diff --git a/packages/harness/test/unit/extract-blocks.test.ts b/packages/harness/test/unit/extract-blocks.test.ts new file mode 100644 index 00000000..b8d56d3d --- /dev/null +++ b/packages/harness/test/unit/extract-blocks.test.ts @@ -0,0 +1,75 @@ +import { spawnSync } from 'node:child_process' +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' + +const script = resolve(import.meta.dirname, '../../../../skills/generate-package-skill/scripts/extract-blocks.mjs') + +const skill = [ + '# Example', + '', + '```ts', + '// server/api/search.ts', + 'export default defineEventHandler(() => fetch(\'https://example.com/api\'))', + '```', + '', + '````md', + '```ts', + 'nested', + '```', + '````', + '', + '```vue', + '', + '', + '```', + '', + '```sh', + 'pnpm add retriv', + '```', + '', + '```ts', + '// ../outside.ts', + 'export {}', + '```', + '', +].join('\n') + +describe('extract-blocks script', () => { + let dir: string + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'extract-blocks-')) + await writeFile(join(dir, 'SKILL.md'), skill) + }) + + afterEach(async () => { + await rm(dir, { recursive: true, force: true }) + }) + + it('writes each block to its own directory under the path its first line names', async () => { + const out = join(dir, 'blocks') + const run = spawnSync(process.execPath, [script, join(dir, 'SKILL.md'), out, '--replace', 'https://example.com=http://localhost:4000'], { encoding: 'utf8' }) + + expect(await readFile(join(out, '1/server/api/search.ts'), 'utf8')).toBe('// server/api/search.ts\nexport default defineEventHandler(() => fetch(\'http://localhost:4000/api\'))\n') + expect(await readFile(join(out, '2/block.md'), 'utf8')).toBe('```ts\nnested\n```\n') + expect(await readFile(join(out, '3/app/pages/index.vue'), 'utf8')).toContain('