From 71f1b6a9addf574a4f30b54f07881f1cf8bc74c8 Mon Sep 17 00:00:00 2001 From: Aimen Khalid Date: Tue, 18 Aug 2026 16:17:42 +0000 Subject: [PATCH 1/3] fix(config): scope context/rules to a specific schema config.yaml's context and rules fields are global across every schema registered in a project. When a project has multiple schemas (e.g. the built-in spec-driven plus one forked via `schema fork`/`schema init`) that happen to share an artifact id - proposal is part of the default workflow shape, so this is common - a rule or note meant for only one schema leaks into the others, with no way to scope it. Add an optional schemas. block in config.yaml that layers schema-specific context/rules on top of the existing project-wide ones: schemas: my-workflow: context: ... rules: proposal: [...] Generating instructions for an artifact now resolves context/rules through resolveEffectiveContext()/resolveEffectiveRules(), which combine the global value with the schemas. override for the active schema only. The flat top-level context/rules fields keep applying to every schema exactly as before, so existing config.yaml files are unaffected. Also extends validateConfigRules() with an optional schemaName parameter so schema-scoped rules are checked against just that schema's own artifacts (more precise than the existing global, cross-schema union check). Found while using multiple custom schemas in one project; see #1694. --- docs/cli.md | 4 +- docs/customization.md | 36 +++ src/core/artifact-graph/instruction-loader.ts | 53 +++- src/core/project-config.ts | 194 ++++++++++++- .../artifact-graph/instruction-loader.test.ts | 157 +++++++++++ test/core/project-config.test.ts | 262 ++++++++++++++++++ 6 files changed, 682 insertions(+), 24 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index d17c6d662f..50e234e3fd 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -834,9 +834,9 @@ openspec instructions design --change add-dark-mode --json **Output includes:** - Template content for the artifact -- Project context from config +- Project context from config, plus any `schemas..context` override for the resolved schema (see [Customization](customization.md#project-configuration)) - Content from dependency artifacts -- Per-artifact rules from config +- Per-artifact rules from config, plus any matching `schemas..rules` override for the resolved schema - Current project context and matching operation guidance for `apply`/`archive` Operation inputs are read from the resolved repo or selected store on every diff --git a/docs/customization.md b/docs/customization.md index b1143b9276..065207c65f 100644 --- a/docs/customization.md +++ b/docs/customization.md @@ -95,6 +95,42 @@ Tech stack: TypeScript, React, Node.js, PostgreSQL - **Context** appears in ALL artifacts - **Rules** ONLY appear for the matching artifact +**Scoping context/rules to one schema:** + +The top-level `context` and `rules` above apply to every schema in the +project - useful, but a problem if you register more than one schema (say, +the built-in `spec-driven` plus a schema forked for a different team) and +want a rule or note to apply to only one of them. Add an optional `schemas` +block, keyed by schema name, to layer schema-specific context and rules on +top of the project-wide ones: + +```yaml +# openspec/config.yaml +schema: spec-driven + +context: | + Tech stack: TypeScript, React, Node.js, PostgreSQL + +rules: + proposal: + - Include rollback plan + +schemas: + my-workflow: + context: | + This schema is for the payments team; flag PCI-scope changes explicitly. + rules: + proposal: + - Tag the proposal with the affected payment provider +``` + +Generating instructions for `my-workflow`'s `proposal` artifact gets the +project-wide context and rule above **plus** the `my-workflow`-only context +and rule; generating instructions for `spec-driven`'s `proposal` artifact +(or any other schema) only gets the project-wide ones. A schema with no +entry under `schemas` is unaffected - existing `config.yaml` files that only +use the flat `context`/`rules` fields keep behaving exactly as before. + **Operation guidance:** `operations.apply.guidance` and `operations.archive.guidance` are optional arrays diff --git a/src/core/artifact-graph/instruction-loader.ts b/src/core/artifact-graph/instruction-loader.ts index 3f12670016..e3368c7ae9 100644 --- a/src/core/artifact-graph/instruction-loader.ts +++ b/src/core/artifact-graph/instruction-loader.ts @@ -13,7 +13,13 @@ import { type ActionContext, type PlanningHomeSummary, } from '../change-status-policy.js'; -import { readProjectConfig, validateConfigRules, type ProjectConfig } from '../project-config.js'; +import { + readProjectConfig, + validateConfigRules, + resolveEffectiveContext, + resolveEffectiveRules, + type ProjectConfig, +} from '../project-config.js'; import type { ReferenceIndexEntry } from '../references.js'; import type { PlanningHome } from '../planning-home.js'; import type { ChangeMetadata } from '../change-metadata/index.js'; @@ -360,13 +366,31 @@ export function generateInstructions( } // Validate rules artifact IDs if config has rules (only once per session). - // The rules map is global while each change can use a different schema, so a - // key is only "unknown" when it matches no artifact in ANY available schema. - if (projectConfig?.rules) { - const validArtifactIds = new Set( - listSchemasWithInfo(effectiveProjectRoot ?? undefined).flatMap((s) => s.artifacts) - ); - const warnings = validateConfigRules(projectConfig.rules, validArtifactIds); + // The global rules map applies while each change can use a different + // schema, so a key is only "unknown" when it matches no artifact in ANY + // available schema. Schema-scoped `schemas..rules` are checked more + // precisely, against just that schema's own artifacts, since they only + // ever apply there. + if (projectConfig?.rules || projectConfig?.schemas) { + const schemasWithInfo = listSchemasWithInfo(effectiveProjectRoot ?? undefined); + const warnings: string[] = []; + + if (projectConfig.rules) { + const validArtifactIds = new Set(schemasWithInfo.flatMap((s) => s.artifacts)); + warnings.push(...validateConfigRules(projectConfig.rules, validArtifactIds)); + } + + if (projectConfig.schemas) { + const artifactIdsBySchema = new Map( + schemasWithInfo.map((s) => [s.name, new Set(s.artifacts)]) + ); + for (const [scopedSchemaName, scoped] of Object.entries(projectConfig.schemas)) { + const scopedValidIds = artifactIdsBySchema.get(scopedSchemaName); + if (scoped.rules && scopedValidIds) { + warnings.push(...validateConfigRules(scoped.rules, scopedValidIds, scopedSchemaName)); + } + } + } // Show each unique warning only once per session for (const warning of warnings) { @@ -377,13 +401,12 @@ export function generateInstructions( } } - // Extract context and rules as separate fields (not prepended to template) - const configContext = projectConfig?.context?.trim() || undefined; - const rulesForArtifact = - projectConfig?.rules && Object.hasOwn(projectConfig.rules, artifactId) - ? projectConfig.rules[artifactId] - : undefined; - const configRules = rulesForArtifact && rulesForArtifact.length > 0 ? rulesForArtifact : undefined; + // Extract context and rules as separate fields (not prepended to template). + // Both layer the schema-scoped `schemas.` override (if any) on + // top of the project-level value, so single-schema projects that only use + // the flat `context`/`rules` fields keep behaving exactly as before. + const configContext = resolveEffectiveContext(projectConfig, context.schemaName); + const configRules = resolveEffectiveRules(projectConfig, context.schemaName, artifactId); return { changeName: context.changeName, diff --git a/src/core/project-config.ts b/src/core/project-config.ts index 922e31505b..c8af6aed46 100644 --- a/src/core/project-config.ts +++ b/src/core/project-config.ts @@ -52,6 +52,22 @@ export const ProjectConfigSchema = z.object({ .optional() .describe('Per-artifact rules, keyed by artifact ID'), + // Optional: schema-scoped context/rules overrides, keyed by schema name. + // A project can register multiple schemas (built-in plus forked/local + // ones); the `context`/`rules` fields above apply to all of them, and this + // is the only way to add context or rules that apply to just one schema. + // Layered on top of (additive to) the fields above, never a replacement. + schemas: z + .record( + z.string(), // schema name + z.object({ + context: z.string().optional(), + rules: z.record(z.string(), z.array(z.string())).optional(), + }) + ) + .optional() + .describe('Schema-scoped context/rules overrides, keyed by schema name'), + // Optional: per-operation advisory guidance, kept separate from artifact rules. operations: z .object({ @@ -92,6 +108,12 @@ export interface DeclarationEntry { remote?: string; } +/** Schema-scoped `context`/`rules` override; see `schemas` on ProjectConfigSchema. */ +export interface ScopedProjectConfig { + context?: string; + rules?: Record; +} + export type ProjectConfig = z.infer & { references?: DeclarationEntry[]; }; @@ -180,6 +202,105 @@ function parseOperations(raw: unknown): OperationsConfig | undefined { return Object.keys(operations).length > 0 ? operations : undefined; } +/** + * Parser for `schemas:` overrides: an object keyed by schema name, each + * value optionally carrying its own `context` string and `rules` map. Mirrors + * the resilient, warn-and-drop parsing used for the top-level `context` and + * `rules` fields (including the null-prototype guard on rule keys), just + * applied once per schema name. + */ +function parseScopedSchemas(raw: unknown): Record | undefined { + if (raw === undefined) { + return undefined; + } + if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) { + console.warn(`Invalid 'schemas' field in config (must be object)`); + return undefined; + } + + const parsedSchemas: Record = Object.create(null); + let hasValidSchemas = false; + + for (const [schemaName, value] of Object.entries(raw)) { + if (!value || typeof value !== 'object' || Array.isArray(value)) { + console.warn( + `Invalid 'schemas.${schemaName}' field in config (must be object), ignoring this schema's overrides` + ); + continue; + } + + const scopedRaw = value as Record; + const unknownFields = Object.keys(scopedRaw).filter( + (field) => field !== 'context' && field !== 'rules' + ); + if (unknownFields.length > 0) { + console.warn( + `Unknown field(s) in 'schemas.${schemaName}': ${unknownFields.join(', ')}. Supported fields: context, rules` + ); + } + + const scoped: ScopedProjectConfig = {}; + + if (scopedRaw.context !== undefined) { + const contextResult = z.string().safeParse(scopedRaw.context); + if (contextResult.success) { + const contextSize = Buffer.byteLength(contextResult.data, 'utf-8'); + if (contextSize > MAX_CONTEXT_SIZE) { + console.warn( + `Context too large in 'schemas.${schemaName}.context' (${(contextSize / 1024).toFixed(1)}KB, limit: ${MAX_CONTEXT_SIZE / 1024}KB)` + ); + console.warn(`Ignoring 'schemas.${schemaName}.context' field`); + } else { + scoped.context = contextResult.data; + } + } else { + console.warn(`Invalid 'schemas.${schemaName}.context' field in config (must be string)`); + } + } + + if (scopedRaw.rules !== undefined) { + if (typeof scopedRaw.rules === 'object' && scopedRaw.rules !== null && !Array.isArray(scopedRaw.rules)) { + const parsedRules: Record = Object.create(null); + let hasValidRules = false; + + for (const [artifactId, rules] of Object.entries(scopedRaw.rules)) { + const rulesArrayResult = z.array(z.string()).safeParse(rules); + + if (rulesArrayResult.success) { + const validRules = rulesArrayResult.data.filter((r) => r.length > 0); + if (validRules.length > 0) { + parsedRules[artifactId] = validRules; + hasValidRules = true; + } + if (validRules.length < rulesArrayResult.data.length) { + console.warn( + `Some rules for '${artifactId}' in 'schemas.${schemaName}.rules' are empty strings, ignoring them` + ); + } + } else { + console.warn( + `Rules for '${artifactId}' in 'schemas.${schemaName}.rules' must be an array of strings, ignoring this artifact's rules` + ); + } + } + + if (hasValidRules) { + scoped.rules = parsedRules; + } + } else { + console.warn(`Invalid 'schemas.${schemaName}.rules' field in config (must be object)`); + } + } + + if (scoped.context !== undefined || scoped.rules !== undefined) { + parsedSchemas[schemaName] = scoped; + hasValidSchemas = true; + } + } + + return hasValidSchemas ? parsedSchemas : undefined; +} + /** * Parser for `references:` declarations: string entries or * {id, remote} maps, normalized to DeclarationEntry[]. Dedup keys on @@ -353,6 +474,11 @@ export function readProjectConfig(projectRoot: string): ProjectConfig | null { } } + const schemas = parseScopedSchemas(raw.schemas); + if (schemas) { + config.schemas = schemas; + } + const operations = parseOperations(raw.operations); if (operations) { config.operations = operations; @@ -409,27 +535,38 @@ function configPathForWarnings(projectRoot: string): string { } /** - * Validate artifact IDs in rules against the artifacts of every available - * schema. The `rules:` map is global, but each change can use a different - * schema, so a key is only unknown when it matches no artifact in ANY schema. - * Returns warnings for keys that are unknown everywhere. + * Validate artifact IDs in rules against a set of valid artifact IDs. + * Returns warnings for keys that don't match. + * + * Called two ways: + * - Global `rules:` map: pass the union of artifact IDs across every + * available schema (a key is only unknown when it matches no artifact in + * ANY schema, since a change can use a different schema than the one + * active when the warning is produced). Omit `schemaName`. + * - Schema-scoped `schemas..rules:` map: pass just that schema's own + * artifact IDs and its name, since scoped rules are only ever applied + * there - an unknown key can be reported precisely. * * @param rules - The rules object from config - * @param validArtifactIds - Set of valid artifact IDs across all schemas + * @param validArtifactIds - Set of valid artifact IDs to check against + * @param schemaName - When set, the warning is worded for this one schema + * instead of "any available schema" * @returns Array of warning messages for unknown artifact IDs */ export function validateConfigRules( rules: Record, - validArtifactIds: Set + validArtifactIds: Set, + schemaName?: string ): string[] { const warnings: string[] = []; + const scopeLabel = schemaName ? `schema '${schemaName}'` : 'any available schema'; for (const artifactId of Object.keys(rules)) { if (!validArtifactIds.has(artifactId)) { const validIds = Array.from(validArtifactIds).sort().join(', '); warnings.push( `Unknown artifact ID in rules: "${artifactId}". ` + - `It matches no artifact in any available schema. Known artifact IDs: ${validIds}` + `It matches no artifact in ${scopeLabel}. Known artifact IDs: ${validIds}` ); } } @@ -437,6 +574,49 @@ export function validateConfigRules( return warnings; } +/** + * Effective project context for a schema: the project-level `context` + * followed by that schema's `schemas..context` override, if any. The + * global field keeps applying everywhere it already did - the override is + * additive, so existing single-schema projects are unaffected. + */ +export function resolveEffectiveContext( + projectConfig: ProjectConfig | null, + schemaName: string +): string | undefined { + const globalContext = projectConfig?.context?.trim() || undefined; + const scopedContext = projectConfig?.schemas?.[schemaName]?.context?.trim() || undefined; + + if (globalContext && scopedContext) { + return `${globalContext}\n\n${scopedContext}`; + } + return globalContext ?? scopedContext; +} + +/** + * Effective rules for one artifact under a schema: the project-level + * `rules.` entries followed by that schema's + * `schemas..rules.` entries, if any. Additive for the same + * reason as `resolveEffectiveContext` - the global map still means "applies + * to this artifact everywhere". + */ +export function resolveEffectiveRules( + projectConfig: ProjectConfig | null, + schemaName: string, + artifactId: string +): string[] | undefined { + const globalRules = + projectConfig?.rules && Object.hasOwn(projectConfig.rules, artifactId) + ? projectConfig.rules[artifactId] + : undefined; + const scopedRules = projectConfig?.schemas?.[schemaName]?.rules; + const scopedRulesForArtifact = + scopedRules && Object.hasOwn(scopedRules, artifactId) ? scopedRules[artifactId] : undefined; + + const combined = [...(globalRules ?? []), ...(scopedRulesForArtifact ?? [])]; + return combined.length > 0 ? combined : undefined; +} + /** * Suggest valid schema names when user provides invalid schema. * Uses fuzzy matching to find similar names. diff --git a/test/core/artifact-graph/instruction-loader.test.ts b/test/core/artifact-graph/instruction-loader.test.ts index ce3e153255..f255f6903a 100644 --- a/test/core/artifact-graph/instruction-loader.test.ts +++ b/test/core/artifact-graph/instruction-loader.test.ts @@ -532,6 +532,163 @@ rules: }); }); + describe('multi-schema projects (schema-scoped context/rules)', () => { + // A project-local schema that reuses the 'proposal' artifact id from + // the built-in spec-driven schema - the common case from the bug + // report, since 'proposal' is part of the default workflow shape. + function createCustomSchemaWithProposal(tempDir: string): void { + const schemaDir = path.join(tempDir, 'openspec', 'schemas', 'custom-schema'); + const templatesDir = path.join(schemaDir, 'templates'); + fs.mkdirSync(templatesDir, { recursive: true }); + fs.writeFileSync( + path.join(schemaDir, 'schema.yaml'), + `name: custom-schema +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Custom proposal + template: proposal.md +` + ); + fs.writeFileSync(path.join(templatesDir, 'proposal.md'), '# Custom Proposal Template\n'); + } + + it('reproduces the reported leak: a global rule meant for one schema also applies to the other', () => { + // This is the exact scenario from the bug report: two schemas share + // the 'proposal' artifact id, and a rule intended for only the + // custom schema is added to the flat, schema-agnostic `rules` map + // because there is no scoping mechanism. + createCustomSchemaWithProposal(tempDir); + const configDir = path.join(tempDir, 'openspec'); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +rules: + proposal: + - Meant only for custom-schema +` + ); + + const specDrivenContext = loadChangeContext(tempDir, 'my-change', 'spec-driven'); + const specDrivenInstructions = generateInstructions(specDrivenContext, 'proposal', tempDir); + + // Documents today's (intentionally unchanged) behavior: a bare + // `rules.proposal` entry is global and applies to every schema's + // 'proposal' artifact, spec-driven included, even though the author + // only meant it for custom-schema. + expect(specDrivenInstructions.rules).toEqual(['Meant only for custom-schema']); + }); + + it('scopes a rule to only the intended schema via schemas..rules, fixing the leak', () => { + createCustomSchemaWithProposal(tempDir); + const configDir = path.join(tempDir, 'openspec'); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + custom-schema: + rules: + proposal: + - Custom-schema-only rule +` + ); + + const specDrivenContext = loadChangeContext(tempDir, 'my-change', 'spec-driven'); + const specDrivenInstructions = generateInstructions(specDrivenContext, 'proposal', tempDir); + expect(specDrivenInstructions.rules).toBeUndefined(); + + const customContext = loadChangeContext(tempDir, 'my-change', 'custom-schema'); + const customInstructions = generateInstructions(customContext, 'proposal', tempDir); + expect(customInstructions.rules).toEqual(['Custom-schema-only rule']); + }); + + it('layers scoped rules on top of global rules for the same artifact', () => { + createCustomSchemaWithProposal(tempDir); + const configDir = path.join(tempDir, 'openspec'); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +rules: + proposal: + - Applies to every schema +schemas: + custom-schema: + rules: + proposal: + - Applies only to custom-schema +` + ); + + const specDrivenContext = loadChangeContext(tempDir, 'my-change', 'spec-driven'); + expect(generateInstructions(specDrivenContext, 'proposal', tempDir).rules).toEqual([ + 'Applies to every schema', + ]); + + const customContext = loadChangeContext(tempDir, 'my-change', 'custom-schema'); + expect(generateInstructions(customContext, 'proposal', tempDir).rules).toEqual([ + 'Applies to every schema', + 'Applies only to custom-schema', + ]); + }); + + it('layers scoped context on top of global context, scoped to just the intended schema', () => { + createCustomSchemaWithProposal(tempDir); + const configDir = path.join(tempDir, 'openspec'); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +context: Project-wide background +schemas: + custom-schema: + context: Custom-schema-only background +` + ); + + const specDrivenContext = loadChangeContext(tempDir, 'my-change', 'spec-driven'); + expect(generateInstructions(specDrivenContext, 'proposal', tempDir).context).toBe( + 'Project-wide background' + ); + + const customContext = loadChangeContext(tempDir, 'my-change', 'custom-schema'); + const customInstructions = generateInstructions(customContext, 'proposal', tempDir); + expect(customInstructions.context).toContain('Project-wide background'); + expect(customInstructions.context).toContain('Custom-schema-only background'); + }); + + it('validates schema-scoped rules against that schema\'s own artifacts, not the global union', () => { + createCustomSchemaWithProposal(tempDir); + const configDir = path.join(tempDir, 'openspec'); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + custom-schema: + rules: + unknown-in-custom-schema-only: + - Some rule +` + ); + const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + try { + const customContext = loadChangeContext(tempDir, 'my-change', 'custom-schema'); + generateInstructions(customContext, 'proposal', tempDir); + + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'Unknown artifact ID in rules: "unknown-in-custom-schema-only"' + ) + ); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("schema 'custom-schema'") + ); + } finally { + consoleWarnSpy.mockRestore(); + } + }); + }); + describe('validation and warnings', () => { let consoleWarnSpy: ReturnType; diff --git a/test/core/project-config.test.ts b/test/core/project-config.test.ts index 2adbdf9ad6..509f087fe3 100644 --- a/test/core/project-config.test.ts +++ b/test/core/project-config.test.ts @@ -8,6 +8,8 @@ import { readProjectConfig, validateConfigRules, suggestSchemas, + resolveEffectiveContext, + resolveEffectiveRules, } from '../../src/core/project-config.js'; describe('project-config', () => { @@ -832,6 +834,254 @@ rules: ]); }); }); + + describe('schemas field (schema-scoped context/rules overrides)', () => { + it('should parse per-schema context and rules, keyed by schema name', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + custom-schema: + context: Custom schema background + rules: + proposal: + - Custom schema rule +` + ); + + const config = readProjectConfig(tempDir); + + expect(config?.schemas).toEqual({ + 'custom-schema': { + context: 'Custom schema background', + rules: { proposal: ['Custom schema rule'] }, + }, + }); + expect(consoleWarnSpy).not.toHaveBeenCalled(); + }); + + it('should support multiple scoped schemas independently', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + schema-a: + rules: + proposal: + - Rule for schema-a + schema-b: + context: Background for schema-b +` + ); + + const config = readProjectConfig(tempDir); + + expect(config?.schemas).toEqual({ + 'schema-a': { rules: { proposal: ['Rule for schema-a'] } }, + 'schema-b': { context: 'Background for schema-b' }, + }); + }); + + it('should return undefined for schemas when absent', () => { + fs.mkdirSync(path.join(tempDir, 'openspec'), { recursive: true }); + fs.writeFileSync(path.join(tempDir, 'openspec', 'config.yaml'), 'schema: spec-driven\n'); + + expect(readProjectConfig(tempDir)?.schemas).toBeUndefined(); + }); + + it('should warn and drop schemas when not an object', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + 'schema: spec-driven\nschemas: ["not", "an", "object"]\n' + ); + + const config = readProjectConfig(tempDir); + + expect(config?.schemas).toBeUndefined(); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("Invalid 'schemas' field") + ); + }); + + it('should warn and drop a single schema entry that is not an object, keeping others', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + broken-schema: "not an object" + good-schema: + context: Fine +` + ); + + const config = readProjectConfig(tempDir); + + expect(config?.schemas).toEqual({ 'good-schema': { context: 'Fine' } }); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("Invalid 'schemas.broken-schema' field") + ); + }); + + it('should warn about unknown fields inside a schema override', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + custom-schema: + unknownField: nope + context: Fine +` + ); + + const config = readProjectConfig(tempDir); + + expect(config?.schemas?.['custom-schema']).toEqual({ context: 'Fine' }); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("Unknown field(s) in 'schemas.custom-schema': unknownField") + ); + }); + + it('should enforce the context size limit per scoped schema', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + const largeContext = 'a'.repeat(51 * 1024); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven\nschemas:\n custom-schema:\n context: "${largeContext}"\n` + ); + + const config = readProjectConfig(tempDir); + + expect(config?.schemas).toBeUndefined(); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("Context too large in 'schemas.custom-schema.context'") + ); + }); + + it('should filter out empty string rules within a scoped schema', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + custom-schema: + rules: + proposal: + - "" + - Real rule +` + ); + + const config = readProjectConfig(tempDir); + + expect(config?.schemas?.['custom-schema']?.rules).toEqual({ proposal: ['Real rule'] }); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("Some rules for 'proposal' in 'schemas.custom-schema.rules'") + ); + }); + + it('should preserve prototype-named schema names as inert data', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + __proto__: + context: Prototype schema context +` + ); + + const schemas = readProjectConfig(tempDir)?.schemas; + + expect(Object.getPrototypeOf(schemas)).toBeNull(); + expect(Object.hasOwn(schemas!, '__proto__')).toBe(true); + expect(schemas?.__proto__).toEqual({ context: 'Prototype schema context' }); + }); + }); + }); + + describe('resolveEffectiveContext', () => { + it('returns the global context when no schema override exists', () => { + const config = { schema: 'spec-driven', context: 'Global background' }; + expect(resolveEffectiveContext(config, 'spec-driven')).toBe('Global background'); + }); + + it('returns only the scoped context when no global context exists', () => { + const config = { + schema: 'spec-driven', + schemas: { 'custom-schema': { context: 'Scoped background' } }, + }; + expect(resolveEffectiveContext(config, 'custom-schema')).toBe('Scoped background'); + }); + + it('combines global and scoped context for the matching schema', () => { + const config = { + schema: 'spec-driven', + context: 'Global background', + schemas: { 'custom-schema': { context: 'Scoped background' } }, + }; + expect(resolveEffectiveContext(config, 'custom-schema')).toBe( + 'Global background\n\nScoped background' + ); + }); + + it('does not leak a scoped context into a different schema', () => { + const config = { + schema: 'spec-driven', + schemas: { 'custom-schema': { context: 'Scoped background' } }, + }; + expect(resolveEffectiveContext(config, 'spec-driven')).toBeUndefined(); + }); + + it('returns undefined when there is no context at all', () => { + expect(resolveEffectiveContext(null, 'spec-driven')).toBeUndefined(); + expect(resolveEffectiveContext({ schema: 'spec-driven' }, 'spec-driven')).toBeUndefined(); + }); + }); + + describe('resolveEffectiveRules', () => { + it('returns global rules for an artifact when no schema override exists', () => { + const config = { schema: 'spec-driven', rules: { proposal: ['Global rule'] } }; + expect(resolveEffectiveRules(config, 'spec-driven', 'proposal')).toEqual(['Global rule']); + }); + + it('does not leak a scoped rule into a different schema', () => { + const config = { + schema: 'spec-driven', + schemas: { 'custom-schema': { rules: { proposal: ['Scoped rule'] } } }, + }; + expect(resolveEffectiveRules(config, 'spec-driven', 'proposal')).toBeUndefined(); + expect(resolveEffectiveRules(config, 'custom-schema', 'proposal')).toEqual(['Scoped rule']); + }); + + it('appends scoped rules after global rules for the same artifact', () => { + const config = { + schema: 'spec-driven', + rules: { proposal: ['Global rule'] }, + schemas: { 'custom-schema': { rules: { proposal: ['Scoped rule'] } } }, + }; + expect(resolveEffectiveRules(config, 'custom-schema', 'proposal')).toEqual([ + 'Global rule', + 'Scoped rule', + ]); + }); + + it('returns undefined when neither global nor scoped rules exist for the artifact', () => { + const config = { schema: 'spec-driven', rules: { specs: ['Other artifact rule'] } }; + expect(resolveEffectiveRules(config, 'spec-driven', 'proposal')).toBeUndefined(); + }); }); describe('loadOperationInputs', () => { @@ -942,6 +1192,18 @@ rules: expect(warnings).toEqual([]); }); + + it('should word the warning for a specific schema when schemaName is passed', () => { + const rules = { 'unknown-artifact': ['Rule 1'] }; + const validIds = new Set(['proposal', 'specs']); + + const warnings = validateConfigRules(rules, validIds, 'custom-schema'); + + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('Unknown artifact ID in rules: "unknown-artifact"'); + expect(warnings[0]).toContain("schema 'custom-schema'"); + expect(warnings[0]).not.toContain('any available schema'); + }); }); describe('suggestSchemas', () => { From a6c98feb206e00661e394685a44b851f080dd1d2 Mon Sep 17 00:00:00 2001 From: Aimen Khalid Date: Tue, 18 Aug 2026 16:47:04 +0000 Subject: [PATCH 2/3] fix(config): warn on a schemas. entry that matches no schema A typo'd or stale schema name under `schemas.` in config.yaml silently never applies - resolveEffectiveContext()/resolveEffectiveRules() can only look it up by the currently active schema name, so a name that matches no registered schema is dead config with no feedback. Warn once per session when this happens, listing the known schema names, same as the existing "unknown artifact ID" warnings. Addresses CodeRabbit review feedback on this PR. --- src/core/artifact-graph/instruction-loader.ts | 13 ++++++- .../artifact-graph/instruction-loader.test.ts | 37 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/core/artifact-graph/instruction-loader.ts b/src/core/artifact-graph/instruction-loader.ts index e3368c7ae9..90f61c34d6 100644 --- a/src/core/artifact-graph/instruction-loader.ts +++ b/src/core/artifact-graph/instruction-loader.ts @@ -386,7 +386,18 @@ export function generateInstructions( ); for (const [scopedSchemaName, scoped] of Object.entries(projectConfig.schemas)) { const scopedValidIds = artifactIdsBySchema.get(scopedSchemaName); - if (scoped.rules && scopedValidIds) { + if (!scopedValidIds) { + // A typo'd or removed schema name here never matches any active + // schema, so its context/rules silently never apply - warn instead + // of leaving that dead config unexplained. + const knownSchemas = Array.from(artifactIdsBySchema.keys()).sort().join(', '); + warnings.push( + `Unknown schema '${scopedSchemaName}' in config 'schemas'. ` + + `It matches no available schema, so its context/rules overrides are ignored. Known schemas: ${knownSchemas}` + ); + continue; + } + if (scoped.rules) { warnings.push(...validateConfigRules(scoped.rules, scopedValidIds, scopedSchemaName)); } } diff --git a/test/core/artifact-graph/instruction-loader.test.ts b/test/core/artifact-graph/instruction-loader.test.ts index f255f6903a..6ff2c835a5 100644 --- a/test/core/artifact-graph/instruction-loader.test.ts +++ b/test/core/artifact-graph/instruction-loader.test.ts @@ -687,6 +687,43 @@ schemas: consoleWarnSpy.mockRestore(); } }); + + it('warns when a schemas. entry names no available schema, instead of silently dropping it', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `schema: spec-driven +schemas: + typo-ed-schema-name: + context: This never applies to anything + rules: + proposal: + - This never applies either +` + ); + const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + try { + const context = loadChangeContext(tempDir, 'my-change', 'spec-driven'); + const instructions = generateInstructions(context, 'proposal', tempDir); + + // The typo'd override matches no real schema, so it never affects + // the resolved instructions... + expect(instructions.context).toBeUndefined(); + expect(instructions.rules).toBeUndefined(); + + // ...but it must not fail silently - the user should learn why. + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining("Unknown schema 'typo-ed-schema-name' in config 'schemas'") + ); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining('Known schemas:') + ); + } finally { + consoleWarnSpy.mockRestore(); + } + }); }); describe('validation and warnings', () => { From 023fb2449f4f2416d45354167cebebfd4a2a1a4e Mon Sep 17 00:00:00 2001 From: Aimen Khalid Date: Tue, 18 Aug 2026 16:52:49 +0000 Subject: [PATCH 3/3] test(instruction-loader): assert the actual known schema name in the unknown-schema warning --- test/core/artifact-graph/instruction-loader.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/core/artifact-graph/instruction-loader.test.ts b/test/core/artifact-graph/instruction-loader.test.ts index 6ff2c835a5..7b56e78c14 100644 --- a/test/core/artifact-graph/instruction-loader.test.ts +++ b/test/core/artifact-graph/instruction-loader.test.ts @@ -713,12 +713,13 @@ schemas: expect(instructions.context).toBeUndefined(); expect(instructions.rules).toBeUndefined(); - // ...but it must not fail silently - the user should learn why. + // ...but it must not fail silently - the user should learn why, + // including which schema name they probably meant. expect(consoleWarnSpy).toHaveBeenCalledWith( expect.stringContaining("Unknown schema 'typo-ed-schema-name' in config 'schemas'") ); expect(consoleWarnSpy).toHaveBeenCalledWith( - expect.stringContaining('Known schemas:') + expect.stringContaining('Known schemas: spec-driven') ); } finally { consoleWarnSpy.mockRestore();