fix(#4010): confine stateReplaceField to same-line whitespace so an empty field's following line survives (#4021)
* fix(#4010): confine stateReplaceField to same-line whitespace so an empty field's following line survives stateReplaceField's bold and plain patterns used `\s*` for the label-to-value gap, which matches the newline after an empty field; `(.*)` then captured the following line and the rebuild discarded it -- silent STATE.md data loss on any `state update` against an empty body field (Status:, Stopped at:, Paused at:), with exit 0 and no warning. Confine the gap to same-line whitespace (`[ \t]*`), mirroring the already-correct read side (stateExtractField, src/state-document.cts:404/:409), and pin the label-value separator to a single space when the label line had none, so an empty field yields `**Status:** value` rather than a glued `**Status:**value`. Non-empty and pipe-table replacements are byte-identical to prior behaviour. ADR-3180 §7.7 makes stateExtractField the same-line-confined owner; this aligns the writer to it. Regression test fails before / passes after and covers bold and plain shapes, LF and CRLF, the non-empty byte-identity guard, and an end-to-end transitionCore characterization at the consumer (ADR-3180 Decision 4(c)). * chore(#4010): add changeset for the stateReplaceField empty-field fix * test(#4010): add boundary and property coverage; scope the changeset's unchanged claim Addresses review on #4021: - Add boundary tests for the shapes the example tests missed: an empty field at end-of-document (no following line, bold + plain), two consecutive empty fields (only the target is filled, the other empty field's line survives), and an empty new value on an empty field (joinFieldReplacement synthesizes no dangling separator and the following line is preserved). - Add a fast-check property over the bold/plain branches and joinFieldReplacement: for any field name, any values (empty fields included), and any new value, replacing one field changes only its own line and never the total line count — the invariant #4010 violated, now guarded directly. - Scope the changeset's "unchanged" claim to ordinary space/tab separators (an exotic vertical-tab/form-feed separator, which no GSD template emits, now normalises to a single space). * test(#4010): pin glued-separator non-empty field, scope joinFieldReplacement JSDoc Round-3 review carried forward a Minor finding: joinFieldReplacement's JSDoc still claimed non-empty replacements are unconditionally "byte-identical to prior behaviour", but a non-empty field written with no label-to-value separator (**Status:**value) gains a single inserted space under the narrowed [ \t]* gap. Round 2 scoped only the changeset prose; the source JSDoc was left making the false unconditional claim. - Scope the JSDoc's byte-identity claim to ordinary space/tab separators and name the no-separator normalization as the one intentional exception. - Add a test pinning the glued-separator case (**Status:**Planning): exactly one space inserted, following line survives, not byte-identical. Emitted .cjs is gitignored (class-1), so no emitted-drift-ack applies. build:lib clean; 74/74 state-document tests pass. Claude-Session: https://claude.ai/code/session_01Mzmut6aeqZ1APfUBAkBZTR --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Rezolv
5
.changeset/state-empty-field-preserve-following-line.md
Normal file
5
.changeset/state-empty-field-preserve-following-line.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4021
|
||||
---
|
||||
**Updating an empty STATE.md field no longer silently deletes the line beneath it.** When a body field such as `**Status:**` had no value after the colon, `gsd-tools state update` consumed the following line break and overwrote the entire next line — e.g. `**Current Plan:** 2 of 5` vanished with exit 0 and no warning. `stateReplaceField`'s bold and plain patterns now confine the label-to-value gap to same-line whitespace (`[ \t]*` instead of `\s*`), matching the read side, and write a single separating space when the label line had none. The following line is preserved byte-for-byte; pipe-table replacements and non-empty replacements whose label-to-value separator is ordinary space/tab whitespace are unchanged. (#4010)
|
||||
@@ -524,17 +524,43 @@ export function stateCurrentPositionSlice(body: string): string | null {
|
||||
return section ? section.body : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Join a matched `**Field:**`/`Field:` label prefix to its new value, inserting a
|
||||
* single space when the prefix does not already end in same-line whitespace.
|
||||
*
|
||||
* On an empty field the same-line `[ \t]*` gap (below) captures nothing, so the
|
||||
* bare `prefix + value` would glue the value to the label (`**Status:**value`);
|
||||
* this inserts the missing separator. A non-empty field whose label-to-value
|
||||
* separator is ordinary space/tab keeps that separator in the prefix, so `[ \t]$`
|
||||
* is true and the output stays byte-identical to prior behaviour. The one
|
||||
* exception is a non-empty field written with NO separator at all (a hand-edited
|
||||
* `**Status:**value`): the narrowed gap captures nothing, `[ \t]$` is false, and a
|
||||
* single space is inserted — an intentional normalization, not byte-identical, and
|
||||
* with no GSD-template trigger. An empty new value inserts no separator, avoiding a
|
||||
* dangling trailing space. See #4010.
|
||||
*/
|
||||
function joinFieldReplacement(prefix: string, newValue: string): string {
|
||||
const value = `${newValue}`;
|
||||
const needsSeparator = value.length > 0 && !/[ \t]$/.test(prefix);
|
||||
return `${prefix}${needsSeparator ? ' ' : ''}${value}`;
|
||||
}
|
||||
|
||||
export function stateReplaceField(content: string, fieldName: string, newValue: string): string | null {
|
||||
const escaped = escapeRegex(fieldName);
|
||||
// Bold inline format: **FieldName:** value
|
||||
const boldPattern = new RegExp(`(\\*\\*${escaped}:\\*\\*\\s*)(.*)`, 'i');
|
||||
// The label-to-value gap is same-line whitespace only (`[ \t]*`, mirroring the
|
||||
// read side at stateExtractField). `\s*` here matched `\n`, so on an empty field
|
||||
// `(.*)` captured the following line and the rebuild discarded it — the #4010
|
||||
// data-loss. ADR-3180 §7.7 makes stateExtractField the same-line-confined owner;
|
||||
// this aligns the writer to it.
|
||||
const boldPattern = new RegExp(`(\\*\\*${escaped}:\\*\\*[ \\t]*)(.*)`, 'i');
|
||||
if (boldPattern.test(content)) {
|
||||
return content.replace(boldPattern, (_match, prefix: string) => `${prefix}${newValue}`);
|
||||
return content.replace(boldPattern, (_match, prefix: string) => joinFieldReplacement(prefix, newValue));
|
||||
}
|
||||
// Plain line-start format: FieldName: value
|
||||
const plainPattern = new RegExp(`(^${escaped}:\\s*)(.*)`, 'im');
|
||||
// Plain line-start format: FieldName: value (same same-line confinement as above)
|
||||
const plainPattern = new RegExp(`(^${escaped}:[ \\t]*)(.*)`, 'im');
|
||||
if (plainPattern.test(content)) {
|
||||
return content.replace(plainPattern, (_match, prefix: string) => `${prefix}${newValue}`);
|
||||
return content.replace(plainPattern, (_match, prefix: string) => joinFieldReplacement(prefix, newValue));
|
||||
}
|
||||
// Pipe-table format: | FieldName | value |
|
||||
// Preserve the surrounding pipe/whitespace structure; only swap the value cell.
|
||||
|
||||
@@ -132,6 +132,95 @@ describe('stateReplaceField — CRLF (#2880)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// #4010: when a STATE.md body field is empty, the label-to-value gap `\s*` in
|
||||
// stateReplaceField's bold/plain patterns crossed the newline and `(.*)` ate the
|
||||
// following line, which the rebuild then discarded — silent data-loss. The fix
|
||||
// confines the gap to same-line whitespace (`[ \t]*`, mirroring the already-
|
||||
// correct read side at stateExtractField) and pins the separator to a single
|
||||
// space when the label line had none (no glued `**Status:**value`). These assert
|
||||
// the EXACT output so the glued shape cannot pass (ADR-3180 §7.7 same-line owner).
|
||||
describe('stateReplaceField — empty field preserves the following line (#4010)', () => {
|
||||
test('bold empty field: value lands on the label line, next line survives, no glue', () => {
|
||||
const input = '**Status:**\n**Current Plan:** 2 of 5';
|
||||
const expected = '**Status:** Executing Phase 5\n**Current Plan:** 2 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 5'), expected);
|
||||
});
|
||||
|
||||
test('plain empty field: value lands on the label line, next line survives, no glue', () => {
|
||||
const input = 'Status:\nCurrent Plan: 2 of 5';
|
||||
const expected = 'Status: Executing Phase 5\nCurrent Plan: 2 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 5'), expected);
|
||||
});
|
||||
|
||||
test('bold empty field on a CRLF document preserves the following line', () => {
|
||||
const input = '**Status:**\r\n**Current Plan:** 2 of 5';
|
||||
const expected = '**Status:** Executing Phase 5\r\n**Current Plan:** 2 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 5'), expected);
|
||||
});
|
||||
|
||||
test('plain empty field on a CRLF document preserves the following line', () => {
|
||||
const input = 'Status:\r\nCurrent Plan: 2 of 5';
|
||||
const expected = 'Status: Executing Phase 5\r\nCurrent Plan: 2 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 5'), expected);
|
||||
});
|
||||
|
||||
test('non-empty bold field stays byte-identical to prior behaviour', () => {
|
||||
const input = '**Status:** Planning\n**Current Plan:** 2 of 5';
|
||||
const expected = '**Status:** Executing Phase 5\n**Current Plan:** 2 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 5'), expected);
|
||||
});
|
||||
|
||||
// Boundary: a NON-EMPTY field written with no label-to-value separator at all
|
||||
// (a hand-edited `**Status:**Planning`). The narrowed `[ \t]*` gap captures
|
||||
// nothing, so joinFieldReplacement synthesizes a single space — an intentional
|
||||
// normalization, NOT byte-identical to the pre-fix glued output. Pins that the
|
||||
// following line still survives and exactly one space is inserted, matching the
|
||||
// scoped byte-identity claim in joinFieldReplacement's JSDoc. (#4010)
|
||||
test('non-empty bold field with no separator gains one inserted space, next line survives', () => {
|
||||
const input = '**Status:**Planning\n**Current Plan:** 2 of 5';
|
||||
const expected = '**Status:** Executing Phase 5\n**Current Plan:** 2 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 5'), expected);
|
||||
});
|
||||
|
||||
test('E2E: transitionCore update of an empty field preserves the next line (ADR-3180 Decision 4(c))', () => {
|
||||
const { transitionCore } = require('../gsd-core/bin/lib/state-transition.cjs');
|
||||
const content = '## Current Position\n\n**Status:**\n**Current Plan:** 2 of 5';
|
||||
const result = transitionCore(content, { kind: 'update', field: 'Status', value: 'Executing Phase 5' });
|
||||
assert.equal(result.content, '## Current Position\n\n**Status:** Executing Phase 5\n**Current Plan:** 2 of 5');
|
||||
assert.deepEqual(result.updated, ['Status']);
|
||||
});
|
||||
|
||||
// Boundary: an empty field that is the LAST line (no following line at all).
|
||||
// `.*` matches the empty tail with no newline to cross, so the value simply
|
||||
// lands on the label line — nothing beyond it to preserve or corrupt.
|
||||
test('bold empty field at end-of-document (no following line) gets the value on its line', () => {
|
||||
assert.equal(stateReplaceField('**Status:**', 'Status', 'Executing Phase 5'), '**Status:** Executing Phase 5');
|
||||
});
|
||||
|
||||
test('plain empty field at end-of-document (no following line) gets the value on its line', () => {
|
||||
assert.equal(stateReplaceField('Status:', 'Status', 'Executing Phase 5'), 'Status: Executing Phase 5');
|
||||
});
|
||||
|
||||
// Boundary: two consecutive empty fields. Only the targeted label line takes
|
||||
// the value; the adjacent empty field's line must survive untouched (the read
|
||||
// side matches the FIRST label, so the second empty field cannot be swallowed).
|
||||
test('two consecutive empty fields: only the target is filled, the other empty field survives', () => {
|
||||
const input = '**Status:**\n**Current Plan:**\n**Progress:** 3 of 5';
|
||||
const expected = '**Status:** Executing Phase 5\n**Current Plan:**\n**Progress:** 3 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', 'Executing Phase 5'), expected);
|
||||
});
|
||||
|
||||
// Boundary: an EMPTY new value on an empty field. joinFieldReplacement's
|
||||
// needsSeparator branch must NOT synthesize a trailing space (value.length is
|
||||
// 0), so the label line is left as the bare `**Status:**` and the following
|
||||
// line is still preserved — no dangling separator, no line eaten.
|
||||
test('empty new value on an empty field leaves a bare label and preserves the following line', () => {
|
||||
const input = '**Status:**\n**Current Plan:** 2 of 5';
|
||||
const expected = '**Status:**\n**Current Plan:** 2 of 5';
|
||||
assert.equal(stateReplaceField(input, 'Status', ''), expected);
|
||||
});
|
||||
});
|
||||
|
||||
describe('stateExtractField (#2880)', () => {
|
||||
test('extracts from a two-cell row', () => {
|
||||
const input = '| Current Phase | 3 |';
|
||||
@@ -226,6 +315,66 @@ describe('property: bounded mutation (#2880, ADR-2143 §4)', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// The #4010 regression was, at heart, a line-boundary violation: replacing a
|
||||
// field crossed a newline and destroyed an adjacent line. This property pins the
|
||||
// invariant that guards it directly on the bold/plain branches (and the new
|
||||
// joinFieldReplacement helper) this fix touches: for any field name, any values
|
||||
// (including empty fields), and any new value, replacing one field changes ONLY
|
||||
// that field's line and never alters the total line count. (#4010)
|
||||
describe('property: bold/plain field replacement stays within its own line (#4010)', () => {
|
||||
test('replacing a bold or plain field changes only the target line and never the line count', () => {
|
||||
const safeValue = fc
|
||||
.array(fc.constantFrom(...'abcdefghijklmnopqrstuvwxyz0123456789 '.split('')), {
|
||||
minLength: 0,
|
||||
maxLength: 8,
|
||||
})
|
||||
.map((chars) => chars.join('').replace(/^ +| +$/g, '')); // trim; may be ''
|
||||
|
||||
fc.assert(
|
||||
fc.property(
|
||||
fc.integer({ min: 1, max: 6 }).chain((n) =>
|
||||
fc.record({
|
||||
n: fc.constant(n),
|
||||
bold: fc.boolean(),
|
||||
values: fc.array(safeValue, { minLength: n, maxLength: n }),
|
||||
targetIndex: fc.integer({ min: 0, max: n - 1 }),
|
||||
newValue: safeValue,
|
||||
}),
|
||||
),
|
||||
({ n, bold, values, targetIndex, newValue }) => {
|
||||
// Distinct single-digit names (Field0..Field5) so no field label is a
|
||||
// prefix of another, and values (alphanumeric+space) can never contain
|
||||
// a `:`/`*`, so the only line matching the target pattern is its own.
|
||||
const fieldNames = Array.from({ length: n }, (_, i) => `Field${i}`);
|
||||
const lines = fieldNames.map((name, i) => {
|
||||
const label = bold ? `**${name}:**` : `${name}:`;
|
||||
return values[i] === '' ? label : `${label} ${values[i]}`;
|
||||
});
|
||||
const doc = lines.join('\n');
|
||||
|
||||
const result = stateReplaceField(doc, fieldNames[targetIndex], newValue);
|
||||
assert.notEqual(result, null);
|
||||
|
||||
const resultLines = result.split('\n');
|
||||
// Core #4010 guarantee: no line eaten, none added — neighbours survive.
|
||||
assert.equal(resultLines.length, lines.length);
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
if (i === targetIndex) continue;
|
||||
assert.equal(resultLines[i], lines[i]);
|
||||
}
|
||||
// The target line still opens with its label and gained no newline.
|
||||
const label = bold ? `**${fieldNames[targetIndex]}:**` : `${fieldNames[targetIndex]}:`;
|
||||
assert.ok(
|
||||
resultLines[targetIndex].startsWith(label),
|
||||
`target line lost its label: ${JSON.stringify(resultLines[targetIndex])}`,
|
||||
);
|
||||
},
|
||||
),
|
||||
{ seed: 40100, numRuns: 300 },
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('property: fieldNameMatchesRawCell case-fold semantics, via stateExtractField (FIX A)', () => {
|
||||
test('match verdict agrees with an explicit ECMAScript non-unicode Canonicalize reference predicate', () => {
|
||||
// Independent, obviously-correct reference for "are these two code units
|
||||
|
||||
Reference in New Issue
Block a user