From 9fa0986617e4856f32eb994838e3d79d0ea37d14 Mon Sep 17 00:00:00 2001 From: Ryan de Melo Date: Sat, 22 Aug 2026 10:25:52 +0800 Subject: [PATCH 1/2] feat(validate): report the deltas archive would refuse validate checked a change's deltas against themselves and, for MODIFIED blocks, against the main spec's scenarios. It never checked whether the main spec can supply the target a delta acts on, so a MODIFIED naming a requirement that is not there, a RENAMED whose source is gone, or an ADDED whose name already exists all validated clean and failed at archive instead - typically weeks later, after the implementing PR had shipped and the authoring session was gone. Run the merge archive runs and report what it refuses. buildUpdatedSpec returns the rebuilt content without writing it, so the preflight is the same function on the same inputs with the result discarded, and cannot disagree with the code that does the writing. That matters here: several of those preconditions deliberately read a missing target as already-synced rather than as a failure, and a second copy of the rules would be free to drift. Reported as INFO so no verdict changes in any mode. A MODIFIED whose target is missing is also what a change modifying a sibling's unarchived requirement looks like, and validate stays valid for that case today; telling the two apart needs the opt-in marker #1112 asks for. What is missing until then is the information, not the verdict. Refs #1112 --- docs-lab/reference/cli.md | 8 + src/core/validation/validator.ts | 84 ++++++++ .../core/validation.archive-preflight.test.ts | 184 ++++++++++++++++++ 3 files changed, 276 insertions(+) create mode 100644 test/core/validation.archive-preflight.test.ts diff --git a/docs-lab/reference/cli.md b/docs-lab/reference/cli.md index d8af89fc6a..5133ee7340 100644 --- a/docs-lab/reference/cli.md +++ b/docs-lab/reference/cli.md @@ -657,6 +657,14 @@ One line per item. Bulk runs end with totals: Totals: 2 passed, 0 failed (2 items) ``` +When a change's deltas are validated against the main specs, validate also runs the merge archive would run and reports anything that merge refuses — a `MODIFIED` naming a requirement the main spec does not have, a `RENAMED` whose source is gone, an `ADDED` whose name already exists. Without this the delta only fails at `openspec archive`, often weeks after the implementing PR shipped. + +These are reported as `INFO` and never change the exit code, including under `--strict`. The same shape has two causes: a mistyped header, and a change modifying a requirement a sibling change introduced but has not archived yet. The second becomes applicable the moment that sibling lands, so the report tells you the collision exists and leaves the verdict to you. + +``` +ℹ [INFO] api/spec.md: Archive would refuse this delta: api MODIFIED failed for header "### Requirement: Rate limiting" - not found +``` + A failing item lists each issue and the fix: ``` diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 56f771e1a8..a30b862c3c 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -5,6 +5,7 @@ import { SpecSchema, ChangeSchema, Spec, Change } from '../schemas/index.js'; import { MarkdownParser } from '../parsers/markdown-parser.js'; import { ChangeParser } from '../parsers/change-parser.js'; import { ValidationReport, ValidationIssue, ValidationLevel } from './types.js'; +import { findSpecUpdates, buildUpdatedSpec } from '../specs-apply.js'; import { MIN_PURPOSE_LENGTH, MAX_REQUIREMENT_TEXT_LENGTH, @@ -394,6 +395,22 @@ export class Validator { } } } + + // Everything above checks the delta against itself. Archive additionally + // refuses a delta the main spec cannot supply a target for - a MODIFIED + // naming a requirement that is not there, a RENAMED whose source is + // gone, an ADDED whose name already exists - and validate has never + // looked at any of those, so they surface at archive time, typically + // weeks after the implementing PR shipped (#1112). + if (options.mainSpecsDir) { + issues.push( + ...(await this.findArchiveBlockers( + changeDir, + options.mainSpecsDir, + new Set(issues.filter((issue) => issue.level === 'ERROR').map((issue) => issue.path)) + )) + ); + } } catch (error) { // A missing specs dir (or a stray `specs` file) means no deltas; // anything else (EACCES, EIO) must stay loud — discoverSpecFiles @@ -765,6 +782,73 @@ export class Validator { return dotIndex > 0 ? fileName.slice(0, dotIndex) : fileName; } + /** + * What archive would refuse when it merges this change's deltas into the + * main specs, found by running that merge in memory and throwing the result + * away. + * + * The preconditions are not restated here on purpose. `buildUpdatedSpec` is + * the code that does the writing, and several of its rules deliberately read + * a missing target as already-synced rather than as a failure (a RENAMED + * whose source is gone but whose target is present, for one). A second model + * of those rules would be free to disagree with the one that decides, and a + * preflight that reports a change archive accepts is worse than no + * preflight. So this calls it: same function, same inputs archive gives it, + * `rebuilt` discarded. It writes nothing. + * + * Reported as INFO, so it never changes a verdict in any mode, because the + * same shape has two causes. A MODIFIED whose target is missing is a real + * blocker when the header is a typo, but it is also what a change looks like + * when it modifies a requirement a sibling change introduced and has not + * archived yet - and that one becomes applicable the moment the sibling + * lands. `validate` deliberately stays valid for that case today, and + * deciding which of the two a delta is needs the opt-in marker #1112 asks + * for, not a guess made here. What is missing until then is not a verdict, + * it is the information: the author cannot currently see the collision at + * all until archive refuses it weeks later. This says it, and leaves the + * verdict alone. + */ + private async findArchiveBlockers( + changeDir: string, + mainSpecsDir: string, + alreadyReported: Set + ): Promise { + // Only ever reaches a generated skeleton's placeholder Purpose, which this + // dry run discards. + const changeName = path.basename(changeDir); + const issues: ValidationIssue[] = []; + + for (const update of await findSpecUpdates(changeDir, mainSpecsDir)) { + // discoverSpecFiles builds both this id and the entryPath the checks + // above report under, from the same walk. + const entryPath = `${update.id}/spec.md`; + // A delta those checks already rejected would be reported twice, the + // second time in archive's wording rather than the wording that names + // the actual mistake. + if (alreadyReported.has(entryPath)) continue; + + try { + await buildUpdatedSpec(update, changeName, { silent: true }); + } catch (error) { + // Only the thrown preconditions, which carry no errno. A filesystem + // error says nothing about whether the delta applies, and `validate + // --all` reads six changes at once, so a transient EMFILE would report + // a collision that is not there - the same reason the scenario-loss + // check above reads only the codes that mean the file is unusable. + if ((error as NodeJS.ErrnoException)?.code !== undefined) continue; + issues.push({ + level: 'INFO', + path: entryPath, + message: `Archive would refuse this delta: ${ + error instanceof Error ? error.message : String(error) + }`, + }); + } + } + + return issues; + } + private createReport(issues: ValidationIssue[]): ValidationReport { const errors = issues.filter(i => i.level === 'ERROR').length; const warnings = issues.filter(i => i.level === 'WARNING').length; diff --git a/test/core/validation.archive-preflight.test.ts b/test/core/validation.archive-preflight.test.ts new file mode 100644 index 0000000000..a599e71af6 --- /dev/null +++ b/test/core/validation.archive-preflight.test.ts @@ -0,0 +1,184 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; +import { Validator } from '../../src/core/validation/validator.js'; +import { buildUpdatedSpec, findSpecUpdates } from '../../src/core/specs-apply.js'; + +/** + * validate reports the deltas archive would refuse to apply (#1112). + * + * Every test here asserts parity in both directions: a warning validate emits + * must correspond to an error archive actually throws, and a change archive + * accepts must produce no warning. Reporting a change that archives cleanly + * would send an author to rewrite working work, which is worse than the gap + * this closes. + */ +describe('validate: deltas archive would refuse (#1112)', () => { + let testDir: string; + let changesDir: string; + let mainSpecsDir: string; + + const REQUIREMENT = `### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Existing scenario\n- **WHEN** queried\n- **THEN** the state is reported`; + + const mainSpec = (body: string) => + `# widgets Specification\n\n## Purpose\nDefine widget behavior for these tests.\n\n## Requirements\n\n${body}\n`; + + const writeMainSpec = async (id: string, body: string) => { + const file = path.join(mainSpecsDir, ...id.split('/'), 'spec.md'); + await fs.mkdir(path.dirname(file), { recursive: true }); + await fs.writeFile(file, mainSpec(body)); + }; + + const writeChange = async (changeName: string, specId: string, delta: string) => { + const changeDir = path.join(changesDir, changeName); + const specDir = path.join(changeDir, 'specs', ...specId.split('/')); + await fs.mkdir(specDir, { recursive: true }); + await fs.writeFile(path.join(specDir, 'spec.md'), delta); + return changeDir; + }; + + const validate = (changeDir: string, strict = false) => + new Validator(strict).validateChangeDeltaSpecs(changeDir, { mainSpecsDir }); + + /** The preflight warning, so assertions cannot pass on an unrelated issue. */ + const blocker = (report: { issues: Array<{ level: string; message: string }> }) => + report.issues.find((i) => i.message.startsWith('Archive would refuse this delta:')); + + /** What archive does with the same change: null when it applies cleanly. */ + const archiveError = async (changeDir: string): Promise => { + for (const update of await findSpecUpdates(changeDir, mainSpecsDir)) { + try { + await buildUpdatedSpec(update, path.basename(changeDir), { silent: true }); + } catch (error) { + return error instanceof Error ? error.message : String(error); + } + } + return null; + }; + + beforeEach(async () => { + testDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-preflight-')); + changesDir = path.join(testDir, 'openspec', 'changes'); + mainSpecsDir = path.join(testDir, 'openspec', 'specs'); + await fs.mkdir(changesDir, { recursive: true }); + await fs.mkdir(mainSpecsDir, { recursive: true }); + }); + + afterEach(async () => { + await fs.rm(testDir, { recursive: true, force: true }); + }); + + it('reports a MODIFIED naming a requirement the main spec does not have', async () => { + await writeMainSpec('widgets', REQUIREMENT); + const changeDir = await writeChange( + 'c1', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Gadget state\nThe system SHALL report the gadget state.\n\n#### Scenario: Queried\n- **WHEN** queried\n- **THEN** reported\n` + ); + + const issue = blocker(await validate(changeDir)); + expect(issue?.message).toContain('MODIFIED failed for header "### Requirement: Gadget state"'); + expect(await archiveError(changeDir)).not.toBeNull(); + }); + + it('reports an ADDED whose requirement already exists in the main spec', async () => { + await writeMainSpec('widgets', REQUIREMENT); + const changeDir = await writeChange( + 'c1', + 'widgets', + `## ADDED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state twice.\n\n#### Scenario: Queried\n- **WHEN** queried\n- **THEN** reported\n` + ); + + expect(blocker(await validate(changeDir))?.message).toContain('already exists'); + expect(await archiveError(changeDir)).not.toBeNull(); + }); + + it('reports a RENAMED whose source is not in the main spec', async () => { + await writeMainSpec('widgets', REQUIREMENT); + const changeDir = await writeChange( + 'c1', + 'widgets', + `## RENAMED Requirements\n\n- FROM: \`### Requirement: Gadget state\`\n- TO: \`### Requirement: Doodad state\`\n` + ); + + expect(blocker(await validate(changeDir))?.message).toContain('source not found'); + expect(await archiveError(changeDir)).not.toBeNull(); + }); + + it('stays silent on a delta that applies cleanly', async () => { + await writeMainSpec('widgets', REQUIREMENT); + const changeDir = await writeChange( + 'c1', + 'widgets', + `## ADDED Requirements\n\n### Requirement: Gadget state\nThe system SHALL report the gadget state.\n\n#### Scenario: Queried\n- **WHEN** queried\n- **THEN** reported\n` + ); + + expect(blocker(await validate(changeDir))).toBeUndefined(); + expect(await archiveError(changeDir)).toBeNull(); + }); + + it('stays silent on a rename the baseline already absorbed', async () => { + // Source gone, target present: specs-apply reads this as an early-synced + // rename and applies it as a no-op. A preflight with its own copy of the + // rules would call it a missing source and fail a change that archives. + await writeMainSpec('widgets', REQUIREMENT.replace('Widget state', 'Doodad state')); + const changeDir = await writeChange( + 'c1', + 'widgets', + `## RENAMED Requirements\n\n- FROM: \`### Requirement: Widget state\`\n- TO: \`### Requirement: Doodad state\`\n` + ); + + expect(blocker(await validate(changeDir))).toBeUndefined(); + expect(await archiveError(changeDir)).toBeNull(); + }); + + it('stays silent when the capability is new, so there is nothing to apply against', async () => { + const changeDir = await writeChange( + 'c1', + 'gizmos', + `## ADDED Requirements\n\n### Requirement: Gizmo state\nThe system SHALL report the gizmo state.\n\n#### Scenario: Queried\n- **WHEN** queried\n- **THEN** reported\n` + ); + + expect(blocker(await validate(changeDir))).toBeUndefined(); + expect(await archiveError(changeDir)).toBeNull(); + }); + + it('reports without changing the verdict, in strict mode too', async () => { + await writeMainSpec('widgets', REQUIREMENT); + const changeDir = await writeChange( + 'c1', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Gadget state\nThe system SHALL report the gadget state.\n\n#### Scenario: Queried\n- **WHEN** queried\n- **THEN** reported\n` + ); + + // The same shape is a typo'd header and a change modifying a sibling's + // unarchived requirement, and validate stays valid for the second one + // today. Telling the two apart needs the opt-in marker #1112 asks for, so + // this reports the collision and leaves the verdict where it was. + for (const strict of [false, true]) { + const report = await validate(changeDir, strict); + expect(report.valid).toBe(true); + expect(blocker(report)?.level).toBe('INFO'); + } + }); + + it('does not restate a failure the delta checks already named', async () => { + // The scenario-loss check reports this one in wording that names the + // dropped scenario; buildUpdatedSpec throws on it too, a few steps later. + await writeMainSpec( + 'widgets', + `${REQUIREMENT}\n\n#### Scenario: Second scenario\n- **WHEN** idle\n- **THEN** idle is reported` + ); + const changeDir = await writeChange( + 'c1', + 'widgets', + `## MODIFIED Requirements\n\n### Requirement: Widget state\nThe system SHALL report the widget state.\n\n#### Scenario: Existing scenario\n- **WHEN** queried\n- **THEN** the state is reported\n` + ); + + const report = await validate(changeDir); + expect(report.issues.some((i) => i.level === 'ERROR')).toBe(true); + expect(blocker(report)).toBeUndefined(); + expect(await archiveError(changeDir)).not.toBeNull(); + }); +}); From 8e243574190858d1bc45b9c991071ac87425e48e Mon Sep 17 00:00:00 2001 From: Ryan de Melo Date: Sat, 22 Aug 2026 10:38:52 +0800 Subject: [PATCH 2/2] fix(validate): skip preflight for deltas whose errors come after the loop missingHeaderSpecs and emptySectionSpecs are collected inside the per-spec loop but only become issues after it, so a suppression set built from the issues raised so far could not see them. A headerless or empty-section delta has nothing for the merge to apply, so the preflight reported that as a blocker of its own, on top of the error that names the actual mistake. --- docs-lab/reference/cli.md | 2 +- src/core/validation/validator.ts | 18 ++++++++++----- .../core/validation.archive-preflight.test.ts | 22 +++++++++++++++++++ 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/docs-lab/reference/cli.md b/docs-lab/reference/cli.md index 5133ee7340..2d7600d674 100644 --- a/docs-lab/reference/cli.md +++ b/docs-lab/reference/cli.md @@ -661,7 +661,7 @@ When a change's deltas are validated against the main specs, validate also runs These are reported as `INFO` and never change the exit code, including under `--strict`. The same shape has two causes: a mistyped header, and a change modifying a requirement a sibling change introduced but has not archived yet. The second becomes applicable the moment that sibling lands, so the report tells you the collision exists and leaves the verdict to you. -``` +```text ℹ [INFO] api/spec.md: Archive would refuse this delta: api MODIFIED failed for header "### Requirement: Rate limiting" - not found ``` diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index a30b862c3c..fe8fe69eff 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -404,11 +404,16 @@ export class Validator { // weeks after the implementing PR shipped (#1112). if (options.mainSpecsDir) { issues.push( - ...(await this.findArchiveBlockers( - changeDir, - options.mainSpecsDir, - new Set(issues.filter((issue) => issue.level === 'ERROR').map((issue) => issue.path)) - )) + ...(await this.findArchiveBlockers(changeDir, options.mainSpecsDir, [ + ...issues.filter((issue) => issue.level === 'ERROR').map((issue) => issue.path), + // Collected in the loop above but not turned into issues until + // after this try block, so they are invisible to the filter. A + // delta with no parsed sections has nothing for the merge to + // apply, which it reports as a failure of its own - on top of the + // error that actually names the mistake. + ...missingHeaderSpecs, + ...emptySectionSpecs.map((spec) => spec.path), + ])) ); } } catch (error) { @@ -811,8 +816,9 @@ export class Validator { private async findArchiveBlockers( changeDir: string, mainSpecsDir: string, - alreadyReported: Set + alreadyReportedPaths: string[] ): Promise { + const alreadyReported = new Set(alreadyReportedPaths); // Only ever reaches a generated skeleton's placeholder Purpose, which this // dry run discards. const changeName = path.basename(changeDir); diff --git a/test/core/validation.archive-preflight.test.ts b/test/core/validation.archive-preflight.test.ts index a599e71af6..e35e960dfc 100644 --- a/test/core/validation.archive-preflight.test.ts +++ b/test/core/validation.archive-preflight.test.ts @@ -163,6 +163,28 @@ describe('validate: deltas archive would refuse (#1112)', () => { } }); + it('does not restate a delta with no parsed sections, reported after the loop', async () => { + // missingHeaderSpecs / emptySectionSpecs are collected inside the loop but + // their errors are pushed after it, so a preflight keyed on issues raised + // so far would not see them and would add a second finding for a file the + // validator is about to name properly. + await writeMainSpec('widgets', REQUIREMENT); + const changeDir = await writeChange('c1', 'widgets', '# notes\n\nNo delta headers here.\n'); + + const report = await validate(changeDir); + expect(report.issues.some((i) => i.message.startsWith('No delta sections found'))).toBe(true); + expect(blocker(report)).toBeUndefined(); + }); + + it('does not restate a section that parsed no requirement entries', async () => { + await writeMainSpec('widgets', REQUIREMENT); + const changeDir = await writeChange('c1', 'widgets', '## ADDED Requirements\n\nNothing here.\n'); + + const report = await validate(changeDir); + expect(report.issues.some((i) => i.message.includes('no requirement entries parsed'))).toBe(true); + expect(blocker(report)).toBeUndefined(); + }); + it('does not restate a failure the delta checks already named', async () => { // The scenario-loss check reports this one in wording that names the // dropped scenario; buildUpdatedSpec throws on it too, a few steps later.