fix(gsd): preserve freeform DECISIONS.md content on decision save (#2319)
`saveDecisionToDb` previously regenerated DECISIONS.md from DB state unconditionally, which silently destroyed any freeform/prose content since `parseDecisionsTable` only parses table rows. Now detects whether the existing file is in canonical table format (starts with "# Decisions Register" + has the standard table header). When freeform content is detected, the original content is preserved and a decisions table section is appended/updated at the end instead of overwriting the entire file. Fixes #2301 Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
6793489b78
commit
30daeeb8f4
2 changed files with 317 additions and 1 deletions
|
|
@ -9,6 +9,7 @@
|
|||
// parseDecisionsTable() and parseRequirementsSections() with field fidelity.
|
||||
|
||||
import { join, resolve } from 'node:path';
|
||||
import { readFileSync, existsSync } from 'node:fs';
|
||||
import type { Decision, Requirement } from './types.js';
|
||||
import { resolveGsdRootFile } from './paths.js';
|
||||
import { saveFile } from './files.js';
|
||||
|
|
@ -17,6 +18,58 @@ import { invalidateStateCache } from './state.js';
|
|||
import { clearPathCache } from './paths.js';
|
||||
import { clearParseCache } from './files.js';
|
||||
|
||||
// ─── Freeform Detection ───────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Detect whether a DECISIONS.md file is in canonical table format
|
||||
* (generated by generateDecisionsMd).
|
||||
*
|
||||
* Returns true only if the file starts with the canonical header
|
||||
* ("# Decisions Register") that generateDecisionsMd produces.
|
||||
* Files with freeform content — even if they contain an appended
|
||||
* decisions table section — return false so the freeform content
|
||||
* is preserved.
|
||||
*/
|
||||
export function isDecisionsTableFormat(content: string): boolean {
|
||||
// The canonical format always starts with "# Decisions Register"
|
||||
const firstLine = content.split('\n')[0]?.trim() ?? '';
|
||||
if (firstLine !== '# Decisions Register') return false;
|
||||
|
||||
// Additionally verify the file has the canonical table header
|
||||
return content.includes('| # | When | Scope | Decision | Choice | Rationale | Revisable?');
|
||||
}
|
||||
|
||||
/**
|
||||
* Generate a minimal decisions table section (header + rows) for appending
|
||||
* to a freeform DECISIONS.md file.
|
||||
*/
|
||||
function generateDecisionsAppendBlock(decisions: Decision[]): string {
|
||||
const lines: string[] = [];
|
||||
lines.push('');
|
||||
lines.push('---');
|
||||
lines.push('');
|
||||
lines.push('## Decisions Table');
|
||||
lines.push('');
|
||||
lines.push('| # | When | Scope | Decision | Choice | Rationale | Revisable? | Made By |');
|
||||
lines.push('|---|------|-------|----------|--------|-----------|------------|---------|');
|
||||
|
||||
for (const d of decisions) {
|
||||
const cells = [
|
||||
d.id,
|
||||
d.when_context,
|
||||
d.scope,
|
||||
d.decision,
|
||||
d.choice,
|
||||
d.rationale,
|
||||
d.revisable,
|
||||
d.made_by ?? 'agent',
|
||||
].map(cell => (cell ?? '').replace(/\|/g, '\\|'));
|
||||
lines.push(`| ${cells.join(' | ')} |`);
|
||||
}
|
||||
|
||||
return lines.join('\n') + '\n';
|
||||
}
|
||||
|
||||
// ─── Markdown Generators ──────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
|
|
@ -230,8 +283,31 @@ export async function saveDecisionToDb(
|
|||
}));
|
||||
}
|
||||
|
||||
const md = generateDecisionsMd(allDecisions);
|
||||
const filePath = resolveGsdRootFile(basePath, 'DECISIONS');
|
||||
|
||||
// Check if existing DECISIONS.md has freeform (non-table) content.
|
||||
// If so, preserve that content and append/update the decisions table
|
||||
// at the end instead of overwriting the entire file.
|
||||
let existingContent: string | null = null;
|
||||
if (existsSync(filePath)) {
|
||||
existingContent = readFileSync(filePath, 'utf-8');
|
||||
}
|
||||
|
||||
let md: string;
|
||||
if (existingContent && !isDecisionsTableFormat(existingContent)) {
|
||||
// Freeform content detected — preserve it and append decisions table.
|
||||
// Strip any previously appended decisions table section to avoid duplication.
|
||||
const marker = '---\n\n## Decisions Table';
|
||||
const markerIdx = existingContent.indexOf(marker);
|
||||
const freeformPart = markerIdx >= 0
|
||||
? existingContent.substring(0, markerIdx).trimEnd()
|
||||
: existingContent.trimEnd();
|
||||
md = freeformPart + '\n' + generateDecisionsAppendBlock(allDecisions);
|
||||
} else {
|
||||
// Table format or no existing file — full regeneration (original behavior)
|
||||
md = generateDecisionsMd(allDecisions);
|
||||
}
|
||||
|
||||
await saveFile(filePath, md);
|
||||
// Invalidate file-read caches so deriveState() sees the updated markdown.
|
||||
// Do NOT clear the artifacts table — we just wrote to it intentionally.
|
||||
|
|
|
|||
240
src/resources/extensions/gsd/tests/freeform-decisions.test.ts
Normal file
240
src/resources/extensions/gsd/tests/freeform-decisions.test.ts
Normal file
|
|
@ -0,0 +1,240 @@
|
|||
import { createTestContext } from './test-helpers.ts';
|
||||
import * as path from 'node:path';
|
||||
import * as os from 'node:os';
|
||||
import * as fs from 'node:fs';
|
||||
import {
|
||||
openDatabase,
|
||||
closeDatabase,
|
||||
} from '../gsd-db.ts';
|
||||
import {
|
||||
parseDecisionsTable,
|
||||
} from '../md-importer.ts';
|
||||
import {
|
||||
saveDecisionToDb,
|
||||
} from '../db-writer.ts';
|
||||
|
||||
const { assertEq, assertTrue, report } = createTestContext();
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
// Helpers
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
|
||||
function makeTmpDir(): string {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-freeform-'));
|
||||
fs.mkdirSync(path.join(dir, '.gsd'), { recursive: true });
|
||||
return dir;
|
||||
}
|
||||
|
||||
function cleanupDir(dir: string): void {
|
||||
try {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
} catch { /* swallow */ }
|
||||
}
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
// Bug reproduction: freeform DECISIONS.md content destroyed (#2301)
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
|
||||
console.log('\n── parseDecisionsTable silently drops freeform content ──');
|
||||
|
||||
{
|
||||
const freeform = `# Project Decisions
|
||||
|
||||
## Architecture
|
||||
We decided to use a microservices architecture because monoliths don't scale.
|
||||
|
||||
## Database
|
||||
PostgreSQL was chosen for its reliability and JSONB support.
|
||||
|
||||
## Deployment
|
||||
- Kubernetes for orchestration
|
||||
- Helm charts for packaging
|
||||
`;
|
||||
|
||||
const parsed = parseDecisionsTable(freeform);
|
||||
assertEq(parsed.length, 0, 'freeform content yields zero parsed decisions (expected — it is not a table)');
|
||||
}
|
||||
|
||||
console.log('\n── saveDecisionToDb destroys freeform DECISIONS.md content ──');
|
||||
|
||||
{
|
||||
const tmpDir = makeTmpDir();
|
||||
const dbPath = path.join(tmpDir, '.gsd', 'gsd.db');
|
||||
const mdPath = path.join(tmpDir, '.gsd', 'DECISIONS.md');
|
||||
openDatabase(dbPath);
|
||||
|
||||
const freeformContent = `# Project Decisions
|
||||
|
||||
## Architecture
|
||||
We decided to use a microservices architecture because monoliths don't scale.
|
||||
|
||||
## Database
|
||||
PostgreSQL was chosen for its reliability and JSONB support.
|
||||
|
||||
## Deployment
|
||||
- Kubernetes for orchestration
|
||||
- Helm charts for packaging
|
||||
`;
|
||||
|
||||
// Pre-populate DECISIONS.md with freeform content
|
||||
fs.writeFileSync(mdPath, freeformContent, 'utf-8');
|
||||
|
||||
try {
|
||||
// Save a new decision — this should NOT destroy the freeform content
|
||||
const result = await saveDecisionToDb({
|
||||
scope: 'testing',
|
||||
decision: 'Use Jest for unit tests',
|
||||
choice: 'Jest',
|
||||
rationale: 'Well-known, good DX',
|
||||
when_context: 'M001',
|
||||
}, tmpDir);
|
||||
|
||||
assertEq(result.id, 'D001', 'decision ID assigned correctly');
|
||||
|
||||
// Read back the file
|
||||
const afterContent = fs.readFileSync(mdPath, 'utf-8');
|
||||
|
||||
// The freeform content MUST still be present
|
||||
assertTrue(
|
||||
afterContent.includes('microservices architecture'),
|
||||
'freeform architecture section preserved after saveDecisionToDb',
|
||||
);
|
||||
assertTrue(
|
||||
afterContent.includes('PostgreSQL was chosen'),
|
||||
'freeform database section preserved after saveDecisionToDb',
|
||||
);
|
||||
assertTrue(
|
||||
afterContent.includes('Kubernetes for orchestration'),
|
||||
'freeform deployment section preserved after saveDecisionToDb',
|
||||
);
|
||||
|
||||
// The new decision MUST also be present
|
||||
assertTrue(
|
||||
afterContent.includes('D001'),
|
||||
'new decision D001 present in file',
|
||||
);
|
||||
assertTrue(
|
||||
afterContent.includes('Use Jest for unit tests'),
|
||||
'new decision text present in file',
|
||||
);
|
||||
|
||||
// Save a second decision — freeform content must still survive
|
||||
const result2 = await saveDecisionToDb({
|
||||
scope: 'ci',
|
||||
decision: 'Use GitHub Actions for CI',
|
||||
choice: 'GitHub Actions',
|
||||
rationale: 'Native integration',
|
||||
when_context: 'M001',
|
||||
}, tmpDir);
|
||||
|
||||
assertEq(result2.id, 'D002', 'second decision ID assigned correctly');
|
||||
|
||||
const afterContent2 = fs.readFileSync(mdPath, 'utf-8');
|
||||
|
||||
assertTrue(
|
||||
afterContent2.includes('microservices architecture'),
|
||||
'freeform content still preserved after second save',
|
||||
);
|
||||
assertTrue(
|
||||
afterContent2.includes('D001'),
|
||||
'first decision still present after second save',
|
||||
);
|
||||
assertTrue(
|
||||
afterContent2.includes('D002'),
|
||||
'second decision present after second save',
|
||||
);
|
||||
assertTrue(
|
||||
afterContent2.includes('Use GitHub Actions for CI'),
|
||||
'second decision text present in file',
|
||||
);
|
||||
} finally {
|
||||
closeDatabase();
|
||||
cleanupDir(tmpDir);
|
||||
}
|
||||
}
|
||||
|
||||
console.log('\n── saveDecisionToDb with table-format DECISIONS.md still regenerates normally ──');
|
||||
|
||||
{
|
||||
const tmpDir = makeTmpDir();
|
||||
const dbPath = path.join(tmpDir, '.gsd', 'gsd.db');
|
||||
const mdPath = path.join(tmpDir, '.gsd', 'DECISIONS.md');
|
||||
openDatabase(dbPath);
|
||||
|
||||
// Pre-populate with canonical table format
|
||||
const tableContent = `# Decisions Register
|
||||
|
||||
<!-- Append-only. Never edit or remove existing rows.
|
||||
To reverse a decision, add a new row that supersedes it.
|
||||
Read this file at the start of any planning or research phase. -->
|
||||
|
||||
| # | When | Scope | Decision | Choice | Rationale | Revisable? | Made By |
|
||||
|---|------|-------|----------|--------|-----------|------------|---------|
|
||||
| D001 | M001 | arch | Use REST API | REST | Simpler | Yes | human |
|
||||
`;
|
||||
|
||||
fs.writeFileSync(mdPath, tableContent, 'utf-8');
|
||||
|
||||
try {
|
||||
const result = await saveDecisionToDb({
|
||||
scope: 'testing',
|
||||
decision: 'Use Vitest',
|
||||
choice: 'Vitest',
|
||||
rationale: 'Fast',
|
||||
when_context: 'M001',
|
||||
}, tmpDir);
|
||||
|
||||
// The pre-existing table decision was NOT in DB, so it won't appear after regen.
|
||||
// But the new decision should be there.
|
||||
assertEq(result.id, 'D001', 'gets D001 since DB was empty');
|
||||
|
||||
const afterContent = fs.readFileSync(mdPath, 'utf-8');
|
||||
// Table-format file gets fully regenerated — this is the normal path
|
||||
assertTrue(
|
||||
afterContent.includes('# Decisions Register'),
|
||||
'table-format file still has header after save',
|
||||
);
|
||||
assertTrue(
|
||||
afterContent.includes('Use Vitest'),
|
||||
'new decision present in regenerated table',
|
||||
);
|
||||
} finally {
|
||||
closeDatabase();
|
||||
cleanupDir(tmpDir);
|
||||
}
|
||||
}
|
||||
|
||||
console.log('\n── saveDecisionToDb with no existing DECISIONS.md creates table ──');
|
||||
|
||||
{
|
||||
const tmpDir = makeTmpDir();
|
||||
const dbPath = path.join(tmpDir, '.gsd', 'gsd.db');
|
||||
const mdPath = path.join(tmpDir, '.gsd', 'DECISIONS.md');
|
||||
openDatabase(dbPath);
|
||||
|
||||
// No DECISIONS.md exists at all
|
||||
assertTrue(!fs.existsSync(mdPath), 'DECISIONS.md does not exist initially');
|
||||
|
||||
try {
|
||||
const result = await saveDecisionToDb({
|
||||
scope: 'arch',
|
||||
decision: 'Brand new decision',
|
||||
choice: 'Option A',
|
||||
rationale: 'Best fit',
|
||||
}, tmpDir);
|
||||
|
||||
assertEq(result.id, 'D001', 'first decision gets D001');
|
||||
assertTrue(fs.existsSync(mdPath), 'DECISIONS.md created');
|
||||
|
||||
const content = fs.readFileSync(mdPath, 'utf-8');
|
||||
assertTrue(content.includes('# Decisions Register'), 'new file has header');
|
||||
assertTrue(content.includes('Brand new decision'), 'new file has decision');
|
||||
} finally {
|
||||
closeDatabase();
|
||||
cleanupDir(tmpDir);
|
||||
}
|
||||
}
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
|
||||
report();
|
||||
Loading…
Add table
Reference in a new issue