Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

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

archive: preserve a requirement's original position when renaming it instead of moving the renamed block to the end of the spec.
20 changes: 13 additions & 7 deletions src/core/specs-apply.ts
Original file line number Diff line number Diff line change
Expand Up @@ -371,11 +371,13 @@ export async function buildUpdatedSpec(
for (const block of parts.bodyBlocks) {
nameToBlock.set(normalizeRequirementName(block.name), block);
}
// Keep source blocks immutable for loss attribution. This parallel key list
// carries only positional identity as renames change lookup keys.
const orderedKeys = parts.bodyBlocks.map((block) => normalizeRequirementName(block.name));

// Apply operations in order: RENAMED → REMOVED → MODIFIED → ADDED
// RENAMED
let renamedApplied = 0;
const renamedTargets = new Map<string, string>();
for (const r of plan.renamed) {
const from = normalizeRequirementName(r.from);
const to = normalizeRequirementName(r.to);
Expand Down Expand Up @@ -413,7 +415,12 @@ export async function buildUpdatedSpec(
};
nameToBlock.delete(from);
nameToBlock.set(to, renamedBlock);
renamedTargets.set(from, to);
// A Map delete+set moves the renamed block to insertion-order tail. Carry
// its new key in the source slot instead; chained renames update it again.
const orderIndex = orderedKeys.indexOf(from);
if (orderIndex >= 0) {
orderedKeys[orderIndex] = to;
}
renamedApplied++;
}

Expand Down Expand Up @@ -499,8 +506,9 @@ export async function buildUpdatedSpec(
// Recompose requirements section preserving original ordering where possible
const keptOrder: RequirementBlock[] = [];
const seen = new Set<string>();
for (const block of parts.bodyBlocks) {
const key = normalizeRequirementName(block.name);
for (let index = 0; index < parts.bodyBlocks.length; index++) {
const block = parts.bodyBlocks[index];
const key = orderedKeys[index];
const replacement = nameToBlock.get(key);
if (replacement) {
keptOrder.push(replacement);
Expand All @@ -512,9 +520,7 @@ export async function buildUpdatedSpec(
// full absorbed suffix. RENAMED carries the original raw content under a
// new map key, and MODIFIED may repeat the suffix deliberately; neither is
// data loss.
const renamedTarget = renamedTargets.get(key);
const replacementFromOriginal =
replacement ?? (renamedTarget ? nameToBlock.get(renamedTarget) : undefined);
const replacementFromOriginal = replacement;
if (replacementFromOriginal !== block) {
const foreign = firstForeignTail(block.raw);
const replacementRaw = replacementFromOriginal?.raw;
Expand Down
124 changes: 124 additions & 0 deletions test/core/archive.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2780,6 +2780,130 @@ content D`;
expect(updated).not.toContain('### Requirement: B');
});

it('should preserve source order and lineage when renaming requirements', async () => {
const changeName = 'rename-order';
const renamed = (pairs: Array<[string, string]>): string =>
`## RENAMED Requirements\n\n${pairs
.map(
([from, to]) =>
`- FROM: \`### Requirement: ${from}\`\n- TO: \`### Requirement: ${to}\``
)
.join('\n\n')}`;
const cases = [
{ capability: 'first', names: ['A', 'B', 'C'], delta: renamed([['A', 'A2']]), expected: ['A2', 'B', 'C'] },
{ capability: 'middle', names: ['A', 'B', 'C'], delta: renamed([['B', 'B2']]), expected: ['A', 'B2', 'C'] },
{ capability: 'last', names: ['A', 'B', 'C'], delta: renamed([['C', 'C2']]), expected: ['A', 'B', 'C2'] },
{
capability: 'multiple',
names: ['A', 'B', 'C'],
delta: renamed([['B', 'B2'], ['A', 'A2']]),
expected: ['A2', 'B2', 'C'],
},
{
capability: 'chained',
names: ['X', 'A', 'Y'],
delta: renamed([['A', 'B'], ['B', 'C']]),
expected: ['X', 'C', 'Y'],
},
{
capability: 'modified',
names: ['A', 'B', 'C'],
delta: `${renamed([['B', 'B2']])}\n\n## MODIFIED Requirements\n\n### Requirement: B2\nModified body.`,
expected: ['A', 'B2', 'C'],
expectedContent: '### Requirement: B2\nModified body.',
},
{
capability: 'readded',
names: ['X', 'A', 'Y'],
delta: `${renamed([['A', 'B']])}\n\n## ADDED Requirements\n\n### Requirement: A\nNew body A.`,
expected: ['X', 'B', 'Y', 'A'],
},
{
capability: 'foreign-tail',
names: ['A', 'B', 'C'],
delta: renamed([['B', 'B2']]),
expected: ['A', 'B2', 'C'],
foreignTail: '### Notes\nAuthored note travels with B.',
expectedContent: '### Requirement: B2\nBody B.\n\n### Notes\nAuthored note travels with B.',
},
];

for (const item of cases) {
const mainSpecDir = path.join(tempDir, 'openspec', 'specs', item.capability);
const changeSpecDir = path.join(
tempDir,
'openspec',
'changes',
changeName,
'specs',
item.capability
);
await fs.mkdir(mainSpecDir, { recursive: true });
await fs.mkdir(changeSpecDir, { recursive: true });
const blocks = item.names.map(
(name) =>
`### Requirement: ${name}\nBody ${name}.` +
(name === 'B' && item.foreignTail ? `\n\n${item.foreignTail}` : '')
);
await fs.writeFile(
path.join(mainSpecDir, 'spec.md'),
`# ${item.capability} Specification\n\n## Purpose\nOrdering fixture.\n\n## Requirements\n\n${blocks.join('\n\n')}\n`
);
await fs.writeFile(
path.join(changeSpecDir, 'spec.md'),
`# ${item.capability} - Changes\n\n${item.delta}\n`
);
}

await archiveCommand.execute(changeName, { yes: true, noValidate: true });

for (const item of cases) {
const updated = await fs.readFile(
path.join(tempDir, 'openspec', 'specs', item.capability, 'spec.md'),
'utf-8'
);
const names = [...updated.matchAll(/^### Requirement:\s*(.+?)\s*$/gm)].map(
(match) => match[1]
);
expect(names).toEqual(item.expected);
if (item.expectedContent) expect(updated).toContain(item.expectedContent);
}
const output = (console.log as unknown as { mock: { calls: unknown[][] } }).mock.calls
.flat()
.map(String)
.join('\n');
expect(output).not.toContain('sits inside requirement "B"');
});

it('should keep the target and change untouched when a later rename collides', async () => {
const changeName = 'late-rename-collision';
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);
const changeSpecDir = path.join(changeDir, 'specs', 'demo');
const mainSpecDir = path.join(tempDir, 'openspec', 'specs', 'demo');
const mainSpecPath = path.join(mainSpecDir, 'spec.md');
const changeSpecPath = path.join(changeSpecDir, 'spec.md');
const mainContent = `# demo Specification\n\n## Purpose\nTransaction fixture.\n\n## Requirements\n\n### Requirement: A\nBody A.\n\n### Requirement: B\nBody B.\n\n### Requirement: C\nBody C.\n`;
const changeContent = `# demo - Changes\n\n## RENAMED Requirements\n\n- FROM: \`### Requirement: A\`\n- TO: \`### Requirement: A2\`\n\n- FROM: \`### Requirement: A2\`\n- TO: \`### Requirement: C\`\n`;
await fs.mkdir(changeSpecDir, { recursive: true });
await fs.mkdir(mainSpecDir, { recursive: true });
await fs.writeFile(mainSpecPath, mainContent);
await fs.writeFile(changeSpecPath, changeContent);

await archiveCommand.execute(changeName, { yes: true, noValidate: true });

expect(console.log).toHaveBeenCalledWith(
expect.stringContaining(
'RENAMED failed for header "### Requirement: C" - target already exists'
)
);
expect(process.exitCode).toBe(1);
await expect(fs.readFile(mainSpecPath, 'utf-8')).resolves.toBe(mainContent);
await expect(fs.readFile(changeSpecPath, 'utf-8')).resolves.toBe(changeContent);
await expect(fs.access(changeDir)).resolves.not.toThrow();
const archives = await fs.readdir(path.join(tempDir, 'openspec', 'changes', 'archive'));
expect(archives.some((entry) => entry.includes(changeName))).toBe(false);
});

it('should abort with error when MODIFIED references non-existent requirements', async () => {
const changeName = 'validate-missing';
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);
Expand Down
30 changes: 30 additions & 0 deletions test/core/specs-apply.salvage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,36 @@ describe('buildUpdatedSpec (content absorbed into a requirement)', () => {
expect(warnings.join('\n')).not.toContain('goes with it');
});

it('warns against the source requirement when a rename-plus-modify drops its tail', async () => {
const tail = [' ### Notes', 'Kept by hand.'];
const renamedRequirement = [...REQUIREMENT];
renamedRequirement[0] = '### Requirement: Renamed';
const { rebuilt, counts, warnings } = await build(SPEC(tail), [
'# demo - Changes',
'',
'## RENAMED Requirements',
'',
'- FROM: `### Requirement: Target`',
'- TO: `### Requirement: Renamed`',
'',
'## MODIFIED Requirements',
'',
...renamedRequirement,
'',
]);

expect([...rebuilt.matchAll(/^### Requirement:\s*(.+?)\s*$/gm)].map((m) => m[1])).toEqual([
'Renamed',
'Other',
]);
expect(rebuilt).not.toContain(tail.join('\n'));
expect(counts).toMatchObject({ modified: 1, renamed: 1 });
expect(warnings.join('\n')).toContain(
'"### Notes" sits inside requirement "Target" and goes with it'
);
expect(warnings.join('\n')).not.toContain('requirement "Renamed"');
});

it('does not warn when MODIFIED carries the full absorbed tail forward', async () => {
const tail = [' ### Notes', 'Kept by hand.'];
const { rebuilt, counts, warnings } = await build(SPEC(tail), [
Expand Down
Loading