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(); + }); }); });