Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/bright-widgets-validate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@fission-ai/openspec": patch
---

Report delta merge conflicts during validation as informational findings, including in successful text reports, without changing validation exit codes. Preserve filesystem read errors so unreadable main specs are not mistaken for missing specs.
14 changes: 13 additions & 1 deletion docs-lab/reference/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -657,14 +657,26 @@ With no name and no bulk flag, validate prompts you to pick items. Outside an in

**Output**

One line per item. Bulk runs end with totals:
Bulk runs print one status line per item, followed by any findings, and end with totals:

```
✓ change/add-rate-limit
✓ spec/api
Totals: 2 passed, 0 failed (2 items)
```

**Archive merge findings**

For changes, validate runs archive's merge builder against the current main specs without writing files. It reports merge conflicts, such as a missing `MODIFIED` target or a conflicting `ADDED` requirement, as `INFO`:

```text
ℹ [INFO] api/spec.md: Archive would refuse this delta: api MODIFIED failed for header "### Requirement: Rate limiting" - not found
```

These findings appear even when validation passes, in both text and JSON output. `INFO` never changes the exit code, including under `--strict`: a missing target may belong to a sibling change that has not archived yet. Deltas already synced into the main specs follow archive's existing merge rules.

This check does not run archive's later merged-spec validation or retirement checks. A clean report does not guarantee that archive will succeed.

A failing item lists each issue and the fix:

```
Expand Down
11 changes: 6 additions & 5 deletions src/commands/change.ts
Original file line number Diff line number Diff line change
Expand Up @@ -545,11 +545,12 @@ export class ChangeCommand {
console.log(`Change "${changeName}" is valid`);
} else {
console.error(`Change "${changeName}" has issues`);
report.issues.forEach(issue => {
const label = issue.level === 'ERROR' ? 'ERROR' : 'WARNING';
const prefix = issue.level === 'ERROR' ? '✗' : '⚠';
console.error(`${prefix} [${label}] ${issue.path}: ${issue.message}`);
});
}
report.issues.forEach(issue => {
const prefix = issue.level === 'ERROR' ? '✗' : issue.level === 'WARNING' ? '⚠' : 'ℹ';
console.error(`${prefix} [${issue.level}] ${issue.path}: ${issue.message}`);
});
if (!report.valid) {
// Next steps footer to guide fixing issues
this.printNextSteps(report.issues);
if (!options?.json) {
Expand Down
15 changes: 10 additions & 5 deletions src/commands/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,11 +245,12 @@ export class ValidateCommand {
console.log(`${type === 'change' ? 'Change' : 'Specification'} '${id}' is valid`);
} else {
console.error(`${type === 'change' ? 'Change' : 'Specification'} '${id}' has issues`);
for (const issue of report.issues) {
const label = issue.level === 'ERROR' ? 'ERROR' : issue.level;
const prefix = issue.level === 'ERROR' ? '✗' : issue.level === 'WARNING' ? '⚠' : 'ℹ';
console.error(`${prefix} [${label}] ${issue.path}: ${issue.message}`);
}
}
for (const issue of report.issues) {
const prefix = issue.level === 'ERROR' ? '✗' : issue.level === 'WARNING' ? '⚠' : 'ℹ';
console.error(`${prefix} [${issue.level}] ${issue.path}: ${issue.message}`);
}
if (!report.valid) {
this.printNextSteps(type, id, root, report.issues);
}
}
Expand Down Expand Up @@ -394,6 +395,10 @@ export class ValidateCommand {
for (const res of results) {
if (res.valid) console.log(`✓ ${res.type}/${res.id}`);
else console.error(`✗ ${res.type}/${res.id}`);
for (const issue of res.issues) {
const prefix = issue.level === 'ERROR' ? '✗' : issue.level === 'WARNING' ? '⚠' : 'ℹ';
console.error(` ${prefix} [${issue.level}] ${issue.path}: ${issue.message}`);
}
}
console.log(`Totals: ${summary.totals.passed} passed, ${summary.totals.failed} failed (${summary.totals.items} items)`);
const firstFailure = results.find((res) => !res.valid);
Expand Down
6 changes: 5 additions & 1 deletion src/core/specs-apply.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,11 @@ export async function buildUpdatedSpec(
);
}
}
} catch {
} catch (error) {
// An unreadable target is not a new spec. Preserve the filesystem error
// for callers, rather than synthesizing a baseline or a missing-target finding.
const code = (error as NodeJS.ErrnoException)?.code;
if (code !== 'ENOENT' && code !== 'ENOTDIR') throw error;
// Target spec does not exist; MODIFIED and RENAMED are not allowed for new specs
// REMOVED will be ignored with a warning since there's nothing to remove
if (plan.modified.length > 0 || plan.renamed.length > 0) {
Expand Down
74 changes: 71 additions & 3 deletions src/core/validation/validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -151,9 +152,10 @@ export class Validator {
* - No duplicates within sections; no cross-section conflicts per spec
*
* When `options.mainSpecsDir` is given, MODIFIED blocks are also checked
* against the current main specs for the scenario loss archive refuses to
* apply (#1477). When `options.projectRoot` is given, the schema's tracked
* task files are checked for ambiguous numbering (#1520). Omitting either
* against the current main specs for scenario loss (#1477), and merge
* conflicts are reported as INFO without changing the verdict (#1112).
* When `options.projectRoot` is given, the schema's tracked task files are
* checked for ambiguous numbering (#1520). Omitting either
* option keeps existing library and archive callers behaving as before.
*/
async validateChangeDeltaSpecs(
Expand Down Expand Up @@ -395,6 +397,23 @@ export class Validator {
}
}
}

// Reuse archive's merge builder to report conflicts with the main specs.
// Keep structural errors and scenario loss in their existing diagnostics.
if (options.mainSpecsDir) {
issues.push(
...(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),
]))
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
} catch (error) {
// A missing specs dir (or a stray `specs` file) means no deltas;
// anything else (EACCES, EIO) must stay loud — discoverSpecFiles
Expand Down Expand Up @@ -779,6 +798,55 @@ export class Validator {
return dotIndex > 0 ? fileName.slice(0, dotIndex) : fileName;
}

/**
* Dry-run archive's merge builder without writing its result. Reusing the
* builder preserves its already-synced delta rules instead of duplicating them.
* INFO leaves the verdict unchanged: a missing target can be a typo or a
* requirement introduced by a sibling change that has not archived yet.
* This does not run archive's later merged-spec validation or retirement checks.
*/
private async findArchiveBlockers(
changeDir: string,
mainSpecsDir: string,
alreadyReportedPaths: string[]
): Promise<ValidationIssue[]> {
const alreadyReported = new Set(alreadyReportedPaths);
// 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;
Expand Down
98 changes: 98 additions & 0 deletions test/commands/validate.enriched-output.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { describe, it, expect, beforeEach, afterEach } from 'vitest';
import { promises as fs } from 'fs';
import path from 'path';
import { execFileSync } from 'child_process';
import { runCLI } from '../helpers/run-cli.js';

describe('validate command enriched human output', () => {
const projectRoot = process.cwd();
Expand All @@ -18,6 +19,103 @@ describe('validate command enriched human output', () => {
await fs.rm(testDir, { recursive: true, force: true });
});

const writeArchiveBlocker = async () => {
const mainDir = path.join(testDir, 'openspec', 'specs', 'widgets');
const changeDir = path.join(changesDir, 'c-archive');
const deltaDir = path.join(changeDir, 'specs', 'widgets');
await fs.mkdir(mainDir, { recursive: true });
await fs.mkdir(deltaDir, { recursive: true });
await fs.writeFile(path.join(mainDir, 'spec.md'), `# Widgets Specification

## Purpose
Define how widgets report their existing state consistently to all callers.

## Requirements

### Requirement: Existing state
The system SHALL report the existing state.

#### Scenario: Query state
- **WHEN** queried
- **THEN** the state is reported
`);
await fs.writeFile(
path.join(changeDir, 'proposal.md'),
'# Widget update\n\n## Why\nUpdate widgets.\n\n## What Changes\n- Update state reporting\n'
);
await fs.writeFile(path.join(deltaDir, 'spec.md'), `## MODIFIED Requirements

### Requirement: Future state
The system SHALL report the future state.

#### Scenario: Query state
- **WHEN** queried
- **THEN** the state is reported
`);
};

const entryPoints = [
['validate', 'c-archive'],
['change', 'validate', 'c-archive'],
['validate', '--changes'],
['validate', '--all'],
];

for (const strict of [false, true]) {
for (const args of entryPoints) {
const invocation = [...args, ...(strict ? ['--strict'] : [])];

it(`shows non-blocking archive advice for ${invocation.join(' ')}`, async () => {
await writeArchiveBlocker();

const result = await runCLI([...invocation, '--no-interactive'], { cwd: testDir });

expect(result.exitCode).toBe(0);
expect(result.stderr).toContain('ℹ [INFO] widgets/spec.md: Archive would refuse this delta:');
expect(result.stderr).toContain('Future state');
expect(result.stderr).not.toContain('Next steps:');
expect(result.stdout).toMatch(/is valid|0 failed/);
});

it(`keeps archive advice structured and non-blocking for ${invocation.join(' ')} --json`, async () => {
await writeArchiveBlocker();

const result = await runCLI([...invocation, '--json', '--no-interactive'], { cwd: testDir });

expect(result.exitCode).toBe(0);
const output = JSON.parse(result.stdout);
const report = args[0] === 'change'
? output
: output.items.find((item: { id: string }) => item.id === 'c-archive');
expect(report.valid).toBe(true);
expect(report.issues).toContainEqual(expect.objectContaining({
level: 'INFO',
path: 'widgets/spec.md',
message: expect.stringContaining('Archive would refuse this delta:'),
}));
expect(result.stderr).not.toContain('Archive would refuse this delta:');
if (args[0] !== 'change') expect(output.summary.totals.failed).toBe(0);
});
}
}

it('preserves INFO severity in the deprecated command when another delta is invalid', async () => {
await writeArchiveBlocker();
const invalidDir = path.join(changesDir, 'c-archive', 'specs', 'broken');
await fs.mkdir(invalidDir, { recursive: true });
await fs.writeFile(
path.join(invalidDir, 'spec.md'),
'## ADDED Requirements\n\n### Requirement: Missing scenario\nThe system SHALL do something.\n'
);

const result = await runCLI(['change', 'validate', 'c-archive', '--no-interactive'], { cwd: testDir });

expect(result.exitCode).toBe(1);
expect(result.stderr).toContain('ℹ [INFO] widgets/spec.md: Archive would refuse this delta:');
expect(result.stderr).toContain('[ERROR]');
expect(result.stderr).toContain('Next steps:');
});

it('prints Next steps footer and guidance on invalid change', async () => {
const changeContent = `# Test Change\n\n## Why\nThis is a sufficiently long explanation to pass the why length requirement for validation purposes.\n\n## What Changes\nThere are changes proposed, but no delta specs provided yet.`;
const changeId = 'c-next-steps';
Expand Down
Loading
Loading