From 3fefb2e0ff1b72457a353a5a8d7a351755f1ae7b Mon Sep 17 00:00:00 2001 From: Ryan de Melo Date: Sat, 15 Aug 2026 12:39:48 +0800 Subject: [PATCH 1/3] feat(core): detect requirement overlap between active changes The validator and archive both refuse a MODIFIED block that would drop scenarios the live spec still has, but both compare one change against the current main spec. Two open changes converging on the same requirement are each individually consistent with a spec neither has landed in, so nothing reports the collision until the first one archives and the second starts failing - after the second author has already implemented against a base that moved. Add read-only detection that groups requirement claims across active changes and reports any requirement claimed by more than one. Delta files are enumerated with the same discoverSpecFiles() walk archive and specs-apply use, and names are matched with normalizeRequirementName, so this agrees with the paths that will actually apply the deltas. Advisory by design: overlap is often intentional, so this reports rather than judges, and never throws on an unreadable change. Core detection only; no CLI surface yet pending a call on placement. Refs #1669 --- src/core/change-overlap.ts | Bin 0 -> 6735 bytes test/core/change-overlap.test.ts | 257 +++++++++++++++++++++++++++++++ 2 files changed, 257 insertions(+) create mode 100644 src/core/change-overlap.ts create mode 100644 test/core/change-overlap.test.ts diff --git a/src/core/change-overlap.ts b/src/core/change-overlap.ts new file mode 100644 index 0000000000000000000000000000000000000000..cf3f821f1e39fbe95f651a21e8a8c95c295ccad4 GIT binary patch literal 6735 zcmcIpQE%JG5$>~p#ULooQXtbsp9&|wz_p#$C>$T0;9d-)w<1^8o+&QNC9SBA|NDJ2 zvrAHv6ZEBixUV=6)xod$u^&Bn}2jAR;P7YcHFhD495Ylj?%hK_WRcN54JtaTlZwom!^8KSH&v^ zTx(a~GjKpSLKEzTEnCA*%QUDPgvLycW@ z5nY%9v~CcljQ;MC&)lzYJWIY3W6^o@U|bbi-h$rdUDF^cZ)t^xveS%$r^-6K zZYVc+Xf3sCPrkMlsg1zC<-S4~9$fW+4aAx_Fbd=eS(7ix1#(s4o?OwHvfS{8!G#vv z(i+>4VQX?oiAUNSXQdKnwFN$>jcqdtZ|=*|1;iY?!r0v6lz`h4p3O^~e`zbW$ERDk z2Is`-Q@DU(#7<&`&+uz#P16F_#+5k7jOH5{aPYbI4(?&4_@;|Z*DjG&_A6x;MN$f^ z;AD!cp}7eN8RvEdq!a@<5Q&v9I=I!gI={9wZfKeZX45D9F(DS zBuB;>*eg@u%)Z&2NOodksP!F?Avxh)dfc-;zd+CtOb$H)Xi7FS30P#S94A=&<`MAX zsN|3MyEd+w3LZ3?NFoeNIu~48Y=rPzijxLthPrEE1>h)L&hh8>5jh--;{z@#`lnxG zRRm>T8n;4R$1Z2Q)P5CyYB#l|8!fw=nq$IypQ6aW(c#&}#pT7J{+=o<{&{!zmX6G^DIG z0A7h29K7d{Hw%+n8Zps1ADN!vw(fIPoFMWI_I`(F93kd0as_rSab8q=>qntH4B}Wg zrpEhrMmBrM=uPW=8Q#d|B=7glj(nhsIRk)Z+E8%F4swi{CG^{!J!o*rza1Nl~E&=|CSU%n?vc<}5sBnt~7?W8Poa#F*7O+WcLygq`D1bNv z=lcdsH43nT3_%1d2>~}?574=hMj8DGVmDUF5P?ySV=yELjO2YlTxg0VZ8K`kBUnPT zfR;UzsNkJLiSq%GzA0?;qZ0wt3~zi9`lO|F+}%soVq^V<+xDn-*hMKICaQ>cB?kvS zg>ND8X{=`MCn%eFhd?V8%5v=%TxeYNCHkId<|m zlc=4ZVz&+b^izD;2cJ&ys{d;`jR7Y(&~{D5=_Y!S|HfT7o0I1rY1(J6K&qptp2W(- zp0mL^y-0dq1R5+r7*#-$s%q38LoYK0wBwXy8+nsy6-T`wCcgVIbedbZ*T3&I#;ytJ zrfGZ38;EyuAa0%X$nKZVLbxDbkHKlMz+b@v_b&`M952r7j1ISyj2MgWTR(aWQ6ulr zUs4`o`Hva+9Lxx~l7+ltojGVmNfbyI^vOL4%=Z8p0yf+3Kp-gQD4`RfZe=N3bt_S? zP?oIo`Fj}=(h&F=R4>EgWJ+sOJ`SX)3YYmN95F*~2<|A9raTFM6YBPt}Z#S4NHZ0sk&gNwYtSG@VQ*+_qg(w$%C8Zxn zE;gnkn|lC*ps$~$s*_VzKFP8IBWI2|P$r(fLq+pj+-$<8LeuLRS`)I%hk>b@wO*n= zCfCO2HiRr+7nA9jx5I}Mq|y{JF{qTfyJyP&MaTK~yVmdSl_Ry196HTuUz&kFe4vS0 z8?@m4NqxdTO$BKGJE4yXFSQ!RJ@!p-kG7;?GE%~YYxu;tp2&X3cqZEd?BJY=k#dON z)l|T*$tWHQjI1Bm2l$BbfNK1@H@35DiI4x%<6s~GKITg$A1>l0v?Jc2o z?0pF&HGa)h4Mjj*#Tha@wvKR#@jFfT7{8_RrE%z}&kPZO_KKk|mj({=V$yg@_7S~l zK$M|()dlStVyog>=PKdiQMe1nc@UN6}*z+Hm>&*@OYOraj=Qgh8V zN76=36OB{e>z_X3i;!_<*!m=G10Z$n;^=P)NN8p|30FJA0aCleb8V|a;ArXk=b*r% zk7Lm4A+@N~&W}0#ADB4mxQ`0w*z2LIMCd{yZwNvO^FHFX)nnr{tAhql7))`1gDM%t zdIohCx)N%b^(U?!m`u?V*chj?8utz5Q8^_K0BjCR)eurB$O84&%D%X5;0myn?;g-3 zyi8K8qt36kNV2EL1#Xif;6qd_Bd*)$wQZdmzuZ!)4;(!l2%6BY>Dtuql`&_pXwsi2 zQtu;JYL^6gX2HXem){hbN_zGy?#H-_Pbs>Vfp#a! zRwNu2Y-!~b?NGnT1ZA>OTxUxRSjkQp*_qLuth<+m0x;cwz|JU|L*P`ElMMG!d5L)@ z@U#!OptE7p>p(S}Secs(hx&8!CR>?$UvZ}8sSZFmUvLwtan=!aBzh-u17iA7O0KU2 zuRIswswv$ZD&L6S6-L8ta~5z-Oz-KhD2ASegny#rT)_-xIDUr`mSf&$TnOBi`DbEq ze<*Ul=gN*gDA4gt~z86k?9HTzkaBt!09Y-G9$IF>>zV5CZ-<0CE4C>N0 zh-Q?|5s@b6e { + 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 claim = ( + changeId: string, + requirement: string, + operation: RequirementClaim['operation'] = 'MODIFIED', + specId = 'tools' + ): RequirementClaim => ({ + changeId, + specId, + requirement, + key: requirement.trim(), + operation, + }); + + it('reports a requirement claimed by two changes', () => { + const overlaps = findOverlaps([ + claim('add-kilo', 'Slash Command Configuration'), + claim('add-zed', 'Slash Command Configuration'), + ]); + + expect(overlaps).toHaveLength(1); + expect(overlaps[0].specId).toBe('tools'); + expect(overlaps[0].claimants.map((c) => c.changeId)).toEqual(['add-kilo', 'add-zed']); + }); + + it('ignores a requirement only one change claims', () => { + expect(findOverlaps([claim('solo', 'Only Mine')])).toEqual([]); + }); + + it('does not treat one change claiming both ends of a rename as an overlap', () => { + expect( + findOverlaps([ + claim('rename-change', 'Old Name', 'RENAMED_FROM'), + claim('rename-change', 'New Name', 'RENAMED_TO'), + ]) + ).toEqual([]); + }); + + it('separates identically named requirements in different specs', () => { + expect( + findOverlaps([ + claim('a', 'Shared Name', 'MODIFIED', 'tools'), + claim('b', 'Shared Name', 'MODIFIED', 'cli'), + ]) + ).toEqual([]); + }); + + it('catches a rename colliding with another change editing the old name', () => { + const overlaps = findOverlaps([ + claim('renamer', 'Slash Command Configuration', 'RENAMED_FROM'), + claim('editor', 'Slash Command Configuration', 'MODIFIED'), + ]); + + expect(overlaps).toHaveLength(1); + expect(overlaps[0].claimants.map((c) => c.operation)).toEqual(['MODIFIED', 'RENAMED_FROM']); + }); + + it('sorts overlaps by spec then requirement, and claimants by change id', () => { + const overlaps = findOverlaps([ + claim('z-change', 'Beta', 'MODIFIED', 'tools'), + claim('a-change', 'Beta', 'MODIFIED', 'tools'), + claim('b-change', 'Alpha', 'MODIFIED', 'cli'), + claim('c-change', 'Alpha', 'MODIFIED', 'cli'), + ]); + + 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']); + }); +}); + +describe('collectRequirementClaims / detectChangeOverlaps', () => { + let root: string; + + async function writeDelta(changeId: string, specId: string, content: string): Promise { + const dir = path.join(root, 'openspec', 'changes', changeId, 'specs', specId); + await fs.mkdir(dir, { recursive: true }); + await fs.writeFile(path.join(dir, 'spec.md'), content); + } + + beforeEach(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-overlap-')); + await fs.mkdir(path.join(root, 'openspec', 'changes', '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 writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + await writeDelta('add-zed', 'tools', MODIFIED_SLASH); + + const overlaps = await detectChangeOverlaps(root); + + expect(overlaps).toHaveLength(1); + expect(overlaps[0].requirement).toBe('Slash Command Configuration'); + expect(overlaps[0].claimants.map((c) => c.changeId)).toEqual(['add-kilo', 'add-zed']); + }); + + it('reports nothing when changes touch different requirements', async () => { + 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 detectChangeOverlaps(root)).toEqual([]); + }); + + it('skips the archive directory', async () => { + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + const archived = path.join( + root, + 'openspec', + 'changes', + 'archive', + '2026-08-15-old', + 'specs', + 'tools' + ); + await fs.mkdir(archived, { recursive: true }); + await fs.writeFile(path.join(archived, 'spec.md'), MODIFIED_SLASH); + + expect(await detectChangeOverlaps(root)).toEqual([]); + }); + + it('discovers deltas in a nested capability layout', async () => { + await writeDelta('a', 'platform/session', MODIFIED_SLASH); + await writeDelta('b', 'platform/session', MODIFIED_SLASH); + + const overlaps = await detectChangeOverlaps(root); + + expect(overlaps).toHaveLength(1); + expect(overlaps[0].specId).toBe('platform/session'); + }); + + it('ignores a change with no specs directory', async () => { + await fs.mkdir(path.join(root, 'openspec', 'changes', 'docs-only'), { recursive: true }); + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + + expect(await collectRequirementClaims(root)).toHaveLength(1); + }); + + it('honors an explicit change id list', async () => { + await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); + await writeDelta('add-zed', 'tools', MODIFIED_SLASH); + + expect(await detectChangeOverlaps(root, ['add-kilo'])).toEqual([]); + }); + + it('returns nothing for a root with no changes at all', async () => { + expect(await detectChangeOverlaps(root)).toEqual([]); + }); +}); From 0440b8f84190b9772524acd3225caab7761d7697 Mon Sep 17 00:00:00 2001 From: Ryan de Melo Date: Wed, 19 Aug 2026 20:59:43 +0800 Subject: [PATCH 2/3] feat(validate): report requirements two active changes both claim Every check compares one change against the current main spec, so two changes converging on one requirement are each individually valid. The collision only surfaces when the first archives and the second starts failing, by which point its author has implemented against a base that moved (#1246, #1669, #1387). `validate --changes` / `--all` now names each contested requirement, the changes claiming it and the operation each applies, and whether the main spec holds it today. Rename deltas report at both ends: the old name collides with anyone editing it, the new name with anyone adding it. Deliberately no severity ranking. Deciding whether a given archive order aborts means reproducing the preconditions in specs-apply.ts, including the cases it treats as already-synced rather than as collisions; a second copy of those rules here would be free to disagree with the code doing the writing, and a wrong verdict would tell an author to rewrite a change that archives cleanly. That needs one applicability check archive and validate both call. Read-only and exit-code neutral. Delta files are enumerated with the same discoverSpecFiles() walk archive uses, and the scan is scoped to the resolved root's changesDir and specsDir so a --store run reads the store it selected. --- .changeset/validate-cross-change-overlap.md | 9 + docs/agent-contract.md | 2 +- docs/cli.md | 13 + src/commands/validate.ts | 71 ++++- src/core/change-overlap.ts | Bin 6735 -> 9992 bytes test/cli-e2e/validate-change-overlap.test.ts | 199 ++++++++++++++ test/core/change-overlap.test.ts | 258 +++++++++++++++---- 7 files changed, 493 insertions(+), 59 deletions(-) create mode 100644 .changeset/validate-cross-change-overlap.md create mode 100644 test/cli-e2e/validate-change-overlap.test.ts 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..844f01369c 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. + +``` +⚠ 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 index cf3f821f1e39fbe95f651a21e8a8c95c295ccad4..33f9511b85b0f5347f7d15d125fb7f0a06041299 100644 GIT binary patch delta 3694 zcmaJ^&yO5O6_(?WL=%LU#EETi#KV!;nPsLY2M%bwyH+AVT99MpO_W0vu{GT_GnMVB zYPza-+_J2`MIdoV)OY>|wBp1ioH%kr9Jp}igv1Rb_};6Y+1Vw@NM85URJ|YH``-7e z|NMhrz5D4WpZ#fkmqkB$PBx^QLiv|6Y@ zTBQq07s}2xrMWUoPs(QaO^1ap34gGEV-6mS$I;+2RS(%7Z}!0zSi^pu&|_s!jh&M_ z(`Bx(KCNltRy0*jme;(irqt!R$uzB8m1nfn%Cf1V)R{rFIJ|bRXl~B5B~_*iJdi6h z1f`}N|4@8$fH;^sn;A}&<4}RkWjbXa{FQP(+!#x~(5WA(qR8t6d8pjkx#w+OBVLtL zs=W4O0*>cqilw0Qn$3k$D?Y)8@R-#jU)xk?ge?&%g*bi8&2uCPy$!P*87&>mHRNUp z$9c{6$|{E=#;k*3s%chg1nej?Ud{P=Wx|3XgVl8^nZq$&T9ruH&No$1SV1;16BBAO zD6uQOKw!zb#_9=Y7EwoDIS?dOAtV&qq0Velf>Ucs2znj*$Sp^yfE>NYpjxdnPtIy$ zJWZ;mCTG9tLZ_$el5wicku#Nl*JMDGrVcr!D$j?ADG*$_qGo8EJ-5&Q<9ajR+v^eG z+y`4ET&k{mAX5&vVS)6vnJJWH$hQ}*iiNeMfEzy|E$JWyvaXfD(T&lXtED4-*U7q+C?iqaL%V_&pp zuz>+`09HKzL|CGiz{nCv!L^PN0dr$R>9Q&njwWJZjK9V{1U|rvdOZ|O@ z5b>e7xpSv9L!m*$pkg0H$E$_eNC0r{wyMzn@x|AhN3n~K5zCP+sz6WSZBwAf+PnM= zc}9F0OSEEeWlbYGDV+;R_sfq=d4hU#lcC}TS_bG(E0YJoJVL>l0-cXxg5)qY=pdNW zBd>F;a<{=KNe?jS($^IVT1E>e`~3Ui*cIAxtB*VP@a>u4>@njKi7Sp`s{y^qBMUsT zjrJPe7uyQ`uc;3J;dcQO13{A$s_A4jk}ywDh%=We7NU#dTtF~EHYLv9k08pt^rWK{?~a+N4KN))1B|{$e}F) zp8sX%pU*X0!;vgdjAFV~fV3(*HDIL~(y6W^I*c%|6IsWjDBIM7GD2a73uvhV)HjxM zWu9^4u2}ME26lnC4IDCaWsr&rjiRElxsF;x9O0gXJ)WHF;peqQ9g41>@F1s9 z7nt4E7Xb{&^#utr zRF*?iQ`AoK!4ZR7fLmD+vG&Q0yG`y?b`}2q7jz^hwHOhe9|hbkV0K-E-e5vM$Mi8? z?_V``D5?u|fZY^zMs2NLQCHU^#E-&$L0$U;RF*`i`tda9aD-Vi69eEtKd$(}06yBk zzVgNk0eXhV|GsjsVVO=ocE-vs4+R=hk|gm6{=>BI?~Zo|L)zuFWSPlx6=tI!?!q^Q zI6Xs^1Gvm{gFhU9f|_=Yc_yrg0~vG&NN8YerJK%zoQ&~5#dOQ+cXF4WdYC@=iW@Iy z9MGJd*)R2bVK71S_WM&dKA?j``s{DF-;7@q7K#Br9MLnzwA7(0F`kjl`5#{W+O_Lg zYd^pF=3Oo_uTK82BYT7Pjn{9r|GxQ)W;~`JTfWj=zC9vhfTb<|WfP1^1{H!M*8>HY z7uRr(hua6vFpkIFoeBxZTav>4KqekWE$;M|xQLW%zPYa)eXzOmfW)Tk?yQ*>L1y^D zvw7*N;%B%69nepYAHADI0Wq_>|Dn7lbSyyA@=HSkk^E?I{_EF%d+i0Z9^!A`y7hSz zA3uESqhu?1MuW?~S1g1ZiB|0=H@?yS=FaV}m}K9(5s~lg98ml1Z+!>vEEsFT zyZ9?}3pG(I1)=&f1u3>YIhcKQc|;CHHgkHhJaAU*ya6lk@Y6 zf#w$Fmlk-ZRsyX}P^nJNNzBaE1L^flsbzrivr{W;6E-_4&ST^RD@e>MDW3dP>3|8G z7i|!$SCm>Zo0?Yw@+=6z<*g9R%`z%1jFXjBtppSj^HL!8E0koU=ITwpplU>6 d7)q+CO@6E { + 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 index 7717f44af5..9ce7ffe331 100644 --- a/test/core/change-overlap.test.ts +++ b/test/core/change-overlap.test.ts @@ -8,6 +8,8 @@ import { collectRequirementClaims, detectChangeOverlaps, findOverlaps, + loadBaseRequirements, + type BaseRequirements, type RequirementClaim, } from '../../src/core/change-overlap.js'; @@ -25,6 +27,22 @@ The system SHALL configure slash commands per tool. - **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'); @@ -90,69 +108,118 @@ The system SHALL do it again. }); describe('findOverlaps', () => { + const REQUIREMENT = 'Shared Requirement'; + const claim = ( changeId: string, - requirement: string, operation: RequirementClaim['operation'] = 'MODIFIED', - specId = 'tools' + overrides: Partial = {} ): RequirementClaim => ({ changeId, - specId, - requirement, - key: requirement.trim(), + 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', 'Slash Command Configuration'), - claim('add-zed', 'Slash Command Configuration'), + 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 }, + ], + }, ]); + }); - expect(overlaps).toHaveLength(1); - expect(overlaps[0].specId).toBe('tools'); - expect(overlaps[0].claimants.map((c) => c.changeId)).toEqual(['add-kilo', 'add-zed']); + 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', 'Only Mine')])).toEqual([]); + 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', 'Old Name', 'RENAMED_FROM'), - claim('rename-change', 'New Name', 'RENAMED_TO'), - ]) + 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', 'Shared Name', 'MODIFIED', 'tools'), - claim('b', 'Shared Name', 'MODIFIED', 'cli'), - ]) + 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', 'Slash Command Configuration', 'RENAMED_FROM'), - claim('editor', 'Slash Command Configuration', 'MODIFIED'), - ]); + const overlaps = findOverlaps( + [claim('renamer', 'RENAMED_FROM'), claim('editor', 'MODIFIED')], + PRESENT + ); expect(overlaps).toHaveLength(1); - expect(overlaps[0].claimants.map((c) => c.operation)).toEqual(['MODIFIED', 'RENAMED_FROM']); + 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', 'Beta', 'MODIFIED', 'tools'), - claim('a-change', 'Beta', 'MODIFIED', 'tools'), - claim('b-change', 'Alpha', 'MODIFIED', 'cli'), - claim('c-change', 'Alpha', 'MODIFIED', 'cli'), - ]); + 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'], @@ -162,18 +229,66 @@ describe('findOverlaps', () => { }); }); +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(root, 'openspec', 'changes', changeId, 'specs', specId); + 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-')); - await fs.mkdir(path.join(root, 'openspec', 'changes', 'archive'), { recursive: true }); + changesDir = path.join(root, 'openspec', 'changes'); + specsDir = path.join(root, 'openspec', 'specs'); + await fs.mkdir(path.join(changesDir, 'archive'), { recursive: true }); }); afterEach(async () => { @@ -181,17 +296,27 @@ describe('collectRequirementClaims / detectChangeOverlaps', () => { }); 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 detectChangeOverlaps(root); + 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', @@ -207,51 +332,72 @@ The system SHALL allow opting out. `) ); - expect(await detectChangeOverlaps(root)).toEqual([]); + expect(await scan(['add-kilo', 'other'])).toEqual([]); }); - it('skips the archive directory', async () => { - await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); - const archived = path.join( - root, - 'openspec', - 'changes', - 'archive', - '2026-08-15-old', - 'specs', - 'tools' - ); - await fs.mkdir(archived, { recursive: true }); - await fs.writeFile(path.join(archived, 'spec.md'), MODIFIED_SLASH); + 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(await detectChangeOverlaps(root)).toEqual([]); + 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 detectChangeOverlaps(root); + 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(root, 'openspec', 'changes', 'docs-only'), { recursive: true }); + await fs.mkdir(path.join(changesDir, 'docs-only'), { recursive: true }); await writeDelta('add-kilo', 'tools', MODIFIED_SLASH); - expect(await collectRequirementClaims(root)).toHaveLength(1); + expect( + await collectRequirementClaims({ + changesDir, + specsDir, + changeIds: ['docs-only', 'add-kilo'], + }) + ).toHaveLength(1); }); - it('honors an explicit change id list', async () => { + 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 detectChangeOverlaps(root, ['add-kilo'])).toEqual([]); + 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 for a root with no changes at all', async () => { - expect(await detectChangeOverlaps(root)).toEqual([]); + it('returns nothing when there are no changes at all', async () => { + expect(await scan([])).toEqual([]); }); }); From e1b3704383ff5bd1c8c47212c1fcb2eb0caf32f5 Mon Sep 17 00:00:00 2001 From: Ryan de Melo Date: Sat, 22 Aug 2026 09:25:10 +0800 Subject: [PATCH 3/3] fix(overlap): order overlap output by code point, not locale localeCompare follows the process ICU locale, so non-ASCII spec ids and requirement names could order differently between machines - defeating the stable, diffable output findOverlaps documents. Extract the code-point comparator discoverSpecFiles already used into a shared util and use it on both sort paths. Also tag the CLI output fence in docs/cli.md as text (MD040). --- docs/cli.md | 2 +- src/core/change-overlap.ts | 11 ++++++++--- src/utils/compare.ts | 12 ++++++++++++ src/utils/spec-discovery.ts | 8 ++++---- test/core/change-overlap.test.ts | 20 ++++++++++++++++++++ 5 files changed, 45 insertions(+), 8 deletions(-) create mode 100644 src/utils/compare.ts diff --git a/docs/cli.md b/docs/cli.md index 844f01369c..1f6e6a9aab 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -571,7 +571,7 @@ When changes are in scope (`--changes` or `--all`), validation also reports requ 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 diff --git a/src/core/change-overlap.ts b/src/core/change-overlap.ts index 33f9511b85..35eabad375 100644 --- a/src/core/change-overlap.ts +++ b/src/core/change-overlap.ts @@ -1,6 +1,7 @@ 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, @@ -210,7 +211,10 @@ export async function loadBaseRequirements( /** * 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. + * 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[], @@ -233,7 +237,8 @@ export function findOverlaps( if (changeIds.size < 2) continue; const sorted = [...group].sort( - (a, b) => a.changeId.localeCompare(b.changeId) || a.operation.localeCompare(b.operation) + (a, b) => + compareCodePoints(a.changeId, b.changeId) || compareCodePoints(a.operation, b.operation) ); overlaps.push({ specId: group[0].specId, @@ -248,7 +253,7 @@ export function findOverlaps( } return overlaps.sort( - (a, b) => a.specId.localeCompare(b.specId) || a.requirement.localeCompare(b.requirement) + (a, b) => compareCodePoints(a.specId, b.specId) || compareCodePoints(a.requirement, b.requirement) ); } 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/core/change-overlap.test.ts b/test/core/change-overlap.test.ts index 9ce7ffe331..1dbd0eb789 100644 --- a/test/core/change-overlap.test.ts +++ b/test/core/change-overlap.test.ts @@ -227,6 +227,26 @@ describe('findOverlaps', () => { ]); 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', () => {