diff --git a/.changeset/validate-cross-change-overlap.md b/.changeset/validate-cross-change-overlap.md new file mode 100644 index 0000000000..97198f5a4e --- /dev/null +++ b/.changeset/validate-cross-change-overlap.md @@ -0,0 +1,9 @@ +--- +"@fission-ai/openspec": minor +--- + +`openspec validate --changes` (and `--all`) now reports requirements that more than one active change claims. Every existing check compares a single change against the *current* main spec, so two changes converging on one requirement are each individually valid — the collision only surfaces when the first one archives and the second starts failing, by which point its author has already implemented against a base that moved. + +Each entry names the claiming changes and the operation each one applies (`ADDED`, `MODIFIED`, `REMOVED`, `RENAMED_FROM`, `RENAMED_TO`), and whether the main spec holds that requirement today — two changes editing shared text is a different situation from two changes each proposing it. Rename deltas are reported at both ends, since the old name collides with anyone editing it and the new name collides with anyone adding it. + +The report is informational: overlap is often deliberate for stacked or sequenced work, so it never changes the exit code and makes no claim about which change is wrong. Under `--json` the entries appear in an `overlaps` array. Addresses [#1669](https://github.com/Fission-AI/OpenSpec/issues/1669) and [#1387](https://github.com/Fission-AI/OpenSpec/issues/1387). diff --git a/docs/agent-contract.md b/docs/agent-contract.md index 9338891f43..541dd070a6 100644 --- a/docs/agent-contract.md +++ b/docs/agent-contract.md @@ -53,7 +53,7 @@ deliberately remains the compatibility bare array documented in §4.13: Change: `{ "id", "title", "deltaCount", "deltas": [...], "root" }`. Spec: `{ "id", "title", "overview", "requirementCount", "requirements": [...], "metadata": { "version", "format", "sourcePath"? }, "root" }`. ### 4.3 `validate --json` -`{ "items": [ { "id", "type": "change"|"spec", "valid", "issues": [ { "level", "path", "message", "line"?, "column"? } ], "durationMs" } ], "summary": { "totals": {items,passed,failed}, "byType": {...} }, "version": "1.0", "root" }`. Exit 1 when any item fails. +`{ "items": [ { "id", "type": "change"|"spec", "valid", "issues": [ { "level", "path", "message", "line"?, "column"? } ], "durationMs" } ], "summary": { "totals": {items,passed,failed}, "byType": {...} }, "overlaps"?: [ { "specId", "requirement", "inMainSpec", "claimants": [{changeId, operation, requirement}] } ], "version": "1.0", "root" }`. Exit 1 when any item fails. `overlaps` is present (possibly empty) whenever changes are in scope (`--changes`/`--all`) and absent otherwise; each entry is a requirement more than one active change claims, with `operation` one of `ADDED`/`MODIFIED`/`REMOVED`/`RENAMED_FROM`/`RENAMED_TO` and `inMainSpec` saying whether the main spec holds it today. It is informational — overlap is often deliberate — and never affects the exit code. ### 4.4 `status --json` `{ "changeName", "schemaName", "planningHome"?: { "kind", "root", "changesDir", "defaultSchema" }, "changeRoot", "artifactPaths": { "": {outputPath, resolvedOutputPath, existingOutputPaths} }, "nextSteps": ["..."], "actionContext": { "mode": "repo-local", "sourceOfTruth": "repo", "planningArtifacts", "linkedContext", "allowedEditRoots", "requiresAffectedAreaSelection", "constraints" }, "isPlanningComplete", "isComplete", "applyRequires", "artifacts": [ {id, outputPath, status: "done"|"skipped"|"ready"|"blocked", requires, missingDeps?} ], "root" }`. `isPlanningComplete` means every non-skipped planning artifact exists; skipped artifacts count as satisfied without being created. It does not mean implementation tasks are complete. `isComplete` is retained as a compatibility alias with the same value. Each artifact's `requires` is its direct dependency ids (present for every status, so the transitive required set is computable even when the artifact is `done`); `missingDeps` appears only when `blocked`. The `artifacts` array is in dependency order, with the schema's `artifacts:` declaration order breaking ties between artifacts that become ready at the same time (never alphabetical), so the first `ready` entry is the artifact to write next; `missingDeps` uses that same order. `"skipped"` marks an artifact whose `generates` path is under `specs/` in a change whose `.openspec.yaml` declares `skip_specs: true`; it satisfies dependencies but must not be created. No active changes: `{ "changes": [], "message", "root" }`, exit 0. diff --git a/docs/cli.md b/docs/cli.md index 7c5a75291b..1f6e6a9aab 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -567,6 +567,19 @@ A change with zero spec deltas fails validation unless its `.openspec.yaml` decl `--archived` is its own scope: it does not validate spec deltas (already applied at archive time), it verifies that every change under `changes/archive/` has all of its `tasks.md` checkboxes ticked, exiting non-zero if any are unchecked. This catches changes that were archived with unfinished work — handy in a pre-commit hook. +When changes are in scope (`--changes` or `--all`), validation also reports requirements that more than one active change claims. Each change is validated against the current main spec, so two changes converging on one requirement are both valid until the first archives — this surfaces that collision before it lands. + +Each entry names the claiming changes and what each one does to the requirement (`ADDED`, `MODIFIED`, `REMOVED`, `RENAMED_FROM`, `RENAMED_TO`), and whether the main spec holds that requirement today — two changes editing shared text is a different situation from two changes each proposing it. Overlap is often deliberate (a stacked pair, sequenced work), so the report is informational: it never changes the exit code and makes no claim about which change is wrong. + +```text +⚠ 1 requirement is claimed by more than one active change: + tools: Slash Command Configuration (in the main spec) + add-kilocode-workflows MODIFIED, add-windsurf-workflows MODIFIED +Whichever of these archives second lands on a spec the first one changed; re-read it before archiving. +``` + +Under `--json` the same entries appear in an `overlaps` array alongside `items` and `summary`, present (possibly empty) whenever changes are in scope. + **Examples:** ```bash diff --git a/src/commands/validate.ts b/src/commands/validate.ts index 8f7428e647..f1d162a634 100644 --- a/src/commands/validate.ts +++ b/src/commands/validate.ts @@ -16,6 +16,7 @@ import { nearestMatches } from '../utils/match.js'; import { promises as fs } from 'fs'; import { getTaskProgressDetailForChange, type SchemaGlobCache } from '../utils/task-progress.js'; import { FileSystemUtils } from '../utils/file-system.js'; +import { detectChangeOverlaps, type RequirementOverlap } from '../core/change-overlap.js'; type ItemType = 'change' | 'spec'; @@ -332,7 +333,15 @@ export class ValidateCommand { } as const; if (opts.json) { - const out = { items: [] as BulkItemResult[], summary, version: '1.0', root: toRootOutput(root) }; + const out = { + items: [] as BulkItemResult[], + summary, + // Present whenever changes are in scope, so a consumer sees the same + // shape here as on the path that actually validated something. + ...(scope.changes ? { overlaps: [] as RequirementOverlap[] } : {}), + version: '1.0', + root: toRootOutput(root), + }; console.log(JSON.stringify(out, null, 2)); } else { console.log('No items found to validate.'); @@ -387,8 +396,22 @@ export class ValidateCommand { }, } as const; + // Every check above compares one change against the *current* main spec, so + // none of them can see two open changes converging on the same requirement: + // each is individually consistent with a spec neither has landed in yet. + // Report that here, only when changes are in scope, and only as + // information — overlap is often deliberate (a stacked pair, sequenced + // work), so it never fails the run or moves the exit code. + const overlaps = scope.changes ? await this.detectOverlaps(root, changeIds) : undefined; + if (opts.json) { - const out = { items: results, summary, version: '1.0', root: toRootOutput(root) }; + const out = { + items: results, + summary, + ...(overlaps ? { overlaps } : {}), + version: '1.0', + root: toRootOutput(root), + }; console.log(JSON.stringify(out, null, 2)); } else { for (const res of results) { @@ -403,11 +426,55 @@ export class ValidateCommand { `Details: openspec validate ${firstFailure.id} --type ${firstFailure.type}${storeFlag}` ); } + this.printOverlaps(overlaps ?? []); } process.exitCode = failed > 0 ? 1 : 0; } + /** + * Cross-change overlap for the changes this run already resolved. + * + * Scoped to `root.changesDir` rather than a path rebuilt from the project + * root, so a `--store` run scans the store it selected. A single change can + * never overlap anything, so the scan is skipped entirely below two. + */ + private async detectOverlaps( + root: ResolvedOpenSpecRoot, + changeIds: string[] + ): Promise { + if (changeIds.length < 2) return []; + try { + return await detectChangeOverlaps({ + changesDir: root.changesDir, + specsDir: root.specsDir, + changeIds, + }); + } catch { + // Advisory output must never be the thing that fails a validate run: + // every delta this reads is also read by the per-change validation + // above, which reports its own errors on its own path. + return []; + } + } + + private printOverlaps(overlaps: RequirementOverlap[]): void { + if (overlaps.length === 0) return; + const label = overlaps.length === 1 ? 'requirement is' : 'requirements are'; + console.log(''); + console.log(`⚠ ${overlaps.length} ${label} claimed by more than one active change:`); + for (const overlap of overlaps) { + const base = overlap.inMainSpec ? 'in the main spec' : 'not in the main spec yet'; + console.log(` ${overlap.specId}: ${overlap.requirement} (${base})`); + console.log( + ` ${overlap.claimants.map((c) => `${c.changeId} ${c.operation}`).join(', ')}` + ); + } + console.log( + 'Whichever of these archives second lands on a spec the first one changed; re-read it before archiving.' + ); + } + /** * Lists archived change ids from the resolved root's archive directory, * mirroring `getArchivedChangeIds` but store-aware (uses `root.archiveDir` diff --git a/src/core/change-overlap.ts b/src/core/change-overlap.ts new file mode 100644 index 0000000000..35eabad375 --- /dev/null +++ b/src/core/change-overlap.ts @@ -0,0 +1,274 @@ +import path from 'path'; +import { promises as fs } from 'fs'; +import { discoverSpecFiles } from '../utils/spec-discovery.js'; +import { compareCodePoints } from '../utils/compare.js'; +import { + parseDeltaSpec, + normalizeRequirementName, + extractRequirementsSection, +} from './parsers/requirement-blocks.js'; + +/** + * Cross-change overlap detection. + * + * The validator already refuses a MODIFIED block that would drop scenarios the + * live spec still has, and archive refuses the same write. Both compare one + * change against the *current* main spec, so neither can see two open changes + * converging on the same requirement: each is individually consistent with a + * spec that neither has landed in yet. The collision only becomes visible when + * the first one archives and the second starts failing, by which point the + * second author has already implemented against a base that moved. + * + * This module reports that overlap up front. It is deliberately read-only and + * advisory: two changes touching one requirement is often intentional + * (sequenced work, a stacked pair), so the finding is information for the + * author, not a verdict on the change. + * + * It reports what each change claims and whether the requirement exists today, + * and stops there. Ranking overlaps by how badly they collide would mean + * predicting whether a given archive order aborts, and the preconditions that + * decide that live in specs-apply.ts alongside several cases it deliberately + * treats as already-synced rather than as collisions. A second model of those + * rules here would be free to disagree with the code that does the writing, + * and a wrong severity is worse than none: it would tell an author to rewrite + * a change that archives cleanly. That needs one applicability check archive + * and validate both call, not a copy of one. + */ + +/** How a change claims a requirement. */ +export type OverlapOperation = + | 'ADDED' + | 'MODIFIED' + | 'REMOVED' + | 'RENAMED_FROM' + | 'RENAMED_TO'; + +/** A single (change, spec, requirement) claim parsed out of one delta file. */ +export interface RequirementClaim { + changeId: string; + /** Spec id relative to the change's specs/ root, e.g. "tools". */ + specId: string; + /** Requirement name as written in the delta, for display. */ + requirement: string; + /** Normalized name used for matching; agrees with validator and archive. */ + key: string; + operation: OverlapOperation; +} + +export interface OverlapClaimant { + changeId: string; + operation: OverlapOperation; + /** The spelling this change used, which may differ in surrounding whitespace. */ + requirement: string; +} + +/** One requirement claimed by two or more active changes. */ +export interface RequirementOverlap { + specId: string; + /** Display name, taken from the first claimant in id order. */ + requirement: string; + /** + * Whether the main spec holds this requirement today. A requirement no + * change has landed yet reads differently from one they are all editing: + * those changes are not converging on shared text, they are each proposing + * it, and only one of them can be the one that introduces it. + */ + inMainSpec: boolean; + claimants: OverlapClaimant[]; +} + +/** Where to look for delta specs, and which changes to look at. */ +export interface OverlapScanInput { + /** + * Resolved changes directory - `root.changesDir`, never a path rebuilt from + * the project root. A store-selected root does not live under + * `/openspec/changes`, so rebuilding the path here would silently scan + * the wrong tree (or nothing) for every `--store` invocation. + */ + changesDir: string; + /** Resolved main specs directory - `root.specsDir`, for the same reason. */ + specsDir: string; + /** + * Active change ids to scan. Supplied by the caller so this module never has + * to re-derive what "active" means; callers already exclude `archive/`. + */ + changeIds: readonly string[]; +} + +/** + * Parse one delta spec file into the claims it makes. + * + * RENAMED contributes two claims. The FROM side collides with anyone editing + * the requirement under its old name, and the TO side collides with an ADDED + * of that name in another change - archive applies RENAMED before MODIFIED, so + * both ends are real contention points, not bookkeeping. + */ +export function claimsFromDelta( + content: string, + changeId: string, + specId: string +): RequirementClaim[] { + const plan = parseDeltaSpec(content); + const claims: RequirementClaim[] = []; + + const push = (name: string, operation: OverlapOperation): void => { + const key = normalizeRequirementName(name); + // A delta that names the same requirement twice in one section is already + // a validator error ("Duplicate requirement in ..."); dropping the repeat + // here keeps one change from being reported as overlapping with itself. + if (claims.some((claim) => claim.key === key && claim.operation === operation)) { + return; + } + claims.push({ changeId, specId, requirement: name, key, operation }); + }; + + for (const block of plan.added) push(block.name, 'ADDED'); + for (const block of plan.modified) push(block.name, 'MODIFIED'); + for (const name of plan.removed) push(name, 'REMOVED'); + for (const { from, to } of plan.renamed) { + push(from, 'RENAMED_FROM'); + push(to, 'RENAMED_TO'); + } + + return claims; +} + +/** + * Collect every requirement claim made by the given active changes. + * + * Delta files are enumerated with the same discoverSpecFiles() walk archive and + * specs-apply use, so this sees exactly the files that will be applied - a + * nested capability layout is included, and nothing is matched that archive + * would ignore. A change with no specs/ directory contributes nothing. + */ +export async function collectRequirementClaims( + input: OverlapScanInput +): Promise { + const claims: RequirementClaim[] = []; + + for (const changeId of input.changeIds) { + const changeSpecsDir = path.join(input.changesDir, changeId, 'specs'); + let discovered; + try { + discovered = await discoverSpecFiles(changeSpecsDir); + } catch { + // discoverSpecFiles throws on an unreadable capability so archive can + // refuse to silently drop it. Overlap reporting is advisory and must + // never be the thing that fails a run, so an unreadable change is + // skipped: the validator and archive still report it on their own paths. + continue; + } + + for (const { id: specId, specFile } of discovered) { + let content: string; + try { + content = await fs.readFile(specFile, 'utf-8'); + } catch { + continue; + } + claims.push(...claimsFromDelta(content, changeId, specId)); + } + } + + return claims; +} + +/** + * Requirement names the main specs currently hold, keyed by spec id. A spec + * absent from this map - including one whose file does not exist yet - holds + * nothing, which is how archive sees it too. + */ +export type BaseRequirements = ReadonlyMap>; + +/** + * Read the requirement names each of the given specs currently holds. + * + * A spec that does not exist yet, or cannot be read, contributes an empty set + * rather than an error: overlap reporting is advisory, and the per-change + * validation running alongside it reports an unreadable spec on its own path. + */ +export async function loadBaseRequirements( + specsDir: string, + specIds: Iterable +): Promise { + const base = new Map>(); + + for (const specId of new Set(specIds)) { + let content: string; + try { + content = await fs.readFile(path.join(specsDir, ...specId.split('/'), 'spec.md'), 'utf-8'); + } catch { + base.set(specId, new Set()); + continue; + } + const { bodyBlocks } = extractRequirementsSection(content); + base.set(specId, new Set(bodyBlocks.map((block) => normalizeRequirementName(block.name)))); + } + + return base; +} + +/** + * Group claims into overlaps: one entry per (spec, requirement) claimed by more + * than one change. Results are sorted by spec then requirement, and claimants + * by change id, so output is stable enough to diff in CI. Ordering is by code + * point rather than locale for the same reason discoverSpecFiles() is: spec + * ids and requirement names are free-form text, and a locale-sensitive sort + * would reorder non-ASCII names between one machine and the next. + */ +export function findOverlaps( + claims: readonly RequirementClaim[], + base: BaseRequirements +): RequirementOverlap[] { + const grouped = new Map(); + for (const claim of claims) { + // Encoded rather than concatenated: a spec id and a requirement name can + // both contain spaces, so a plain join lets two different pairs collide. + const groupKey = JSON.stringify([claim.specId, claim.key]); + const existing = grouped.get(groupKey); + if (existing) existing.push(claim); + else grouped.set(groupKey, [claim]); + } + + const overlaps: RequirementOverlap[] = []; + for (const group of grouped.values()) { + const changeIds = new Set(group.map((claim) => claim.changeId)); + // Two claims from one change (e.g. a RENAMED pair) are not a collision. + if (changeIds.size < 2) continue; + + const sorted = [...group].sort( + (a, b) => + compareCodePoints(a.changeId, b.changeId) || compareCodePoints(a.operation, b.operation) + ); + overlaps.push({ + specId: group[0].specId, + requirement: sorted[0].requirement, + inMainSpec: base.get(group[0].specId)?.has(group[0].key) ?? false, + claimants: sorted.map(({ changeId, operation, requirement }) => ({ + changeId, + operation, + requirement, + })), + }); + } + + return overlaps.sort( + (a, b) => compareCodePoints(a.specId, b.specId) || compareCodePoints(a.requirement, b.requirement) + ); +} + +/** + * Convenience wrapper: collect claims across active changes, read the main + * specs those claims land in, and group them. Only specs some change actually + * claims are read. + */ +export async function detectChangeOverlaps( + input: OverlapScanInput +): Promise { + const claims = await collectRequirementClaims(input); + const base = await loadBaseRequirements( + input.specsDir, + claims.map((claim) => claim.specId) + ); + return findOverlaps(claims, base); +} diff --git a/src/utils/compare.ts b/src/utils/compare.ts new file mode 100644 index 0000000000..18a7d5fe72 --- /dev/null +++ b/src/utils/compare.ts @@ -0,0 +1,12 @@ +/** + * Compare two strings by UTF-16 code point, never by locale. + * + * `localeCompare()` follows the process's ICU locale, so the same inputs can + * order differently across OSes and CI images - and for output a caller + * promises is stable (diffed in CI, snapshotted in tests, emitted as JSON), + * that difference is a spurious failure. Code-point ordering is the same + * everywhere. + */ +export function compareCodePoints(a: string, b: string): number { + return a < b ? -1 : a > b ? 1 : 0; +} diff --git a/src/utils/spec-discovery.ts b/src/utils/spec-discovery.ts index ab6b8a5eb3..d9dda0316d 100644 --- a/src/utils/spec-discovery.ts +++ b/src/utils/spec-discovery.ts @@ -1,6 +1,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import { FileSystemUtils } from './file-system.js'; +import { compareCodePoints } from './compare.js'; export interface DiscoveredSpec { /** Spec id relative to the specs root, forward-slash separated on every platform (e.g. "web" or "platform/session-layout"). */ @@ -69,10 +70,9 @@ export async function discoverSpecFiles(specsRoot: string): Promise (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); + // Code-point comparison, not localeCompare, so the deterministic order the + // docstring promises does not vary with the process's ICU locale. + return results.sort((a, b) => compareCodePoints(a.id, b.id)); } /** diff --git a/test/cli-e2e/validate-change-overlap.test.ts b/test/cli-e2e/validate-change-overlap.test.ts new file mode 100644 index 0000000000..6bcaf3d374 --- /dev/null +++ b/test/cli-e2e/validate-change-overlap.test.ts @@ -0,0 +1,199 @@ +import { afterAll, describe, it, expect, beforeAll } from 'vitest'; +import { promises as fs } from 'fs'; +import path from 'path'; +import { tmpdir } from 'os'; +import { runCLI } from '../helpers/run-cli.js'; + +/** + * Two changes editing one requirement are each valid on their own: every check + * the validator runs compares a single change against the current main spec, + * which neither of them has landed in yet. The collision only surfaces when the + * first archives. These exercise the advisory report through the real CLI — + * what it says, and that it never moves the exit code (#1669). + */ +describe('openspec validate reports requirements two active changes both claim (#1669)', () => { + const tempRoots: string[] = []; + let projectDir: string; + + const write = async (relative: string, content: string) => { + const file = path.join(projectDir, relative); + await fs.mkdir(path.dirname(file), { recursive: true }); + await fs.writeFile(file, content); + }; + + const MAIN_SPEC = `# widgets Specification + +## Purpose +Define widget behavior for the end-to-end check. + +## Requirements + +### Requirement: Widget state +The system SHALL report the widget state. + +#### Scenario: Existing scenario +- **WHEN** queried +- **THEN** the state is reported +`; + + /** A MODIFIED block that keeps the live scenario, so the change is valid alone. */ + const modifies = (extraScenario: string) => `## MODIFIED Requirements + +### Requirement: Widget state +The system SHALL report the widget state. + +#### Scenario: Existing scenario +- **WHEN** queried +- **THEN** the state is reported + +#### Scenario: ${extraScenario} +- **WHEN** ${extraScenario} happens +- **THEN** it is reported +`; + + const adds = (body: string) => `## ADDED Requirements + +### Requirement: Widget colors +The system SHALL report ${body}. + +#### Scenario: Colors queried +- **WHEN** colors are queried +- **THEN** ${body} is reported +`; + + const proposal = (changeId: string) => + `# ${changeId}\n\n## Why\nExercise overlap reporting.\n\n## What Changes\n- Extend widget reporting\n`; + + beforeAll(async () => { + const base = await fs.mkdtemp(path.join(tmpdir(), 'openspec-overlap-e2e-')); + tempRoots.push(base); + projectDir = path.join(base, 'project'); + await fs.mkdir(projectDir, { recursive: true }); + + await write('openspec/specs/widgets/spec.md', MAIN_SPEC); + + for (const [changeId, scenario, added] of [ + ['adds-hover', 'Hover state', 'the hover color'], + ['adds-focus', 'Focus state', 'the focus color'], + ] as const) { + await write(`openspec/changes/${changeId}/proposal.md`, proposal(changeId)); + await write(`openspec/changes/${changeId}/specs/widgets/spec.md`, modifies(scenario)); + await write(`openspec/changes/${changeId}/specs/colors/spec.md`, adds(added)); + } + }); + + afterAll(async () => { + await Promise.all(tempRoots.map((dir) => fs.rm(dir, { recursive: true, force: true }))); + }); + + it('reports the overlap without failing the run', async () => { + const result = await runCLI(['validate', '--changes'], { cwd: projectDir }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain('✓ change/adds-focus'); + expect(result.stdout).toContain('✓ change/adds-hover'); + expect(result.stdout).toContain('2 requirements are claimed by more than one active change'); + }); + + it('names the claiming changes and whether the requirement exists yet', async () => { + const result = await runCLI(['validate', '--changes'], { cwd: projectDir }); + + // Both changes MODIFY a requirement the main spec already holds. + expect(result.stdout).toContain('widgets: Widget state (in the main spec)'); + // Both changes ADD one it does not hold yet. + expect(result.stdout).toContain('colors: Widget colors (not in the main spec yet)'); + expect(result.stdout).toContain('adds-focus MODIFIED, adds-hover MODIFIED'); + expect(result.stdout).toContain('adds-focus ADDED, adds-hover ADDED'); + }); + + it('emits the overlaps under --json', async () => { + const result = await runCLI(['validate', '--changes', '--json'], { cwd: projectDir }); + + expect(result.exitCode).toBe(0); + const payload = JSON.parse(result.stdout); + expect(payload.summary.totals).toMatchObject({ items: 2, passed: 2, failed: 0 }); + expect(payload.overlaps.map((o: any) => [o.specId, o.requirement, o.inMainSpec])).toEqual([ + ['colors', 'Widget colors', false], + ['widgets', 'Widget state', true], + ]); + expect(payload.overlaps[0].claimants).toEqual([ + { changeId: 'adds-focus', operation: 'ADDED', requirement: 'Widget colors' }, + { changeId: 'adds-hover', operation: 'ADDED', requirement: 'Widget colors' }, + ]); + }); + + it('lists every claimant when three changes claim one requirement', async () => { + const threeDir = path.join(tempRoots[0], 'three'); + await fs.mkdir(threeDir, { recursive: true }); + const original = projectDir; + projectDir = threeDir; + try { + await write('openspec/specs/widgets/spec.md', MAIN_SPEC); + for (const [changeId, scenario] of [ + ['adds-hover', 'Hover state'], + ['adds-focus', 'Focus state'], + ['adds-active', 'Active state'], + ] as const) { + await write(`openspec/changes/${changeId}/proposal.md`, proposal(changeId)); + await write(`openspec/changes/${changeId}/specs/widgets/spec.md`, modifies(scenario)); + } + } finally { + projectDir = original; + } + + const result = await runCLI(['validate', '--changes'], { cwd: threeDir }); + + expect(result.exitCode).toBe(0); + expect(result.stdout).toContain( + 'adds-active MODIFIED, adds-focus MODIFIED, adds-hover MODIFIED' + ); + expect(result.stdout).toContain('1 requirement is claimed by more than one active change'); + }); + + it('omits the overlap scan when only specs are validated', async () => { + const result = await runCLI(['validate', '--specs', '--json'], { cwd: projectDir }); + + expect(result.exitCode).toBe(0); + expect(JSON.parse(result.stdout).overlaps).toBeUndefined(); + }); + + it('keeps the overlaps key present for a changes-scoped run with no changes', async () => { + const emptyDir = path.join(tempRoots[0], 'empty'); + await fs.mkdir(emptyDir, { recursive: true }); + const original = projectDir; + projectDir = emptyDir; + try { + await write('openspec/specs/widgets/spec.md', MAIN_SPEC); + } finally { + projectDir = original; + } + + const result = await runCLI(['validate', '--changes', '--json'], { cwd: emptyDir }); + + expect(result.exitCode).toBe(0); + expect(JSON.parse(result.stdout).overlaps).toEqual([]); + }); + + it('reports no overlap for a project with a single change', async () => { + const soloDir = path.join(tempRoots[0], 'solo'); + await fs.mkdir(soloDir, { recursive: true }); + const original = projectDir; + projectDir = soloDir; + try { + await write('openspec/specs/widgets/spec.md', MAIN_SPEC); + await write('openspec/changes/adds-hover/proposal.md', proposal('adds-hover')); + await write('openspec/changes/adds-hover/specs/widgets/spec.md', modifies('Hover state')); + } finally { + projectDir = original; + } + + const json = await runCLI(['validate', '--changes', '--json'], { cwd: soloDir }); + expect(json.exitCode).toBe(0); + expect(JSON.parse(json.stdout).overlaps).toEqual([]); + + // The human-readable report is only reachable without --json. + const text = await runCLI(['validate', '--changes'], { cwd: soloDir }); + expect(text.exitCode).toBe(0); + expect(text.stdout).not.toContain('claimed by more than one active change'); + }); +}); diff --git a/test/core/change-overlap.test.ts b/test/core/change-overlap.test.ts new file mode 100644 index 0000000000..1dbd0eb789 --- /dev/null +++ b/test/core/change-overlap.test.ts @@ -0,0 +1,423 @@ +import { describe, expect, it, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'fs'; +import os from 'os'; +import path from 'path'; + +import { + claimsFromDelta, + collectRequirementClaims, + detectChangeOverlaps, + findOverlaps, + loadBaseRequirements, + type BaseRequirements, + type RequirementClaim, +} from '../../src/core/change-overlap.js'; + +function delta(sections: string): string { + return sections.trimStart(); +} + +const MODIFIED_SLASH = delta(` +## MODIFIED Requirements +### Requirement: Slash Command Configuration +The system SHALL configure slash commands per tool. + +#### Scenario: Cursor commands +- **WHEN** Cursor selected +- **THEN** write .cursor commands +`); + +const MAIN_SLASH = delta(` +# tools Specification + +## Purpose +Configure tools. + +## Requirements + +### Requirement: Slash Command Configuration +The system SHALL configure slash commands per tool. + +#### Scenario: Cursor commands +- **WHEN** Cursor selected +- **THEN** write .cursor commands +`); + +describe('claimsFromDelta', () => { + it('claims a MODIFIED requirement', () => { + const claims = claimsFromDelta(MODIFIED_SLASH, 'add-kilo', 'tools'); + + expect(claims).toEqual([ + { + changeId: 'add-kilo', + specId: 'tools', + requirement: 'Slash Command Configuration', + key: 'Slash Command Configuration', + operation: 'MODIFIED', + }, + ]); + }); + + it('claims both ends of a RENAMED pair', () => { + const claims = claimsFromDelta( + delta(` +## RENAMED Requirements +- FROM: \`### Requirement: Old Name\` +- TO: \`### Requirement: New Name\` +`), + 'rename-change', + 'tools' + ); + + expect(claims.map((c) => [c.requirement, c.operation])).toEqual([ + ['Old Name', 'RENAMED_FROM'], + ['New Name', 'RENAMED_TO'], + ]); + }); + + it('does not claim the same requirement twice for one operation', () => { + // A duplicate header is already a validator error; counting it twice here + // would report the change as overlapping with itself. + const claims = claimsFromDelta( + delta(` +## ADDED Requirements +### Requirement: Dup +The system SHALL do it. + +#### Scenario: One +- **WHEN** a +- **THEN** b + +### Requirement: Dup +The system SHALL do it again. + +#### Scenario: Two +- **WHEN** c +- **THEN** d +`), + 'dup-change', + 'tools' + ); + + expect(claims).toHaveLength(1); + }); + + it('returns nothing for a delta with no recognized sections', () => { + expect(claimsFromDelta('## Why\nJust prose.\n', 'noop', 'tools')).toEqual([]); + }); +}); + +describe('findOverlaps', () => { + const REQUIREMENT = 'Shared Requirement'; + + const claim = ( + changeId: string, + operation: RequirementClaim['operation'] = 'MODIFIED', + overrides: Partial = {} + ): RequirementClaim => ({ + changeId, + specId: 'tools', + requirement: REQUIREMENT, + key: REQUIREMENT, + operation, + ...overrides, + }); + + const PRESENT: BaseRequirements = new Map([['tools', new Set([REQUIREMENT])]]); + const ABSENT: BaseRequirements = new Map([['tools', new Set()]]); + + it('reports a requirement claimed by two changes', () => { + const overlaps = findOverlaps([claim('add-kilo'), claim('add-zed')], PRESENT); + + expect(overlaps).toEqual([ + { + specId: 'tools', + requirement: REQUIREMENT, + inMainSpec: true, + claimants: [ + { changeId: 'add-kilo', operation: 'MODIFIED', requirement: REQUIREMENT }, + { changeId: 'add-zed', operation: 'MODIFIED', requirement: REQUIREMENT }, + ], + }, + ]); + }); + + it('marks a requirement no change has landed yet', () => { + expect(findOverlaps([claim('a', 'ADDED'), claim('b', 'ADDED')], ABSENT)[0].inMainSpec).toBe( + false + ); + }); + + it('treats a spec the base says nothing about as holding nothing', () => { + expect(findOverlaps([claim('a'), claim('b')], new Map())[0].inMainSpec).toBe(false); + }); + + it('ignores a requirement only one change claims', () => { + expect(findOverlaps([claim('solo')], PRESENT)).toEqual([]); + }); + + it('does not treat one change claiming both ends of a rename as an overlap', () => { + expect( + findOverlaps( + [ + claim('rename-change', 'RENAMED_FROM', { key: 'Old Name', requirement: 'Old Name' }), + claim('rename-change', 'RENAMED_TO', { key: 'New Name', requirement: 'New Name' }), + ], + PRESENT + ) + ).toEqual([]); + }); + + it('separates identically named requirements in different specs', () => { + expect( + findOverlaps( + [claim('a', 'MODIFIED', { specId: 'tools' }), claim('b', 'MODIFIED', { specId: 'cli' })], + PRESENT + ) + ).toEqual([]); + }); + + it('does not merge two groups whose spec and requirement concatenate alike', () => { + // "tools" + "cli Shared" and "tools cli" + "Shared" join to the same string + // under a space delimiter; they are different requirements. + expect( + findOverlaps( + [ + claim('a', 'MODIFIED', { specId: 'tools', key: 'cli Shared', requirement: 'cli Shared' }), + claim('b', 'MODIFIED', { specId: 'tools cli', key: 'Shared', requirement: 'Shared' }), + ], + PRESENT + ) + ).toEqual([]); + }); + + it('catches a rename colliding with another change editing the old name', () => { + const overlaps = findOverlaps( + [claim('renamer', 'RENAMED_FROM'), claim('editor', 'MODIFIED')], + PRESENT + ); + + expect(overlaps).toHaveLength(1); + expect(overlaps[0].claimants.map((c) => [c.changeId, c.operation])).toEqual([ + ['editor', 'MODIFIED'], + ['renamer', 'RENAMED_FROM'], + ]); + }); + + it('lists every claimant when more than two changes claim one requirement', () => { + const overlaps = findOverlaps([claim('c'), claim('a'), claim('b', 'REMOVED')], PRESENT); + + expect(overlaps[0].claimants.map((c) => c.changeId)).toEqual(['a', 'b', 'c']); + }); + + it('sorts overlaps by spec then requirement, and claimants by change id', () => { + const overlaps = findOverlaps( + [ + claim('z-change', 'MODIFIED', { specId: 'tools', key: 'Beta', requirement: 'Beta' }), + claim('a-change', 'MODIFIED', { specId: 'tools', key: 'Beta', requirement: 'Beta' }), + claim('b-change', 'MODIFIED', { specId: 'cli', key: 'Alpha', requirement: 'Alpha' }), + claim('c-change', 'MODIFIED', { specId: 'cli', key: 'Alpha', requirement: 'Alpha' }), + ], + PRESENT + ); + + expect(overlaps.map((o) => [o.specId, o.requirement])).toEqual([ + ['cli', 'Alpha'], + ['tools', 'Beta'], + ]); + expect(overlaps[1].claimants.map((c) => c.changeId)).toEqual(['a-change', 'z-change']); + }); + + it('orders non-ASCII names by code point, not by the process locale', () => { + // 'ä' (U+00E4) sorts after 'z' by code point but before it under most ICU + // collations, so a locale-sensitive sort would reorder these depending on + // the machine the run happens on. + const overlaps = findOverlaps( + [ + claim('a', 'MODIFIED', { specId: 'ändern', key: 'Ähnlich', requirement: 'Ähnlich' }), + claim('b', 'MODIFIED', { specId: 'ändern', key: 'Ähnlich', requirement: 'Ähnlich' }), + claim('a', 'MODIFIED', { specId: 'zebra', key: 'Zulu', requirement: 'Zulu' }), + claim('b', 'MODIFIED', { specId: 'zebra', key: 'Zulu', requirement: 'Zulu' }), + claim('ä-change', 'MODIFIED'), + claim('z-change', 'MODIFIED'), + ], + PRESENT + ); + + expect(overlaps.map((o) => o.specId)).toEqual(['tools', 'zebra', 'ändern']); + expect(overlaps[0].claimants.map((c) => c.changeId)).toEqual(['z-change', 'ä-change']); + }); +}); + +describe('loadBaseRequirements', () => { + let specsDir: string; + + beforeEach(async () => { + specsDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-overlap-base-')); + }); + + afterEach(async () => { + await fs.rm(specsDir, { recursive: true, force: true }); + }); + + it('reads the requirement names a spec currently holds', async () => { + await fs.mkdir(path.join(specsDir, 'tools'), { recursive: true }); + await fs.writeFile(path.join(specsDir, 'tools', 'spec.md'), MAIN_SLASH); + + const base = await loadBaseRequirements(specsDir, ['tools']); + + expect([...(base.get('tools') ?? [])]).toEqual(['Slash Command Configuration']); + }); + + it('resolves a nested capability id to its own directory', async () => { + await fs.mkdir(path.join(specsDir, 'platform', 'session'), { recursive: true }); + await fs.writeFile(path.join(specsDir, 'platform', 'session', 'spec.md'), MAIN_SLASH); + + const base = await loadBaseRequirements(specsDir, ['platform/session']); + + expect(base.get('platform/session')?.has('Slash Command Configuration')).toBe(true); + }); + + it('treats a spec with no file yet as holding nothing', async () => { + const base = await loadBaseRequirements(specsDir, ['tools', 'tools']); + + expect(base.get('tools')?.size).toBe(0); + }); +}); + +describe('collectRequirementClaims / detectChangeOverlaps', () => { + let root: string; + let changesDir: string; + let specsDir: string; + + async function writeDelta(changeId: string, specId: string, content: string): Promise { + const dir = path.join(changesDir, changeId, 'specs', specId); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'spec.md'), content); + } + + async function writeMainSpec(specId: string, content: string): Promise { + const dir = path.join(specsDir, ...specId.split('/')); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'spec.md'), content); + } + + const scan = (changeIds: string[]) => detectChangeOverlaps({ changesDir, specsDir, changeIds }); + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-overlap-')); + changesDir = path.join(root, 'openspec', 'changes'); + specsDir = path.join(root, 'openspec', 'specs'); + await fs.mkdir(path.join(changesDir, 'archive'), { recursive: true }); + }); + + afterEach(async () => { + await fs.rm(root, { recursive: true, force: true }); + }); + + it('finds the collision two individually valid changes cannot see', async () => { + await writeMainSpec('tools', MAIN_SLASH); + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + await writeDelta('add-zed', 'tools', MODIFIED_SLASH); + + const overlaps = await scan(['add-kilo', 'add-zed']); + + expect(overlaps).toHaveLength(1); + expect(overlaps[0].requirement).toBe('Slash Command Configuration'); + expect(overlaps[0].inMainSpec).toBe(true); + expect(overlaps[0].claimants.map((c) => c.changeId)).toEqual(['add-kilo', 'add-zed']); + }); + + it('reports a requirement no main spec holds yet', async () => { + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + await writeDelta('add-zed', 'tools', MODIFIED_SLASH); + + expect((await scan(['add-kilo', 'add-zed']))[0].inMainSpec).toBe(false); + }); + + it('reports nothing when changes touch different requirements', async () => { + await writeMainSpec('tools', MAIN_SLASH); + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + await writeDelta( + 'other', + 'tools', + delta(` +## ADDED Requirements +### Requirement: Telemetry Opt Out +The system SHALL allow opting out. + +#### Scenario: Opt out +- **WHEN** flag set +- **THEN** disabled +`) + ); + + expect(await scan(['add-kilo', 'other'])).toEqual([]); + }); + + it('reads deltas and specs under the directories it is given, not rebuilt paths', async () => { + // A store-selected root does not live under /openspec, so a scan that + // rebuilt either path from a project root would find nothing here. + const storeChanges = path.join(root, 'store', 'planning', 'changes'); + const storeSpecs = path.join(root, 'store', 'planning', 'specs'); + for (const changeId of ['add-kilo', 'add-zed']) { + const dir = path.join(storeChanges, changeId, 'specs', 'tools'); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'spec.md'), MODIFIED_SLASH); + } + await fs.mkdir(path.join(storeSpecs, 'tools'), { recursive: true }); + await fs.writeFile(path.join(storeSpecs, 'tools', 'spec.md'), MAIN_SLASH); + + const overlaps = await detectChangeOverlaps({ + changesDir: storeChanges, + specsDir: storeSpecs, + changeIds: ['add-kilo', 'add-zed'], + }); + + expect(overlaps).toHaveLength(1); + // The base came from the store's specs, not the project's. + expect(overlaps[0].inMainSpec).toBe(true); + }); + + it('discovers deltas in a nested capability layout', async () => { + await writeMainSpec('platform/session', MAIN_SLASH); + await writeDelta('a', 'platform/session', MODIFIED_SLASH); + await writeDelta('b', 'platform/session', MODIFIED_SLASH); + + const overlaps = await scan(['a', 'b']); + + expect(overlaps).toHaveLength(1); + expect(overlaps[0].specId).toBe('platform/session'); + expect(overlaps[0].inMainSpec).toBe(true); + }); + + it('ignores a change with no specs directory', async () => { + await fs.mkdir(path.join(changesDir, 'docs-only'), { recursive: true }); + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + + expect( + await collectRequirementClaims({ + changesDir, + specsDir, + changeIds: ['docs-only', 'add-kilo'], + }) + ).toHaveLength(1); + }); + + it('scans only the change ids it is given', async () => { + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + await writeDelta('add-zed', 'tools', MODIFIED_SLASH); + + expect(await scan(['add-kilo'])).toEqual([]); + }); + + it('ignores a change id with no directory on disk', async () => { + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + + expect(await scan(['add-kilo', 'never-scaffolded'])).toEqual([]); + }); + + it('returns nothing when there are no changes at all', async () => { + expect(await scan([])).toEqual([]); + }); +});