From f88c55da1f998b75828bbaa32ead66a85bebfb65 Mon Sep 17 00:00:00 2001 From: hujie03 Date: Tue, 28 Apr 2026 21:48:25 +0800 Subject: [PATCH] feat: add instructionFile support for external instruction file references Add instructionFile field to artifact schema (mirroring the template mechanism), allowing instruction content to be loaded from external files in the schema's instructions/ directory instead of being embedded inline in schema.yaml. This enables multiple schemas to share instruction content via symlinks or shared directories, eliminating the need for manual sync scripts. Key changes: - types.ts: add instructionFile to ArtifactSchema & ApplyPhaseSchema - schema.ts: mutual exclusivity validation (instruction vs instructionFile) - instruction-loader.ts: add InstructionLoadError + loadInstruction() - instruction-loader.ts: generateInstructions() resolves instructionFile - instructions.ts: apply phase also supports instructionFile - templates.ts: output includes instruction file paths - Tests: 7 new tests for validation + loading + resolution --- src/commands/workflow/instructions.ts | 7 +- src/commands/workflow/templates.ts | 39 +++- src/core/artifact-graph/index.ts | 2 + src/core/artifact-graph/instruction-loader.ts | 68 ++++++- src/core/artifact-graph/schema.ts | 24 ++- src/core/artifact-graph/types.ts | 6 + test/commands/artifact-workflow.test.ts | 6 +- .../artifact-graph/instruction-loader.test.ts | 185 ++++++++++++++++++ test/core/artifact-graph/schema.test.ts | 102 ++++++++++ 9 files changed, 423 insertions(+), 16 deletions(-) diff --git a/src/commands/workflow/instructions.ts b/src/commands/workflow/instructions.ts index 7afba1475..98ec42a4f 100644 --- a/src/commands/workflow/instructions.ts +++ b/src/commands/workflow/instructions.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import { loadChangeContext, generateInstructions, + loadInstruction, resolveSchema, resolveArtifactOutputs, type ArtifactInstructions, @@ -260,7 +261,9 @@ export async function generateApplyInstructions( // Fallback: if no apply block, require all artifacts const requiredArtifactIds = applyConfig?.requires ?? schema.artifacts.map((a) => a.id); const tracksFile = applyConfig?.tracks ?? null; - const schemaInstruction = applyConfig?.instruction ?? null; + const schemaInstruction = applyConfig?.instructionFile + ? loadInstruction(context.schemaName, applyConfig.instructionFile, projectRoot)?.trim() + : applyConfig?.instruction?.trim() ?? null; // Check which required artifacts are missing const missingArtifacts: string[] = []; @@ -320,7 +323,7 @@ export async function generateApplyInstructions( } else if (!tracksFile) { // No tracking file configured in schema - ready to apply state = 'ready'; - instruction = schemaInstruction?.trim() ?? 'All required artifacts complete. Proceed with implementation.'; + instruction = schemaInstruction ?? 'All required artifacts complete. Proceed with implementation.'; } else { state = 'ready'; instruction = schemaInstruction?.trim() ?? 'Read context files, work through pending tasks, mark complete as you go.\nPause if you hit blockers or need clarification.'; diff --git a/src/commands/workflow/templates.ts b/src/commands/workflow/templates.ts index fedd323e0..60a03941b 100644 --- a/src/commands/workflow/templates.ts +++ b/src/commands/workflow/templates.ts @@ -26,6 +26,7 @@ export interface TemplatesOptions { export interface TemplateInfo { artifactId: string; templatePath: string; + instructionFilePath: string | null; source: 'project' | 'user' | 'package'; } @@ -67,20 +68,35 @@ export async function templatesCommand(options: TemplatesOptions): Promise source = 'package'; } - const templates: TemplateInfo[] = graph.getAllArtifacts().map((artifact) => ({ - artifactId: artifact.id, - templatePath: FileSystemUtils.canonicalizeExistingPath( - path.join(schemaDir, 'templates', artifact.template) - ), - source, - })); + const templates: TemplateInfo[] = graph.getAllArtifacts().map((artifact) => { + const instructionFilePath = artifact.instructionFile + ? FileSystemUtils.canonicalizeExistingPath( + path.join(schemaDir, 'instructions', artifact.instructionFile) + ) + : null; + + return { + artifactId: artifact.id, + templatePath: FileSystemUtils.canonicalizeExistingPath( + path.join(schemaDir, 'templates', artifact.template) + ), + instructionFilePath, + source, + }; + }); spinner?.stop(); if (options.json) { - const output: Record = {}; + const output: Record = {}; for (const t of templates) { - output[t.artifactId] = { path: t.templatePath, source: t.source }; + const entry: { template: { path: string; source: string }; instructionFile?: { path: string } } = { + template: { path: t.templatePath, source: t.source }, + }; + if (t.instructionFilePath) { + entry.instructionFile = { path: t.instructionFilePath }; + } + output[t.artifactId] = entry; } console.log(JSON.stringify(output, null, 2)); return; @@ -92,7 +108,10 @@ export async function templatesCommand(options: TemplatesOptions): Promise for (const t of templates) { console.log(`${t.artifactId}:`); - console.log(` ${t.templatePath}`); + console.log(` template: ${t.templatePath}`); + if (t.instructionFilePath) { + console.log(` instruction: ${t.instructionFilePath}`); + } } } catch (error) { spinner?.stop(); diff --git a/src/core/artifact-graph/index.ts b/src/core/artifact-graph/index.ts index 8ec732846..de5c8b210 100644 --- a/src/core/artifact-graph/index.ts +++ b/src/core/artifact-graph/index.ts @@ -33,10 +33,12 @@ export { // Instruction loading export { loadTemplate, + loadInstruction, loadChangeContext, generateInstructions, formatChangeStatus, TemplateLoadError, + InstructionLoadError, type ChangeContext, type ArtifactInstructions, type DependencyInfo, diff --git a/src/core/artifact-graph/instruction-loader.ts b/src/core/artifact-graph/instruction-loader.ts index b8b2675bb..e53da84d3 100644 --- a/src/core/artifact-graph/instruction-loader.ts +++ b/src/core/artifact-graph/instruction-loader.ts @@ -24,6 +24,19 @@ export class TemplateLoadError extends Error { } } +/** + * Error thrown when loading an instruction file fails. + */ +export class InstructionLoadError extends Error { + constructor( + message: string, + public readonly instructionPath: string + ) { + super(message); + this.name = 'InstructionLoadError'; + } +} + /** * Change context containing graph, completion state, and metadata. */ @@ -160,6 +173,53 @@ export function loadTemplate( } } +/** + * Loads an instruction file from a schema's instructions directory. + * + * Mirrors loadTemplate() — resolves the file relative to + * /instructions/. + * + * @param schemaName - Schema name (e.g., "spec-driven") + * @param instructionPath - Relative path within the instructions directory (e.g., "requirements.md") + * @param projectRoot - Optional project root for project-local schema resolution + * @returns The instruction content + * @throws InstructionLoadError if the instruction file cannot be loaded + */ +export function loadInstruction( + schemaName: string, + instructionPath: string, + projectRoot?: string +): string { + const schemaDir = getSchemaDir(schemaName, projectRoot); + if (!schemaDir) { + throw new InstructionLoadError( + `Schema '${schemaName}' not found`, + instructionPath + ); + } + + const instructionPathOnDisk = path.join(schemaDir, 'instructions', instructionPath); + + if (!fs.existsSync(instructionPathOnDisk)) { + throw new InstructionLoadError( + `Instruction file not found: ${instructionPathOnDisk}`, + instructionPathOnDisk + ); + } + + const fullPath = FileSystemUtils.canonicalizeExistingPath(instructionPathOnDisk); + + try { + return fs.readFileSync(fullPath, 'utf-8'); + } catch (err) { + const ioError = err instanceof Error ? err : new Error(String(err)); + throw new InstructionLoadError( + `Failed to read instruction file: ${ioError.message}`, + fullPath + ); + } +} + /** * Loads change context combining graph and completion state. * @@ -224,6 +284,12 @@ export function generateInstructions( } const templateContent = loadTemplate(context.schemaName, artifact.template, context.projectRoot); + + // Resolve instruction: external file (instructionFile) takes priority over inline (instruction) + const instructionContent = artifact.instructionFile + ? loadInstruction(context.schemaName, artifact.instructionFile, context.projectRoot) + : artifact.instruction; + const dependencies = getDependencyInfo(artifact, context.graph, context.completed); const unlocks = getUnlockedArtifacts(context.graph, artifactId); @@ -270,7 +336,7 @@ export function generateInstructions( changeDir: context.changeDir, outputPath: artifact.generates, description: artifact.description, - instruction: artifact.instruction, + instruction: instructionContent, context: configContext, rules: configRules, template: templateContent, diff --git a/src/core/artifact-graph/schema.ts b/src/core/artifact-graph/schema.ts index 6371745e2..ecc4aa18b 100644 --- a/src/core/artifact-graph/schema.ts +++ b/src/core/artifact-graph/schema.ts @@ -1,6 +1,6 @@ import * as fs from 'node:fs'; import { parse as parseYaml } from 'yaml'; -import { SchemaYamlSchema, type SchemaYaml, type Artifact } from './types.js'; +import { SchemaYamlSchema, type SchemaYaml, type Artifact, type ApplyPhase } from './types.js'; export class SchemaValidationError extends Error { constructor(message: string) { @@ -41,6 +41,9 @@ export function parseSchema(yamlContent: string): SchemaYaml { // Check for cycles validateNoCycles(schema.artifacts); + // Check instruction / instructionFile mutual exclusivity + validateInstructionExclusivity(schema.artifacts, schema.apply); + return schema; } @@ -74,6 +77,25 @@ function validateRequiresReferences(artifacts: Artifact[]): void { } } +/** + * Validates that instruction and instructionFile are mutually exclusive. + * An artifact (or apply phase) cannot have both fields at the same time. + */ +function validateInstructionExclusivity(artifacts: Artifact[], apply?: ApplyPhase): void { + for (const artifact of artifacts) { + if (artifact.instruction && artifact.instructionFile) { + throw new SchemaValidationError( + `Artifact '${artifact.id}' cannot have both 'instruction' and 'instructionFile'` + ); + } + } + if (apply?.instruction && apply?.instructionFile) { + throw new SchemaValidationError( + `Apply phase cannot have both 'instruction' and 'instructionFile'` + ); + } +} + /** * Validates that there are no cyclic dependencies. * Uses DFS to detect cycles and reports the full cycle path. diff --git a/src/core/artifact-graph/types.ts b/src/core/artifact-graph/types.ts index fb0d12703..033515b13 100644 --- a/src/core/artifact-graph/types.ts +++ b/src/core/artifact-graph/types.ts @@ -7,6 +7,9 @@ export const ArtifactSchema = z.object({ description: z.string(), template: z.string().min(1, { error: 'template field is required' }), instruction: z.string().optional(), + // External instruction file (relative to schema's instructions/ directory). + // Mutually exclusive with instruction — validated in parseSchema(). + instructionFile: z.string().optional(), requires: z.array(z.string()).default([]), }); @@ -18,6 +21,9 @@ export const ApplyPhaseSchema = z.object({ tracks: z.string().nullable().optional(), // Custom guidance for the apply phase instruction: z.string().optional(), + // External instruction file for apply phase (relative to schema's instructions/ directory). + // Mutually exclusive with instruction — validated in parseSchema(). + instructionFile: z.string().optional(), }); // Full schema YAML structure diff --git a/test/commands/artifact-workflow.test.ts b/test/commands/artifact-workflow.test.ts index 613e37f98..a47acaa30 100644 --- a/test/commands/artifact-workflow.test.ts +++ b/test/commands/artifact-workflow.test.ts @@ -318,8 +318,10 @@ describe('artifact-workflow CLI commands', () => { const json = JSON.parse(result.stdout); expect(json.proposal).toBeDefined(); - expect(json.proposal.path).toContain('proposal.md'); - expect(json.proposal.source).toBe('package'); + expect(json.proposal.template.path).toContain('proposal.md'); + expect(json.proposal.template.source).toBe('package'); + // spec-driven schema uses inline instruction, so no instructionFile + expect(json.proposal.instructionFile).toBeUndefined(); }); it('errors for unknown schema', async () => { diff --git a/test/core/artifact-graph/instruction-loader.test.ts b/test/core/artifact-graph/instruction-loader.test.ts index 9d8f612cd..69f1d2efa 100644 --- a/test/core/artifact-graph/instruction-loader.test.ts +++ b/test/core/artifact-graph/instruction-loader.test.ts @@ -4,10 +4,12 @@ import * as path from 'node:path'; import * as os from 'node:os'; import { loadTemplate, + loadInstruction, loadChangeContext, generateInstructions, formatChangeStatus, TemplateLoadError, + InstructionLoadError, } from '../../../src/core/artifact-graph/instruction-loader.js'; describe('instruction-loader', () => { @@ -43,6 +45,85 @@ describe('instruction-loader', () => { }); }); + describe('loadInstruction', () => { + let tempDir: string; + let schemaDir: string; + + beforeEach(() => { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-test-')); + + // Create a project-local test schema with instructions directory + const schemasDir = path.join(tempDir, 'openspec', 'schemas', 'test-instruction-schema'); + const instructionsDir = path.join(schemasDir, 'instructions'); + const templatesDir = path.join(schemasDir, 'templates'); + + fs.mkdirSync(instructionsDir, { recursive: true }); + fs.mkdirSync(templatesDir, { recursive: true }); + + // Write schema.yaml + fs.writeFileSync( + path.join(schemasDir, 'schema.yaml'), + `name: test-instruction-schema +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: proposal.md + instructionFile: proposal.md + requires: [] +` + ); + + // Write instruction file + fs.writeFileSync( + path.join(instructionsDir, 'proposal.md'), + '# Proposal Instructions\nCreate the proposal document.' + ); + + // Write template file + fs.writeFileSync( + path.join(templatesDir, 'proposal.md'), + '## Why\n\n## What Changes' + ); + + schemaDir = schemasDir; + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + it('should load instruction file from schema instructions directory', () => { + const instruction = loadInstruction('test-instruction-schema', 'proposal.md', tempDir); + + expect(instruction).toContain('Proposal Instructions'); + expect(instruction).toContain('Create the proposal document'); + }); + + it('should throw InstructionLoadError for non-existent instruction file', () => { + expect(() => loadInstruction('test-instruction-schema', 'nonexistent.md', tempDir)).toThrow( + InstructionLoadError + ); + }); + + it('should throw InstructionLoadError for non-existent schema', () => { + expect(() => loadInstruction('nonexistent-schema', 'proposal.md', tempDir)).toThrow( + InstructionLoadError + ); + }); + + it('should include instruction path in error', () => { + try { + loadInstruction('test-instruction-schema', 'nonexistent.md', tempDir); + expect.fail('Should have thrown'); + } catch (err) { + expect(err).toBeInstanceOf(InstructionLoadError); + expect((err as InstructionLoadError).instructionPath).toContain('nonexistent.md'); + } + }); + }); + describe('loadChangeContext', () => { let tempDir: string; @@ -505,6 +586,110 @@ rules: expect(consoleWarnSpy).not.toHaveBeenCalled(); }); }); + + describe('instructionFile resolution', () => { + let testDir: string; + + beforeEach(() => { + testDir = fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-test-')); + + // Create a project-local test schema with instructionFile + const schemasDir = path.join(testDir, 'openspec', 'schemas', 'test-if-schema'); + const instructionsDir = path.join(schemasDir, 'instructions'); + const templatesDir = path.join(schemasDir, 'templates'); + + fs.mkdirSync(instructionsDir, { recursive: true }); + fs.mkdirSync(templatesDir, { recursive: true }); + + // Write schema.yaml with instructionFile + fs.writeFileSync( + path.join(schemasDir, 'schema.yaml'), + `name: test-if-schema +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal artifact + template: proposal.md + instructionFile: proposal.md + requires: [] + - id: design + generates: design.md + description: Design artifact + template: design.md + instruction: | + Create the design document (inline). + requires: + - proposal +` + ); + + // Write instruction file for proposal (uses instructionFile) + fs.writeFileSync( + path.join(instructionsDir, 'proposal.md'), + 'Create the proposal document (from file).' + ); + + // Write template files + fs.writeFileSync( + path.join(templatesDir, 'proposal.md'), + '## Why\n\n## What Changes' + ); + fs.writeFileSync( + path.join(templatesDir, 'design.md'), + '## Context\n\n## Goals / Non-Goals' + ); + }); + + afterEach(() => { + fs.rmSync(testDir, { recursive: true, force: true }); + }); + + it('should resolve instructionFile content for artifact', () => { + const context = loadChangeContext(testDir, 'my-change', 'test-if-schema'); + const instructions = generateInstructions(context, 'proposal', testDir); + + // instruction should come from the external file + expect(instructions.instruction).toContain('Create the proposal document (from file)'); + }); + + it('should fall back to inline instruction when instructionFile absent', () => { + const context = loadChangeContext(testDir, 'my-change', 'test-if-schema'); + const instructions = generateInstructions(context, 'design', testDir); + + // design uses inline instruction, no instructionFile + expect(instructions.instruction).toContain('Create the design document (inline)'); + }); + + it('should return undefined instruction when neither field present', () => { + // Create a schema without instruction or instructionFile for an artifact + const schemasDir2 = path.join(testDir, 'openspec', 'schemas', 'test-no-instr'); + const templatesDir2 = path.join(schemasDir2, 'templates'); + fs.mkdirSync(templatesDir2, { recursive: true }); + + fs.writeFileSync( + path.join(schemasDir2, 'schema.yaml'), + `name: test-no-instr +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal without instruction + template: proposal.md + requires: [] +` + ); + fs.writeFileSync( + path.join(templatesDir2, 'proposal.md'), + '## Why' + ); + + const context = loadChangeContext(testDir, 'my-change', 'test-no-instr'); + const instructions = generateInstructions(context, 'proposal', testDir); + + expect(instructions.instruction).toBeUndefined(); + }); + }); }); describe('formatChangeStatus', () => { diff --git a/test/core/artifact-graph/schema.test.ts b/test/core/artifact-graph/schema.test.ts index 069216a3a..d3902f1cf 100644 --- a/test/core/artifact-graph/schema.test.ts +++ b/test/core/artifact-graph/schema.test.ts @@ -203,5 +203,107 @@ artifacts: const schema = parseSchema(yaml); expect(schema.artifacts[0].requires).toEqual([]); }); + + it('should accept instructionFile as optional field', () => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: templates/proposal.md + instructionFile: instructions/proposal.md +`; + const schema = parseSchema(yaml); + expect(schema.artifacts[0].instructionFile).toBe('instructions/proposal.md'); + expect(schema.artifacts[0].instruction).toBeUndefined(); + }); + + it('should accept instruction as optional field (backward compat)', () => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: templates/proposal.md + instruction: | + Create the proposal document. +`; + const schema = parseSchema(yaml); + expect(schema.artifacts[0].instruction).toContain('Create the proposal document'); + expect(schema.artifacts[0].instructionFile).toBeUndefined(); + }); + + it('should accept neither instruction nor instructionFile', () => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: templates/proposal.md +`; + const schema = parseSchema(yaml); + expect(schema.artifacts[0].instruction).toBeUndefined(); + expect(schema.artifacts[0].instructionFile).toBeUndefined(); + }); + + it('should throw when artifact has both instruction and instructionFile', () => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: templates/proposal.md + instruction: Create the proposal + instructionFile: instructions/proposal.md +`; + expect(() => parseSchema(yaml)).toThrow(SchemaValidationError); + expect(() => parseSchema(yaml)).toThrow(/cannot have both 'instruction' and 'instructionFile'/); + }); + + it('should throw when apply phase has both instruction and instructionFile', () => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: templates/proposal.md +apply: + requires: [proposal] + tracks: tasks.md + instruction: Implement the tasks + instructionFile: instructions/apply.md +`; + expect(() => parseSchema(yaml)).toThrow(SchemaValidationError); + expect(() => parseSchema(yaml)).toThrow(/Apply phase cannot have both 'instruction' and 'instructionFile'/); + }); + + it('should accept apply instructionFile alone', () => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: templates/proposal.md +apply: + requires: [proposal] + tracks: tasks.md + instructionFile: instructions/apply.md +`; + const schema = parseSchema(yaml); + expect(schema.apply?.instructionFile).toBe('instructions/apply.md'); + expect(schema.apply?.instruction).toBeUndefined(); + }); }); });