mirror of
https://github.com/Fission-AI/OpenSpec.git
synced 2026-10-02 05:24:34 +08:00
fix(archive): refuse requirement names differing only in case (#1864)
ADDED and the RENAMED target compared requirement names exactly, while REMOVED and the RENAMED source already treated a name that differs only in case or interior whitespace as a mistyped header. An ADDED `late fees` beside an existing `Late Fees`, or a rename to `LATE FEES`, therefore archived cleanly and left two contradicting copies of one requirement in the main spec, which validate then accepted. Both now refuse with an error naming the existing requirement. The source of a rename is exempt from the target check, so a case-only rename of a requirement to its own name still applies, and ADDED is still checked against the spec as it stands after the earlier operations, so a variant of a requirement the same delta removes or renames away is allowed.
This commit is contained in:
Dwin Gharibi
@@ -0,0 +1,5 @@
|
||||
---
|
||||
'@fission-ai/openspec': patch
|
||||
---
|
||||
|
||||
Stop archive adding a second copy of an existing requirement under a name that differs only in case or spacing. ADDED and the RENAMED target compared requirement names exactly, while REMOVED and the RENAMED source already treated a case or whitespace variant as a mistyped header, so an ADDED `late fees` beside an existing `Late Fees`, or a rename to `LATE FEES`, archived cleanly and left two contradicting requirements in the main spec, which `validate` then accepted. Both now refuse with an error naming the existing requirement, in the same form REMOVED already used. The exact-duplicate error is unchanged, a case-only rename of a requirement to its own name still works, and a variant of a requirement the same delta removes or renames away is still allowed, because ADDED is checked against the spec as it stands after the earlier operations, as the exact check already was.
|
||||
@@ -21,9 +21,10 @@ export function normalizeRequirementName(name: string): string {
|
||||
/**
|
||||
* Case- and whitespace-insensitive fold of a requirement name. Requirement
|
||||
* matching itself is case-sensitive (normalizeRequirementName); this fold
|
||||
* exists only for typo detection - near-miss REMOVED headers and the
|
||||
* RENAMED+REMOVED cross-section conflict - where two spellings that differ
|
||||
* only in case or interior whitespace mean a mistake, never two requirements.
|
||||
* exists only for typo detection - near-miss REMOVED, ADDED and RENAMED
|
||||
* headers and the RENAMED+REMOVED cross-section conflict - where two spellings
|
||||
* that differ only in case or interior whitespace mean a mistake, never two
|
||||
* requirements.
|
||||
*/
|
||||
export function foldRequirementName(name: string): string {
|
||||
return normalizeRequirementName(name).toLowerCase().replace(/\s+/g, ' ');
|
||||
|
||||
@@ -448,6 +448,17 @@ export async function buildUpdatedSpec(
|
||||
if (nameToBlock.has(to)) {
|
||||
throw new Error(`${specName} RENAMED failed for header "### Requirement: ${r.to}" - target already exists`);
|
||||
}
|
||||
// A target that differs from another requirement only in case or interior
|
||||
// whitespace would leave two copies of one requirement. The source itself
|
||||
// is exempt, so a case-only rename of a requirement stays allowed.
|
||||
const targetNearMiss = [...nameToBlock.keys()].find(
|
||||
(k) => k !== from && foldRequirementName(k) === foldRequirementName(to)
|
||||
);
|
||||
if (targetNearMiss !== undefined) {
|
||||
throw new Error(
|
||||
`${specName} RENAMED failed for header "### Requirement: ${r.to}" - "### Requirement: ${nameToBlock.get(targetNearMiss)!.name}" already exists and differs only in case or spacing; choose a distinct name`
|
||||
);
|
||||
}
|
||||
const block = nameToBlock.get(from)!;
|
||||
const newHeader = `### Requirement: ${to}`;
|
||||
const rawLines = block.raw.split('\n');
|
||||
@@ -541,6 +552,17 @@ export async function buildUpdatedSpec(
|
||||
}
|
||||
throw new Error(`${specName} ADDED failed for header "### Requirement: ${add.name}" - already exists`);
|
||||
}
|
||||
// A name that differs from an existing requirement only in case or
|
||||
// interior whitespace is that requirement written again: adding it would
|
||||
// leave two contradicting copies in the spec. Like the exact check above,
|
||||
// this compares against the spec as it stands after the earlier operations,
|
||||
// so a variant of a requirement this delta removed or renamed away is fine.
|
||||
const nearMiss = [...nameToBlock.keys()].find((k) => foldRequirementName(k) === foldRequirementName(key));
|
||||
if (nearMiss !== undefined) {
|
||||
throw new Error(
|
||||
`${specName} ADDED failed for header "### Requirement: ${add.name}" - "### Requirement: ${nameToBlock.get(nearMiss)!.name}" already exists and differs only in case or spacing; use MODIFIED with that exact header to change it, or choose a distinct name`
|
||||
);
|
||||
}
|
||||
nameToBlock.set(key, add);
|
||||
addedApplied++;
|
||||
}
|
||||
|
||||
@@ -0,0 +1,147 @@
|
||||
import { afterAll, describe, expect, it } from 'vitest';
|
||||
import { promises as fs } from 'fs';
|
||||
import path from 'path';
|
||||
import os from 'os';
|
||||
import { runCLI } from '../helpers/run-cli.js';
|
||||
|
||||
/**
|
||||
* End to end: archive must refuse an ADDED or RENAMED name that differs only in
|
||||
* case or spacing from an existing requirement, instead of writing a second,
|
||||
* contradicting copy of it into the main spec.
|
||||
*/
|
||||
const tempRoots: string[] = [];
|
||||
afterAll(async () => {
|
||||
await Promise.all(tempRoots.map((dir) => fs.rm(dir, { recursive: true, force: true })));
|
||||
});
|
||||
const TIMEOUT = 120_000;
|
||||
const exists = (p: string) => fs.access(p).then(() => true, () => false);
|
||||
|
||||
const PROPOSAL = [
|
||||
'# Edit billing',
|
||||
'',
|
||||
'## Why',
|
||||
'We need to keep the billing contract accurate for operators and customers over time.',
|
||||
'',
|
||||
'## What Changes',
|
||||
'- **billing**: updates billing requirements',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
const SEED = [
|
||||
'## ADDED Requirements',
|
||||
'### Requirement: Invoice Generation',
|
||||
'The system SHALL generate an invoice for every completed billing period.',
|
||||
'',
|
||||
'#### Scenario: Period closes',
|
||||
'- **WHEN** a billing period closes',
|
||||
'- **THEN** an invoice is generated',
|
||||
'',
|
||||
'### Requirement: Late Fees',
|
||||
'The system SHALL apply a late fee to invoices overdue by 30 days.',
|
||||
'',
|
||||
'#### Scenario: Thirty days overdue',
|
||||
'- **WHEN** an invoice is 30 days overdue',
|
||||
'- **THEN** a late fee is applied',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
const addedLateFees = (name: string) =>
|
||||
[
|
||||
'## ADDED Requirements',
|
||||
`### Requirement: ${name}`,
|
||||
'The system SHALL apply a late fee of 10 percent to invoices overdue by 15 days.',
|
||||
'',
|
||||
'#### Scenario: Fifteen days overdue',
|
||||
'- **WHEN** an invoice is 15 days overdue',
|
||||
'- **THEN** a 10 percent late fee is applied',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
const renamed = (from: string, to: string) =>
|
||||
['## RENAMED Requirements', `- FROM: \`### Requirement: ${from}\``, `- TO: \`### Requirement: ${to}\``, ''].join('\n');
|
||||
|
||||
/**
|
||||
* A project whose main billing spec was written by archiving a seed change,
|
||||
* plus an open `edit` change waiting for a delta.
|
||||
*/
|
||||
async function seededProject() {
|
||||
const base = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-near-miss-e2e-'));
|
||||
tempRoots.push(base);
|
||||
const home = path.join(base, 'home');
|
||||
const project = path.join(base, 'project');
|
||||
await fs.mkdir(home, { recursive: true });
|
||||
await fs.mkdir(project, { recursive: true });
|
||||
const env = {
|
||||
HOME: home,
|
||||
USERPROFILE: home,
|
||||
XDG_CONFIG_HOME: path.join(home, '.config'),
|
||||
XDG_DATA_HOME: path.join(home, '.local', 'share'),
|
||||
OPENSPEC_NO_ANIMATION: '1',
|
||||
};
|
||||
const cli = (args: string[]) => runCLI(args, { cwd: project, env, timeoutMs: 60_000 });
|
||||
|
||||
expect((await cli(['init', '--tools', 'claude'])).exitCode).toBe(0);
|
||||
expect((await cli(['new', 'change', 'seed'])).exitCode).toBe(0);
|
||||
const seedDir = path.join(project, 'openspec', 'changes', 'seed');
|
||||
await fs.mkdir(path.join(seedDir, 'specs', 'billing'), { recursive: true });
|
||||
await fs.writeFile(path.join(seedDir, 'proposal.md'), PROPOSAL);
|
||||
await fs.writeFile(path.join(seedDir, 'tasks.md'), '## 1. Work\n- [x] 1.1 Done\n');
|
||||
await fs.writeFile(path.join(seedDir, 'specs', 'billing', 'spec.md'), SEED);
|
||||
expect((await cli(['archive', 'seed', '--yes'])).exitCode).toBe(0);
|
||||
|
||||
expect((await cli(['new', 'change', 'edit'])).exitCode).toBe(0);
|
||||
const editDir = path.join(project, 'openspec', 'changes', 'edit');
|
||||
await fs.mkdir(path.join(editDir, 'specs', 'billing'), { recursive: true });
|
||||
await fs.writeFile(path.join(editDir, 'proposal.md'), PROPOSAL);
|
||||
await fs.writeFile(path.join(editDir, 'tasks.md'), '## 1. Work\n- [x] 1.1 Done\n');
|
||||
|
||||
const mainSpec = path.join(project, 'openspec', 'specs', 'billing', 'spec.md');
|
||||
return {
|
||||
cli,
|
||||
editDir,
|
||||
writeDelta: (body: string) => fs.writeFile(path.join(editDir, 'specs', 'billing', 'spec.md'), body),
|
||||
headers: async () =>
|
||||
(await fs.readFile(mainSpec, 'utf-8'))
|
||||
.split('\n')
|
||||
.filter((line) => line.startsWith('### Requirement:'))
|
||||
.map((line) => line.slice('### Requirement:'.length).trim()),
|
||||
};
|
||||
}
|
||||
|
||||
describe('archive refuses requirement names that differ only in case or spacing', () => {
|
||||
it('refuses an exact duplicate ADDED name (control)', async () => {
|
||||
const p = await seededProject();
|
||||
await p.writeDelta(addedLateFees('Late Fees'));
|
||||
const result = await p.cli(['archive', 'edit', '--yes']);
|
||||
expect(result.exitCode).not.toBe(0);
|
||||
expect(result.stdout + result.stderr).toMatch(/already exists/);
|
||||
}, TIMEOUT);
|
||||
|
||||
it('refuses an ADDED name that differs only in case and leaves the spec and change untouched', async () => {
|
||||
const p = await seededProject();
|
||||
await p.writeDelta(addedLateFees('late fees'));
|
||||
const result = await p.cli(['archive', 'edit', '--yes']);
|
||||
expect(result.exitCode).not.toBe(0);
|
||||
expect(result.stdout + result.stderr).toContain(
|
||||
'"### Requirement: Late Fees" already exists and differs only in case or spacing'
|
||||
);
|
||||
expect(await p.headers()).toEqual(['Invoice Generation', 'Late Fees']);
|
||||
expect(await exists(p.editDir)).toBe(true);
|
||||
}, TIMEOUT);
|
||||
|
||||
it('refuses a RENAMED target that differs only in case from another requirement', async () => {
|
||||
const p = await seededProject();
|
||||
await p.writeDelta(renamed('Invoice Generation', 'LATE FEES'));
|
||||
const result = await p.cli(['archive', 'edit', '--yes']);
|
||||
expect(result.exitCode).not.toBe(0);
|
||||
expect(await p.headers()).toEqual(['Invoice Generation', 'Late Fees']);
|
||||
}, TIMEOUT);
|
||||
|
||||
it('still archives a case-only rename of a requirement to its own name', async () => {
|
||||
const p = await seededProject();
|
||||
await p.writeDelta(renamed('Late Fees', 'LATE FEES'));
|
||||
const result = await p.cli(['archive', 'edit', '--yes']);
|
||||
expect(result.exitCode).toBe(0);
|
||||
expect(await p.headers()).toEqual(['Invoice Generation', 'LATE FEES']);
|
||||
}, TIMEOUT);
|
||||
});
|
||||
@@ -0,0 +1,219 @@
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { promises as fs } from 'fs';
|
||||
import path from 'path';
|
||||
import os from 'os';
|
||||
import { buildUpdatedSpec, findSpecUpdates } from '../../src/core/specs-apply.js';
|
||||
|
||||
/**
|
||||
* Two requirement names that differ only in case or interior whitespace are
|
||||
* one requirement written twice (foldRequirementName). REMOVED and the RENAMED
|
||||
* source already refused such a near-miss, but ADDED and the RENAMED target
|
||||
* compared names exactly: archive wrote `late fees` next to `Late Fees`, two
|
||||
* contradicting copies of one requirement, and `validate` accepted the result.
|
||||
*/
|
||||
describe('buildUpdatedSpec (requirement name near-misses)', () => {
|
||||
let tempDir: string;
|
||||
|
||||
beforeEach(async () => {
|
||||
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-near-miss-'));
|
||||
});
|
||||
afterEach(async () => {
|
||||
await fs.rm(tempDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
const MAIN_SPEC = [
|
||||
'# billing Specification',
|
||||
'',
|
||||
'## Purpose',
|
||||
'Defines how billing behaves for customers and operators.',
|
||||
'',
|
||||
'## Requirements',
|
||||
'### Requirement: Invoice Generation',
|
||||
'The system SHALL generate an invoice for every completed billing period.',
|
||||
'',
|
||||
'#### Scenario: Period closes',
|
||||
'- **WHEN** a billing period closes',
|
||||
'- **THEN** an invoice is generated',
|
||||
'',
|
||||
'### Requirement: Late Fees',
|
||||
'The system SHALL apply a late fee to invoices overdue by 30 days.',
|
||||
'',
|
||||
'#### Scenario: Thirty days overdue',
|
||||
'- **WHEN** an invoice is 30 days overdue',
|
||||
'- **THEN** a late fee is applied',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
/**
|
||||
* Write the main spec and a delta into a temp project, then run the merge
|
||||
* without touching any real project.
|
||||
*/
|
||||
async function build(deltaBody: string) {
|
||||
const specsRoot = path.join(tempDir, 'openspec', 'specs');
|
||||
const specsDir = path.join(specsRoot, 'billing');
|
||||
const changeDir = path.join(tempDir, 'openspec', 'changes', 'c');
|
||||
await fs.mkdir(specsDir, { recursive: true });
|
||||
await fs.mkdir(path.join(changeDir, 'specs', 'billing'), { recursive: true });
|
||||
await fs.writeFile(path.join(specsDir, 'spec.md'), MAIN_SPEC);
|
||||
await fs.writeFile(path.join(changeDir, 'specs', 'billing', 'spec.md'), deltaBody);
|
||||
const [update] = await findSpecUpdates(changeDir, specsRoot);
|
||||
return buildUpdatedSpec(update, 'c', { silent: true });
|
||||
}
|
||||
|
||||
/** One ADDED requirement, by default contradicting the seeded `Late Fees`. */
|
||||
const added = (
|
||||
name: string,
|
||||
body = 'The system SHALL apply a late fee of 10 percent to invoices overdue by 15 days.'
|
||||
) =>
|
||||
[
|
||||
`### Requirement: ${name}`,
|
||||
body,
|
||||
'',
|
||||
'#### Scenario: Overdue',
|
||||
'- **WHEN** an invoice is overdue',
|
||||
'- **THEN** a late fee is applied',
|
||||
'',
|
||||
].join('\n');
|
||||
|
||||
const renamed = (from: string, to: string) =>
|
||||
['## RENAMED Requirements', `- FROM: \`### Requirement: ${from}\``, `- TO: \`### Requirement: ${to}\``, ''].join(
|
||||
'\n'
|
||||
);
|
||||
|
||||
const headers = (spec: string) =>
|
||||
spec
|
||||
.split('\n')
|
||||
.filter((line) => line.startsWith('### Requirement:'))
|
||||
.map((line) => line.slice('### Requirement:'.length).trim());
|
||||
|
||||
const ADDED_NEAR_MISS = (name: string, existing: string) =>
|
||||
`billing ADDED failed for header "### Requirement: ${name}" - "### Requirement: ${existing}" already exists and differs only in case or spacing`;
|
||||
const RENAMED_NEAR_MISS = (name: string, existing: string) =>
|
||||
`billing RENAMED failed for header "### Requirement: ${name}" - "### Requirement: ${existing}" already exists and differs only in case or spacing`;
|
||||
|
||||
describe('ADDED', () => {
|
||||
it('adds a requirement whose name is distinct (control)', async () => {
|
||||
const result = await build(['## ADDED Requirements', added('Credit Notes')].join('\n'));
|
||||
expect(headers(result.rebuilt)).toEqual(['Invoice Generation', 'Late Fees', 'Credit Notes']);
|
||||
expect(result.counts.added).toBe(1);
|
||||
});
|
||||
|
||||
it('still refuses an exact duplicate name with different content (control)', async () => {
|
||||
await expect(build(['## ADDED Requirements', added('Late Fees')].join('\n'))).rejects.toThrow(
|
||||
'billing ADDED failed for header "### Requirement: Late Fees" - already exists'
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a name that differs only in case from an existing requirement', async () => {
|
||||
await expect(build(['## ADDED Requirements', added('late fees')].join('\n'))).rejects.toThrow(
|
||||
ADDED_NEAR_MISS('late fees', 'Late Fees')
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a name that differs only in interior whitespace', async () => {
|
||||
await expect(build(['## ADDED Requirements', added('Late Fees')].join('\n'))).rejects.toThrow(
|
||||
ADDED_NEAR_MISS('Late Fees', 'Late Fees')
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a case variant even when its body matches the existing requirement', async () => {
|
||||
// Identical content is the early-sync no-op only for the exact header; a
|
||||
// case variant would still write a second header.
|
||||
const sameBody = added('LATE FEES', 'The system SHALL apply a late fee to invoices overdue by 30 days.');
|
||||
await expect(build(['## ADDED Requirements', sameBody].join('\n'))).rejects.toThrow(
|
||||
ADDED_NEAR_MISS('LATE FEES', 'Late Fees')
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses two ADDED requirements in one delta that differ only in case', async () => {
|
||||
await expect(
|
||||
build(['## ADDED Requirements', added('Credit Notes'), added('credit notes')].join('\n'))
|
||||
).rejects.toThrow(ADDED_NEAR_MISS('credit notes', 'Credit Notes'));
|
||||
});
|
||||
|
||||
it('allows a case variant of a requirement the same delta removes', async () => {
|
||||
// Operations apply RENAMED -> REMOVED -> MODIFIED -> ADDED, and ADDED is
|
||||
// checked against the spec as it stands after the earlier operations, the
|
||||
// same order the exact-name check already uses. The old spelling is gone
|
||||
// by then, so the result holds one requirement, not two.
|
||||
const result = await build(
|
||||
[
|
||||
'## REMOVED Requirements',
|
||||
'### Requirement: Late Fees',
|
||||
'**Reason**: replaced by the stricter policy below',
|
||||
'',
|
||||
'## ADDED Requirements',
|
||||
added('late fees'),
|
||||
].join('\n')
|
||||
);
|
||||
expect(headers(result.rebuilt)).toEqual(['Invoice Generation', 'late fees']);
|
||||
expect(result.counts).toMatchObject({ removed: 1, added: 1 });
|
||||
});
|
||||
|
||||
it('allows a case variant of a requirement the same delta renames away', async () => {
|
||||
const result = await build(
|
||||
[renamed('Late Fees', 'Overdue Fees'), '## ADDED Requirements', added('late fees')].join('\n')
|
||||
);
|
||||
expect(headers(result.rebuilt)).toEqual(['Invoice Generation', 'Overdue Fees', 'late fees']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('RENAMED', () => {
|
||||
it('still refuses a target that exists exactly (control)', async () => {
|
||||
await expect(build(renamed('Invoice Generation', 'Late Fees'))).rejects.toThrow(
|
||||
'billing RENAMED failed for header "### Requirement: Late Fees" - target already exists'
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a target that differs only in case from another requirement', async () => {
|
||||
await expect(build(renamed('Invoice Generation', 'LATE FEES'))).rejects.toThrow(
|
||||
RENAMED_NEAR_MISS('LATE FEES', 'Late Fees')
|
||||
);
|
||||
});
|
||||
|
||||
it('refuses a target that differs only in interior whitespace from another requirement', async () => {
|
||||
await expect(build(renamed('Invoice Generation', 'Late Fees'))).rejects.toThrow(
|
||||
RENAMED_NEAR_MISS('Late Fees', 'Late Fees')
|
||||
);
|
||||
});
|
||||
|
||||
it('still allows a case-only rename of a requirement to its own name', async () => {
|
||||
const result = await build(renamed('Late Fees', 'LATE FEES'));
|
||||
expect(headers(result.rebuilt)).toEqual(['Invoice Generation', 'LATE FEES']);
|
||||
expect(result.rebuilt).toContain('overdue by 30 days');
|
||||
expect(result.counts.renamed).toBe(1);
|
||||
});
|
||||
|
||||
it('refuses a target that collides with a requirement the same delta removes, as an exact target does', async () => {
|
||||
// RENAMED runs before REMOVED, so the removed requirement still exists
|
||||
// when the target is checked. That was already true for an exact target.
|
||||
const removeLateFees = ['## REMOVED Requirements', '### Requirement: Late Fees', '**Reason**: gone', ''].join('\n');
|
||||
await expect(build([renamed('Invoice Generation', 'Late Fees'), removeLateFees].join('\n'))).rejects.toThrow(
|
||||
'target already exists'
|
||||
);
|
||||
await expect(build([renamed('Invoice Generation', 'late fees'), removeLateFees].join('\n'))).rejects.toThrow(
|
||||
RENAMED_NEAR_MISS('late fees', 'Late Fees')
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('MODIFIED', () => {
|
||||
it('still refuses a header that matches an existing requirement only by case', async () => {
|
||||
// Unchanged: MODIFIED already refused this, as "not found".
|
||||
await expect(
|
||||
build(
|
||||
[
|
||||
'## MODIFIED Requirements',
|
||||
'### Requirement: late fees',
|
||||
'The system SHALL apply a late fee to invoices overdue by 45 days.',
|
||||
'',
|
||||
'#### Scenario: Thirty days overdue',
|
||||
'- **WHEN** an invoice is 45 days overdue',
|
||||
'- **THEN** a late fee is applied',
|
||||
'',
|
||||
].join('\n')
|
||||
)
|
||||
).rejects.toThrow('billing MODIFIED failed for header "### Requirement: late fees" - not found');
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user